Fix upload redirections when project has moved

Previously if a user attempted to click on a link to an attachment for a
renamed project (e.g. https://gitlab-org/gitlab-ce/uploads/123.png),
Rails would redirect the user to an erroneous URL
(e.g. https://gitlab-org/gitlab-foss/uploads/123.png.png). Notice the
redirect URL contains a redundant extension.

It turns out Rails 5 parses the extension of a route as the format
(https://github.com/rails/rails/issues/28901#issuecomment-297747521),
and the URL generator appends the format to the redirected route.

To fix this, we need to disable the format path parameter and explicitly
set the format to `nil`. If we do not set the format to `nil`, the URL
generator will append a query string (e.g `?format=png`), which would
still work but not necessary.

Closes https://gitlab.com/gitlab-org/gitlab/issues/196232

app/controllers/projects/uploads_controller.rb

@@ -29,4 +29,13 @@ class Projects::UploadsController < Projects::ApplicationController
 
     Project.find_by_full_path("#{namespace}/#{id}")
   end
+
+  # Overrides ApplicationController#build_canonical_path since there are
+  # multiple routes that match project uploads
+  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 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
 
     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 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
 
       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,10 @@ scope path: :uploads do
       as: 'appearance_upload'
 
   # Project markdown uploads
+  # Consider deprecating this in GitLab 13.x because this is redundant to show_namespace_project_uploads
   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{[^/]+} }
+    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 @@ describe Groups::UploadsController do
     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 @@ describe Projects::UploadsController do
     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)