Prevent double decoding of % in url params

[zeripath]

There was an unfortunate regression in #14293 which has led to the double decoding
of url parameter elements if they contain a '%'. This is due to an issue
with the way chi decodes its RoutePath. In detail the problem lies in
mux.go where the routeHTTP path uses the URL.RawPath or even the
URL.Path instead of the escaped path to do routing.

This PR simply forcibly sets the routePath to that of the EscapedPath.

Fix #17938

⚠️ BREAKING ⚠️

Although this fixes a bug - users may have worked around this bug by creating links that would make this work - especially in the wiki. As users may need to be aware of this I am marking this PR as breaking in order to highlight this issue.

Signed-off-by: Andrew Thornton art27@cantab.net

[zeripath]

Reviewers please check this PR with your own specific evil test cases.

We will need to double check the redirects are correct too.

[zeripath]

Ah ok the problem is much more complicated...

The issue is that chi has:

// routeHTTP routes a http.Request through the Mux routing tree to serve
// the matching handler for a particular http method.
func (mx *Mux) routeHTTP(w http.ResponseWriter, r *http.Request) {
	// Grab the route context object
	rctx := r.Context().Value(RouteCtxKey).(*Context)

	// The request routing path
	routePath := rctx.RoutePath
	if routePath == "" {
		if r.URL.RawPath != "" {
			routePath = r.URL.RawPath
		} else {
			routePath = r.URL.Path
		}
		if routePath == "" {
			routePath = "/"
		}
	}
...

The routePath should really use the r.URL.EscapedPath() not r.URL.RawPath or even r.URL.Path

[KN4CK3R]

Are you sure we are not using chi in the wrong way? It looks like a workaround now.

[zeripath]

If you don't use the escapedpath you use partially decoded URLs for routing and get partially decoded results assigned to ctx.Params. (partially decoded in that you can't safely decode or just use them as is because some things have been decoded and somethings haven't. The result is that %25 will often be predecoded to % but other things won't be - and because you don't have the original URL (except as the full RequestURI) you don't really know how to interpret a % encoded string.

The only consistent (and safe) thing to do is to use the encoded URL for routing.

Feel free to play around with it though.

Here are few (unencoded) filenames you might want to try to make it possible to route to:

10%.md
£15&$6.txt
%3Fis?and#afile
%253Fisnotaquestion%253F
This+file%20has 1space
%%2525mightnotplaywell

[wxiaoguang]

Nice catch.

Should we add some documents and unit tests to describe the expected behavior?

And I am not sure how most reverse proxies process these encoded URLs (what will be passed to backend)

• /%.html (it should be /%.html, although it is incorrect)
• /%25.html (questionable, /%.html or /%25.html for backend ? )
• /%2525.html ...

[lunny]

Isn't this an upstream problem?

[zeripath]

Not sure if it's definitely an upstream problem - this may be by design.

Partial decoding of the urls may or may not be nice depending on your application - in our case it's bad because we're rendering urls that we have little to no control over so we have to have a simple mapping. Other users may not mind that if you want to have a % in your url it needs to be double escaped.

The solution proposed here is to use the mechanisms already available to us as provided by chi to tell it to use the EscapedPath instead of the partially escaped one.

[zeripath]

Nice catch.

Should we add some documents and unit tests to describe the expected behavior?

I've added unit tests. I'm not sure what kind of document would be beneficial - this PR is simply making sure that ctx.Params() always gives you the non-url encoded parameter from the url.

And I am not sure how most reverse proxies process these encoded URLs (what will be passed to backend)

That's a configuration issue. The reverse proxy should be passing the escaped url correctly - if it's de-escaping things then that's incorrect configuration.

• /%.html (it should be /%.html, although it is incorrect)

It's simple: a file that is named /%.html should be exposed as an url at /%25.html and if you send GET /owner/repo/src/branch/master/%25.html you should get that file. If you send a request GET /owner/repo/src/branch/master/%.html then go's url handler will clean that up to .../%25.html and you'll get the same file.

• /%25.html (questionable, /%.html or /%25.html for backend ? )

a file that is named /%25.html should be exposed as an url at .../%2525.html and if you send GET /owner/repo/src/branch/master/%2525.html you should get that file. If you send a request GET /owner/repo/src/branch/master/%25.html then you should get %.html not %25.html.

• /%2525.html ...

a file that is named /%2525.html should be exposed as an url at /%252525.html and if you send GET /owner/repo/src/branch/master/%252525.html you should get that file. If you send a request GET /owner/repo/src/branch/master/%2525.html then you should get %25.html not %2525.html and certainly not %.html.

[wxiaoguang]

@zeripath I didn't say clearly. Let's see an example:

For a simple Go http server:

package main

import (
	"fmt"
	"net/http"
)

func main() {
	http.HandleFunc("/", func (w http.ResponseWriter, r *http.Request) {
		_, _ = fmt.Fprintf(w, "URI: %s\n", r.RequestURI)
		_, _ = fmt.Fprintf(w, "URL: %s\n", r.URL.Path)
	})
	_ = http.ListenAndServe(":3000", nil)
}

nginx config:

server {
    listen 127.0.0.1:80;
    listen 127.0.0.1:443 ssl http2;
    location / {
        proxy_pass http://127.0.0.1:3000/;
    }
}

We see different behaviors:

Reference: https://serverfault.com/questions/459369/disabling-url-decoding-in-nginx-proxy

That's what I mean about document. Current Gitea document for nginx (with sub-path) will cause such double-decoding.

Users should tune their reverse proxy carefully.

[techknowlogick]

❤️ the tests

[zeripath]

@wxiaoguang this partial decoding of URI paths containing %2f is not an actual problem. It would only be a problem if filenames were allowed to contain / in them (which they're not - even on Windows) - and we don't use the URI but the escaped URL which is the same in both of these cases. (IIRC URLs aren't actually allowed %2f in path components.)

It would be a problem if %252f in an URI became %2f in an URL because then that would map to looking at a file with '/' in its name and prevent looking at files with %2f in their names.