Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Added escaping of ampersands in source url #207

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

When a URL contains an ampersand, i.e. it has multiple
query params, PDFKit should escape the ampersands so that
they can be passed to wkhtmltopdf. If the ampersands are
not escaped, Bash interprets as "run this in background"

@rafaelsales rafaelsales Added escaping of ampersands in source url
When a URL contains an ampersand, i.e. it has multiple
query params, PDFKit should escape the ampersands so that
they can be passed to wkhtmltopdf. If the ampersands are
not escaped, Bash interprets as "run this in background"
7b6bbf6
Owner

sigmavirus24 commented Oct 30, 2013

@cdwort @joshuarh opinions?

@sigmavirus24 -- I currently have this change as a monkey patch in my app because I don't think it's the app responsability to know what bash expects.

Even if I quote the URL like this PDFKit.new(%Q["#{url}"]) I could get error if one of the params include quotes

Owner

sigmavirus24 commented Nov 2, 2013

If you're not URL-encoding your URL then I have no sympathy for you with respect to your last comment. I was simply asking @cdwort and @joshuarh for their thoughts on the actual code.

@sigmavirus24 I was testing with URI.encode, which doesn't encode single quotes and was causing similar issues. E.g. "http://host/q='is_a_quote" == URI.encode(http://host/q='is_a_quote")
Fortunately I am now using url_for, which encodes single quotes, and hence just PDFKit.new(%Q["#{url}"]) works.
Even thought, it would be nicer if we had this in PDFKit. What do you think?

Owner

sigmavirus24 commented Nov 2, 2013

I'd prefer a solution like that to this one actually.

Owner

sigmavirus24 commented Aug 10, 2014

Hey @rafaelsales I'm having trouble understanding why this is necessary as I review it. The way we generate the command we use Array#shelljoin. If you have an array like so: ["wkhtmltopdf", "--quiet", "https://example.com/?foo=bar&biz=baz"] then calling #shelljoin results in a string with "https://example.com/\\?foo\\=bar\\&biz\\=baz" inside of it. So it should already be escaped. Are you seeing behaviour that would indicate otherwise?

@cdwort cdwort added the escaping label Jul 8, 2015

Contributor

cdwort commented Jul 8, 2015

Looks like we will go in another direction for how to handle ampersands, but they will no longer send the command into the background. I'm closing this and considering the work to be part of #312 .

@cdwort cdwort closed this Jul 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment