Commit f5576b16 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'namespace-validation-fixes' into 'master'

Correct namespace validation to forbid bad names #21077

## What does this MR do?
Updates master namespace regex to forbid any namespace ending in `.git` or `.atom` and corrects and adds relevant tests

## Are there points in the code the reviewer needs to double check?
I think it's all good. I could use help with the creation of tests for usernames with trailing `.atom` or `.git` as the testing framework is a bit over my head.

## Why was this MR needed?
A group that ends in `.atom` will cause the relevent dashboard to crash if the user (ANY user, not just the creator) has visibility of the group until it is deleted through the admin panel (it cannot be renamed, the edit page will crash. It may be fixable through the API, that wasn't checked.)

This allows a malicious user with group creation privileges to bulk add users to a group, rename the group to a bad name, and crash the groups dashboard for all members of the group. The same applies if the group is internal or public and users navigate to the explore tab of the groups dashboard.

The same applies to usernames ending in `.atom`.

In many places of the code, it implies that `.git` in not allowed at the end of namespaces, but many allowed it anyway. This MR forbids it everywhere to prevent potential issues (like the one with `.atom` going forward).

## What are the relevant issue numbers?
Group path validation incomplete, crashes groups dashboard #21077

## Does this MR meet the acceptance criteria?

- [X] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [ ] API support added
- Tests
  - [X] Added for this feature/bug
  - [X] All builds are passing
- [X] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [X] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [X] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5994
parents b26a3d53 4f1de5fa
...@@ -32,6 +32,7 @@ v 8.13.0 (unreleased) ...@@ -32,6 +32,7 @@ v 8.13.0 (unreleased)
- Allow the Koding integration to be configured through the API - Allow the Koding integration to be configured through the API
- Add new issue button to each list on Issues Board - Add new issue button to each list on Issues Board
- Added soft wrap button to repository file/blob editor - Added soft wrap button to repository file/blob editor
- Update namespace validation to forbid reserved names (.git and .atom) (Will Starms)
- Add word-wrap to issue title on issue and milestone boards (ClemMakesApps) - Add word-wrap to issue title on issue and milestone boards (ClemMakesApps)
- Fix todos page mobile viewport layout (ClemMakesApps) - Fix todos page mobile viewport layout (ClemMakesApps)
- Fix inconsistent highlighting of already selected activity nav-links (ClemMakesApps) - Fix inconsistent highlighting of already selected activity nav-links (ClemMakesApps)
......
...@@ -62,14 +62,12 @@ class Namespace < ActiveRecord::Base ...@@ -62,14 +62,12 @@ class Namespace < ActiveRecord::Base
path = path.dup path = path.dup
# Get the email username by removing everything after an `@` sign. # Get the email username by removing everything after an `@` sign.
path.gsub!(/@.*\z/, "") path.gsub!(/@.*\z/, "")
# Usernames can't end in .git, so remove it.
path.gsub!(/\.git\z/, "")
# Remove dashes at the start of the username.
path.gsub!(/\A-+/, "")
# Remove periods at the end of the username.
path.gsub!(/\.+\z/, "")
# Remove everything that's not in the list of allowed characters. # Remove everything that's not in the list of allowed characters.
path.gsub!(/[^a-zA-Z0-9_\-\.]/, "") path.gsub!(/[^a-zA-Z0-9_\-\.]/, "")
# Remove trailing violations ('.atom', '.git', or '.')
path.gsub!(/(\.atom|\.git|\.)*\z/, "")
# Remove leading violations ('-')
path.gsub!(/\A\-+/, "")
# Users with the great usernames of "." or ".." would end up with a blank username. # Users with the great usernames of "." or ".." would end up with a blank username.
# Work around that by setting their username to "blank", followed by a counter. # Work around that by setting their username to "blank", followed by a counter.
......
...@@ -2,7 +2,7 @@ module Gitlab ...@@ -2,7 +2,7 @@ module Gitlab
module Regex module Regex
extend self extend self
NAMESPACE_REGEX_STR = '(?:[a-zA-Z0-9_\.][a-zA-Z0-9_\-\.]*[a-zA-Z0-9_\-]|[a-zA-Z0-9_])'.freeze NAMESPACE_REGEX_STR = '(?:[a-zA-Z0-9_\.][a-zA-Z0-9_\-\.]*[a-zA-Z0-9_\-]|[a-zA-Z0-9_])(?<!\.git|\.atom)'.freeze
def namespace_regex def namespace_regex
@namespace_regex ||= /\A#{NAMESPACE_REGEX_STR}\z/.freeze @namespace_regex ||= /\A#{NAMESPACE_REGEX_STR}\z/.freeze
...@@ -10,7 +10,7 @@ module Gitlab ...@@ -10,7 +10,7 @@ module Gitlab
def namespace_regex_message def namespace_regex_message
"can contain only letters, digits, '_', '-' and '.'. " \ "can contain only letters, digits, '_', '-' and '.'. " \
"Cannot start with '-' or end in '.'." \ "Cannot start with '-' or end in '.', '.git' or '.atom'." \
end end
def namespace_name_regex def namespace_name_regex
......
...@@ -5,6 +5,12 @@ feature 'Group', feature: true do ...@@ -5,6 +5,12 @@ feature 'Group', feature: true do
login_as(:admin) login_as(:admin)
end end
matcher :have_namespace_error_message do
match do |page|
page.has_content?("Path can contain only letters, digits, '_', '-' and '.'. Cannot start with '-' or end in '.', '.git' or '.atom'.")
end
end
describe 'creating a group with space in group path' do describe 'creating a group with space in group path' do
it 'renders new group form with validation errors' do it 'renders new group form with validation errors' do
visit new_group_path visit new_group_path
...@@ -13,7 +19,31 @@ feature 'Group', feature: true do ...@@ -13,7 +19,31 @@ feature 'Group', feature: true do
click_button 'Create group' click_button 'Create group'
expect(current_path).to eq(groups_path) expect(current_path).to eq(groups_path)
expect(page).to have_content("Path can contain only letters, digits, '_', '-' and '.'. Cannot start with '-' or end in '.'.") expect(page).to have_namespace_error_message
end
end
describe 'creating a group with .atom at end of group path' do
it 'renders new group form with validation errors' do
visit new_group_path
fill_in 'Group path', with: 'atom_group.atom'
click_button 'Create group'
expect(current_path).to eq(groups_path)
expect(page).to have_namespace_error_message
end
end
describe 'creating a group with .git at end of group path' do
it 'renders new group form with validation errors' do
visit new_group_path
fill_in 'Group path', with: 'git_group.git'
click_button 'Create group'
expect(current_path).to eq(groups_path)
expect(page).to have_namespace_error_message
end end
end end
......
...@@ -114,6 +114,7 @@ describe Namespace, models: true do ...@@ -114,6 +114,7 @@ describe Namespace, models: true do
it "cleans the path and makes sure it's available" do it "cleans the path and makes sure it's available" do
expect(Namespace.clean_path("-john+gitlab-ETC%.git@gmail.com")).to eq("johngitlab-ETC2") expect(Namespace.clean_path("-john+gitlab-ETC%.git@gmail.com")).to eq("johngitlab-ETC2")
expect(Namespace.clean_path("--%+--valid_*&%name=.git.%.atom.atom.@email.com")).to eq("valid_name")
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