diff --git a/src/lib/handlers.ts b/src/lib/handlers.ts index ab6808d..0dbec06 100644 --- a/src/lib/handlers.ts +++ b/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 { 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'); } diff --git a/src/lib/withOAuthValidation.ts b/src/lib/withOAuthValidation.ts index 20a114a..4089c9e 100644 --- a/src/lib/withOAuthValidation.ts +++ b/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 any>( ResourceClass: T, diff --git a/test/lib/handlers.test.js b/test/lib/handlers.test.js index d7b14bd..45bbc42 100644 --- a/test/lib/handlers.test.js +++ b/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); diff --git a/test/lib/sessionValidator.test.js b/test/lib/sessionValidator.test.js index e532815..f11ec58 100644 --- a/test/lib/sessionValidator.test.js +++ b/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 () => { diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index d09f5c9..b690afb 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/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' ); }); });