diff --git a/src/node_task_runner.cc b/src/node_task_runner.cc index a719314d245718..4b7d541340a79d 100644 --- a/src/node_task_runner.cc +++ b/src/node_task_runner.cc @@ -6,15 +6,14 @@ namespace node::task_runner { #ifdef _WIN32 -static constexpr char bin_path[] = "\\node_modules\\.bin"; +static constexpr const char* bin_path = "\\node_modules\\.bin"; #else -static constexpr char bin_path[] = "/node_modules/.bin"; +static constexpr const char* bin_path = "/node_modules/.bin"; #endif // _WIN32 -ProcessRunner::ProcessRunner( - std::shared_ptr result, - std::string_view command, - const std::optional& positional_args) { +ProcessRunner::ProcessRunner(std::shared_ptr result, + std::string_view command, + const PositionalArgs& positional_args) { memset(&options_, 0, sizeof(uv_process_options_t)); // Get the current working directory. @@ -54,10 +53,6 @@ ProcessRunner::ProcessRunner( std::string command_str(command); - if (positional_args.has_value()) { - command_str += " " + EscapeShell(positional_args.value()); - } - // Set environment variables uv_env_item_t* env_items; int env_count; @@ -69,33 +64,45 @@ ProcessRunner::ProcessRunner( // ProcessRunner instance. for (int i = 0; i < env_count; i++) { std::string name = env_items[i].name; - std::string value = env_items[i].value; + auto value = env_items[i].value; #ifdef _WIN32 // We use comspec environment variable to find cmd.exe path on Windows // Example: 'C:\\Windows\\system32\\cmd.exe' // If we don't find it, we fallback to 'cmd.exe' for Windows - if (name.size() == 7 && StringEqualNoCaseN(name.c_str(), "comspec", 7)) { + if (StringEqualNoCase(name.c_str(), "comspec")) { file_ = value; } #endif // _WIN32 // Check if environment variable key is matching case-insensitive "path" - if (name.size() == 4 && StringEqualNoCaseN(name.c_str(), "path", 4)) { - value.insert(0, current_bin_path); + if (StringEqualNoCase(name.c_str(), "path")) { + env_vars_.push_back(name + "=" + current_bin_path + value); + } else { + // Environment variables should be in "KEY=value" format + env_vars_.push_back(name + "=" + value); } - - // Environment variables should be in "KEY=value" format - value.insert(0, name + "="); - env_vars_.push_back(value); } uv_os_free_environ(env_items, env_count); // Use the stored reference on the instance. options_.file = file_.c_str(); + // Add positional arguments to the command string. + // Note that each argument needs to be escaped. + if (!positional_args.empty()) { + for (const auto& arg : positional_args) { + command_str += " " + EscapeShell(arg); + } + } + #ifdef _WIN32 - if (file_.find("cmd.exe") != std::string::npos) { + // We check whether file_ ends with cmd.exe in a case-insensitive manner. + // C++20 provides ends_with, but we roll our own for compatibility. + const char* cmdexe = "cmd.exe"; + if (file_.size() >= strlen(cmdexe) && + StringEqualNoCase(cmdexe, + file_.c_str() + file_.size() - strlen(cmdexe))) { // If the file is cmd.exe, use the following command line arguments: // "/c" Carries out the command and exit. // "/d" Disables execution of AutoRun commands. @@ -104,6 +111,9 @@ ProcessRunner::ProcessRunner( command_args_ = { options_.file, "/d", "/s", "/c", "\"" + command_str + "\""}; } else { + // If the file is not cmd.exe, and it is unclear wich shell is being used, + // so assume -c is the correct syntax (Unix-like shells use -c for this + // purpose). command_args_ = {options_.file, "-c", command_str}; } #else @@ -126,12 +136,19 @@ ProcessRunner::ProcessRunner( } // EscapeShell escapes a string to be used as a command line argument. +// Under Windows, we follow: +// https://daviddeley.com/autohotkey/parameters/parameters.htm +// Elsewhere: // It replaces single quotes with "\\'" and double quotes with "\\\"". // It also removes excessive quote pairs and handles edge cases. -std::string EscapeShell(const std::string& input) { +std::string EscapeShell(const std::string_view input) { // If the input is an empty string, return a pair of quotes if (input.empty()) { +#ifdef _WIN32 + return "\"\""; +#else return "''"; +#endif } static const std::string_view forbidden_characters = @@ -140,21 +157,32 @@ std::string EscapeShell(const std::string& input) { // Check if input contains any forbidden characters // If it doesn't, return the input as is. if (input.find_first_of(forbidden_characters) == std::string::npos) { - return input; + return std::string(input); } - // Replace single quotes("'") with "\\'" - std::string escaped = std::regex_replace(input, std::regex("'"), "\\'"); + static const std::regex leadingQuotePairs("^(?:'')+(?!$)"); - // Wrap the result in single quotes +#ifdef _WIN32 + // Replace double quotes with single quotes and surround the string + // with double quotes for Windows. + std::string escaped = + std::regex_replace(std::string(input), std::regex("\""), "\"\""); + escaped = "\"" + escaped + "\""; + // Remove excessive quote pairs and handle edge cases + static const std::regex tripleSingleQuote("\\\\\"\"\""); + escaped = std::regex_replace(escaped, leadingQuotePairs, ""); + escaped = std::regex_replace(escaped, tripleSingleQuote, "\\\""); +#else + // Replace single quotes("'") with "\\'" and wrap the result + // in single quotes. + std::string escaped = + std::regex_replace(std::string(input), std::regex("'"), "\\'"); escaped = "'" + escaped + "'"; - // Remove excessive quote pairs and handle edge cases - static const std::regex leadingQuotePairs("^(?:'')+(?!$)"); static const std::regex tripleSingleQuote("\\\\'''"); - escaped = std::regex_replace(escaped, leadingQuotePairs, ""); escaped = std::regex_replace(escaped, tripleSingleQuote, "\\'"); +#endif // _WIN32 return escaped; } @@ -188,7 +216,7 @@ void ProcessRunner::Run() { void RunTask(std::shared_ptr result, std::string_view command_id, - const std::optional& positional_args) { + const std::vector& positional_args) { std::string_view path = "package.json"; std::string raw_json; @@ -256,20 +284,21 @@ void RunTask(std::shared_ptr result, // If the "--" flag is not found, it returns an empty optional. // Otherwise, it returns the positional arguments as a single string. // Example: "node -- script.js arg1 arg2" returns "arg1 arg2". -std::optional GetPositionalArgs( - const std::vector& args) { +PositionalArgs GetPositionalArgs(const std::vector& args) { // If the "--" flag is not found, return an empty optional // Otherwise, return the positional arguments as a single string if (auto dash_dash = std::find(args.begin(), args.end(), "--"); dash_dash != args.end()) { - std::string positional_args; + PositionalArgs positional_args{}; for (auto it = dash_dash + 1; it != args.end(); ++it) { - positional_args += it->c_str(); + // SAFETY: The following code is safe because the lifetime of the + // arguments is guaranteed to be valid until the end of the task runner. + positional_args.push_back(std::string_view(it->c_str(), it->size())); } return positional_args; } - return std::nullopt; + return {}; } } // namespace node::task_runner diff --git a/src/node_task_runner.h b/src/node_task_runner.h index 0d5bea9bcb7d29..3d8973db98ab42 100644 --- a/src/node_task_runner.h +++ b/src/node_task_runner.h @@ -14,6 +14,8 @@ namespace node { namespace task_runner { +using PositionalArgs = std::vector; + // ProcessRunner is the class responsible for running a process. // A class instance is created for each process to be run. // The class is responsible for spawning the process and handling its exit. @@ -22,7 +24,7 @@ class ProcessRunner { public: ProcessRunner(std::shared_ptr result, std::string_view command_id, - const std::optional& positional_args); + const PositionalArgs& positional_args); void Run(); static void ExitCallback(uv_process_t* req, int64_t exit_status, @@ -51,10 +53,9 @@ class ProcessRunner { void RunTask(std::shared_ptr result, std::string_view command_id, - const std::optional& positional_args); -std::optional GetPositionalArgs( - const std::vector& args); -std::string EscapeShell(const std::string& command); + const PositionalArgs& positional_args); +PositionalArgs GetPositionalArgs(const std::vector& args); +std::string EscapeShell(const std::string_view command); } // namespace task_runner } // namespace node diff --git a/test/cctest/test_node_task_runner.cc b/test/cctest/test_node_task_runner.cc index 5e30b979f3d1d7..a2c520d2709f2e 100644 --- a/test/cctest/test_node_task_runner.cc +++ b/test/cctest/test_node_task_runner.cc @@ -9,6 +9,20 @@ class TaskRunnerTest : public EnvironmentTestFixture {}; TEST_F(TaskRunnerTest, EscapeShell) { std::vector> expectations = { +#ifdef _WIN32 + {"", "\"\""}, + {"test", "test"}, + {"test words", "\"test words\""}, + {"$1", "\"$1\""}, + {"\"$1\"", "\"\"\"$1\"\"\""}, + {"'$1'", "\"'$1'\""}, + {"\\$1", "\"\\$1\""}, + {"--arg=\"$1\"", "\"--arg=\"\"$1\"\"\""}, + {"--arg=node exec -c \"$1\"", "\"--arg=node exec -c \"\"$1\"\"\""}, + {"--arg=node exec -c '$1'", "\"--arg=node exec -c '$1'\""}, + {"'--arg=node exec -c \"$1\"'", "\"'--arg=node exec -c \"\"$1\"\"'\""} + +#else {"", "''"}, {"test", "test"}, {"test words", "'test words'"}, @@ -19,7 +33,9 @@ TEST_F(TaskRunnerTest, EscapeShell) { {"--arg=\"$1\"", "'--arg=\"$1\"'"}, {"--arg=node exec -c \"$1\"", "'--arg=node exec -c \"$1\"'"}, {"--arg=node exec -c '$1'", "'--arg=node exec -c \\'$1\\''"}, - {"'--arg=node exec -c \"$1\"'", "'\\'--arg=node exec -c \"$1\"\\''"}}; + {"'--arg=node exec -c \"$1\"'", "'\\'--arg=node exec -c \"$1\"\\''"} +#endif + }; for (const auto& [input, expected] : expectations) { EXPECT_EQ(node::task_runner::EscapeShell(input), expected); diff --git a/test/fixtures/run-script/node_modules/.bin/positional-args b/test/fixtures/run-script/node_modules/.bin/positional-args index 47d2d7d6a26d93..2d8092378ba1da 100755 --- a/test/fixtures/run-script/node_modules/.bin/positional-args +++ b/test/fixtures/run-script/node_modules/.bin/positional-args @@ -1,2 +1,3 @@ #!/bin/bash -echo $@ +echo "Arguments: '$@'" +echo "The total number of arguments are: $#" diff --git a/test/fixtures/run-script/node_modules/.bin/positional-args.bat b/test/fixtures/run-script/node_modules/.bin/positional-args.bat index b95ea66f90851a..4b94ed34d51daf 100755 --- a/test/fixtures/run-script/node_modules/.bin/positional-args.bat +++ b/test/fixtures/run-script/node_modules/.bin/positional-args.bat @@ -1,2 +1,15 @@ -@shift -@echo %* +@echo off +setlocal enabledelayedexpansion +set argv=0 +set "output=" +for %%x in (%*) do ( + Set /A argv+=1 + if "!output!" == "" ( + Set "output=%%~x" + ) else ( + Set "output=!output! %%~x" + ) +) +@echo Raw '%*' +@echo Arguments: '%output%' +@echo The total number of arguments are: %argv% diff --git a/test/parallel/test-node-run.js b/test/parallel/test-node-run.js index 69a7c9d9a46db1..11a5b413148f4e 100644 --- a/test/parallel/test-node-run.js +++ b/test/parallel/test-node-run.js @@ -57,10 +57,15 @@ describe('node run [command]', () => { it('appends positional arguments', async () => { const child = await common.spawnPromisified( process.execPath, - [ '--no-warnings', '--run', `positional-args${envSuffix}`, '--', '--help "hello world test"'], + [ '--no-warnings', '--run', `positional-args${envSuffix}`, '--', '--help "hello world test"', 'A', 'B', 'C'], { cwd: fixtures.path('run-script') }, ); - assert.match(child.stdout, /--help "hello world test"/); + if (common.isWindows) { + assert.match(child.stdout, /Arguments: '--help ""hello world test"" A B C'/); + } else { + assert.match(child.stdout, /Arguments: '--help "hello world test" A B C'/); + } + assert.match(child.stdout, /The total number of arguments are: 4/); assert.strictEqual(child.stderr, ''); assert.strictEqual(child.code, 0); });