From 59a8ca99e46ea880e8c0515047c05c820bcddc81 Mon Sep 17 00:00:00 2001 From: Andrew De Ponte Date: Sat, 23 Jan 2016 19:29:48 -0800 Subject: [PATCH 1/8] Fix empty SCRIPT_NAME with partial match route bug Why you made the change: I made this change so that SCRIPT_NAME and PATH_INFO would be set appropriately in the scenario where a partial match route is used. How the change addresses the need: There is an issue with the current version of rewrite_partial_path_info where it does not set SCRIPT_NAME appropriately. When the request environments PATH_INFO is set, it causes the request.rack_request.path_info helper return the updated version, not the original. This is requst.rack_request.path_info eventually just reads the PATH_INFO value out of the request environment. As a side effect of this, the original code would always set the SCRIPT_NAME to "" because it would be slicing from request.rack_request.path_info[0, 0] in actuality. This change resolves this issue by first temporarily storing a dup of the original path_info and using that to determine the end index for the SCRIPT_NAME. This change also handles setting SCRIPT_NAME correctly for the somewhat unique but common case of matching the partial match exactly. I have added tests for each of these cases as I couldn't find any tests covering this before. --- lib/http_router.rb | 10 ++++++++-- test/rack/test_route.rb | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/lib/http_router.rb b/lib/http_router.rb index a7a378c..ef9e009 100644 --- a/lib/http_router.rb +++ b/lib/http_router.rb @@ -220,8 +220,14 @@ def clone(klass = self.class) end def rewrite_partial_path_info(env, request) - env['PATH_INFO'] = "/#{request.path.join('/')}" - env['SCRIPT_NAME'] += request.rack_request.path_info[0, request.rack_request.path_info.size - env['PATH_INFO'].size] + path_info_before = request.rack_request.path_info.dup + if request.path.empty? + env['PATH_INFO'] = "/" + env['SCRIPT_NAME'] += path_info_before + else + env['PATH_INFO'] = "/#{request.path.join('/')}" + env['SCRIPT_NAME'] += path_info_before[0, path_info_before.size - env['PATH_INFO'].size] + end end def rewrite_path_info(env, request) diff --git a/test/rack/test_route.rb b/test/rack/test_route.rb index dabb35a..ac1ac82 100644 --- a/test/rack/test_route.rb +++ b/test/rack/test_route.rb @@ -36,4 +36,40 @@ def test_custom_status def test_raise_error_on_invalid_status assert_raises(ArgumentError) { router.get("/index.html").redirect("/", 200) } end + + def test_path_info_from_partial_match + request_env = nil + router do + add("/sidekiq*").to { |env| request_env = env; [200, {}, []] } + end + router.call(Rack::MockRequest.env_for("/sidekiq/queues")) + assert_equal('/queues', request_env['PATH_INFO']) + end + + def test_script_name_from_partial_match + request_env = nil + router do + add("/sidekiq*").to { |env| request_env = env; [200, {}, []] } + end + router.call(Rack::MockRequest.env_for("/sidekiq/queues")) + assert_equal('/sidekiq', request_env['SCRIPT_NAME']) + end + + def test_path_info_from_partial_match_of_single + request_env = nil + router do + add("/sidekiq*").to { |env| request_env = env; [200, {}, []] } + end + router.call(Rack::MockRequest.env_for("/sidekiq")) + assert_equal('/', request_env['PATH_INFO']) + end + + def test_script_name_from_partial_match_of_single + request_env = nil + router do + add("/sidekiq*").to { |env| request_env = env; [200, {}, []] } + end + router.call(Rack::MockRequest.env_for("/sidekiq")) + assert_equal('/sidekiq', request_env['SCRIPT_NAME']) + end end From 14043cff7f1eca5a429a31bfb5c84232718bb601 Mon Sep 17 00:00:00 2001 From: Andrew De Ponte Date: Fri, 29 Jan 2016 14:43:18 -0800 Subject: [PATCH 2/8] Upgrade rake development dependency Why you made the change: I did this so it was ancient, as well as because there was a missmatch between the version of rake specified and how it was used which was having problems. Upgrading to the latest version of rake resolves this issue so that `bundle exec rake` can successfully be run. --- http_router.gemspec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http_router.gemspec b/http_router.gemspec index 0dc6d00..f40eda6 100644 --- a/http_router.gemspec +++ b/http_router.gemspec @@ -24,7 +24,7 @@ Gem::Specification.new do |s| s.add_runtime_dependency 'url_mount', '~> 0.2.1' s.add_development_dependency 'minitest', '~> 2.0.0' s.add_development_dependency 'code_stats' - s.add_development_dependency 'rake', '~> 0.8.7' + s.add_development_dependency 'rake', '~> 10.5.0' s.add_development_dependency 'rbench' s.add_development_dependency 'json' s.add_development_dependency 'phocus' From afb82c8427a3c21dee5697f0c59be696108f9bf0 Mon Sep 17 00:00:00 2001 From: Andrew De Ponte Date: Sat, 30 Jan 2016 09:14:41 -0800 Subject: [PATCH 3/8] Add basic .travis.yml Why you made the change: I did this because travis ci was failing for weird reasons and figured maybe it had something to do with the default .travis.yml. --- .travis.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 .travis.yml diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000..a974ff0 --- /dev/null +++ b/.travis.yml @@ -0,0 +1,4 @@ +language: ruby +rvm: + - 1.9.3 + - 2.3.0 From 4d14d30dd3b0dac55a413611688500f4f9017e68 Mon Sep 17 00:00:00 2001 From: Andrew De Ponte Date: Sun, 31 Jan 2016 12:01:22 -0800 Subject: [PATCH 4/8] Have travis install latest bundler Why you made the change: I did this in hopes to avoid the `NoMethodError: undefined method `spec' for nil:NilClass` while using bundler 1.7.6 which is travis default bundler version. --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index a974ff0..bdd9113 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,3 +2,4 @@ language: ruby rvm: - 1.9.3 - 2.3.0 +before_install: gem install bundler From 95f5fb306a709959c3ad69568274f6e37bd6170a Mon Sep 17 00:00:00 2001 From: Andrew De Ponte Date: Sun, 31 Jan 2016 12:17:08 -0800 Subject: [PATCH 5/8] Make first travis version the 1.9.3-p551 Why you made the change: I did this because that is the default travis was testing against before so figured we shouldn't change it. --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index bdd9113..eb84e13 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,5 +1,5 @@ language: ruby rvm: - - 1.9.3 + - 1.9.3-p551 - 2.3.0 before_install: gem install bundler From 5170ae7aa1a6617d28206f37d30671c2368f1a3d Mon Sep 17 00:00:00 2001 From: Andrew De Ponte Date: Sun, 31 Jan 2016 12:18:27 -0800 Subject: [PATCH 6/8] Revert "Upgrade rake development dependency" This reverts commit 14043cff7f1eca5a429a31bfb5c84232718bb601. --- http_router.gemspec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http_router.gemspec b/http_router.gemspec index f40eda6..0dc6d00 100644 --- a/http_router.gemspec +++ b/http_router.gemspec @@ -24,7 +24,7 @@ Gem::Specification.new do |s| s.add_runtime_dependency 'url_mount', '~> 0.2.1' s.add_development_dependency 'minitest', '~> 2.0.0' s.add_development_dependency 'code_stats' - s.add_development_dependency 'rake', '~> 10.5.0' + s.add_development_dependency 'rake', '~> 0.8.7' s.add_development_dependency 'rbench' s.add_development_dependency 'json' s.add_development_dependency 'phocus' From 1706d403785a754abc873f5b2c266527bce882f7 Mon Sep 17 00:00:00 2001 From: Andrew De Ponte Date: Sun, 31 Jan 2016 12:22:12 -0800 Subject: [PATCH 7/8] Revert "Revert "Upgrade rake development dependency"" This reverts commit 5170ae7aa1a6617d28206f37d30671c2368f1a3d. --- http_router.gemspec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http_router.gemspec b/http_router.gemspec index 0dc6d00..f40eda6 100644 --- a/http_router.gemspec +++ b/http_router.gemspec @@ -24,7 +24,7 @@ Gem::Specification.new do |s| s.add_runtime_dependency 'url_mount', '~> 0.2.1' s.add_development_dependency 'minitest', '~> 2.0.0' s.add_development_dependency 'code_stats' - s.add_development_dependency 'rake', '~> 0.8.7' + s.add_development_dependency 'rake', '~> 10.5.0' s.add_development_dependency 'rbench' s.add_development_dependency 'json' s.add_development_dependency 'phocus' From 1f967356704c7cb5dce6fbebc1bdc0dd11c5ad29 Mon Sep 17 00:00:00 2001 From: Ryan Hedges Date: Mon, 8 Feb 2016 16:56:38 -0800 Subject: [PATCH 8/8] Add URI encoding for path info I did this because sometimes the request path will be encoded. Since HTTP router unencodes the path, it loses the ability to correctly calculate the path info by performing math on the path length. This encodes the path info since it is used for correctly assigning the script name. --- lib/http_router.rb | 2 +- test/rack/test_route.rb | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/http_router.rb b/lib/http_router.rb index ef9e009..85b5167 100644 --- a/lib/http_router.rb +++ b/lib/http_router.rb @@ -225,7 +225,7 @@ def rewrite_partial_path_info(env, request) env['PATH_INFO'] = "/" env['SCRIPT_NAME'] += path_info_before else - env['PATH_INFO'] = "/#{request.path.join('/')}" + env['PATH_INFO'] = "/#{URI.encode(request.path.join('/'))}" env['SCRIPT_NAME'] += path_info_before[0, path_info_before.size - env['PATH_INFO'].size] end end diff --git a/test/rack/test_route.rb b/test/rack/test_route.rb index ac1ac82..2a7ed4c 100644 --- a/test/rack/test_route.rb +++ b/test/rack/test_route.rb @@ -72,4 +72,22 @@ def test_script_name_from_partial_match_of_single router.call(Rack::MockRequest.env_for("/sidekiq")) assert_equal('/sidekiq', request_env['SCRIPT_NAME']) end + + def test_path_info_with_encoded_request_path + request_env = nil + router do + add("/sidekiq*").to { |env| request_env = env; [200, {}, []] } + end + router.call(Rack::MockRequest.env_for("/sidekiq/queues/some%20path")) + assert_equal('/queues/some%20path', request_env['PATH_INFO']) + end + + def test_script_name_with_encoded_request_path + request_env = nil + router do + add("/sidekiq*").to { |env| request_env = env; [200, {}, []] } + end + router.call(Rack::MockRequest.env_for("/sidekiq/queues/some%20path")) + assert_equal('/sidekiq', request_env['SCRIPT_NAME']) + end end