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

Keep encoded plus signs in query parameters intact #3448

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dvag-yannick-reifschneider

This change keeps encoded + sign intact in query parameters. Otherwise the plus sign gets decoded and is then interpreted as space character.

@gatewaynovice
Copy link

I ran into the same issue, I hope this can be merged soon and available in the next release.

@@ -84,14 +84,12 @@ private void init() {
@Override
public ServerResponse handle(ServerRequest serverRequest) {
URI uri = uriResolver.apply(serverRequest);
boolean encoded = containsEncodedQuery(serverRequest.uri(), serverRequest.params());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this check being removed? Can we assume URI is always encoded?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why this check was added in the first place. In my opinion, spring-cloud-gateway should not encode or decode parameters or URI parts, but leave them untouched. A similar change is also proposed in https://github.com/spring-cloud/spring-cloud-gateway/pull/3437/files

Copy link

@gatewaynovice gatewaynovice Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the intent may be if the incoming URI is already encoded, keep the encoding...otherwise don't. Not working that way though, I'm seeing the request is decoded by the time it reaches the backend.

The key is handling replaceQueryParms piece below, may be we need to encode the request parms if the request is already encoded ?

something like

    boolean encoded = containsEncodedQuery(serverRequest.uri(), serverRequest.params());
    var parms = serverRequest.params();;
    if(encoded) {
        //pseudo code
        parms = encode(serverRequest.params());
    }
URI url = UriComponentsBuilder.fromUri(serverRequest.uri())
			.scheme(uri.getScheme())
			.host(uri.getHost())
			.port(uri.getPort())
			.replaceQueryParams(parms))
			.build(encoded)
			.toUri();

// @formatter:off
URI url = UriComponentsBuilder.fromUri(serverRequest.uri())
.scheme(uri.getScheme())
.host(uri.getHost())
.port(uri.getPort())
.replaceQueryParams(serverRequest.params())
Copy link

@dmyronenko dmyronenko Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you add/change some query params in pre filters they will not be added to request I guess

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I assumed query params were already included in serverRequest.uri(). I think this needs an additional integration test.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been trying to fix this issue in our project aswell, and what I find to be working best is this

URI url = UriComponentsBuilder.fromUri(serverRequest.uri())
            .scheme(uri.getScheme())
            .host(uri.getHost())
            .port(uri.getPort())
            .replaceQueryParams(serverRequest.params())
            .encode()
            .build()
            .toUri();

It solves all cases we have found so far.

@gatewaynovice
Copy link

This issue is closely related to #2065 which has been open for a while now.

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

Successfully merging this pull request may close these issues.

5 participants