From cf39bb7adeb6db623bc75b7293ea59966a292b79 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 7 Sep 2026 18:58:12 -0400 Subject: [PATCH] fix(session): stop repeating the credentials in the body that sets the 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. --- .changeset/gentle-pandas-hide.md | 35 +++++ packages/core/src/handlers/finishLogin.ts | 7 +- packages/core/src/handlers/oauthHandlers.ts | 7 +- .../pollMagicLinkConfirmationHandler.ts | 7 +- .../src/handlers/switchOrganizationHandler.ts | 7 +- .../src/handlers/verifyLoginOtpHandler.ts | 7 +- packages/core/src/upstreamSession.ts | 20 +++ .../core/tests/sessionBodyMaterial.test.js | 132 ++++++++++++++++++ 8 files changed, 212 insertions(+), 10 deletions(-) create mode 100644 .changeset/gentle-pandas-hide.md create mode 100644 packages/core/tests/sessionBodyMaterial.test.js diff --git a/.changeset/gentle-pandas-hide.md b/.changeset/gentle-pandas-hide.md new file mode 100644 index 0000000..9bf2417 --- /dev/null +++ b/.changeset/gentle-pandas-hide.md @@ -0,0 +1,35 @@ +--- +'@seamless-auth/core': minor +'@seamless-auth/express': minor +'@seamless-auth/fastify': minor +--- + +Stop repeating the session credentials in the body of the response that sets them as +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. So 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 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. + +`finishRegisterHandler` already had this right, answering `204` with no body at all. The five +that did not are `finishLoginHandler`, `verifyLoginOtpHandler`, `finishOAuthLoginHandler`, +`pollMagicLinkConfirmationHandler` and `switchOrganizationHandler`. + +Only `token` and `refreshToken` are removed, by one exported helper rather than five copies, +so the handlers cannot drift apart on it again. Everything callers actually read survives: +`message`, `sub`, `email`, `roles`, `phone`, `organizationId`, `ttl`, `refreshTtl` and +`returnTo`. The cookies are unchanged, so sessions work exactly as before. + +Nothing in `@seamless-auth/react` reads either field off a response, and 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. An adopter reading `token` or `refreshToken` +directly off one of these responses, against that guidance, no longer can. diff --git a/packages/core/src/handlers/finishLogin.ts b/packages/core/src/handlers/finishLogin.ts index e5f4e09..ff3df51 100644 --- a/packages/core/src/handlers/finishLogin.ts +++ b/packages/core/src/handlers/finishLogin.ts @@ -1,5 +1,8 @@ import { authFetch } from "../authFetch.js"; -import { issueSessionCookies } from "../upstreamSession.js"; +import { + issueSessionCookies, + withoutSessionMaterial, +} from "../upstreamSession.js"; import { readPassthroughFailure } from "../upstreamError.js"; import type { ResultFailure } from "../result.js"; import type { CookiePayload } from "../ensureCookies.js"; @@ -53,7 +56,7 @@ export async function finishLoginHandler( return { status: 200, - body: data, + body: withoutSessionMaterial(data), setCookies: await issueSessionCookies(data, { authServerUrl: opts.authServerUrl, audience: opts.audience, diff --git a/packages/core/src/handlers/oauthHandlers.ts b/packages/core/src/handlers/oauthHandlers.ts index 77522a8..f708367 100644 --- a/packages/core/src/handlers/oauthHandlers.ts +++ b/packages/core/src/handlers/oauthHandlers.ts @@ -1,5 +1,8 @@ import { authFetch } from "../authFetch.js"; -import { issueSessionCookies } from "../upstreamSession.js"; +import { + issueSessionCookies, + withoutSessionMaterial, +} from "../upstreamSession.js"; import { readPassthroughFailure } from "../upstreamError.js"; import type { ResultFailure } from "../result.js"; import type { CookiePayload } from "../ensureCookies.js"; @@ -92,7 +95,7 @@ export async function finishOAuthLoginHandler( return { status: up.status, - body: data, + body: withoutSessionMaterial(data), setCookies: await issueSessionCookies(data, { authServerUrl: opts.authServerUrl, audience: opts.audience, diff --git a/packages/core/src/handlers/pollMagicLinkConfirmationHandler.ts b/packages/core/src/handlers/pollMagicLinkConfirmationHandler.ts index 1b3c749..3833088 100644 --- a/packages/core/src/handlers/pollMagicLinkConfirmationHandler.ts +++ b/packages/core/src/handlers/pollMagicLinkConfirmationHandler.ts @@ -1,5 +1,8 @@ import { authFetch } from "../authFetch.js"; -import { issueSessionCookies } from "../upstreamSession.js"; +import { + issueSessionCookies, + withoutSessionMaterial, +} from "../upstreamSession.js"; import { readPassthroughFailure } from "../upstreamError.js"; import type { ResultFailure } from "../result.js"; import type { CookiePayload } from "../ensureCookies.js"; @@ -64,7 +67,7 @@ export async function pollMagicLinkConfirmationHandler( return { status: 200, - body: data, + body: withoutSessionMaterial(data), setCookies: await issueSessionCookies(data, { authServerUrl: opts.authServerUrl, audience: opts.audience, diff --git a/packages/core/src/handlers/switchOrganizationHandler.ts b/packages/core/src/handlers/switchOrganizationHandler.ts index b32ffc2..77ee5b9 100644 --- a/packages/core/src/handlers/switchOrganizationHandler.ts +++ b/packages/core/src/handlers/switchOrganizationHandler.ts @@ -1,5 +1,8 @@ import { authFetch } from "../authFetch.js"; -import { issueSessionCookies } from "../upstreamSession.js"; +import { + issueSessionCookies, + withoutSessionMaterial, +} from "../upstreamSession.js"; import { readPassthroughFailure } from "../upstreamError.js"; import type { ResultFailure } from "../result.js"; import type { CookiePayload } from "../ensureCookies.js"; @@ -61,7 +64,7 @@ export async function switchOrganizationHandler( return { status: up.status, - body: data, + body: withoutSessionMaterial(data), setCookies: await issueSessionCookies(data, { authServerUrl: opts.authServerUrl, audience: opts.audience, diff --git a/packages/core/src/handlers/verifyLoginOtpHandler.ts b/packages/core/src/handlers/verifyLoginOtpHandler.ts index 46cf0a7..dffbcad 100644 --- a/packages/core/src/handlers/verifyLoginOtpHandler.ts +++ b/packages/core/src/handlers/verifyLoginOtpHandler.ts @@ -1,5 +1,8 @@ import { authFetch } from "../authFetch.js"; -import { issueSessionCookies } from "../upstreamSession.js"; +import { + issueSessionCookies, + withoutSessionMaterial, +} from "../upstreamSession.js"; import { readPassthroughFailure } from "../upstreamError.js"; import type { ResultFailure } from "../result.js"; import type { CookiePayload } from "../ensureCookies.js"; @@ -67,7 +70,7 @@ async function verifyOtp( return { status: up.status, - body: data, + body: withoutSessionMaterial(data), setCookies: await issueSessionCookies(data, { authServerUrl: opts.authServerUrl, audience: opts.audience, diff --git a/packages/core/src/upstreamSession.ts b/packages/core/src/upstreamSession.ts index fe3fd99..1f6bbae 100644 --- a/packages/core/src/upstreamSession.ts +++ b/packages/core/src/upstreamSession.ts @@ -25,6 +25,26 @@ export interface UpstreamSessionResponse { [key: string]: unknown; } +/** + * The response body minus the credentials the cookies now carry. + * + * The access and refresh tokens go out as `httpOnly` cookies precisely so that + * page scripts cannot read them. Returning the upstream body unchanged handed the + * same two values straight back to the caller in the response that set those + * cookies, which put them everywhere a body goes and a cookie does not: a devtools + * export, a service worker, an APM tool that records payloads, a proxy logging + * bodies. + * + * Everything else survives. Callers read `message`, `email`, `roles`, + * `organizationId` and `returnTo` off these responses, so this removes the + * credentials rather than the body. + */ +export function withoutSessionMaterial(data: T) { + const { token: _token, refreshToken: _refreshToken, ...rest } = data; + + return rest; +} + export interface VerifiedUpstreamSession { /** The `sid` claim, when the access token carries one. */ sessionId?: string; diff --git a/packages/core/tests/sessionBodyMaterial.test.js b/packages/core/tests/sessionBodyMaterial.test.js new file mode 100644 index 0000000..103ca93 --- /dev/null +++ b/packages/core/tests/sessionBodyMaterial.test.js @@ -0,0 +1,132 @@ +import { jest } from "@jest/globals"; + +const verifySignedAuthResponseMock = jest.fn(); + +jest.unstable_mockModule("../dist/verifySignedAuthResponse.js", () => ({ + verifySignedAuthResponse: verifySignedAuthResponseMock, +})); + +function createJsonResponse(status, body) { + return { + ok: status >= 200 && status < 300, + status, + json: async () => body, + }; +} + +const UPSTREAM = { + message: "Success", + token: "access-token", + refreshToken: "refresh-token", + sub: "user-123", + roles: ["user"], + email: "person@example.com", + phone: null, + organizationId: "org-123", + returnTo: "https://app.example.com/dashboard", + ttl: 900, + refreshTtl: 3600, +}; + +const OPTS = { + authServerUrl: "https://auth.example.com", + audience: "https://auth.example.com", + accessCookieName: "access", + refreshCookieName: "refresh", +}; + +/** + * Every handler that issues session cookies, and how to call it. + * + * Enumerated in one place so a handler added later is covered by adding a line + * rather than by remembering this invariant exists. + */ +const SESSION_HANDLERS = [ + { + name: "finishLoginHandler", + module: "../dist/handlers/finishLogin.js", + input: { body: {} }, + }, + { + name: "verifyLoginOtpHandler", + module: "../dist/handlers/verifyLoginOtpHandler.js", + input: { body: {}, channel: "email" }, + }, + { + name: "finishOAuthLoginHandler", + module: "../dist/handlers/oauthHandlers.js", + input: { providerId: "google", body: {} }, + }, + { + name: "pollMagicLinkConfirmationHandler", + module: "../dist/handlers/pollMagicLinkConfirmationHandler.js", + input: { body: {} }, + }, + { + name: "switchOrganizationHandler", + module: "../dist/handlers/switchOrganizationHandler.js", + input: { organizationId: "org-123" }, + }, +]; + +describe("session responses do not repeat the credentials in the body", () => { + const originalFetch = global.fetch; + + beforeEach(() => { + global.fetch = jest.fn(); + verifySignedAuthResponseMock.mockReset(); + verifySignedAuthResponseMock.mockResolvedValue({ + sub: "user-123", + sid: "session-123", + }); + }); + + afterEach(() => { + global.fetch = originalFetch; + }); + + // The tokens go out as httpOnly cookies so page scripts cannot read them. + // Returning them in the body of the same response handed them straight back. + it.each(SESSION_HANDLERS)("$name keeps the tokens out of the body", async entry => { + const module = await import(entry.module); + global.fetch.mockResolvedValue(createJsonResponse(200, UPSTREAM)); + + const result = await module[entry.name](entry.input, OPTS); + + expect(result.body).toBeDefined(); + expect(result.body).not.toHaveProperty("token"); + expect(result.body).not.toHaveProperty("refreshToken"); + expect(JSON.stringify(result.body)).not.toContain("access-token"); + expect(JSON.stringify(result.body)).not.toContain("refresh-token"); + }); + + it.each(SESSION_HANDLERS)("$name still carries what callers read", async entry => { + const module = await import(entry.module); + global.fetch.mockResolvedValue(createJsonResponse(200, UPSTREAM)); + + const result = await module[entry.name](entry.input, OPTS); + + expect(result.body).toMatchObject({ + message: "Success", + email: "person@example.com", + organizationId: "org-123", + returnTo: "https://app.example.com/dashboard", + }); + }); + + it.each(SESSION_HANDLERS)("$name still puts the access token in a cookie", async entry => { + const module = await import(entry.module); + global.fetch.mockResolvedValue(createJsonResponse(200, UPSTREAM)); + + const result = await module[entry.name](entry.input, OPTS); + + expect(result.setCookies).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + name: "access", + value: expect.objectContaining({ token: "access-token" }), + }), + ]), + ); + }); +});