fix(security): close a guardrail bypass, a repo-scope auth gap, and fail-open telemetry ingest#9112
Merged
Merged
Conversation
…ail-open telemetry ingest Three security fixes, each applied in the form that prevents the whole class rather than the one instance. #9017 (GHSA-rjhf-3xrf-j72w) -- githubPaginatedList stops silently at its 10-page x 100-file cap and returns the TRUNCATED file list with no truncation flag. isGuardrailHit failed safe only on an EMPTY list, so a truncated non-empty list that happened to omit the guarded file read as "no guardrail hit": a PR padded past 1000 files could hide a .github/workflows/** or config edit from the hard guardrail and auto-merge it -- arbitrary CI execution, the highest-value target in the system. A list at/above the cap is now UNVERIFIABLE and fails safe, which also serves as the hard changed-file ceiling the advisory asks for (a legitimate contributor PR here is never 1000 files). Putting the rule INSIDE isGuardrailHit rather than threading a truncation flag through the fetch layer means no future caller can obtain paths some other way and forget to pass it. The fetch layer additionally warns so the truncation is visible, matching what the GraphQL supplement paths already do at their caps. #9045 -- GET /v1/repos/:owner/:repo/pulls/:number/maintainer-packet gated only on requireStaticProtectedApiToken, which admits ANY static identity including the shared, end-user-obtainable `mcp` token, and never checked isMcpReadRepoAllowed. Its three sibling repo-scoped routes each hand-rolled that check; this one simply omitted it -- so the HTTP surface granted what the MCP surface denied for the same token, exposing every issue, PR, file, review, and check summary for ANY repo. MCP_READ_REPO_ALLOWLIST is fail-closed by default (unset ⇒ deny all), so the intended posture was "deny everything" while this route allowed everything. Rather than adding a fourth copy of the check, it is folded INTO the gate via an optional repoFullName argument and the three hand-rolled copies are removed; a source-level drift test fails if any repo-scoped route calls the gate without it. #9046 -- both telemetry-collector ingest endpoints returned true for an UNSET token (the shipped default), so anyone with network access could POST batches, and Orb's batches feed the PUBLISHED accuracy numbers. Network isolation was doing all the work, which the planned hosted Orb removes. They were separate near-identical copies, which is exactly how AMS ended up both fail-open AND missing from the strict rate class; they are now ONE shared isAuthorizedIngest helper (per-product secret still passed in, so credentials rotate independently), and /v1/ams/ingest joins /v1/orb/ingest in the strict class. Closes #9017 Closes #9045 Closes #9046 Not included: #9046's suggestion to move /metrics behind a separate admin port/interface is a deployment-topology decision for the operator, not a code change I should make unilaterally -- left on the issue for a follow-up. Tests: 3 guardrail regressions (fails safe at the cap, unchanged just under it, still permissive with no guardrails configured), a fetch-layer truncation-warning test, 6 route-security tests (all four repo-scoped routes deny an unallowlisted mcp token and admit an allowlisted one; operator tokens stay unscoped; both ingest endpoints reject an unset token, reject a wrong bearer, and accept a correct one), and a structural drift guard verified to fail when the original #9045 omission is reintroduced. 100% line+branch coverage on all 83 added lines; 987/987 across 14 suites; all 9 drift gates green.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | fca9f55 | Commit Preview URL Branch Preview URL |
Jul 26 2026, 08:27 PM |
loopover-orb
Bot
added
the
gittensor:bug
❌ 13 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
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
1 participant
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.
Summary
Three security fixes. Each is applied in the form that prevents the class, not just the instance — every one of these was a single omission in a place where three sibling call sites got it right.
#9017 (GHSA-rjhf-3xrf-j72w) — guardrail bypass via silent file-list truncation.
githubPaginatedListstops at its 10×100 cap and returns the truncated list with no truncation flag.isGuardrailHitfailed safe only on an empty list, so a truncated non-empty list that happened to omit the guarded file read as "no guardrail hit" — a PR padded past 1000 files could hide a.github/workflows/**edit from the hard guardrail and auto-merge it (arbitrary CI execution).A list at/above the cap is now unverifiable → fails safe, which doubles as the hard changed-file ceiling the advisory also asks for. Deliberately placed inside
isGuardrailHitrather than threading a truncation flag through the fetch layer: that way no future caller can obtain paths some other way and forget to pass the flag. The fetch layer additionally warns so the truncation is visible rather than silent.#9045 — maintainer-packet route skipped the MCP repo allowlist. It gated only on
requireStaticProtectedApiToken, which admits any static identity including the shared, end-user-obtainablemcptoken, and never calledisMcpReadRepoAllowed. Its three siblings each hand-rolled that check; this one omitted it — so the HTTP surface granted what the MCP surface denied for the same token, exposing every issue, PR, file, review and check summary for any repo.MCP_READ_REPO_ALLOWLISTis fail-closed by default (unset ⇒ deny all), so the intended posture was "deny everything" while this route allowed everything.Rather than adding a fourth copy, the check is folded into the gate (optional
repoFullName) and the three hand-rolled copies removed. A source-level drift test fails if any repo-scoped route calls the gate without it — verified by reintroducing the original omission and confirming the test names the exact route.#9046 — telemetry ingest fails open. Both collectors returned
truefor an unset token (the shipped default), so anyone with network access could POST batches — and Orb's batches feed the published accuracy numbers. Network isolation was doing all the work, which the planned hosted Orb removes. They were separate near-identical copies, which is precisely how AMS ended up both fail-open and missing from the strict rate class. Now one sharedisAuthorizedIngest(per-product secret still passed in, so credentials rotate independently), and/v1/ams/ingestjoins/v1/orb/ingestin the strict rate class.Closes #9017
Closes #9045
Closes #9046
Standardization
Per the AMS/ORB consistency goal, the two duplicated ingest guards are now a single shared helper — the duplication is what let AMS silently drift on two separate axes. Any future collector route gets the correct posture by calling it instead of hand-rolling one.
Deliberately not included
#9046 also suggests moving
/metricsbehind a separate admin port/interface. That's a deployment-topology decision for the operator rather than a code change I should make unilaterally, so I left it on the issue for a follow-up. The metrics-render caching suggestion is likewise deferred — it's a perf change, not a security boundary.Test plan
mcptoken and admit an allowlisted one (the guard scopes, it doesn't blanket-deny); operator tokens stay unscoped; both ingest endpoints reject an unset token, reject a wrong bearer, and accept a correct one/v1/repos/:owner/:repo/pulls/:number/maintainer-packetby nametsc --noEmitclean; 987/987 across 14 suites; all 9 drift gates green