#30: call python3 with execFileSync to block shell injection in path

[kreinba]

The MCP find_the_most_critical_design_issue tool passes the user-supplied path straight into /bin/bash -c "...${path}..." via execSync in src/aibolit.ts, so any value containing shell metacharacters is parsed by the shell instead of being treated as a filename. A crafted path such as foo.java; touch /tmp/pwned runs both halves on the host.

The shell wrapper is now gone. The call uses execFileSync('python3', ['-m', 'aibolit', 'check', '--full', '--filenames', path]) so Node hands path to python3 as a single argv element and the shell is never involved. The previous || true tolerance is preserved by catching the non-zero-exit error and reading whatever stdout the failing call already produced.

Ran make test (28 tests pass, branch coverage stays at 100%), make lint, npx tsc --target es2020 --module nodenext, and npx . --version / npx . --help locally. A new regression test asserts execFileSync is called with the exact argv shape so a path with ; cannot reach a shell again.

Fixes #30

Summary by CodeRabbit

• Bug Fixes
  • Improved error handling and recovery in code analysis to ensure checks complete successfully even when unexpected errors occur.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR fixes a shell injection vulnerability by replacing shell-based execSync execution with direct execFileSync argument passing. The aibolit function now passes file paths as separate argv elements instead of interpolating them into a shell command, and error handling moves from shell's || true to try/catch with stdout extraction.

Changes

Shell injection fix via execFileSync

Layer / File(s)	Summary
Shell-safe execution via execFileSync src/aibolit.ts	Import execFileSync alongside execSync. Replace shell-interpolated execSync invocation with direct execFileSync('python3', ['-m','aibolit','check','--full','--filenames', path]) call, wrapped in try/catch to extract stdout from thrown errors or default to empty string for downstream parsing.
Test coverage for execFileSync execution test/aibolit.test.ts	Update Jest mock setup to provide both execFileSync and execSync with typed helpers. Existing test cases ("accepts perfect code", "detects simple issue", version checks) now set execFileSync mock return values. Add comprehensive warning parsing and error handling tests: verify stdout parsing on non-zero exit, fallback to "perfect" when stdout is unavailable, and assert exact execFileSync invocation with file path as separate argv element.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A rabbit hops through shells no more, 🐰
Where argv arrays guard the door,
No metacharacters slip through now,
Direct invocation takes the bow! 🏹

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically summarizes the main change: replacing shell-based execSync with execFileSync to prevent shell injection via an unescaped path parameter.
Linked Issues check	✅ Passed	The code changes fully implement the requirements from issue #30: replaces the vulnerable shell-invoking execSync with execFileSync, passes the path as a separate argv element, preserves non-zero exit handling via try/catch, and adds regression tests.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to addressing the shell injection vulnerability in issue #30: modifications to src/aibolit.ts for the fix and updates to test/aibolit.test.ts for regression testing.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/aibolit.ts (1)

4-4: 💤 Low value

Consider using execFileSync for check_version() too.

For consistency with the security fix, check_version() could also avoid the shell wrapper. The current code is safe since there's no user input, but using execFileSync throughout would make the shell-avoidance pattern explicit.

♻️ Suggested refactor

-import { execFileSync, execSync } from 'child_process';
+import { execFileSync } from 'child_process';

 function check_version(): void {
-  const stdout = execSync(`/bin/bash -c "python3 -m aibolit --version"`).toString();
+  const stdout = execFileSync('python3', ['-m', 'aibolit', '--version']).toString();
   const match = stdout.match(/^[^ ]+ (\d+\.\d+\.\d+)\n$/);

Also applies to: 10-10

🤖 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/aibolit.ts` at line 4, Replace the execSync call in check_version() with
execFileSync to avoid using a shell: call execFileSync(binary, argsArray, {
encoding: 'utf8' }) instead of execSync(commandString); construct argsArray
(e.g., ['--version'] or whatever flags were in the string), remove any
shell:true usage, and preserve existing stdout parsing and error handling so
check_version() returns the same version string on success and throws/handles
errors the same way.

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

Nitpick comments:
In `@src/aibolit.ts`:
- Line 4: Replace the execSync call in check_version() with execFileSync to
avoid using a shell: call execFileSync(binary, argsArray, { encoding: 'utf8' })
instead of execSync(commandString); construct argsArray (e.g., ['--version'] or
whatever flags were in the string), remove any shell:true usage, and preserve
existing stdout parsing and error handling so check_version() returns the same
version string on success and throws/handles errors the same way.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e2c3837e-9c19-43f4-97c2-2224b13db124

📥 Commits

Reviewing files that changed from the base of the PR and between 6a356aa and 08d8e53.

📒 Files selected for processing (2)

• src/aibolit.ts
• test/aibolit.test.ts