From bbdfbd2318d950770515b539b84500c910e3070a Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Mon, 7 Feb 2022 16:06:13 -0800 Subject: [PATCH 1/3] Enforce auth checks on uploads Uploads should also pass through auth checks, since they could contain privileged information, but be part of an otherwise private project or group. --- app/controllers/groups/uploads_controller.rb | 2 -- app/controllers/projects/uploads_controller.rb | 4 +--- .../controllers/uploads_actions_shared_examples.rb | 8 ++++---- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/app/controllers/groups/uploads_controller.rb b/app/controllers/groups/uploads_controller.rb index 49249f87d31d92..60814c975a4c8f 100644 --- a/app/controllers/groups/uploads_controller.rb +++ b/app/controllers/groups/uploads_controller.rb @@ -4,8 +4,6 @@ class Groups::UploadsController < Groups::ApplicationController include UploadsActions include WorkhorseRequest - skip_before_action :group, if: -> { action_name == 'show' && embeddable? } - before_action :authorize_upload_file!, only: [:create, :authorize] before_action :verify_workhorse_api!, only: [:authorize] diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb index c15768e7bbb634..a2d8df8856ef9d 100644 --- a/app/controllers/projects/uploads_controller.rb +++ b/app/controllers/projects/uploads_controller.rb @@ -5,9 +5,7 @@ class Projects::UploadsController < Projects::ApplicationController include WorkhorseRequest # These will kick you out if you don't have access. - skip_before_action :project, :repository, - if: -> { action_name == 'show' && embeddable? } - + # before_action :authorize_upload_file!, only: [:create, :authorize] before_action :verify_workhorse_api!, only: [:authorize] diff --git a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb index 62b35923bcd267..94ba2ffa519e79 100644 --- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb @@ -205,10 +205,10 @@ allow_any_instance_of(FileUploader).to receive(:image?).and_return(true) end - it "responds with status 200" do + it "responds with status 302" do show_upload - expect(response).to have_gitlab_http_status(:ok) + expect(response).to have_gitlab_http_status(:redirect) end end @@ -276,10 +276,10 @@ allow_any_instance_of(FileUploader).to receive(:image?).and_return(true) end - it "responds with status 200" do + it "responds with status 404" do show_upload - expect(response).to have_gitlab_http_status(:ok) + expect(response).to have_gitlab_http_status(:not_found) end end -- GitLab From 7b740b2f5e4f134c49746c898349ce26f4f92c12 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Tue, 8 Feb 2022 09:19:05 -0800 Subject: [PATCH 2/3] Add enforce_auth_checks_on_uploads feature flag --- app/controllers/concerns/uploads_actions.rb | 8 +++ app/controllers/groups/uploads_controller.rb | 2 + .../projects/uploads_controller.rb | 4 +- .../enforce_auth_checks_on_uploads.yml | 8 +++ .../uploads_actions_shared_examples.rb | 52 ++++++++++++++++--- 5 files changed, 67 insertions(+), 7 deletions(-) create mode 100644 config/feature_flags/development/enforce_auth_checks_on_uploads.yml diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index f489de42864ec5..e1bfe92f61bb92 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -142,6 +142,14 @@ def embeddable? uploader && uploader.exists? && uploader.embeddable? end + def bypass_auth_checks_on_uploads? + if ::Feature.enabled?(:enforce_auth_checks_on_uploads, default_enabled: :yaml) + false + else + action_name == 'show' && embeddable? + end + end + def find_model nil end diff --git a/app/controllers/groups/uploads_controller.rb b/app/controllers/groups/uploads_controller.rb index 60814c975a4c8f..387f7be56cd5bb 100644 --- a/app/controllers/groups/uploads_controller.rb +++ b/app/controllers/groups/uploads_controller.rb @@ -4,6 +4,8 @@ class Groups::UploadsController < Groups::ApplicationController include UploadsActions include WorkhorseRequest + skip_before_action :group, if: -> { bypass_auth_checks_on_uploads? } + before_action :authorize_upload_file!, only: [:create, :authorize] before_action :verify_workhorse_api!, only: [:authorize] diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb index a2d8df8856ef9d..ed5bd73d6d14cf 100644 --- a/app/controllers/projects/uploads_controller.rb +++ b/app/controllers/projects/uploads_controller.rb @@ -5,7 +5,9 @@ class Projects::UploadsController < Projects::ApplicationController include WorkhorseRequest # These will kick you out if you don't have access. - # + skip_before_action :project, :repository, + if: -> { bypass_auth_checks_on_uploads? } + before_action :authorize_upload_file!, only: [:create, :authorize] before_action :verify_workhorse_api!, only: [:authorize] diff --git a/config/feature_flags/development/enforce_auth_checks_on_uploads.yml b/config/feature_flags/development/enforce_auth_checks_on_uploads.yml new file mode 100644 index 00000000000000..d0cbe123a8f804 --- /dev/null +++ b/config/feature_flags/development/enforce_auth_checks_on_uploads.yml @@ -0,0 +1,8 @@ +--- +name: enforce_auth_checks_on_uploads +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80117 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/352291 +milestone: '14.8' +type: development +group: group::code review +default_enabled: false diff --git a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb index 94ba2ffa519e79..570fddd402a381 100644 --- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb @@ -205,10 +205,30 @@ allow_any_instance_of(FileUploader).to receive(:image?).and_return(true) end - it "responds with status 302" do - show_upload + context "extract_mr_diff_commit_deletions feature flag" do + context "with flag enabled" do + before do + stub_feature_flags(enforce_auth_checks_on_uploads: true) + end + + it "responds with status 302" do + show_upload + + expect(response).to have_gitlab_http_status(:redirect) + end + end - expect(response).to have_gitlab_http_status(:redirect) + context "with flag disabled" do + before do + stub_feature_flags(enforce_auth_checks_on_uploads: false) + end + + it "responds with status 200" do + show_upload + + expect(response).to have_gitlab_http_status(:ok) + end + end end end @@ -276,10 +296,30 @@ allow_any_instance_of(FileUploader).to receive(:image?).and_return(true) end - it "responds with status 404" do - show_upload + context "extract_mr_diff_commit_deletions feature flag" do + context "with flag enabled" do + before do + stub_feature_flags(enforce_auth_checks_on_uploads: true) + end - expect(response).to have_gitlab_http_status(:not_found) + it "responds with status 404" do + show_upload + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context "with flag disabled" do + before do + stub_feature_flags(enforce_auth_checks_on_uploads: false) + end + + it "responds with status 200" do + show_upload + + expect(response).to have_gitlab_http_status(:ok) + end + end end end -- GitLab From 8999ca18545d2a666a38ab7a30bf0faed522439a Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Wed, 9 Feb 2022 09:09:46 +0000 Subject: [PATCH 3/3] Fix typo in spec context --- .../controllers/uploads_actions_shared_examples.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb index 570fddd402a381..5ed8dc7ce981a0 100644 --- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb @@ -205,7 +205,7 @@ allow_any_instance_of(FileUploader).to receive(:image?).and_return(true) end - context "extract_mr_diff_commit_deletions feature flag" do + context "enforce_auth_checks_on_uploads feature flag" do context "with flag enabled" do before do stub_feature_flags(enforce_auth_checks_on_uploads: true) @@ -296,7 +296,7 @@ allow_any_instance_of(FileUploader).to receive(:image?).and_return(true) end - context "extract_mr_diff_commit_deletions feature flag" do + context "enforce_auth_checks_on_uploads feature flag" do context "with flag enabled" do before do stub_feature_flags(enforce_auth_checks_on_uploads: true) -- GitLab