diff --git a/.changeset/logout-partitioned-cookie-clear.md b/.changeset/logout-partitioned-cookie-clear.md new file mode 100644 index 00000000000..03e45b0b65e --- /dev/null +++ b/.changeset/logout-partitioned-cookie-clear.md @@ -0,0 +1,5 @@ +--- +"@agent-native/core": patch +--- + +Clear framework auth cookies from the CHIPS partition they were set in, so logout cannot leave a live session cookie behind, and re-resolve the client session when a request comes back 401 instead of painting a generic load error. diff --git a/packages/core/src/client/use-action.spec.ts b/packages/core/src/client/use-action.spec.ts index 320eac29496..e4cad6bc930 100644 --- a/packages/core/src/client/use-action.spec.ts +++ b/packages/core/src/client/use-action.spec.ts @@ -5,6 +5,11 @@ const analyticsMocks = vi.hoisted(() => ({ })); vi.mock("./analytics.js", () => analyticsMocks); +const sessionMocks = vi.hoisted(() => ({ + recheckSessionAfterUnauthorized: vi.fn(), +})); +vi.mock("./use-session.js", () => sessionMocks); + import { ACTION_KEEPALIVE_BODY_BUDGET_BYTES, actionErrorMessage, @@ -226,6 +231,49 @@ describe("callAction", () => { ); }); + it("re-resolves the session when an action is refused as unauthenticated", async () => { + // 401 means the server stopped recognising this browser. Without telling + // the session gate, the shell stays mounted on its last "authenticated" + // read and this failure surfaces as a generic load error instead of a + // redirect to sign-in - the screen reported after the logout race. + vi.stubGlobal( + "fetch", + vi + .fn() + .mockResolvedValue( + jsonResponse({ error: "Unauthorized" }, { status: 401 }), + ), + ); + + await expect( + callAction("list-designs", {}, { method: "GET" }), + ).rejects.toMatchObject({ status: 401 }); + + expect(sessionMocks.recheckSessionAfterUnauthorized).toHaveBeenCalledTimes( + 1, + ); + }); + + it("leaves the session alone when an action is refused as forbidden", async () => { + // 403 is an authenticated caller being refused one thing. Re-reading the + // session here would be noise, and treating it as signed-out would sign a + // working session out. + vi.stubGlobal( + "fetch", + vi + .fn() + .mockResolvedValue( + jsonResponse({ error: "Forbidden" }, { status: 403 }), + ), + ); + + await expect( + callAction("list-designs", {}, { method: "GET" }), + ).rejects.toMatchObject({ status: 403 }); + + expect(sessionMocks.recheckSessionAfterUnauthorized).not.toHaveBeenCalled(); + }); + it("calls mutating actions through the framework action transport", async () => { const fetchMock = vi .fn() diff --git a/packages/core/src/client/use-action.ts b/packages/core/src/client/use-action.ts index cb2749229cb..a80bbc8fbb6 100644 --- a/packages/core/src/client/use-action.ts +++ b/packages/core/src/client/use-action.ts @@ -43,6 +43,7 @@ import { reloadForClientCompatibilityMismatch, } from "./build-compatibility.js"; import { ensureEmbedAuthFetchInterceptor } from "./embed-auth.js"; +import { recheckSessionAfterUnauthorized } from "./use-session.js"; const ACTION_PREFIX = agentNativePath("/_agent-native/actions"); @@ -493,6 +494,14 @@ async function performActionFetch( } if (!res.ok) { + // The server does not recognise this browser any more. Nothing else + // tells the session gate that, so without this the shell stays mounted + // on a stale authenticated answer and the failure reaches the user as a + // generic load error instead of a redirect to sign-in. 403 is + // deliberately excluded: that is an authenticated caller being refused + // one thing. + if (res.status === 401) recheckSessionAfterUnauthorized(); + // Text the action itself wrote for the caller, as opposed to transport // noise. Only a JSON `error`/`message` qualifies: an HTML error page or a // bare status line is not something a UI should ever put in a toast. diff --git a/packages/core/src/client/use-session.spec.tsx b/packages/core/src/client/use-session.spec.tsx index 80d19f3cf37..9c2595de340 100644 --- a/packages/core/src/client/use-session.spec.tsx +++ b/packages/core/src/client/use-session.spec.tsx @@ -10,7 +10,11 @@ const analyticsMocks = vi.hoisted(() => ({ })); vi.mock("./analytics.js", () => analyticsMocks); -import { notifySessionInvalidated, useSession } from "./use-session.js"; +import { + notifySessionInvalidated, + recheckSessionAfterUnauthorized, + useSession, +} from "./use-session.js"; /** * A fresh copy of the session module. `signingOut` is one-way for the life of a @@ -489,6 +493,89 @@ describe("useSession", () => { expect(container.textContent).toBe("signing-out"); }); + it("re-resolves the session after an authenticated request comes back 401", async () => { + // The half-authenticated state behind the reported logout race: the last + // completed read said "authenticated", so the shell stays mounted and every + // data query paints its own generic load error. Nothing else tells the gate + // the server stopped recognising this browser. + const fetchMock = vi + .fn() + .mockResolvedValueOnce( + jsonResponse({ userId: "user-401", email: "stale@example.com" }), + ) + .mockResolvedValueOnce(jsonResponse({ error: "Not authenticated" })); + vi.stubGlobal("fetch", fetchMock); + + await renderConsumers(["first"]); + expect(container.textContent).toBe("stale@example.com"); + + await act(async () => { + recheckSessionAfterUnauthorized(); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(container.textContent).toBe("signed-out"); + }); + + it("throttles the 401 re-check so one failing screen cannot storm the session endpoint", async () => { + // A listing page fails many queries at once. Each invalidation schedules a + // fresh read, so an unthrottled re-check turns one expired cookie into a + // request per failing query. + const fetchMock = vi.fn(async () => + jsonResponse({ userId: "user-storm", email: "storm@example.com" }), + ); + vi.stubGlobal("fetch", fetchMock); + + await renderConsumers(["first"]); + expect(fetchMock).toHaveBeenCalledTimes(1); + + await act(async () => { + recheckSessionAfterUnauthorized(); + recheckSessionAfterUnauthorized(); + recheckSessionAfterUnauthorized(); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it("ignores a 401 re-check once sign-out has started", async () => { + // Sign-out revokes the session and then navigates, so the 401s it produces + // are expected. Asking again here is exactly how a late reply used to + // resurrect the session the document had already given up. + const { + beginSignOut: begin, + recheckSessionAfterUnauthorized: recheck, + useSession: useFreshSession, + } = await freshSessionModule(); + function Probe() { + return
{useFreshSession().status}
; + } + const fetchMock = vi.fn(async () => + jsonResponse({ userId: "user-out", email: "leaving@example.com" }), + ); + vi.stubGlobal("fetch", fetchMock); + + await act(async () => { + root.render(); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + await act(async () => { + begin(); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + fetchMock.mockClear(); + + await act(async () => { + recheck(); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + expect(fetchMock).not.toHaveBeenCalled(); + expect(container.textContent).toBe("signing-out"); + }); + it("revalidates a cached session when the browser regains focus", async () => { const fetchMock = vi .fn() diff --git a/packages/core/src/client/use-session.ts b/packages/core/src/client/use-session.ts index 59e1528b813..9af9c882701 100644 --- a/packages/core/src/client/use-session.ts +++ b/packages/core/src/client/use-session.ts @@ -161,6 +161,40 @@ export function isSigningOut(): boolean { return signingOut; } +const UNAUTHORIZED_RECHECK_MIN_INTERVAL_MS = 5_000; +let lastUnauthorizedRecheckAt = 0; + +/** + * Re-resolve the session because an authenticated request came back 401. + * + * `status` is only written when a session fetch resolves, so a 401 anywhere + * else leaves every mounted consumer holding the previous `"authenticated"` + * answer. The app shell stays mounted over a session the server no longer + * recognises, and each data query paints its own generic load error instead of + * the visitor being sent to sign in — which is what a stale cookie surviving + * logout looks like on screen. Re-reading the session lets the gate reach the + * truth and redirect. + * + * This asks the server rather than forcing `"unauthenticated"` from here: a + * 401 can also come from one request a live session is not allowed to make, + * and assuming otherwise would sign that visitor out of a working session. + * Throttled because one screen can fail many requests at once, and each + * invalidation schedules a fresh read. + */ +export function recheckSessionAfterUnauthorized(): void { + if (typeof window === "undefined") return; + // Already leaving. The sign-out flow owns the navigation from here, and + // re-reading could only reintroduce the session this document gave up. + if (signingOut) return; + const now = Date.now(); + if (now - lastUnauthorizedRecheckAt < UNAUTHORIZED_RECHECK_MIN_INTERVAL_MS) { + return; + } + lastUnauthorizedRecheckAt = now; + installSessionInvalidationListeners(); + invalidateSessionCache(); +} + /** * Enter the terminal `"signing-out"` state for this document. * diff --git a/packages/core/src/server/auth.spec.ts b/packages/core/src/server/auth.spec.ts index f01b4a71b68..de7fc9d2b0a 100644 --- a/packages/core/src/server/auth.spec.ts +++ b/packages/core/src/server/auth.spec.ts @@ -1435,6 +1435,165 @@ describe("server/auth", () => { ); }); + it("clears the HttpOnly session cookie in the same partition it was set in", async () => { + // CHIPS keeps a `Partitioned` cookie and an unpartitioned cookie of the + // same name in separate jars. `setFrameworkSessionCookie` writes + // `an_session` with `Partitioned` on HTTPS, so a delete without it + // targets the wrong jar: the browser keeps sending the session token + // after logout, and any instance whose session-email cache still holds + // that token answers "authenticated" as the previous account. + // + // The looser `toContain("Partitioned")` assertion above passes on the + // non-HttpOnly hint cookie alone, so it never inspected the cookie that + // actually carries the session. + vi.stubEnv("NODE_ENV", "production"); + vi.stubEnv("COOKIE_DOMAIN", ".example.com"); + delete process.env.ACCESS_TOKEN; + delete process.env.ACCESS_TOKENS; + + vi.doMock("./better-auth-instance.js", () => ({ + getBetterAuth: vi.fn(async () => ({ + handler: vi.fn(async () => new Response("{}")), + api: { + getSession: vi.fn(async () => null), + signInEmail: vi.fn(), + signUpEmail: vi.fn(), + signOut: vi.fn(async () => ({ headers: new Headers() })), + }, + })), + getBetterAuthSync: vi.fn(() => undefined), + })); + vi.doMock("../db/client.js", () => ({ + getDbExec: () => ({ execute: vi.fn(async () => ({ rows: [] })) }), + isLocalDatabase: () => true, + retryOnDdlRace: (fn: () => Promise) => fn(), + describeDbError: (error: unknown) => String(error), + })); + + const { autoMountAuth, COOKIE_NAME } = await import("./auth.js"); + const app = createMockApp(); + await autoMountAuth(app); + + const logoutHandler = app.use.mock.calls.find( + (call: any[]) => call[0] === "/_agent-native/auth/logout", + )?.[1]; + const event = createJsonPostEvent( + "/_agent-native/auth/logout", + {}, + { + "x-forwarded-proto": "https", + cookie: `${COOKIE_NAME}=session-token-abc`, + }, + ); + + await logoutHandler(event); + + const clears = (event.res.headers.get("set-cookie") ?? "") + .split(/,\s*(?=[^;,\s]+=)/) + .map((cookie: string) => cookie.trim()) + .filter((cookie: string) => + cookie.startsWith(`${COOKIE_NAME}=; Max-Age=0`), + ); + + // Both jars, in both domain scopes. The partitioned delete is what + // logout was missing; the unpartitioned one still has to go out for a + // cookie stored before CHIPS or over plain HTTP on a host later served + // over HTTPS. h3's set-cookie dedupe ignores `Partitioned`, so these two + // only coexist because the helper works around it. + expect(new Set(clears)).toEqual( + new Set([ + `${COOKIE_NAME}=; Max-Age=0; Path=/; Secure; Partitioned; SameSite=None`, + `${COOKIE_NAME}=; Max-Age=0; Path=/; Secure; SameSite=None`, + `${COOKIE_NAME}=; Max-Age=0; Domain=.example.com; Path=/; Secure; Partitioned; SameSite=None`, + `${COOKIE_NAME}=; Max-Age=0; Domain=.example.com; Path=/; Secure; SameSite=None`, + ]), + ); + }); + + it("leaves the new token as the last word when a session replaces an old one", async () => { + // setFrameworkSessionCookie clears before it sets, and clearing now + // emits a delete per CHIPS jar. h3 already lets a domain-scoped delete + // survive alongside the set (it does on `main` too), which is harmless + // only because a browser applies Set-Cookie in order. So the invariant + // is not "one header" — it is that nothing after the set takes the + // session back off. A stray trailing delete would log the user out on + // the very request that signed them in. + vi.stubEnv("NODE_ENV", "production"); + vi.stubEnv("COOKIE_DOMAIN", ".example.com"); + + vi.doMock("../db/client.js", () => ({ + getDbExec: () => ({ execute: vi.fn(async () => ({ rows: [] })) }), + isLocalDatabase: () => true, + retryOnDdlRace: (fn: () => Promise) => fn(), + describeDbError: (error: unknown) => String(error), + })); + + const { setFrameworkSessionCookie, COOKIE_NAME } = + await import("./auth.js"); + const event = createMockEvent({ + headers: { "x-forwarded-proto": "https" }, + }); + + setFrameworkSessionCookie(event, "fresh-token"); + + const sessionCookies = event.res.headers + .getSetCookie() + .filter((cookie: string) => cookie.startsWith(`${COOKIE_NAME}=`)); + + expect(sessionCookies.at(-1)).toContain(`${COOKIE_NAME}=fresh-token`); + expect(sessionCookies.at(-1)).toContain("Partitioned"); + // And the clear does not emit the same header twice. + expect(new Set(sessionCookies).size).toBe(sessionCookies.length); + }); + + it("keeps logout cookie clears unpartitioned over plain HTTP", async () => { + // `Partitioned` requires `Secure`; emitting it on a plain-HTTP dev + // origin would make the serializer throw and take the whole logout + // response down. + vi.stubEnv("NODE_ENV", "production"); + delete process.env.COOKIE_DOMAIN; + delete process.env.ACCESS_TOKEN; + delete process.env.ACCESS_TOKENS; + + vi.doMock("./better-auth-instance.js", () => ({ + getBetterAuth: vi.fn(async () => ({ + handler: vi.fn(async () => new Response("{}")), + api: { + getSession: vi.fn(async () => null), + signInEmail: vi.fn(), + signUpEmail: vi.fn(), + signOut: vi.fn(async () => ({ headers: new Headers() })), + }, + })), + getBetterAuthSync: vi.fn(() => undefined), + })); + vi.doMock("../db/client.js", () => ({ + getDbExec: () => ({ execute: vi.fn(async () => ({ rows: [] })) }), + isLocalDatabase: () => true, + retryOnDdlRace: (fn: () => Promise) => fn(), + describeDbError: (error: unknown) => String(error), + })); + + const { autoMountAuth, COOKIE_NAME } = await import("./auth.js"); + const app = createMockApp(); + await autoMountAuth(app); + + const logoutHandler = app.use.mock.calls.find( + (call: any[]) => call[0] === "/_agent-native/auth/logout", + )?.[1]; + const event = createJsonPostEvent( + "/_agent-native/auth/logout", + {}, + { cookie: `${COOKIE_NAME}=session-token-abc` }, + ); + + await expect(logoutHandler(event)).resolves.toEqual({ ok: true }); + + const setCookie = event.res.headers.get("set-cookie") ?? ""; + expect(setCookie).toContain(`${COOKIE_NAME}=; Max-Age=0; Path=/`); + expect(setCookie).not.toContain("Partitioned"); + }); + it("revokes the Better Auth session row directly so logout can't be resurrected by the legacy-cookie fallback", async () => { // Reproduces the reported bug: a token whose legacy `sessions` row was // never written (the magic-link `addSession` mirror is best-effort — diff --git a/packages/core/src/server/auth.ts b/packages/core/src/server/auth.ts index a06150ae83e..1e63bd745bd 100644 --- a/packages/core/src/server/auth.ts +++ b/packages/core/src/server/auth.ts @@ -578,26 +578,81 @@ async function enrichLegacySessionIdentity( }; } +/** + * Delete one framework auth cookie from every jar this app could have written + * it into. + * + * A cookie's identity is name + domain + path + partition key, and a delete + * only removes an exact match, so two axes have to be swept: + * + * - **Domain**: a host-only cookie and a `Domain=` cookie of the same name are + * separate entries, so a stale shared-domain cookie keeps shadowing the + * isolated app session. + * - **Partition**: under CHIPS a `Partitioned` cookie lives in a jar keyed by + * the top-level site, entirely separate from the unpartitioned cookie of the + * same name. Framework auth cookies are written through + * `crossSiteCookieAttrs`, which sets `Partitioned` on HTTPS, so a delete + * without it empties the wrong jar and the browser keeps sending a revoked + * session token. The reverse misses too: a cookie stored before CHIPS, or + * over plain HTTP on a host later served over HTTPS, sits unpartitioned and + * survives a `Partitioned`-only delete. + * + * `crossSiteCookieAttrs` is applied here rather than left to callers because a + * mismatched delete fails silently: the logout response still looks + * successful, and the surviving cookie only surfaces later as the previous + * account coming back for as long as any instance's session-email cache still + * resolves the revoked token. + */ function deleteCookieFromEveryScope( event: H3Event, name: string, attributes: Parameters[2] = {}, ): void { + const scoped = { ...crossSiteCookieAttrs(event), ...attributes, path: "/" }; // Clear host-only cookies first. Then clear any configured domain scope so // stale shared cookies stop shadowing isolated app sessions. - deleteCookie(event, name, { ...attributes, path: "/" }); + deleteCookieFromBothPartitions(event, name, scoped); for (const domain of AUTH_COOKIE_NAMESPACE.frameworkCookieDomainsToClear) { - deleteCookie(event, name, { ...attributes, path: "/", domain }); + deleteCookieFromBothPartitions(event, name, { ...scoped, domain }); + } +} + +/** + * Emit the partitioned AND unpartitioned delete for one name/domain/path. + * + * h3 dedupes `set-cookie` on name/domain/path and ignores `Partitioned`, so + * two `deleteCookie` calls that differ only by partition can collapse into + * one. It is not consistent about it — the eviction fires for a host-only + * cookie and misses when a `Domain` is present, because the scan side of the + * dedupe recovers the key from a re-parsed header rather than from the + * options — so this cannot rely on either outcome. Let h3 serialize the + * unpartitioned delete (cookie-es validates the name, the domain, and the + * `Partitioned`-requires-`Secure` pairing), then put it back only if the + * partitioned delete actually evicted it. + */ +function deleteCookieFromBothPartitions( + event: H3Event, + name: string, + scope: Parameters[2], +): void { + if (!scope?.partitioned) { + deleteCookie(event, name, scope); + return; + } + deleteCookie(event, name, { ...scope, partitioned: false }); + const unpartitioned = event.res.headers.getSetCookie().at(-1); + deleteCookie(event, name, scope); + if ( + unpartitioned && + !event.res.headers.getSetCookie().includes(unpartitioned) + ) { + event.res.headers.append("set-cookie", unpartitioned); } } export function clearFrameworkSessionHintCookies(event: H3Event): void { for (const name of frameworkSessionCookieNamesToClear()) { - deleteCookieFromEveryScope( - event, - frameworkSessionHintCookieName(name), - crossSiteCookieAttrs(event), - ); + deleteCookieFromEveryScope(event, frameworkSessionHintCookieName(name)); } }