Commit f2f06b50 authored by GitLab Release Tools Bot's avatar GitLab Release Tools Bot

Merge remote-tracking branch 'dev/12-3-stable' into 12-3-stable

parents 50d93f8d 82ff7486
......@@ -2,6 +2,26 @@
documentation](doc/development/changelog.md) for instructions on adding your own
entry.
## 12.3.6
### Security (14 changes)
- Standardize error response when route is missing.
- Do not display project labels that are not visible for user accessing group labels.
- Show cross-referenced label and milestones in issues' activities only to authorized users.
- Analyze incoming GraphQL queries and check for recursion.
- Disallow unprivileged users from commenting on private repository commits.
- Don't allow maintainers of a target project to delete the source branch of a merge request from a fork.
- Require Maintainer permission on group where project is transferred to.
- Don't leak private members in project member autocomplete suggestions.
- Return 404 on LFS request if project doesn't exist.
- Mask sentry auth token in Error Tracking dashboard.
- Fixes a Open Redirect issue in `InternalRedirect`.
- Sanitize search text to prevent XSS.
- Sanitize all wiki markup formats with GitLab sanitization pipelines.
- Fix stored XSS issue for grafana_url.
## 12.3.5
- No changes.
......
......@@ -5,6 +5,7 @@ import fuzzaldrinPlus from 'fuzzaldrin-plus';
import axios from '~/lib/utils/axios_utils';
import flash from '~/flash';
import { __ } from '~/locale';
import sanitize from 'sanitize-html';
// highlight text(awefwbwgtc -> <b>a</b>wefw<b>b</b>wgt<b>c</b> )
const highlighter = function(element, text, matches) {
......@@ -74,7 +75,7 @@ export default class ProjectFindFile {
findFile() {
var result, searchText;
searchText = this.inputElement.val();
searchText = sanitize(this.inputElement.val());
result =
searchText.length > 0 ? fuzzaldrinPlus.filter(this.filePaths, searchText) : this.filePaths;
return this.renderList(result, searchText);
......
......@@ -14,7 +14,7 @@ class ApplicationController < ActionController::Base
include SessionlessAuthentication
include ConfirmEmailWarning
before_action :authenticate_user!
before_action :authenticate_user!, except: [:route_not_found]
before_action :enforce_terms!, if: :should_enforce_terms?
before_action :validate_user_service_ticket!
before_action :check_password_expiration
......@@ -92,7 +92,9 @@ class ApplicationController < ActionController::Base
if current_user
not_found
else
authenticate_user!
store_location_for(:user, request.fullpath) unless request.xhr?
redirect_to new_user_session_path, alert: I18n.t('devise.failure.unauthenticated')
end
end
......
......@@ -6,7 +6,7 @@ module InternalRedirect
def safe_redirect_path(path)
return unless path
# Verify that the string starts with a `/` and a known route character.
return unless path =~ %r{^/[-\w].*$}
return unless path =~ %r{\A/[-\w].*\z}
uri = URI(path)
# Ignore anything path of the redirect except for the path, querystring and,
......
......@@ -34,6 +34,7 @@ module LfsRequest
end
def lfs_check_access!
return render_lfs_not_found unless project
return if download_request? && lfs_download_access?
return if upload_request? && lfs_upload_access?
......
......@@ -51,7 +51,7 @@ class LabelsFinder < UnionFinder
end
label_ids << Label.where(group_id: projects.group_ids)
label_ids << Label.where(project_id: projects.select(:id)) unless only_group_labels?
label_ids << Label.where(project_id: ids_user_can_read_labels(projects)) unless only_group_labels?
end
label_ids
......@@ -188,4 +188,10 @@ class LabelsFinder < UnionFinder
groups.select { |group| authorized_to_read_labels?(group) }
end
end
# rubocop: disable CodeReuse/ActiveRecord
def ids_user_can_read_labels(projects)
Project.where(id: projects.select(:id)).ids_with_issuables_available_for(current_user)
end
# rubocop: enable CodeReuse/ActiveRecord
end
......@@ -18,15 +18,15 @@ class GitlabSchema < GraphQL::Schema
use Gitlab::Graphql::GenericTracing
query_analyzer Gitlab::Graphql::QueryAnalyzers::LoggerAnalyzer.new
query(Types::QueryType)
default_max_page_size 100
query_analyzer Gitlab::Graphql::QueryAnalyzers::RecursionAnalyzer.new
max_complexity DEFAULT_MAX_COMPLEXITY
max_depth DEFAULT_MAX_DEPTH
mutation(Types::MutationType)
query Types::QueryType
mutation Types::MutationType
default_max_page_size 100
class << self
def multiplex(queries, **kwargs)
......
......@@ -133,15 +133,7 @@ module MarkupHelper
issuable_state_filter_enabled: true
)
html =
case wiki_page.format
when :markdown
markdown_unsafe(text, context)
when :asciidoc
asciidoc_unsafe(text)
else
wiki_page.formatted_content.html_safe
end
html = markup_unsafe(wiki_page.path, text, context)
prepare_for_rendering(html, context)
end
......
......@@ -6,6 +6,13 @@ class ApplicationSetting < ApplicationRecord
include TokenAuthenticatable
include ChronicDurationAttribute
GRAFANA_URL_RULES = {
allow_localhost: true,
allow_local_network: true,
enforce_sanitization: true,
require_absolute: false
}.freeze
add_authentication_token_field :runners_registration_token, encrypted: -> { Feature.enabled?(:application_settings_tokens_optional_encryption, default_enabled: true) ? :optional : :required }
add_authentication_token_field :health_check_access_token
add_authentication_token_field :static_objects_external_storage_auth_token
......@@ -48,6 +55,11 @@ class ApplicationSetting < ApplicationRecord
allow_nil: false,
qualified_domain_array: true
validates :grafana_url,
allow_blank: true,
allow_nil: true,
addressable_url: GRAFANA_URL_RULES
validates :session_expire_delay,
presence: true,
numericality: { only_integer: true, greater_than_or_equal_to: 0 }
......@@ -65,7 +77,6 @@ class ApplicationSetting < ApplicationRecord
validates :after_sign_out_path,
allow_blank: true,
addressable_url: true
validates :admin_notification_email,
devise_email: true,
allow_blank: true
......@@ -303,6 +314,14 @@ class ApplicationSetting < ApplicationRecord
current_without_cache
end
def grafana_url
if Gitlab::UrlBlocker.blocked_url?(self[:grafana_url], GRAFANA_URL_RULES)
ApplicationSetting.column_defaults["grafana_url"]
else
self[:grafana_url]
end
end
# By default, the backend is Rails.cache, which uses
# ActiveSupport::Cache::RedisStore. Since loading ApplicationSetting
# can cause a significant amount of load on Redis, let's cache it in
......
......@@ -13,7 +13,9 @@ module Mentionable
def self.other_patterns
[
Commit.reference_pattern,
MergeRequest.reference_pattern
MergeRequest.reference_pattern,
Label.reference_pattern,
Milestone.reference_pattern
]
end
......
......@@ -16,6 +16,7 @@ class Discussion
:commit_id,
:for_commit?,
:for_merge_request?,
:noteable_ability_name,
:to_ability_name,
:editable?,
:visible_for?,
......
......@@ -8,6 +8,7 @@ class Member < ApplicationRecord
include Gitlab::Access
include Presentable
include Gitlab::Utils::StrongMemoize
include FromUnion
attr_accessor :raw_invite_token
......
......@@ -69,6 +69,14 @@ class MergeRequest < ApplicationRecord
has_many :merge_request_assignees
has_many :assignees, class_name: "User", through: :merge_request_assignees
KNOWN_MERGE_PARAMS = [
:auto_merge_strategy,
:should_remove_source_branch,
:force_remove_source_branch,
:commit_message,
:squash_commit_message,
:sha
].freeze
serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize
after_create :ensure_merge_request_diff
......
......@@ -262,6 +262,10 @@ class Milestone < ApplicationRecord
end
alias_method :resource_parent, :parent
def to_ability_name
model_name.singular
end
def group_milestone?
group_id.present?
end
......
......@@ -350,6 +350,10 @@ class Note < ApplicationRecord
end
def to_ability_name
model_name.singular
end
def noteable_ability_name
for_snippet? ? noteable.class.name.underscore : noteable_type.demodulize.underscore
end
......
......@@ -591,11 +591,11 @@ class Project < ApplicationRecord
joins(:namespace).where(namespaces: { type: 'Group' }).select(:namespace_id)
end
# Returns ids of projects with milestones available for given user
# Returns ids of projects with issuables available for given user
#
# Used on queries to find milestones which user can see
# For example: Milestone.where(project_id: ids_with_milestone_available_for(user))
def ids_with_milestone_available_for(user)
# Used on queries to find milestones or labels which user can see
# For example: Milestone.where(project_id: ids_with_issuables_available_for(user))
def ids_with_issuables_available_for(user)
with_issues_enabled = with_issues_available_for_user(user).select(:id)
with_merge_requests_enabled = with_merge_requests_available_for_user(user).select(:id)
......@@ -1242,6 +1242,10 @@ class Project < ApplicationRecord
end
end
def to_ability_name
model_name.singular
end
# rubocop: disable CodeReuse/ServiceClass
def execute_hooks(data, hooks_scope = :push_hooks)
run_after_commit_or_now do
......
......@@ -10,6 +10,7 @@ class SystemNoteMetadata < ApplicationRecord
commit cross_reference
close duplicate
moved merge
label milestone
].freeze
ICON_TYPES = %w[
......
......@@ -138,6 +138,12 @@ class WikiPage
@version ||= @page.version
end
def path
return unless persisted?
@path ||= @page.path
end
def versions(options = {})
return [] unless persisted?
......
......@@ -4,4 +4,5 @@ class CommitPolicy < BasePolicy
delegate { @subject.project }
rule { can?(:download_code) }.enable :read_commit
rule { ~can?(:read_commit) }.prevent :create_note
end
......@@ -128,6 +128,8 @@ class GroupPolicy < BasePolicy
rule { owner | admin }.enable :read_statistics
rule { maintainer & can?(:create_projects) }.enable :transfer_projects
def access_level
return GroupMember::NO_ACCESS if @user.nil?
......
......@@ -15,6 +15,8 @@ class NamespacePolicy < BasePolicy
end
rule { personal_project & ~can_create_personal_project }.prevent :create_projects
rule { (owner | admin) & can?(:create_projects) }.enable :transfer_projects
end
NamespacePolicy.prepend_if_ee('EE::NamespacePolicy')
......@@ -9,7 +9,7 @@ class NotePolicy < BasePolicy
condition(:editable, scope: :subject) { @subject.editable? }
condition(:can_read_noteable) { can?(:"read_#{@subject.to_ability_name}") }
condition(:can_read_noteable) { can?(:"read_#{@subject.noteable_ability_name}") }
condition(:is_visible) { @subject.visible_for?(@user) }
......
......@@ -3,12 +3,13 @@
module AutoMerge
class BaseService < ::BaseService
include Gitlab::Utils::StrongMemoize
include MergeRequests::AssignsMergeParams
def execute(merge_request)
merge_request.merge_params.merge!(params)
assign_allowed_merge_params(merge_request, params.merge(auto_merge_strategy: strategy))
merge_request.auto_merge_enabled = true
merge_request.merge_user = current_user
merge_request.auto_merge_strategy = strategy
return :failed unless merge_request.save
......@@ -21,7 +22,7 @@ module AutoMerge
end
def update(merge_request)
merge_request.merge_params.merge!(params)
assign_allowed_merge_params(merge_request, params.merge(auto_merge_strategy: strategy))
return :failed unless merge_request.save
......
# frozen_string_literal: true
module MergeRequests
module AssignsMergeParams
def self.included(klass)
raise "#{self} can not be included in #{klass} without implementing #current_user" unless klass.method_defined?(:current_user)
end
def assign_allowed_merge_params(merge_request, merge_params)
known_merge_params = merge_params.to_h.with_indifferent_access.slice(*MergeRequest::KNOWN_MERGE_PARAMS)
# Not checking `MergeRequest#can_remove_source_branch` as that includes
# other checks that aren't needed here.
known_merge_params.delete(:force_remove_source_branch) unless current_user.can?(:push_code, merge_request.source_project)
merge_request.merge_params.merge!(known_merge_params)
# Delete the known params now that they're assigned, so we don't try to
# assign them through an `#assign_attributes` later.
# They could be coming in as strings or symbols
merge_params.to_h.with_indifferent_access.except!(*MergeRequest::KNOWN_MERGE_PARAMS)
end
end
end
......@@ -32,7 +32,7 @@ module ErrorTracking
project_slug: 'proj'
)
setting.token = params[:token]
setting.token = token(setting)
setting.enabled = true
end
end
......@@ -40,5 +40,12 @@ module ErrorTracking
def can_read?
can?(current_user, :read_sentry_issue, project)
end
def token(setting)
# Use param token if not masked, otherwise use database token
return params[:token] unless /\A\*+\z/.match?(params[:token])
setting.token
end
end
end
......@@ -2,6 +2,8 @@
module MergeRequests
class BaseService < ::IssuableBaseService
include MergeRequests::AssignsMergeParams
def create_note(merge_request, state = merge_request.state)
SystemNoteService.change_status(merge_request, merge_request.target_project, current_user, state, nil)
end
......@@ -29,6 +31,18 @@ module MergeRequests
private
def create(merge_request)
self.params = assign_allowed_merge_params(merge_request, params)
super
end
def update(merge_request)
self.params = assign_allowed_merge_params(merge_request, params)
super
end
def handle_wip_event(merge_request)
if wip_event = params.delete(:wip_event)
# We update the title that is provided in the params or we use the mr title
......
......@@ -10,13 +10,14 @@ module MergeRequests
# TODO: this should handle all quick actions that don't have side effects
# https://gitlab.com/gitlab-org/gitlab-foss/issues/53658
merge_quick_actions_into_params!(merge_request, only: [:target_branch])
merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) if params.has_key?(:force_remove_source_branch)
# Assign the projects first so we can use policies for `filter_params`
merge_request.author = current_user
merge_request.source_project = find_source_project
merge_request.target_project = find_target_project
self.params = assign_allowed_merge_params(merge_request, params)
filter_params(merge_request)
# merge_request.assign_attributes(...) below is a Rails
......
......@@ -9,7 +9,6 @@ module MergeRequests
merge_request.target_project = @project
merge_request.source_project = @source_project
merge_request.source_branch = params[:source_branch]
merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch)
create(merge_request)
end
......
......@@ -16,10 +16,6 @@ module MergeRequests
params.delete(:force_remove_source_branch)
end
if params.has_key?(:force_remove_source_branch)
merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch)
end
handle_wip_event(merge_request)
update_task_event(merge_request) || update(merge_request)
end
......
......@@ -281,7 +281,7 @@ class NotificationService
end
def send_new_note_notifications(note)
notify_method = "note_#{note.to_ability_name}_email".to_sym
notify_method = "note_#{note.noteable_ability_name}_email".to_sym
recipients = NotificationRecipientService.build_new_note_recipients(note)
recipients.each do |recipient|
......
......@@ -34,15 +34,17 @@ module Projects
organization_slug: settings.dig(:project, :organization_slug)
)
{
params = {
error_tracking_setting_attributes: {
api_url: api_url,
token: settings[:token],
enabled: settings[:enabled],
project_name: settings.dig(:project, :name),
organization_name: settings.dig(:project, :organization_name)
}
}
params[:error_tracking_setting_attributes][:token] = settings[:token] unless /\A\*+\z/.match?(settings[:token]) # Don't update token if we receive masked value
params
end
end
end
......
......@@ -7,16 +7,69 @@ module Projects
def execute(noteable)
@noteable = noteable
participants = noteable_owner + participants_in_noteable + all_members + groups + project_members
participants =
noteable_owner +
participants_in_noteable +
all_members +
groups +
project_members
participants.uniq
end
def project_members
@project_members ||= sorted(project.team.members)
@project_members ||= sorted(get_project_members)
end
def get_project_members
members = Member.from_union([project_members_through_ancestral_groups,
project_members_through_invited_groups,
individual_project_members])
User.id_in(members.select(:user_id))
end
def all_members
[{ username: "all", name: "All Project and Group Members", count: project_members.count }]
end
private
def project_members_through_invited_groups
groups_with_ancestors_ids = Gitlab::ObjectHierarchy
.new(visible_groups)
.base_and_ancestors
.pluck_primary_key
GroupMember
.active_without_invites_and_requests
.with_source_id(groups_with_ancestors_ids)
end
def visible_groups
visible_groups = project.invited_groups
unless project_owner?
visible_groups = visible_groups.public_or_visible_to_user(current_user)
end
visible_groups
end
def project_members_through_ancestral_groups
project.group.present? ? project.group.members_with_parents : Member.none
end
def individual_project_members
project.project_members
end
def project_owner?
if project.group.present?
project.group.owners.include?(current_user)
else
project.namespace.owner == current_user
end
end
end
end
......@@ -98,7 +98,7 @@ module Projects
@new_namespace &&
can?(current_user, :change_namespace, project) &&
@new_namespace.id != project.namespace_id &&
current_user.can?(:create_projects, @new_namespace)
current_user.can?(:transfer_projects, @new_namespace)
end
def update_namespace_and_visibility(to_namespace)
......
......@@ -55,7 +55,8 @@ class AddressableUrlValidator < ActiveModel::EachValidator
ascii_only: false,
enforce_user: false,
enforce_sanitization: false,
dns_rebind_protection: false
dns_rebind_protection: false,
require_absolute: true
}.freeze
DEFAULT_OPTIONS = BLOCKER_VALIDATE_OPTIONS.merge({
......
......@@ -17,4 +17,4 @@
project: error_tracking_setting_project_json,
api_host: setting.api_host,
enabled: setting.enabled.to_json,
token: setting.token } }
token: setting.token.present? ? '*' * 12 : nil } }
......@@ -44,6 +44,10 @@ Admins are able to share projects with any group in the system.
In the example above, the maximum access level of 'Developer' for members from 'Engineering' means that users with higher access levels in 'Engineering' ('Maintainer' or 'Owner') will only have 'Developer' access to 'Project Acme'.
## Sharing public project with private group
When sharing a public project with a private group, owners and maintainers of the project will see the name of the group in the `members` page. Owners will also have the possibility to see members of the private group they don't have access to when mentioning them in the issue or merge request.
## Share project with group lock
It is possible to prevent projects in a group from [sharing
......
# frozen_string_literal: true
# Recursive queries, with relatively low effort, can quickly spiral out of control exponentially
# and may not be picked up by depth and complexity alone.
module Gitlab
module Graphql
module QueryAnalyzers
class RecursionAnalyzer
IGNORED_FIELDS = %w(node edges ofType).freeze
RECURSION_THRESHOLD = 2
def initial_value(query)
{
recurring_fields: {}
}
end
def call(memo, visit_type, irep_node)
return memo if skip_node?(irep_node)
node_name = irep_node.ast_node.name
times_encountered = memo[node_name] || 0
if visit_type == :enter
times_encountered += 1
memo[:recurring_fields][node_name] = times_encountered if recursion_too_deep?(node_name, times_encountered)
else
times_encountered -= 1
end
memo[node_name] = times_encountered
memo
end
def final_value(memo)
recurring_fields = memo[:recurring_fields]
recurring_fields = recurring_fields.select { |k, v| recursion_too_deep?(k, v) }
if recurring_fields.any?
GraphQL::AnalysisError.new("Recursive query - too many of fields '#{recurring_fields}' detected in single branch of the query")
end
end
private
def recursion_too_deep?(node_name, times_encountered)
return if IGNORED_FIELDS.include?(node_name)
times_encountered > recursion_threshold
end
def skip_node?(irep_node)
ast_node = irep_node.ast_node
!ast_node.is_a?(GraphQL::Language::Nodes::Field) || ast_node.selections.empty?
end
def recursion_threshold
RECURSION_THRESHOLD
end
end
end
end
end
......@@ -10,7 +10,7 @@ module Gitlab
def self.render(file_name, input, context)
html = GitHub::Markup.render(file_name, input)
.force_encoding(input.encoding)