security: replace pull_request_target with two-workflow pattern (fixes #472)

[CodingAngel1]

Summary

Resolves #472 — the pull_request_target trigger in .github/workflows/auto-review.yml exposes secrets.OWNER_PAT and secrets.GROQ_API_KEY to code running in the context of fork PRs, enabling supply-chain attacks.

This PR removes the vulnerable workflow and replaces it with the GitHub-recommended two-workflow pattern.

---

The Vulnerability (CVE class: pwn-request / poisoned pipeline execution)

Root cause

pull_request_target is a privileged event: it always runs in the context of the base repository, giving the workflow full access to repository secrets, even when the triggering commit comes from a fork. GitHub's documentation explicitly warns:

"If the pull_request_target event is used with the pull_request trigger, the workflow will have write access to the target repository's resources, including secrets."

What the old workflow did

on:
  pull_request_target:            # <- privileged event
    types: [opened, synchronize, ...]

permissions:
  contents: write
  pull-requests: write

env:
  GROQ_API_KEY: ${{ secrets.GROQ_API_KEY }}   # <- secret injected
  GH_TOKEN:     ${{ secrets.OWNER_PAT }}       # <- PAT injected

A malicious contributor could craft a PR that:

1. Modifies the workflow itself (before it was disabled) or any script invoked by it.
2. Causes the runner to exfiltrate GROQ_API_KEY and OWNER_PAT to an attacker-controlled endpoint — e.g., via a curl in a Makefile, a Cargo build script, or a modified shell helper.
3. Uses the leaked OWNER_PAT (which has admin-level PR write access) to approve, merge, or close arbitrary PRs, inject malicious commits, or pivot to other repositories the token can access.

The workflow was disabled (.disabled suffix) but the file remained in the repository; re-enabling it (intentionally or accidentally) would immediately reintroduce the vulnerability.

---

The Fix — Two-Workflow Pattern

The core principle: never mix untrusted code execution with secret access.

Workflow 1 — pr-collector.yml (unprivileged)

Property	Value
Trigger	pull_request
Runs in context of	fork commit (untrusted)
Secrets available	None — only ephemeral GITHUB_TOKEN (read-only)
What it does	Fetches PR metadata + diff via gh pr view/diff, serialises to JSON, uploads as a GitHub Actions artifact keyed by PR number
Untrusted code executed?	No — actions/checkout checks out the base SHA, not the fork head

By using ref: ${{ github.event.pull_request.base.sha }} in the checkout step, even the filesystem is free of attacker-controlled code.

Workflow 2 — pr-review.yml (privileged, triple-gated)

Property	Value
Trigger	pull_request_review (submitted)
Runs in context of	Base repository (safe for secrets)
Secrets available	GROQ_API_KEY, OWNER_PAT — but only after all three guards pass
What it does	Downloads the artifact, calls Groq, posts review, optionally merges/closes

The three guards (all must pass before any secret is touched):

1. Review state — the triggering review must be a real submission (approved / changes_requested / commented). Dismissed reviews are skipped.
2. Trigger phrase — the review body must contain the literal string /run-auto-review. This prevents the Groq API call from firing on every review comment.
3. Trusted reviewer — the reviewer must either:
   • Appear in the TRUSTED_REVIEWERS repository variable (comma-separated login list), or
   • Hold write or admin collaborator permission on the repository (verified live via the GitHub API).

If any guard fails, the workflow logs the reason and exits without touching secrets.

Data flow

Fork PR opened / updated
        |
        v
+-----------------------------------------------------+
|  pr-collector.yml  (pull_request — NO secrets)      |
|  * Checks out base SHA (not fork code)              |
|  * Fetches metadata + diff via GITHUB_TOKEN         |
|  * Uploads artifact: pr-{number}/                   |
|    +-- pr-meta.json                                 |
|    +-- pr-diff-truncated.txt                        |
|    +-- issues-context.txt                           |
+-----------------------------------------------------+
                            |  artifact
                            v
  Trusted maintainer leaves review with `/run-auto-review`
                            |
                            v
+-----------------------------------------------------+
|  pr-review.yml  (pull_request_review — HAS secrets) |
|  Guard 1: review state is actionable?               |
|  Guard 2: body contains /run-auto-review?           |
|  Guard 3: reviewer is trusted?                      |
|       +- NO to any -> exit, log reason              |
|       +- YES to all -> download artifact            |
|            -> call Groq (GROQ_API_KEY)              |
|            -> post review / merge / close (OWNER_PAT)|
+-----------------------------------------------------+

---

Files Changed

File	Action	Reason
.github/workflows/auto-review.yml.disabled	Deleted	Removes the vulnerable workflow permanently
.github/workflows/pr-collector.yml	Added	Unprivileged collector (Workflow 1)
.github/workflows/pr-review.yml	Added	Privileged reviewer, triple-gated (Workflow 2)

No application code, tests, or CI configuration was changed.

---

Operational Notes for Maintainers

One-time setup

1. Set the TRUSTED_REVIEWERS repository variable (Settings → Secrets and variables → Actions → Variables):

   Name:  TRUSTED_REVIEWERS
   Value: your-github-login,other-maintainer
   

   If this variable is absent, the workflow falls back to checking collaborator write/admin permission — which is still safe, just slightly broader.

2. Ensure GROQ_API_KEY and OWNER_PAT remain configured as repository secrets (they are consumed exactly as before, just in the new privileged workflow).

Triggering a review

After a PR lands and pr-collector.yml completes (the artifact is uploaded), a trusted maintainer submits a review comment containing /run-auto-review. The privileged workflow fires, downloads the artifact, and proceeds exactly as the old workflow did.

Artifact retention

Artifacts are retained for 7 days — enough for any review cycle. After that the artifact is deleted automatically by GitHub; re-triggering will fail gracefully with a download-artifact error.

---

Security Properties After This Change

Threat	Before	After
Fork PR exfiltrates OWNER_PAT	Possible	Impossible — PAT only used in privileged workflow, never touches fork code
Fork PR exfiltrates GROQ_API_KEY	Possible	Impossible — same isolation
Attacker merges arbitrary PRs via leaked PAT	Possible	Impossible
Reviewer impersonation triggers Groq call	N/A	Prevented — collaborator permission check
Automated trigger flooding Groq API	N/A	Prevented — explicit trigger phrase required

---

Testing

The CI workflow (ci.yml) is unaffected and continues to run cargo fmt, cargo clippy, and cargo test on every PR. The new workflow files are YAML-only; no Rust code was changed.

Manual verification checklist (for the reviewer):

• pr-collector.yml has no secrets.* references
• pr-review.yml only references secrets after all three guards evaluate to true
• auto-review.yml.disabled is absent from the repository
• pr-collector.yml checks out base.sha, not head.sha

Closes #472

[drips-wave]

@CodingAngel1 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

[davidmaronio]

the security model is sound, splitting the pull_request_target flow into an unprivileged pr-collector (checks out base.sha, no secrets) + a privileged pr-review gated on trigger phrase + trusted reviewer correctly closes the pwn-request in #472, nice. a few fixes before it works end to end:

1. the artifact hand-off is likely broken, pr-review uses actions/download-artifact@v4 with just name: pr-<num> and no run-id/github-token, v4 only looks in the current run and the collector uploads in a separate pull_request run, so the download won't find it. pass run-id + github-token (or use dawidd6/action-download-artifact).
2. minor: in the collector's "write metadata json" step the pr title/body are interpolated straight into the jq command via ${{ }}, move them into env: to avoid script-injection (low impact since the collector is unprivileged, but worth it).
3. there's leftover hackathon prompt text ("Lernza", "EXTREMELY LENIENT", "ci has been disabled") in the groq system prompt, clean that out.
   the required fmt/clippy/test haven't run yet (first-time contributor), i'll approve the run.

[CodingAngel1]

@davidmaronio - All four points addressed in the latest commit (cc4375b):

Artifact hand-off — swapped actions/download-artifact@v4 for dawidd6/action-download-artifact@v6 with github_token and search_artifacts: true, so the privileged workflow can locate the artifact uploaded by the separate collector run.

Script injection — moved all twelve jq --arg values in the "Write metadata JSON" step out of inline ${{ }} expressions and into env: vars. Agreed it's low impact given the collector is unprivileged, but clean is clean.

Stale prompt text — removed the Lernza/hackathon/CI-disabled lines from the Groq system prompt and replaced them with a ProofOfHeart-specific identity. The fraud-detection logic itself is unchanged.

CI runs — ready whenever you approve them, thanks.

[davidmaronio]

re-reviewed, and you've addressed everything from my last pass, nice work:

• the cross-run artifact fetch now uses dawidd6/action-download-artifact@v6, so pr-review can actually pull the artifact that pr-collector uploaded in its separate run.
• the pr title/body are now passed as env vars (PR_TITLE/PR_BODY) and marshalled with jq -n --arg, so no more ${{ }} interpolation into shell/prompt bodies, script-injection closed.
• the stray "EXTREMELY LENIENT / Lernza" prompt text is gone.
• and the split is sound, pr-collector on pull_request checks out base.sha (untrusted code never runs privileged) and pr-review on pull_request_review holds the secrets.
  the reason it showed BLOCKED with no checks was just that GitHub was holding the workflow runs pending maintainer approval (standard for a pr that edits .github/workflows) plus my prior changes-requested, ci.yml itself is untouched so the fmt/clippy/test contexts are fine. i've approved the held runs; once they come back green this is good to merge. approving from my side.

[davidmaronio]

update: main was actually broken this whole time, not your pr. PR #574 (per-campaign tokens / milestone withdrawals / emergency pause) got merged on 07-01 while its CI was red and it didn't compile, so every branch that rebased or merged main inherited those exact errors (the duplicate set_emergency_pause_signers, the CreateCampaignParams missing-fields cascade, iter_mut, etc). that wasn't your merge, it was the base. i've reverted #574 in #584 and main is green again. so please rebase onto the current main and those inherited errors will clear: git fetch origin && git rebase origin/main && git push --force-with-lease. (#574's feature will be re-submitted properly.) sorry for the earlier note pinning it on your rebase.

[CodingAngel1]

@davidmaronio thank you so much for your patience. The PR has been rebased and awaiting you to run allow it run the CI and merge. Thanks for the heads up and for reverting in #584

[CodingAngel1]

@davidmaronio the adjustments have been made for your review and merge. Thank you.

[CodingAngel1]

@davidmaronio CI Fix Pushed to the PR, the commit is in b3d146c . Pleae review and merge. Thank you