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

How to escape question mark in proxy rewrite #1798

Closed
eloo opened this issue Mar 3, 2021 · 14 comments · Fixed by #1799 or #1802
Closed

How to escape question mark in proxy rewrite #1798

eloo opened this issue Mar 3, 2021 · 14 comments · Fixed by #1799 or #1802

Comments

@eloo
Copy link

eloo commented Mar 3, 2021

Issue Description

Hi,
im currently trying to something with the proxy middleware and i want to proxy a redirect to an upstream url and i want to automatically append some parameters.
But sadly the question mark (?) to indicate that now parameters are following is automatically encoded in the proxy and so my upstream is not accepting it as parameter start.

Checklist

  • [/] Dependencies installed
  • [/] No typos
  • [/] Searched existing issues and docs

Expected behaviour

My request is forwarded like this

/proxy/pull-requests -> /stash/rest/api/latest/dashboard/pull-requests?role=AUTHOR&limit=1000

Actual behaviour

the question mark in my request is escaped

/proxy/pull-requests -> /stash/rest/api/latest/dashboard/pull-requests%3Frole=AUTHOR&limit=1000

Steps to reproduce

Working code to debug

	pullrequestsProxy := middleware.ProxyWithConfig(middleware.ProxyConfig{
		Balancer: middleware.NewRoundRobinBalancer([]*middleware.ProxyTarget{
			{
				URL: bitbucketUrl,
			},
		},
		),
		Rewrite: map[string]string{
			"^/proxy/pull-requests": "/stash/rest/api/latest/dashboard/pull-requests?role=AUTHOR&limit=1000",
		},
       }

Version/commit

Version: 4.2.0

Thanks

@aldas
Copy link
Contributor

aldas commented Mar 4, 2021

I think this relates to #1799 and url.Path/RawPath

@lammel
Copy link
Contributor

lammel commented Mar 5, 2021

I think this relates to #1799 and url.Path/RawPath

I think so too. The rewrite should be considered as an already escaped URL path, otherwise it will be hard to support an URL with query params, which is a valid usecase.

lammel added a commit that referenced this issue Mar 8, 2021
* Fix performance regression #1777 and avoid double escaping in rewrite/proxy middleware.
* Add rewrite test for correct escaping of replacement (#1798)

Co-authored-by: Roland Lammel <rl@neotel.at>
@lammel
Copy link
Contributor

lammel commented Mar 8, 2021

Should be fixed by #1799 and the upcoming echo v4.2.1

@eloo
Copy link
Author

eloo commented Mar 8, 2021

@lammel
Seems not be fixed in 4.2.1

@lammel
Copy link
Contributor

lammel commented Mar 8, 2021

In that case the added unit tests do not reflect the real world and are based on a wrong assumption.
The request path is actually set correctly, probably the Request-URI is not.
Thanks for the feedback.

@aldas
Copy link
Contributor

aldas commented Mar 8, 2021

@eloo could you provide example

For example when I try on master

func main() {
	upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		url := r.URL.String()
		fmt.Println(url)
		w.Write([]byte(url + "\n"))
	}))
	defer upstream.Close()
	url, _ := url.Parse(upstream.URL)
	rrb := middleware.NewRoundRobinBalancer([]*middleware.ProxyTarget{{Name: "upstream", URL: url}})

	e := echo.New()
	e.Use(middleware.ProxyWithConfig(middleware.ProxyConfig{
		Balancer: rrb,
		Rewrite: map[string]string{
			"^/a/*": "/v1/$1",
		},
	}))

	e.Start(":8080")
}

I'll send 1 request

x@x:~/code/echo curl -v "http://localhost:8080/a/test/pull-requests?role=AUTHOR&limit=1000"
*   Trying 127.0.0.1:8080...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET /a/test/pull-requests?role=AUTHOR&limit=1000 HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.68.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Length: 46
< Content-Type: text/plain; charset=utf-8
< Date: Mon, 08 Mar 2021 08:09:41 GMT
< 
/v1/test/pull-requests?role=AUTHOR&limit=1000
* Connection #0 to host localhost left intact

In application out

   ____    __
  / __/___/ /  ___
 / _// __/ _ \/ _ \
/___/\__/_//_/\___/ v4.2.1
High performance, minimalist Go web framework
https://echo.labstack.com
____________________________________O/_______
                                    O\
⇨ http server started on [::]:8080
/v1/test/pull-requests?role=AUTHOR&limit=1000

@lammel
Copy link
Contributor

lammel commented Mar 8, 2021

@aldas I think the issue here is that the query params are set within the replacement string of a rewrite rule (not the input URL being processed by the rule) appending ?role=AUTHOR&limit=1000 there when applying the replacement, which seems to be escaped when sending the request to the backend server.

@aldas
Copy link
Contributor

aldas commented Mar 8, 2021

ok, I see now

@aldas
Copy link
Contributor

aldas commented Mar 8, 2021

problem is that role=AUTHOR&limit=1000 is not path. this is query part of url.

Probably could be fixed in rewritePath by doing

			path := replacer.Replace(v)
			req.URL, _ = req.URL.Parse(path)

This way path and query parts are set correctly from replaced path

@lammel
Copy link
Contributor

lammel commented Mar 8, 2021

problem is that role=AUTHOR&limit=1000 is not path. this is query part of url.

Probably could be fixed in rewritePath by doing

			path := replacer.Replace(v)
			req.URL, _ = req.URL.Parse(path)

This way path and query parts are set correctly from replaced path

I think that should work. The unit tests I added yesterday for in rewrite_test.go should be adjusted then to check against the resulting URL (I think I only used the path there, which was the wrong assumption)

@lammel
Copy link
Contributor

lammel commented Mar 9, 2021

@eloo Could you please verify if PR #1802 fixes your issue?

@eloo
Copy link
Author

eloo commented Mar 9, 2021

@lammel not sure how i can tests a PR?

but for the code it looks better.. at least there seems to be a test which should cover my use case

@lammel
Copy link
Contributor

lammel commented Mar 9, 2021

I usually check out echo locally and use the repo/branch of the PR and use the replace directive of go.mod.

sh# git clone https://github.com/aldas/echo.git /tmp/echo
sh# cd /tmp/echo
sh# git checkout fix_1798_proxy_question_mark

And then edit your go.mod:

replace github.com/labstack/echo/v4 => /tmp/echo

(I hope I made no typos, just out of my head)

@eloo
Copy link
Author

eloo commented Mar 9, 2021

@lammel
thanks for this tip. works like a charm!

and yes it looks like the bug will be fixed with the PR.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants