feat(auth): replace the global admin allowlist with a live per-repo permission check for hosted deployments - #8063
Conversation
…ermission check for hosted deployments (JSONbored#4889)
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-22 18:18:23 UTC
Review summary Nits — 1 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8063 +/- ##
==========================================
+ Coverage 91.82% 92.29% +0.47%
==========================================
Files 733 650 -83
Lines 75017 54366 -20651
Branches 23035 19397 -3638
==========================================
- Hits 68883 50179 -18704
+ Misses 5034 3312 -1722
+ Partials 1100 875 -225
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Summary
Closes #4889.
Implements the ratified decision (2026-07-21) exactly: a live GitHub permission check replaces the single global
ADMIN_GITHUB_LOGINSallowlist at every listed fleet-operator exemption site, for hosted deployments only — self-host keeps the global allowlist as its default, byte-identical.src/auth/security.ts, the file that ownsparseGitHubLoginList):isPerTenantAdmin(env, installationId, repoFullName, login)— per-repo admin mode OFF (default): exactADMIN_GITHUB_LOGINSmembership, unchanged semantics; mode ON:GET /repos/{owner}/{repo}/collaborators/{username}/permissionvia the existinggetRepositoryCollaboratorPermission(src/github/app.ts) —admin/maintainpasses, anything else denies. Fail-closed on every can't-verify path (no installation to ask through, an API error, an unknown collaborator), per the issue's explicit guardrail — an API blip must never silently grant fleet-operator trust. The mode flag isLOOPOVER_PER_REPO_ADMIN(truthy-string, OFF by default, declared inwrangler.jsoncvars +env.d.ts;worker-configuration.d.tsregenerated viacf-typegen).src/queue/processors.ts(7 sites + 1 special): the contributor-cap exemptions (PR open + issue paths), the agent-maintenance close-eligibility computation, the account-age labeling exemption, the manual-review freeze exemption, both comment-throttle guards (review-nag, monitored mentions), andauthorizeFeedbackActor(now async, threading the webhook's installation id) all consult the helper. The visual follow-up notify set is the one non-mechanical site: live permissions cannot be enumerated (the API answers per-login queries only), so in per-repo admin mode the allowlist contributes no notify logins — the repo owner + the repo's own configuredbug_analysis_notifylist remain the notify surface (documented at the site, pinned by test).src/queue/review-evasion.ts(3 sites) andsrc/review/linked-issue-label-propagation-fetch.ts: these sites already follow their allowlist shortcut with a live collaborator check — in per-repo admin mode the shortcut simply stops granting and that existing live check (already fail-safe) becomes the sole non-owner permission source. The repo-owner shortcut is untouched everywhere (it predates and is independent of the allowlist this replaces, mirroring thelogin === repoOwnerprecedent the issue cites).Out of scope, deliberately: the session-path gates (
canSessionAccessPath, MCP) — they carry no repo context to be per-repo about and are not in the issue's confirmed call-site list.Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123) — a linked open issue is required for every contributor PR.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateIf any required check was skipped, explain why:
test:coverage(30+ min on this machine), I ran every affected suite with coverage — the newauth-per-repo-admin+review-evasion-per-repo-adminfiles, pluslinked-issue-label-propagation-fetch,queue-2/3/4/5,global-contributor-cap,account-age-throttle,visual-followup,auth-security-helpers(655 tests, all green) — and measured patch coverage empirically by intersectingcoverage/lcov.infoDA/BRDArecords withgit diff -U0againstupstream/main: 100% of changed lines and 100% of changed branches in every changedsrc/**file.actionlint(no workflow change),test:workers/build:mcp/test:mcp-pack/ui:*(no workers/mcp/UI change — the diff is backendsrc/**+wrangler.jsonc/worker-configuration.d.ts/env.d.tstypegen),npm audit(zero dependency changes; the current moderate findings exist onupstream/mainitself).Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.The two unchecked Safety boxes are N/A: no UI change of any kind (backend-only diff; zero
apps/**files touched).Negative paths pinned by test: mode ON fails closed on a thrown permission lookup (Error and non-Error), on a null installation id, and on an unknown collaborator; a blank login is denied in either mode; the tests assert by observed API traffic that mode OFF makes zero permission calls for allowlisted actors and that mode ON actually consults the live endpoint where the allowlist used to short-circuit.
UI Evidence
N/A — no UI change (backend only).
Notes
LOOPOVER_PER_REPO_ADMINunset (every existing deployment), every converted site resolves the identical allowlist answer with zero extra API calls — pinned by the mode-OFF tests asserting empty fetch traffic. The flag flips the permission source, not any site's control flow.hasMaintainerOrOwnerPermissionpattern the codebase already trusts on the review-evasion paths.