Harden Cursor post-review hook validation#1
Conversation
📝 WalkthroughWalkthroughThe post-review hook was rewritten to use safe command tokenization for detecting review tool invocations, structured JSON parsing of tool output to determine clean/issues outcomes, exit-code-based success checks, and hashed-digest state file naming under an XDG state directory. A new test script validates this behavior end-to-end. ChangesPost-review hook rewrite and tests
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant Hook as post-review-context.mjs
participant ToolOutput as Tool Output (NDJSON)
participant StateFile as State Directory
Agent->>Hook: invoke with command + tool_output
Hook->>Hook: tokenizeSimpleCommand(command)
Hook->>Hook: detect review tool + --agent, reject autofix
alt command recognized
Hook->>Hook: toolSucceeded(exit code fields)
Hook->>ToolOutput: parse NDJSON events
ToolOutput-->>Hook: complete event with findings
alt outcome == clean and tool succeeded
Hook->>StateFile: write hashed digest reminder state
Hook-->>Agent: additional_context (clean reminder)
else
Hook->>StateFile: clear stale clean state
Hook-->>Agent: generic complete message
end
else
Hook-->>Agent: log unrecognized output
end
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
CodeRabbit posted this review as a comment because GitHub doesn't allow pull request authors to request changes on their own pull requests.
Actionable comments posted: 3
🧹 Nitpick comments (2)
scripts/test-post-review-context.mjs (2)
163-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative reminder check for the failed-exit-code case.
The
clean(Lines 79-106) andfindings(Lines 108-135) blocks both follow up with a secondrunHookcall to confirm reminder state was (not) persisted. Thefailedblock only asserts the immediate output is empty but never confirms that no clean-review reminder state was written despiteexit_code: 1. Without this, a regression that persists state on failed exits would go undetected.✅ Suggested addition
assert(failed === "", "failed review commands must not emit clean context"); + + const reminder = runHook( + { + conversation_id: "failed", + tool_name: "shell", + tool_input: { command: "git status --short" }, + tool_output: "", + }, + stateDir, + ); + assert(reminder === "", "failed review commands must not persist clean review reminders"); });🤖 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 `@scripts/test-post-review-context.mjs` around lines 163 - 175, The failed-exit-code path in runHook is only checking that no clean context is emitted, but it does not verify that reminder state is not persisted. Update the failed block in test-post-review-context.mjs to add a second runHook/assertion like the clean and findings cases, so it explicitly confirms no reminder state is written when exit_code is 1. Use the existing withStateDir, runHook, and failed test setup to mirror the negative persistence check.
54-176: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd coverage for the 0600 state-file permission hardening.
The PR objectives specifically call out moving reminder state to hashed,
0600-permission files as a security hardening measure, but no test in this suite verifies the resulting file's mode bits. Consider adding an assertion (e.g., viafs.statSyncon the file created understateDirafter a successful clean-review call) to lock in this guarantee against regression.🤖 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 `@scripts/test-post-review-context.mjs` around lines 54 - 176, The test suite in runTests currently verifies reminder behavior but not the new state-file hardening; add coverage that inspects the reminder file created under the stateDir after a successful clean-review path. Use the existing clean-review flow around runHook and parseHookOutput, then assert the created state file’s permissions are 0600 (for example by statting the file under the hashed state storage path), so the permission guarantee is locked in against regressions.
🤖 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 `@hooks/post-review-context.mjs`:
- Around line 38-51: `postReviewContext` should treat an empty token list from
`tokenizeSimpleCommand` the same as a missing parse result, because `[]`
currently bypasses the `if (!argv)` guard and leads to `path.basename(argv[0])`
throwing on undefined. Update the early-return check in
`isCodeRabbitReviewCommand` so it rejects both null and zero-length argv before
dereferencing `argv[0]`, preserving the hook’s “must never break” behavior for
empty or whitespace-only commands.
- Around line 182-189: The state directory creation in statePath(input) is
unguarded, so mkdirSync can abort the hook if the state location is unwritable.
Wrap the directory setup and path creation path in the existing best-effort
handling used by the post-review context flow, and make statePath(input) fall
back safely when mkdirSync fails so the review context can still be emitted.
- Around line 63-73: tokenizeSimpleCommand() is treating every backslash as an
escape, which breaks Windows path-qualified commands before the
coderabbit.exe/cr.exe allowlist check. Update the tokenization logic in
tokenizeSimpleCommand() to preserve or specially handle backslashes when they
appear as Windows path separators, so inputs like C:\Tools\coderabbit.exe review
--agent remain intact for command validation.
---
Nitpick comments:
In `@scripts/test-post-review-context.mjs`:
- Around line 163-175: The failed-exit-code path in runHook is only checking
that no clean context is emitted, but it does not verify that reminder state is
not persisted. Update the failed block in test-post-review-context.mjs to add a
second runHook/assertion like the clean and findings cases, so it explicitly
confirms no reminder state is written when exit_code is 1. Use the existing
withStateDir, runHook, and failed test setup to mirror the negative persistence
check.
- Around line 54-176: The test suite in runTests currently verifies reminder
behavior but not the new state-file hardening; add coverage that inspects the
reminder file created under the stateDir after a successful clean-review path.
Use the existing clean-review flow around runHook and parseHookOutput, then
assert the created state file’s permissions are 0600 (for example by statting
the file under the hashed state storage path), so the permission guarantee is
locked in against regressions.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 00b64575-2a35-4608-8874-c46a4ed9357c
⛔ Files ignored due to path filters (1)
package.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
hooks/post-review-context.mjsscripts/test-post-review-context.mjs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
coderabbitai/bitbucket(manual)
📜 Review details
🔇 Additional comments (3)
scripts/test-post-review-context.mjs (2)
1-176: LGTM!
30-34: 🎯 Functional CorrectnessNo issue:
CODERABBIT_HOOK_DEBUGis the hook’s debug gate, and0keeps stderr empty here.> Likely an incorrect or invalid review comment.hooks/post-review-context.mjs (1)
123-180: LGTM!
|
|
||
| const argv = tokenizeSimpleCommand(command); | ||
| if (!argv) { | ||
| return false; | ||
| } | ||
|
|
||
| const binary = path.basename(argv[0]).toLowerCase(); | ||
| return ( | ||
| ["coderabbit", "coderabbit.exe", "cr", "cr.exe"].includes(binary) && | ||
| argv[1] === "review" && | ||
| argv.includes("--agent") && | ||
| !argv.includes("autofix") | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard against an empty token list before dereferencing argv[0].
tokenizeSimpleCommand returns an empty array [] for an empty or whitespace-only command (the loop never pushes a token). The guard at Line 40 (if (!argv)) only catches null, so [] passes through and path.basename(argv[0]) receives undefined, throwing TypeError [ERR_INVALID_ARG_TYPE]. Since this runs at module top level (Line 210), the exception crashes the hook — contradicting the "must never break the hook" intent. command === "" or " " from tool_input reaches this path.
🛡️ Proposed fix
const argv = tokenizeSimpleCommand(command);
- if (!argv) {
+ if (!argv || argv.length === 0) {
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const argv = tokenizeSimpleCommand(command); | |
| if (!argv) { | |
| return false; | |
| } | |
| const binary = path.basename(argv[0]).toLowerCase(); | |
| return ( | |
| ["coderabbit", "coderabbit.exe", "cr", "cr.exe"].includes(binary) && | |
| argv[1] === "review" && | |
| argv.includes("--agent") && | |
| !argv.includes("autofix") | |
| ); | |
| } | |
| const argv = tokenizeSimpleCommand(command); | |
| if (!argv || argv.length === 0) { | |
| return false; | |
| } | |
| const binary = path.basename(argv[0]).toLowerCase(); | |
| return ( | |
| ["coderabbit", "coderabbit.exe", "cr", "cr.exe"].includes(binary) && | |
| argv[1] === "review" && | |
| argv.includes("--agent") && | |
| !argv.includes("autofix") | |
| ); | |
| } |
🤖 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 `@hooks/post-review-context.mjs` around lines 38 - 51, `postReviewContext`
should treat an empty token list from `tokenizeSimpleCommand` the same as a
missing parse result, because `[]` currently bypasses the `if (!argv)` guard and
leads to `path.basename(argv[0])` throwing on undefined. Update the early-return
check in `isCodeRabbitReviewCommand` so it rejects both null and zero-length
argv before dereferencing `argv[0]`, preserving the hook’s “must never break”
behavior for empty or whitespace-only commands.
| if (escaped) { | ||
| token += char; | ||
| hasToken = true; | ||
| escaped = false; | ||
| continue; | ||
| } | ||
|
|
||
| if (char === "\\") { | ||
| escaped = true; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for how the command is produced/invoked and any Windows path expectations in tests.
rg -nP -C3 '(coderabbit(\.exe)?|cr\.exe|--agent)' scripts/test-post-review-context.mjs
rg -nP -C3 'command' hooks/post-review-context.mjsRepository: coderabbitai/cursor-plugin
Length of output: 2955
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== hooks/post-review-context.mjs (relevant ranges) =="
nl -ba hooks/post-review-context.mjs | sed -n '1,140p'
echo
echo "== scripts/test-post-review-context.mjs (relevant ranges) =="
nl -ba scripts/test-post-review-context.mjs | sed -n '1,240p'
echo
echo "== Search for basename and Windows path handling =="
rg -n "basename|win32|Windows|\\\\|coderabbit\\.exe|cr\\.exe" hooks scripts -SRepository: coderabbitai/cursor-plugin
Length of output: 258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== hooks/post-review-context.mjs (1-140) =="
sed -n '1,140p' hooks/post-review-context.mjs | cat -n
echo
echo "== scripts/test-post-review-context.mjs (1-240) =="
sed -n '1,240p' scripts/test-post-review-context.mjs | cat -n
echo
echo "== Search for basename and Windows path handling =="
rg -n "basename|win32|Windows|\\\\|coderabbit\\.exe|cr\\.exe" hooks scripts -SRepository: coderabbitai/cursor-plugin
Length of output: 12087
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Windows/support mentions =="
rg -n "Windows|win32|cross-platform|path-qualified|\\.exe|coderabbit review --agent|cr review --agent" . -S
echo
echo "== Tokenizer behavior probe =="
node <<'JS'
const path = require('node:path');
function tokenizeSimpleCommand(command) {
const argv = [];
let token = "";
let quote = "";
let escaped = false;
let hasToken = false;
for (let index = 0; index < command.length; index += 1) {
const char = command[index];
if (escaped) { token += char; hasToken = true; escaped = false; continue; }
if (char === "\\") { escaped = true; continue; }
if (quote) { if (char === quote) { quote = ""; } else { token += char; hasToken = true; } continue; }
if (char === "'" || char === '"') { quote = char; hasToken = true; continue; }
if (char === "$" && command[index + 1] === "(") return null;
if (char === "#" || char === "\n" || char === "\r" || "|;&<>()`".includes(char)) return null;
if (/\s/.test(char)) { if (hasToken) { argv.push(token); token = ""; hasToken = false; } continue; }
token += char; hasToken = true;
}
if (escaped || quote) return null;
if (hasToken) argv.push(token);
return argv;
}
for (const cmd of [
String.raw`coderabbit review --agent`,
String.raw`C:\Tools\coderabbit.exe review --agent`,
String.raw`"C:\Tools\coderabbit.exe" review --agent`,
String.raw`/home/user/.local/bin/cr review --agent`,
String.raw`C:\\Tools\\coderabbit.exe review --agent`,
]) {
const argv = tokenizeSimpleCommand(cmd);
const binaryPosix = argv ? path.basename(argv[0]).toLowerCase() : null;
const binaryWin32 = argv ? path.win32.basename(argv[0]).toLowerCase() : null;
console.log(JSON.stringify({cmd, argv, binaryPosix, binaryWin32}, null, 2));
}
JSRepository: coderabbitai/cursor-plugin
Length of output: 3139
Handle Windows path separators before tokenizing. tokenizeSimpleCommand() treats every \ as an escape, so a path-qualified invocation like C:\Tools\coderabbit.exe review --agent gets mangled before the coderabbit.exe/cr.exe allowlist check. If full Windows paths should be accepted, \ needs to be preserved or handled as a path separator here.
🤖 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 `@hooks/post-review-context.mjs` around lines 63 - 73, tokenizeSimpleCommand()
is treating every backslash as an escape, which breaks Windows path-qualified
commands before the coderabbit.exe/cr.exe allowlist check. Update the
tokenization logic in tokenizeSimpleCommand() to preserve or specially handle
backslashes when they appear as Windows path separators, so inputs like
C:\Tools\coderabbit.exe review --agent remain intact for command validation.
| function statePath(input) { | ||
| const key = String(input?.conversation_id || input?.generation_id || "global").replace(/[^A-Za-z0-9_-]/g, ""); | ||
| return path.join(os.tmpdir(), `coderabbit-clean-review-${key || "global"}.json`); | ||
| const key = String(input?.conversation_id || input?.generation_id || "global"); | ||
| const digest = createHash("sha256").update(key).digest("hex").slice(0, 32); | ||
| const baseDir = | ||
| process.env.XDG_STATE_HOME || (os.homedir() ? path.join(os.homedir(), ".local", "state") : os.tmpdir()); | ||
| const dir = path.join(baseDir, "coderabbit", "cursor-plugin"); | ||
| mkdirSync(dir, { recursive: true, mode: 0o700 }); | ||
| return path.join(dir, `clean-review-${digest}.json`); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the unshown region around line 242 to confirm error handling.
sed -n '190,260p' hooks/post-review-context.mjsRepository: coderabbitai/cursor-plugin
Length of output: 3031
Wrap statePath(input) in the best-effort guard. mkdirSync can throw on an unwritable state dir, and this unguarded call will abort the hook before it emits the review context.
🤖 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 `@hooks/post-review-context.mjs` around lines 182 - 189, The state directory
creation in statePath(input) is unguarded, so mkdirSync can abort the hook if
the state location is unwritable. Wrap the directory setup and path creation
path in the existing best-effort handling used by the post-review context flow,
and make statePath(input) fall back safely when mkdirSync fails so the review
context can still be emitted.
Summary
hooks/post-review-context.mjsso it only injects review-complete context for a simplecoderabbitorcr review --agentcommand shape.--agentJSONL completion output instead of loose phrases such asCodeRabbit raised 0 issues./tmp/coderabbit-clean-review-*.jsonfiles to a per-user state directory with hashed keys and0600files.Evidence And Reasoning
Pylon issue
25004/572363e0-6184-4996-93ad-35bf83cb4c31reports that the Cursor pluginpostToolUsehook can be tricked into injecting a clean CodeRabbit review result when the raw shell command merely contains# coderabbit reviewand the raw output contains a clean-looking phrase. PagerDuty incidentQ30LPOMXDYA13A/#2175later escalated the same Pylon issue throughSupport On-call Serviceat2026-07-05T12:06:54Zwith high urgency.I reproduced the exact reporter PoC on
mainat0066dea: the spoof command emitted the clean additional context, and a follow-up unrelatedgit status --shortemitted the persisted clean-review reminder. The root cause is permissive regex validation over untrusted shell command text and output. A shell comment satisfies the command regex without invoking CodeRabbit, and a loose phrase satisfies the clean-result regex without structured CLI output.The affected behavior was introduced in
f24249d(Make CodeRabbit the default reviewer and prevent duplicate manual reviews), then remained present through0066dea(Pre-publication cleanup). No matching GitHub issue, repository security advisory, or Linear issue was found forpost-review-context,coderabbit-clean-review,additional_context,spoofable, orCWE-345.Datadog log search for
2026-07-05T10:00:00Zthrough2026-07-05T13:00:00Zfound zero hits forcoderabbitai/cursor-plugin,post-review-context.mjs, the PagerDuty incident ID, the Pylon issue UUID, andCWE-345. A broader.cursor-plugin/plugin.jsonsearch surfaced unrelatedpr-reviewer-saaswarnings againsthttps://github.com/gbh-tech/ai-plugin/pull/49, so there is no evidence of a backend outage, traffic shift, dependency failure, or traceable production error for this local plugin hook path.The CLI docs for
cr review --agentstate that agent mode writes JSONL events and includes acompleteevent with the final finding count, so the hook can key off structured completion output instead of natural-language output. The new command parser narrows the trusted command shape tocoderabbit|cr review --agentand rejects shell syntax that would let extra commands or comments create misleading input.Confidence: high for blocking the reported spoof path and the persisted false reminder. Residual risk: the hook still relies on Cursor's
postToolUsepayload fields because Cursor does not provide a separately authenticated executable identity in this repository. If Cursor later exposes richer hook metadata, the hook should prefer that over command text.Verification
Result:
GitHub
validatecheck is passing on this PR.Final Prompt
Triage PagerDuty incident
Q30LPOMXDYA13Afrom Pylon issue25004as a production/security alert. Identify service, environment, time window, severity, symptom, customer impact, strongest evidence, likely cause, immediate mitigation, and follow-up steps. Check observability/infrastructure, recent deploys/config/commits, issue tracking, docs, and knowledge sources. If the code fix is clear from evidence, open or use the active PR and ensure the PR description includes evidence, reasoning, and confidence.Final Plan
main.Initiative Context
Q30LPOMXDYA13A/ Pylon issue25004