Do not escape URI when extracting path
What does this MR do?
- remove escaping of URI parameters after they have already been unescaped in
ExtractsPath - remove spec that is relying on that behavior
Why was this MR needed?
- spaces in directory names were escaped leading to 404 errors on repository files page
What are the relevant issue numbers?
fixes gitlab-com/support-forum#952 (closed), #21106 (closed)
Screenshot
Merge request reports
Activity
Added 1 commit:
- 3d1e35b7 - Do not escape URI when extracting path (!5878 (merged))
94 94 @options = params.select {|key, value| allowed_options.include?(key) && !value.blank? } 95 95 @options = HashWithIndifferentAccess.new(@options) 96 96 97 @id = Addressable::URI.normalize_component(get_id) That is not correct in all cases, please read the ticket corresponding to that change! The problem is that the {ref} in a path like this is not unescaped http://example.com/user/project/refs/branch%2Fwith%2Fslashes/logs_tree/?_=1480415245487 that is the problem.
-
@nemoinho89 Could you please open a new issue to discuss the problem that you are experiencing? Especially whether you are using Apache (which has known issues) or nginx.
30 30 expect(@logs_path).to eq("/#{@project.path_with_namespace}/refs/#{ref}/logs_tree/files/ruby/popen.rb") 31 31 end 32 32 33 context 'escaped slash character in ref' do 34 let(:ref) { 'improve%2Fawesome' } 35 36 it 'has no escape sequences in @ref or @logs_path' do 37 assign_ref_vars 38 39 expect(@ref).to eq('improve/awesome') 40 expect(@logs_path).to eq("/#{@project.path_with_namespace}/refs/#{ref}/logs_tree/files/ruby/popen.rb") 41 end 42 end 43 -
As mentioned above,
paramsalready come in unescaped. For example http://localhost:3000/john/doe/tree/with%2Fslash/ results inparams[:id] = 'with/slash/'.This test was introduced by ecbe119a which fixed network and graphs pages for branches with encoded slashes. I have succesfully tested that use case:
-
@nemoinho89 This topic was obsolete, please read my comment above.
-
@rspeicher Can you please take a look at this?
@stanhu and me discussed the encoding of the slashes because Apache and nginx handle them differently by default. We decided to go for the nginx version which unencodes the slashes for us, i.e. https://gitlab.com/gitlab-org/gitlab-ce/tree/with%2Fslash would lead to
params[:id] = 'with/slash'. Apache by default returns error 404. This can be changed by settingAllowEncodedSlashestoOn(which adopts the behavior of nginx). The behavior ofOnchanged with introducingNoDecodein 2.2.18 or 2.3.12.We should probably mention this setting in the release blog post.
See also @stanhu's original comment: https://gitlab.com/gitlab-org/gitlab-ce/issues/1804#note_1386456
I explicitly linked the English version of the Apache docs above (and not the translated one) because at least the German translation does not contain the
NoDecodeoption yet.Edited by username-removed-14714 Reassigned to @rspeicher
-
I fixed the comment above, mixed up
OnandNoDecodeagain.😒 Prior to 2.2.18 or 2.3.12Onhad the behavior thatNoDecodehas now (compare 2.0 docs). So these Apache versions won't work. Added 232 commits:
-
3d1e35b7...12fe6a6f - 230 commits from branch
gitlab-org:master - 7d76635b - Add failing test for gitlab-com/support-forum#952 (closed)
- 743b1517 - Do not escape URI when extracting path (!5878 (merged))
-
3d1e35b7...12fe6a6f - 230 commits from branch
mentioned in issue #20946 (closed)
Milestone changed to %8.11
@winniehell Please resolve conflicts.
Reassigned to @winniehell
-
@rspeicher resolved
🎉 Added 76 commits:
-
743b1517...1f127006 - 74 commits from branch
gitlab-org:master - f3ee7fc8 - Add failing test for gitlab-com/support-forum#952 (closed)
- bb48a0fa - Do not escape URI when extracting path (!5878 (merged))
-
743b1517...1f127006 - 74 commits from branch
Reassigned to @rspeicher
mentioned in issue #21106 (closed)
Removed repository label
mentioned in commit daa0cc1c
mentioned in commit ebc9d34d
-
@stanhu We have
NoDecodehere: https://gitlab.com/gitlab-org/gitlab-recipes/blob/b9afcfd8f9543342df46bfb4327be6466d1097ce/web-server/apache/gitlab-apache24.conf#L20 That's bad...😟 mentioned in issue #21248 (closed)
mentioned in issue #21334 (closed)
mentioned in merge request !5974 (merged)
mentioned in commit 910da231
mentioned in merge request !6050 (merged)
mentioned in issue #21755 (closed)
Mentioned in commit bb48a0fa
Mentioned in issue #22400 (closed)
Mentioned in merge request !6050 (merged)
