Add latest changes from gitlab-org/security/gitlab@12-4-stable-ee

changelogs/unreleased/security-34072-project-name-disclosed.yml

+---
+title: Hide project name and path when unsusbcribing from an issue or merge request
+merge_request:
+author:
+type: security

changelogs/unreleased/security-fix-invalid-byte-sequence-upload-links-master.yml

+---
+title: Fix 500 error caused by invalid byte sequences in uploads links
+merge_request:
+author:
+type: security

config/initializers/graphql.rb

@@ -5,3 +5,7 @@ GraphQL::Field.accepts_definitions(authorize: GraphQL::Define.assign_metadata_ke
 GraphQL::Schema::Object.accepts_definition(:authorize)
 GraphQL::Schema::Field.accepts_definition(:authorize)
+
+GitlabSchema.middleware << GraphQL::Schema::TimeoutMiddleware.new(max_seconds: ENV.fetch('GITLAB_RAILS_GRAPHQL_TIMEOUT', 30).to_i) do |timeout_error, query|
+  Gitlab::GraphqlLogger.error(message: timeout_error.to_s, query: query.query_string, query_variables: query.provided_variables)
+end

doc/development/pipelines.md

@@ -38,7 +38,8 @@ The current stages are:
 ## Default image
 The default image is currently
-`gitlab.com/gitlab-org/gitlab-build-images:ruby-2.6.3-golang-1.11-git-2.22-chrome-73.0-node-12.x-yarn-1.16-postgresql-9.6-graphicsmagick-1.3.33`.
+`registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-2.6.3-golang-1.11-git-2.22-chrome-73.0-node-12.x-yarn-1.16-postgresql-9.6-graphicsmagick-1.3.33`.
+
 It includes Ruby 2.6.3, Go 1.11, Git 2.22, Chrome 73, Node 12, Yarn 1.16,
 PostgreSQL 9.6, and Graphics Magick 1.3.33.
@@ -48,24 +49,13 @@ project, which is push-mirrored to <https://dev.gitlab.org/gitlab/gitlab-build-i
 for redundancy.
 The current version of the build images can be found in the
-["Used by GitLab CE/EE section"](https://gitlab.com/gitlab-org/gitlab-build-images/blob/master/.gitlab-ci.yml).
+["Used by GitLab section"](https://gitlab.com/gitlab-org/gitlab-build-images/blob/master/.gitlab-ci.yml).
 ## Default variables
 In addition to the [predefined variables](../ci/variables/predefined_variables.md),
-each pipeline includes the following [variables](../ci/variables/README.md):
-
-- `RAILS_ENV: "test"`
-- `NODE_ENV: "test"`
-- `SIMPLECOV: "true"`
-- `GIT_DEPTH: "50"`
-- `GIT_SUBMODULE_STRATEGY: "none"`
-- `GET_SOURCES_ATTEMPTS: "3"`
-- `KNAPSACK_RSPEC_SUITE_REPORT_PATH: knapsack/${CI_PROJECT_NAME}/rspec_report-master.json`
-- `FLAKY_RSPEC_SUITE_REPORT_PATH: rspec_flaky/report-suite.json`
-- `BUILD_ASSETS_IMAGE: "false"`
-- `ES_JAVA_OPTS: "-Xms256m -Xmx256m"`
-- `ELASTIC_URL: "http://elastic:changeme@docker.elastic.co-elasticsearch-elasticsearch:9200"`
+each pipeline includes default variables defined in
+<https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab-ci.yml>.
 ## Common job definitions
@@ -85,22 +75,35 @@ These common definitions are:
   Ruby/Rails and frontend tasks.
 - `.default-only`: Restricts the cases where a job is created. This currently
   includes `master`, `/^[\d-]+-stable(-ee)?$/` (stable branches),
-  `/^\d+-\d+-auto-deploy-\d+$/` (security branches), `merge_requests`, `tags`.
+  `/^\d+-\d+-auto-deploy-\d+$/` (auto-deploy branches), `/^security\//` (security branches), `merge_requests`, `tags`.
   Note that jobs won't be created for branches with this default configuration.
-- `.only-review`: Only creates a job for the `gitlab-org` namespace and if
-  Kubernetes integration is available. Also, prevents a job from being created
-  for `master` and auto-deploy branches.
-- `.only-review-schedules`: Same as `.only-review` but also restrict a job to
-  only run for [schedules](../user/project/pipelines/schedules.md).
-- `.only-canonical-schedules`: Only creates a job for scheduled pipelines in
-  the `gitlab-org/gitlab` and `gitlab-org/gitlab-foss` projects
+- `.only:variables-canonical-dot-com`: Only creates a job if the project is
+  located under <https://gitlab.com/gitlab-org>.
+- `.only:variables_refs-canonical-dot-com-schedules`: Same as
+  `.only:variables-canonical-dot-com` but add the condition that pipeline is scheduled.
+- `.except:refs-deploy`: Don't create a job if the `ref` is an auto-deploy branch.
+- `.except:refs-master-tags-stable-deploy`: Don't create a job if the `ref` is one of:
+  - `master`
+  - a tag
+  - a stable branch
+  - an auto-deploy branch
+- `.only:kubernetes`: Only creates a job if a Kubernetes integration is enabled
+  on the project.
+- `.only-review`: This extends from:
+  - `.only:variables-canonical-dot-com`
+  - `.only:kubernetes`
+  - `.except:refs-master-tags-stable-deploy`
+- `.only-review-schedules`: This extends from:
+  - `.only:variables_refs-canonical-dot-com-schedules`
+  - `.only:kubernetes`
+  - `.except:refs-deploy`
 - `.use-pg9`: Allows a job to use the `postgres:9.6` and `redis:alpine` services.
 - `.use-pg10`: Allows a job to use the `postgres:10.9` and `redis:alpine` services.
 - `.use-pg9-ee`: Same as `.use-pg9` but also use the
   `docker.elastic.co/elasticsearch/elasticsearch:5.6.12` services.
 - `.use-pg10-ee`: Same as `.use-pg10` but also use the
   `docker.elastic.co/elasticsearch/elasticsearch:5.6.12` services.
-- `.only-ee`: Only creates a job for the `gitlab` project.
+- `.only-ee`: Only creates a job for the `gitlab` or `gitlab-ee` project.
 - `.only-ee-as-if-foss`: Same as `.only-ee` but simulate the FOSS project by
   setting the `FOSS_ONLY='1'` environment variable.
@@ -111,11 +114,13 @@ the cases where it should be created
 [based on the changes](../ci/yaml/README.md#onlychangesexceptchanges)
 from a commit or MR by extending from the following CI definitions:
-- `.only-code-changes`: Allows a job to only be created upon code-related changes.
-- `.only-qa-changes`: Allows a job to only be created upon QA-related changes.
-- `.only-docs-changes`: Allows a job to only be created upon docs-related changes.
-- `.only-code-qa-changes`: Allows a job to only be created upon code-related or QA-related changes.
-- `.only-graphql-changes`: Allows a job to only be created upon graphql-related changes.
+- `.only:changes-code`: Allows a job to only be created upon code-related changes.
+- `.only:changes-qa`: Allows a job to only be created upon QA-related changes.
+- `.only:changes-docs`: Allows a job to only be created upon docs-related changes.
+- `.only:changes-graphql`: Allows a job to only be created upon GraphQL-related changes.
+- `.only:changes-code-backstage`: Allows a job to only be created upon code-related or backstage-related (e.g. Danger, RuboCop, specs) changes.
+- `.only:changes-code-qa`: Allows a job to only be created upon code-related or QA-related changes.
+- `.only:changes-code-backstage-qa`: Allows a job to only be created upon code-related, backstage-related (e.g. Danger, RuboCop, specs) or QA-related changes.
 **See <https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab/ci/global.gitlab-ci.yml>
 for the list of exact patterns.**

lib/banzai/filter/relative_link_filter.rb

@@ -116,7 +116,7 @@ module Banzai
       end
       def process_link_to_upload_attr(html_attr)
-        path_parts = [Addressable::URI.unescape(html_attr.value)]
+        path_parts = [unescape_and_scrub_uri(html_attr.value)]
         if project
           path_parts.unshift(relative_url_root, project.full_path)
@@ -172,7 +172,7 @@ module Banzai
       end
       def cleaned_file_path(uri)
-        Addressable::URI.unescape(uri.path).scrub.delete("\0").chomp("/")
+        unescape_and_scrub_uri(uri.path).delete("\0").chomp("/")
       end
       def relative_file_path(uri)
@@ -184,7 +184,7 @@ module Banzai
       def request_path
         return unless context[:requested_path]
-        Addressable::URI.unescape(context[:requested_path]).chomp("/")
+        unescape_and_scrub_uri(context[:requested_path]).chomp("/")
       end
       # Convert a relative path into its correct location based on the currently
@@ -266,6 +266,12 @@ module Banzai
       def repository
         @repository ||= project&.repository
       end
+
+      private
+
+      def unescape_and_scrub_uri(uri)
+        Addressable::URI.unescape(uri).scrub
+      end
     end
   end
 end

locale/gitlab.pot

@@ -7886,6 +7886,9 @@ msgstr ""
 msgid "GitHub import"
 msgstr ""
+msgid "GitLab / Unsubscribe"
+msgstr ""
+
 msgid "GitLab CI Linter has been moved"
 msgstr ""

spec/controllers/profiles/notifications_controller_spec.rb

@@ -52,6 +52,35 @@ describe Profiles::NotificationsController do
         end.to exceed_query_limit(control)
       end
     end
+
+    context 'with project notifications' do
+      let!(:notification_setting) { create(:notification_setting, source: project, user: user, level: :watch) }
+
+      before do
+        sign_in(user)
+        get :show
+      end
+
+      context 'when project is public' do
+        let(:project) { create(:project, :public) }
+
+        it 'shows notification setting for project' do
+          expect(assigns(:project_notifications).map(&:source_id)).to include(project.id)
+        end
+      end
+
+      context 'when project is public' do
+        let(:project) { create(:project, :private) }
+
+        it 'shows notification setting for project' do
+          # notification settings for given project were created before project was set to private
+          expect(user.notification_settings.for_projects.map(&:source_id)).to include(project.id)
+
+          # check that notification settings for project where user does not have access are filtered
+          expect(assigns(:project_notifications)).to be_empty
+        end
+      end
+    end
   end
   describe 'POST update' do

spec/controllers/sent_notifications_controller_spec.rb

@@ -56,7 +56,7 @@ describe SentNotificationsController do
           get(:unsubscribe, params: { id: sent_notification.reply_key })
         end
-        shared_examples 'unsubscribing as anonymous' do
+        shared_examples 'unsubscribing as anonymous' do |project_visibility|
           it 'does not unsubscribe the user' do
             expect(noteable.subscribed?(user, target_project)).to be_truthy
           end
@@ -69,6 +69,18 @@ describe SentNotificationsController do
             expect(response.status).to eq(200)
             expect(response).to render_template :unsubscribe
           end
+
+          if project_visibility == :private
+            it 'does not show project name or path' do
+              expect(response.body).not_to include(noteable.project.name)
+              expect(response.body).not_to include(noteable.project.full_name)
+            end
+          else
+            it 'shows project name or path' do
+              expect(response.body).to include(noteable.project.name)
+              expect(response.body).to include(noteable.project.full_name)
+            end
+          end
         end
         context 'when project is public' do
@@ -79,7 +91,7 @@ describe SentNotificationsController do
               expect(response.body).to include(issue.title)
             end
-            it_behaves_like 'unsubscribing as anonymous'
+            it_behaves_like 'unsubscribing as anonymous', :public
           end
           context 'when unsubscribing from confidential issue' do
@@ -90,7 +102,7 @@ describe SentNotificationsController do
               expect(response.body).to include(confidential_issue.to_reference)
             end
-            it_behaves_like 'unsubscribing as anonymous'
+            it_behaves_like 'unsubscribing as anonymous', :public
           end
           context 'when unsubscribing from merge request' do
@@ -100,7 +112,12 @@ describe SentNotificationsController do
               expect(response.body).to include(merge_request.title)
             end
-            it_behaves_like 'unsubscribing as anonymous'
+            it 'shows project name or path' do
+              expect(response.body).to include(issue.project.name)
+              expect(response.body).to include(issue.project.full_name)
+            end
+
+            it_behaves_like 'unsubscribing as anonymous', :public
           end
         end
@@ -110,11 +127,11 @@ describe SentNotificationsController do
           context 'when unsubscribing from issue' do
             let(:noteable) { issue }
-            it 'shows issue title' do
+            it 'does not show issue title' do
               expect(response.body).not_to include(issue.title)
             end
-            it_behaves_like 'unsubscribing as anonymous'
+            it_behaves_like 'unsubscribing as anonymous', :private
           end
           context 'when unsubscribing from confidential issue' do
@@ -125,17 +142,17 @@ describe SentNotificationsController do
               expect(response.body).to include(confidential_issue.to_reference)
             end
-            it_behaves_like 'unsubscribing as anonymous'
+            it_behaves_like 'unsubscribing as anonymous', :private
           end
           context 'when unsubscribing from merge request' do
             let(:noteable) { merge_request }
-            it 'shows merge request title' do
+            it 'dos not show merge request title' do
               expect(response.body).not_to include(merge_request.title)
             end
-            it_behaves_like 'unsubscribing as anonymous'
+            it_behaves_like 'unsubscribing as anonymous', :private
           end
         end
       end

spec/lib/banzai/filter/relative_link_filter_spec.rb

@@ -124,6 +124,15 @@ describe Banzai::Filter::RelativeLinkFilter do
     expect { filter(act) }.not_to raise_error
   end
+  it 'does not raise an exception on URIs containing invalid utf-8 byte sequences in uploads' do
+    act = link("/uploads/%FF")
+    expect { filter(act) }.not_to raise_error
+  end
+
+  it 'does not raise an exception on URIs containing invalid utf-8 byte sequences in context requested path' do
+    expect { filter(link("files/test.md"), requested_path: '%FF') }.not_to raise_error
+  end
+
   it 'does not raise an exception with a garbled path' do
     act = link("open(/var/tmp/):%20/location%0Afrom:%20/test")
     expect { filter(act) }.not_to raise_error

spec/models/user_spec.rb

@@ -2533,8 +2533,8 @@ describe User do
           add_user(:maintainer)
         end
-        it 'loads' do
-          expect(user.ci_owned_runners).to contain_exactly(runner)
+        it 'does not load' do
+          expect(user.ci_owned_runners).to be_empty
         end
       end
@@ -2549,6 +2549,20 @@ describe User do
       end
     end
+    shared_examples :group_member do
+      context 'when the user is owner' do
+        before do
+          add_user(:owner)
+        end
+
+        it 'loads' do
+          expect(user.ci_owned_runners).to contain_exactly(runner)
+        end
+      end
+
+      it_behaves_like :member
+    end
+
     context 'with groups projects runners' do
       let(:group) { create(:group) }
       let!(:project) { create(:project, group: group) }
@@ -2557,7 +2571,7 @@ describe User do
         group.add_user(user, access)
       end
-      it_behaves_like :member
+      it_behaves_like :group_member
     end
     context 'with groups runners' do
@@ -2568,14 +2582,14 @@ describe User do
         group.add_user(user, access)
       end
-      it_behaves_like :member
+      it_behaves_like :group_member
     end
     context 'with other projects runners' do
       let!(:project) { create(:project) }
       def add_user(access)
-        project.add_role(user, access)
+        project.add_user(user, access)
       end
       it_behaves_like :member
@@ -2593,7 +2607,7 @@ describe User do
         subgroup.add_user(another_user, :owner)
       end
-      it_behaves_like :member
+      it_behaves_like :group_member
     end
   end

spec/requests/api/graphql/gitlab_schema_spec.rb

@@ -6,6 +6,18 @@ describe 'GitlabSchema configurations' do
   set(:project) { create(:project) }
   shared_examples 'imposing query limits' do
+    describe 'timeouts' do
+      context 'when timeout is reached' do
+        it 'shows an error' do
+          Timecop.scale(50000000) do # ludicrously large number because the timeout has to happen before the query even begins
+            subject
+
+            expect_graphql_errors_to_include /Timeout/
+          end
+        end
+      end
+    end
+
     describe '#max_complexity' do
       context 'when complexity is too high' do
         it 'shows an error' do

spec/requests/api/runners_spec.rb

@@ -4,6 +4,7 @@ describe API::Runners do
   let(:admin) { create(:user, :admin) }
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
+  let(:group_maintainer) { create(:user) }
   let(:project) { create(:project, creator_id: user.id) }
   let(:project2) { create(:project, creator_id: user.id) }
@@ -18,6 +19,7 @@ describe API::Runners do
   before do
     # Set project access for users
+    create(:group_member, :maintainer, user: group_maintainer, group: group)
     create(:project_member, :maintainer, user: user, project: project)
     create(:project_member, :maintainer, user: user, project: project2)
     create(:project_member, :reporter, user: user2, project: project)
@@ -523,6 +525,20 @@ describe API::Runners do
           end.to change { Ci::Runner.project_type.count }.by(-1)
         end
+        it 'does not delete group runner with maintainer access' do
+          delete api("/runners/#{group_runner.id}", group_maintainer)
+
+          expect(response).to have_http_status(403)
+        end
+
+        it 'deletes group runner with owner access' do
+          expect do
+            delete api("/runners/#{group_runner.id}", user)
+
+            expect(response).to have_http_status(204)
+          end.to change { Ci::Runner.group_type.count }.by(-1)
+        end
+
         it_behaves_like '412 response' do
           let(:request) { api("/runners/#{project_runner.id}", user) }
         end