Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix to ignore empty captures from params #1390

Merged
merged 2 commits into from
Feb 21, 2018
Merged

Fix to ignore empty captures from params #1390

merged 2 commits into from
Feb 21, 2018
+16 −1

Conversation

iguchi1124
Copy link
Contributor

@iguchi1124 iguchi1124 commented Feb 19, 2018 •
edited
Loading

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
iguchi1124 commented Feb 19, 2018
View reviewed changes
lib/sinatra/base.rb Outdated
add_filter(:after, path, options, &block)
end

# add a filter
def add_filter(type, path = /.*/, **options, &block)
def add_filter(type, path, **options, &block)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@namusyaka
Copy link
Member

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

@namusyaka namusyaka added this to the v2.0.2 milestone Feb 19, 2018
namusyaka
namusyaka requested changes Feb 20, 2018
View reviewed changes
Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lib/sinatra/base.rb Outdated
add_filter(:before, path, options, &block)
end

# Define an after filter; runs after all requests within the same
# context as route handlers and may access/modify the request and
# response.
def after(path = /.*/, **options, &block)
def after(path = /(.*)/, **options, &block)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't change this regexp.

lib/sinatra/base.rb Outdated
add_filter(:after, path, options, &block)
end

# add a filter
def add_filter(type, path = /.*/, **options, &block)
def add_filter(type, path, **options, &block)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@namusyaka
Copy link
Member

@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
Copy link
Contributor Author

Thanks! Updated pull request description and code changes!

namusyaka
namusyaka approved these changes Feb 20, 2018
View reviewed changes
Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lib/sinatra/base.rb Outdated
@@ -1029,7 +1029,7 @@ def process_route(pattern, conditions, block = nil, values = [])
if regexp_exists
captures = pattern.match(route).captures
values += captures
@params[:captures] = captures
@params[:captures] = captures if !captures.nil? && !captures.empty?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored and fixup in this commit 51fe0fc

@iguchi1124 iguchi1124 changed the title Fix captured path params Fix to ignore empty captures from params Feb 20, 2018
@namusyaka
Copy link
Member

Great!

@namusyaka namusyaka merged commit 05cb1fe into sinatra:master Feb 21, 2018
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Sep 23, 2018
www/ruby-sinatra: update to 2.0.4 with ruby-sinatra-contrib
945334f 
## 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
@dependabot dependabot bot mentioned this pull request Mar 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@namusyaka namusyaka namusyaka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.0.2
Development

Successfully merging this pull request may close these issues.

Before filter adds empty captures to params
2 participants
@iguchi1124 @namusyaka