From 262c4e9046704a7e20f7b476bc15840baddb7d52 Mon Sep 17 00:00:00 2001 From: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:08:52 -0300 Subject: [PATCH] fix(security): container/Fly REQUIRE_API_KEY posture + free-tier usage leak (#13679) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR E of the #13679 insecure-defaults umbrella (items #6, #7; item #8 analyzed as by-design, no change). The published Docker image and fly.toml shipped without REQUIRE_API_KEY set, so a bare `docker run` (README/QUICK-START one-liners, no --env-file) or a `fly deploy` combined "keyless" with "world-reachable" for the anonymous /v1 LLM proxy. docker-compose.yml already mitigates this via loopback-only binding (#12568) and correctly keeps following the operator's own .env, so it is untouched. The npm/CLI local-first REQUIRE_API_KEY=false default in featureFlagDefinitions.ts is also untouched per the owner's decision. /api/free-tier/summary ships an unconditional Access-Control-Allow-Origin: "*" and always included the operator's own local usedThisMonth/remaining usage regardless of auth — a low-severity info leak to any reachable origin. Both fields are now withheld from unauthenticated callers while the intentionally public catalog data stays served to everyone. The gemini-SSE (openai-to-gemini-sse.ts) sub-finding needed no code change: /v1beta/models/*:streamGenerateContent is already classified CLIENT_API and fronted by clientApiPolicy through src/proxy.ts before the translator ever runs, and its CORS-header echo was already hardened fail-closed by #12573. REQUIRE_API_KEY=true (this PR's container/Fly default) closes the dependency that finding cited. Added a locking regression test confirming this chain. Regression tests: - tests/unit/issue-13679-container-posture-require-api-key.test.ts - tests/unit/issue-13679-free-tier-summary-usage-leak.test.ts - tests/unit/issue-13679-gemini-sse-requires-api-key.test.ts (confirmation) Refs #13679 --- Dockerfile | 12 +++ .../fixes/13679-container-posture-cors.md | 1 + fly.toml | 10 ++- src/app/api/free-tier/summary/route.ts | 17 ++++- ...-container-posture-require-api-key.test.ts | 60 +++++++++++++++ ...13679-free-tier-summary-usage-leak.test.ts | 73 +++++++++++++++++++ ...-13679-gemini-sse-requires-api-key.test.ts | 72 ++++++++++++++++++ 7 files changed, 241 insertions(+), 4 deletions(-) create mode 100644 changelog.d/fixes/13679-container-posture-cors.md create mode 100644 tests/unit/issue-13679-container-posture-require-api-key.test.ts create mode 100644 tests/unit/issue-13679-free-tier-summary-usage-leak.test.ts create mode 100644 tests/unit/issue-13679-gemini-sse-requires-api-key.test.ts diff --git a/Dockerfile b/Dockerfile index e5979b4d21..fe49083103 100644 --- a/Dockerfile +++ b/Dockerfile @@ -227,6 +227,18 @@ ENV NODE_OPTIONS="--max-old-space-size=${OMNIROUTE_MEMORY_MB}" ENV DATA_DIR=/app/data RUN mkdir -p /app/data +# #13679: default the PUBLISHED image to requiring an API key. A bare +# `docker run -p 20128:20128 … diegosouzapw/omniroute` (README/QUICK-START +# one-liners) does not pass `--env-file .env`, so without this default the +# anonymous /v1 LLM proxy would be both keyless AND world-reachable on the +# published container. This does NOT change the npm/CLI local-dev default +# (`REQUIRE_API_KEY` stays `"false"` in featureFlagDefinitions.ts) — only the +# shipped deployment artifact's posture. docker-compose.yml is unaffected: it +# loads the operator's own `.env` (env_file:) which overrides this ENV, and +# already binds loopback-only by default (#12568). Override with +# `-e REQUIRE_API_KEY=false` for an intentionally keyless deployment. +ENV REQUIRE_API_KEY=true + # `npm run build` (build-next-isolated → assembleStandalone) bundles ALL runtime # files into .build/next/standalone/ — .next, node_modules, migrations, scripts, # docs, and the previously hand-COPY'd modules below (@swc/helpers, pino-*, split2, diff --git a/changelog.d/fixes/13679-container-posture-cors.md b/changelog.d/fixes/13679-container-posture-cors.md new file mode 100644 index 0000000000..c662c2bb10 --- /dev/null +++ b/changelog.d/fixes/13679-container-posture-cors.md @@ -0,0 +1 @@ +- **fix(security):** default the published Docker image and `fly.toml` deployment to `REQUIRE_API_KEY=true` (npm/CLI local-dev default unchanged), and stop `/api/free-tier/summary`'s wildcard CORS from leaking the operator's local token usage to unauthenticated callers (#13679) diff --git a/fly.toml b/fly.toml index e1dea96834..e7c25b3e22 100644 --- a/fly.toml +++ b/fly.toml @@ -37,8 +37,16 @@ primary_region = 'sin' [env] TZ = "Asia/Shanghai" - + # Bind to all interfaces for Fly runtime networking. HOST = "0.0.0.0" HOSTNAME = "0.0.0.0" BIND = "0.0.0.0" + + # #13679: a Fly deployment is reachable over the public internet by design + # (force_https + Fly's edge proxy in [http_service] above) — unlike the + # docker-compose path, there is no loopback-bind option here. Require an + # API key by default so a `fly deploy` from this manifest is never keyless + # AND world-reachable at once. This does not change the npm/CLI local-dev + # default; it is specific to this published Fly manifest. + REQUIRE_API_KEY = "true" diff --git a/src/app/api/free-tier/summary/route.ts b/src/app/api/free-tier/summary/route.ts index f391634f53..ddd24fdc6f 100644 --- a/src/app/api/free-tier/summary/route.ts +++ b/src/app/api/free-tier/summary/route.ts @@ -79,7 +79,14 @@ export async function GET(req: Request): Promise { meta.generatedAt !== null && meta.generatedAt.slice(0, 10) >= FREE_CATALOG_CURATED_AT; - const serveOverlay = overlayIsFresh && (meta.tier !== "live" || (await isAuthenticated(req))); + // #13679: computed once and reused below to also gate the operator-specific + // usage fields — this endpoint ships an unconditional wildcard CORS header + // (the community free-tier catalog is intentionally public), so anything + // that is NOT meant to be world-readable must be withheld here rather than + // relying on Origin checks the browser CORS model doesn't actually enforce + // for a same-origin-looking (DNS-rebinding) caller. + const authed = await isAuthenticated(req); + const serveOverlay = overlayIsFresh && (meta.tier !== "live" || authed); // Withheld only because it is stale: drop the feed, keep the operator's own // local state. Falling back to the raw baseline here would resurrect models the @@ -95,11 +102,15 @@ export async function GET(req: Request): Promise { }) : computeFreeModelTotals({ excludeTosAvoid }); - const usedThisMonth = sumUsageTokensThisMonth(); + // #13679: usedThisMonth/remaining reveal the operator's own local usage — + // unlike the catalog totals, that is not meant to be public. Withhold both + // from unauthenticated callers instead of gating the whole route, so the + // intentionally-public catalog fields stay served to everyone. + const usedThisMonth = authed ? sumUsageTokensThisMonth() : null; const body = { ...totals, usedThisMonth, - remaining: Math.max(0, totals.steadyRecurringTokens - usedThisMonth), + remaining: authed ? Math.max(0, totals.steadyRecurringTokens - usedThisMonth!) : null, // Which source answered, and the date of what was actually served — the // feed's own build date when the overlay answers (null when a cache row // predates build-date tracking; never the download time standing in), diff --git a/tests/unit/issue-13679-container-posture-require-api-key.test.ts b/tests/unit/issue-13679-container-posture-require-api-key.test.ts new file mode 100644 index 0000000000..80b23ff796 --- /dev/null +++ b/tests/unit/issue-13679-container-posture-require-api-key.test.ts @@ -0,0 +1,60 @@ +/** + * Issue #13679 (PR E, item #6) — published container/Fly deployment posture. + * + * A default `docker run … diegosouzapw/omniroute` (README/QUICK-START one-liners, + * which do NOT pass `--env-file .env`) and the shipped `fly.toml` both publish + * the app on a public interface (Fly always; a bare `docker run -p 20128:20128` + * binds 0.0.0.0 on the host) while `REQUIRE_API_KEY` defaulted to unset/false — + * combining "world-reachable" with "keyless" for the anonymous `/v1` LLM proxy. + * + * `docker-compose.yml` already mitigates this for the compose path (loopback + * bind by default, #12568) and intentionally still follows the operator's own + * `.env` (own `REQUIRE_API_KEY` choice) — this test does NOT touch that file. + * + * The npm/CLI local-dev default in `src/shared/constants/featureFlagDefinitions.ts` + * (`REQUIRE_API_KEY` defaultValue `"false"`) is explicitly OUT of scope and must + * stay unchanged — see AGENTS.md Hard Rule #20 precedent and the owner decision + * on #13679 PR E. + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import path from "node:path"; + +const ROOT = path.resolve(import.meta.dirname, "../.."); + +function read(relPath: string): string { + return readFileSync(path.join(ROOT, relPath), "utf-8"); +} + +test("Dockerfile ships REQUIRE_API_KEY=true by default for the published image", () => { + const dockerfile = read("Dockerfile"); + assert.match( + dockerfile, + /^ENV REQUIRE_API_KEY=true$/m, + "the published Dockerfile must default to REQUIRE_API_KEY=true so `docker run` without " + + "an --env-file does not ship keyless AND world-reachable at once — override with " + + "`-e REQUIRE_API_KEY=false` for an intentionally keyless deployment" + ); +}); + +test("fly.toml ships REQUIRE_API_KEY=true in [env] for the always-public Fly path", () => { + const flyToml = read("fly.toml"); + assert.match( + flyToml, + /^\s*REQUIRE_API_KEY\s*=\s*"true"\s*$/m, + "fly.toml's [env] block must set REQUIRE_API_KEY=true — a Fly deployment is reachable " + + "over the public internet by design (force_https + Fly's edge proxy), unlike the " + + "docker-compose path which binds loopback-only by default (#12568)" + ); +}); + +test("the npm/CLI local-first REQUIRE_API_KEY default is untouched (#13679 PR E constraint)", () => { + const definitions = read("src/shared/constants/featureFlagDefinitions.ts"); + assert.match( + definitions, + /key:\s*"REQUIRE_API_KEY"[\s\S]{0,200}?defaultValue:\s*"false"/, + "the local-first npm/CLI default must stay REQUIRE_API_KEY=false — only the shipped " + + "container/Fly deployment manifests change posture, per the #13679 owner decision" + ); +}); diff --git a/tests/unit/issue-13679-free-tier-summary-usage-leak.test.ts b/tests/unit/issue-13679-free-tier-summary-usage-leak.test.ts new file mode 100644 index 0000000000..e19c558c32 --- /dev/null +++ b/tests/unit/issue-13679-free-tier-summary-usage-leak.test.ts @@ -0,0 +1,73 @@ +/** + * Issue #13679 (PR E, item #7) — `/api/free-tier/summary` ships an unconditional + * `Access-Control-Allow-Origin: "*"` and, until this fix, always included the + * operator's own local usage numbers (`usedThisMonth` / `remaining`) regardless + * of whether the caller was authenticated. Combined with the CORS wildcard, any + * reachable origin's browser JS could read the operator's local token-usage + * stats — a low-severity but real info leak (the plan-file rates this "low": + * aggregate usage, not secrets, but still per-operator data that should not be + * unconditionally public). + * + * The catalog itself (perModel, steadyRecurringTokens, …) is intentionally + * public — the community free-tier catalog is served to everyone by design + * (see the route's own `serveOverlay` comment) — so this fix scopes down only + * the operator-specific usage fields, gated behind the same `isAuthenticated()` + * check the route already uses for the "live" tier, rather than 401-ing the + * whole endpoint (which would break the intended public-catalog use case). + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { PEER_IP_HEADER, VIA_PROXY_HEADER } from "../../src/server/authz/headers.ts"; + +process.env.DATA_DIR = mkdtempSync(join(tmpdir(), "omniroute-freetier-leak-")); + +const { GET } = await import("../../src/app/api/free-tier/summary/route.ts"); + +const STAMP_TOKEN = "issue-13679-free-tier-stamp-token"; + +/** + * A remote (non-loopback, non-management-session) caller with no credentials — + * the same shape a DNS-rebinding / cross-origin attacker page would present. + * Uses the trusted peer-stamp header (like tests/unit/api-auth.test.ts) so the + * verdict is deterministic and not dependent on real socket info. + */ +function remoteUnauthenticatedRequest(): Request { + process.env.OMNIROUTE_PEER_STAMP_TOKEN = STAMP_TOKEN; + const headers = new Headers(); + headers.set(PEER_IP_HEADER, `${STAMP_TOKEN}|203.0.113.5`); + headers.set(VIA_PROXY_HEADER, `${STAMP_TOKEN}|0`); + return new Request("http://localhost/api/free-tier/summary", { headers }); +} + +test("issue #13679: unauthenticated remote GET must not leak the operator's local usedThisMonth/remaining", async () => { + const res = await GET(remoteUnauthenticatedRequest()); + assert.equal(res.status, 200); + const body = await res.json(); + + assert.equal( + body.usedThisMonth, + null, + "an unauthenticated remote caller must not learn the operator's local token usage " + + `(got ${JSON.stringify(body.usedThisMonth)})` + ); + assert.equal( + body.remaining, + null, + "remaining is derived from usedThisMonth — must also be withheld from unauthenticated callers" + ); + + // The public catalog itself stays public — this endpoint is intentionally + // served to everyone for the community free-tier catalog. + assert.ok( + Array.isArray(body.perModel) && body.perModel.length > 0, + "the public catalog data must still be served to unauthenticated callers" + ); + assert.ok(body.steadyRecurringTokens > 0); +}); + +test.after(() => { + delete process.env.OMNIROUTE_PEER_STAMP_TOKEN; +}); diff --git a/tests/unit/issue-13679-gemini-sse-requires-api-key.test.ts b/tests/unit/issue-13679-gemini-sse-requires-api-key.test.ts new file mode 100644 index 0000000000..2716057db1 --- /dev/null +++ b/tests/unit/issue-13679-gemini-sse-requires-api-key.test.ts @@ -0,0 +1,72 @@ +/** + * Issue #13679 (PR E, item #6, gemini-SSE sub-finding) — CONFIRMATION test, + * not a code fix. + * + * The audit cited `open-sse/translator/response/openai-to-gemini-sse.ts:289`, + * but that line is a plain `return new Response(...)` — it is not where any + * auth/CORS decision is made (verified by reading the file; also noted in the + * plan-file). The actual DNS-rebinding-style exposure the audit is pointing + * at depends entirely on the global `REQUIRE_API_KEY` posture: with it + * enabled, `/v1beta/models/{model}:streamGenerateContent` (this route) is + * classified CLIENT_API (`src/server/authz/classify.ts`) and fronted by + * `clientApiPolicy` (`src/server/authz/policies/clientApi.ts`) through the + * central `src/proxy.ts` middleware BEFORE the route handler (and therefore + * the translator) ever runs — an anonymous/credential-less request is + * rejected with 401 regardless of what Origin/Referer it presents (which is + * exactly what defeats DNS rebinding: the attacker page can forge Origin but + * not a valid bearer token). The CORS-header echo for this same route was + * already hardened fail-closed by #12573 (tests/unit/gemini-cors-wildcard- + * bypass.test.ts). + * + * This test locks that chain end-to-end so a future change to classify.ts / + * clientApiPolicy.ts cannot silently reopen it. #13679 PR E closes the + * "keyless AND world-reachable" combination the finding actually depends on + * by shipping REQUIRE_API_KEY=true as the default posture for the published + * container/Fly deployment (Dockerfile + fly.toml — see + * issue-13679-container-posture-require-api-key.test.ts) — this test is what + * proves that default, once applied, actually closes the gemini-SSE path. + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import { classifyRoute } from "../../src/server/authz/classify.ts"; +import { clientApiPolicy } from "../../src/server/authz/policies/clientApi.ts"; + +const GEMINI_STREAM_PATH = "/v1beta/models/gemini-2.5-flash:streamGenerateContent"; + +test("issue #13679: the gemini-SSE route is classified CLIENT_API (fronted by clientApiPolicy)", () => { + const classification = classifyRoute(GEMINI_STREAM_PATH, "POST"); + assert.equal(classification.routeClass, "CLIENT_API"); +}); + +test("issue #13679: an anonymous/DNS-rebinding-style request to the gemini-SSE route is rejected when REQUIRE_API_KEY=true", async () => { + const originalRequireApiKey = process.env.REQUIRE_API_KEY; + process.env.REQUIRE_API_KEY = "true"; + + try { + const classification = classifyRoute(GEMINI_STREAM_PATH, "POST"); + + // A DNS-rebinding attacker page can forge whatever Origin/Referer it + // likes — it presents none of the credentials clientApiPolicy checks. + const forgedOriginRequest = new Request(`http://localhost${GEMINI_STREAM_PATH}`, { + method: "POST", + headers: { Origin: "http://attacker.example", Referer: "http://attacker.example/exploit" }, + }); + + const outcome = await clientApiPolicy.evaluate({ + request: forgedOriginRequest, + classification, + requestId: "issue-13679-test", + }); + + assert.equal(outcome.allow, false); + if (!outcome.allow) { + assert.equal(outcome.status, 401); + } + } finally { + if (originalRequireApiKey === undefined) { + delete process.env.REQUIRE_API_KEY; + } else { + process.env.REQUIRE_API_KEY = originalRequireApiKey; + } + } +});