Merge branch 'security-404-difference' into 'master'

Add routes for unmatched url for not-get requests

See merge request gitlab-org/security/gitlab!1119

changelogs/unreleased/secutity-404-difference.yml

+---
+title: Add routes for unmatched url for not-get requests
+merge_request:
+author:
+type: security

config/routes.rb

@@ -276,6 +276,7 @@ Rails.application.routes.draw do
   draw :dashboard
   draw :user
   draw :project
+  draw :unmatched_project
 
   # Issue https://gitlab.com/gitlab-org/gitlab/-/issues/210024
   scope as: 'deprecated' do

config/routes/unmatched_project.rb

+# frozen_string_literal: true
+
+scope(path: '*namespace_id',
+  as: :namespace,
+  namespace_id: Gitlab::PathRegex.full_namespace_route_regex) do
+  scope(path: ':project_id',
+    constraints: { project_id: Gitlab::PathRegex.project_route_regex },
+    as: :project) do
+    post '*all', to: 'application#route_not_found'
+    put '*all', to: 'application#route_not_found'
+    patch '*all', to: 'application#route_not_found'
+    delete '*all', to: 'application#route_not_found'
+    post '/', to: 'application#route_not_found'
+    put '/', to: 'application#route_not_found'
+    patch '/', to: 'application#route_not_found'
+    delete '/', to: 'application#route_not_found'
+  end
+end

ee/spec/routing/git_http_routing_spec.rb

@@ -9,5 +9,11 @@ RSpec.describe 'EE git_http routing' do
       let(:container_path) { '/gitlab-org/gitlab-test' }
       let(:params) { { geo_node_id: 'node', repository_path: 'gitlab-org/gitlab-test.git' } }
     end
+
+    it_behaves_like 'git repository routes with fallback for git-upload-pack' do
+      let(:path) { '/-/push_from_secondary/node/gitlab-org/gitlab-test.git' }
+      let(:container_path) { '/gitlab-org/gitlab-test' }
+      let(:params) { { geo_node_id: 'node', repository_path: 'gitlab-org/gitlab-test.git' } }
+    end
   end
 end

spec/requests/git_http_spec.rb

@@ -159,13 +159,17 @@ RSpec.describe 'Git HTTP requests' do
 
     context "POST git-upload-pack" do
       it "fails to find a route" do
-        expect { clone_post(repository_path) }.to raise_error(ActionController::RoutingError)
+        clone_post(repository_path) do |response|
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
       end
     end
 
     context "POST git-receive-pack" do
       it "fails to find a route" do
-        expect { push_post(repository_path) }.to raise_error(ActionController::RoutingError)
+        push_post(repository_path) do |response|
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
       end
     end
   end

spec/routing/git_http_routing_spec.rb

@@ -7,6 +7,10 @@ RSpec.describe 'git_http routing' do
     it_behaves_like 'git repository routes' do
       let(:path) { '/gitlab-org/gitlab-test.git' }
     end
+
+    it_behaves_like 'git repository routes with fallback for git-upload-pack' do
+      let(:path) { '/gitlab-org/gitlab-test.git' }
+    end
   end
 
   describe 'wiki repositories' do
@@ -14,6 +18,7 @@ RSpec.describe 'git_http routing' do
       let(:path) { '/gitlab-org/gitlab-test.wiki.git' }
 
       it_behaves_like 'git repository routes'
+      it_behaves_like 'git repository routes with fallback for git-upload-pack'
 
       describe 'redirects', type: :request do
         let(:web_path) { '/gitlab-org/gitlab-test/-/wikis' }
@@ -37,12 +42,20 @@ RSpec.describe 'git_http routing' do
       it_behaves_like 'git repository routes' do
         let(:path) { '/gitlab-org.wiki.git' }
       end
+
+      it_behaves_like 'git repository routes with fallback for git-upload-pack' do
+        let(:path) { '/gitlab-org.wiki.git' }
+      end
     end
 
     context 'in child group' do
       it_behaves_like 'git repository routes' do
         let(:path) { '/gitlab-org/child.wiki.git' }
       end
+
+      it_behaves_like 'git repository routes with fallback for git-upload-pack' do
+        let(:path) { '/gitlab-org/child.wiki.git' }
+      end
     end
   end
 
@@ -51,12 +64,20 @@ RSpec.describe 'git_http routing' do
       it_behaves_like 'git repository routes' do
         let(:path) { '/snippets/123.git' }
       end
+
+      it_behaves_like 'git repository routes without fallback' do
+        let(:path) { '/snippets/123.git' }
+      end
     end
 
     context 'project snippet' do
       it_behaves_like 'git repository routes' do
         let(:path) { '/gitlab-org/gitlab-test/snippets/123.git' }
       end
+
+      it_behaves_like 'git repository routes with fallback' do
+        let(:path) { '/gitlab-org/gitlab-test/snippets/123.git' }
+      end
     end
   end
 end

spec/routing/project_routing_spec.rb

@@ -876,4 +876,73 @@ RSpec.describe 'project routing' do
       )
     end
   end
+
+  context 'with a non-existent project' do
+    it 'routes to 404 with get request' do
+      expect(get: "/gitlab/not_exist").to route_to(
+        'application#route_not_found',
+        unmatched_route: 'gitlab/not_exist'
+      )
+    end
+
+    it 'routes to 404 with delete request' do
+      expect(delete: "/gitlab/not_exist").to route_to(
+        'application#route_not_found',
+        namespace_id: 'gitlab',
+        project_id: 'not_exist'
+      )
+    end
+
+    it 'routes to 404 with post request' do
+      expect(post: "/gitlab/not_exist").to route_to(
+        'application#route_not_found',
+        namespace_id: 'gitlab',
+        project_id: 'not_exist'
+      )
+    end
+
+    it 'routes to 404 with put request' do
+      expect(put: "/gitlab/not_exist").to route_to(
+        'application#route_not_found',
+        namespace_id: 'gitlab',
+        project_id: 'not_exist'
+      )
+    end
+
+    context 'with route to some action' do
+      it 'routes to 404 with get request to' do
+        expect(get: "/gitlab/not_exist/some_action").to route_to(
+          'application#route_not_found',
+          unmatched_route: 'gitlab/not_exist/some_action'
+        )
+      end
+
+      it 'routes to 404 with delete request' do
+        expect(delete: "/gitlab/not_exist/some_action").to route_to(
+          'application#route_not_found',
+          namespace_id: 'gitlab',
+          project_id: 'not_exist',
+          all: 'some_action'
+        )
+      end
+
+      it 'routes to 404 with post request' do
+        expect(post: "/gitlab/not_exist/some_action").to route_to(
+          'application#route_not_found',
+          namespace_id: 'gitlab',
+          project_id: 'not_exist',
+          all: 'some_action'
+        )
+      end
+
+      it 'routes to 404 with put request' do
+        expect(put: "/gitlab/not_exist/some_action").to route_to(
+          'application#route_not_found',
+          namespace_id: 'gitlab',
+          project_id: 'not_exist',
+          all: 'some_action'
+        )
+      end
+    end
+  end
 end

spec/support/matchers/route_to_route_not_found_matcher.rb

+# frozen_string_literal: true
+
+RSpec::Matchers.define :route_to_route_not_found do
+  match do |actual|
+    expect(actual).to route_to(controller: 'application', action: 'route_not_found')
+  rescue RSpec::Expectations::ExpectationNotMetError => e
+    # `route_to` matcher requires providing all params for exact match. As we use it in shared examples and we provide different paths,
+    # this matcher checks if provided route matches controller and action, without checking params.
+    expect(e.message).to include("-{\"controller\"=>\"application\", \"action\"=>\"route_not_found\"}\n+{\"controller\"=>\"application\", \"action\"=>\"route_not_found\",")
+  end
+
+  failure_message do |_|
+    "expected #{actual} to route to route_not_found"
+  end
+end

spec/support/shared_examples/routing/git_http_routing_shared_examples.rb

@@ -16,10 +16,6 @@ RSpec.shared_examples 'git repository routes' do
       expect(get("#{container_path}/info/refs?service=git-upload-pack")).to redirect_to("#{container_path}.git/info/refs?service=git-upload-pack")
       expect(get("#{container_path}/info/refs?service=git-receive-pack")).to redirect_to("#{container_path}.git/info/refs?service=git-receive-pack")
     end
-
-    it 'does not redirect other requests' do
-      expect(post("#{container_path}/git-upload-pack")).not_to be_routable
-    end
   end
 
   it 'routes LFS endpoints' do
@@ -35,6 +31,56 @@ RSpec.shared_examples 'git repository routes' do
     expect(get("#{path}/gitlab-lfs/objects/#{oid}")).to route_to('repositories/lfs_storage#download', oid: oid, **params)
     expect(put("#{path}/gitlab-lfs/objects/#{oid}/456/authorize")).to route_to('repositories/lfs_storage#upload_authorize', oid: oid, size: '456', **params)
     expect(put("#{path}/gitlab-lfs/objects/#{oid}/456")).to route_to('repositories/lfs_storage#upload_finalize', oid: oid, size: '456', **params)
+  end
+end
+
+RSpec.shared_examples 'git repository routes without fallback' do
+  let(:container_path) { path.delete_suffix('.git') }
+
+  context 'requests without .git format' do
+    it 'does not redirect other requests' do
+      expect(post("#{container_path}/git-upload-pack")).not_to be_routable
+    end
+  end
+
+  it 'routes LFS endpoints for unmatched routes' do
+    oid = generate(:oid)
+
+    expect(put("#{path}/gitlab-lfs/objects/foo")).not_to be_routable
+    expect(put("#{path}/gitlab-lfs/objects/#{oid}/foo")).not_to be_routable
+    expect(put("#{path}/gitlab-lfs/objects/#{oid}/foo/authorize")).not_to be_routable
+  end
+end
+
+RSpec.shared_examples 'git repository routes with fallback' do
+  let(:container_path) { path.delete_suffix('.git') }
+
+  context 'requests without .git format' do
+    it 'does not redirect other requests' do
+      expect(post("#{container_path}/git-upload-pack")).to route_to_route_not_found
+    end
+  end
+
+  it 'routes LFS endpoints' do
+    oid = generate(:oid)
+
+    expect(put("#{path}/gitlab-lfs/objects/foo")).to route_to_route_not_found
+    expect(put("#{path}/gitlab-lfs/objects/#{oid}/foo")).to route_to_route_not_found
+    expect(put("#{path}/gitlab-lfs/objects/#{oid}/foo/authorize")).to route_to_route_not_found
+  end
+end
+
+RSpec.shared_examples 'git repository routes with fallback for git-upload-pack' do
+  let(:container_path) { path.delete_suffix('.git') }
+
+  context 'requests without .git format' do
+    it 'does not redirect other requests' do
+      expect(post("#{container_path}/git-upload-pack")).to route_to_route_not_found
+    end
+  end
+
+  it 'routes LFS endpoints for unmatched routes' do
+    oid = generate(:oid)
 
     expect(put("#{path}/gitlab-lfs/objects/foo")).not_to be_routable
     expect(put("#{path}/gitlab-lfs/objects/#{oid}/foo")).not_to be_routable