Don't sanitize project names on project members page
What does this MR do?
It will remove the usage of sanitize_project_name(name)
for project members page views.
What are the relevant issue numbers?
Fixes #58751 (closed) .
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated via this MR -
Documentation reviewed by technical writer or follow-up review issue created -
Tests added for this feature/bug -
Tested in all supported browsers -
Conforms to the code review guidelines -
Conforms to the merge request performance guidelines -
Conforms to the style guides -
Conforms to the database guides -
Link to e2e tests MR added if this MR has Requires e2e tests label. See the Test Planning Process. -
Security reports checked/validated by reviewer
Merge request reports
Activity
added 1 commit
- 6bfd9989 - Don't sanitize project names on project members page
marked the checklist item Changelog entry added, if necessary as completed
EE Compat Job failed.
EE has some advanced project sharing options so they use a different method to show an explenation with who you are able to share your project. This method 1 do not use the
sanitize_project_name(name)
function so this conflict should be resolved by dropping changes inindex.html.haml
for EE.(The other two files are also affected in EE.)
added Community contribution label
added Plan [DEPRECATED] frontend labels
assigned to @bephinix
@filipa I am not a security expert but AFAIK we are using the project name as it is everywhere else. The sanitize function was only used in import processes before.
@filipa
There is the following validation statement inapp/models/project.rb
:validates :name, presence: true, length: { maximum: 255 }, format: { with: Gitlab::Regex.project_name_regex, message: Gitlab::Regex.project_name_regex_message }
It uses the following regex method from
lib/gitlab/regex.rb
:def project_name_regex @project_name_regex ||= /\A[\p{Alnum}\u{00A9}-\u{1f9c0}_][\p{Alnum}\p{Pd}\u{00A9}-\u{1f9c0}_\. ]*\z/.freeze end
So every string you enter as a project name will be checked against this regex on every creation or update. So AFAIK you cannot inject malicious content into the
project_name
field.@smcgivern can you give me a hand with this? Thanks!
There are a couple of things here:
- The validation does not guarantee that no records exist outside that format. For instance, records could have been created before the validation was added.
- The
sanitize_project_name
helper was added for a very specific purpose in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/21367 (importing a project), and I don't think it was meant for use here. - The helper was added to these pages in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/23227#note_122513680 because we wanted to sanitise this, but I think we didn't see what that helper was for, precisely (helpers all share one namespace).
I think here, we could use
sanitize(..., tags: [])
to avoid XSS without stripping other characters? @jameslopez what do you think?I think here, we could use
sanitize(..., tags: [])
to avoid XSS without stripping other characters?@smcgivern If we do so, shouldn't this be done everytime we use
project_name
?@bephinix it depends on how it's used. Here it's used as
html_safe
, so we need to provide some guarantee that it is, in fact, HTML safe If it's used as a regular string somewhere, that will be escaped further down the line anyway.@smcgivern Got it!
html_safe
sounds like it makes it HTML safe and not set it HTML safe (which is the case). Now it makes sense.@smcgivern It seems that @jameslopez is sick. (Get well soon.)
@smcgivern @jameslopez Is it possible to get this in April's release? This would be great!
changed milestone to %11.10
@bephinix actually, just had a quick look and Sean's suggestion make sense to me. I would certainly use
sanitize
there.@jameslopez Should we simply use
sanitize()
without additional parameters? Are they set as default elsewhere?added 1058 commits
-
6bfd9989...afbc8274 - 1057 commits from branch
gitlab-org:master
- d948e3ae - Don't sanitize project names on project members page
-
6bfd9989...afbc8274 - 1057 commits from branch
@jameslopez See new commit. :)
Thanks @bephinix, this looks good to me! Assigning to @smcgivern for a final review/merge.
assigned to @smcgivern
- Resolved by Elias Werberich
@bephinix thanks, one question!
assigned to @bephinix
added 1 commit
- 83fb62d4 - Don't sanitize project names on project members page
@smcgivern You are right. Without stripping all html tags it does not make much sense. I added
tags: []
.enabled an automatic merge when the pipeline for 83fb62d4 succeeds
Thanks @bephinix!
@smcgivern/@bephinix it looks like there's a pipeline failure?
@rpaik Ok, I forgot to push the new ee commits.
The failing mysql rspec job throws some strange errors.@smcgivern can you help with the mysql error?
The failure there is https://gitlab.com/gitlab-org/gitlab-ce/issues/59747, which should be fixed in current master.
assigned to @smcgivern
mentioned in commit 5ddd4f0f
mentioned in issue gitlab-org/release/tasks#731 (closed)
added devopsplan label