fix: escape shell arg single quotes#107
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
commit: |
![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 POSIX shell arguments by replacing single quotes with '\'' instead of escaping backslashes and single quotes. It also updates the documentation to clarify its POSIX-only usage and adds corresponding tests. The review feedback suggests removing the node: prefix from the imports to maintain compatibility with older Node.js versions in the 3.x branch, and using assert.strictEqual instead of assert.equal for strict equality checks.
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.
| const assert = require('node:assert/strict'); | ||
| const { exec: childProcessExec } = require('node:child_process'); | ||
| const { promisify } = require('node:util'); |
There was a problem hiding this comment.
Since this is a backport to the 3.x branch, it may be run on older Node.js versions (such as Node.js 14 < 14.18.0) that do not support the node: prefix or node:assert/strict. To ensure maximum compatibility across all supported Node.js versions in the 3.x release line, it is safer to use standard require statements without the node: prefix.
| const assert = require('node:assert/strict'); | |
| const { exec: childProcessExec } = require('node:child_process'); | |
| const { promisify } = require('node:util'); | |
| const assert = require('assert'); | |
| const { exec: childProcessExec } = require('child_process'); | |
| const { promisify } = require('util'); |
| const payload = '\'; echo EGG_SECURITY_INJECTED; #'; | ||
| const { stdout } = await exec(`printf 'ARG:%s\\n' ${escapeShellArg(payload)}`); | ||
|
|
||
| assert.equal(stdout, 'ARG:\'; echo EGG_SECURITY_INJECTED; #\n'); |
There was a problem hiding this comment.
Use assert.strictEqual instead of assert.equal to ensure strict equality comparison, especially when using the standard assert module instead of the strict assertion mode.
| assert.equal(stdout, 'ARG:\'; echo EGG_SECURITY_INJECTED; #\n'); | |
| assert.strictEqual(stdout, 'ARG:\'; echo EGG_SECURITY_INJECTED; #\n'); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 3.x #107 +/- ##
=======================================
Coverage 97.29% 97.29%
=======================================
Files 32 32
Lines 1367 1367
Branches 344 344
=======================================
Hits 1330 1330
Misses 37 37 ☔ 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 backports a security fix to the escapeShellArg helper on the 3.x line so single quotes embedded in an argument are escaped using the POSIX-safe '\'' pattern, preventing quoted-string termination when callers build child_process.exec() command strings.
Changes:
- Update
escapeShellArg()to escape embedded single quotes using the POSIX-safe quoting sequence. - Add a POSIX-only regression test that verifies a payload like
'; echo ...; #remains a single argument when executed via/bin/sh. - Improve test reliability/cleanup by closing mock apps in
after()hooks and by waiting for logs indtatests; update docs to clarify POSIX-only scope and recommendexecFile()/spawn()argument arrays.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/helper/escapeShellArg.js | Implements POSIX-safe embedded single-quote escaping for shell single-argument quoting. |
| test/app/extends/escapeShellArg.test.js | Adds a POSIX-only exec-based regression test to ensure injected metacharacters don’t break argument boundaries; improves teardown. |
| test/fixtures/apps/helper-escapeShellArg-app/app/router.js | Updates expected escaped output for the fixture route to match the new POSIX escaping behavior. |
| README.md | Clarifies the helper is POSIX-shell-only and recommends execFile()/spawn() argument arrays. |
| README.zh-CN.md | Same clarification as README.md, in Chinese. |
| test/utils.test.js | Refactors utils.checkIfIgnore tests to create/close apps per test; adds missing app close for isSafeDomain suite. |
| test/dta.test.js | Replaces a platform-specific sleep with a bounded retry helper (waitForLog) and ensures app closure. |
| test/app/extends/spath.test.js | Ensures app is closed in after() and restores mocks. |
| test/app/extends/sjson.test.js | Ensures app is closed in after() and restores mocks. |
| test/app/extends/sjs.test.js | Ensures app is closed in after() and restores mocks. |
| test/app/extends/helper.test.js | Ensures all apps are closed in after() and restores mocks. |
| test/app/extends/escapeShellCmd.test.js | Ensures app is closed in after() and restores mocks. |
| test/app/extends/cliFilter.test.js | Ensures app is closed in after() and restores mocks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const str = '' + string; | ||
| return '\'' + str.replace(/\\/g, '\\\\').replace(/\'/g, '\\\'') + '\''; | ||
| return '\'' + str.replace(/'/g, '\'\\\'\'') + '\''; |
Summary
Backport the
escapeShellArgfix to3.xso 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 test -- --grep escapeShellArgran lint successfully, but the script treatedescapeShellArgas a file filter and reported no test files.npm run test-local -- test/app/extends/escapeShellArg.test.jspassed: 4 passing.git diff --checkchild_process.exec: payload'; echo EGG_SECURITY_INJECTED; #is printed as one argument and does not execute the injectedecho.Note: the first non-escalated
test-localattempt failed with sandboxlisten EPERMwhile binding the Egg test app; rerunning the same command with local port permission passed.