cake-build / cake-build/cake

Improper quoting of all command line arguments

Open
#1,272 4 comments 0 reactions 0 assignees View on GitHub
Waiting for issuer feedback
Dominant language
C#
Stars
4.2k
Forks
778
Avg merge
1h 15m
Merged PRs (30d)
19

Description

As they are today, [`QuotedArgument.Render`](https://github.com/cake-build/cake/blob/b5f3cf1f72e60defe1c4be69219e2fc42ba3d4e6/src/Cake.Core/IO/Arguments/QuotedArgument.cs#L30) and [`.RenderSafe`](https://github.com/cake-build/cake/blob/b5f3cf1f72e60defe1c4be69219e2fc42ba3d4e6/src/Cake.Core/IO/Arguments/QuotedArgument.cs#L40) will improperly quote anything ending with a backslash. Also, if the quoted argument is not a path, it may contain a quote and that is not properly escaped.
[See the MSDN spec.](https://msdn.microsoft.com/en-us/library/windows/desktop/bb776391.aspx)

Here's a draft of proper process argument escaping with tests, following the [MSDN spec](https://msdn.microsoft.com/en-us/library/windows/desktop/bb776391.aspx): https://gist.github.com/jnm2/c5c840bf317605a40f5f56f944db4892

Points to consider:
- I don't know about non-Windows OSs. Hopefully this subject can stay platform agnostic.
- This is something that, while standardized on Windows, could one day end up needing special handlers for non-standard executables.
- It should be sufficient to unit test `QuotedArgument` in the style of the tests in my gist, as long as every command line argument is forced to use this escaping handler (or some custom escaping handler). That's the pit of success and [as we saw here](https://github.com/cake-build/cake/pull/1268/commits/80886c65719914b14c6cd4e7cd8de490ee9f1d2d#diff-b016fe18ec37745cbacaeb41abdf2a64L72), it needs to be harder to not escape arguments for both contributors to this code base and for script writers sending paths via `ArgumentCustomization`.
- All arguments could go through a default escaping handler that only quotes if necessary so you wouldn't have `*Quoted` versions of things. It would just quote if necessary. Another alternative would be to throw an exception (to prevent mistakes and injection) if a space or quote is present in a non-quoting argument. Again you could avoid the default escaping handler and send raw arguments but that would be a noticeable opt-in step.
- This would be a breaking change. It would be nice if this issue could be at least partially addressed (better quoting algorithm) in the next major version.

This issue occurred to me while looking into #1268. Here's some confirmation that I'm on base here: https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/
[Raymond Chen](https://blogs.msdn.microsoft.com/oldnewthing/20100917-00/?p=12833) (because he's fun.)

I'm willing to PR.

Contributor guide

Open the contributing guide

Research direction

Start with src/Cake.Core/IO/Arguments/QuotedArgument.cs, especially QuotedArgument.Render and RenderSafe, and review the existing QuotedArgument unit tests. Use the linked MSDN specification and draft tests to cover trailing backslashes and embedded quotes; done means the escaping behavior is verified by tests and applies consistently to command-line arguments.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
cli, tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.