From 029ecb8275bce0ef35b072039d1ca9d87f9033c1 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Fri, 14 Aug 2026 14:42:17 -0700 Subject: [PATCH 1/6] fix: actually invalidate the session on OAuth logout (F4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit clearOAuthSession gated on `typeof session.delete === 'function'`, but the real Harper request.session is a shallow copy of the hdb_session record exposing only `.update` (a persisting put) — never `.delete`. So the delete branch never ran and the in-memory fallback (`session.user = null`) was never persisted: the stored session kept authenticating, so an explicit logout, a captured cookie, or an upstream-revoked/expired token left the session fully valid until cookie TTL. Invalidate by persisting `session.update({ user: null, oauth: null, oauthUser: null })`, mirroring Harper's own logout() — the next request resolves session.user to no user. Falls back to an in-memory clear only when no `.update` exists (non-session transports). Confirmed against harper@5.1.9 (security/auth.ts:110 session shape, :381 logout()). Updates the logout/sessionValidator/withOAuthValidation tests that had encoded the inverted delete-based model. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NxkByn5VD9Mwh7Pm9wtmBH --- src/lib/handlers.ts | 22 ++++--- test/lib/handlers.test.js | 20 +++--- test/lib/sessionValidator.test.js | 11 ++-- test/lib/withOAuthValidation.test.js | 93 ++++++++++++++-------------- 4 files changed, 77 insertions(+), 69 deletions(-) diff --git a/src/lib/handlers.ts b/src/lib/handlers.ts index ab6808d..948beec 100644 --- a/src/lib/handlers.ts +++ b/src/lib/handlers.ts @@ -532,21 +532,25 @@ 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 { 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); + if (typeof session.update === 'function') { + await session.update({ user: null, oauth: null, oauthUser: null }); } else { - // Fallback for sessions without delete method - clear in-memory + // No persistence available (e.g. non-session transports / tests): clear in memory. session.user = null; delete session.oauth; delete session.oauthUser; diff --git a/test/lib/handlers.test.js b/test/lib/handlers.test.js index d7b14bd..f362c74 100644 --- a/test/lib/handlers.test.js +++ b/test/lib/handlers.test.js @@ -1214,21 +1214,24 @@ 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. + mockRequest.session.update = createMockFn(); const result = await handleLogout(mockRequest, mockHookManager, mockLogger); assert.equal(result.status, 200); assert.equal(result.body.message, 'Logged out successfully'); - - // 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(mockRequest.session.update.mock.calls.length, 1); + const persisted = mockRequest.session.update.mock.calls[0].arguments[0]; + assert.equal(persisted.user, null, 'persists user: null so the next request is unauthenticated'); + assert.equal(persisted.oauth, null); + assert.equal(persisted.oauthUser, null); }); - 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 = { user: 'test-user', oauthUser: { username: 'test' }, @@ -1238,7 +1241,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..4bfcc30 100644 --- a/test/lib/sessionValidator.test.js +++ b/test/lib/sessionValidator.test.js @@ -256,8 +256,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 - assert.strictEqual(session.oauth, undefined); + // Session is invalidated — clearOAuthSession persists { user: null, oauth: null } + assert.strictEqual(session.oauth, null); + assert.strictEqual(session.user, null); }); test('should handle refresh failure for expired token', async () => { @@ -279,8 +280,8 @@ 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 - assert.strictEqual(session.oauth, undefined); + // Session invalidated after failed refresh — persisted { user: null, oauth: null } + assert.strictEqual(session.oauth, null); }); test('should not logout when refresh fails for non-expired token', async () => { @@ -513,7 +514,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, null, 'Session invalidated (user/oauth null) 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..5f64d86 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -77,18 +77,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 +957,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 +1003,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 +1085,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 +1108,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 +1132,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 + // In the fallback path, clearOAuthSession clears the in-memory // oauth fields directly, so the resource observes an empty session. 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 +1175,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. From 293fac7c8f66cfd7f33781c5b5fc8fe8b24c4480 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Mon, 24 Aug 2026 11:47:35 -0700 Subject: [PATCH 2/6] refactor: null the in-memory-fallback session fields (match persisted shape) Review nit: the no-`.update` fallback used `delete`; use `= null` so it matches the persisted invalidation shape ({ user: null, oauth: null, oauthUser: null }). Tests updated accordingly. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NxkByn5VD9Mwh7Pm9wtmBH --- src/lib/handlers.ts | 7 ++++--- test/lib/handlers.test.js | 4 ++-- test/lib/withOAuthValidation.test.js | 8 ++++---- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/lib/handlers.ts b/src/lib/handlers.ts index 948beec..8395645 100644 --- a/src/lib/handlers.ts +++ b/src/lib/handlers.ts @@ -550,10 +550,11 @@ export async function clearOAuthSession(session: any, logger?: Logger): Promise< if (typeof session.update === 'function') { await session.update({ user: null, oauth: null, oauthUser: null }); } else { - // No persistence available (e.g. non-session transports / tests): clear in memory. + // No persistence available (e.g. non-session transports / tests): clear in + // memory. Null (not delete) to match the persisted invalidation shape. session.user = null; - delete session.oauth; - delete session.oauthUser; + session.oauth = null; + session.oauthUser = null; } logger?.info?.('OAuth session cleared'); diff --git a/test/lib/handlers.test.js b/test/lib/handlers.test.js index f362c74..7070a16 100644 --- a/test/lib/handlers.test.js +++ b/test/lib/handlers.test.js @@ -1242,8 +1242,8 @@ describe('OAuth Handlers', () => { assert.equal(result.status, 200); assert.equal(mockRequest.session.user, null); - assert.equal(mockRequest.session.oauth, undefined); - assert.equal(mockRequest.session.oauthUser, undefined); + assert.equal(mockRequest.session.oauth, null); + assert.equal(mockRequest.session.oauthUser, null); }); it('should handle missing session', async () => { diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index 5f64d86..65ee9ee 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -1132,10 +1132,10 @@ 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 clears the in-memory - // oauth fields directly, so the resource observes an empty session. - assert.equal(calls[0].oauthAfterValidate, undefined); - assert.equal(calls[0].oauthUserAfterValidate, undefined); + // In the fallback path, clearOAuthSession nulls the in-memory oauth + // fields (matching the persisted shape), so the resource observes them cleared. + assert.equal(calls[0].oauthAfterValidate, null); + assert.equal(calls[0].oauthUserAfterValidate, null); }); it('production path (session has update): underlying method runs, invalidation persisted via update({user:null})', async () => { From 7fad2e90c39412affe58d6ba0276b8d572370eeb Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Mon, 24 Aug 2026 12:10:55 -0700 Subject: [PATCH 3/6] fix: guard logout persistence on session.id; persist { user: null } (F4 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-model review (Codex) of the F4 logout fix caught two problems: - BLOCKER (regression this fix introduced): Harper defines `.update` on EVERY request, including anonymous cookie-less ones, and calling it mints a fresh hdb_session row (new UUID) with no expiry when `authentication.cookieExpires` is unset. So an unauthenticated POST /oauth/logout would spam non-expiring rows. Guard persistence on `session.id` — only invalidate an existing session. - The earlier `{ oauth: null }` shape broke the `Session` type (oauth is not nullable) and the downstream `session.oauth === undefined` check. Persist `session.update({ user: null })` (matching Harper's own logout); the full-replace put drops oauth/oauthUser (absent, not null). In-memory fallback goes back to `delete`. Also refreshes the stale `session.delete` JSDoc in withOAuthValidation.ts and adds a test that anonymous logout does NOT persist a row. Follow-ups (tracked separately, not bolted in): logout-vs-refresh / refresh-vs-refresh CAS races (session resurrection), and the global middleware calling next(request) without clearing request.user on an invalidated session. Integration-level login→logout→401 proof pending (reusing the human-login harness from the F2 repro). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NxkByn5VD9Mwh7Pm9wtmBH --- src/lib/handlers.ts | 22 ++++++++++++++------ src/lib/withOAuthValidation.ts | 23 +++++++++++---------- test/lib/handlers.test.js | 31 +++++++++++++++++++++------- test/lib/sessionValidator.test.js | 12 ++++++----- test/lib/withOAuthValidation.test.js | 8 +++---- 5 files changed, 63 insertions(+), 33 deletions(-) diff --git a/src/lib/handlers.ts b/src/lib/handlers.ts index 8395645..33b0011 100644 --- a/src/lib/handlers.ts +++ b/src/lib/handlers.ts @@ -547,14 +547,24 @@ export async function handleCallback( export async function clearOAuthSession(session: any, logger?: Logger): Promise { if (!session) return; - if (typeof session.update === 'function') { - await session.update({ user: null, oauth: null, oauthUser: null }); + // 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. + 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 { - // No persistence available (e.g. non-session transports / tests): clear in - // memory. Null (not delete) to match the persisted invalidation shape. + // No existing session / no persistence (anonymous logout, non-session + // transport, tests): clear in memory only. session.user = null; - session.oauth = null; - session.oauthUser = 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..20660b7 100644 --- a/src/lib/withOAuthValidation.ts +++ b/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 any>( ResourceClass: T, diff --git a/test/lib/handlers.test.js b/test/lib/handlers.test.js index 7070a16..3f459be 100644 --- a/test/lib/handlers.test.js +++ b/test/lib/handlers.test.js @@ -1217,8 +1217,10 @@ describe('OAuth Handlers', () => { 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. - mockRequest.session.update = createMockFn(); + // 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); @@ -1226,13 +1228,28 @@ describe('OAuth Handlers', () => { 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.equal(persisted.user, null, 'persists user: null so the next request is unauthenticated'); - assert.equal(persisted.oauth, null); - assert.equal(persisted.oauthUser, null); + assert.deepEqual( + persisted, + { user: null }, + 'persists only { user: null } — oauth keys dropped by the full-replace put' + ); + }); + + 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('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' }, @@ -1242,8 +1259,8 @@ describe('OAuth Handlers', () => { assert.equal(result.status, 200); assert.equal(mockRequest.session.user, null); - assert.equal(mockRequest.session.oauth, null); - assert.equal(mockRequest.session.oauthUser, null); + assert.equal(mockRequest.session.oauth, undefined); + assert.equal(mockRequest.session.oauthUser, undefined); }); it('should handle missing session', async () => { diff --git a/test/lib/sessionValidator.test.js b/test/lib/sessionValidator.test.js index 4bfcc30..edb37c1 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,9 +257,10 @@ 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 is invalidated — clearOAuthSession persists { user: null, oauth: null } - assert.strictEqual(session.oauth, null); + // 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); }); test('should handle refresh failure for expired token', async () => { @@ -280,8 +282,8 @@ test('should handle refresh failure for expired token', async () => { assert.strictEqual(result.valid, false); assert.ok(result.error.includes('Token refresh failed')); - // Session invalidated after failed refresh — persisted { user: null, oauth: null } - assert.strictEqual(session.oauth, null); + // Session invalidated after failed refresh — persisted { user: null }, oauth dropped + assert.strictEqual(session.oauth, undefined); }); test('should not logout when refresh fails for non-expired token', async () => { @@ -514,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, null, 'Session invalidated (user/oauth null) 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 65ee9ee..17c961d 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -1132,10 +1132,10 @@ 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 nulls the in-memory oauth - // fields (matching the persisted shape), so the resource observes them cleared. - assert.equal(calls[0].oauthAfterValidate, null); - assert.equal(calls[0].oauthUserAfterValidate, null); + // 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 has update): underlying method runs, invalidation persisted via update({user:null})', async () => { From c1e07fb894325c2420ab75382e352b70052806e7 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Mon, 24 Aug 2026 12:55:31 -0700 Subject: [PATCH 4/6] docs: note the session.id guard limitation; refresh stale test comment (#211 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex re-review confirmed the anonymous-logout blocker is closed and next-request invalidation works. Its one new "significant" — that the session.id guard can't distinguish "never persisted" from "persisted this request via a separate update() payload" — is a real Harper-API limitation but NOT reachable via any OAuth flow (every clearOAuthSession caller runs on a cookie-loaded session with an id; handleCallback, the only id-less session-creator, never calls clearOAuthSession). Documented on the guard. Also refreshes a stale makeSession test comment. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NxkByn5VD9Mwh7Pm9wtmBH --- src/lib/handlers.ts | 8 ++++++++ test/lib/withOAuthValidation.test.js | 11 +++++------ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/lib/handlers.ts b/src/lib/handlers.ts index 33b0011..d20e603 100644 --- a/src/lib/handlers.ts +++ b/src/lib/handlers.ts @@ -553,6 +553,14 @@ export async function clearOAuthSession(session: any, logger?: Logger): Promise< // 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 diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index 17c961d..ac0ea07 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/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: { From 4c20cc1700d0194ea2206aad8a99fd9c45413a78 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Tue, 1 Sep 2026 08:40:04 -0700 Subject: [PATCH 5/6] fix: clear in-memory session fields on logout so the current request sees no identity On the production path (session.id + update present), clearOAuthSession was persisting { user: null } to hdb_session but not clearing the in-memory request.session fields. Any middleware or requireAuth:false resource that ran later in the same request still saw the stale identity and tokens. Move the in-memory clear (session.user = null; delete session.oauth; delete session.oauthUser) out of the else branch so it runs unconditionally in every code path. The session.update() guard for DB persistence is unchanged. Update two tests in withOAuthValidation.test.js that were asserting the old buggy behavior (in-memory fields untouched on the production path) and add a new test in handlers.test.js pinning the correct post-fix behavior. Co-Authored-By: Claude Sonnet 4.6 --- src/lib/handlers.ts | 13 +++++---- src/lib/withOAuthValidation.ts | 13 ++++----- test/lib/handlers.test.js | 25 +++++++++++++++++ test/lib/withOAuthValidation.test.js | 42 +++++++++++++--------------- 4 files changed, 58 insertions(+), 35 deletions(-) diff --git a/src/lib/handlers.ts b/src/lib/handlers.ts index d20e603..1d8ce32 100644 --- a/src/lib/handlers.ts +++ b/src/lib/handlers.ts @@ -567,13 +567,14 @@ export async function clearOAuthSession(session: any, logger?: Logger): Promise< // 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 { - // No existing session / no persistence (anonymous logout, non-session - // transport, tests): clear in memory only. - session.user = null; - delete session.oauth; - delete session.oauthUser; } + // Always clear the in-memory copy so the current request no longer sees a + // valid identity (requireAuth:false resources / later middleware). The + // persisted row is already null via update() above when a session.id was present; + // this keeps the in-memory view consistent in every path. + 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 20660b7..16165ac 100644 --- a/src/lib/withOAuthValidation.ts +++ b/src/lib/withOAuthValidation.ts @@ -39,13 +39,12 @@ export interface OAuthValidationOptions { * `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 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. + * `clearOAuthSession` internally before the callback runs. + * `clearOAuthSession` always clears the in-memory session fields + * (`session.user = null`, `delete session.oauth`, `delete session.oauthUser`) + * and, when a session.id is present, also persists `session.update({ user: null })` + * to the DB. As a result, the callback sees `request.session.oauth` and + * `.oauthUser` as `undefined`. Either way the callback is still invoked. */ onValidationError?: (request: Request, error: string) => any; } diff --git a/test/lib/handlers.test.js b/test/lib/handlers.test.js index 3f459be..f92a1d0 100644 --- a/test/lib/handlers.test.js +++ b/test/lib/handlers.test.js @@ -1235,6 +1235,31 @@ describe('OAuth Handlers', () => { ); }); + it('clears in-memory session fields on the production path so the current request sees no identity', async () => { + // Regression: before the fix, clearOAuthSession only cleared in-memory + // fields in the else branch. On the production path (session.id + update), + // update() persisted { user: null } to the DB but the in-memory session + // object still held the stale identity for the remainder of the request. + 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); + + 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 () => { // Harper defines `.update` on every request, including cookie-less ones, // and calling it mints a fresh non-expiring hdb_session row. An diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index ac0ea07..198b61c 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -953,14 +953,13 @@ 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 `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. + it('production-path session: callback sees cleared in-memory session after clearOAuthSession', async () => { + // `validateAndRefreshSession` calls `clearOAuthSession` before returning + // `{valid: false}`. clearOAuthSession now unconditionally clears the + // in-memory session fields (user=null, oauth/oauthUser deleted) AND + // persists via session.update({ user: null }) when session.id is present. + // The onValidationError callback therefore sees the session AFTER the + // in-memory clear — oauth and oauthUser are undefined. const session = makeProductionLikeSession({ oauth: { provider: 'github', @@ -999,9 +998,10 @@ 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'); + // 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'); }); @@ -1137,15 +1137,13 @@ describe('withOAuthValidation', () => { assert.equal(calls[0].oauthUserAfterValidate, undefined); }); - it('production path (session has update): underlying method runs, invalidation persisted via update({user:null})', 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` 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. + // clearOAuthSession now always clears the in-memory session fields, + // so by the time the resource runs, oauth is undefined even on + // the production path. calls.push({ target, oauthAfterValidate: this._context.session.oauth, @@ -1181,11 +1179,11 @@ describe('withOAuthValidation', () => { 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' ); }); }); From 0cb731eb24d80fb84f71373815e28497cc6e9a1a Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Tue, 1 Sep 2026 12:06:00 -0700 Subject: [PATCH 6/6] style: trim clearOAuthSession and related comments to essentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reduces verbose JSDoc and inline blocks added in PR #211 to their load-bearing facts: the H5 no-.delete constraint, the session.id anonymous-row guard, and the in-memory clear purpose. Cuts history, narrative, and repeated elaboration. Comment-only — no logic changed. Co-Authored-By: Claude Sonnet 4.6 --- src/lib/handlers.ts | 40 +++++----------------------- src/lib/withOAuthValidation.ts | 21 +++++---------- test/lib/handlers.test.js | 15 +++-------- test/lib/sessionValidator.test.js | 3 +-- test/lib/withOAuthValidation.test.js | 35 +++++------------------- 5 files changed, 23 insertions(+), 91 deletions(-) diff --git a/src/lib/handlers.ts b/src/lib/handlers.ts index 1d8ce32..0dbec06 100644 --- a/src/lib/handlers.ts +++ b/src/lib/handlers.ts @@ -532,46 +532,20 @@ export async function handleCallback( } /** - * Clear OAuth session data and log out the user. - * Shared by explicit logout and automatic logout on token expiration. - * - * `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. + * 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; - // 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. + // 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') { - // 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 }); } - // Always clear the in-memory copy so the current request no longer sees a - // valid identity (requireAuth:false resources / later middleware). The - // persisted row is already null via update() above when a session.id was present; - // this keeps the in-memory view consistent in every path. + // Clear in memory too so the current request sees no identity. session.user = null; delete session.oauth; delete session.oauthUser; diff --git a/src/lib/withOAuthValidation.ts b/src/lib/withOAuthValidation.ts index 16165ac..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. - * `clearOAuthSession` always clears the in-memory session fields - * (`session.user = null`, `delete session.oauth`, `delete session.oauthUser`) - * and, when a session.id is present, also persists `session.update({ user: null })` - * to the DB. As a result, the callback sees `request.session.oauth` and - * `.oauthUser` as `undefined`. Either way the callback is still invoked. + * `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 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. + * - 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 f92a1d0..45bbc42 100644 --- a/test/lib/handlers.test.js +++ b/test/lib/handlers.test.js @@ -1215,11 +1215,7 @@ describe('OAuth Handlers', () => { describe('handleLogout', () => { 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). + // 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); @@ -1236,10 +1232,7 @@ describe('OAuth Handlers', () => { }); it('clears in-memory session fields on the production path so the current request sees no identity', async () => { - // Regression: before the fix, clearOAuthSession only cleared in-memory - // fields in the else branch. On the production path (session.id + update), - // update() persisted { user: null } to the DB but the in-memory session - // object still held the stale identity for the remainder of the request. + // Production path must also clear in-memory so the current request sees no identity. mockRequest.session = { id: 'session-123', update: createMockFn(), @@ -1261,9 +1254,7 @@ describe('OAuth Handlers', () => { }); 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. + // .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); diff --git a/test/lib/sessionValidator.test.js b/test/lib/sessionValidator.test.js index edb37c1..f11ec58 100644 --- a/test/lib/sessionValidator.test.js +++ b/test/lib/sessionValidator.test.js @@ -257,8 +257,7 @@ 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 invalidated — clearOAuthSession persists { user: null }; the - // full-replace put drops oauth, so the reloaded record has no oauth. + // clearOAuthSession persisted { user: null }, dropping oauth. assert.strictEqual(session.user, null); assert.strictEqual(session.oauth, undefined); }); diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index 198b61c..b690afb 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -57,11 +57,7 @@ describe('withOAuthValidation', () => { } function makeSession(overrides = {}) { - // 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. + // Typical authenticated session — id + update so clearOAuthSession takes the persist branch. return { id: 'sess-1', oauth: { @@ -76,9 +72,7 @@ describe('withOAuthValidation', () => { }; } - // 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. + // Spy on .update so tests can assert clearOAuthSession persisted { user: null }. function makeProductionLikeSession(overrides = {}) { const base = makeSession(overrides); const updateCalls = []; @@ -954,12 +948,7 @@ describe('withOAuthValidation', () => { }); it('production-path session: callback sees cleared in-memory session after clearOAuthSession', async () => { - // `validateAndRefreshSession` calls `clearOAuthSession` before returning - // `{valid: false}`. clearOAuthSession now unconditionally clears the - // in-memory session fields (user=null, oauth/oauthUser deleted) AND - // persists via session.update({ user: null }) when session.id is present. - // The onValidationError callback therefore sees the session AFTER the - // in-memory clear — oauth and oauthUser are undefined. + // clearOAuthSession clears in-memory fields and persists — callback sees undefined oauth. const session = makeProductionLikeSession({ oauth: { provider: 'github', @@ -1084,14 +1073,7 @@ 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 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). - // + // 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. @@ -1107,8 +1089,7 @@ describe('withOAuthValidation', () => { return { status: 200, body: { ran: true } }; } } - // A session with NO update method (and no delete) — e.g. a - // non-session transport — exercises the in-memory fallback. + // No .update method — exercises the in-memory fallback. const session = makeSession({ oauth: { provider: 'github', @@ -1131,8 +1112,7 @@ 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, so the resource observes an empty session (undefined, not null). + // Fallback: oauth fields deleted in memory. assert.equal(calls[0].oauthAfterValidate, undefined); assert.equal(calls[0].oauthUserAfterValidate, undefined); }); @@ -1141,9 +1121,6 @@ describe('withOAuthValidation', () => { const calls = []; class MyResource extends MockResource { async get(target) { - // clearOAuthSession now always clears the in-memory session fields, - // so by the time the resource runs, oauth is undefined even on - // the production path. calls.push({ target, oauthAfterValidate: this._context.session.oauth,