Fixes bug: unable to spawn .bat with %<environment variable>% in name #51

cspotcode · 2016-12-08T22:37:27Z

Spawning cmd /D /S /C <command string> is susceptible to environment variable expansion. I've added a failing test that illustrates the problem.

The only solution I've devised is to pass the command to be executed as an environment variable, not after the /c flag. Here's an example:

child_process.spawnSync('cmd', [
  '/d /s /c set secret_env_variable_with_cmd_string=& %secret_env_variable_with_cmd_string%'
], {
  stdio: 'inherit',
  windowsVerbatimArguments: true,
  env: {
    secret_env_variable_with_cmd_string: "%CD%.bat firstArgument secondArgument"
  }
});

…able name>% Passes command string to cmd.exe as an environment variable cmd.exe unsets the environment variable and then runs the command cross-spawn avoids environment variable naming conflicts

cspotcode · 2016-12-11T21:29:12Z

I updated this PR to fix the bug using the technique I described above. All tests are passing, so it should be ready to merge. Let me know if you have any questions.

satazor · 2016-12-24T11:18:30Z

@cspotcode Thanks for the PR, it's definitively something that I haven't though. I will request a few changes to the code.

satazor · 2016-12-24T11:19:26Z

lib/parse.js

@@ -36,13 +40,20 @@ function parseNonShell(parsed) {
    if (needsShell) {
        // Escape command & arguments
        applyQuotes = (parsed.command !== 'echo');  // Do not quote arguments for the special "echo" command
-        parsed.command = escapeCommand(parsed.command);


Can we keep everything in the parsed object? Code is concise and no additional variables are necessary.

Keeping everything on the parsed object is more confusing to a new reader. Doing so would mean we are storing a temporary value onto the object with no intention of keeping that value, since it is overwritten on line 57. By using a variable, we're indicating that this is a temporary value used in computation. command would ideally be declared const to make the code's intent even clearer.

satazor · 2016-12-24T11:20:06Z

lib/parse.js

@@ -107,4 +118,16 @@ function parse(command, args, options) {
    return options.shell ? parseShell(parsed) : parseNonShell(parsed);
 }

+// Like Object.assign, for older node versions
+function assign(target, source) {


Add lodash.assign as a dependency and use it instead.

Coolcool, it's done.

satazor · 2016-12-24T11:23:11Z

lib/parse.js

-        parsed.args = ['/d', '/s', '/c', '"' + parsed.command + (parsed.args.length ? ' ' + parsed.args.join(' ') : '') + '"'];
+        // Pass command as environment variable
+        parsed.options.env = assign({}, parsed.options.env || process.env);
+        do {


I don't think this is necessary, just assigning _cross_spawn_command_<Math.round(Math.random() * 10e15).toString(35)> is fine.

My implementation is technically safer, although I agree the possibility of a collision is remote. If you're dedicated to saving 2 lines of code then I'll change it, but at this point the code's already written, so I think it should stay the way it is.

satazor · 2016-12-24T11:24:02Z

lib/parse.js

+            envVariableName = 'cross_spawn_command_' + envVariableSuffix;
+            envVariableSuffix += 1;
+        } while (Object.prototype.hasOwnProperty.call(parsed.options.env, envVariableName));
+        parsed.options.env[envVariableName] = command + (args.length ? ' ' + args.join(' ') : '');


Add a comment explaining this technique and why it's necessary for future reading.

Ok, I added a description.

cspotcode · 2017-02-26T00:36:31Z

@satazor Thanks for the comments. I implemented the requested changes or replied with comments explaining why I think my implementation should remain as-is. Although I do have strong opinions, I don't have a problem changing my code if you really want it implemented a certain way. Just let me know.

In the process I also noticed that the passed options object is mutated in certain situations, so I added a failing test and then fixed it.

EDIT: I forgot to mention that I can squash these commits downs, too. I left them separate to make the revisions more obvious.

…able name>% Passes command string to cmd.exe as an environment variable cmd.exe unsets the environment variable and then runs the command cross-spawn avoids environment variable naming conflicts

…rectly

…on Windows

cspotcode · 2017-10-19T20:32:21Z

Any interest in merging this? I published a proof-of-concept at @cspotcode/cross-spawn. Tests pass; everything appears to work correctly.

- Remove NodeJS v0.10 and v0.12 support - Change escaping on Windows to use `^` instead of quotes: - Fix a bug that made it impossible to escape an argument that contained quotes followed by `>` or other special chars, e.g.: `"foo|bar"`, fixes #82 - Fix a bug were a command containing `%x%` would be replaced with the contents of the `x` environment variable, fixes #51 - Fix `options` argument being mutated - Add a work around for a NodeJS bug when spawning a command with spaces when `options.shell` was enabled - Remove support for running `echo` on Windows

- Remove NodeJS v0.10 and v0.12 support - Change escaping on Windows to use `^` instead of quotes: - Fix a bug that made it impossible to escape an argument that contained quotes followed by `>` or other special chars, e.g.: `"foo|bar"`, fixes #82 - Fix a bug were a command containing `%x%` would be replaced with the contents of the `x` environment variable, fixes #51 - Add a work around for a NodeJS bug when spawning a command with spaces when `options.shell` was enabled, fixes #77 - Fix `options` argument being mutated - Remove support for running `echo` on Windows

- Remove NodeJS v0.10 and v0.12 support - Change escaping on Windows to use `^` instead of quotes: - Fix a bug that made it impossible to escape an argument that contained quotes followed by `>` or other special chars, e.g.: `"foo|bar"`, fixes #82 - Fix a bug were a command containing `%x%` would be replaced with the contents of the `x` environment variable, fixes #51 - Fix `options` argument being mutated

satazor · 2017-11-11T01:55:37Z

@cspotcode I've been working on #83 that fixes this issue by using a different escaping technique using ^.

I will be closing this in favor of that MR that I will eventually release tomorrow as a major release.
Thanks for reporting the issue and for the time you took to explore solutions to it!

(your failing test was copied as well as the clone of the options arg)

- Remove NodeJS v0.10 and v0.12 support - Change escaping on Windows to use `^` instead of quotes: - Fix a bug that made it impossible to escape an argument that contained quotes followed by `>` or other special chars, e.g.: `"foo|bar"`, fixes #82 - Fix a bug were a command containing `%x%` would be replaced with the contents of the `x` environment variable, fixes #51 - Fix `options` argument being mutated

- Remove NodeJS v0.10 and v0.12 support - Add `^` to also escape Windows metachars: - Fix a bug that made it impossible to escape an argument that contained quotes followed by `>` or other special chars, e.g.: `"foo|bar"`, fixes #82 - Fix a bug were a command containing `%x%` would be replaced with the contents of the `x` environment variable, fixes #51 - Fix `options` argument being mutated

cspotcode · 2017-11-13T02:46:10Z

Awesome, thanks for fixing it.

satazor · 2017-11-13T02:47:41Z

I'm still waiting some feedback on #83 from some people.
If you could, please give it a test. All the feedback is welcome.

cross-spawn is being used by a lot of people, so I must release stuff with special care.

…ch syntax error More specifically: - Fix a bug that made it impossible to escape an argument that contained quotes followed by `>` or other special chars, e.g.: `"foo|bar"`, fixes #82 - Fix a bug were a command containing `%x%` would be replaced with the contents of the `x` environment variable, fixes #51 This was resolved by using `^` to escape all meta chars. Additionally, double escaping was necessary for cmd-shim files located in `node_modules./bin/`. Also, this commit was a major overhaul: - Upgrade tooling - Upgrate project to es6 (node v4) - Fix commands as posix unix relatixe paths not working correctly - Fix `options` argument being mutated - Improve compliance with node's ENOENT errors - Improve detection of node's shell option support - Migrate project to moxystudio BREAKING CHANGE: remove support for older nodejs versions, only node >= 4 is supported

…ch syntax error More specifically: - Fix a bug that made it impossible to escape an argument that contained quotes followed by `>` or other special chars, e.g.: `"foo|bar"`, fixes #82 - Fix a bug were a command containing `%x%` would be replaced with the contents of the `x` environment variable, fixes #51 This was resolved by using `^` to escape all meta chars. Additionally, double escaping was necessary for cmd-shim files located in `node_modules./bin/`. Also, this commit was a major overhaul: - Upgrade tooling - Upgrate project to es6 (node v4) - Fix commands as posix unix relatixe paths not working correctly - Fix `options` argument being mutated - Improve compliance with node's ENOENT errors - Improve detection of node's shell option support - Migrate project to moxystudio BREAKING CHANGE: remove support for older nodejs versions, only node >= 4 is supported Fixes #82, #51

cspotcode force-pushed the feature/env-variable-paths branch from 6817897 to 8c6ef59 Compare December 11, 2016 21:22

cspotcode force-pushed the feature/env-variable-paths branch from 8c6ef59 to a284ab9 Compare December 11, 2016 21:24

cspotcode changed the title ~~Adds failing test: unable to spawn .bat with %<environment variable>% in name~~ Fixes bug: unable to spawn .bat with %<environment variable>% in name Dec 11, 2016

satazor reviewed Feb 25, 2017

View reviewed changes

cspotcode force-pushed the feature/env-variable-paths branch from bab1783 to 6b3ba1a Compare February 26, 2017 00:32

cspotcode added 8 commits October 19, 2017 15:41

Adds failing test

fd24a5e

Fixes moxystudio#51: can spawn commands containing %<environment vari…

401bc6a

…able name>% Passes command string to cmd.exe as an environment variable cmd.exe unsets the environment variable and then runs the command cross-spawn avoids environment variable naming conflicts

Adds tests for passing environment variables to spawned processes cor…

0c29332

…rectly

Adds failing test that checks for mutation of options and env objects

f741885

Replaces assign function with 'lodash.assign' module

c4a23bf

Adds comments explaining why environment variable trick is necessary …

f01830e

…on Windows

Adds comment explaining why it is necessary to clone options.env

57f49ee

Clones options object to avoid mutating the original

0a4824a

cspotcode force-pushed the feature/env-variable-paths branch from 6b3ba1a to 0a4824a Compare October 19, 2017 19:42

satazor mentioned this pull request Nov 11, 2017

Fix certain arguments not being correctly escaped or causing batch syntax error #83

Merged

satazor closed this Nov 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes bug: unable to spawn .bat with %<environment variable>% in name #51

Fixes bug: unable to spawn .bat with %<environment variable>% in name #51

Fixes bug: unable to spawn .bat with %<environment variable>% in name #51

Fixes bug: unable to spawn .bat with %<environment variable>% in name #51

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment