Skip to content

Conversation

@arun0009
Copy link
Contributor

@arun0009 arun0009 commented Aug 28, 2020

Some context:

Golang recommends using EscapedPath instead of calling u.RawPath directly.

URL's String method uses the EscapedPath method to obtain the path. See the EscapedPath method for more details.

Go reverseproxy uses escapedPath to create ReverseProxy URL.

What's escapedPath?

EscapedPath returns the escaped form of u.Path. In general, there are multiple possible escaped forms of any path. EscapedPath returns u.RawPath when it is a valid escaping of u.Path. Otherwise EscapedPath ignores u.RawPath and computes an escaped form on its own. The String and RequestURI methods use EscapedPath to construct their results. In general, code should call EscapedPath instead of reading u.RawPath directly.

Issue(s): So if we don't set RawPath as a valid escape of Path, EscapedPath ignores RawPath and computes on its own. This can cause double escaping when using reverseproxy when you have escaped path parameters e.g.

proxying : /user/jill/order/T%2FcO4lW%2Ft%2FVp%2F would construct proxy URL as : /users/jill/orders/T%252FcO4lW%252Ft%252FVp%252F (double escaping) 

This PR sets Path and RawPath in proxy.go and rewrite.go using rewritePath from middleware.go and updated proxy_test.go rewrite_test.go to assert with Url String method.

References: golang/go#41082 (comment)
URL double escaping if the raw path isn't set: https://play.golang.org/p/rOrVzW8ZJCQ

Also, rewrite was updated with the same logic:

e.g.

e.Pre(middleware.Rewrite(map[string]string{	"/api/*": "/$1", })) e.POST("/test/*", func(context echo.Context) error {	return context.String(http.StatusAccepted, "test endpoint called") }) 

before PR curling: /api/test/EbnfwIoiiXbtr6Ec44sfedeEsjrf0RcXkJneYukTXa%2BIFVla4ZdfRiMzfh%2FEGs7f returns {"message":"Not Found"}

with this PR curling /api/test/EbnfwIoiiXbtr6Ec44sfedeEsjrf0RcXkJneYukTXa%2BIFVla4ZdfRiMzfh%2FEGs7f returns "test endpoint called"

@codecov
Copy link

codecov bot commented Aug 28, 2020

Codecov Report

Merging #1628 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #1628 +/- ## ========================================== + Coverage 85.12% 85.15% +0.02%  ========================================== Files 28 28 Lines 2192 2196 +4 ========================================== + Hits 1866 1870 +4  Misses 212 212 Partials 114 114 
Impacted Files Coverage Δ
echo.go 85.93% <100.00%> (-0.22%) ⬇️
middleware/middleware.go 100.00% <100.00%> (ø)
middleware/proxy.go 66.33% <100.00%> (+0.33%) ⬆️
middleware/rewrite.go 70.37% <100.00%> (+1.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb84205...1a6ec73. Read the comment docs.

@lammel lammel self-assigned this Aug 31, 2020
@lammel
Copy link
Contributor

lammel commented Aug 31, 2020

Looking good so far!
Haven't come around to check on performance regressions yet.

@arun0009: Does it make sense to use URL.String() in the tests, although the actual purpose for testing is against the encoded path where URL.EscapedPath() would suffice. So either using EscapedPath() or adding additional tests that check on other parts of the URL (like fragment, query params, ...) would make sense.

…ed to middleware - used by proxy and rewrite
@arun0009
Copy link
Contributor Author

Updated test to check expected against url.EscapedPath().

@lammel
Copy link
Contributor

lammel commented Sep 1, 2020

No performance regression noticable, no API changes.
Should not affect standard routes, as usually no escaping is required.

On release we should note the corrected handling of URL Paths though, as some applications out there have workarounds for the current behaviour.

Thanks @arun0009

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants