Support private repositories in the web app - #77
Conversation
Private repos already clone and review fine through the App installation token — what was missing was authorization. The web UI gated only on the login allowlist, so any signed-in user could submit a review for any private repo the App can see, and webhook/task jobs were readable by every signed-in user regardless of the target repo. Add a fail-closed access check for private repos: submitting a review, and opening an existing review or task, now require the user to be a collaborator on the target repo. The lookup is App-side (installation token + Metadata: read), so it needs no extra OAuth scope and isn't defeated by SAML SSO. Verdicts are cached 5 minutes per (user, repo). Public repos keep the previous posture. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@askserge could you review this PR? |
There was a problem hiding this comment.
🤗 Serge says:
This PR adds a well-designed, fail-closed authorization gate for private repositories in the web app. The implementation correctly uses the GitHub App installation token (Metadata: read) to check collaborator status without requiring extra OAuth scopes, caches verdicts with a 5-minute TTL, and handles edge cases (transient failures, missing installations, inconclusive answers) safely. Tests are comprehensive and cover both submit and view paths for reviews and tasks.
Key observations:
- Fail-closed behavior:
requests.RequestExceptionis not cached (retries on next request), whileNone(inconclusive) andFalse(denied) are cached.AppNotInstalledErrorpropagates to callers which surface appropriate 400/403 responses. - Cache design: Simple dict with a 512-entry cap and full-clear eviction; thread-safe via
threading.Lock. Case-normalized keys prevent case-sensitivity bypasses. - Endpoint coverage: The gate is applied consistently to all content-serving JSON endpoints (
/reviews/.../info,/draft,/stream,/publish,/discard,/tasks/.../info,/stream). Static HTML pages (/reviews/...,/tasks/...) and the machine-facing OIDC endpoint (/tasks/.../status) correctly remain ungated. The cross-user/journal/dataendpoint intentionally lists metadata only, which is documented. - GitHub client:
is_collaboratorusesallow_redirects=Falseto avoid following302s into misleading answers, and the tri-state return (True/False/None) is handled correctly by callers.
No issues found. The code is ready to merge.
serge v0.1.0 · model: moonshotai/Kimi-K2.6 · 27 LLM turns · 26 tool calls · 100.2s · 488854 in / 15330 out tokens
|
Looks good to me, but I would like @paulinebm to also have a look as well for sec |
|
Also, notice that |
WEB_ALLOWED_USERS / WEB_ALLOWED_ORG bootstrap a deployment at setup time and aren't edited afterwards; per-repo grants live in the provider configs in the DB, edited from the web UI. Neither authorizes access to a repository — a provider config can be created by any signed-in user for any repo pattern — which is the gap the collaborator gate closes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Good catch — my wording described The docs (and the code comment above the gate) now describe the layers as they actually work:
Point 2 is really the reason this PR exists: with the first two layers being setup-time and self-service respectively, GitHub is the only thing that actually knows who may read a private repo. Nothing changed in the gate's behavior — docs and one comment only. |
What
Makes private repositories usable in serge.
The plumbing already works: clones and API calls both go through the GitHub App installation token, so a private repo the App is installed on reviews fine. What was missing is authorization — the web UI only gated on the login allowlist (
WEB_ALLOWED_USERS/WEB_ALLOWED_ORG) plus a provider config's repo pattern. With anorg/*config, any signed-in org member could submit a review for any private repo the App can see, and webhook- or task-triggered jobs were readable by every signed-in user regardless of target repo. That is fine while everything reviewed is public; it is not fine once private code is in scope.Changes
GitHubClient.get_repo()andGitHubClient.is_collaborator()— the latter tri-state (True/False/None), whereNonemeans GitHub gave an inconclusive answer (permission missing, 5xx).webapp.py:POST /reviews), and opening an existing review or task, require the user to be a collaborator on the target repo. This is the UI counterpart of the webhook's existingauthor_associationcheck.Metadata: read), so it needs no extra OAuth scope from the user and isn't defeated by SAML SSO — same reasoning as the existinguser_is_org_memberfallback.(user, owner, repo)so the check stays off the streaming/publish hot path. That TTL also bounds how long revoked access keeps working.Notes / limitations
/journalpage; only review and task content is gated. Called out in the docs; can be tightened separately if wanted.huggingfacedoes not imply access to every private repo in it.Docs
docs/web-app.md(new "Private Repositories" section),docs/security.md, a pointer fromdocs/github-app.md, and a CHANGELOG entry.Tests
New
tests/test_webapp_private_repos.py(14 tests): public repos skip the collaborator call; collaborator allowed / non-collaborator denied / inconclusive denied; GitHub errors deny without poisoning the cache; caching is per user;DEV_NO_AUTHbypasses; submit returns 403 / 400 as appropriate and never queues a worker; webhook reviews and tasks on a private repo 403 for non-collaborators while public ones stay readable.Full suite: 506 passed.
🤖 Generated with Claude Code