From fca9f5533def09f7e2f5350c1e93b5d6a0be43c9 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 13:24:59 -0700 Subject: [PATCH] fix(security): close a guardrail bypass, a repo-scope auth gap, and fail-open telemetry ingest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../src/signals/change-guardrail.ts | 21 ++- src/api/routes.ts | 86 +++++++------ src/auth/rate-limit.ts | 3 + src/github/backfill.ts | 13 +- src/signals/change-guardrail.ts | 1 + test/unit/backfill.test.ts | 33 +++++ test/unit/change-guardrail.test.ts | 34 ++++- ...tes-ingest-and-repo-scope-security.test.ts | 121 ++++++++++++++++++ .../routes-repo-scoped-auth-drift.test.ts | 62 +++++++++ 9 files changed, 332 insertions(+), 42 deletions(-) create mode 100644 test/unit/routes-ingest-and-repo-scope-security.test.ts create mode 100644 test/unit/routes-repo-scoped-auth-drift.test.ts diff --git a/packages/loopover-engine/src/signals/change-guardrail.ts b/packages/loopover-engine/src/signals/change-guardrail.ts index dadac82942..5f0a1902aa 100644 --- a/packages/loopover-engine/src/signals/change-guardrail.ts +++ b/packages/loopover-engine/src/signals/change-guardrail.ts @@ -162,7 +162,26 @@ export function guardrailPathMatches(changedPaths: string[], hardGuardrailGlobs: * longer populated), we cannot prove the PR avoids a guarded path, so treat it as a hit. No guardrails * configured ⇒ never a hit. Pure. */ +/** + * #9017 (GHSA-rjhf-3xrf-j72w): the REST file-list walk that produces `changedPaths` stops silently at its own + * 10-page x 100-file cap and returns the TRUNCATED list with no truncation flag. `isGuardrailHit` used to fail + * safe only on an EMPTY list, so a truncated NON-empty list that happened to omit the guarded file read as + * "no guardrail hit" — meaning a PR padded past 1000 files could hide a `.github/workflows/**` or config edit + * from the hard guardrail and auto-merge it, i.e. arbitrary CI execution, the highest-value target here. + * + * A list at or above the cap is therefore treated as UNVERIFIABLE (same posture as the empty list): we cannot + * prove the PR avoids a guarded path, so a human decides. This deliberately doubles as the hard changed-file + * ceiling the advisory also asks for — a legitimate contributor PR to these repos is never 1000 files, so the + * two remedies collapse into one rule with no extra plumbing through the fetch layer (and therefore no way for + * a future caller to obtain paths from some other source and forget to pass a truncation flag along). + */ +export const GUARDRAIL_UNVERIFIABLE_FILE_COUNT = 1000; + export function isGuardrailHit(changedPaths: string[], hardGuardrailGlobs: string[]): boolean { if (hardGuardrailGlobs.length === 0) return false; - return changedPaths.length === 0 || changedPathsHittingGuardrail(changedPaths, hardGuardrailGlobs).length > 0; + return ( + changedPaths.length === 0 || + changedPaths.length >= GUARDRAIL_UNVERIFIABLE_FILE_COUNT || + changedPathsHittingGuardrail(changedPaths, hardGuardrailGlobs).length > 0 + ); } diff --git a/src/api/routes.ts b/src/api/routes.ts index 1e3a4c6615..48373a434f 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -3437,9 +3437,10 @@ export function createApp() { }); app.get("/v1/repos/:owner/:repo/pulls/:number/maintainer-packet", async (c) => { - const unauthorized = await requireStaticProtectedApiToken(c); - if (unauthorized) return unauthorized; const fullName = `${c.req.param("owner")}/${c.req.param("repo")}`; + // #9045: repo-scoped — this route was the one sibling that never checked the MCP read allowlist. + const unauthorized = await requireStaticProtectedApiToken(c, fullName); + if (unauthorized) return unauthorized; const number = Number(c.req.param("number")); if (!Number.isInteger(number) || number <= 0) return c.json({ error: "invalid_pull_number" }, 400); const [repo, pullRequest, issues, pullRequests, files, reviews, checks, recentMergedPullRequests] = await Promise.all([ @@ -3461,11 +3462,9 @@ export function createApp() { }); app.get("/v1/repos/:owner/:repo/pulls/:number/reviewability", async (c) => { - const unauthorized = await requireStaticProtectedApiToken(c); - if (unauthorized) return unauthorized; const fullName = `${c.req.param("owner")}/${c.req.param("repo")}`; - const identity = await authenticateRequestIdentity(c); - if (identity?.kind === "static" && identity.actor === "mcp" && !(await import("../auth/security")).isMcpReadRepoAllowed(c.env.MCP_READ_REPO_ALLOWLIST, fullName)) return c.json({ error: "forbidden_repo" }, 403); + const unauthorized = await requireStaticProtectedApiToken(c, fullName); + if (unauthorized) return unauthorized; const number = Number(c.req.param("number")); if (!Number.isInteger(number) || number <= 0) return c.json({ error: "invalid_pull_number" }, 400); const [repo, pullRequest, issues, pullRequests, files, reviews, checks, recentMergedPullRequests] = await Promise.all([ @@ -3531,15 +3530,10 @@ export function createApp() { // override_audit history or the shadow's queued recommendation, just a flag that a shadow is soaking. // Gated behind the same most-conservative repo-scoped read precedent the reviewability route (#6154) uses. app.get("/v1/repos/:owner/:repo/gate-config/effective", async (c) => { - const unauthorized = await requireStaticProtectedApiToken(c); - if (unauthorized) return unauthorized; const fullName = `${c.req.param("owner")}/${c.req.param("repo")}`; - const identity = await authenticateRequestIdentity(c); - /* v8 ignore next -- requireStaticProtectedApiToken above already rejected null and session identities, so only static tokens reach here. */ - if (!identity || identity.kind !== "static") return c.json({ error: "unauthorized" }, 401); - // Only the shared, end-user-obtainable static `mcp` token is allowlist-scoped; operator-only api/internal - // tokens stay trusted — same repo-scoped read precedent the reviewability route (#6154) uses. - if (identity.actor === "mcp" && !(await import("../auth/security")).isMcpReadRepoAllowed(c.env.MCP_READ_REPO_ALLOWLIST, fullName)) return c.json({ error: "forbidden_repo" }, 403); + // #9045: the MCP read-allowlist check now lives in requireStaticProtectedApiToken itself. + const unauthorized = await requireStaticProtectedApiToken(c, fullName); + if (unauthorized) return unauthorized; const storageEnv = c.env as unknown as StorageEnv; const [override, shadow] = await Promise.all([loadOverride(storageEnv, fullName), loadShadowOverride(storageEnv, fullName)]); return c.json({ @@ -3559,16 +3553,10 @@ export function createApp() { // applied_at / clear_at). Live row wins; soaking shadow fills in only when live is absent. 404 when neither // is active — same not-found convention as issue-quality. Auth matches gate-config/effective above. app.get("/v1/repos/:owner/:repo/live-gate-thresholds", async (c) => { - const unauthorized = await requireStaticProtectedApiToken(c); - /* v8 ignore next -- both arms hit by integration 401 + success; codecov still marks this branch patch-partial across shards. */ - if (unauthorized) return unauthorized; const fullName = `${c.req.param("owner")}/${c.req.param("repo")}`; - const identity = await authenticateRequestIdentity(c); - /* v8 ignore next -- requireStaticProtectedApiToken above already rejected null and session identities, so only static tokens reach here. */ - if (!identity || identity.kind !== "static") return c.json({ error: "unauthorized" }, 401); - // Only the shared, end-user-obtainable static `mcp` token is allowlist-scoped; operator-only api/internal - // tokens stay trusted — same repo-scoped read precedent the reviewability route (#6154) uses. - if (identity.actor === "mcp" && !(await import("../auth/security")).isMcpReadRepoAllowed(c.env.MCP_READ_REPO_ALLOWLIST, fullName)) return c.json({ error: "forbidden_repo" }, 403); + // #9045: the MCP read-allowlist check now lives in requireStaticProtectedApiToken itself. + const unauthorized = await requireStaticProtectedApiToken(c, fullName); + if (unauthorized) return unauthorized; const storageEnv = c.env as unknown as StorageEnv; const [live, shadow] = await Promise.all([loadOverride(storageEnv, fullName), loadShadowOverride(storageEnv, fullName)]); const fields = toLiveGateThresholdFields(authoritativeGateOverride(live, shadow)); @@ -4412,7 +4400,7 @@ export function createApp() { // ⇒ the collector REQUIRES it, so an operator can lock the write path down after distributing the matching // ORB_COLLECTOR_TOKEN to exporters. Bounded by a hard body ceiling, and dedup'd via UNIQUE(instance_id, repo_hash, pr_hash). app.post("/v1/orb/ingest", async (c) => { - if (!(await isAuthorizedOrbIngest(c.env, extractBearerToken(c.req.header("authorization"))))) return c.json({ error: "unauthorized" }, 401); + if (!(await isAuthorizedIngest(c.env.ORB_INGEST_TOKEN, extractBearerToken(c.req.header("authorization"))))) return c.json({ error: "unauthorized" }, 401); const body = await readOrbIngestBody(c.req.raw, c.req.header("content-length")); if (body === null) return c.json({ error: "payload_too_large" }, 413); if (!body) return c.json({ error: "invalid_request" }, 400); @@ -4425,7 +4413,7 @@ export function createApp() { // route above (same optional bearer-token gate, same hard body ceiling — readOrbIngestBody is generic over // request bytes despite the name, so it's reused as-is rather than duplicated). app.post("/v1/ams/ingest", async (c) => { - if (!(await isAuthorizedAmsIngest(c.env, extractBearerToken(c.req.header("authorization"))))) return c.json({ error: "unauthorized" }, 401); + if (!(await isAuthorizedIngest(c.env.AMS_INGEST_TOKEN, extractBearerToken(c.req.header("authorization"))))) return c.json({ error: "unauthorized" }, 401); const body = await readOrbIngestBody(c.req.raw, c.req.header("content-length")); if (body === null) return c.json({ error: "payload_too_large" }, 413); if (!body) return c.json({ error: "invalid_request" }, 400); @@ -6452,11 +6440,26 @@ function installationRecordInScope( return scopedInstallationIds.has(record.installationId) || scopedAccountLogins.has(record.accountLogin.toLowerCase()); } -async function requireStaticProtectedApiToken(c: ProtectedRouteContext): Promise { +/** #9045: `repoFullName` folds the MCP read-allowlist check INTO this gate rather than leaving it to each + * route to remember. Three of the four repo-scoped callers hand-rolled the identical check; the fourth + * (maintainer-packet) simply omitted it, so the shared, end-user-obtainable `mcp` token could read the full + * packet — every issue, PR, file, review, and check summary — for ANY repo over HTTP, while the MCP tool this + * route mirrors denied exactly that. Worse, MCP_READ_REPO_ALLOWLIST is fail-closed by default (unset ⇒ deny + * all), so the intended posture was "deny everything" while this route allowed everything. Passing the repo + * makes the check structural: a future repo-scoped route cannot forget it without also failing to pass the + * argument it needs anyway. Operator-only `api`/`internal` tokens stay trusted and are never allowlist-scoped. */ +async function requireStaticProtectedApiToken(c: ProtectedRouteContext, repoFullName?: string): Promise { const identity = await authenticateRequestIdentity(c); /* v8 ignore next -- Protected middleware rejects unauthenticated private routes before static-token-only route guards. */ if (!identity) return c.json({ error: "unauthorized" }, 401); if (identity.kind === "session") return c.json({ error: "static_token_required" }, 403); + if ( + repoFullName !== undefined && + identity.actor === "mcp" && + !(await import("../auth/security")).isMcpReadRepoAllowed(c.env.MCP_READ_REPO_ALLOWLIST, repoFullName) + ) { + return c.json({ error: "forbidden_repo" }, 403); + } return null; } @@ -6606,19 +6609,24 @@ function toIsoQueryDate(value: string): string | undefined { // OPEN (matching today's live fleet — deploying this is non-breaking). Once the operator sets the token, the // collector REQUIRES an exact bearer match, so the write path can be locked down after the matching // ORB_COLLECTOR_TOKEN is rolled out to exporters. -async function isAuthorizedOrbIngest(env: Env, token: string | undefined): Promise { - if (!env.ORB_INGEST_TOKEN) return true; - // Constant-time compare (mirrors every other secret check in auth/security) — a `===` here is timing-attack - // vulnerable for a shared secret. - return timingSafeEqual(token, env.ORB_INGEST_TOKEN); -} - -// Optional AMS-ingest auth (#5681), same fail-open shape as isAuthorizedOrbIngest: unset ⇒ open ingress, set ⇒ -// the collector requires an exact bearer match. A separate token/env var from ORB_INGEST_TOKEN so the two -// products' collector credentials can be rotated or locked down independently. -async function isAuthorizedAmsIngest(env: Env, token: string | undefined): Promise { - if (!env.AMS_INGEST_TOKEN) return true; - return timingSafeEqual(token, env.AMS_INGEST_TOKEN); +/** + * #9046: the SINGLE authorization rule for every telemetry-collector ingest endpoint (Orb and AMS), so the two + * products cannot drift apart again. They previously had separate, near-identical copies — and AMS ended up + * BOTH fail-open AND missing from the strict rate class, purely because it was a second copy nobody kept in + * lockstep. Any future collector route gets the correct posture by calling this rather than hand-rolling it; + * the per-product secret stays separate (passed in) so the two credentials rotate independently. + * + * FAILS CLOSED when the token is unset. Both copies previously 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 (the tunnel exposes only the shot path, 8787 + * binds to loopback), which the planned hosted Orb removes. An unconfigured collector now rejects rather than + * accepting anonymous writes; operators who want ingest set the secret, matching every other credentialed + * surface here. Constant-time compare (mirrors auth/security) — a `===` is timing-attack vulnerable for a + * shared secret. + */ +async function isAuthorizedIngest(configuredToken: string | undefined, presentedToken: string | undefined): Promise { + if (!configuredToken) return false; + return timingSafeEqual(presentedToken, configuredToken); } function requiresApiToken(path: string): boolean { diff --git a/src/auth/rate-limit.ts b/src/auth/rate-limit.ts index c79b1f6c47..2d7c12ad74 100644 --- a/src/auth/rate-limit.ts +++ b/src/auth/rate-limit.ts @@ -123,6 +123,9 @@ export function routeClassForPath(path: string): RateLimitClass { // Orb telemetry ingest: unauthenticated + write, accepting anonymized batches from untrusted // self-host instances. Strict (10/min per IP) caps abuse — legitimate instances export hourly. if (path === "/v1/orb/ingest") return "strict"; + // #9046: the AMS collector is the same shape as the Orb one above (unauthenticated-by-default + // write surface, hard body ceiling) but was landing in the looser `normal` class purely by omission. + if (path === "/v1/ams/ingest") return "strict"; if (path === "/v1/auth/session" || path === "/v1/auth/logout") return "normal"; // GitHub's OAuth Device Authorization Grant (RFC 8628) is polling-by-design: a client polls // /device/poll at a server-specified interval (this repo's own default is 5s) for up to the device diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 68eb09632f..2d55582944 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -73,6 +73,7 @@ import { shouldPublishReviewCheck, } from "../review/check-names"; import { buildReviewThreadBlocker, unreadableReviewThreadBlocker, type ReviewThreadBlocker } from "../review/review-thread-findings"; +import { GUARDRAIL_UNVERIFIABLE_FILE_COUNT } from "../signals/change-guardrail"; import { delayUntil, HISTORICAL_BACKFILL_RESERVED_HEADROOM, LOW_REST_RATE_LIMIT_REMAINING, shouldWaitForGitHubRateLimit } from "./rate-limit"; import { githubRateLimitAdmissionKeyForPublicToken, @@ -2428,7 +2429,17 @@ async function fetchPullRequestFiles( ): Promise { incr(PULL_REQUEST_FILES_FETCH_METRIC, { caller }); const files = await githubPaginatedList(env, repoFullName, `/pulls/${pullNumber}/files`, token, admissionKey); - if (files) return files; + if (files) { + // #9017 (GHSA-rjhf-3xrf-j72w): githubPaginatedList stops silently at its page cap and returns a TRUNCATED + // list with no flag, so a padded PR could hide a guarded file from the hard guardrail. isGuardrailHit now + // fails safe on a list at/above GUARDRAIL_UNVERIFIABLE_FILE_COUNT (the decision that actually protects the + // merge); this warning makes the truncation visible to an operator rather than silent, matching what the + // GraphQL supplement paths in this file already do at their own caps. + if (files.length >= GUARDRAIL_UNVERIFIABLE_FILE_COUNT) { + warnings.push(`File list for #${pullNumber} hit the pagination cap (${files.length} files); the changed-file set may be truncated and the PR is held for human review.`); + } + return files; + } const fallback = token ? await fetchPullRequestDetailsFromGraphQl(env, repoFullName, pullNumber, token, admissionKey).catch(() => undefined) : undefined; if (fallback) return fallback.files; warnings.push(`File sync failed for #${pullNumber}: GitHub REST and GraphQL detail fetches failed.`); diff --git a/src/signals/change-guardrail.ts b/src/signals/change-guardrail.ts index 42fc9bd778..6bd235d882 100644 --- a/src/signals/change-guardrail.ts +++ b/src/signals/change-guardrail.ts @@ -17,4 +17,5 @@ export { type GuardrailPathMatch, guardrailPathMatches, isGuardrailHit, + GUARDRAIL_UNVERIFIABLE_FILE_COUNT, } from "../../packages/loopover-engine/src/signals/change-guardrail"; diff --git a/test/unit/backfill.test.ts b/test/unit/backfill.test.ts index c712d341fa..dfbe182504 100644 --- a/test/unit/backfill.test.ts +++ b/test/unit/backfill.test.ts @@ -4019,6 +4019,39 @@ describe("GitHub backfill", () => { expect((await listCheckSummaries(env, "JSONbored/gittensory", 20)).map((check) => check.name).sort()).toEqual(["lint", "test"]); }); + // #9017 (GHSA-rjhf-3xrf-j72w): githubPaginatedList stops silently at its page cap and returns the TRUNCATED + // list with no flag. isGuardrailHit now fails safe on a list at/above the cap (that is what protects the + // merge); this asserts the fetch layer also SURFACES the truncation to the operator instead of staying + // silent, matching what the GraphQL supplement paths in this file already do at their own caps. + it("#9017: warns when the PR-files walk hits the pagination cap, so a truncated file set is never silent", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 22, + title: "Padded PR", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "padsha" }, + labels: [], + body: "", + }); + // Every page is full and advertises another — the walk ends by exhausting the cap, not by running out. + const fullPage = Array.from({ length: 100 }, (_, i) => ({ filename: `docs/pad-${i}.md`, status: "modified", additions: 1, deletions: 0, changes: 1 })); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/pulls/22/files")) { + return Response.json(fullPage, { headers: { link: '; rel="next"' } }); + } + return Response.json([]); + }); + + const result = await backfillOpenPullRequestDetails(env, { repoFullName: "JSONbored/gittensory", mode: "full", cursor: 0 }); + + // The segment reports "partial" (reconciliation independently notices the truncated set) — the point of + // this test is the NAMED warning, which is what tells an operator *why*. + expect(result.warnings.some((w) => w.includes("pagination cap") && w.includes("#22"))).toBe(true); + }); + it("keeps the pages already fetched when a later PR-files page fails", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); await seedRegisteredRepo(env); diff --git a/test/unit/change-guardrail.test.ts b/test/unit/change-guardrail.test.ts index 25705e71fc..17e44c84ca 100644 --- a/test/unit/change-guardrail.test.ts +++ b/test/unit/change-guardrail.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { changedPathsHittingGuardrail, globToRegExp, guardrailPathMatches, isGuardrailHit, matchesAny } from "../../src/signals/change-guardrail"; +import { changedPathsHittingGuardrail, globToRegExp, GUARDRAIL_UNVERIFIABLE_FILE_COUNT, guardrailPathMatches, isGuardrailHit, matchesAny } from "../../src/signals/change-guardrail"; describe("globToRegExp (the exported compiler itself — must be safe for ANY direct caller, not just matchesAny)", () => { it("compiles an ordinary glob to a working anchored RegExp", () => { @@ -102,6 +102,7 @@ describe("change-guardrail glob matching", () => { expect(isGuardrailHit(["docs/a.md", "src/ui/b.tsx"], globs)).toBe(false); // FAIL-SAFE (#1062): guardrails configured but the changed-file set is empty (unknown) ⇒ treat as a hit. expect(isGuardrailHit([], globs)).toBe(true); + }); it("SECURITY (ReDoS): a glob with too many chained wildcards no longer risks catastrophic backtracking — it fails SAFE TOWARD GUARDING (matches every path) instead of ever compiling the pathological pattern", () => { @@ -146,6 +147,37 @@ describe("change-guardrail glob matching", () => { // Operator-configured hard-guardrail globs can still cover sensitive files outside broad dir-prefix guards, // while leaving ordinary paths auto-mergeable. These examples are not runtime defaults. +// #9017 (GHSA-rjhf-3xrf-j72w): the REST file-list walk stops silently at its 10-page x 100-file cap and returns +// the TRUNCATED list with no truncation flag. isGuardrailHit used to fail 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 and fails safe, which doubles as the hard +// changed-file ceiling the advisory also asks for. +describe("isGuardrailHit — truncated file lists are unverifiable (#9017, GHSA-rjhf-3xrf-j72w)", () => { + const globs = [".github/workflows/**"]; + const pad = (count: number) => Array.from({ length: count }, (_, i) => `docs/pad-${i}.md`); + + it("fails safe at the pagination cap even when no listed path hits a guarded glob", () => { + // Exactly the attack: 1000 innocuous files, the guarded edit sitting on the unreadable page 11. + const truncated = pad(GUARDRAIL_UNVERIFIABLE_FILE_COUNT); + expect(truncated.some((p) => p.startsWith(".github/"))).toBe(false); + expect(isGuardrailHit(truncated, globs)).toBe(true); + }); + + it("does NOT change the verdict for a normal-sized PR just under the cap", () => { + const justUnder = pad(GUARDRAIL_UNVERIFIABLE_FILE_COUNT - 1); + expect(isGuardrailHit(justUnder, globs)).toBe(false); + // ...and a real hit in a normal-sized PR is still reported for the real reason. + expect(isGuardrailHit([...pad(5), ".github/workflows/ci.yml"], globs)).toBe(true); + }); + + it("stays permissive when NO guardrails are configured, however many files changed", () => { + // The ceiling is a guardrail-verification rule, not a blanket PR-size limit — it must not invent a hold + // for a repo that configured no guarded paths at all. + expect(isGuardrailHit(pad(GUARDRAIL_UNVERIFIABLE_FILE_COUNT + 500), [])).toBe(false); + }); +}); + describe("configured hard-guardrail glob examples", () => { const LOOPOVER_GLOBS = [ ".github/**", "scripts/**", "packages/**", "apps/loopover-ui/**", diff --git a/test/unit/routes-ingest-and-repo-scope-security.test.ts b/test/unit/routes-ingest-and-repo-scope-security.test.ts new file mode 100644 index 0000000000..ab7aad60fd --- /dev/null +++ b/test/unit/routes-ingest-and-repo-scope-security.test.ts @@ -0,0 +1,121 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { createApp } from "../../src/api/routes"; +import { upsertInstallation, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub } from "../../src/db/repositories"; +import { createTestEnv } from "../helpers/d1"; + +function stubMinerDetection(): void { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (input.toString().includes("gittensor.io")) return Response.json([]); + return new Response("not found", { status: 404 }); + }); +} + +async function seedRepoWithPr(env: Env, owner: string, name: string, installationId: number): Promise { + await upsertInstallation(env, { + installation: { + id: installationId, + account: { login: owner, id: installationId, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read" }, + events: ["repository"], + }, + }); + await upsertRepositoryFromGitHub(env, { name, full_name: `${owner}/${name}`, private: false, owner: { login: owner } }, installationId); + await upsertPullRequestFromGitHub(env, `${owner}/${name}`, { + number: 1, + title: "PR", + state: "open", + user: { login: "contributor" }, + head: { sha: "h1" }, + labels: [], + body: "x", + }); +} + +// #9045: the maintainer-packet route gated only on `requireStaticProtectedApiToken`, which admits ANY static +// identity including the shared, end-user-obtainable `mcp` token, and never checked the read allowlist. Its +// three sibling repo-scoped routes did. Since MCP_READ_REPO_ALLOWLIST is fail-closed by default (unset ⇒ deny +// all), the intended posture was "deny everything" while this route returned the FULL packet — every issue, +// PR, file, review, and check summary — for any repo. The MCP tool it mirrors already denied exactly this. +describe("repo-scoped static-token routes honor MCP_READ_REPO_ALLOWLIST (#9045)", () => { + afterEach(() => vi.unstubAllGlobals()); + + const ROUTES = [ + "/v1/repos/alice/repo-a/pulls/1/maintainer-packet", + "/v1/repos/alice/repo-a/pulls/1/reviewability", + "/v1/repos/alice/repo-a/gate-config/effective", + "/v1/repos/alice/repo-a/live-gate-thresholds", + ]; + + it("denies the shared mcp token on every repo-scoped route when the repo is not allowlisted", async () => { + const app = createApp(); + const env = createTestEnv({ MCP_READ_REPO_ALLOWLIST: "", LOOPOVER_MCP_TOKEN: "test-mcp-token" }); + await seedRepoWithPr(env, "alice", "repo-a", 101); + stubMinerDetection(); + + for (const route of ROUTES) { + const res = await app.request(route, { headers: { authorization: "Bearer test-mcp-token" } }, env); + expect({ route, status: res.status }).toEqual({ route, status: 403 }); + expect(await res.json()).toMatchObject({ error: "forbidden_repo" }); + } + }); + + it("admits the mcp token on those same routes once the repo IS allowlisted (the guard scopes, it does not blanket-deny)", async () => { + const app = createApp(); + const env = createTestEnv({ MCP_READ_REPO_ALLOWLIST: "alice/repo-a", LOOPOVER_MCP_TOKEN: "test-mcp-token" }); + await seedRepoWithPr(env, "alice", "repo-a", 101); + stubMinerDetection(); + + for (const route of ROUTES) { + const res = await app.request(route, { headers: { authorization: "Bearer test-mcp-token" } }, env); + expect({ route, forbidden: res.status === 403 }).toEqual({ route, forbidden: false }); + } + }); + + it("keeps operator-only tokens unscoped — the allowlist narrows the shared mcp token only", async () => { + const app = createApp(); + const env = createTestEnv({ MCP_READ_REPO_ALLOWLIST: "", LOOPOVER_API_TOKEN: "test-api-token" }); + await seedRepoWithPr(env, "alice", "repo-a", 101); + stubMinerDetection(); + + const res = await app.request(ROUTES[0]!, { headers: { authorization: "Bearer test-api-token" } }, env); + expect(res.status).not.toBe(403); + }); +}); + +// #9046: both collector 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. They were separate +// near-identical copies, which is how AMS ended up both fail-open and (separately) missing from the strict +// rate class; they are now one shared helper. +describe("telemetry ingest endpoints fail closed when their token is unset (#9046)", () => { + afterEach(() => vi.unstubAllGlobals()); + + const BODY = JSON.stringify({ instance_id: "i1", batch: [] }); + + it("rejects Orb ingest when ORB_INGEST_TOKEN is unset, instead of accepting anonymous writes", async () => { + const app = createApp(); + const env = createTestEnv(); + delete (env as { ORB_INGEST_TOKEN?: string }).ORB_INGEST_TOKEN; + const res = await app.request("/v1/orb/ingest", { method: "POST", body: BODY, headers: { "content-type": "application/json" } }, env); + expect(res.status).toBe(401); + }); + + it("rejects AMS ingest when AMS_INGEST_TOKEN is unset (the copy that had drifted)", async () => { + const app = createApp(); + const env = createTestEnv(); + delete (env as { AMS_INGEST_TOKEN?: string }).AMS_INGEST_TOKEN; + const res = await app.request("/v1/ams/ingest", { method: "POST", body: BODY, headers: { "content-type": "application/json" } }, env); + expect(res.status).toBe(401); + }); + + it("still rejects a WRONG bearer when the token IS configured, and stops rejecting once it matches", async () => { + const app = createApp(); + for (const [path, key] of [["/v1/orb/ingest", "ORB_INGEST_TOKEN"], ["/v1/ams/ingest", "AMS_INGEST_TOKEN"]] as const) { + const env = createTestEnv({ [key]: "s3cret" } as Partial); + const wrong = await app.request(path, { method: "POST", body: BODY, headers: { authorization: "Bearer nope", "content-type": "application/json" } }, env); + expect({ path, status: wrong.status }).toEqual({ path, status: 401 }); + const right = await app.request(path, { method: "POST", body: BODY, headers: { authorization: "Bearer s3cret", "content-type": "application/json" } }, env); + expect({ path, unauthorized: right.status === 401 }).toEqual({ path, unauthorized: false }); + } + }); +}); diff --git a/test/unit/routes-repo-scoped-auth-drift.test.ts b/test/unit/routes-repo-scoped-auth-drift.test.ts new file mode 100644 index 0000000000..7899547b2f --- /dev/null +++ b/test/unit/routes-repo-scoped-auth-drift.test.ts @@ -0,0 +1,62 @@ +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; + +/** + * #9045 structural guard. `GET /v1/repos/:owner/:repo/pulls/:number/maintainer-packet` shipped gated only by + * `requireStaticProtectedApiToken`, which admits ANY static identity including the shared, end-user-obtainable + * `mcp` token — it 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, and `MCP_READ_REPO_ALLOWLIST` is fail-closed by default (unset ⇒ deny all) — the intended posture was + * "deny everything" while this route allowed everything. + * + * The fix folded the allowlist INTO the gate (an optional `repoFullName` argument), so the correct behavior is + * now the one you get by passing the repo you already have. This test closes the remaining gap: it fails if a + * repo-scoped route calls the gate WITHOUT that argument, so the omission cannot be reintroduced silently by a + * future route. A source-text assertion is the right tool here precisely because the defect was an *absent* + * call — there is no runtime behavior to observe on the route that forgot it. + */ +describe("repo-scoped routes must pass repoFullName to requireStaticProtectedApiToken (#9045)", () => { + const source = readFileSync(join(process.cwd(), "src/api/routes.ts"), "utf8"); + + it("every requireStaticProtectedApiToken call inside a /v1/repos/:owner/:repo route is repo-scoped", () => { + // Walk each `app.("", ...)` registration and pair it with the gate calls in its handler body, + // bounded by the next registration so a call can never be attributed to the wrong route. + const registrations = [...source.matchAll(/app\.(get|post|put|patch|delete)\("([^"]+)"/g)]; + const offenders: string[] = []; + + for (const [index, match] of registrations.entries()) { + const routePath = match[2]!; + const bodyStart = match.index!; + const bodyEnd = registrations[index + 1]?.index ?? source.length; + const body = source.slice(bodyStart, bodyEnd); + if (!body.includes("requireStaticProtectedApiToken(")) continue; + // Only routes that actually carry a repo in the path can be repo-scoped. + if (!routePath.includes("/v1/repos/:owner/:repo")) continue; + // The gate must receive a second argument (the repo), not just the context. + const bareCalls = [...body.matchAll(/requireStaticProtectedApiToken\(\s*c\s*\)/g)]; + if (bareCalls.length > 0) offenders.push(routePath); + } + + expect(offenders).toEqual([]); + }); + + it("the gate itself still enforces the allowlist when a repo is supplied", () => { + // Guards against the inverse regression: someone keeps the call sites but guts the check inside the gate. + const gate = source.slice(source.indexOf("async function requireStaticProtectedApiToken(")); + const gateBody = gate.slice(0, gate.indexOf("\n}\n") + 3); + expect(gateBody).toContain("isMcpReadRepoAllowed"); + expect(gateBody).toContain("MCP_READ_REPO_ALLOWLIST"); + expect(gateBody).toContain("forbidden_repo"); + }); + + it("finds the repo-scoped routes it is meant to be guarding (the walker is not silently matching nothing)", () => { + const registrations = [...source.matchAll(/app\.(get|post|put|patch|delete)\("([^"]+)"/g)]; + const repoScopedGated = registrations.filter((match, index) => { + const body = source.slice(match.index!, registrations[index + 1]?.index ?? source.length); + return match[2]!.includes("/v1/repos/:owner/:repo") && body.includes("requireStaticProtectedApiToken("); + }); + // maintainer-packet, reviewability, gate-config/effective, live-gate-thresholds. + expect(repoScopedGated.length).toBeGreaterThanOrEqual(4); + }); +});