From 32a4ebd0b41388a84bb442b5724bc4af61981ea0 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Sat, 25 Apr 2026 12:37:08 -0500 Subject: [PATCH 1/2] =?UTF-8?q?fix:=20make=20gh-aw=20skill=20fully=20porta?= =?UTF-8?q?ble=20=E2=80=94=203-model=20audit=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review by Opus/Sonnet/Codex found 8 portability issues: SKILL.md: - Removed empty 'When to Read the Full Reference' heading - Re-linked references/architecture.md from Further Reading section references/architecture.md: - Replaced all Checkout-GhAwPr.ps1 references with inline bash example (6 instances) - Changed 'our workflows' to generic language (3 instances) - Fixed '.github/skills/some-script.ps1' to '.github/scripts/' - Inlined the full checkout-then-restore pattern with comments scripts/: - Renamed Check-WorkflowSecurity.ps1 → Test-GhAwWorkflowSecurity.ps1 (Test- is the approved PowerShell verb for validation) - Fixed fix-message to not reference Checkout-GhAwPr.ps1 by name Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .claude/skills/gh-aw-guide/SKILL.md | 15 ++++--- .../gh-aw-guide/references/architecture.md | 41 +++++++++++-------- ...rity.ps1 => Test-GhAwWorkflowSecurity.ps1} | 6 +-- 3 files changed, 34 insertions(+), 28 deletions(-) rename .claude/skills/gh-aw-guide/scripts/{Check-WorkflowSecurity.ps1 => Test-GhAwWorkflowSecurity.ps1} (97%) diff --git a/.claude/skills/gh-aw-guide/SKILL.md b/.claude/skills/gh-aw-guide/SKILL.md index eb2b3dc23..60aa6e788 100644 --- a/.claude/skills/gh-aw-guide/SKILL.md +++ b/.claude/skills/gh-aw-guide/SKILL.md @@ -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) in this skill directory. + +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 diff --git a/.claude/skills/gh-aw-guide/references/architecture.md b/.claude/skills/gh-aw-guide/references/architecture.md index fa85538de..b0f3beea2 100644 --- a/.claude/skills/gh-aw-guide/references/architecture.md +++ b/.claude/skills/gh-aw-guide/references/architecture.md @@ -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. --- @@ -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) @@ -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 @@ -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 @@ -259,7 +259,7 @@ 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: @@ -267,21 +267,28 @@ steps: 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 + 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') + # 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 ``` -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 @@ -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: diff --git a/.claude/skills/gh-aw-guide/scripts/Check-WorkflowSecurity.ps1 b/.claude/skills/gh-aw-guide/scripts/Test-GhAwWorkflowSecurity.ps1 similarity index 97% rename from .claude/skills/gh-aw-guide/scripts/Check-WorkflowSecurity.ps1 rename to .claude/skills/gh-aw-guide/scripts/Test-GhAwWorkflowSecurity.ps1 index aee966cd4..e03deac06 100644 --- a/.claude/skills/gh-aw-guide/scripts/Check-WorkflowSecurity.ps1 +++ b/.claude/skills/gh-aw-guide/scripts/Test-GhAwWorkflowSecurity.ps1 @@ -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()] @@ -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" } } } From b014ec5577a4c399ae13798f79f6570b3ffd5e6e Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Sat, 25 Apr 2026 13:20:42 -0500 Subject: [PATCH 2/2] fix: final portability cleanup from 3-model review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - architecture.md line 3: removed 'SKILL.md' self-reference - architecture.md line 70: 'Our gh-aw workflows' → generic - SKILL.md line 439: removed 'in this skill directory' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .claude/skills/gh-aw-guide/SKILL.md | 2 +- .claude/skills/gh-aw-guide/references/architecture.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.claude/skills/gh-aw-guide/SKILL.md b/.claude/skills/gh-aw-guide/SKILL.md index 60aa6e788..af95cd2f1 100644 --- a/.claude/skills/gh-aw-guide/SKILL.md +++ b/.claude/skills/gh-aw-guide/SKILL.md @@ -436,7 +436,7 @@ Supported runtimes: `node`, `python`, `go`, `uv`, `bun`, `deno`, `ruby`, `java`, ## Further Reading -For deep-dive details on execution model, security boundaries, fork handling, safe output types, and known issues, see [`references/architecture.md`](references/architecture.md) in this skill directory. +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 diff --git a/.claude/skills/gh-aw-guide/references/architecture.md b/.claude/skills/gh-aw-guide/references/architecture.md index b0f3beea2..88c475ca8 100644 --- a/.claude/skills/gh-aw-guide/references/architecture.md +++ b/.claude/skills/gh-aw-guide/references/architecture.md @@ -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 @@ -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.