Partially escaped URLs should be escaped

[KWkyle]

In my opinion, the CVE-2022-25765 issue was caused by partially escaped URLs. They need to be escaped.

def url_needs_escaping?
  URI::DEFAULT_PARSER.unescape(@source) == @source ||
    URI::DEFAULT_PARSER.escape(URI::DEFAULT_PARSER.unescape(@source)) != @source
end

We add an additional condition for method url_needs_escaping? for checking url was a partially escaped URLs or not, by escaped it after unescaped . If the result not matched to source it should be escaped again.

[dblanken]

@KWkyle Great work and thank you for doing this.

The only thing I wanted to mention here so that it doesn't get missed is if we are doing this, I don't think it's relevant to have the comment provided when invoking inside of pdfkit.rb stating:

# In order to allow for URL parameters (e.g. https://www.google.com/search?q=pdfkit) we do
# not escape the source. The user is responsible for ensuring that no vulnerabilities exist
# in the source. Please see https://github.com/pdfkit/pdfkit/issues/164.

While it's true we should always be responsible, the code is now attempting to take care of vulnerabilities. (A GOOD THING!)

[ChrisNelsonBHG]

@serene (assuming you're current maintainer of the gem?) wondering if you or any other maintainer have plans to review this?

[richardwan]

the change on line 50 looks good, but the original condition on line 49 now looks redundant. how confident are we in the test coverage about simply removing line 49 if the specs pass without it?

[KWkyle]

Thanks for the review @richardwan, I've fixed it.

[snaka]

To make it easier to find the issue, here is a link to the Issue

#507

[serene]

Closes #507.

[Vivek8079]

Thanks for this update

[dwagner-evi]

can we bump the version too as part of this or is that usually done as a separate PR?

[serene]

@dwagner-evi I usually do it as a separate PR

[serene]

@KWkyle can you see if you can figure out why the build is broken so we can get this merged?

[dblanken]

Pretty sure it’s due to using rack 3.0. When I attempted my PR I had to use rack 2 for the tests to run.

[KWkyle]

Uppercase headers was not allow anymore after rack 3.0 for HTTP/2 spec.

rack/rack#1592

[serene]

Opened #511 to fix the build.

[serene]

@KWkyle Can you update your fork from master now that the fix has been merged?

[KWkyle]

@serene Thanks, I've finished it.