fix(auth): re-auth when proxy identity differs from accessToken JWT#19
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes a stale-session bug where Outline kept serving a previously authenticated user after the upstream oauth2-proxy/Cognito session was rotated to a different identity. Because parseAuthentication prefers the accessToken JWT cookie over x-auth-request-email for performance, the cookie outlived the upstream identity. The fix detects mismatch in the JWT branch and throws 401, which the existing catch block converts into a Set-Cookie expiring the stale cookie so the next request re-authenticates via the ForwardAuth header path.
Changes:
- Extracts proxy email normalization (lowercase/trim + bare-username synthesis with
DEFAULT_EMAIL_DOMAIN) into a sharednormalizeProxyEmailhelper, reused by thefwd:branch. - In the JWT branch of
validateAuthentication, whenAUTH_TYPE=SSOand transport iscookie, compares the normalizedx-auth-request-emailheader againstuser.emailand throws 401 on mismatch. - Adds two test cases covering the stale-cookie clear path and the steady-state no-op path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| server/middlewares/authentication.ts | Adds normalizeProxyEmail helper and SSO stale-session mismatch check in the JWT branch. |
| server/middlewares/authentication.test.ts | Adds tests for stale-cookie clearing and matching-email no-op behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot Pressingly/plane#29 use this information for review. |
Thanks — reviewed the Plane PR #29 for architectural context. The primary actionable item here was the CodeQL finding about the polynomial regex at line 217. Fixed in commit |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
server/middlewares/authentication.ts:423
- If
x-auth-request-emailis absent (e.g. a request that reached this app bypassing oauth2-proxy, or a background/internal call that still carries the cookie), the stale-session check is silently skipped becauseheaderRawis falsy. In an SSO deployment that means a stolen/leakedaccessTokencookie remains valid even when the proxy is no longer asserting any identity — arguably the opposite of the intended invariant ("never serve a request whose proxy identity disagrees with the JWT"). Consider whether absence of the header inAUTH_TYPE=SSO+ cookie transport should also be treated as stale, or at least documenting why it's deliberately permissive.
const headerRaw = ctx.request.get("x-auth-request-email");
if (
headerRaw &&
normalizeProxyEmail(headerRaw) !== (user.email ?? "").toLowerCase()
) {
throw AuthenticationError("Stale session — proxy identity changed");
}
server/middlewares/authentication.ts:420
user.emailfromgetUserForJWTis already stored in canonical lowercase form at user-creation time, butnormalizeProxyEmailperforms.toLowerCase().trim()on the header only. If a User row was ever created with mixed-case email by a code path that bypasses normalization (e.g. legacy data, a fixture, or a different provider), this comparison will spuriously trigger a 401 → cookie clear → re-auth loop. Defensive.toLowerCase().trim()on both sides — or routing both throughnormalizeProxyEmail— would make the comparison symmetric. The existing.toLowerCase()on line 420 handles case but not whitespace.
normalizeProxyEmail(headerRaw) !== (user.email ?? "").toLowerCase()
| function normalizeProxyEmail(raw: string): string { | ||
| const trimmed = raw.toLowerCase().trim(); | ||
| // Avoid polynomial-backtracking regex on uncontrolled input. | ||
| // An email-shaped value must have "@" after the first character and a "." | ||
| // after the "@". isEmailShaped short-circuits on atIdx <= 0 so dotIdx | ||
| // is only meaningful when atIdx > 0. | ||
| const atIdx = trimmed.indexOf("@"); | ||
| const dotIdx = trimmed.indexOf(".", atIdx + 1); | ||
| const isEmailShaped = atIdx > 0 && dotIdx > atIdx + 1; | ||
| return isEmailShaped | ||
| ? trimmed | ||
| : `${trimmed.split("@")[0]}@${env.DEFAULT_EMAIL_DOMAIN}`; | ||
| } |
| // auth() catch block: the accessToken + lastSignedIn cookies are | ||
| // expired via err.headers, and the client's next request takes the | ||
| // ForwardAuth header path → new JWT for the new user. | ||
| if (env.AUTH_TYPE === "SSO" && transport === "cookie") { |
| function normalizeProxyEmail(raw: string): string { | ||
| const trimmed = raw.toLowerCase().trim(); | ||
| // Avoid polynomial-backtracking regex on uncontrolled input. | ||
| // An email-shaped value must have "@" after the first character and a "." | ||
| // after the "@". isEmailShaped short-circuits on atIdx <= 0 so dotIdx | ||
| // is only meaningful when atIdx > 0. | ||
| const atIdx = trimmed.indexOf("@"); | ||
| const dotIdx = trimmed.indexOf(".", atIdx + 1); | ||
| const isEmailShaped = atIdx > 0 && dotIdx > atIdx + 1; | ||
| return isEmailShaped | ||
| ? trimmed | ||
| : `${trimmed.split("@")[0]}@${env.DEFAULT_EMAIL_DOMAIN}`; | ||
| } |
|
Addressed the two open review points in commit
CodeQL scan returns 0 alerts. |
awais786
changed the title
awais786
changed the title
PR #19's mismatch detection threw a 401 + Set-Cookie clearing, which correctly fixed the security bug but produced a rough UX: the SPA saw the 401 mid-page-load and showed a "no access to this doc" toast, and the user had to hard-reload to recover. Switch to a 302 redirect to /home. The browser / fetch auto-follows the redirect with the just-cleared cookie gone — lands on Outline's home route, ForwardAuth adds the new user's X-Auth-Request-Email header, the fwd: path issues a fresh JWT, and the page renders as the new user. No 401 surfaces, no manual reload. Why /home and not ctx.originalUrl: the previous user may have had access to a doc the new user can't see; redirecting back to the same URL would just trade the 401 for a 404/403. /home is a known-good landing page for any authenticated user. Adds StaleSessionRedirect (httpErrors 302 with redirectTo property) so the existing err.headers mechanism can carry both Set-Cookie and Location through Koa's onerror handler. The 302 handling sits in the outer auth() catch block before the existing 401 cookie-cleanup branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
awais786
force-pushed
the
fix/proxy-auth-stale-session-on-user-switch
branch
from
7130f9a to
369b8c2
Compare
When the bundle's portal "Log out of all apps" clears the shared
oauth2-proxy cookie + Cognito SSO session but a different user then
logs in, Outline's per-subdomain accessToken JWT survives — every
subsequent request keeps serving the previous user's session. Fixed
in two layers:
Server-side (validateAuthentication):
- JWT branch detects mismatch when SSO mode + cookie transport +
x-auth-request-email header email != user.email (lowercase + trim
on both sides; required by openspec). Throws StaleSessionRedirect
(302 → /home + Set-Cookie clearing accessToken) so the followed
request re-enters the fwd: header path, provisions the new user,
and issues a fresh JWT. 302 chosen over 401 to avoid SPA "no
access" toasts on doc URLs the new user can't see.
- fwd: provisioning serialised behind pg_advisory_xact_lock keyed on
hashtext(email). Outline's users table has no unique email
constraint (multi-team support), so parallel first-login requests
would race to create duplicate rows — production hit 5 dupes
before audit caught it. Lock is per-email so distinct users don't
contend.
- normalizeProxyEmail rewritten to use indexOf chain instead of regex
to close a polynomial-backtracking CodeQL finding; same semantics.
Client-side (userContinuity, ApiClient, AuthStore):
- checkUserContinuity at SPA boot compares accessToken JWT id to a
localStorage marker; on mismatch wipes localStorage + IndexedDB +
Cache API + unregisters service workers before window.location
.replace("/home") so AuthStore rehydrates from clean state.
- ApiClient.fetch detects 302→HTML bounces on /api/* (either via
response.redirected off the /api namespace or text/html
Content-Type) and triggers the same wipeAndReload. Belt-and-
suspenders fallback in AuthStore.fetchAuth wipes if /auth.info
returns without data.user. Required because Outline's auth
middleware only runs on /api/*, not on / — so at first SPA mount
after switch the accessToken cookie hasn't rotated yet and the
boot-time check can't see the mismatch.
Tests cover: server-side mismatch + cookie clearing, case- and
whitespace-variant emails, SQL-LIKE wildcard rejection (uses
findOne with exact-equality, not LIKE), advisory lock acquisition
before User.create.
awais786
force-pushed
the
fix/proxy-auth-stale-session-on-user-switch
branch
from
369b8c2 to
bbf9ea3
Compare
|
@copilot review Squashed all 14 prior commits + new client-side wipe layer into a single commit
|
Agent-Logs-Url: https://github.com/Pressingly/outline/sessions/2112e912-6aad-49c0-b0d5-8e0bdc465145 Co-authored-by: awais786 <445320+awais786@users.noreply.github.com>
Review complete. One real bug found and fixed (commit CI failure ( Server-side review (
Client-side review (
CodeQL: 0 alerts. TypeScript: clean after the |
The 302 → wipe-and-reload flow already clears localStorage on user
switch, but `postLoginRedirectPath` lives in sessionStorage AND a
cookie, so it survived the wipe. AuthenticatedLayout reads that key
on mount via `usePostLoginPath` and issues `<Redirect to={path} />`,
sending the new user to the previous user's URL (e.g. a Collection
they have no access to). Net effect: even after the wipe lands the
SPA on /home, layout immediately bounces it back to the stale URL.
Three changes:
- `userContinuity.clearAuxiliaryUserState`: drops
`sessionStorage.postLoginRedirectPath`, the `postLoginRedirectPath`
cookie, and the `lastSignedIn` cookie. Called from all wipe paths
in `userContinuity.ts` (cookie-present mismatch, cookie-absent +
marker-present, `wipeAndReload`).
- `userContinuity.checkUserContinuity`: also wipes when the
accessToken cookie is absent but a `LAST_USER_KEY` marker exists —
the portal-logout + re-login-as-different-user path, where
oauth2-proxy clears the cookie but the SPA still has the previous
user's `AUTH_STORE` rehydrated.
- `useLastVisitedPath.clearLastVisitedPath` + call from
`AuthStore.logout`: belt-and-suspenders so an explicit in-app
logout also resets the remembered path. Without this, a same-
browser sign-out + sign-in-as-different-user via the in-app
button would still surface the previous path.
- `ApiClient.fetch`: tightened the `text/html` fallback to fire
only on success (2xx) responses, so a transient Traefik 502 or
oauth2-proxy login-redirect HTML page no longer triggers an
unnecessary state wipe.
|
@copilot review New commit The bug it fixes: even after my prior wipe + 302 → Changes:
Please re-review for correctness and any leaks I might have missed. |
Review complete. CI and CodeQL both pass on
|
…flag After the wipe + reload to /home, `AuthStore.fetchAuth` may still redirect away (team domain change, default collection, postLogin redirects in AuthenticatedLayout) before the previous fix's cleanup takes hold. Add a one-shot sessionStorage flag `outline_post_switch_redirect_home_once` set immediately before each wipe-path navigation. `fetchAuth` consumes it after data is loaded and forces `window.location.replace(homePath())`, overriding any subsequent redirect logic. Flag is sessionStorage-scoped so it naturally clears on tab close and doesn't leak across switches.
Adds a 1-hour cookie fallback alongside the sessionStorage entry for the one-shot post-switch redirect flag. sessionStorage is per-tab and may be unavailable in private-browsing or quota-exhausted modes; the cookie ensures `consumePostSwitchRedirectHome` still returns true on the post-wipe boot in those environments. Consume path clears both surfaces.
If the post-switch wipe lands the SPA on /home and AuthStore.fetchAuth consumes the redirect flag, calling window.location.replace(homePath()) on a page already at homePath() causes a redundant reload. Guard the replace with a pathname comparison so the flag still gets consumed (clearing the marker) but the navigation is a no-op when unnecessary.
|
@copilot review Final review request. Branch is at Full commit chain on this PR:
Question for review: Is the layered approach overkill, or are all five layers load-bearing? Specifically:
Each was added in response to a real reproduction. Confirm or recommend collapsing. |
Review complete. CI passes on All 5 layers are load-bearing — each was triggered by a real repro gap:
One minor structural note (not a bug): the No code changes needed. Architecture is correct as-is. |
Summary
parseAuthenticationprefers theaccessTokencookie over thex-auth-request-emailheader (intentional perf optimisation — avoids per-request DB lookup). Once Outline has issued its JWT cookie, the upstream identity is no longer consulted._oauth2_proxycookie and Cognito SSO session but not Outline'saccessTokencookie ondocs.<domain>— refreshing this app's tab kept serving the previous user.validateAuthentication, whenAUTH_TYPE=SSOandtransport=cookie, compare the normalisedx-auth-request-emailagainst the JWT user's email. Mismatch → throw 401.auth()already handles cookie cleanup for cookie-transported 401s (setsaccessToken=; expires=...viaerr.headers), so the next request takes the ForwardAuth header path and issues a fresh JWT for the new user.normalizeProxyEmailso the mismatch check and the existingfwd:-branch synthesis use identical logic — no drift possible ifDEFAULT_EMAIL_DOMAINsemantics change.Repro (before this PR)
docs.<domain>) in a tab.Same architectural pattern as Pressingly/plane#29 — a native session cookie that outlives upstream auth state. SurfSense doesn't exhibit it because FastAPI re-derives identity from headers on every request rather than persisting a native cookie.
Test plan
pnpm test server/middlewares/authentication.test.tspasses — new cases:should clear stale accessToken cookie when proxy identity changes— alice's JWT cookie + bob's email header → 401 +Set-Cookie: accessToken=; expires=...should NOT clear cookie when proxy email matches JWT user— steady-state regression guardshould treat case- and whitespace-variant proxy email as matching the JWT user— bidirectional normalisation guardfwd:-branch synthesis now goes throughnormalizeProxyEmailbut produces the same canonical email.Also included (incidental, surfaced during review)
While auditing this code path, a separate concurrent-creation race was found in the
fwd:-branch provisioning logic (Outline has nousers.emailunique constraint upstream, so N parallel first-login requests could insert N duplicate rows). A Postgres advisory lock was added around the find+create as part of this PR. Independent of the user-switch repro above. Has its own test (should take the advisory lock before creating the ForwardAuth user) and a small CodeQL fix (polynomial regex →indexOf).Out of scope
This addresses Outline only. Plane has the analogous fix in Pressingly/plane#29. Penpot in Pressingly/penpot#18. Twenty in Pressingly/twenty#8.
🤖 Generated with Claude Code