From e50f4bc066e4477e9c59708f978383b071dc2959 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 18 Apr 2017 16:27:11 +0200 Subject: 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` --- spec/validators/dynamic_path_validator_spec.rb | 140 ++++++++++++++----------- 1 file changed, 80 insertions(+), 60 deletions(-) (limited to 'spec/validators') diff --git a/spec/validators/dynamic_path_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb index b931a4a2dc3..71098b17dfd 100644 --- a/spec/validators/dynamic_path_validator_spec.rb +++ b/spec/validators/dynamic_path_validator_spec.rb @@ -12,23 +12,30 @@ describe DynamicPathValidator do # Pass in a full path and get the last segment before a wildcard # That's not a parameter # `/*namespace_id/:project_id/builds/artifacts/*ref_name_and_path` - # -> 'artifacts' - def segment_before_last_wildcard(path) - path_segments = path.split('/').reject { |segment| segment.empty? } - last_wildcard_index = path_segments.rindex { |part| part.starts_with?('*') } - - index_of_non_param_segment = last_wildcard_index - 1 - part_before_wildcard = path_segments[index_of_non_param_segment] - while parameter?(part_before_wildcard) - index_of_non_param_segment -= 1 - part_before_wildcard = path_segments[index_of_non_param_segment] + # -> 'builds/artifacts' + def path_between_wildcards(path) + path = path.gsub(STARTING_WITH_NAMESPACE, "") + path_segments = path.split('/').reject(&:empty?) + wildcard_index = path_segments.index { |segment| segment.starts_with?('*') } + + segments_before_wildcard = path_segments[0..wildcard_index - 1] + + param_index = segments_before_wildcard.index { |segment| segment.starts_with?(':') } + if param_index + segments_before_wildcard = segments_before_wildcard[param_index + 1..-1] end - part_before_wildcard + segments_before_wildcard.join('/') end - def parameter?(path_segment) - path_segment.starts_with?(':') || path_segment.starts_with?('*') + # If the path is reserved. Then no conflicting paths can# be created for any + # 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 let(:all_routes) do @@ -42,14 +49,20 @@ describe DynamicPathValidator do # all routes not starting with a param 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 # path-segments before having another wildcard parameter. # - Starting with paths: # - `/*namespace_id/:project_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 `*` - # 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/ NON_PARAM_PARTS = /[^:*][a-z\-_\/]*/ ANY_OTHER_PATH_PART = /[a-z\-_\/:]*/ @@ -60,82 +73,89 @@ describe DynamicPathValidator do 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 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) end end describe 'WILDCARD_ROUTES' do it 'includes all paths that can be used after a namespace/project path' do - all_wildcard_paths = namespaced_wildcard_routes.map do |path| - segment_before_last_wildcard(path) - end.uniq - - expect(described_class::WILDCARD_ROUTES).to include(*all_wildcard_paths) + aggregate_failures do + all_wildcard_paths.each do |path| + expect(wildcards_include?(path)).to be(true), "Expected #{path} to be rejected" + end + end end end - describe ".valid?" do - it 'is not case sensitive' do - expect(described_class.valid?("Users", type: :top_level)).to be(false) + describe '.contains_path_part?' do + it 'recognizes a path part' do + expect(described_class.contains_path_part?('user/some/path', 'user')). + to be_truthy 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 - it "isn't valid if any of the path segments is reserved" do - test_path = 'the-wildcard/wikis/is-a-reserved-path' + it 'skips partial path matchies' do + 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 - it "is valid if the path doen't contain reserved words" do - test_path = 'there-are/no-wildcards/in-this-path' + it 'only recognizes full paths' do + 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 - describe '#validation_type' do - it 'uses top level validation for groups without parent' do - group = build(:group) - - type = validator.validation_type(group) - - expect(type).to eq(:top_level) + describe ".valid?" do + it 'is not case sensitive' do + expect(described_class.valid?("Users")).to be(false) end - it 'uses wildcard validation for groups with a parent' do - group = build(:group, parent: create(:group)) - - type = validator.validation_type(group) + it "isn't valid when the top level is reserved" do + test_path = 'u/should-be-a/reserved-word' - expect(type).to eq(:wildcard) + expect(described_class.valid?(test_path)).to be(false) end - it 'uses wildcard validation for a project' do - project = build(:project) - - type = validator.validation_type(project) + it "isn't valid if any of the path segments is reserved" do + test_path = 'the-wildcard/wikis/is-not-allowed' - expect(type).to eq(:wildcard) + expect(described_class.valid?(test_path)).to be(false) end - it 'uses strict validation for everything else' do - type = validator.validation_type(double) + it "is valid if the path doesn't contain reserved words" do + 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 -- cgit v1.2.1