sec: Prevent Arbitrary Command Injection in Terminal Execution Helpers (#3202) - #3281
sec: Prevent Arbitrary Command Injection in Terminal Execution Helpers (#3202)#3281knoxiboy wants to merge 2 commits into
Conversation
github-actions
Bot
added
area:jsx
📝 WalkthroughWalkthrough
ChangesSubprocess validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 3
🤖 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 `@packages/data/src/hooks/_exec.ts`:
- Line 18: Update the execFile options in the execution flow to spread caller
options before explicitly setting shell: false, ensuring opts.shell cannot
override the safety setting. Add a regression test covering opts.shell: true and
verify shell execution remains disabled.
In `@packages/jsx/src/hooks/useSubprocess.test.ts`:
- Line 25: Remove the unannotated any casts in the useSubprocess tests,
including the mockSpawn setup and the additional occurrences at the referenced
test cases. Use properly typed subprocess and application fixtures; if a type
assertion remains necessary, replace any with the narrowest type and add an
inline comment describing its specific purpose.
- Around line 40-50: Update the application fixtures in the affected hook tests
to instantiate the real Screen from `@termuijs/core` instead of providing fake
screen objects. Spy on the real Screen instance’s invalidate method while
preserving the existing terminal and requestRender test behavior in the relevant
test cases.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f3b39404-c17e-46e2-a044-db4c64536428
📒 Files selected for processing (4)
packages/data/src/hooks/_exec.test.tspackages/data/src/hooks/_exec.tspackages/jsx/src/hooks/useSubprocess.test.tspackages/jsx/src/hooks/useSubprocess.ts
| it('exits raw mode before spawning and restores the TUI after exit', async () => { | ||
| const app = { | ||
| terminal: { | ||
| exitRawMode: vi.fn(), | ||
| enterRawMode: vi.fn(), | ||
| }, | ||
| screen: { | ||
| invalidate: vi.fn(), | ||
| }, | ||
| requestRender: vi.fn(), | ||
| } as any; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use the real Screen in these hook tests.
Both fixtures provide a fake screen object. Construct the application fixture with Screen from @termuijs/core and spy on its invalidate() method instead.
As per coding guidelines, package tests must use the real Screen and must not write fake screen objects.
Also applies to: 72-82
🤖 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 `@packages/jsx/src/hooks/useSubprocess.test.ts` around lines 40 - 50, Update
the application fixtures in the affected hook tests to instantiate the real
Screen from `@termuijs/core` instead of providing fake screen objects. Spy on the
real Screen instance’s invalidate method while preserving the existing terminal
and requestRender test behavior in the relevant test cases.
Source: Coding guidelines
…jot786#3202) - Fix critical spread order: change { shell: false, ...opts } to { ...opts, shell: false } so callers cannot override the shell: false security invariant - Add regression test that verifies shell: true in opts is always overridden - Replace raw 'any' casts in useSubprocess.test.ts with typed makeAppStub() factory using intentional 'unknown' cast (explained inline) - Replace 'proc as any' with ReturnType<typeof spawn> cast Addresses review comments from coderabbitai
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/jsx/src/hooks/useSubprocess.test.ts`:
- Around line 110-116: Add an assertion to the null-byte rejection test for
useSubprocess that verifies mockSpawn was never called after subprocess.run
rejects. Keep the existing error-message assertion unchanged and explicitly
protect the no-execution behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f68d8bf-c465-4fcb-afd3-12dee0ae3fcf
📒 Files selected for processing (3)
packages/data/src/hooks/_exec.test.tspackages/data/src/hooks/_exec.tspackages/jsx/src/hooks/useSubprocess.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/data/src/hooks/_exec.test.ts
| it('throws when command contains null bytes', async () => { | ||
| const subprocess = useSubprocess(); | ||
|
|
||
| await expect(subprocess.run(['ls', 'dir\0malicious'])).rejects.toThrow( | ||
| 'useSubprocess: command contains null bytes', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Assert that null-byte input never invokes spawn.
This test only checks rejection and the error message. Add expect(mockSpawn).not.toHaveBeenCalled() to protect the no-execution security invariant.
🤖 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 `@packages/jsx/src/hooks/useSubprocess.test.ts` around lines 110 - 116, Add an
assertion to the null-byte rejection test for useSubprocess that verifies
mockSpawn was never called after subprocess.run rejects. Keep the existing
error-message assertion unchanged and explicitly protect the no-execution
behavior.
|
No activity on this PR for 14 days. Rebase, resolve conflicts, or comment to keep it open. It closes in 7 days otherwise. |
|
Defensive hardening mislabeled as a fix — no current injection vector. execFile/spawn already throw synchronously on null bytes (→ rejection inside the existing Promise), so the checks are redundant with Node; and shell:false guards a shell:true no call site uses. Same no-op shape as #2469. Reframe as defense-in-depth, not a vuln fix. |
Description
This PR hardens command execution helpers (execFileAsync in @termuijs/data and useSubprocess in @termuijs/jsx) against command injection, null-byte injection, and dangerous shell execution.
Related Issue
Closes #3202
Which package(s)?
@termuijs/data, @termuijs/jsx
Type of Change
Checklist
eeds-star check blocks your merge otherwise.
GSSoC 2026 Participation
Notes for the Reviewer
Added null-byte and argument validation to execFileAsync and useSubprocess to prevent command injection and untrusted subprocess execution.
Summary by CodeRabbit
Bug Fixes
Tests