Preserve original encoding of query parameters in ProxyExchangeHandlerFunction

[raccoonback]

Description

This PR improves ProxyExchangeHandlerFunction to ensure that the original encoding of query parameters is preserved when proxying requests.

The return value of ServerRequest.params() is already decoded, so reconstructing the query using it results in the loss of the original encoding information.
To preserve the original encoded state, this PR removes the unnecessary query parameter replacement performed in ProxyExchangeHandlerFunction.handle().

For example.

[AS-IS]
"http://localhost/path?foo=A&bar=B%3D" -> "http://localhost/path?foo=A&bar=B="

[TO-BE]
"http://localhost/path?foo=A&bar=B%3D" -> "http://localhost/path?foo=A&bar=B%3D"

Related issue

• Strip Prefix breaking OAuth Flow - spring-cloud-gateway-server-mvc-4.3.0-M3 #3759

[ryanjbaxter]

Is this only present in the main branch?

[raccoonback]

@ryanjbaxter
Good point.
Since this seems to occur starting from 4.1.x, I'll update it based on the 4.1.x branch.

[Shawyeok]

Related #3795

[Shawyeok]

Since this seems to occur starting from 4.1.x, I'll update it based on the 4.1.x branch.

I'm new to spring-cloud-gateway contribution workflow, does it should be merge to the main branch then have it cherry-picked to the 4.1.x branch?

[Shawyeok]

Since serverRequest.params() can be modified by user-defined filters that precede this step, we should always encode the parameters to ensure correctness.

Additionally, note that the plus symbol (+) is not encoded by UriUtils.encodeQueryParams. To handle this case properly, we should use UriUtils.encode instead, which will correctly encode all special characters, including the plus symbol, you could see more detail in #3795.

	private static MultiValueMap<String, String> encodeQueryParams(MultiValueMap<String, String> params) {
		Charset charset = StandardCharsets.UTF_8;
		MultiValueMap<String, String> result = new LinkedMultiValueMap<>(params.size());
		for (Map.Entry<String, List<String>> entry : params.entrySet()) {
			for (String value : entry.getValue()) {
				result.add(UriUtils.encode(entry.getKey(), charset), UriUtils.encode(value, charset));
			}
		}
		return result;
	}

[raccoonback]

@Shawyeok
Good point. I'll check it.

[Shawyeok]

Please also add a case for encode plus symbol (%2B).

[raccoonback]

@Shawyeok
I have updated it.

[raccoonback]

@Shawyeok

Since this seems to occur starting from 4.1.x, I'll update it based on the 4.1.x branch.

I'm new to spring-cloud-gateway contribution workflow, does it should be merge to the main branch then have it cherry-picked to the 4.1.x branch?

As far as I know, the changes are applied to 4.1.x, 4.2.x, and main.

[ryanjbaxter]

@Shawyeok we merge into the lowest maintained branch and then merge the change forward to the other branches

[Shawyeok]

@Shawyeok we merge into the lowest maintained branch and then merge the change forward to the other branches

@ryanjbaxter Thanks for letting me know! That's interesting! Does this approach help to reduce the cost of maintaining multiple branches? Is there any documentation or notes where I can learn more about this?

[Shawyeok]

User can modify the query parameters in preceding filters, for example, add additional parameters to the request, these parameters (serverRequest.params()) should be encoded regardless of whether the original request url contains % or not.

Suggested change

	MultiValueMap<String, String> params = ensureEncodedQueryParameters(serverRequest);
	MultiValueMap<String, String> params = MvcUtils.encodeQueryParams(serverRequest.params());

[raccoonback]

@Shawyeok

First of all, If query parameters were added in a preceding filter, I believe that filter should be responsible for ensuring those parameters are properly encoded.

serverRequest.params() relies on HttpServletRequest.getParameterMap(), and from what I can tell, most major servlet containers (Tomcat, Jetty, Undertow) return already-decoded values.

So as you pointed out, it's likely safe to assume that serverRequest.params() provides decoded query parameters.

(That said, if we can guarantee that serverRequest.params() always returns decoded values, then it's fine to encode them again here. But if that guarantee isn't solid, we risk double-encoding parameters that were already percent-encoded by earlier filters.)

[Shawyeok]

if we can guarantee that serverRequest.params() always returns decoded values, then it's fine to encode them again here.

IMO, the only reliable way to handle URI encoding is to treat the values returned by serverRequest.params() as raw (i.e. decoded values). We're unable to determine whether a string is URI encoded or not, for example: Is %25 a encoded string? We cannot tell, cause user might intend to send the literal string %25 rather than the percent symbol. Therefore, we should consistently treat all parameter values as decoded and handle encoding at this stage.

[spencergibb]

@ryanjbaxter Thanks for letting me know! That's interesting! Does this approach help to reduce the cost of maintaining multiple branches? Is there any documentation or notes where I can learn more about this?

It does help. There's no documentation on it as we are a small team.