From adaf20b7fa2c09c2111a2506c6a3e53ed0831f88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Cruz?= Date: Sat, 20 Jan 2018 22:00:09 +0000 Subject: [PATCH] Fix arguments with parenthesis causing an error on Windows 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: https://github.com/npm/cmd-shim/pull/26 Credit: @satazor Close: #26 Reviewed-by: @isaacs --- index.js | 30 ++++++++----- test/basic.js | 122 +++++++++++++++++++++++++++----------------------- 2 files changed, 85 insertions(+), 67 deletions(-) diff --git a/index.js b/index.js index e0c3e99..0949162 100644 --- a/index.js +++ b/index.js @@ -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" } diff --git a/test/basic.js b/test/basic.js index f28575e..8b8cdf0 100755 --- a/test/basic.js +++ b/test/basic.js @@ -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() }) }) @@ -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"+ @@ -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() }) }) @@ -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"+ @@ -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() }) }) @@ -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" + @@ -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() }) }) @@ -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" + @@ -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() }) })