fix(auth): paginate session maintenance safely

2026-08-17 09:05:20 +02:00
parent 3e7bb11313
commit c86f01e886
12 changed files with 1021 additions and 79 deletions
@@ -12,6 +12,7 @@ const state = "s".repeat(43);
 const nonce = "n".repeat(43);
 const verifier = "v".repeat(43);
 const transactionCookieName = "__Host-thothii_oidc_tx";
+const loopbackTransactionCookieName = "thothii_oidc_tx";
 const createdApps: Array<ReturnType<typeof Fastify>> = [];

 function setCookieHeaders(response: { headers: Record<string, string | string[] | undefined> }): string[] {
@@ -19,15 +20,42 @@ function setCookieHeaders(response: { headers: Record<string, string | string[]
  return header === undefined ? [] : Array.isArray(header) ? header : [header];
 }

-function transactionCookie(response: { headers: Record<string, string | string[] | undefined> }): string | undefined {
+function transactionCookie(
+  response: { headers: Record<string, string | string[] | undefined> },
+  name = transactionCookieName,
+): string | undefined {
  return setCookieHeaders(response)
-    .find((header) => header.startsWith(`${transactionCookieName}=`))
+    .find((header) => header.startsWith(`${name}=`) && !header.includes("Max-Age=0"))
    ?.split(";", 1)[0];
 }

-function expectTransactionCleared(response: { headers: Record<string, string | string[] | undefined> }): void {
-  expect(setCookieHeaders(response).some((header) =>
-    header.startsWith(`${transactionCookieName}=`) && header.includes("Max-Age=0"))).toBe(true);
+function expectTransactionCleared(
+  response: { headers: Record<string, string | string[] | undefined> },
+  name = transactionCookieName,
+  secure = true,
+): void {
+  const header = setCookieHeaders(response).find((candidate) =>
+    candidate.startsWith(`${name}=`) && candidate.includes("Max-Age=0"));
+  expect(header).toBeDefined();
+  expect(header).toContain("HttpOnly");
+  expect(header).toContain("SameSite=Lax");
+  expect(header).toContain("Path=/");
+  expect(header).not.toContain("Domain=");
+  if (secure) expect(header).toContain("Secure");
+  else expect(header).not.toContain("Secure");
+}
+
+function expectBothTransactionVariantsCleared(
+  response: { headers: Record<string, string | string[] | undefined> },
+  activeName: string,
+  activeSecure: boolean,
+): void {
+  expectTransactionCleared(response, activeName, activeSecure);
+  expectTransactionCleared(
+    response,
+    activeName === transactionCookieName ? loopbackTransactionCookieName : transactionCookieName,
+    activeName !== transactionCookieName,
+  );
 }

 function config(overrides: Partial<LoadedAuthConfig["value"]> = {}): LoadedAuthConfig {
@@ -58,6 +86,7 @@ function stateRecord(extra: Partial<OidcStateRecord> = {}): OidcStateRecord {
    version: 1, nonce, codeVerifier: verifier, returnTo: "/",
    authConfigRevision: revision, issuer,
    browserTransactionDigest: "b".repeat(64),
+    browserTransactionTransport: "https",
    createdAt: "2030-01-01T00:00:00.000Z", expiresAt: "2030-01-01T00:10:00.000Z",
    ...extra,
  } as OidcStateRecord;
@@ -106,6 +135,8 @@ function fixture(options: {
      });
      (record as OidcStateRecord & { browserTransactionDigest: string }).browserTransactionDigest =
        input.browserTransactionDigest as string;
+      (record as OidcStateRecord & { browserTransactionTransport?: string }).browserTransactionTransport =
+        input.browserTransactionTransport as string | undefined ?? "https";
      if (options.stateReturnTo) (record as { returnTo: string }).returnTo = options.stateReturnTo;
      storedState = record;
      return { state, record: storedState };
@@ -149,7 +180,10 @@ afterEach(async () => {

 async function beginOidcLogin(subject: ReturnType<typeof fixture>) {
  const response = await subject.app.inject({ method: "GET", url: "/auth/oidc/login" });
-  return { response, cookie: transactionCookie(response) };
+  return {
+    response,
+    cookie: transactionCookie(response) ?? transactionCookie(response, loopbackTransactionCookieName),
+  };
 }

 async function finishOidcLogin(
@@ -208,12 +242,16 @@ test("creates digest-only bound state, maps exact groups, creates a cookie sessi
  expect(subject.stateInputs[0]).toMatchObject({
    returnTo: "/", authConfigRevision: revision, issuer,
    browserTransactionDigest: expect.stringMatching(/^[a-f0-9]{64}$/),
+    browserTransactionTransport: "https",
  });
  expect(cookie).toMatch(new RegExp(`^${transactionCookieName}=[A-Za-z0-9_-]{43}$`));
-  expect(setCookieHeaders(start).join("\n")).toContain("HttpOnly");
-  expect(setCookieHeaders(start).join("\n")).toContain("SameSite=Lax");
-  expect(setCookieHeaders(start).join("\n")).toContain("Secure");
-  expect(setCookieHeaders(start).join("\n")).toContain("Path=/");
+  const issued = setCookieHeaders(start)
+    .find((header) => header.startsWith(`${transactionCookieName}=`) && !header.includes("Max-Age=0"));
+  expect(issued).toContain("HttpOnly");
+  expect(issued).toContain("SameSite=Lax");
+  expect(issued).toContain("Secure");
+  expect(issued).toContain("Path=/");
+  expect(issued).not.toContain("Domain=");
  expect(JSON.stringify(subject.stateInputs)).not.toContain(cookie?.split("=", 2)[1] ?? "missing-cookie");

  const callback = await subject.app.inject({
@@ -226,7 +264,7 @@ test("creates digest-only bound state, maps exact groups, creates a cookie sessi
  expect(setCookieHeaders(callback).join("\n")).toContain("HttpOnly");
  expect(setCookieHeaders(callback).join("\n")).toContain("SameSite=Lax");
  expect(setCookieHeaders(callback).join("\n")).toContain("Secure");
-  expectTransactionCleared(callback);
+  expectBothTransactionVariantsCleared(callback, transactionCookieName, true);
  expect(subject.callbacks[0]?.href).toBe(`https://thothii.example.test/api/auth/oidc/callback?code=good&state=${state}`);
  expect(subject.creates).toHaveLength(1);
  expect(subject.creates[0]).toMatchObject({
@@ -242,12 +280,103 @@ test("creates digest-only bound state, maps exact groups, creates a cookie sessi
  expect(JSON.stringify(subject.creates)).not.toContain("refresh-token-must-not-leak");
 });

+test.each([
+  "http://127.42.0.1:8787",
+  "http://[::1]:8787",
+])("uses the non-prefixed, non-Secure transaction cookie for literal loopback OIDC %s", async (publicUrl) => {
+  const subject = fixture({ loaded: config({ publicUrl }) });
+  const { response: start, cookie } = await beginOidcLogin(subject);
+
+  expect(start.statusCode).toBe(302);
+  expect(subject.stateInputs[0]).toMatchObject({ browserTransactionTransport: "loopback_http" });
+  expect(cookie).toMatch(new RegExp(`^${loopbackTransactionCookieName}=[A-Za-z0-9_-]{43}$`));
+  const issued = setCookieHeaders(start).find((header) => header.startsWith(`${loopbackTransactionCookieName}=`));
+  expect(issued).toContain("HttpOnly");
+  expect(issued).toContain("SameSite=Lax");
+  expect(issued).toContain("Path=/");
+  expect(issued).not.toContain("Secure");
+  expect(issued).not.toContain("Domain=");
+
+  const callback = await finishOidcLogin(subject, cookie);
+  expect(callback.statusCode).toBe(302);
+  expect(subject.callbacks[0]?.href).toBe(`${publicUrl}/api/auth/oidc/callback?code=good&state=${state}`);
+  expectBothTransactionVariantsCleared(callback, loopbackTransactionCookieName, false);
+});
+
+test("uses the consumed state transport to reject cross-mode and shadowed transaction cookies", async () => {
+  const subject = fixture({ loaded: config({ publicUrl: "http://127.0.0.1:8787" }) });
+  const { cookie } = await beginOidcLogin(subject);
+  expect(cookie).toBeDefined();
+
+  const failed = await finishOidcLogin(subject, [
+    cookie,
+    `${transactionCookieName}=${"x".repeat(43)}`,
+  ].join("; "));
+
+  expect(failed.statusCode).toBe(401);
+  expect(subject.callbacks).toEqual([]);
+  expectBothTransactionVariantsCleared(failed, loopbackTransactionCookieName, false);
+});
+
+test("rejects a transaction presented only under the wrong transport cookie name", async () => {
+  const subject = fixture();
+  const { cookie } = await beginOidcLogin(subject);
+  const wrongTransportCookie = cookie?.replace(transactionCookieName, loopbackTransactionCookieName);
+
+  const failed = await finishOidcLogin(subject, wrongTransportCookie);
+
+  expect(failed.statusCode).toBe(401);
+  expect(subject.callbacks).toEqual([]);
+  expectBothTransactionVariantsCleared(failed, transactionCookieName, true);
+});
+
+test("rejects duplicate same-mode transaction cookies instead of trusting a parser-selected value", async () => {
+  const subject = fixture();
+  const { cookie } = await beginOidcLogin(subject);
+  expect(cookie).toBeDefined();
+
+  const failed = await finishOidcLogin(subject, [
+    cookie,
+    `${transactionCookieName}=${"x".repeat(43)}`,
+  ].join("; "));
+
+  expect(failed.statusCode).toBe(401);
+  expect(subject.callbacks).toEqual([]);
+  expect(subject.stateWasConsumed()).toBe(true);
+  expectBothTransactionVariantsCleared(failed, transactionCookieName, true);
+});
+
+test("pins the callback transaction transport to its captured configuration snapshot", async () => {
+  const subject = fixture({ loaded: config({ publicUrl: "http://127.42.0.1:8787" }) });
+  const { cookie } = await beginOidcLogin(subject);
+  subject.setConfig(config({ publicUrl: "https://thothii.example.test" }));
+
+  const failed = await finishOidcLogin(subject, cookie);
+
+  expect(failed.statusCode).toBe(401);
+  expect(subject.callbacks).toEqual([]);
+  expect(subject.stateWasConsumed()).toBe(true);
+  expectBothTransactionVariantsCleared(failed, loopbackTransactionCookieName, false);
+});
+
+test("clears the state-pinned loopback transaction variant after a terminal callback failure", async () => {
+  const subject = fixture({
+    loaded: config({ publicUrl: "http://127.0.0.1:8787" }),
+    callbackFailure: true,
+  });
+  const { cookie } = await beginOidcLogin(subject);
+  const failed = await finishOidcLogin(subject, cookie);
+
+  expect(failed.statusCode).toBe(401);
+  expectBothTransactionVariantsCleared(failed, loopbackTransactionCookieName, false);
+});
+
 test("consumes state on callback failure and refuses replay", async () => {
  const subject = fixture({ callbackFailure: true });
  const { cookie } = await beginOidcLogin(subject);
  const failed = await finishOidcLogin(subject, cookie);
  expect(failed.statusCode).toBe(401);
-  expectTransactionCleared(failed);
+  expectBothTransactionVariantsCleared(failed, transactionCookieName, true);
  const replay = await finishOidcLogin(subject, cookie);
  expect(replay.statusCode).toBe(401);
  expect(subject.callbacks).toHaveLength(1);
@@ -260,6 +389,7 @@ test("consumes state when the protocol becomes unavailable before callback", asy
  const failed = await finishOidcLogin(subject, cookie);
  expect(failed.statusCode).toBe(401);
  expect(subject.stateWasConsumed()).toBe(true);
+  expectBothTransactionVariantsCleared(failed, transactionCookieName, true);
 });

 test("rejects a consumed state with a non-root return target", async () => {
@@ -269,6 +399,7 @@ test("rejects a consumed state with a non-root return target", async () => {
  expect(callback.statusCode).toBe(401);
  expect(subject.callbacks).toEqual([]);
  expect(subject.creates).toEqual([]);
+  expectBothTransactionVariantsCleared(callback, transactionCookieName, true);
 });

 test("rejects an OIDC state when its configuration revision changes before callback", async () => {
@@ -278,6 +409,7 @@ test("rejects an OIDC state when its configuration revision changes before callb
  const callback = await finishOidcLogin(subject, cookie);
  expect(callback.statusCode).toBe(401);
  expect(subject.callbacks).toEqual([]);
+  expectBothTransactionVariantsCleared(callback, transactionCookieName, true);
 });

 test("rejects an OIDC state when its issuer changes before callback", async () => {
@@ -310,7 +442,7 @@ test("rejects a callback from a different browser and clears the transaction coo

  const failed = await finishOidcLogin(subject, `${transactionCookieName}=${"x".repeat(43)}`);
  expect(failed.statusCode).toBe(401);
-  expectTransactionCleared(failed);
+  expectBothTransactionVariantsCleared(failed, transactionCookieName, true);
  expect(subject.stateWasConsumed()).toBe(true);
  expect(subject.creates).toEqual([]);

@@ -328,7 +460,7 @@ test.each([
  const { cookie } = await beginOidcLogin(subject);
  const failed = await finishOidcLogin(subject, cookie, query);
  expect(failed.statusCode).toBe(401);
-  expectTransactionCleared(failed);
+  expectBothTransactionVariantsCleared(failed, transactionCookieName, true);
  expect(subject.stateWasConsumed()).toBe(true);
  expect(subject.callbacks).toEqual([]);

@@ -345,7 +477,7 @@ test("boundedly burns a canonical state from an oversized callback URL", async (
  const failed = await finishOidcLogin(subject, cookie, oversized);

  expect(failed.statusCode).toBe(401);
-  expectTransactionCleared(failed);
+  expectBothTransactionVariantsCleared(failed, transactionCookieName, true);
  expect(subject.stateWasConsumed()).toBe(true);
  expect(subject.callbacks).toEqual([]);
  expect((await finishOidcLogin(subject, cookie)).statusCode).toBe(401);
@@ -358,11 +490,19 @@ test("rejects an empty direct groups claim without creating a session cookie", a
  const { cookie } = await beginOidcLogin(subject);
  const callback = await finishOidcLogin(subject, cookie);
  expect(callback.statusCode).toBe(401);
-  expectTransactionCleared(callback);
+  expectBothTransactionVariantsCleared(callback, transactionCookieName, true);
  expect(subject.creates).toEqual([]);
  expect(setCookieHeaders(callback).join("\n")).not.toContain("opaque-session-token");
 });

+test("clears both transaction variants when the callback state cannot be consumed", async () => {
+  const subject = fixture();
+  const failed = await finishOidcLogin(subject, `${transactionCookieName}=${"x".repeat(43)}`);
+
+  expect(failed.statusCode).toBe(401);
+  expectBothTransactionVariantsCleared(failed, transactionCookieName, true);
+});
+
 test("uses one captured instant for token TTL derivation and session creation", async () => {
  const tokenExpiresAt = new Date(Date.now() + 120_000);
  const subject = fixture({ identity: {