Merge remote-tracking branch 'dev/master'

CHANGELOG.md

@@ -2,6 +2,18 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 11.3.1 (2018-09-26)
+
+### Security (6 changes)
+
+- Redact confidential events in the API.
+- Set timeout for syntax highlighting.
+- Sanitize JSON data properly to fix XSS on Issue details page.
+- Fix stored XSS in merge requests from imported repository.
+- Fix xss vulnerability sourced from package.json.
+- Block loopback addresses in UrlBlocker.
+
+
 ## 11.3.0 (2018-09-22)
 
 ### Security (5 changes, 1 of them is from the community)
@@ -249,6 +261,18 @@ entry.
 - Creates Vue component for artifacts block on job page.
 
 
+## 11.2.4 (2018-09-26)
+
+### Security (6 changes)
+
+- Redact confidential events in the API.
+- Set timeout for syntax highlighting.
+- Sanitize JSON data properly to fix XSS on Issue details page.
+- Fix stored XSS in merge requests from imported repository.
+- Fix xss vulnerability sourced from package.json.
+- Block loopback addresses in UrlBlocker.
+
+
 ## 11.2.3 (2018-08-28)
 
 ### Fixed (1 change)
@@ -516,6 +540,18 @@ entry.
 - Moves help_popover component to a common location.
 
 
+## 11.1.7 (2018-09-26)
+
+### Security (6 changes)
+
+- Redact confidential events in the API.
+- Set timeout for syntax highlighting.
+- Sanitize JSON data properly to fix XSS on Issue details page.
+- Fix stored XSS in merge requests from imported repository.
+- Fix xss vulnerability sourced from package.json.
+- Block loopback addresses in UrlBlocker.
+
+
 ## 11.1.6 (2018-08-28)
 
 ### Fixed (1 change)

app/assets/javascripts/issue_show/index.js

 import Vue from 'vue';
+import sanitize from 'sanitize-html';
 import issuableApp from './components/app.vue';
 import '../vue_shared/vue_resource_interceptor';
 
-document.addEventListener('DOMContentLoaded', () => {
+export default function initIssueableApp() {
   const initialDataEl = document.getElementById('js-issuable-app-initial-data');
-  const props = JSON.parse(initialDataEl.innerHTML.replace(/"/g, '"'));
+  const props = JSON.parse(sanitize(initialDataEl.textContent).replace(/"/g, '"'));
 
   return new Vue({
     el: document.getElementById('js-issuable-app'),
@@ -17,4 +18,4 @@ document.addEventListener('DOMContentLoaded', () => {
       });
     },
   });
-});
+}

app/assets/javascripts/pages/projects/issues/show.js

@@ -3,9 +3,10 @@ import Issue from '~/issue';
 import ShortcutsIssuable from '~/behaviors/shortcuts/shortcuts_issuable';
 import ZenMode from '~/zen_mode';
 import '~/notes/index';
-import '~/issue_show/index';
+import initIssueableApp from '~/issue_show';
 
 export default function () {
+  initIssueableApp();
   new Issue(); // eslint-disable-line no-new
   new ShortcutsIssuable(); // eslint-disable-line no-new
   new ZenMode(); // eslint-disable-line no-new

app/controllers/projects/merge_requests/creations_controller.rb

@@ -106,6 +106,10 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
     @commits = set_commits_for_rendering(@merge_request.commits)
     @commit = @merge_request.diff_head_commit
 
+    # FIXME: We have to assign a presenter to another instance variable
+    # due to class_name checks being made with issuable classes
+    @mr_presenter = @merge_request.present(current_user: current_user)
+
     @labels = LabelsFinder.new(current_user, project_id: @project.id).execute
 
     set_pipeline_variables

app/controllers/projects/merge_requests_controller.rb

@@ -333,6 +333,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     @target_project = @merge_request.target_project
     @target_branches = @merge_request.target_project.repository.branch_names
     @noteable = @merge_request
+
+    # FIXME: We have to assign a presenter to another instance variable
+    # due to class_name checks being made with issuable classes
+    @mr_presenter = @merge_request.present(current_user: current_user)
   end
 
   def finder_type

app/controllers/projects_controller.rb

@@ -16,6 +16,7 @@ class ProjectsController < Projects::ApplicationController
   before_action :tree, only: [:show], if: [:repo_exists?, :project_view_files?]
   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]
 
   # Authorize
   before_action :authorize_admin_project!, only: [:edit, :update, :housekeeping, :download_export, :export, :remove_export, :generate_new_export]
@@ -433,4 +434,8 @@ class ProjectsController < Projects::ApplicationController
   def whitelist_query_limiting
     Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42440')
   end
+
+  def present_project
+    @project = @project.present(current_user: current_user)
+  end
 end

app/finders/events_finder.rb

@@ -12,6 +12,7 @@ class EventsFinder
   # Arguments:
   #   source - which user or project to looks for events on
   #   current_user - only return events for projects visible to this user
+  #                  WARNING: does not consider project feature visibility!
   #   params:
   #     action: string
   #     target_type: string

app/finders/joined_groups_finder.rb

 # frozen_string_literal: true
 
-class JoinedGroupsFinder < UnionFinder
+class JoinedGroupsFinder
   def initialize(user)
     @user = user
   end
@@ -8,19 +8,8 @@ class JoinedGroupsFinder < UnionFinder
   # Finds the groups of the source user, optionally limited to those visible to
   # the current user.
   def execute(current_user = nil)
-    segments = all_groups(current_user)
-
-    find_union(segments, Group).order_id_desc
-  end
-
-  private
-
-  def all_groups(current_user)
-    groups = []
-
-    groups << @user.authorized_groups.visible_to_user(current_user) if current_user
-    groups << @user.authorized_groups.public_to_user(current_user)
-
-    groups
+    @user.authorized_groups
+      .public_or_visible_to_user(current_user)
+      .order_id_desc
   end
 end

app/finders/user_recent_events_finder.rb

@@ -3,6 +3,7 @@
 # Get user activity feed for projects common for a user and a logged in user
 #
 # - current_user: The user viewing the events
+#                 WARNING: does not consider project feature visibility!
 # - user: The user for which to load the events
 # - params:
 #   - offset: The page of events to return

app/mailers/emails/merge_requests.rb

@@ -3,13 +3,14 @@
 module Emails
   module MergeRequests
     def new_merge_request_email(recipient_id, merge_request_id, reason = nil)
-      setup_merge_request_mail(merge_request_id, recipient_id)
+      setup_merge_request_mail(merge_request_id, recipient_id, present: true)
 
       mail_new_thread(@merge_request, merge_request_thread_options(@merge_request.author_id, recipient_id, reason))
     end
 
     def new_mention_in_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil)
-      setup_merge_request_mail(merge_request_id, recipient_id)
+      setup_merge_request_mail(merge_request_id, recipient_id, present: true)
+
       mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
     end
 
@@ -75,11 +76,16 @@ module Emails
 
     private
 
-    def setup_merge_request_mail(merge_request_id, recipient_id)
+    def setup_merge_request_mail(merge_request_id, recipient_id, present: false)
       @merge_request = MergeRequest.find(merge_request_id)
       @project = @merge_request.project
       @target_url = project_merge_request_url(@project, @merge_request)
 
+      if present
+        recipient = User.find(recipient_id)
+        @mr_presenter = @merge_request.present(current_user: recipient)
+      end
+
       @sent_notification = SentNotification.record(@merge_request, recipient_id, reply_key)
     end
 

app/models/blob_viewer/package_json.rb

@@ -33,7 +33,8 @@ module BlobViewer
     end
 
     def homepage
-      json_data['homepage']
+      url = json_data['homepage']
+      url if Gitlab::UrlSanitizer.valid?(url)
     end
 
     def npm_url

app/models/event.rb

@@ -148,6 +148,8 @@ class Event < ActiveRecord::Base
     end
   end
 
+  # rubocop:disable Metrics/CyclomaticComplexity
+  # rubocop:disable Metrics/PerceivedComplexity
   def visible_to_user?(user = nil)
     if push? || commit_note?
       Ability.allowed?(user, :download_code, project)
@@ -159,12 +161,18 @@ class Event < ActiveRecord::Base
       Ability.allowed?(user, :read_issue, note? ? note_target : target)
     elsif merge_request? || merge_request_note?
       Ability.allowed?(user, :read_merge_request, note? ? note_target : target)
+    elsif personal_snippet_note?
+      Ability.allowed?(user, :read_personal_snippet, note_target)
+    elsif project_snippet_note?
+      Ability.allowed?(user, :read_project_snippet, note_target)
     elsif milestone?
-      Ability.allowed?(user, :read_project, project)
+      Ability.allowed?(user, :read_milestone, project)
     else
       false # No other event types are visible
     end
   end
+  # rubocop:enable Metrics/PerceivedComplexity
+  # rubocop:enable Metrics/CyclomaticComplexity
 
   def project_name
     if project
@@ -306,6 +314,10 @@ class Event < ActiveRecord::Base
     note? && target && target.for_snippet?
   end
 
+  def personal_snippet_note?
+    note? && target && target.for_personal_snippet?
+  end
+
   def note_target
     target.noteable
   end

app/models/group.rb

@@ -82,8 +82,17 @@ class Group < Namespace
       User.reference_pattern
     end
 
-    def visible_to_user(user)
-      where(id: user.authorized_groups.select(:id).reorder(nil))
+    # WARNING: This method should never be used on its own
+    # please do make sure the number of rows you are filtering is small
+    # enough for this query
+    def public_or_visible_to_user(user)
+      return public_to_user unless user
+
+      public_for_user = public_to_user_arel(user)
+      visible_for_user = visible_to_user_arel(user)
+      public_or_visible = public_for_user.or(visible_for_user)
+
+      where(public_or_visible)
     end
 
     def select_for_project_authorization
@@ -95,6 +104,23 @@ class Group < Namespace
         super
       end
     end
+
+    private
+
+    def public_to_user_arel(user)
+      self.arel_table[:visibility_level]
+        .in(Gitlab::VisibilityLevel.levels_for_user(user))
+    end
+
+    def visible_to_user_arel(user)
+      groups_table = self.arel_table
+      authorized_groups = user.authorized_groups.as('authorized')
+
+      groups_table.project(1)
+        .from(authorized_groups)
+        .where(authorized_groups[:id].eq(groups_table[:id]))
+        .exists
+    end
   end
 
   # Overrides notification_settings has_many association

app/models/merge_request.rb

@@ -6,6 +6,7 @@ class MergeRequest < ActiveRecord::Base
   include Issuable
   include Noteable
   include Referable
+  include Presentable
   include IgnorableColumn
   include TimeTrackable
   include ManualInverseAssociation

app/models/user.rb

@@ -674,10 +674,12 @@ class User < ActiveRecord::Base
 
   # Returns the groups a user has access to, either through a membership or a project authorization
   def authorized_groups
-    Group.from_union([
-      groups,
-      authorized_projects.joins(:namespace).select('namespaces.*')
-    ])
+    Group.unscoped do
+      Group.from_union([
+        groups,
+        authorized_projects.joins(:namespace).select('namespaces.*')
+      ])
+    end
   end
 
   # Returns the groups a user is a member of, either directly or through a parent group

app/serializers/diff_line_entity.rb

@@ -9,6 +9,6 @@ class DiffLineEntity < Grape::Entity
   expose :meta_positions, as: :meta_data
 
   expose :rich_text do |line|
-    line.rich_text || CGI.escapeHTML(line.text)
+    ERB::Util.html_escape(line.rich_text || line.text)
   end
 end

app/services/clusters/applications/check_installation_progress_service.rb

@@ -14,8 +14,8 @@ module Clusters
         else
           check_timeout
         end
-      rescue Kubeclient::HttpError => ke
-        app.make_errored!("Kubernetes error: #{ke.message}") unless app.errored?
+      rescue Kubeclient::HttpError
+        app.make_errored!("Kubernetes error") unless app.errored?
       end
 
       private
@@ -27,7 +27,7 @@ module Clusters
       end
 
       def on_failed
-        app.make_errored!(installation_errors || 'Installation silently failed')
+        app.make_errored!('Installation failed')
       ensure
         remove_installation_pod
       end

app/services/clusters/applications/install_service.rb

@@ -12,10 +12,10 @@ module Clusters
 
           ClusterWaitForAppInstallationWorker.perform_in(
             ClusterWaitForAppInstallationWorker::INTERVAL, app.name, app.id)
-        rescue Kubeclient::HttpError => ke
-          app.make_errored!("Kubernetes error: #{ke.message}")
-        rescue StandardError => e
-          app.make_errored!("Can't start installation process. #{e.message}")
+        rescue Kubeclient::HttpError
+          app.make_errored!("Kubernetes error.")
+        rescue StandardError
+          app.make_errored!("Can't start installation process.")
         end
       end
     end

app/views/notify/new_merge_request_email.html.haml

@@ -9,7 +9,7 @@
   %p
     Assignee: #{@merge_request.assignee_name}
 
-= render_if_exists 'notify/merge_request_approvers', merge_request: @merge_request
+= render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter
 
 - if @merge_request.description
   %div

app/views/notify/new_merge_request_email.text.erb

@@ -5,6 +5,6 @@ New Merge Request <%= @merge_request.to_reference %>
 <%= merge_path_description(@merge_request, 'to') %>
 Author:    <%= @merge_request.author_name %>
 Assignee:  <%= @merge_request.assignee_name %>
-<%= render_if_exists 'notify/merge_request_approvers', merge_request: @merge_request %>
+<%= render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter %>
 
 <%= @merge_request.description %>

app/views/projects/merge_requests/_form.html.haml

 = form_for [@project.namespace.becomes(Namespace), @project, @merge_request],
            html: { class: 'merge-request-form common-note-form js-requires-input js-quick-submit' },
            data: { markdown_version: @merge_request.cached_markdown_version } do |f|
-  = render 'shared/issuable/form', f: f, issuable: @merge_request
+  = render 'shared/issuable/form', f: f, issuable: @merge_request, presenter: @mr_presenter

app/views/projects/merge_requests/creations/_new_submit.html.haml

@@ -11,7 +11,7 @@
     = link_to 'Change branches', mr_change_branches_path(@merge_request)
 %hr
 = form_for [@project.namespace.becomes(Namespace), @project, @merge_request], html: { class: 'merge-request-form common-note-form js-requires-input js-quick-submit' } do |f|
-  = render 'shared/issuable/form', f: f, issuable: @merge_request, commits: @commits
+  = render 'shared/issuable/form', f: f, issuable: @merge_request, commits: @commits, presenter: @mr_presenter
   = f.hidden_field :source_project_id
   = f.hidden_field :source_branch
   = f.hidden_field :target_project_id

app/views/shared/issuable/_form.html.haml

 - form = local_assigns.fetch(:f)
 - commits = local_assigns[:commits]
 - project = @target_project || @project
+- presenter = local_assigns.fetch(:presenter, nil)
 
 = form_errors(issuable)
 
@@ -29,7 +30,7 @@
 
 = render 'shared/issuable/form/metadata', issuable: issuable, form: form
 
-= render_if_exists 'shared/issuable/approvals', issuable: issuable, form: form
+= render_if_exists 'shared/issuable/approvals', issuable: issuable, presenter: presenter, form: form
 
 = render 'shared/issuable/form/branch_chooser', issuable: issuable, form: form
 

changelogs/unreleased/fix-events-finder-incomplete.yml

+---
+title: Redact confidential events in the API
+merge_request:
+author:
+type: security

changelogs/unreleased/security-2697-code-highlight-timeout.yml

+---
+title: Set timeout for syntax highlighting
+merge_request:
+author:
+type: security

changelogs/unreleased/security-acet-issue-details.yml

+---
+title: Sanitize JSON data properly to fix XSS on Issue details page
+merge_request:
+author:
+type: security

changelogs/unreleased/security-fj-stored-xss-in-repository-imports.yml

+---
+title: Fix stored XSS in merge requests from imported repository
+merge_request:
+author:
+type: security

changelogs/unreleased/security-package-json-xss.yml

+---
+title: Fix xss vulnerability sourced from package.json
+merge_request:
+author:
+type: security

lib/api/events.rb

@@ -18,12 +18,27 @@ module API
                         desc: 'Return events sorted in ascending and descending order'
       end
 
+      RedactedEvent = OpenStruct.new(target_title: 'Confidential event').freeze
+
+      def redact_events(events)
+        events.map do |event|
+          if event.visible_to_user?(current_user)
+            event
+          else
+            RedactedEvent
+          end
+        end
+      end
+
       # rubocop: disable CodeReuse/ActiveRecord
-      def present_events(events)
+      def present_events(events, redact: true)
         events = events.reorder(created_at: params[:sort])
                  .with_associations
 
-        present paginate(events), with: Entities::Event
+        events = paginate(events)
+        events = redact_events(events) if redact
+
+        present events, with: Entities::Event
       end
       # rubocop: enable CodeReuse/ActiveRecord
     end
@@ -46,7 +61,8 @@ module API
 
         events = EventsFinder.new(params.merge(source: current_user, current_user: current_user)).execute.preload(:author, :target)
 
-        present_events(events)
+        # Since we're viewing our own events, redaction is unnecessary
+        present_events(events, redact: false)
       end
       # rubocop: enable CodeReuse/ActiveRecord
     end

lib/gitlab/diff/highlight.rb

@@ -24,7 +24,7 @@ module Gitlab
           # ignore highlighting for "match" lines
           next diff_line if diff_line.meta?
 
-          rich_line = highlight_line(diff_line) || diff_line.text
+          rich_line = highlight_line(diff_line) || ERB::Util.html_escape(diff_line.text)
 
           if line_inline_diffs = inline_diffs[i]
             begin

lib/gitlab/highlight.rb

 module Gitlab
   class Highlight
+    TIMEOUT_BACKGROUND = 30.seconds
+    TIMEOUT_FOREGROUND = 3.seconds
+
     def self.highlight(blob_name, blob_content, repository: nil, plain: false)
       new(blob_name, blob_content, repository: repository)
         .highlight(blob_content, continue: false, plain: plain)
@@ -51,11 +54,20 @@ module Gitlab
     end
 
     def highlight_rich(text, continue: true)
-      @formatter.format(lexer.lex(text, continue: continue), tag: lexer.tag).html_safe
+      tag = lexer.tag
+      tokens = lexer.lex(text, continue: continue)
+      Timeout.timeout(timeout_time) { @formatter.format(tokens, tag: tag).html_safe }
+    rescue Timeout::Error => e
+      Gitlab::Sentry.track_exception(e)
+      highlight_plain(text)
     rescue
       highlight_plain(text)
     end
 
+    def timeout_time
+      Sidekiq.server? ? TIMEOUT_BACKGROUND : TIMEOUT_FOREGROUND
+    end
+
     def link_dependencies(text, highlighted_text)
       Gitlab::DependencyLinker.link(blob_name, text, highlighted_text)
     end

rubocop/cop/group_public_or_visible_to_user.rb

+# frozen_string_literal: true
+#
+module RuboCop
+  module Cop
+    # Cop that blacklists the usage of Group.public_or_visible_to_user
+    class GroupPublicOrVisibleToUser < RuboCop::Cop::Cop
+      MSG = '`Group.public_or_visible_to_user` should be used with extreme care. ' \
+        'Please ensure that you are not using it on its own and that the amount ' \
+        'of rows being filtered is reasonable.'
+
+      def_node_matcher :public_or_visible_to_user?, <<~PATTERN
+        (send (const nil? :Group) :public_or_visible_to_user ...)
+      PATTERN
+
+      def on_send(node)
+        return unless public_or_visible_to_user?(node)
+
+        add_offense(node, location: :expression)
+      end
+    end
+  end
+end

rubocop/rubocop.rb

@@ -38,3 +38,4 @@ require_relative 'cop/code_reuse/service_class'
 require_relative 'cop/code_reuse/presenter'
 require_relative 'cop/code_reuse/serializer'
 require_relative 'cop/code_reuse/active_record'
+require_relative 'cop/group_public_or_visible_to_user'

spec/features/issues/issue_detail_spec.rb

@@ -18,6 +18,23 @@ describe 'Issue Detail', :js do
     end
   end
 
+  context 'when issue description has xss snippet' do
+    before do
+      issue.update!(description: '![xss" onload=alert(1);//](a)')
+      sign_in(user)
+      visit project_issue_path(project, issue)
+      wait_for_requests
+    end
+
+    it 'should encode the description to prevent xss issues' do
+      page.within('.issuable-details .detail-page-description') do
+        expect(page).to have_selector('img', count: 1)
+        expect(find('img')['onerror']).to be_nil
+        expect(find('img')['src']).to end_with('/a')
+      end
+    end
+  end
+
   context 'when edited by a user who is later deleted' do
     before do
       sign_in(user)

spec/javascripts/issue_show/index_spec.js

+import initIssueableApp from '~/issue_show';
+
+describe('Issue show index', () => {
+  describe('initIssueableApp', () => {
+    it('should initialize app with no potential XSS attack', () => {
+      const d = document.createElement('div');
+      d.id = 'js-issuable-app-initial-data';
+      d.innerHTML = JSON.stringify({
+        initialDescriptionHtml: '<img src=x onerror=alert(1)>',
+      });
+      document.body.appendChild(d);
+
+      const alertSpy = spyOn(window, 'alert');
+      initIssueableApp();
+
+      expect(alertSpy).not.toHaveBeenCalled();
+    });
+  });
+});

spec/lib/gitlab/diff/highlight_spec.rb

@@ -8,6 +8,20 @@ describe Gitlab::Diff::Highlight do
   let(:diff) { commit.raw_diffs.first }
   let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) }
 
+  shared_examples 'without inline diffs' do
+    let(:code) { '<h2 onmouseover="alert(2)">Test</h2>' }
+
+    before do
+      allow(Gitlab::Diff::InlineDiff).to receive(:for_lines).and_return([])
+      allow_any_instance_of(Gitlab::Diff::Line).to receive(:text).and_return(code)
+    end
+
+    it 'returns html escaped diff text' do
+      expect(subject[1].rich_text).to eq html_escape(code)
+      expect(subject[1].rich_text).to be_html_safe
+    end
+  end
+
   describe '#highlight' do
     context "with a diff file" do
       let(:subject) { described_class.new(diff_file, repository: project.repository).highlight }
@@ -38,6 +52,16 @@ describe Gitlab::Diff::Highlight do
 
         expect(subject[5].rich_text).to eq(code)
       end
+
+      context 'when no diff_refs' do
+        before do
+          allow(diff_file).to receive(:diff_refs).and_return(nil)
+        end
+
+        context 'when no inline diffs' do
+          it_behaves_like 'without inline diffs'
+        end
+      end
     end
 
     context "with diff lines" do
@@ -93,6 +117,10 @@ describe Gitlab::Diff::Highlight do
           expect { subject }. to raise_exception(RangeError)
         end
       end
+
+      context 'when no inline diffs' do
+        it_behaves_like 'without inline diffs'
+      end
     end
   end
 end

spec/lib/gitlab/highlight_spec.rb

@@ -56,5 +56,22 @@ describe Gitlab::Highlight do
 
       described_class.highlight('file.name', 'Contents')
     end
+
+    context 'timeout' do
+      subject { described_class.new('file.name', 'Contents') }
+
+      it 'utilizes timeout for web' do
+        expect(Timeout).to receive(:timeout).with(described_class::TIMEOUT_FOREGROUND).and_call_original
+
+        subject.highlight("Content")
+      end
+
+      it 'utilizes longer timeout for sidekiq' do
+        allow(Sidekiq).to receive(:server?).and_return(true)
+        expect(Timeout).to receive(:timeout).with(described_class::TIMEOUT_BACKGROUND).and_call_original
+
+        subject.highlight("Content")
+      end
+    end
   end
 end

spec/models/blob_viewer/package_json_spec.rb

@@ -40,13 +40,14 @@ describe BlobViewer::PackageJson do
   end
 
   context 'when package.json has "private": true' do
+    let(:homepage) { 'http://example.com' }
     let(:data) do
       <<-SPEC.strip_heredoc
       {
         "name": "module-name",
         "version": "10.3.1",
         "private": true,
-        "homepage": "myawesomepackage.com"
+        "homepage": #{homepage.to_json}
       }
       SPEC
     end
@@ -54,10 +55,22 @@ describe BlobViewer::PackageJson do
     subject { described_class.new(blob) }
 
     describe '#package_url' do
-      it 'returns homepage if any' do
-        expect(subject).to receive(:prepare!)
+      context 'when the homepage has a valid URL' do
+        it 'returns homepage URL' do
+          expect(subject).to receive(:prepare!)
+
+          expect(subject.package_url).to eq(homepage)
+        end
+      end
+
+      context 'when the homepage has an invalid URL' do
+        let(:homepage) { 'javascript:alert()' }
+
+        it 'returns nil' do
+          expect(subject).to receive(:prepare!)
 
-        expect(subject.package_url).to eq('myawesomepackage.com')
+          expect(subject.package_url).to be_nil
+        end
       end
     end
 

spec/models/event_spec.rb

@@ -148,9 +148,14 @@ describe Event do
     let(:admin) { create(:admin) }
     let(:issue) { create(:issue, project: project, author: author, assignees: [assignee]) }
     let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignees: [assignee]) }
+    let(:project_snippet) { create(:project_snippet, :public, project: project, author: author) }
+    let(:personal_snippet) { create(:personal_snippet, :public, author: author) }
     let(:note_on_commit) { create(:note_on_commit, project: project) }
     let(:note_on_issue) { create(:note_on_issue, noteable: issue, project: project) }
     let(:note_on_confidential_issue) { create(:note_on_issue, noteable: confidential_issue, project: project) }
+    let(:note_on_project_snippet) { create(:note_on_project_snippet, author: author, noteable: project_snippet, project: project) }
+    let(:note_on_personal_snippet) { create(:note_on_personal_snippet, author: author, noteable: personal_snippet, project: nil) }
+    let(:milestone_on_project) { create(:milestone, project: project) }
     let(:event) { described_class.new(project: project, target: target, author_id: author.id) }
 
     before do
@@ -268,6 +273,125 @@ describe Event do
         end
       end
     end
+
+    context 'milestone event' do
+      let(:target) { milestone_on_project }
+
+      it do
+        expect(event.visible_to_user?(nil)).to be_truthy
+        expect(event.visible_to_user?(non_member)).to be_truthy
+        expect(event.visible_to_user?(member)).to be_truthy
+        expect(event.visible_to_user?(guest)).to be_truthy
+        expect(event.visible_to_user?(admin)).to be_truthy
+      end
+
+      context 'on public project with private issue tracker and merge requests' do
+        let(:project) { create(:project, :public, :issues_private, :merge_requests_private) }
+
+        it do
+          expect(event.visible_to_user?(nil)).to be_falsy
+          expect(event.visible_to_user?(non_member)).to be_falsy
+          expect(event.visible_to_user?(member)).to be_truthy
+          expect(event.visible_to_user?(guest)).to be_truthy
+          expect(event.visible_to_user?(admin)).to be_truthy
+        end
+      end
+
+      context 'on private project' do
+        let(:project) { create(:project, :private) }
+
+        it do
+          expect(event.visible_to_user?(nil)).to be_falsy
+          expect(event.visible_to_user?(non_member)).to be_falsy
+          expect(event.visible_to_user?(member)).to be_truthy
+          expect(event.visible_to_user?(guest)).to be_truthy
+          expect(event.visible_to_user?(admin)).to be_truthy
+        end
+      end
+    end
+
+    context 'project snippet note event' do
+      let(:target) { note_on_project_snippet }
+
+      it do
+        expect(event.visible_to_user?(nil)).to be_truthy
+        expect(event.visible_to_user?(non_member)).to be_truthy
+        expect(event.visible_to_user?(author)).to be_truthy
+        expect(event.visible_to_user?(member)).to be_truthy
+        expect(event.visible_to_user?(guest)).to be_truthy
+        expect(event.visible_to_user?(admin)).to be_truthy
+      end
+
+      context 'on public project with private snippets' do
+        let(:project) { create(:project, :public, :snippets_private) }
+
+        it do
+          expect(event.visible_to_user?(nil)).to be_falsy
+          expect(event.visible_to_user?(non_member)).to be_falsy
+
+          # Normally, we'd expect the author of a comment to be able to view it.
+          # However, this doesn't seem to be the case for comments on snippets.
+          expect(event.visible_to_user?(author)).to be_falsy
+
+          expect(event.visible_to_user?(member)).to be_truthy
+          expect(event.visible_to_user?(guest)).to be_truthy
+          expect(event.visible_to_user?(admin)).to be_truthy
+        end
+      end
+
+      context 'on private project' do
+        let(:project) { create(:project, :private) }
+
+        it do
+          expect(event.visible_to_user?(nil)).to be_falsy
+          expect(event.visible_to_user?(non_member)).to be_falsy
+
+          # Normally, we'd expect the author of a comment to be able to view it.
+          # However, this doesn't seem to be the case for comments on snippets.
+          expect(event.visible_to_user?(author)).to be_falsy
+
+          expect(event.visible_to_user?(member)).to be_truthy
+          expect(event.visible_to_user?(guest)).to be_truthy
+          expect(event.visible_to_user?(admin)).to be_truthy
+        end
+      end
+    end
+
+    context 'personal snippet note event' do
+      let(:target) { note_on_personal_snippet }
+
+      it do
+        expect(event.visible_to_user?(nil)).to be_truthy
+        expect(event.visible_to_user?(non_member)).to be_truthy
+        expect(event.visible_to_user?(author)).to be_truthy
+        expect(event.visible_to_user?(admin)).to be_truthy
+      end
+
+      context 'on internal snippet' do
+        let(:personal_snippet) { create(:personal_snippet, :internal, author: author) }
+
+        it do
+          expect(event.visible_to_user?(nil)).to be_falsy
+          expect(event.visible_to_user?(non_member)).to be_truthy
+          expect(event.visible_to_user?(author)).to be_truthy
+          expect(event.visible_to_user?(admin)).to be_truthy
+        end
+      end
+
+      context 'on private snippet' do
+        let(:personal_snippet) { create(:personal_snippet, :private, author: author) }
+
+        it do
+          expect(event.visible_to_user?(nil)).to be_falsy
+          expect(event.visible_to_user?(non_member)).to be_falsy
+          expect(event.visible_to_user?(author)).to be_truthy
+
+          # It is very unexpected that a private personal snippet is not visible
+          # to an instance administrator. This should be fixed in the future.
+          expect(event.visible_to_user?(admin)).to be_falsy
+        end
+      end
+    end
   end
 
   describe '.limit_recent' do

spec/models/group_spec.rb

@@ -169,22 +169,42 @@ describe Group do
     end
   end
 
-  describe '.visible_to_user' do
-    let!(:group) { create(:group) }
-    let!(:user)  { create(:user) }
+  describe '.public_or_visible_to_user' do
+    let!(:private_group)  { create(:group, :private)  }
+    let!(:internal_group) { create(:group, :internal) }
 
-    subject { described_class.visible_to_user(user) }
+    subject { described_class.public_or_visible_to_user(user) }
 
-    describe 'when the user has access to a group' do
-      before do
-        group.add_user(user, Gitlab::Access::MAINTAINER)
-      end
+    context 'when user is nil' do
+      let!(:user) { nil }
 
-      it { is_expected.to eq([group]) }
+      it { is_expected.to match_array([group]) }
     end
 
-    describe 'when the user does not have access to any groups' do
-      it { is_expected.to eq([]) }
+    context 'when user' do
+      let!(:user) { create(:user) }
+
+      context 'when user does not have access to any private group' do
+        it { is_expected.to match_array([internal_group, group]) }
+      end
+
+      context 'when user is a member of private group' do
+        before do
+          private_group.add_user(user, Gitlab::Access::DEVELOPER)
+        end
+
+        it { is_expected.to match_array([private_group, internal_group, group]) }
+      end
+
+      context 'when user is a member of private subgroup', :postgresql do
+        let!(:private_subgroup) { create(:group, :private, parent: private_group) }
+
+        before do
+          private_subgroup.add_user(user, Gitlab::Access::DEVELOPER)
+        end
+
+        it { is_expected.to match_array([private_subgroup, internal_group, group]) }
+      end
     end
   end
 

spec/requests/api/groups_spec.rb

@@ -155,7 +155,7 @@ describe API::Groups do
         expect(response).to have_gitlab_http_status(200)
         expect(response).to include_pagination_headers
         expect(json_response).to be_an Array
-        expect(response_groups).to eq(Group.visible_to_user(user1).order(:name).pluck(:name))
+        expect(response_groups).to eq(groups_visible_to_user(user1).order(:name).pluck(:name))
       end
 
       it "sorts in descending order when passed" do
@@ -164,7 +164,7 @@ describe API::Groups do
         expect(response).to have_gitlab_http_status(200)
         expect(response).to include_pagination_headers
         expect(json_response).to be_an Array
-        expect(response_groups).to eq(Group.visible_to_user(user1).order(name: :desc).pluck(:name))
+        expect(response_groups).to eq(groups_visible_to_user(user1).order(name: :desc).pluck(:name))
       end
 
       it "sorts by path in order_by param" do
@@ -173,7 +173,7 @@ describe API::Groups do
         expect(response).to have_gitlab_http_status(200)
         expect(response).to include_pagination_headers
         expect(json_response).to be_an Array
-        expect(response_groups).to eq(Group.visible_to_user(user1).order(:path).pluck(:name))
+        expect(response_groups).to eq(groups_visible_to_user(user1).order(:path).pluck(:name))
       end
 
       it "sorts by id in the order_by param" do
@@ -182,7 +182,7 @@ describe API::Groups do
         expect(response).to have_gitlab_http_status(200)
         expect(response).to include_pagination_headers
         expect(json_response).to be_an Array
-        expect(response_groups).to eq(Group.visible_to_user(user1).order(:id).pluck(:name))
+        expect(response_groups).to eq(groups_visible_to_user(user1).order(:id).pluck(:name))
       end
 
       it "sorts also by descending id with pagination fix" do
@@ -191,7 +191,7 @@ describe API::Groups do
         expect(response).to have_gitlab_http_status(200)
         expect(response).to include_pagination_headers
         expect(json_response).to be_an Array
-        expect(response_groups).to eq(Group.visible_to_user(user1).order(id: :desc).pluck(:name))
+        expect(response_groups).to eq(groups_visible_to_user(user1).order(id: :desc).pluck(:name))
       end
 
       it "sorts identical keys by id for good pagination" do
@@ -211,6 +211,10 @@ describe API::Groups do
         expect(json_response).to be_an Array
         expect(response_groups_ids).to eq(Group.select { |group| group['name'] == 'same-name' }.map { |group| group['id'] }.sort)
       end
+
+      def groups_visible_to_user(user)
+        Group.where(id: user.authorized_groups.select(:id).reorder(nil))
+      end
     end
 
     context 'when using owned in the request' do

spec/requests/api/redacted_events_spec.rb

+require 'spec_helper'
+
+describe 'Redacted events in API::Events' do
+  shared_examples 'private events are redacted' do
+    it 'redacts events the user does not have access to' do
+      expect_any_instance_of(Event).to receive(:visible_to_user?).and_call_original
+
+      get api(path), user
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response).to contain_exactly(
+        'project_id' => nil,
+        'action_name' => nil,
+        'target_id' => nil,
+        'target_iid' => nil,
+        'target_type' => nil,
+        'author_id' => nil,
+        'target_title' => 'Confidential event',
+        'created_at' => nil,
+        'author_username' => nil
+      )
+    end
+  end
+
+  describe '/users/:id/events' do
+    let(:project) { create(:project, :public) }
+    let(:path) { "/users/#{project.owner.id}/events" }
+    let(:issue) { create(:issue, :confidential, project: project) }
+
+    before do
+      EventCreateService.new.open_issue(issue, issue.author)
+    end
+
+    context 'unauthenticated user views another user with private events' do
+      let(:user) { nil }
+
+      include_examples 'private events are redacted'
+    end
+
+    context 'authenticated user without access views another user with private events' do
+      let(:user) { create(:user) }
+
+      include_examples 'private events are redacted'
+    end
+  end
+
+  describe '/projects/:id/events' do
+    let(:project) { create(:project, :public) }
+    let(:path) { "/projects/#{project.id}/events" }
+    let(:issue) { create(:issue, :confidential, project: project) }
+
+    before do
+      EventCreateService.new.open_issue(issue, issue.author)
+    end
+
+    context 'unauthenticated user views public project' do
+      let(:user) { nil }
+
+      include_examples 'private events are redacted'
+    end
+
+    context 'authenticated user without access views public project' do
+      let(:user) { create(:user) }
+
+      include_examples 'private events are redacted'
+    end
+  end
+end

spec/rubocop/cop/group_public_or_visible_to_user_spec.rb

+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../rubocop/cop/group_public_or_visible_to_user'
+
+describe RuboCop::Cop::GroupPublicOrVisibleToUser do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  it 'flags the use of Group.public_or_visible_to_user with a constant receiver' do
+    inspect_source('Group.public_or_visible_to_user')
+
+    expect(cop.offenses.size).to eq(1)
+  end
+
+  it 'does not flat the use of public_or_visible_to_user with a constant that is not Group' do
+    inspect_source('Project.public_or_visible_to_user')
+
+    expect(cop.offenses.size).to eq(0)
+  end
+
+  it 'does not flag the use of Group.public_or_visible_to_user with a send receiver' do
+    inspect_source('foo.public_or_visible_to_user')
+
+    expect(cop.offenses.size).to eq(0)
+  end
+end

spec/serializers/diff_line_entity_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DiffLineEntity do
+  include RepoHelpers
+
+  let(:code) { 'hello world' }
+  let(:line) { Gitlab::Diff::Line.new(code, 'new', 1, nil, 1) }
+  let(:entity) { described_class.new(line, request: {}) }
+
+  subject { entity.as_json }
+
+  it 'exposes correct attributes' do
+    expect(subject).to include(
+      :line_code, :type, :old_line, :new_line, :text, :meta_data, :rich_text
+    )
+  end
+
+  describe '#rich_text' do
+    let(:code) { '<h2 onmouseover="alert(2)">Test</h2>' }
+    let(:rich_text_value) { nil }
+
+    before do
+      line.instance_variable_set(:@rich_text, rich_text_value)
+    end
+
+    shared_examples 'escapes html tags' do
+      it do
+        expect(subject[:rich_text]).to eq html_escape(code)
+        expect(subject[:rich_text]).to be_html_safe
+      end
+    end
+
+    context 'when rich_line is present' do
+      let(:rich_text_value) { code }
+
+      it_behaves_like 'escapes html tags'
+    end
+
+    context 'when rich_line is not present' do
+      it_behaves_like 'escapes html tags'
+    end
+  end
+end

spec/services/clusters/applications/check_installation_progress_service_spec.rb

@@ -82,7 +82,7 @@ describe Clusters::Applications::CheckInstallationProgressService do
         service.execute
 
         expect(application).to be_errored
-        expect(application.status_reason).to eq(errors)
+        expect(application.status_reason).to eq("Installation failed")
       end
     end
 

spec/services/clusters/applications/install_service_spec.rb

@@ -42,7 +42,7 @@ describe Clusters::Applications::InstallService do
         service.execute
 
         expect(application).to be_errored
-        expect(application.status_reason).to match(/kubernetes error:/i)
+        expect(application.status_reason).to match('Kubernetes error.')
       end
     end
 

spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb

@@ -12,6 +12,7 @@ describe 'projects/merge_requests/creations/_new_submit.html.haml' do
     assign(:hidden_commit_count, 0)
     assign(:total_commit_count, merge_request.commits.count)
     assign(:project, merge_request.target_project)
+    assign(:mr_presenter, merge_request.present(current_user: merge_request.author))
 
     allow(view).to receive(:can?).and_return(true)
     allow(view).to receive(:url_for).and_return('#')

spec/views/projects/merge_requests/edit.html.haml_spec.rb

@@ -24,6 +24,7 @@ describe 'projects/merge_requests/edit.html.haml' do
   before do
     assign(:project, project)
     assign(:merge_request, closed_merge_request)
+    assign(:mr_presenter, closed_merge_request.present(current_user: user))
 
     allow(view).to receive(:can?).and_return(true)
     allow(view).to receive(:current_user)