Skip to content

Commit

Permalink
Ignore script_name query parameter in generated URLs
Browse files Browse the repository at this point in the history
  • Loading branch information
mislav committed Sep 20, 2016
1 parent 4626f6e commit ec9b985
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 3 deletions.
4 changes: 3 additions & 1 deletion lib/will_paginate/view_helpers/action_view.rb
Expand Up @@ -99,6 +99,8 @@ def infer_collection_from_controller
class LinkRenderer < ViewHelpers::LinkRenderer
protected

GET_PARAMS_BLACKLIST = [:script_name]

def default_url_params
{}
end
Expand All @@ -118,7 +120,7 @@ def url(page)

def merge_get_params(url_params)
if @template.respond_to? :request and @template.request and @template.request.get?
symbolized_update(url_params, @template.params)
symbolized_update(url_params, @template.params, GET_PARAMS_BLACKLIST)
end
url_params
end
Expand Down
5 changes: 3 additions & 2 deletions lib/will_paginate/view_helpers/link_renderer.rb
Expand Up @@ -114,11 +114,12 @@ def rel_value(page)
end
end

def symbolized_update(target, other)
def symbolized_update(target, other, blacklist = nil)
other.each do |key, value|
key = key.to_sym
existing = target[key]

next if blacklist && blacklist.include?(key)

if value.is_a?(Hash) and (existing.is_a?(Hash) or existing.nil?)
symbolized_update(existing || (target[key] = {}), value)
else
Expand Down
7 changes: 7 additions & 0 deletions spec/view_helpers/action_view_spec.rb
Expand Up @@ -201,6 +201,13 @@ def renderer.gap() '<span class="my-gap">~~</span>' end
assert_no_links_match /99/
assert_no_links_match /ftp/
end

it "doesn't allow tampering with script_name" do
request.params :script_name => 'p0wned'
paginate
assert_links_match %r{^/foo/bar}
assert_no_links_match /p0wned/
end

it "should not preserve parameters on POST" do
request.post
Expand Down

7 comments on commit ec9b985

@aripollak
Copy link

Choose a reason for hiding this comment

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

@mislav was this a security vulnerability? Should people be upgrading?

@mislav
Copy link
Owner Author

@mislav mislav commented on ec9b985 Oct 28, 2016

Choose a reason for hiding this comment

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

Yes

@aripollak
Copy link

Choose a reason for hiding this comment

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

@mislav could you explain a bit more about the vulnerability? I have a hard time finding what tampering with the script_name could result in.

@phillmv
Copy link

Choose a reason for hiding this comment

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

Hey @mislav, if in addition to providing a short description, you could also just confirm the upgradeable version ranges @aripollak listed here, that would be highly appreciated!

@mislav
Copy link
Owner Author

@mislav mislav commented on ec9b985 Oct 28, 2016

Choose a reason for hiding this comment

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

Yep you people are right about upgrade ranges. Is anything not clear from my release notes?

@phillmv
Copy link

@phillmv phillmv commented on ec9b985 Nov 1, 2016

Choose a reason for hiding this comment

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

Ah ha, are you referring to this page?

Everyone has a slightly different release process, so it's better to be safe than sorry :).

Re: the description, it's useful to know what the specific security vulnerability was so we can rank how important fixing it is. If you recall off the top of your head rather, that'd also save us having to figure out what went wrong. At a glance I can't seem to find anything in the issues tab.

I'm mindful of the drudgery of open source work :P, and I'm sorry if it seems we're just adding to it.

@mislav
Copy link
Owner Author

@mislav mislav commented on ec9b985 Nov 1, 2016

Choose a reason for hiding this comment

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

Sure thing: passing maliciously crafted content in script_name GET parameter could cause pagination links to get output pointing to another site, or even XSS.

Please sign in to comment.