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); + }); +});