Uh oh!
There was an error while loading. Please reload this page.
fix: correctly double escape when script runs a known .cmd file - #80
Conversation
bbc3947 to
b39f99bCompareb39f99b to
9f187dfCompare| if (doubleEscape) { | ||
| result = escape.cmd(result) | ||
| } |
There was a problem hiding this comment.
This is going to double up your backslash escapes too, which I believe you don't want. There are two escape mechanisms at work here: a low-level one that is part of the Windows command line conventions, and a higher-level one specific to cmd.exe. Only the latter should be applied twice in case of a batch file. (That means doubling up your carets, and possibly in your implementation doubling up your percents as well. When I implemented this in Puka, I escaped percents with carets just like any other character special to cmd.exe, so I don't have tests covering what happens when you try to escape percents by doubling them.)
(You could just import that escaping function as quoteForCmd from Puka's public exports, BTW, instead of rolling your own. No modifications necessary. If batch file, provide 1 as the third argument (this parameter admittedly is currently undocumented but it probably makes sense for me to make it public API). I'm not trying to pressure you into using my library out of pride, but my automated tests throw hundreds of randomly-generated strings that focus on all the edge cases I know about at that implementation and actually run the results through cmd.exe to make sure that the escaping is correct. I'm very confident in it. If I'm reviewing an implementation that does something different and doesn't have a similarly robust test suite behind it, I'm going to be a lot less confident I've caught all the problems in it.)
There was a problem hiding this comment.
This is going to double up your backslash escapes too, which I believe you don't want. There are two escape mechanisms at work here: a low-level one that is part of the Windows command line conventions, and a higher-level one specific to cmd.exe. Only the latter should be applied twice in case of a batch file.
ahhhhh! i understand. i did some hands on testing on my windows machine and adapted the code to not double up the backslashes, as you mentioned. i also refactored the tests and added a whole bunch more cases.
When I implemented this in Puka, I escaped percents with carets just like any other character special to cmd.exe
the % encoding to %% behavior is specific to batch files, which we're now using, so that's why we have differences there
my automated tests throw hundreds of randomly-generated strings that focus on all the edge cases I know about at that implementation and actually run the results through cmd.exe to make sure that the escaping is correct
I really liked this idea, so for now I at least made it so each of the expectations in the tests will spawn an actual process and ensures that the output matches the input. this will at least make it really easy to add regression tests
296d395 to
7052862Compare7052862 to
8c62010Compare| pathToInitial = initialCmd.toLowerCase() | ||
| } | ||
| const doubleEscape = pathToInitial.endsWith('.cmd') || pathToInitial.endsWith('.bat') |
There was a problem hiding this comment.
How certain are we this list is comprehensive?
There was a problem hiding this comment.
the default value of %PATHEXT% at least on my machine is .COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC
of these, .BAT and .CMD are the only two that run through cmd.exe so i feel pretty ok about it
wraithgar
left a comment
There was a problem hiding this comment.
Approved w/ one double check question.
as called out by @rhendric, i neglected to do the necessary checks to see if we need to double escape additional arguments being passed to a .cmd file
this change makes it so that we attempt to look up the full path that the command will resolve to, and if we can and that thing is a .cmd file, then we double escape the arguments we pass to it. if not, we single escape.
as noted in his comment this does not mean that all scripts will work perfectly, as there are many complex use cases that we will fail to support here. however this gets us significantly further along than we were and i believe addresses the vast majority of scripts in the wild.