Skip to content

fix(ci): allow jq in issue-labeler assign-label step - #1367

Open
jth-nw wants to merge 3 commits into
devfrom
fix/issue-labeler-jq-permission
Open

fix(ci): allow jq in issue-labeler assign-label step#1367
jth-nw wants to merge 3 commits into
devfrom
fix/issue-labeler-jq-permission

Conversation

@jth-nw

@jth-nw jth-nw commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • The Step 3 — Assign label job in claude-issue-labeler.yml restricts tool use to Bash(gh:*), so any gh ... | jq ... pipe the agent tries gets denied.
  • Traced this via issue Feedback: https://docs.netwrix.com/docs/identitymanager/current/integration-guide/api/server/ #1361: that run hit 4 permission denials and 19 turns (vs. a normal ~7 turns / 0 denials) and never actually applied a label to the issue, despite the step reporting success.
  • Widens the allowlist to Bash(gh:*),Bash(jq:*),Skill(assign-label) so filtering gh JSON output through jq no longer gets blocked.

Test plan

  • Open/edit a test issue and confirm Step 3 — Assign label completes with permission_denials_count: 0 and the issue receives the expected labels.

The assign-label step's tool allowlist only permitted `gh`, so any
`gh ... | jq ...` pipe the agent tried got denied — burning turns on
retries and sometimes leaving the issue with no label at all.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ailures

Address code review feedback on the jq allowlist fix:
- Extend Bash(jq:*) to Steps 1 and 2 as well, so all three
  claude-code-action steps have consistent tool access instead of
  leaving two of three still degraded.
- Add Read,Grep,Glob to Step 3 to match Steps 1/2, since the sparse
  checkout includes .github/.claude and the agent may reasonably try
  to inspect local files.
- Turn Step 4's silent "no labels found" log line into a `::warning::`
  annotation so a repeat of #1361 (assign-label reporting success but
  applying nothing) surfaces in the run summary instead of disappearing.

Note: whether jq alone fully resolves the original denials is still
unverified — Claude Code's Bash permission engine denies command
substitution (`$(...)`) outright regardless of allowlist, which is a
plausible alternate cause. Confirming that requires a live run with
show_full_output: true, which wasn't done here.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…n opened issues

Address second round of review feedback:

- Bash(jq:*) was a mistake: standalone jq can dump the whole process
  environment (`jq -n env`) and read arbitrary local files
  (`--rawfile`, `-R`), and Steps 1-3 process fully attacker-controlled
  issue/comment content with allowed_non_write_users: '*'. Combined
  with Bash(gh:*) (which can post comments), that's a prompt-injection
  path to exfiltrate ANTHROPIC_API_KEY out of the process environment.
  None of the three skills' documented flows use standalone jq anyway
  (identify-security-vuln-discussion and code-of-conduct-check use no
  jq at all; assign-label only uses gh's own --jq flag) — this was
  guesswork addressing an unverified failure, not a real requirement.
- Removed Bash(jq:*) from all three steps and added an explicit prompt
  line telling the agent to use gh's built-in --jq instead of piping
  to a separate jq process. This also sidesteps the original
  permission-denial theory by construction: a single `gh ... --jq`
  call never crosses a pipe boundary, so it can't hit the
  Bash-permission-checks-each-pipeline-segment issue in the first
  place, regardless of whether that was the real cause of #1361's
  denials.
- The zero-labels warning only fires when an issue ends with no
  labels at all, which misses #1361-style cases where template labels
  are present but Step 3 added nothing on top. Added a same-set
  comparison (pre vs. post Step 3) scoped to newly-opened issues,
  which is the case where "no change" is most likely a silent
  failure rather than a legitimate conservative decision.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Code Review

Scope: one file changed (.github/workflows/claude-issue-labeler.yml). No Docusaurus config, sidebar, or build-script changes, so build/routing risk is nil. YAML structure is valid and the new shell logic is correctly quoted — no injection via label names or issue title (both passed through env:, and the inline expression is the numeric issue number).

No blocking bugs. Four things worth a look before merge:

1. The PR title, description, and test plan no longer describe this branch

The stated change — "widens the allowlist to Bash(gh:*),Bash(jq:*)" — was added in commit 1, extended in commit 2, and then fully reverted in commit 3. The net diff against dev contains no allowlist widening for jq at all. What actually lands is:

  • three advisory prompt lines telling the agent to prefer gh --jq over a jq pipe,
  • Read,Grep,Glob added to Step 3's allowlist (Steps 1/2 already had them),
  • two ::warning:: annotations in Step 4.

The test plan checkbox ("confirm Step 3 completes with permission_denials_count: 0") isn't verifiable from anything in the diff, and commit 2's own message concedes the root cause of #1361 is still unverified. Net effect: this PR adds detection for the failure mode plus a hint that may sidestep it, not a fix. That's a reasonable thing to merge — but please update the title/body so the history doesn't claim a jq permission fix that isn't here.

2. Dropping Bash(jq:*) does not close the exfiltration path commit 3 cites

Commit 3's rationale for removing Bash(jq:*) is that standalone jq can read arbitrary local files and dump the environment, which matters because Steps 1–3 run over attacker-controlled issue content with allowed_non_write_users: '*'. The reasoning is sound, but Bash(gh:*) on its own already provides both halves of that chain: gh issue comment --body-file <path> reads an arbitrary file and posts it publicly. So the step is not hardened by this change — it's unchanged in practice.

Relatedly, Step 3 gains Read,Grep,Glob here, which isn't mentioned in the PR description. Parity with Steps 1/2 is a defensible goal, but it's a tool-surface expansion on the most attacker-adjacent step and deserves to be called out explicitly rather than arriving as a side effect. If the team actually wants this class of risk closed, the lever is narrowing Bash(gh:*) to specific subcommands — not the jq/Read distinction.

3. The "labels unchanged" warning will fire on legitimate no-op runs

.claude/skills/assign-label/SKILL.md explicitly instructs the skill to be conservative ("better to under-label than to mislabel", line 50) and its own Scenario 2 shows Add: none as a correct outcome. On a newly-opened issue where the template applied documentation + fix and the body is too vague to justify a product label, "add nothing" is the skill working as designed — and this check will annotate it as a possible silent failure every time. It's warning-only so nothing breaks, but expect recurring noise that trains people to ignore the annotation, which defeats the point of adding it. Consider scoping the check to issues where the pre-set is only template labels with no product/area label, or downgrading to ::notice::.

4. PRE_LABELS is the pre-Step-1 snapshot, not pre-Step-3

join(github.event.issue.labels.*.name, ',') is the webhook payload from event time, so the comparison is really "labels at event time vs. labels after Step 3." Steps 1 and 2 don't currently touch labels (verified — code-of-conduct-check only edits title/body, identify-security-vuln-discussion closes and tags), so there's no impact today. But if either skill ever gains a labeling action, this silently becomes a false negative rather than failing visibly. Capturing the pre-set with a gh read immediately before Step 3 would make the check say what its comment claims it says.

Nits

  • Splitting PRE_LABELS on , would mis-split a label name containing a comma. Checked all 43 repo labels — none do, so this is theoretical only.
  • The ::warning:: strings aren't escaped for % or newlines (GitHub's annotation format requires URL-encoding those). Label names and the issue number can't currently contain either, so it renders fine today.
  • Step 4 has no if: always(), so a hard Step 3 failure produces no annotation — but that case already fails the job loudly, so the gap is only in the silent-success path this targets. Working as intended, just noting the boundary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant