fix: treat dangerous flags inside permissions.deny/ask rules as prohibitions, not usages - #103
Conversation
…bitions The permissions-dangerous-skip rule string-matches dangerous flags anywhere in a file, so a deny rule that exists to BLOCK a flag (e.g. "Bash(git commit --no-verify:*)") is reported as a CRITICAL usage and zeroes the Permissions category — the scanner penalizes its own recommended remediation of adding a deny list. Extend the existing prohibition handling (negated markdown mentions -> INFO) to matches whose index falls inside a permissions.deny or permissions.ask rule value in settings JSON. allow-rule matches stay CRITICAL, and unparseable JSON yields no spans so matches fail closed. Fixes affaan-m#102
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Prohibitive span detection and dangerous-skip integration src/rules/permissions.ts |
Added prohibitivePermissionRuleSpans() helper parsing settings-json to extract character ranges of all permissions.deny and permissions.ask string values. Updated permissions-dangerous-skip rule to compute these spans and emit INFO-level findings when dangerous pattern matches fall within those spans, skipping normal CRITICAL reporting. |
Test coverage for context-aware severity downgrade tests/rules/permissions.test.ts |
Added test cases validating --no-verify downgraded to INFO inside permissions.deny and permissions.ask, remains CRITICAL inside permissions.allow, and dangerously-skip-permissions inside permissions.deny downgraded to INFO. Included fail-closed test ensuring invalid JSON containing deny-shaped text with --no-verify still surfaces the flag as CRITICAL. |
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The PR title accurately and concisely describes the main change: treating dangerous flags inside permissions.deny/ask rules as prohibitions rather than usages, directly addressing the core issue. |
| Linked Issues check | ✅ Passed | The PR fully addresses issue #102 by implementing prohibitive permission rule span detection that downgrades dangerous flags in deny/ask rules to INFO while keeping allow rules CRITICAL. |
| Out of Scope Changes check | ✅ Passed | All changes are directly related to the linked issue #102: prohibitive span detection, rule downgrading logic, and comprehensive test coverage with no unrelated modifications. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Warning
There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.
🔧 ESLint
If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.
ESLint install failed due to a network error.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (const entry of entries) { | ||
| let from = 0; | ||
| let at: number; | ||
| while ((at = file.content.indexOf(entry, from)) !== -1) { | ||
| spans.push([at, at + entry.length]); | ||
| from = at + entry.length; | ||
| } |
There was a problem hiding this comment.
file.content.indexOf(entry, from) is position-agnostic: it finds every verbatim occurrence of the deny/ask entry string in the raw JSON, including any occurrence that happens to land inside a permissions.allow value with identical text. Consider a config with deny: ["Bash(git commit --no-verify:*)"] and allow: ["Bash(git commit --no-verify:*)"]. The deny entry scan calls indexOf twice and adds both occurrences to spans. The dangerous-flag match inside the allow value then falls within the second span and is downgraded to INFO — silently suppressing what should be a CRITICAL finding.
The PR's stated invariant ("Matches inside permissions.allow values are unaffected and stay CRITICAL") holds only when no deny/ask entry has the same text as an allow entry. There is no test covering the case where identical strings co-exist in deny and allow.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rules/permissions.ts`:
- Around line 172-186: The current span mapping uses file.content.indexOf(entry)
across the entire file which can misclassify matches outside the permissions
block; restrict the search to the textual region that corresponds to this
config.permissions object instead of the whole file. Locate the permissions
block in the file content (e.g., find the substring for JSON.stringify(perms)
or, preferably, use the AST/source range for the config node if available),
compute a base offset for that block, then run the existing while loop searching
only within that slice (use file.content.slice(baseOffset, baseOffset +
blockLength) and adjust pushed spans by adding baseOffset) so entries, spans,
perms, entries, and file.content.indexOf(entry) only match occurrences inside
the declared permissions block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 467e157f-c705-4477-9d9d-ae702452728f
📒 Files selected for processing (2)
src/rules/permissions.tstests/rules/permissions.test.ts
| const perms = (config as { permissions?: { deny?: unknown; ask?: unknown } }) | ||
| ?.permissions; | ||
| const entries = [ | ||
| ...(Array.isArray(perms?.deny) ? perms.deny : []), | ||
| ...(Array.isArray(perms?.ask) ? perms.ask : []), | ||
| ].filter((entry): entry is string => typeof entry === "string"); | ||
|
|
||
| const spans: Array<readonly [number, number]> = []; | ||
| for (const entry of entries) { | ||
| let from = 0; | ||
| let at: number; | ||
| while ((at = file.content.indexOf(entry, from)) !== -1) { | ||
| spans.push([at, at + entry.length]); | ||
| from = at + entry.length; | ||
| } |
There was a problem hiding this comment.
Global indexOf span mapping can misclassify dangerous allow entries as prohibitions.
At Line 183, span detection matches every occurrence of a deny/ask value across the whole file content. If the same string appears in permissions.allow (or elsewhere), that occurrence is also treated as prohibitive, so the dangerous flag can be downgraded to info incorrectly instead of staying critical.
Suggested fix direction
- const entries = [
- ...(Array.isArray(perms?.deny) ? perms.deny : []),
- ...(Array.isArray(perms?.ask) ? perms.ask : []),
- ].filter((entry): entry is string => typeof entry === "string");
-
- const spans: Array<readonly [number, number]> = [];
- for (const entry of entries) {
- let from = 0;
- let at: number;
- while ((at = file.content.indexOf(entry, from)) !== -1) {
- spans.push([at, at + entry.length]);
- from = at + entry.length;
- }
- }
+ // Compute spans from the deny/ask array regions in raw JSON content,
+ // then extract string-token ranges only within those regions.
+ // Do not map spans by searching value text globally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rules/permissions.ts` around lines 172 - 186, The current span mapping
uses file.content.indexOf(entry) across the entire file which can misclassify
matches outside the permissions block; restrict the search to the textual region
that corresponds to this config.permissions object instead of the whole file.
Locate the permissions block in the file content (e.g., find the substring for
JSON.stringify(perms) or, preferably, use the AST/source range for the config
node if available), compute a base offset for that block, then run the existing
while loop searching only within that slice (use file.content.slice(baseOffset,
baseOffset + blockLength) and adjust pushed spans by adding baseOffset) so
entries, spans, perms, entries, and file.content.indexOf(entry) only match
occurrences inside the declared permissions block.
|
Local review recommendation: request changes or add coverage before merge. The current fix for #102 may still miss the case where the same permission appears in both |
3 participants
Fixes #102.
Problem
The
permissions-dangerous-skiprule string-matches dangerous flags anywhere in a file. Apermissions.denyrule whose entire purpose is to block a flag — e.g."Bash(git commit --no-verify:*)"— is reported as a CRITICAL "Git hook verification bypass" and zeroes the Permissions category. Since "add a deny list" is the scanner's own remediation for thepermissions-no-deny-listHIGH finding, following the tool's advice makes the reported posture worse (full feedback loop documented in #102).Fix
The rule already downgrades prohibitive context to INFO for negated markdown mentions ("NEVER use --no-verify" → "Prohibition of --no-verify (good practice)"). This PR extends that same treatment to matches whose character span falls inside a
permissions.denyorpermissions.askrule value in settings JSON:prohibitivePermissionRuleSpans()parses the settings JSON and returns the content spans of every deny/ask rule value. Non-settings files and unparseable JSON return no spans, so matches fail closed (stay CRITICAL).Deny/ask rule blocking --no-verify (good practice)), consistent with the existing negation downgrade.permissions.allowvalues are unaffected and stay CRITICAL — allowing a dangerous flag is a real finding.The span check runs before the 100-char negation-context check, so deny entries are classified by their structural position rather than by whatever text happens to precede them.
Tests
Six new cases in
tests/rules/permissions.test.ts(95 total, all passing):--no-verify→ INFO ×2, not CRITICAL--no-verify→ INFO--no-verify→ stays CRITICALdangerously-skip-permissions→ INFOnpm run typecheckandnpm run lintclean.Real-world verification
Against the repo that prompted #102 (a
.claude/settings.jsonwith deny rules blocking--no-verify), built from this branch:settings.json:8,9, Permissions 0/100Related: this is the same context-insensitivity family as #100 (string literals in hook scripts) — this PR deliberately fixes only the structurally-decidable JSON case and doesn't touch the markdown/script heuristics.
Summary by CodeRabbit
New Features
Tests
Greptile Summary
This PR fixes a false-positive where
permissions-dangerous-skipwould flag dangerous flags (e.g.--no-verify) found insidepermissions.denyorpermissions.askentries as CRITICAL, even though those entries exist to block the flag. A new helperprohibitivePermissionRuleSpans()parses the settings JSON, collects the character spans of every deny/ask value, and downgrades any match that falls within those spans to INFO — consistent with the existing negation-word heuristic for markdown prose.prohibitivePermissionRuleSpans()locates deny/ask entry spans in the raw JSON by exact-stringindexOf; non-settings files and unparseable JSON return no spans so the rule fails closed.dangerously-skip-permissions, and invalid-JSON (fail-closed) scenarios, bringing the suite to 95 tests.Confidence Score: 3/5
The core logic change is sound for the common case, but the span-building strategy can silently suppress a CRITICAL finding when the same entry string appears in both deny and allow — the exact scenario the PR claims is safe.
The prohibitivePermissionRuleSpans helper searches the raw JSON text with indexOf for each deny/ask entry string without constraining the search to the deny/ask array regions. If an identical string also exists in the allow array, the second indexOf hit lands on the allow value and adds it to the prohibitive spans. A dangerous flag inside that allow entry then gets silently downgraded from CRITICAL to INFO — a false negative in a security scanner where the PR's stated invariant breaks. The test suite validates the allow case in isolation but not in the presence of a matching deny entry, leaving this gap undetected.
The span-building loop in src/rules/permissions.ts (lines 180–186) needs a position-aware approach — for example tracking the JSON-parsed offset of each entry rather than using raw content search — to guarantee that allow-list occurrences can never be captured in the prohibitive spans.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Dangerous flag match found\nin file content] --> B{File is\nsettings-json?} B -- No --> E[Check negation\ncontext 100 chars] B -- Yes --> C{JSON\nparseable?} C -- No --> E C -- Yes --> D[Build prohibitiveSpans\nfrom deny + ask entries\nvia indexOf] D --> F{match.index\ninside a span?} F -- Yes --> G[INFO finding\nDeny/ask rule blocking flag\ngood practice] F -- No --> E E --> H{Negation word\nin context?} H -- Yes --> I[INFO finding\nProhibition — good practice] H -- No --> J[CRITICAL finding\nDangerous flag] style G fill:#d4edda style I fill:#d4edda style J fill:#f8d7daReviews (1): Last reviewed commit: "fix: treat dangerous flags inside permis..." | Re-trigger Greptile