Merge branch 'sh-disable-formats-in-uploads-routes' into 'master'

Fix upload redirections when project has moved

Closes #196232

See merge request !22822

app/controllers/projects/uploads_controller.rb

@@ -29,4 +29,14 @@ def find_model
 
     Project.find_by_full_path("#{namespace}/#{id}")
   end
+
+  # Overrides ApplicationController#build_canonical_path since there are
+  # multiple routes that match project uploads:
+  # https://gitlab.com/gitlab-org/gitlab/issues/196396
+  def build_canonical_path(project)
+    return super unless action_name == 'show'
+    return super unless params[:secret] && params[:filename]
+
+    show_namespace_project_uploads_url(project.namespace.to_param, project.to_param, params[:secret], params[:filename])
+  end
 end

changelogs/unreleased/sh-disable-formats-in-uploads-routes.yml

+---
+title: Fix upload redirections when project has moved
+merge_request: 22822
+author:
+type: fixed

config/routes/group.rb

@@ -68,7 +68,7 @@
 
     resources :uploads, only: [:create] do
       collection do
-        get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }
+        get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }, format: false, defaults: { format: nil }
         post :authorize
       end
     end

config/routes/project.rb

@@ -452,7 +452,7 @@
 
       resources :uploads, only: [:create] do
         collection do
-          get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }
+          get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }, format: false, defaults: { format: nil }
           post :authorize
         end
       end

config/routes/uploads.rb

@@ -21,9 +21,11 @@
       as: 'appearance_upload'
 
   # Project markdown uploads
+  # DEPRECATED: Remove this in GitLab 13.0 because this is redundant to show_namespace_project_uploads
+  # https://gitlab.com/gitlab-org/gitlab/issues/196396
   get ":namespace_id/:project_id/:secret/:filename",
-    to:           "projects/uploads#show",
-    constraints:  { namespace_id: /[a-zA-Z.0-9_\-]+/, project_id: /[a-zA-Z.0-9_\-]+/, filename: %r{[^/]+} }
+    to: redirect("%{namespace_id}/%{project_id}/uploads/%{secret}/%{filename}"),
+    constraints:  { namespace_id: /[a-zA-Z.0-9_\-]+/, project_id: /[a-zA-Z.0-9_\-]+/, filename: %r{[^/]+} }, format: false, defaults: { format: nil }
 
   # create uploads for models, snippets (notes) available for now
   post ':model',

spec/controllers/groups/uploads_controller_spec.rb

@@ -19,6 +19,22 @@
     let(:uploader_class) { NamespaceFileUploader }
   end
 
+  context 'with a moved group' do
+    let!(:upload) { create(:upload, :issuable_upload, :with_file, model: model) }
+    let(:group) { model }
+    let(:old_path) { group.to_param + 'old' }
+    let!(:redirect_route) { model.redirect_routes.create(path: old_path) }
+    let(:upload_path) { File.basename(upload.path) }
+
+    it 'redirects to a file with the proper extension' do
+      get :show, params: { group_id: old_path, filename: upload_path, secret: upload.secret }
+
+      expect(response.location).to eq(show_group_uploads_url(group, upload.secret, upload_path))
+      expect(response.location).to end_with(upload.path)
+      expect(response).to have_gitlab_http_status(:redirect)
+    end
+  end
+
   def post_authorize(verified: true)
     request.headers.merge!(workhorse_internal_api_request_header) if verified
 

spec/controllers/projects/uploads_controller_spec.rb

@@ -25,6 +25,21 @@
     end
   end
 
+  context 'with a moved project' do
+    let!(:upload) { create(:upload, :issuable_upload, :with_file, model: model) }
+    let(:project) { model }
+    let(:upload_path) { File.basename(upload.path) }
+    let!(:redirect_route) { project.redirect_routes.create(path: project.full_path + 'old') }
+
+    it 'redirects to a file with the proper extension' do
+      get :show, params: { namespace_id: project.namespace, project_id: project.to_param + 'old', filename: File.basename(upload.path), secret: upload.secret }
+
+      expect(response.location).to eq(show_project_uploads_url(project, upload.secret, upload_path))
+      expect(response.location).to end_with(upload.path)
+      expect(response).to have_gitlab_http_status(:redirect)
+    end
+  end
+
   context "when exception occurs" do
     before do
       allow(FileUploader).to receive(:workhorse_authorize).and_raise(SocketError.new)

spec/routing/uploads_routing_spec.rb

@@ -28,4 +28,12 @@
       expect(post("/uploads/#{model}?id=1")).not_to be_routable
     end
   end
+
+  describe 'legacy paths' do
+    include RSpec::Rails::RequestExampleGroup
+
+    it 'redirects project uploads to canonical path under project namespace' do
+      expect(get('/uploads/namespace/project/12345/test.png')).to redirect_to('/namespace/project/uploads/12345/test.png')
+    end
+  end
 end