Fix single quote bug

[brendon]

Hi @markevans, I spent about a day trying to fix this but it's more complicated than it looks. I've added in a failing spec so you can at least see the problem.

The issue is, that shellsplit always fails if there are unbalance quotes in the string, even if we escape them first it would seem. Here's an excerpt from the documentation:

Usage You can use Shellwords to parse a string into a Bourne shell friendly Array.

require 'shellwords'

argv = Shellwords.split('three blind "mice"')
argv #=> ["three", "blind", "mice"]

Once you’ve required Shellwords, you can use the split alias String#shellsplit.

argv = "see how they run".shellsplit
argv #=> ["see", "how", "they", "run"]

Be careful you don’t leave a quote unmatched.

argv = "they all ran after the farmer's wife".shellsplit
     #=> ArgumentError: Unmatched double quote: ...

In this case, you might want to use ::escape, or its alias String#shellescape.

This method will escape the String for you to safely use with a Bourne shell.

argv = Shellwords.escape("special's.txt")
argv #=> "special\\'s.txt"
system("cat " + argv)

The way forward would to either explicitly escape all user supplied strings (file names etc...), or use a library like https://github.com/thoughtbot/terrapin.

Can you give me some direction? Are you still fairly active on this project and happy to accept fixes like this?

Fixes: #364

[brendon]

It might be an idea to look at: https://makandracards.com/makandra/44452-running-external-commands-with-open3

[markevans]

hi - I am still active on this project, the problem is finding the time - these days it's more bug/security fixes rather than new features.

It's a slightly tricky one this one. In hindsight I possibly should have used an array rather than a string in the first place to separate args for all the commands that use the shell.

I'd rather not depend on other gems as much as possible unless absolutely necessary.

I think you're right that using Shellwords.escape for old_path (instead of shell.quote) should work - I just need to work out whether this should be used everywhere instead of quote, and what the security implications are. Feel free to write a PR if you get it to work though. I think what I'd do would be hide Shellwords.escape behind the method shell.escape (similar to shell.quote)

[brendon]

Hi @markevans :) Thanks for getting back to me on this :D I agree regarding depending on other gems for this, especially given Paperclip is deprecated and that was the main user of Terrapin/Cocaine.

I did a bit of hunt and came up with this. Never heard of Open3 before but it looks good:

https://makandracards.com/makandra/1243-execution-of-shell-code-in-ruby-scripts https://makandracards.com/makandra/44452-running-external-commands-with-open3

Unfortunately Shellwords.escape won't work for escaping the path because we still run the whole lot through Shellwords.split which will still balk at the unbalanced quotes (it doesn't care about whether they're escaped or not - perhaps it should, but I think it just uses a regex).

An array for arguments is probably the best way forward. I couldn't quite tell, but I think Open3 escapes each of the arguments in the array.

I'm happy to do some work on this. If I could get some help from you on the tests when the time comes that would be cool.

Let me know what strategy you'd like to go for and then I can get started :)

[tomasc]

@brendon @markevans this would be very helpful bug fix – has bitten me badly recently. Perhaps could build upon #488 merged recently.

[brendon]

Lol, didn't even notice we were already using Open3 :)

[tomasc]

@brendon Well … for a day and on master ;-)

[brendon]

Sorry, I meant the Open3 library which seems to have been used (via popen3) for a while :D Terrible library and method names though! :D

[tomasc]

I see. Do you think the escaping can be solved now?

[brendon]

I think we need to fully switch to an array based setup and stop using Shellwords.split in order to solve this. It shouldn't be too hard since we're just passing a string around and splitting it at the end.

[tomasc]

@brendon sounds good. Do you want to start a PR and then we can take it from there?

[brendon]

Yep, I'll probably use this PR and add to it since it has the failing test. I think a lot of the tests will need to be rewritten as they rely on the string method. I'll look at it over the coming days :)

[tomasc]

OK, this will be very helpful. Thanks in advance!

[brendon]

Ok wow, that was pretty intense. Spent about 5 hour working on this, but all the tests pass. It feels messy passing arrays around but it's really the only way to go.

I'd advise you to check out the tests commit to see the changes there. I've added commit message details that outline the important things.

[brendon]

Also, I don't see travis running this. I removed the special execution condition for jruby as that won't work going forward if we rely on Open3.

[brendon]

Hi @markevans and @tomasc :) Did you have any opinions on this PR? Could you add travis testing to this project so that GitHub runs the test suites?

[tomasc]

Hi @brendon, first thanks a lot for this!

I reviewed the changes briefly and it seems all commands need to be updated from string format to an Array, am I correct? If so, I think we should look for ways of possibly staying compatible with the old format, as this change would mean all Dragonfly plugins would need to be refactored and through that they would become incompatible with older versions of Dragonfly. Sounds like a bit of a headache to me – would be great to avoid that.

Do you have any ideas how to solve this?

[brendon]

Hi @tomasc, yes that's correct, in this change, arrays are the only way to send through a command. We could look at doing it the Paperclip way (it's actually Terrapin) where we come up with a string interpolation format (e.g. convert :source :destination and any 'symbols' are interpolated with their values passed in as a hash. It's still not backward compatible though.

I guess there could be a fork when evaluating the command in shell.rb and if the command is a string, we could run it like it used to be run. Messy though, and doesn't really solve the bug for those plugins.

Not quite sure where to head from here... :)

[tomasc]

@brendon I see. How about we prefer arrays, but try to split strings to make this backward compatible. I want to think linux command line args have quite parseable format usually, so we could get away with most?

[brendon]

Splitting the strings will still cause the bug we currently have if the filename has quotes in it (if we use Ruby's string splitter). Are we happy for plugin creators to keep on unaware of the issue? Or would it better to release a major version that has this breaking change in order to get plugin authors to upgrade their plugins? I suppose it's a hard one as it's a hard edge that may leave people stranded if their plugin author doesn't exist anymore.

If I gave you write access to my branch would you like to look at how to add this compatibility? :)

[tomasc]

@brendon how about we simply provide both options?

• when passing String, Dragonfly would use the current method which does not handle quotes well (and it served most just fine so far, it seems)
• when passing an Array, Dragonfly would use the lovely refactor via capture3

That way no-one has to change anything, authors of Dragonfly plugins (such as myself with 13 and counting :-)) can refactor and upgrade at their own pace?

[brendon]

Woah! Where are these plugins listed? I'd be keen to take a look at what they do :D

How about supporting String but with a deprecation warning?

[tomasc]

https://github.com/tomasc?utf8=✓&tab=repositories&q=dragonfly_&type=&language=

I like the deprecation warning thing!

[brendon]

Prolific! :) Perhaps those should be added somewhere on the dragonfly site. I wasn't aware that the plugin ecosystem was so active :D

I'll work on that change in the coming days.

[tomasc]

They are – I just need to update the list https://github.com/markevans/dragonfly/wiki/Dragonfly-add-ons

[tomasc]

And yes, I think Dragonfly is a terrific gem. Very flexible and easy to extend – underrated in the Rails community.

[brendon]

Indeed :) It should definitely either be a link, or linked to on this page: http://markevans.github.io/dragonfly/plugins

[brendon]

Hi @tomasc, I made a start on this but I don't think it's easy to maintain backward compatibility because of this:

    def shell_generate(opts={})
      ext = opts[:ext] || self.ext
      should_escape = opts[:escape] != false
      tempfile = Utils.new_tempfile(ext)
      new_path = should_escape ? shell.quote(tempfile.path) : tempfile.path
      command = yield(new_path)
      run(command, :escape => should_escape)
      update(tempfile)
    end

As you can see there escaping is turned on by default, but we've removed the concept of escaping with the array syntax because things are always escaped in that method (not by us but by Open3). We're also unable to determine at the time that we're checking whether to escape or not whether we're dealing with an Array or a String being generated as that is determined by the return of the yield.

Did you have any ideas?

[brendon]

Actually, I managed to figure it out. I just force false from all over the code for now. Once this is removed we can revert all this code :) Do you want to try this with your plugins to see if it works? It should test green.

[brendon]

@markevans, could you please turn on travis CI for this repository so we can get the tests to run for PR's?

[markevans]

@brendon it already is no? https://travis-ci.org/markevans/dragonfly

[brendon]

@markevans, ah righty, it's just not showing in GitHub. I think that must be a seperate integration? It should show up as a check on the PR down the bottom :)

Other than that are you happy with the current solution? I'm keen to get this released so I can stop manually telling my customers to not upload files with quote marks in them :)

[tomasc]

@brendon I can test this PR on some of my plugins over the weekend. Would that be good?

[brendon]

That would be great :D

[markevans]

@brendon @tomasc I just pushed a branch with a commit that replaces quote with escape - would this work for files with spaces/apostrophes in? https://github.com/markevans/dragonfly/compare/paths-with-apostrophes Sorry I haven't had time to test it out properly but it's quicker to code it than explain what I was thinking. There are a couple of failing tests but if it works I'm happy to finish it off. It avoids needing to change the interface. I just wanna be super-careful with regards to backwards-compatibility and security that it does the right thing

[brendon]

Hi @markevans, and thanks for that idea. Unfortunately what is broken is shellsplit. We just can't use it if there's a chance of an unmatched quote in the string. See: http://ruby-doc.org/stdlib-2.3.4/libdoc/shellwords/rdoc/Shellwords.html#module-Shellwords-label-Usage at the third code example in that section.

If we can't use shellsplit then that limits our options which is why I've gone with the array approach. Everything is escaped (apart from the command name) in the array approach with Open3 so there's no need to worry about that. There's a test in the suite that tests that escaping works. All my PR removes is the ability to turn off escaping (except for old string style commands), and this is probably a good thing I assume?

[markevans]

The things is that with escape there won't be an unmatched single quote. For example, this fails as we know,

"hel'lo".shellsplit  # ===> ArgumentError: Unmatched double quote: "hel'lo"

but this doesn't

"hel\\'lo".shellsplit  # ===> ["hel'lo"]

(Note that \\ above really means \). In that commit I showed, the path is escaped before being yielded to the block

[brendon]

Oh my gosh! I played around with escaping originally for quite a while but I clearly did it wrong. This is much more elegant a solution. Let's run with that :) I'll look into the tests for you.

[tomasc]

Hi @markevans, tested your branch with no luck, unfortunately. Perhaps I am doing something wrong?

Trying this: https://github.com/tomasc/dragonfly_harfbuzz/blob/args-with-apostrophes/test/dragonfly_harfbuzz/processors/hb_view_test.rb#L8

With your branch: https://github.com/tomasc/dragonfly_harfbuzz/blob/args-with-apostrophes/Gemfile#L11

Using shell update: https://github.com/tomasc/dragonfly_harfbuzz/blob/args-with-apostrophes/lib/dragonfly_harfbuzz/processors/hb_view.rb#L21-L35

Fails with:

ArgumentError:         ArgumentError: Unmatched double quote: "hb-view --font-file=~/Devel/dragonfly_harfbuzz/samples/sample.ttf --output-file=/var/folders/2b/2s17x6l927bc6k57ghfwtn680000gn/T/dragonfly20181105-34614-un1g9i.svg --output-format=svg --text=F'OO"
            ~/.rbenv/versions/2.5.1/lib/ruby/2.5.0/shellwords.rb:84:in `block in shellsplit'
            ~/.rbenv/versions/2.5.1/lib/ruby/2.5.0/shellwords.rb:82:in `scan'
            ~/.rbenv/versions/2.5.1/lib/ruby/2.5.0/shellwords.rb:82:in `shellsplit'
            ~/.rbenv/versions/2.5.1/lib/ruby/2.5.0/shellwords.rb:205:in `shellsplit'
            ~/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/bundler/gems/dragonfly-16a6cfe30880/lib/dragonfly/shell.rb:18:in `escape_args'
            ~/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/bundler/gems/dragonfly-16a6cfe30880/lib/dragonfly/shell.rb:12:in `run'
            ~/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/bundler/gems/dragonfly-16a6cfe30880/lib/dragonfly/content.rb:216:in `run'
            ~/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/bundler/gems/dragonfly-16a6cfe30880/lib/dragonfly/content.rb:175:in `shell_update'
            ~/Devel/dragonfly_harfbuzz/lib/dragonfly_harfbuzz/processors/hb_view.rb:21:in `call'
            ~/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/bundler/gems/dragonfly-16a6cfe30880/lib/dragonfly/job/process.rb:23:in `apply'
            ~/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/bundler/gems/dragonfly-16a6cfe30880/lib/dragonfly/job.rb:119:in `block in apply'
            ~/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/bundler/gems/dragonfly-16a6cfe30880/lib/dragonfly/job.rb:119:in `each'
            ~/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/bundler/gems/dragonfly-16a6cfe30880/lib/dragonfly/job.rb:119:in `apply'
            ~/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/bundler/gems/dragonfly-16a6cfe30880/lib/dragonfly/job.rb:250:in `result'
            ~/.rbenv/versions/2.5.1/lib/ruby/2.5.0/forwardable.rb:223:in `ext'
            ~/Devel/dragonfly_harfbuzz/test/dragonfly_harfbuzz/processors/hb_view_test.rb:22:in `block (5 levels) in <top (required)>'

[markevans]

hi @tomasc does it work if you remove the single quotes around str in https://github.com/tomasc/dragonfly_harfbuzz/blob/args-with-apostrophes/lib/dragonfly_harfbuzz/processors/hb_view.rb#L28?

[markevans]

don't think you need Shellwords.escape either as it's already escaped

[tomasc]

@markevans I made both changes and pushed – still no luck :(

[tomasc]

here the tests https://travis-ci.org/tomasc/dragonfly_harfbuzz

[markevans]

not sure @tomasc - there's a lot going on there so it may be something else, but the tests in the branch https://github.com/markevans/dragonfly/compare/paths-with-apostrophes are passing now and it seems to work (?)

[tomasc]

@markevans I see – I was too quick in assuming your branch would fix all args – while it only escapes the paths.

So I escaped my args individually, which solved the issue: https://github.com/tomasc/dragonfly_harfbuzz/blob/args-with-apostrophes/lib/dragonfly_harfbuzz/processors/hb_view.rb#L26

However, it makes me think whether it would not be actually simpler – in Dragonfly and in your new branch – to escape the whole command as it passed to the shell_update etc. methods? What do you think?

[brendon]

What if you add quote back in as a method and still just quote the shell-splitted string? Escaping just the incoming path parameters should do the trick? Or will that leave the other arguments not properly escaped?

[brendon]

I should add, in my experiments with adding the quote feature back only the hello there tests fail + one other that I'm looking into.

[brendon]

I've done those changes and created a new PR: #495. All the spec's should be green. @tomasc, could you give that PR a test against your stuff? Should be the best of both worlds.

[brendon]

@markevans, can you trigger a rebuild on the 1.9.3 suite? There isn't a good reason for that install failure given your suite passed.

[tomasc]

@brendon I just plugged your branch in, with no escaping – tests fail:

https://github.com/tomasc/dragonfly_harfbuzz/tree/new-quote-bug https://travis-ci.org/tomasc/dragonfly_harfbuzz

Anything particular I should change?

[markevans]

@markevans I see – I was too quick in assuming your branch would fix all args – while it only escapes the paths.

So I escaped my args individually, which solved the issue: https://github.com/tomasc/dragonfly_harfbuzz/blob/args-with-apostrophes/lib/dragonfly_harfbuzz/processors/hb_view.rb#L26

However, it makes me think whether it would not be actually simpler – in Dragonfly and in your new branch – to escape the whole command as it passed to the shell_update etc. methods? What do you think?

I think this behaviour is fine actually. Dragonfly does escape every word, so you can't do any shell injection, but it's not for Dragonfly to assume how you input commands, e.g. I couldn't type in echo ab'cd onto the command line as a single command, so putting that command into shell update should also fail. It absolutely is up to the user to make sure they input a valid command. The one thing that Dragonfly should fix, though is escaping the file path, as this is the thing that gets yielded, that the user doesn't have so much control over