From a10925e1c37e7dab19de346c553311adfaccb86c Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 3 Nov 2017 19:48:15 +0100 Subject: [PATCH] Reallow project paths ending in periods --- app/models/namespace.rb | 2 +- app/models/project.rb | 4 +- app/models/user.rb | 2 +- app/validators/abstract_path_validator.rb | 38 ++++++++ app/validators/dynamic_path_validator.rb | 53 ---------- app/validators/namespace_path_validator.rb | 19 ++++ app/validators/project_path_validator.rb | 19 ++++ app/validators/user_path_validator.rb | 15 +++ ...-reallow-project-path-ending-in-period.yml | 5 + lib/constraints/group_url_constrainer.rb | 2 +- lib/constraints/project_url_constrainer.rb | 2 +- lib/constraints/user_url_constrainer.rb | 2 +- lib/gitlab/o_auth/user.rb | 2 +- spec/models/project_spec.rb | 6 ++ .../validators/dynamic_path_validator_spec.rb | 97 ------------------- .../namespace_path_validator_spec.rb | 38 ++++++++ .../validators/project_path_validator_spec.rb | 38 ++++++++ spec/validators/user_path_validator_spec.rb | 38 ++++++++ 18 files changed, 223 insertions(+), 159 deletions(-) create mode 100644 app/validators/abstract_path_validator.rb delete mode 100644 app/validators/dynamic_path_validator.rb create mode 100644 app/validators/namespace_path_validator.rb create mode 100644 app/validators/project_path_validator.rb create mode 100644 app/validators/user_path_validator.rb create mode 100644 changelogs/unreleased/dm-reallow-project-path-ending-in-period.yml delete mode 100644 spec/validators/dynamic_path_validator_spec.rb create mode 100644 spec/validators/namespace_path_validator_spec.rb create mode 100644 spec/validators/project_path_validator_spec.rb create mode 100644 spec/validators/user_path_validator_spec.rb diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 0601a61a926..4d401e7ba18 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -36,7 +36,7 @@ class Namespace < ActiveRecord::Base validates :path, presence: true, length: { maximum: 255 }, - dynamic_path: true + namespace_path: true validate :nesting_level_allowed diff --git a/app/models/project.rb b/app/models/project.rb index 2f9b80d0514..5a0cbfeb282 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -240,10 +240,8 @@ class Project < ActiveRecord::Base message: Gitlab::Regex.project_name_regex_message } validates :path, presence: true, - dynamic_path: true, + project_path: true, length: { maximum: 255 }, - format: { with: Gitlab::PathRegex.project_path_format_regex, - message: Gitlab::PathRegex.project_path_format_message }, uniqueness: { scope: :namespace_id } validates :namespace, presence: true diff --git a/app/models/user.rb b/app/models/user.rb index bcda4564595..87a6c63cfaa 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -146,7 +146,7 @@ class User < ActiveRecord::Base presence: true, numericality: { greater_than_or_equal_to: 0, less_than_or_equal_to: Gitlab::Database::MAX_INT_VALUE } validates :username, - dynamic_path: true, + user_path: true, presence: true, uniqueness: { case_sensitive: false } diff --git a/app/validators/abstract_path_validator.rb b/app/validators/abstract_path_validator.rb new file mode 100644 index 00000000000..adbccb65a84 --- /dev/null +++ b/app/validators/abstract_path_validator.rb @@ -0,0 +1,38 @@ +class AbstractPathValidator < ActiveModel::EachValidator + extend Gitlab::EncodingHelper + + def self.path_regex + raise NotImplementedError + end + + def self.format_regex + raise NotImplementedError + end + + def self.format_error_message + raise NotImplementedError + end + + def self.full_path(record, value) + value + end + + def self.valid_path?(path) + encode!(path) + "#{path}/" =~ path_regex + end + + def validate_each(record, attribute, value) + unless value =~ self.class.format_regex + record.errors.add(attribute, self.class.format_error_message) + return + end + + full_path = self.class.full_path(record, value) + return unless full_path + + unless self.class.valid_path?(full_path) + record.errors.add(attribute, "#{value} is a reserved name") + end + end +end diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb deleted file mode 100644 index 4688aabc2a8..00000000000 --- a/app/validators/dynamic_path_validator.rb +++ /dev/null @@ -1,53 +0,0 @@ -# DynamicPathValidator -# -# Custom validator for GitLab path values. -# These paths are assigned to `Namespace` (& `Group` as a subclass) & `Project` -# -# Values are checked for formatting and exclusion from a list of illegal path -# names. -class DynamicPathValidator < ActiveModel::EachValidator - extend Gitlab::EncodingHelper - - class << self - def valid_user_path?(path) - encode!(path) - "#{path}/" =~ Gitlab::PathRegex.root_namespace_path_regex - end - - def valid_group_path?(path) - encode!(path) - "#{path}/" =~ Gitlab::PathRegex.full_namespace_path_regex - end - - def valid_project_path?(path) - encode!(path) - "#{path}/" =~ Gitlab::PathRegex.full_project_path_regex - end - end - - def path_valid_for_record?(record, value) - full_path = record.respond_to?(:build_full_path) ? record.build_full_path : value - - return true unless full_path - - case record - when Project - self.class.valid_project_path?(full_path) - when Group - self.class.valid_group_path?(full_path) - else # User or non-Group Namespace - self.class.valid_user_path?(full_path) - end - end - - def validate_each(record, attribute, value) - unless value =~ Gitlab::PathRegex.namespace_format_regex - record.errors.add(attribute, Gitlab::PathRegex.namespace_format_message) - return - end - - unless path_valid_for_record?(record, value) - record.errors.add(attribute, "#{value} is a reserved name") - end - end -end diff --git a/app/validators/namespace_path_validator.rb b/app/validators/namespace_path_validator.rb new file mode 100644 index 00000000000..4a0aa64ae0c --- /dev/null +++ b/app/validators/namespace_path_validator.rb @@ -0,0 +1,19 @@ +class NamespacePathValidator < AbstractPathValidator + extend Gitlab::EncodingHelper + + def self.path_regex + Gitlab::PathRegex.full_namespace_path_regex + end + + def self.format_regex + Gitlab::PathRegex.namespace_format_regex + end + + def self.format_error_message + Gitlab::PathRegex.namespace_format_message + end + + def self.full_path(record, value) + record.build_full_path + end +end diff --git a/app/validators/project_path_validator.rb b/app/validators/project_path_validator.rb new file mode 100644 index 00000000000..829b596ad3c --- /dev/null +++ b/app/validators/project_path_validator.rb @@ -0,0 +1,19 @@ +class ProjectPathValidator < AbstractPathValidator + extend Gitlab::EncodingHelper + + def self.path_regex + Gitlab::PathRegex.full_project_path_regex + end + + def self.format_regex + Gitlab::PathRegex.project_path_format_regex + end + + def self.format_error_message + Gitlab::PathRegex.project_path_format_message + end + + def self.full_path(record, value) + record.build_full_path + end +end diff --git a/app/validators/user_path_validator.rb b/app/validators/user_path_validator.rb new file mode 100644 index 00000000000..adf02901802 --- /dev/null +++ b/app/validators/user_path_validator.rb @@ -0,0 +1,15 @@ +class UserPathValidator < AbstractPathValidator + extend Gitlab::EncodingHelper + + def self.path_regex + Gitlab::PathRegex.root_namespace_path_regex + end + + def self.format_regex + Gitlab::PathRegex.namespace_format_regex + end + + def self.format_error_message + Gitlab::PathRegex.namespace_format_message + end +end diff --git a/changelogs/unreleased/dm-reallow-project-path-ending-in-period.yml b/changelogs/unreleased/dm-reallow-project-path-ending-in-period.yml new file mode 100644 index 00000000000..ad41d9b84c3 --- /dev/null +++ b/changelogs/unreleased/dm-reallow-project-path-ending-in-period.yml @@ -0,0 +1,5 @@ +--- +title: Reallow project paths ending in periods +merge_request: +author: +type: fixed diff --git a/lib/constraints/group_url_constrainer.rb b/lib/constraints/group_url_constrainer.rb index 6fc1d56d7a0..fd2ac2db0a9 100644 --- a/lib/constraints/group_url_constrainer.rb +++ b/lib/constraints/group_url_constrainer.rb @@ -2,7 +2,7 @@ class GroupUrlConstrainer def matches?(request) full_path = request.params[:group_id] || request.params[:id] - return false unless DynamicPathValidator.valid_group_path?(full_path) + return false unless NamespacePathValidator.valid_path?(full_path) Group.find_by_full_path(full_path, follow_redirects: request.get?).present? end diff --git a/lib/constraints/project_url_constrainer.rb b/lib/constraints/project_url_constrainer.rb index 5bef29eb1da..e90ecb5ec69 100644 --- a/lib/constraints/project_url_constrainer.rb +++ b/lib/constraints/project_url_constrainer.rb @@ -4,7 +4,7 @@ class ProjectUrlConstrainer project_path = request.params[:project_id] || request.params[:id] full_path = [namespace_path, project_path].join('/') - return false unless DynamicPathValidator.valid_project_path?(full_path) + return false unless ProjectPathValidator.valid_path?(full_path) # We intentionally allow SELECT(*) here so result of this query can be used # as cache for further Project.find_by_full_path calls within request diff --git a/lib/constraints/user_url_constrainer.rb b/lib/constraints/user_url_constrainer.rb index d16ae7f3f40..b7633aa7cbb 100644 --- a/lib/constraints/user_url_constrainer.rb +++ b/lib/constraints/user_url_constrainer.rb @@ -2,7 +2,7 @@ class UserUrlConstrainer def matches?(request) full_path = request.params[:username] - return false unless DynamicPathValidator.valid_user_path?(full_path) + return false unless UserPathValidator.valid_path?(full_path) User.find_by_full_path(full_path, follow_redirects: request.get?).present? end diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index 47c2a422387..b4b3b00c84d 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -179,7 +179,7 @@ module Gitlab valid_username = ::Namespace.clean_path(username) uniquify = Uniquify.new - valid_username = uniquify.string(valid_username) { |s| !DynamicPathValidator.valid_user_path?(s) } + valid_username = uniquify.string(valid_username) { |s| !UserPathValidator.valid_path?(s) } name = auth_hash.name name = valid_username if name.strip.empty? diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index e8588975118..0e50909988b 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -276,6 +276,12 @@ describe Project do expect(project).to be_valid end + + it 'allows a path ending in a period' do + project = build(:project, path: 'foo.') + + expect(project).to be_valid + end end end diff --git a/spec/validators/dynamic_path_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb deleted file mode 100644 index 08e1c5a728a..00000000000 --- a/spec/validators/dynamic_path_validator_spec.rb +++ /dev/null @@ -1,97 +0,0 @@ -require 'spec_helper' - -describe DynamicPathValidator do - let(:validator) { described_class.new(attributes: [:path]) } - - def expect_handles_invalid_utf8 - expect { yield('\255invalid') }.to be_falsey - end - - describe '.valid_user_path' do - it 'handles invalid utf8' do - expect(described_class.valid_user_path?("a\0weird\255path")).to be_falsey - end - end - - describe '.valid_group_path' do - it 'handles invalid utf8' do - expect(described_class.valid_group_path?("a\0weird\255path")).to be_falsey - end - end - - describe '.valid_project_path' do - it 'handles invalid utf8' do - expect(described_class.valid_project_path?("a\0weird\255path")).to be_falsey - end - end - - describe '#path_valid_for_record?' do - context 'for project' do - it 'calls valid_project_path?' do - project = build(:project, path: 'activity') - - expect(described_class).to receive(:valid_project_path?).with(project.full_path).and_call_original - - expect(validator.path_valid_for_record?(project, 'activity')).to be_truthy - end - end - - context 'for group' do - it 'calls valid_group_path?' do - group = build(:group, :nested, path: 'activity') - - expect(described_class).to receive(:valid_group_path?).with(group.full_path).and_call_original - - expect(validator.path_valid_for_record?(group, 'activity')).to be_falsey - end - end - - context 'for user' do - it 'calls valid_user_path?' do - user = build(:user, username: 'activity') - - expect(described_class).to receive(:valid_user_path?).with(user.full_path).and_call_original - - expect(validator.path_valid_for_record?(user, 'activity')).to be_truthy - end - end - - context 'for user namespace' do - it 'calls valid_user_path?' do - user = create(:user, username: 'activity') - namespace = user.namespace - - expect(described_class).to receive(:valid_user_path?).with(namespace.full_path).and_call_original - - expect(validator.path_valid_for_record?(namespace, 'activity')).to be_truthy - end - end - end - - describe '#validates_each' do - it 'adds a message when the path is not in the correct format' do - group = build(:group) - - validator.validate_each(group, :path, "Path with spaces, and comma's!") - - expect(group.errors[:path]).to include(Gitlab::PathRegex.namespace_format_message) - end - - it 'adds a message when the path is not in the correct format' do - group = build(:group, path: 'users') - - validator.validate_each(group, :path, 'users') - - expect(group.errors[:path]).to include('users is a reserved name') - end - - it 'updating to an invalid path is not allowed' do - project = create(:project) - project.path = 'update' - - validator.validate_each(project, :path, 'update') - - expect(project.errors[:path]).to include('update is a reserved name') - end - end -end diff --git a/spec/validators/namespace_path_validator_spec.rb b/spec/validators/namespace_path_validator_spec.rb new file mode 100644 index 00000000000..61e2845f35f --- /dev/null +++ b/spec/validators/namespace_path_validator_spec.rb @@ -0,0 +1,38 @@ +require 'spec_helper' + +describe NamespacePathValidator do + let(:validator) { described_class.new(attributes: [:path]) } + + describe '.valid_path?' do + it 'handles invalid utf8' do + expect(described_class.valid_path?("a\0weird\255path")).to be_falsey + end + end + + describe '#validates_each' do + it 'adds a message when the path is not in the correct format' do + group = build(:group) + + validator.validate_each(group, :path, "Path with spaces, and comma's!") + + expect(group.errors[:path]).to include(Gitlab::PathRegex.namespace_format_message) + end + + it 'adds a message when the path is reserved when creating' do + group = build(:group, path: 'help') + + validator.validate_each(group, :path, 'help') + + expect(group.errors[:path]).to include('help is a reserved name') + end + + it 'adds a message when the path is reserved when updating' do + group = create(:group) + group.path = 'help' + + validator.validate_each(group, :path, 'help') + + expect(group.errors[:path]).to include('help is a reserved name') + end + end +end diff --git a/spec/validators/project_path_validator_spec.rb b/spec/validators/project_path_validator_spec.rb new file mode 100644 index 00000000000..8bb5e72dc22 --- /dev/null +++ b/spec/validators/project_path_validator_spec.rb @@ -0,0 +1,38 @@ +require 'spec_helper' + +describe ProjectPathValidator do + let(:validator) { described_class.new(attributes: [:path]) } + + describe '.valid_path?' do + it 'handles invalid utf8' do + expect(described_class.valid_path?("a\0weird\255path")).to be_falsey + end + end + + describe '#validates_each' do + it 'adds a message when the path is not in the correct format' do + project = build(:project) + + validator.validate_each(project, :path, "Path with spaces, and comma's!") + + expect(project.errors[:path]).to include(Gitlab::PathRegex.project_path_format_message) + end + + it 'adds a message when the path is reserved when creating' do + project = build(:project, path: 'blob') + + validator.validate_each(project, :path, 'blob') + + expect(project.errors[:path]).to include('blob is a reserved name') + end + + it 'adds a message when the path is reserved when updating' do + project = create(:project) + project.path = 'blob' + + validator.validate_each(project, :path, 'blob') + + expect(project.errors[:path]).to include('blob is a reserved name') + end + end +end diff --git a/spec/validators/user_path_validator_spec.rb b/spec/validators/user_path_validator_spec.rb new file mode 100644 index 00000000000..a46089cc24f --- /dev/null +++ b/spec/validators/user_path_validator_spec.rb @@ -0,0 +1,38 @@ +require 'spec_helper' + +describe UserPathValidator do + let(:validator) { described_class.new(attributes: [:username]) } + + describe '.valid_path?' do + it 'handles invalid utf8' do + expect(described_class.valid_path?("a\0weird\255path")).to be_falsey + end + end + + describe '#validates_each' do + it 'adds a message when the path is not in the correct format' do + user = build(:user) + + validator.validate_each(user, :username, "Path with spaces, and comma's!") + + expect(user.errors[:username]).to include(Gitlab::PathRegex.namespace_format_message) + end + + it 'adds a message when the path is reserved when creating' do + user = build(:user, username: 'help') + + validator.validate_each(user, :username, 'help') + + expect(user.errors[:username]).to include('help is a reserved name') + end + + it 'adds a message when the path is reserved when updating' do + user = create(:user) + user.username = 'help' + + validator.validate_each(user, :username, 'help') + + expect(user.errors[:username]).to include('help is a reserved name') + end + end +end