Repository navigation
fix(authz): REQUIRE_API_KEY=false never meant "the internet may call this" - #69
Merged
Merged
Conversation
…this"
A security audit of this tree demonstrated an unauthenticated, credential-free
POST /v1/chat/completions being ROUTED and EXECUTED — six real upstream
attempts, 502 rather than 401 — on an instance whose dashboard password was
set and whose every /api/** route correctly answered 401.
The gap: `requireLogin` governs /api/** only. /v1/** — the inference surface —
is governed by REQUIRE_API_KEY, which ships `false`. On a public domain that
is an open LLM relay billed to the operator, with arbitrary prompts landing in
call_logs. My own 3.8.54 smoke test missed it because I probed /api/** and
GET /v1/models (gated by a different switch) and concluded the instance was
closed.
The allowed identity was already literally named `local`. It just was not
checked. `clientApiPolicy` now requires the caller to actually be local:
isLoopbackRequest(ctx) || isPrivateLanRequest(ctx)
Both helpers already exist, are used by the management policy, are proxy-aware
and fail closed — a request that arrived through a reverse proxy is never
local even though its socket peer is loopback, and an unresolvable peer counts
as remote. So domain → Caddy/nginx/cloudflared → OmniRoute is refused while
localhost and a LAN IDE are untouched. There is deliberately no flag to
re-open anonymous remote access: an operator who wants remote callers issues
them a key, which is what REQUIRE_API_KEY=true has always been for.
Deployment recipes disagreed with each other, and the one for a public domain
was the wrong one:
docs/ops/VM_DEPLOYMENT_GUIDE.md REQUIRE_API_KEY=false -> true, with why
docker-compose.prod.yml (unset -> false) -> ${...:-true}
fly.toml (unset -> false) -> "true"
contrib/vps/compose.yaml already :-true unchanged
contrib/podman/omniroute.container already true unchanged
`tests/unit/authz/route-origin-auth-matrix.test.ts` had encoded the defect as
the contract — it expected ALLOW for an anonymous CLIENT_API request from a
`public` origin. Corrected, with the reason written down.
Also from the same audit, HIGH: `quality.yml`'s `lint-guard` selected the
maintainer's persistent self-hosted LAN pool with no fork clause, while
running `npm ci` (no --ignore-scripts) against the fork's lockfile — arbitrary
code execution on the build machine, going green because the next line sets
continue-on-error for forks. The file states that rule in its own comments and
ci.yml's build job implements it; this copies the same own-origin clause.
`tests/unit/workflows-self-hosted-fork-guard.test.ts` compares every
self-hosted runs-on in a pull_request workflow against it — proven failable.
tests/unit/authz/** + fork-guard 320 pass / 0 fail
client-api-anonymous-is-local-only 5 pass (loopback, ::1, LAN allowed;
public and unresolvable refused)
typecheck:core exit 0, 0 errors; eslint exit 0; prettier clean
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
LMPrado-DZ23
pushed a commit
that referenced
this pull request
Sep 26, 2026
…e before the tag All six full sweeps on release/v3.8.55 failed on the same ~50 integration tests. I had read "the slow-suite job concluded success" as "the suite passed"; those jobs upload their report and succeed, and only the aggregator reads it. Reproduced locally and fixed by cause: 1. 22 files failed the network guard: each chat dispatch warms the egress IP via api64/api4.ipify.org. Pin the documented OMNIROUTE_PROXY_ECHO_URL override to a closed loopback port in tests/_setup/isolateDataDir.ts. 2. /api/v1/models refreshes the AI Horde catalog (aihorde.net): inject an empty catalog in three more files, as api-routes-critical already does. modelsDevSync.test.ts is a real live test — gate it behind RUN_LIVE_TESTS=1. provider-journey.contract used example.com, a real domain, as its fake proxy host: use a .test name. 3. Combo fixtures routed to claude-3-5-sonnet-20241022, which the model-lifecycle registry rejects (retired 2025-10-28): 8 files now use claude-sonnet-4-6. 4. The three HTTP e2e suites boot `next dev` via run-next-playwright.mjs, which has no custom server to stamp the peer, so #69 answered them AUTH_002. They now send the token-stamped header the custom server would (tests/helpers/localPeerStamp.ts); a wrong token is still ignored, so the check under test is not weakened. 5. combo-failover-e2e used an 80ms per-target timeout, shorter than the request setup before the first upstream fetch: the first target timed out before the stub was called, so the test asserted a timeout that had not happened where it claimed. Local: combo-matrix 25/26 (DRR fairness still red), the fixed files 82+/91. Not verified locally: the three HTTP e2e suites (Turbopack rejects this worktree's node_modules junction; webpack dev timed out compiling) — CI is the check for those. Still open: quota-share DRR fairness; combo-routing-e2e call-log persistence; weighted distribution. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CRITICAL + HIGH from the security audit of this tree. Both were reproduced before being fixed.
The CRITICAL: a published instance is an open LLM relay
The audit sent this to a local instance with a dashboard password set,
requireLoginon, and every/api/**route correctly answering 401 — with no credential of any kind:502, not 401. The request passed authorization, passed routing, and the gateway made six real upstream calls on an anonymous caller's behalf.
requireLogingoverns/api/**./v1/**— the inference surface — is governed byREQUIRE_API_KEY, which shipsfalse. On a public domain that is a free LLM relay billed to the operator, running on their provider credentials and quota, with arbitrary prompts landing incall_logs.It is inconsistent even inside
/v1:GET /v1/modelskeys onrequireLoginand does 401. That is exactly why spot-checking makes the instance look closed. My own 3.8.54 smoke test made that mistake — I probed/api/**and/v1/models, saw 401 everywhere, and reported the image as closed by default.The fix
The allowed identity was already literally named
local. It just was not checked:isLoopbackRequest/isPrivateLanRequestalready exist, are used by the management policy, are proxy-aware and fail closed — a request that arrived through a reverse proxy is never local even though its socket peer is loopback, and an unresolvable peer counts as remote.So
domain → Caddy/nginx/cloudflared → OmniRouteis refused, whilelocalhostand a LAN IDE are untouched. There is deliberately no flag to re-open anonymous remote access: an operator who wants remote callers issues them a key, which is whatREQUIRE_API_KEY=truehas always been for. A flag to restore the hole is not a feature.The deployment recipes disagreed, and the public one was wrong
docs/ops/VM_DEPLOYMENT_GUIDE.md— "Deployment on VM with Cloudflare"REQUIRE_API_KEY=falsetrue, with the reason inlinedocker-compose.prod.yml${REQUIRE_API_KEY:-true}fly.toml"true"contrib/vps/compose.yaml:-truecontrib/podman/omniroute.containertrueThe VM guide is the one for a public domain, and it printed the open setting two lines below
AUTH_COOKIE_SECURE=true, with no warning.A test had encoded the defect as the contract
tests/unit/authz/route-origin-auth-matrix.test.tsexpectedALLOW("anonymous")for a CLIENT_API request from apublicorigin. That row is corrected, with the reason written down — this is the class of test that makes a vulnerability look intentional.tests/unit/authz/client-api-anonymous-is-local-only.test.tspins both directions, because a fix that only closed the hole would break every local and LAN user of the default configuration: loopback (v4 and v6) and private LAN still allowed; public peer and unresolvable peer refused;REQUIRE_API_KEY=truestill refuses loopback.The HIGH: fork PRs execute on the maintainer's LAN runner
No fork clause. It triggers on
pull_requestagainstrelease/**and runsnpm ci— without--ignore-scripts— against the fork's own lockfile. That is arbitrary code execution on a persistent machine that keeps state between jobs, and the very next line setscontinue-on-errorfor forks, so it goes green.The file states that rule in its own comments ("a fork PR must never execute on the LAN runner") and
ci.yml's build job implements it. This copies the same own-origin clause.tests/unit/workflows-self-hosted-fork-guard.test.tscompares every self-hostedruns-onin apull_requestworkflow against the guard, and refuses to pass vacuously if the selectors move. Proven failable: reverting the one line takes it from 1 pass to 1 fail.Gates
tests/unit/authz/**+ fork guardclient-api-anonymous-is-local-onlytypecheck:coreeslint --max-warnings=0prettier --checkSix pre-existing tests failed on first run because their policy contexts carried no peer address — the locality helpers correctly fail closed on that. Giving them a loopback peer is the honest fix: a local caller is loopback, and the contexts now resemble the requests they claim to model.
🤖 Generated with Claude Code