Fix ensure_canonical_path for top level routes

Don’t replace a substring of the path if it is part of the top level route.

E.g. When redirecting from `/groups/ups` to `/groups/foo`, be careful not to do `/grofoo/ups`.

Projects are unaffected by this issue, but I am grouping the `#ensure_canonical_path` tests similar to the group and user tests.

app/controllers/concerns/routable_actions.rb

@@ -24,15 +24,27 @@ module RoutableActions
     end
   end
 
-  def ensure_canonical_path(routable, requested_path)
+  def ensure_canonical_path(routable, requested_full_path)
     return unless request.get?
 
     canonical_path = routable.full_path
-    if canonical_path != requested_path
-      if canonical_path.casecmp(requested_path) != 0
-        flash[:notice] = "#{routable.class.to_s.titleize} '#{requested_path}' was moved to '#{canonical_path}'. Please update any links and bookmarks that may still have the old path."
+    if canonical_path != requested_full_path
+      if canonical_path.casecmp(requested_full_path) != 0
+        flash[:notice] = "#{routable.class.to_s.titleize} '#{requested_full_path}' was moved to '#{canonical_path}'. Please update any links and bookmarks that may still have the old path."
       end
-      redirect_to request.original_fullpath.sub(requested_path, canonical_path)
+      redirect_to full_canonical_path(canonical_path, requested_full_path)
+    end
+  end
+
+  def full_canonical_path(canonical_path, requested_full_path)
+    request_path = request.original_fullpath
+    top_level_route_regex = %r{\A(/#{Regexp.union(DynamicPathValidator::TOP_LEVEL_ROUTES)}/)#{requested_full_path}}
+    top_level_route_match = request_path.match(top_level_route_regex)
+
+    if top_level_route_match
+      request_path.sub(top_level_route_regex, "\\1#{canonical_path}")
+    else
+      request_path.sub(requested_full_path, canonical_path)
     end
   end
 end

spec/controllers/groups_controller_spec.rb

@@ -84,26 +84,6 @@ describe GroupsController do
         expect(assigns(:issues)).to eq [issue_2, issue_1]
       end
     end
-
-    context 'when requesting the canonical path with different casing' do
-      it 'redirects to the correct casing' do
-        get :issues, id: group.to_param.upcase
-
-        expect(response).to redirect_to(issues_group_path(group.to_param))
-        expect(controller).not_to set_flash[:notice]
-      end
-    end
-
-    context 'when requesting a redirected path' do
-      let(:redirect_route) { group.redirect_routes.create(path: 'old-path') }
-
-      it 'redirects to the canonical path' do
-        get :issues, id: redirect_route.path
-
-        expect(response).to redirect_to(issues_group_path(group.to_param))
-        expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
-      end
-    end
   end
 
   describe 'GET #merge_requests' do
@@ -129,26 +109,6 @@ describe GroupsController do
         expect(assigns(:merge_requests)).to eq [merge_request_2, merge_request_1]
       end
     end
-
-    context 'when requesting the canonical path with different casing' do
-      it 'redirects to the correct casing' do
-        get :merge_requests, id: group.to_param.upcase
-
-        expect(response).to redirect_to(merge_requests_group_path(group.to_param))
-        expect(controller).not_to set_flash[:notice]
-      end
-    end
-
-    context 'when requesting a redirected path' do
-      let(:redirect_route) { group.redirect_routes.create(path: 'old-path') }
-
-      it 'redirects to the canonical path' do
-        get :merge_requests, id: redirect_route.path
-
-        expect(response).to redirect_to(merge_requests_group_path(group.to_param))
-        expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
-      end
-    end
   end
 
   describe 'DELETE #destroy' do
@@ -178,53 +138,140 @@ describe GroupsController do
 
         expect(response).to redirect_to(root_path)
       end
+    end
+  end
 
-      context 'when requesting the canonical path with different casing' do
-        it 'does not 404' do
-          delete :destroy, id: group.to_param.upcase
+  describe 'PUT update' do
+    before do
+      sign_in(user)
+    end
 
-          expect(response).not_to have_http_status(404)
+    it 'updates the path successfully' do
+      post :update, id: group.to_param, group: { path: 'new_path' }
+
+      expect(response).to have_http_status(302)
+      expect(controller).to set_flash[:notice]
     end
 
-        it 'does not redirect to the correct casing' do
-          delete :destroy, id: group.to_param.upcase
+    it 'does not update the path on error' do
+      allow_any_instance_of(Group).to receive(:move_dir).and_raise(Gitlab::UpdatePathError)
+      post :update, id: group.to_param, group: { path: 'new_path' }
 
-          expect(response).not_to redirect_to(group_path(group.to_param))
+      expect(assigns(:group).errors).not_to be_empty
+      expect(assigns(:group).path).not_to eq('new_path')
+    end
+  end
+
+  describe '#ensure_canonical_path' do
+    before do
+      sign_in(user)
+    end
+
+    context 'for a GET request' do
+      context 'when requesting groups at the root path' do
+        before do
+          allow(request).to receive(:original_fullpath).and_return("/#{group_full_path}")
+          get :show, id: group_full_path
+        end
+
+        context 'when requesting the canonical path with different casing' do
+          let(:group_full_path) { group.to_param.upcase }
+
+          it 'redirects to the correct casing' do
+            expect(response).to redirect_to(group)
+            expect(controller).not_to set_flash[:notice]
           end
         end
 
         context 'when requesting a redirected path' do
           let(:redirect_route) { group.redirect_routes.create(path: 'old-path') }
+          let(:group_full_path) { redirect_route.path }
 
-        it 'returns not found' do
-          delete :destroy, id: redirect_route.path
+          it 'redirects to the canonical path' do
+            expect(response).to redirect_to(group)
+            expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
+          end
 
-          expect(response).to have_http_status(404)
+          context 'when the old group path is a substring of the scheme or host' do
+            let(:redirect_route) { group.redirect_routes.create(path: 'http') }
+
+            it 'does not modify the requested host' do
+              expect(response).to redirect_to(group)
+              expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
+            end
+          end
+
+          context 'when the old group path is substring of groups' do
+            # I.e. /groups/oups should not become /grfoo/oups
+            let(:redirect_route) { group.redirect_routes.create(path: 'oups') }
+
+            it 'does not modify the /groups part of the path' do
+              expect(response).to redirect_to(group)
+              expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
             end
           end
         end
       end
 
-  describe 'PUT update' do
-    before do
-      sign_in(user)
+      context 'when requesting groups under the /groups path' do
+        context 'when requesting the canonical path with different casing' do
+          it 'redirects to the correct casing' do
+            get :issues, id: group.to_param.upcase
+
+            expect(response).to redirect_to(issues_group_path(group.to_param))
+            expect(controller).not_to set_flash[:notice]
+          end
         end
 
-    it 'updates the path successfully' do
-      post :update, id: group.to_param, group: { path: 'new_path' }
+        context 'when requesting a redirected path' do
+          let(:redirect_route) { group.redirect_routes.create(path: 'old-path') }
 
-      expect(response).to have_http_status(302)
-      expect(controller).to set_flash[:notice]
+          it 'redirects to the canonical path' do
+            get :issues, id: redirect_route.path
+
+            expect(response).to redirect_to(issues_group_path(group.to_param))
+            expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
           end
 
-    it 'does not update the path on error' do
-      allow_any_instance_of(Group).to receive(:move_dir).and_raise(Gitlab::UpdatePathError)
-      post :update, id: group.to_param, group: { path: 'new_path' }
+          context 'when the old group path is a substring of the scheme or host' do
+            let(:redirect_route) { group.redirect_routes.create(path: 'http') }
 
-      expect(assigns(:group).errors).not_to be_empty
-      expect(assigns(:group).path).not_to eq('new_path')
+            it 'does not modify the requested host' do
+              get :issues, id: redirect_route.path
+
+              expect(response).to redirect_to(issues_group_path(group.to_param))
+              expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
+            end
           end
 
+          context 'when the old group path is substring of groups' do
+            # I.e. /groups/oups should not become /grfoo/oups
+            let(:redirect_route) { group.redirect_routes.create(path: 'oups') }
+
+            it 'does not modify the /groups part of the path' do
+              get :issues, id: redirect_route.path
+
+              expect(response).to redirect_to(issues_group_path(group.to_param))
+              expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
+            end
+          end
+
+          context 'when the old group path is substring of groups plus the new path' do
+            # I.e. /groups/oups/oup should not become /grfoos
+            let(:redirect_route) { group.redirect_routes.create(path: 'oups/oup') }
+
+            it 'does not modify the /groups part of the path' do
+              get :issues, id: redirect_route.path
+
+              expect(response).to redirect_to(issues_group_path(group.to_param))
+              expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
+            end
+          end
+        end
+      end
+    end
+
+    context 'for a POST request' do
       context 'when requesting the canonical path with different casing' do
         it 'does not 404' do
           post :update, id: group.to_param.upcase, group: { path: 'new_path' }
@@ -235,7 +282,7 @@ describe GroupsController do
         it 'does not redirect to the correct casing' do
           post :update, id: group.to_param.upcase, group: { path: 'new_path' }
 
-        expect(response).not_to redirect_to(group_path(group.to_param))
+          expect(response).not_to have_http_status(301)
         end
       end
 
@@ -250,15 +297,29 @@ describe GroupsController do
       end
     end
 
-  describe 'ensure_canonical_path' do
-    context 'when the old group path is a substring of the scheme or host' do
-      let(:redirect_route) { group.redirect_routes.create(path: 'http') }
+    context 'for a DELETE request' do
+      context 'when requesting the canonical path with different casing' do
+        it 'does not 404' do
+          delete :destroy, id: group.to_param.upcase
 
-      it 'does not modify the requested host' do
-        get :issues, id: redirect_route.path
+          expect(response).not_to have_http_status(404)
+        end
 
-        expect(response).to redirect_to(issues_group_path(group.to_param))
-        expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
+        it 'does not redirect to the correct casing' do
+          delete :destroy, id: group.to_param.upcase
+
+          expect(response).not_to have_http_status(301)
+        end
+      end
+
+      context 'when requesting a redirected path' do
+        let(:redirect_route) { group.redirect_routes.create(path: 'old-path') }
+
+        it 'returns not found' do
+          delete :destroy, id: redirect_route.path
+
+          expect(response).to have_http_status(404)
+        end
       end
     end
   end

spec/controllers/projects_controller_spec.rb

@@ -169,27 +169,6 @@ describe ProjectsController do
       end
     end
 
-    context "when requested with case sensitive namespace and project path" do
-      context "when there is a match with the same casing" do
-        it "loads the project" do
-          get :show, namespace_id: public_project.namespace, id: public_project
-
-          expect(assigns(:project)).to eq(public_project)
-          expect(response).to have_http_status(200)
-        end
-      end
-
-      context "when there is a match with different casing" do
-        it "redirects to the normalized path" do
-          get :show, namespace_id: public_project.namespace, id: public_project.path.upcase
-
-          expect(assigns(:project)).to eq(public_project)
-          expect(response).to redirect_to("/#{public_project.full_path}")
-          expect(controller).not_to set_flash[:notice]
-        end
-      end
-    end
-
     context "when the url contains .atom" do
       let(:public_project_with_dot_atom) { build(:empty_project, :public, name: 'my.atom', path: 'my.atom') }
 
@@ -219,17 +198,6 @@ describe ProjectsController do
         expect(response).to redirect_to(namespace_project_path)
       end
     end
-
-    context 'when requesting a redirected path' do
-      let!(:redirect_route) { public_project.redirect_routes.create!(path: "foo/bar") }
-
-      it 'redirects to the canonical path' do
-        get :show, namespace_id: 'foo', id: 'bar'
-
-        expect(response).to redirect_to(public_project)
-        expect(controller).to set_flash[:notice].to(project_moved_message(redirect_route, public_project))
-      end
-    end
   end
 
   describe "#update" do
@@ -256,34 +224,6 @@ describe ProjectsController do
       expect(assigns(:repository).path).to eq(project.repository.path)
       expect(response).to have_http_status(302)
     end
-
-    context 'when requesting the canonical path' do
-      it "is case-insensitive" do
-        controller.instance_variable_set(:@project, project)
-
-        put :update,
-            namespace_id: 'FOo',
-            id: 'baR',
-            project: project_params
-
-        expect(project.repository.path).to include(new_path)
-        expect(assigns(:repository).path).to eq(project.repository.path)
-        expect(response).to have_http_status(302)
-      end
-    end
-
-    context 'when requesting a redirected path' do
-      let!(:redirect_route) { project.redirect_routes.create!(path: "foo/bar") }
-
-      it 'returns not found' do
-        put :update,
-            namespace_id: 'foo',
-            id: 'bar',
-            project: project_params
-
-        expect(response).to have_http_status(404)
-      end
-    end
   end
 
   describe "#destroy" do
@@ -319,31 +259,6 @@ describe ProjectsController do
         expect(merge_request.reload.state).to eq('closed')
       end
     end
-
-    context 'when requesting the canonical path' do
-      it "is case-insensitive" do
-        controller.instance_variable_set(:@project, project)
-        sign_in(admin)
-
-        orig_id = project.id
-        delete :destroy, namespace_id: project.namespace, id: project.path.upcase
-
-        expect { Project.find(orig_id) }.to raise_error(ActiveRecord::RecordNotFound)
-        expect(response).to have_http_status(302)
-        expect(response).to redirect_to(dashboard_projects_path)
-      end
-    end
-
-    context 'when requesting a redirected path' do
-      let!(:redirect_route) { project.redirect_routes.create!(path: "foo/bar") }
-
-      it 'returns not found' do
-        sign_in(admin)
-        delete :destroy, namespace_id: 'foo', id: 'bar'
-
-        expect(response).to have_http_status(404)
-      end
-    end
   end
 
   describe 'PUT #new_issue_address' do
@@ -465,11 +380,56 @@ describe ProjectsController do
       expect(parsed_body["Tags"]).to include("v1.0.0")
       expect(parsed_body["Commits"]).to include("123456")
     end
+  end
+
+  describe 'POST #preview_markdown' do
+    it 'renders json in a correct format' do
+      sign_in(user)
+
+      post :preview_markdown, namespace_id: public_project.namespace, id: public_project, text: '*Markdown* text'
+
+      expect(JSON.parse(response.body).keys).to match_array(%w(body references))
+    end
+  end
+
+  describe '#ensure_canonical_path' do
+    before do
+      sign_in(user)
+    end
+
+    context 'for a GET request' do
+      context 'when requesting the canonical path' do
+        context "with exactly matching casing" do
+          it "loads the project" do
+            get :show, namespace_id: public_project.namespace, id: public_project
+
+            expect(assigns(:project)).to eq(public_project)
+            expect(response).to have_http_status(200)
+          end
+        end
+
+        context "with different casing" do
+          it "redirects to the normalized path" do
+            get :show, namespace_id: public_project.namespace, id: public_project.path.upcase
+
+            expect(assigns(:project)).to eq(public_project)
+            expect(response).to redirect_to("/#{public_project.full_path}")
+            expect(controller).not_to set_flash[:notice]
+          end
+        end
+      end
 
       context 'when requesting a redirected path' do
         let!(:redirect_route) { public_project.redirect_routes.create!(path: "foo/bar") }
 
         it 'redirects to the canonical path' do
+          get :show, namespace_id: 'foo', id: 'bar'
+
+          expect(response).to redirect_to(public_project)
+          expect(controller).to set_flash[:notice].to(project_moved_message(redirect_route, public_project))
+        end
+
+        it 'redirects to the canonical path (testing non-show action)' do
           get :refs, namespace_id: 'foo', id: 'bar'
 
           expect(response).to redirect_to(refs_namespace_project_path(namespace_id: public_project.namespace, id: public_project))
@@ -478,13 +438,60 @@ describe ProjectsController do
       end
     end
 
-  describe 'POST #preview_markdown' do
-    it 'renders json in a correct format' do
-      sign_in(user)
+    context 'for a POST request' do
+      context 'when requesting the canonical path with different casing' do
+        it 'does not 404' do
+          post :toggle_star, namespace_id: public_project.namespace, id: public_project.path.upcase
 
-      post :preview_markdown, namespace_id: public_project.namespace, id: public_project, text: '*Markdown* text'
+          expect(response).not_to have_http_status(404)
+        end
 
-      expect(JSON.parse(response.body).keys).to match_array(%w(body references))
+        it 'does not redirect to the correct casing' do
+          post :toggle_star, namespace_id: public_project.namespace, id: public_project.path.upcase
+
+          expect(response).not_to have_http_status(301)
+        end
+      end
+
+      context 'when requesting a redirected path' do
+        let!(:redirect_route) { public_project.redirect_routes.create!(path: "foo/bar") }
+
+        it 'returns not found' do
+          post :toggle_star, namespace_id: 'foo', id: 'bar'
+
+          expect(response).to have_http_status(404)
+        end
+      end
+    end
+
+    context 'for a DELETE request' do
+      before do
+        sign_in(create(:admin))
+      end
+
+      context 'when requesting the canonical path with different casing' do
+        it 'does not 404' do
+          delete :destroy, namespace_id: project.namespace, id: project.path.upcase
+
+          expect(response).not_to have_http_status(404)
+        end
+
+        it 'does not redirect to the correct casing' do
+          delete :destroy, namespace_id: project.namespace, id: project.path.upcase
+
+          expect(response).not_to have_http_status(301)
+        end
+      end
+
+      context 'when requesting a redirected path' do
+        let!(:redirect_route) { project.redirect_routes.create!(path: "foo/bar") }
+
+        it 'returns not found' do
+          delete :destroy, namespace_id: 'foo', id: 'bar'
+
+          expect(response).to have_http_status(404)
+        end
+      end
     end
   end