Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

set raw path and path in proxy, so url.EscapePath uses raw path #1628

Merged
merged 1 commit into from Sep 1, 2020
Merged

set raw path and path in proxy, so url.EscapePath uses raw path #1628

merged 1 commit into from Sep 1, 2020

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants