Skip to content

Commit

Permalink
src: fix positional args in task runner
Browse files Browse the repository at this point in the history
PR-URL: #52810
Fixes: #52740
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
  • Loading branch information
anonrig authored and targos committed May 11, 2024
1 parent 5055663 commit d169d0f
Show file tree
Hide file tree
Showing 6 changed files with 109 additions and 44 deletions.
95 changes: 62 additions & 33 deletions src/node_task_runner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<InitializationResultImpl> result,
std::string_view command,
const std::optional<std::string>& positional_args) {
ProcessRunner::ProcessRunner(std::shared_ptr<InitializationResultImpl> result,
std::string_view command,
const PositionalArgs& positional_args) {
memset(&options_, 0, sizeof(uv_process_options_t));

// Get the current working directory.
Expand Down Expand Up @@ -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;
Expand All @@ -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.
Expand All @@ -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
Expand All @@ -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 =
Expand All @@ -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;
}
Expand Down Expand Up @@ -188,7 +216,7 @@ void ProcessRunner::Run() {

void RunTask(std::shared_ptr<InitializationResultImpl> result,
std::string_view command_id,
const std::optional<std::string>& positional_args) {
const std::vector<std::string_view>& positional_args) {
std::string_view path = "package.json";
std::string raw_json;

Expand Down Expand Up @@ -256,20 +284,21 @@ void RunTask(std::shared_ptr<InitializationResultImpl> 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<std::string> GetPositionalArgs(
const std::vector<std::string>& args) {
PositionalArgs GetPositionalArgs(const std::vector<std::string>& 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
11 changes: 6 additions & 5 deletions src/node_task_runner.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
namespace node {
namespace task_runner {

using PositionalArgs = std::vector<std::string_view>;

// 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.
Expand All @@ -22,7 +24,7 @@ class ProcessRunner {
public:
ProcessRunner(std::shared_ptr<InitializationResultImpl> result,
std::string_view command_id,
const std::optional<std::string>& positional_args);
const PositionalArgs& positional_args);
void Run();
static void ExitCallback(uv_process_t* req,
int64_t exit_status,
Expand Down Expand Up @@ -51,10 +53,9 @@ class ProcessRunner {

void RunTask(std::shared_ptr<InitializationResultImpl> result,
std::string_view command_id,
const std::optional<std::string>& positional_args);
std::optional<std::string> GetPositionalArgs(
const std::vector<std::string>& args);
std::string EscapeShell(const std::string& command);
const PositionalArgs& positional_args);
PositionalArgs GetPositionalArgs(const std::vector<std::string>& args);
std::string EscapeShell(const std::string_view command);

} // namespace task_runner
} // namespace node
Expand Down
18 changes: 17 additions & 1 deletion test/cctest/test_node_task_runner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,20 @@ class TaskRunnerTest : public EnvironmentTestFixture {};

TEST_F(TaskRunnerTest, EscapeShell) {
std::vector<std::pair<std::string, std::string>> 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'"},
Expand All @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion test/fixtures/run-script/node_modules/.bin/positional-args

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 15 additions & 2 deletions test/fixtures/run-script/node_modules/.bin/positional-args.bat

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions test/parallel/test-node-run.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down

0 comments on commit d169d0f

Please sign in to comment.