Skip to content

Restrict allowedTools to prevent approvals and broad API access#100

Merged
francoisferrand merged 1 commit intomainfrom
bugfix/SOR-238
Apr 16, 2026
Merged

Restrict allowedTools to prevent approvals and broad API access#100
francoisferrand merged 1 commit intomainfrom
bugfix/SOR-238

Conversation

@francoisferrand
Copy link
Copy Markdown
Contributor

Replace the broad "Bash(gh api )" allow rule with three narrow
patterns that match only the endpoints the review skills actually use:
pulls/
/comments (inline comments), issues/*/comments (summary
comments), and check-runs (check run create/update).

The broad rule allowed Claude to POST to .../pulls//reviews, which
is the approval endpoint. Narrowing it closes that path while still
supporting both review styles (inline + check run, inline + summary
comment) across consuming repos.

Issue: SOR-238

@francoisferrand francoisferrand requested a review from a team as a code owner April 15, 2026 15:47
@claude
Copy link
Copy Markdown

claude Bot commented Apr 15, 2026

LGTM

The change correctly narrows Bash(gh api *) to three specific endpoint patterns (*pulls/*/comments*, *issues/*/comments*, *check-runs*), closing the path to the pulls/<n>/reviews approval endpoint while preserving all functionality used by the review skill (inline comments via the pulls comments API, summary comments via gh pr comment, and check run creation/updates).

Review by Claude Code

Comment thread .github/workflows/claude-code-review.yml Outdated
@claude
Copy link
Copy Markdown

claude Bot commented Apr 15, 2026

  • The gh api allowedTools patterns are inconsistent: repos/*/pulls/*/comments* and repos/*/check-runs* lack the leading wildcard that *issues/*/comments* has. Without it, commands that include CLI flags (-X POST -H "Accept:...") between gh api and the URL won't match. Add a leading * to all three patterns.
    - Change repos/*/pulls/*/comments**pulls/*/comments*
    - Change repos/*/check-runs**check-runs*
    - The skill definition (.claude/skills/review-pr/SKILL.md line 6) still lists Bash(gh api *) in its allowed-tools. Consider narrowing it to match the workflow's patterns for defense in depth.

    Review by Claude Code

Copy link
Copy Markdown

@SylvainSenechal SylvainSenechal left a comment

Choose a reason for hiding this comment

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

Claude's comment looks legit 🤔
Edit: maybe not always problematic, it depends how claude will use the cli, but I guess safer to add the wildcard

Replace the broad "Bash(gh api *)" allow rule with three narrow
patterns that match only the endpoints the review skills actually use:
pulls/*/comments (inline comments), issues/*/comments (summary
comments), and check-runs (check run create/update).

The broad rule allowed Claude to POST to .../pulls/<n>/reviews, which
is the approval endpoint. Narrowing it closes that path while still
supporting both review styles (inline + check run, inline + summary
comment) across consuming repos.

Issue: SOR-238
@claude
Copy link
Copy Markdown

claude Bot commented Apr 16, 2026

LGTM — good security tightening.

One optional follow-up:
- .claude/skills/review-pr/SKILL.md line 6 still declares the broad Bash(gh api *) in its allowed-tools frontmatter. The CLI --allowedTools flag is the enforcement point so this isn't a security gap, but updating the skill declaration to use the same three narrow patterns would keep them consistent.

Review by Claude Code

@francoisferrand
Copy link
Copy Markdown
Contributor Author

Claude's comment looks legit 🤔 Edit: maybe not always problematic, it depends how claude will use the cli, but I guess safer to add the wildcard

It is somewhat legit, but not for the right reasons: permissions should actually be restricted, which does not really hinder functionality as the command can be written gh api repos/[...]/coments, but really enforces the security.

The problem is really that this could break existing SKILLS accross many repos...

@francoisferrand francoisferrand merged commit 9c82a80 into main Apr 16, 2026
14 of 16 checks passed
@francoisferrand francoisferrand deleted the bugfix/SOR-238 branch April 16, 2026 09:11
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.

3 participants