How to escape question mark in proxy rewrite

[eloo]

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]

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

[lammel]

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]

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

[eloo]

@lammel
Seems not be fixed in 4.2.1

[lammel]

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]

@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]

@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]

ok, I see now

[aldas]

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]

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]

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

[eloo]

@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]

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]

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

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

Thanks