feat: write script and args to temp file and run that

[nlf]

1. we now properly escape strings for shells instead of using the horribly incorrect JSON.stringify() approach
2. rather than trying to build a string to pass to the shell that needs to be (in windows) double escaped, we can instead write the script and the singly escaped arguments to a file and run that file

i've done my best in spelunking through open issues and verifying this approach fixes our reported issues, though it's very likely there are more than these:

closes#31
closes#60
closesnpm/cli#3067
closesnpm/cli#3337
closesnpm/cli#3600
closesnpm/cli#3680
closesnpm/cli#4873
closesnpm/cli#4968
closesnpm/cli#5004

[wraithgar]

Just wanna be sure it's intentional that we're only looking for certain whitespace characters, and not using \s. Will approve assuming this is intentional.

[nlf]

it was meant to be a list of each individual character that causes us to need single quotes to avoid expansion/keep the argument in one piece, the switch could likely be made but i felt like this was more explicit and clear

[rhendric]

Did you test this with scripts that call executables from dependency packages? As we've discussed in the past, scripts that call executables like node require a different amount of argument escaping from scripts that call batch-file wrappers like what npm creates (used to create?) for, e.g., rimraf. I suspect putting the arguments into another batch file doesn't change this requirement, but I can't test that right this moment and I'm wondering if you did.

I still stand ready to assist with doing this the right way. The first step is still merging npm/exec#15, unless your plans have changed.

[nlf]

Did you test this with scripts that call executables from dependency packages?

> npx node@16 -e 'console.log("hello^&")'
hello&
> npx cowsay "hello^&"
_________
< hello& >
---------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

seems to be working right?

[rhendric]

Literal carets are the problem character here, and the problems sometimes don't show up until you try lots of them. Try npx cowsay "hello^^^^^^" and see if you get the same results as cowsay "hello^^^^^^" after installing cowsay. I know of trickier edge cases involving pipes and more advanced shell syntax, but if you pass that test I expect most users would consider the result good enough.

[nlf]

oh interesting..

> npx cowsay "hello^^^^^^"
__________
< hello^^^ >
----------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||
> cowsay "hello^^^^^^"
_____________
< hello^^^^^^ >
-------------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

so you're right, we do still have the problem with shim scripts. thank you for the reminder!

the solution is to double up the escaping if the first thing we're calling is a batch file, right? i think it's likely we're not going to go down the road of re-escaping the script as defined in the package.json, but we can certainly grab the initial command and use that to trigger double escaping if we need to

[rhendric]

That'd be an improvement over this patch (which is already an improvement over the status quo), and quite possibly ‘good enough’. It is possible to write scripts for which this would do the wrong thing: consider "rimraf build & rimraf tmp & node regenerate.js"; the relevant command is node, which is not a batch file, and double escaping would be incorrect because rimraf is one. Another example: "node print-data.js | node this-script-accepts-args.js"; neither command in this pipeline is a batch file but any arguments to the second one need double escaping because of the pipe (and if that command were a batch file, they would need quadruple escaping!).

If you want to enable correct escaping for scripts like the above, a true solution requires parsing the syntax of the script, not to add escape characters to it necessarily, but to determine the level of escaping that terminal arguments would need. If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut.

[nlf]

If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut

I think this is likely to be the case. We certainly get few enough complaints about extremely complicated scripts for me to not feel like this further parsing is necessary. At least not right now.

I'll have a pull request up that double escapes if the first command in the script resolves to a .cmd file in a few minutes

[nlf]

i pushed #80 which i think gets us "close enough" for now at least. let me know your thoughts, if you have a moment to review. i really appreciate your continued eyes on this problem. i think if this series of changes isn't good enough, then we'll be circling back to adopting and modifying puka, but i'm really hopeful this is enough for the 99%

[nlf]

1. we now properly escape strings for shells instead of using the horribly incorrect JSON.stringify() approach
2. rather than trying to build a string to pass to the shell that needs to be (in windows) double escaped, we can instead write the script and the singly escaped arguments to a file and run that file

i've done my best in spelunking through open issues and verifying this approach fixes our reported issues, though it's very likely there are more than these:

closes#31
closes#60
closesnpm/cli#3067
closesnpm/cli#3337
closesnpm/cli#3600
closesnpm/cli#3680
closesnpm/cli#4873
closesnpm/cli#4968
closesnpm/cli#5004

[wraithgar]

Just wanna be sure it's intentional that we're only looking for certain whitespace characters, and not using \s. Will approve assuming this is intentional.

[nlf]

it was meant to be a list of each individual character that causes us to need single quotes to avoid expansion/keep the argument in one piece, the switch could likely be made but i felt like this was more explicit and clear

[rhendric]

Did you test this with scripts that call executables from dependency packages? As we've discussed in the past, scripts that call executables like node require a different amount of argument escaping from scripts that call batch-file wrappers like what npm creates (used to create?) for, e.g., rimraf. I suspect putting the arguments into another batch file doesn't change this requirement, but I can't test that right this moment and I'm wondering if you did.

I still stand ready to assist with doing this the right way. The first step is still merging npm/exec#15, unless your plans have changed.

[nlf]

Did you test this with scripts that call executables from dependency packages?

> npx node@16 -e 'console.log("hello^&")'
hello&
> npx cowsay "hello^&"
_________
< hello& >
---------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

seems to be working right?

[rhendric]

Literal carets are the problem character here, and the problems sometimes don't show up until you try lots of them. Try npx cowsay "hello^^^^^^" and see if you get the same results as cowsay "hello^^^^^^" after installing cowsay. I know of trickier edge cases involving pipes and more advanced shell syntax, but if you pass that test I expect most users would consider the result good enough.

[nlf]

oh interesting..

> npx cowsay "hello^^^^^^"
__________
< hello^^^ >
----------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||
> cowsay "hello^^^^^^"
_____________
< hello^^^^^^ >
-------------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

so you're right, we do still have the problem with shim scripts. thank you for the reminder!

the solution is to double up the escaping if the first thing we're calling is a batch file, right? i think it's likely we're not going to go down the road of re-escaping the script as defined in the package.json, but we can certainly grab the initial command and use that to trigger double escaping if we need to

[rhendric]

That'd be an improvement over this patch (which is already an improvement over the status quo), and quite possibly ‘good enough’. It is possible to write scripts for which this would do the wrong thing: consider "rimraf build & rimraf tmp & node regenerate.js"; the relevant command is node, which is not a batch file, and double escaping would be incorrect because rimraf is one. Another example: "node print-data.js | node this-script-accepts-args.js"; neither command in this pipeline is a batch file but any arguments to the second one need double escaping because of the pipe (and if that command were a batch file, they would need quadruple escaping!).

If you want to enable correct escaping for scripts like the above, a true solution requires parsing the syntax of the script, not to add escape characters to it necessarily, but to determine the level of escaping that terminal arguments would need. If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut.

[nlf]

If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut

I think this is likely to be the case. We certainly get few enough complaints about extremely complicated scripts for me to not feel like this further parsing is necessary. At least not right now.

I'll have a pull request up that double escapes if the first command in the script resolves to a .cmd file in a few minutes

[nlf]

i pushed #80 which i think gets us "close enough" for now at least. let me know your thoughts, if you have a moment to review. i really appreciate your continued eyes on this problem. i think if this series of changes isn't good enough, then we'll be circling back to adopting and modifying puka, but i'm really hopeful this is enough for the 99%

[nlf]

1. we now properly escape strings for shells instead of using the horribly incorrect JSON.stringify() approach
2. rather than trying to build a string to pass to the shell that needs to be (in windows) double escaped, we can instead write the script and the singly escaped arguments to a file and run that file

i've done my best in spelunking through open issues and verifying this approach fixes our reported issues, though it's very likely there are more than these:

closes#31
closes#60
closesnpm/cli#3067
closesnpm/cli#3337
closesnpm/cli#3600
closesnpm/cli#3680
closesnpm/cli#4873
closesnpm/cli#4968
closesnpm/cli#5004

[wraithgar]

Just wanna be sure it's intentional that we're only looking for certain whitespace characters, and not using \s. Will approve assuming this is intentional.

[nlf]

it was meant to be a list of each individual character that causes us to need single quotes to avoid expansion/keep the argument in one piece, the switch could likely be made but i felt like this was more explicit and clear

[rhendric]

Did you test this with scripts that call executables from dependency packages? As we've discussed in the past, scripts that call executables like node require a different amount of argument escaping from scripts that call batch-file wrappers like what npm creates (used to create?) for, e.g., rimraf. I suspect putting the arguments into another batch file doesn't change this requirement, but I can't test that right this moment and I'm wondering if you did.

I still stand ready to assist with doing this the right way. The first step is still merging npm/exec#15, unless your plans have changed.

[nlf]

Did you test this with scripts that call executables from dependency packages?

> npx node@16 -e 'console.log("hello^&")'
hello&
> npx cowsay "hello^&"
_________
< hello& >
---------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

seems to be working right?

[rhendric]

Literal carets are the problem character here, and the problems sometimes don't show up until you try lots of them. Try npx cowsay "hello^^^^^^" and see if you get the same results as cowsay "hello^^^^^^" after installing cowsay. I know of trickier edge cases involving pipes and more advanced shell syntax, but if you pass that test I expect most users would consider the result good enough.

[nlf]

oh interesting..

> npx cowsay "hello^^^^^^"
__________
< hello^^^ >
----------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||
> cowsay "hello^^^^^^"
_____________
< hello^^^^^^ >
-------------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

so you're right, we do still have the problem with shim scripts. thank you for the reminder!

the solution is to double up the escaping if the first thing we're calling is a batch file, right? i think it's likely we're not going to go down the road of re-escaping the script as defined in the package.json, but we can certainly grab the initial command and use that to trigger double escaping if we need to

[rhendric]

That'd be an improvement over this patch (which is already an improvement over the status quo), and quite possibly ‘good enough’. It is possible to write scripts for which this would do the wrong thing: consider "rimraf build & rimraf tmp & node regenerate.js"; the relevant command is node, which is not a batch file, and double escaping would be incorrect because rimraf is one. Another example: "node print-data.js | node this-script-accepts-args.js"; neither command in this pipeline is a batch file but any arguments to the second one need double escaping because of the pipe (and if that command were a batch file, they would need quadruple escaping!).

If you want to enable correct escaping for scripts like the above, a true solution requires parsing the syntax of the script, not to add escape characters to it necessarily, but to determine the level of escaping that terminal arguments would need. If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut.

[nlf]

If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut

I think this is likely to be the case. We certainly get few enough complaints about extremely complicated scripts for me to not feel like this further parsing is necessary. At least not right now.

I'll have a pull request up that double escapes if the first command in the script resolves to a .cmd file in a few minutes

[nlf]

i pushed #80 which i think gets us "close enough" for now at least. let me know your thoughts, if you have a moment to review. i really appreciate your continued eyes on this problem. i think if this series of changes isn't good enough, then we'll be circling back to adopting and modifying puka, but i'm really hopeful this is enough for the 99%

[nlf]

1. we now properly escape strings for shells instead of using the horribly incorrect JSON.stringify() approach
2. rather than trying to build a string to pass to the shell that needs to be (in windows) double escaped, we can instead write the script and the singly escaped arguments to a file and run that file

i've done my best in spelunking through open issues and verifying this approach fixes our reported issues, though it's very likely there are more than these:

closes#31
closes#60
closesnpm/cli#3067
closesnpm/cli#3337
closesnpm/cli#3600
closesnpm/cli#3680
closesnpm/cli#4873
closesnpm/cli#4968
closesnpm/cli#5004

[wraithgar]

Just wanna be sure it's intentional that we're only looking for certain whitespace characters, and not using \s. Will approve assuming this is intentional.

[nlf]

it was meant to be a list of each individual character that causes us to need single quotes to avoid expansion/keep the argument in one piece, the switch could likely be made but i felt like this was more explicit and clear

[rhendric]

Did you test this with scripts that call executables from dependency packages? As we've discussed in the past, scripts that call executables like node require a different amount of argument escaping from scripts that call batch-file wrappers like what npm creates (used to create?) for, e.g., rimraf. I suspect putting the arguments into another batch file doesn't change this requirement, but I can't test that right this moment and I'm wondering if you did.

I still stand ready to assist with doing this the right way. The first step is still merging npm/exec#15, unless your plans have changed.

[nlf]

Did you test this with scripts that call executables from dependency packages?

> npx node@16 -e 'console.log("hello^&")'
hello&
> npx cowsay "hello^&"
_________
< hello& >
---------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

seems to be working right?

[rhendric]

Literal carets are the problem character here, and the problems sometimes don't show up until you try lots of them. Try npx cowsay "hello^^^^^^" and see if you get the same results as cowsay "hello^^^^^^" after installing cowsay. I know of trickier edge cases involving pipes and more advanced shell syntax, but if you pass that test I expect most users would consider the result good enough.

[nlf]

oh interesting..

> npx cowsay "hello^^^^^^"
__________
< hello^^^ >
----------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||
> cowsay "hello^^^^^^"
_____________
< hello^^^^^^ >
-------------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

so you're right, we do still have the problem with shim scripts. thank you for the reminder!

the solution is to double up the escaping if the first thing we're calling is a batch file, right? i think it's likely we're not going to go down the road of re-escaping the script as defined in the package.json, but we can certainly grab the initial command and use that to trigger double escaping if we need to

[rhendric]

That'd be an improvement over this patch (which is already an improvement over the status quo), and quite possibly ‘good enough’. It is possible to write scripts for which this would do the wrong thing: consider "rimraf build & rimraf tmp & node regenerate.js"; the relevant command is node, which is not a batch file, and double escaping would be incorrect because rimraf is one. Another example: "node print-data.js | node this-script-accepts-args.js"; neither command in this pipeline is a batch file but any arguments to the second one need double escaping because of the pipe (and if that command were a batch file, they would need quadruple escaping!).

If you want to enable correct escaping for scripts like the above, a true solution requires parsing the syntax of the script, not to add escape characters to it necessarily, but to determine the level of escaping that terminal arguments would need. If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut.

[nlf]

If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut

I think this is likely to be the case. We certainly get few enough complaints about extremely complicated scripts for me to not feel like this further parsing is necessary. At least not right now.

I'll have a pull request up that double escapes if the first command in the script resolves to a .cmd file in a few minutes

[nlf]

i pushed #80 which i think gets us "close enough" for now at least. let me know your thoughts, if you have a moment to review. i really appreciate your continued eyes on this problem. i think if this series of changes isn't good enough, then we'll be circling back to adopting and modifying puka, but i'm really hopeful this is enough for the 99%

[nlf]

1. we now properly escape strings for shells instead of using the horribly incorrect JSON.stringify() approach
2. rather than trying to build a string to pass to the shell that needs to be (in windows) double escaped, we can instead write the script and the singly escaped arguments to a file and run that file

i've done my best in spelunking through open issues and verifying this approach fixes our reported issues, though it's very likely there are more than these:

closes#31
closes#60
closesnpm/cli#3067
closesnpm/cli#3337
closesnpm/cli#3600
closesnpm/cli#3680
closesnpm/cli#4873
closesnpm/cli#4968
closesnpm/cli#5004

[wraithgar]

Just wanna be sure it's intentional that we're only looking for certain whitespace characters, and not using \s. Will approve assuming this is intentional.

[nlf]

it was meant to be a list of each individual character that causes us to need single quotes to avoid expansion/keep the argument in one piece, the switch could likely be made but i felt like this was more explicit and clear

[rhendric]

Did you test this with scripts that call executables from dependency packages? As we've discussed in the past, scripts that call executables like node require a different amount of argument escaping from scripts that call batch-file wrappers like what npm creates (used to create?) for, e.g., rimraf. I suspect putting the arguments into another batch file doesn't change this requirement, but I can't test that right this moment and I'm wondering if you did.

I still stand ready to assist with doing this the right way. The first step is still merging npm/exec#15, unless your plans have changed.

[nlf]

Did you test this with scripts that call executables from dependency packages?

> npx node@16 -e 'console.log("hello^&")'
hello&
> npx cowsay "hello^&"
_________
< hello& >
---------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

seems to be working right?

[rhendric]

Literal carets are the problem character here, and the problems sometimes don't show up until you try lots of them. Try npx cowsay "hello^^^^^^" and see if you get the same results as cowsay "hello^^^^^^" after installing cowsay. I know of trickier edge cases involving pipes and more advanced shell syntax, but if you pass that test I expect most users would consider the result good enough.

[nlf]

oh interesting..

> npx cowsay "hello^^^^^^"
__________
< hello^^^ >
----------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||
> cowsay "hello^^^^^^"
_____________
< hello^^^^^^ >
-------------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

so you're right, we do still have the problem with shim scripts. thank you for the reminder!

the solution is to double up the escaping if the first thing we're calling is a batch file, right? i think it's likely we're not going to go down the road of re-escaping the script as defined in the package.json, but we can certainly grab the initial command and use that to trigger double escaping if we need to

[rhendric]

That'd be an improvement over this patch (which is already an improvement over the status quo), and quite possibly ‘good enough’. It is possible to write scripts for which this would do the wrong thing: consider "rimraf build & rimraf tmp & node regenerate.js"; the relevant command is node, which is not a batch file, and double escaping would be incorrect because rimraf is one. Another example: "node print-data.js | node this-script-accepts-args.js"; neither command in this pipeline is a batch file but any arguments to the second one need double escaping because of the pipe (and if that command were a batch file, they would need quadruple escaping!).

If you want to enable correct escaping for scripts like the above, a true solution requires parsing the syntax of the script, not to add escape characters to it necessarily, but to determine the level of escaping that terminal arguments would need. If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut.

[nlf]

If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut

I think this is likely to be the case. We certainly get few enough complaints about extremely complicated scripts for me to not feel like this further parsing is necessary. At least not right now.

I'll have a pull request up that double escapes if the first command in the script resolves to a .cmd file in a few minutes

[nlf]

i pushed #80 which i think gets us "close enough" for now at least. let me know your thoughts, if you have a moment to review. i really appreciate your continued eyes on this problem. i think if this series of changes isn't good enough, then we'll be circling back to adopting and modifying puka, but i'm really hopeful this is enough for the 99%

[nlf]

1. we now properly escape strings for shells instead of using the horribly incorrect JSON.stringify() approach
2. rather than trying to build a string to pass to the shell that needs to be (in windows) double escaped, we can instead write the script and the singly escaped arguments to a file and run that file

i've done my best in spelunking through open issues and verifying this approach fixes our reported issues, though it's very likely there are more than these:

closes#31
closes#60
closesnpm/cli#3067
closesnpm/cli#3337
closesnpm/cli#3600
closesnpm/cli#3680
closesnpm/cli#4873
closesnpm/cli#4968
closesnpm/cli#5004

[wraithgar]

Just wanna be sure it's intentional that we're only looking for certain whitespace characters, and not using \s. Will approve assuming this is intentional.

[nlf]

it was meant to be a list of each individual character that causes us to need single quotes to avoid expansion/keep the argument in one piece, the switch could likely be made but i felt like this was more explicit and clear

[rhendric]

Did you test this with scripts that call executables from dependency packages? As we've discussed in the past, scripts that call executables like node require a different amount of argument escaping from scripts that call batch-file wrappers like what npm creates (used to create?) for, e.g., rimraf. I suspect putting the arguments into another batch file doesn't change this requirement, but I can't test that right this moment and I'm wondering if you did.

I still stand ready to assist with doing this the right way. The first step is still merging npm/exec#15, unless your plans have changed.

[nlf]

Did you test this with scripts that call executables from dependency packages?

> npx node@16 -e 'console.log("hello^&")'
hello&
> npx cowsay "hello^&"
_________
< hello& >
---------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

seems to be working right?

[rhendric]

Literal carets are the problem character here, and the problems sometimes don't show up until you try lots of them. Try npx cowsay "hello^^^^^^" and see if you get the same results as cowsay "hello^^^^^^" after installing cowsay. I know of trickier edge cases involving pipes and more advanced shell syntax, but if you pass that test I expect most users would consider the result good enough.

[nlf]

oh interesting..

> npx cowsay "hello^^^^^^"
__________
< hello^^^ >
----------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||
> cowsay "hello^^^^^^"
_____________
< hello^^^^^^ >
-------------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

so you're right, we do still have the problem with shim scripts. thank you for the reminder!

the solution is to double up the escaping if the first thing we're calling is a batch file, right? i think it's likely we're not going to go down the road of re-escaping the script as defined in the package.json, but we can certainly grab the initial command and use that to trigger double escaping if we need to

[rhendric]

That'd be an improvement over this patch (which is already an improvement over the status quo), and quite possibly ‘good enough’. It is possible to write scripts for which this would do the wrong thing: consider "rimraf build & rimraf tmp & node regenerate.js"; the relevant command is node, which is not a batch file, and double escaping would be incorrect because rimraf is one. Another example: "node print-data.js | node this-script-accepts-args.js"; neither command in this pipeline is a batch file but any arguments to the second one need double escaping because of the pipe (and if that command were a batch file, they would need quadruple escaping!).

If you want to enable correct escaping for scripts like the above, a true solution requires parsing the syntax of the script, not to add escape characters to it necessarily, but to determine the level of escaping that terminal arguments would need. If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut.

[nlf]

If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut

I think this is likely to be the case. We certainly get few enough complaints about extremely complicated scripts for me to not feel like this further parsing is necessary. At least not right now.

I'll have a pull request up that double escapes if the first command in the script resolves to a .cmd file in a few minutes

[nlf]

i pushed #80 which i think gets us "close enough" for now at least. let me know your thoughts, if you have a moment to review. i really appreciate your continued eyes on this problem. i think if this series of changes isn't good enough, then we'll be circling back to adopting and modifying puka, but i'm really hopeful this is enough for the 99%

[nlf]

1. we now properly escape strings for shells instead of using the horribly incorrect JSON.stringify() approach
2. rather than trying to build a string to pass to the shell that needs to be (in windows) double escaped, we can instead write the script and the singly escaped arguments to a file and run that file

i've done my best in spelunking through open issues and verifying this approach fixes our reported issues, though it's very likely there are more than these:

closes#31
closes#60
closesnpm/cli#3067
closesnpm/cli#3337
closesnpm/cli#3600
closesnpm/cli#3680
closesnpm/cli#4873
closesnpm/cli#4968
closesnpm/cli#5004

[wraithgar]

Just wanna be sure it's intentional that we're only looking for certain whitespace characters, and not using \s. Will approve assuming this is intentional.

[nlf]

it was meant to be a list of each individual character that causes us to need single quotes to avoid expansion/keep the argument in one piece, the switch could likely be made but i felt like this was more explicit and clear

[rhendric]

Did you test this with scripts that call executables from dependency packages? As we've discussed in the past, scripts that call executables like node require a different amount of argument escaping from scripts that call batch-file wrappers like what npm creates (used to create?) for, e.g., rimraf. I suspect putting the arguments into another batch file doesn't change this requirement, but I can't test that right this moment and I'm wondering if you did.

I still stand ready to assist with doing this the right way. The first step is still merging npm/exec#15, unless your plans have changed.

[nlf]

Did you test this with scripts that call executables from dependency packages?

> npx node@16 -e 'console.log("hello^&")'
hello&
> npx cowsay "hello^&"
_________
< hello& >
---------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

seems to be working right?

[rhendric]

Literal carets are the problem character here, and the problems sometimes don't show up until you try lots of them. Try npx cowsay "hello^^^^^^" and see if you get the same results as cowsay "hello^^^^^^" after installing cowsay. I know of trickier edge cases involving pipes and more advanced shell syntax, but if you pass that test I expect most users would consider the result good enough.

[nlf]

oh interesting..

> npx cowsay "hello^^^^^^"
__________
< hello^^^ >
----------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||
> cowsay "hello^^^^^^"
_____________
< hello^^^^^^ >
-------------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

so you're right, we do still have the problem with shim scripts. thank you for the reminder!

the solution is to double up the escaping if the first thing we're calling is a batch file, right? i think it's likely we're not going to go down the road of re-escaping the script as defined in the package.json, but we can certainly grab the initial command and use that to trigger double escaping if we need to

[rhendric]

That'd be an improvement over this patch (which is already an improvement over the status quo), and quite possibly ‘good enough’. It is possible to write scripts for which this would do the wrong thing: consider "rimraf build & rimraf tmp & node regenerate.js"; the relevant command is node, which is not a batch file, and double escaping would be incorrect because rimraf is one. Another example: "node print-data.js | node this-script-accepts-args.js"; neither command in this pipeline is a batch file but any arguments to the second one need double escaping because of the pipe (and if that command were a batch file, they would need quadruple escaping!).

If you want to enable correct escaping for scripts like the above, a true solution requires parsing the syntax of the script, not to add escape characters to it necessarily, but to determine the level of escaping that terminal arguments would need. If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut.

[nlf]

If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut

I think this is likely to be the case. We certainly get few enough complaints about extremely complicated scripts for me to not feel like this further parsing is necessary. At least not right now.

I'll have a pull request up that double escapes if the first command in the script resolves to a .cmd file in a few minutes

[nlf]

i pushed #80 which i think gets us "close enough" for now at least. let me know your thoughts, if you have a moment to review. i really appreciate your continued eyes on this problem. i think if this series of changes isn't good enough, then we'll be circling back to adopting and modifying puka, but i'm really hopeful this is enough for the 99%

[nlf]

1. we now properly escape strings for shells instead of using the horribly incorrect JSON.stringify() approach
2. rather than trying to build a string to pass to the shell that needs to be (in windows) double escaped, we can instead write the script and the singly escaped arguments to a file and run that file

i've done my best in spelunking through open issues and verifying this approach fixes our reported issues, though it's very likely there are more than these:

closes#31
closes#60
closesnpm/cli#3067
closesnpm/cli#3337
closesnpm/cli#3600
closesnpm/cli#3680
closesnpm/cli#4873
closesnpm/cli#4968
closesnpm/cli#5004

[wraithgar]

Just wanna be sure it's intentional that we're only looking for certain whitespace characters, and not using \s. Will approve assuming this is intentional.

[nlf]

it was meant to be a list of each individual character that causes us to need single quotes to avoid expansion/keep the argument in one piece, the switch could likely be made but i felt like this was more explicit and clear

[rhendric]

Did you test this with scripts that call executables from dependency packages? As we've discussed in the past, scripts that call executables like node require a different amount of argument escaping from scripts that call batch-file wrappers like what npm creates (used to create?) for, e.g., rimraf. I suspect putting the arguments into another batch file doesn't change this requirement, but I can't test that right this moment and I'm wondering if you did.

I still stand ready to assist with doing this the right way. The first step is still merging npm/exec#15, unless your plans have changed.

[nlf]

Did you test this with scripts that call executables from dependency packages?

> npx node@16 -e 'console.log("hello^&")'
hello&
> npx cowsay "hello^&"
_________
< hello& >
---------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

seems to be working right?

[rhendric]

Literal carets are the problem character here, and the problems sometimes don't show up until you try lots of them. Try npx cowsay "hello^^^^^^" and see if you get the same results as cowsay "hello^^^^^^" after installing cowsay. I know of trickier edge cases involving pipes and more advanced shell syntax, but if you pass that test I expect most users would consider the result good enough.

[nlf]

oh interesting..

> npx cowsay "hello^^^^^^"
__________
< hello^^^ >
----------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||
> cowsay "hello^^^^^^"
_____________
< hello^^^^^^ >
-------------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||

so you're right, we do still have the problem with shim scripts. thank you for the reminder!

the solution is to double up the escaping if the first thing we're calling is a batch file, right? i think it's likely we're not going to go down the road of re-escaping the script as defined in the package.json, but we can certainly grab the initial command and use that to trigger double escaping if we need to

[rhendric]

That'd be an improvement over this patch (which is already an improvement over the status quo), and quite possibly ‘good enough’. It is possible to write scripts for which this would do the wrong thing: consider "rimraf build & rimraf tmp & node regenerate.js"; the relevant command is node, which is not a batch file, and double escaping would be incorrect because rimraf is one. Another example: "node print-data.js | node this-script-accepts-args.js"; neither command in this pipeline is a batch file but any arguments to the second one need double escaping because of the pipe (and if that command were a batch file, they would need quadruple escaping!).

If you want to enable correct escaping for scripts like the above, a true solution requires parsing the syntax of the script, not to add escape characters to it necessarily, but to determine the level of escaping that terminal arguments would need. If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut.

[nlf]

If you think examples like the above are acceptably rare, then what you proposed is a reasonable shortcut

I think this is likely to be the case. We certainly get few enough complaints about extremely complicated scripts for me to not feel like this further parsing is necessary. At least not right now.

I'll have a pull request up that double escapes if the first command in the script resolves to a .cmd file in a few minutes

[nlf]

i pushed #80 which i think gets us "close enough" for now at least. let me know your thoughts, if you have a moment to review. i really appreciate your continued eyes on this problem. i think if this series of changes isn't good enough, then we'll be circling back to adopting and modifying puka, but i'm really hopeful this is enough for the 99%