Encode route parameters using :default_encoding setting #1412
Conversation
|
@bk2204 Thanks for your contribution! I'm going to review this in a few days. |
There was a problem hiding this comment.
This is a clear bug.
In fact, we have the default encoding of sinatra for query parameters. The issue in question is due to the fact that force_encoding is not properly applied to params generated by mustermann.
In the current implementation, the default encoding of sinatra coexists with the default encoding of ruby, which is not inconsistent, it does not seem to have any reason to be there at all.
However, this patch is not enough to me.
Captured params and another block params should also be force encoded.
See
Lines 1030 to 1032 in ba935c9
|
Ah, I did miss those. I'll fix up the patch and add some more tests. |
bk2204
force-pushed
the
default-encoding-params
branch
from
ed91d9f to
cf26bf4
Compare
|
I fixed the named captures and added tests for those and the block parameters. Due to I also switched the tests to use a non-standard default encoding (ISO-8859-1) to ensure that we were honoring the setting properly. |
|
|
||
| regexp_exists = pattern.is_a?(Mustermann::Regular) || (pattern.respond_to?(:patterns) && pattern.patterns.any? {|subpattern| subpattern.is_a?(Mustermann::Regular)} ) | ||
| if regexp_exists | ||
| captures = pattern.match(route).captures | ||
| values += captures | ||
| @params[:captures] = captures unless captures.nil? || captures.empty? | ||
| @params[:captures] = force_encoding(captures) unless captures.nil? || captures.empty? | ||
| else | ||
| values += params.values.flatten |
There was a problem hiding this comment.
Is it also necessary to handle this line to properly encode the block arguments as follows?
get '/:id' |id|
"#{id.encoding}"
endThere was a problem hiding this comment.
I don't believe it's necessary to change that line. force_encoding operates recursively in place, so if there were any items to change the encoding of, we'd have already done it on line 1026. I agree this behavior of force_encoding is surprising, but it's probably that way for efficiency reasons.
I did add a test for this case below to demonstrate that it works.
I suppose to make it more obvious, I could hoist the force_encoding above line 1026 and put it in void context. Let me make that change, as I think it would make things easier to understand.
There was a problem hiding this comment.
It makes sense. Thanks for your explanation.
bk2204
force-pushed
the
default-encoding-params
branch
from
cf26bf4 to
b058835
Compare
|
I've made the change I suggested in reply to your review. The block arguments did work before (as the tests demonstrate), but it wasn't as obvious why, and it was potentially confusing. This should be less confusing and more straightforward. |
|
|
||
| regexp_exists = pattern.is_a?(Mustermann::Regular) || (pattern.respond_to?(:patterns) && pattern.patterns.any? {|subpattern| subpattern.is_a?(Mustermann::Regular)} ) | ||
| if regexp_exists | ||
| captures = pattern.match(route).captures | ||
| values += captures | ||
| @params[:captures] = captures unless captures.nil? || captures.empty? | ||
| @params[:captures] = force_encoding(captures) unless captures.nil? || captures.empty? | ||
| else | ||
| values += params.values.flatten |
There was a problem hiding this comment.
It makes sense. Thanks for your explanation.
## 2.0.4 / 2018-09-15 * Don't blow up when passing frozen string to `send_file` disposition [#1137](sinatra/sinatra#1137) by Andrew Selder * Fix ubygems LoadError [#1436](sinatra/sinatra#1436) by Pavel Rosick�«ò * Unescape regex captures [#1446](sinatra/sinatra#1446) by Jordan Owens * Slight performance improvements for IndifferentHash [#1427](sinatra/sinatra#1427) by Mike Pastore * Improve development support and documentation and source code by Will Yang, Jake Craige, Grey Baker and Guilherme Goettems Schneider ## 2.0.3 / 2018-06-09 * Fix the backports gem regression [#1442](sinatra/sinatra#1442) by Marc-Andr�«± Lafortune ## 2.0.2 / 2018-06-05 * Escape invalid query parameters [#1432](sinatra/sinatra#1432) by Kunpei Sakai * The patch fixes [CVE-2018-11627](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-11627). * Fix undefined method error for `Sinatra::RequiredParams` with hash key [#1431](sinatra/sinatra#1431) by Arpit Chauhan * Add xml content-types to valid html_types for Rack::Protection [#1413](sinatra/sinatra#1413) by Reenan Arbitrario * Encode route parameters using :default_encoding setting [#1412](sinatra/sinatra#1412) by Brian m. Carlson * Fix unpredictable behaviour from Sinatra::ConfigFile [#1244](sinatra/sinatra#1244) by John Hope * Add Sinatra::IndifferentHash#slice [#1405](sinatra/sinatra#1405) by Shota Iguchi * Remove status code 205 from drop body response [#1398](sinatra/sinatra#1398) by Shota Iguchi * Ignore empty captures from params [#1390](sinatra/sinatra#1390) by Shota Iguchi * Improve development support and documentation and source code by Zp Yuan, Andreas Finger, Olle Jonsson, Shota Iguchi, Nikita Bulai and Joshua O'Brien
Honor the :default_encoding setting for any extracted parameters from the URL. The force_encoding was scoped to modify only the necessary items for performance. In the test, be explicit about our dependency on this value.
Fixes #1362.