Commit e50f4bc0 authored by Bob Van Landuyt's avatar Bob Van Landuyt

The dynamic path validator can block out partial paths

So we can block `objects` only when it is contained in `info/lfs` or `gitlab-lfs`
parent ab5f9027
...@@ -71,40 +71,42 @@ class DynamicPathValidator < ActiveModel::EachValidator ...@@ -71,40 +71,42 @@ class DynamicPathValidator < ActiveModel::EachValidator
# without tree as reserved name routing can match 'group/project' as group name, # without tree as reserved name routing can match 'group/project' as group name,
# 'tree' as project name and 'deploy_keys' as route. # 'tree' as project name and 'deploy_keys' as route.
# #
WILDCARD_ROUTES = Set.new(%w[tree commits wikis new edit create update logs_tree WILDCARD_ROUTES = Set.new(%w[tree commits wikis new edit create update logs_tree
preview blob blame raw files create_dir find_file preview blob blame raw files create_dir find_file
artifacts graphs refs badges objects folders file]) artifacts graphs refs badges info/lfs/objects
gitlab-lfs/objects environments/folders])
STRICT_RESERVED = (TOP_LEVEL_ROUTES | WILDCARD_ROUTES).freeze STRICT_RESERVED = (TOP_LEVEL_ROUTES | WILDCARD_ROUTES).freeze
def self.valid_full_path?(full_path) def self.valid?(path)
path_segments = full_path.split('/') path_segments = path.split('/')
root_segment = path_segments.shift
valid?(root_segment, type: :top_level) && valid_wildcard_segments?(path_segments) !reserved?(path) && path_segments.all? { |value| follow_format?(value) }
end end
def self.valid_wildcard_segments?(segments) def self.reserved?(path)
segments.all? { |segment| valid?(segment, type: :wildcard) } path = path.to_s.downcase
top_level, wildcard_part = path.split('/', 2)
includes_reserved_top_level?(top_level) || includes_reserved_wildcard?(wildcard_part)
end end
def self.valid?(value, type: :strict) def self.includes_reserved_wildcard?(path)
!reserved?(value, type: type) && follow_format?(value) WILDCARD_ROUTES.any? do |reserved_word|
contains_path_part?(path, reserved_word)
end
end end
def self.reserved?(value, type: :strict) def self.includes_reserved_top_level?(path)
value = value.to_s.downcase TOP_LEVEL_ROUTES.any? do |reserved_route|
case type contains_path_part?(path, reserved_route)
when :wildcard
WILDCARD_ROUTES.include?(value)
when :top_level
TOP_LEVEL_ROUTES.include?(value)
else
STRICT_RESERVED.include?(value)
end end
end end
def self.contains_path_part?(path, part)
path =~ /(.*\/|\A)#{Regexp.quote(part)}(\/.*|\z)/
end
def self.follow_format?(value) def self.follow_format?(value)
value =~ Gitlab::Regex.namespace_regex value =~ Gitlab::Regex.namespace_regex
end end
...@@ -116,21 +118,10 @@ class DynamicPathValidator < ActiveModel::EachValidator ...@@ -116,21 +118,10 @@ class DynamicPathValidator < ActiveModel::EachValidator
record.errors.add(attribute, Gitlab::Regex.namespace_regex_message) record.errors.add(attribute, Gitlab::Regex.namespace_regex_message)
end end
if reserved?(value, type: validation_type(record)) full_path = record.respond_to?(:full_path) ? record.full_path : value
record.errors.add(attribute, "#{value} is a reserved name")
end
end
def validation_type(record) if reserved?(full_path)
case record record.errors.add(attribute, "#{value} is a reserved name")
when Namespace
record.has_parent? ? :wildcard : :top_level
when Project
:wildcard
when User
:top_level
else
:strict
end end
end end
end end
...@@ -2,7 +2,7 @@ class GroupUrlConstrainer ...@@ -2,7 +2,7 @@ class GroupUrlConstrainer
def matches?(request) def matches?(request)
id = request.params[:id] id = request.params[:id]
return false unless DynamicPathValidator.valid_full_path?(id) return false unless DynamicPathValidator.valid?(id)
Group.find_by_full_path(id).present? Group.find_by_full_path(id).present?
end end
......
...@@ -4,7 +4,7 @@ class ProjectUrlConstrainer ...@@ -4,7 +4,7 @@ class ProjectUrlConstrainer
project_path = request.params[:project_id] || request.params[:id] project_path = request.params[:project_id] || request.params[:id]
full_path = namespace_path + '/' + project_path full_path = namespace_path + '/' + project_path
unless DynamicPathValidator.valid_full_path?(full_path) unless DynamicPathValidator.valid?(full_path)
return false return false
end end
......
...@@ -34,6 +34,13 @@ describe Namespace, models: true do ...@@ -34,6 +34,13 @@ describe Namespace, models: true do
let(:group) { build(:group, :nested, path: 'tree') } let(:group) { build(:group, :nested, path: 'tree') }
it { expect(group).not_to be_valid } it { expect(group).not_to be_valid }
it 'rejects nested paths' do
parent = create(:group, :nested, path: 'environments')
namespace = build(:project, path: 'folders', namespace: parent)
expect(namespace).not_to be_valid
end
end end
context 'top-level group' do context 'top-level group' do
......
...@@ -266,6 +266,13 @@ describe Project, models: true do ...@@ -266,6 +266,13 @@ describe Project, models: true do
expect(project).not_to be_valid expect(project).not_to be_valid
end end
it 'rejects nested paths' do
parent = create(:group, :nested, path: 'environments')
project = build(:project, path: 'folders', namespace: parent)
expect(project).not_to be_valid
end
end end
end end
......
...@@ -12,23 +12,30 @@ describe DynamicPathValidator do ...@@ -12,23 +12,30 @@ describe DynamicPathValidator do
# Pass in a full path and get the last segment before a wildcard # Pass in a full path and get the last segment before a wildcard
# That's not a parameter # That's not a parameter
# `/*namespace_id/:project_id/builds/artifacts/*ref_name_and_path` # `/*namespace_id/:project_id/builds/artifacts/*ref_name_and_path`
# -> 'artifacts' # -> 'builds/artifacts'
def segment_before_last_wildcard(path) def path_between_wildcards(path)
path_segments = path.split('/').reject { |segment| segment.empty? } path = path.gsub(STARTING_WITH_NAMESPACE, "")
last_wildcard_index = path_segments.rindex { |part| part.starts_with?('*') } path_segments = path.split('/').reject(&:empty?)
wildcard_index = path_segments.index { |segment| segment.starts_with?('*') }
index_of_non_param_segment = last_wildcard_index - 1
part_before_wildcard = path_segments[index_of_non_param_segment] segments_before_wildcard = path_segments[0..wildcard_index - 1]
while parameter?(part_before_wildcard)
index_of_non_param_segment -= 1 param_index = segments_before_wildcard.index { |segment| segment.starts_with?(':') }
part_before_wildcard = path_segments[index_of_non_param_segment] if param_index
segments_before_wildcard = segments_before_wildcard[param_index + 1..-1]
end end
part_before_wildcard segments_before_wildcard.join('/')
end end
def parameter?(path_segment) # If the path is reserved. Then no conflicting paths can# be created for any
path_segment.starts_with?(':') || path_segment.starts_with?('*') # route using this reserved word.
#
# Both `builds/artifacts` & `artifacts/file` are covered by reserving the word
# `artifacts`
def wildcards_include?(path)
described_class::WILDCARD_ROUTES.include?(path) ||
path.split('/').any? { |segment| described_class::WILDCARD_ROUTES.include?(segment) }
end end
let(:all_routes) do let(:all_routes) do
...@@ -42,14 +49,20 @@ describe DynamicPathValidator do ...@@ -42,14 +49,20 @@ describe DynamicPathValidator do
# all routes not starting with a param # all routes not starting with a param
let(:routes_not_starting_in_wildcard) { routes_without_format.select { |p| p !~ %r{^/[:*]} } } let(:routes_not_starting_in_wildcard) { routes_without_format.select { |p| p !~ %r{^/[:*]} } }
let(:top_level_words) do
routes_not_starting_in_wildcard.map do |route|
route.split('/')[1]
end.compact.uniq
end
# All routes that start with a namespaced path, that have 1 or more # All routes that start with a namespaced path, that have 1 or more
# path-segments before having another wildcard parameter. # path-segments before having another wildcard parameter.
# - Starting with paths: # - Starting with paths:
# - `/*namespace_id/:project_id/` # - `/*namespace_id/:project_id/`
# - `/*namespace_id/:id/` # - `/*namespace_id/:id/`
# - Followed by one or more path-parts not starting with `:` or `/` # - Followed by one or more path-parts not starting with `:` or `*`
# - Followed by a path-part that includes a wildcard parameter `*` # - Followed by a path-part that includes a wildcard parameter `*`
# At the time of writing these routes match: http://rubular.com/r/QDxulzZlxZ # At the time of writing these routes match: http://rubular.com/r/Rv2pDE5Dvw
STARTING_WITH_NAMESPACE = /^\/\*namespace_id\/:(project_)?id/ STARTING_WITH_NAMESPACE = /^\/\*namespace_id\/:(project_)?id/
NON_PARAM_PARTS = /[^:*][a-z\-_\/]*/ NON_PARAM_PARTS = /[^:*][a-z\-_\/]*/
ANY_OTHER_PATH_PART = /[a-z\-_\/:]*/ ANY_OTHER_PATH_PART = /[a-z\-_\/:]*/
...@@ -60,82 +73,89 @@ describe DynamicPathValidator do ...@@ -60,82 +73,89 @@ describe DynamicPathValidator do
end end
end end
# This will return all paths that are used in a namespaced route
# before another wildcard path:
#
# /*namespace_id/:project_id/builds/artifacts/*ref_name_and_path
# /*namespace_id/:project_id/info/lfs/objects/*oid
# /*namespace_id/:project_id/commits/*id
# /*namespace_id/:project_id/builds/:build_id/artifacts/file/*path
# -> ['builds/artifacts', 'info/lfs/objects', 'commits', 'artifacts/file']
let(:all_wildcard_paths) do
namespaced_wildcard_routes.map do |route|
path_between_wildcards(route)
end.uniq
end
describe 'TOP_LEVEL_ROUTES' do describe 'TOP_LEVEL_ROUTES' do
it 'includes all the top level namespaces' do it 'includes all the top level namespaces' do
top_level_words = routes_not_starting_in_wildcard.
map { |p| p.split('/')[1] }. # Get the first part of the path
compact.
uniq
expect(described_class::TOP_LEVEL_ROUTES).to include(*top_level_words) expect(described_class::TOP_LEVEL_ROUTES).to include(*top_level_words)
end end
end end
describe 'WILDCARD_ROUTES' do describe 'WILDCARD_ROUTES' do
it 'includes all paths that can be used after a namespace/project path' do it 'includes all paths that can be used after a namespace/project path' do
all_wildcard_paths = namespaced_wildcard_routes.map do |path| aggregate_failures do
segment_before_last_wildcard(path) all_wildcard_paths.each do |path|
end.uniq expect(wildcards_include?(path)).to be(true), "Expected #{path} to be rejected"
end
expect(described_class::WILDCARD_ROUTES).to include(*all_wildcard_paths) end
end end
end end
describe ".valid?" do describe '.contains_path_part?' do
it 'is not case sensitive' do it 'recognizes a path part' do
expect(described_class.valid?("Users", type: :top_level)).to be(false) expect(described_class.contains_path_part?('user/some/path', 'user')).
to be_truthy
end end
end
describe '.valid_full_path' do
it "isn't valid when the top level is reserved" do
test_path = 'u/should-be-a/reserved-word'
expect(described_class.valid_full_path?(test_path)).to be(false) it 'recognizes a path ending in the path part' do
expect(described_class.contains_path_part?('some/path/user', 'user')).
to be_truthy
end end
it "isn't valid if any of the path segments is reserved" do it 'skips partial path matchies' do
test_path = 'the-wildcard/wikis/is-a-reserved-path' expect(described_class.contains_path_part?('some/user1/path', 'user')).
to be_falsy
end
expect(described_class.valid_full_path?(test_path)).to be(false) it 'can recognise combined paths' do
expect(described_class.contains_path_part?('some/user/admin/path', 'user/admin')).
to be_truthy
end end
it "is valid if the path doen't contain reserved words" do it 'only recognizes full paths' do
test_path = 'there-are/no-wildcards/in-this-path' expect(described_class.contains_path_part?('frontend-fixtures', 's')).
to be_falsy
end
expect(described_class.valid_full_path?(test_path)).to be(true) it 'handles regex chars gracefully' do
expect(described_class.contains_path_part?('frontend-fixtures', '-')).
to be_falsy
end end
end end
describe '#validation_type' do describe ".valid?" do
it 'uses top level validation for groups without parent' do it 'is not case sensitive' do
group = build(:group) expect(described_class.valid?("Users")).to be(false)
type = validator.validation_type(group)
expect(type).to eq(:top_level)
end end
it 'uses wildcard validation for groups with a parent' do it "isn't valid when the top level is reserved" do
group = build(:group, parent: create(:group)) test_path = 'u/should-be-a/reserved-word'
type = validator.validation_type(group)
expect(type).to eq(:wildcard) expect(described_class.valid?(test_path)).to be(false)
end end
it 'uses wildcard validation for a project' do it "isn't valid if any of the path segments is reserved" do
project = build(:project) test_path = 'the-wildcard/wikis/is-not-allowed'
type = validator.validation_type(project)
expect(type).to eq(:wildcard) expect(described_class.valid?(test_path)).to be(false)
end end
it 'uses strict validation for everything else' do it "is valid if the path doesn't contain reserved words" do
type = validator.validation_type(double) test_path = 'there-are/no-wildcards/in-this-path'
expect(type).to eq(:strict) expect(described_class.valid?(test_path)).to be(true)
end end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment