Skip to content

fix: gh-aw skill portability — 3-model audit findings #771

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
PureWeen wants to merge 2 commits into
base: main
Choose a base branch
from
+36 −30
Open

fix: gh-aw skill portability — 3-model audit findings #771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32a4ebd
fix: make gh-aw skill fully portable — 3-model audit findings
PureWeen Apr 25, 2026
b014ec5
fix: final portability cleanup from 3-model review
PureWeen Apr 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions .claude/skills/gh-aw-guide/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,13 +434,12 @@ tools:

Supported runtimes: `node`, `python`, `go`, `uv`, `bun`, `deno`, `ruby`, `java`, `dotnet`, `elixir`.

## When to Read the Full Reference

## Further Reading

See the [official gh-aw documentation](https://gh.io/gh-aw) for:
- **Execution model** details (step ordering, credential availability, pre-agent-steps/post-steps)
- **Security boundaries** (defense layers, integrity filtering, protected files)
- **Fork PR handling** (platform restore, threat model, trigger-by-trigger behavior)
- **Safe outputs** (complete list of 30+ types, key options for each)
- **Troubleshooting** specific errors
For deep-dive details on execution model, security boundaries, fork handling, safe output types, and known issues, see [`references/architecture.md`](references/architecture.md).

See also the [official gh-aw documentation](https://gh.io/gh-aw) for:
- **Triggers** — complete trigger reference with activity types
- **Frontmatter** — all configuration options
- **Safe outputs** — complete list of 30+ types, key options for each
- **Integrity filtering** — content trust hierarchy and configuration
45 changes: 26 additions & 19 deletions .claude/skills/gh-aw-guide/references/architecture.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# gh-aw Architecture & Security Reference

Deep reference for gh-aw execution model, security boundaries, fork handling, safe outputs, and troubleshooting. Read this file when the SKILL.md quick-start and common patterns aren't sufficient.
Deep reference for gh-aw execution model, security boundaries, fork handling, safe outputs, and troubleshooting. Read this file when the quick-start guide's common patterns aren't sufficient.

## Execution Model

Expand Down Expand Up @@ -57,7 +57,7 @@ The prompt is built in the **activation job** via `{{#runtime-import .github/wor

By default, `gh aw compile` automatically injects a fork guard into the activation job's `if:` condition: `head.repo.id == repository_id`. This blocks fork PRs on `pull_request` events.

To **allow fork PRs**, add `forks: ["*"]` to the `pull_request` trigger in the `.md` frontmatter. The compiler removes the auto-injected guard from the compiled `if:` conditions. This is safe when the workflow uses the `Checkout-GhAwPr.ps1` pattern (checkout + trusted-infra restore) and the agent is sandboxed.
To **allow fork PRs**, add `forks: ["*"]` to the `pull_request` trigger in the `.md` frontmatter. The compiler removes the auto-injected guard from the compiled `if:` conditions. This is safe when the workflow uses a checkout-then-restore pattern (checkout + trusted-infra restore) and the agent is sandboxed.

---

Expand All @@ -67,7 +67,7 @@ To **allow fork PRs**, add `forks: ["*"]` to the `pull_request` trigger in the `

1. **Never execute untrusted PR code with elevated credentials.** The classic "pwn-request" attack is `pull_request_target` + checkout PR + run build scripts with `GITHUB_TOKEN`. The attack surface includes build scripts (`make`, `build.ps1`), package manager hooks (`npm postinstall`, MSBuild targets), and test runners.

2. **Treating PR contents as passive data is safe.** Reading, analyzing, or diffing PR code is fine — the danger is *executing* it. Our gh-aw workflows read code for evaluation; they never build or run it.
2. **Treating PR contents as passive data is safe.** Reading, analyzing, or diffing PR code is fine — the danger is *executing* it. gh-aw workflows should read code for evaluation but never build or run it.

3. **`pull_request_target` grants write permissions and secrets access.** This is by design — the workflow YAML comes from the base branch (trusted). But any step that checks out and runs fork code in this context creates a vulnerability.

Expand Down Expand Up @@ -122,7 +122,7 @@ tools:
| `none` | All content including `FIRST_TIMER` and no-association users |
| `blocked` | Users in `blocked-users` — always denied, cannot be promoted |

**Recommendation for our workflows:** Omit `min-integrity` and rely on the automatic runtime lockdown (see Known Issue above). The `determine-automatic-lockdown` step applies appropriate integrity levels based on event type and actor trust.
**Recommendation:** Omit `min-integrity` and rely on the automatic runtime lockdown (see Known Issue above). The `determine-automatic-lockdown` step applies appropriate integrity levels based on event type and actor trust.

### Protected Files (Auto-Enabled)

Expand All @@ -143,7 +143,7 @@ Configure behavior with `protected-files:` on the safe output:

- ✅ **DO** treat PR contents as passive data (read, analyze, diff)
- ✅ **DO** run data-gathering scripts in `steps:` (pre-agent, trusted context) not inside the agent
- ✅ **DO** use `Checkout-GhAwPr.ps1` for `workflow_dispatch` to restore trusted `.github/` from base
- ✅ **DO** implement a checkout-then-restore step for `workflow_dispatch` to restore trusted `.github/` from base
- ✅ **DO** narrow `slash_command: events:` to the minimum needed (e.g., `[pull_request_comment]`)
- ✅ **DO** use `cancel-in-progress: false` for `slash_command:` workflows to prevent non-matching events from killing in-progress agent runs
- ✅ **DO** prefer `slash_command:` or `schedule` over `pull_request` trigger — `pull_request` causes the "Approve and run" gate that approves ALL workflows with a single click
Expand Down Expand Up @@ -204,7 +204,7 @@ The platform now **automatically preserves `.github/` and `.agents/` from the ba
**Remaining risks (not fixed by #23769):**
- `steps:` and `pre-agent-steps:` that execute workspace code after checkout still run with `GITHUB_TOKEN` — if they run fork PR scripts, it's a pwn-request
- The agent container has `COPILOT_TOKEN` in the environment — build commands (`dotnet build`, `npm install`) executed by the agent on fork PR code can read it via build hooks
- `workflow_dispatch` skips `checkout_pr_branch.cjs` entirely — use `Checkout-GhAwPr.ps1` for defense-in-depth
- `workflow_dispatch` skips `checkout_pr_branch.cjs` entirely — implement a checkout-then-restore step for defense-in-depth
- **Multi-repo `push_to_pull_request_branch`** (fixed v0.70.0): Previously, git operations were scoped to the wrong working directory in multi-repo checkout patterns. This is now fixed — side-repo push targets the correct directory automatically. Recompile affected workflows.

### Dangerous Triggers Checklist
Expand Down Expand Up @@ -259,29 +259,36 @@ Use this checklist when reviewing any workflow that uses high-risk triggers. The

### Safe Pattern: Checkout + Restore

Use the shared `.github/scripts/Checkout-GhAwPr.ps1` script, which implements checkout + restore in a single reusable step:
Implement a checkout-then-restore step for `workflow_dispatch` workflows that evaluate PR branches. This pattern verifies write access, checks out the PR, and restores trusted agent infrastructure from the base branch:

```yaml
steps:
- name: Checkout PR and restore agent infrastructure
env:
GH_TOKEN: ${{ github.token }}
PR_NUMBER: ${{ github.event.pull_request.number || inputs.pr_number }}
run: pwsh .github/scripts/Checkout-GhAwPr.ps1
run: |
# 1. Verify PR author has write access, reject forks
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Apr 25, 2026

Choose a reason for hiding this comment

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

🟡 MODERATE — Comment claims "reject forks" but code doesn't

The comment says # 1. Verify PR author has write access, reject forks but the code only checks the permission level. The old script explicitly rejected fork PRs as a separate check. The write-access check does not fully subsume fork rejection: a collaborator with write access can still submit a PR from their personal fork, and that PR's .github/ would be fork-controlled until the restore step runs.

This is partially mitigated by the restore in step 4 (which overwrites .github/ from the base branch), but if the restore silently fails (see the || true finding above), the fork-controlled .github/ survives.

Suggested fix: Add an explicit fork check before the permission check:

HEAD_REPO=$(gh pr view "$PR_NUMBER" --json headRepository --jq '.headRepository.owner.login')
BASE_REPO=$(gh pr view "$PR_NUMBER" --json baseRepository --jq '.baseRepository.owner.login')
if [[ "$HEAD_REPO" != "$BASE_REPO" ]]; then
  echo "::error::Fork PRs not allowed via workflow_dispatch"
  exit 1
fi

Or remove "reject forks" from the comment to avoid being misleading.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Apr 25, 2026

Choose a reason for hiding this comment

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

🟢 MINOR — Comment says "reject forks" but no fork detection exists

The code checks the author's collaborator permission (write/admin/maintain) — it does not check head.repo.id != base.repo.id or isCrossRepository. A collaborator with write access who submits from a fork would pass this check.

This is a documentation accuracy issue, not a security gap — the real defense is step 4 (the restore), and the write-access gate is a valid authorization check on its own. But the misleading comment could confuse security reviewers into believing fork rejection happens here when it doesn't.

Suggested fix: Reword the comment:

# 1. Verify PR author is a trusted collaborator (requires write+ access)

Flagged by: 3/3 reviewers after follow-up adjudication (originally 1/3, confirmed by 2 follow-up reviewers at reduced severity)

AUTHOR=$(gh pr view "$PR_NUMBER" --json author --jq '.author.login')
PERM=$(gh api "repos/$GITHUB_REPOSITORY/collaborators/$AUTHOR/permission" --jq '.permission')
if [[ "$PERM" != "admin" && "$PERM" != "write" && "$PERM" != "maintain" ]]; then
echo "::error::PR author $AUTHOR has $PERM access — requires write+"
exit 1
fi
# 2. Capture base branch SHA before checkout
BASE_SHA=$(gh pr view "$PR_NUMBER" --json baseRefOid --jq '.baseRefOid')
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Apr 25, 2026

Choose a reason for hiding this comment

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

🔴 CRITICAL — BASE_SHA not validated; empty value cascades into silent restore bypass

If PR_NUMBER is unset, the API call fails, or jq returns null, BASE_SHA will be empty. The subsequent git checkout "" -- .github/ .agents/ will fail — but that error is swallowed by 2>/dev/null || true on line 283. The restore is silently skipped.

Suggested fix: Add validation immediately after assignment:

BASE_SHA=$(gh pr view "$PR_NUMBER" --json baseRefOid --jq '.baseRefOid')
if [[ -z "$BASE_SHA" ]]; then
  echo "::error::Could not determine base branch SHA for PR $PR_NUMBER"
  exit 1
fi

Flagged by: 2/3 reviewers (compounded by Finding 1 — both reviewers who flagged the || true issue also identified this failure path)

# 3. Check out the PR branch
gh pr checkout "$PR_NUMBER"
# 4. Restore trusted agent infrastructure from base branch
git checkout "$BASE_SHA" -- .github/ .agents/ 2>/dev/null || true
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Apr 25, 2026

Choose a reason for hiding this comment

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

🔴 CRITICAL — 2>/dev/null || true silently swallows a security-critical restore failure

The old Checkout-GhAwPr.ps1 script was documented as fatal on failure for the restore step (PowerShell $ErrorActionPreference = 'Stop'). The new inline pattern inverts that — 2>/dev/null || true suppresses all errors and exits 0.

Failing scenario: For workflow_dispatch, the platform skips checkout_pr_branch.cjs entirely, so this restore IS the final workspace state. If git checkout fails (empty $BASE_SHA, shallow clone, git error), the PR branch's .github/ and .agents/ remain in place — untrusted agent infrastructure survives, defeating the entire defense-in-depth pattern.

Suggested fix: Remove 2>/dev/null || true so the step fails hard on restore error:

git checkout "$BASE_SHA" -- .github/ .agents/

If suppressing stderr for legitimately non-existent paths is needed, use a targeted pre-check rather than blanket error suppression.

Flagged by: 3/3 reviewers

```
Comment on lines +280 to 284
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Apr 25, 2026

Choose a reason for hiding this comment

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

🔴 CRITICAL — Silent failure on security-critical restore step

The old PowerShell script documented restore as fatal on failure (step 4: "Restores ... from the base branch SHA (fatal on failure)"). The new inline pattern uses:

git checkout "$BASE_SHA" -- .github/ .agents/ 2>/dev/null || true

2>/dev/null || true silently swallows ALL failures — including the failure to restore .github/. If the base branch doesn't contain a .github/ directory (new repo), or the SHA is unreachable (shallow clone, GC'd), or the command errors for any reason, the workflow continues with the untrusted PR branch's .github/ files intact. For workflow_dispatch, where there's no platform restore, this means fork-controlled agent infrastructure (skills, instructions, copilot-instructions.md) would be used by the agent.

Concrete scenario: An attacker with write access opens a PR that modifies .github/copilot-instructions.md to inject malicious agent instructions. A workflow_dispatch run triggers; the git checkout restore silently fails (e.g. due to a shallow clone missing $BASE_SHA); the agent runs with attacker-controlled instructions.

Suggested fix: Split into two operations — one that must succeed (.github/), one that may not exist (.agents/):

# MUST succeed — fatal if .github/ can't be restored
git checkout "$BASE_SHA" -- .github/
# Optional — .agents/ may not exist in all repos
git checkout "$BASE_SHA" -- .agents/ 2>/dev/null || true


The script:
1. Verifies the PR author has write access and rejects fork PRs
2. Captures the base branch SHA before checkout
3. Checks out the PR branch via `gh pr checkout`
4. Restores `.github/skills/`, `.github/instructions/`, and `.github/copilot-instructions.md` from the base branch SHA (fatal on failure)

**Behavior by trigger:**
- **`workflow_dispatch`**: Platform checkout is skipped, so the script's restore IS the final workspace state (trusted files from base branch)
- **`slash_command`** (same-repo): Platform's `checkout_pr_branch.cjs` handles checkout. Skill files typically match main unless the PR modified them.
- **`slash_command`** (fork): Platform restores `.github/` from base branch artifact after checkout — fork cannot inject modified skills/instructions
- **`workflow_dispatch`**: Platform checkout is skipped, so the restore step IS the final workspace state (trusted files from base branch)
- **`slash_command`** (same-repo): Platform's `checkout_pr_branch.cjs` handles checkout. Agent infrastructure typically matches main unless the PR modified it.
- **`slash_command`** (fork): Platform restores `.github/` from base branch artifact after checkout — fork cannot inject modified agent infrastructure

**Note:** While the platform now handles `.github/` restore automatically for fork PRs, our `Checkout-GhAwPr.ps1` script still provides defense-in-depth for `workflow_dispatch` triggers (where platform checkout is skipped) and adds the write-access check that the platform doesn't enforce.
**Note:** While the platform now handles `.github/` restore automatically for fork PRs, a checkout-then-restore step still provides defense-in-depth for `workflow_dispatch` triggers (where platform checkout is skipped) and adds the write-access check that the platform doesn't enforce.

### Anti-Patterns

Expand All @@ -304,7 +311,7 @@ Skipping checkout means the agent evaluates the wrong files. The correct approac
- name: Checkout PR
run: gh pr checkout "$PR_NUMBER" ...
- name: Run analysis
run: pwsh .github/skills/some-script.ps1
run: pwsh .github/scripts/some-script.ps1
```

If you need to run scripts, either:
Expand Down
6 changes: 3 additions & 3 deletions ...-guide/scripts/Check-WorkflowSecurity.ps1 → ...ide/scripts/Test-GhAwWorkflowSecurity.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@
Directory containing workflow .md files. Default: .github/workflows

.EXAMPLE
pwsh Check-WorkflowSecurity.ps1
pwsh Check-WorkflowSecurity.ps1 -WorkflowDir .github/workflows
pwsh Test-GhAwWorkflowSecurity.ps1
pwsh Test-GhAwWorkflowSecurity.ps1 -WorkflowDir .github/workflows
#>

[CmdletBinding()]
Expand Down Expand Up @@ -165,7 +165,7 @@ function Test-Workflow {
severity = "HIGH"
rule = "steps-execute-workspace-scripts"
message = "steps: or pre-agent-steps: execute workspace scripts (.ps1/.sh/.py/.js) after PR checkout. Fork PRs can inject malicious scripts that run with GITHUB_TOKEN."
fix = "Move script execution to before checkout, or only run scripts from the base branch (use Checkout-GhAwPr.ps1 pattern)"
fix = "Move script execution to before checkout, or implement a checkout-then-restore step that verifies write access and restores trusted agent infrastructure from the base branch"
}
}
}
Expand Down