Byron / open-rs

Open a path or URL with the system-defined program
http://byron.github.io/open-rs
MIT License
319 stars 49 forks source link

Fix Windows URL encoding for multiple query params #68

Closed Eusebius1920 closed 1 year ago

Eusebius1920 commented 1 year ago

Previously, passing a URL with multiple query parameters on Windows would result in improper escaping, causing unexpected behavior when using it with "cmd /c". This commit addresses this issue by properly escaping all query parameters in the URL. Now, URLs with multiple query parameters are handled correctly on Windows.

See also:

Related #67

Eusebius1920 commented 1 year ago

Uff that was harder than initially thought or anticipated. I am not a windows expert.

Digging into the documentation of the cmd and start commands (see links above), I decided that instead of escaping all occurences of & with ^& it would be better to wrap the url/path into quotes.

These quotes however lead to start using this as the title of the generated window instead of the url. So you have to supply a empty pair of quotes between start and the quoted url/path (which results in the newly created window to have an empty title).

However cmd /c handles arguments completly different than other commands on windows. So the supplied arguments had to be supplied via std::os::windows::process::CommandExt'-trait method raw_arg which disables extra escaping and quoting and cites exactly our use-case (see link above). Otherwise some magic somewhere would replace the quoted stuff with backslashes. (See also: https://github.com/rust-lang/rust/issues/29494)

Points that should be considered in review:

Byron commented 1 year ago

Thanks so much for this fantastic PR!

I love the amount of thought and care that went into it, and learned a whole lot in the process! Thus far I only thought that 'windows is strange in a lot of ways', but now there is more of a handle to that strangeness thanks to raw_arg(). This will probably make the invocation more robust than ever (and it's amazing that making a command-invocation on Windows can be so hard to get right).

  • I also adjusted the app-name that is moved into as argument to be a raw_arg. This might break existing code? Should we use regular arg there as before? Nobody seemed to have any problems with that part.
  • raw_arg (see link above) needs rust >= 1.62 (a platform independant version directly in the command-trait will presumably land in rust in 1.70)

I have made the next release a major one which should alleviate any issue with this. If code breaks due to some escaping the caller might have done, now the code should be simpler as the additional processing of app can be removed.

  • fn wrap_in_quotes could potentially used in other places. It is not windows-specific, but there is not utils.rs or anything like that, so I placed it in windows.rs

Let's keep it where it's definitely needed which seems to be windows only. On other platforms the shell is not used to launch a program, so any argument can be passed without prior escapes.

The fix is now available in a new major release. Thanks again!