fix(audit): close the five findings from the verification round - #42
Merged
Merged
Conversation
The TypeScript SDK gated redirect following only on whether a custom credential header was present, so a 307 from the configured baseUrl host to a different origin made fetch re-POST the whole chat-completion body — the prompt — to that other origin. There was also no scheme check anywhere in sdk/typescript/src, so an https -> http redirect was followed in cleartext. Every request now uses redirect: "manual" and the SDK vets the 3xx itself, in vetRedirect(), mirroring the Python SDK's _CredentialSafeRedirectHandler: an https -> non-https downgrade and any change of origin are refused; a same-origin target is followed, capped at 5 hops. Refusal raises the SDK's own OmniRouteError with code "redirect_refused"; the message is built from the response status and the Location origin only (userinfo dropped), so no request body and no header value can reach it. A refused redirect is a policy decision, so #execute rethrows it instead of treating it as a retryable network failure. Regression tests (loopback servers only, no provider calls): a cross-origin 307 is refused and the target records zero requests, so the body never arrives; an https -> http redirect is refused, mirroring the Python test_https_to_http_redirect_is_refused; a same-origin redirect is still followed. The previous test asserted the old, laxer behaviour (that fetch merely strips Authorization) and now asserts the refusal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GET /api/omniroute/route/decisions/[id] called requireManagementAuth without
{ alwaysRequireAuth: true }, so under requireLogin=false any anonymous caller
could read a live routing decision. /api/metrics already passes that option,
with the rationale that the payload names the providers and models in use; the
decision payload names the same providers and models, plus their scores, their
exclusion reasons and the policy version. Pass the same option here.
The regression test asserts an anonymous lookup is refused (401/403) and that
the refusal body does not leak the decision, following the pattern of the
existing /api/metrics "anonymous callers are rejected" test. The two lookups in
this file that previously relied on the implicit anonymous pass now build a
management session request, as the metrics tests do.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
escapeLabelValue() escaped backslash, LF and double quote but left a raw CR in the label value. A parser that splits the exposition on CRLF ends the line at that CR, so a model id carrying one could put the remainder of the value on what reads as its own metric line. Escape it as \r alongside the other control characters. The existing escaping test now also renders a label containing CR LF and asserts the value survives as "a\r\nb", that no raw CR reaches the output, and that the tail of the value cannot appear as a line of its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…EW-3)
`npm run check:lockfile` failed on Windows with "lockfile-lint found policy
violations" and no output, pointing at supply-chain poisoning that does not
exist. Two causes, both fixed here:
1. The gate passed `node_modules/.bin/lockfile-lint` to execFileSync. That shim
is extensionless and Windows cannot spawn it, so execFileSync raised ENOENT —
which the single catch block reported as a lint violation. The package's own
JS entry point is now resolved from its manifest and run with
`process.execPath`, on every platform.
2. With the tool actually running, `--path` was an absolute Windows path.
lockfile-lint resolves --path as a glob, where a backslash is an escape
character, so `C:\...\package-lock.json` matched nothing and the run hung
walking the tree (>240s, observed) instead of failing. lockfilePath now uses
forward slashes, which are a valid absolute path on Windows and a no-op on
POSIX.
runLockfileLint() now returns a `kind`, so "not-runnable" (ENOENT, a missing
package, or a run that exceeds the new 120s cap) is reported as a runner problem
that says nothing about the lockfile, separately from a real "violation" with
its diagnostics. Both stdout and stderr are kept: lockfile-lint writes its
findings to stderr.
Verified on this Windows host — the gate now passes:
[check-lockfile] OK — ✔ No issues detected
[check-lockfile] OK — workspace lock entries match their manifests
and still rejects a poisoned lockfile: a fixture resolving a package over
http:// exits 1 with "detected invalid protocol for package".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…er (NEW-2) `npm audit --omit=dev --package-lock-only` reported `high: 1` at the repo root: GHSA-7q85-xj36-vmfc, "adm-zip: Uncontrolled memory allocation via the declared uncompressed size (DoS)", affects <0.6.1, reached through the optional chain @huggingface/transformers -> onnxruntime-node -> adm-zip. The register still said "Root production tree: no critical or high advisory" and recorded adm-zip as moderate-only (R-01, GHSA-vwc7-r8mq-g2x9). The remedy was the register's own proposed item 2, and it works. `npm update adm-zip --package-lock-only --ignore-scripts` moved adm-zip 0.6.0 -> 0.6.1 inside the existing root override `^0.6.0`. package.json is unchanged, no package was installed, node_modules was not touched, and the whole diff is three lines of package-lock.json (version, resolved, integrity). A plain `npm install --package-lock-only` was tried first and changed nothing, since ^0.6.0 is already satisfied by 0.6.0. Real npm audit numbers, lockfile only: root prod (--omit=dev) before: {"critical":0,"high":1,"moderate":2,"low":0,"total":3} root prod (--omit=dev) after: {"critical":0,"high":0,"moderate":0,"low":0,"total":0} root full before: {"critical":0,"high":5,"moderate":4,"low":1,"total":10} root full after: {"critical":0,"high":4,"moderate":2,"low":1,"total":7} Both adm-zip advisories are closed; the 7 remaining full-run entries are the dev-only records R-02..R-09. Verified: tests/unit/onnxruntime-single-copy.test.ts 2/2 pass (the transformers/onnxruntime version pair the register warns about is intact) and `node scripts/check/check-lockfile.mjs` is OK. The register now carries R-11 for GHSA-7q85-xj36-vmfc with its real reach — an optional dependency chain, used by onnxruntime-node's install script to unpack its own prebuilt native archive, not reachable from OmniRoute request handling — a dated re-measurement section with the before/after table, and the stale "no critical or high advisory" sentence is marked expired with the reason it went stale (the advisory was published after the 2026-09-14 run) and pointed at what the audit reports now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 19, 2026
…ale claim The verification round found the supply-chain claim had gone stale: a second, higher advisory on adm-zip (GHSA-7q85-xj36-vmfc, high, fixed in 0.6.1) had appeared in the root production tree through the optional @huggingface/transformers -> onnxruntime-node chain, while the register — and the sentence this file carried — still said there was no high advisory there. PR #42 fixed it rather than re-documenting it: a lockfile-only bump to 0.6.1 inside the existing ^0.6.0 override. This commit replaces the assertion with the numbers actually measured on the merged tree, per tree, so the claim can be re-run instead of trusted. Measured with `npm audit --omit=dev --package-lock-only` (isolated env): root production {"info":0,"low":0,"moderate":0,"high":0,"critical":0} electron production {"info":0,"low":0,"moderate":0,"high":1,"critical":0} The remaining high is R-10 (js-yaml 4.3.1), reachable only from the Electron shell's update-check chain — not in the container image, the npm package or the server runtime. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LMPrado-DZ23
pushed a commit
that referenced
this pull request
Sep 19, 2026
The verification round — the three auditors re-checking their own fixes on the final tree — produced two more pull requests, so the release documents have to carry them. CHANGELOG [3.8.54] is dated 2026-09-19, the day the tag is cut, and gains the work from #41 and #42: the cross-origin redirect body leak in the TypeScript SDK, the eight health read paths that were transitioning an OPEN breaker to HALF_OPEN just by being read, the decision lookup now requiring auth when requireLogin is off, the routing-decision build going from 17.0ms/591KiB to 4.6ms/26.5KiB per request at 300 candidates, the adm-zip bump that took the root production audit to zero, and the check:lockfile fix. The two bullets the base tree already carried are still untouched, so the diff stays additive; the 41 i18n mirrors are resynced. EVOLUTION_STATUS now names all five audit pull requests and separates the first audit round from the verification round. Verification (isolated env): check-changelog-integrity exit 0 no base bullets lost check-docs-frontmatter exit 0 check-doc-links exit 0 check-fabricated-docs exit 0 check-docs-sync exit 0 41 locales Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LMPrado-DZ23
pushed a commit
that referenced
this pull request
Sep 19, 2026
Records both rounds of the independent audit in FINAL_THREE_AGENT_REVIEW, and restructures the file by release line so the v3.8.51 record stays intact underneath its own heading instead of being overwritten. Round 1, on 20d5b2c: three auditors in parallel, no access to each other's conclusions. Two HIGH (the decision store bounded only by count, and the webhook wizard leaving an enabled all-events webhook behind on cancel), plus the medium, low and improvement findings, each with where it was fixed. Round 2, the verification round on the final tree 68a00d0: the same three auditors re-checking their own fixes. None was found missing, partial or wrong. All three returned PASS with CRITICAL 0 and HIGH 0. The behaviour evidence that mattered most for this release is recorded: provider selection identical to v3.8.53 over 700 of 700 seeded cases, and previewRoutingDecision calling Math.random zero times over 200 previews with the rotator provably untouched. The findings round 2 raised were fixed, not deferred (#41, #42, #43). The only item accepted without a fix is B-03 / R-10, js-yaml 4.3.1 in the Electron update-check chain — not in the container image, the npm package or the server runtime. AUTONOMOUS_MISSION_STATE moves to CANDIDATE_COMPLETED -> RELEASING, with the phase table, the verification round, the Windows-only gate failures, and a note that the publication evidence has to live in the Release notes because publishing the Release locks this branch. Verification (isolated env): check-doc-links exit 0 172 docs, 1043 internal links check-fabricated-docs exit 0 check-docs-frontmatter exit 0 check-changelog-integrity exit 0 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LMPrado-DZ23
pushed a commit
that referenced
this pull request
Sep 19, 2026
Items 1 and 2 of that section are both applied now — adm-zip 0.6.1 in PR #42 and js-yaml 4.3.2 in this one — so a heading reading "Proposed dependency PR (not applied)" states the opposite of what the section records. Renaming it moves the anchor, which is why it was left alone. But all three references live inside this same file, so there was nothing external to break: the heading and its three links are updated together. check-doc-links exit 0 — 172 docs, 1044 internal links, none broken Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LMPrado-DZ23
pushed a commit
that referenced
this pull request
Sep 19, 2026
The handler had no auth check of its own. It now calls
requireManagementAuth(request, { alwaysRequireAuth: true }) — the pattern
/api/metrics and the routing-decision lookup (finding NEW-4, #42) use —
because error rates and request volume are as sensitive as the
provider/model data /api/metrics protects.
Behaviour BEFORE this commit, measured by driving runAuthzPipeline (the
central middleware; route classified MANAGEMENT / management_api):
- requireLogin=true (+password): anonymous -> 401 from the middleware.
- requireLogin=false: anonymous -> passed through to the
handler, which answered 200 with the full telemetry body — on loopback
AND on a non-loopback Host alike.
Behaviour AFTER: 401 for an anonymous caller in both modes, 403 for a key
without the manage scope, 200 for a dashboard session or a manage-scoped
key. So the change is only visible under requireLogin=false.
KNOWN CONSEQUENCE, flagged for review: under requireLogin=false the
dashboard has no session cookie, so the telemetry half of
dashboard/health/TelemetryCard.tsx will now get a 401 (the card uses
Promise.allSettled, so the health half still renders). The /api/metrics
precedent never hit this because the dashboard does not call metrics.
The A2A health-report skill already tolerates a failed telemetry fetch.
Middleware tier intentionally untouched: the path is NOT added to
ALWAYS_PROTECTED_API_PATHS, so the spec does not claim
x-always-protected; it documents security + 401/403 instead.
Tests:
- core-read-contract: new 401 anonymous, 401 anonymous under
requireLogin=false, 403 under-scoped. Verified failable: dropping
alwaysRequireAuth turns the requireLogin=false test red.
- telemetry-summary-route.test.ts previously called the handler
anonymously; it now authenticates with a manage-scoped key. Every
payload assertion is unchanged.
113 tests across the five suites that drive this handler: 109 pass,
0 fail (4 pre-existing skips). tsc -p tsconfig.typecheck-api.json: 0
errors.
Co-Authored-By: Claude Opus 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.
Closes the five findings raised on the final v3.8.54 tree in the verification round. One concern per commit, each written red-first.
NEW-1 (MEDIUM) — TypeScript SDK replayed the request body to a cross-origin redirect target
Was: the only redirect control in
sdk/typescript/src/client.tswasredirectModeFor(), which fell back toredirect: "follow"unless a custom credential header was present. A307from the configuredbaseUrlhost to a different origin therefore madefetchre-POST the whole chat-completion JSON body — the prompt — to that other origin.fetchstripsAuthorization, so this leaked content, not credentials. There was also no scheme check anywhere insdk/typescript/src/, sohttpstohttpwas followed in cleartext.Now: every request uses
redirect: "manual"and the SDK vets the 3xx itself in the new exportedvetRedirect(), mirroring the Python SDK's_CredentialSafeRedirectHandler:httpsto non-httpsis refused;Refusal raises the SDK's own
OmniRouteErrorwith the new coderedirect_refused. The message is built only from the response status and the Location origin (userinfo dropped), so no request body and no header value can reach it.#executerethrows anOmniRouteErrorfrom the fetch phase instead of folding it intonetworkError, so a refused redirect is never retried.Evidence — before (red),
tests/unit/sdk-typescript-client.test.ts:Adding the direct
vetRedirecttests (mirroring the Pythontest_https_to_http_redirect_is_refused, which calls its handler directly) then made the whole file red on the missing export:ℹ tests 1 / pass 0 / fail 1.After:
ℹ tests 34 / pass 34 / fail 0.New/changed tests: a cross-origin
307is refused and the loopback target records zero requests (asserted on a server that buffers the full body, so we prove the body never arrived); anhttpstohttpredirect is refused; a missing/unparseable Location is refused; userinfo in a Location is never echoed; a same-origin redirect is still followed. The previous test asserted the old, laxer behaviour ("fetch strips Authorization" while the redirect is followed) and now asserts the refusal — strengthened, not weakened. Loopback servers only; no provider calls.Python SDK suite unchanged and still green:
Ran 21 tests ... OK.NEW-4 (LOW) — routing decision lookup was open to anonymous callers when
requireLogin=falsesrc/app/api/omniroute/route/decisions/[id]/route.tscalledrequireManagementAuth(request)without{ alwaysRequireAuth: true }./api/metricspasses it, with the rationale "the payload names providers and models in use"; the decision payload names the same providers and models plus scores, exclusion reasons and the policy version. Now passes the same option, with that rationale in the docstring.Before (red):
After:
ℹ tests 7 / pass 7 / fail 0.The regression test follows the existing
/api/metrics"anonymous callers are rejected" pattern (accepts 401 or 403) and additionally asserts the refusal body does not leak the decision. The two lookups in that file which relied on the implicit anonymous pass now build a management session request, as the metrics tests do.Behaviour note for reviewers: under
requireLogin=falsethe dashboard's decision-lookup panel now needs a management credential, exactly as/api/metricsalready does. That is the intended consequence of the finding.NEW-5 (IMPROVEMENT) —
escapeLabelValuedid not escape carriage returnopen-sse/services/routing/prometheusText.tsescaped backslash, LF and double quote, but left a raw CR in the label value. A parser that splits the exposition on CRLF ends the line at that CR, so a model id carrying one could put the tail of the value on what reads as its own metric line. Now escaped as\ralongside the other control characters.Before (red):
✖ Prometheus rendering emits HELP/TYPE, cumulative buckets and escaped labels—ℹ tests 13 / pass 12 / fail 1.After:
ℹ tests 13 / pass 13 / fail 0.The existing escaping test now also renders a label containing CR LF and asserts it survives as
a\r\nb, that no raw CR reaches the output, and that the tail of the value cannot appear as a line of its own.NEW-2 (MEDIUM) — stale vulnerability register; adm-zip high advisory
(a) The remedy was tried and it works — lockfile only
npm install --package-lock-onlywas tried first and changed nothing (the override^0.6.0is already satisfied by 0.6.0).npm update adm-zip --package-lock-only --ignore-scriptsthen movedadm-zip0.6.0 to 0.6.1 inside the existing override.package.jsonis unchanged, no package was installed,node_moduleswas not touched, and the entire diff is three lines:"node_modules/adm-zip": { - "version": "0.6.0", - "resolved": "https://registry.npmjs.org/adm-zip/-/adm-zip-0.6.0.tgz", - "integrity": "sha512-XleryMhbuksdKtofnWZ9Sk+4CUTbms4Mb/EU32SZwToAyZ5RgVos/ki8n+yr0LWHOGKuakbXTuuYNHLQjhddgg==", + "version": "0.6.1", + "resolved": "https://registry.npmjs.org/adm-zip/-/adm-zip-0.6.1.tgz", + "integrity": "sha512-Xwrja8nx9e5o2N1my4DsKCeKpdrnACyr1wtbPxBDgGzKzKyE9kRtBFA8mWldI+RVlD7CBZNWY/wQ2+ydwOR6kQ==",Real
npm auditJSON summaries,--package-lock-only:npm audit --omit=dev --package-lock-only{"info":0,"low":0,"moderate":2,"high":1,"critical":0,"total":3}{"info":0,"low":0,"moderate":0,"high":0,"critical":0,"total":0}npm audit --package-lock-only{"info":0,"low":1,"moderate":4,"high":5,"critical":0,"total":10}{"info":0,"low":1,"moderate":2,"high":4,"critical":0,"total":7}Both adm-zip advisories (GHSA-7q85-xj36-vmfc high, GHSA-vwc7-r8mq-g2x9 moderate) are closed. Nothing unrelated moved — the 7 remaining full-run entries are the dev-only records R-02 through R-09, so the bump was kept, not reverted.
Verified afterwards:
tests/unit/onnxruntime-single-copy.test.ts2/2 pass (the@huggingface/transformers/onnxruntime-nodeversion-pair contract the register warns about is intact) andnode scripts/check/check-lockfile.mjsis OK.(b) Register corrected either way
docs/security/VULNERABILITY_REGISTER.mdnow has:<0.6.1, first fixed 0.6.1) with its real reach: an optional dependency chain, used byonnxruntime-node's install script to unpack its own prebuilt native archive (linux/x64 and CUDA-absent only), not reachable from OmniRoute request handling — an install-time availability risk, never a running gateway;Docs gates:
check-docs-frontmatterOK,check-doc-linksPASS (1041 internal links),check-fabricated-docsOK.NEW-3 (LOW) —
npm run check:lockfilefailed on Windows and blamed the wrong thingBefore (reproduced on this host):
Empty output, pointing at supply-chain poisoning that does not exist. Two causes, both fixed:
node_modules/.bin/lockfile-linttoexecFileSync, which Windows cannot spawn —ENOENTfell into the single catch block and was reported as a lint violation. The package's JS entry point is now resolved from its own manifest and run withprocess.execPath, on every platform.--pathwas an absolute Windows path, and lockfile-lint resolves--pathas a glob, where a backslash is an escape character.C:\...\package-lock.jsonmatched nothing and the run hung walking the tree — measured at over 240s before being killed, then reported as a violation with empty output.lockfilePathnow uses forward slashes (a valid absolute path on Windows, a no-op on POSIX).runLockfileLint()now returns akind, so "not-runnable" (ENOENT, missing package, or a run exceeding the new 120s cap) is reported as a runner problem that says nothing about the lockfile, distinct from a real "violation" with its diagnostics. Both stdout and stderr are kept — lockfile-lint writes findings to stderr, which was part of the "empty output".After (real output, this Windows host):
Still catches poisoning: a fixture lockfile resolving a package over
http://exits 1 withdetected invalid protocol for package: evil@1.0.0 / expected: https: / actual: http:.Tests added to
tests/unit/build/check-lockfile.test.ts(red first —ℹ tests 1 / pass 0 / fail 1on the missing exports): the command isprocess.execPathplus an existing.jsentry point;lockfilePathcontains no backslash and still resolves on disk;ENOENTmaps tonot-runnable;ETIMEDOUTmaps tonot-runnable; a non-zero exit with diagnostics maps toviolationwith stdout preserved. After:ℹ tests 28 / pass 28 / fail 0.Verification
All commands run with
DATA_DIR,HOME,USERPROFILEandAPPDATApointed at fresh temp dirs;C:/Users/zodyp/.omniroutewas never touched. No real AI provider calls — loopback servers only.Typechecks (
node node_modules/typescript/bin/tsc --pretty false --noEmit -p <cfg>; thecheck-*-typecheck.mjswrappers fail on Windows withspawnSync npx.cmd EINVAL):ESLint —
npx eslint --max-warnings=0 --suppressions-location config/quality/eslint-suppressions.json --pass-on-unpruned-suppressions --no-warn-ignored <changed files>givesESLINT_EXIT=0.Prettier — clean on all 11 changed source/doc files. Checked against the committed blobs, because
core.autocrlf=truerewrites the Windows working tree to CRLF while the committed content is LF (confirmed:git show HEAD:<file> | file -reports no CRLF).PRETTIER_ALL_EXIT=0.Tests — the touched suites, together:
Python SDK:
Ran 21 tests in 27.727s — OK.Gates:
One red gate, pre-existing and untouched by this PR
src/lib/db/core.tsis not in this PR's diff. Confirmed pre-existing by stashing every change and re-running on the pristine base — byte-identical output. Not fixed here: it is a separate concern and shrinking that file is out of scope for this PR.Also worth flagging, both environmental rather than code:
tests/unit/ui/routing-decision-lookup.test.tsxcannot run on this host (the vitest pool times out starting a worker —[vitest-pool]: Failed to start threads worker). That test stubs globalfetchand never reaches the route handler, so NEW-4 cannot affect it, but it was not executed here.🤖 Generated with Claude Code