Fix to ignore empty captures from params by iguchi1124 · Pull Request #1390 · sinatra/sinatra

iguchi1124 · 2018-02-19T15:46:48Z

I'm trying to fix #1388

params should be empty hash if requests doesn't matches any regex of routings. But It was returned {"captures" => []}. so I fixed to return empty hash object.

before do
  # noop
end

get "/" do
  params.to_s # returns `{}`
end

iguchi1124 · 2018-02-19T15:48:01Z


      # add a filter
-      def add_filter(type, path = /.*/, **options, &block)
+      def add_filter(type, path, **options, &block)


I think this method's argument of path doesn't need default value

Potentially breaking compatibility, I don't think there is a reason for breaking it.

namusyaka · 2018-02-19T16:13:35Z

@iguchi1124 Thank you for the patch, I'll take care of it soon.

namusyaka

The main issue is that empty captures, which we do not expect, will be added to params.
Your change not only introduces unintended behavioral changes, it doesn't solve the issue.

namusyaka · 2018-02-20T12:46:41Z

      # context as route handlers and may access/modify the request and
      # response.
-      def after(path = /.*/, **options, &block)
+      def after(path = /(.*)/, **options, &block)


You shouldn't change this regexp.

namusyaka · 2018-02-20T12:48:30Z


      # add a filter
-      def add_filter(type, path = /.*/, **options, &block)
+      def add_filter(type, path, **options, &block)


Potentially breaking compatibility, I don't think there is a reason for breaking it.

namusyaka · 2018-02-20T12:52:34Z

@iguchi1124 If you're still planning on addressing the problem, please consider the following points:

Needs a test.
I guess the patch would be something like:

diff --git a/lib/sinatra/base.rb b/lib/sinatra/base.rb
index b2dff314..5d1a09f3 100644
--- a/lib/sinatra/base.rb
+++ b/lib/sinatra/base.rb
@@ -1029,7 +1029,7 @@ module Sinatra
       if regexp_exists
         captures           = pattern.match(route).captures
         values            += captures
-        @params[:captures] = captures
+        @params[:captures] = captures if captures && !captures.empty?
       else
         values += params.values.flatten
       end

iguchi1124 · 2018-02-20T15:30:47Z

Thanks! Updated pull request description and code changes!

namusyaka

Looks good with a nit, thanks for your contribution!
I'm pleased to see your contribution.

namusyaka · 2018-02-20T16:21:19Z

        captures           = pattern.match(route).captures
        values            += captures
-        @params[:captures] = captures
+        @params[:captures] = captures if !captures.nil? && !captures.empty?


If you hope that the captures checking is using Object#nil?, you could simplify the condition by using De Morgan's laws.
unless captures.nil? || captures.empty?

refactored and fixup in this commit 51fe0fc

namusyaka · 2018-02-21T14:29:18Z

Great!

## 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

iguchi1124 commented Feb 19, 2018

View reviewed changes

namusyaka added this to the v2.0.2 milestone Feb 19, 2018

namusyaka requested changes Feb 20, 2018

View reviewed changes

namusyaka approved these changes Feb 20, 2018

View reviewed changes

iguchi1124 added 2 commits February 21, 2018 02:17

Fix params when http request path mismatching with filter regex

51fe0fc

Add filtered and returns empty params test

06f8c83

iguchi1124 changed the title ~~Fix captured path params~~ Fix to ignore empty captures from params Feb 20, 2018

namusyaka merged commit 05cb1fe into sinatra:master Feb 21, 2018

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

Fix to ignore empty captures from params#1390

Fix to ignore empty captures from params#1390
namusyaka merged 2 commits into
sinatra:masterfrom
iguchi1124:fix-captured-paths

iguchi1124 commented Feb 19, 2018 •

edited

Loading

Uh oh!

iguchi1124 Feb 19, 2018

Uh oh!

namusyaka Feb 20, 2018

Uh oh!

namusyaka commented Feb 19, 2018

Uh oh!

namusyaka left a comment

Uh oh!

namusyaka Feb 20, 2018

Uh oh!

namusyaka Feb 20, 2018

Uh oh!

namusyaka commented Feb 20, 2018

Uh oh!

iguchi1124 commented Feb 20, 2018

Uh oh!

namusyaka left a comment

Uh oh!

namusyaka Feb 20, 2018

Uh oh!

iguchi1124 Feb 20, 2018

Uh oh!

namusyaka commented Feb 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

iguchi1124 commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iguchi1124 Feb 19, 2018

Choose a reason for hiding this comment

Uh oh!

namusyaka Feb 20, 2018

Choose a reason for hiding this comment

Uh oh!

namusyaka commented Feb 19, 2018

Uh oh!

namusyaka left a comment

Choose a reason for hiding this comment

Uh oh!

namusyaka Feb 20, 2018

Choose a reason for hiding this comment

Uh oh!

namusyaka Feb 20, 2018

Choose a reason for hiding this comment

Uh oh!

namusyaka commented Feb 20, 2018

Uh oh!

iguchi1124 commented Feb 20, 2018

Uh oh!

namusyaka left a comment

Choose a reason for hiding this comment

Uh oh!

namusyaka Feb 20, 2018

Choose a reason for hiding this comment

Uh oh!

iguchi1124 Feb 20, 2018

Choose a reason for hiding this comment

Uh oh!

namusyaka commented Feb 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

iguchi1124 commented Feb 19, 2018 •

edited

Loading