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" }), + }), + ]), + ); + }); +});