Fix #876, Correct handling of quotation marks in commands #1931

dougbu · 2015-05-22T01:37:07Z

don't strip quotes surrounding command-line arguments when used in scripts
- prevented command paths and grouping arguments containing spaces
- pass preserveSurroundingQuotes into CommandGrammar to control stripping
support expected shell capabilities such as quoting command and redirections
- use cmd /s /c when executing scripts on Windows
- use /bin/bash -c when executing scripts on Linux
add functional tests of commands and scripts (together with dnu restore)
- no dnu restore for .NET Core due to extra time required

note:

CommandGrammar may need additional generalizations
- e.g. unquoted term can't contain more than one escape sequence
- but it's probably good enough for now
not adding quotes around variables replacements inserted into commands
- no way of determining if replacement is already quoted
- basically, leave it to project.json author to perform appropriate quoting for their platform

nits:

~~let VS do its thing with test services~~
make CommandGrammar constructor private; used only in Process() method
update a few ScriptExecutor comments, long lines, et cetera

dougbu · 2015-05-22T01:37:43Z

~~Any suggestions on unit / integration / functional testing of scripts? I don't see anything in this repo at the moment.~~

dougbu · 2015-05-23T00:03:38Z

@davidfowl since you like to look at individual commits, I rebased and added a couple more 😺

Latest version is more resilient and I've tested it numerous ways -- manually. ~~(Don't seem to have test infrastructure in place to automated anything here.)~~

dougbu · 2015-05-24T01:32:54Z

Rebased again and added tests.

@davidfowl ~~this bug is for Beta 5.~~

davidfowl · 2015-05-24T04:11:07Z

@dougbu This is broken on *nix. Look at travis

BrennanConroy · 2015-05-24T05:06:09Z

test/Microsoft.Framework.PackageManager.FunctionalTests/DnuRestoreTests.cs

+
+        [ConditionalTheory]
+        [MemberData(nameof(RuntimeComponents))]
+        [OSSkipCondition(OperatingSystems.Linux | OperatingSystems.MacOSX)]


Because i am positive my bash-writing skills are out-of-date and don't have a Linux system to play on. We can follow-up later with additional scripts.

dougbu · 2015-05-24T06:05:39Z

@davidfowl Travis builds now successful ~~because all new tests are disabled on Mac and Linux~~. AppVeyor build queued; no idea when it will start but local verify builds are fine.

dougbu · 2015-05-24T06:06:55Z

Should have mentioned: No regressions on Mac or Linux ~~i.e. I disabled only my new tests there~~.

ChengTian · 2015-05-26T20:44:06Z

test/Microsoft.Framework.ApplicationHost.FunctionalTests/AppHostTests.cs

+
+                Assert.Equal(0, exitCode);
+                Assert.Empty(error);
+                Assert.Equal(expectedOutput, output);


DNX_TRACE is turned on by default in our TeamCity CI and you'll get extra info in output.

Use Assert.Contains() or turn off DNX_TRACE in the subprocess with something like:
https://github.com/aspnet/dnx/blob/dev/test/Bootstrapper.FunctionalTests/BootstrapperTests.cs#L114

🆗 I turned off DNX_TRACE for other added tests in this PR. Will do the same here.

dougbu · 2015-05-26T21:15:34Z

src/Microsoft.Framework.Runtime.Sources/Impl/EntryPointExecutor.cs

+                            args[index] = arg.Substring(1, arg.Length - 2);
+                        }
+                    }
+


As @davidfowl and I discussed, will undo this change.

dougbu · 2015-06-03T06:31:35Z

src/Microsoft.Framework.PackageManager/Scripts/ScriptExecutor.cs

+                    scriptArguments = new[] { "/bin/bash", "-c", "\"" }
+                        .Concat(scriptArguments.Select(argument => argument.Replace("\"", "\\\"")))
+                        .Concat(new[] { "\"" })
+                        .ToArray();
                }


/cc @troydai I fixed the problems in this else block that we discussed today. E.g. code no longer checks for the script file in whatever random directory dnu was executing.

dougbu · 2015-06-03T06:46:18Z

@davidfowl rebased, updated, and working on Linux. (Not sure why the Travix-ci/pr build hung but the push build succeeded.)

victorhurdugaci · 2015-06-08T23:31:16Z

src/Microsoft.Framework.PackageManager/Scripts/ScriptExecutor.cs

+                    // Special-case a script name that, perhaps with added .sh, matches an existing file.
+                    var surroundWithQuotes = false;
+                    var scriptCandidate = scriptArguments[0];
+                    if (scriptCandidate.StartsWith("\"", StringComparison.Ordinal) &&


Can you have spaces before the first quote or after the last one?

That's possible in the project.json file. But CommandGrammar removes spaces between and before all arguments and ignores the (remote) possibility of an argument that contains more than a correctly-quoted string.

victorhurdugaci · 2015-06-08T23:39:15Z

Looks good to me and I only had the two comments above

victorhurdugaci · 2015-06-08T23:55:19Z

- don't strip quotes surrounding command-line arguments when used in `scripts` - prevented command paths and grouping arguments containing spaces - pass `preserveSurroundingQuotes` into `CommandGrammar` to control stripping - support expected shell capabilities such as quoting command and redirections - use `cmd /s /c` when executing `scripts` on Windows - use `/bin/bash -c` when executing `scripts` on Linux - add functional tests of commands and scripts (together with `dnu restore`) - no `dnu restore` for .NET Core due to extra time required note: - `CommandGrammar` may need additional generalizations - e.g. unquoted term can't contain more than one escape sequence - but it's probably good enough for now - _not_ adding quotes around variables replacements inserted into commands - no way of determining if replacement is already quoted - basically, leave it to project.json author to perform appropriate quoting for their platform nits: - let VS do its thing with test services - make `CommandGrammar` constructor private; used only in `Process()` method - update a few `ScriptExecutor` comments, long lines, et cetera

dougbu · 2015-06-09T01:57:43Z

90f1f03

dnfclas added the cla-already-signed label May 22, 2015

dougbu force-pushed the dougbu/script.spaces.876 branch from 4c0daa8 to 2a5aaac Compare May 23, 2015 00:01

dougbu force-pushed the dougbu/script.spaces.876 branch from 2a5aaac to d0252a1 Compare May 24, 2015 01:32

BrennanConroy reviewed May 24, 2015
View reviewed changes

dougbu force-pushed the dougbu/script.spaces.876 branch from d16f9ca to 9c365ff Compare May 24, 2015 20:27

ChengTian reviewed May 26, 2015
View reviewed changes

dougbu mentioned this pull request May 26, 2015

Script blocks with spaces in arguments will fail. #876

Closed

dougbu reviewed May 26, 2015
View reviewed changes

dougbu force-pushed the dougbu/script.spaces.876 branch 6 times, most recently from af4b0aa to 96f85ca Compare June 3, 2015 06:25

dougbu reviewed Jun 3, 2015
View reviewed changes

dougbu mentioned this pull request Jun 4, 2015

Use IRuntimeEnvironment for PlatformHelper #1692

Merged

victorhurdugaci reviewed Jun 8, 2015
View reviewed changes

dougbu force-pushed the dougbu/script.spaces.876 branch from 96f85ca to 90f1f03 Compare June 9, 2015 01:37

dougbu merged commit 90f1f03 into dev Jun 9, 2015

dougbu deleted the dougbu/script.spaces.876 branch June 9, 2015 01:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #876, Correct handling of quotation marks in commands #1931

Fix #876, Correct handling of quotation marks in commands #1931

dougbu commented May 22, 2015

dougbu commented May 22, 2015

dougbu commented May 23, 2015

dougbu commented May 24, 2015

davidfowl commented May 24, 2015

BrennanConroy May 24, 2015

dougbu May 24, 2015

dougbu commented May 24, 2015

dougbu commented May 24, 2015

ChengTian May 26, 2015

dougbu May 26, 2015

dougbu May 26, 2015

dougbu Jun 3, 2015

dougbu commented Jun 3, 2015

victorhurdugaci Jun 8, 2015

dougbu Jun 8, 2015

victorhurdugaci commented Jun 8, 2015

victorhurdugaci commented Jun 8, 2015

dougbu commented Jun 9, 2015

Fix #876, Correct handling of quotation marks in commands #1931

Fix #876, Correct handling of quotation marks in commands #1931

Conversation

dougbu commented May 22, 2015

dougbu commented May 22, 2015

dougbu commented May 23, 2015

dougbu commented May 24, 2015

davidfowl commented May 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented May 24, 2015

dougbu commented May 24, 2015

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

dougbu commented Jun 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorhurdugaci commented Jun 8, 2015

victorhurdugaci commented Jun 8, 2015

dougbu commented Jun 9, 2015