fix(authz/clientApi): fall through to anonymous on invalid bearer when REQUIRE_API_KEY=false (#2257)

There was an asymmetry in the CLIENT_API policy: with REQUIRE_API_KEY
off, a request with no bearer was allowed as anonymous, but a request
with an *invalid* bearer was rejected with 401 "Invalid API key". That
surprises CLI integrations (Codex Desktop auto-config, Hermes Agent)
that ship a stale Bearer in their saved config — they'd see a 401 even
though the operator explicitly opted out of auth.

Fix: when validateApiKey fails and REQUIRE_API_KEY != "true", log a
single warning carrying the masked key id (last-4) and fall through to
anonymous. When REQUIRE_API_KEY is "true", the strict 401 path is
preserved.

The warning preserves observability:
  [clientApiPolicy] invalid bearer presented to /api/v1/responses
    but REQUIRE_API_KEY=false — falling through to anonymous
    (key_id=key_XYZW)

Tests are added in a standalone file because the existing
client-api-policy.test.ts shares a DB-backed setup with a pre-existing
SQLite migration race (5/7 of its tests already failed on baseline).
The new file mocks validateApiKey via a require-resolve interceptor so
the policy's invalid-bearer branch is exercised without touching SQLite.

Reported by @k00shi on the Codex Desktop auto-config path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

CHANGELOG.md

@@ -23,6 +23,7 @@
 - **fix(utils/publicCreds):** `decodePublicCred()` no longer silently mangles raw credential overrides that don't match `RAW_VALUE_PATTERN`.
 - **fix(auth/extractApiKey):** `x-api-key` fallback now only triggers when the request also carries an `anthropic-version` header.
 - **fix(providers/qoder):** the OAuth+PAT disambiguation message now actually surfaces.
+- **fix(authz/clientApi):** when `REQUIRE_API_KEY=false`, an invalid Bearer no longer 401s the whole request — falls through to anonymous (matching the "no auth required" semantics of the flag) with a single warning log carrying the masked key id. Fixes the surprise 401s that hit CLI integrations (Codex Desktop auto-config, Hermes Agent) that ship a stale Bearer in their saved config. (#2257)
 
 ### Fixed
 

src/server/authz/policies/clientApi.ts

@@ -46,6 +46,19 @@ export const clientApiPolicy: RoutePolicy = {
     const { validateApiKey } = await import("../../../lib/db/apiKeys");
     const ok = await validateApiKey(bearer);
     if (!ok) {
+      // Issue #2257: when REQUIRE_API_KEY is off, a stale CLI config (Codex
+      // Desktop auto-config, Hermes, etc.) carrying an invalid Bearer
+      // shouldn't 401 the whole request — REQUIRE_API_KEY=false means
+      // "anonymous traffic is allowed", so an invalid key should degrade to
+      // anonymous instead of rejecting. We log a warning so the bad key is
+      // still observable in the request log.
+      if (process.env.REQUIRE_API_KEY !== "true") {
+        console.warn(
+          `[clientApiPolicy] invalid bearer presented to ${ctx.classification.normalizedPath} ` +
+            `but REQUIRE_API_KEY=false — falling through to anonymous (key_id=${maskKeyId(bearer)})`
+        );
+        return allow({ kind: "anonymous", id: "local" });
+      }
       return reject(401, "AUTH_002", "Invalid API key");
     }
 

tests/unit/authz/client-api-policy-fallback.test.ts

@@ -0,0 +1,168 @@
+/**
+ * Issue #2257 — clientApi policy behavior when an invalid Bearer is sent and
+ * REQUIRE_API_KEY=false.
+ *
+ * The existing `client-api-policy.test.ts` shares a DB-backed setup via
+ * `resetStorage()` and `apiKeysDb` that has SQLite migration races on this
+ * branch. This standalone file mocks `validateApiKey` to test the policy's
+ * fallback branch in isolation — no DB, no migration runner.
+ */
+
+import test from "node:test";
+import assert from "node:assert/strict";
+import Module from "node:module";
+
+// ─── Mock validateApiKey via require interception (so the dynamic import in
+// the policy module returns our stub instead of hitting the real DB module) ─
+
+type ValidateFn = (key: string) => boolean | Promise<boolean>;
+let mockValidateApiKey: ValidateFn = () => false;
+
+const originalResolve = (Module as unknown as { _resolveFilename: typeof Module._resolveFilename })
+  ._resolveFilename;
+
+// Intercept require() / import() resolution for the apiKeys DB module and
+// substitute it for our stub. This runs only for the exact path the policy
+// imports — production code paths are unaffected.
+const POLICY_IMPORT_TARGET = "src/lib/db/apiKeys";
+
+(Module as unknown as { _resolveFilename: typeof Module._resolveFilename })._resolveFilename =
+  function patched(this: unknown, request: string, ...rest: unknown[]) {
+    if (request.includes(POLICY_IMPORT_TARGET)) {
+      // Resolve to a stub file we create below
+      const stubPath = new URL("./__stub_apiKeys.mjs", import.meta.url).pathname;
+      // @ts-expect-error - rest spread to original
+      return originalResolve.call(this, stubPath, ...rest);
+    }
+    // @ts-expect-error - rest spread to original
+    return originalResolve.call(this, request, ...rest);
+  };
+
+// Write the stub file ad-hoc (Node's loader needs a real file)
+import fs from "node:fs";
+import path from "node:path";
+import { fileURLToPath } from "node:url";
+
+const __dirname = path.dirname(fileURLToPath(import.meta.url));
+const STUB_PATH = path.join(__dirname, "__stub_apiKeys.mjs");
+fs.writeFileSync(
+  STUB_PATH,
+  `export const validateApiKey = (key) => globalThis.__mockValidateApiKey(key);\n`
+);
+
+// Wire the stub to our local variable
+(globalThis as unknown as { __mockValidateApiKey: ValidateFn }).__mockValidateApiKey = (key) =>
+  mockValidateApiKey(key);
+
+test.after(() => {
+  try {
+    fs.unlinkSync(STUB_PATH);
+  } catch {
+    /* ignore */
+  }
+});
+
+// ─── Load policy fresh (after the interceptor is in place) ────────────────
+
+async function loadPolicy() {
+  const mod = await import(`../../../src/server/authz/policies/clientApi.ts?ts=${Date.now()}`);
+  return mod.clientApiPolicy;
+}
+
+function ctx(headers: Headers, normalizedPath = "/api/v1/chat/completions") {
+  return {
+    request: { method: "POST", headers, url: `http://localhost${normalizedPath}` },
+    classification: {
+      routeClass: "CLIENT_API" as const,
+      reason: "client_api_v1" as const,
+      normalizedPath,
+    },
+    requestId: "req_test",
+  };
+}
+
+// ─── Tests ────────────────────────────────────────────────────────────────
+
+test.beforeEach(() => {
+  // Default to "every key fails" — individual tests override as needed.
+  mockValidateApiKey = () => false;
+  delete process.env.REQUIRE_API_KEY;
+});
+
+test("#2257 — invalid bearer + REQUIRE_API_KEY=true → 401", async () => {
+  process.env.REQUIRE_API_KEY = "true";
+  const policy = await loadPolicy();
+  const headers = new Headers({ authorization: "Bearer sk-stub-bogus" });
+  const out = await policy.evaluate(ctx(headers));
+  assert.equal(out.allow, false);
+  if (!out.allow) {
+    assert.equal(out.status, 401);
+    assert.equal(out.code, "AUTH_002");
+  }
+});
+
+test("#2257 — invalid bearer + REQUIRE_API_KEY=false → anonymous (with warning log)", async () => {
+  const originalWarn = console.warn;
+  const warnings: string[] = [];
+  console.warn = (msg: string) => warnings.push(String(msg));
+  try {
+    const policy = await loadPolicy();
+    const headers = new Headers({ authorization: "Bearer sk-stub-bogus" });
+    const out = await policy.evaluate(ctx(headers));
+    assert.equal(out.allow, true);
+    if (out.allow) {
+      assert.equal(out.subject.kind, "anonymous");
+      assert.equal(out.subject.id, "local");
+    }
+    assert.ok(
+      warnings.some((w) => w.includes("[clientApiPolicy]") && w.includes("REQUIRE_API_KEY=false")),
+      "expected a warning about the fallback"
+    );
+  } finally {
+    console.warn = originalWarn;
+  }
+});
+
+test("#2257 — fallback warning masks the bearer (only last-4 in log)", async () => {
+  const originalWarn = console.warn;
+  const warnings: string[] = [];
+  console.warn = (msg: string) => warnings.push(String(msg));
+  try {
+    const policy = await loadPolicy();
+    const headers = new Headers({ authorization: "Bearer sk-secretprefix-secretmiddle-XYZW" });
+    const out = await policy.evaluate(ctx(headers));
+    assert.equal(out.allow, true);
+    assert.ok(
+      warnings.every((w) => !w.includes("secretprefix") && !w.includes("secretmiddle")),
+      "warning leaked the full bearer; only masked key id should be logged"
+    );
+    assert.ok(
+      warnings.some((w) => w.includes("key_XYZW")),
+      "expected masked key id (last-4) in the warning"
+    );
+  } finally {
+    console.warn = originalWarn;
+  }
+});
+
+test("#2257 — no bearer + REQUIRE_API_KEY=false → anonymous (unchanged, no fallback warning)", async () => {
+  const originalWarn = console.warn;
+  const warnings: string[] = [];
+  console.warn = (msg: string) => warnings.push(String(msg));
+  try {
+    const policy = await loadPolicy();
+    const out = await policy.evaluate(ctx(new Headers()));
+    assert.equal(out.allow, true);
+    if (out.allow) {
+      assert.equal(out.subject.kind, "anonymous");
+    }
+    // No warning should fire when no bearer is sent in the first place —
+    // the warning is specifically for the "invalid-bearer-fell-through" case.
+    assert.ok(
+      warnings.every((w) => !w.includes("[clientApiPolicy]")),
+      "no fallback warning expected when no bearer was sent"
+    );
+  } finally {
+    console.warn = originalWarn;
+  }
+});