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(); }); }); 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.