fix(security): replace pull_request_target with pull_request trigger

[kami619]

Summary

• Replace vulnerable pull_request_target trigger with secure pull_request trigger
• Add explicit PR number in prompt to fix wrong-PR bug
• Update documentation to reflect new security model

Fixes

• RHOAIENG-51622 — Disable pull_request_target to prevent prompt injection
• #324 — PR review posting to wrong pull request

Root Cause

The pr-review-auto-fix.yml workflow used pull_request_target trigger, which:

1. Runs with access to repository secrets even for fork PRs
2. Exposes secrets to potential prompt injection attacks via malicious PR content
3. Combined with ambiguous prompt wording, occasionally posted reviews to wrong PRs

Changes

File	Change
.github/workflows/pr-review.yml	Created — Secure workflow with pull_request trigger
.github/workflows/pr-review-auto-fix.yml	Deleted — Removed vulnerable workflow
.github/AUTOMATED_REVIEW.md	Updated — New security model documentation

Trade-off

Fork PRs no longer receive automated reviews. Contributors should push to username/branch in the main repo instead.

Test Plan

After merge:

1. Open a PR from a branch in the main repo
2. Verify PR Review workflow triggers
3. Verify review comment posts to correct PR
4. (Optional) Verify fork PRs are skipped

---

Co-Authored-By: Claude Opus 4.5 noreply@anthropic.com

[github-actions]

📈 Test Coverage Report

Branch	Coverage
This PR	66.4%
Main	66.4%
Diff	✅ +0%

---

Coverage calculated from unit tests only

[github-actions]

Outdated review (click to expand)

AgentReady Code Review

PR: #328 — fix(security): replace pull_request_target with pull_request trigger
Reviewed by: Claude Sonnet 4.6 via /review-agentready

---

Summary

Category	Status
Security	✅ Major improvement (core fix is correct)
AgentReady Attribute Compliance	⚠️ Minor gaps
Code Quality	✅ Good
Documentation	⚠️ Stale references remain

Score Impact: Neutral to slight positive — security hardening with acceptable trade-off on fork PR coverage.

---

✅ What's Done Well

Critical Security Fix — pull_request_target → pull_request

The core change is correct and important. pull_request_target runs with repository secrets accessible even for fork PRs, creating a prompt injection attack surface when combined with LLM-powered actions. Switching to pull_request is the right call.

Fork filtering via if condition

if: github.event.pull_request.head.repo.full_name == github.repository

This explicit guard correctly skips fork PRs and fails cleanly with a clear reason, rather than silently failing due to missing secrets. This is good defensive coding.

Explicit PR number in prompt

prompt: |
  Run the /review-agentready command on pull request #${{ github.event.pull_request.number }}.
  ...
  Post the review as a comment on PR #${{ github.event.pull_request.number }}.

Good fix for the wrong-PR bug. Grounding the prompt with an explicit PR number eliminates ambiguity when pull_request_target was used with a base SHA checkout that didn't reflect the actual PR context.

allowed_non_write_users: '*' removed

Correct cleanup — this option was compensating for a security limitation that no longer exists with the new approach.

---

🔴 Issues to Address

1. contents: write permission is overly broad (Security)

Attribute: 3.1 Security Best Practices (Tier 3)
Score Impact: −2 points potential

With auto-fix removed, the workflow only needs to post PR comments — it no longer pushes commits to branches. The contents: write permission should be downgraded to contents: read.

# Current (overly permissive):
permissions:
  contents: write
  pull-requests: write
  issues: write

# Recommended:
permissions:
  contents: read
  pull-requests: write
  issues: write

Principle of least privilege applies especially to workflows using LLM-powered actions.

2. Actionlint not verified (CI/CD Compliance)

Attribute: 2.9 CI/CD Best Practices (Tier 2)
CLAUDE.md requirement: "ALWAYS run actionlint and fix any issues before pushing changes to GitHub Actions workflows"

The PR description does not mention running actionlint. Since this modifies GitHub Actions workflows, this is a required step per project standards. Please run:

actionlint .github/workflows/pr-review.yml

---

⚠️ Minor Issues

3. Stale documentation in AUTOMATED_REVIEW.md — Customization section

The Customization section at line ~179 still references pr-review-auto-fix.yml:

Edit `.github/workflows/pr-review-auto-fix.yml`:

This section was not updated in the PR diff. The stale filename reference could confuse users trying to customize the workflow.

Additionally, the "Auto-Fix Not Running" and "Fixes Causing Test Failures" troubleshooting sections appear to still be present in the file (not deleted in the diff). With auto-fix removed, these sections are misleading and should be removed or noted as historical.

4. External JIRA reference in workflow comments

# Fixes: RHOAIENG-51622 (security), GitHub #324 (wrong PR context)
# See: https://issues.redhat.com/browse/RHOAIENG-51622

The Red Hat JIRA link (issues.redhat.com) is a private issue tracker — external contributors cannot access it. The GitHub issue #324 reference is appropriate and sufficient. Consider removing the JIRA reference or replacing it with a public explanation.

5. Comment minimization — emoji change is backward compatible but worth noting

-if (comment.body.includes('🤖 AgentReady Code Review') &&
+if (comment.body.includes('AgentReady Code Review') &&

The change from the emoji-prefixed string to a plain string is actually more robust (substring match will still catch the emoji version). This is a good change, but existing old-format comments will be minimized on first run, which is the desired behavior. No action needed — just confirming the logic is correct.

---

AgentReady Attribute Mapping

Finding	Attribute	Tier	Impact
contents: write overly broad	3.1 Security Best Practices	3	−2 pts
Actionlint not run	2.9 CI/CD Best Practices	2	−1 pt
Stale documentation	1.2 README/Documentation	1	−1 pt

Net assessment: The security benefit of this PR outweighs the minor issues. Once contents: write is narrowed and actionlint passes, this is merge-ready.

---

Recommended Changes Before Merge

1. Change contents: write → contents: read in workflow permissions
2. Run actionlint .github/workflows/pr-review.yml and fix any findings
3. Update the Customization section in AUTOMATED_REVIEW.md (replace pr-review-auto-fix.yml with pr-review.yml)
4. Remove or archive the "Auto-Fix Not Running" and "Fixes Causing Test Failures" troubleshooting sections

---

🤖 Generated with Claude Code via /review-agentready

[github-actions]

🤖 AgentReady Code Review

PR Status: 2 issues found (0 🔴 Critical, 1 🟡 Major, 1 🔵 Minor)
Score Impact: Current 80.0/100 → 80.5/100 potential if issues addressed (security improvements offset compliance gap)
Certification: Gold → Gold (maintained; security hardening adds value)

---

✅ Security Improvements Verified

This PR correctly addresses a critical pull_request_target vulnerability (RHOAIENG-51622 / #324):

Change	Security Impact
pull_request_target → pull_request	Secrets no longer accessible to fork PRs
contents: write → contents: read	Principle of least privilege enforced
Fork check added (head.repo.full_name == github.repository)	Defense-in-depth against fork execution
allowed_non_write_users: '*' removed	Eliminates broad write authorization
Explicit #${{ github.event.pull_request.number }} in prompt	Fixes wrong-PR posting bug (#324)

These changes are correct and well-implemented.

---

🟡 Major Issues (Confidence 80-89) — Manual Review Required

1. Actionlint validation not confirmed

Attribute: CI/CD Workflow Quality (Tier 3: Important)
Confidence: 88%
Score Impact: −0.5 points (if actionlint finds issues in the workflow YAML)
Location: https://github.com/ambient-code/agentready/blob/c180a7bf36f9fc802d07b15b41bdff5cc904e28f/.github/workflows/pr-review.yml

Issue Details:
CLAUDE.md explicitly requires:

"ALWAYS run actionlint and fix any issues before pushing changes to GitHub Actions workflows"
"All workflows must pass actionlint validation with zero errors/warnings"

No evidence of actionlint validation in the PR. The workflow YAML structure appears valid, but this is a mandatory step per project guidelines.

Remediation:

# Install actionlint if not present
brew install actionlint  # or: go install github.com/rhysd/actionlint/cmd/actionlint@latest

# Validate the workflow
actionlint .github/workflows/pr-review.yml

---

🔵 Minor Issues (Confidence 80) — Optional

2. Private JIRA reference inaccessible to external contributors

Attribute: Documentation Quality (Tier 1: Essential)
Confidence: 80%
Score Impact: −0.0 points (documentation clarity, not self-assessment attribute)
Location:

agentready/.github/workflows/pr-review.yml

Line 14 in c180a7b

Issue Details:
The workflow header comment references RHOAIENG-51622 on issues.redhat.com, which is a private Red Hat JIRA inaccessible to external contributors. The corresponding public issue is #324.

# Current (private):
# Security: RHOAIENG-51622

# Suggested (public reference):
# Security: Prevents prompt injection attacks (see #324)

Remediation:

# Replace private JIRA reference with public GitHub issue
sed -i 's/RHOAIENG-51622/GitHub #324/' .github/workflows/pr-review.yml

---

ℹ️ Informational Notes

Checkout ref removal is intentional and correct: The old ref: ${{ github.event.pull_request.base.sha }} was a partial mitigation for pull_request_target risks. With pull_request trigger + the fork check, checking out the merge commit is the correct and expected behavior. The defense-in-depth is provided by the if: condition, not the checkout ref.

dependabot-auto-merge.yml out of scope: That workflow also uses pull_request_target but is not modified by this PR. It has a mitigating actor check (github.actor == 'dependabot[bot]') and is a separate concern.

---

Summary

• Auto-Fix Candidates: 0 (no blocker-level issues)
• Manual Review: 1 major issue (actionlint validation)
• Total Score Improvement Potential: +0.5 points if issues addressed
• AgentReady Assessment: Run agentready assess . after fixes to verify score

This is a well-executed security fix. The core changes are correct, targeted, and address a real vulnerability. The actionlint check is the primary blocker per CLAUDE.md requirements.

---

🤖 Generated with Claude Code

_{If this review was useful, react with 👍. Otherwise, react with 👎.}

[github-actions]

🎉 This PR is included in version 2.29.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀