UriTemplate handles encoded variables improperly on expand call

[billyyarosh]

When testing the UriTemplate, we ran into several issues with encoding. The URI template, when given a path like /foo%20bar will return /foo%252520bar on a call to expand. It looks like both the hateoas UriTemplate and the spring UriTemplate are not handling ecoded URLs. Also if you provide a URL with a character needing encoding, you get the % sign escaped only once.

Here is a spock test to shows the errors. The last three tests will fail due to the issues.

@Unroll("template expand with params '#params' and uri '#uri' results in '#expectedUri ")
    def "template expands with parameter map"() {
        when:
        UriTemplate template = new UriTemplate(uri)
        String expandedUri = template.expand(params)

        then:
        expandedUri == expectedUri

        where:
        uri                             | params                      | expectedUri
        '/foo/bar{?x}'                  | ['x': 1]                    | '/foo/bar?x=1'
        '/foo/bar{?x,y}'                | ['x': 1, 'y': "2"]          | '/foo/bar?x=1&y=2'
        '/foo/bar{?x}{&y}'              | ['x': 1, 'y': "2"]          | '/foo/bar?x=1&y=2'
        '/foo/bar?x=1{&y}'              | ['y': 2]                    | '/foo/bar?x=1&y=2'
        '/foo/bar?x=1{&y,z}'            | ['y': 2, 'z': 3L]           | '/foo/bar?x=1&y=2&z=3'
        '/foo{/x}'                      | ['x': 1]                    | '/foo/1'
        '/foo{/x,y}'                    | ['x': 1, 'y': "2"]          | '/foo/1/2'
        '/foo{/x}{/y}'                  | ['x': 1, 'y': "2"]          | '/foo/1/2'
        '/foo{/x}{/y}{?z}'              | ['x': 1, 'y': "2", 'z': 3L] | '/foo/1/2?z=3'
        '/foo/{x}'                      | ['x': 1]                    | '/foo/1'
        '/foo/{x}/bar'                  | ['x': 1]                    | '/foo/1/bar'
        '/services/foo/{x}/bar/{y}/gaz' | ['x': 1, 'y': "2"]          | '/services/foo/1/bar/2/gaz'
        '/foo/{x}/bar/{y}/bar{?z}'      | ['x': 1, 'y': "2", 'z': 3L] | '/foo/1/bar/2/bar?z=3'
        '/foo/{x}/bar/{y}/bar{?z}'      | ['x': 1, 'y': "2"]          | '/foo/1/bar/2/bar'
        '/foo/bar{?x,y,z}'              | ['x': 1]                    | '/foo/bar?x=1'
        '/foo/bar{?x,y,z}'              | ['x': 1, 'y': "2"]          | '/foo/bar?x=1&y=2'
        '/foo/bar{?x,y,z}'              | ['x': 1, 'z': 3L]           | '/foo/bar?x=1&z=3'
        '/foo/{x}/bar{/y}{?z}'          | ['x': 1, 'y': "2", 'z': 3L] | '/foo/1/bar/2?z=3'
        '/foo/{x}/bar{/y}{?z}'          | ['x': 1, 'z': 3L]           | '/foo/1/bar?z=3'
        '/foo/{x}/bar{?y}{#z}'          | ['x': 1, 'y': "2"]          | '/foo/1/bar?y=2'
        '/foo/{x}/bar{?y}{#z}'          | ['x': 1, 'y': "2", 'z': 3L] | '/foo/1/bar?y=2#3'
        '/foo/{x}/bar{?y}{#z}'          | ['x': 1, 'z': 3L]           | '/foo/1/bar#3'
        '/foo/b%20ar{?x}'               | ['x': 1]                    | '/foo/b%20ar?x=1'
        '/foo/b"ar{?x}'                 | ['x': 1]                    | '/foo/b%22ar?x=1'
        '/foo/b%22ar{?x}'               | ['x': 1]                    | '/foo/b%22ar?x=1'
    }

[gregturn]

@keaplogik That looks like a great test case! Certainly guard against regressions. Since Spring HATEOAS doesn't use Spock, I can work on folding that into our test suite while additionally tracking down the holes (Spring Framework or Spring HATEOAS).

I also would like to test this against the Affordances API branch, which makes several changes to UriTemplate.

[gregturn]

I have converted that test case to a JUnit/Hamcrest based one:

@Test
public void uriTemplateExpansionsShouldWork() {

	assertThat(new UriTemplate("/foo/bar{?x}").expand(new HashMap<String, Object>() {{ put("x", 1); }}).expand().toUri().toString(), is("/foo/bar?x=1"));
	assertThat(new UriTemplate("/foo/bar{?x,y}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).expand().toUri().toString(), is("/foo/bar?x=1&y=2"));
	assertThat(new UriTemplate("/foo/bar{?x}{&y}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).expand().toUri().toString(), is("/foo/bar?x=1&y=2"));
	assertThat(new UriTemplate("/foo/bar?x=1{&y}").expand(new HashMap<String, Object>() {{ put("y", 2); }}).expand().toUri().toString(), is("/foo/bar?x=1&y=2"));
	assertThat(new UriTemplate("/foo/bar?x=1{&y,z}").expand(new HashMap<String, Object>() {{ put("y", 2); put("z", 3L); }}).expand().toUri().toString(), is("/foo/bar?x=1&y=2&z=3"));
	assertThat(new UriTemplate("/foo{/x}").expand(new HashMap<String, Object>() {{ put("x", 1); }}).expand().toUri().toString(), is("/foo/1"));
	assertThat(new UriTemplate("/foo{/x,y}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).expand().toUri().toString(), is("/foo/1/2"));
	assertThat(new UriTemplate("/foo{/x}{/y}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).expand().toUri().toString(), is("/foo/1/2"));
	assertThat(new UriTemplate("/foo{/x}{/y}{?z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); put("z", 3L); }}).expand().toUri().toString(), is("/foo/1/2?z=3"));
	assertThat(new UriTemplate("/foo/{x}").expand(new HashMap<String, Object>() {{ put("x", 1); }}).expand().toUri().toString(), is("/foo/1"));
	assertThat(new UriTemplate("/foo/{x}/bar").expand(new HashMap<String, Object>() {{ put("x", 1); }}).expand().toUri().toString(), is("/foo/1/bar"));
	assertThat(new UriTemplate("/services/foo/{x}/bar/{y}/gaz").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).expand().toUri().toString(), is("/services/foo/1/bar/2/gaz"));
	assertThat(new UriTemplate("/foo/{x}/bar/{y}/bar{?z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); put("z", 3L); }}).expand().toUri().toString(), is("/foo/1/bar/2/bar?z=3"));
	assertThat(new UriTemplate("/foo/{x}/bar/{y}/bar{?z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).expand().toUri().toString(), is("/foo/1/bar/2/bar"));
	assertThat(new UriTemplate("/foo/{x}/bar/{y}/bar{?z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).expand().toUri().toString(), is("/foo/1/bar/2/bar"));
	assertThat(new UriTemplate("/foo/bar{?x,y,z}").expand(new HashMap<String, Object>() {{ put("x", 1); }}).expand().toUri().toString(), is("/foo/bar?x=1"));
	assertThat(new UriTemplate("/foo/bar{?x,y,z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).expand().toUri().toString(), is("/foo/bar?x=1&y=2"));
	assertThat(new UriTemplate("/foo/bar{?x,y,z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("z", 3L); }}).expand().toUri().toString(), is("/foo/bar?x=1&z=3"));
	assertThat(new UriTemplate("/foo/{x}/bar{/y}{?z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); put("z", 3L); }}).expand().toUri().toString(), is("/foo/1/bar/2?z=3"));
	assertThat(new UriTemplate("/foo/{x}/bar{/y}{?z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("z", 3L); }}).expand().toUri().toString(), is("/foo/1/bar?z=3"));
	assertThat(new UriTemplate("/foo/{x}/bar{?y}{#z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).expand().toUri().toString(), is("/foo/1/bar?y=2"));
	assertThat(new UriTemplate("/foo/{x}/bar{?y}{#z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); put("z", 3L); }}).expand().toUri().toString(), is("/foo/1/bar?y=2#3"));
	assertThat(new UriTemplate("/foo/{x}/bar{?y}{#z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("z", 3L); }}).expand().toUri().toString(), is("/foo/1/bar#3"));
	assertThat(new UriTemplate("/foo/b%20ar{?x}").expand(new HashMap<String, Object>() {{ put("x", 1); }}).expand().toUri().toString(), is("/foo/b%20ar?x=1"));
	assertThat(new UriTemplate("/foo/b\"ar{?x}").expand(new HashMap<String, Object>() {{ put("x", 1); }}).expand().toUri().toString(), is("/foo/b%22ar?x=1"));
	assertThat(new UriTemplate("/foo/b%22ar{?x}").expand(new HashMap<String, Object>() {{ put("x", 1); }}).expand().toUri().toString(), is("/foo/b%22ar?x=1"));
}

On the master branch it fails on /foo/b%20ar{?x} because of the percentage getting double encoded. On the Affordances branch (which overhauls UriTemplate to allow partial expansions) does NOT fail, because it only tries to encode variables. This causes that branch to fail on /foo/b"ar{?x}.

The real solution cannot materialize until that branch is reviewed and merged, and then we can probably migrate to Spring's UriComponentsBuilder properly handle each path component.

So we can hold this test here until the time is right to offer a corresponding fix.

[gregturn]

Depends on #581

[billyyarosh]

Thanks for the prompt response.Good to know this information.

[mle-enso]

This bothers us also heavily. Anyone who needs help fixing this issue as well as the related blocking ones, please let me know!

[mle-enso]

Now that #581 is closed and merged what does this mean for the work still to do for this issue here?

[gregturn]

We must instate he test cases and track down the issues.

[gregturn]

I have updated the test case, and verified it still breaks at the same place:

	@Test
	public void uriTemplateExpansionsShouldWork() {

		assertThat(new UriTemplate("/foo/bar{?x}").expand(new HashMap<String, Object>() {{ put("x", 1); }}).toString()).isEqualTo("/foo/bar?x=1");
		assertThat(new UriTemplate("/foo/bar{?x,y}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).toString()).isEqualTo("/foo/bar?x=1&y=2");
		assertThat(new UriTemplate("/foo/bar{?x}{&y}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).toString()).isEqualTo("/foo/bar?x=1&y=2");
		assertThat(new UriTemplate("/foo/bar?x=1{&y}").expand(new HashMap<String, Object>() {{ put("y", 2); }}).toString()).isEqualTo("/foo/bar?x=1&y=2");
		assertThat(new UriTemplate("/foo/bar?x=1{&y,z}").expand(new HashMap<String, Object>() {{ put("y", 2); put("z", 3L); }}).toString()).isEqualTo("/foo/bar?x=1&y=2&z=3");
		assertThat(new UriTemplate("/foo{/x}").expand(new HashMap<String, Object>() {{ put("x", 1); }}).toString()).isEqualTo("/foo/1");
		assertThat(new UriTemplate("/foo{/x,y}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).toString()).isEqualTo("/foo/1/2");
		assertThat(new UriTemplate("/foo{/x}{/y}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).toString()).isEqualTo("/foo/1/2");
		assertThat(new UriTemplate("/foo{/x}{/y}{?z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); put("z", 3L); }}).toString()).isEqualTo("/foo/1/2?z=3");
		assertThat(new UriTemplate("/foo/{x}").expand(new HashMap<String, Object>() {{ put("x", 1); }}).toString()).isEqualTo("/foo/1");
		assertThat(new UriTemplate("/foo/{x}/bar").expand(new HashMap<String, Object>() {{ put("x", 1); }}).toString()).isEqualTo("/foo/1/bar");
		assertThat(new UriTemplate("/services/foo/{x}/bar/{y}/gaz").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).toString()).isEqualTo("/services/foo/1/bar/2/gaz");
		assertThat(new UriTemplate("/foo/{x}/bar/{y}/bar{?z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); put("z", 3L); }}).toString()).isEqualTo("/foo/1/bar/2/bar?z=3");
		assertThat(new UriTemplate("/foo/{x}/bar/{y}/bar{?z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).toString()).isEqualTo("/foo/1/bar/2/bar");
		assertThat(new UriTemplate("/foo/{x}/bar/{y}/bar{?z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).toString()).isEqualTo("/foo/1/bar/2/bar");
		assertThat(new UriTemplate("/foo/bar{?x,y,z}").expand(new HashMap<String, Object>() {{ put("x", 1); }}).toString()).isEqualTo("/foo/bar?x=1");
		assertThat(new UriTemplate("/foo/bar{?x,y,z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).toString()).isEqualTo("/foo/bar?x=1&y=2");
		assertThat(new UriTemplate("/foo/bar{?x,y,z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("z", 3L); }}).toString()).isEqualTo("/foo/bar?x=1&z=3");
		assertThat(new UriTemplate("/foo/{x}/bar{/y}{?z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); put("z", 3L); }}).toString()).isEqualTo("/foo/1/bar/2?z=3");
		assertThat(new UriTemplate("/foo/{x}/bar{/y}{?z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("z", 3L); }}).toString()).isEqualTo("/foo/1/bar?z=3");
		assertThat(new UriTemplate("/foo/{x}/bar{?y}{#z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); }}).toString()).isEqualTo("/foo/1/bar?y=2");
		assertThat(new UriTemplate("/foo/{x}/bar{?y}{#z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("y", "2"); put("z", 3L); }}).toString()).isEqualTo("/foo/1/bar?y=2#3");
		assertThat(new UriTemplate("/foo/{x}/bar{?y}{#z}").expand(new HashMap<String, Object>() {{ put("x", 1); put("z", 3L); }}).toString()).isEqualTo("/foo/1/bar#3");
		assertThat(new UriTemplate("/foo/b%20ar{?x}").expand(new HashMap<String, Object>() {{ put("x", 1); }}).toString()).isEqualTo("/foo/b%20ar?x=1");
		assertThat(new UriTemplate("/foo/b\"ar{?x}").expand(new HashMap<String, Object>() {{ put("x", 1); }}).toString()).isEqualTo("/foo/b%22ar?x=1");
		assertThat(new UriTemplate("/foo/b%22ar{?x}").expand(new HashMap<String, Object>() {{ put("x", 1); }}).toString()).isEqualTo("/foo/b%22ar?x=1");
	}

Inspecting both Spring HATEOAS and Spring Framework, there appears no current way to stop encoding. Hence it indeed gets encoded by both.

I'm inquiring what the proper approach is to fix this bug.

[gregturn]

Since this intersects with Spring Framework's underlying UriTemplate utility class, I opened https://jira.spring.io/browse/SPR-16279 to extend this discussion.

[mdvorak]

The bug still exists in 0.24.RELEASE.

According to the comment from Spring, its question whether it should be changed in spring mvc or in hateoas. As I look at the sources, it should be fixable on the library side - just mark UriComponentsBuilder as already encoded (I know it won't be that simple). Or don't encode anything that is passed to the builder itself (its explicitly encoded in many cases).

Now it breaks whenever any identifier (both path variable or request) contains space or other special character, without any way to work around it. Some kind of fix (even temporary) is needed ASAP.
Thanks

[gregturn]

I know the bug still exists. To solve it requires either a critical update to Spring Framework's support for UriTemplates, or a rewrite of how Spring HATEOAS uses Spring Framework. Based on comments on SPR-16279, it looks like we'll have to revisit how we use it.

The trick in all of this is to recognize when encoding is needed and when it's not needed. Otherwise, you can double encode something, or not encode it all, in a given scenario.

[odrotbohm]

After some work on this in the course of #1127 I found this one and coded a fix for that. We're now trying to detect whether the original URI source string needs to be encoded by trying to decode it and comparing lengths.