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

Command prompt escaping in Process.quote_windows #14300

Open
HertzDevil opened this issue Feb 16, 2024 · 2 comments
Open

Command prompt escaping in Process.quote_windows #14300

HertzDevil opened this issue Feb 16, 2024 · 2 comments

Comments

@HertzDevil
Copy link
Contributor

From #13567 we know that Process.quote_windows, which only performs the opposite of CommandLineToArgvW, does not handle metacharacter escaping for the command prompt. We also need both kinds of escaping, and there isn't a one-size-fits-all solution. So I am proposing the following addition to the standard library:

class Process
  def self.quote_windows(args : Enumerable(String), *, c_runtime : Bool = true, shell : Bool = false)
    # if `c_runtime` is true, do what the method does right now; then
    # if `shell` is true, prepend a `^` to every metacharacter
    # (`(`, `)`, `%`, `!`, `^`, `"`, `<`, `>`, `&`, `|`), and also
    # raise if any `\n` appears
  end

  def self.quote_windows(args : String, *, c_runtime : Bool = true, shell : Bool = false)
    quote_windows({args}, c_runtime: c_runtime, shell: shell)
  end
end

Process.quote_windows(%q(foo <"bar">), c_runtime: false)              # => %q(foo <"bar">)
Process.quote_windows(%q(foo <"bar">), c_runtime: false, shell: true) # => %q(foo ^<^"bar^"^>)
Process.quote_windows(%q(foo <"bar">))                                # => %q("foo <\"bar\">")
Process.quote_windows(%q(foo <"bar">), shell: true)                   # => %q(^"foo ^<\^"bar\^"^>^")

Process.quote is not supposed to be used in platform-specific scenarios, so it doesn't have to expose those new parameters.

This still leaves some questions unanswered:

  • Should we continue to expect that the backtick is portable?
  • Should we continue to expect that Process.quote inside a backtick is portable?
  • Do these parameters' defaults for .quote_windows also hold for .quote?
@oprypin
Copy link
Member

oprypin commented Feb 17, 2024

And the big question that I keep insisting on...

  • Should we assume that the "cmd" shell is the shell and deserves to be tied to a boolean shell: true

It might be cool to instead expand Process.run to accept an enum as the shell argument, which would then have implementations for sh, cmd, windows backed by corresponding quote implementations.

As for backticks, I don't have much hope of them ever doing anything sensible...

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Feb 17, 2024

Sometimes I've been thinking what the shell would be for a hypothetical MinGW-based toolchain MSYS2-based build environment. Crystal programs built with it can definitely expect the availability of sh.exe as a default POSIX shell. So maybe replacing shell: true with an enum isn't a bad idea after all...?

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

No branches or pull requests

2 participants