From 1cefb73a9c067b1e2367a28b5c6852cf52d6b886 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 7 Mar 2016 10:06:54 +0100 Subject: [PATCH 1/3] Check redirect path in the continue_params Fixes https://dev.gitlab.org/gitlab/gitlabhq/issues/2649 https://gitlab.com/gitlab-org/gitlab-ce/issues/13956 --- app/controllers/concerns/continue_to_params.rb | 13 +++++++++++++ app/controllers/projects/forks_controller.rb | 13 ++----------- app/controllers/projects/imports_controller.rb | 12 ++---------- 3 files changed, 17 insertions(+), 21 deletions(-) create mode 100644 app/controllers/concerns/continue_to_params.rb diff --git a/app/controllers/concerns/continue_to_params.rb b/app/controllers/concerns/continue_to_params.rb new file mode 100644 index 0000000000..8b6c705196 --- /dev/null +++ b/app/controllers/concerns/continue_to_params.rb @@ -0,0 +1,13 @@ +module ContinueToParams + extend ActiveSupport::Concern + + def continue_params + continue_params = params[:continue] + return nil unless continue_params + + continue_params = continue_params.permit(:to, :notice, :notice_now) + continue_params[:to] = root_url unless continue_params[:to].start_with?('/') + + continue_params + end +end diff --git a/app/controllers/projects/forks_controller.rb b/app/controllers/projects/forks_controller.rb index 7b202f3862..c4884c13b1 100644 --- a/app/controllers/projects/forks_controller.rb +++ b/app/controllers/projects/forks_controller.rb @@ -1,4 +1,6 @@ class Projects::ForksController < Projects::ApplicationController + include ContinueToParams + # Authorize before_action :require_non_empty_project before_action :authorize_download_code! @@ -53,15 +55,4 @@ class Projects::ForksController < Projects::ApplicationController render :error end end - - private - - def continue_params - continue_params = params[:continue] - if continue_params - continue_params.permit(:to, :notice, :notice_now) - else - nil - end - end end diff --git a/app/controllers/projects/imports_controller.rb b/app/controllers/projects/imports_controller.rb index 196996f175..3756fc9139 100644 --- a/app/controllers/projects/imports_controller.rb +++ b/app/controllers/projects/imports_controller.rb @@ -1,4 +1,6 @@ class Projects::ImportsController < Projects::ApplicationController + include ContinueToParams + # Authorize before_action :authorize_admin_project! before_action :require_no_repo, only: [:new, :create] @@ -44,16 +46,6 @@ class Projects::ImportsController < Projects::ApplicationController private - def continue_params - continue_params = params[:continue] - - if continue_params - continue_params.permit(:to, :notice, :notice_now) - else - nil - end - end - def finished_notice if @project.forked? 'The project was successfully forked.' From dfb96ed84bd7533abc411b148f0b27bf65321b3e Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 7 Mar 2016 10:36:16 +0100 Subject: [PATCH 2/3] ContinueToParams -> ContinueParams --- CHANGELOG | 1 + .../concerns/{continue_to_params.rb => continue_params.rb} | 4 ++-- app/controllers/projects/forks_controller.rb | 2 +- app/controllers/projects/imports_controller.rb | 2 +- 4 files changed, 5 insertions(+), 4 deletions(-) rename app/controllers/concerns/{continue_to_params.rb => continue_params.rb} (69%) diff --git a/CHANGELOG b/CHANGELOG index d4554b9619..1929b6306d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -45,6 +45,7 @@ v 8.5.3 - Show commit message in JIRA mention comment - Makes issue page and merge request page usable on mobile browsers. - Improved UI for profile settings + - Continue parameters are checked to ensure redirection goes to the same instance v 8.5.2 - Fix sidebar overlapping content when screen width was below 1200px diff --git a/app/controllers/concerns/continue_to_params.rb b/app/controllers/concerns/continue_params.rb similarity index 69% rename from app/controllers/concerns/continue_to_params.rb rename to app/controllers/concerns/continue_params.rb index 8b6c705196..2ff7250922 100644 --- a/app/controllers/concerns/continue_to_params.rb +++ b/app/controllers/concerns/continue_params.rb @@ -1,4 +1,4 @@ -module ContinueToParams +module ContinueParams extend ActiveSupport::Concern def continue_params @@ -6,7 +6,7 @@ module ContinueToParams return nil unless continue_params continue_params = continue_params.permit(:to, :notice, :notice_now) - continue_params[:to] = root_url unless continue_params[:to].start_with?('/') + return unless continue_params[:to] && continue_params[:to].start_with?('/') continue_params end diff --git a/app/controllers/projects/forks_controller.rb b/app/controllers/projects/forks_controller.rb index c4884c13b1..a1b8632df9 100644 --- a/app/controllers/projects/forks_controller.rb +++ b/app/controllers/projects/forks_controller.rb @@ -1,5 +1,5 @@ class Projects::ForksController < Projects::ApplicationController - include ContinueToParams + include ContinueParams # Authorize before_action :require_non_empty_project diff --git a/app/controllers/projects/imports_controller.rb b/app/controllers/projects/imports_controller.rb index 3756fc9139..7756f0f0ed 100644 --- a/app/controllers/projects/imports_controller.rb +++ b/app/controllers/projects/imports_controller.rb @@ -1,5 +1,5 @@ class Projects::ImportsController < Projects::ApplicationController - include ContinueToParams + include ContinueParams # Authorize before_action :authorize_admin_project! From 5352ec2e21ba72d77a542b158ce1a98a1a3a9389 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 7 Mar 2016 11:45:14 +0100 Subject: [PATCH 3/3] Fix denting and spec --- CHANGELOG | 2 +- app/controllers/concerns/continue_params.rb | 2 +- spec/controllers/projects/imports_controller_spec.rb | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 1929b6306d..7c63414e58 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -29,6 +29,7 @@ v 8.6.0 (unreleased) - Add main language of a project in the list of projects (Tiago Botelho) - Add ability to show archived projects on dashboard, explore and group pages - Move group activity to separate page + - Continue parameters are checked to ensure redirection goes to the same instance v 8.5.5 - Ensure removing a project removes associated Todo entries @@ -45,7 +46,6 @@ v 8.5.3 - Show commit message in JIRA mention comment - Makes issue page and merge request page usable on mobile browsers. - Improved UI for profile settings - - Continue parameters are checked to ensure redirection goes to the same instance v 8.5.2 - Fix sidebar overlapping content when screen width was below 1200px diff --git a/app/controllers/concerns/continue_params.rb b/app/controllers/concerns/continue_params.rb index 2ff7250922..0a995c45bd 100644 --- a/app/controllers/concerns/continue_params.rb +++ b/app/controllers/concerns/continue_params.rb @@ -5,7 +5,7 @@ module ContinueParams continue_params = params[:continue] return nil unless continue_params - continue_params = continue_params.permit(:to, :notice, :notice_now) + continue_params = continue_params.permit(:to, :notice, :notice_now) return unless continue_params[:to] && continue_params[:to].start_with?('/') continue_params diff --git a/spec/controllers/projects/imports_controller_spec.rb b/spec/controllers/projects/imports_controller_spec.rb index 0147bd2b95..2acbba469e 100644 --- a/spec/controllers/projects/imports_controller_spec.rb +++ b/spec/controllers/projects/imports_controller_spec.rb @@ -19,7 +19,7 @@ describe Projects::ImportsController do end it 'sets flash.now if params is present' do - get :show, namespace_id: project.namespace.to_param, project_id: project.to_param, continue: { notice_now: 'Started' } + get :show, namespace_id: project.namespace.to_param, project_id: project.to_param, continue: { to: '/', notice_now: 'Started' } expect(flash.now[:notice]).to eq 'Started' end @@ -45,7 +45,7 @@ describe Projects::ImportsController do end it 'sets flash.now if params is present' do - get :show, namespace_id: project.namespace.to_param, project_id: project.to_param, continue: { notice_now: 'In progress' } + get :show, namespace_id: project.namespace.to_param, project_id: project.to_param, continue: { to: '/', notice_now: 'In progress' } expect(flash.now[:notice]).to eq 'In progress' end