-
Notifications
You must be signed in to change notification settings - Fork 968
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve GitLab projects name verification #16262
base: main
Are you sure you want to change the base?
Conversation
Thanks @DarkaMaul! I want to pare this back a bit to avoid depending on assumptions internal to GitLab's code, but the consecutive character check makes a lot of sense to me to add. |
Is this ready for review? |
I think we are still unsure if we want to introduce a dependency on Gitlab internal code (for the list of forbidden project names). I can refactor the PR to only include the following rules :
We can leave the two others out of this PR:
|
IMO having those lists of forbidden names is fine, since they are documented on GitLab's docs. For me the important question is if having such exhaustive checking is worth it. @di what do you think? |
The purpose of these validators is to prevent likely typos or misunderstandings about what the value of this field should be. I think it's fairly unlikely that a user would frequently be trying to use these forbidden names, and even if they were, it would be fairly clear why it was failing. TL;DR: I don't think being so exhaustive is necessary or worth it. |
I think this could happen frequently enough that it's worth including. |
Agreed! Let's cover those two suffixes but leave the reserved names out. |
…warehouse into dm/validate-gitlab-names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @DarkaMaul!
This PR addresses #15852
Notably, it prevents names from :
.atom
or.git
a-zA-Z0-9
The keywords list are extracted from GitLab code. As noted by @facutuesca , this is mostly a cosmetic change, so having unsynchronized lists should not introduce a security risk.
Of note, we could slightly improve the regexes by limiting the number of potential matches to a fixed upper bound and prevent ReDos types of attacks.