New upstream version 12.6.8

.gitlab/ci/global.gitlab-ci.yml

@@ -200,6 +200,8 @@
     - name: postgres:9.6
       command: ["postgres", "-c", "fsync=off", "-c", "synchronous_commit=off", "-c", "full_page_writes=off"]
     - name: redis:alpine
+  variables:
+    POSTGRES_HOST_AUTH_METHOD: trust
 
 .use-pg10:
   image: "registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-2.6.3-golang-1.12-git-2.24-lfs-2.9-chrome-73.0-node-12.x-yarn-1.16-postgresql-10-graphicsmagick-1.3.33"
@@ -207,6 +209,8 @@
     - name: postgres:10.9
       command: ["postgres", "-c", "fsync=off", "-c", "synchronous_commit=off", "-c", "full_page_writes=off"]
     - name: redis:alpine
+  variables:
+    POSTGRES_HOST_AUTH_METHOD: trust
 
 .use-pg9-ee:
   services:
@@ -214,6 +218,8 @@
       command: ["postgres", "-c", "fsync=off", "-c", "synchronous_commit=off", "-c", "full_page_writes=off"]
     - name: redis:alpine
     - name: elasticsearch:5.6.12
+  variables:
+    POSTGRES_HOST_AUTH_METHOD: trust
 
 .use-pg10-ee:
   image: "registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-2.6.3-golang-1.12-git-2.24-lfs-2.9-chrome-73.0-node-12.x-yarn-1.16-postgresql-10-graphicsmagick-1.3.33"
@@ -222,6 +228,8 @@
       command: ["postgres", "-c", "fsync=off", "-c", "synchronous_commit=off", "-c", "full_page_writes=off"]
     - name: redis:alpine
     - name: elasticsearch:5.6.12
+  variables:
+    POSTGRES_HOST_AUTH_METHOD: trust
 
 .only-ee:
   only:

.gitlab/issue_templates/Security developer workflow.md

@@ -1,60 +1,57 @@
 <!--
 # Read me first!
 
-Create this issue under https://dev.gitlab.org/gitlab/gitlabhq
+Create this issue under https://gitlab.com/gitlab-org/security
 
 Set the title to: `Description of the original issue`
 -->
 
-### Prior to starting the security release work
+## Prior to starting the security release work
 
 - [ ] Read the [security process for developers] if you are not familiar with it.
-- [ ] Link to the original issue adding it to the [links section](#links)
+- [ ] Mark this [issue as related] to the Security Release tracking issue. You can find it on the topic of the `#releases` Slack channel.
-- [ ] Run `scripts/security-harness` in the CE, EE, and/or Omnibus to prevent pushing to any remote besides `dev.gitlab.org`
+- [ ] Run `scripts/security-harness` in your local repository to prevent accidentally pushing to any remote besides `gitlab.com/gitlab-org/security`.
-- [ ] Create a new branch prefixing it with `security-`
+- Fill out the [Links section](#links):
-- [ ] Create a MR targeting `dev.gitlab.org` `master`
+  - [ ] Next to **Issue on GitLab**, add a link to the `gitlab-org/gitlab` issue that describes the security vulnerability.
-- [ ] Add a link to this issue in the original security issue on `gitlab.com`.
+  - [ ] Next to **Security Release tracking issue**, add a link to the security release issue that will include this security issue.
 
-#### Backports
+## Development
+
+- [ ] Create a new branch prefixing it with `security-`.
+- [ ] Create a merge request targeting `master` on `gitlab.com/gitlab-org/security` and use the [Security Release merge request template].
+- [ ] Follow the same [code review process]: Assign to a reviewer, then to a maintainer.
+
+After your merge request has being approved according to our [approval guidelines], you're ready to prepare the backports
+
+## Backports
 
 - [ ] Once the MR is ready to be merged, create MRs targeting the latest 3 stable branches
-    - [ ] At this point, it might be easy to squash the commits from the MR into one
+   * At this point, it might be easy to squash the commits from the MR into one
-    - You can use the script `bin/secpick` instead of the following steps, to help you cherry-picking. See the [secpick documentation]
+   * You can use the script `bin/secpick` instead of the following steps, to help you cherry-picking. See the [secpick documentation]
-    - [ ] Create each MR targeting the stable branch `X-Y-stable`, using the "Security Release" merge request template.
+- [ ] Create each MR targeting the stable branch `X-Y-stable`, using the [Security Release merge request template].
-    - Every merge request will have its own set of TODOs, so make sure to
+   * Every merge request will have its own set of TODOs, so make sure to complete those.
-      complete those.
+- [ ] On the "Related merge requests" section, ensure all MRs are linked to this issue.
-- [ ] Make sure all MRs have a link in the [links section](#links)
+   * This section should only list the merge requests created for this issue: One targeting `master` and the 3 backports.
 
-[secpick documentation]: https://gitlab.com/gitlab-org/release/docs/blob/master/general/security/developer.md#secpick-script
+## Documentation and final details
 
-#### Documentation and final details
+- [ ] Ensure the [Links section](#links) is completed.
-
-- [ ] Check the topic on #releases to see when the next release is going to happen and add a link to the [links section](#links)
-- [ ] Add links to this issue and your MRs in the description of the security release issue
 - [ ] Find out the versions affected (the Git history of the files affected may help you with this) and add them to the [details section](#details)
 - [ ] Fill in any upgrade notes that users may need to take into account in the [details section](#details)
 - [ ] Add Yes/No and further details if needed to the migration and settings columns in the [details section](#details)
 - [ ] Add the nickname of the external user who found the issue (and/or HackerOne profile) to the Thanks row in the [details section](#details)
 - [ ] Once your `master` MR is merged, comment on the original security issue with a link to that MR indicating the issue is fixed.
 
-### Summary
+## Summary
 
-#### Links
+### Links
 
 | Description | Link |
 | -------- | -------- |
-| Original issue   | #TODO  |
+| Issue on [GitLab](https://gitlab.com/gitlab-org/gitlab/issues) | #TODO  |
-| Security release issue   | #TODO  |
+| Security Release tracking issue | #TODO  |
-| `master` MR | !TODO   |
-| `master` MR (EE) | !TODO   |
-| `Backport X.Y` MR | !TODO   |
-| `Backport X.Y` MR | !TODO   |
-| `Backport X.Y` MR | !TODO   |
-| `Backport X.Y` MR (EE) | !TODO   |
-| `Backport X.Y` MR (EE) | !TODO   |
-| `Backport X.Y` MR (EE) | !TODO   |
 
-#### Details
+### Details
 
 | Description | Details | Further details|
 | -------- | -------- | -------- |
@@ -65,6 +62,10 @@ Set the title to: `Description of the original issue`
 | Thanks | | |
 
 [security process for developers]: https://gitlab.com/gitlab-org/release/docs/blob/master/general/security/developer.md
-[RM list]:  https://about.gitlab.com/release-managers/
+[secpick documentation]: https://gitlab.com/gitlab-org/release/docs/blob/master/general/security/developer.md#secpick-script
+[security Release merge request template]: https://gitlab.com/gitlab-org/security/gitlab/blob/master/.gitlab/merge_request_templates/Security%20Release.md
+[code review process]: https://docs.gitlab.com/ee/development/code_review.html
+[approval guidelines]: https://docs.gitlab.com/ee/development/code_review.html#approval-guidelines
+[issue as related]: https://docs.gitlab.com/ee/user/project/issues/related_issues.html#adding-a-related-issue
 
 /label ~security

.gitlab/merge_request_templates/Security Release.md

@@ -1,35 +1,37 @@
 <!--
 # README first!
-This MR should be created on `dev.gitlab.org`.
+This MR should be created on `gitlab.com/gitlab-org/security/gitlab`.
 
 See [the general developer security release guidelines](https://gitlab.com/gitlab-org/release/docs/blob/master/general/security/developer.md).
 
-This merge request _must not_ close the corresponding security issue _unless_ it
-targets master.
-
-When submitting a merge request for CE, a corresponding EE merge request is
-always required. This makes it easier to merge security merge requests, as
-manually merging CE into EE is no longer required.
-
 -->
+
 ## Related issues
 
-<!-- Mention the issue(s) this MR is related to -->
+<!-- Mention the GitLab Security issue this MR is related to -->
 
 ## Developer checklist
 
-- [ ] Link to the developer security workflow issue on `dev.gitlab.org`
+- [ ] **Make sure this merge request mentions the [GitLab Security] issue it belongs to (i.e. `Related to <issue_id>`).**
-- [ ] MR targets `master`, or `X-Y-stable` for backports
+- [ ] Merge request targets `master`, or `X-Y-stable` for backports.
-- [ ] Milestone is set for the version this MR applies to
+- [ ] Milestone is set for the version this merge request applies to. A closed milestone can be assigned via [quick actions].
-- [ ] Title of this MR is the same as for all backports
+- [ ] Title of this merge request is the same as for all backports.
 - [ ] A [CHANGELOG entry](https://docs.gitlab.com/ee/development/changelog.html) is added without a `merge_request` value, with `type` set to `security`
-- [ ] Add a link to this MR in the `links` section of related issue
+- [ ] Assign to a reviewer and maintainer, per our [Code Review process].
-- [ ] Set up an EE MR (always required for CE merge requests): EE_MR_LINK_HERE
+- [ ] For the MR targeting `master`:
-- [ ] Assign to a reviewer (that is not a release manager)
+  - [ ] Ping appsec team member who created the issue and ask for a non-blocking review with `Please review this MR`.
+  - [ ] Ensure it's approved according to our [Approval Guidelines].
+- [ ] Merge request _must not_ close the corresponding security issue, _unless_ it targets `master`.
 
-## Reviewer checklist
+**Note:** Reviewer/maintainer should not be a Release Manager
 
+## Maintainer checklist
 - [ ] Correct milestone is applied and the title is matching across all backports
 - [ ] Assigned to `@gitlab-release-tools-bot` with passing CI pipelines
 
 /label ~security
+
+[GitLab Security]: https://gitlab.com/gitlab-org/security/gitlab
+[approval guidelines]: https://docs.gitlab.com/ee/development/code_review.html#approval-guidelines
+[Code Review process]: https://docs.gitlab.com/ee/development/code_review.html
+[quick actions]: https://docs.gitlab.com/ee/user/project/quick_actions.html#quick-actions-for-issues-merge-requests-and-epics

CHANGELOG-EE.md

@@ -1,5 +1,9 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
+## 12.6.7
+
+- No changes.
+
 ## 12.6.6
 
 - No changes.

CHANGELOG.md

@@ -2,6 +2,28 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 12.6.8
+
+### Security (16 changes)
+
+- Respect member access level for group shares.
+- Prevent an endless checking loop for two merge requests targeting each other.
+- Update user 2fa when accepting a group invite.
+- Fix for XSS in branch names.
+- Prevent directory traversal through FileUploader.
+- Run project badge images through the asset proxy.
+- Check merge requests read permissions before showing them in the pipeline widget.
+- Update container registry authentication to account for login request when checking permissions.
+- Remove OID filtering during LFS imports.
+- Protect against denial of service using pipeline webhook recursion.
+- Expire account confirmation token.
+- Prevent XSS in admin grafana URL setting.
+- Don't require base_sha in DiffRefsType.
+- Sanitize output by dependency linkers.
+- Recalculate ProjectAuthorizations for all users.
+- Escape special chars in Sentry error header.
+
+
 ## 12.6.7
 
 ### Security (1 change)

GITALY_SERVER_VERSION

@@ -1 +1 @@
-1.77.1
+12.6.8

VERSION

@@ -1 +1 @@
-12.6.7
+12.6.8

app/assets/javascripts/error_tracking/components/error_details.vue

@@ -1,7 +1,7 @@
 <script>
 import { mapActions, mapGetters, mapState } from 'vuex';
 import dateFormat from 'dateformat';
-import { GlFormInput, GlLink, GlLoadingIcon } from '@gitlab/ui';
+import { GlFormInput, GlLink, GlLoadingIcon, GlSprintf } from '@gitlab/ui';
 import { __, sprintf, n__ } from '~/locale';
 import LoadingButton from '~/vue_shared/components/loading_button.vue';
 import Icon from '~/vue_shared/components/icon.vue';
@@ -17,6 +17,7 @@ export default {
     GlFormInput,
     GlLink,
     GlLoadingIcon,
+    GlSprintf,
     TooltipOnTruncate,
     Icon,
     Stacktrace,
@@ -51,16 +52,6 @@ export default {
   computed: {
     ...mapState('details', ['error', 'loading', 'loadingStacktrace', 'stacktraceData']),
     ...mapGetters('details', ['stacktrace']),
-    reported() {
-      return sprintf(
-        __('Reported %{timeAgo} by %{reportedBy}'),
-        {
-          reportedBy: `<strong>${this.error.culprit}</strong>`,
-          timeAgo: this.timeFormatted(this.stacktraceData.date_received),
-        },
-        false,
-      );
-    },
     firstReleaseLink() {
       return `${this.error.external_base_url}/releases/${this.error.first_release_short_version}`;
     },
@@ -120,7 +111,16 @@ export default {
     </div>
     <div v-else-if="showDetails" class="error-details">
       <div class="top-area align-items-center justify-content-between py-3">
-        <span v-if="!loadingStacktrace && stacktrace" v-html="reported"></span>
+        <div v-if="!loadingStacktrace && stacktrace" data-qa-selector="reported_text">
+          <gl-sprintf :message="__('Reported %{timeAgo} by %{reportedBy}')">
+            <template #reportedBy>
+              <strong>{{ error.culprit }}</strong>
+            </template>
+            <template #timeAgo>
+              {{ timeFormatted(stacktraceData.date_received) }}
+            </template>
+          </gl-sprintf>
+        </div>
         <form ref="sentryIssueForm" :action="projectIssuesPath" method="POST">
           <gl-form-input class="hidden" name="issue[title]" :value="issueTitle" />
           <input name="issue[description]" :value="issueDescription" type="hidden" />

app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue

@@ -1,5 +1,6 @@
 <script>
 import { GlLoadingIcon } from '@gitlab/ui';
+import { escape } from 'lodash';
 import simplePoll from '../../../lib/utils/simple_poll';
 import eventHub from '../../event_hub';
 import statusIcon from '../mr_widget_status_icon.vue';
@@ -44,11 +45,10 @@ export default {
     fastForwardMergeText() {
       return sprintf(
         __(
-          `Fast-forward merge is not possible. Rebase the source branch onto %{startTag}${this.mr.targetBranch}%{endTag} to allow this merge request to be merged.`,
+          'Fast-forward merge is not possible. Rebase the source branch onto %{targetBranch} to allow this merge request to be merged.',
         ),
         {
-          startTag: '<span class="label-branch">',
+          targetBranch: `<span class="label-branch">${escape(this.mr.targetBranch)}</span>`,
-          endTag: '</span>',
         },
         false,
       );

app/controllers/concerns/uploads_actions.rb

@@ -9,6 +9,7 @@ module UploadsActions
 
   included do
     prepend_before_action :set_request_format_from_path_extension
+    rescue_from FileUploader::InvalidSecret, with: :render_404
   end
 
   def create

app/graphql/types/diff_refs_type.rb

@@ -8,7 +8,7 @@ module Types
 
     field :head_sha, GraphQL::STRING_TYPE, null: false,
           description: 'SHA of the HEAD at the time the comment was made'
-    field :base_sha, GraphQL::STRING_TYPE, null: false,
+    field :base_sha, GraphQL::STRING_TYPE, null: true,
           description: 'Merge base of the branch the comment was made on'
     field :start_sha, GraphQL::STRING_TYPE, null: false,
           description: 'SHA of the branch being compared against'

app/models/application_setting.rb

@@ -6,6 +6,9 @@ class ApplicationSetting < ApplicationRecord
   include TokenAuthenticatable
   include ChronicDurationAttribute
 
+  GRAFANA_URL_ERROR_MESSAGE = 'Please check your Grafana URL setting in ' \
+    'Admin Area > Settings > Metrics and profiling > Metrics - Grafana'
+
   add_authentication_token_field :runners_registration_token, encrypted: -> { Feature.enabled?(:application_settings_tokens_optional_encryption, default_enabled: true) ? :optional : :required }
   add_authentication_token_field :health_check_access_token
   add_authentication_token_field :static_objects_external_storage_auth_token
@@ -35,6 +38,14 @@ class ApplicationSetting < ApplicationRecord
 
   chronic_duration_attr_writer :archive_builds_in_human_readable, :archive_builds_in_seconds
 
+  validates :grafana_url,
+            system_hook_url: {
+              blocked_message: "is blocked: %{exception_message}. " + GRAFANA_URL_ERROR_MESSAGE
+            },
+            if: :grafana_url_absolute?
+
+  validate :validate_grafana_url
+
   validates :uuid, presence: true
 
   validates :outbound_local_requests_whitelist,
@@ -345,6 +356,19 @@ class ApplicationSetting < ApplicationRecord
   end
   after_commit :expire_performance_bar_allowed_user_ids_cache, if: -> { previous_changes.key?('performance_bar_allowed_group_id') }
 
+  def validate_grafana_url
+    unless parsed_grafana_url
+      self.errors.add(
+        :grafana_url,
+        "must be a valid relative or absolute URL. #{GRAFANA_URL_ERROR_MESSAGE}"
+      )
+    end
+  end
+
+  def grafana_url_absolute?
+    parsed_grafana_url&.absolute?
+  end
+
   def sourcegraph_url_is_com?
     !!(sourcegraph_url =~ /\Ahttps:\/\/(www\.)?sourcegraph\.com/)
   end
@@ -369,6 +393,12 @@ class ApplicationSetting < ApplicationRecord
   def recaptcha_or_login_protection_enabled
     recaptcha_enabled || login_recaptcha_protection_enabled
   end
+
+  private
+
+  def parsed_grafana_url
+    @parsed_grafana_url ||= Gitlab::Utils.parse_url(grafana_url)
+  end
 end
 
 ApplicationSetting.prepend_if_ee('EE::ApplicationSetting')

app/models/badge.rb

@@ -32,7 +32,9 @@ class Badge < ApplicationRecord
   end
 
   def rendered_image_url(project = nil)
+    Gitlab::AssetProxy.proxy_url(
       build_rendered_url(image_url, project)
+    )
   end
 
   private

app/models/group.rb

@@ -497,18 +497,29 @@ class Group < Namespace
 
     group_group_links_query = GroupGroupLink.where(shared_group_id: self_and_ancestors_ids)
     cte = Gitlab::SQL::CTE.new(:group_group_links_cte, group_group_links_query)
+    cte_alias = cte.table.alias(GroupGroupLink.table_name)
 
     link = GroupGroupLink
              .with(cte.to_arel)
+             .select(smallest_value_arel([cte_alias[:group_access], group_member_table[:access_level]],
+                                         'group_access'))
              .from([group_member_table, cte.alias_to(group_group_link_table)])
              .where(group_member_table[:user_id].eq(user.id))
+             .where(group_member_table[:requested_at].eq(nil))
              .where(group_member_table[:source_id].eq(group_group_link_table[:shared_with_group_id]))
+             .where(group_member_table[:source_type].eq('Namespace'))
              .reorder(Arel::Nodes::Descending.new(group_group_link_table[:group_access]))
              .first
 
     link&.group_access
   end
 
+  def smallest_value_arel(args, column_alias)
+    Arel::Nodes::As.new(
+      Arel::Nodes::NamedFunction.new('LEAST', args),
+      Arel::Nodes::SqlLiteral.new(column_alias))
+  end
+
   def self.groups_including_descendants_by(group_ids)
     Gitlab::ObjectHierarchy
       .new(Group.where(id: group_ids))

app/models/members/group_member.rb

@@ -66,6 +66,7 @@ class GroupMember < Member
 
   def after_accept_invite
     notification_service.accept_group_invite(self)
+    update_two_factor_requirement
 
     super
   end

app/presenters/ci/pipeline_presenter.rb

@@ -128,7 +128,11 @@ module Ci
 
     def all_related_merge_requests
       strong_memoize(:all_related_merge_requests) do
-        pipeline.ref ? pipeline.all_merge_requests_by_recency.to_a : []
+        if pipeline.ref && can?(current_user, :read_merge_request, pipeline.project)
+          pipeline.all_merge_requests_by_recency.to_a
+        else
+          []
+        end
       end
     end
   end

app/services/auth/container_registry_authentication_service.rb

@@ -3,12 +3,24 @@
 module Auth
   class ContainerRegistryAuthenticationService < BaseService
     AUDIENCE = 'container_registry'
+    REGISTRY_LOGIN_ABILITIES = [
+      :read_container_image,
+      :create_container_image,
+      :destroy_container_image,
+      :update_container_image,
+      :admin_container_image,
+      :build_read_container_image,
+      :build_create_container_image,
+      :build_destroy_container_image
+    ].freeze
 
     def execute(authentication_abilities:)
       @authentication_abilities = authentication_abilities
 
       return error('UNAVAILABLE', status: 404, message: 'registry not enabled') unless registry.enabled
 
+      return error('DENIED', status: 403, message: 'access forbidden') unless has_registry_ability?
+
       unless scopes.any? || current_user || project
         return error('DENIED', status: 403, message: 'access forbidden')
       end
@@ -197,5 +209,11 @@ module Auth
     def has_authentication_ability?(capability)
       @authentication_abilities.to_a.include?(capability)
     end
+
+    def has_registry_ability?
+      @authentication_abilities.any? do |ability|
+        REGISTRY_LOGIN_ABILITIES.include?(ability)
+      end
+    end
   end
 end

app/services/projects/lfs_pointers/lfs_download_service.rb

@@ -16,17 +16,14 @@ module Projects
         @lfs_download_object = lfs_download_object
       end
 
-      # rubocop: disable CodeReuse/ActiveRecord
       def execute
         return unless project&.lfs_enabled? && lfs_download_object
         return error("LFS file with oid #{lfs_oid} has invalid attributes") unless lfs_download_object.valid?
-        return if LfsObject.exists?(oid: lfs_oid)
 
         wrap_download_errors do
           download_lfs_file!
         end
       end
-      # rubocop: enable CodeReuse/ActiveRecord
 
       private
 
@@ -39,14 +36,24 @@ module Projects
       def download_lfs_file!
         with_tmp_file do |tmp_file|
           download_and_save_file!(tmp_file)
-          project.all_lfs_objects << LfsObject.new(oid: lfs_oid,
+
-                                                   size: lfs_size,
+          project.lfs_objects << find_or_create_lfs_object(tmp_file)
-                                                   file: tmp_file)
 
           success
         end
       end
 
+      def find_or_create_lfs_object(tmp_file)
+        lfs_obj = LfsObject.safe_find_or_create_by!(
+          oid:  lfs_oid,
+          size: lfs_size
+        )
+
+        lfs_obj.update!(file: tmp_file) unless lfs_obj.file.file
+
+        lfs_obj
+      end
+
       def download_and_save_file!(file)
         digester = Digest::SHA256.new
         response = Gitlab::HTTP.get(lfs_sanitized_url, download_headers) do |fragment|

app/services/projects/lfs_pointers/lfs_object_download_list_service.rb

@@ -26,12 +26,12 @@ module Projects
           return []
         end
 
-        # Getting all Lfs pointers already in the database and linking them to the project
+        # Downloading the required information and gathering it inside an
-        linked_oids = LfsLinkService.new(project).execute(lfs_pointers_in_repository.keys)
+        #   LfsDownloadObject for each oid
-        # Retrieving those oids not present in the database which we need to download
+        #
-        missing_oids = lfs_pointers_in_repository.except(*linked_oids)
+        LfsDownloadLinkListService
-        # Downloading the required information and gathering it inside a LfsDownloadObject for each oid
+          .new(project, remote_uri: current_endpoint_uri)
-        LfsDownloadLinkListService.new(project, remote_uri: current_endpoint_uri).execute(missing_oids)
+          .execute(lfs_pointers_in_repository)
       rescue LfsDownloadLinkListService::DownloadLinksError => e
         raise LfsObjectDownloadListError, "The LFS objects download list couldn't be imported. Error: #{e.message}"
       end

app/services/web_hook_service.rb

@@ -11,8 +11,14 @@ class WebHookService
     end
   end
 
+  GITLAB_EVENT_HEADER = 'X-Gitlab-Event'
+
   attr_accessor :hook, :data, :hook_name, :request_options
 
+  def self.hook_to_event(hook_name)
+    hook_name.to_s.singularize.titleize
+  end
+
   def initialize(hook, data, hook_name)
     @hook = hook
     @data = data
@@ -110,7 +116,7 @@ class WebHookService
     @headers ||= begin
       {
         'Content-Type' => 'application/json',
-        'X-Gitlab-Event' => hook_name.singularize.titleize
+        GITLAB_EVENT_HEADER => self.class.hook_to_event(hook_name)
       }.tap do |hash|
         hash['X-Gitlab-Token'] = Gitlab::Utils.remove_line_breaks(hook.token) if hook.token.present?
       end

app/uploaders/file_uploader.rb

@@ -16,6 +16,9 @@ class FileUploader < GitlabUploader
 
   MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}.freeze
   DYNAMIC_PATH_PATTERN = %r{.*(?<secret>\h{32})/(?<identifier>.*)}.freeze
+  VALID_SECRET_PATTERN = %r{\A\h{10,32}\z}.freeze
+
+  InvalidSecret = Class.new(StandardError)
 
   after :remove, :prune_store_dir
 
@@ -153,6 +156,10 @@ class FileUploader < GitlabUploader
 
   def secret
     @secret ||= self.class.generate_secret
+
+    raise InvalidSecret unless @secret =~ VALID_SECRET_PATTERN
+
+    @secret
   end
 
   # return a new uploader with a file copy on another project

app/validators/addressable_url_validator.rb

@@ -23,7 +23,8 @@
 # protect against Server-side Request Forgery (SSRF), or check for the right port.
 #
 # Configuration options:
-# * <tt>message</tt> - A custom error message (default is: "must be a valid URL").
+# * <tt>message</tt> - A custom error message, used when the URL is blank. (default is: "must be a valid URL").
+# * <tt>blocked_message</tt> - A custom error message, used when the URL is blocked. Default: +'is blocked: %{exception_message}'+.
 # * <tt>schemes</tt> - Array of URI schemes. Default: +['http', 'https']+
 # * <tt>allow_localhost</tt> - Allow urls pointing to +localhost+. Default: +true+
 # * <tt>allow_local_network</tt> - Allow urls pointing to private network addresses. Default: +true+
@@ -59,7 +60,8 @@ class AddressableUrlValidator < ActiveModel::EachValidator
   }.freeze
 
   DEFAULT_OPTIONS = BLOCKER_VALIDATE_OPTIONS.merge({
-    message: 'must be a valid URL'
+    message: 'must be a valid URL',
+    blocked_message: 'is blocked: %{exception_message}'
   }).freeze
 
   def initialize(options)
@@ -80,7 +82,7 @@ class AddressableUrlValidator < ActiveModel::EachValidator
 
     Gitlab::UrlBlocker.validate!(value, blocker_args)
   rescue Gitlab::UrlBlocker::BlockedUrlError => e
-    record.errors.add(attribute, "is blocked: #{e.message}")
+    record.errors.add(attribute, options.fetch(:blocked_message) % { exception_message: e.message })
   end
 
   private

app/views/admin/application_settings/_grafana.html.haml

@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-grafana-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: metrics_and_profiling_admin_application_settings_path(anchor: 'js-grafana-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset

app/views/layouts/nav/sidebar/_admin.html.haml

@@ -83,7 +83,7 @@
                 = _('Requests Profiles')
           - if Gitlab::CurrentSettings.current_application_settings.grafana_enabled?
             = nav_link do
-              = link_to Gitlab::CurrentSettings.current_application_settings.grafana_url, target: '_blank', title: _('Metrics Dashboard') do
+              = link_to Gitlab::CurrentSettings.current_application_settings.grafana_url, target: '_blank', title: _('Metrics Dashboard'), rel: 'noopener noreferrer' do
                 %span
                   = _('Metrics Dashboard')
           = render_if_exists 'layouts/nav/ee/admin/new_monitoring_sidebar'

app/views/shared/issuable/form/_branch_chooser.html.haml

@@ -8,7 +8,9 @@
 
 .form-group.row.d-flex.gl-pl-3.gl-pr-3.branch-selector
   .align-self-center
-    %span= s_('From %{source_title} into').html_safe % { source_title: "<code>#{source_title}</code>".html_safe }
+    %span
+      = _('From <code>%{source_title}</code> into').html_safe % { source_title: source_title }
+
     - if issuable.new_record?
       %code= target_title
       &nbsp;

config/initializers/8_devise.rb

@@ -80,8 +80,16 @@ Devise.setup do |config|
   # When allow_unconfirmed_access_for is zero, the user won't be able to sign in without confirming.
   # You can use this to let your user access some features of your application
   # without confirming the account, but blocking it after a certain period
-  # (ie 2 days).
+  # (e.g. 3 days).
-  config.allow_unconfirmed_access_for = 30.days
+  config.allow_unconfirmed_access_for = 3.days
+
+  # A period that the user is allowed to confirm their account before their
+  # token becomes invalid. For example, if set to 1.day, the user can confirm
+  # their account within 1 days after the mail was sent, but on the second day
+  # their account can't be confirmed with the token any more.
+  # Default is nil, meaning there is no restriction on how long a user can take
+  # before confirming their account.
+  config.confirm_within = 1.day
 
   # Defines which key will be used when confirming an account
   # config.confirmation_keys = [ :email ]

db/migrate/20200214085940_clean_grafana_url.rb

@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+class CleanGrafanaUrl < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def up
+    execute(
+      <<-SQL
+      UPDATE
+        application_settings
+      SET
+        grafana_url = default
+      WHERE
+        position('javascript:' IN btrim(application_settings.grafana_url)) = 1
+      SQL
+    )
+  end
+
+  def down
+    # no-op
+  end
+end

db/post_migrate/20200204113224_schedule_recalculate_project_authorizations_second_run.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+class ScheduleRecalculateProjectAuthorizationsSecondRun < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  MIGRATION = 'RecalculateProjectAuthorizationsWithMinMaxUserId'
+  BATCH_SIZE = 2_500
+  DELAY_INTERVAL = 2.minutes.to_i
+
+  disable_ddl_transaction!
+
+  class User < ActiveRecord::Base
+    include ::EachBatch
+
+    self.table_name = 'users'
+  end
+
+  def up
+    say "Scheduling #{MIGRATION} jobs"
+
+    User.each_batch(of: BATCH_SIZE) do |batch, index|
+      delay = index * DELAY_INTERVAL
+      range = batch.pluck('MIN(id)', 'MAX(id)').first
+      BackgroundMigrationWorker.perform_in(delay, MIGRATION, range)
+    end
+  end
+
+  def down
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2020_02_04_113223) do
+ActiveRecord::Schema.define(version: 2020_02_14_085940) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"

doc/administration/monitoring/performance/grafana_configuration.md

@@ -113,8 +113,9 @@ If you have set up Grafana, you can enable a link to access it easily from the s
    and expand "Metrics - Grafana".
 1. Check the "Enable access to Grafana" checkbox.
 1. If Grafana is enabled through Omnibus GitLab and on the same server,
-   leave "Grafana URL" unchanged. In any other case, enter the full URL
+   leave **Grafana URL** unchanged. It should be `/-/grafana`.
-   path of the Grafana instance.
+
+   In any other case, enter the full URL of the Grafana instance.
 1. Click **Save changes**.
 1. The new link will be available in the admin area under **Monitoring > Metrics Dashboard**.
 

doc/api/graphql/reference/gitlab_schema.graphql

@@ -1124,7 +1124,7 @@ type DiffRefs {
   """
   Merge base of the branch the comment was made on
   """
-  baseSha: String!
+  baseSha: String
 
   """
   SHA of the HEAD at the time the comment was made

doc/api/graphql/reference/gitlab_schema.json

@@ -7118,13 +7118,9 @@
 
               ],
               "type": {
-                "kind": "NON_NULL",
-                "name": null,
-                "ofType": {
                 "kind": "SCALAR",
                 "name": "String",
                 "ofType": null
-                }
               },
               "isDeprecated": false,
               "deprecationReason": null

doc/api/graphql/reference/index.md

@@ -195,7 +195,7 @@ The API can be explored interactively using the [GraphiQL IDE](../index.md#graph
 | Name  | Type  | Description |
 | ---   |  ---- | ----------  |
 | `headSha` | String! | SHA of the HEAD at the time the comment was made |
-| `baseSha` | String! | Merge base of the branch the comment was made on |
+| `baseSha` | String | Merge base of the branch the comment was made on |
 | `startSha` | String! | SHA of the branch being compared against |
 
 ### Discussion

lib/api/triggers.rb

@@ -4,6 +4,8 @@ module API
   class Triggers < Grape::API
     include PaginationParams
 
+    HTTP_GITLAB_EVENT_HEADER = "HTTP_#{WebHookService::GITLAB_EVENT_HEADER}".underscore.upcase
+
     params do
       requires :id, type: String, desc: 'The ID of a project'
     end
@@ -19,6 +21,8 @@ module API
       post ":id/(ref/:ref/)trigger/pipeline", requirements: { ref: /.+/ } do
         Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42283')
 
+        forbidden! if gitlab_pipeline_hook_request?
+
         # validate variables
         params[:variables] = params[:variables].to_h
         unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) }
@@ -128,5 +132,11 @@ module API
         destroy_conditionally!(trigger)
       end
     end
+
+    helpers do
+      def gitlab_pipeline_hook_request?
+        request.get_header(HTTP_GITLAB_EVENT_HEADER) == WebHookService.hook_to_event(:pipeline_hooks)
+      end
+    end
   end
 end

lib/gitlab/asset_proxy.rb

@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+# This is based on https://github.com/jch/html-pipeline/blob/v2.12.2/lib/html/pipeline/camo_filter.rb
+# and Banzai::Filter::AssetProxyFilter which we use to proxy images in Markdown
+
+module Gitlab
+  module AssetProxy
+    class << self
+      def proxy_url(url)
+        return url unless Gitlab.config.asset_proxy.enabled
+        return url if asset_host_whitelisted?(url)
+
+        "#{Gitlab.config.asset_proxy.url}/#{asset_url_hash(url)}/#{hexencode(url)}"
+      end
+
+      private
+
+      def asset_host_whitelisted?(url)
+        parsed_url = URI.parse(url)
+
+        Gitlab.config.asset_proxy.domain_regexp&.match?(parsed_url.host)
+      end
+
+      def asset_url_hash(url)
+        OpenSSL::HMAC.hexdigest('sha1', Gitlab.config.asset_proxy.secret_key, url)
+      end
+
+      def hexencode(str)
+        str.unpack1('H*')
+      end
+    end
+  end
+end

lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id.rb

@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # rubocop:disable Style/Documentation
+    class RecalculateProjectAuthorizationsWithMinMaxUserId
+      def perform(min_user_id, max_user_id)
+        User.where(id: min_user_id..max_user_id).find_each do |user|
+          service = Users::RefreshAuthorizedProjectsService.new(
+            user,
+            incorrect_auth_found_callback:
+              ->(project_id, access_level) do
+                logger.info(message: 'Removing ProjectAuthorizations',
+                            user_id: user.id,
+                            project_id: project_id,
+                            access_level: access_level)
+              end,
+            missing_auth_found_callback:
+              ->(project_id, access_level) do
+                logger.info(message: 'Creating ProjectAuthorizations',
+                            user_id: user.id,
+                            project_id: project_id,
+                            access_level: access_level)
+              end
+          )
+
+          service.execute
+        end
+      end
+
+      private
+
+      def logger
+        @logger ||= Gitlab::BackgroundMigration::Logger.build
+      end
+    end
+  end
+end

lib/gitlab/dependency_linker/base_linker.rb

@@ -7,6 +7,8 @@ module Gitlab
       GIT_INVALID_URL_REGEX = /^git\+#{URL_REGEX}/.freeze
       REPO_REGEX = %r{[^/'" ]+/[^/'" ]+}.freeze
 
+      include ActionView::Helpers::SanitizeHelper
+
       class_attribute :file_type
 
       def self.support?(blob_name)
@@ -62,7 +64,10 @@ module Gitlab
       end
 
       def link_tag(name, url)
-        %{<a href="#{ERB::Util.html_escape_once(url)}" rel="nofollow noreferrer noopener" target="_blank">#{ERB::Util.html_escape_once(name)}</a>}.html_safe
+        sanitize(
+          %{<a href="#{ERB::Util.html_escape_once(url)}" rel="nofollow noreferrer noopener" target="_blank">#{ERB::Util.html_escape_once(name)}</a>},
+          attributes: %w[href rel target]
+        )
       end
 
       # Links package names based on regex.

lib/gitlab/project_authorizations.rb

@@ -62,13 +62,18 @@ module Gitlab
       cte = Gitlab::SQL::RecursiveCTE.new(:namespaces_cte)
       members = Member.arel_table
       namespaces = Namespace.arel_table
+      group_group_links = GroupGroupLink.arel_table
 
       # Namespaces the user is a member of.
       cte << user.groups
         .select([namespaces[:id], members[:access_level]])
         .except(:order)
 
-      cte << Group.select([namespaces[:id], 'group_group_links.group_access AS access_level'])
+      # Namespaces shared with any of the group
+      cte << Group.select([namespaces[:id],
+                           least(members[:access_level],
+                                 group_group_links[:group_access],
+                                 'access_level')])
                   .joins(join_group_group_links)
                   .joins(join_members_on_group_group_links)
 

lib/gitlab/user_access.rb

@@ -67,7 +67,13 @@ module Gitlab
       return false unless can_access_git?
       return false unless project
 
+      # Checking for an internal project to prevent an infinite loop:
+      # https://gitlab.com/gitlab-org/gitlab/issues/36805
+      if project.internal?
+        return false unless user.can?(:push_code, project)
+      else
         return false if !user.can?(:push_code, project) && !project.branch_allows_collaboration?(user, ref)
+      end
 
       if protected?(ProtectedBranch, project, ref)
         protected_branch_accessible_to?(ref, action: :push)

lib/gitlab/utils.rb

@@ -130,5 +130,14 @@ module Gitlab
       IPAddr.new(str)
     rescue IPAddr::InvalidAddressError
     end
+
+    # Converts a string to an Addressable::URI object.
+    # If the string is not a valid URI, it returns nil.
+    # Param uri_string should be a String object.
+    # This method returns an Addressable::URI object or nil.
+    def parse_url(uri_string)
+      Addressable::URI.parse(uri_string)
+    rescue Addressable::URI::InvalidURIError, TypeError
+    end
   end
 end

locale/gitlab.pot

@@ -7562,6 +7562,9 @@ msgstr ""
 msgid "Failure"
 msgstr ""
 
+msgid "Fast-forward merge is not possible. Rebase the source branch onto %{targetBranch} to allow this merge request to be merged."
+msgstr ""
+
 msgid "Fast-forward merge is not possible. Rebase the source branch onto the target branch or merge target branch into source branch to allow this merge request to be merged."
 msgstr ""
 
@@ -7987,7 +7990,7 @@ msgstr ""
 msgid "From %{providerTitle}"
 msgstr ""
 
-msgid "From %{source_title} into"
+msgid "From <code>%{source_title}</code> into"
 msgstr ""
 
 msgid "From Bitbucket"

spec/controllers/snippets_controller_spec.rb

@@ -242,8 +242,10 @@ describe SnippetsController do
     context 'when the snippet description contains a file' do
       include FileMoverHelpers
 
-      let(:picture_file) { "/-/system/user/#{user.id}/secret56/picture.jpg" }
+      let(:picture_secret) { SecureRandom.hex }
-      let(:text_file) { "/-/system/user/#{user.id}/secret78/text.txt" }
+      let(:text_secret) { SecureRandom.hex }
+      let(:picture_file) { "/-/system/user/#{user.id}/#{picture_secret}/picture.jpg" }
+      let(:text_file) { "/-/system/user/#{user.id}/#{text_secret}/text.txt" }
       let(:description) do
         "Description with picture: ![picture](/uploads#{picture_file}) and "\
         "text: [text.txt](/uploads#{text_file})"
@@ -266,8 +268,8 @@ describe SnippetsController do
         snippet = subject
 
         expected_description = "Description with picture: "\
-          "![picture](/uploads/-/system/personal_snippet/#{snippet.id}/secret56/picture.jpg) and "\
+          "![picture](/uploads/-/system/personal_snippet/#{snippet.id}/#{picture_secret}/picture.jpg) and "\
-          "text: [text.txt](/uploads/-/system/personal_snippet/#{snippet.id}/secret78/text.txt)"
+          "text: [text.txt](/uploads/-/system/personal_snippet/#{snippet.id}/#{text_secret}/text.txt)"
 
         expect(snippet.description).to eq(expected_description)
       end

spec/features/merge_request/user_creates_merge_request_spec.rb

@@ -5,9 +5,9 @@ require "spec_helper"
 describe "User creates a merge request", :js do
   include ProjectForksHelper
 
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:user) { create(:user) }
   let(:title) { "Some feature" }
-  let(:project) { create(:project, :repository) }
-  let(:user) { create(:user) }
 
   before do
     project.add_maintainer(user)
@@ -38,6 +38,26 @@ describe "User creates a merge request", :js do
     end
   end
 
+  context "XSS branch name exists" do
+    before do
+      project.repository.create_branch("<img/src='x'/onerror=alert('oops')>", "master")
+    end
+
+    it "doesn't execute the dodgy branch name" do
+      visit(project_new_merge_request_path(project))
+
+      find(".js-source-branch").click
+      click_link("<img/src='x'/onerror=alert('oops')>")
+
+      find(".js-target-branch").click
+      click_link("feature")
+
+      click_button("Compare branches")
+
+      expect { page.driver.browser.switch_to.alert }.to raise_error(Selenium::WebDriver::Error::NoSuchAlertError)
+    end
+  end
+
   context "to a forked project" do
     let(:forked_project) { fork_project(project, user, namespace: user.namespace, repository: true) }
 

spec/frontend/error_tracking/components/error_details_spec.js

@@ -1,6 +1,6 @@
 import { createLocalVue, shallowMount } from '@vue/test-utils';
 import Vuex from 'vuex';
-import { GlLoadingIcon, GlLink } from '@gitlab/ui';
+import { GlLoadingIcon, GlLink, GlSprintf } from '@gitlab/ui';
 import LoadingButton from '~/vue_shared/components/loading_button.vue';
 import Stacktrace from '~/error_tracking/components/stacktrace.vue';
 import ErrorDetails from '~/error_tracking/components/error_details.vue';
@@ -16,7 +16,7 @@ describe('ErrorDetails', () => {
 
   function mountComponent() {
     wrapper = shallowMount(ErrorDetails, {
-      stubs: { LoadingButton },
+      stubs: { LoadingButton, GlSprintf },
       localVue,
       store,
       propsData: {
@@ -188,5 +188,28 @@ describe('ErrorDetails', () => {
         });
       });
     });
+
+    describe('Escape unsafe chars for culprit field', () => {
+      const findReportedText = () => wrapper.find('[data-qa-selector="reported_text"]');
+      const culprit = '<script>console.log("surprise!")</script>';
+      beforeEach(() => {
+        store.state.details.loadingStacktrace = false;
+        store.state.details.loading = false;
+        store.state.details.error = {
+          id: 1,
+          culprit,
+        };
+        mountComponent();
+      });
+
+      it('should not convert interpolated text to html entities', () => {
+        expect(findReportedText().findAll('script').length).toEqual(0);
+        expect(findReportedText().findAll('strong').length).toEqual(1);
+      });
+
+      it('should render text instead of converting to html entities', () => {
+        expect(findReportedText().text()).toContain(culprit);
+      });
+    });
   });
 });

spec/graphql/types/diff_refs_type_spec.rb

@@ -5,5 +5,9 @@ require 'spec_helper'
 describe GitlabSchema.types['DiffRefs'] do
   it { expect(described_class.graphql_name).to eq('DiffRefs') }
 
-  it { expect(described_class).to have_graphql_fields(:base_sha, :head_sha, :start_sha) }
+  it { is_expected.to have_graphql_fields(:head_sha, :base_sha, :start_sha) }
+
+  it { expect(described_class.fields['headSha'].type).to be_non_null }
+  it { expect(described_class.fields['baseSha'].type).not_to be_non_null }
+  it { expect(described_class.fields['startSha'].type).to be_non_null }
 end

spec/lib/gitlab/asset_proxy_spec.rb

@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::AssetProxy do
+  context 'when asset proxy is disabled' do
+    before do
+      stub_asset_proxy_setting(enabled: false)
+    end
+
+    it 'returns the original URL' do
+      url = 'http://example.com/test.png'
+
+      expect(described_class.proxy_url(url)).to eq(url)
+    end
+  end
+
+  context 'when asset proxy is enabled' do
+    before do
+      stub_asset_proxy_setting(whitelist: %w(gitlab.com *.mydomain.com))
+      stub_asset_proxy_setting(
+        enabled: true,
+        url: 'https://assets.example.com',
+        secret_key: 'shared-secret',
+        domain_regexp: Banzai::Filter::AssetProxyFilter.compile_whitelist(Gitlab.config.asset_proxy.whitelist)
+      )
+    end
+
+    it 'returns a proxied URL' do
+      url = 'http://example.com/test.png'
+      proxied_url = 'https://assets.example.com/08df250eeeef1a8cf2c761475ac74c5065105612/687474703a2f2f6578616d706c652e636f6d2f746573742e706e67'
+
+      expect(described_class.proxy_url(url)).to eq(proxied_url)
+    end
+
+    context 'whitelisted domain' do
+      it 'returns original URL for single domain whitelist' do
+        url = 'http://gitlab.com/test.png'
+
+        expect(described_class.proxy_url(url)).to eq(url)
+      end
+
+      it 'returns original URL for wildcard subdomain whitelist' do
+        url = 'http://test.mydomain.com/test.png'
+
+        expect(described_class.proxy_url(url)).to eq(url)
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id_spec.rb

@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::RecalculateProjectAuthorizationsWithMinMaxUserId, :migration, schema: 20200204113224 do
+  let(:users_table) { table(:users) }
+  let(:min) { 1 }
+  let(:max) { 5 }
+
+  before do
+    min.upto(max) do |i|
+      users_table.create!(id: i, email: "user#{i}@example.com", projects_limit: 10)
+    end
+  end
+
+  describe '#perform' do
+    it 'initializes Users::RefreshAuthorizedProjectsService with correct users' do
+      min.upto(max) do |i|
+        user = User.find(i)
+        expect(Users::RefreshAuthorizedProjectsService).to(
+          receive(:new).with(user, any_args).and_call_original)
+      end
+
+      described_class.new.perform(min, max)
+    end
+
+    it 'executes Users::RefreshAuthorizedProjectsService' do
+      expected_call_counts = max - min + 1
+
+      service = instance_double(Users::RefreshAuthorizedProjectsService)
+      expect(Users::RefreshAuthorizedProjectsService).to(
+        receive(:new).exactly(expected_call_counts).times.and_return(service))
+      expect(service).to receive(:execute).exactly(expected_call_counts).times
+
+      described_class.new.perform(min, max)
+    end
+  end
+end

spec/lib/gitlab/dependency_linker/base_linker_spec.rb

@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::DependencyLinker::BaseLinker do
+  let(:linker_class) do
+    Class.new(described_class) do
+      def link_dependencies
+        link_regex(%r{^(?<name>https?://[^ ]+)}, &:itself)
+      end
+    end
+  end
+
+  let(:plain_content) do
+    <<~CONTENT
+      http://\\njavascript:alert(1)
+      https://gitlab.com/gitlab-org/gitlab
+    CONTENT
+  end
+
+  let(:highlighted_content) do
+    <<~CONTENT
+      <span><span>http://</span><span>\\n</span><span>javascript:alert(1)</span></span>
+      <span><span>https://gitlab.com/gitlab-org/gitlab</span></span>
+    CONTENT
+  end
+
+  let(:linker) { linker_class.new(plain_content, highlighted_content) }
+
+  describe '#link' do
+    subject { linker.link }
+
+    it 'only converts valid links' do
+      expect(subject).to eq(
+        <<~CONTENT
+          <span><span>#{link('http://')}</span><span>#{link('\n', url: '%5Cn')}</span><span>#{link('javascript:alert(1)', url: nil)}</span></span>
+          <span><span>#{link('https://gitlab.com/gitlab-org/gitlab')}</span></span>
+        CONTENT
+      )
+    end
+  end
+
+  def link(text, url: text)
+    attrs = [
+      'rel="nofollow noreferrer noopener"',
+      'target="_blank"'
+    ]
+
+    attrs.unshift(%{href="#{url}"}) if url
+
+    %{<a #{attrs.join(' ')}>#{text}</a>}
+  end
+end

spec/lib/gitlab/project_authorizations_spec.rb

@@ -109,6 +109,20 @@ describe Gitlab::ProjectAuthorizations do
       end
     end
 
+    context 'with lower group access level than max access level for share' do
+      let(:user) { create(:user) }
+
+      it 'creates proper authorizations' do
+        group.add_reporter(user)
+
+        mapping = map_access_levels(authorizations)
+
+        expect(mapping[project_parent.id]).to be_nil
+        expect(mapping[project.id]).to eq(Gitlab::Access::REPORTER)
+        expect(mapping[project_child.id]).to eq(Gitlab::Access::REPORTER)
+      end
+    end
+
     context 'parent group user' do
       let(:user) { parent_group_user }
 

spec/lib/gitlab/user_access_spec.rb

@@ -30,6 +30,17 @@ describe Gitlab::UserAccess do
       end
     end
 
+    describe 'push to branch in an internal project' do
+      it 'will not infinitely loop when a project is internal' do
+        project.visibility_level = Gitlab::VisibilityLevel::INTERNAL
+        project.save!
+
+        expect(project).not_to receive(:branch_allows_collaboration?)
+
+        access.can_push_to_branch?('master')
+      end
+    end
+
     describe 'push to empty project' do
       let(:empty_project) { create(:project_empty_repo) }
       let(:project_access) { described_class.new(user, project: empty_project) }

spec/lib/gitlab/utils_spec.rb

@@ -252,4 +252,18 @@ describe Gitlab::Utils do
       expect(described_class.string_to_ip_object('1:0:0:0:0:0:0:0/124')).to eq(IPAddr.new('1:0:0:0:0:0:0:0/124'))
     end
   end
+
+  describe '.parse_url' do
+    it 'returns Addressable::URI object' do
+      expect(described_class.parse_url('http://gitlab.com')).to be_instance_of(Addressable::URI)
+    end
+
+    it 'returns nil when URI cannot be parsed' do
+      expect(described_class.parse_url('://gitlab.com')).to be nil
+    end
+
+    it 'returns nil with invalid parameter' do
+      expect(described_class.parse_url(1)).to be nil
+    end
+  end
 end

spec/migrations/clean_grafana_url_spec.rb

@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20200214085940_clean_grafana_url.rb')
+
+describe CleanGrafanaUrl, :migration do
+  let(:application_settings_table) { table(:application_settings) }
+
+  [
+    'javascript:alert(window.opener.document.location)',
+    '  javascript:alert(window.opener.document.location)'
+  ].each do |grafana_url|
+    it "sets grafana_url back to its default value when grafana_url is '#{grafana_url}'" do
+      application_settings = application_settings_table.create!(grafana_url: grafana_url)
+
+      migrate!
+
+      expect(application_settings.reload.grafana_url).to eq('/-/grafana')
+    end
+  end
+
+  ['/-/grafana', '/some/relative/url', 'http://localhost:9000'].each do |grafana_url|
+    it "does not modify grafana_url when grafana_url is '#{grafana_url}'" do
+      application_settings = application_settings_table.create!(grafana_url: grafana_url)
+
+      migrate!
+
+      expect(application_settings.reload.grafana_url).to eq(grafana_url)
+    end
+  end
+
+  context 'when application_settings table has no rows' do
+    it 'does not fail' do
+      migrate!
+    end
+  end
+end

spec/migrations/schedule_recalculate_project_authorizations_second_run_spec.rb

@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200204113224_schedule_recalculate_project_authorizations_second_run.rb')
+
+describe ScheduleRecalculateProjectAuthorizationsSecondRun, :migration, :sidekiq do
+  let(:users_table) { table(:users) }
+
+  before do
+    stub_const("#{described_class}::BATCH_SIZE", 2)
+
+    1.upto(4) do |i|
+      users_table.create!(id: i, name: "user#{i}", email: "user#{i}@example.com", projects_limit: 1)
+    end
+  end
+
+  it 'schedules background migration' do
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+        expect(described_class::MIGRATION).to be_scheduled_migration(1, 2)
+        expect(described_class::MIGRATION).to be_scheduled_migration(3, 4)
+      end
+    end
+  end
+end

spec/models/application_setting_spec.rb

@@ -19,6 +19,7 @@ describe ApplicationSetting do
     let(:http)  { 'http://example.com' }
     let(:https) { 'https://example.com' }
     let(:ftp)   { 'ftp://example.com' }
+    let(:javascript) { 'javascript:alert(window.opener.document.location)' }
 
     it { is_expected.to allow_value(nil).for(:home_page_url) }
     it { is_expected.to allow_value(http).for(:home_page_url) }
@@ -74,6 +75,53 @@ describe ApplicationSetting do
     it { is_expected.not_to allow_value('abc').for(:minimum_password_length) }
     it { is_expected.to allow_value(10).for(:minimum_password_length) }
 
+    context 'grafana_url validations' do
+      before do
+        subject.instance_variable_set(:@parsed_grafana_url, nil)
+      end
+
+      it { is_expected.to allow_value(http).for(:grafana_url) }
+      it { is_expected.to allow_value(https).for(:grafana_url) }
+      it { is_expected.not_to allow_value(ftp).for(:grafana_url) }
+      it { is_expected.not_to allow_value(javascript).for(:grafana_url) }
+      it { is_expected.to allow_value('/-/grafana').for(:grafana_url) }
+      it { is_expected.to allow_value('http://localhost:9000').for(:grafana_url) }
+
+      context 'when local URLs are not allowed in system hooks' do
+        before do
+          stub_application_setting(allow_local_requests_from_system_hooks: false)
+        end
+
+        it { is_expected.not_to allow_value('http://localhost:9000').for(:grafana_url) }
+      end
+
+      context 'with invalid grafana URL' do
+        it 'adds an error' do
+          subject.grafana_url = ' ' + http
+          expect(subject.save).to be false
+
+          expect(subject.errors[:grafana_url]).to eq([
+            'must be a valid relative or absolute URL. ' \
+            'Please check your Grafana URL setting in ' \
+            'Admin Area > Settings > Metrics and profiling > Metrics - Grafana'
+          ])
+        end
+      end
+
+      context 'with blocked grafana URL' do
+        it 'adds an error' do
+          subject.grafana_url = javascript
+          expect(subject.save).to be false
+
+          expect(subject.errors[:grafana_url]).to eq([
+            'is blocked: Only allowed schemes are http, https. Please check your ' \
+            'Grafana URL setting in ' \
+            'Admin Area > Settings > Metrics and profiling > Metrics - Grafana'
+          ])
+        end
+      end
+    end
+
     context 'when snowplow is enabled' do
       before do
         setting.snowplow_enabled = true

spec/models/badge_spec.rb

@@ -91,6 +91,22 @@ describe Badge do
       let(:method) { :image_url }
 
       it_behaves_like 'rendered_links'
+
+      context 'when asset proxy is enabled' do
+        let(:placeholder_url) { 'http://www.example.com/image' }
+
+        before do
+          stub_asset_proxy_setting(
+            enabled: true,
+            url: 'https://assets.example.com',
+            secret_key: 'shared-secret'
+          )
+        end
+
+        it 'returns a proxied URL' do
+          expect(badge.rendered_image_url).to start_with('https://assets.example.com')
+        end
+      end
     end
   end
 end

spec/models/group_spec.rb

@@ -562,6 +562,18 @@ describe Group do
             expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER)
             expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER)
           end
+
+          context 'with lower group access level than max access level for share' do
+            let(:user) { create(:user) }
+
+            it 'returns correct access level' do
+              group.add_reporter(user)
+
+              expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+              expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER)
+              expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER)
+            end
+          end
         end
 
         context 'with user in the parent group' do
@@ -583,6 +595,33 @@ describe Group do
             expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
           end
         end
+
+        context 'unrelated project owner' do
+          let(:common_id) { [Project.maximum(:id).to_i, Namespace.maximum(:id).to_i].max + 999 }
+          let!(:group) { create(:group, id: common_id) }
+          let!(:unrelated_project) { create(:project, id: common_id) }
+          let(:user) { unrelated_project.owner }
+
+          it 'returns correct access level' do
+            expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+          end
+        end
+
+        context 'user without accepted access request' do
+          let!(:user) { create(:user) }
+
+          before do
+            create(:group_member, :developer, :access_request, user: user, group: group)
+          end
+
+          it 'returns correct access level' do
+            expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+          end
+        end
       end
 
       context 'when feature flag share_group_with_group is disabled' do

spec/models/members/group_member_spec.rb

@@ -65,10 +65,10 @@ describe GroupMember do
   end
 
   describe '#update_two_factor_requirement' do
-    let(:user) { build :user }
-    let(:group_member) { build :group_member, user: user }
-
     it 'is called after creation and deletion' do
+      user = build :user
+      group_member = build :group_member, user: user
+
       expect(user).to receive(:update_two_factor_requirement)
 
       group_member.save
@@ -79,6 +79,21 @@ describe GroupMember do
     end
   end
 
+  describe '#after_accept_invite' do
+    it 'calls #update_two_factor_requirement' do
+      email = 'foo@email.com'
+      user = build(:user, email: email)
+      group = create(:group, require_two_factor_authentication: true)
+      group_member = create(:group_member, group: group, invite_token: '1234', invite_email: email)
+
+      expect(user).to receive(:require_two_factor_authentication_from_group).and_call_original
+
+      group_member.accept_invite!(user)
+
+      expect(user.require_two_factor_authentication_from_group).to be_truthy
+    end
+  end
+
   context 'access levels' do
     context 'with parent group' do
       it_behaves_like 'inherited access level as a member of entity' do

spec/models/project_spec.rb

@@ -4766,6 +4766,38 @@ describe Project do
     end
   end
 
+  context 'with cross internal project merge requests' do
+    let(:project) { create(:project, :repository, :internal) }
+    let(:forked_project) { fork_project(project, nil, repository: true) }
+    let(:user) { double(:user) }
+
+    it "does not endlessly loop for internal projects with MRs to each other", :sidekiq_inline do
+      allow(user).to receive(:can?).and_return(true, false, true)
+      allow(user).to receive(:id).and_return(1)
+
+      create(
+        :merge_request,
+        target_project: project,
+        target_branch: 'merge-test',
+        source_project: forked_project,
+        source_branch: 'merge-test',
+        allow_collaboration: true
+      )
+
+      create(
+        :merge_request,
+        target_project: forked_project,
+        target_branch: 'merge-test',
+        source_project: project,
+        source_branch: 'merge-test',
+        allow_collaboration: true
+      )
+
+      expect(user).to receive(:can?).at_most(5).times
+      project.branch_allows_collaboration?(user, "merge-test")
+    end
+  end
+
   context 'with cross project merge requests' do
     let(:user) { create(:user) }
     let(:target_project) { create(:project, :repository) }

spec/presenters/ci/pipeline_presenter_spec.rb

@@ -6,6 +6,7 @@ describe Ci::PipelinePresenter do
   include Gitlab::Routing
 
   let(:user) { create(:user) }
+  let(:current_user) { user }
   let(:project) { create(:project) }
   let(:pipeline) { create(:ci_pipeline, project: project) }
 
@@ -15,7 +16,7 @@ describe Ci::PipelinePresenter do
 
   before do
     project.add_developer(user)
-    allow(presenter).to receive(:current_user) { user }
+    allow(presenter).to receive(:current_user) { current_user }
   end
 
   it 'inherits from Gitlab::View::Presenter::Delegated' do
@@ -215,10 +216,90 @@ describe Ci::PipelinePresenter do
   describe '#all_related_merge_requests' do
     it 'memoizes the returned relation' do
       query_count = ActiveRecord::QueryRecorder.new do
-        2.times { presenter.send(:all_related_merge_requests).count }
+        3.times { presenter.send(:all_related_merge_requests).count }
       end.count
 
-      expect(query_count).to eq(1)
+      expect(query_count).to eq(2)
+    end
+
+    context 'permissions' do
+      let!(:merge_request) do
+        create(:merge_request, project: project, source_project: project)
+      end
+
+      subject(:all_related_merge_requests) do
+        presenter.send(:all_related_merge_requests)
+      end
+
+      shared_examples 'private merge requests' do
+        context 'when not logged in' do
+          let(:current_user) {}
+
+          it { is_expected.to be_empty }
+        end
+
+        context 'when logged in as a non_member' do
+          let(:current_user) { create(:user) }
+
+          it { is_expected.to be_empty }
+        end
+
+        context 'when logged in as a guest' do
+          let(:current_user) { create(:user) }
+
+          before do
+            project.add_guest(current_user)
+          end
+
+          it { is_expected.to be_empty }
+        end
+
+        context 'when logged in as a developer' do
+          it { is_expected.to contain_exactly(merge_request) }
+        end
+
+        context 'when logged in as a maintainer' do
+          let(:current_user) { create(:user) }
+
+          before do
+            project.add_maintainer(current_user)
+          end
+
+          it { is_expected.to contain_exactly(merge_request) }
+        end
+      end
+
+      context 'with a private project' do
+        it_behaves_like 'private merge requests'
+      end
+
+      context 'with a public project with private merge requests' do
+        before do
+          project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+
+          project
+            .project_feature
+            .update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+        end
+
+        it_behaves_like 'private merge requests'
+      end
+
+      context 'with a public project with public merge requests' do
+        before do
+          project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+
+          project
+            .project_feature
+            .update!(merge_requests_access_level: ProjectFeature::ENABLED)
+        end
+
+        context 'when not logged in' do
+          let(:current_user) {}
+
+          it { is_expected.to contain_exactly(merge_request) }
+        end
+      end
     end
   end
 

spec/requests/api/triggers_spec.rb

@@ -132,6 +132,18 @@ describe API::Triggers do
         end
       end
     end
+
+    context 'when is triggered by a pipeline hook' do
+      it 'does not create a new pipeline' do
+        expect do
+          post api("/projects/#{project.id}/ref/master/trigger/pipeline?token=#{trigger_token}"),
+            params: { ref: 'refs/heads/other-branch' },
+            headers: { WebHookService::GITLAB_EVENT_HEADER => 'Pipeline Hook' }
+        end.not_to change(Ci::Pipeline, :count)
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+    end
   end
 
   describe 'GET /projects/:id/triggers' do

spec/services/auth/container_registry_authentication_service_spec.rb

@@ -769,6 +769,15 @@ describe Auth::ContainerRegistryAuthenticationService do
     context 'when deploy token has read_registry as a scope' do
       let(:current_user) { create(:deploy_token, projects: [project]) }
 
+      shared_examples 'able to login' do
+        context 'registry provides read_container_image authentication_abilities' do
+          let(:current_params) { {} }
+          let(:authentication_abilities) { [:read_container_image] }
+
+          it_behaves_like 'an authenticated'
+        end
+      end
+
       context 'for public project' do
         let(:project) { create(:project, :public) }
 
@@ -783,6 +792,8 @@ describe Auth::ContainerRegistryAuthenticationService do
 
           it_behaves_like 'an inaccessible'
         end
+
+        it_behaves_like 'able to login'
       end
 
       context 'for internal project' do
@@ -799,6 +810,8 @@ describe Auth::ContainerRegistryAuthenticationService do
 
           it_behaves_like 'an inaccessible'
         end
+
+        it_behaves_like 'able to login'
       end
 
       context 'for private project' do
@@ -815,18 +828,38 @@ describe Auth::ContainerRegistryAuthenticationService do
 
           it_behaves_like 'an inaccessible'
         end
+
+        it_behaves_like 'able to login'
       end
     end
 
     context 'when deploy token does not have read_registry scope' do
       let(:current_user) { create(:deploy_token, projects: [project], read_registry: false) }
 
+      shared_examples 'unable to login' do
+        context 'registry provides no container authentication_abilities' do
+          let(:current_params) { {} }
+          let(:authentication_abilities) { [] }
+
+          it_behaves_like 'a forbidden'
+        end
+
+        context 'registry provides inapplicable container authentication_abilities' do
+          let(:current_params) { {} }
+          let(:authentication_abilities) { [:download_code] }
+
+          it_behaves_like 'a forbidden'
+        end
+      end
+
       context 'for public project' do
         let(:project) { create(:project, :public) }
 
         context 'when pulling' do
           it_behaves_like 'a pullable'
         end
+
+        it_behaves_like 'unable to login'
       end
 
       context 'for internal project' do
@@ -835,6 +868,8 @@ describe Auth::ContainerRegistryAuthenticationService do
         context 'when pulling' do
           it_behaves_like 'an inaccessible'
         end
+
+        it_behaves_like 'unable to login'
       end
 
       context 'for private project' do
@@ -843,6 +878,15 @@ describe Auth::ContainerRegistryAuthenticationService do
         context 'when pulling' do
           it_behaves_like 'an inaccessible'
         end
+
+        context 'when logging in' do
+          let(:current_params) { {} }
+          let(:authentication_abilities) { [] }
+
+          it_behaves_like 'a forbidden'
+        end
+
+        it_behaves_like 'unable to login'
       end
     end
 

spec/services/projects/lfs_pointers/lfs_download_service_spec.rb

@@ -133,6 +133,21 @@ describe Projects::LfsPointers::LfsDownloadService do
       end
     end
 
+    context 'when an lfs object with the same oid already exists' do
+      let!(:existing_lfs_object) { create(:lfs_object, oid: oid) }
+
+      before do
+        stub_full_request(download_link).to_return(body: lfs_content)
+      end
+
+      it_behaves_like 'no lfs object is created'
+
+      it 'does not update the file attached to the existing LfsObject' do
+        expect { subject.execute }
+          .not_to change { existing_lfs_object.reload.file.file.file }
+      end
+    end
+
     context 'when credentials present' do
       let(:download_link_with_credentials) { "http://user:password@gitlab.com/#{oid}" }
       let(:lfs_object) { LfsDownloadObject.new(oid: oid, size: size, link: download_link_with_credentials) }
@@ -210,17 +225,5 @@ describe Projects::LfsPointers::LfsDownloadService do
         subject.execute
       end
     end
-
-    context 'when an lfs object with the same oid already exists' do
-      before do
-        create(:lfs_object, oid: oid)
-      end
-
-      it 'does not download the file' do
-        expect(subject).not_to receive(:download_lfs_file!)
-
-        subject.execute
-      end
-    end
   end
 end

spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb

@@ -24,7 +24,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do
   describe '#execute' do
     context 'when no lfs pointer is linked' do
       before do
-        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return([])
         allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links)
         expect(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:new).with(project, remote_uri: URI.parse(default_endpoint)).and_call_original
       end
@@ -35,12 +34,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do
         subject.execute
       end
 
-      it 'links existent lfs objects to the project' do
-        expect_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute)
-
-        subject.execute
-      end
-
       it 'retrieves the download links of non existent objects' do
         expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(all_oids)
 
@@ -48,32 +41,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do
       end
     end
 
-    context 'when some lfs objects are linked' do
-      before do
-        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(existing_lfs_objects.keys)
-        allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links)
-      end
-
-      it 'retrieves the download links of non existent objects' do
-        expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(oids)
-
-        subject.execute
-      end
-    end
-
-    context 'when all lfs objects are linked' do
-      before do
-        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(all_oids.keys)
-        allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute)
-      end
-
-      it 'retrieves no download links' do
-        expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with({}).and_call_original
-
-        expect(subject.execute).to be_empty
-      end
-    end
-
     context 'when lfsconfig file exists' do
       before do
         allow(project.repository).to receive(:lfsconfig_for).and_return("[lfs]\n\turl = #{lfs_endpoint}\n")

spec/support/shared_contexts/policies/group_policy_shared_context.rb

@@ -7,6 +7,7 @@ RSpec.shared_context 'GroupPolicy context' do
   let_it_be(:maintainer) { create(:user) }
   let_it_be(:owner) { create(:user) }
   let_it_be(:admin) { create(:admin) }
+  let_it_be(:non_group_member) { create(:user) }
   let_it_be(:group, refind: true) { create(:group, :private, :owner_subgroup_creation_only) }
 
   let(:guest_permissions) do

spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb

@@ -69,13 +69,39 @@ shared_examples 'handle uploads' do
   end
 
   describe "GET #show" do
+    let(:filename) { "rails_sample.jpg" }
+
+    let(:upload_service) do
+      UploadService.new(model, jpg, uploader_class).execute
+    end
+
     let(:show_upload) do
-      get :show, params: params.merge(secret: secret, filename: "rails_sample.jpg")
+      get :show, params: params.merge(secret: secret, filename: filename)
     end
 
     before do
       allow(FileUploader).to receive(:generate_secret).and_return(secret)
-      UploadService.new(model, jpg, uploader_class).execute
+      upload_service
+    end
+
+    context 'when the secret is invalid' do
+      let(:secret) { "../../../../../../../../" }
+      let(:filename) { "Gemfile.lock" }
+      let(:upload_service) { nil }
+
+      it 'responds with status 404' do
+        show_upload
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+
+      it 'is a working exploit without the validation' do
+        allow_any_instance_of(FileUploader).to receive(:secret) { secret }
+
+        show_upload
+
+        expect(response).to have_gitlab_http_status(:ok)
+      end
     end
 
     context 'when accessing a specific upload via different model' do

spec/uploaders/file_mover_spec.rb

@@ -7,7 +7,7 @@ describe FileMover do
 
   let(:user) { create(:user) }
   let(:filename) { 'banana_sample.gif' }
-  let(:secret) { 'secret55' }
+  let(:secret) { SecureRandom.hex }
   let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", secret, filename) }
 
   let(:temp_description) do
@@ -47,8 +47,8 @@ describe FileMover do
           subject
 
           expect(snippet.reload.description)
-            .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\
+            .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) "\
-                   "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) ")
+                   "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) ")
         end
 
         it 'updates existing upload record' do
@@ -75,8 +75,8 @@ describe FileMover do
           subject
 
           expect(snippet.reload.description)
-            .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\
+            .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) "\
-                   "same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) ")
+                   "same ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) ")
         end
 
         it 'does not change the upload record' do
@@ -101,8 +101,8 @@ describe FileMover do
           subject
 
           expect(snippet.reload.description)
-            .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\
+            .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) "\
-                   "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) ")
+                   "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) ")
         end
 
         it 'creates new target upload record an delete the old upload' do
@@ -121,8 +121,8 @@ describe FileMover do
           subject
 
           expect(snippet.reload.description)
-            .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\
+            .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) "\
-                   "same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) ")
+                   "same ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) ")
         end
 
         it 'does not change the upload record' do
@@ -138,12 +138,8 @@ describe FileMover do
       let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", '..', 'another_subdir_of_temp') }
 
       it 'does not trigger move if path is outside designated directory' do
-        stub_file_mover("uploads/-/system/user/#{user.id}/another_subdir_of_temp")
         expect(FileUtils).not_to receive(:move)
-
+        expect { subject }.to raise_error(FileUploader::InvalidSecret)
-        subject
-
-        expect(snippet.reload.description).to eq(temp_description)
       end
     end
 

spec/uploaders/file_uploader_spec.rb

@@ -6,7 +6,8 @@ describe FileUploader do
   let(:group) { create(:group, name: 'awesome') }
   let(:project) { create(:project, :legacy_storage, namespace: group, name: 'project') }
   let(:uploader) { described_class.new(project, :avatar) }
-  let(:upload) { double(model: project, path: 'secret/foo.jpg') }
+  let(:upload) { double(model: project, path: "#{secret}/foo.jpg") }
+  let(:secret) { "55dc16aa0edd05693fd98b5051e83321" } # this would be nicer as SecureRandom.hex, but the shared_examples breaks
 
   subject { uploader }
 
@@ -14,7 +15,7 @@ describe FileUploader do
     include_examples 'builds correct paths',
                      store_dir: %r{awesome/project/\h+},
                      upload_path: %r{\h+/<filename>},
-                     absolute_path: %r{#{described_class.root}/awesome/project/secret/foo.jpg}
+                     absolute_path: %r{#{described_class.root}/awesome/project/55dc16aa0edd05693fd98b5051e83321/foo.jpg}
   end
 
   context 'legacy storage' do
@@ -51,11 +52,11 @@ describe FileUploader do
   end
 
   describe 'initialize' do
-    let(:uploader) { described_class.new(double, secret: 'secret') }
+    let(:uploader) { described_class.new(double, secret: secret) }
 
     it 'accepts a secret parameter' do
       expect(described_class).not_to receive(:generate_secret)
-      expect(uploader.secret).to eq('secret')
+      expect(uploader.secret).to eq(secret)
     end
   end
 
@@ -154,8 +155,39 @@ describe FileUploader do
 
   describe '#secret' do
     it 'generates a secret if none is provided' do
-      expect(described_class).to receive(:generate_secret).and_return('secret')
+      expect(described_class).to receive(:generate_secret).and_return(secret)
-      expect(uploader.secret).to eq('secret')
+      expect(uploader.secret).to eq(secret)
+      expect(uploader.secret.size).to eq(32)
+    end
+
+    context "validation" do
+      before do
+        uploader.instance_variable_set(:@secret, secret)
+      end
+
+      context "32-byte hexadecimal" do
+        let(:secret) { SecureRandom.hex }
+
+        it "returns the secret" do
+          expect(uploader.secret).to eq(secret)
+        end
+      end
+
+      context "10-byte hexadecimal" do
+        let(:secret) { SecureRandom.hex(10) }
+
+        it "returns the secret" do
+          expect(uploader.secret).to eq(secret)
+        end
+      end
+
+      context "invalid secret supplied" do
+        let(:secret) { "%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2Fgrafana%2Fconf%2F" }
+
+        it "raises an exception" do
+          expect { uploader.secret }.to raise_error(described_class::InvalidSecret)
+        end
+      end
     end
   end
 

spec/uploaders/personal_file_uploader_spec.rb

@@ -6,12 +6,13 @@ describe PersonalFileUploader do
   let(:model) { create(:personal_snippet) }
   let(:uploader) { described_class.new(model) }
   let(:upload) { create(:upload, :personal_snippet_upload) }
+  let(:secret) { SecureRandom.hex }
 
   subject { uploader }
 
   shared_examples '#base_dir' do
     before do
-      subject.instance_variable_set(:@secret, 'secret')
+      subject.instance_variable_set(:@secret, secret)
     end
 
     it 'is prefixed with uploads/-/system' do
@@ -23,12 +24,12 @@ describe PersonalFileUploader do
 
   shared_examples '#to_h' do
     before do
-      subject.instance_variable_set(:@secret, 'secret')
+      subject.instance_variable_set(:@secret, secret)
     end
 
     it 'is correct' do
       allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name'))
-      expected_url = "/uploads/-/system/personal_snippet/#{model.id}/secret/file_name"
+      expected_url = "/uploads/-/system/personal_snippet/#{model.id}/#{secret}/file_name"
 
       expect(uploader.to_h).to eq(
         alt: 'file_name',

spec/validators/addressable_url_validator_spec.rb

@@ -5,6 +5,9 @@ require 'spec_helper'
 describe AddressableUrlValidator do
   let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
 
+  let(:validator) { described_class.new(validator_options.reverse_merge(attributes: [:link_url])) }
+  let(:validator_options) { {} }
+
   subject { validator.validate(badge) }
 
   include_examples 'url validator examples', described_class::DEFAULT_OPTIONS[:schemes]
@@ -114,6 +117,19 @@ describe AddressableUrlValidator do
     end
   end
 
+  context 'when blocked_message is set' do
+    let(:message) { 'is not allowed due to: %{exception_message}' }
+    let(:validator_options) { { blocked_message: message } }
+
+    it 'blocks url with provided error message' do
+      badge.link_url = 'javascript:alert(window.opener.document.location)'
+
+      subject
+
+      expect(badge.errors.first[1]).to eq 'is not allowed due to: Only allowed schemes are http, https'
+    end
+  end
+
   context 'when allow_nil is set to true' do
     let(:validator) { described_class.new(attributes: [:link_url], allow_nil: true) }