Skip to content

Support private repositories in the web app - #77

Open
sayakpaul wants to merge 2 commits into
mainfrom
private-repos
Open

Support private repositories in the web app#77
sayakpaul wants to merge 2 commits into
mainfrom
private-repos

Conversation

@sayakpaul

Copy link
Copy Markdown
Member

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 an org/* 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() and GitHubClient.is_collaborator() — the latter tri-state (True/False/None), where None means GitHub gave an inconclusive answer (permission missing, 5xx).
  • A private-repo gate in webapp.py:
    • Public repos: unchanged — any signed-in user may review them and follow anyone's review.
    • Private repos: submitting a review (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 existing author_association check.
    • The lookup runs against the App installation token (Metadata: read), so it needs no extra OAuth scope from the user and isn't defeated by SAML SSO — same reasoning as the existing user_is_org_member fallback.
    • Fail-closed: an unreachable GitHub, a missing installation, or an inconclusive collaborator answer all deny. Transient failures aren't cached.
    • Verdicts are cached 5 minutes per (user, owner, repo) so the check stays off the streaming/publish hot path. That TTL also bounds how long revoked access keeps working.
    • A missing installation on the submit path now surfaces as an actionable 400 up front, instead of the job failing seconds later.

Notes / limitations

  • Repo names and PR numbers of private repos remain visible to all signed-in users on the cross-user /journal page; only review and task content is gated. Called out in the docs; can be tightened separately if wanted.
  • Org membership alone is deliberately not sufficient — being in huggingface does not imply access to every private repo in it.

Docs

docs/web-app.md (new "Private Repositories" section), docs/security.md, a pointer from docs/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_AUTH bypasses; 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

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>
@sayakpaul

Copy link
Copy Markdown
Member Author

@askserge could you review this PR?

@sergereview sergereview Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤗 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.RequestException is not cached (retries on next request), while None (inconclusive) and False (denied) are cached. AppNotInstalledError propagates 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/data endpoint intentionally lists metadata only, which is documented.
  • GitHub client: is_collaborator uses allow_redirects=False to 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

@sayakpaul
sayakpaul requested a review from tarekziade July 28, 2026 08:37
@tarekziade
tarekziade requested a review from paulinebm July 28, 2026 08:46
@tarekziade

Copy link
Copy Markdown
Collaborator

Looks good to me, but I would like @paulinebm to also have a look as well for sec

@tarekziade

Copy link
Copy Markdown
Collaborator

Also, notice that WEB_ALLOWED_USERS and WEB_ALLOWED_ORG are environment variables that we use to bootstrap the system, e.g. when people use the web ui they have to set up what users and orgs can use Serge in their repo already, and that goes into the DB. e.g. we don't change the env variable after initial setup

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>
@sayakpaul

Copy link
Copy Markdown
Member Author

Good catch — my wording described WEB_ALLOWED_USERS / WEB_ALLOWED_ORG as if they were the ongoing access-control gate, which isn't how they're used. Fixed in 526b60e.

The docs (and the code comment above the gate) now describe the layers as they actually work:

  1. Sign-in — the env allowlist, set once at initial setup to bootstrap the deployment; it decides who gets an account, not what they may read.
  2. Provider config — in the DB, set up from the web UI per repo. Decides which key a review runs on. Worth noting explicitly: POST /admin/providers only requires a signed-in user and doesn't validate repo_pattern against the creator, so any account can add a config for any repo pattern and list itself in allowed_users. That makes it key routing rather than a boundary between signed-in users.
  3. GitHub itself — the new collaborator check, for private repos only.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants