diff --git a/.changeset/coerce-upstream-cookie-ttl.md b/.changeset/coerce-upstream-cookie-ttl.md new file mode 100644 index 0000000..c684aaf --- /dev/null +++ b/.changeset/coerce-upstream-cookie-ttl.md @@ -0,0 +1,23 @@ +--- +'@seamless-auth/core': patch +'@seamless-auth/express': patch +'@seamless-auth/fastify': patch +--- + +Normalize a cookie lifetime arriving from upstream, so the two adapters cannot disagree about it. + +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 the problem, so it has always worked. Fastify passes the value through +to `cookie`, whose `Number.isInteger` check rejects a string and throws, failing the request. + +`applyCookies` now parses the lifetime once, before it reaches an adapter, 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. diff --git a/packages/core/src/applyResult.ts b/packages/core/src/applyResult.ts index 37bacb8..b585f8d 100644 --- a/packages/core/src/applyResult.ts +++ b/packages/core/src/applyResult.ts @@ -106,6 +106,34 @@ export function signSessionCookie( }); } +/** + * Normalizes a cookie lifetime arriving from upstream. + * + * Handler results declare `ttl` as a number, but they populate it from a parsed + * JSON body, which is untyped. When the value arrives as a numeric string the + * declaration is simply wrong, and the two adapters then disagree: Express + * multiplies it into milliseconds, which coerces it to a number and hides the + * problem, while Fastify passes it through to `cookie`, whose `Number.isInteger` + * check rejects it and fails the request. That is the difference between an + * adapter working and an adapter throwing on the same upstream response, so it + * is settled here rather than in either one. + * + * Anything that is not a positive whole number of seconds throws. A cookie is + * the session, and emitting one with a lifetime nobody can vouch for is worse + * than refusing the response. + */ +function toTtlSeconds(ttl: unknown): number { + const seconds = typeof ttl === "string" ? Number(ttl) : ttl; + + if (typeof seconds !== "number" || !Number.isInteger(seconds) || seconds <= 0) { + throw new Error( + `Upstream returned an unusable cookie ttl: ${JSON.stringify(ttl)}`, + ); + } + + return seconds; +} + function requireSecret(opts: CookieSecurityOptions): string { if (!opts.cookieSecret) { throw new Error("Missing cookieSecret"); @@ -150,12 +178,14 @@ export function applyCookies( const now = Date.now(); for (const cookie of result.setCookies) { + const ttlSeconds = toTtlSeconds(cookie.ttl); + adapter.setCookie({ name: cookie.name, - value: signSessionCookie(cookie.value, secret, cookie.ttl), + value: signSessionCookie(cookie.value, secret, ttlSeconds), domain: cookie.domain, - maxAgeSeconds: cookie.ttl, - expires: new Date(now + cookie.ttl * 1000), + maxAgeSeconds: ttlSeconds, + expires: new Date(now + ttlSeconds * 1000), httpOnly: true, secure, sameSite, diff --git a/packages/core/tests/applyResult.test.js b/packages/core/tests/applyResult.test.js index 6499d1f..70cc63e 100644 --- a/packages/core/tests/applyResult.test.js +++ b/packages/core/tests/applyResult.test.js @@ -271,3 +271,60 @@ describe("signSessionCookie", () => { expect(decoded.exp - decoded.iat).toBe(300); }); }); + +// The auth API's registration response has sent `ttl` as the string "300". +// Handler results declare it a number, but they fill it from a parsed JSON body, +// so nothing caught it. Express hid it by multiplying into milliseconds, which +// coerces; Fastify passed it to `cookie`, whose Number.isInteger check threw and +// failed the request. Normalizing here is what stops the two disagreeing. +describe("cookie ttl arriving from an untyped upstream body", () => { + const setCookie = (ttl) => { + const adapter = recorder(); + + applyCookies( + { setCookies: [{ name: "seamless-ephemeral", value: { sub: "u1" }, ttl }] }, + adapter, + { cookieSecret: SECRET }, + ); + + return adapter.calls.set[0]; + }; + + it("treats a numeric string exactly like the number", () => { + const fromString = setCookie("300"); + const fromNumber = setCookie(300); + + expect(fromString.maxAgeSeconds).toBe(300); + expect(fromString.maxAgeSeconds).toBe(fromNumber.maxAgeSeconds); + }); + + it("emits a maxAge the cookie library will accept", () => { + expect(Number.isInteger(setCookie("300").maxAgeSeconds)).toBe(true); + }); + + it("dates the expiry from the parsed seconds", () => { + const before = Date.now(); + const { expires } = setCookie("300"); + + expect(expires.getTime()).toBeGreaterThanOrEqual(before + 300 * 1000); + expect(expires.getTime()).toBeLessThan(before + 301 * 1000); + }); + + it("signs the cookie for the same lifetime either way", () => { + const decoded = jwt.verify(setCookie("300").value, SECRET); + + expect(decoded.exp - decoded.iat).toBe(300); + }); + + it.each([ + ["a non-numeric string", "15m"], + ["an empty string", ""], + ["a fraction", 300.5], + ["zero", 0], + ["a negative", -300], + ["null", null], + ["undefined", undefined], + ])("refuses %s rather than issuing a cookie nobody can vouch for", (_l, ttl) => { + expect(() => setCookie(ttl)).toThrow(/unusable cookie ttl/); + }); +}); diff --git a/packages/fastify/tests/parity.test.js b/packages/fastify/tests/parity.test.js index 0ba58f7..d772f3e 100644 --- a/packages/fastify/tests/parity.test.js +++ b/packages/fastify/tests/parity.test.js @@ -306,6 +306,35 @@ describe("fastify and express adapters agree", () => { ); }); + // The auth API's registration response sends `ttl` as the string "300". Every + // scenario in this file hand-writes a number, so the suite agreed on input the + // real upstream does not send: Express coerced the string by multiplying into + // milliseconds, Fastify handed it to `cookie` and got a TypeError, and + // registration failed on Fastify only. + it("issues identical session cookies when upstream sends ttl as a string", async () => { + const scenario = { + method: "post", + path: "/registration/register", + payload: { email: "user@example.com" }, + }; + const upstreamResponse = upstream(200, { + message: "Registration started", + sub: "user-123", + token: "registration-token", + ttl: "300", + }); + + const { fastify, express: expressResult } = await bothAdapters( + scenario, + upstreamResponse, + ); + + expect(fastify.status).toBe(200); + expect(fastify.status).toBe(expressResult.status); + expect(fastify.cookies).toEqual(expressResult.cookies); + expect(fastify.cookies.length).toBeGreaterThan(0); + }); + it.each([ ["default policy", {}], ["insecure dev", { cookieSecure: false }],