fix: escape shell arg single quotes#106
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughUpdates POSIX shell argument escaping and its documentation, adds execution-based tests, refines ChangesPOSIX shell escaping
URL helper typings
Dependency version overrides
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the escapeShellArg helper to correctly escape single quotes for POSIX shells by closing the single quote, appending an escaped single quote, and reopening the single quote, rather than incorrectly escaping backslashes and single quotes inside single quotes. It also updates the English and Chinese documentation to clarify that this helper is designed for POSIX shells and recommends using child_process.execFile or child_process.spawn when possible. Additionally, the test suite has been updated with a new POSIX-specific test to verify the escaping behavior. I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #106 +/- ##
=======================================
Coverage 95.77% 95.77%
=======================================
Files 32 32
Lines 1798 1800 +2
Branches 308 308
=======================================
+ Hits 1722 1724 +2
Misses 76 76 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR tightens POSIX shell-argument escaping by fixing escapeShellArg to use the POSIX-safe '\'' pattern for embedded single quotes, and adds regression coverage/documentation to reduce shell-injection risk when callers concatenate commands for child_process.exec().
Changes:
- Fix
escapeShellArgto escape embedded'using the POSIX-safe'\''sequence. - Add a POSIX-only regression test that ensures an injection-shaped payload remains a single argument.
- Clarify documentation to scope the helper to POSIX shells and recommend
execFile()/spawn()argument arrays.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/fixtures/apps/helper-escapeShellArg-app/app/router.js | Updates fixture expectation to match POSIX-safe single-quote escaping output. |
| test/app/extends/escapeShellArg.test.ts | Adds a POSIX-only exec-based regression test for embedded single quotes/injection-shaped payloads. |
| src/lib/utils.ts | Updates getFromUrl overloads/typing (note: see API contract comment). |
| src/lib/helper/escapeShellArg.ts | Implements the POSIX-safe single-quote escaping pattern. |
| README.zh-CN.md | Clarifies POSIX-only scope and recommends safer process APIs. |
| README.md | Clarifies POSIX-only scope and recommends safer process APIs. |
| package.json | Adds npm overrides (needs rationale alignment with PR description). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function getFromUrl(url: string, prop: string): string | null; | ||
| export function getFromUrl(url: string, prop?: string): URL | string | null { | ||
| try { | ||
| const parsed = new URL(url); | ||
| return prop ? Reflect.get(parsed, prop) : parsed; |
| "overrides": { | ||
| "fflate": "0.8.2", | ||
| "urllib": "4.4.0" | ||
| }, |
Summary
Fix
escapeShellArgso embedded single quotes use the POSIX-safe'\''quoting pattern instead of\', which does not escape single quotes inside POSIX shell single-quoted strings.Why
The previous output could terminate the quoted argument and allow a following shell metacharacter sequence to execute when callers used the documented
child_process.exec()string-concatenation pattern.Changes
'\''sequence.'; echo ...; #remains one argument.execFile()/spawn()argument arrays where possible.Validation
npm run lint -- --fixvianpm test -- --grep escapeShellArggit diff --checknode --loader ts-node/esm: payload'; echo EGG_SECURITY_INJECTED; #is printed as one argument and does not execute the injectedecho.Note:
npm test -- --grep escapeShellArgcould not complete on this checkout because an existing unrelated type error blocks test startup:src/lib/utils.ts(197,47): Type 'URL' is not assignable to type 'string'.Summary by CodeRabbit
Documentation
Bug Fixes
Tests