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?

• CHANGELOG entry added
• Documentation created/updated
• API support added
• Tests
  • Added for this feature/bug
  • All builds are passing
• Conform by the style guides
• Branch has no merge conflicts with master (if you do - rebase it please)
• Squashed related commits together

Marked the task Documentation created/updated N/A as completed

Marked the task Documentation created/updated N/A as incomplete

Added 28 commits:

• 349ea209...7b0b2417 - 27 commits from branch gitlab-org:master
• a797db15 - Correct namespace validation to forbid bad names #21077

Thanks @vilhelmen!

Looks pretty good to me, just a question about one part. I take it you're happy that there may be existing namespaces out there that end in these, and that only new ones will be validated like this?

Added ~164274 label

Added 1 commit:

• e64d660b - Correct namespace validation to forbid bad names #21077

@smcgivern I suppose it may be worthwhile to fix existing ones. .atom namespaces are actively bad, so those should get corrected. Those ending in .git, at least currently, are not causing problems, so they could be left as they are.

If we want to be forward thinking, it may be best to append _ to the end of either namespaces or something similar. I'm not sure I'm familiar enough with any of this to put together a migration, but I can try!

@vilhelmen let's not worry about adding a migration for now, and just fix the clean_path method. It would be good to add some more specs in spec/models/namespace_spec.rb for the problem cases we've talked about (At the moment, that method has one test case! )

Added 1 commit:

• ae95aa7a - Correct cleaning regex and add more complex test

Added 1 commit:

• f4d2e998 - 1 commit from branch gitlab-org:master

@smcgivern Perfect, because I haven't had the chance to look into them. Latest commit (assuming the tests pass, I'll fight with it until I get it working) should take care of everything.

/\A\-+[^a-zA-Z0-9_\-\.](\.atom|\.git|\.)*\z/ wasn't quite working for testing and I replaced it with a 3 step clean that should handle everything without allowing for the previous problem cases. At least, it worked in local testing.

Resolved all discussions

Added 1 commit:

• e68dea6b - Dialed back overzealous test to at least try to conform to email spec

Reassigned to @rspeicher

What do we do about namespaces violating this new validation that already exist in the database?

Reassigned to @vilhelmen