diff options
author | Sean McGivern <sean@gitlab.com> | 2017-04-06 01:52:19 +0300 |
---|---|---|
committer | DJ Mountney <david@twkie.net> | 2017-04-06 02:24:34 +0300 |
commit | 0a4c76aaaa136a34fbd3966d5206c106ff2ef526 (patch) | |
tree | 47c99334be2b4d81db419f3231d5656cbf3bb90d | |
parent | 9e31e2a68949b2ba9115f7e6496051d12b56b9de (diff) |
Merge branch 'open-redirect-host-fix' into 'security'
Fix for three open redirect vulns using redirect_to url_for(params.merge)))
See merge request !2082
7 files changed, 37 insertions, 3 deletions
diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb index e3933e3d7b1..9d7e9656b57 100644 --- a/app/controllers/dashboard/todos_controller.rb +++ b/app/controllers/dashboard/todos_controller.rb @@ -5,7 +5,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController @sort = params[:sort] @todos = @todos.page(params[:page]) if @todos.out_of_range? && @todos.total_pages != 0 - redirect_to url_for(params.merge(page: @todos.total_pages)) + redirect_to url_for(params.merge(page: @todos.total_pages, only_path: true)) end end diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index a019351a1a2..ef393985ac7 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -26,7 +26,7 @@ class Projects::IssuesController < Projects::ApplicationController @issues = issues_collection @issues = @issues.page(params[:page]) if @issues.out_of_range? && @issues.total_pages != 0 - return redirect_to url_for(params.merge(page: @issues.total_pages)) + return redirect_to url_for(params.merge(page: @issues.total_pages, only_path: true)) end if params[:label_name].present? diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 279e686b2e2..a76223d1af1 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -39,7 +39,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController @merge_requests = merge_requests_collection @merge_requests = @merge_requests.page(params[:page]) if @merge_requests.out_of_range? && @merge_requests.total_pages != 0 - return redirect_to url_for(params.merge(page: @merge_requests.total_pages)) + return redirect_to url_for(params.merge(page: @merge_requests.total_pages, only_path: true)) end if params[:label_name].present? diff --git a/changelogs/unreleased/open-redirect-host-field.yml b/changelogs/unreleased/open-redirect-host-field.yml new file mode 100644 index 00000000000..bed4b47cf04 --- /dev/null +++ b/changelogs/unreleased/open-redirect-host-field.yml @@ -0,0 +1,4 @@ +--- +title: Fix for open redirect vulnerabilities in todos, issues, and MR controllers. +merge_request: +author: diff --git a/spec/controllers/dashboard/todos_controller_spec.rb b/spec/controllers/dashboard/todos_controller_spec.rb index 19fbc2f7748..707a20c37f2 100644 --- a/spec/controllers/dashboard/todos_controller_spec.rb +++ b/spec/controllers/dashboard/todos_controller_spec.rb @@ -32,6 +32,13 @@ describe Dashboard::TodosController do expect(assigns(:todos).current_page).to eq(last_page) expect(response).to have_http_status(200) end + + it 'does not redirect to external sites when provided a host field' do + external_host = "www.example.com" + get :index, page: (last_page + 1).to_param, host: external_host + + expect(response).to redirect_to(dashboard_todos_path(page: last_page)) + end end end end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index ebd9272dfc2..07d0e2a2164 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -81,6 +81,17 @@ describe Projects::IssuesController do expect(assigns(:issues).current_page).to eq(last_page) expect(response).to have_http_status(200) end + + it 'does not redirect to external sites when provided a host field' do + external_host = "www.example.com" + get :index, + namespace_id: project.namespace.to_param, + project_id: project, + page: (last_page + 1).to_param, + host: external_host + + expect(response).to redirect_to(namespace_project_issues_path(page: last_page, state: controller.params[:state], scope: controller.params[:scope])) + end end end diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 263cb009e62..b3fe9ce378c 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -150,6 +150,18 @@ describe Projects::MergeRequestsController do expect(assigns(:merge_requests).current_page).to eq(last_page) expect(response).to have_http_status(200) end + + it 'does not redirect to external sites when provided a host field' do + external_host = "www.example.com" + get :index, + namespace_id: project.namespace.to_param, + project_id: project, + state: 'opened', + page: (last_page + 1).to_param, + host: external_host + + expect(response).to redirect_to(namespace_project_merge_requests_path(page: last_page, state: controller.params[:state], scope: controller.params[:scope])) + end end context 'when filtering by opened state' do |