Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly escape extra args #31

Closed
wants to merge 5 commits into from
Closed

Properly escape extra args #31

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
19443b8
Properly escape extra args
lydell May 18, 2021
2004ecc
Add test case for double escaping on Windows
lydell May 20, 2021
d1c4c26
Fix `node server.js` args escaping on Windows
lydell May 20, 2021
74df3ab
Add node.cmd to "files" in package.json
lydell May 20, 2021
8a5639c
Rename node.cmd to nodewrapper.cmd to avoid potential infinite loops
lydell May 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions lib/run-script-pkg.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ const packageEnvs = require('./package-envs.js')
const { isNodeGypPackage, defaultGypInstallScript } = require('@npmcli/node-gyp')
const signalManager = require('./signal-manager.js')
const isServerPackage = require('./is-server-package.js')
const { ShellString, unquoted } = require('puka');

const sh = (...args) =>
ShellString.sh(...args).toString(process.env.__FAKE_TESTING_PLATFORM__ || process.platform)
Comment on lines +11 to +12
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

puka comes with a sh export (defined as const sh = (...args) => ShellString.sh(...args).toString() basically), but we’re making our own so we can test the Windows version more easily.

https://gitlab.com/rhendric/puka#shellstring

process.env.__FAKE_TESTING_PLATFORM__ || process.platform is copied from is-windows.js – let me know if you’d like to share that, and if so, how.


// you wouldn't like me when I'm angry...
const bruce = (id, event, cmd) =>
Expand Down Expand Up @@ -31,7 +35,7 @@ const runScriptPkg = async options => {
if (options.cmd)
cmd = options.cmd
else if (pkg.scripts && pkg.scripts[event])
cmd = pkg.scripts[event] + args.map(a => ` ${JSON.stringify(a)}`).join('')
cmd = sh`${unquoted(pkg.scripts[event])} ${args}`
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how Yarn does it:

https://github.com/yarnpkg/yarn/blob/3119382885ea373d3c13d6a846de743eca8c914b/src/cli/commands/run.js#L136

And how the original PR by the author of puka did it:

https://github.com/npm/cli/pull/52/files#diff-5fe49e56d3dc3f4bb6cead3ec153dbee2b55cc91f22bf991cac6546f6aa42623R172

else if ( // If there is no preinstall or install script, default to rebuilding node-gyp packages.
event === 'install' &&
!scripts.install &&
Expand All @@ -41,7 +45,7 @@ const runScriptPkg = async options => {
)
cmd = defaultGypInstallScript
else if (event === 'start' && await isServerPackage(path))
cmd = 'node server.js' + args.map(a => ` ${JSON.stringify(a)}`).join('')
cmd = sh`node server.js ${args}`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could still have issues if node resolves to node.exe (and not some node.cmd or node.bat alias). server.js will probably see extra escaping sigils if args involves anything that needs escaping.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I’ll test this on a Windows machine soon

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this. You are right: If node points to node.exe, the escaping is a problem.

I created server.js like this:

console.log(process.argv)

And ran this from the Node.js REPL:

> require("./lib/run-script-pkg")({event:"start",path:process.cwd(),pkg:{},args:['"foo"']}).then(r=>console.log(r.stdout.toString()), console.error)
Promise { <pending> }
> [
  'C:\\Program Files\\nodejs\\node.exe',
  'C:\\Users\\Simon\\stuff\\run-script\\server.js',
  '^\\^foo\\^^'
]

As one can see, server.js didn’t receive "foo" as expected but ^\\^foo\\^^.

However, if I install Node.js using npm i node, then server.js does receive "foo" since we go through npm’s batch script wrapper in ./node_modules/.bin/node.cmd.

I fixed this by shipping with a tiny node.cmd wrapper that we can use always. With it, I get this in the REPL, both with node.exe and after npm i node:

> require("./lib/run-script-pkg")({event:"start",path:process.cwd(),pkg:{},args:['"foo"']}).then(r=>console.log(r.stdout.toString()), console.error)
Promise { <pending> }
> [
  'C:\\Program Files\\nodejs\\node.exe',
  'C:\\Users\\Simon\\stuff\\run-script\\server.js',
  '"foo"'
]

Here’s the fix: d1c4c26 (#31)

Do you see any potential problems with this approach?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should work, although just to be safe maybe rename the wrapper to something like nodewrapper.cmd, or really anything other than node.cmd? If the directory containing the wrapper happened to be the working directory, or if it ever ended up in %PATH% somehow, you'd have an infinite loop on your hands.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also do what cross-spawn does, which is to take a dependency on the which package, use that to find node, and check to see if it's an .exe or something else; and then use Puka's quoteForWin32 or quoteForCmd respectively to escape the arguments. Or just use cross-spawn for the whole thing, assuming its escaping is as complete as Puka's (I have no reason to believe it is or isn't).


if (!cmd)
return { code: 0, signal: null }
Expand Down
175 changes: 144 additions & 31 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"@npmcli/promise-spawn": "^1.3.2",
"infer-owner": "^1.0.4",
"node-gyp": "^7.1.0",
"puka": "^1.0.1",
"read-package-json-fast": "^2.0.1"
},
"files": [
Expand Down
19 changes: 12 additions & 7 deletions test/run-script-pkg.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const { EventEmitter } = require('events')
const t = require('tap')
const requireInject = require('require-inject')
const isWindows = require('../lib/is-windows.js')

let fakeIsNodeGypPackage = false
let SIGNAL = null
Expand Down Expand Up @@ -96,7 +97,7 @@ t.test('pkg has server.js, start not specified, with args', async t => {
scripts: {},
},
})
t.strictSame(res, ['sh', ['-c', 'node server.js "a" "b" "c"'], {
t.strictSame(res, ['sh', ['-c', 'node server.js a b c'], {
stdioString: false,
event: 'start',
path,
Expand All @@ -105,10 +106,10 @@ t.test('pkg has server.js, start not specified, with args', async t => {
environ: 'value',
},
stdio: 'pipe',
cmd: 'node server.js "a" "b" "c"',
cmd: 'node server.js a b c',
}, {
event: 'start',
script: 'node server.js "a" "b" "c"',
script: 'node server.js a b c',
pkgid: 'foo@1.2.3',
path,
}])
Expand Down Expand Up @@ -288,6 +289,10 @@ t.test('pkg has foo script', t => runScriptPkg({
path: 'path',
}])))

const expectedCommand = isWindows
? 'bar a --flag "markdown `code`" ^^^"$X^^^ \\\\\\^^^"blah\\\\\\^^^"^^^" $PWD ^^^%CD^^^% "^" ! \\ ">" "<" "|" "&" \' ^^^"\\^^^"^^^" ` " " ""'
: "bar a --flag 'markdown `code`' '$X \\\"blah\\\"' '$PWD' %CD% ^ ! '\\' '>' '<' '|' '&' \\' '\"' '`' ' ' ''"

t.test('pkg has foo script, with args', t => runScriptPkg({
event: 'foo',
path: 'path',
Expand All @@ -302,8 +307,8 @@ t.test('pkg has foo script, with args', t => runScriptPkg({
foo: 'bar',
},
},
args: ['a', 'b', 'c'],
}).then(res => t.strictSame(res, ['sh', ['-c', 'bar "a" "b" "c"'], {
args: ['a', '--flag', 'markdown `code`', '$X \\"blah\\"', '$PWD', '%CD%', '^', '!', '\\', '>', '<', '|', '&', "'", '"', '`', ' ', ''],
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inspired by Yarn’s tests: https://github.com/yarnpkg/yarn/blob/3119382885ea373d3c13d6a846de743eca8c914b/__tests__/integration.js#L424

}).then(res => t.strictSame(res, ['sh', ['-c', expectedCommand,], {
stdioString: false,
event: 'foo',
path: 'path',
Expand All @@ -312,10 +317,10 @@ t.test('pkg has foo script, with args', t => runScriptPkg({
environ: 'value',
},
stdio: 'pipe',
cmd: 'bar "a" "b" "c"',
cmd: expectedCommand,
}, {
event: 'foo',
script: 'bar "a" "b" "c"',
script: expectedCommand,
pkgid: 'foo@1.2.3',
path: 'path',
}])))
Expand Down