From b52c9cdb2cdbe40d2368e6ce4e0097c86ee89c8a Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Sun, 30 Aug 2026 17:36:19 -0400 Subject: [PATCH 1/2] fix(core): answer 401, not 400, when nobody is signed in ensureCookies gates every access-required route. When the required cookie was absent and there was no refresh cookie to fall back on, which is exactly the signed-out case, it answered 400. The request was perfectly well formed. There was simply no session, and that is what 401 means. Two things went wrong with the old status. Every signed-out page view produced 400s, so 400 became ordinary background traffic in adopter logs and would hide a real malformed request from anyone watching. And consumers translate status codes into words for readers: 400 asks a product to say "that request did not come through in a form we could use" when the true sentence is "you have been signed out, sign in again", and there was no way to tell the two apart from outside. The neighbouring branches in the same function already answered 401 for a refresh that failed and for a cookie that was invalid or expired, so this branch was the outlier rather than the convention. All three now agree, and the status no longer depends on which way the session happened to be absent. The response body is unchanged. Two tests pinned the old status, both with a comment explaining the 400 as intended behaviour. Both now pin 401, and the express one keeps its real assertion that the request is never forwarded upstream. Closes #148 --- .changeset/eager-owls-signal.md | 27 +++++++++++++++ packages/core/src/ensureCookies.ts | 6 +++- packages/core/tests/ensureCookies.test.js | 34 +++++++++++++++++-- .../express/tests/oauthProvidersProxy.test.js | 6 ++-- 4 files changed, 67 insertions(+), 6 deletions(-) create mode 100644 .changeset/eager-owls-signal.md diff --git a/.changeset/eager-owls-signal.md b/.changeset/eager-owls-signal.md new file mode 100644 index 0000000..24dc516 --- /dev/null +++ b/.changeset/eager-owls-signal.md @@ -0,0 +1,27 @@ +--- +'@seamless-auth/core': minor +--- + +Answer 401, not 400, when nobody is signed in. + +`ensureCookies` gates every access-required route. When the required cookie was +absent and there was no refresh cookie to fall back on, which is exactly the +signed-out case, it answered `400`. The request was perfectly well formed. There +was simply no session, and that is what `401` means. + +This is a behaviour change for adopters. Anything branching on `400` from an +`/auth` route to detect a malformed request will now see `401` for a signed-out +visitor instead. + +Two things went wrong with the old status. Every signed-out page view produced +`400`s, so `400` became ordinary background traffic in adopter logs and would +hide a real malformed request from anyone watching. And consumers translate +status codes into words for readers: `400` asks a product to say "that request +did not come through in a form we could use" when the true sentence is "you have +been signed out, sign in again". + +The neighbouring branches in the same function already answered `401` for a +refresh that failed and for a cookie that was invalid or expired, so this branch +was the outlier rather than the convention. All three now agree, and the status +no longer depends on which way the session happened to be absent. The response +body is unchanged. diff --git a/packages/core/src/ensureCookies.ts b/packages/core/src/ensureCookies.ts index 0dddd85..56f227e 100644 --- a/packages/core/src/ensureCookies.ts +++ b/packages/core/src/ensureCookies.ts @@ -309,10 +309,14 @@ export async function ensureCookies( opts, ); + // 401, not 400: the request is well formed, there is simply nobody signed + // in. Answering 400 made every signed-out page view look like a malformed + // request in the adopter's logs, and left consumers unable to tell "sign in + // again" from "that request was not understood". if (!refreshed) { return { type: "error", - status: 400, + status: 401, errorCode: `Missing required cookie "${cookieName}"`, }; } diff --git a/packages/core/tests/ensureCookies.test.js b/packages/core/tests/ensureCookies.test.js index 3d97a77..060769e 100644 --- a/packages/core/tests/ensureCookies.test.js +++ b/packages/core/tests/ensureCookies.test.js @@ -67,7 +67,9 @@ describe("ensureCookies", () => { }); }); - it("returns error when required cookie missing and no refresh cookie", async () => { + // Nobody is signed in, which is 401. The request itself is perfectly well + // formed, so 400 both misreported it and buried real 400s in adopter logs. + it("answers 401 when required cookie missing and no refresh cookie", async () => { const { ensureCookies } = await import("../dist/ensureCookies.js"); const result = await ensureCookies( @@ -79,7 +81,35 @@ describe("ensureCookies", () => { ); expect(result.type).toBe("error"); - expect(result.status).toBe(400); + expect(result.status).toBe(401); + expect(result.errorCode).toBe('Missing required cookie "access"'); + }); + + it("answers 401 for a pre-auth route reached with no cookies", async () => { + const { ensureCookies } = await import("../dist/ensureCookies.js"); + + const result = await ensureCookies( + { path: "/webAuthn/login/start", cookies: {} }, + BASE_OPTS, + ); + + expect(result.status).toBe(401); + }); + + // The neighbouring signed-out branches already answered 401. This pins that + // all three now agree, so the status no longer depends on which way the + // session happened to be absent. + it("answers 401 when the refresh cookie is present but refreshing fails", async () => { + const { ensureCookies } = await import("../dist/ensureCookies.js"); + + refreshAccessTokenMock.mockResolvedValue(null); + + const result = await ensureCookies( + { path: "/users/me", cookies: { refresh: "refresh.jwt" } }, + BASE_OPTS, + ); + + expect(result.status).toBe(401); }); it("refreshes session when required cookie missing but refresh cookie exists", async () => { diff --git a/packages/express/tests/oauthProvidersProxy.test.js b/packages/express/tests/oauthProvidersProxy.test.js index 88bcde7..7b50946 100644 --- a/packages/express/tests/oauthProvidersProxy.test.js +++ b/packages/express/tests/oauthProvidersProxy.test.js @@ -154,9 +154,9 @@ describe("oauth provider admin proxy routes", () => { ); // ensureCookies gates access-required routes before the handler: with no - // access or refresh cookie present it returns a 400 "missing cookie" rather - // than forwarding upstream. - expect(res.status).toBe(400); + // access or refresh cookie present it answers 401 rather than forwarding + // upstream. + expect(res.status).toBe(401); expect(global.fetch).not.toHaveBeenCalled(); }); }); From e27441174aaed77b87a9c4311047192bd42ce306 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Sun, 30 Aug 2026 17:46:03 -0400 Subject: [PATCH 2/2] test(fastify): pin the signed-out status on both adapters, not just express The parity suite already drove a session-less request through both adapters, but it only asserted that the two agree, not what they agree on, so it passed just as happily when both answered 400. The status itself is what a consumer reads to tell "sign in again" from "that request was not understood", so it is now pinned by value on an access-gated route and a pre-auth gated one, along with the fact that neither adapter asks upstream anything. Checked against the previous behaviour: both new cases fail with 400 received where 401 was expected, so this catches the regression rather than merely restating the fix. --- packages/fastify/tests/parity.test.js | 29 +++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/packages/fastify/tests/parity.test.js b/packages/fastify/tests/parity.test.js index d772f3e..81c8418 100644 --- a/packages/fastify/tests/parity.test.js +++ b/packages/fastify/tests/parity.test.js @@ -263,6 +263,35 @@ describe("fastify and express adapters agree", () => { expect(fastify.cookies).toEqual(expressResult.cookies); }); + // The parity case above proves the two adapters agree on this, not what they + // agree on, so it passed just as happily when both answered 400. The status + // itself is the contract a consumer reads to tell "sign in again" from "that + // request was not understood", so it is pinned here by value. + it.each([ + ["access-gated", { method: "get", path: "/organizations" }], + [ + "pre-auth gated", + { method: "post", path: "/webAuthn/login/start", payload: {} }, + ], + ])("answers 401 on a %s route with no session, and asks upstream nothing", async ( + _label, + scenario, + ) => { + const upstreamResponse = upstream(200, {}); + + global.fetch = jest.fn(async () => upstreamResponse); + const fastifyResult = await viaFastify(scenario); + const fastifyCalls = global.fetch.mock.calls.length; + + global.fetch = jest.fn(async () => upstreamResponse); + const expressResult = await viaExpress(scenario); + + expect(fastifyResult.status).toBe(401); + expect(expressResult.status).toBe(401); + expect(fastifyCalls).toBe(0); + expect(global.fetch.mock.calls.length).toBe(0); + }); + // The sign-in screens call this with no session at all. Forwarding an identity // would be pointless on a route upstream serves publicly, and it would put a // stale cookie in the path of the one call a signed-out client has to make.