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.

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

escaping pass-through arguments is too eager #487

Open
soletan opened this issue Jun 14, 2024 · 4 comments
Open

escaping pass-through arguments is too eager #487

soletan opened this issue Jun 14, 2024 · 4 comments
Labels

Comments

@soletan
Copy link

soletan commented Jun 14, 2024

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
Copy link
Author

soletan commented Jun 14, 2024

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
Copy link
Author

soletan commented Jun 14, 2024

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
Copy link
Member

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
Copy link
Author

soletan commented Jun 22, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants