Skip to content

fix(security): harden fork workflow-run context - #88

Merged
Sayt-0 merged 7 commits into
mainfrom
fix/aia-16-e2e-fork-pr-security
Aug 21, 2026
Merged

fix(security): harden fork workflow-run context#88
Sayt-0 merged 7 commits into
mainfrom
fix/aia-16-e2e-fork-pr-security

Conversation

@aheritier

@aheritieraheritier commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

This seven-commit security remediation closes the fork workflow_run context-forgery path while preserving direct-review behavior.

  • Removes the privileged fork-PR E2E bridge; credential-bearing E2E routes are trusted-only.
  • Adds a trusted resolver that derives PR/comment context from live GitHub API data, rather than fork artifacts.
  • Validates and binds an immutable 40-hex PR head SHA before review, checkout, and check-run use.
  • Transports canonical context only through a same-run artifact selected by immutable artifact ID.
  • Rebuilds feedback and mention routing from canonical live context, including correct threaded-reply routing.
  • Adds regression/routing-matrix coverage and updates the trust-boundary documentation.

Comment threadsrc/resolve-trigger-context/index.ts Fixed
@aheritier
aheritierforce-pushed the fix/aia-16-e2e-fork-pr-security branch from b6f6c88 to 870b845CompareAugust 18, 2026 12:36
Comment threadsrc/resolve-trigger-context/__tests__/resolve-trigger-context.test.ts Dismissed
@aheritier
aheritier marked this pull request as ready for review August 18, 2026 13:16

@Sayt-0Sayt-0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The threat model is right and the design closes the fork workflow_run forgery path: server-derived context, actor binding, immutable 40-hex SHA, same-run artifact by ID. I traced the main attack chain and it's refuted — a non-member can't borrow a member's comment since comment.user.login must equal run.actor. Removing the privileged E2E bridge is a clear win.

Requesting changes on one blocker and two regressions:

  • BlockerfindAssociatedPr() compares pr.base.repo.full_name, which GET /actions/runs/{id} never returns (pull-request-minimal{id, name, url}). It therefore always throws when pull_requests is non-empty, i.e. for every same-repo PR. Forks still work via the pulls.list fallback, but this repo's own dogfooding and the documented 2-workflow pattern break 100% of the time once released. Repro inline. CI is green only because the unit fixture invents the field and the handwritten interfaces hide it from tsc.
  • SHA race — the headSha !== runHeadSha hard failure turns an ordinary push into a review lost with no PR-visible signal, and the documented trigger has no synchronize to re-fire it.
  • Lost inline contextCanonicalComment drops path/line/diff_hunk, so fork mentions render empty FILE_PATH=/LINE= and fork feedback records File: unknown (line ?). These come from the live comment, so restoring them costs nothing security-wise.

Smaller items inline: the routing-matrix test is tautological, SKILL.md's trigger template has no reviewer gate, AGENTS.md layout, migration/rollback ordering.

One option to land this fast: blocker + SHA race + a realistic fixture here, the rest as an immediate follow-up.

Checked: 978 unit tests / biome / tsc / actionlint all green (no coverage of the blocker); ~10 read-only gh api calls on live runs to establish the real pull_requests[] shape; standalone repro with only @octokit/rest stubbed.

Comment threadsrc/resolve-trigger-context/resolve-trigger-context.ts Outdated
Comment threadsrc/resolve-trigger-context/resolve-trigger-context.ts Outdated
Comment threadsrc/resolve-trigger-context/__tests__/resolve-trigger-context.test.ts Outdated
Comment threadsrc/resolve-trigger-context/resolve-trigger-context.ts Outdated
Comment threadsrc/resolve-trigger-context/resolve-trigger-context.ts
Comment thread.github/workflows/review-pr.yml Outdated
Comment thread.github/workflows/review-pr.yml Outdated
Comment threadsrc/resolve-trigger-context/__tests__/workflow-security.test.ts Outdated
Comment threadAGENTS.md
- Uses a **best-effort cache lock** (`pr-review-lock-<repo>-<pr>-*` cache key) to avoid concurrent reviews on the same PR. Completed runs release the lock by saving a `-released` marker cache entry that shadows their lock entry (cache saves work regardless of token scopes; the REST cache DELETE is best-effort cleanup only). The 3600s TTL is a fallback for crashed holders and must stay above the review agent's 2700s wall-clock budget (45 min, enforced by the root action's `total-timeout` across all attempts) so an in-flight review is never treated as stale. Reviews are idempotent so the small race window is acceptable.
- **Memory persistence** uses `actions/cache` keyed by `pr-review-memory-<repo>-<job>-<run_id>` with prefix-based restore. The DB lives at `${{ github.workspace }}/.cache/pr-review-memory.db`.
- **Memory persistence** uses `actions/cache` keyed by `pr-review-memory-<repo>-<job>-<run_id>` with prefix-based restore. The review memory database lives at `${{ github.workspace }}/.cache/pr-review-memory.db`.
- **Fork workflow-run private context files** are canonicalized from GitHub API data. Trigger artifacts are untrusted locators only; server-derived PR/comment data and an immutable 40-hex SHA drive authorization, prompts, posting, and checkout. Attempt-specific randomized `runner.temp` roots are `0700`; the resolver exclusively creates canonical JSON at `0600`, and a pre-upload guard verifies containment, non-symlink status, and exact modes. Isolated consumers select the same-run artifact by immutable ID, verify its digest, then restore and verify `0700/0600` because artifact modes are not preserved. Canonical-derived files are exclusively created at `0600` in the same private job root. Never use predictable shared `/tmp` paths for locator, canonical, or derived trigger context; unrelated reviewed runtime temporary files are outside this invariant. The artifact name includes the run ID and run attempt to avoid rerun collisions. If the pinned bundle has no resolver, workflow-run routes skip fail-closed while direct routes continue.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While editing AGENTS.md: the "Repo layout" tree above lists every src/<name>/ helper, but src/resolve-trigger-context/ is missing (grep -c resolve-trigger-context AGENTS.md → 0). Worth adding an entry next to src/rate-limit/, in the same index.ts / <name>.ts / __tests__/ style.

Also, this paragraph plus the ones in SECURITY.md and review-pr/README.md restate the same 0700/0600 invariants three times in dense single blocks. One authoritative description with links from the other two would age better.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Fixed in eca6cad: the missing resolve-trigger-context layout entry is now documented. Optional 0700/0600 documentation de-duplication is deferred; this thread is intentionally left unresolved.

Comment threadreview-pr/README.md Outdated

@docker-agentdocker-agent 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.

Assessment: 🟢 APPROVE

This PR introduces a well-designed security overhaul replacing the insecure fork workflow pattern (raw artifact data passed into privileged jobs) with a server-side canonical trigger context resolver. The core security properties are structurally sound:

  • Workflow run metadata is fetched live from the GitHub API (status, repo, SHA cross-checked)
  • The canonical context is written to a mode-600 file in a private runner.temp directory
  • Artifact transport uses immutable numeric artifact IDs, not mutable names
  • Downstream jobs guard against symlinks and verify permissions before consuming context
  • ${{ interpolation in run: bodies is eliminated; immutable resolved SHAs replace refs/pull/N/head checkouts

No high- or medium-severity bugs were found. Only low-severity observations remain (misleading error message when both GITHUB_APP_TOKEN and GITHUB_TOKEN are unset; redundant chmodSync after writeFileSync with the same mode; requireRunId accepting leading-zero strings — all harmless). The security model is correct and the PR is safe to merge.

Comment thread.github/workflows/review-pr.yml
@aheritier
aheritierforce-pushed the fix/aia-16-e2e-fork-pr-security branch from 870b845 to 6348707CompareAugust 20, 2026 14:17
@aheritier
aheritierforce-pushed the fix/aia-16-e2e-fork-pr-security branch from 6348707 to e15a069CompareAugust 20, 2026 20:18
@aheritier
aheritierforce-pushed the fix/aia-16-e2e-fork-pr-security branch from e15a069 to eca6cadCompareAugust 20, 2026 20:53
Comment threadsrc/resolve-trigger-context/__tests__/workflow-security.test.ts Dismissed
@aheritier
aheritier requested a review from Sayt-0August 20, 2026 21:36
@Sayt-0
Sayt-0 merged commit 3776045 into mainAug 21, 2026
34 checks passed
@Sayt-0
Sayt-0 deleted the fix/aia-16-e2e-fork-pr-security branch August 21, 2026 10:06
Sign up for freeto 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.

5 participants

@aheritier@github-advanced-security@Sayt-0@docker-agent@mickael-docker