fix(cookies): parse an upstream cookie ttl before it reaches an adapter - #145
Merged
Conversation
Registration failed on Fastify with `TypeError: option maxAge is invalid: 300`. The auth API returns ttl as the string "300" on its registration response. Handler results declare ttl as a number but fill it from a parsed JSON body, so nothing caught the mismatch. From there the adapters diverged. Express multiplies the value into milliseconds, which coerces the string to a number and hides it, so registration has always worked there. Fastify passes the value through to `cookie`, whose Number.isInteger check rejects a string and throws. applyCookies now parses the lifetime once, before an adapter sees it, and uses the parsed value for the Max-Age, the Expires, and the signed cookie's own expiry. Anything that is not a positive whole number of seconds throws with the offending value, rather than issuing a session cookie with a lifetime nobody can vouch for. The parity suite hand-wrote ttl as a number in every scenario, so it agreed on input the real upstream does not send. It now covers a string ttl through the registration route, and fails without this change. The API returning a string here is a separate defect, fixed alongside this. Parsing at the boundary is what stops the next untyped upstream value from splitting the adapters the same way. Verified with the core, express, and fastify suites: 225, 150, and 44 passing.
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
1 participant
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.
Third of three, and the one that stops this class of bug recurring. Not blocked on the other two: it fixes the symptom regardless of what upstream sends, and can merge first.
The bug
Registration fails on the Fastify adapter with
TypeError: option maxAge is invalid: 300. The auth API sendsttlas the string"300"on its registration response. Handler results declarettl: numberbut populate it fromawait up.json(), which isany, so nothing catches the mismatch.From there the adapters diverge:
internal/respond.tsmaxAge: command.maxAgeSeconds * 1000"300" * 1000 === 300000, coerced, worksmaxAge: command.maxAgeSecondscookie,Number.isIntegerfails, throwsBoth are correct for their own library's units. The difference is that one of them coerces as a side effect and the other does not, which is not a property either adapter should be relied on for.
The fix
applyCookiesparses the lifetime once, before an adapter sees it, and uses the parsed value for all three things that derive from it: theMax-Age, theExpires, and the signed cookie's own expiry.applyCookiesis the single choke point. It handles both the handler results and the silent-refresh path inensureCookies, in both adapters, so nothing else needs touching.Anything that is not a positive whole number of seconds now throws with the offending value. A cookie is the session, and issuing one with a lifetime nobody can vouch for is worse than refusing the response. Previously garbage produced
NaNand threw somewhere less legible anyway.Why the parity suite missed it
Every scenario hand-writes
ttl: 300as a genuine number, so the suite agreed on input the real upstream does not send. Both adapters do agree on correct input; they only diverge on the wrong input.Added a scenario that sends
ttlas a string through/registration/registerand asserts both adapters emit identical cookies. I confirmed it fails without the core change by stashing the fix, rebuilding, and re-running.Verification
core 225 passing, express 150 passing, fastify 44 passing. Twelve new core tests cover the numeric string matching the number exactly (maxAge, expires, and the signed expiry), plus seven rejection cases.
The rest of the fix
fells-code/seamless-auth-types#27 and the matching API change stop the string being sent at all. This PR is what stops the next untyped upstream value splitting the adapters the same way.