fix(session): stop repeating the credentials in the body that sets the cookies - #153
Merged
Merged
Conversation
…e cookies Five handlers that issue session cookies returned the upstream body unchanged, and that body carries the access token and the refresh token, because it is the same body issueSessionCookies reads them out of to build the cookies. Every completed sign-in answered with Set-Cookie httpOnly and then handed the same two values to the caller as JSON. The httpOnly flag exists to keep those tokens away from page scripts. A response body is not: it reaches a devtools or HAR export shared while debugging, a service worker, an extension with request access, an APM tool that records payloads, a proxy logging bodies. The refresh token is the durable session credential, so it is the half that matters. finishRegisterHandler already had this right, answering 204 with no body. finishLoginHandler, verifyLoginOtpHandler, finishOAuthLoginHandler, pollMagicLinkConfirmationHandler and switchOrganizationHandler did not. Only token and refreshToken are removed, through one exported helper rather than five copies, so the handlers cannot drift apart on it again. message, sub, email, roles, phone, organizationId, ttl, refreshTtl and returnTo all survive, and the cookies are unchanged. The new test enumerates the cookie-issuing handlers so a sixth is covered by adding a line rather than by remembering the invariant exists.
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.
Five handlers that issue session cookies returned the upstream body unchanged, and that body
carries the access token and the refresh token, because it is the same body
issueSessionCookiesreads them out of to build the cookies. Every completed sign-inanswered with
Set-Cookie: httpOnlyand then handed the same two values to the caller asJSON.
Confirmed by running it, not by reading it
Invoking the real handlers against a mocked upstream, before the change:
After:
Why it matters, and where it stops
The
httpOnlyflag on those cookies exists to keep the tokens out of reach of page scripts.A response body is not: it is readable by anything that can see the response, and it reaches
places a cookie does not, including a devtools or HAR export shared while debugging, a
service worker, a browser extension with request access, an APM tool that records payloads,
and a proxy configured to log bodies. The refresh token is the durable session credential, so
it is the half that matters.
Being fair about the ceiling: an attacker has to already be able to observe that specific
response, nothing here persists the value, and the cookie itself was never exposed. This
restores a defence rather than closing an open door.
The correct implementation already existed
finishRegisterHandleranswers204with no body at all. The five that did not arefinishLoginHandler,verifyLoginOtpHandler,finishOAuthLoginHandler,pollMagicLinkConfirmationHandlerandswitchOrganizationHandler. Both the Express andFastify adapters route through these same core handlers, so both were affected.
A blanket
204was not an option: these bodies carry fields callers do read. Onlytokenand
refreshTokenare removed, through one exported helper rather than five copies, so thehandlers cannot drift apart on it again.
message,sub,email,roles,phone,organizationId,ttl,refreshTtlandreturnToall survive, and the cookies areuntouched.
Blast radius
Nothing in
@seamless-auth/reactreads.tokenor.refreshTokenoff a response body: zeromatches across that package. Its result types already declare them absent, with the comment
that sessions are carried by cookies so adopters have no reason to handle raw tokens. So the
wire was contradicting the first-party client's own stated contract.
It is still a wire change for an adopter reading those fields directly against that guidance,
which is why this is
minoron all three packages rather than a patch.Tests
A new test enumerates the cookie-issuing handlers in one list and asserts three things of
each: no token in the body, the fields callers read still present, and the access token still
in the cookie. A sixth handler is covered by adding a line rather than by remembering the
invariant exists.
pnpm testacross the workspace: 448 tests pass, up from 433. No existing test neededchanging, which is itself the evidence that the leak was incidental rather than intended.