fix double encoded %-signs in request path

[shieldwed]

According to go URL documentation (https://golang.org/pkg/net/url/#URL) URL.Path returns a decoded version of the originally requested URL.
Filling this field with a value, but not RawPath causes URL to encode the value of Path and return it when called by URL.EscapedPath() and hence URL.String().

[shieldwed]

I am not quite sure whether the condition is actually needed. In case the requestURI.RawPath is empty, the outReq.URL.RawPath is going to be empty anyway too.

if requestURI.RawPath != ""

Please feel free to improve my snippet.

[ldez]

Could you add tests to prove that you are repairing something and do not create regression?

[shieldwed]

Well, I guess I fixed the test 😉 .

Other than that, I created my own little test setup with a small python script as back end server and additionally now I have my own Collabora cloud running after applying my own patch. See referenced issue with traefik.

[ldez]

you have fixed an already working test ? 🤔

Maybe I missed something, could you explains?

[ldez]

I have seen issue but your test snippet match with nothing in TestURLWithURLEncodedChar.

Could you create a PR on Traefik with a failed test? or just add a test in this PR? Thanks.

[shieldwed]

From my point of view, the test is testing the wrong thing. According to https://golang.org/pkg/net/url/#URL:

Note that the Path field is stored in decoded form: /%47%6f%2f becomes /Go/. A consequence is that it is impossible to tell which slashes in the Path were slashes in the raw URL and which were %2f. This distinction is rarely important, but when it is, code must not use Path directly. The Parse function sets both Path and RawPath in the URL it returns, and URL's String method uses RawPath if it is a valid encoding of Path, by calling the EscapedPath method.

So the test is not really testing what the back end server is receiving, but is testing whether the decoded version is the same as the decoded version from the initial HTTP Request. This is what happens currently:

TestWebsocketRequestWithEncodedChar is setting up the initial request with the function withPath which in turn sets the URL.Path field (Note: the URL.RawPath field is empty). This leads to a request on the network (use tcpdump or sth similar or set up your own HTTP server like me with this snippet: https://gist.github.com/shieldwed/0312c39fa486912060c68bfd314f2393 if you don't believe me) which is actually requesting the resource /%253A%252F%252F.

The set up HandlerFunc within TestWebsocketRequestWithEncodedChar then receives the request and compares the decoded request path (r.URL.Path) with the string /%3A%2F%2F which works like a charm, so the test is working from a go test standpoint.

But the test does not represent what is actually sent over the network.

[ldez]

I trust you and I understand the problem, but I think we can reproduce the problem with a test.

Without testing, we can not preserve ourselves from a regression in the future.

[shieldwed]

TestWebsocketRequestWithEncodedChar was introduced by PR traefik/traefik#2199 which in turn was caused by Issue traefik/traefik#2196. This is the same issue I referenced earlier.

So I think "fixing" this test should be okay. Short of doing that, you could go one level deeper and tear apart the HTTP protocol yourself instead of using the net/http/httptest library to depend on, but I guess this would be too much effort for to little outcome. Using the correct field of the URL struct seems to be enough in my view.

[shieldwed]

How are we going to proceed with this?

[timoreimann]

@marco-jantke is this the issue we've been looking at as well?

[m3co-code]

Sorry for the late response..

@timoreimann no, it's not the same problem. Especially because this is in the realm of websockets and ours, fixed with traefik/traefik#2382 is about things with the strip prefix middleware.

The implementation looks good to me. As I understand @shieldwed explanation, it makes sense to me to adapt the formerly wrong test. My opinion.

[shieldwed]

Partially included and enhanced by #44.