fix: safer os command execution #45
Conversation
WalkthroughReplaces direct Changes
Sequence Diagram(s)sequenceDiagram
participant DevScript as scripts/generate.v3.ts
participant Helper as runTradetrust
participant Spawn as spawnSync (node)
participant ChildProc as External tradetrust CLI
DevScript->>Helper: call runTradetrust(args, options)
note right of Helper #DDFFDD: prepares args & options\n(inherit stdio, timeout)
Helper->>Spawn: spawnSync(command, args, options)
Spawn->>ChildProc: execute CLI
alt child exits normally (status 0)
Spawn-->>Helper: { status: 0, stdout, stderr }
Helper-->>DevScript: returns result
else non-zero exit or signal
Spawn-->>Helper: { status: non-zero | signal, error? }
Helper-->>DevScript: throw Error (includes code/signal and stderr)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/generate.v3.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests / Run Tests (20.x)
- GitHub Check: Tests / Run Tests (18.x)
🔇 Additional comments (2)
scripts/generate.v3.ts (2)
4-4: LGTM: Import change aligns with safer execution.The switch from
execSynctospawnSyncsupports the security improvement, asspawnSyncwith array arguments (no shell) prevents command injection vulnerabilities.
123-134: LGTM: All command invocations are properly structured.All six call sites correctly structure arguments as array elements, preventing command injection. The consistent use of the
runTradetrusthelper ensures uniform error handling and timeout management across all external command executions.Also applies to: 248-259, 261-272, 274-285, 287-298, 329-342, 344-357
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/generate.v3.ts (1)
39-54: Past review comment addressed; consider enriching error messages.The signal handling from the previous review has been correctly implemented. The error handling now covers execution errors, signal termination, and non-zero exit codes.
Based on learnings
For improved debugging, consider including the command and arguments in error messages:
const runTradetrust = (args: string[], timeoutMs?: number) => { const result = spawnSync("tradetrust", args, { timeout: timeoutMs ?? ethereumDocumentConfig.timeout, stdio: "inherit", }); if (result.error) { - throw result.error; + throw new Error(`Failed to execute tradetrust ${args.join(" ")}: ${result.error.message}`); } if (result.signal) { - throw new Error(`Command terminated by signal ${result.signal}`); + throw new Error(`tradetrust ${args.join(" ")} terminated by signal ${result.signal}`); } if (result.status !== 0) { - throw new Error(`Command failed with exit code ${result.status}`); + throw new Error(`tradetrust ${args.join(" ")} failed with exit code ${result.status}`); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/generate.v3.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests / Run Tests (18.x)
- GitHub Check: Tests / Run Tests (20.x)
🔇 Additional comments (2)
scripts/generate.v3.ts (2)
4-4: LGTM: Safer alternative to execSync.The switch to
spawnSyncprovides better control over child process execution and more granular error handling.
126-137: LGTM: Secure command invocations with controlled arguments.All invocations of
runTradetrustuse controlled arguments from configuration and derived merkle roots. No user input is directly passed, eliminating command injection risks.Also applies to: 251-262, 264-275, 277-288, 290-301, 332-345, 347-360
|
🎉 This PR is included in version 9.6.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit