fix: actually invalidate the session on OAuth logout

src/lib/handlers.ts

@@ -532,25 +532,23 @@ export async function handleCallback(
 }
 
 /**
- * Clear OAuth session data and log out the user
- * Shared function for 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.
+ * Clear OAuth session data and log out (explicit logout and token-expiry logout).
+ * Harper 5's request.session is a shallow copy with only `.update` (a full-replace put) and
+ * no `.delete`; in-memory mutation never persists — so invalidate by persisting `{ user: null }`,
+ * mirroring Harper's own logout().
  */
 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);
-	} else {
-		// Fallback for sessions without delete method - clear in-memory
-		session.user = null;
-		delete session.oauth;
-		delete session.oauthUser;
+	// Persist only for an existing session: `.update` on an anonymous request would mint a
+	// fresh, non-expiring hdb_session row.
+	if (session.id && typeof session.update === 'function') {
+		await session.update({ user: null });
 	}
+	// Clear in memory too so the current request sees no identity.
+	session.user = null;
+	delete session.oauth;
+	delete session.oauthUser;
 
 	logger?.info?.('OAuth session cleared');
 }

src/lib/withOAuthValidation.ts

@@ -38,13 +38,8 @@ export interface OAuthValidationOptions {
 	 *   callback is invoked BEFORE session cleanup, so
 	 *   `request.session.oauth` and `.oauthUser` are readable.
 	 * - `!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.
+	 *   `validateAndRefreshSession` has already called `clearOAuthSession` before the
+	 *   callback runs; the callback sees `session.oauth` and `.oauthUser` as `undefined`.
 	 */
 	onValidationError?: (request: Request, error: string) => any;
 }
@@ -247,14 +242,10 @@ async function validateOAuthForRequest(context: MaybeContext, options: OAuthVali
  *   and `oauthUser` fields via a local helper. The session record
  *   itself survives — "provider not configured" may be a recoverable
  *   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.
+ * - Expired-token path (`validateAndRefreshSession` returns `{valid: false}`):
+ *   `clearOAuthSession` persists `{ user: null }` and clears in-memory fields —
+ *   terminal logout. `requireAuth: false` resources pass through but observe an
+ *   invalidated session.
  */
 export function withOAuthValidation<T extends abstract new (...args: any[]) => any>(
 	ResourceClass: T,

test/lib/handlers.test.js

@@ -1214,22 +1214,58 @@ 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): Harper session exposes only .update, never .delete — logout must persist { user: null }.
+			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'
+			);
+		});
+
+		it('clears in-memory session fields on the production path so the current request sees no identity', async () => {
+			// Production path must also clear in-memory so the current request sees no identity.
+			mockRequest.session = {
+				id: 'session-123',
+				update: createMockFn(),
+				user: { username: 'alice', role: 'superuser' },
+				oauth: { accessToken: 'tok-abc' },
+				oauthUser: { username: 'alice', provider: 'github' },
+			};
+
+			const result = await handleLogout(mockRequest, mockHookManager, mockLogger);
 
-			// 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');
+			assert.equal(result.status, 200);
+			// update() must still have been called once with { user: null }
+			assert.equal(mockRequest.session.update.mock.calls.length, 1);
+			assert.deepEqual(mockRequest.session.update.mock.calls[0].arguments[0], { user: null });
+			// In-memory fields must be cleared so the current request sees no identity
+			assert.equal(mockRequest.session.user, null, 'session.user cleared in memory');
+			assert.equal(mockRequest.session.oauth, undefined, 'session.oauth deleted in memory');
+			assert.equal(mockRequest.session.oauthUser, undefined, 'session.oauthUser deleted in memory');
+		});
+
+		it('does NOT persist a row for an anonymous logout (session with .update but no id)', async () => {
+			// .update on an anonymous request mints a non-expiring hdb_session row — must not call it.
+			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 +1274,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,8 @@ 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
+	// clearOAuthSession persisted { user: null }, dropping oauth.
+	assert.strictEqual(session.user, null);
 	assert.strictEqual(session.oauth, undefined);
 });
 
@@ -279,7 +281,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 +515,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,7 @@ 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.
+		// Typical authenticated session — id + update so clearOAuthSession takes the persist branch.
 		return {
 			id: 'sess-1',
 			oauth: {
@@ -77,18 +72,14 @@ 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.
+	// Spy on .update so tests can assert clearOAuthSession persisted { user: null }.
 	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;
 	}
 
@@ -956,15 +947,8 @@ describe('withOAuthValidation', () => {
 			assert.equal(calls.length, 0, 'protected method must NOT run — silent-bypass guard');
 		});
 
-		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.
+		it('production-path session: callback sees cleared in-memory session after clearOAuthSession', async () => {
+			// clearOAuthSession clears in-memory fields and persists — callback sees undefined oauth.
 			const session = makeProductionLikeSession({
 				oauth: {
 					provider: 'github',
@@ -1003,10 +987,12 @@ describe('withOAuthValidation', () => {
 
 			assert.equal(calls.length, 0, 'protected method must not run');
 			assert.equal(seen.length, 1);
-			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');
+			// In-memory fields are cleared before the callback runs
+			assert.equal(seen[0].oauthProvider, undefined, 'session.oauth is cleared in memory before callback');
+			assert.equal(seen[0].oauthAccessToken, undefined, 'session.oauth is cleared in memory before callback');
+			assert.equal(seen[0].oauthUserEmail, undefined, 'session.oauthUser is cleared in memory before callback');
+			assert.equal(session.__updateCalls.length, 1, 'clearOAuthSession persisted an invalidation via update');
+			assert.equal(session.__updateCalls[0].user, null, 'persisted user: null');
 		});
 	});
 
@@ -1087,18 +1073,11 @@ 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 takes the persist path when session.update exists, otherwise clears in-memory only.
 		// 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 +1089,17 @@ 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,
-					},
-				}),
-			};
+			// No .update method — 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 +1112,15 @@ 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.
+			// Fallback: oauth fields deleted in memory.
 			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 and in-memory cleared', 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.
 					calls.push({
 						target,
 						oauthAfterValidate: this._context.session.oauth,
@@ -1175,17 +1149,18 @@ 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.
-			assert.ok(
-				calls[0].oauthAfterValidate !== undefined,
-				'production path does not mutate the in-memory session object'
+			// In-memory session is now cleared too — the resource sees oauth as undefined.
+			assert.equal(
+				calls[0].oauthAfterValidate,
+				undefined,
+				'in-memory oauth cleared so the current request sees no identity'
 			);
 		});
 	});