Branch staging: promote staged attachments for fork PRs #317

Activity

1. zachdunn commented on Jul 21, 2026

   

   Zach Dunn (zachdunn)

   on Jul 21, 2026

   MemberAuthor

   More actions

   Trust-model analysis for cross-repo (fork) promotion, from a read-through of the shipped implementation.

   What the code actually does today

   Webhook skip. apps/api/src/github-webhook.ts::autoPromoteAndComment:

   if (headRepo.toLowerCase() !== repo.toLowerCase()) return;

   repo is payload.repository.full_name (the base repo); headRepo is payload.pull_request.head.repo.full_name. For a fork PR these differ, so the function returns immediately — no lookup, no promote, no comment.

   When they match: findRepoLink(env.DB, repo) → loadWorkspaceRecord → promoteBranchAttachments(env, ws, link.workspaceName, { repo, num, branch: headRef }) → gatherCommentBody → upsertBotComment using an installation token resolved via installationForRepo(env, cfg, repo) (base repo only).

   Staged-prefix key layout. packages/uploads/src/github.ts::ghBranchKeyPrefix(repo, branch) → gh/<owner>/<name>/branch/<branch>/, mirrored server-side in apps/api/src/github-promote.ts::stagedPrefix. The owner/name is whatever --repo (or git-remote inference) resolved at attach --branch time, and the objects live in the authenticated CLI workspace's own R2 bucket. So a fork-based agent stages under gh/<fork-owner>/<fork-name>/branch/<branch>/ inside its own workspace's storage — a different bucket/prefix pair than the base repo's workspace entirely.

   github_repo_links binding. apps/api/src/github-repo-links.ts: keyed by lowercased repo_full_name → workspace_name, first-claim-wins via INSERT OR IGNORE. A fork and its upstream are two entirely distinct rows — nothing in the schema associates them.

   Comment/render trust boundary (#290). apps/api/src/github-comment.ts: the server renders ONLY from the calling workspace's own data (its own bucket/prefix, its own D1 rows). upsertBotComment posts as the GitHub App installation into the base PR thread, with a namespaced marker ws=<workspaceName> naming the base repo's bound workspace.

   Installation scoping. installationForRepo (apps/api/src/github-app.ts) resolves the installation via GET /repos/:repo/installation — the App is installed on the upstream, not on a contributor's fork, so only the base repo's installation token is available to write the comment.

   1. Actors and what each can currently cause

   Actor	Can currently cause
   Base-repo workspace (owns the github_repo_links row for the upstream)	Its own staged files get auto-promoted into any same-repo PR, under its ws= marker. No ability to have fork-staged content promoted.
   Fork-owner workspace	Can stage under its own bucket and call /github/promote / /github/link for the fork's coordinates — but the webhook never fires for the fork, so its staged content is invisible to the upstream flow.
   PR author	Fully controls pull_request.head.ref / head.repo.full_name in the payload — attacker-controlled strings whenever the PR comes from a fork. Today they only feed the guard condition.
   Webhook/installation	Delivers the HMAC-verified event and mints the base repo's installation token only; no concept of the fork's identity or credentials.

   2. Attack/abuse surface of naive cross-repo promotion

   If the webhook, on headRepo !== repo, looked up a binding for the head repo and promoted across into the base PR's prefix:

   • Content injection under the base workspace's identity. Promoted images appear in the App-authored comment under the base workspace's ws= marker — content chosen by an arbitrary fork-owner workspace, looking vetted by the base workspace. Any external contributor buys a write path into the upstream's official managed comment just by opening a PR.
   • Fork-binding squatting. Bindings are first-claim-wins and forks are unclaimed by default; an attacker trivially claims their own fork's binding, then opens PRs the naive design treats as authoritative. No relationship check ties the fork's workspace to the base repo.
   • Cross-workspace read/write coupling. promoteBranchAttachments reads and writes within a single workspace's storage. Cross-repo promotion needs a fork-bucket → base-bucket copy — a primitive that exists nowhere in the codebase, and exactly the boundary gatherCommentBody's doc-comment declares sacred.
   • Quota/cap abuse. PROMOTE_STAGED_CAP (50) and storage accounting are per-workspace. Copy into the base bucket and the base workspace's quota is consumed by data it didn't stage; link out to the fork's bucket instead and the upstream's official comment serves content out of third-party storage it can't moderate or take down.
   • Freshness gaming. isFresh treats a missing/malformed staged-at as fresh — combined with cross-repo trust, stale or forged staged objects could resurface in a live upstream PR.
   • The "real access to the repo" assumption disappears. Same-repo binding races are tie-breaks between parties who both have write access. Anyone can fork.

   3. Candidate trust models

   (a) Same-workspace-only — promote only if findRepoLink(headRepo) resolves to the same workspace as the base repo's binding.
   Safe (no cross-tenant read; it's one workspace with two repo keys) and cheap (one extra lookup + equality check; pass headRepo/headRef as source coordinates). But it doesn't solve the stated problem: an external contributor's fork is, by construction, a different workspace. Only helps a maintainer/org agent staging from its own fork.

   (b) Attributed fork section — the fork's workspace gets its own clearly-marked section (or second managed comment), rendered entirely from its own bucket and D1 rows, posted via the base repo's installation token with explicit "staged from fork <owner>/<name> via workspace <slug>" attribution.
   Preserves the #290 invariant exactly — each workspace only ever renders its own data; no cross-workspace copy primitive. Costs a second marker variant, a second gatherCommentBody/promoteBranchAttachments pass, and comment-render work to keep sections distinct. Bends the "1 comment = 1 workspace" invariant (#315) into one-comment-per-contributing-workspace. Identity is still self-claimed (first workspace to bind the fork), just honestly labeled.

   (c) Opt-in per-repo fork allowlist — base workspace explicitly allowlists fork repos/workspaces (new table, admin-gated CRUD mirroring the #264 scope tier); webhook checks base link + fork link + allowlist before promoting.
   Correct security posture but no zero-setup fork support, and it forces the cross-workspace copy problem (or embedding third-party-hosted URLs in the upstream's comment).

   (d) Keep the v1 skip — zero new surface, zero schema; the fork gap simply persists as a silent no-op.

   4. Recommendation

   Keep (d) now; build (b) when the trigger fires. (a) is cheap but misses the use case; (c) requires inventing a cross-workspace copy primitive that cuts against the codebase's central trust boundary. (b) solves the fork case while keeping every workspace rendering only its own data, and degrades safely — no fork binding, no change from today.

   Minimal changes for (b) when warranted: no new table (github_repo_links is already row-per-repo); branch instead of early-return in autoPromoteAndComment (second, independent promote+gather pass scoped to the fork's workspace and headRepo/headRef); extend attachmentsMarker/attachmentsCommentBody with an attributed fork section reusing the #315 per-workspace marker pattern; posting identity unchanged (base installation token).

   Trigger: the "low priority until fork workflows show up" stance holds, refined — watch for evidence of silently dropped fork promotions (webhook logs where head ≠ base with staged content, or "attach did nothing" reports on fork PRs), not fork PRs in general.

   One gap none of the sketched models cover, (b) included: the fork's binding must not be mintable as a side effect of an unauthenticated promote call. Before shipping even (b), require the fork binding to come from an explicit authenticated /github/link claim (or enforce a minimum binding age) so an attacker can't manufacture the trust signal by simply being first to touch attacker-fork/name.