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 3 commits
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
2 changes: 2 additions & 0 deletions lib/node.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
@ECHO off
node %*
18 changes: 16 additions & 2 deletions lib/run-script-pkg.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
const pathModule = require('path')
const makeSpawnArgs = require('./make-spawn-args.js')
const promiseSpawn = require('@npmcli/promise-spawn')
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 isWindows = require('./is-windows.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 +37,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 +47,15 @@ const runScriptPkg = async options => {
)
cmd = defaultGypInstallScript
else if (event === 'start' && await isServerPackage(path))
cmd = 'node server.js' + args.map(a => ` ${JSON.stringify(a)}`).join('')
// `node` probably means `C:\Program Files\nodejs\node.exe`.
// But it can also point to `.\node_modules\.bin\node.cmd` if you install a
// Node.js version using `npm i node`.
// So it might point to an executable, or a batch file. They require
// different escaping. Also, `puka` only supports batch-style escaping.
// The solution is to always use a batch file on Windows.
Copy link
Contributor

@nlf nlf May 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

does this comment mean that if a user has a script that runs a command that is not a batch file, we will be escaping it wrong?

if i have

{
  "scripts": {
    "go": "node server.js"
  }
}

and i run npm run go -- some args, then we won't be using the batch file wrapper, so does that mean that we're now producing a command that doesn't work correctly? what if the first command is a shell builtin, like cd?

Copy link
Author

@lydell lydell May 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

You’re right, this won’t be using the batch file wrapper. node is most likely an executable, so the escaping would be wrong (except in cases where people have installed node using npm i node). Dang, Windows escaping sucks :( See this comment for more details: #31 (comment)

Running node in a script like in your example feels like a very common use case, so it would be nice if that worked good.

Ideas:

In summary: This is super easy to do right on macOS and Linux, and a minefield on Windows.

Copy link
Author

Choose a reason for hiding this comment

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

The more I think about this, the more I realize something:

  • I don’t use -- extra args with npm run very much, so I don’t care super much about that. It’s a bit of an impossible task to append extra args to an unknown shell string.

  • I do use npx a lot and would like it to be intuitive. I think implementing it in terms of npm run feels pretty hacky – it’s like creating a string and using eval() in JavaScript for something that can be done just fine in the language.

cmd = isWindows
? sh`${pathModule.join(__dirname, 'node.cmd')} server.js ${args}`
: sh`node server.js ${args}`
Copy link
Author

@lydell lydell May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Note: This means that npm start with no "start" in package.json but with a server.js will print something like this on Windows:

> C:\absolue\path\to\npm\node_modules\run-script\lib\nodewrapper.cmd server.js

instead of:

> node server.js

Let me know if you’d rather want to print > node server.js on Windows too (even though that’s not what we’re actually executing).


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
Loading