Skip to content

WIP: Project path endings issue

Harry Kiselev requested to merge harrykiselev/gitlab-ce:project-path-endings-issue into master

What does this MR do?

Hey! Can I try to resolve this #54175 (moved) issue?

I have thoughts about this problem and I want to share it.

This issue caused by default rails controller responders for different mime-types of requests. More information is here: https://api.rubyonrails.org/v4.1/classes/ActionController/MimeResponds.html.

For example, when requests url ends with [json, xml, rss, yaml... etc] rails try to response with appropriate mime-type response.

Here I collect full list of extensions by /actionpack/lib/action_dispatch/http/mime_types.rb:

pry(main)> Mime::EXTENSION_LOOKUP.keys
=> ['html', 'xhtml', 'text', 'txt', 'js', 'css', 'ics', 'csv', 'vcf',
'png', 'jpeg', 'jpg', 'jpe', 'pjpeg', 'gif', 'bmp', 'tiff', 'tif',
'svg', 'mpeg', 'mpg', 'mpe', 'xml', 'rss', 'atom', 'yaml', 'yml',
'multipart_form', 'url_encoded_form', 'pdf', 'zip', 'gzip', 'gz',
'diff', 'patch', 'markdown', 'md', 'mp4', 'm4v', 'mov', 'webm',
'ogv', 'json', 'ico']

Therefore .json is not single case of incorrect paths that crashes a view project page. We can crash it by any of item of array above.

When we update projects path to 'test.json' and request it by /user_namespace/test.json, gitlab find this project and route good but fails on page rendering because of absent an appropriate responder.

I think we have a pair of solutions to resolve the issue.

First is to forbid by validations any paths ends with list of extensions above.

Second solution is let it be as it is and:

  • to allow all type of endings for path project
  • rewrite request format in show action to :html if request is not .atom–request. We need to insert one line of code before respond_to method:
request.format = :html if request.format.symbol != :atom

This line fixes the issue but probably it's not best solution and the first variant is preferred and we need to forbid list of mime-type extenstions on a project path validation.

What do you think about that?

Thank you :)

What are the relevant issue numbers?

#54175 (moved)

Does this MR meet the acceptance criteria?

Edited by Harry Kiselev

Merge request reports