Call IO.popen with an Array of command arguments (#518).
#519
Conversation
|
@postmodern Thanks for this PR! To continue the conversation from #518 ,
I'm not the maintainer, so would be ultimately up to them...but I personally would prefer to not URI escape at all. Escaping that input is the responsibility of the caller. If they want to call a malformed URL, that's their choice. As long as the library handles its responsibility (escaping arguments passed to the shell) then its done its job. Short of some bug or vulnerability in wkhtmltopdf that warrants additional tweaking of the URL, I'd let developers call whatever they want and receive the errors caused by that mistake. And like I mentioned in my issue ticket, the URI escape attempt broke my apps, so it caused more harm than good. I can't be the only one affected by that double escaping bug...
(Been a while since I've run Ruby on Windows...) I'm pretty sure the arguments should still work the same way, and outside of cygwin/mingw there's some path related differences (spaces in path, slash direction, etc) and maybe no shell handling, but overall I think the changes here should work fine. The important part is it'll still execute the command and treat the arguments separately without interpolation. |
* By calling `IO.popen` with an Array of command arguments (ex: `['ls', '-l', ...]`) it runs the command as a separate process instead of running it in a sub-shell as a shell command. This prevents any arbitrary command injection or env variable interpolation, without needing complex shell-escaping logic. https://ruby-doc.org/core-3.1.2/IO.html#method-c-popen * Changed `Configuration#executable` to return a String or an Array for when xvfb mode is enabled. * Changed `PDFKit#command` to return an Array of command arguments for `IO.popen`. * Removed argument quoting logic as it's not necessary when calling `IO.popen` with an Array of arguments. * Rewrote some specs to test if the command's Array of arguments contains specific argument values. * Added a custom RSpec `contain` matcher for testing if an expected Array exists within another Array.
|
@postmodern Can you please let us know the ETA to merge this PR and publish it? This gem has vulnerability and we need this fix. I'm hoping this code change will fix it. |
|
@adiln we are waiting for the maintainers to review, provide feedback or merge the PR. |
|
We did some testing on our production apps to verify that this is not a breaking change. 👍 |
I hope this is a simpler solution to #518 as it avoids complex shell-escaping/input-sanitizing logic in favor of calling
IO.popenwith an Array of separate command arguments.IO.popenwith an Array of command arguments (ex:['ls', '-l', ...]) it runs the command as a separate process instead of running it in a sub-shell as a shell command. This prevents any arbitrary command injection, without needing complex shell-escaping logic. https://ruby-doc.org/core-3.1.2/IO.html#method-c-popenConfiguration#executableto return a String or an Array for when xvfb mode is enabled.PDFKit#commandto return an Array of command arguments forIO.popen.IO.popenwith an Array of arguments.containmatcher for testing if an expected Array exists within another Array.