Possible regression with 4.0.0, && operator have a strange behaviour

[ebriand]

Hi

I'm running on windows 7 with node 7.7.3. I updated cross-env to 4.0.0 and migrate my npm scripts but one of my script doesn't work anymore. I tested this behavior with an empty project :

With cross-env 3.2.4 and the npm script :

 "greet": "cross-env MESSAGE=hello nodemon --exec \"echo %MESSAGE% && echo there is no problem \""

output :

$ cross-env MESSAGE=hello nodemon --exec "echo %MESSAGE% && echo there is no problem "
[nodemon] 1.11.0
[nodemon] to restart at any time, enter `rs`
[nodemon] watching: *.*
[nodemon] starting `echo hello && echo there is no problem`
hello
there is no problem
[nodemon] clean exit - waiting for changes before restart

With cross-env 4.0.0 and the script :

"greet": "cross-env MESSAGE=hello nodemon --exec \"echo $MESSAGE && echo there is no problem \""

output :

$ cross-env MESSAGE=hello nodemon --exec "echo $MESSAGE & echo there is no problem "
[nodemon] 1.11.0
[nodemon] to restart at any time, enter `rs`
[nodemon] watching: *.*
[nodemon] starting `echo hello`
hello
[nodemon] clean exit - waiting for changes before restart

Maybe I'm missing something ?

[DanReyLop]

Thanks, I've managed to reproduce it with your instructions. It looks like a bug indeed :). After debugging a little, I've found the cause:

This command:
cross-env MESSAGE=hello nodemon --exec "echo $MESSAGE && echo there is no problem"
Will execute nodemon with these arguments:
nodemon [ '--exec', 'echo $MESSAGE && echo there is no problem ' ]
Since that gets passed to a shell, it gets converted to:
nodemon '--exec echo $MESSAGE && echo there is no problem
Which is wrong.

I don't have time to solve this right now (contributions welcome!), but I've found a workaround that may get you by:
"greet": "cross-env MESSAGE=hello nodemon --exec \\\"echo $MESSAGE && echo there is no problem\\\"" (Note the double quote escaping)
Will execute nodemon with these arguments:
nodemon [ '--exec', '"echo $MESSAGE && echo there is no problem "' ]
Which fixes your problem.

[ebriand]

Thanks a lot for your response and the workaround, I will look if I can make a PR for the fix 😃

[hgwood]

Shouldn't the shell option quote arguments? It does not : https://github.com/nodejs/node/blob/b9f6a2dc059a1062776133f3d4fd848c4da7d150/lib/child_process.js#L335

[hgwood]

I'm not sure if it's a bug or what we actually want. 🤔

[hgwood]

We were warned. But I misunderstood the warning. I thought it meant "prefer cross-spawn's shell over Node's shell, cross-spawn's shell will ensure proper escaping", when actually, if the shell option is true, cross-spawn does nothing at all.

[DanReyLop]

I'm not sure if it's a bug or what we actually want. 🤔

I think it's definitely a bug. Taking the script on the example:
cross-env MESSAGE=hello nodemon --exec "echo $MESSAGE && echo there is no problem"
The expected executed program should be:
nodemon --exec "echo $MESSAGE && echo there is no problem"
And instead it's being:
nodemon --exec echo $MESSAGE && echo there is no problem
Which is very different in this context.

Actually the arguments passed to spawn make sense, they are these:

command: 'nodemon'
commandArgs: [ '--exec', 'echo $MESSAGE && echo there is no problem ' ]

Which, if they were properly escaped by spawn, should work as expected.

If the problem is with cross-spawn, maybe we should consider dropping it and using options.shell directly. I don't know what else cross-spawn is offering us right now, apart from compatibility with node < 4.8.

[kentcdodds]

If the problem is with cross-spawn, maybe we should consider dropping it and using options.shell directly. I don't know what else cross-spawn is offering us right now, apart from compatibility with node < 4.8.

If we can verify that doing that wont break a bunch of use cases, then I'm all for it.

[jharris4]

I just hit a possible duplicate of this bug after upgrading from 3.2.4 to 4.0.0.

I have the following npm script:

cross-env NODE_ENV=production rollup -c --banner \"$(preamble)\"

With 3.2.4 this works fine, but with 4.0.0 I get an error from rollup saying rollup can only bundle one file at a time (because the --banner option is getting mangled)

[jharris4]

@DanReyLop Thanks for the workaround, with cross-env 4.0.0, I am able to workaround the issue by changing my npm script to the following:

cross-env NODE_ENV=production rollup -c --banner \\\"$(preamble)\\\"

[hgwood]

@DanReyLop I agree it is a bug. cross-env users expect everything after env setters to be passed to their shell verbatim (except for variable syntax conversion of course).

You are right that cross-spawn is bringing nothing more than compatibility with Node <4.8. It does nothing more, nothing less. Which is to say that the problem is not with cross-spawn. We could drop it and use spawn instead, the problem would be the same.

When the shell option is active, the command and args arguments passed to spawn are simply joined with spaces. There is no escaping. You can see the code here for 7.8 and here for 4.8. This is true in cross-spawn too.

This is what we could do:

• Drop cross-spawn (raise requirement to Node 4.8).
• Use shell-quotes to get a quoted shell command, that we would pass as spawn's first argument. The second argument would always be an empty array.

Obviously, that would be a breaking change. Fixing this bug can only be a breaking change.

I think we also need stronger tests to avoid this kind of problems. The use of cross-spawn prevented us from really testing the command we produced. Without it we could have better end-to-end tests (given this process.argv then I spawn this shell command).

@DanReyLop @kentcdodds @ebriand Thoughts?

I should have time to work on this in the coming week.

[DanReyLop]

Which is to say that the problem is not with cross-spawn. We could drop it and use spawn instead, the problem would be the same.

My bad, I misead how shell: true works.

Use shell-quotes to get a quoted shell command, that we would pass as spawn's first argument. The second argument would always be an empty array.

It wouldn't be as simple as that, because shell-quotes also escapes $ characters. Maybe we could resolve env vars to their values before spawning the command? So this:
cross-env MESSAGE=hello nodemon --exec "echo $MESSAGE && echo there is no problem"
Gets parsed into:

command: "nodemon"
commandArgs: ["--exec", "echo hello && echo there is no problem"

And then escaped using shell-quotes:
spawn('nodemon --exec "echo hello && echo there is no problem"')

[kentcdodds]

If we do that we'll definer want thorough testing to make sure the variable replacement doesn't really in an invalid command. But I'm good with it if we can achieve that. Would rather do something else though...

[hgwood]

So maybe it's not shell-quotes that we need but something else. It should put quotes around arguments and escapes existing quotes inside the args. Eg:

• ['a', 'b c', 'd'] => a 'b c' d
• ['a', "b 'c'", 'd'] => a 'b \'c\'' d

Should it do something else that I'm missing?

[hgwood]

Just to clarify how the workaround works:

• The original text / what we can read in package.json: cross-env nodemon --exec \\\"echo $MESSAGE && echo there is no problem\\\"
• The actual JSON value / what the shell sees: cross-env nodemon --exec \"echo $MESSAGE && echo there is no problem\"
• The shell-parsed value / what cross-env gets as process.argv:

[ 'nodemon',
  '--exec',
  '"echo',
  '$MESSAGE',
  '&&',
  'echo',
  'there',
  'is',
  'no',
  'problem"' ]

• Spawn call: spawn('/bin/sh', ['-c', 'nodemon --exec "echo $MESSAGE && echo there is no problem"'])
• What the spawned shell sees: nodemon --exec "echo $MESSAGE && echo there is no problem"

[hgwood]

FYI, discussion on solutions continued over at #102.

[mariotacke]

I ran into this or a related issue by using this command line:

cross-env DEBUG=myapp:* nodemon

It works on 3.x, but it seems like it breaks on the colon (:) now. If i log what the current DEBUG is when running the app I get this:

console.log(process.env.DEBUG);

myapp;*

Notice the : became a ;.

Indeed, when I try cross-env DEBUG=::::: nodemon it prints ;;;;;. Weird.

[kentcdodds]

This was a documented breaking change to support windows better:

If an env variable has : or ; in its value, it will be converted to : on UNIX systems or ; on Windows systems. To keep the old functionality, you will need to escape those characters with a backslash.

So I think this should work:

cross-env DEBUG=myapp\:* nodemon

[mariotacke]

I guess I should've RTFM. Thanks for the clarification.

[kentcdodds]

Hey friends! Could you test out cross-env@5.0.0-beta.0? Let us know whether it works in #108

Check out the docs, specifically this bit. Thanks!