From 5e949276d4e4bf546f0fa3e6266cb30b70422aea Mon Sep 17 00:00:00 2001 From: diegosouzapw Date: Fri, 15 May 2026 10:15:57 -0300 Subject: [PATCH] fix(authz/clientApi): fall through to anonymous on invalid bearer when REQUIRE_API_KEY=false (#2257) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CHANGELOG.md | 1 + src/server/authz/policies/clientApi.ts | 13 ++ .../authz/client-api-policy-fallback.test.ts | 168 ++++++++++++++++++ 3 files changed, 182 insertions(+) create mode 100644 tests/unit/authz/client-api-policy-fallback.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d4a0beb1a..ea4cecd0a3 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/server/authz/policies/clientApi.ts b/src/server/authz/policies/clientApi.ts index b41a1d72ff..47022b1919 100644 --- a/src/server/authz/policies/clientApi.ts +++ b/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"); } diff --git a/tests/unit/authz/client-api-policy-fallback.test.ts b/tests/unit/authz/client-api-policy-fallback.test.ts new file mode 100644 index 0000000000..c419a1ae6d --- /dev/null +++ b/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; +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; + } +});