fix: persist the mfa session cookie outside the browser - #52
Open
lakhansamani wants to merge 1 commit into
Open
Conversation
`credentials: 'include'` is browser-only, so node dropped every Set-Cookie the server returned. Since server 2.4.0 MFA is on by default: signup/login withhold the access token, return "Proceed to mfa setup", and identify the pending user by an mfa_session cookie. skipMfaSetup / verifyOtp / the webauthn MFA-setup path resolve it only if that cookie comes back, so all of them failed with "invalid session" — the entire MFA surface was unreachable from node while the methods existed and read as correct. Store the mfa_session cookies per instance and replay them on the graphql/rest choke point. Scoped to those cookies rather than a general jar on purpose: the server resolves identity from the `cookie` session before the Authorization header, so replaying a login session would silently override a caller-supplied bearer token.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
authorizer-jshas the same class of bug the Go SDK had: no cookie persistence outside the browser.Every call goes through
graphqlQuery/restQuery, both of which passcredentials: 'include'. That is a browser-only mechanism — node's fetch has no cookie store, so everySet-Cookiethe server returns is silently dropped.Since server 2.4.0 MFA is on by default.
signup/loginwithhold the access token, return"Proceed to mfa setup"withshould_show_totp_screen: true, and identify the pending user by anmfa_sessioncookie.skipMfaSetup,verifyOtpand the webauthn MFA-setup path resolve that offer only if the cookie comes back.So on node every one of them failed with
invalid session, while the methods existed and read as correct.User-visible impact
Any server-side (node) integration — SSR, a BFF, CLI tooling, tests — could sign a user up or log them in and then had no way to finish authentication. The token was withheld and the only method that releases it always failed. The whole MFA surface was unreachable from node.
Browsers were unaffected (the browser owns the cookie).
Reproduced against a real 2.4.0+ server on
localhost:8280, both protocols:After the fix, same script,
signup -> MFA offer -> skipMfaSetup -> getProfile:The fix
One choke point,
fetchWithCookies, used by bothgraphqlQueryandrestQuery. In a browser it is a plain fetch (the browser owns cookies andCookieis a forbidden request header anyway); elsewhere it replays the stored MFA session and records what the response sets. Caller-supplied headers still win.Scoped to the
mfa_session*cookies on purpose, not a general cookie jar. The server resolves a request's identity from thecookiesession before theAuthorizationheader (GetUserIDFromSessionOrAccessToken), so a general jar would make a stored login session silently override a bearer token the caller passed explicitly. That is not theoretical — a first pass at this fix used a full jar and broke three existing integration tests, including one wheregetProfileover REST returned a different user id than over GraphQL. The MFA session is bound to one user id server-side and consumed on use, so it cannot be traded for another user's token.Cookies are stored per
Authorizerinstance (same model as the Go SDK's jar), and dropped when the server expires them (Max-Age<=0).Tests
__test__/mfaMethods.test.ts: asserts themfa_sessioncookie set bysignupis replayed on the follow-upskipMfaSetup, that the login session cookie is not replayed, and that an expired cookie is dropped rather than resent.