Skip to content

fix: actually invalidate the session on OAuth logout - #211

Merged
heskew merged 7 commits into
mainfrom
security/f2-f4-identity-session
Sep 1, 2026
Merged

fix: actually invalidate the session on OAuth logout#211
heskew merged 7 commits into
mainfrom
security/f2-f4-identity-session

Conversation

@heskew

@heskew heskew commented Aug 24, 2026

Copy link
Copy Markdown
Member

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 (confirmed against harper@5.1.9, security/auth.ts:110). So the delete branch never ran, and the in-memory fallback (session.user = null) was never persisted: after an explicit logout — or when a token is expired/revoked and clearOAuthSession runs — the stored hdb_session record kept authenticating until cookie TTL.

Fix: invalidate by persisting session.update({ user: null, oauth: null, oauthUser: null }), mirroring Harper's own logout() (security/auth.ts:381). The next request resolves session.user to no user. Falls back to an in-memory clear only when no .update exists (non-session transports).

Tests updated across handlers / sessionValidator / withOAuthValidation — several had encoded the inverted delete-based model (asserting session.delete(id) and in-memory clears that never actually persisted). Full suite: 1157 unit + 15 integration, 0 fail.

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NxkByn5VD9Mwh7Pm9wtmBH
@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

Suggestions (non-blocking)

  • src/lib/withOAuthValidation.ts:112clearStaleOAuth should also clear session.user = null for in-memory consistency, matching the updated clearOAuthSession logic.
  • src/lib/handlers.ts:546 — The session.update({ user: null }) is a full-replace write without a version/CAS check; a concurrent token-refresh write could resurrect the session.

Comment thread src/lib/handlers.ts Outdated

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the clearOAuthSession function to use session.update instead of session.delete to invalidate sessions, aligning with the actual API of Harper's hdb_session record which only exposes .update. This ensures that a null-user record is persisted to invalidate the session, rather than attempting a non-existent delete method. Corresponding unit tests have been updated to mock and assert the new .update behavior and verify that session properties are correctly set to null upon invalidation. There are no review comments, and I have no additional feedback to provide.

@claude

claude Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found. This push (0cb731e) is comment-only — trims JSDoc/inline comments in handlers.ts and withOAuthValidation.ts with no logic change; prior threads remain resolved.

heskew and others added 3 commits August 24, 2026 11:46
… 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NxkByn5VD9Mwh7Pm9wtmBH
…F4 review)

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NxkByn5VD9Mwh7Pm9wtmBH
#211 review)

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NxkByn5VD9Mwh7Pm9wtmBH
@heskew heskew closed this Aug 25, 2026
@heskew heskew reopened this Aug 25, 2026
Comment thread src/lib/handlers.ts
Comment thread src/lib/handlers.ts
…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 <noreply@anthropic.com>
@heskew

heskew commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

Addressed in 4c20cc1: in-memory session fields are now cleared unconditionally on logout, closing both in-memory-state blockers. The CAS-race suggestion is pre-existing (tracked in #212). All threads resolved.

@heskew heskew left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at 4c20cc1: no blockers.

I traced the implementation against Harper 5.1.9. Persisting update({ user: null }) is the correct full-replacement invalidation, the session.id guard avoids minting rows on anonymous logout, and the unconditional in-memory clear means the current request no longer sees stale OAuth identity. The stale-write race tracked in #212 is pre-existing and remains a separate follow-up, not a blocker here.

Two non-blocking cleanup items:

  • The PR body still says the final implementation persists user/oauth/oauthUser as null. The current code correctly persists only { user: null } and deletes oauth/oauthUser from the in-memory copy.
  • test/lib/withOAuthValidation.test.js:1087-1093 still says the production update path does not mutate the in-memory session, which now contradicts both the implementation and the production-path test below it.

For the 1.x release train: keep this PR on main. Harper 4.7.19 assigns the live TableResource to request.session, so the existing 1.x session.delete(session.id) path is valid; Harper 5 creates the shallow copy that made this fix necessary. No mechanical 1.x backport is needed.

Validation: full local suite 1,157 passed / 2 skipped; explicit TypeScript check passed; lint has no errors; GitHub Node 22/24 unit and integration checks are green.

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 <noreply@anthropic.com>
Comment thread src/lib/handlers.ts
@heskew
heskew merged commit 8091f7b into main Sep 1, 2026
14 checks passed
@heskew
heskew deleted the security/f2-f4-identity-session branch September 1, 2026 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant