Skip to content

Fix ambiguous routing issues by teaching router about reserved words

Douwe Maan requested to merge dm-fix-routes into master

This fixes the 422 on https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/blob/viewer/index.js.

The Rails router interpreted that URL as { namespace_id: 'gitlab-org/gitlab-ce/blob/master/app/assets', project_id: 'javascripts', id: 'viewer/index.js' } instead of the actual { namespace_id: 'gitlab-org', project_id: 'gitlab-ce', id: 'master/app/assets/javascripts/blob/viewer/index.js' }.

Since namespace gitlab-org/gitlab-ce/blob/master/app/assets obviously doesn't exist, this request gets handled by the *unmatched_route route, which renders 404. We see 422 instead because some Rails middleware sees the .js extension and raises an error:

Security warning: an embedded <script> tag on another site requested protected JavaScript. If you know what you're doing, go ahead and disable forgery protection on this action to permit cross-origin JavaScript embedding.

This will happen for any blob or tree URL that includes blob or tree in the path itself, because of Rails's greedy matching.

This MR "teaches" the router about reserved words using a constraint regex for namespace_id and project_id that includes a negative lookahead for these reserved words, so that the router will not even try to parse the namespace_id or project_id using those words.

This MR is so big because we need to construct appropriate regexes for the router to use, which the original design of DynamicPathValidator did not allow for. I've refactored DynamicPathValidator and extracted some of its previous responsibility into Gitlab::PathRegex to give us the regexes we need, and hopefully make the whole thing easier to understand.

I think this issue was introduced by https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10413, because as far as I can see it exists on 9-2-stable but not 9-1-stable and because the issue seems related, but I cannot actually pinpoint the line in that MR that causes the issue, since it doesn't touch the routing files at all...

@reprazent Curious to hear your thoughts about the issue and my solution!

Merge request reports