All download handlers support target paths with quotes

[lazerwalker]

This fixes CocoaPods/CocoaPods#1532.

Installing pods into any path with double or single quotes should now work (with explicit test coverage).

[joshkalpin]

👍 Will this work for spaces as well?

[coveralls]

Coverage remained the same when pulling d0884c2 on lazerwalker:master into 9ab9050 on CocoaPods:master.

[lazerwalker]

Yep. Any time a filepath is passed as an argument to a shell command, it's being quoted with double quotes, which should safely handle pretty much anything that isn't itself a double quote (which is handled through explicitly escaping double-quote characters).

Making sure spaces continue to work sounds worth codifying in a test, though. I'll update the tmp_folder_with_quotes spec helper to add a space character, so the new tests that check for single- and double-quotes will automatically test spaces as well.

[coveralls]

Coverage remained the same when pulling f4248d1 on lazerwalker:master into 9ab9050 on CocoaPods:master.

[alloy]

Can you replace this by using String#shellescape instead please?

http://apidock.com/ruby/v1_8_7_72/Shellwords/shellescape

[lazerwalker]

Huh. When I was working on this earlier, I was seeing unexpected behavior using shellescape (hence this jankiness) but trying it again now it seems to work. New commit incoming — thanks for the suggestion!

[alloy]

Awesome, thanks! It’s high time this got some attention.

However, I wonder if we shouldn’t just fix this in the executable helpers by using the ‘Shellwords’ stdlib API, though, as that way it’s finally fixed for once and for good.

• https://github.com/CocoaPods/cocoapods-downloader/blob/master/lib/cocoapods-downloader/api.rb#L13
• https://github.com/CocoaPods/CocoaPods/blob/master/lib/cocoapods/executable.rb#L60

[lazerwalker]

I started off trying to tackle this from the level of the executable helper, but I ran into difficulties surrounding not being able to distinguish parts of a command string that should be shellescape'd (e.g. "/some/path/that/contains spaces/") and parts that shouldn't be (e.g. 2>&1, which occurs fairly regularly in the codebase). It's not ideal, but I don't think it's unreasonable to place the responsibility of figuring that out on the person manually constructing the argument string.

That being said, if you've got any ideas I'd love to get the executable helper approach to work, since handling it there feels like conceptually the right level of abstraction.

[coveralls]

Coverage remained the same when pulling 1d29826 on lazerwalker:master into 9ab9050 on CocoaPods:master.

[lazerwalker]

Ping! Any chance of getting this merged in, @alloy?

I took another quick look to make sure I'm not crazy and that there isn't a simpler way to do this with Shellwords.

Each individual subclass is currently manually wrapping URLs and file-paths in double-quotes when it calls the executable helper (e.g. https://github.com/CocoaPods/cocoapods-downloader/blob/master/lib/cocoapods-downloader/git.rb#L67). Without escaping the to and from variables before wrapping them in double-quotes, any embedded double-quotes will be escaped at the same level as the wrapping quotes, which makes parsing ambiguous. A trivially-simple example:

Shellwords.split "\"foo\" bar" == ["foo", "bar"] # Properly handles double-quotes
Shellwords.split "\"foo\\\"\" bar" == ["foo\\\"", "bar"] # Properly handles nested double-quotes that are double-escaped.
Shellwords.split "\"foo\"\" bar" # ArgumentError: Unmatched double quote

The code in each individual handler currently makes the command variable in https://github.com/CocoaPods/cocoapods-downloader/blob/master/lib/cocoapods-downloader/api.rb#L13 look like the third example; unless I'm missing something blindingly obvious (always a possibility!), no amount of Shellwords magic is going to make that string parseable by the time it gets to execute_command.

I'm fairly convinced the correct solution here is to escape each of those variables at the time they're wrapped in double quotes, as per the second example (what the code in this PR essentially does).

Thoughts?

[alloy]

Oops, sorry for the delay.

Yeah I think you are correct. We should change it in the future to make the various command methods take a list of arguments instead of just one string. I will review and merge it tomorrow during my travel to Fosdem.

[alloy]

I made a small change where I moved escaping into the Pathname class.

Thanks!