escaping pass-through arguments is too eager

soletan commented 2 months ago

Background

When trying to use concurrently with --passthrough-arguments option, any passed arguments get escaped too eagerly.

The issue has been observed while developing an application when trying to concurrently run a local web server and gherkin-testcafe for testing with the latter requring tags to start with @ character.

Since that application can't be shared here for legal reasons I've tried a simpler scenario which is reproducing the issue, too.

Scenario

windows platform
npm 10.5.2
node 20.13.1
concurrently 8.2.2

files:

package.json contains scripts:

{"scripts": {
"foo": "concurrently -k -n \"bar,baz\" -P npm:foo:bar \"npm:foo:baz -- {@}\"",
"foo:bar": "node block.js",
"foo:baz": "node log.js",
}}

file block.js looks like this:

setTimeout(() => console.log("delayed"), 1000);

file log.js looks like this:
```
console.dir(process.argv);
```

Test

invoke without arguments

$ npm run foo

> logistics-client@0.0.0 foo
> concurrently -k -n "bar,baz" -P npm:foo:bar "npm:foo:baz -- {@}"

[bar] 
[bar] > logistics-client@0.0.0 foo:bar
[bar] > node block.js
[bar]
[baz]
[baz] > logistics-client@0.0.0 foo:baz
[baz] > node log.js
[baz]
[baz] [
[baz]   'C:\\Program Files\\nodejs\\node.exe',
[baz]   'C:\\Users\\cepharum\\PhpstormProjects\\UID\\Logistics\\WebClient\\Source\\UiNew\\log.js'
[baz] ]
[baz] npm run foo:baz --  exited with code 0
--> Sending SIGTERM to other processes..
[bar] npm run foo:bar exited with code 1

looks good ... setup is working as expected

invoke with arguments to pass

$ npm run foo -- -- --tags=@foo

> logistics-client@0.0.0 foo
> concurrently -k -n "bar,baz" -P npm:foo:bar "npm:foo:baz -- {@}" -- --tags=@foo

[bar]
[bar] > logistics-client@0.0.0 foo:bar
[bar] > node block.js
[bar]
[baz]
[baz] > logistics-client@0.0.0 foo:baz
[baz] > node log.js --tags\=\@foo
[baz]
[baz] [
[baz]   'C:\\Program Files\\nodejs\\node.exe',
[baz]   'C:\\Users\\cepharum\\PhpstormProjects\\UID\\Logistics\\WebClient\\Source\\UiNew\\log.js',
[baz]   '--tags\\=\\@foo'
[baz] ]
[baz] npm run foo:baz -- --tags\=\@foo exited with code 0
--> Sending SIGTERM to other processes..
[bar] npm run foo:bar exited with code 1

Problem here:

the assignment operator gets escaped
the @ gets escaped

Escaping seems to be eligible due to cmd.exe being involved. But eventually that escaping is found in invoked sub-process, too. Trying to fix it in that sub-process is not an option for it is a third-party application (gherkin-testcafe).

gherkin-testcafe illustrates the provision of tags without assignment operator, so I tried that one, too:

$ npm run foo -- -- --tags @foo

> logistics-client@0.0.0 foo
> concurrently -k -n "bar,baz" -P npm:foo:bar "npm:foo:baz -- {@}" -- --tags @foo

[baz] 
[baz] > logistics-client@0.0.0 foo:baz
[baz] > node log.js --tags \@foo
[baz] 
[bar] 
[bar] > logistics-client@0.0.0 foo:bar
[bar] > node block.js
[bar] 
[baz] [
[baz]   'C:\\Program Files\\nodejs\\node.exe',
[baz]   'C:\\Users\\cepharum\\PhpstormProjects\\UID\\Logistics\\WebClient\\Source\\UiNew\\log.js',
[baz]   '--tags',
[baz]   '\\@foo'
[baz] ]
[baz] npm run foo:baz -- --tags \@foo exited with code 0
--> Sending SIGTERM to other processes..
[bar] npm run foo:bar exited with code 1

The @ character still ends up escaped.

I've also tried using {*} on invoking concurrently. So, the script in package.json is now:

"foo": "concurrently -k -n \"bar,baz\" -P npm:foo:bar \"npm:foo:baz -- {*}\"",

Invoking as before prevents the escaping, but yields a different issue:

$ npm run foo -- -- --tags @foo

> logistics-client@0.0.0 foo
> concurrently -k -n "bar,baz" -P npm:foo:bar "npm:foo:baz -- {*}" -- --tags @foo

[baz] 
[baz] > logistics-client@0.0.0 foo:baz
[baz] > node log.js '--tags @foo'
[baz]
[bar]
[bar] > logistics-client@0.0.0 foo:bar
[bar] > node block.js
[bar]
[baz] [
[baz]   'C:\\Program Files\\nodejs\\node.exe',
[baz]   'C:\\Users\\cepharum\\PhpstormProjects\\UID\\Logistics\\WebClient\\Source\\UiNew\\log.js',
[baz]   "'--tags",
[baz]   "@foo'"
[baz] ]
[baz] npm run foo:baz -- '--tags @foo' exited with code 0
--> Sending SIGTERM to other processes..
[bar] npm run foo:bar exited with code 1

So, the {*} seems to wrap all additional arguments in a single pair quotes. This may be intentional, but it definitely doesn't help here:

$ npm run foo -- -- --tags @foo --debug-on-fail

> logistics-client@0.0.0 foo
> concurrently -k -n "bar,baz" -P npm:foo:bar "npm:foo:baz -- {*}" -- --tags @foo --debug-on-fail

[baz] 
[baz] > logistics-client@0.0.0 foo:baz
[baz] > node log.js '--tags @foo --debug-on-fail'
[baz]
[bar]
[bar] > logistics-client@0.0.0 foo:bar
[bar] > node block.js
[bar]
[baz] [
[baz]   'C:\\Program Files\\nodejs\\node.exe',
[baz]   'C:\\Users\\cepharum\\PhpstormProjects\\UID\\Logistics\\WebClient\\Source\\UiNew\\log.js',
[baz]   "'--tags",
[baz]   '@foo',
[baz]   "--debug-on-fail'"
[baz] ]
[baz] npm run foo:baz -- '--tags @foo --debug-on-fail' exited with code 0
--> Sending SIGTERM to other processes..
[bar] npm run foo:bar exited with code 1

soletan commented 2 months ago

I've just cloned concurrently locally to create a build off current main branch, then linked it into that other project.

$ git clone https://github.com/open-cli-tools/concurrently.git
$ cd concurrently
$ npm i
$ npm build
$ npm link

Then, in my project:

$ npm link concurrently

The outcome is the same. ~~I thought it might have been related to that windowVerbatimArguments option set. But nope.~~ I'm a fool ...

soletan commented 2 months ago

I've kept on trying to nail down the issue. From concurrently perspective, relying on shell-quote to do the job seems to cause this issue. As pointed out in the linked issue (comment1 comment2), maybe shell-quote isn't meant to support Windows' cmd.exe. What about concurrently? ... just asking.

gustavohenke commented 2 months ago

Hey, thanks for the report. Yes, concurrently should work on Windows. We have, however, been naive not to test with certain special characters such as @ and =, like you did.

I'm willing to be more lenient with escaping/wrapping in quotes than the shell-quote package if it means having less bugs like yours. E.g. arguments with a space need wrapping, unless they're already wrapped, of course. But I'm not sure about the others, might need to go through the list below and reimplement it in concurrently:

https://github.com/ljharb/shell-quote/blob/9eecafc0486c9321be223415cf3fb76a5bd07dda/quote.js#L14

(that said, I'm admittedly ignorant of risks of getting this wrong 😅 )

soletan commented 2 months ago

Well, I was hesitant to provide a solution I found based on my follow-up investigations as documented in the related issue at shell-quote repository for I was hoping to get a solution done there. However, since I wasn't able to wait for that with the original issue I was facing I've created a patched version of concurrently with shell-quote being replaced with my package. Maybe, I should provide it as a PR here.

open-cli-tools / concurrently