Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect argument escaping #2197

Closed
Elonon opened this issue Nov 12, 2018 · 12 comments
Closed

Incorrect argument escaping #2197

Elonon opened this issue Nov 12, 2018 · 12 comments

Comments

@Elonon
Copy link

Elonon commented Nov 12, 2018

Description

I'm using CreateProcess to call msdeploy.exe. Unfortunately it has unique argument requirements and the latest escaping changes broke them. For example, I need to pass -source:iisapp="C:\Some\path\" but it's being escaped as "-source:iisapp=C:\Some\path\". Wrapping the entire switch in quotes is rejected.

Because this scenario is likely really rare, I'd be happy if there were just some way to create a Command that expects me to do all escaping. E.g. Command.EscapedCommand or something similar.

Repro steps

Please provide the steps required to reproduce the problem

  1. Call CreateProcess.fromRawWindowsCommandLine "tool.exe" """-source:iisapp="C:\some\path\"""

Expected behavior

emits tool.exe -source:iisapp="C:\some\path\"

Actual behavior

emits tool.exe "-source:iisapp=C:\some\path\"

Known workarounds

Use an older version of Fake.

Related information

  • Windows 10
  • FAKE 5.9.3
@matthid
Copy link
Member

matthid commented Nov 12, 2018

/cc @vbfox any idea?

@vbfox
Copy link
Contributor

vbfox commented Nov 12, 2018

A few quick remarks :

  1. @Elonon you're missing the closing quote in your call, if you want it there in the end result you should specify it (still wouldn't work here but...) :)

  2. Some MS tools are really misbehaved regarding quoting and doesn't use full msvcr parsing (Fun thing is that their parsing IS CORRECT foo="bar baz" is a perfectly good encoding but it should be exactly equivalent to "foo=bar baz")

    And i've seen this EXACT problem previously in some MS SQL tools.

    It's why BlackFox.CommandLine default to not quoting when it can (in addition to me feeling that it look better 😝) and why it has support for Raw (unquotted) arguments (CmdLine.appendRaw) because I have a custom argument encoder for some old microsoft SQL tools doing that...

  3. All considered msvcr argument parsing is a highly followed standard but some programs depart from it and users need an escape hatch.

The best course for me is to stop OfWindowsCommandLine round tripping through windows encoding and have Args internal representation either allow raw arguments mixed with the rest (Like my lib) or allow it to be either a single raw string or an array of strings to be escaped.

@matthid
Copy link
Member

matthid commented Nov 12, 2018

It's why BlackFox.CommandLine default to not quoting when it can (in addition to me feeling that it look better 😝)

yes I thought I added that as well because of a previously reported issue. But I guess it doesn't apply here because his original path has spaces...

Maybe we need to take a look at how to extend the current implementation without breaking too much people...

@matthid
Copy link
Member

matthid commented Nov 12, 2018

@vbfox Honestly, if you can come up with a pr switching to your implementation while trying to keep the breaking change to a minimum I can imagine myself accepting it...

@Elonon
Copy link
Author

Elonon commented Nov 13, 2018

I'm out of my league here, so feel free to ignore if this won't work. But I was thinking that you could add a new item to your Command DU. Something like | Custom of BlackFox.CmdLine

This would let you keep backwards compatibility and only people who really need it can use it.

@vbfox
Copy link
Contributor

vbfox commented Nov 14, 2018

@matthid I might try it, I just released 0.6.0 with support of Mono-on-unix specific quirks (Tested against a C# port of the original mono C source code) and tests that the current code works against .Net core implementation on unix (Both via FsCheck). So I'm starting to be relatively confident in my implementation.

@Elonon There is a simpler way, as Arguments fields are internal they can directly be changed to a BlackFox.CmdLine instance if we want to go that way.

@matthid
Copy link
Member

matthid commented Nov 18, 2018

All the mess and netstandard2.1 will have an API for that anyway :(

@matthid
Copy link
Member

matthid commented Nov 18, 2018

E:\Projects\FAKE\src\app\Fake.Core.Process\Fake.Core.Process.fsproj : error NU1202: Package BlackFox.CommandLine 0.6.0 is not compatible with netstandard1.6 (.NETStandard,Version=v1.6). Package BlackFox.CommandLine 0.6.0 supports:
E:\Projects\FAKE\src\app\Fake.Core.Process\Fake.Core.Process.fsproj : error NU1202:   - net45 (.NETFramework,Version=v4.5)
E:\Projects\FAKE\src\app\Fake.Core.Process\Fake.Core.Process.fsproj : error NU1202:   - netstandard2.0 (.NETStandard,Version=v2.0)

@vbfox I guess we cannot get that down to netstandard16 and net45?

@matthid
Copy link
Member

matthid commented Nov 18, 2018

Ok I started with #2206 not sure if we should take that as is. Review/suggestions appreciated.

Do we still need to change any module? @Elonon Did you use CreateProcess directly or did that happen in any particular module?

@matthid
Copy link
Member

matthid commented Nov 18, 2018

I have a custom argument encoder for some old microsoft SQL tools doing that...

@vbfox is that surfaced in the library as well? It seems to not too uncommon apparently (#2206 (comment)) so maybe we need some helpers for that particular situation?

@Elonon
Copy link
Author

Elonon commented Nov 18, 2018

@Elonon Did you use CreateProcess directly or did that happen in any particular module?

I used Arguments.OfArgs and Command.RawCommand, but I'm happy to change it to use the new functions you forwarded.

(I only used fromRawWindowsCommandLine in my report because it was a simpler repro - Which explains why I screwed up the quoting 😜 )

@vbfox
Copy link
Contributor

vbfox commented Nov 18, 2018

@matthid it's not in the lib as it's code I did at work but if I remember well I didn't care doing an exact version and it's a very crude helper like CmdLine.appendf "-%s=\"%s\" key (removeSeparatorAtEnd value) but it worked so it was good enough XD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants