0.5.3 cannot handle windows paths with spaces #183

Closed
damphyr opened this Issue Jul 4, 2013 · 5 comments

Comments

Projects
None yet
3 participants

damphyr commented Jul 4, 2013

wkhtmltopdf on Windows by default installs itself under C:\Program Files\wkhtmltopdf

Using the config block to point to this path results in an error when the command is put together because it needs "" around the command.

When I set the config option as '"c:/Program Files/wkhtmltopdf/wkhtmltopdf.exe"' then the File.exists? check on line 31 fails. Leaving it empty bombs out because of the use of 'which' to find the correct command and various other combinations ended up in bitter failure.

0.5.2 works.

damphyr commented Jul 4, 2013

Test and patch once I get off work, unless this has already been solved

damphyr commented Jul 4, 2013

This gist allows you to reproduce the error with 0.5.3

Owner

sigmavirus24 commented Aug 15, 2013

@damphyr is this fixed in 0.5.4? If not could you try reverting the change on this line to see if it fixes it?

jdearl commented Sep 17, 2013

Reverting from 0.5.4 to 0.5.2 worked for me, and also fixed not being able to have spaces in options like :header_center => "Document #{number}".

Owner

sigmavirus24 commented Aug 8, 2014

So the issue arises from the usage of Array#shelljoin (provided by shellwords).

1.9.3-p448 :001 > require 'shellwords'
 => true
1.9.3-p448 :002 > p = ["C:/Program Files/ruby/ruby.exe", "foo"]
 => ["C:/Program Files/ruby/ruby.exe", "foo"]
1.9.3-p448 :003 > p.shelljoin
 => "C:/Program\\ Files/ruby/ruby.exe foo"

The problem is that not using Array#shelljoin results in a vulnerability. It's clear (to me) that the problem here is the fact that the path to wkhtmltopdf has a space which then becomes c:/Program\ Files/wkhtmltopdf/wkhtmltopdf. The first exception you're seeing @damphyr is not coming from File.exists? It comes from popen not being able to find the executable at this new path. (File.exists? works because it checks the path before it has the \ inserted.)

One solution to this is to exclude the path for wkhtmltopdf from the array when using Array#shelljoin. This will prevent the \ from being inserted. I'm just curious if there are reasons why this behaviour might actually be desirable though because I'd like to prevent PDFKit from generating another CVE.

@sigmavirus24 sigmavirus24 added a commit that referenced this issue Aug 9, 2014

@sigmavirus24 sigmavirus24 Add failing test for #183 b672282

@sigmavirus24 sigmavirus24 added a commit that referenced this issue Aug 9, 2014

@sigmavirus24 sigmavirus24 Add failing test for #183 a3d0c1e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment