Commit b80653bb authored by Sean McGivern's avatar Sean McGivern Committed by DJ Mountney

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
parent 0d8fba4e
...@@ -7,7 +7,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController ...@@ -7,7 +7,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
@sort = params[:sort] @sort = params[:sort]
@todos = @todos.page(params[:page]) @todos = @todos.page(params[:page])
if @todos.out_of_range? && @todos.total_pages != 0 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
end end
......
...@@ -31,7 +31,7 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -31,7 +31,7 @@ class Projects::IssuesController < Projects::ApplicationController
@issuable_meta_data = issuable_meta_data(@issues, @collection_type) @issuable_meta_data = issuable_meta_data(@issues, @collection_type)
if @issues.out_of_range? && @issues.total_pages != 0 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 end
if params[:label_name].present? if params[:label_name].present?
......
...@@ -43,7 +43,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -43,7 +43,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@issuable_meta_data = issuable_meta_data(@merge_requests, @collection_type) @issuable_meta_data = issuable_meta_data(@merge_requests, @collection_type)
if @merge_requests.out_of_range? && @merge_requests.total_pages != 0 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 end
if params[:label_name].present? if params[:label_name].present?
......
---
title: Fix for open redirect vulnerabilities in todos, issues, and MR controllers.
merge_request:
author:
...@@ -35,6 +35,13 @@ describe Dashboard::TodosController do ...@@ -35,6 +35,13 @@ describe Dashboard::TodosController do
expect(assigns(:todos).current_page).to eq(last_page) expect(assigns(:todos).current_page).to eq(last_page)
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
end 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 end
......
...@@ -83,6 +83,17 @@ describe Projects::IssuesController do ...@@ -83,6 +83,17 @@ describe Projects::IssuesController do
expect(assigns(:issues).current_page).to eq(last_page) expect(assigns(:issues).current_page).to eq(last_page)
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
end 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
end end
......
...@@ -176,6 +176,18 @@ describe Projects::MergeRequestsController do ...@@ -176,6 +176,18 @@ describe Projects::MergeRequestsController do
expect(assigns(:merge_requests).current_page).to eq(last_page) expect(assigns(:merge_requests).current_page).to eq(last_page)
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
end 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 end
context 'when filtering by opened state' do context 'when filtering by opened state' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment