Support RFC 6570 style URI template encoding [SPR-12942]

[spring-projects-issues]

daniel carter opened SPR-12942 and commented

As per RFC6570, the URI template "http://host/path/${var1}" where var1=a/b should expand to "http://host/path/a%2Fb"

actual result is "http://host/path/a/b"

Current spring behaviour, as per RFC6570, should only occur when the + operator is specified, ie "http://host/path/${+var1}"

Relevant parts of the spec:
3.2.1 Variable Expansion

The allowed set for a given expansion depends on the expression type: reserved ("+") and fragment ("#") expansions allow the set of characters in the union of ( unreserved / reserved / pct-encoded ) to be passed through without pct-encoding, whereas all other expression types allow only unreserved characters to be passed through without pct-encoding.

Section 3.2.3 shows an explicit example illustrating that / should be encoded unless the + operator is used. (base := "http://example.com/home/")

{base}index http%3A%2F%2Fexample.com%2Fhome%2Findex
{+base}index http://example.com/home/index

Spring is also falling over with it's new ${/var} support as it is failing to add the /

3.2.6

For each defined variable in the variable-list, append "/" to the result string and then perform variable expansion

Here are some test cases to further illustrate the issue, and a comparison with another RFC6570 template library. Run against 4.2.0.BUILD-SNAPSHOT

import org.hamcrest.Matchers;
import org.junit.Assert;
import org.junit.Test;

/**
 * @author cartedan
 *
 */
public class URLEncodingTest {

    @Test
    public void testDamnHandyDefaultSyntax() {
        String uri = com.damnhandy.uri.template.UriTemplate.fromTemplate("http://localhost:80/path/{var1}").set("var1", "my/Id").expand();
        Assert.assertThat(uri, Matchers.is("http://localhost:80/path/my%2FId"));
    }

    @Test
    public void testDamnHandyPlusSyntax() {
        String uri = com.damnhandy.uri.template.UriTemplate.fromTemplate("http://localhost:80/path/{+var1}").set("var1", "my/Id").expand();
        Assert.assertThat(uri, Matchers.is("http://localhost:80/path/my/Id"));
    }

    @Test
    public void testDamnHandySlashSyntax() {
        String uri = com.damnhandy.uri.template.UriTemplate.fromTemplate("http://localhost:80/path{/var1}").set("var1", "my/Id").expand();
        Assert.assertThat(uri, Matchers.is("http://localhost:80/path/my%2FId"));
    }

    @Test
    public void testSpringDefaultSyntax() {
        String uri = new org.springframework.web.util.UriTemplate("http://localhost:80/path/{var1}").expand("my/Id").toString();
        // Fails, as spring does not correctly encode / in variable values
        Assert.assertThat(uri, Matchers.is("http://localhost:80/path/my%2FId"));
    }

    @Test
    public void testSpringPlusSyntax() {
        String uri = new org.springframework.web.util.UriTemplate("http://localhost:80/path/{+var1}").expand("my/Id").toString();
        Assert.assertThat(uri, Matchers.is("http://localhost:80/path/my/Id"));
    }

    @Test
    public void testSpringSlashSyntax() {
        String uri = new org.springframework.web.util.UriTemplate("http://localhost:80/path{/var1}").expand("my/Id").toString();
        // Fails as spring does not append the slash before doing the variable substitution.
        Assert.assertThat(uri, Matchers.is("http://localhost:80/path/my%2FId"));
    }

}

<dependency>
  <groupId>com.damnhandy</groupId>
  <artifactId>handy-uri-templates</artifactId>
  <version>2.0.2</version>
</dependency>

I understand the desire expressed in earlier bug reports to maintain backward compatibility. Now that RFC6570 is becoming well supported, spring 4.2 would seem a good time to switch to the RFC behaviour, while allowing a configuration parameter to switch back to springs earlier behaviour.

---

Affects: 4.1.6

This issue is a sub-task of #15137

Issue Links:

• Support limited subset of RFC 6570 syntax including operators "", "?", "/", "#" [SPR-14134] #18706 Support limited subset of RFC 6570 syntax including operators "", "?", "/", "#"

Referenced from: commits ca410fe

[spring-projects-issues]

Rossen Stoyanchev commented

I'd like to leave the discussion around 3.2.6 on "{/...}" path segment expansion under #17347 so I won't reply to that part here.

UriTemplate predates the final version of RFC 6750 by about 3 years. Much of the RFC syntax is not yet supported in the UriTemplate and UriComponents/Bulider. We have #15137 to explore this but it's clear the complete feature set is overwhelming and not intended to be always fully implemented. The spec talks about template levels in section 1.2. UriTemplate is clearly level 1. The "+" expansion is level 2.

Would it be fair to describe this as a request to support level 2 of RFC 6750 and the "+" operator which also means that (existing) URI variables without the "+" may be subject to different encoding rules. It's something to discuss more. For now I want to make sure I understand the scope of the request.

[spring-projects-issues]

daniel carter commented

Hi. Well the Level 2 + operator is already supported, see my testSpringPlusSyntax() above. I would consider this more about the default expression support in Level 1 i.e. the encoding of / in a {var1} template variable, my testSpringDefaultSyntax above.

1.2. Levels and Expression Types

URI Templates are similar to a macro language with a fixed set of macro definitions: the expression type determines the expansion process. The default expression type is simple string expansion, wherein a single named variable is replaced by its value as a string after pct-encoding any characters not in the set of unreserved URI characters (Section 1.5).

Since most template processors implemented prior to this specification have only implemented the default expression type, we refer to these as Level 1 templates.

1.5 Notational Conventions

unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"

Given that spring predates this spec as you say, has a large historical install base, and therefore it's likely there are some (rare?) uses cases where people are relying on "/" to not be encoded when using default expressions... then some sort of RFC6570 strict mode flag would do the job, and then at some stage you might want to make that the default, which would force people to use the + operator when they want an unencoded "/"

As it is, for me it turns out, rather unexpectedly, that an external system we interface with occasionally contains a "/" in an id field (which it describes as hex, but now clearly isn't). A bug has been raised, and it's too late in the process / too close to release to start using spring 4.2-RC1 with it's {/var1} operator (not that i need the / operator, i just want variables to be encoded so the server (with the same URITemplate) doesn't mis-interpret them.) So solution is to use a third party URITemplate library, and change every call to RestTemplate in the client library to first use this library and then use the RestTemplate method variants that take a URI instead of a string template and array of variables.

There's a dissymmetry at the moment, if i have /contacts/byExternalId/{id} on the controller, and use the same template on the client, the server fails to find the handler.
Anyway, rant over, thanks for the hardwork.

[spring-projects-issues]

Rossen Stoyanchev commented

Well, we don't support level 2 templates, I should know. We don't have the concept of a "+" operator within a URI variable. For example you can modify the test to to refer to the variable by name and that won't work:

@Test
public void testSpringPlusSyntax() {
	String uri = new org.springframework.web.util.UriTemplate("http://localhost:80/path/{+var1}")
			.expand(Collections.singletonMap("var1", "my/Id")).toString();

	Assert.assertThat(uri, Matchers.is("http://localhost:80/path/my/Id"));
}

then some sort of RFC6570 strict mode flag would do the job

Yes there could be something that enables RFC 6570 beyond level 1 templates. Then we can change the defaults. Literally every existing URI variable does not use operators so changing the encoding rules is guaranteed to upset a lot of people who may not wish to make changes. So it needs to be on an opt-in basis and then it all makes sense.

So solution is to use a third party URITemplate library, and change every call to RestTemplate in the client library to first use this library and then use the RestTemplate method variants that take a URI instead

You could follow the same approach but use UriComponentsBuilder instead. Either using pathSegments to build the path one segment at time or you could do something like this to break the path down into path segments:

public static void main(String[] args) {
    UriComponentsBuilder builder = UriComponentsBuilder.fromUriString("http://localhost:80/path/{var1}");
    builder = replacePathWithPathSegments(builder);
    URI url = builder.buildAndExpand("my/Id").encode().toUri();
    // "http://localhost:80/path/my%2FId"
}

private UriComponentsBuilder replacePathWithPathSegments(UriComponentsBuilder builder) {
    List<String> pathSegments = builder.build().getPathSegments();
    builder.replacePath(null);
    for (String ps : pathSegments) {
        builder.pathSegment(ps);
    }
    return builder;
}

[spring-projects-issues]

Rossen Stoyanchev commented

Modified title (was: "Correct encoding of / in URI Templates")

[spring-projects-issues]

Rossen Stoyanchev commented

I'm turning this into a sub-task of #15137 for consideration together with overall support for RFC 6570 for 4.2.

[spring-projects-issues]

daniel carter commented

Thanks for the example workaround, i've now adopted that, as i hit a defect in damn handy where it fails to percent encode the backtick character ` . Who woulda though %encoding was so complicated!

[spring-projects-issues]

daniel carter commented

OK no, off to the FGE implementation now. https://github.com/fge/uri-template.

The UriComponentsBuilder approach fails on a number of other characters. For instance ';' should be %encoded. Spring on the client doesn't encode this, and then on the server side it is silently dropped (probably parsing it as the start of a matrix variable rather than part of the path variable.

Likewise the UriComponentsBuilder approach fails to encode & ; and @.

    @Test
    public void testUriComponentsSlash() {
        testUriComponents("/", "%2F");
    }

    @Test
    public void testUriComponentsSemiColon() {
        testUriComponents(";", "%3B");
    }

    @Test
    public void testUriComponentsColon() {
        testUriComponents(":", "%3A");
    }

    public void testUriComponents(String var, String substitution) {
        UriComponentsBuilder builder = UriComponentsBuilder.fromUriString("http://localhost:80/path/{var1}");

        builder = replacePathWithPathSegments(builder);
        String url = builder.buildAndExpand(var).encode().toUriString();

        Assert.assertThat(url, Matchers.is("http://localhost:80/path/" + substitution));
    }

    private UriComponentsBuilder replacePathWithPathSegments(UriComponentsBuilder builder) {
        List<String> pathSegments = builder.build().getPathSegments();
        builder.replacePath(null);
        for (String ps : pathSegments) {
            builder.pathSegment(ps);
        }
        return builder;
    }

I think in the future i will be strongly advocating against using path variables, they are just so unsafe unless you are in full control of all systems that you are interfacing and know for sure that the possible values are only alphanumeric. Every single project i have worked on that uses path variables these encoding issues have killed me.

Latest problem is that tomcat strips any trailing dots in URIs so "/contacts/a1." sent by the client to a requestmapping of /contacts/{var}, results in var="a1" whereas i'm expecting var="a1."

Can't see a way around this one. Might have to change the API and have clients Base64 encode the path variables.

[spring-projects-issues]

Rossen Stoyanchev commented

Fair point and thanks for making these points. I've added a summary comment under the umbrella ticket for RFC 6570 (see comment).

[spring-projects-issues]

Rossen Stoyanchev commented

We've decided to pull back RFC 6570 support from 4.2. It seems to go much deeper than expected and most likely will result in a new set of classes anyway.

That said I've made a couple of commits that should help you. First see my comment under #17347 about the ability to customize how the RestTemplate expands URI templates. That means you can configure the RestTemplate to apply path segment encoding.

Of course as you mentioned you'd like to achieve full encoding of all reserved URI characters, which is not how our encoding works currently. For that I've added a new encode method in UriUtils (see commit ca410f|ca410fea531c5d486e034df3ab815229d7356f86]). You can create a custom UriTemplateHandler that uses this UriUtils method to encode URI variable values before passing them to UriComponents.expand.

Or you can also plug in any other URI template library through the UriTemplateHandler strategy.

[spring-projects-issues]

Rossen Stoyanchev commented

Postponing together with the parent ticket #15137.

[spring-projects-issues]

Rossen Stoyanchev commented

Resolving together with the parent ticket #15137. Note also the resolution of #16275 which provides a way to encode all characters outside the unreserved charset, which the original motivation for this ticket.