fix: pin the PR diff to a commit SHA, deny Bash outright, tighten secret redaction

[syed-ahsan-ishtiaque]

Summary

Follow-up to #49 (shipped in v1.2.0 via #51). Rebased onto main; the diff here is only new work.

#49 closed the reported TOCTOU on issues events. This PR does three things:

1. Closes the same class of hole in the PR diff path, which fix: harden issues-event comment trust boundary, reduce agent privilege, keep headless runs synchronous #49 did not touch.
2. Makes fix: harden issues-event comment trust boundary, reduce agent privilege, keep headless runs synchronous #49's privilege and secret-handling claims actually hold, rather than resting on CLI defaults or on a file mode that does not apply to the threat.
3. Corrects two statements in the released docs/security.md that overstate what the code does.

1. The PR diff is the reported vulnerability with a different carrier

fetchPRDiffContext read live PR state via pulls.listFiles, with no SHA pin and no timestamp binding, and the returned patch text goes verbatim into <changed_files> in the agent prompt. The exploit path is the ordinary review flow:

1. External contributor opens a PR from a fork.
2. Maintainer comments @awsapm review this PR. Authorization is checked against the maintainer.
3. Contributor pushes a new commit.
4. The action calls pulls.listFiles and receives the new diff.
5. Attacker-authored patch text enters the already-authorized prompt.

Same shape as V2337707056, with the diff rather than comments as the carrier.

Fix. The diff is pinned to a commit SHA instead of read from live state:

• pull_request_review_comment carries the full pull_request object in its payload, so head.sha is part of the authorizing event and no live read happens at all.
• issue_comment on a PR carries no head SHA, so it is resolved once and every read is pinned to it.
• The diff is omitted if the head commit is dated at or after the authorizing event.
• repos.compareCommits replaces pulls.listFiles, because listFiles always returns live PR state and cannot be pinned.
• The SHA is recorded in the prompt so the analyzed snapshot is auditable.
• Requires a valid cutoff; fails closed.

Base endpoint. pull_request.base.sha is used rather than the branch name, so both ends are immutable and the comparison is reproducible. Three-dot compare derives the merge base from it, which matches the merge base against the current branch tip in the normal case. They diverge only when the PR author merged the base branch into their branch, where the diff is larger by those base-branch commits — content from the protected base branch, so maintainer-authored, not a new injection surface.

Coverage trade-off, stated. compareCommits caps files at 300 and reports no total, so a full-length response is the only truncation signal available; a capped diff is now labelled in the prompt. listFiles paginates to 3000 but cannot be pinned. In practice this is 300 vs the 30 the shipped code actually saw, since it never paginated.

2. Bash is denied outright, not left to a CLI default

src/config/mcp-config.js still carried the comment "Bash tools are already allowed by claude-code-base-action." If that were true, #49 removing the Bash(cat:*) grants would have been a no-op and the exfiltration path in the report would still be open.

Upstream check: claude-code-base-action passes ["-p","--verbose","--output-format","stream-json"] plus --allowedTools, with no --dangerously-skip-permissions, and setup-claude-code-settings.ts writes only enableAllProjectMcpServers. So Bash should be denied — but only because the CLI defaults that way, and @beta pins the CLI to 1.0.88, making that a version-specific behaviour the action does not control.

New getDisallowedToolsForClaude() → new disallowed_tools action output → claude-code-base-action's disallowed_tools input, which becomes --disallowedTools and takes precedence over the allow list. Denies:

• Bash — its arguments cannot be path-scoped, so any grant reaches every file the runner user can read, including the generated MCP credential config.
• WebFetch / WebSearch — a direct exfiltration channel for anything the agent has read, and the investigation needs neither.

Stale comment removed. Also: LS was the only file tool with no path scope, which is the reconnaissance step for locating the credential config, and the patterns carried a stray leading slash producing Read(//home/runner/...). Both fixed.

3. .git/config holds a usable token inside the agent's read scope

template/awsapm.yaml checked out with the default persist-credentials: true, which writes AUTHORIZATION: basic <base64(x-access-token:TOKEN)> into .git/config — inside the directory the agent's file tools are scoped to, in a base64 form that value-based redaction does not match. That token carries contents: write, issues: write, pull-requests: write.

Set persist-credentials: false. The action talks to GitHub through the REST API, so nothing needs git credentials.

4. The 0600 mode silently did not apply

fs.writeFileSync's mode applies only when it creates the file. On an existing path it is ignored:

pre-existing mode: 644
after writeFileSync({mode:0o600}): 644
after explicit chmodSync: 600

RUNNER_TEMP persists across steps within a job and is reused on self-hosted runners, so the old mode survived. Added fs.chmodSync.

Separately, the mode was never what kept that file from the agent — the agent runs as the same OS user that writes it. The path placement plus the Bash denial are what do that. Code comments and docs corrected accordingly.

5. Redaction gaps

Input	before	after
exact env values, AKIA…/ASIA…, gh*_, lowercase MCP JSON	redacted	redacted
base64 of a known secret value	leaked	redacted
AUTHORIZATION: basic <base64> (the .git/config form)	leaked	redacted
sk-ant-…	leaked	redacted
hex-encoded, or value split by a separator	leaked	leaked (inherent)

ANTHROPIC_API_KEY and CLAUDE_CODE_OAUTH_TOKEN were absent from secretEnvVars despite both being supported auth paths for claude-code-base-action. Added.

The remaining bypasses are inherent to regex redaction. It also covers only one channel: the agent holds mcp__github__create_or_update_file and push_files with contents: write, so it could commit rather than comment. Documented as a backstop, not a boundary.

6. Two correctness fixes

isBackgroundAgentPlaceholder was too broad. It matched waiting on the background … agent anywhere in the result, plus the bare phrase background exploration agent, then discarded the entire result. Confirmed false positives:

• "## Root Cause\nThe worker is waiting on the background queue.\n\n## Fix\nRestart the agent."
• "The action disables the built-in background exploration agent via an env var."

Because the result is influenced by issue and comment text, anyone could suppress an investigation by getting the phrase echoed. Now requires a short result with the phrase at the start.

filterCommentsByTriggerTime still failed open. It returned every comment on a falsy cutoff, and it is exported. #49 guarded the single call site; the guard now lives inside the function so a future caller cannot reopen it.

7. Docs

docs/security.md now separates protections enforced in code from best-effort model instructions, since the latter are exactly what a prompt-injection attack targets. Two claims from #49 are corrected:

• The 0600 mode is not why the credential config is unreachable from the agent (same OS user); the path scoping and Bash denial are.
• Redaction "strips credential material" only with qualification — it misses transformed values and covers only the comment channel.

Adds a Residual risk section, and documents that allowed_non_write_users: '*' allows any user to trigger the action, removing the trust boundary entirely. That wildcard was undocumented in both action.yml and the docs.

Also adds an untrusted-data notice fencing the user-content sections of the prompt. This is a model instruction, not a control, and the docs classify it that way.

Verification

Unit tests: 189 pass, up from 175 on main. New coverage: diff SHA pinning, late-push rejection, the no-live-read path for pull_request_review_comment, diff capping at and below the limit, fail-closed filtering at the function level, the LS scope and single-slash patterns, the Bash denial, each redaction addition, and the placeholder false positives.

The reported vulnerability, reproduced and confirmed fixed. Using the VDP reporter's own harness from the V2337707056 attachment, which drives the real init.js → prepare-claude-config.js → post-result.js:

revision	canary in prompt	Bash(cat:*) granted	shim read the secret	secret in posted comment
23ce62f (pre-#49)	true	true	true	true
#49	false	false	false	false
this PR	false	false	false	false

Four mechanical adaptations were required, none touching the injection-detection logic: no-op setSecret/exportVariable on the @actions/core stub (both are called by #49 and this PR), parameterising the assert(catAutoApproved) expectation since that grant is the thing being removed, and skipping the Linux-only /sys/class/net check when not containerised. Docker was unavailable so it ran under local Node; the harness makes no network calls.

That run also confirms the full real chain still executes, including chmodSync against a real filesystem and post-result parsing a real execution file — neither of which the Jest mocks cover.

The PR diff path, end to end through the real init.js. The reporter's harness sets issue.pull_request = null, so it never reaches fetchPRDiffContext. A separate check covers it:

revision	event	push timing	canary in prompt	live PR read	result
23ce62f	either	either	—	—	uses listFiles, not pinned
#49	either	either	—	—	uses listFiles, not pinned
this PR	issue_comment	before trigger	true	yes	authorized content included
this PR	issue_comment	after trigger	false	yes	blocked
this PR	pull_request_review_comment	before trigger	true	no	included, no window
this PR	pull_request_review_comment	after trigger	false	no	blocked

The fork-PR assumption, against real GitHub. The riskiest assumption here is that compareCommits on the base repo can resolve a head SHA living in a contributor's fork. If it cannot, the fetch fails, the code returns null, and PR review silently proceeds with no knowledge of the changes. Mocks cannot catch that, so it was checked against 10 real fork PRs across 4 repositories, including this one:

repo	PR	fork owner	compare status	files	with patch
this repo	38	AdnaneKhan	ahead	1	1
this repo	37	ezhang6811	ahead	2	2
this repo	36	ezhang6811	ahead	1	1
aws-otel-python-instrumentation	863	haneric00	ahead	1	1
aws-otel-python-instrumentation	862	liustve	ahead	9	9
aws-otel-python-instrumentation	853	liustve	ahead	15	15
claude-code-base-action	91	lucky-verma	diverged	1	1
claude-code-base-action	89	JosephDoUrden	ahead	1	1
claude-code-base-action	88	MaxwellCalkin	ahead	1	1
opentelemetry-python	5588	webdevsamran	ahead	2	2

All resolve and return patches, including the diverged case (the base-branch-moved scenario described in section 1).

The resulting file set was also compared against pulls/{n}/files — what the UI shows — for 7 of these. It matched exactly every time, diverged case included, so using base.sha rather than the branch name does not change what the agent sees in practice.

Output-name wiring. disallowed_tools traverses the same three hops as allowed_tools (core.setOutput → action.yml output mapping → template input), with names verified matching at each hop. allowed_tools already works in production over that exact path, so this is not a new mechanism.

Not verified

No live GitHub Actions run. #49 had one; this does not. Two things remain open:

1. Whether the pinned CLI (1.0.88) accepts LS(<path>/**) as a permission rule. The failure mode is benign: an unrecognised rule means LS is simply not granted, LS is read-only, and the scoped Glob/Grep grants cover the same ground. Worst case is marginally less capability, not a break and not a security gap.
2. Whether denying Bash measurably degrades investigation quality. This is a quality question rather than a correctness one, and fix: harden issues-event comment trust boundary, reduce agent privilege, keep headless runs synchronous #49 already removed the Bash grants that mattered.

Reproduction steps for a live run are in a comment below. Neither verification script is committed here: the first is derived from a VDP submission and is not mine to publish, and the second is a research artifact rather than a test. Both can be attached to V2337707056.

Out of scope, noted for follow-up

• core.setOutput('GITHUB_TOKEN', …) in src/init.js publishes the token as an action output. action.yml consumes it internally at three points and re-exposes it as the github_token output, so removing it is a breaking change. core.setSecret is added here so Actions masks it in logs meanwhile.
• containsTriggerPhrase in src/init.js hardcodes @awsapm and ignores the bot_name input. bot_name only strips the name from the request text; it does not control what triggers the action. A caller setting bot_name: "@myteam" is still triggered by @awsapm and never by @myteam.

[syed-ahsan-ishtiaque]

How to validate this with a live run

Two prerequisites, because the action needs an AWS account for both the model and the telemetry:

• A repo where you control the default branch. For issues events GitHub runs the workflow from the default branch only, so the test workflow has to be committed there. aws-observability/application-signals-demo cannot be used casually for this: main requires 2 approvals with enforce_admins: true.
• An AWS account with Bedrock InvokeModel for the model you pick, and Application Signals data worth investigating. An account with no instrumented services will produce a correct but empty investigation, which tells you nothing about answer quality.

1. Point a workflow at this branch

uses: accepts a branch name, so no release or tag is needed:

name: awsapm (PR 50 validation)

on:
  issue_comment:
    types: [created, edited]
  issues:
    types: [opened, assigned, edited]

jobs:
  awsapm-investigation:
    if: |
      (github.event_name == 'issue_comment' && contains(github.event.comment.body, '@awsapm')) ||
      (github.event_name == 'issues' && (contains(github.event.issue.body, '@awsapm') || contains(github.event.issue.title, '@awsapm')))
    runs-on: ubuntu-latest
    permissions:
      contents: write
      pull-requests: write
      issues: write
      id-token: write
    steps:
      - uses: actions/checkout@v4
        with:
          persist-credentials: false        # part of what this PR changes

      - uses: aws-actions/configure-aws-credentials@v4
        with:
          role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
          aws-region: ${{ vars.AWS_REGION || 'us-east-1' }}

      - name: Prepare Investigation Context
        id: prepare
        uses: aws-actions/application-observability-for-aws@harden/pr-diff-toctou-and-least-privilege
        with:
          bot_name: "@awsapm"
          cli_tool: "claude_code"

      - name: Run Claude Investigation
        id: claude
        uses: anthropics/claude-code-base-action@beta
        with:
          use_bedrock: "true"
          model: "us.anthropic.claude-sonnet-4-5-20250929-v1:0"
          prompt_file: ${{ steps.prepare.outputs.prompt_file }}
          mcp_config: ${{ steps.prepare.outputs.mcp_config_file }}
          allowed_tools: ${{ steps.prepare.outputs.allowed_tools }}
          disallowed_tools: ${{ steps.prepare.outputs.disallowed_tools }}   # new in this PR

      - name: Post Investigation Results
        if: always()
        uses: aws-actions/application-observability-for-aws@harden/pr-diff-toctou-and-least-privilege
        with:
          cli_tool: "claude_code"
          comment_id: ${{ steps.prepare.outputs.awsapm_comment_id }}
          output_file: ${{ steps.claude.outputs.execution_file }}
          output_status: ${{ steps.claude.outputs.conclusion }}

Note: if an existing awsapm.yml is already in the same repo, it will also fire, because containsTriggerPhrase matches any text containing @awsapm regardless of the bot_name input. Either remove the old workflow for the duration of the test or expect two bot comments — which is a usable side-by-side comparison if the other one is pinned to a release.

2. Test A — does an ordinary investigation still work?

Open an issue whose body names a real service in your account:

@awsapm investigate elevated latency in <service-name>

Check three things:

1. The result comment contains an actual investigation, not ⚠️ Investigation completed but no result was generated.

2. The run log shows --disallowedTools Bash,WebFetch,WebSearch in the claude step's arguments.

3. The execution file contains no Bash tool calls. In the run log, or:

   gh run view <run-id> --log | grep -c '"name": *"Bash"'

Item 1 is the one worth judging carefully — Bash is now denied, so if the agent needed it, quality drops here.

3. Test B — the security fix, on a fork PR

This is the part unit tests cannot reach. Needs a PR from a fork, not a same-repo branch.

1. From a second account, fork the repo and open a PR that changes a file.

2. As a maintainer, comment @awsapm review this PR.

3. Wait for the result. Confirm it discusses the actual code changes, and that the prompt recorded the head SHA. With ACTIONS_STEP_DEBUG=true the full prompt is in the log:

   The following files were changed in this PR, as of commit <sha> (compared against <sha>).
   

4. Now the negative case. Push another commit to the fork PR, then comment @awsapm review this PR again, and — this is the important part — make the comment's timestamp earlier than the new commit is not possible, so instead re-run the first workflow run from the Actions tab. Re-running replays the original event, whose comment.created_at predates the new push.

5. That re-run should omit the diff and log:

   PR #<n> head commit <sha> is dated at or after the authorizing event (...); omitting the PR diff. Re-trigger to review it.
   

If step 5 shows the new commit's contents instead, the fix is not working.

4. What a failure means

Symptom	Cause	Severity
Bot reviews the PR but never mentions the code changes	compareCommits failed; check the log for Could not fetch PR changes	breaks PR review; would need a listFiles fallback
Investigation quality noticeably worse than a release run	the agent wanted Bash	quality regression, reconsider the denial
--disallowedTools absent from the claude step args	output wiring	Bash protection falls back to the CLI default
Diff shown on the step-5 re-run	date check not firing	the fix is not working
A legitimate diff omitted on a normal run	contributor clock skew ahead of the trigger time	fails closed; documented in docs/security.md

5. Cleanup

Delete the validation workflow, the test issue, and any tracking comments the bot posted. If an OIDC role was created for the test, remove that too.