Merge branch '26781-enforce-auth-checks-on-uploads' into 'master'

Enforce auth checks on uploads

See merge request gitlab-org/gitlab!80117

app/controllers/concerns/uploads_actions.rb

@@ -142,6 +142,14 @@ module UploadsActions
     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

app/controllers/groups/uploads_controller.rb

@@ -4,7 +4,7 @@ class Groups::UploadsController < Groups::ApplicationController
   include UploadsActions
   include WorkhorseRequest
 
-  skip_before_action :group, if: -> { action_name == 'show' && embeddable? }
+  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]

app/controllers/projects/uploads_controller.rb

@@ -6,7 +6,7 @@ class Projects::UploadsController < Projects::ApplicationController
 
   # These will kick you out if you don't have access.
   skip_before_action :project, :repository,
-    if: -> { action_name == 'show' && embeddable? }
+    if: -> { bypass_auth_checks_on_uploads? }
 
   before_action :authorize_upload_file!, only: [:create, :authorize]
   before_action :verify_workhorse_api!, only: [:authorize]

config/feature_flags/development/enforce_auth_checks_on_uploads.yml

+---
+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

spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb

@@ -205,12 +205,32 @@ RSpec.shared_examples 'handle uploads' do
               allow_any_instance_of(FileUploader).to receive(:image?).and_return(true)
             end
 
+            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)
+                end
+
+                it "responds with status 302" do
+                  show_upload
+
+                  expect(response).to have_gitlab_http_status(:redirect)
+                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
 
           context "when the file is not an image" do
             before do
@@ -276,12 +296,32 @@ RSpec.shared_examples 'handle uploads' do
                 allow_any_instance_of(FileUploader).to receive(:image?).and_return(true)
               end
 
+              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)
+                  end
+
+                  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
 
             context "when the file is not an image" do
               before do