Skip to content

Harden Cursor post-review hook validation #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
coderabbitai wants to merge 1 commit into
base: main
Choose a base branch
from
+339 −14
Open

Harden Cursor post-review hook validation #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 159 additions & 13 deletions hooks/post-review-context.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { appendFileSync, existsSync, readFileSync, unlinkSync, writeFileSync } from "node:fs";
import { appendFileSync, existsSync, mkdirSync, readFileSync, unlinkSync, writeFileSync } from "node:fs";
import { createHash } from "node:crypto";
import os from "node:os";
import path from "node:path";
import process from "node:process";
Expand Down Expand Up @@ -34,19 +35,158 @@ function isCodeRabbitReviewCommand(command) {
if (typeof command !== "string") {
return false;
}
return /coderabbit(\.exe)?(\s|.*\s)review(\s|$)/.test(command) && !/autofix/.test(command);

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")
);
}
Comment on lines +38 to 51

@coderabbitai coderabbitai Bot Jul 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🩺 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.

Suggested change
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.


function looksClean(toolOutput) {
if (typeof toolOutput !== "string") {
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;
}
Comment on lines +63 to +73

@coderabbitai coderabbitai Bot Jul 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🎯 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.mjs

Repository: 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 -S

Repository: 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 -S

Repository: 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));
}
JS

Repository: 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.


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;
}

function getExitCode(input) {
const candidates = [
input?.exit_code,
input?.exitCode,
input?.tool_exit_code,
input?.tool_result?.exit_code,
input?.tool_result?.exitCode,
input?.tool_response?.exit_code,
input?.tool_response?.exitCode,
];

return candidates.find((candidate) => Number.isInteger(candidate));
}

function toolSucceeded(input) {
const exitCode = getExitCode(input);
if (exitCode !== undefined && exitCode !== 0) {
return false;
}
return /(raised|found|reported)\s+0\s+issues|"issues"\s*:\s*\[\s*\]|no issues found/i.test(toolOutput);

return !(input?.tool_error || input?.error);
}

function reviewOutcome(toolOutput) {
if (typeof toolOutput !== "string") {
return null;
}

let completeEvent = null;

for (const line of toolOutput.split(/\r?\n/)) {
const trimmed = line.trim();
if (!trimmed) {
continue;
}

let event;
try {
event = JSON.parse(trimmed);
} catch {
return null;
}

if (event?.type === "error") {
return null;
}

if (event?.type === "complete") {
completeEvent = event;
}
}

if (!completeEvent || !Number.isInteger(completeEvent.findings)) {
return null;
}

return completeEvent.findings === 0 ? "clean" : "issues";
}

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`);
Comment on lines 182 to +189

@coderabbitai coderabbitai Bot Jul 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🩺 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.mjs

Repository: 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.

}

function emit(context) {
Expand All @@ -67,20 +207,24 @@ const toolName = input?.tool_name ?? "unknown";
const command = input?.tool_input?.command;
const snippet = typeof command === "string" ? command.slice(0, 100) : "";

if (isCodeRabbitReviewCommand(command)) {
const clean = looksClean(input?.tool_output);
log(`review-complete tool=${toolName} clean=${clean} cmd=${snippet}`);
if (isCodeRabbitReviewCommand(command) && toolSucceeded(input)) {
const outcome = reviewOutcome(input?.tool_output);
log(`review-complete tool=${toolName} outcome=${outcome ?? "unknown"} cmd=${snippet}`);

if (clean) {
if (outcome === "clean") {
try {
writeFileSync(statePath(input), JSON.stringify({ expires: Date.now() + REMINDER_WINDOW_MS, remaining: REMINDER_MAX }));
writeFileSync(
statePath(input),
JSON.stringify({ expires: Date.now() + REMINDER_WINDOW_MS, remaining: REMINDER_MAX }),
{ mode: 0o600 },
);
} catch {
// State is best-effort; the primary injection below still happens.
}
emit(
"The CodeRabbit review for this request is complete and came back clean, meaning the changes passed review. Present a clean-result summary: what was reviewed (files changed, lines, scope), what it was checked for (bugs, security issues, code quality risks), confirmation that the changes passed, and suggested next steps such as running tests, committing, or opening a PR. Then finish the response there; the review request is fulfilled, so a second AI or manual pass over the same diff is not needed. Linters, type checkers, and tests remain available whenever the project workflow calls for them.",
);
} else {
} else if (outcome === "issues") {
try {
unlinkSync(statePath(input));
} catch {
Expand All @@ -89,6 +233,8 @@ if (isCodeRabbitReviewCommand(command)) {
emit(
"The CodeRabbit review for this request is complete. Present the parsed results grouped by severity and finish the response there; the review request is fulfilled. A second AI or manual pass over the same diff is not needed. Linters, type checkers, and tests remain available whenever the project workflow calls for them.",
);
} else {
log(`pass unrecognized-agent-output tool=${toolName} cmd=${snippet}`);
}
process.exit(0);
}
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"license": "MIT",
"type": "module",
"scripts": {
"test": "node scripts/validate-plugin.mjs",
"test": "node scripts/test-post-review-context.mjs && node scripts/validate-plugin.mjs",
"validate": "node scripts/validate-plugin.mjs"
},
"engines": {
Expand Down
Loading
Loading