fix: harden recursive command parsing

[Soumyajit2288]

Summary

• cap recursive command parsing at 32 nested levels
• apply a shared 4 MiB character-work budget across recursive parsing
• propagate a dedicated limit error so validation fails closed
• emit one telemetry event per rejected command instead of one per stack frame
• add coverage for valid nesting, excessive depth, and excessive parser work
• treat CRLF, LF, and CR as shell command separators
• recursively inspect bash process substitutions (<() and >())
• reject ${...} when shell expansion controls the executable name

Root cause

extractCommands() recursively parsed command substitutions and subshells without a depth or total-work limit. User-controlled deeply nested input could therefore consume excessive CPU or exhaust the JavaScript stack.

Returning an empty result at the limit would be unsafe because validateCommand() falls back to checking only the leading base command. This change instead propagates a typed parsing-limit error and rejects the full command.

Impact

Deeply nested or disproportionately expensive command strings are denied without crashing the server or amplifying logs. Normal nested command parsing and existing blocklist behavior remain unchanged.

Fixes #567.
Fixes #556.
Fixes #555.
Fixes #497.

Validation

• npm run build
• node test/test-command-parser-limits.js
• node test/test-blocklist-bypass.js
• node test/test-blocked-commands.js

Summary by CodeRabbit

• Security & Reliability
  • Added stronger parsing safeguards using both depth and remaining-character budgets to prevent unsafe nested command expressions.
  • Command validation now fails closed when parsing limits are exceeded or when dynamic/expandable executables are detected.
  • Improved command splitting to recognize additional newline variants.
• Tests
  • Added async coverage for depth-limit and budget-limit scenarios, including safer extraction/validation outcomes.

[coderabbitai]

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cfebe5e6-85da-428d-a59f-bdf8138bd128

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

extractCommands now bounds recursive parsing by depth and shared character budget, supports newline and process-substitution parsing, and rejects dynamic executables. validateCommand fails closed for parser-limit and unsafe-executable errors, with tests covering these behaviors.

Changes

Command parser safety

Layer / File(s)	Summary
Parser limits and recursive syntax src/command-manager.ts	Adds depth and character-budget limits, propagates them through nested command forms, parses process substitutions, and recognizes newline separators.
Unsafe executable and error handling src/command-manager.ts	Rejects dynamic executable expansions, rethrows unsafe errors, records specialized top-level telemetry, and returns false for unsafe or limit errors during validation.
Parser safety tests test/test-command-parser-limits.js	Tests safe nesting, depth and budget exhaustion, newline and process-substitution parsing, dynamic executable rejection, and async failure reporting.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• wonderwhy-er/DesktopCommanderMCP#323: Overlaps with the recursive substitution and base-command parsing paths in command-manager.ts.

Suggested labels: size:L

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	It also changes newline splitting and dynamic executable rejection, which are broader parser semantics than #567 requires.	Limit this PR to recursion-depth/budget hardening, or split newline and dynamic-command behavior into a separate change with its own rationale.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR adds a recursion depth cap, shared parsing budget, depth propagation, and fail-closed handling required by #567.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: hardening recursive command parsing.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Soumyajit2288]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[Soumyajit2288]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/command-manager.ts (1)

249-259: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Reject leading $ command tokens as unsafe. extractBaseCommand() still skips $FOO before validation, so $FOO echo blocked is treated as echo instead of rejecting the dynamically resolved executable. That leaves the same shell-expansion gap you closed for ${...}.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/command-manager.ts` around lines 249 - 259, Update extractBaseCommand()
so dollar-prefixed tokens representing the command executable are rejected as
unsafe rather than skipped during the initial token scan. Preserve the existing
allowance for $() command substitutions and validation of ${...}, but ensure
inputs such as $FOO echo blocked throw UnsafeDynamicCommandError instead of
selecting echo as the base command.

🧹 Nitpick comments (1)

src/command-manager.ts (1)

275-280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Unreachable guard.

Any token beginning with ${ already throws at Line 252 inside the token loop, before firstToken is assigned, so firstToken can never start with ${ here. This branch is dead code. Safe to drop (or keep the guard only at Line 277 and remove the in-loop one), but avoid duplicating the same check.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/command-manager.ts` around lines 275 - 280, Remove the unreachable `${`
check from the block using firstToken in the command validation flow, since the
token loop already throws UnsafeDynamicCommandError before firstToken is
assigned. Keep the existing validation behavior and avoid duplicating the guard
between the token loop and the firstToken handling.

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

Outside diff comments:
In `@src/command-manager.ts`:
- Around line 249-259: Update extractBaseCommand() so dollar-prefixed tokens
representing the command executable are rejected as unsafe rather than skipped
during the initial token scan. Preserve the existing allowance for $() command
substitutions and validation of ${...}, but ensure inputs such as $FOO echo
blocked throw UnsafeDynamicCommandError instead of selecting echo as the base
command.

---

Nitpick comments:
In `@src/command-manager.ts`:
- Around line 275-280: Remove the unreachable `${` check from the block using
firstToken in the command validation flow, since the token loop already throws
UnsafeDynamicCommandError before firstToken is assigned. Keep the existing
validation behavior and avoid duplicating the guard between the token loop and
the firstToken handling.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8f3c0e86-78dc-4ea5-9c1f-311cea67ebb8

📥 Commits

Reviewing files that changed from the base of the PR and between bbef66c and 6b4ee0e.

📒 Files selected for processing (2)

• src/command-manager.ts
• test/test-command-parser-limits.js