fix(core): answer 401, not 400, when nobody is signed in

.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.

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}"`,
       };
     }

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 () => {

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

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.