Skip to content
This repository has been archived by the owner on Mar 21, 2018. It is now read-only.

Use quoting rather than shellescape to give Windows compatibility #216

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

palfrey
Copy link

@palfrey palfrey commented Sep 17, 2013

Ran into a similar error as #128, but on Windows.

Ridley::Errors::CookbookSyntaxError: Invalid ruby files in cookbook: windows (1.10.1).
C:/opscode/chef/embedded/lib/ruby/gems/1.9.1/gems/ridley-1.2.5/lib/ridley/chef/cookbook.rb:175:in `validate'

I'm doing a berks upload with no paths specified, but my home directory has a space in the name.

Turned out that shellescape does the wrong thing with Windows paths, as to quote the Shellwords manual page, "This module manipulates strings according to the word parsing rules of the UNIX Bourne shell". Instead, if we put quotes around the paths, then everything (or at least spaces) should work on both Windows and Unix platforms.

@sethvargo
Copy link
Contributor

Hey @palfrey thank you for the Pull Request. Unfortunately just quoting the value opens other security issues. For example, it would be possible for someone to craft a malicious file path and execute shell commands on a system. Given that Ridley is used as a library and has the potential to accept user input, I don't feel 100% comfortable just using quotes. This seems like the equivalent of SQL inject from back in my PHP days 😄.

@palfrey
Copy link
Author

palfrey commented Sep 23, 2013

I've been thinking about the other options for this on Windows, and they're not pretty. All of this would be done only for Windows, as everyone else can use shellescape.

Options are:

  1. Do the quoting.
  2. Do some Win32 API magic (http://stackoverflow.com/a/10244481/320546) to get the "short path" names which don't need escaping and use them instead.
  3. Figure out how to properly escape things in Windows. http://www.robvanderwoude.com/escapechars.php has a good guide to various other characters, but every bit of advice I can find for spaces says "use quotes"....

Any thoughts on the best option?

@sethvargo
Copy link
Contributor

  1. Looks to be the simplest to me... but then again, I haven't used Windows since 2007 😄. I defer to @reset and @ivey

@palfrey
Copy link
Author

palfrey commented Sep 23, 2013

Odd synchronisation of times :) I've added a version that uses the Win32 magic, but will defer to @reset/@ivey's opinions if they differ.

@reset
Copy link
Collaborator

reset commented Sep 23, 2013

@palfrey I have no objection to using win32 to get the short path name.

We don't want to clutter the top namespace with the #get_short_win32_filename function and we don't want to re-open String like this, though. Ideally this code goes into a mixin called 'shellwords' which extends the shellwords module.

String#shellescape actually just delegates to Shellwords.escape with self. So just re-opening Shellwords will get it done.

@palfrey
Copy link
Author

palfrey commented Oct 4, 2013

@reset: Does this do all the things you wanted? I'm somewhat new to Ruby, and wasn't quite sure if you wanted to change Shellwords.escape or String.shellescape...

@palfrey
Copy link
Author

palfrey commented Jun 27, 2014

I suspect, but have not yet confirmed (due to current lack of a spare Windows box, to be fixed Monday) that the #274 merge may well have fixed this due it stopping shelling out being done at all....

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants