Skip to content

Commit

Permalink
Fix arguments with parenthesis causing an error on Windows
Browse files Browse the repository at this point in the history 
Because %* is expanded before anything, any arguments with parenthesis
will make the .cmd fail with the following message: \"" was unexpected
at this time

This might actually be a security vulnerability because with the rightly
crafted arguments, one would possibly execute arbirtrary commands, even
if the arguments were properly escaped.  Read
https://gist.github.com/satazor/4e21543e5cd380032c2b6b38c3700223 for a
more detailed explanation.

To solve this, %* was moved out of the condition.

PR-URL: #26
Credit: @satazor
Close: #26
Reviewed-by: @isaacs
  • Loading branch information
@satazor @isaacs
satazor authored and isaacs committed Aug 12, 2019
1 parent 2d277f8 commit adaf20b
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 67 deletions.
30 changes: 18 additions & 12 deletions index.js
View file
Expand Up @@ -96,28 +96,34 @@ function writeShim_ (from, to, prog, args, variables, cb) {
shTarget = "\"$basedir/" + shTarget + "\""
}

// @SETLOCAL
//
// @IF EXIST "%~dp0\node.exe" (
// "%~dp0\node.exe" "%~dp0\.\node_modules\npm\bin\npm-cli.js" %*
// @SET "_prog=%~dp0\node.exe"
// ) ELSE (
// SETLOCAL
// SET PATHEXT=%PATHEXT:;.JS;=;%
// node "%~dp0\.\node_modules\npm\bin\npm-cli.js" %*
// @SET "_prog=node"
// @SET PATHEXT=%PATHEXT:;.JS;=;%
// )
//
// "%_prog%" "%~dp0\.\node_modules\npm\bin\npm-cli.js" %*
// @ENDLOCAL
var cmd
if (longProg) {
shLongProg = shLongProg.trim();
args = args.trim();
var variableDeclarationsAsBatch = toBatchSyntax.convertToSetCommands(variables) || "";
cmd = ((variableDeclarationsAsBatch.length > 0) ? ("@SETLOCAL\r\n"
+ variableDeclarationsAsBatch) : "")
var variableDeclarationsAsBatch = toBatchSyntax.convertToSetCommands(variables)
cmd = "@SETLOCAL\r\n"
+ variableDeclarationsAsBatch
+ "\r\n"
+ "@IF EXIST " + longProg + " (\r\n"
+ " " + longProg + " " + args + " " + target + " %*\r\n"
+ " @SET \"_prog=" + longProg.replace(/(^")|("$)/g, '') + "\"\r\n"
+ ") ELSE (\r\n"
+ " @SETLOCAL\r\n"
+ " @SET \"_prog=" + prog.replace(/(^")|("$)/g, '') + "\"\r\n"
+ " @SET PATHEXT=%PATHEXT:;.JS;=;%\r\n"
+ " " + prog + " " + args + " " + target + " %*\r\n"
+ ")"
+ ((variableDeclarationsAsBatch.length > 0) ? "\r\n@ENDLOCAL" : "")
+ ")\r\n"
+ "\r\n"
+ "\"%_prog%\" " + args + " " + target + " %*\r\n"
+ '@ENDLOCAL\r\n'
} else {
cmd = "@" + prog + " " + args + " " + target + " %*\r\n"
}
Expand Down
122 changes: 67 additions & 55 deletions test/basic.js
View file
Expand Up @@ -33,34 +33,37 @@ test('env shebang', function (t) {
cmdShim(from, to, function(er) {
if (er)
throw er
console.error('%j', fs.readFileSync(to, 'utf8'))
console.error('%j', fs.readFileSync(to + '.cmd', 'utf8'))

t.equal(fs.readFileSync(to, 'utf8'),
"#!/bin/sh"+
"\nbasedir=$(dirname \"$(echo \"$0\" | sed -e 's,\\\\,/,g')\")"+
"\n"+
"\ncase `uname` in"+
"\n *CYGWIN*) basedir=`cygpath -w \"$basedir\"`;;"+
"\nesac"+
"\n"+
"\nif [ -x \"$basedir/node\" ]; then"+
"\n \"$basedir/node\" \"$basedir/from.env\" \"$@\""+
"\n ret=$?"+
"\nelse "+
"\n node \"$basedir/from.env\" \"$@\""+
"\n ret=$?"+
"\nfi"+
"\nexit $ret"+
"#!/bin/sh" +
"\nbasedir=$(dirname \"$(echo \"$0\" | sed -e 's,\\\\,/,g')\")" +
"\n" +
"\ncase `uname` in" +
"\n *CYGWIN*) basedir=`cygpath -w \"$basedir\"`;;" +
"\nesac" +
"\n" +
"\nif [ -x \"$basedir/node\" ]; then" +
"\n \"$basedir/node\" \"$basedir/from.env\" \"$@\"" +
"\n ret=$?" +
"\nelse " +
"\n node \"$basedir/from.env\" \"$@\"" +
"\n ret=$?" +
"\nfi" +
"\nexit $ret" +
"\n")
t.equal(fs.readFileSync(to + '.cmd', 'utf8'),
"@IF EXIST \"%~dp0\\node.exe\" (\r"+
"\n \"%~dp0\\node.exe\" \"%~dp0\\from.env\" %*\r"+
"\n) ELSE (\r"+
"\n @SETLOCAL\r"+
"\n @SET PATHEXT=%PATHEXT:;.JS;=;%\r"+
"\n node \"%~dp0\\from.env\" %*\r"+
"\n)")
"@SETLOCAL\r" +
"\n\r" +
"\n@IF EXIST \"%~dp0\\node.exe\" (\r" +
"\n @SET \"_prog=%~dp0\\node.exe\"\r" +
"\n) ELSE (\r" +
"\n @SET \"_prog=node\"\r" +
"\n @SET PATHEXT=%PATHEXT:;.JS;=;%\r" +
"\n)\r" +
"\n\r" +
"\n\"%_prog%\" \"%~dp0\\from.env\" %*\r" +
"\n@ENDLOCAL\r" +
"\n")
t.end()
})
})
Expand All @@ -71,8 +74,6 @@ test('env shebang with args', function (t) {
cmdShim(from, to, function(er) {
if (er)
throw er
console.error('%j', fs.readFileSync(to, 'utf8'))
console.error('%j', fs.readFileSync(to + '.cmd', 'utf8'))

t.equal(fs.readFileSync(to, 'utf8'),
"#!/bin/sh"+
Expand All @@ -92,13 +93,18 @@ test('env shebang with args', function (t) {
"\nexit $ret"+
"\n")
t.equal(fs.readFileSync(to + '.cmd', 'utf8'),
"@IF EXIST \"%~dp0\\node.exe\" (\r"+
"\n \"%~dp0\\node.exe\" --expose_gc \"%~dp0\\from.env.args\" %*\r"+
"\n) ELSE (\r"+
"\n @SETLOCAL\r"+
"\n @SET PATHEXT=%PATHEXT:;.JS;=;%\r"+
"\n node --expose_gc \"%~dp0\\from.env.args\" %*\r"+
"\n)")
"@SETLOCAL\r" +
"\n\r" +
"\n@IF EXIST \"%~dp0\\node.exe\" (\r" +
"\n @SET \"_prog=%~dp0\\node.exe\"\r" +
"\n) ELSE (\r" +
"\n @SET \"_prog=node\"\r" +
"\n @SET PATHEXT=%PATHEXT:;.JS;=;%\r" +
"\n)\r" +
"\n\r" +
"\n\"%_prog%\" --expose_gc \"%~dp0\\from.env.args\" %*\r" +
"\n@ENDLOCAL\r" +
"\n")
t.end()
})
})
Expand All @@ -109,8 +115,6 @@ test('env shebang with variables', function (t) {
cmdShim(from, to, function(er) {
if (er)
throw er
console.error('%j', fs.readFileSync(to, 'utf8'))
console.error('%j', fs.readFileSync(to + '.cmd', 'utf8'))

t.equal(fs.readFileSync(to, 'utf8'),
"#!/bin/sh"+
Expand All @@ -132,14 +136,16 @@ test('env shebang with variables', function (t) {
t.equal(fs.readFileSync(to + '.cmd', 'utf8'),
"@SETLOCAL\r"+
"\n@SET NODE_PATH=./lib:%NODE_PATH%\r"+
"\n\r" +
"\n@IF EXIST \"%~dp0\\node.exe\" (\r"+
"\n \"%~dp0\\node.exe\" \"%~dp0\\from.env.variables\" %*\r"+
"\n @SET \"_prog=%~dp0\\node.exe\"\r" +
"\n) ELSE (\r"+
"\n @SETLOCAL\r" +
"\n @SET \"_prog=node\"\r"+
"\n @SET PATHEXT=%PATHEXT:;.JS;=;%\r" +
"\n node \"%~dp0\\from.env.variables\" %*\r"+
"\n)\r"+
"\n@ENDLOCAL")
"\n\r"+
"\n\"%_prog%\" \"%~dp0\\from.env.variables\" %*\r"+
"\n@ENDLOCAL\r\n")
t.end()
})
})
Expand All @@ -150,8 +156,6 @@ test('explicit shebang', function (t) {
cmdShim(from, to, function(er) {
if (er)
throw er
console.error('%j', fs.readFileSync(to, 'utf8'))
console.error('%j', fs.readFileSync(to + '.cmd', 'utf8'))

t.equal(fs.readFileSync(to, 'utf8'),
"#!/bin/sh" +
Expand All @@ -172,13 +176,18 @@ test('explicit shebang', function (t) {
"\n")

t.equal(fs.readFileSync(to + '.cmd', 'utf8'),
"@IF EXIST \"%~dp0\\/usr/bin/sh.exe\" (\r" +
"\n \"%~dp0\\/usr/bin/sh.exe\" \"%~dp0\\from.sh\" %*\r" +
"@SETLOCAL\r" +
"\n\r" +
"\n@IF EXIST \"%~dp0\\/usr/bin/sh.exe\" (\r" +
"\n @SET \"_prog=%~dp0\\/usr/bin/sh.exe\"\r" +
"\n) ELSE (\r" +
"\n @SETLOCAL\r"+
"\n @SET PATHEXT=%PATHEXT:;.JS;=;%\r"+
"\n /usr/bin/sh \"%~dp0\\from.sh\" %*\r" +
"\n)")
"\n @SET \"_prog=/usr/bin/sh\"\r" +
"\n @SET PATHEXT=%PATHEXT:;.JS;=;%\r" +
"\n)\r" +
"\n\r" +
"\n\"%_prog%\" \"%~dp0\\from.sh\" %*\r" +
"\n@ENDLOCAL\r" +
"\n")
t.end()
})
})
Expand All @@ -189,8 +198,6 @@ test('explicit shebang with args', function (t) {
cmdShim(from, to, function(er) {
if (er)
throw er
console.error('%j', fs.readFileSync(to, 'utf8'))
console.error('%j', fs.readFileSync(to + '.cmd', 'utf8'))

t.equal(fs.readFileSync(to, 'utf8'),
"#!/bin/sh" +
Expand All @@ -211,13 +218,18 @@ test('explicit shebang with args', function (t) {
"\n")

t.equal(fs.readFileSync(to + '.cmd', 'utf8'),
"@IF EXIST \"%~dp0\\/usr/bin/sh.exe\" (\r" +
"\n \"%~dp0\\/usr/bin/sh.exe\" -x \"%~dp0\\from.sh.args\" %*\r" +
"@SETLOCAL\r" +
"\n\r" +
"\n@IF EXIST \"%~dp0\\/usr/bin/sh.exe\" (\r" +
"\n @SET \"_prog=%~dp0\\/usr/bin/sh.exe\"\r" +
"\n) ELSE (\r" +
"\n @SETLOCAL\r"+
"\n @SET PATHEXT=%PATHEXT:;.JS;=;%\r"+
"\n /usr/bin/sh -x \"%~dp0\\from.sh.args\" %*\r" +
"\n)")
"\n @SET \"_prog=/usr/bin/sh\"\r" +
"\n @SET PATHEXT=%PATHEXT:;.JS;=;%\r" +
"\n)\r" +
"\n\r" +
"\n\"%_prog%\" -x \"%~dp0\\from.sh.args\" %*\r" +
"\n@ENDLOCAL\r" +
"\n")
t.end()
})
})

0 comments on commit adaf20b

Please sign in to comment.