Automatic merge of gitlab-org/gitlab-ce master

app/assets/javascripts/gfm_auto_complete.js

@@ -263,7 +263,7 @@ class GfmAutoComplete {
       displayTpl(value) {
         let tmpl = GfmAutoComplete.Loading.template;
         if (value.title != null) {
-          tmpl = GfmAutoComplete.Milestones.template;
+          tmpl = GfmAutoComplete.Milestones.templateFunction(value.title);
         }
         return tmpl;
       },
@@ -330,7 +330,7 @@ class GfmAutoComplete {
       searchKey: 'search',
       data: GfmAutoComplete.defaultLoadingData,
       displayTpl(value) {
-        let tmpl = GfmAutoComplete.Labels.template;
+        let tmpl = GfmAutoComplete.Labels.templateFunction(value.color, value.title);
         if (GfmAutoComplete.isLoading(value)) {
           tmpl = GfmAutoComplete.Loading.template;
         }
@@ -595,9 +595,11 @@ GfmAutoComplete.Members = {
   },
 };
 GfmAutoComplete.Labels = {
-  template:
-    // eslint-disable-next-line no-template-curly-in-string
-    '<li><span class="dropdown-label-box" style="background: ${color}"></span> ${title}</li>',
+  templateFunction(color, title) {
+    return `<li><span class="dropdown-label-box" style="background: ${_.escape(
+      color,
+    )}"></span> ${_.escape(title)}</li>`;
+  },
 };
 // Issues, MergeRequests and Snippets
 GfmAutoComplete.Issues = {
@@ -607,8 +609,9 @@ GfmAutoComplete.Issues = {
 };
 // Milestones
 GfmAutoComplete.Milestones = {
-  // eslint-disable-next-line no-template-curly-in-string
-  template: '<li>${title}</li>',
+  templateFunction(title) {
+    return `<li>${_.escape(title)}</li>`;
+  },
 };
 GfmAutoComplete.Loading = {
   template:

app/controllers/groups/settings/ci_cd_controller.rb

@@ -4,7 +4,7 @@ module Groups
   module Settings
     class CiCdController < Groups::ApplicationController
       skip_cross_project_access_check :show
-      before_action :authorize_admin_pipeline!
+      before_action :authorize_admin_group!
 
       def show
         define_ci_variables
@@ -26,8 +26,8 @@ module Groups
           .map { |variable| variable.present(current_user: current_user) }
       end
 
-      def authorize_admin_pipeline!
-        return render_404 unless can?(current_user, :admin_pipeline, group)
+      def authorize_admin_group!
+        return render_404 unless can?(current_user, :admin_group, group)
       end
     end
   end

app/controllers/projects/snippets_controller.rb

@@ -75,7 +75,14 @@ class Projects::SnippetsController < Projects::ApplicationController
       format.json do
         render_blob_json(blob)
       end
-      format.js { render 'shared/snippets/show'}
+
+      format.js do
+        if @snippet.embeddable?
+          render 'shared/snippets/show'
+        else
+          head :not_found
+        end
+      end
     end
   end
 

app/controllers/projects_controller.rb

@@ -19,6 +19,7 @@ class ProjectsController < Projects::ApplicationController
   before_action :lfs_blob_ids, only: [:show], if: [:repo_exists?, :project_view_files?]
   before_action :project_export_enabled, only: [:export, :download_export, :remove_export, :generate_new_export]
   before_action :present_project, only: [:edit]
+  before_action :authorize_download_code!, only: [:refs]
 
   # Authorize
   before_action :authorize_admin_project!, only: [:edit, :update, :housekeeping, :download_export, :export, :remove_export, :generate_new_export]

app/controllers/snippets_controller.rb

@@ -80,7 +80,13 @@ class SnippetsController < ApplicationController
         render_blob_json(blob)
       end
 
-      format.js { render 'shared/snippets/show' }
+      format.js do
+        if @snippet.embeddable?
+          render 'shared/snippets/show'
+        else
+          head :not_found
+        end
+      end
     end
   end
 

app/helpers/snippets_helper.rb

@@ -130,12 +130,4 @@ module SnippetsHelper
 
     link_to external_snippet_icon('download'), download_url, class: 'btn', target: '_blank', title: 'Download', rel: 'noopener noreferrer'
   end
-
-  def public_snippet?
-    if @snippet.project_id?
-      can?(nil, :read_project_snippet, @snippet)
-    else
-      can?(nil, :read_personal_snippet, @snippet)
-    end
-  end
 end

app/models/project.rb

@@ -324,10 +324,9 @@ class Project < ActiveRecord::Base
 
   validates :namespace, presence: true
   validates :name, uniqueness: { scope: :namespace_id }
-  validates :import_url, url: { protocols: ->(project) { project.persisted? ? VALID_MIRROR_PROTOCOLS : VALID_IMPORT_PROTOCOLS },
-                                ports: ->(project) { project.persisted? ? VALID_MIRROR_PORTS : VALID_IMPORT_PORTS },
-                                allow_localhost: false,
-                                enforce_user: true }, if: [:external_import?, :import_url_changed?]
+  validates :import_url, public_url: { protocols: ->(project) { project.persisted? ? VALID_MIRROR_PROTOCOLS : VALID_IMPORT_PROTOCOLS },
+                                       ports: ->(project) { project.persisted? ? VALID_MIRROR_PORTS : VALID_IMPORT_PORTS },
+                                       enforce_user: true }, if: [:external_import?, :import_url_changed?]
   validates :star_count, numericality: { greater_than_or_equal_to: 0 }
   validate :check_limit, on: :create
   validate :check_repository_path_availability, on: :update, if: ->(project) { project.renamed? }

app/models/remote_mirror.rb

@@ -17,7 +17,7 @@ class RemoteMirror < ActiveRecord::Base
 
   belongs_to :project, inverse_of: :remote_mirrors
 
-  validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true, enforce_user: true }
+  validates :url, presence: true, public_url: { protocols: %w(ssh git http https), allow_blank: true, enforce_user: true }
 
   before_save :set_new_remote_name, if: :mirror_url_changed?
 

app/models/snippet.rb

@@ -175,6 +175,12 @@ class Snippet < ActiveRecord::Base
     :visibility_level
   end
 
+  def embeddable?
+    ability = project_id? ? :read_project_snippet : :read_personal_snippet
+
+    Ability.allowed?(nil, ability, self)
+  end
+
   def notes_with_associations
     notes.includes(:author)
   end

app/policies/issuable_policy.rb

@@ -11,7 +11,7 @@ class IssuablePolicy < BasePolicy
     @user && @subject.assignee_or_author?(@user)
   end
 
-  rule { assignee_or_author }.policy do
+  rule { can?(:guest_access) & assignee_or_author }.policy do
     enable :read_issue
     enable :update_issue
     enable :reopen_issue

app/services/merge_requests/build_service.rb

@@ -18,7 +18,7 @@ module MergeRequests
       merge_request.source_project  = find_source_project
       merge_request.target_project  = find_target_project
       merge_request.target_branch   = find_target_branch
-      merge_request.can_be_created  = branches_valid?
+      merge_request.can_be_created  = projects_and_branches_valid?
 
       # compare branches only if branches are valid, otherwise
       # compare_branches may raise an error
@@ -49,15 +49,19 @@ module MergeRequests
              to: :merge_request
 
     def find_source_project
-      return source_project if source_project.present? && can?(current_user, :read_project, source_project)
+      return source_project if source_project.present? && can?(current_user, :create_merge_request_from, source_project)
 
       project
     end
 
     def find_target_project
-      return target_project if target_project.present? && can?(current_user, :read_project, target_project)
+      return target_project if target_project.present? && can?(current_user, :create_merge_request_in, target_project)
 
-      project.default_merge_request_target
+      target_project = project.default_merge_request_target
+
+      return target_project if target_project.present? && can?(current_user, :create_merge_request_in, target_project)
+
+      project
     end
 
     def find_target_branch
@@ -72,10 +76,11 @@ module MergeRequests
       params[:target_branch].present?
     end
 
-    def branches_valid?
+    def projects_and_branches_valid?
+      return false if source_project.nil? || target_project.nil?
       return false unless source_branch_specified? || target_branch_specified?
 
-      validate_branches
+      validate_projects_and_branches
       errors.blank?
     end
 
@@ -94,7 +99,12 @@ module MergeRequests
       end
     end
 
-    def validate_branches
+    def validate_projects_and_branches
+      merge_request.validate_target_project
+      merge_request.validate_fork
+
+      return if errors.any?
+
       add_error('You must select source and target branch') unless branches_present?
       add_error('You must select different branches') if same_source_and_target?
       add_error("Source branch \"#{source_branch}\" does not exist") unless source_branch_exists?

app/services/projects/lfs_pointers/lfs_download_service.rb

@@ -12,28 +12,43 @@ module Projects
 
         return if LfsObject.exists?(oid: oid)
 
-        sanitized_uri = Gitlab::UrlSanitizer.new(url)
-        Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url, protocols: VALID_PROTOCOLS)
+        sanitized_uri = sanitize_url!(url)
 
         with_tmp_file(oid) do |file|
-          size = download_and_save_file(file, sanitized_uri)
-          lfs_object = LfsObject.new(oid: oid, size: size, file: file)
+          download_and_save_file(file, sanitized_uri)
+          lfs_object = LfsObject.new(oid: oid, size: file.size, file: file)
 
           project.all_lfs_objects << lfs_object
         end
+      rescue Gitlab::UrlBlocker::BlockedUrlError => e
+        Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded: #{e.message}")
       rescue StandardError => e
-        Rails.logger.error("LFS file with oid #{oid} could't be downloaded from #{sanitized_uri.sanitized_url}: #{e.message}")
+        Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded from #{sanitized_uri.sanitized_url}: #{e.message}")
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
       private
 
+      def sanitize_url!(url)
+        Gitlab::UrlSanitizer.new(url).tap do |sanitized_uri|
+          # Just validate that HTTP/HTTPS protocols are used. The
+          # subsequent Gitlab::HTTP.get call will do network checks
+          # based on the settings.
+          Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url,
+                                       protocols: VALID_PROTOCOLS)
+        end
+      end
+
       def download_and_save_file(file, sanitized_uri)
-        IO.copy_stream(open(sanitized_uri.sanitized_url, headers(sanitized_uri)), file) # rubocop:disable Security/Open
+        response = Gitlab::HTTP.get(sanitized_uri.sanitized_url, headers(sanitized_uri)) do |fragment|
+          file.write(fragment)
+        end
+
+        raise StandardError, "Received error code #{response.code}" unless response.success?
       end
 
       def headers(sanitized_uri)
-        {}.tap do |headers|
+        query_options.tap do |headers|
           credentials = sanitized_uri.credentials
 
           if credentials[:user].present? || credentials[:password].present?
@@ -43,10 +58,14 @@ module Projects
         end
       end
 
+      def query_options
+        { stream_body: true }
+      end
+
       def with_tmp_file(oid)
         create_tmp_storage_dir
 
-        File.open(File.join(tmp_storage_dir, oid), 'w') { |file| yield file }
+        File.open(File.join(tmp_storage_dir, oid), 'wb') { |file| yield file }
       end
 
       def create_tmp_storage_dir

app/views/shared/snippets/_header.html.haml

@@ -30,7 +30,7 @@
   - if @snippet.updated_at != @snippet.created_at
     = edited_time_ago_with_tooltip(@snippet, placement: 'bottom', html_class: 'snippet-edited-ago', exclude_author: true)
 
-  - if public_snippet?
+  - if @snippet.embeddable?
     .embed-snippet
       .input-group
         .input-group-prepend

changelogs/unreleased/security-48259-private-snippet.yml

+---
+title: Prevent private snippets from being embeddable
+merge_request:
+author:
+type: security

changelogs/unreleased/security-53543-user-keeps-access-to-mr-issue-when-removed-from-team.yml

+---
+title: Issuable no longer is visible to users when project can't be viewed
+merge_request:
+author:
+type: security

changelogs/unreleased/security-54377-label-milestone-name-xss.yml

+---
+title: Escape label and milestone titles to prevent XSS in GFM autocomplete
+merge_request: 2693
+author:
+type: security

changelogs/unreleased/security-bvl-fix-cross-project-mr-exposure.yml

+---
+title: Don't expose cross project repositories through diffs when creating merge reqeusts
+merge_request:
+author:
+type: security

changelogs/unreleased/security-fix-ssrf-import-url-remote-mirror.yml

+---
+title: Fix SSRF with import_url and remote mirror url
+merge_request:
+author:
+type: security

changelogs/unreleased/security-master-group-cicd-settings-accessible-to-maintainer.yml

+---
+title: Allow changing group CI/CD settings only for owners.
+merge_request:
+author:
+type: security

changelogs/unreleased/security-master-guests-jobs-api.yml

+---
+title: Authorize before reading job information via API.
+merge_request:
+author:
+type: security

changelogs/unreleased/security-refs-available-to-project-guest.yml

+---
+title: Project guests no longer are able to see refs page
+merge_request:
+author:
+type: security

lib/api/jobs.rb

@@ -38,6 +38,8 @@ module API
       end
       # rubocop: disable CodeReuse/ActiveRecord
       get ':id/jobs' do
+        authorize_read_builds!
+
         builds = user_project.builds.order('id DESC')
         builds = filter_builds(builds, params[:scope])
 
@@ -56,7 +58,10 @@ module API
       end
       # rubocop: disable CodeReuse/ActiveRecord
       get ':id/pipelines/:pipeline_id/jobs' do
+        authorize!(:read_pipeline, user_project)
         pipeline = user_project.ci_pipelines.find(params[:pipeline_id])
+        authorize!(:read_build, pipeline)
+
         builds = pipeline.builds
         builds = filter_builds(builds, params[:scope])
         builds = builds.preload(:job_artifacts_archive, :job_artifacts, project: [:namespace])

spec/controllers/groups/settings/ci_cd_controller_spec.rb

@@ -5,30 +5,65 @@ describe Groups::Settings::CiCdController do
   let(:user) { create(:user) }
 
   before do
-    group.add_maintainer(user)
     sign_in(user)
   end
 
   describe 'GET #show' do
-    it 'renders show with 200 status code' do
-      get :show, params: { group_id: group }
+    context 'when user is owner' do
+      before do
+        group.add_owner(user)
+      end
 
-      expect(response).to have_gitlab_http_status(200)
-      expect(response).to render_template(:show)
+      it 'renders show with 200 status code' do
+        get :show, params: { group_id: group }
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(response).to render_template(:show)
+      end
+    end
+
+    context 'when user is not owner' do
+      before do
+        group.add_maintainer(user)
+      end
+
+      it 'renders a 404' do
+        get :show, params: { group_id: group }
+
+        expect(response).to have_gitlab_http_status(404)
+      end
     end
   end
 
   describe 'PUT #reset_registration_token' do
     subject { put :reset_registration_token, params: { group_id: group } }
 
-    it 'resets runner registration token' do
-      expect { subject }.to change { group.reload.runners_token }
+    context 'when user is owner' do
+      before do
+        group.add_owner(user)
+      end
+
+      it 'resets runner registration token' do
+        expect { subject }.to change { group.reload.runners_token }
+      end
+
+      it 'redirects the user to admin runners page' do
+        subject
+
+        expect(response).to redirect_to(group_settings_ci_cd_path)
+      end
     end
 
-    it 'redirects the user to admin runners page' do
-      subject
+    context 'when user is not owner' do
+      before do
+        group.add_maintainer(user)
+      end
+
+      it 'renders a 404' do
+        subject
 
-      expect(response).to redirect_to(group_settings_ci_cd_path)
+        expect(response).to have_gitlab_http_status(404)
+      end
     end
   end
 end

spec/controllers/projects/snippets_controller_spec.rb

@@ -379,6 +379,46 @@ describe Projects::SnippetsController do
     end
   end
 
+  describe "GET #show for embeddable content" do
+    let(:project_snippet) { create(:project_snippet, snippet_permission, project: project, author: user) }
+
+    before do
+      sign_in(user)
+
+      get :show, namespace_id: project.namespace, project_id: project, id: project_snippet.to_param, format: :js
+    end
+
+    context 'when snippet is private' do
+      let(:snippet_permission) { :private }
+
+      it 'responds with status 404' do
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+
+    context 'when snippet is public' do
+      let(:snippet_permission) { :public }
+
+      it 'responds with status 200' do
+        expect(assigns(:snippet)).to eq(project_snippet)
+        expect(response).to have_gitlab_http_status(200)
+      end
+    end
+
+    context 'when the project is private' do
+      let(:project) { create(:project_empty_repo, :private) }
+
+      context 'when snippet is public' do
+        let(:project_snippet) { create(:project_snippet, :public, project: project, author: user) }
+
+        it 'responds with status 404' do
+          expect(assigns(:snippet)).to eq(project_snippet)
+          expect(response).to have_gitlab_http_status(404)
+        end
+      end
+    end
+  end
+
   describe 'GET #raw' do
     let(:project_snippet) do
       create(

spec/controllers/projects_controller_spec.rb

@@ -644,10 +644,10 @@ describe ProjectsController do
   end
 
   describe "GET refs" do
-    let(:public_project) { create(:project, :public, :repository) }
+    let(:project) { create(:project, :public, :repository) }
 
     it 'gets a list of branches and tags' do
-      get :refs, params: { namespace_id: public_project.namespace, id: public_project, sort: 'updated_desc' }
+      get :refs, params: { namespace_id: project.namespace, id: project, sort: 'updated_desc' }
 
       parsed_body = JSON.parse(response.body)
       expect(parsed_body['Branches']).to include('master')
@@ -657,7 +657,7 @@ describe ProjectsController do
     end
 
     it "gets a list of branches, tags and commits" do
-      get :refs, params: { namespace_id: public_project.namespace, id: public_project, ref: "123456" }
+      get :refs, params: { namespace_id: project.namespace, id: project, ref: "123456" }
 
       parsed_body = JSON.parse(response.body)
       expect(parsed_body["Branches"]).to include("master")
@@ -672,7 +672,7 @@ describe ProjectsController do
       end
 
       it "gets a list of branches, tags and commits" do
-        get :refs, params: { namespace_id: public_project.namespace, id: public_project, ref: "123456" }
+        get :refs, params: { namespace_id: project.namespace, id: project, ref: "123456" }
 
         parsed_body = JSON.parse(response.body)
         expect(parsed_body["Branches"]).to include("master")
@@ -680,6 +680,22 @@ describe ProjectsController do
         expect(parsed_body["Commits"]).to include("123456")
       end
     end
+
+    context 'when private project' do
+      let(:project) { create(:project, :repository) }
+
+      context 'as a guest' do
+        it 'renders forbidden' do
+          user = create(:user)
+          project.add_guest(user)
+
+          sign_in(user)
+          get :refs, namespace_id: project.namespace, id: project
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+      end
+    end
   end
 
   describe 'GET edit' do

spec/controllers/snippets_controller_spec.rb

@@ -80,6 +80,12 @@ describe SnippetsController do
             expect(assigns(:snippet)).to eq(personal_snippet)
             expect(response).to have_gitlab_http_status(200)
           end
+
+          it 'responds with status 404 when embeddable content is requested' do
+            get :show, id: personal_snippet.to_param, format: :js
+
+            expect(response).to have_gitlab_http_status(404)
+          end
         end
       end
 
@@ -106,6 +112,12 @@ describe SnippetsController do
           expect(assigns(:snippet)).to eq(personal_snippet)
           expect(response).to have_gitlab_http_status(200)
         end
+
+        it 'responds with status 404 when embeddable content is requested' do
+          get :show, id: personal_snippet.to_param, format: :js
+
+          expect(response).to have_gitlab_http_status(404)
+        end
       end
 
       context 'when not signed in' do
@@ -131,6 +143,13 @@ describe SnippetsController do
           expect(assigns(:snippet)).to eq(personal_snippet)
           expect(response).to have_gitlab_http_status(200)
         end
+
+        it 'responds with status 200 when embeddable content is requested' do
+          get :show, id: personal_snippet.to_param, format: :js
+
+          expect(assigns(:snippet)).to eq(personal_snippet)
+          expect(response).to have_gitlab_http_status(200)
+        end
       end
 
       context 'when not signed in' do

spec/features/group_variables_spec.rb

@@ -7,7 +7,7 @@ describe 'Group variables', :js do
   let(:page_path) { group_settings_ci_cd_path(group) }
 
   before do
-    group.add_maintainer(user)
+    group.add_owner(user)
     gitlab_sign_in(user)
 
     visit page_path

spec/features/issues/gfm_autocomplete_spec.rb

@@ -3,6 +3,8 @@ require 'rails_helper'
 describe 'GFM autocomplete', :js do
   let(:issue_xss_title) { 'This will execute alert<img src=x onerror=alert(2)&lt;img src=x onerror=alert(1)&gt;' }
   let(:user_xss_title) { 'eve <img src=x onerror=alert(2)&lt;img src=x onerror=alert(1)&gt;' }
+  let(:label_xss_title) { 'alert label &lt;img src=x onerror="alert(\'Hello xss\');" a'}
+  let(:milestone_xss_title) { 'alert milestone &lt;img src=x onerror="alert(\'Hello xss\');" a' }
 
   let(:user_xss) { create(:user, name: user_xss_title, username: 'xss.user') }
   let(:user)    { create(:user, name: '💃speciąl someone💃', username: 'someone.special') }
@@ -25,10 +27,14 @@ describe 'GFM autocomplete', :js do
 
     simulate_input('#issue-description', "@#{user.name[0...3]}")
 
+    wait_for_requests
+
     find('.atwho-view .cur').click
 
     click_button 'Save changes'
 
+    wait_for_requests
+
     expect(find('.description')).to have_content(user.to_reference)
   end
 
@@ -47,6 +53,8 @@ describe 'GFM autocomplete', :js do
       find('#note-body').native.send_keys('#')
     end
 
+    wait_for_requests
+
     expect(page).to have_selector('.atwho-container')
 
     page.within '.atwho-container #at-view-issues' do
@@ -59,6 +67,8 @@ describe 'GFM autocomplete', :js do
       find('#note-body').native.send_keys('@ev')
     end
 
+    wait_for_requests
+
     expect(page).to have_selector('.atwho-container')
 
     page.within '.atwho-container #at-view-users' do
@@ -66,6 +76,22 @@ describe 'GFM autocomplete', :js do
     end
   end
 
+  it 'opens autocomplete menu for Milestone when field starts with text with item escaping HTML characters' do
+    create(:milestone, project: project, title: milestone_xss_title)
+
+    page.within '.timeline-content-form' do
+      find('#note-body').native.send_keys('%')
+    end
+
+    wait_for_requests
+
+    expect(page).to have_selector('.atwho-container')
+
+    page.within '.atwho-container #at-view-milestones' do
+      expect(find('li').text).to have_content('alert milestone')
+    end
+  end
+
   it 'doesnt open autocomplete menu character is prefixed with text' do
     page.within '.timeline-content-form' do
       find('#note-body').native.send_keys('testing')
@@ -258,12 +284,28 @@ describe 'GFM autocomplete', :js do
     let!(:bug)              { create(:label, project: project, title: 'bug') }
     let!(:feature_proposal) { create(:label, project: project, title: 'feature proposal') }
 
+    it 'opens autocomplete menu for Labels when field starts with text with item escaping HTML characters' do
+      create(:label, project: project, title: label_xss_title)
+
+      note = find('#note-body')
+
+      # It should show all the labels on "~".
+      type(note, '~')
+
+      wait_for_requests
+
+      page.within '.atwho-container #at-view-labels' do
+        expect(find('.atwho-view-ul').text).to have_content('alert label')
+      end
+    end
+
     context 'when no labels are assigned' do
       it 'shows labels' do
         note = find('#note-body')
 
         # It should show all the labels on "~".
         type(note, '~')
+        wait_for_requests
         expect_labels(shown: [backend, bug, feature_proposal])
 
         # It should show all the labels on "/label ~".
@@ -290,6 +332,7 @@ describe 'GFM autocomplete', :js do
 
         # It should show all the labels on "~".
         type(note, '~')
+        wait_for_requests
         expect_labels(shown: [backend, bug, feature_proposal])
 
         # It should show only unset labels on "/label ~".
@@ -316,6 +359,7 @@ describe 'GFM autocomplete', :js do
 
         # It should show all the labels on "~".
         type(note, '~')
+        wait_for_requests
         expect_labels(shown: [backend, bug, feature_proposal])
 
         # It should show no labels on "/label ~".

spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb

+require 'spec_helper'
+
+describe 'Merge Request > Tries to access private repo of public project' do
+  let(:current_user) { create(:user) }
+  let(:private_project) do
+    create(:project, :public, :repository,
+           path: 'nothing-to-see-here',
+           name: 'nothing to see here',
+           repository_access_level: ProjectFeature::PRIVATE)
+  end
+  let(:owned_project) do
+    create(:project, :public, :repository,
+           namespace: current_user.namespace,
+           creator: current_user)
+  end
+
+  context 'when the user enters the querystring info for the other project' do
+    let(:mr_path) do
+      project_new_merge_request_diffs_path(
+        owned_project,
+        merge_request: {
+          source_project_id: private_project.id,
+          source_branch: 'feature'
+        }
+      )
+    end
+
+    before do
+      sign_in current_user
+      visit mr_path
+    end
+
+    it "does not mention the project the user can't see the repo of" do
+      expect(page).not_to have_content('nothing-to-see-here')
+    end
+  end
+end

spec/features/runners_spec.rb

@@ -259,8 +259,9 @@ describe 'Runners' do
 
   context 'group runners in group settings' do
     let(:group) { create(:group) }
+
     before do
-      group.add_maintainer(user)
+      group.add_owner(user)
     end
 
     context 'group with no runners' do

spec/models/event_spec.rb

@@ -243,6 +243,20 @@ describe Event do
           expect(event.visible_to_user?(admin)).to eq true
         end
       end
+
+      context 'private project' do
+        let(:project) { create(:project, :private) }
+        let(:target) { note_on_issue }
+
+        it do
+          expect(event.visible_to_user?(non_member)).to eq false
+          expect(event.visible_to_user?(author)).to eq false
+          expect(event.visible_to_user?(assignee)).to eq false
+          expect(event.visible_to_user?(member)).to eq true
+          expect(event.visible_to_user?(guest)).to eq true
+          expect(event.visible_to_user?(admin)).to eq true
+        end
+      end
     end
 
     context 'merge request diff note event' do
@@ -265,8 +279,8 @@ describe Event do
 
         it do
           expect(event.visible_to_user?(non_member)).to eq false
-          expect(event.visible_to_user?(author)).to eq true
-          expect(event.visible_to_user?(assignee)).to eq true
+          expect(event.visible_to_user?(author)).to eq false
+          expect(event.visible_to_user?(assignee)).to eq false
           expect(event.visible_to_user?(member)).to eq true
           expect(event.visible_to_user?(guest)).to eq false
           expect(event.visible_to_user?(admin)).to eq true

spec/models/project_spec.rb

@@ -325,6 +325,13 @@ describe Project do
         expect(project.errors[:import_url].first).to include('Requests to localhost are not allowed')
       end
 
+      it 'does not allow import_url pointing to the local network' do
+        project = build(:project, import_url: 'https://192.168.1.1')
+
+        expect(project).to be_invalid
+        expect(project.errors[:import_url].first).to include('Requests to the local network are not allowed')
+      end
+
       it "does not allow import_url with invalid ports for new projects" do
         project = build(:project, import_url: 'http://github.com:25/t.git')
 

spec/models/remote_mirror_spec.rb

@@ -24,6 +24,20 @@ describe RemoteMirror, :mailer do
         expect(remote_mirror).to be_invalid
         expect(remote_mirror.errors[:url].first).to include('Username needs to start with an alphanumeric character')
       end
+
+      it 'does not allow url pointing to localhost' do
+        remote_mirror = build(:remote_mirror, url: 'http://127.0.0.2/t.git')
+
+        expect(remote_mirror).to be_invalid
+        expect(remote_mirror.errors[:url].first).to include('Requests to loopback addresses are not allowed')
+      end
+
+      it 'does not allow url pointing to the local network' do
+        remote_mirror = build(:remote_mirror, url: 'https://192.168.1.1')
+
+        expect(remote_mirror).to be_invalid
+        expect(remote_mirror.errors[:url].first).to include('Requests to the local network are not allowed')
+      end
     end
   end
 

spec/models/snippet_spec.rb

@@ -423,4 +423,41 @@ describe Snippet do
       expect(blob.data).to eq(snippet.content)
     end
   end
+
+  describe '#embeddable?' do
+    context 'project snippet' do
+      [
+        { project: :public,   snippet: :public,   embeddable: true },
+        { project: :internal, snippet: :public,   embeddable: false },
+        { project: :private,  snippet: :public,   embeddable: false },
+        { project: :public,   snippet: :internal, embeddable: false },
+        { project: :internal, snippet: :internal, embeddable: false },
+        { project: :private,  snippet: :internal, embeddable: false },
+        { project: :public,   snippet: :private,  embeddable: false },
+        { project: :internal, snippet: :private,  embeddable: false },
+        { project: :private,  snippet: :private,  embeddable: false }
+      ].each do |combination|
+        it 'only returns true when both project and snippet are public' do
+          project = create(:project, combination[:project])
+          snippet = create(:project_snippet, combination[:snippet], project: project)
+
+          expect(snippet.embeddable?).to eq(combination[:embeddable])
+        end
+      end
+    end
+
+    context 'personal snippet' do
+      [
+        { snippet: :public,   embeddable: true },
+        { snippet: :internal, embeddable: false },
+        { snippet: :private,  embeddable: false }
+      ].each do |combination|
+        it 'only returns true when snippet is public' do
+          snippet = create(:personal_snippet, combination[:snippet])
+
+          expect(snippet.embeddable?).to eq(combination[:embeddable])
+        end
+      end
+    end
+  end
 end

spec/policies/issuable_policy_spec.rb

@@ -7,6 +7,33 @@ describe IssuablePolicy, models: true do
   let(:policies) { described_class.new(user, issue) }
 
   describe '#rules' do
+    context 'when user is author of issuable' do
+      let(:merge_request) { create(:merge_request, source_project: project, author: user) }
+      let(:policies) { described_class.new(user, merge_request) }
+
+      context 'when user is able to read project' do
+        it 'enables user to read and update issuables' do
+          expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request)
+        end
+      end
+
+      context 'when project is private' do
+        let(:project) { create(:project, :private) }
+
+        context 'when user belongs to the projects team' do
+          it 'enables user to read and update issuables' do
+            project.add_maintainer(user)
+
+            expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request)
+          end
+        end
+
+        it 'disallows user from reading and updating issuables from that project' do
+          expect(policies).to be_disallowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request)
+        end
+      end
+    end
+
     context 'when discussion is locked for the issuable' do
       let(:issue) { create(:issue, project: project, discussion_locked: true) }
 

spec/requests/api/jobs_spec.rb

@@ -143,10 +143,20 @@ describe API::Jobs do
     end
 
     context 'unauthorized user' do
-      let(:api_user) { nil }
+      context 'when user is not logged in' do
+        let(:api_user) { nil }
 
-      it 'does not return project jobs' do
-        expect(response).to have_gitlab_http_status(401)
+        it 'does not return project jobs' do
+          expect(response).to have_gitlab_http_status(401)
+        end
+      end
+
+      context 'when user is guest' do
+        let(:api_user) { guest }
+
+        it 'does not return project jobs' do
+          expect(response).to have_gitlab_http_status(403)
+        end
       end
     end
 
@@ -242,10 +252,20 @@ describe API::Jobs do
     end
 
     context 'unauthorized user' do
-      let(:api_user) { nil }
+      context 'when user is not logged in' do
+        let(:api_user) { nil }
 
-      it 'does not return jobs' do
-        expect(response).to have_gitlab_http_status(401)
+        it 'does not return jobs' do
+          expect(response).to have_gitlab_http_status(401)
+        end
+      end
+
+      context 'when user is guest' do
+        let(:api_user) { guest }
+
+        it 'does not return jobs' do
+          expect(response).to have_gitlab_http_status(403)
+        end
       end
     end
   end

spec/services/issuable/bulk_update_service_spec.rb

@@ -28,6 +28,33 @@ describe Issuable::BulkUpdateService do
       expect(project.issues.opened).to be_empty
       expect(project.issues.closed).not_to be_empty
     end
+
+    context 'when issue for a different project is created' do
+      let(:private_project) { create(:project, :private) }
+      let(:issue) { create(:issue, project: private_project, author: user) }
+
+      context 'when user has access to the project' do
+        it 'closes all issues passed' do
+          private_project.add_maintainer(user)
+
+          bulk_update(issues + [issue], state_event: 'close')
+
+          expect(project.issues.opened).to be_empty
+          expect(project.issues.closed).not_to be_empty
+          expect(private_project.issues.closed).not_to be_empty
+        end
+      end
+
+      context 'when user does not have access to project' do
+        it 'only closes all issues that the user has access to' do
+          bulk_update(issues + [issue], state_event: 'close')
+
+          expect(project.issues.opened).to be_empty
+          expect(project.issues.closed).not_to be_empty
+          expect(private_project.issues.closed).to be_empty
+        end
+      end
+    end
   end
 
   describe 'reopen issues' do

spec/services/merge_requests/build_service_spec.rb

@@ -3,6 +3,7 @@ require 'spec_helper'
 describe MergeRequests::BuildService do
   using RSpec::Parameterized::TableSyntax
   include RepoHelpers
+  include ProjectForksHelper
 
   let(:project) { create(:project, :repository) }
   let(:source_project) { nil }
@@ -49,7 +50,7 @@ describe MergeRequests::BuildService do
 
   describe '#execute' do
     it 'calls the compare service with the correct arguments' do
-      allow_any_instance_of(described_class).to receive(:branches_valid?).and_return(true)
+      allow_any_instance_of(described_class).to receive(:projects_and_branches_valid?).and_return(true)
       expect(CompareService).to receive(:new)
                                   .with(project, Gitlab::Git::BRANCH_REF_PREFIX + source_branch)
                                   .and_call_original
@@ -393,11 +394,27 @@ describe MergeRequests::BuildService do
       end
     end
 
+    context 'target_project is set but repo is not accessible by current_user' do
+      let(:target_project) do
+        create(:project, :public, :repository, repository_access_level: ProjectFeature::PRIVATE)
+      end
+
+      it 'sets target project correctly' do
+        expect(merge_request.target_project).to eq(project)
+      end
+    end
+
     context 'source_project is set and accessible by current_user' do
       let(:source_project) { create(:project, :public, :repository)}
       let(:commits) { Commit.decorate([commit_1], project) }
 
-      it 'sets target project correctly' do
+      before do
+        # To create merge requests _from_ a project the user needs at least
+        # developer access
+        source_project.add_developer(user)
+      end
+
+      it 'sets source project correctly' do
         expect(merge_request.source_project).to eq(source_project)
       end
     end
@@ -406,11 +423,43 @@ describe MergeRequests::BuildService do
       let(:source_project) { create(:project, :private, :repository)}
       let(:commits) { Commit.decorate([commit_1], project) }
 
-      it 'sets target project correctly' do
+      it 'sets source project correctly' do
+        expect(merge_request.source_project).to eq(project)
+      end
+    end
+
+    context 'source_project is set but the user cannot create merge requests from the project' do
+      let(:source_project) do
+        create(:project, :public, :repository, merge_requests_access_level: ProjectFeature::PRIVATE)
+      end
+
+      it 'sets the source_project correctly' do
         expect(merge_request.source_project).to eq(project)
       end
     end
 
+    context 'target_project is not in the fork network of source_project' do
+      let(:target_project) { create(:project, :public, :repository) }
+
+      it 'adds an error to the merge request' do
+        expect(merge_request.errors[:validate_fork]).to contain_exactly('Source project is not a fork of the target project')
+      end
+    end
+
+    context 'target_project is in the fork network of source project but no longer accessible' do
+      let!(:project) { fork_project(target_project, user, namespace: user.namespace, repository: true) }
+      let(:source_project) { project }
+      let(:target_project) { create(:project, :public, :repository) }
+
+      before do
+        target_project.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+      end
+
+      it 'sets the target_project correctly' do
+        expect(merge_request.target_project).to eq(project)
+      end
+    end
+
     context 'when specifying target branch in the description' do
       let(:description) { "A merge request targeting another branch\n\n/target_branch with-codeowners" }
 

spec/services/projects/lfs_pointers/lfs_download_service_spec.rb

@@ -4,17 +4,15 @@ describe Projects::LfsPointers::LfsDownloadService do
   let(:project) { create(:project) }
   let(:oid) { '9e548e25631dd9ce6b43afd6359ab76da2819d6a5b474e66118c7819e1d8b3e8' }
   let(:download_link) { "http://gitlab.com/#{oid}" }
-  let(:lfs_content) do
-    <<~HEREDOC
-    whatever
-    HEREDOC
-  end
+  let(:lfs_content) { SecureRandom.random_bytes(10) }
 
   subject { described_class.new(project) }
 
   before do
     allow(project).to receive(:lfs_enabled?).and_return(true)
     WebMock.stub_request(:get, download_link).to_return(body: lfs_content)
+
+    allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false)
   end
 
   describe '#execute' do
@@ -32,7 +30,7 @@ describe Projects::LfsPointers::LfsDownloadService do
       it 'stores the content' do
         subject.execute(oid, download_link)
 
-        expect(File.read(LfsObject.first.file.file.file)).to eq lfs_content
+        expect(File.binread(LfsObject.first.file.file.file)).to eq lfs_content
       end
     end
 
@@ -54,18 +52,61 @@ describe Projects::LfsPointers::LfsDownloadService do
       end
     end
 
+    context 'when localhost requests are allowed' do
+      let(:download_link) { 'http://192.168.2.120' }
+
+      before do
+        allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true)
+      end
+
+      it 'downloads the file' do
+        expect(subject).to receive(:download_and_save_file).and_call_original
+
+        expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.by(1)
+      end
+    end
+
     context 'when a bad URL is used' do
-      where(download_link: ['/etc/passwd', 'ftp://example.com', 'http://127.0.0.2'])
+      where(download_link: ['/etc/passwd', 'ftp://example.com', 'http://127.0.0.2', 'http://192.168.2.120'])
 
       with_them do
         it 'does not download the file' do
-          expect(subject).not_to receive(:download_and_save_file)
-
           expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count }
         end
       end
     end
 
+    context 'when the URL points to a redirected URL' do
+      context 'that is blocked' do
+        where(redirect_link: ['ftp://example.com', 'http://127.0.0.2', 'http://192.168.2.120'])
+
+        with_them do
+          before do
+            WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link })
+          end
+
+          it 'does not follow the redirection' do
+            expect(Rails.logger).to receive(:error).with(/LFS file with oid #{oid} couldn't be downloaded/)
+
+            expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count }
+          end
+        end
+      end
+
+      context 'that is valid' do
+        let(:redirect_link) { "http://example.com/"}
+
+        before do
+          WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link })
+          WebMock.stub_request(:get, redirect_link).to_return(body: lfs_content)
+        end
+
+        it 'follows the redirection' do
+          expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.from(0).to(1)
+        end
+      end
+    end
+
     context 'when an lfs object with the same oid already exists'  do
       before do
         create(:lfs_object, oid: 'oid')

spec/services/todo_service_spec.rb

@@ -19,6 +19,7 @@ describe TodoService do
   before do
     project.add_guest(guest)
     project.add_developer(author)
+    project.add_developer(assignee)
     project.add_developer(member)
     project.add_developer(john_doe)
     project.add_developer(skipped)