fix: use git root for session tracking to prevent session loss#339
fix: use git root for session tracking to prevent session loss#339lukaskraic wants to merge 10 commits intositeboon:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChat, shell, and Git handlers now resolve the repository root via Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server
participant GitCLI as Git CLI
participant SessionStore as Session Store
participant PTY as PTY Process
Client->>Server: Request (chat / shell / git) with projectPath, sessionId
Server->>GitCLI: git rev-parse --show-toplevel (cwd=projectPath)
GitCLI-->>Server: gitRoot or error
alt gitRoot returned
Server->>SessionStore: sanitize sessionId, derive pty/session key using gitRoot
Server->>GitCLI: run requested Git command (cwd=gitRoot)
GitCLI-->>Server: command result
alt shell resume flow
Server->>PTY: attempt resume from resumePath (gitRoot or fallback)
PTY-->>Server: resumed or failed
alt resumed
Server->>Client: replay buffered output
Server->>Client: send CSI 999999 H (scroll-to-bottom)
else failed
Server->>PTY: start new shell (cwd fallback)
end
end
Server-->>Client: Response contextualized to gitRoot
else error
Server-->>Client: Validation/fallback error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/routes/git.js (2)
61-76: FixvalidateGitRepository:gitRootis used before assignment.
{ cwd: gitRoot }throws aReferenceError, so validation always fails. UseprojectPathas the cwd, then normalize the returned root.🐛 Proposed fix
- const { stdout: gitRoot } = await execAsync('git rev-parse --show-toplevel', { cwd: gitRoot }); - const normalizedGitRoot = path.resolve(gitRoot.trim()); + const { stdout } = await execAsync('git rev-parse --show-toplevel', { cwd: projectPath }); + const normalizedGitRoot = path.resolve(stdout.trim());
406-411: DefinegitRootbefore use in these routes.
checkout,create-branch,commits,commit-diff, andgenerate-commit-messagecallexecAsync(..., { cwd: gitRoot })without defininggitRoot, which will crash at runtime. Addconst gitRoot = await validateGitRepository(projectPath);in each handler.🐛 Example fix (apply similarly to the other handlers)
try { const projectPath = await getActualProjectPath(project); + const gitRoot = await validateGitRepository(projectPath); // Checkout the branch const { stdout } = await execAsync(`git checkout "${branch}"`, { cwd: gitRoot });Also applies to: 427-432, 448-477, 499-506, 528-538
🤖 Fix all issues with AI agents
In `@server/index.js`:
- Around line 49-67: The getGitRoot function uses execSync with string
interpolation which allows shell injection via projectPath; replace this with
execFileSync (or spawnSync) calling git directly with an args array (e.g.,
['-C', projectPath, 'rev-parse', '--show-toplevel']) and { encoding: 'utf8' } to
avoid shell interpretation, preserve the try/catch behavior, trim and return
gitRoot when present, and keep the console.log debug message; update references
to execSync to execFileSync and ensure projectPath is validated (non-empty)
before use.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routes/git.js (1)
170-191: UsegitRootfor filesystem paths that originate from git commands.
filevalues come from git commands executed atgitRoot, making them relative to the git root. Joining them withprojectPath(which may be a subdirectory) breaks path resolution when the project directory is not the repository root. Switch all filesystem operations at the mentioned locations to usegitRootinstead ofprojectPath.Affected locations
- Lines 170–191: untracked file diff
- Lines 249–258: tracked file content read
- Lines 557–563: generate-commit-message file read
- Lines 1069–1076: discard-untracked delete
- Lines 1118–1127: delete-untracked delete
🤖 Fix all issues with AI agents
In `@server/routes/git.js`:
- Around line 341-348: The git command usage in the staging/commit and branch
operations uses execAsync with interpolated strings (see the loop over files
using `file`, the `git commit` using `message`, and branch operations using
`branch`) which permits command injection; replace these calls to
shell-interpolated exec with a safe execFile/spawn variant that accepts an
argument array (e.g., create or use an `execFileAsync` wrapper and call
execFileAsync('git', ['add', file'], {cwd: gitRoot}) for adds,
execFileAsync('git', ['commit', '-m', message], {cwd: gitRoot}) for commits, and
execFileAsync('git', ['checkout', '-b', branch], ...) or similar for branch
operations), ensuring you stop building command strings and pass user-controlled
variables as separate arguments to the execFile call (update all uses of
`execAsync` that pass `file`, `message`, or `branch`).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/index.js`:
- Around line 1063-1075: The shellCommand construction interpolates unsanitized
sessionId into shell strings (see variables shellCommand, resumePath,
hasSession, sessionId, projectPath, shellGitRoot) causing command-injection
risk; create a sanitized safeSessionId (use the same safe pattern used elsewhere
in the repo to either validate/whitelist or properly escape session IDs) and
replace all direct uses of sessionId in the commands that build shellCommand
(both Windows PowerShell branch and non-Windows branch, including the cursor
provider command sites referenced) with safeSessionId so only safe characters
are allowed/escaped before embedding into the `claude --resume ...` invocation.
🧹 Nitpick comments (1)
server/index.js (1)
49-72: Function defined before its dependency is imported.
getGitRootusesexecFileSync(line 59), but the import statement forexecFileSyncis at line 80, after the function definition. In ES modules, imports are hoisted, so this will work correctly. However, this ordering is unconventional and may confuse readers.Consider moving the function definition after the imports for better code organization and clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/index.js`:
- Around line 952-955: The sanitized sessionId handling currently allows an
empty string to pass through, which can cause session collisions; update the
logic around rawSessionId/sessionId (and where hasSession is used) to explicitly
reject requests when rawSessionId was provided but
String(rawSessionId).replace(/[^a-zA-Z0-9._-]/g, '') yields an empty
string—e.g., send a validation error/HTTP 400 or close the connection and return
early instead of falling back to a default session key; ensure the check uses
the same sanitization expression so you only reject when the original input
existed but sanitization removed all characters.
🧹 Nitpick comments (1)
server/index.js (1)
49-72: Add logging when Git is missing to aid deployment debugging.Lines 56-72 silently fall back when git isn't available, which can mask deployment issues where the git binary is absent. The function will return
projectPathinstead of the git root, causing sessions to be keyed differently than intended. Add a warning for ENOENT to make this obvious.♻️ Suggested improvement
function getGitRoot(projectPath) { if (!projectPath) return projectPath; try { const gitRoot = execFileSync( 'git', ['-C', projectPath, 'rev-parse', '--show-toplevel'], { encoding: 'utf8' } ).trim(); if (gitRoot) { console.log('🔧 Git root detected:', gitRoot, 'from:', projectPath); return gitRoot; } } catch (e) { - // Not a git repository, use the original path + // Not a git repository, use the original path + if (e?.code === 'ENOENT') { + console.warn('[WARN] git not found; falling back to projectPath for session tracking'); + } } return projectPath; }
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routes/git.js (1)
446-460:⚠️ Potential issue | 🔴 CriticalUnvalidated
limitparameter enables command injection.The
limitquery parameter is interpolated directly into the shell command without validation. An attacker can inject shell commands:GET /commits?project=foo&limit=10;rm%20-rf%20/🔒 Recommended fix
- const { project, limit = 10 } = req.query; + const { project, limit: limitParam = '10' } = req.query; + const limit = Math.min(Math.max(1, parseInt(limitParam, 10) || 10), 100); ... - const { stdout } = await execAsync( - `git log --pretty=format:'%H|%an|%ae|%ad|%s' --date=relative -n ${limit}`, + const { stdout } = await execFileAsync( + 'git', + ['log', '--pretty=format:%H|%an|%ae|%ad|%s', '--date=relative', '-n', String(limit)], { cwd: gitRoot } );
🤖 Fix all issues with AI agents
In `@server/routes/git.js`:
- Around line 180-182: The code joins user-supplied file with gitRoot into
filePath without validation, allowing path traversal; fix by normalizing and
resolving the resulting path and rejecting any paths that escape gitRoot (e.g.,
compute resolved = path.resolve(gitRoot, file) or use path.relative and ensure
the result does not start with '..' or escape separators), then only proceed
with fs.stat / read / write using the validated filePath; apply this check to
all places that build filePath (references: the filePath variable and gitRoot
usage in server/routes/git.js, including the other occurrences you noted) and
return a 400/403 error when validation fails.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routes/git.js (1)
1-12:⚠️ Potential issue | 🔴 CriticalSummary: Security foundations are good, but command injection fixes are incomplete.
The PR adds solid security infrastructure:
- ✅
validateFilePathfor path traversal protection- ✅
execFileAsyncfor safe command execution- ✅ Limit parameter sanitization in
/commits- ✅ Git root resolution for session persistence
However,
execFileAsyncis only used forgit log. All other git commands (~15+ locations) still useexecAsyncwith string interpolation, leaving command injection vulnerabilities. The past review marked these as "Addressed," but the current code shows they are not.Consider a systematic pass to convert all
execAsynccalls that include user input (file,message,branch,commit) to useexecFileAsyncwith argument arrays.
🤖 Fix all issues with AI agents
In `@server/routes/git.js`:
- Line 1089: The discard endpoint is vulnerable because the user-controlled file
variable is directly interpolated into shell commands passed to execAsync (see
execAsync calls using `git status --porcelain "${file}"`, and the similar git
commands at the other two sites), so replace these shell-interpolated calls with
a safe child_process invocation that passes the file as a distinct argument
(e.g., use execFile/spawn or execAsync with an args array) or explicitly
sanitize/escape the file name (reject inputs with shell metacharacters or
path-traversal) before calling git; ensure validation logic around `file` and
`gitRoot` (the existing path checks) is reused and applied to all three
execAsync invocations in the discard endpoint to avoid command injection.
- Line 258: The git file-with-diff handler is vulnerable because user-controlled
file is interpolated into execAsync commands; replace those execAsync calls with
execFileAsync (or spawn/child_process.execFile) and pass the git command and
file as separate args instead of a shell-interpolated string—specifically update
the calls that produce statusOutput (currently using execAsync(`git status
--porcelain "${file}"`, { cwd: gitRoot })), the call that produces the diff
output, and the call that produces blame/other outputs to use
execFileAsync('git', ['status','--porcelain', file], { cwd: gitRoot }) (and
analogous arg arrays for diff/blame) so the file parameter is not interpreted by
the shell; ensure error handling and cwd usage remain the same.
- Line 194: The git commands interpolate the untrusted req.query.file into shell
strings via execAsync (e.g., the call that runs `git status --porcelain
"${file}"` and similar calls later), which allows shell injection; change these
to invoke git without a shell by using a spawn/execFile-style API that accepts
args arrays (e.g., replace execAsync with a promisified child_process.execFile
or child_process.spawn with args: ['status','--porcelain', file], and similarly
for the other git invocations referenced in the diff), keep using
validateFilePath for filesystem checks but stop string-interpolating file into
shell commands so the file value is passed as an argument not parsed by a shell.
- Line 436: The checkout/create-branch code is vulnerable because branch is
interpolated into a shell string passed to execAsync; replace this with a safe
execFile/child_process spawn call that passes the command and branch as separate
arguments (e.g., call execFile('git', ['checkout', branch], { cwd: gitRoot })
via the same promisified helper) and/or validate/sanitize the branch parameter
before use (allow only an explicit safe regex like alphanumerics, dot, dash,
slash and reject names containing whitespace, shell metacharacters, or '..' and
leading '-' ). Update both occurrences that call execAsync with inline templates
(the one using `git checkout "${branch}"` and the other at the later occurrence)
to use the argument-based exec and add the branch validation logic where the
request body branch is read.
- Line 1040: The publish endpoint is vulnerable because the branch value from
req.body is interpolated into a shell string passed to execAsync (see execAsync
call using `git push --set-upstream ${remoteName} ${branch}`); fix by
validating/sanitizing inputs (ensure branch and remoteName match a strict
allowlist/regex like /^[A-Za-z0-9._/-]+$/) and avoid shell interpolation by
calling Git with a non-shell exec variant (use
child_process.execFile/execFileSync or spawn with args: e.g., run git with args
['push','--set-upstream', remoteName, branch'] and cwd set to gitRoot) so
user-controlled branch cannot inject commands. Ensure both branch and remoteName
are validated before calling execFile/spawn.
- Around line 568-570: The git diff invocation interpolates the user-controlled
variable file into a shell string, enabling command injection; replace the
string-based execAsync call with a safe execFile/spawn variant that passes
arguments separately (e.g., call execFileAsync or spawn('git',
['diff','HEAD','--', file], {cwd: gitRoot}) or use a library like execa to pass
args) so the file name is never interpreted by a shell, and keep/ensure
validateFilePath is applied before calling execFile/spawn; update the call site
where execAsync is used for the generate-commit-message flow (the execAsync call
around `git diff HEAD -- "${file}"`) to use this arguments-based API.
🧹 Nitpick comments (1)
server/routes/git.js (1)
505-507: UseexecFileAsyncfor consistency in commit stats lookup.While
commit.hashcomes from git output rather than direct user input, usingexecFileAsynchere maintains consistency with the security pattern used for the main log command above.♻️ Suggested refactor
try { - const { stdout: stats } = await execAsync( - `git show --stat --format='' ${commit.hash}`, - { cwd: gitRoot } - ); + const { stdout: stats } = await execFileAsync( + 'git', + ['show', '--stat', '--format=', commit.hash], + { cwd: gitRoot } + ); commit.stats = stats.trim().split('\n').pop(); // Get the summary line
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routes/git.js (1)
194-224:⚠️ Potential issue | 🟠 MajorAdd
--before file pathspecs to prevent option injection.Several git commands pass user-supplied
fileas a pathspec without--. A filename starting with-will be interpreted as an option (e.g.,--all), potentially altering behavior (stage/restore more files than intended). Add--forstatus(lines 194, 258, 1093, 1141),add(line 368),restore(line 1115), andreset(line 1118).🛠️ Suggested fix pattern
- const { stdout: statusOutput } = await execFileAsync('git', ['status', '--porcelain', file], { cwd: gitRoot }); + const { stdout: statusOutput } = await execFileAsync('git', ['status', '--porcelain', '--', file], { cwd: gitRoot });Apply similarly to
add,restore, andresetcommands.
🤖 Fix all issues with AI agents
In `@server/routes/git.js`:
- Around line 22-31: The current validateFilePath function fails when the
repository root is filesystem root because resolvedRoot + path.sep can produce
'//' and reject valid paths; replace the startsWith check with a containment
check using path.relative: compute const rel = path.relative(resolvedRoot,
resolvedPath) and throw an Error if rel === ''? no — instead throw if
rel.startsWith('..') or path.isAbsolute(rel); otherwise return resolvedPath.
Update the logic inside validateFilePath to use this path.relative-based
containment check to correctly handle root-path and other edge cases.
lukaskraic
force-pushed
the
fix/git-root-session-persistence
branch
from
805df58 to
c7b80c2
Compare
|
Hey @lukaskraic , thanks for the PR! Can you resolve the merge conflicts? |
When Claude changes directories during work (e.g., cd into subdirectory), sessions were being lost because: 1. Chat sessions used the current working directory (cwd) as identifier 2. Terminal sessions used projectPath for session key 3. Source Control operations failed in subdirectories This fix detects the git repository root and uses it consistently: - Chat tab: Uses git root for cwd option in Claude SDK calls - Terminal tab: Uses git root for PTY session key and resume commands - Source Control tab: validateGitRepository now returns git root instead of throwing error, allowing all git operations from subdirectories Fixes session loss when working in monorepos or when Claude navigates to different directories within a project.
- Fix validateGitRepository to use projectPath instead of undefined gitRoot - Add missing gitRoot definition to checkout, create-branch, commits, commit-diff, and generate-commit-message routes - Rename stdout destructuring to avoid variable shadowing
Use execFileSync with argument array instead of execSync with string interpolation to prevent potential command injection via projectPath.
- Add sessionId sanitization to prevent command injection - Scroll terminal to bottom after buffer replay on reconnect
- Reject and close connection if sessionId contains only invalid chars - Add warning log when git binary is not found (ENOENT)
File paths returned by git commands (status --porcelain, etc.) are relative to the git repository root, not the current projectPath. When Claude changes directories during work, projectPath may be a subdirectory, causing file operations to fail. This fixes file read/write/delete operations in: - /diff endpoint (untracked file content) - /file-with-diff endpoint (current file content) - /generate-commit-message endpoint (untracked file content) - /discard endpoint (delete untracked files) - /delete-untracked endpoint (delete untracked files)
- Add validateFilePath() helper to prevent path traversal attacks by ensuring file paths stay within the git repository root - Fix /commits endpoint: validate and sanitize limit parameter, use execFileAsync instead of string interpolation to prevent command injection (e.g., limit=10;rm -rf /) - Apply path validation to all endpoints that read/write files: /diff, /file-with-diff, /generate-commit-message, /discard, /delete-untracked Security issues identified by CodeRabbit review.
…ection prevention Convert all git command executions from shell-interpolated execAsync to argument-based execFileAsync to prevent command injection attacks. Affected endpoints: - /diff: git status, git show, git diff - /file-with-diff: git status, git show - /commit: git add, git commit - /checkout: git checkout - /create-branch: git checkout -b - /commits: git show --stat - /commit-diff: git show - /generate-commit-message: git diff - /remote-status: git rev-parse, git remote, git rev-list - /fetch: git rev-parse, git fetch - /pull: git rev-parse, git pull - /push: git rev-parse, git push - /publish: git rev-parse, git remote, git push - /discard: git status, git restore, git reset - /delete-untracked: git status Using execFileAsync with argument arrays prevents shell interpretation of user-controlled values like file paths, branch names, and commit messages.
- Add '--' before file pathspecs in git commands to prevent option injection (e.g., filename starting with '-' being interpreted as option) Affected commands: status, add, restore, reset - Improve validateFilePath to use path.relative() for robust containment check that correctly handles edge cases like filesystem root as repo root
lukaskraic
force-pushed
the
fix/git-root-session-persistence
branch
from
c7b80c2 to
5bb5c79
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
server/routes/git.js (2)
585-589: Verify that unsanitizedcommithash from query params is safe withexecFileAsync.The
commitvariable (fromreq.query.commit, line 574) is passed directly togit showviaexecFileAsync. SinceexecFileAsyncdoesn't use a shell, there's no command injection risk. However, arbitrary strings passed togit showcould cause unexpected behavior (e.g.,commit=--help). Consider validating the format.♻️ Optional: validate commit hash format
+ // Validate commit hash format (hex string, 4-40 chars) + if (!/^[0-9a-fA-F]{4,40}$/.test(commit)) { + return res.status(400).json({ error: 'Invalid commit hash format' }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/git.js` around lines 585 - 589, Validate and sanitize the commit query value before calling execFileAsync: ensure the commit variable (from req.query.commit) is either a valid hex commit-ish (e.g. match /^[0-9a-fA-F]{7,40}$/) or at minimum does not start with '-' (to prevent git interpreting it as a flag); if validation fails, return a 400 error and do not call execFileAsync('git', ['show', commit], ...). Keep using execFileAsync but perform this check in the same request handler where commit is read so only validated values reach the execFileAsync call.
369-379:initial-commitstill usesexecAsync(shell-based) — inconsistent but safe.Lines 369, 376, and 379 use
execAsyncwith hardcoded command strings (git rev-parse HEAD,git add .,git commit -m "Initial commit"). No user input is interpolated so there's no injection risk, but it's inconsistent with theexecFileAsyncpattern applied everywhere else.♻️ Optional: align with execFileAsync pattern
- await execAsync('git rev-parse HEAD', { cwd: gitRoot }); + await execFileAsync('git', ['rev-parse', 'HEAD'], { cwd: gitRoot }); ... - await execAsync('git add .', { cwd: gitRoot }); + await execFileAsync('git', ['add', '.'], { cwd: gitRoot }); ... - const { stdout } = await execAsync('git commit -m "Initial commit"', { cwd: gitRoot }); + const { stdout } = await execFileAsync('git', ['commit', '-m', 'Initial commit'], { cwd: gitRoot });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/git.js` around lines 369 - 379, The code in initial commit flow uses execAsync with literal shell strings (execAsync('git rev-parse HEAD'...), execAsync('git add .'...), execAsync('git commit -m "Initial commit"') ) which is inconsistent with the execFileAsync pattern used elsewhere; change these to execFileAsync calls invoking the git binary with argument arrays (e.g., execFileAsync('git', ['rev-parse','HEAD'], { cwd: gitRoot }), execFileAsync('git', ['add','.'], { cwd: gitRoot }), execFileAsync('git', ['commit','-m','Initial commit'], { cwd: gitRoot })) and preserve existing try/catch and the use of the returned stdout where applicable to keep behavior identical.server/index.js (1)
34-60:getGitRootblocks the event loop with synchronous I/O.
execFileSyncis called on every chat message (claude-command) and every shellinit, which blocks the Node.js event loop. If the git process is slow (large repo, network filesystem, or missing git binary timing out), this stalls all other connections. Consider using an async variant (execFilewithpromisifyorexecFileAsync) consistent withvalidateGitRepositoryingit.js.♻️ Suggested async refactor
+import { spawn, execFileSync, execFile } from 'child_process'; +import { promisify } from 'util'; +const execFileAsync = promisify(execFile); + -function getGitRoot(projectPath) { +async function getGitRoot(projectPath) { if (!projectPath) return projectPath; try { - const gitRoot = execFileSync( + const { stdout } = await execFileAsync( 'git', ['-C', projectPath, 'rev-parse', '--show-toplevel'], { encoding: 'utf8' } - ).trim(); + ); + const gitRoot = stdout.trim(); if (gitRoot) { console.log('🔧 Git root detected:', gitRoot, 'from:', projectPath); return gitRoot; } } catch (e) { if (e?.code === 'ENOENT') { console.warn('[WARN] git not found; falling back to projectPath for session tracking'); } } return projectPath; }Then update call sites to
await getGitRoot(...)(they're already in async contexts at lines 968 and 1133).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/index.js` around lines 34 - 60, getGitRoot currently uses blocking execFileSync; change it to an async function (async function getGitRoot(projectPath)) that uses a non-blocking child process call (e.g., util.promisify(execFile) or child_process.execFile wrapped in a Promise) to run git -C <projectPath> rev-parse --show-toplevel, returning the trimmed git root string on success or the original projectPath on any error (preserving the ENOENT warning behavior). Update the function signature and internal error handling to mirror the existing behavior (log detected git root, warn on missing git) and then update call sites that invoke getGitRoot (e.g., where getGitRoot(...) is called at the async contexts mentioned) to await getGitRoot(...) so the non-blocking version is properly awaited.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/index.js`:
- Around line 1231-1243: The shell command construction using resumePath and
projectPath when building shellCommand (in the hasSession/sessionId branches for
both Windows and non-Windows) is vulnerable to shell injection; update the
implementation to avoid interpolating raw paths into a single shell string:
either (preferred) call the claude command with argument arrays (e.g., use
child_process.spawn or execFile with args) so you pass resumePath/projectPath as
safe arguments, or (if keeping shell strings) tightly sanitize/escape resumePath
and projectPath (e.g., reject/control characters like ", `, $, \\ and newlines
and wrap safely) and replace the interpolated usage of resumePath/projectPath in
shellCommand with the safe variables (safeResumePath/safeProjectPath) used in
both the Windows (Set-Location ...; claude ...) and POSIX (cd ... && claude ||
(cd ... && claude)) branches to eliminate injection risk.
---
Nitpick comments:
In `@server/index.js`:
- Around line 34-60: getGitRoot currently uses blocking execFileSync; change it
to an async function (async function getGitRoot(projectPath)) that uses a
non-blocking child process call (e.g., util.promisify(execFile) or
child_process.execFile wrapped in a Promise) to run git -C <projectPath>
rev-parse --show-toplevel, returning the trimmed git root string on success or
the original projectPath on any error (preserving the ENOENT warning behavior).
Update the function signature and internal error handling to mirror the existing
behavior (log detected git root, warn on missing git) and then update call sites
that invoke getGitRoot (e.g., where getGitRoot(...) is called at the async
contexts mentioned) to await getGitRoot(...) so the non-blocking version is
properly awaited.
In `@server/routes/git.js`:
- Around line 585-589: Validate and sanitize the commit query value before
calling execFileAsync: ensure the commit variable (from req.query.commit) is
either a valid hex commit-ish (e.g. match /^[0-9a-fA-F]{7,40}$/) or at minimum
does not start with '-' (to prevent git interpreting it as a flag); if
validation fails, return a 400 error and do not call execFileAsync('git',
['show', commit], ...). Keep using execFileAsync but perform this check in the
same request handler where commit is read so only validated values reach the
execFileAsync call.
- Around line 369-379: The code in initial commit flow uses execAsync with
literal shell strings (execAsync('git rev-parse HEAD'...), execAsync('git add
.'...), execAsync('git commit -m "Initial commit"') ) which is inconsistent with
the execFileAsync pattern used elsewhere; change these to execFileAsync calls
invoking the git binary with argument arrays (e.g., execFileAsync('git',
['rev-parse','HEAD'], { cwd: gitRoot }), execFileAsync('git', ['add','.'], {
cwd: gitRoot }), execFileAsync('git', ['commit','-m','Initial commit'], { cwd:
gitRoot })) and preserve existing try/catch and the use of the returned stdout
where applicable to keep behavior identical.
server/index.js
Outdated
| // Use git root for resume to ensure session is found even if cwd changed | ||
| const resumePath = shellGitRoot || projectPath; | ||
| if (os.platform() === 'win32') { | ||
| if (hasSession && sessionId) { | ||
| // Try to resume session, but with fallback to new session if it fails | ||
| shellCommand = `Set-Location -Path "${projectPath}"; claude --resume ${sessionId}; if ($LASTEXITCODE -ne 0) { claude }`; | ||
| // Try to resume session from git root, fallback to new session in projectPath | ||
| shellCommand = `Set-Location -Path "${resumePath}"; claude --resume ${sessionId}; if ($LASTEXITCODE -ne 0) { Set-Location -Path "${projectPath}"; claude }`; | ||
| } else { | ||
| shellCommand = `Set-Location -Path "${projectPath}"; ${command}`; | ||
| } | ||
| } else { | ||
| if (hasSession && sessionId) { | ||
| shellCommand = `cd "${projectPath}" && claude --resume ${sessionId} || claude`; | ||
| // Resume from git root, fallback to new session in projectPath if resume fails | ||
| shellCommand = `cd "${resumePath}" && claude --resume ${sessionId} || (cd "${projectPath}" && claude)`; |
There was a problem hiding this comment.
Shell command injection risk via resumePath in PTY commands.
resumePath (derived from git output) and projectPath (from WebSocket input) are interpolated into shell command strings with only double-quote wrapping. A projectPath containing ", `, $(), or \ can escape the quotes and inject commands. While resumePath from git rev-parse output is generally safe, the projectPath fallback paths at lines 1236 and 1243 pass user-controlled input through a shell.
This is a pre-existing pattern in the file, but the new fallback logic at lines 1236 and 1243 duplicates and extends it. Consider using execFileAsync/argument arrays for the git-root detection part, or at minimum sanitizing projectPath before interpolation.
🔒 Minimal mitigation for projectPath
+// Sanitize path for shell interpolation - reject paths with shell metacharacters
+function sanitizePathForShell(p) {
+ if (/[`$"\\!]/.test(p)) {
+ throw new Error('Invalid characters in project path');
+ }
+ return p;
+}
+
// In the init handler, before building shellCommand:
+const safeProjectPath = sanitizePathForShell(projectPath);
+const safeResumePath = sanitizePathForShell(resumePath);Then use safeProjectPath / safeResumePath in shell command strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/index.js` around lines 1231 - 1243, The shell command construction
using resumePath and projectPath when building shellCommand (in the
hasSession/sessionId branches for both Windows and non-Windows) is vulnerable to
shell injection; update the implementation to avoid interpolating raw paths into
a single shell string: either (preferred) call the claude command with argument
arrays (e.g., use child_process.spawn or execFile with args) so you pass
resumePath/projectPath as safe arguments, or (if keeping shell strings) tightly
sanitize/escape resumePath and projectPath (e.g., reject/control characters like
", `, $, \\ and newlines and wrap safely) and replace the interpolated usage of
resumePath/projectPath in shellCommand with the safe variables
(safeResumePath/safeProjectPath) used in both the Windows (Set-Location ...;
claude ...) and POSIX (cd ... && claude || (cd ... && claude)) branches to
eliminate injection risk.
- Convert getGitRoot from sync (execFileSync) to async to avoid blocking the event loop on every chat/shell connection - Add sanitizePathForShell() to reject shell metacharacters in paths before interpolation into PTY command strings - Validate commit hash format in /commit-diff endpoint - Convert /initial-commit from execAsync to execFileAsync for consistency with the rest of the codebase
2 participants
Summary
When Claude changes directories during work (e.g.,
cdinto a subdirectory), sessions were being lost because the UI tracked sessions by the current working directory rather than the git repository root.This fix detects the git repository root and uses it consistently across all components:
cwdoption in Claude SDK callsvalidateGitRepositorynow returns git root instead of throwing an error, allowing all git operations from subdirectoriesProblem
/home/user/myproject/home/user/myproject/src/componentsduring workSolution
Added
getGitRoot()helper function that detects the git repository root usinggit rev-parse --show-toplevel. This root is then used for:Test plan
Files changed
server/index.js- AddedgetGitRoot()helper, patched chat and terminal handlersserver/routes/git.js- ModifiedvalidateGitRepository()to return git rootSummary by CodeRabbit
New Features
Bug Fixes
Improvements