fix: prevent false positives for deny rules and PreToolUse block hooks#9
Open
kai-agent-free wants to merge 4 commits intoaffaan-m:mainfrom
Open
fix: prevent false positives for deny rules and PreToolUse block hooks#9kai-agent-free wants to merge 4 commits intoaffaan-m:mainfrom
kai-agent-free wants to merge 4 commits intoaffaan-m:mainfrom
Conversation
Fixes affaan-m#5 and affaan-m#8. The scanner was running dangerous-pattern regex (sudo, rm -rf, etc.) against the raw text of settings.json without checking whether the matched string is inside a permissions.deny array, permissions.allow array, or a PreToolUse block hook (exit 1/2). This caused well-hardened configs with deny lists and block hooks to score F when they should score A. Changes: - Add safe-context detection in src/rules/hooks.ts that identifies byte ranges belonging to deny/allow list entries and PreToolUse block hooks (hooks that exit 1 or exit 2) - Modified findAllMatches() to automatically filter out matches that fall within safe contexts - Added 5 tests covering deny-list false positives, block hook false positives, allow-list false positives, combined configs, and ensuring real dangerous hooks are still flagged Before: deny-only config scored F (10/100) with 5+ false positives After: same config scores A (99/100) with 0 false positives
📝 WalkthroughWalkthroughAdds safe-context detection to hook scanning: computes cached byte-ranges for deny-list entries and PreToolUse block hooks, collects string leaves from parsed JSON, and filters regex matches to ignore hits within those safe ranges to prevent false positives. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Scanner
participant Parser as JSON Parser
participant SafeBuilder as SafeRanges Builder
participant Cache as SafeRanges Cache
participant Matcher as Hook Pattern Matcher
CLI->>Parser: read file bytes & parse JSON
Parser->>SafeBuilder: provide JSON structure
SafeBuilder->>SafeBuilder: locate deny-list & PreToolUse string leaves
SafeBuilder->>Cache: store computed SafeRanges for file
CLI->>Matcher: run hook regexes over file bytes
Matcher->>Cache: request SafeRanges for file
Cache-->>Matcher: return SafeRanges
Matcher->>Matcher: filter out matches within SafeRanges
Matcher-->>CLI: emit filtered findings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rules/hooks.ts`:
- Around line 8-11: Update the mutable types to immutable variants: make the
SafeRange interface fields readonly (e.g., readonly start and readonly end) and
replace all plain array types in this file (instances of SafeRange[] and
string[]) with ReadonlyArray (e.g., ReadonlyArray<SafeRange>,
ReadonlyArray<string>); search for usages referenced in this file (including the
occurrences around lines shown: 21-23, 62-63, 105-107, 123-124, 211) and change
any parameter, variable, or return type annotations to use readonly fields and
ReadonlyArray to comply with the repository immutability rule.
- Around line 41-49: The current approach iterates over `target`, stringifies
each entry and searches `content` globally, which incorrectly marks occurrences
outside the intended JSON node context as safe; replace the global substring
search with JSON AST-based node location matching: parse `content` into an AST
(so you can get node offsets/lengths), walk to the specific path-scoped nodes
(e.g., `permissions.*` string literals and block-hook entries referenced in the
later block at 89-96), and for each AST string node whose value equals the
`entry` push a single range using that node's exact start/end offsets (instead
of using `content.indexOf(needle, idx)`); update both the loop that references
`target`/`content`/`ranges.push` and the similar logic at 89-96 to use AST node
offsets so matches are limited to the correct JSON locations only.
- Around line 207-219: The current cache keeps strong references via
contentKeyMap and will grow unbounded; replace the two-map approach with a
bounded cache: introduce a max size constant (e.g., MAX_SAFE_RANGE_CACHE) and
implement an LRU or simple FIFO eviction for entries stored by content key, so
getSafeRanges uses that bounded cache to store the result of buildSafeRanges
instead of keeping contentKeyMap forever; update references to safeRangeCache,
contentKeyMap and getSafeRanges (and still call buildSafeRanges when a miss) and
evict the least-recently-used/oldest entry when inserting past the max size to
avoid unbounded memory growth.
- Line 83: Replace the direct RegExp.test usage in the commands.some callback:
instead of /exit\s+[12]\b/.test(c), call the project's helper (findAllMatches)
or use String.matchAll on the string to search for the pattern; update the
predicate that defines isBlock (the commands.some((c) => ... ) expression) to
use findAllMatches(/exit\s+[12]\b/, c) or Array.from(c.matchAll(...)) and check
for any matches, so no RegExp.prototype.test is called in src/rules/hooks.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 217873f0-126a-4322-89e2-8e7dac24319c
⛔ Files ignored due to path filters (2)
dist/action.jsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (2)
src/rules/hooks.tstests/rules/hooks.test.ts
Comment on lines
+8
to
+11
| interface SafeRange { | ||
| start: number; | ||
| end: number; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use immutable typings for interfaces and arrays in changed code
Changed segments still use mutable fields/array types (start, end, SafeRange[], string[]). This violates the repository immutability rule.
🧩 Minimal typing updates
interface SafeRange {
- start: number;
- end: number;
+ readonly start: number;
+ readonly end: number;
}
function findStringRangesAtPath(
content: string,
- path: string[],
-): SafeRange[] {
+ path: ReadonlyArray<string>,
+): ReadonlyArray<SafeRange> {
function findBlockHookRanges(content: string): SafeRange[] {
- const ranges: SafeRange[] = [];
+ const ranges: Array<SafeRange> = [];
function collectStrings(obj: unknown): string[] {
- const result: string[] = [];
+ const result: Array<string> = [];As per coding guidelines: src/**/*.ts: "Use ReadonlyArray for all array types and readonly fields in all interfaces for immutability."
Also applies to: 21-23, 62-63, 105-107, 123-124, 211-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rules/hooks.ts` around lines 8 - 11, Update the mutable types to
immutable variants: make the SafeRange interface fields readonly (e.g., readonly
start and readonly end) and replace all plain array types in this file
(instances of SafeRange[] and string[]) with ReadonlyArray (e.g.,
ReadonlyArray<SafeRange>, ReadonlyArray<string>); search for usages referenced
in this file (including the occurrences around lines shown: 21-23, 62-63,
105-107, 123-124, 211) and change any parameter, variable, or return type
annotations to use readonly fields and ReadonlyArray to comply with the
repository immutability rule.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/rules/hooks.ts">
<violation number="1" location="src/rules/hooks.ts:45">
P1: Safe-range detection is global-text based, so identical strings outside the intended deny/allow or block-hook context can be incorrectly suppressed.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Make SafeRange interface fields readonly - Use ReadonlyArray types throughout for immutability compliance - Replace global substring search with path-scoped region matching to prevent cross-context false negatives - Replace RegExp.test() with String.matchAll() per project guidelines - Fix cache memory leak: replace unbounded WeakMap/Map with bounded Map (FIFO eviction at 256 entries)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/rules/hooks.ts (1)
117-123:⚠️ Potential issue | 🔴 CriticalFail closed when block-hook region cannot be located
On Line 117,
content.indexOf(JSON.stringify(hookEntry))can miss pretty-printed JSON (whitespace/newlines). The current fallback (Line 118-123) marks the entire file as safe, which can hide real malicious matches outside the block hook context.🔧 Minimal safe fix
const hookJson = JSON.stringify(hookEntry); const hookIdx = content.indexOf(hookJson); - let hookStart = 0; - let hookEnd = content.length; - if (hookIdx !== -1) { - hookStart = hookIdx; - hookEnd = hookIdx + hookJson.length; - } + if (hookIdx === -1) { + // Fail closed: do not widen safe context to the whole document. + continue; + } + const hookStart = hookIdx; + const hookEnd = hookIdx + hookJson.length;Based on learnings: Implement false positive prevention in permission analysis by JSON-parsing settings to check only the allow array, with negation-aware context checking to downgrade prohibitive mentions to
infolevel.Also applies to: 124-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rules/hooks.ts` around lines 117 - 123, The current logic uses content.indexOf(hookJson) and falls back to marking the whole file safe when it can't find the block, which can hide malicious matches; change the behavior in the hook-location logic that sets hookStart/hookEnd (references: hookJson, hookEntry, hookIdx, hookStart, hookEnd) to first attempt JSON-aware matching: parse hookJson/hookEntry to extract the "allow" array, then JSON.parse the file content (or use a JSON/AST-aware extractor) to locate that allow array instance rather than doing a plain substring search; if the block still cannot be reliably located, fail closed by restricting analysis to only the parsed allow array (do not treat the whole file as safe) and add negation-aware context checking so mentions that appear in negated contexts are downgraded to info-level findings (apply same change around the similar logic at the other region referenced at lines 124-131).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rules/hooks.ts`:
- Around line 54-61: The bracket-matching loop that sets arrayEnd (scanning
content from bracketIdx) is blind to quoted strings so brackets inside JSON
strings break the logic; update the routine that computes arrayEnd (starting at
bracketIdx and updating depth) to be string-aware (track inString and escape
sequences to ignore brackets inside quotes) or, better, extract the JSON
substring and JSON.parse the settings to locate the actual "allow" array
boundaries instead of manual scanning; also incorporate negation-aware context
checks when analyzing entries so prohibitive mentions found only in negated
contexts are downgraded to info severity.
---
Duplicate comments:
In `@src/rules/hooks.ts`:
- Around line 117-123: The current logic uses content.indexOf(hookJson) and
falls back to marking the whole file safe when it can't find the block, which
can hide malicious matches; change the behavior in the hook-location logic that
sets hookStart/hookEnd (references: hookJson, hookEntry, hookIdx, hookStart,
hookEnd) to first attempt JSON-aware matching: parse hookJson/hookEntry to
extract the "allow" array, then JSON.parse the file content (or use a
JSON/AST-aware extractor) to locate that allow array instance rather than doing
a plain substring search; if the block still cannot be reliably located, fail
closed by restricting analysis to only the parsed allow array (do not treat the
whole file as safe) and add negation-aware context checking so mentions that
appear in negated contexts are downgraded to info-level findings (apply same
change around the similar logic at the other region referenced at lines
124-131).
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/rules/hooks.ts">
<violation number="1" location="src/rules/hooks.ts:54">
P1: The manual array parsing logic fails on strings containing brackets, prematurely ending the array region and causing false positives.</violation>
<violation number="2" location="src/rules/hooks.ts:116">
P1: Finding object boundaries via `JSON.stringify` fails on formatted JSON, falling back to a global search that causes false negatives.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Address CodeRabbit review comments: 1. Line 54: Bracket counting now skips brackets inside JSON string values (tracks inString/escaped state) to prevent premature array region ending. 2. Line 116: Replace JSON.stringify-based object matching (fails on formatted JSON) with raw-text brace counting that walks the actual PreToolUse array to find each hook entry's boundaries.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/rules/hooks.ts">
<violation number="1" location="src/rules/hooks.ts:147">
P1: Hook-region lookup is ambiguous and can mark the wrong PreToolUse object as safe, causing block hooks to still produce false positives.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/rules/hooks.ts (1)
112-120: Consider removing defensive checks for non-existent schema fields.Lines 112, 114-119 check for
h.command,h.hooks,s.commandfields, but the SettingsSchema defines PreToolUse entries with onlymatcherandhookfields. Line 113 correctly accessesh.hook, making the other branches dead code for schema-compliant configurations.♻️ Proposed simplification
// Collect all command strings from the hook - const commands: Array<string> = []; - if (typeof h.command === "string") commands.push(h.command); - if (typeof h.hook === "string") commands.push(h.hook); - if (Array.isArray(h.hooks)) { - for (const sub of h.hooks) { - const s = sub as Record<string, unknown>; - if (typeof s.command === "string") commands.push(s.command); - if (typeof s.hook === "string") commands.push(s.hook); - } - } + const command = typeof h.hook === "string" ? h.hook : ""; + if (!command) continue; // Only mark as safe if the hook blocks/rejects (exit 1 or exit 2) - const isBlock = commands.some( - (command) => Array.from(command.matchAll(/exit\s+[12]\b/g)).length > 0, + const isBlock = Array.from(command.matchAll(/exit\s+[12]\b/g)).length > 0; );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rules/hooks.ts` around lines 112 - 120, The code in src/rules/hooks.ts contains defensive checks for non-existent fields (checks for h.command, Array.isArray(h.hooks), s.command) that are dead for schema-compliant PreToolUse entries; simplify the logic in the function that builds the commands array by removing the branches that check h.command and the entire Array.isArray(h.hooks) loop and only push h.hook when typeof h.hook === "string" (and likewise only check s.hook if you keep nested structures), ensuring you reference the same variables (h, s, commands) and update any related types/comments to reflect the simplified schema-aware behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rules/hooks.ts`:
- Around line 150-159: The backward brace-walk around variables
braceDepth/objStart (starting at needleIdx and iterating to preToolUseBracket)
isn't string-aware and miscounts braces inside JSON strings; update that loop to
mirror the forward loop's string/escape handling by tracking inStr (and the
quote char) and esc flags so that { and } are ignored while inStr is true (and
toggling inStr only when not escaped), then only count braces when not inside a
string; this will ensure the computed objStart correctly skips braces that are
part of string values.
---
Nitpick comments:
In `@src/rules/hooks.ts`:
- Around line 112-120: The code in src/rules/hooks.ts contains defensive checks
for non-existent fields (checks for h.command, Array.isArray(h.hooks),
s.command) that are dead for schema-compliant PreToolUse entries; simplify the
logic in the function that builds the commands array by removing the branches
that check h.command and the entire Array.isArray(h.hooks) loop and only push
h.hook when typeof h.hook === "string" (and likewise only check s.hook if you
keep nested structures), ensuring you reference the same variables (h, s,
commands) and update any related types/comments to reflect the simplified
schema-aware behavior.
…ard scan Addresses review comments: 1. Hook-region lookup was ambiguous — finding a 'unique' string and walking backwards could match the wrong PreToolUse object 2. Backward brace matching was not string-aware — braces inside JSON strings (e.g. "matcher": "}") would miscount depth 3. JSON.stringify for finding object boundaries fails on formatted JSON — the old approach compared minified JSON.stringify output against potentially formatted source text Fix: enumerate top-level objects in the PreToolUse array by walking forward with string-aware brace counting, matching each hook entry by its array index. This is deterministic and handles all formatting.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rules/hooks.ts (1)
111-120: Remove dead code that checks for non-existent schema fields.Per the schema in
src/types.ts(lines 350-357),PreToolUseentries only havematcherandhookfields. Lines 112 and 114-120 check forh.commandandh.hookswhich don't exist in the schema.Only line 113 (
h.hook) is valid. The extra checks are harmless but add confusion.♻️ Proposed simplification
// Collect all command strings from the hook - const commands: Array<string> = []; - if (typeof h.command === "string") commands.push(h.command); - if (typeof h.hook === "string") commands.push(h.hook); - if (Array.isArray(h.hooks)) { - for (const sub of h.hooks) { - const s = sub as Record<string, unknown>; - if (typeof s.command === "string") commands.push(s.command); - if (typeof s.hook === "string") commands.push(s.hook); - } - } + const hookScript = typeof h.hook === "string" ? h.hook : ""; // Only mark as safe if the hook blocks/rejects (exit 1 or exit 2) const isBlock = commands.some( - (command) => Array.from(command.matchAll(/exit\s+[12]\b/g)).length > 0, + () => Array.from(hookScript.matchAll(/exit\s+[12]\b/g)).length > 0, );Or if you want to keep the defensive approach for future schema changes, add a comment explaining the intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rules/hooks.ts` around lines 111 - 120, The loop building commands contains dead checks for non-existent PreToolUse fields (h.command and h.hooks) — update the logic in the function that processes PreToolUse entries (the block where commands: Array<string> is populated and the variable h is iterated) to only read h.hook per the schema (remove the checks for h.command, the Array.isArray(h.hooks) branch, and the inner sub handling), or if you want to keep defensive code add a short comment explaining that those checks are intentional for future schema changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/rules/hooks.ts`:
- Around line 111-120: The loop building commands contains dead checks for
non-existent PreToolUse fields (h.command and h.hooks) — update the logic in the
function that processes PreToolUse entries (the block where commands:
Array<string> is populated and the variable h is iterated) to only read h.hook
per the schema (remove the checks for h.command, the Array.isArray(h.hooks)
branch, and the inner sub handling), or if you want to keep defensive code add a
short comment explaining that those checks are intentional for future schema
changes.
Owner
|
Thanks for the PR. It currently has merge conflicts with main. Could you rebase and push an updated branch so we can review merge readiness? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #5 and #8.
The scanner was running dangerous-pattern regex (
sudo,rm -rf, etc.) against the raw text ofsettings.jsonwithout checking whether the matched string is inside apermissions.denyarray,permissions.allowarray, or a PreToolUse block hook (exit 1/exit 2).This caused well-hardened configs with deny lists and block hooks to score F (10/100) when they should score A.
Changes
src/rules/hooks.ts: Added safe-context detection that identifies byte ranges belonging to:permissions.denyentries (deny rules are security controls, not threats)permissions.allowentries (permission rules are not hooks)exit 1orexit 2(these reject/block actions)Modified
findAllMatches()to automatically filter out regex matches that fall within safe contextsResults are cached per content string to avoid re-parsing JSON for every pattern
tests/rules/hooks.test.ts: Added 5 new tests:Results
All 1287 tests pass (1282 existing + 5 new).
Summary by cubic
Prevents false positives in hook scanning by skipping dangerous-pattern matches inside
permissions.deny,permissions.allow, and PreToolUse block hooks (exit 1/2). Fixes #5 and #8; hardened configs now score correctly (F → A) with format-safe, deterministic detection.hooks.PreToolUseto bound block-hook regions reliably.findAllMatches()to usematchAlland filter out matches inside safe ranges.Written for commit 1266bce. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests