From 7b3619373bb29182a79f60219c514bc3efbb5ba8 Mon Sep 17 00:00:00 2001 From: cleanjunc Date: Wed, 22 Jul 2026 17:45:23 +0000 Subject: [PATCH] feat(auth): replace the global admin allowlist with a live per-repo permission check for hosted deployments (#4889) --- src/auth/security.ts | 57 +++++++ src/env.d.ts | 5 + src/queue/processors.ts | 44 +++-- src/queue/review-evasion.ts | 15 +- .../linked-issue-label-propagation-fetch.ts | 6 +- test/unit/auth-per-repo-admin.test.ts | 91 +++++++++++ ...nked-issue-label-propagation-fetch.test.ts | 25 +++ test/unit/queue-4.test.ts | 74 +++++++++ .../review-evasion-per-repo-admin.test.ts | 151 ++++++++++++++++++ worker-configuration.d.ts | 4 +- wrangler.jsonc | 1 + 11 files changed, 453 insertions(+), 20 deletions(-) create mode 100644 test/unit/auth-per-repo-admin.test.ts create mode 100644 test/unit/review-evasion-per-repo-admin.test.ts diff --git a/src/auth/security.ts b/src/auth/security.ts index b04d2bb278..ac2ea13715 100644 --- a/src/auth/security.ts +++ b/src/auth/security.ts @@ -136,6 +136,63 @@ export function isAuthorizedGitHubSessionLogin(env: Env, login: string): boolean return allowedLogins.has(login.toLowerCase()); } +/** #4889 hosted per-repo admin mode. When ON, the global ADMIN_GITHUB_LOGINS allowlist stops granting + * fleet-wide maintainer trust at the review/queue exemption sites — each consults the live per-repo GitHub + * permission instead ({@link isPerTenantAdmin}). OFF (the default) keeps self-host's existing + * global-allowlist behavior byte-identical. Truthy convention matches isOpsEnabled (ops-wire.ts). */ +export function isPerRepoAdminModeEnabled(env: { LOOPOVER_PER_REPO_ADMIN?: string | undefined }): boolean { + return /^(1|true|yes|on)$/i.test((env.LOOPOVER_PER_REPO_ADMIN ?? "").trim()); +} + +/** Injectable seam for {@link isPerTenantAdmin}'s live lookup (tests; production callers omit it). Matches + * getRepositoryCollaboratorPermission's shape (src/github/app.ts). */ +export type PerTenantAdminPermissionFetch = ( + env: Env, + installationId: number, + repoFullName: string, + login: string, +) => Promise; + +/** + * #4889: whether `login` holds admin trust for THIS repo — the hosted replacement for a bare + * `parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(login)` at the exemption sites. + * + * - Per-repo admin mode OFF (self-host default): exact ADMIN_GITHUB_LOGINS membership, unchanged semantics. + * - Mode ON (hosted): GitHub's real-time collaborator permission on `repoFullName` — `admin`/`maintain` + * passes, anything else denies. Fail-CLOSED: no installation to ask through, a lookup error, or an + * unknown collaborator all deny — an API blip must never silently grant fleet-operator trust (#4889's + * explicit safety guardrail). The repo-owner shortcut stays at the call sites (it predates and is + * independent of the allowlist this replaces). + */ +export async function isPerTenantAdmin( + env: Env, + installationId: number | null, + repoFullName: string, + login: string, + getPermission?: PerTenantAdminPermissionFetch, +): Promise { + const normalized = login.trim().toLowerCase(); + if (!normalized) return false; + if (!isPerRepoAdminModeEnabled(env)) return parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(normalized); + if (installationId === null) return false; + let permission: string | null; + try { + const fetchPermission = getPermission ?? (await import("../github/app")).getRepositoryCollaboratorPermission; + permission = await fetchPermission(env, installationId, repoFullName, normalized); + } catch (error) { + console.log( + JSON.stringify({ + event: "per_tenant_admin_check_failed", + repoFullName, + login: normalized, + message: error instanceof Error ? error.message.slice(0, 150) : String(error).slice(0, 150), + }), + ); + return false; + } + return permission === "admin" || permission === "maintain"; +} + /** Parse a GitHub-login allowlist env (e.g. ADMIN_GITHUB_LOGINS) into a lowercased Set. Splits on whitespace OR * commas so every caller agrees on the same parse (#audit-3.13). */ export function parseGitHubLoginList(value: string | undefined): Set { diff --git a/src/env.d.ts b/src/env.d.ts index 68a3c4a5f0..a72c059f34 100644 --- a/src/env.d.ts +++ b/src/env.d.ts @@ -381,6 +381,11 @@ declare global { * byte-identical to today. NOTE: this is read-only OBSERVABILITY only; the auto-tune / config-mutation * self-improve loop (src/review/auto-apply.ts) is deliberately NOT wired here — see ops-wire.ts. */ LOOPOVER_REVIEW_OPS?: string; + /** #4889 hosted per-repo admin mode: when truthy, the global ADMIN_GITHUB_LOGINS allowlist stops granting + * fleet-wide maintainer trust at the review/queue exemption sites — each consults GitHub's real-time + * per-repo collaborator permission instead (isPerTenantAdmin, src/auth/security.ts), failing CLOSED on + * any API error. Default OFF — unset/false keeps self-host's global-allowlist behavior byte-identical. */ + LOOPOVER_PER_REPO_ADMIN?: string; /** Self-heal: when truthy, an hourly watchdog scans the SAME acting-autonomy repo set the scheduled regate * sweep covers for a repo whose sweep marker hasn't advanced despite having open PRs to regate, emits a * structured `sweep_liveness_stale` log (Sentry-visible), and re-enqueues a targeted `agent-regate-sweep` diff --git a/src/queue/processors.ts b/src/queue/processors.ts index bd30231655..5109306103 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -218,7 +218,8 @@ import { preparePrPacketWithAgent, } from "../services/agent-orchestrator"; import { - isAuthorizedGitHubSessionLogin, + isPerRepoAdminModeEnabled, + isPerTenantAdmin, parseGitHubLoginList, } from "../auth/security"; import { @@ -2687,7 +2688,8 @@ async function maybeCloseForContributorCapOnOpen( * repoFullName.split("/")[0] is never undefined for any non-empty repoFullName (every real caller's). */ const repoOwner = repoFullName.split("/")[0] ?? ""; const authorIsOwner = pr.authorLogin.toLowerCase() === repoOwner.toLowerCase(); - const authorIsAdmin = parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(pr.authorLogin.toLowerCase()); + // #4889: per-repo admin mode swaps the global-allowlist grant for the live per-repo permission. + const authorIsAdmin = await isPerTenantAdmin(env, installationId, repoFullName, pr.authorLogin); const authorIsAutomationBot = isProtectedAutomationAuthor(pr.authorLogin); if (authorIsOwner || authorIsAdmin || authorIsAutomationBot) return false; // #ignore-authors-parity: a manifest ignore_authors match (e.g. "release-please*") means the bot treats @@ -2937,7 +2939,8 @@ async function runAgentMaintenancePlanAndExecute( // admin login (not the literal repo owner) gets the identical never-auto-closed exemption everywhere. const authorIsAdmin = authorLogin.length > 0 && - parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(authorLogin.toLowerCase()); + // #4889: per-repo admin mode swaps the global-allowlist grant for the live per-repo permission. + (await isPerTenantAdmin(env, installationId, repoFullName, authorLogin)); const authorIsAutomationBot = isProtectedAutomationAuthor(pr.authorLogin); // Linked-issue HARD-RULE close (#linked-issue-hard-rules): when the repo enabled any rule, a body that links @@ -5351,7 +5354,8 @@ async function maybeCloseIssueOverContributorCap( const repoOwner = repoOwnerLoginFromFullName(repoFullName); const authorIsOwner = authorLogin.toLowerCase() === repoOwner.toLowerCase(); - const authorIsAdmin = parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(authorLogin.toLowerCase()); + // #4889: per-repo admin mode swaps the global-allowlist grant for the live per-repo permission. + const authorIsAdmin = await isPerTenantAdmin(env, args.installationId, repoFullName, authorLogin); const authorIsAutomationBot = isProtectedAutomationAuthor(authorLogin); if (authorIsOwner || authorIsAdmin || authorIsAutomationBot) return; @@ -6606,7 +6610,8 @@ async function handleIssueWebhookEvent( const repoOwner = repoOwnerLoginFromFullName(payload.repository.full_name); const authorLogin = issue.authorLogin; const authorIsOwner = authorLogin.toLowerCase() === repoOwner.toLowerCase(); - const authorIsAdmin = parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(authorLogin.toLowerCase()); + // #4889: per-repo admin mode swaps the global-allowlist grant for the live per-repo permission. + const authorIsAdmin = await isPerTenantAdmin(env, installationId, payload.repository.full_name, authorLogin); const authorIsAutomationBot = isProtectedAutomationAuthor(authorLogin); const accountAgeThresholdDays = issueSettings.accountAgeThresholdDays; if ( @@ -7947,7 +7952,14 @@ async function maybePostVisualFollowupComment( const visualConfig = await resolveVisualCaptureConfig(env, repoFullName); if (!visualConfig.bugAnalysis) return; const { owner } = repoParts(repoFullName); - const notifyLogins = resolveVisualFollowupNotifyLogins(visualConfig.bugAnalysisNotify, owner, parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS)); + // #4889: live per-repo permissions cannot be ENUMERATED (the API answers per-login queries only), so in + // per-repo admin mode the notify set carries no allowlist contribution — the repo owner + the repo's own + // configured bugAnalysisNotify list remain the notify surface. + const notifyLogins = resolveVisualFollowupNotifyLogins( + visualConfig.bugAnalysisNotify, + owner, + isPerRepoAdminModeEnabled(env) ? new Set() : parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS), + ); const body = buildVisualFollowupComment(priorAdvisory.findings, notifyLogins); if (!body) return; await createOrUpdateVisualFollowupComment(env, installationId, repoFullName, pullNumber, body); @@ -9491,7 +9503,8 @@ async function maybePublishPrPublicSurface( const authorIsExemptFromFreeze = author !== null && (author.toLowerCase() === repoOwnerLoginFromFullName(repoFullName).toLowerCase() || - parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(author.toLowerCase()) || + // #4889: per-repo admin mode swaps the global-allowlist grant for the live per-repo permission. + (await isPerTenantAdmin(env, installationId, repoFullName, author)) || isProtectedAutomationAuthor(author)); const isFrozenForManualReview = webhook.forceAiReview !== true && @@ -13058,7 +13071,8 @@ async function maybeThrottleReviewNagPing( // malformed/synthetic payload from ever matching an empty commenter login as "the owner". const repoOwner = repoFullName.includes("/") ? repoFullName.slice(0, repoFullName.indexOf("/")) : ""; if (commenter.toLowerCase() === repoOwner.toLowerCase()) return false; - if (parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(commenter.toLowerCase())) return false; + // #4889: per-repo admin mode swaps the global-allowlist grant for the live per-repo permission. + if (await isPerTenantAdmin(env, installationId, repoFullName, commenter)) return false; // NOTE: no separate isProtectedAutomationAuthor(commenter) check here — every entry in that set (e.g. // "dependabot[bot]") already ends in "[bot]" and was rejected by the bot-suffix guard above, so it would be // unreachable dead code at this point (unlike the PR-webhook maintenance path, which checks a PR's stored @@ -13244,7 +13258,8 @@ async function maybeThrottleMonitoredMentions( const repoOwner = repoFullName.includes("/") ? repoFullName.slice(0, repoFullName.indexOf("/")) : ""; if (commenter.toLowerCase() === repoOwner.toLowerCase()) return false; - if (parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(commenter.toLowerCase())) return false; + // #4889: per-repo admin mode swaps the global-allowlist grant for the live per-repo permission. + if (await isPerTenantAdmin(env, installationId, repoFullName, commenter)) return false; if (isAutoCloseExempt(commenter, settings.autoCloseExemptLogins)) return false; const mentionedLogin = monitoredLogins.find((login) => bodyMentionsLogin(body, login)); @@ -14331,7 +14346,8 @@ async function maybeProcessAgentCommandFeedbackReaction( deliveryId, }) : undefined; - const authorization = authorizeFeedbackActor(env, { + const authorization = await authorizeFeedbackActor(env, { + installationId: getInstallationId(payload), actor, repoFullName, pullRequestAuthor, @@ -14388,15 +14404,16 @@ function reactionVote( return null; } -function authorizeFeedbackActor( +async function authorizeFeedbackActor( env: Env, args: { actor: string; repoFullName: string; + installationId: number | null; pullRequestAuthor?: string | null | undefined; officialAuthorDetection?: OfficialGittensorMinerDetection | undefined; }, -): { authorized: boolean; reason: string; actorKind: "maintainer" | "author" } { +): Promise<{ authorized: boolean; reason: string; actorKind: "maintainer" | "author" }> { const [owner] = args.repoFullName.split("/"); if (owner && owner.toLowerCase() === args.actor.toLowerCase()) { return { @@ -14405,7 +14422,8 @@ function authorizeFeedbackActor( actorKind: "maintainer", }; } - if (isAuthorizedGitHubSessionLogin(env, args.actor)) { + // #4889: per-repo admin mode swaps the global-allowlist operator grant for the live per-repo permission. + if (await isPerTenantAdmin(env, args.installationId, args.repoFullName, args.actor)) { return { authorized: true, reason: "operator_feedback", diff --git a/src/queue/review-evasion.ts b/src/queue/review-evasion.ts index c65aa90568..c2a33cd401 100644 --- a/src/queue/review-evasion.ts +++ b/src/queue/review-evasion.ts @@ -21,7 +21,7 @@ import { getRepositoryCollaboratorPermission } from "../github/app"; import { ensurePullRequestLabel } from "../github/labels"; import { fetchPullRequestFreshness, pullRequestFreshnessDetail, type PullRequestFreshness } from "../github/pr-freshness"; import { closePullRequest, createIssueComment, getLastCloserLogin, getLastReopenerLogin, reopenPullRequest } from "../github/pr-actions"; -import { parseGitHubLoginList } from "../auth/security"; +import { isPerRepoAdminModeEnabled, isPerTenantAdmin, parseGitHubLoginList } from "../auth/security"; import { isAutoCloseExempt } from "../settings/auto-close-exempt"; import { resolveAutonomy } from "../settings/autonomy"; import { isGlobalAgentPause, resolveAgentActionMode, resolveAgentPermissionReadiness } from "../settings/agent-execution"; @@ -254,7 +254,8 @@ async function closeDraftDodgeAttemptIfBlocked( // actuation path's trusted-operator definition. const authorIsAdmin = draftDodgeAuthorLogin.length > 0 && - parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(draftDodgeAuthorLogin); + // #4889: per-repo admin mode swaps the global-allowlist grant for the live per-repo permission. + (await isPerTenantAdmin(env, installationId, repoFullName, draftDodgeAuthorLogin)); if ( block && block.headSha === pr.headSha && @@ -369,7 +370,11 @@ async function recloseDisallowedReopenIfNeeded( const repoOwner = repoFullName.includes("/") ? repoFullName.slice(0, repoFullName.indexOf("/")).toLowerCase() : ""; - const admins = parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS); // unified parse: whitespace OR comma (#audit-3.13) + // #4889: in per-repo admin mode the global allowlist stops granting — the live collaborator check below + // (admin/maintain/write) is the sole permission source; self-host keeps the allowlist shortcut unchanged. + const admins = isPerRepoAdminModeEnabled(env) + ? new Set() + : parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS); // unified parse: whitespace OR comma (#audit-3.13) const hasMaintainerPermission = async (login: string): Promise => { if (login === repoOwner || admins.has(login)) return true; const permission = await getRepositoryCollaboratorPermission( @@ -549,7 +554,9 @@ async function hasMaintainerOrOwnerPermission(env: Env, installationId: number, // repository match on that exact format before any review-evasion handler runs. /* v8 ignore next */ const repoOwner = repoFullName.includes("/") ? repoFullName.slice(0, repoFullName.indexOf("/")).toLowerCase() : ""; - if (login === repoOwner || parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(login)) return true; + // #4889: in per-repo admin mode the global allowlist stops granting — the live collaborator check below + // is the sole permission source; self-host keeps the allowlist shortcut unchanged. + if (login === repoOwner || (!isPerRepoAdminModeEnabled(env) && parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(login))) return true; const permission = await getRepositoryCollaboratorPermission(env, installationId, repoFullName, login).catch(() => null); return permission === "admin" || permission === "maintain" || permission === "write"; } diff --git a/src/review/linked-issue-label-propagation-fetch.ts b/src/review/linked-issue-label-propagation-fetch.ts index c965616b8e..272899ddd0 100644 --- a/src/review/linked-issue-label-propagation-fetch.ts +++ b/src/review/linked-issue-label-propagation-fetch.ts @@ -7,7 +7,7 @@ import { } from "../github/backfill"; import { createInstallationToken, getRepositoryCollaboratorPermission } from "../github/app"; import { githubRateLimitAdmissionKeyForToken, type GitHubRateLimitAdmissionKey } from "../github/client"; -import { parseGitHubLoginList } from "../auth/security"; +import { isPerRepoAdminModeEnabled, parseGitHubLoginList } from "../auth/security"; import { errorMessage } from "../utils/json"; import type { LinkedIssueLabelPropagationMapping } from "../types"; @@ -56,7 +56,9 @@ async function isRepoMaintainerLogin(env: Env, installationId: number, repoFullN // pattern + rationale in `hasMaintainerOrOwnerPermission`, `src/queue/processors.ts`). /* v8 ignore next */ const repoOwner = repoFullName.includes("/") ? repoFullName.slice(0, repoFullName.indexOf("/")).toLowerCase() : ""; - if (login === repoOwner || parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(login)) return "maintainer"; + // #4889: in per-repo admin mode the global allowlist stops granting — the live collaborator check below + // is the sole permission source; self-host keeps the allowlist shortcut unchanged. + if (login === repoOwner || (!isPerRepoAdminModeEnabled(env) && parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(login))) return "maintainer"; let permission: Awaited>; try { permission = await getRepositoryCollaboratorPermission(env, installationId, repoFullName, login); diff --git a/test/unit/auth-per-repo-admin.test.ts b/test/unit/auth-per-repo-admin.test.ts new file mode 100644 index 0000000000..852e092e28 --- /dev/null +++ b/test/unit/auth-per-repo-admin.test.ts @@ -0,0 +1,91 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { isPerRepoAdminModeEnabled, isPerTenantAdmin } from "../../src/auth/security"; +import { createTestEnv } from "../helpers/d1"; + +// #4889: the hosted per-repo admin helpers. isPerTenantAdmin replaces bare ADMIN_GITHUB_LOGINS membership at +// the review/queue exemption sites: mode OFF (self-host default) must stay byte-identical allowlist +// membership; mode ON swaps in GitHub's real-time collaborator permission and fails CLOSED on every +// can't-verify path (no installation, API error, unknown collaborator) — an API blip must never grant +// fleet-operator trust. + +afterEach(() => vi.unstubAllGlobals()); + +describe("isPerRepoAdminModeEnabled (#4889)", () => { + it("accepts the standard truthy spellings, case-insensitively, with whitespace", () => { + for (const value of ["1", "true", "TRUE", "yes", "on", " on "]) { + expect(isPerRepoAdminModeEnabled({ LOOPOVER_PER_REPO_ADMIN: value })).toBe(true); + } + }); + + it("is OFF for unset, empty, and non-truthy values — self-host default", () => { + for (const value of [undefined, "", "false", "0", "off", "enabled"]) { + expect(isPerRepoAdminModeEnabled({ LOOPOVER_PER_REPO_ADMIN: value })).toBe(false); + } + }); +}); + +describe("isPerTenantAdmin (#4889)", () => { + const REPO = "acme/widgets"; + + it("mode OFF: exact ADMIN_GITHUB_LOGINS membership, case-insensitive, no permission lookup", async () => { + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "FleetOp, other-admin" }); + const getPermission = vi.fn(); + expect(await isPerTenantAdmin(env, 42, REPO, "fleetop", getPermission)).toBe(true); + expect(await isPerTenantAdmin(env, 42, REPO, " FleetOp ", getPermission)).toBe(true); + expect(await isPerTenantAdmin(env, 42, REPO, "stranger", getPermission)).toBe(false); + // A null installation doesn't matter in allowlist mode — no API is ever consulted. + expect(await isPerTenantAdmin(env, null, REPO, "fleetop", getPermission)).toBe(true); + expect(getPermission).not.toHaveBeenCalled(); + }); + + it("mode ON: admin and maintain pass; write, read, and unknown deny — allowlist membership no longer grants", async () => { + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "fleetop", LOOPOVER_PER_REPO_ADMIN: "true" }); + for (const [permission, expected] of [ + ["admin", true], + ["maintain", true], + ["write", false], + ["read", false], + [null, false], + ] as const) { + const getPermission = vi.fn().mockResolvedValue(permission); + expect(await isPerTenantAdmin(env, 42, REPO, "FleetOp", getPermission)).toBe(expected); + expect(getPermission).toHaveBeenCalledWith(env, 42, REPO, "fleetop"); + } + }); + + it("mode ON: fails closed when there is no installation to ask through", async () => { + const env = createTestEnv({ LOOPOVER_PER_REPO_ADMIN: "true" }); + const getPermission = vi.fn(); + expect(await isPerTenantAdmin(env, null, REPO, "someone", getPermission)).toBe(false); + expect(getPermission).not.toHaveBeenCalled(); + }); + + it("mode ON: fails closed (and logs) when the permission lookup throws — Error and non-Error alike", async () => { + const env = createTestEnv({ LOOPOVER_PER_REPO_ADMIN: "true" }); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + expect(await isPerTenantAdmin(env, 42, REPO, "someone", vi.fn().mockRejectedValue(new Error("github down")))).toBe(false); + expect(await isPerTenantAdmin(env, 42, REPO, "someone", vi.fn().mockRejectedValue("string-throw"))).toBe(false); + const events = log.mock.calls.map((call) => JSON.parse(String(call[0])) as Record); + expect(events).toHaveLength(2); + for (const event of events) expect(event).toMatchObject({ event: "per_tenant_admin_check_failed", repoFullName: REPO, login: "someone" }); + expect(events[0]!.message).toBe("github down"); + log.mockRestore(); + }); + + it("denies a blank login in either mode without any lookup", async () => { + const getPermission = vi.fn(); + expect(await isPerTenantAdmin(createTestEnv({ ADMIN_GITHUB_LOGINS: "fleetop" }), 42, REPO, " ", getPermission)).toBe(false); + expect(await isPerTenantAdmin(createTestEnv({ LOOPOVER_PER_REPO_ADMIN: "true" }), 42, REPO, "", getPermission)).toBe(false); + expect(getPermission).not.toHaveBeenCalled(); + }); + + it("mode ON without an injected fetcher uses the real permission lookup, still failing closed on error", async () => { + const env = createTestEnv({ LOOPOVER_PER_REPO_ADMIN: "true" }); + // The real getRepositoryCollaboratorPermission path starts with an installation-token exchange; a stubbed + // global fetch that refuses everything proves the default path is wired AND that its failure denies. + vi.stubGlobal("fetch", async () => new Response("nope", { status: 500 })); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + expect(await isPerTenantAdmin(env, 42, REPO, "someone")).toBe(false); + log.mockRestore(); + }); +}); diff --git a/test/unit/linked-issue-label-propagation-fetch.test.ts b/test/unit/linked-issue-label-propagation-fetch.test.ts index 6511c917a6..470791bc24 100644 --- a/test/unit/linked-issue-label-propagation-fetch.test.ts +++ b/test/unit/linked-issue-label-propagation-fetch.test.ts @@ -674,6 +674,31 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( expectPropagation(result, ["gittensor:feature"]); }); + it("stops trusting the global allowlist in per-repo admin mode (#4889): a fleet-operator author with only read access no longer propagates", async () => { + stubFetch((url) => { + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/11")) + return Response.json({ number: 11, state: "open", user: { login: "fleetop" }, assignees: [], labels: ["gittensor:feature"] }); + // The live per-repo answer now decides where the allowlist used to shortcut. + if (url.includes("/collaborators/fleetop/permission")) return Response.json({ permission: "read" }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ + ADMIN_GITHUB_LOGINS: "fleetop", + LOOPOVER_PER_REPO_ADMIN: "true", + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + }); + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [11], + installationId: 123, + prAuthorLogin: "contrib", + mappings: RELAXABLE_MAPPINGS, + }); + expectPropagation(result, []); + }); + it("propagates a relaxable label from an issue authored by a live write-collaborator (not the owner, not in the admin allowlist)", async () => { stubFetch((url) => { if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); diff --git a/test/unit/queue-4.test.ts b/test/unit/queue-4.test.ts index 7cc93d6382..3ef503f022 100644 --- a/test/unit/queue-4.test.ts +++ b/test/unit/queue-4.test.ts @@ -3987,6 +3987,80 @@ describe("queue processors", () => { expect(followupCommentBody).toContain(""); }); + // #4889: the follow-up's notify set draws on ADMIN_GITHUB_LOGINS only outside per-repo admin mode. Live + // per-repo permissions cannot be ENUMERATED (the API answers per-login queries only), so in per-repo admin + // mode the allowlist contributes nothing — the owner (and any configured bug_analysis_notify list) remains + // the notify surface. + it.each([ + ["off", undefined, true], + ["on", "true", false], + ] as const)( + "visual follow-up notify set: per-repo admin mode %s — fleet-operator allowlist mention expected=%s (#4889)", + async (_label, perRepoAdmin, expectFleetOpMention) => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + LOOPOVER_REVIEW_SCREENSHOTS: "true", + ADMIN_GITHUB_LOGINS: "fleetop", + ...(perRepoAdmin !== undefined ? { LOOPOVER_PER_REPO_ADMIN: perRepoAdmin } : {}), + }); + await persistRegistrySnapshot( + asCloudEnv(env), + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autoLabelEnabled: false, autonomy: { update_branch: "auto" } }); + await persistAdvisory(env, { + id: crypto.randomUUID(), + targetType: "pull_request", + targetKey: "JSONbored/gittensory#45", + repoFullName: "JSONbored/gittensory", + pullNumber: 45, + headSha: "closed125", + conclusion: "neutral", + severity: "warning", + title: "LoopOver advisory available", + summary: "1 advisory finding generated.", + findings: [{ code: "visual_unrelated_issue_finding", severity: "warning", title: "Possible unrelated visual issue: /footer", detail: "Notify-set fixture." }], + generatedAt: "2026-05-23T00:00:00.000Z", + }); + let followupCommentBody = ""; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://raw.githubusercontent.com/JSONbored/gittensory/HEAD/.loopover.yml") { + return new Response("review:\n visual:\n bug_analysis: true\n"); + } + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token", expires_at: "2026-05-28T00:04:00.000Z" }); + if (url.includes("/issues/45/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/45/comments") && method === "POST") { + followupCommentBody = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + return Response.json({ id: 910, html_url: "https://github.com/comment/910" }, { status: 201 }); + } + if (url.includes("/check-runs") && method === "GET") return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/check-runs") && method === "POST") return Response.json({ id: 902 }, { status: 201 }); + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: `pr-visual-followup-notify-${perRepoAdmin ?? "unset"}`, + eventName: "pull_request", + payload: { + action: "closed", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 45, title: "Footer tweak", state: "closed", user: { login: "contributor" }, head: { sha: "closed125" }, labels: [], body: "Fixes #1" }, + }, + }); + + expect(followupCommentBody).toContain("@jsonbored"); + expect(followupCommentBody.includes("@fleetop")).toBe(expectFleetOpMention); + }, + ); + it("does NOT post a follow-up comment on close when review.visual.bugAnalysis is off, even with a recorded unrelated finding (stale from before the operator turned it off)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), LOOPOVER_REVIEW_SCREENSHOTS: "true" }); await persistRegistrySnapshot( diff --git a/test/unit/review-evasion-per-repo-admin.test.ts b/test/unit/review-evasion-per-repo-admin.test.ts new file mode 100644 index 0000000000..2ed589bf4d --- /dev/null +++ b/test/unit/review-evasion-per-repo-admin.test.ts @@ -0,0 +1,151 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + maybeCloseDraftDodgeAttempt, + maybeCloseReviewEvasionSelfClose, + maybeRecloseDisallowedReopen, +} from "../../src/queue/review-evasion"; +import { createTestEnv } from "../helpers/d1"; +import type { GitHubWebhookPayload, PullRequestRecord, RepositorySettings } from "../../src/types"; + +// #4889: the review-evasion guards' fleet-operator exemptions in per-repo admin mode. Mode OFF (self-host +// default) keeps the ADMIN_GITHUB_LOGINS shortcut byte-identical — an allowlisted actor is exempt with NO +// GitHub permission lookup. Mode ON drops that shortcut: the live per-repo collaborator permission is the +// sole non-owner permission source, so an allowlisted login with no real access on THIS repo stops being +// exempt. Each test pins which path ran by inspecting exactly which GitHub API calls the guard made. + +const PEM_HEADER = ["-----BEGIN", "PRIVATE KEY-----"].join(" "); +const PEM_FOOTER = ["-----END", "PRIVATE KEY-----"].join(" "); + +async function generatePrivateKeyPem(): Promise { + const key = (await crypto.subtle.generateKey( + { name: "RSASSA-PKCS1-v1_5", modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: "SHA-256" }, + true, + ["sign", "verify"], + )) as CryptoKeyPair; + const exported = await crypto.subtle.exportKey("pkcs8", key.privateKey); + const base64 = Buffer.from(exported as ArrayBuffer) + .toString("base64") + .replace(/(.{64})/g, "$1\n"); + return `${PEM_HEADER}\n${base64}\n${PEM_FOOTER}`; +} + +type StubHandler = (url: string) => Response | undefined; + +function stubGitHub(handler: StubHandler = () => undefined): string[] { + const urls: string[] = []; + vi.stubGlobal("fetch", async (input: string | URL | Request) => { + const url = String(input instanceof Request ? input.url : input); + urls.push(url); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + const handled = handler(url); + if (handled) return handled; + return new Response("not found", { status: 404 }); + }); + return urls; +} + +afterEach(() => vi.unstubAllGlobals()); + +function pr(overrides: Partial = {}): PullRequestRecord { + return { + repoFullName: "owner/repo", + number: 7, + title: "t", + state: "closed", + authorLogin: "fleetop", + headSha: "abc123", + ...overrides, + } as PullRequestRecord; +} + +const SETTINGS = { reviewEvasionProtection: "on", autoCloseExemptLogins: [] } as unknown as RepositorySettings; + +function reopenPayload(sender: string): GitHubWebhookPayload { + return { sender: { login: sender } } as GitHubWebhookPayload; +} + +describe("maybeRecloseDisallowedReopen fleet-operator exemption (#4889)", () => { + it("mode OFF: an allowlisted reopener is exempt via the allowlist alone — zero GitHub calls", async () => { + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "fleetop" }); + const urls = stubGitHub(); + const outcome = await maybeRecloseDisallowedReopen(env, "d1", 123, "owner/repo", pr(), reopenPayload("fleetop")); + expect(outcome).toBe("allowed"); + expect(urls).toEqual([]); + }); + + it("mode OFF: the repo owner is exempt without a lookup regardless of the allowlist", async () => { + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "" }); + const urls = stubGitHub(); + expect(await maybeRecloseDisallowedReopen(env, "d1", 123, "owner/repo", pr(), reopenPayload("owner"))).toBe("allowed"); + expect(urls).toEqual([]); + }); + + it("mode ON: the allowlist stops granting — the guard consults the live per-repo permission instead", async () => { + const env = createTestEnv({ + ADMIN_GITHUB_LOGINS: "fleetop", + LOOPOVER_PER_REPO_ADMIN: "true", + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + }); + const urls = stubGitHub((url) => { + if (url.includes("/collaborators/fleetop/permission")) return Response.json({ permission: "read" }); + // No qualifying closer on a fully-covered timeline → the reopen stays allowed after the deeper checks. + if (url.includes("/timeline")) return Response.json([]); + return undefined; + }); + const outcome = await maybeRecloseDisallowedReopen(env, "d1", 123, "owner/repo", pr(), reopenPayload("fleetop")); + expect(outcome).toBe("allowed"); + // The decisive difference from mode OFF: the live permission endpoint WAS consulted. + expect(urls.some((url) => url.includes("/collaborators/fleetop/permission"))).toBe(true); + }); + + it("mode ON: the repo owner shortcut is untouched — still zero permission lookups", async () => { + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "fleetop", LOOPOVER_PER_REPO_ADMIN: "true" }); + const urls = stubGitHub(); + expect(await maybeRecloseDisallowedReopen(env, "d1", 123, "owner/repo", pr(), reopenPayload("owner"))).toBe("allowed"); + expect(urls).toEqual([]); + }); +}); + +describe("maybeCloseReviewEvasionSelfClose fleet-operator exemption (#4889)", () => { + it("mode OFF: an allowlisted self-closing author is maintainer-exempt with zero GitHub calls", async () => { + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "fleetop" }); + const urls = stubGitHub(); + await maybeCloseReviewEvasionSelfClose(env, "d1", 123, "owner/repo", pr(), reopenPayload("fleetop"), SETTINGS); + expect(urls).toEqual([]); + }); + + it("mode ON: the allowlist stops granting — the live per-repo permission is consulted and a read-only author is no longer exempt", async () => { + const env = createTestEnv({ + ADMIN_GITHUB_LOGINS: "fleetop", + LOOPOVER_PER_REPO_ADMIN: "true", + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + }); + const urls = stubGitHub((url) => { + if (url.includes("/collaborators/fleetop/permission")) return Response.json({ permission: "read" }); + return undefined; + }); + // No review recorded for this headSha in the DB, so after the (now-failing) exemption the guard exits at + // the has-reviewed gate — the observable difference is the permission lookup itself. + await maybeCloseReviewEvasionSelfClose(env, "d1", 123, "owner/repo", pr(), reopenPayload("fleetop"), SETTINGS); + expect(urls.some((url) => url.includes("/collaborators/fleetop/permission"))).toBe(true); + }); + + it("mode ON: the self-closing repo owner stays exempt without any lookup", async () => { + const env = createTestEnv({ LOOPOVER_PER_REPO_ADMIN: "true" }); + const urls = stubGitHub(); + await maybeCloseReviewEvasionSelfClose(env, "d1", 123, "owner/repo", pr({ authorLogin: "owner" }), reopenPayload("owner"), SETTINGS); + expect(urls).toEqual([]); + }); +}); + +describe("maybeCloseDraftDodgeAttempt fleet-operator exemption (#4889)", () => { + it("computes the author's admin exemption through the mode-aware helper (allowlist in mode OFF), and skips it for an authorless PR", async () => { + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "fleetop" }); + const urls = stubGitHub(); + // No gate-block row exists in the DB, so the guard evaluates the exemptions and does nothing — no + // comment, no close, no GitHub call in either case. + await maybeCloseDraftDodgeAttempt(env, "d1", 123, "owner/repo", pr(), SETTINGS); + await maybeCloseDraftDodgeAttempt(env, "d1", 123, "owner/repo", pr({ authorLogin: null }), SETTINGS); + expect(urls).toEqual([]); + }); +}); diff --git a/worker-configuration.d.ts b/worker-configuration.d.ts index 5e841fa0d1..c318f582f3 100644 --- a/worker-configuration.d.ts +++ b/worker-configuration.d.ts @@ -1,5 +1,5 @@ /* eslint-disable */ -// Generated by Wrangler by running `wrangler types` (hash: 941f69060238ea20a568fa8ea50b814c) +// Generated by Wrangler by running `wrangler types` (hash: d41cd54bc0c9395266c55f6e42615d90) // Runtime types generated with workerd@1.20260701.1 2026-05-28 nodejs_compat interface __BaseEnv_Env { REVIEW_AUDIT: R2Bucket; @@ -26,6 +26,7 @@ interface __BaseEnv_Env { LOOPOVER_REVIEW_CONTINUOUS: "false"; LOOPOVER_REVIEW_REPUTATION: "false"; LOOPOVER_REVIEW_OPS: "false"; + LOOPOVER_PER_REPO_ADMIN: "false"; LOOPOVER_SWEEP_WATCHDOG: "false"; LOOPOVER_LOOP_ESCALATION: "false"; LOOPOVER_PR_RECONCILIATION: "false"; @@ -81,6 +82,7 @@ declare namespace NodeJS { | "LOOPOVER_LOOP_ESCALATION" | "LOOPOVER_MAINTAINER_RECAP" | "LOOPOVER_OPEN_PR_FILE_COLLISION" + | "LOOPOVER_PER_REPO_ADMIN" | "LOOPOVER_PR_RECONCILIATION" | "LOOPOVER_PUBLIC_STATS" | "LOOPOVER_PUBLIC_STATS_REPOS" diff --git a/wrangler.jsonc b/wrangler.jsonc index e17085a92b..22e5cd0ee5 100644 --- a/wrangler.jsonc +++ b/wrangler.jsonc @@ -96,6 +96,7 @@ // aggregate. Read-only observability; the auto-tune/config-mutation self-improve loop is NOT wired here. // Default OFF — flag-OFF the cron enqueues no ops job and the endpoint 404s, byte-identical to today. "LOOPOVER_REVIEW_OPS": "false", + "LOOPOVER_PER_REPO_ADMIN": "false", // Self-heal (#audit-sweep-fanout-isolation follow-up): an hourly watchdog over the SAME acting-autonomy repo // set the scheduled regate sweep covers — a repo with open PRs whose last-regated marker hasn't advanced in // over the staleness window gets a structured `sweep_liveness_stale` log (Sentry-visible) AND a single