Skip to content

fix(security): close 3 advisories — search baseUrl exfil, sk- in the error sanitizer, bifrost relay header leak - #12620

Merged
diegosouzapw merged 7 commits into
release/v3.8.51from
fix/sec-3adv-baseurl-sk-bifrost
Sep 3, 2026
Merged

diegosouzapw merged 7 commits into
release/v3.8.51from
fix/sec-3adv-baseurl-sk-bifrost

Conversation

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Closes GHSA-3f8g-pfh9-j687 (HIGH), GHSA-qv45-56jc-4wmj and GHSA-9m72-44hg-w32g, all reported by @skeletonsec. Targets release/v3.8.51.

All three are the same meta-pattern: a hardened path exists and a sibling path bypasses it. Each is fixed at the shared source rather than on the reported side, because "fix one copy" is what produced them.


1. provider_options.baseUrl sent the operator's key to a caller-chosen host

A caller with search scope could redirect the SaaS search builders, so the operator's third-party API key went wherever they named — key=/api_key= in the query for google-pse/searchapi, X-API-Key/Authorization for you.com/linkup/nimble/ollama — and the parsed response came back, making the same primitive an SSRF with readback.

My own GHSA-j7j4 fix is the one this slipped past. That fix added parseAndValidateNonMetadataUrl to this exact resolver, which blocks IMDS and nothing else. Against key exfiltration it does nothing: the attacker names their own public host. I treated a credential-destination problem as an SSRF problem.

And the j7j4 regression test missed it for a sharper reason worth recording: its fixture was searxng-search — the one keyless provider where redirecting the base URL leaks no credential. The test picked the benign case and passed.

The fix separates two sources that were being read through one call:

Source Origin Now
providerSpecificData stored provider connection (credentials?.providerSpecificData) — operator honored, block-metadata, loopback/LAN keeps working for self-hosted searxng
providerOptions body.provider_options — caller refused outright

No provider needs a per-request caller-chosen fetch target; the self-hosted story is served by the operator field. Operator config now also wins over a caller value instead of being shadowed by it.

Deliberate behaviour change: a caller sending provider_options.baseUrl gets an explicit refusal instead of a silently redirected request. Silently ignoring it would leave them believing the override applied.

The new suite iterates every keyed provider in the registry instead of one fixture, and asserts the registry still has keyed providers so it cannot degrade to zero coverage. search-baseurl-ssrf-guard.test.ts had one assertion encoding the wrong contract (caller-supplied loopback allowed) — moved to the operator source with the reasoning inline; the j7j4 metadata cases are untouched.


2. The error sanitizer had no sk- pattern

upstreamErrorPassthrough.ts recognized \bsk-[…]{8,} and refused verbatim passthrough for any upstream 4xx carrying it — correctly calling it a leak. The body then fell through to buildErrorBody → sanitizeErrorMessage → redactSensitiveErrorText, which had no such pattern. The layer that identified the credential handed it to a layer that could not see it, so Incorrect API key provided: sk-proj-… came back to the caller intact.

The passthrough file's comment claimed it "mirrors the vocabulary of redactSensitiveErrorText". The mirror had drifted — so the raw-token shapes now live once, in error.ts as RAW_CREDENTIAL_PATTERNS, and the passthrough layer imports them. Covers sk-/sk_, Google AIza… and three-segment JWTs, with upper-bounded quantifiers per AGENTS.md → PII learnings §1 (these run over untrusted upstream bodies).

The suite's last test is the anti-drift assertion: for every body the passthrough layer refuses as leaky, the fallback sanitizer must not return it unchanged. It also pins that ordinary prose containing "risk" or "task sk" is left alone.


3. The bifrost relay returned the sidecar's headers and body verbatim

The relay sends Authorization: Bearer ${BIFROST_API_KEY} to the sidecar, and new Headers(upstream.headers) handed back anything the sidecar echoed — that header, its own set-cookie, an x-api-key — to whoever holds the relay token. set-cookie matters as much as the credential: it is a session, and a browser would store it against our origin.

New shared stripSensitiveResponseHeaders(), built on the filterUpstreamResponseHeaderEntries helper that already sat in that file unused. Applied to both relay routes — the TS sibling copies headers wholesale too, so fixing only the reported one would leave the same hole next door plus a second copy waiting to drift. The bifrost route also normalizes non-2xx through parseUpstreamError + buildErrorBody + sanitizeErrorMessage, reaching parity with the sibling and Hard Rule #12.


Validation

TDD on all three, hermetic (env -u OMNIROUTE_API_KEY):

Suite Result
search-baseurl-client-override-3f8g 23/23 (19 red before)
error-sanitizer-sk-key-qv45 9/9 (7 red before)
bifrost-relay-response-leak-9m72 6/6 (all red before)
sibling sweep (upstream-response-headers-strip, upstream-error-passthrough, search-baseurl-ssrf-guard, free-web-search, firecrawl-search-ssrf-guard + the three above) 91/91
  • typecheck:core — exit 0, no output
  • check:mutation-test-coverage --strict — ✓ No drift (the three new test files cover mutated modules; registered up front rather than after CI caught it, as in fix(authz): hard-gate every credential export and CLI-config write (GHSA-5926-2w35-7h4q) #12600)
  • check:tracked-artifacts, check:lockfile — OK
  • prettier --check clean; eslint — 37 errors across the touched files, exactly the 37 already frozen in eslint-suppressions.json for search.ts (34) and error.ts (3). This diff adds none.

One flake seen and discriminated: cursor-renewal's SIGTERM-timing case failed under the parallel sweep and passes 28/28 in isolation. Unrelated to these files.

⚠️ check:public-creds fails on this branch — the zcodeProtocol.ts base-red, fixed separately in #12615. Not touched by this diff.

GHSA-3f8g-pfh9-j687: a caller with search scope could set
`provider_options.baseUrl` and redirect the SaaS search builders, so the
OPERATOR's third-party API key went to a caller-chosen host — `key=`/`api_key=`
in the query for google-pse/searchapi, `X-API-Key`/`Authorization` for
you.com/linkup/nimble/ollama — and the parsed response came back, making the
same primitive an SSRF with readback.

My own GHSA-j7j4-g9qc-q69c fix is the one this slipped past. That fix added
`parseAndValidateNonMetadataUrl` to this exact resolver, which blocks IMDS and
nothing else: against key exfiltration it does nothing at all, because the
attacker just names their own public host. I treated a credential-destination
problem as an SSRF problem.

The j7j4 regression test missed it for a sharper reason worth recording: its
fixture was `searxng-search`, the one keyless provider where redirecting the base
URL leaks no credential. The test picked the benign case and passed.

The two override sources are not equally trusted, and reading them through one
`getProviderSettingString()` call erased the distinction:

  - `providerSpecificData` comes from the stored provider connection
    (`credentials?.providerSpecificData`) — operator config. This is how an
    operator points at a self-hosted searxng, so it keeps working on
    loopback/LAN under block-metadata, and IMDS stays rejected.
  - `providerOptions` is `body.provider_options` — caller input. Now refused
    outright rather than validated. No provider needs a per-request
    caller-chosen fetch target; the self-hosted story is served by the operator
    field. If one ever does, it needs a per-provider opt-in plus a host
    allow-list, not a validated free-form URL.

Operator config now also wins over a caller value instead of being shadowed by
it.

Behaviour change, deliberate: a caller that today sends
`provider_options.baseUrl` gets an explicit refusal instead of a silently
redirected request. Silently ignoring it would leave the caller believing the
override applied.

tests/unit/search-baseurl-client-override-3f8g.test.ts — 23 tests, red before
the fix. It iterates EVERY keyed provider in the registry instead of one
fixture, and asserts the registry still has keyed providers so the suite cannot
degrade to zero coverage.

search-baseurl-ssrf-guard.test.ts had one assertion encoding the wrong contract
(caller-supplied loopback allowed). Moved to the operator source with the
reasoning inline — the j7j4 metadata cases are untouched.

Reported by @skeletonsec.

Closes GHSA-3f8g-pfh9-j687
…izer

GHSA-qv45-56jc-4wmj: two copies of one redaction rule, and only one of them
learned about `sk-`.

`upstreamErrorPassthrough.ts` recognized `\bsk-[…]{8,}` and REFUSED verbatim
passthrough for any upstream 4xx carrying it — correctly treating it as a leak.
The body then fell through to `buildErrorBody` → `sanitizeErrorMessage` →
`redactSensitiveErrorText`, which had no such pattern. So the layer that
identified the credential handed it to a layer that could not see it, and
OpenAI-style `Incorrect API key provided: sk-proj-…` bodies came back to the
caller intact. For a gateway pooling operator keys that is cross-tenant
disclosure of the operator's upstream credential.

The passthrough file's comment said it "mirrors the vocabulary of
redactSensitiveErrorText". The mirror had drifted, which is why this is fixed at
the source rather than by adding a pattern on each side: the raw-token shapes now
live once, in `error.ts` as `RAW_CREDENTIAL_PATTERNS`, and the passthrough layer
imports them. Both sides move together or not at all.

Covers `sk-`/`sk_`, Google `AIza…`, and three-segment JWTs. Quantifiers are
upper-bounded per AGENTS.md → PII learnings §1 — these run over untrusted
upstream error bodies.

tests/unit/error-sanitizer-sk-key-qv45.test.ts — 9 tests, 7 red before the fix.
The last one is the anti-drift assertion: for every body the passthrough layer
refuses as leaky, the fallback sanitizer must not return it unchanged. It also
pins that ordinary prose containing "risk"/"task sk" is left alone.

Reported by @skeletonsec.

Closes GHSA-qv45-56jc-4wmj
GHSA-9m72-44hg-w32g: the standalone bifrost relay copied ALL upstream response
headers via `new Headers(upstream.headers)` and returned non-2xx bodies verbatim,
while its TypeScript sibling routed non-2xx through parseUpstreamError +
buildErrorBody + stripStaleEncodingHeaders.

The relay sends `Authorization: Bearer ${BIFROST_API_KEY}` to the sidecar, so
anything the sidecar or a further upstream echoes back — that header, its own
`set-cookie`, an `x-api-key` — reached whoever holds the relay token. `set-cookie`
matters as much as the credential: it is a session, and a browser would store it
against our origin.

Both fixes:

  - New shared `stripSensitiveResponseHeaders()` in upstreamResponseHeaders.ts,
    built on the `filterUpstreamResponseHeaderEntries` helper that already
    existed there unused. Drops authorization / proxy-authorization / x-api-key /
    x-goog-api-key / api-key / cookie / set-cookie on top of the stale framing
    set. Applied to BOTH relay routes — the TS sibling copied headers wholesale
    too, so fixing only the reported one would have left the same hole next door
    and a second copy waiting to drift, which is the exact failure mode of
    GHSA-v7g9 and GHSA-qv45.
  - The bifrost route normalizes non-2xx through parseUpstreamError +
    buildErrorBody + sanitizeErrorMessage, reaching parity with the sibling and
    Hard Rule #12.

tests/unit/bifrost-relay-response-leak-9m72.test.ts — 6 tests, red before the
fix: the helper's behaviour (including that it does not mutate its input), plus
source guards pinning BOTH routes to the shared strip with a negative assertion
that the wholesale copy has not come back.

Reported by @skeletonsec.

Closes GHSA-9m72-44hg-w32g
The previous commit added `buildErrorBody`/`sanitizeErrorMessage` and pulled
`parseUpstreamError` from `handlers/usageExtractor`, where it does not live —
it is exported from `utils/error.ts` alongside the other two. That produced
TS2300 (duplicate `buildErrorBody`, already imported below) and TS2305 on the
same route, caught by check:api-typecheck.

One import from utils/error.ts for all three. check:api-typecheck now reports
OK — 289 pre-existing errors, all within the frozen baseline (was FAIL with 2
new/regressed). Relay tests 6/6, prettier clean.
…ng it

Correcting my own over-tightening in this branch. The previous commit refused
EVERY caller-supplied `provider_options.baseUrl`, which broke a documented,
tested flow: `tests/unit/search-route.test.ts` covers "v1 search POST accepts
authless SearXNG with provider_options baseUrl". SearXNG is self-hosted by
definition and the caller naming their own instance is the supported way to use
it — I read the field as purely hostile when part of its use is legitimate.

This is the gate the reporter actually recommended and I went past: allow the
caller override only for a provider that is BOTH keyless and explicitly opted in
via the new `allowClientBaseUrlOverride` flag. Today that is `searxng-search`
alone. Every `authType: "apikey"` provider still refuses, which is the half that
matters — those are the ones whose builder attaches the operator's key to
whatever host the base URL resolves to.

The flag is checked alongside `authType === "apikey"` at the callsite AND pinned
by a registry-wide invariant in the test: no provider may carry
`allowClientBaseUrlOverride` while holding a key. Inferring the capability from
which builder happens to call the resolver is what let this drift in the first
place, so it is now declared, not inferred.

The opted-in path keeps the block-metadata guard: loopback/LAN is the entire
point of a self-hosted instance, IMDS is not.

search-route 8/8, the 3f8g + j7j4 suites 33/33, sibling sweep 68/68,
typecheck:core exit 0, check:api-typecheck OK (289, within baseline).

Also registers error-sanitizer-sk-key-qv45 in stryker tap.testFiles —
check:mutation-test-coverage flagged it once error.ts entered the mutated set.
check:file-size flagged `open-sse/handlers/search.ts: 1824 > frozen 1789`. The
growth was mine — the GHSA-3f8g fix added the trust split plus the reasoning for
it to a file that was already at its ceiling.

Shrinking beats loosening the ratchet, and the gate's own message says so
("modularize/extraia (DRY) para encolher"), so the resolver moves to
`open-sse/handlers/search/baseUrl.ts` with its refusal error and the
single-source reader. search.ts drops 1824 -> 1772, under the ceiling, and the
base-URL trust decision — which source is operator, which is caller, which
providers may be redirected — is now one small unit instead of a passage inside
a 1800-line handler. search.ts re-exports both symbols, so every existing
importer is untouched.

Also drops the `parseAndValidateNonMetadataUrl` import that the extraction left
orphaned in search.ts; eslint on that file is back to exactly its 34 frozen
violations.

The remaining file-size failures (`open-sse/executors/codex.ts` 1503 > 1499,
`open-sse/utils/stream.ts` 3078 > 3072) are base debt in files this branch does
not touch.

search + baseUrl suites 52/52, typecheck:core exit 0, check:api-typecheck OK
(289, within baseline), check:cycles OK, check:mutation-test-coverage no drift.
@diegosouzapw
diegosouzapw merged commit 4a37c7f into release/v3.8.51 Sep 3, 2026
20 of 21 checks passed
diegosouzapw added a commit that referenced this pull request Sep 4, 2026
…attern #12620 had that it lacked

#12620 landed first and fixed GHSA-qv45-56jc-4wmj by adding
RAW_CREDENTIAL_PATTERNS to error.ts and importing them from
upstreamErrorPassthrough.ts. This branch fixes the same class differently, by
splitting error.ts into errorSanitization.ts + errorPathRedaction.ts.

Kept this branch's split. Its STRONG_CREDENTIAL_TOKEN already covered sk-/sk_
(with a lookbehind and an embedded-prefix variant that catches sk-proj-…),
plus Slack xox-, AWS AKIA/ASIA, github_pat_/ghp_/glpat- and three-segment JWTs
— a strictly wider vocabulary than the three patterns #12620 introduced. The
one shape #12620 carried and this set did not is Google's AIza…, now added
here with the same bounded quantifier its siblings use.

Verified against #12620's own suites rather than by inspection: 48/48 across
error-sanitizer-sk-key-qv45, bifrost-relay-response-leak-9m72,
search-baseurl-client-override-3f8g and search-baseurl-ssrf-guard — including
that suite's anti-drift assertion (for every body the passthrough layer refuses
as leaky, the fallback sanitizer must not return it unchanged). This branch's
own 21 test files: 259/259. typecheck:core clean.
diegosouzapw added a commit that referenced this pull request Sep 4, 2026
Validado sobre o tip de `release/v3.8.51` depois de reconciliar com o #12620, que entrou primeiro nesta mesma sessão e ataca a mesma classe de problema por outra arquitetura.

**A colisão e como foi resolvida.** O #12620 consertou o GHSA-qv45-56jc-4wmj adicionando `RAW_CREDENTIAL_PATTERNS` a `error.ts` e importando-os em `upstreamErrorPassthrough.ts`. Este PR resolve o mesmo problema quebrando `error.ts` em `errorSanitization.ts` + `errorPathRedaction.ts`. Mantive a divisão em módulos deste PR, porque ao comparar os dois vocabulários o dele já era mais amplo: o `STRONG_CREDENTIAL_TOKEN` daqui cobre `sk-`/`sk_` **com lookbehind e uma variante para a forma embutida** (que pega `sk-proj-…`), mais Slack `xox-`, AWS `AKIA`/`ASIA`, `github_pat_`/`ghp_`/`glpat-` e JWT de três segmentos.

A única forma que o #12620 carregava e este conjunto não tinha era a chave do Google (`AIza…`) — adicionada aqui, com o mesmo quantificador limitado que os irmãos usam (AGENTS.md → PII §1, já que isso roda sobre corpos upstream não confiáveis).

**A verificação não foi por inspeção.** Rodei as suítes do próprio #12620 contra esta estrutura: **48/48** em `error-sanitizer-sk-key-qv45`, `bifrost-relay-response-leak-9m72`, `search-baseurl-client-override-3f8g` e `search-baseurl-ssrf-guard` — incluindo a asserção anti-drift daquela suíte, que é o oráculo certo aqui: *para todo corpo que a camada de passthrough recusa como vazante, o sanitizador de fallback não pode devolvê-lo intacto*. Ela passa, então a propriedade de segurança dos três GHSAs sobrevive à troca de arquitetura.

Os 21 arquivos de teste deste PR: **259/259**. `typecheck:core` limpo.
diegosouzapw added a commit that referenced this pull request Sep 8, 2026
…12964)

Vazamento de credencial em corpo de erro. `tests/unit/error-sanitizer-sk-key-qv45.test.ts` falhava no tip em 8ms:

```
AssertionError: Google key survived: Bad credentials for AIzaSyA1B2C3D4E5F6G7H8I9J0KaLbMcNdOeP
```

O padrão era `/AIza[0-9A-Za-z_-]{35}/` — comprimento **exato**. Uma chave Google padrão tem 39 caracteres e casa; qualquer credencial `AIza…` mais curta ou mais longa passava direto para o corpo do erro.

Os dois lados divergiram na reconciliação de dois PRs do mesmo GHSA: o padrão com `{35}` veio do #12506, o teste anti-drift que cobra `/\\bAIza[A-Za-z0-9_-]{20,}/` veio do #12620. Está vermelho desde que os dois entraram em sequência.

`{20,}` no lugar de `{35}`. Numa mensagem de erro, redigir demais uma string que apenas começa com `AIza` não custa nada; redigir de menos vaza credencial — o lado errado para errar é claro.

Evidência: o arquivo vai de 7/9 para **9/9**. Bateria de sanitização com 538 testes: 533 passam, e as 5 restantes são pré-existentes no tip, não desta mudança (4 levam 21–25s por spawn de processo isolado sob carga; `tunnel-routes-error-sanitization` falha igual no tip puro, verificado). Nenhum teste foi enfraquecido — o padrão foi ampliado para satisfazer uma asserção que já existia.
JJAbrams-eng added a commit to JJAbrams-eng/OmniRoute that referenced this pull request Sep 22, 2026
Combining cheaper-inference#12454 (adds zai_stream_error) with the error-boundary hardening
commits (cheaper-inference#12506/cheaper-inference#12620) left the new code missing from
SAFE_PUBLIC_ERROR_IDENTIFIERS, so buildErrorBody() silently downgraded it
to the generic bad_gateway fallback.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…error sanitizer, bifrost relay header leak (cheaper-inference#12620)

Validado em worktree combinada sobre o tip de release/v3.8.51: os dois boardaram sem conflito, typecheck:core limpo, check-file-size sem violação nova (as duas restantes — codex.ts e stream.ts — são drift anterior) e 51/51 nos 5 arquivos de teste que os PRs trazem.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
Validado sobre o tip de `release/v3.8.51` depois de reconciliar com o cheaper-inference#12620, que entrou primeiro nesta mesma sessão e ataca a mesma classe de problema por outra arquitetura.

**A colisão e como foi resolvida.** O cheaper-inference#12620 consertou o GHSA-qv45-56jc-4wmj adicionando `RAW_CREDENTIAL_PATTERNS` a `error.ts` e importando-os em `upstreamErrorPassthrough.ts`. Este PR resolve o mesmo problema quebrando `error.ts` em `errorSanitization.ts` + `errorPathRedaction.ts`. Mantive a divisão em módulos deste PR, porque ao comparar os dois vocabulários o dele já era mais amplo: o `STRONG_CREDENTIAL_TOKEN` daqui cobre `sk-`/`sk_` **com lookbehind e uma variante para a forma embutida** (que pega `sk-proj-…`), mais Slack `xox-`, AWS `AKIA`/`ASIA`, `github_pat_`/`ghp_`/`glpat-` e JWT de três segmentos.

A única forma que o cheaper-inference#12620 carregava e este conjunto não tinha era a chave do Google (`AIza…`) — adicionada aqui, com o mesmo quantificador limitado que os irmãos usam (AGENTS.md → PII §1, já que isso roda sobre corpos upstream não confiáveis).

**A verificação não foi por inspeção.** Rodei as suítes do próprio cheaper-inference#12620 contra esta estrutura: **48/48** em `error-sanitizer-sk-key-qv45`, `bifrost-relay-response-leak-9m72`, `search-baseurl-client-override-3f8g` e `search-baseurl-ssrf-guard` — incluindo a asserção anti-drift daquela suíte, que é o oráculo certo aqui: *para todo corpo que a camada de passthrough recusa como vazante, o sanitizador de fallback não pode devolvê-lo intacto*. Ela passa, então a propriedade de segurança dos três GHSAs sobrevive à troca de arquitetura.

Os 21 arquivos de teste deste PR: **259/259**. `typecheck:core` limpo.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…heaper-inference#12964)

Vazamento de credencial em corpo de erro. `tests/unit/error-sanitizer-sk-key-qv45.test.ts` falhava no tip em 8ms:

```
AssertionError: Google key survived: Bad credentials for AIzaSyA1B2C3D4E5F6G7H8I9J0KaLbMcNdOeP
```

O padrão era `/AIza[0-9A-Za-z_-]{35}/` — comprimento **exato**. Uma chave Google padrão tem 39 caracteres e casa; qualquer credencial `AIza…` mais curta ou mais longa passava direto para o corpo do erro.

Os dois lados divergiram na reconciliação de dois PRs do mesmo GHSA: o padrão com `{35}` veio do cheaper-inference#12506, o teste anti-drift que cobra `/\\bAIza[A-Za-z0-9_-]{20,}/` veio do cheaper-inference#12620. Está vermelho desde que os dois entraram em sequência.

`{20,}` no lugar de `{35}`. Numa mensagem de erro, redigir demais uma string que apenas começa com `AIza` não custa nada; redigir de menos vaza credencial — o lado errado para errar é claro.

Evidência: o arquivo vai de 7/9 para **9/9**. Bateria de sanitização com 538 testes: 533 passam, e as 5 restantes são pré-existentes no tip, não desta mudança (4 levam 21–25s por spawn de processo isolado sob carga; `tunnel-routes-error-sanitization` falha igual no tip puro, verificado). Nenhum teste foi enfraquecido — o padrão foi ampliado para satisfazer uma asserção que já existia.
@diegosouzapw
diegosouzapw deleted the fix/sec-3adv-baseurl-sk-bifrost branch October 18, 2026 23:51
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.

2 participants