fix(claude-config): flag tilde-user paths and inert grant tokens

[kyle-sexton]

Fixes #2397.

Adds P2 detection for Bash(~user/…) rules that leak a username and are not expanded at runtime, and a new P4 check for inert ${CLAUDE_PLUGIN_ROOT}, %USERPROFILE%, and $env:USERPROFILE grants with a branched remedy (skill-local → ${CLAUDE_SKILL_DIR}; otherwise → bare PATH command).

Verification

• permission-rule-check.test.sh: 94 checks passed

Related

• fix(claude-config): permission-rule-check has no inert-grant check, and Bash(~user/...) leaks a username unflagged #2397 — permission-rule-check A7b/A12

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

Claude finished @kyle-sexton's task in 5m 34s —— View job

---

Review complete

• Gather context (read CLAUDE.md, REVIEW.md, PR diff)
• Review permission-rule-check.sh changes
• Review permission-rule-check.test.sh changes
• Review CHANGELOG.md and plugin.json version bump
• Post findings

Repo has a claude-security-review.yml workflow, so per REVIEW.md this lane owns correctness/design/tests/docs and leaves security findings to that lane (none of the changes here touch a security-relevant surface anyway — this is a local advisory scanner).

Posted two inline comments:

1. 🔴 Important — permission-rule-check.sh L220-223: the new P4 check uses a single boolean grep -qE over the whole allowed-tools text block and embeds that whole block in the finding, instead of looping over distinct matches the way P1/P2/P2b (just above it) do. Confirmed by tracing the code and by checking that 5 real SKILL.md files in this repo (plugins/repo-hygiene/skills/clean/SKILL.md, plugins/context7/skills/lookup/SKILL.md, etc.) use the multi-line list form this breaks on: one inert entry among several clean ones dumps the whole block into the report, and two entries with two different inert tokens in the same block only produce one finding — the second is silently dropped. This is the tool's core job (catching fragile grants), so under-reporting is a real correctness gap, not cosmetic.
2. 🟡 Nit — same file, permission-rule-check.test.sh L422-434: the new Case 8g only exercises the single-line allowed-tools form, so it doesn't catch chore: initialize marketplace scaffold #1 above.

Not anchorable to a changed line — documentation drift: the script's header comment (now "Flags five anti-patterns...") and the two new checks (P4, and the P2b tilde-user variant of P2) were not mirrored into the docs that are supposed to be their source of truth:

• reference/criteria.md — has ## P1/## P2/## P3 sections only; no ## P4 section, and no mention of the tilde-user variant under P2. The script itself calls this "the criteria file" for these checks.
• SKILL.md L21 and the severity table, L83-86 — still describe only P1/P2/P3; the "error" row's rationale ("leaks a username / breaks on other machines") doesn't describe P4's failure mode (inert token, grant never matches — unrelated to leaking a username).

Neither doc file is covered by an automated sync check, so this won't break CI, but it leaves the documented spec out of sync with the implementation for both new checks added in this PR while the script's own header docs were updated. Worth a follow-up pass on criteria.md and SKILL.md (and bumping criteria.md's Version: header per its own convention).

Everything else — the P2_TILDE_USER_RULE_ERE/P4_INERT_ERE regexes, the inert_grant_remedy() branching on */skills/*/SKILL.md, the scan_rule signature change (optional third file arg, both call sites updated correctly), and the CHANGELOG/plugin.json version bump — checked out fine on a careful read-through.

Note: the author-claimed "94 checks passed" test-run result in the PR description is unverified by me — I was not able to execute the modified shell scripts in this sandbox (script execution on changed PR files requires approval that isn't available in this lane), so I reviewed statically and via Grep/Read against the actual repo content instead.
· Branch

[claude]

Claude finished @kyle-sexton's task in 1m 13s —— View job

---

Security Review — PR #2407

• Gather context (fetch diff, read changed files)
• Understand the request (security review of PR fix(claude-config): flag tilde-user paths and inert grant tokens #2407 changes)
• Review shell script changes for injection/logic issues
• Review test script changes
• Post findings

No security issues found.

Reviewed the full diff (git diff origin/main...HEAD) across all 4 changed files: plugin.json (version bump), CHANGELOG.md, permission-rule-check.sh, and permission-rule-check.test.sh.

The substantive change is confined to permission-rule-check.sh, a read-only, local static-analysis script that scans repo/config files for fragile permission-grant patterns and prints advisory findings. Specifically checked for:

• Injection surfaces: no eval, no unsanitized command substitution, no data-driven code paths. New code (inert_grant_remedy(), the P4 grep -qE inert-token check, the P2 tilde-user grep -oE) only reads scanned text via printf '%s' and matches it against fixed regex literals (P2_TILDE_USER_RULE_ERE, P4_INERT_ERE) — the scanned text is never itself interpreted as a pattern, command, or path to execute. L173, L177, L186-L197.
• inert_grant_remedy()'s case "$file" in (L189): pure glob pattern match, $file comes from find-discovered paths, never executed or eval'd.
• New regexes (P2_TILDE_USER_RULE_ERE, P4_INERT_ERE): bounded, no nested unbounded quantifiers — no ReDoS concern under POSIX ERE (grep -E).
• Trust boundary: this script only reads local repo/settings files and prints text; it doesn't write, execute, or transmit anything derived from scanned content. Its CI invocation (run-plugin-tests.sh in ci.yml) runs under a plain pull_request trigger with contents: read and no secrets — not pull_request_target/workflow_run, so no privileged-context risk, and that workflow file isn't part of this PR's diff anyway.
• Test fixtures (permission-rule-check.test.sh): new cases (8f/8g) are static fixture data exercising the new detections; no new execution surface.

No logic flaws, authz gaps, injection surfaces, secret-handling issues, or workflow permission changes identified in this diff.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5d84343994

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

Claude finished @kyle-sexton's task in 1m 35s —— View job

---

Security Review — PR #2407 (re-run on latest commit 3d0ccbd)

• Gather context (diff 95d95a2..3d0ccbd against origin/main's fork point, read changed files)
• Review shell script changes for injection/logic/trust-boundary issues
• Review test script changes
• Review plugin.json / CHANGELOG.md
• Post findings

No security issues found.

Reviewed the full diff across all 4 changed files: plugin.json (version bump 0.36.0 → 0.36.1), CHANGELOG.md, permission-rule-check.sh, and permission-rule-check.test.sh.

The substantive change is confined to permission-rule-check.sh, a read-only local static-analysis script. It scans repo/config files for fragile permission-grant patterns and prints advisory findings; it never executes, writes, or transmits anything derived from scanned content. Checked specifically for this PR's new surfaces:

• inert_grant_remedy() (L186-L197): its case "$file" in */skills/*/SKILL.md) is a pure glob match against a path discovered via local find (L299-L305) — never eval'd or executed, only used to pick a hardcoded remedy string.
• P4 inert-token check and P2 tilde-user check (L206-L223): both feed scanned text through printf '%s' into grep -oE/-qE against fixed regex literals (P2_TILDE_USER_RULE_ERE, P4_INERT_ERE) — the scanned text is always the haystack, never interpreted as a pattern, command, or executable path. No eval, no dynamic source, no command substitution of scanned content.
• scan_rule signature change (new optional third file arg, L199-L224): both call sites updated correctly — the frontmatter site passes the real file (L296), the settings-scope site passes "" (L316), correctly routing to the non-skill remedy branch.
• New regexes (P2_TILDE_USER_RULE_ERE, P4_INERT_ERE): bounded character classes, no nested/ambiguous quantifiers — no ReDoS concern.
• Trust boundary / CI context: this script only reads local repo/settings files and prints text. No workflow files are touched by this PR's diff, so no pull_request_target/workflow_run, permission-widening, or script-injection-via-github-context surface applies here (out of scope for this diff either way, and covered by zizmor's advisory lane when workflows do change).
• Test fixtures (permission-rule-check.test.sh Cases 8f/8g): static fixture data exercising the new detections; no new execution surface, no secrets.

No logic flaws, authz gaps, injection surfaces, secret-handling issues, or workflow permission changes identified in this diff.

(Note: this PR's non-security correctness/coverage findings — the P4 single-match-per-block under-reporting, and the doc-drift in criteria.md/SKILL.md — were already raised on this thread by the earlier code-review pass and by Codex; not re-litigated here since this is the security-focused lane.)
· Branch

[claude]

Claude finished @kyle-sexton's task in 2m 35s —— View job

---

Review complete

• Gather context (diff via gh pr diff, CLAUDE.md/REVIEW.md lane split, current file state at HEAD 3d0ccbd)
• Review new commit against prior feedback
• Review test coverage changes
• Post findings

Repo has a claude-security-review.yml workflow, so per REVIEW.md this lane owns correctness/design/tests/docs and leaves security to that dedicated lane (this diff doesn't touch a security-relevant surface either way — it's a local, read-only advisory scanner).

Note on scope: the diff content at HEAD 3d0ccbd is identical to what was reviewed in the previous round on this PR (only the commit SHA changed, from 5d84343 — looks like a rebase, not a content fix). None of the three issues raised in that round — by Codex and by the prior Claude pass — have been addressed yet, so I'm re-flagging them below rather than silently repeating; treat this as confirmation they're still open, not new findings.

Posted 3 inline comments:

1. 🔴 Important — permission-rule-check.sh L220-223: the P4 check is still a single grep -qE over the whole $text block instead of looping over distinct matches like P1/P2/P2b do. Confirmed by re-reading the code: on a multi-line allowed-tools list (the format 5 real SKILL.md files in this repo use), two different inert tokens in two different entries only produce one finding, and the check isn't scoped to Bash(...), so a non-Bash rule like Read(%USERPROFILE%/notes.txt) also matches and gets a Bash-only "PATH command" remedy that doesn't fit.
2. 🟡 Nit — permission-rule-check.sh L173: P2_TILDE_USER_RULE_ERE has no boundary requirement before ~, so it matches a ~user/ segment embedded in a URL (e.g. Bash(curl https://example.com/~alice/index.html)) and misreports a portable URL as a leaked machine username (this is the issue Codex's automated review already flagged on this PR).
3. 🟡 Nit — permission-rule-check.test.sh L422-434: Case 8g only exercises the single-line allowed-tools form, so it doesn't catch chore: initialize marketplace scaffold #1 above.

Not anchorable to a changed line — documentation drift (also unaddressed from the prior round):

• reference/criteria.md still defines only ## P1/## P2/## P3 — no ## P4 section, and no mention of the ~user variant under P2. The script's own header calls this file "the criteria file" for these checks.
• SKILL.md L21 and the severity table, L83-86 still describe only P1/P2/P3 and don't cover P4's failure mode (inert token → grant never matches, unrelated to leaking a username).

Neither doc file is covered by an automated sync check, so this won't break CI, but the header comment now says "Flags five anti-patterns" while the docs it points to as the source of truth only describe three. Worth a follow-up pass on criteria.md (and bumping its own Version: header per its stated convention) and SKILL.md.

Checked out fine on this pass: CHANGELOG.md/plugin.json version bump (0.36.0 → 0.36.1, matches the [Fixed] entry), the scan_rule signature change (optional third file arg, both call sites — frontmatter and settings — updated correctly), and inert_grant_remedy()'s branch on */skills/*/SKILL.md.

Author-claimed test result ("94 checks passed") is unverified by me — I reviewed statically via Read/Grep/gh pr diff and did not execute the modified shell scripts in this lane.
· Branch

[github-actions]

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/31587496809
• Failure class: other
• Last SDK result: (no execution file was produced)

The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging.

Re-run the job to retry the review; a new push also retries it only if the caller's pull_request triggers include synchronize (the canonical security caller keeps it). An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator.

Re-running does NOT help for every class:

• rate-limit that persists across re-runs, or auth — the credential or usage budget needs an operator; retrying will not clear it.
• a run that exhausted its turn budget ("subtype":"error_max_turns" above) will exhaust it again. As the PR author, split the change into smaller PRs; raising --max-turns is a change to the caller workflow, not something you can set on this PR.

[github-actions]

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/31587496856
• Failure class: other
• Last SDK result: (no execution file was produced)

Re-run the job to retry the review. A new push re-triggers this lane only if the caller's pull_request triggers include synchronize (the canonical caller omits it).
An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator (auth).