From 52aba709ffbe2f5410334beca6c114739054ab4c Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Thu, 18 May 2017 09:36:15 -0700
Subject: Fix #32506

The host should not be modified, nor is it needed for the redirect.
---
 app/controllers/concerns/routable_actions.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/controllers/concerns/routable_actions.rb b/app/controllers/concerns/routable_actions.rb
index afd110adcad..76c2908463c 100644
--- a/app/controllers/concerns/routable_actions.rb
+++ b/app/controllers/concerns/routable_actions.rb
@@ -32,7 +32,7 @@ module RoutableActions
       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."
       end
-      redirect_to request.original_url.sub(requested_path, canonical_path)
+      redirect_to request.original_fullpath.sub(requested_path, canonical_path)
     end
   end
 end
-- 
cgit v1.2.3


From 11f82de1efc087ee812764764e31161347e593cb Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Thu, 18 May 2017 10:30:17 -0700
Subject: Add test that fails without the fix

---
 spec/controllers/groups_controller_spec.rb | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index 15dae3231ca..0392315b62f 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -250,6 +250,19 @@ 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') }
+
+      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
+  end
+
   def group_moved_message(redirect_route, group)
     "Group '#{redirect_route.path}' was moved to '#{group.full_path}'. Please update any links and bookmarks that may still have the old path."
   end
-- 
cgit v1.2.3


From f9785dcec34c4205732871523f95b9743db00965 Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Thu, 18 May 2017 12:56:39 -0700
Subject: Fix ensure_canonical_path for top level routes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 |  22 ++-
 spec/controllers/groups_controller_spec.rb   | 229 +++++++++++++++---------
 spec/controllers/projects_controller_spec.rb | 199 +++++++++++----------
 spec/controllers/users_controller_spec.rb    | 253 +++++++++++++--------------
 4 files changed, 386 insertions(+), 317 deletions(-)

diff --git a/app/controllers/concerns/routable_actions.rb b/app/controllers/concerns/routable_actions.rb
index 76c2908463c..a5b793081b2 100644
--- a/app/controllers/concerns/routable_actions.rb
+++ b/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
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index 0392315b62f..993654fddaa 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/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,30 +138,6 @@ describe GroupsController do
 
         expect(response).to redirect_to(root_path)
       end
-
-      context 'when requesting the canonical path with different casing' do
-        it 'does not 404' do
-          delete :destroy, id: group.to_param.upcase
-
-          expect(response).not_to have_http_status(404)
-        end
-
-        it 'does not redirect to the correct casing' do
-          delete :destroy, id: group.to_param.upcase
-
-          expect(response).not_to redirect_to(group_path(group.to_param))
-        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
 
@@ -224,41 +160,166 @@ describe GroupsController do
       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
-      it 'does not 404' do
-        post :update, id: group.to_param.upcase, group: { path: 'new_path' }
+        context 'when requesting the canonical path with different casing' do
+          let(:group_full_path) { group.to_param.upcase }
 
-        expect(response).not_to have_http_status(404)
+          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 '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
+
+          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
 
-      it 'does not redirect to the correct casing' do
-        post :update, id: group.to_param.upcase, group: { path: 'new_path' }
+      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).not_to redirect_to(group_path(group.to_param))
+            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
+
+          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
+              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 'when requesting a redirected path' do
-      let(:redirect_route) { group.redirect_routes.create(path: 'old-path') }
+    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' }
+
+          expect(response).not_to have_http_status(404)
+        end
+
+        it 'does not redirect to the correct casing' do
+          post :update, id: group.to_param.upcase, group: { path: 'new_path' }
+
+          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
-        post :update, id: redirect_route.path, group: { path: 'new_path' }
+        it 'returns not found' do
+          post :update, id: redirect_route.path, group: { path: 'new_path' }
 
-        expect(response).to have_http_status(404)
+          expect(response).to have_http_status(404)
+        end
       end
     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
diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb
index e230944d52e..a8be6768a47 100644
--- a/spec/controllers/projects_controller_spec.rb
+++ b/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,17 +380,6 @@ describe ProjectsController do
       expect(parsed_body["Tags"]).to include("v1.0.0")
       expect(parsed_body["Commits"]).to include("123456")
     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 :refs, namespace_id: 'foo', id: 'bar'
-
-        expect(response).to redirect_to(refs_namespace_project_path(namespace_id: public_project.namespace, id: public_project))
-        expect(controller).to set_flash[:notice].to(project_moved_message(redirect_route, public_project))
-      end
-    end
   end
 
   describe 'POST #preview_markdown' do
@@ -488,6 +392,109 @@ describe ProjectsController do
     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))
+          expect(controller).to set_flash[:notice].to(project_moved_message(redirect_route, public_project))
+        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 :toggle_star, namespace_id: public_project.namespace, id: public_project.path.upcase
+
+          expect(response).not_to have_http_status(404)
+        end
+
+        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
+
   def project_moved_message(redirect_route, project)
     "Project '#{redirect_route.path}' was moved to '#{project.full_path}'. Please update any links and bookmarks that may still have the old path."
   end
diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb
index 1d61719f1d0..d33e2ba1e53 100644
--- a/spec/controllers/users_controller_spec.rb
+++ b/spec/controllers/users_controller_spec.rb
@@ -53,40 +53,6 @@ describe UsersController do
       end
     end
 
-    context 'when requesting the canonical path' do
-      let(:user) { create(:user, username: 'CamelCaseUser') }
-
-      before { sign_in(user) }
-
-      context 'with exactly matching casing' do
-        it 'responds with success' do
-          get :show, username: user.username
-
-          expect(response).to be_success
-        end
-      end
-
-      context 'with different casing' do
-        it 'redirects to the correct casing' do
-          get :show, username: user.username.downcase
-
-          expect(response).to redirect_to(user)
-          expect(controller).not_to set_flash[:notice]
-        end
-      end
-    end
-
-    context 'when requesting a redirected path' do
-      let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') }
-
-      it 'redirects to the canonical path' do
-        get :show, username: redirect_route.path
-
-        expect(response).to redirect_to(user)
-        expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
-      end
-    end
-
     context 'when a user by that username does not exist' do
       context 'when logged out' do
         it 'redirects to login page' do
@@ -131,40 +97,6 @@ describe UsersController do
         expect(assigns(:contributions_calendar).projects.count).to eq(2)
       end
     end
-
-    context 'when requesting the canonical path' do
-      let(:user) { create(:user, username: 'CamelCaseUser') }
-
-      before { sign_in(user) }
-
-      context 'with exactly matching casing' do
-        it 'responds with success' do
-          get :calendar, username: user.username
-
-          expect(response).to be_success
-        end
-      end
-
-      context 'with different casing' do
-        it 'redirects to the correct casing' do
-          get :calendar, username: user.username.downcase
-
-          expect(response).to redirect_to(user_calendar_path(user))
-          expect(controller).not_to set_flash[:notice]
-        end
-      end
-    end
-
-    context 'when requesting a redirected path' do
-      let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') }
-
-      it 'redirects to the canonical path' do
-        get :calendar, username: redirect_route.path
-
-        expect(response).to redirect_to(user_calendar_path(user))
-        expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
-      end
-    end
   end
 
   describe 'GET #calendar_activities' do
@@ -187,38 +119,6 @@ describe UsersController do
       get :calendar_activities, username: user.username
       expect(response).to render_template('calendar_activities')
     end
-
-    context 'when requesting the canonical path' do
-      let(:user) { create(:user, username: 'CamelCaseUser') }
-
-      context 'with exactly matching casing' do
-        it 'responds with success' do
-          get :calendar_activities, username: user.username
-
-          expect(response).to be_success
-        end
-      end
-
-      context 'with different casing' do
-        it 'redirects to the correct casing' do
-          get :calendar_activities, username: user.username.downcase
-
-          expect(response).to redirect_to(user_calendar_activities_path(user))
-          expect(controller).not_to set_flash[:notice]
-        end
-      end
-    end
-
-    context 'when requesting a redirected path' do
-      let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') }
-
-      it 'redirects to the canonical path' do
-        get :calendar_activities, username: redirect_route.path
-
-        expect(response).to redirect_to(user_calendar_activities_path(user))
-        expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
-      end
-    end
   end
 
   describe 'GET #snippets' do
@@ -241,38 +141,6 @@ describe UsersController do
         expect(JSON.parse(response.body)).to have_key('html')
       end
     end
-
-    context 'when requesting the canonical path' do
-      let(:user) { create(:user, username: 'CamelCaseUser') }
-
-      context 'with exactly matching casing' do
-        it 'responds with success' do
-          get :snippets, username: user.username
-
-          expect(response).to be_success
-        end
-      end
-
-      context 'with different casing' do
-        it 'redirects to the correct casing' do
-          get :snippets, username: user.username.downcase
-
-          expect(response).to redirect_to(user_snippets_path(user))
-          expect(controller).not_to set_flash[:notice]
-        end
-      end
-    end
-
-    context 'when requesting a redirected path' do
-      let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') }
-
-      it 'redirects to the canonical path' do
-        get :snippets, username: redirect_route.path
-
-        expect(response).to redirect_to(user_snippets_path(user))
-        expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
-      end
-    end
   end
 
   describe 'GET #exists' do
@@ -321,6 +189,127 @@ describe UsersController do
     end
   end
 
+  describe '#ensure_canonical_path' do
+    before do
+      sign_in(user)
+    end
+
+    context 'for a GET request' do
+      context 'when requesting users at the root path' do
+        context 'when requesting the canonical path' do
+          let(:user) { create(:user, username: 'CamelCaseUser') }
+
+          context 'with exactly matching casing' do
+            it 'responds with success' do
+              get :show, username: user.username
+
+              expect(response).to be_success
+            end
+          end
+
+          context 'with different casing' do
+            it 'redirects to the correct casing' do
+              get :show, username: user.username.downcase
+
+              expect(response).to redirect_to(user)
+              expect(controller).not_to set_flash[:notice]
+            end
+          end
+        end
+
+        context 'when requesting a redirected path' do
+          let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-path') }
+
+          it 'redirects to the canonical path' do
+            get :show, username: redirect_route.path
+
+            expect(response).to redirect_to(user)
+            expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
+          end
+
+          context 'when the old path is a substring of the scheme or host' do
+            let(:redirect_route) { user.namespace.redirect_routes.create(path: 'http') }
+
+            it 'does not modify the requested host' do
+              get :show, username: redirect_route.path
+
+              expect(response).to redirect_to(user)
+              expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
+            end
+          end
+
+          context 'when the old path is substring of users' do
+            let(:redirect_route) { user.namespace.redirect_routes.create(path: 'ser') }
+
+            it 'redirects to the canonical path' do
+              get :show, username: redirect_route.path
+
+              expect(response).to redirect_to(user)
+              expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
+            end
+          end
+        end
+      end
+
+      context 'when requesting users under the /users path' do
+        context 'when requesting the canonical path' do
+          let(:user) { create(:user, username: 'CamelCaseUser') }
+
+          context 'with exactly matching casing' do
+            it 'responds with success' do
+              get :projects, username: user.username
+
+              expect(response).to be_success
+            end
+          end
+
+          context 'with different casing' do
+            it 'redirects to the correct casing' do
+              get :projects, username: user.username.downcase
+
+              expect(response).to redirect_to(user_projects_path(user))
+              expect(controller).not_to set_flash[:notice]
+            end
+          end
+        end
+
+        context 'when requesting a redirected path' do
+          let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-path') }
+
+          it 'redirects to the canonical path' do
+            get :projects, username: redirect_route.path
+
+            expect(response).to redirect_to(user_projects_path(user))
+            expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
+          end
+
+          context 'when the old path is a substring of the scheme or host' do
+            let(:redirect_route) { user.namespace.redirect_routes.create(path: 'http') }
+
+            it 'does not modify the requested host' do
+              get :projects, username: redirect_route.path
+
+              expect(response).to redirect_to(user_projects_path(user))
+              expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
+            end
+          end
+
+          context 'when the old path is substring of users' do
+            let(:redirect_route) { user.namespace.redirect_routes.create(path: 'ser') }
+
+            # I.e. /users/ser should not become /ufoos/ser
+            it 'does not modify the /users part of the path' do
+              get :projects, username: redirect_route.path
+
+              expect(response).to redirect_to(user_projects_path(user))
+              expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user))
+            end
+          end
+        end
+      end
+    end
+  end
+
   def user_moved_message(redirect_route, user)
     "User '#{redirect_route.path}' was moved to '#{user.full_path}'. Please update any links and bookmarks that may still have the old path."
   end
-- 
cgit v1.2.3


From 49697bc8df613dfe8e88f5f7cd8eae57e26c786f Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Thu, 18 May 2017 16:23:05 -0700
Subject: Refactor to more robust implementation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In order to avoid string manipulation or modify route params (to make them unambiguous for `url_for`), we are accepting a behavior change:

When being redirected to the canonical path for a group, if you requested a group show path starting with `/groups/…` then you’ll now be redirected to the group at root `/…`.
---
 app/controllers/concerns/routable_actions.rb       |  14 +--
 app/controllers/groups/application_controller.rb   |   6 +
 app/controllers/groups_controller.rb               |   8 ++
 app/controllers/projects/application_controller.rb |   7 ++
 app/controllers/projects_controller.rb             |   7 ++
 app/controllers/users_controller.rb                |   4 +
 .../groups/milestones_controller_spec.rb           | 135 ++++++++++++++++++++-
 spec/controllers/groups_controller_spec.rb         |  41 ++++++-
 .../controllers/projects/labels_controller_spec.rb |  70 +++++++++++
 spec/support/milestone_tabs_examples.rb            |   2 +-
 10 files changed, 272 insertions(+), 22 deletions(-)

diff --git a/app/controllers/concerns/routable_actions.rb b/app/controllers/concerns/routable_actions.rb
index a5b793081b2..4199da9cdf5 100644
--- a/app/controllers/concerns/routable_actions.rb
+++ b/app/controllers/concerns/routable_actions.rb
@@ -32,19 +32,7 @@ module RoutableActions
       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 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)
+      redirect_to build_canonical_path(routable)
     end
   end
 end
diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb
index afffb813b44..c0ac47e363d 100644
--- a/app/controllers/groups/application_controller.rb
+++ b/app/controllers/groups/application_controller.rb
@@ -31,4 +31,10 @@ class Groups::ApplicationController < ApplicationController
       return render_403
     end
   end
+
+  def build_canonical_path(group)
+    params[:group_id] = group.to_param
+    
+    url_for(params)
+  end
 end
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 1515173d0ac..965ced4d372 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -169,4 +169,12 @@ class GroupsController < Groups::ApplicationController
       @notification_setting = current_user.notification_settings_for(group)
     end
   end
+
+  def build_canonical_path(group)
+    return group_path(group) if action_name == 'show' # root group path
+    
+    params[:id] = group.to_param
+
+    url_for(params)
+  end
 end
diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb
index 12e4a6999ae..cb4bd0ad5f5 100644
--- a/app/controllers/projects/application_controller.rb
+++ b/app/controllers/projects/application_controller.rb
@@ -29,6 +29,13 @@ class Projects::ApplicationController < ApplicationController
     @project = find_routable!(Project, path, extra_authorization_proc: auth_proc)
   end
 
+  def build_canonical_path(project)
+    params[:namespace_id] = project.namespace.to_param
+    params[:project_id] = project.to_param
+
+    url_for(params)
+  end
+
   def repository
     @repository ||= project.repository
   end
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 63d018c8cbf..544715d62ea 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -365,4 +365,11 @@ class ProjectsController < Projects::ApplicationController
   def project_view_files_allowed?
     !project.empty_repo? && can?(current_user, :download_code, project)
   end
+
+  def build_canonical_path(project)
+    params[:namespace_id] = project.namespace.to_param
+    params[:id] = project.to_param
+
+    url_for(params)
+  end
 end
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index ba22b2f9d29..19fc1e5de49 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -138,4 +138,8 @@ class UsersController < ApplicationController
   def projects_for_current_user
     ProjectsFinder.new(current_user: current_user).execute
   end
+
+  def build_canonical_path(user)
+    url_for(params.merge(username: user.to_param))
+  end
 end
diff --git a/spec/controllers/groups/milestones_controller_spec.rb b/spec/controllers/groups/milestones_controller_spec.rb
index 7cf2996ffd0..f3263bc177d 100644
--- a/spec/controllers/groups/milestones_controller_spec.rb
+++ b/spec/controllers/groups/milestones_controller_spec.rb
@@ -21,7 +21,6 @@ describe Groups::MilestonesController do
     sign_in(user)
     group.add_owner(user)
     project.team << [user, :master]
-    controller.instance_variable_set(:@group, group)
   end
 
   it_behaves_like 'milestone tabs'
@@ -29,7 +28,7 @@ describe Groups::MilestonesController do
   describe "#create" do
     it "creates group milestone with Chinese title" do
       post :create,
-           group_id: group.id,
+           group_id: group.to_param,
            milestone: { project_ids: [project.id, project2.id], title: title }
 
       expect(response).to redirect_to(group_milestone_path(group, title.to_slug.to_s, title: title))
@@ -37,9 +36,139 @@ describe Groups::MilestonesController do
     end
 
     it "redirects to new when there are no project ids" do
-      post :create, group_id: group.id, milestone: { title: title, project_ids: [""] }
+      post :create, group_id: group.to_param, milestone: { title: title, project_ids: [""] }
       expect(response).to render_template :new
       expect(assigns(:milestone).errors).not_to be_nil
     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 'non-show path' do
+          context 'with exactly matching casing' do
+            it 'does not redirect' do
+              get :index, group_id: group.to_param
+
+              expect(response).not_to have_http_status(301)
+            end
+          end
+
+          context 'with different casing' do
+            it 'redirects to the correct casing' do
+              get :index, group_id: group.to_param.upcase
+
+              expect(response).to redirect_to(group_milestones_path(group.to_param))
+              expect(controller).not_to set_flash[:notice]
+            end
+          end
+        end
+
+        context 'show path' do
+          context 'with exactly matching casing' do
+            it 'does not redirect' do
+              get :show, group_id: group.to_param, id: title
+
+              expect(response).not_to have_http_status(301)
+            end
+          end
+
+          context 'with different casing' do
+            it 'redirects to the correct casing' do
+              get :show, group_id: group.to_param.upcase, id: title
+
+              expect(response).to redirect_to(group_milestone_path(group.to_param, title))
+              expect(controller).not_to set_flash[:notice]
+            end
+          end
+        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, group_id: redirect_route.path, id: title
+
+          expect(response).to redirect_to(merge_requests_group_milestone_path(group.to_param, title))
+          expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
+        end
+
+        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
+            get :merge_requests, group_id: redirect_route.path, id: title
+
+            expect(response).to redirect_to(merge_requests_group_milestone_path(group.to_param, title))
+            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 :merge_requests, group_id: redirect_route.path, id: title
+
+            expect(response).to redirect_to(merge_requests_group_milestone_path(group.to_param, title))
+            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 :merge_requests, group_id: redirect_route.path, id: title
+
+            expect(response).to redirect_to(merge_requests_group_milestone_path(group.to_param, title))
+            expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group))
+          end
+        end
+      end
+    end
+  end
+
+  context 'for a non-GET request' do
+    context 'when requesting the canonical path with different casing' do
+      it 'does not 404' do
+        post :create,
+             group_id: group.to_param,
+             milestone: { project_ids: [project.id, project2.id], title: title }
+
+        expect(response).not_to have_http_status(404)
+      end
+
+      it 'does not redirect to the correct casing' do
+        post :create,
+             group_id: group.to_param,
+             milestone: { project_ids: [project.id, project2.id], title: title }
+
+        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
+        post :create,
+             group_id: redirect_route.path,
+             milestone: { project_ids: [project.id, project2.id], title: title }
+
+        expect(response).to have_http_status(404)
+      end
+    end
+  end
+
+  def group_moved_message(redirect_route, group)
+    "Group '#{redirect_route.path}' was moved to '#{group.full_path}'. Please update any links and bookmarks that may still have the old path."
+  end
 end
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index 993654fddaa..4626f1ebc29 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -214,12 +214,43 @@ describe GroupsController do
       end
 
       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
+        context 'when requesting the canonical path' do
+          context 'non-show path' do
+            context 'with exactly matching casing' do
+              it 'does not redirect' do
+                get :issues, id: group.to_param
+
+                expect(response).not_to have_http_status(301)
+              end
+            end
 
-            expect(response).to redirect_to(issues_group_path(group.to_param))
-            expect(controller).not_to set_flash[:notice]
+            context '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
+          end
+
+          context 'show path' do
+            context 'with exactly matching casing' do
+              it 'does not redirect' do
+                get :show, id: group.to_param
+
+                expect(response).not_to have_http_status(301)
+              end
+            end
+
+            context 'with different casing' do
+              it 'redirects to the correct casing at the root path' do
+                get :show, id: group.to_param.upcase
+
+                expect(response).to redirect_to(group)
+                expect(controller).not_to set_flash[:notice]
+              end
+            end
           end
         end
 
diff --git a/spec/controllers/projects/labels_controller_spec.rb b/spec/controllers/projects/labels_controller_spec.rb
index 05999431d8f..130b0b744b5 100644
--- a/spec/controllers/projects/labels_controller_spec.rb
+++ b/spec/controllers/projects/labels_controller_spec.rb
@@ -157,4 +157,74 @@ describe Projects::LabelsController do
       end
     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 'non-show path' do
+          context 'with exactly matching casing' do
+            it 'does not redirect' do
+              get :index, namespace_id: project.namespace, project_id: project.to_param
+
+              expect(response).not_to have_http_status(301)
+            end
+          end
+
+          context 'with different casing' do
+            it 'redirects to the correct casing' do
+              get :index, namespace_id: project.namespace, project_id: project.to_param.upcase
+
+              expect(response).to redirect_to(namespace_project_labels_path(project.namespace, project))
+              expect(controller).not_to set_flash[:notice]
+            end
+          end
+        end
+      end
+
+      context 'when requesting a redirected path' do
+        let!(:redirect_route) { project.redirect_routes.create(path: project.full_path + 'old') }
+
+        it 'redirects to the canonical path' do
+          get :index, namespace_id: project.namespace, project_id: project.to_param + 'old'
+
+          expect(response).to redirect_to(namespace_project_labels_path(project.namespace, project))
+          expect(controller).to set_flash[:notice].to(project_moved_message(redirect_route, project))
+        end
+      end
+    end
+  end
+
+  context 'for a non-GET request' do
+    context 'when requesting the canonical path with different casing' do
+      it 'does not 404' do
+        post :generate, namespace_id: project.namespace, project_id: project
+
+        expect(response).not_to have_http_status(404)
+      end
+
+      it 'does not redirect to the correct casing' do
+        post :generate, namespace_id: project.namespace, project_id: project
+
+        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: project.full_path + 'old') }
+
+      it 'returns not found' do
+        post :generate, namespace_id: project.namespace, project_id: project.to_param + 'old'
+
+        expect(response).to have_http_status(404)
+      end
+    end
+  end
+
+  def project_moved_message(redirect_route, project)
+    "Project '#{redirect_route.path}' was moved to '#{project.full_path}'. Please update any links and bookmarks that may still have the old path."
+  end
 end
diff --git a/spec/support/milestone_tabs_examples.rb b/spec/support/milestone_tabs_examples.rb
index c69f8e11008..4ad8b0a16e1 100644
--- a/spec/support/milestone_tabs_examples.rb
+++ b/spec/support/milestone_tabs_examples.rb
@@ -1,7 +1,7 @@
 shared_examples 'milestone tabs' do
   def go(path, extra_params = {})
     params = if milestone.is_a?(GlobalMilestone)
-               { group_id: group.id, id: milestone.safe_title, title: milestone.title }
+               { group_id: group.to_param, id: milestone.safe_title, title: milestone.title }
              else
                { namespace_id: project.namespace.to_param, project_id: project, id: milestone.iid }
              end
-- 
cgit v1.2.3