Skip to content

security(self-hosted): entry divert skips enforceApiKeyPolicy; shared key compared without timing-safe check (#14485) - #14571

Merged
diegosouzapw merged 5 commits into
release/v3.8.51from
fix/14485-selfhosted-entry-policy
Sep 24, 2026
Merged

diegosouzapw merged 5 commits into
release/v3.8.51from
fix/14485-selfhosted-entry-policy

Conversation

@diegosouzapw

@diegosouzapw diegosouzapw commented Sep 22, 2026 •

Copy link
Copy Markdown
Owner

Stacked on #14566 — merge that first.

Closes #14485

Root cause

Two confirmed defects in the self-hosted unified entry (RIC-738/D4):

  1. src/app/api/v1/chat/completions/route.ts diverts to handleSelfHostedCompletions() and
    returns its response before enforceApiKeyPolicy() ever runs — that call lives deep inside
    handleChat() (src/sse/handlers/chat.ts:692), which the divert never reaches. Once
    OMNIROUTE_SELF_HOSTED_PROVIDERS is configured, every request through the unified entry skips
    the endpoint allowlist / schedule / usage-cap / rate-limit / allowedModels checks entirely,
    regardless of which OmniRoute API key is presented. Proven live: a disabled OmniRoute API key
    (which enforceApiKeyPolicy normally rejects with 403) reached the self-hosted upstream and
    got a 200.
  2. open-sse/services/selfHostedEntry.ts compared the optional shared
    OMNIROUTE_SELF_HOSTED_API_KEY with authHeader !== expected instead of a constant-time
    comparison — a CWE-208 timing side-channel, inconsistent with this repo's own
    timingSafeCompare() convention used in 14+ other auth paths.

Fix

  • src/app/api/v1/chat/completions/route.ts: call enforceApiKeyPolicy(request, model) before
    the self-hosted divert and return its rejection if any — the same gate the rest of the cloud
    pipeline already uses. Both paths now share one policy gate.
  • open-sse/services/selfHostedEntry.ts: replace authHeader !== expected with
    timingSafeCompare(authHeader, expected) (src/shared/utils/timingSafeCompare.ts, which
    already guards the length mismatch separately so the comparison itself never throws).
  • Updated the module's D4/RIC-738 design comment, which previously documented the
    now-corrected open-by-default contract.

Behavior change

⚠️ Deployments running self-hosted providers today, where an OmniRoute API key has a schedule
restriction, rate limit, usage cap, or allowedModels allowlist, will now see those limits
enforced on self-hosted-routed requests too (previously silently bypassed). This is the intended
fix — the bypass was the defect — called out in the changelog fragment.

Regression test

tests/unit/self-hosted-entry-policy-bypass-14485.test.ts (new file, promoted from the
plan-file's TDD probe), 5 tests:

  • RED (unfixed code): #14485: self-hosted divert must not bypass enforceApiKeyPolicy for a disabled OmniRoute key — AssertionError: BUG #14485: a DISABLED OmniRoute API key reached the self-hosted upstream (status=200, upstream received 1 call(s)).
  • GREEN (fixed code), all 4 assertions pass:
    • disabled key → 403, upstream never called
    • allowedModels-restricted key requesting a disallowed model → rejected, upstream never called
    • unrestricted, policy-compliant key → still reaches the self-hosted upstream (200)
    • implementation-seam assertion: the shared key comparison goes through timingSafeCompare(),
      not a raw !== (asserted on source, not wall-clock timing, per the task's guidance that a
      timing-based assertion would be flaky)

Also improved the test's teardown (force-closing the stub upstream's connections, Connection: close on its response) after observing the original repro process hang in post-assertion
cleanup — the same "Promise resolution is still pending but the event loop has already resolved"
hang reproduces identically on an unrelated, untouched pre-existing test (api-key-policy.test.ts)
when run standalone under a timeout wrapper on this devbox, so it is a pre-existing systemic
test-runner/singleton-timer characteristic of this suite, not something this PR introduced. All
assertions in this PR's test file consistently print and pass before that unrelated hang.

Existing tests

Ran the touched-area suite (97 tests total; 96 passed, 0 failed — the 1 "cancelled" is the
pre-existing api-key-policy.test.ts process-level hang described above, not a test failure):
tests/unit/self-hosted-entry.test.ts, tests/unit/timing-safe-compare.test.ts,
tests/unit/api-key-policy.test.ts, tests/unit/chat-completions-correlation.test.ts,
tests/unit/chat-completions-parse-once-7847.test.ts,
tests/unit/chat-completions-route-shape-gate.test.ts,
tests/unit/provider-scoped-chat-completions-validation.test.ts,
tests/unit/v1-chat-completions-content-type-6414.test.ts.

Gates run

  • npm run typecheck:core → exit 0
  • npx eslint --suppressions-location config/quality/eslint-suppressions.json <changed files> → exit 0
  • node scripts/check/check-file-size.mjs → OK
  • node scripts/check/check-changelog-integrity.mjs → OK
  • node scripts/check/check-public-creds.mjs → OK
  • node scripts/check/check-mutation-test-coverage.mjs --strict → pre-existing drift, unrelated (see below)
  • node scripts/check/check-open-sse-typecheck.mjs → pre-existing drift, unrelated (see below)

⚠️ Both check-mutation-test-coverage.mjs --strict and check-open-sse-typecheck.mjs report
failures that reproduce byte-identically on the sibling #14566 branch, which does not touch
open-sse/ at all — confirming they are inherited base-tip drift, not introduced by this PR:

  • Mutation: 8 covering tests missing from stryker.conf.json across
    open-sse/services/accountFallback.ts, src/sse/services/auth.ts,
    open-sse/services/combo/comboStructure.ts, open-sse/services/combo/quotaScoring.ts — none
    touched by this PR.
  • open-sse typecheck: 2 new/regressed errors in open-sse/executors/auggie.ts (TS2769,
    TS18047) — a file this PR does not touch. This PR's own touched file
    (open-sse/services/selfHostedEntry.ts) introduces 0 new typecheck errors.

Plan-file: _tasks/pipeline/bugs/2-implementing/14485-security-self-hosted-entry-divert-skips-enforceapikeypolicy-sh.plan.md

Rework (merge-batch 2026-09-23)

  • Merged origin/release/v3.8.51 into the branch. The fix(resilience): admission lease cleanup is incomplete while the handler is still pending (#14456) #14566 content this PR used to carry (chatAdmissionRelease.ts, the fix(resilience): admission lease cleanup is incomplete while the handler is still pending #14456 test and its changelog fragment) has already landed on the release, so it no longer shows in the diff.
  • Fixed a defect in the original fix: enforceApiKeyPolicy() ran before the self-hosted divert on every keyed chat request. Cloud requests then hit the policy twice, because handleChat() runs it again. That consumed the rate-limit window twice, applied throttleDelayMs twice and checked allowedModels before alias resolution. The route now runs the policy only when isSelfHostedEntryConfigured() is true. isSelfHostedEntryConfigured() is new, exported from selfHostedEntry.ts, and handleSelfHostedCompletions uses it too. Once configured, the divert answers every request, so each request runs the policy exactly once on each path.
  • timingSafeCompare for the shared self-hosted key is unchanged.
  • New regression test: a cloud request with a key limited to 1 request per minute must not get a 429. It fails on the previous PR head (actual: 429) and passes now. The existing tests still pass: disabled key gets 403 and allowedModels blocks the self-hosted divert (security(self-hosted): entry divert skips enforceApiKeyPolicy; shared key compared without timingSafeEqual #14485 stays closed), an unrestricted key reaches self-hosted, and the timing-safe source check holds.
  • Gates: focused tests (self-hosted-entry + 14485) 29/29 pass. typecheck:core shows only the inherited cliproxyAccountHealth.ts(157,5) error. check:open-sse-typecheck fails only on open-sse/executors/auggie.ts, which is inherited from the tip and identical to it. eslint on the changed files is clean with suppressions, and file-size is OK.

…divert

The pre-divert enforceApiKeyPolicy() ran for every keyed chat request, so
cloud requests hit the policy twice (again inside handleChat): rate-limit
window consumed 2x, throttleDelayMs applied 2x, allowedModels checked
before alias resolution. Run it only when isSelfHostedEntryConfigured()
(the divert then answers every request); cloud path keeps its single run.

Adds a regression test: a 1-req/min key on a cloud request must not 429.
@diegosouzapw
diegosouzapw merged commit f6bba6a into release/v3.8.51 Sep 24, 2026
14 of 21 checks passed
diegosouzapw added a commit that referenced this pull request Sep 24, 2026
…ders (#14116) (#14465)

Merge-batch 2026-09-23 (PRs do mantenedor, Trilha B).

A reconciliação com o tip atual de `release/v3.8.51` foi feita no branch da PR; o detalhe está na seção "Rework (merge-batch 2026-09-23)" do corpo, quando existe.
- #14571: a política de API key passa a rodar só quando o divert self-hosted está configurado (antes rodava 2x em toda request cloud). Prova red→green: o teste "cloud request runs the key policy ONCE" dava 429 na versão anterior e agora 5/5 passam; 29/29 no total.
- #14465: re-medido depois do merge do tip. chat.ts 2560→2561 e chatHelpers.ts 1257→1258 (+1 cada, plumbing de `forcedConnectionId`), com anotação datada dentro de `frozen`. 11/11 testes focados (inclui o guard #5849).
- #14467: reconciliada depois da #14468 (suno). Contagem de providers regerada = 358 (`gen:provider-reference` + `check:provider-consistency`); REMOVED_PROVIDERS/blocklist com as duas entradas; tripwire de prefixos reservados re-medido em 412; AGENTS.md/llm.txt mudam só o número (aprovado pelo dono). 57/57 testes focados.
- Em todas: `typecheck:core` mostra só o herdado `cliproxyAccountHealth.ts:157`, e o único vermelho de `check:open-sse-typecheck` é o herdado `auggie.ts` (#14547).
diegosouzapw added a commit that referenced this pull request Sep 24, 2026
…nt to validate rewrite (#14217) (#14467)

Merge-batch 2026-09-23 (PRs do mantenedor, Trilha B).

A reconciliação com o tip atual de `release/v3.8.51` foi feita no branch da PR; o detalhe está na seção "Rework (merge-batch 2026-09-23)" do corpo, quando existe.
- #14571: a política de API key passa a rodar só quando o divert self-hosted está configurado (antes rodava 2x em toda request cloud). Prova red→green: o teste "cloud request runs the key policy ONCE" dava 429 na versão anterior e agora 5/5 passam; 29/29 no total.
- #14465: re-medido depois do merge do tip. chat.ts 2560→2561 e chatHelpers.ts 1257→1258 (+1 cada, plumbing de `forcedConnectionId`), com anotação datada dentro de `frozen`. 11/11 testes focados (inclui o guard #5849).
- #14467: reconciliada depois da #14468 (suno). Contagem de providers regerada = 358 (`gen:provider-reference` + `check:provider-consistency`); REMOVED_PROVIDERS/blocklist com as duas entradas; tripwire de prefixos reservados re-medido em 412; AGENTS.md/llm.txt mudam só o número (aprovado pelo dono). 57/57 testes focados.
- Em todas: `typecheck:core` mostra só o herdado `cliproxyAccountHealth.ts:157`, e o único vermelho de `check:open-sse-typecheck` é o herdado `auggie.ts` (#14547).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security(self-hosted): entry divert skips enforceApiKeyPolicy; shared key compared without timingSafeEqual

1 participant