diff options
author | Bob Van Landuyt <bob@vanlanduyt.co> | 2018-10-01 19:49:43 +0300 |
---|---|---|
committer | Bob Van Landuyt <bob@vanlanduyt.co> | 2018-10-01 19:49:43 +0300 |
commit | 1cd07610664ab955c8a044b20c71224594a9a9bb (patch) | |
tree | e96e765ba0a148442efd477221fa9b77628f0f05 | |
parent | c874a481346d0cd83801a510135f29c72fd8d3ae (diff) | |
parent | 7cb9957a33d37394cd884106865e4aedef519e97 (diff) |
Merge remote-tracking branch 'dev/master'
48 files changed, 622 insertions, 70 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index e514a42108c..c9ab8599d99 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/app/assets/javascripts/issue_show/index.js b/app/assets/javascripts/issue_show/index.js index 75dfdedcf1b..d08e8ba0c4b 100644 --- a/app/assets/javascripts/issue_show/index.js +++ b/app/assets/javascripts/issue_show/index.js @@ -1,10 +1,11 @@ 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', () => { }); }, }); -}); +} diff --git a/app/assets/javascripts/pages/projects/issues/show.js b/app/assets/javascripts/pages/projects/issues/show.js index 74b3a515e84..ef65196872c 100644 --- a/app/assets/javascripts/pages/projects/issues/show.js +++ b/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 diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index 86583adc6a4..5639402a1e9 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/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 diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index dfb69de650b..d691744d72a 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/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 diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 7352c5e9bec..a9417369ca2 100644 --- a/app/controllers/projects_controller.rb +++ b/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 diff --git a/app/finders/events_finder.rb b/app/finders/events_finder.rb index fd7aeca0d8b..2e82bda8730 100644 --- a/app/finders/events_finder.rb +++ b/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 diff --git a/app/finders/joined_groups_finder.rb b/app/finders/joined_groups_finder.rb index 18cc6891ca4..4d8128dd824 100644 --- a/app/finders/joined_groups_finder.rb +++ b/app/finders/joined_groups_finder.rb @@ -1,6 +1,6 @@ # 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 diff --git a/app/finders/user_recent_events_finder.rb b/app/finders/user_recent_events_finder.rb index a4daf5b5841..eeca5026da1 100644 --- a/app/finders/user_recent_events_finder.rb +++ b/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 diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index 67af0a4eb98..be085496731 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/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 diff --git a/app/models/blob_viewer/package_json.rb b/app/models/blob_viewer/package_json.rb index d12dd93ce2e..7cae60a74d6 100644 --- a/app/models/blob_viewer/package_json.rb +++ b/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 diff --git a/app/models/event.rb b/app/models/event.rb index 596155a9525..2e690f8c013 100644 --- a/app/models/event.rb +++ b/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 diff --git a/app/models/group.rb b/app/models/group.rb index 62af20d2142..612c546ca57 100644 --- a/app/models/group.rb +++ b/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 diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index dd5d494997d..0481a4a3d28 100644 --- a/app/models/merge_request.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index eeac87e2e52..cd3b1c95b7e 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/serializers/diff_line_entity.rb b/app/serializers/diff_line_entity.rb index 2119a1017d3..942714b7787 100644 --- a/app/serializers/diff_line_entity.rb +++ b/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 diff --git a/app/services/clusters/applications/check_installation_progress_service.rb b/app/services/clusters/applications/check_installation_progress_service.rb index 35f5cff0e0c..5017fa093f3 100644 --- a/app/services/clusters/applications/check_installation_progress_service.rb +++ b/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 diff --git a/app/services/clusters/applications/install_service.rb b/app/services/clusters/applications/install_service.rb index 7e3c0e77a83..dd8d2ed5eb6 100644 --- a/app/services/clusters/applications/install_service.rb +++ b/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 diff --git a/app/views/notify/new_merge_request_email.html.haml b/app/views/notify/new_merge_request_email.html.haml index dd6a84e503d..5acd45b74a7 100644 --- a/app/views/notify/new_merge_request_email.html.haml +++ b/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 diff --git a/app/views/notify/new_merge_request_email.text.erb b/app/views/notify/new_merge_request_email.text.erb index d5b8f8d764f..754f4bca1cd 100644 --- a/app/views/notify/new_merge_request_email.text.erb +++ b/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 %> diff --git a/app/views/projects/merge_requests/_form.html.haml b/app/views/projects/merge_requests/_form.html.haml index 5a59f956cb5..13b967beba1 100644 --- a/app/views/projects/merge_requests/_form.html.haml +++ b/app/views/projects/merge_requests/_form.html.haml @@ -1,4 +1,4 @@ = 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 diff --git a/app/views/projects/merge_requests/creations/_new_submit.html.haml b/app/views/projects/merge_requests/creations/_new_submit.html.haml index d5c4134dee2..464f8fa65e9 100644 --- a/app/views/projects/merge_requests/creations/_new_submit.html.haml +++ b/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 diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index 5b28a43a361..b33c758b464 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -1,6 +1,7 @@ - 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 diff --git a/changelogs/unreleased/fix-events-finder-incomplete.yml b/changelogs/unreleased/fix-events-finder-incomplete.yml new file mode 100644 index 00000000000..f3a4e421d33 --- /dev/null +++ b/changelogs/unreleased/fix-events-finder-incomplete.yml @@ -0,0 +1,5 @@ +--- +title: Redact confidential events in the API +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-2697-code-highlight-timeout.yml b/changelogs/unreleased/security-2697-code-highlight-timeout.yml new file mode 100644 index 00000000000..66ad9ff822b --- /dev/null +++ b/changelogs/unreleased/security-2697-code-highlight-timeout.yml @@ -0,0 +1,5 @@ +--- +title: Set timeout for syntax highlighting +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-acet-issue-details.yml b/changelogs/unreleased/security-acet-issue-details.yml new file mode 100644 index 00000000000..64147a9d6e8 --- /dev/null +++ b/changelogs/unreleased/security-acet-issue-details.yml @@ -0,0 +1,5 @@ +--- +title: Sanitize JSON data properly to fix XSS on Issue details page +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-fj-stored-xss-in-repository-imports.yml b/changelogs/unreleased/security-fj-stored-xss-in-repository-imports.yml new file mode 100644 index 00000000000..7520aa624c7 --- /dev/null +++ b/changelogs/unreleased/security-fj-stored-xss-in-repository-imports.yml @@ -0,0 +1,5 @@ +--- +title: Fix stored XSS in merge requests from imported repository +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-package-json-xss.yml b/changelogs/unreleased/security-package-json-xss.yml new file mode 100644 index 00000000000..6ab4854e44f --- /dev/null +++ b/changelogs/unreleased/security-package-json-xss.yml @@ -0,0 +1,5 @@ +--- +title: Fix xss vulnerability sourced from package.json +merge_request: +author: +type: security diff --git a/lib/api/events.rb b/lib/api/events.rb index bca4db1a80e..6e0b508be19 100644 --- a/lib/api/events.rb +++ b/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 diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index 1f012043e56..a605ddb5c33 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/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 diff --git a/lib/gitlab/highlight.rb b/lib/gitlab/highlight.rb index 5408a1a6838..0b6cc893db1 100644 --- a/lib/gitlab/highlight.rb +++ b/lib/gitlab/highlight.rb @@ -1,5 +1,8 @@ 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 diff --git a/rubocop/cop/group_public_or_visible_to_user.rb b/rubocop/cop/group_public_or_visible_to_user.rb new file mode 100644 index 00000000000..beda0b7f8ba --- /dev/null +++ b/rubocop/cop/group_public_or_visible_to_user.rb @@ -0,0 +1,22 @@ +# 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 diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index ff929c7b6ce..76d6037706e 100644 --- a/rubocop/rubocop.rb +++ b/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' diff --git a/spec/features/issues/issue_detail_spec.rb b/spec/features/issues/issue_detail_spec.rb index 088ab114df3..76bc93e9766 100644 --- a/spec/features/issues/issue_detail_spec.rb +++ b/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) diff --git a/spec/javascripts/issue_show/index_spec.js b/spec/javascripts/issue_show/index_spec.js new file mode 100644 index 00000000000..fa0b426c06c --- /dev/null +++ b/spec/javascripts/issue_show/index_spec.js @@ -0,0 +1,19 @@ +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(); + }); + }); +}); diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb index 3c8cf9c56cc..5d0a603d11d 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/highlight_spec.rb b/spec/lib/gitlab/highlight_spec.rb index 29e61d15726..88f7099ff3c 100644 --- a/spec/lib/gitlab/highlight_spec.rb +++ b/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 diff --git a/spec/models/blob_viewer/package_json_spec.rb b/spec/models/blob_viewer/package_json_spec.rb index 5ed2f4400bc..fbaa8d47a71 100644 --- a/spec/models/blob_viewer/package_json_spec.rb +++ b/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 diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index c1eac4fa489..81748681528 100644 --- a/spec/models/event_spec.rb +++ b/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 diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 0729eb99e78..1bf8f89e126 100644 --- a/spec/models/group_spec.rb +++ b/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 diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 3a8948f8477..3802b5c6848 100644 --- a/spec/requests/api/groups_spec.rb +++ b/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 diff --git a/spec/requests/api/redacted_events_spec.rb b/spec/requests/api/redacted_events_spec.rb new file mode 100644 index 00000000000..086dd3df9ba --- /dev/null +++ b/spec/requests/api/redacted_events_spec.rb @@ -0,0 +1,68 @@ +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 diff --git a/spec/rubocop/cop/group_public_or_visible_to_user_spec.rb b/spec/rubocop/cop/group_public_or_visible_to_user_spec.rb new file mode 100644 index 00000000000..7b5235a3da7 --- /dev/null +++ b/spec/rubocop/cop/group_public_or_visible_to_user_spec.rb @@ -0,0 +1,28 @@ +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 diff --git a/spec/serializers/diff_line_entity_spec.rb b/spec/serializers/diff_line_entity_spec.rb new file mode 100644 index 00000000000..2549f64bcd3 --- /dev/null +++ b/spec/serializers/diff_line_entity_spec.rb @@ -0,0 +1,45 @@ +# 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 diff --git a/spec/services/clusters/applications/check_installation_progress_service_spec.rb b/spec/services/clusters/applications/check_installation_progress_service_spec.rb index 986f11410fd..1a565bb734d 100644 --- a/spec/services/clusters/applications/check_installation_progress_service_spec.rb +++ b/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 diff --git a/spec/services/clusters/applications/install_service_spec.rb b/spec/services/clusters/applications/install_service_spec.rb index a744ec30b65..4bd19f5bd79 100644 --- a/spec/services/clusters/applications/install_service_spec.rb +++ b/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 diff --git a/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb b/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb index 8befae39d3a..0206928a211 100644 --- a/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb +++ b/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('#') diff --git a/spec/views/projects/merge_requests/edit.html.haml_spec.rb b/spec/views/projects/merge_requests/edit.html.haml_spec.rb index 9b74a7e1946..c13eab30054 100644 --- a/spec/views/projects/merge_requests/edit.html.haml_spec.rb +++ b/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) |