From 3571a16ade385deb5622c7841cccdfef01b354fb Mon Sep 17 00:00:00 2001
From: Pirate Praveen <praveen@debian.org>
Date: Thu, 4 Feb 2021 15:43:07 +0530
Subject: [PATCH] New upstream version 13.6.6

---
 CHANGELOG.md                                  | 11 +++
 GITALY_SERVER_VERSION                         |  2 +-
 VERSION                                       |  2 +-
 .../mr_widget_pipeline_container.vue          |  3 +-
 .../projects/releases_controller.rb           |  3 +
 app/presenters/release_presenter.rb           |  2 +
 config/routes.rb                              |  1 +
 config/routes/unmatched_project.rb            | 18 +++++
 .../query_analyzers/logger_analyzer.rb        | 14 +++-
 lib/gitlab/url_blocker.rb                     |  4 +-
 .../projects/releases_controller_spec.rb      |  9 +++
 .../mr_widget_pipeline_container_spec.js      | 12 ++++
 .../query_analyzers/logger_analyzer_spec.rb   | 18 +++++
 spec/lib/gitlab/url_blocker_spec.rb           | 15 ++++
 spec/presenters/release_presenter_spec.rb     |  6 ++
 spec/requests/git_http_spec.rb                |  8 ++-
 spec/routing/project_routing_spec.rb          | 69 +++++++++++++++++++
 .../route_to_route_not_found_matcher.rb       | 15 ++++
 18 files changed, 203 insertions(+), 9 deletions(-)
 create mode 100644 config/routes/unmatched_project.rb
 create mode 100644 spec/support/matchers/route_to_route_not_found_matcher.rb

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d995ed28de..e520b162a2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,17 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 13.6.6 (2021-02-01)
+
+### Security (5 changes)
+
+- Filter sensitive GraphQL variables from logs.
+- Avoid exposing release links when the user cannot read git-tag/repository.
+- Sanitize target branch on MR page.
+- Fix DNS rebinding protection bypass when allowing an IP address in Outbound Requests setting.
+- Add routes for unmatched url for not-get requests.
+
+
 ## 13.6.5 (2021-01-13)
 
 ### Security (1 change)
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index b22e91bfa5..4c39112331 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-13.6.5
\ No newline at end of file
+13.6.6
\ No newline at end of file
diff --git a/VERSION b/VERSION
index b22e91bfa5..4c39112331 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-13.6.5
\ No newline at end of file
+13.6.6
\ No newline at end of file
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline_container.vue b/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline_container.vue
index 55efd7e7d3..953710ccbf 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline_container.vue
+++ b/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline_container.vue
@@ -1,5 +1,6 @@
 <script>
 import { isNumber } from 'lodash';
+import { sanitize } from '~/lib/dompurify';
 import ArtifactsApp from './artifacts_list_app.vue';
 import Deployment from './deployment/deployment.vue';
 import MrWidgetContainer from './mr_widget_container.vue';
@@ -41,7 +42,7 @@ export default {
       return this.isPostMerge ? this.mr.targetBranch : this.mr.sourceBranch;
     },
     branchLink() {
-      return this.isPostMerge ? this.mr.targetBranch : this.mr.sourceBranchLink;
+      return this.isPostMerge ? sanitize(this.mr.targetBranch) : this.mr.sourceBranchLink;
     },
     deployments() {
       return this.isPostMerge ? this.mr.postMergeDeployments : this.mr.deployments;
diff --git a/app/controllers/projects/releases_controller.rb b/app/controllers/projects/releases_controller.rb
index a6e795a2b9..614bada09e 100644
--- a/app/controllers/projects/releases_controller.rb
+++ b/app/controllers/projects/releases_controller.rb
@@ -5,6 +5,9 @@ class Projects::ReleasesController < Projects::ApplicationController
   before_action :require_non_empty_project, except: [:index]
   before_action :release, only: %i[edit show update downloads]
   before_action :authorize_read_release!
+  # We have to check `download_code` permission because detail URL path
+  # contains git-tag name.
+  before_action :authorize_download_code!, except: [:index]
   before_action do
     push_frontend_feature_flag(:graphql_release_data, project, default_enabled: true)
     push_frontend_feature_flag(:graphql_milestone_stats, project, default_enabled: true)
diff --git a/app/presenters/release_presenter.rb b/app/presenters/release_presenter.rb
index b11585d0d1..aa6429ab01 100644
--- a/app/presenters/release_presenter.rb
+++ b/app/presenters/release_presenter.rb
@@ -20,6 +20,8 @@ class ReleasePresenter < Gitlab::View::Presenter::Delegated
   end
 
   def self_url
+    return unless can_download_code?
+
     project_release_url(project, release)
   end
 
diff --git a/config/routes.rb b/config/routes.rb
index 867e5c2ec4..254b4b21fb 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -267,6 +267,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
diff --git a/config/routes/unmatched_project.rb b/config/routes/unmatched_project.rb
new file mode 100644
index 0000000000..b4fe243c7b
--- /dev/null
+++ b/config/routes/unmatched_project.rb
@@ -0,0 +1,18 @@
+# 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
diff --git a/lib/gitlab/graphql/query_analyzers/logger_analyzer.rb b/lib/gitlab/graphql/query_analyzers/logger_analyzer.rb
index 1285365376..0665ea8b6c 100644
--- a/lib/gitlab/graphql/query_analyzers/logger_analyzer.rb
+++ b/lib/gitlab/graphql/query_analyzers/logger_analyzer.rb
@@ -49,13 +49,21 @@ module Gitlab
         private
 
         def process_variables(variables)
-          if variables.respond_to?(:to_s)
-            variables.to_s
+          filtered_variables = filter_sensitive_variables(variables)
+
+          if filtered_variables.respond_to?(:to_s)
+            filtered_variables.to_s
           else
-            variables
+            filtered_variables
           end
         end
 
+        def filter_sensitive_variables(variables)
+          ActiveSupport::ParameterFilter
+            .new(::Rails.application.config.filter_parameters)
+            .filter(variables)
+        end
+
         def duration(time_started)
           Gitlab::Metrics::System.monotonic_time - time_started
         end
diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb
index eece2c343d..10822f943b 100644
--- a/lib/gitlab/url_blocker.rb
+++ b/lib/gitlab/url_blocker.rb
@@ -49,10 +49,12 @@ module Gitlab
         return [uri, nil] unless address_info
 
         ip_address = ip_address(address_info)
-        return [uri, nil] if domain_allowed?(uri) || ip_allowed?(ip_address, port: get_port(uri))
+        return [uri, nil] if domain_allowed?(uri)
 
         protected_uri_with_hostname = enforce_uri_hostname(ip_address, uri, dns_rebind_protection)
 
+        return protected_uri_with_hostname if ip_allowed?(ip_address, port: get_port(uri))
+
         # Allow url from the GitLab instance itself but only for the configured hostname and ports
         return protected_uri_with_hostname if internal?(uri)
 
diff --git a/spec/controllers/projects/releases_controller_spec.rb b/spec/controllers/projects/releases_controller_spec.rb
index 07fb03b39c..0670674cfe 100644
--- a/spec/controllers/projects/releases_controller_spec.rb
+++ b/spec/controllers/projects/releases_controller_spec.rb
@@ -9,6 +9,7 @@ RSpec.describe Projects::ReleasesController do
   let_it_be(:private_project) { create(:project, :repository, :private) }
   let_it_be(:developer)  { create(:user) }
   let_it_be(:reporter)   { create(:user) }
+  let_it_be(:guest)      { create(:user) }
   let_it_be(:user)       { developer }
   let!(:release_1)       { create(:release, project: project, released_at: Time.zone.parse('2018-10-18')) }
   let!(:release_2)       { create(:release, project: project, released_at: Time.zone.parse('2019-10-19')) }
@@ -16,6 +17,7 @@ RSpec.describe Projects::ReleasesController do
   before do
     project.add_developer(developer)
     project.add_reporter(reporter)
+    project.add_guest(guest)
   end
 
   shared_examples_for 'successful request' do
@@ -199,6 +201,13 @@ RSpec.describe Projects::ReleasesController do
 
       it_behaves_like 'not found'
     end
+
+    context 'when user is a guest' do
+      let(:project) { private_project }
+      let(:user) { guest }
+
+      it_behaves_like 'not found'
+    end
   end
 
   # `GET #downloads` is addressed in spec/requests/projects/releases_controller_spec.rb
diff --git a/spec/frontend/vue_mr_widget/components/mr_widget_pipeline_container_spec.js b/spec/frontend/vue_mr_widget/components/mr_widget_pipeline_container_spec.js
index d67f1adadf..e4da729bec 100644
--- a/spec/frontend/vue_mr_widget/components/mr_widget_pipeline_container_spec.js
+++ b/spec/frontend/vue_mr_widget/components/mr_widget_pipeline_container_spec.js
@@ -78,6 +78,18 @@ describe('MrWidgetPipelineContainer', () => {
       });
     });
 
+    it('sanitizes the targetBranch', () => {
+      factory({
+        isPostMerge: true,
+        mr: {
+          ...mockStore,
+          targetBranch: 'Foo<script>alert("XSS")</script>',
+        },
+      });
+
+      expect(wrapper.find(MrWidgetPipeline).props().sourceBranchLink).toBe('Foo');
+    });
+
     it('renders deployments', () => {
       const expectedProps = mockStore.postMergeDeployments.map(dep =>
         expect.objectContaining({
diff --git a/spec/lib/gitlab/graphql/query_analyzers/logger_analyzer_spec.rb b/spec/lib/gitlab/graphql/query_analyzers/logger_analyzer_spec.rb
index c843251318..138765afd8 100644
--- a/spec/lib/gitlab/graphql/query_analyzers/logger_analyzer_spec.rb
+++ b/spec/lib/gitlab/graphql/query_analyzers/logger_analyzer_spec.rb
@@ -40,4 +40,22 @@ RSpec.describe Gitlab::Graphql::QueryAnalyzers::LoggerAnalyzer do
       end
     end
   end
+
+  describe '#initial_value' do
+    it 'filters out sensitive variables' do
+      doc = GraphQL.parse <<-GRAPHQL
+        mutation createNote($body: String!) {
+          createNote(input: {noteableId: "1", body: $body}) {
+            note {
+              id
+            }
+          }
+        }
+      GRAPHQL
+
+      query = GraphQL::Query.new(GitlabSchema, document: doc, context: {}, variables: { body: "some note" })
+
+      expect(subject.initial_value(query)[:variables]).to eq('{:body=>"[FILTERED]"}')
+    end
+  end
 end
diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb
index f466d11785..686382dc26 100644
--- a/spec/lib/gitlab/url_blocker_spec.rb
+++ b/spec/lib/gitlab/url_blocker_spec.rb
@@ -91,6 +91,21 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only do
       end
     end
 
+    context 'DNS rebinding protection with IP allowed' do
+      let(:import_url) { 'http://a.192.168.0.120.3times.127.0.0.1.1time.repeat.rebind.network:9121/scrape?target=unix:///var/opt/gitlab/redis/redis.socket&amp;check-keys=*' }
+
+      before do
+        stub_dns(import_url, ip_address: '192.168.0.120')
+
+        allow(Gitlab::UrlBlockers::UrlAllowlist).to receive(:ip_allowed?).and_return(true)
+      end
+
+      it_behaves_like 'validates URI and hostname' do
+        let(:expected_uri) { 'http://192.168.0.120:9121/scrape?target=unix:///var/opt/gitlab/redis/redis.socket&amp;check-keys=*' }
+        let(:expected_hostname) { 'a.192.168.0.120.3times.127.0.0.1.1time.repeat.rebind.network' }
+      end
+    end
+
     context 'disabled DNS rebinding protection' do
       subject { described_class.validate!(import_url, dns_rebind_protection: false) }
 
diff --git a/spec/presenters/release_presenter_spec.rb b/spec/presenters/release_presenter_spec.rb
index b518584569..4bf12183ef 100644
--- a/spec/presenters/release_presenter_spec.rb
+++ b/spec/presenters/release_presenter_spec.rb
@@ -62,6 +62,12 @@ RSpec.describe ReleasePresenter do
     it 'returns its own url' do
       is_expected.to eq(project_release_url(project, release))
     end
+
+    context 'when user is guest' do
+      let(:user) { guest }
+
+      it { is_expected.to be_nil }
+    end
   end
 
   describe '#opened_merge_requests_url' do
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 32aeeed43b..3049ca46d7 100644
--- a/spec/requests/git_http_spec.rb
+++ b/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
diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb
index a683dc28f4..b414e8403b 100644
--- a/spec/routing/project_routing_spec.rb
+++ b/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
diff --git a/spec/support/matchers/route_to_route_not_found_matcher.rb b/spec/support/matchers/route_to_route_not_found_matcher.rb
new file mode 100644
index 0000000000..4105f0f919
--- /dev/null
+++ b/spec/support/matchers/route_to_route_not_found_matcher.rb
@@ -0,0 +1,15 @@
+# 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