Refactor usage of Dir[] to eliminate unsafe glob construction

[pixeltrix]

Contructing globs by concatenation through File.join or string interpolation is
susceptible to errors caused by paths inadvertently including glob characters.
This commit fixes this by using Dir.chdir to change the current directory to a
safe point from which to execute the Dir.glob and then reconstructs the path.

There are no specific tests that have been added but the temporary test
directory name has been altered to include the {} characters. This causes a
significant number of the existing tests to fail which should prevent any
regressions.

[travisbot]

This pull request fails (merged 32326ea into 8f39d56).

[travisbot]

This pull request fails (merged ce34bd0 into 8f39d56).

[drbrain]

o_O

[pixeltrix]

I'm assuming at some point a whole bunch of these methods would be moved to Gem::Utils - that doesn't exist at the moment does it? I didn't want to pollute the pull request with unrelated changes.

[drbrain]

Do you have a real-world problem this solves?

I've never heard of this happening with RubyGems and I've never seen a bug report due to users having glob specials in their files or paths when building gems, installing gems or using installed gems.

I think "fixing" this in RubyGems is the wrong place. I think you should open a feature request against ruby to add a method that will escape glob special characters so they may be used in Dir.glob. Once the feature is added RubyGems can then use it.

[pixeltrix]

Do you have a real-world problem this solves?

rails/rails#6010

I think "fixing" this in RubyGems is the wrong place. I think you should open a feature request against
ruby to add a method that will escape glob special characters so they may be used in Dir.glob.
Once the feature is added RubyGems can then use it.

Or I just close rails/rails#6010 and tell them not to use * ? { } [ ] in their paths because that's the effect of what you're suggesting. That's not a jibe - I was all for printing a warning message and terminating because I knew that trying to solve this would be awkward.

[pixeltrix]

One thing to note is that RubyGems uses rm_rf in quite a few places - if someone was crazy enough to start using * in their paths then it's possible that RubyGems could end up deleting extra files. In light of this I think waiting for Ruby to add a method is wrong - if you want to fix it by escaping glob characters within RubyGems and tell JRuby to fix their bug then I'm fine with that.

[drbrain]

That rails has a problem isn't a demonstration that RubyGems also has a similar problem in the wild. While it's easy to quickly make one up, nobody has done it by accident in eight years. There are many other ways you could cause RubyGems to break due to odd input that haven't been fixed either. Can you provide a justification for this beyond the hypothetical?

I'd rather maintain code to fix bugs in RubyGems that people experience infrequently over maintaining code for hypothetical issues. There's only so many ways in which you can prevent a user from shooting themselves in the foot before you collapse under the weight of preventative measures.

I don't see what the decisions the rails committers make on their issue has to do with the decisions we make for RubyGems. I don't see why the rejection of one issue should cause the rejection of the other.

Bugs in ruby implementations besides CRuby generally don't affect decisions made for RubyGems, especially of that bug is on behavior from the 1.8 series. JRuby bundles a known good copy of RubyGems which protects them from their bugs. In this case, it seems the JRuby bug is only triggered in the hypothetical case of a glob character in a file path.

Since this is a potential problem for many gems, fixing it in ruby seems like the best place. Reimplementing the same fix across hundreds or thousands of gems seems like a waste of developer effort if they can all use a ruby-provided solution to the problem. I strongly suggest you file a feature request on bugs.ruby-lang.org for this.

While I am dubious, I will continue to discuss this issue with the other committers.

[travisbot]

This pull request fails (merged a1d8313 into 8f39d56).

[pixeltrix]

That rails has a problem isn't a demonstration that RubyGems also has a similar problem in the wild. While it's easy to quickly make one up, nobody has done it by accident in eight years. There are many other ways you could cause RubyGems to break due to odd input that haven't been fixed either. Can you provide a justification for this beyond the hypothetical?

Standard deployment configuration for a Rails app using Bundler is to install gems into a shared 'bundle' folder within the application root. If the path to that application root has any glob characters then installed_specs will return an empty array. Whilst the reporter in the Rails ticket appears to be using his folder structure within development mode later comments seem to suggest a wish to use some of those characters in a deployment configuration.

I'd rather maintain code to fix bugs in RubyGems that people experience infrequently over maintaining code for hypothetical issues. There's only so many ways in which you can prevent a user from shooting themselves in the foot before you collapse under the weight of preventative measures.

I agree, but the pattern I'm using is already implemented in a couple of place within the RubyGems codebase - see install_executables and install_lib.

I don't see what the decisions the rails committers make on their issue has to do with the decisions we make for RubyGems. I don't see why the rejection of one issue should cause the rejection of the other.

If you decide not to fix it then it will affect how I resolve the ticket - if it will never work because of issues within RubyGems then I'd prefer to exit the application with an error message informing the developer that they should correct their paths. It's completely your choice - in fact you not fixing it will mean less work for me.

Bugs in ruby implementations besides CRuby generally don't affect decisions made for RubyGems, especially of that bug is on behavior from the 1.8 series. JRuby bundles a known good copy of RubyGems which protects them from their bugs. In this case, it seems the JRuby bug is only triggered in the hypothetical case of a glob character in a file path.

As I'm not currently using JRuby, or likely to to be in the future I'm not particularly fussed - just trying to be a good OSS citizen by not breaking things intentionally.

Since this is a potential problem for many gems, fixing it in ruby seems like the best place. Reimplementing the same fix across hundreds or thousands of gems seems like a waste of developer effort if they can all use a ruby-provided solution to the problem. I strongly suggest you file a feature request on bugs.ruby-lang.org for this.

Whilst adding a method to Ruby would be of benefit it would possibly only reduce the developer effort required by a fraction unless Dir[] could magically detect whether a glob should be interpreted literally or not. This may be possible - perhaps Ruby could see if a directory entry exists for the literal path segment and then only apply the glob pattern if it doesn't.

While I am dubious, I will continue to discuss this issue with the other committers.

Thank you.

[pixeltrix]

Just to prove the bundler failure: https://gist.github.com/28c43d6ea2f3aca3f8ec