From 163740c842e5ad00c01df035d8cc9d728c1b9ebb Mon Sep 17 00:00:00 2001 From: Kevin Davis Date: Fri, 1 Feb 2019 16:54:38 -0500 Subject: [PATCH] Adding URI segment specific encoding (#882) * Adding URI segment specific encoding Fixes #879 URI encoding introduced in Feign 10.x was refactored to be more in line with URI and URI Template specifications respectively. One change was to ensure that certain reserved characters were not encoded incorrectly. The result was that path segment specific reserved characters were being preserved on the query string as well. This change updates the `UriTemplate` and `Expression` classes to recognize the segment of the URI that is being processed and apply the segment specific encoding correctly. One important change regarding the `+` sign. Per the URI specification, a `+` sign is allowed in both the path and query segments of a URI, however, handling of the symbol on the query can be inconsistent. In some legacy systems, the `+` is equivalent to the a space. Feign takes the approach of modern systems, where a `+` symbol should not reprsent a space and is explicitly encoded as `%2B` when found on a query string. If you wish to use `+` as a space, then use the literal ` ` character or encode the value directly as `%20` --- README.md | 8 + .../main/java/feign/template/Expressions.java | 32 +-- .../main/java/feign/template/Template.java | 9 +- .../main/java/feign/template/UriUtils.java | 203 ++++++++++++------ core/src/test/java/feign/FeignTest.java | 2 +- .../java/feign/template/UriTemplateTest.java | 9 + .../java/feign/template/UriUtilsTest.java | 72 ++++++- .../http2client/test/Http2ClientTest.java | 2 + 8 files changed, 236 insertions(+), 101 deletions(-) diff --git a/README.md b/README.md index 6ada43d5c..3f0903805 100644 --- a/README.md +++ b/README.md @@ -113,6 +113,14 @@ See [Advanced Usage](#advanced-usage) for more examples. > > `@RequestLine` and `@QueryMap` templates do not encode slash `/` characters by default. To change this behavior, set the `decodeSlash` property on the `@RequestLine` to `false`. +> **What about plus? `+`** +> +> Per the URI specification, a `+` sign is allowed in both the path and query segments of a URI, however, handling of +> the symbol on the query can be inconsistent. In some legacy systems, the `+` is equivalent to the a space. Feign takes the approach of modern systems, where a +> `+` symbol should not represent a space and is explicitly encoded as `%2B` when found on a query string. +> +> If you wish to use `+` as a space, then use the literal ` ` character or encode the value directly as `%20` + ##### Custom Expansion The `@Param` annotation has an optional property `expander` allowing for complete control over the individual parameter's expansion. diff --git a/core/src/main/java/feign/template/Expressions.java b/core/src/main/java/feign/template/Expressions.java index 77c115a1d..93cd767b2 100644 --- a/core/src/main/java/feign/template/Expressions.java +++ b/core/src/main/java/feign/template/Expressions.java @@ -14,6 +14,7 @@ package feign.template; import feign.Util; +import feign.template.UriUtils.FragmentType; import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; @@ -28,11 +29,10 @@ public final class Expressions { static { expressions = new LinkedHashMap<>(); - expressions.put(Pattern.compile("\\+(\\w[-\\w.]*[ ]*)(:(.+))?"), ReservedExpression.class); expressions.put(Pattern.compile("(\\w[-\\w.]*[ ]*)(:(.+))?"), SimpleExpression.class); } - public static Expression create(final String value) { + public static Expression create(final String value, final FragmentType type) { /* remove the start and end braces */ final String expression = stripBraces(value); @@ -52,7 +52,6 @@ public static Expression create(final String value) { Entry> matchedExpression = matchedExpressionEntry.get(); Pattern expressionPattern = matchedExpression.getKey(); - Class expressionType = matchedExpression.getValue(); /* create a new regular expression matcher for the expression */ String variableName = null; @@ -67,7 +66,7 @@ public static Expression create(final String value) { } } - return new SimpleExpression(variableName, variablePattern); + return new SimpleExpression(variableName, variablePattern, type); } private static String stripBraces(String expression) { @@ -80,36 +79,21 @@ private static String stripBraces(String expression) { return expression; } - /** - * Expression that does not encode reserved characters. This expression adheres to RFC 6570 Reserved Expansion (Level 2) - * specification. - */ - public static class ReservedExpression extends SimpleExpression { - private final String RESERVED_CHARACTERS = ":/?#[]@!$&\'()*+,;="; - - ReservedExpression(String expression, String pattern) { - super(expression, pattern); - } - - @Override - String encode(Object value) { - return UriUtils.encodeReserved(value.toString(), RESERVED_CHARACTERS, Util.UTF_8); - } - } - /** * Expression that adheres to Simple String Expansion as outlined in > 4) & 0xF, 16)); + char hex2 = Character.toUpperCase(Character.forDigit(data & 0xF, 16)); + bos.write(hex1); + bos.write(hex2); + } + + enum FragmentType { + URI { + @Override + boolean isAllowed(int c) { + return isUnreserved(c); + } + }, + RESERVED { + @Override + boolean isAllowed(int c) { + return isUnreserved(c) || isReserved(c); + } + }, + PATH_SEGMENT { + @Override + boolean isAllowed(int c) { + return this.isPchar(c) || (c == '/'); + } + }, + QUERY { + @Override + boolean isAllowed(int c) { + /* although plus signs are allowed, their use is inconsistent. force encoding */ + if (c == '+') { + return false; } - if (i != index) { - /* we are in the middle of the value, so we need to encode mid string */ - encoded.append(urlEncode(value.substring(index, i), charset)); + return this.isPchar(c) || c == '/' || c == '?'; + } + }, + QUERY_PARAM { + @Override + boolean isAllowed(int c) { + /* explicitly encode equals, ampersands, questions */ + if (c == '=' || c == '&' || c == '?') { + return false; } - encoded.append(character); - index = i + 1; + return QUERY.isAllowed(c); } + }; + + abstract boolean isAllowed(int c); + + protected boolean isAlpha(int c) { + return (c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z'); } - /* if there are no reserved characters, encode the original value */ - if (encoded == null) { - return urlEncode(value, charset); + protected boolean isDigit(int c) { + return (c >= '0' && c <= '9'); } - /* encode the rest of the string */ - if (index < length) { - encoded.append(urlEncode(value.substring(index, length), charset)); + protected boolean isGenericDelimiter(int c) { + return (c == ':') + || (c == '/') + || (c == '?') + || (c == '#') + || (c == '[') + || (c == ']') + || (c == '@'); + } + + protected boolean isSubDelimiter(int c) { + return (c == '!') + || (c == '$') + || (c == '&') + || (c == '\'') + || (c == '(') + || (c == ')') + || (c == '*') + || (c == '+') + || (c == ',') + || (c == ';') + || (c == '='); + } + + protected boolean isUnreserved(int c) { + return this.isAlpha(c) || this.isDigit(c) || c == '-' || c == '.' || c == '_' || c == '~'; + } + + protected boolean isReserved(int c) { + return this.isGenericDelimiter(c) || this.isSubDelimiter(c); + } + + protected boolean isPchar(int c) { + return this.isUnreserved(c) || this.isSubDelimiter(c) || c == ':' || c == '@'; } - return encoded.toString(); } } diff --git a/core/src/test/java/feign/FeignTest.java b/core/src/test/java/feign/FeignTest.java index 34c292cd4..65f4850e9 100644 --- a/core/src/test/java/feign/FeignTest.java +++ b/core/src/test/java/feign/FeignTest.java @@ -787,7 +787,7 @@ public void encodedQueryParam() throws Exception { api.encodedQueryParam("5.2FSi+"); - assertThat(server.takeRequest()).hasPath("/?trim=5.2FSi+"); + assertThat(server.takeRequest()).hasPath("/?trim=5.2FSi%2B"); } @Test diff --git a/core/src/test/java/feign/template/UriTemplateTest.java b/core/src/test/java/feign/template/UriTemplateTest.java index f46b7a68e..5ded27b94 100644 --- a/core/src/test/java/feign/template/UriTemplateTest.java +++ b/core/src/test/java/feign/template/UriTemplateTest.java @@ -222,6 +222,15 @@ public void ensurePlusIsSupportedOnPath() { assertThat(expanded).isEqualToIgnoringCase("https://www.example.com/sam+adams/beer/"); } + @Test + public void ensurePlusInEncodedAs2BOnQuery() { + String template = "https://www.example.com/beer?type={type}"; + UriTemplate uriTemplate = UriTemplate.create(template, Util.UTF_8); + Map parameters = Collections.singletonMap("type", "sam+adams"); + String expanded = uriTemplate.expand(parameters); + assertThat(expanded).isEqualToIgnoringCase("https://www.example.com/beer?type=sam%2Badams"); + } + @Test public void incompleteTemplateIsALiteral() { String template = "https://www.example.com/testing/foo}}"; diff --git a/core/src/test/java/feign/template/UriUtilsTest.java b/core/src/test/java/feign/template/UriUtilsTest.java index 84d86ea34..de766582d 100644 --- a/core/src/test/java/feign/template/UriUtilsTest.java +++ b/core/src/test/java/feign/template/UriUtilsTest.java @@ -13,24 +13,78 @@ */ package feign.template; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; -import java.nio.charset.StandardCharsets; +import java.nio.charset.Charset; import org.junit.Test; public class UriUtilsTest { + /** + * Verify that values outside of the allowed characters in a path segment are pct-encoded. The + * list of approved characters used are those listed in The list of approved characters used are those listed in The list of approved characters used are those listed in