Encode route parameters using :default_encoding setting by bk2204 · Pull Request #1412 · sinatra/sinatra

bk2204 · 2018-03-25T20:06:04Z

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.

namusyaka · 2018-03-28T17:31:42Z

@bk2204 Thanks for your contribution! I'm going to review this in a few days.

namusyaka

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

sinatra/lib/sinatra/base.rb

Lines 1030 to 1032 in ba935c9

    
             @params[:captures] = captures 
        
           else 
        
             values += params.values.flatten

bk2204 · 2018-03-30T19:49:16Z

Ah, I did miss those. I'll fix up the patch and add some more tests.

bk2204 · 2018-03-31T19:14:00Z

I fixed the named captures and added tests for those and the block parameters. Due to @params being set on line 1026, block parameters appear to be handled correctly using the same code as named parameters.

I also switched the tests to use a non-standard default encoding (ISO-8859-1) to ensure that we were honoring the setting properly.

namusyaka

Thanks for reflecting my comments.
Probably the next step is the last.

namusyaka · 2018-04-01T06:31:36Z

-        @params[:captures] = captures unless captures.nil? || captures.empty?
+        @params[:captures] = force_encoding(captures) unless captures.nil? || captures.empty?
      else
        values += params.values.flatten


Is it also necessary to handle this line to properly encode the block arguments as follows?

get '/:id' |id| "#{id.encoding}" end

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.

It makes sense. Thanks for your explanation.

Fixes sinatra#1362

bk2204 · 2018-04-01T19:06:24Z

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.

namusyaka

Looks good to me. Thank you so much!

namusyaka · 2018-04-01T22:34:41Z

-        @params[:captures] = captures unless captures.nil? || captures.empty?
+        @params[:captures] = force_encoding(captures) unless captures.nil? || captures.empty?
      else
        values += params.values.flatten


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

namusyaka added this to the v2.0.2 milestone Mar 28, 2018

namusyaka self-requested a review March 30, 2018 17:00

namusyaka requested changes Mar 30, 2018

View reviewed changes

bk2204 force-pushed the default-encoding-params branch from ed91d9f to cf26bf4 Compare March 31, 2018 19:10

namusyaka reviewed Apr 1, 2018

View reviewed changes

Encode route parameters using :default_encoding setting

b058835

Fixes sinatra#1362

bk2204 force-pushed the default-encoding-params branch from cf26bf4 to b058835 Compare April 1, 2018 19:04

namusyaka approved these changes Apr 1, 2018

View reviewed changes

namusyaka merged commit 3fa1f2c into sinatra:master Apr 1, 2018

jkowens mentioned this pull request Jun 15, 2018

Unescape regex captures #1446

Merged

tagliala mentioned this pull request Apr 8, 2020

Uploading files with non-ASCII names generate invalid download URLs ifad/colore#8

Closed

dependabot Bot mentioned this pull request Mar 18, 2021

Bump sinatra from 2.0.1 to 2.0.2 cdhenry/four-burner-event-organizer#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encode route parameters using :default_encoding setting#1412

Encode route parameters using :default_encoding setting#1412
namusyaka merged 1 commit into
sinatra:masterfrom
bk2204:default-encoding-params

bk2204 commented Mar 25, 2018

Uh oh!

namusyaka commented Mar 28, 2018

Uh oh!

namusyaka left a comment

Uh oh!

bk2204 commented Mar 30, 2018

Uh oh!

bk2204 commented Mar 31, 2018

Uh oh!

namusyaka left a comment

Uh oh!

namusyaka Apr 1, 2018

Uh oh!

bk2204 Apr 1, 2018

Uh oh!

namusyaka Apr 1, 2018

Uh oh!

bk2204 commented Apr 1, 2018

Uh oh!

namusyaka left a comment

Uh oh!

namusyaka Apr 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	@params[:captures] = captures
	else
	values += params.values.flatten

Conversation

bk2204 commented Mar 25, 2018

Uh oh!

namusyaka commented Mar 28, 2018

Uh oh!

namusyaka left a comment

Choose a reason for hiding this comment

Uh oh!

bk2204 commented Mar 30, 2018

Uh oh!

bk2204 commented Mar 31, 2018

Uh oh!

namusyaka left a comment

Choose a reason for hiding this comment

Uh oh!

namusyaka Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

bk2204 Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

namusyaka Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

bk2204 commented Apr 1, 2018

Uh oh!

namusyaka left a comment

Choose a reason for hiding this comment

Uh oh!

namusyaka Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants