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

UriComponentBuilder doesn't work with encoded HTTP URL having '+'. [SPR-14828] #19394

Closed
spring-projects-issues opened this issue Oct 20, 2016 · 17 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Oct 20, 2016

Johnny Lim opened SPR-14828 and commented

'+' is valid for a space but with the following code:

String httpUrl = "http://localhost:8080/test/print?value=%EA%B0%80+%EB%82%98";
URI uri = UriComponentsBuilder.fromHttpUrl(httpUrl).build(true).toUri();
System.out.println(uri);

I got the following error:

java.lang.IllegalArgumentException: Invalid character '+' for QUERY_PARAM in "%EA%B0%80+%EB%82%98"

	at org.springframework.web.util.HierarchicalUriComponents.verifyUriComponent(HierarchicalUriComponents.java:313)
	at org.springframework.web.util.HierarchicalUriComponents.verify(HierarchicalUriComponents.java:281)
	at org.springframework.web.util.HierarchicalUriComponents.<init>(HierarchicalUriComponents.java:90)
	at org.springframework.web.util.UriComponentsBuilder.build(UriComponentsBuilder.java:336)
	at learningtest.org.springframework.web.UriComponentBuilderTests.testFromHttpUrlBuildEncoded(UriComponentBuilderTests.java:19)

Affects: 4.3.3

Reference URL: https://github.com/izeye/spring-boot-throwaway-branches/blob/rest-and-logback-access/src/test/java/learningtest/org/springframework/web/UriComponentBuilderTests.java

Issue Links:

Referenced from: commits f2e293a

1 votes, 6 watchers

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Jan 4, 2017

Rossen Stoyanchev commented

According to the URI spec RFC 3986, a "+" is allowed in the query (i.e. pchar > sub-delimiters > ";"). This matches our own HierarchicalUriComponents.QUERY except QUERY_PARAM further excludes "=", "&", and "+". The "=", "&" make sense since those are commonly used as query parameter delimiters but I'm not sure where the "+" comes from.

Arjen Poutsma what's your take on this? Is the "+" encoded as an extra precaution against it being decoded and interpreted as a space? I found an old discussion on this here. Or could it be based on the older version of the spec RFC 2396 which does seem to call out "+" as a reserved character in the query?

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Jan 9, 2017

Arjen Poutsma commented

It's been a while since I wrote that code, so I can't remember what was the reason for excluding the '+'. My guess would be the same as yours: it was based on an older version of the RFC.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Jan 10, 2017

Alexandre Navarro commented

I have exactly the same problem. Interested to fix it in a future version.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Oct 3, 2017

Kazuki Shimizu commented

Hi,

Probably by this changes, + does't encode to %2B .
Is this behavior working as design?
In this case, I think it should encode to the %2B because there is a case that + is decoded as space by the application server.
What do you think?

@Test
public void test() {
    // http://localhost:8080/?q=+
    System.out.println(UriComponentsBuilder.fromHttpUrl(
            "http://localhost:8080/").queryParam("q", "+").build().encode()
            .toUri());
    // http://localhost:8080/?q=%20
    System.out.println(UriComponentsBuilder.fromHttpUrl(
            "http://localhost:8080/").queryParam("q", " ").build().encode()
            .toUri());
    // http://localhost:8080/?q=%26
    System.out.println(UriComponentsBuilder.fromHttpUrl(
            "http://localhost:8080/").queryParam("q", "&").build().encode()
            .toUri());
    // http://localhost:8080/?q=%3D
    System.out.println(UriComponentsBuilder.fromHttpUrl(
            "http://localhost:8080/").queryParam("q", "=").build().encode()
            .toUri());
}

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Oct 3, 2017

Rossen Stoyanchev commented

This is by design indeed. Only reserved characters are reserved but "+" is allowed. Is this through the RestTemplate? If yes take a look at the uriTemplateHandler and the DefaultUriTemplateHandler implementation (DefaultUriBuilderFactory in 5.0). It has a strict encoding property.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Oct 4, 2017

Kazuki Shimizu commented

Thanks for quick answer!

RestTemplate ?

No, I don't use the RestTemplate in my application.
I was making a query string for pagination link using the UriComponentsBuilder like above example.
There is a case that includes 0x2b(+) *1 in a pagination link because it is a value that input by an end user.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Oct 4, 2017

Kazuki Shimizu commented

Oh... Sorry, I click the "Add" button by mistake.

I understood that need to change to use an another APIs.
Thanks.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Nov 1, 2017

Dmitry Katsubo commented

Rossen Stoyanchev: I've tried to locate "strict" mode in DefaultUriBuilderFactory, but it has only URI_COMPONENT in enum EncodingMode which is the default. So this one does not encode (escape) + in path and query:

assertEquals("http://localhost:8080/test+/print?value=%25EA%25B0%2580+%25EB%2582%2598", new DefaultUriBuilderFactory().uriString("http://localhost:8080/{path}/print?value=%EA%B0%80+%EB%82%98").build("test+").toString());

DefaultUriTemplateHandler encodes only path, but not query:

DefaultUriTemplateHandler defaultUriTemplateHandler = new DefaultUriTemplateHandler();

defaultUriTemplateHandler.setStrictEncoding(true);

assertEquals("http://localhost:8080/test%2B/print?value=%EA%B0%80+%EB%82%98", defaultUriTemplateHandler.expand("http://localhost:8080/{path}/print?value=%EA%B0%80+%EB%82%98", "test+").toString());

Now I have difficulty in how to escape query part. Actually in my case I start from unencoded URL string, hence all characters that have special meaning should be encoded. How to do it?

assertEquals("http://localhost:8080/test+/print?value=v1%20v2%2Bv3", UriComponentsBuilder.fromHttpUrl("http://localhost:8080/{path}/print?value={val}").buildAndExpand("test+", "v1 v2+v3").encode().toUri().toString());

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Nov 2, 2017

Rossen Stoyanchev commented

Dmitry Katsubo, the EncodingMode is an enum with 3 values. You need this:

DefaultUriBuilderFactory factory = new DefaultUriBuilderFactory();
factory.setEncodingMode(EncodingMode.VALUES_ONLY);
URI uri = factory.uriString("http://localhost:8080/{path}/print?value=%EA%B0%80+%EB%82%98").build("test+");

Note that strict encoding is only applied to expanded URI variable values (and the same is true for DefaultUriTemplateHandler). This is why your query is not getting encoded. See the Javadoc for details.

Kazuki Shimizu the above DefaultUriBuilderFactory internally uses UriComponentsBuilder and essentially does this:

List<String> vars = UriUtils.encodeUriVariables("+");
System.out.println(UriComponentsBuilder.fromHttpUrl(
            "http://localhost:8080/?").queryParam("q", "{value}").build().expand(vars)
            .toUri());

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Nov 2, 2017

Dmitry Katsubo commented

OK, here is an example which I expect to succeeded because I set query parameter as unencoded value with intent that + should be passed literally to remote server hence should be encoded:

DefaultUriBuilderFactory factory = new DefaultUriBuilderFactory();
factory.setEncodingMode(EncodingMode.VALUES_ONLY);
UriBuilder uriString = factory.uriString("http://localhost/path");
uriString.queryParam("value", "v1+v2");

assertEquals("http://localhost/path?value=v1%2Bv2", uriString.build().toString());

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Nov 2, 2017

Rossen Stoyanchev commented

Did you miss this part "strict encoding is only applied to expanded URI variable values" ?

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Nov 3, 2017

Dmitry Katsubo commented

Is it possible to further extend API so that one can encode URL query parameters in one go? Namely:

  • Add method ```
    UriComponentsBuilder#getQueryParams() { return queryParams; }
* Make methods `UriUtils.encodeUriVariables()` public.

Then it would be possible to encode query params in one go, without extra substitution step.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Nov 3, 2017

Rossen Stoyanchev commented

I've made UriUtils.encodeUriVariables() public. As for UriComponentsBuilder#getQueryParams(), I don't think we can add just one accessor, there are no others on the builder. Is there a reason you can't call build() first and then access the queryParams?

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Nov 3, 2017

Dmitry Katsubo commented

build() transforms query params into non-mutable state, see HierarchicalUriComponents constructor, line 96. So at the moment it is only possible to build(), then get query params, encode them, put back via UriComponentsBuilder#replaceQueryParams(MultiValueMap<String, String> params) and then build() again...
Another alternative is to encapsulate UriComponentsBuilder the same way as DefaultUriBuilderFactory$DefaultUriBuilder does and do necessary preparations using UriUtils.encodeUriVariables() in corresponding methods (queryParam(), replaceQueryParam(), ...).
Any better idea?

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Nov 3, 2017

Rossen Stoyanchev commented

I see, or you could build a MultiValueMap of query params, use UriUtils to encode, and pass into UriComponentsBuilder#query. The only catch is we need to enhance UriUtils to support MultiValueMaps.

I think adding another encoding mode to DefaultUriBuilderFactory.EncodingMode would be perfectly okay to do. It's exactly what it's meant for -- provide control over encoding strategies. However I am not sure I understand exactly what the new mode is, i.e. aside from fully encoding query params, how does it work for others?

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Nov 4, 2017

Dmitry Katsubo commented

I tried somehow to apply the behaviour I need using the strategies that I've mentioned above, but honestly I've failed. The problem is that once I replace + with %2B, it (correctly) gets double-encoded as %252B when encode() is called. So there are not so many alternatives: either use queryParam("q", "{value}").build().expand(vars) as advised above or do some massaging of URL after it is built.

At the moment I've decided to switch to javax.ws.rs.core.UriBuilder which provides very similar API with minor effect that last one is more strict and %-encodes (among others) , as %2C and / as %2F as well, but that is perfectly OK as receiver should interpret that correctly.

If somebody is interested in code I came up with, here it goes:

public static class UriBuilderFactory extends UriComponentsBuilder {
	
	public static UriComponentsBuilder fromHttpUrl(String httpUrl) {
		return new UriBuilderFactory(UriComponentsBuilder.fromHttpUrl(httpUrl));
	}

	public UriBuilderFactory(UriComponentsBuilder other) {
		super(other);
	}
	
	@Override
	// queryParams(), replaceQueryParam() and replaceQueryParams() should be overridden and values should be processed in similar way 
	public UriComponentsBuilder queryParam(String name, Object... values) {
		return super.queryParam(name, encode(values));
	}
	
	// To be replaced by UriUtils.encodeUriVariables()
	private static Object[] encode(Object[] values) {
		Object[] encodedValues = new Object[values.length];
		
		for (int i = 0; i < values.length; i++) {
			if (values[i] != null) {
				encodedValues[i] = values[i].toString().replace("+", "%2B");
			}
		}
		
		return encodedValues;
	}
}

// Looks OK however not calling encode() has other side effects like passing through '=' which should be encoded:
assertEquals("http://host.com/path?key=%2Ba,b%2Bc&d=e/", UriBuilderFactory.fromHttpUrl("http://host.com/{var}").queryParam("key", "%2Ba,b+c&d=e/").buildAndExpand("path").toUriString());

// This implicitly calls encode() and hence '+' gets double-encoded:
assertEquals("http://host.com/path?key=%252Ba,b%252Bc%26d%3De/", UriBuilderFactory.fromHttpUrl("http://host.com/{var}").queryParam("key", "%2Ba,b+c&d=e/").build("path").toString());

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Jul 13, 2018

Rossen Stoyanchev commented

Dmitry Katsubo, Kazuki Shimizu I've created #21577 which should provide a solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants