fix: actually invalidate the session on OAuth logout

src/lib/handlers.ts

@@ -532,21 +532,44 @@ export async function handleCallback(
 }
 
 /**
- * Clear OAuth session data and log out the user
- * Shared function for explicit logout and automatic logout on token expiration
+ * Clear OAuth session data and log out the user.
+ * Shared by explicit logout and automatic logout on token expiration.
  *
- * Deletes the session record from the hdb_session table, completely removing it
- * rather than just clearing the user field. This ensures no orphaned sessions remain.
+ * `request.session` is a shallow copy of the `hdb_session` record exposing only
+ * `.update` (a full-replace `put` keyed on the session id) — it has NO `.delete`,
+ * and mutating the copy in memory never persists. So we INVALIDATE by persisting
+ * a null-user record via `.update`, mirroring Harper's own `logout()`: on the
+ * next request `session.user` is null, so the bearer resolves to no user. The
+ * previous code's `session.delete` branch never ran (no such method) and its
+ * in-memory fallback left the stored `hdb_session` record fully valid — a
+ * captured cookie (or an upstream-revoked account) kept authenticating.
  */
 export async function clearOAuthSession(session: any, logger?: Logger): Promise<void> {
 	if (!session) return;
 
-	// Delete the session record from the hdb_session table
-	// This completely removes the session on logout, rather than just nulling the user field
-	if (typeof session.delete === 'function') {
-		await session.delete(session.id);
+	// Only persist an invalidation when there is an EXISTING session to invalidate.
+	// Harper defines `.update` on every request — including anonymous, cookie-less
+	// ones — and calling it mints a fresh hdb_session row (new UUID + Set-Cookie),
+	// with no expiry when `authentication.cookieExpires` is unset. Guarding on
+	// `session.id` stops an unauthenticated POST /oauth/logout from spamming
+	// non-expiring rows.
+	//
+	// `session.id` is the right signal here because every caller of this function
+	// (logout, validateAndRefreshSession, the provider-gone middleware) runs on a
+	// session LOADED FROM A COOKIE, which carries its id. Known limitation, not
+	// reachable via any OAuth flow today: a session created id-less earlier in the
+	// SAME request via a separate `update({...})` payload wouldn't expose an id
+	// here (Harper mints it onto the payload, not back onto request.session), so
+	// this would no-op. OAuth never creates-then-clears in one request.
+	if (session.id && typeof session.update === 'function') {
+		// Match Harper's own logout(): a full-replace put of `{ user: null }` clears
+		// the identity (unauthenticated on the next request) AND drops the oauth
+		// tokens — leaving them absent, not `null`, which keeps the `Session` type
+		// and the downstream `session.oauth === undefined` checks honest.
+		await session.update({ user: null });
 	} else {
-		// Fallback for sessions without delete method - clear in-memory
+		// No existing session / no persistence (anonymous logout, non-session
+		// transport, tests): clear in memory only.
 		session.user = null;
 		delete session.oauth;
 		delete session.oauthUser;

src/lib/withOAuthValidation.ts

@@ -40,11 +40,12 @@ export interface OAuthValidationOptions {
 	 * - `!validation.valid` (expired token with no refresh token) —
 	 *   `validateAndRefreshSession` has ALREADY called
 	 *   `clearOAuthSession` internally before the callback runs. On a
-	 *   production Harper session this calls `session.delete(session.id)`
-	 *   (DB record destroyed; in-memory fields untouched). On a session
-	 *   without a `delete()` method it falls back to in-memory deletion
-	 *   of `.oauth` / `.oauthUser`. The callback is still invoked, but
-	 *   the session state it observes depends on which path ran.
+	 *   production Harper session this persists `session.update({ user: null })`
+	 *   (the hdb_session record survives but is invalidated — user null, oauth
+	 *   dropped; the in-memory `request.session` copy is untouched, so the
+	 *   callback still sees `.oauth` / `.oauthUser`). On a session with no
+	 *   `.update()` (or no id) it falls back to an in-memory clear. Either way
+	 *   the callback is still invoked.
 	 */
 	onValidationError?: (request: Request, error: string) => any;
 }
@@ -249,12 +250,12 @@ async function validateOAuthForRequest(context: MaybeContext, options: OAuthVali
  *   config issue.
  * - Expired-token path (`validateAndRefreshSession` returns
  *   `{valid: false}`): `validateAndRefreshSession` internally calls
- *   `clearOAuthSession`, which on a Harper production session
- *   invokes `session.delete(session.id)` — the DB record is destroyed.
- *   This is terminal: the user is logged out, not just detached from
- *   OAuth. `requireAuth: false` resources still receive the
- *   passthrough call, but they observe a session that is about to
- *   stop existing on the next request.
+ *   `clearOAuthSession`, which on a Harper production session persists
+ *   `session.update({ user: null })` — the hdb_session record survives but
+ *   is invalidated (user null, oauth dropped), so the next request resolves
+ *   to no user. This is terminal: the user is logged out, not just detached
+ *   from OAuth. `requireAuth: false` resources still receive the passthrough
+ *   call, but they observe a session that is invalidated for the next request.
  */
 export function withOAuthValidation<T extends abstract new (...args: any[]) => any>(
 	ResourceClass: T,

test/lib/handlers.test.js

@@ -1214,22 +1214,42 @@ describe('OAuth Handlers', () => {
 	});
 
 	describe('handleLogout', () => {
-		it('should clear session data', async () => {
-			// Add delete method mock to session
-			mockRequest.session.delete = createMockFn();
+		it('persists an invalidated session record (user: null) — not just an in-memory clear', async () => {
+			// Regression (F4): the real Harper session exposes only `.update` (a
+			// put to hdb_session), never `.delete`. Logout must persist a null-user
+			// record or the stored session keeps authenticating. `{ user: null }`
+			// mirrors Harper's own logout(); a full-replace put drops oauth/oauthUser
+			// (absent, not null — the `Session` type has no nullable oauth).
+			mockRequest.session.update = createMockFn(); // session already has id: 'session-123'
 
 			const result = await handleLogout(mockRequest, mockHookManager, mockLogger);
 
 			assert.equal(result.status, 200);
 			assert.equal(result.body.message, 'Logged out successfully');
+			assert.equal(mockRequest.session.update.mock.calls.length, 1);
+			const persisted = mockRequest.session.update.mock.calls[0].arguments[0];
+			assert.deepEqual(
+				persisted,
+				{ user: null },
+				'persists only { user: null } — oauth keys dropped by the full-replace put'
+			);
+		});
 
-			// Should call session.delete with session ID
-			assert.equal(mockRequest.session.delete.mock.calls.length, 1);
-			assert.equal(mockRequest.session.delete.mock.calls[0].arguments[0], 'session-123');
+		it('does NOT persist a row for an anonymous logout (session with .update but no id)', async () => {
+			// Harper defines `.update` on every request, including cookie-less ones,
+			// and calling it mints a fresh non-expiring hdb_session row. An
+			// unauthenticated POST /oauth/logout must not create session rows.
+			mockRequest.session = { update: createMockFn() }; // no id → nothing to invalidate
+
+			const result = await handleLogout(mockRequest, mockHookManager, mockLogger);
+
+			assert.equal(result.status, 200);
+			assert.equal(mockRequest.session.update.mock.calls.length, 0, 'no persistence for an anonymous logout');
 		});
 
-		it('should handle session without delete function', async () => {
+		it('falls back to an in-memory clear when the session cannot persist (no update)', async () => {
 			mockRequest.session = {
+				id: 'session-123',
 				user: 'test-user',
 				oauthUser: { username: 'test' },
 				oauth: { accessToken: 'token' },
@@ -1238,7 +1258,6 @@ describe('OAuth Handlers', () => {
 			const result = await handleLogout(mockRequest, mockHookManager, mockLogger);
 
 			assert.equal(result.status, 200);
-			// Falls back to clearing fields when delete method isn't available
 			assert.equal(mockRequest.session.user, null);
 			assert.equal(mockRequest.session.oauth, undefined);
 			assert.equal(mockRequest.session.oauthUser, undefined);

test/lib/sessionValidator.test.js

@@ -36,6 +36,7 @@ function createMockProvider(overrides = {}) {
  */
 function createMockSession(overrides = {}) {
 	const session = {
+		id: 'sess-validator', // an authenticated session has an id → clearOAuthSession persists via update
 		user: 'test@example.com',
 		oauthUser: {
 			username: 'test@example.com',
@@ -256,7 +257,9 @@ test('should logout when token expired and no refresh token', async () => {
 
 	assert.strictEqual(result.valid, false);
 	assert.strictEqual(result.error, 'Token expired and no refresh token available');
-	// Session should be cleared
+	// Session invalidated — clearOAuthSession persists { user: null }; the
+	// full-replace put drops oauth, so the reloaded record has no oauth.
+	assert.strictEqual(session.user, null);
 	assert.strictEqual(session.oauth, undefined);
 });
 
@@ -279,7 +282,7 @@ test('should handle refresh failure for expired token', async () => {
 
 	assert.strictEqual(result.valid, false);
 	assert.ok(result.error.includes('Token refresh failed'));
-	// Session should be cleared after failed refresh of expired token
+	// Session invalidated after failed refresh — persisted { user: null }, oauth dropped
 	assert.strictEqual(session.oauth, undefined);
 });
 
@@ -513,7 +516,7 @@ test('should logout when periodic validation fails (token revoked)', async () =>
 
 	assert.strictEqual(result.valid, false);
 	assert.strictEqual(result.error, 'Token validation failed - token may have been revoked');
-	assert.strictEqual(session.oauth, undefined, 'Session should be cleared after validation failure');
+	assert.strictEqual(session.oauth, undefined, 'Session invalidated (oauth dropped) after validation failure');
 });
 
 test('should handle validation errors gracefully (network issues)', async () => {

test/lib/withOAuthValidation.test.js

@@ -57,12 +57,11 @@ describe('withOAuthValidation', () => {
 	}
 
 	function makeSession(overrides = {}) {
-		// IMPORTANT: this session has NO `delete()` method. That matters
-		// for tests that hit `clearOAuthSession` (e.g. expired-token
-		// paths): those tests exercise the in-memory fallback inside
-		// `clearOAuthSession`, not the production `session.delete(id)`
-		// path. For production-shaped behavior use
-		// `makeProductionLikeSession` below.
+		// Production-shaped: has an `id` and an `.update` (a persisting put), like a
+		// real cookie-loaded hdb_session. So `clearOAuthSession` takes the persist
+		// branch (`update({ user: null })`); the in-memory fallback only runs when a
+		// test strips `.update` (or the session has no id). `makeProductionLikeSession`
+		// below adds an `.update` spy for asserting the persisted invalidation.
 		return {
 			id: 'sess-1',
 			oauth: {
@@ -77,18 +76,16 @@ describe('withOAuthValidation', () => {
 		};
 	}
 
-	// A Harper-production-shaped session: provides `delete(id)` like the
-	// real hdb_session record so `clearOAuthSession` takes the
-	// `session.delete(id)` branch (full DB destruction) instead of the
-	// in-memory fallback.
+	// A Harper-production-shaped session: the real hdb_session record exposes
+	// only `.update` (a persisting put keyed on id), never `.delete`. Spy on it
+	// so tests can assert `clearOAuthSession` persists an invalidation.
 	function makeProductionLikeSession(overrides = {}) {
 		const base = makeSession(overrides);
-		const deleteCalls = [];
-		base.delete = async (id) => {
-			deleteCalls.push(id);
+		const updateCalls = [];
+		base.update = async (updated) => {
+			updateCalls.push(updated);
 		};
-		// Expose the spy ledger for assertions
-		base.__deleteCalls = deleteCalls;
+		base.__updateCalls = updateCalls;
 		return base;
 	}
 
@@ -959,12 +956,11 @@ describe('withOAuthValidation', () => {
 		it('production-path session: callback sees full oauth data (not mutated by clearOAuthSession)', async () => {
 			// `validateAndRefreshSession` calls `clearOAuthSession` as a
 			// side effect before returning `{valid: false}`. In the
-			// production path (session with a `delete()` method),
-			// `clearOAuthSession` calls `session.delete(session.id)` and
-			// does NOT mutate the in-memory session object. So the
-			// `onValidationError` callback — invoked after that —
-			// still observes the full oauth/oauthUser data. Pin this
-			// behavior so it can't regress silently.
+			// production path `clearOAuthSession` persists an invalidation
+			// via `session.update({ user: null, ... })` and does NOT mutate
+			// the in-memory session object. So the `onValidationError`
+			// callback — invoked after that — still observes the full
+			// oauth/oauthUser data. Pin this behavior so it can't regress.
 			const session = makeProductionLikeSession({
 				oauth: {
 					provider: 'github',
@@ -1006,7 +1002,8 @@ describe('withOAuthValidation', () => {
 			assert.equal(seen[0].oauthProvider, 'github', 'oauth.provider must be readable in production path');
 			assert.equal(seen[0].oauthAccessToken, 'expired', 'oauth.accessToken must be readable in production path');
 			assert.equal(seen[0].oauthUserEmail, 'alice@example.com', 'oauthUser.email must be readable');
-			assert.deepEqual(session.__deleteCalls, ['sess-1'], 'session.delete(id) was called');
+			assert.equal(session.__updateCalls.length, 1, 'clearOAuthSession persisted an invalidation via update');
+			assert.equal(session.__updateCalls[0].user, null, 'persisted user: null');
 		});
 	});
 
@@ -1087,18 +1084,18 @@ describe('withOAuthValidation', () => {
 		// through to the underlying method, which runs with a session
 		// that's about to be (or has already been) cleaned up.
 		//
-		// `clearOAuthSession` has TWO code paths depending on whether
-		// the session provides a `delete(id)` method:
-		//   - with `delete`:  production path — Harper destroys the DB
-		//                     session record. The in-memory session
-		//                     object's oauth fields are NOT touched.
-		//   - without:        in-memory fallback — deletes `oauth` and
-		//                     `oauthUser` fields directly.
+		// `clearOAuthSession` has TWO code paths depending on whether the
+		// session provides an `update()` method:
+		//   - with `update`:  production path — persists an invalidated
+		//                     record (`user: null`) to hdb_session. The
+		//                     in-memory session object is NOT mutated.
+		//   - without:        in-memory fallback — clears `oauth`/`oauthUser`
+		//                     on the object directly (no persistence).
 		//
 		// Both paths are exercised below so behavior is pinned down for
 		// integrators.
 
-		it('fallback path (no session.delete): underlying method runs, oauth fields cleared in-memory', async () => {
+		it('fallback path (session has no update): underlying method runs, oauth fields cleared in-memory', async () => {
 			const calls = [];
 			class MyResource extends MockResource {
 				async get(target) {
@@ -1110,16 +1107,18 @@ describe('withOAuthValidation', () => {
 					return { status: 200, body: { ran: true } };
 				}
 			}
-			const context = {
-				session: makeSession({
-					oauth: {
-						provider: 'github',
-						accessToken: 'expired-token',
-						expiresAt: Date.now() - 60_000,
-						refreshToken: undefined,
-					},
-				}),
-			};
+			// A session with NO update method (and no delete) — e.g. a
+			// non-session transport — exercises the in-memory fallback.
+			const session = makeSession({
+				oauth: {
+					provider: 'github',
+					accessToken: 'expired-token',
+					expiresAt: Date.now() - 60_000,
+					refreshToken: undefined,
+				},
+			});
+			delete session.update;
+			const context = { session };
 			const Wrapped = withOAuthValidation(MyResource, {
 				providers: mockProviders,
 				logger: mockLogger,
@@ -1132,21 +1131,21 @@ describe('withOAuthValidation', () => {
 			assert.equal(result.status, 200, 'underlying method must run when requireAuth is false');
 			assert.equal(result.body.ran, true);
 			assert.equal(calls.length, 1);
-			// In the fallback path, clearOAuthSession deletes the in-memory
-			// oauth fields directly, so the resource observes an empty session.
+			// In the fallback path, clearOAuthSession deletes the in-memory oauth
+			// fields, so the resource observes an empty session (undefined, not null).
 			assert.equal(calls[0].oauthAfterValidate, undefined);
 			assert.equal(calls[0].oauthUserAfterValidate, undefined);
 		});
 
-		it('production path (session.delete present): underlying method runs, delete(id) called with session id', async () => {
+		it('production path (session has update): underlying method runs, invalidation persisted via update({user:null})', async () => {
 			const calls = [];
 			class MyResource extends MockResource {
 				async get(target) {
-					// In the production path, `clearOAuthSession` invokes
-					// `session.delete(session.id)` — it does NOT mutate the
-					// in-memory session object. So by the time the resource
-					// runs, the DB record is doomed but the in-memory oauth
-					// fields may still be populated.
+					// In the production path, `clearOAuthSession` persists an
+					// invalidation via `session.update({ user: null, ... })` — it
+					// does NOT mutate the in-memory session object. So by the time
+					// the resource runs, the DB record is invalidated but the
+					// in-memory oauth fields may still be populated.
 					calls.push({
 						target,
 						oauthAfterValidate: this._context.session.oauth,
@@ -1175,11 +1174,12 @@ describe('withOAuthValidation', () => {
 			assert.equal(result.status, 200, 'underlying method must still run when requireAuth is false');
 			assert.equal(result.body.ran, true);
 			assert.equal(calls.length, 1);
-			// The production path destroys the DB record via session.delete(session.id).
-			assert.deepEqual(
-				session.__deleteCalls,
-				['sess-1'],
-				'clearOAuthSession must call session.delete(session.id) in the production path'
+			// The production path persists an invalidated record (user: null).
+			assert.equal(session.__updateCalls.length, 1, 'clearOAuthSession persisted via update');
+			assert.equal(
+				session.__updateCalls[0].user,
+				null,
+				'clearOAuthSession must persist user: null in the production path'
 			);
 			// The in-memory session object isn't mutated by the production path —
 			// documenting this so integrators know what the resource observes.