From ca342d6c3c54ae2417d4e61c925ba6f63b31a571 Mon Sep 17 00:00:00 2001 From: jwilson Date: Sun, 1 Nov 2015 12:16:09 -0500 Subject: [PATCH] Apply upstream OkHttp HttpUrl fix Upstream information: Comment: Encode enough to make URI happy. Plus a bunch of test cases around this unfortunate case. Closes https://github.com/square/okhttp/issues/1872 SHA: 080e7536ab8c2d07db58628f75a4140012e2a581 Bug: 27590872 (cherry picked from commit 6dde4fd274c15786415a313faf5b4506e3d712e4) Change-Id: I8aab0e72639474d1ca2c6a2ca454e4a38e30bd84 --- .../java/com/squareup/okhttp/HttpUrlTest.java | 117 ++++++++++++++++-- .../squareup/okhttp/URLConnectionTest.java | 4 +- .../okhttp/UrlComponentEncodingTester.java | 53 ++++---- .../java/com/squareup/okhttp/HttpUrl.java | 56 ++++++--- 4 files changed, 177 insertions(+), 53 deletions(-) diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/HttpUrlTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/HttpUrlTest.java index 10291f1..4f04af9 100644 --- a/okhttp-tests/src/test/java/com/squareup/okhttp/HttpUrlTest.java +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/HttpUrlTest.java @@ -196,11 +196,20 @@ public final class HttpUrlTest { assertEquals(HttpUrl.parse("http://user@host/path"), HttpUrl.parse("http://user@host/path")); } + /** Given multiple '@' characters, the last one is the delimiter. */ @Test public void authorityWithMultipleAtSigns() throws Exception { - assertEquals(HttpUrl.parse("http://foo%40bar@baz/path"), - HttpUrl.parse("http://foo@bar@baz/path")); - assertEquals(HttpUrl.parse("http://foo:pass1%40bar%3Apass2@baz/path"), - HttpUrl.parse("http://foo:pass1@bar:pass2@baz/path")); + HttpUrl httpUrl = HttpUrl.parse("http://foo@bar@baz/path"); + assertEquals("foo@bar", httpUrl.username()); + assertEquals("", httpUrl.password()); + assertEquals(HttpUrl.parse("http://foo%40bar@baz/path"), httpUrl); + } + + /** Given multiple ':' characters, the first one is the delimiter. */ + @Test public void authorityWithMultipleColons() throws Exception { + HttpUrl httpUrl = HttpUrl.parse("http://foo:pass1@bar:pass2@baz/path"); + assertEquals("foo", httpUrl.username()); + assertEquals("pass1@bar:pass2", httpUrl.password()); + assertEquals(HttpUrl.parse("http://foo:pass1%40bar%3Apass2@baz/path"), httpUrl); } @Test public void usernameAndPassword() throws Exception { @@ -928,19 +937,103 @@ public final class HttpUrlTest { assertEquals("http://host/?d=abc!@[]%5E%60%7B%7D%7C%5C", uri.toString()); } - @Test public void toUriSpecialPathCharacters() throws Exception { + @Test public void toUriWithUsernameNoPassword() throws Exception { + HttpUrl httpUrl = new HttpUrl.Builder() + .scheme("http") + .username("user") + .host("host") + .build(); + assertEquals("http://user@host/", httpUrl.toString()); + assertEquals("http://user@host/", httpUrl.uri().toString()); + } + + @Test public void toUriUsernameSpecialCharacters() throws Exception { HttpUrl url = new HttpUrl.Builder() .scheme("http") - .host("example.com") - .addPathSegment("data=[out:json];node[\"name\"~\"Karlsruhe\"]" + - "[\"place\"~\"city|village|town\"];out body;") + .host("host") + .username("=[]:;\"~|?#@^/$%*") + .build(); + assertEquals("http://%3D%5B%5D%3A%3B%22~%7C%3F%23%40%5E%2F$%25*@host/", url.toString()); + assertEquals("http://%3D%5B%5D%3A%3B%22~%7C%3F%23%40%5E%2F$%25*@host/", url.uri().toString()); + } + + @Test public void toUriPasswordSpecialCharacters() throws Exception { + HttpUrl url = new HttpUrl.Builder() + .scheme("http") + .host("host") + .username("user") + .password("=[]:;\"~|?#@^/$%*") .build(); - URI uri = url.uri(); - assertEquals("http://example.com/data=%5Bout:json%5D;node%5B%22name%22~%22Karlsruhe%22%5D" + - "%5B%22place%22~%22city%7Cvillage%7Ctown%22%5D;out%20body;", - uri.toString()); + assertEquals("http://user:%3D%5B%5D%3A%3B%22~%7C%3F%23%40%5E%2F$%25*@host/", url.toString()); + assertEquals("http://user:%3D%5B%5D%3A%3B%22~%7C%3F%23%40%5E%2F$%25*@host/", + url.uri().toString()); + } + + @Test public void toUriPathSpecialCharacters() throws Exception { + HttpUrl url = new HttpUrl.Builder() + .scheme("http") + .host("host") + .addPathSegment("=[]:;\"~|?#@^/$%*") + .build(); + assertEquals("http://host/=[]:;%22~%7C%3F%23@%5E%2F$%25*", url.toString()); + assertEquals("http://host/=%5B%5D:;%22~%7C%3F%23@%5E%2F$%25*", url.uri().toString()); } + @Test public void toUriQueryParameterNameSpecialCharacters() throws Exception { + HttpUrl url = new HttpUrl.Builder() + .scheme("http") + .host("host") + .addQueryParameter("=[]:;\"~|?#@^/$%*", "a") + .build(); + assertEquals("http://host/?%3D[]:;%22~|?%23@^/$%25*=a", url.toString()); + assertEquals("http://host/?%3D[]:;%22~%7C?%23@%5E/$%25*=a", url.uri().toString()); + } + + @Test public void toUriQueryParameterValueSpecialCharacters() throws Exception { + HttpUrl url = new HttpUrl.Builder() + .scheme("http") + .host("host") + .addQueryParameter("a", "=[]:;\"~|?#@^/$%*") + .build(); + assertEquals("http://host/?a=%3D[]:;%22~|?%23@^/$%25*", url.toString()); + assertEquals("http://host/?a=%3D[]:;%22~%7C?%23@%5E/$%25*", url.uri().toString()); + } + + @Test public void toUriQueryValueSpecialCharacters() throws Exception { + HttpUrl url = new HttpUrl.Builder() + .scheme("http") + .host("host") + .query("=[]:;\"~|?#@^/$%*") + .build(); + assertEquals("http://host/?=[]:;%22~|?%23@^/$%25*", url.toString()); + assertEquals("http://host/?=[]:;%22~%7C?%23@%5E/$%25*", url.uri().toString()); + } + + @Test public void toUriFragmentSpecialCharacters() throws Exception { + HttpUrl url = new HttpUrl.Builder() + .scheme("http") + .host("host") + .fragment("=[]:;\"~|?#@^/$%*") + .build(); + assertEquals("http://host/#=[]:;\"~|?#@^/$%25*", url.toString()); + assertEquals("http://host/#=[]:;%22~%7C?%23@%5E/$%25*", url.uri().toString()); + } + + @Test public void toUriWithMalformedPercentEscape() throws Exception { + HttpUrl url = new HttpUrl.Builder() + .scheme("http") + .host("host") + .encodedPath("/%xx") + .build(); + assertEquals("http://host/%xx", url.toString()); + try { + url.uri(); + fail(); + } catch (IllegalStateException expected) { + assertEquals("not valid as a java.net.URI: http://host/%xx", expected.getMessage()); + } + } + @Test public void fromJavaNetUrl() throws Exception { URL javaNetUrl = new URL("http://username:password@host/path?query#fragment"); HttpUrl httpUrl = HttpUrl.get(javaNetUrl); diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/URLConnectionTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/URLConnectionTest.java index 4f0ca58..59cbc54 100644 --- a/okhttp-tests/src/test/java/com/squareup/okhttp/URLConnectionTest.java +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/URLConnectionTest.java @@ -395,7 +395,7 @@ private void testRequestBodySurvivesRetries(TransferKind transferKind) throws Ex try { connection.connect(); fail(); - } catch (IllegalStateException expected) { + } catch (UnknownHostException expected) { } } @@ -2429,7 +2429,7 @@ private void testFlushAfterStreamTransmitted(TransferKind transferKind) throws I } @Test public void malformedUrlThrowsUnknownHostException() throws IOException { - connection = client.open(new URL("http:///foo.html")); + connection = client.open(new URL("http://./foo.html")); try { connection.connect(); fail(); diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/UrlComponentEncodingTester.java b/okhttp-tests/src/test/java/com/squareup/okhttp/UrlComponentEncodingTester.java index 199279f..99e9791 100644 --- a/okhttp-tests/src/test/java/com/squareup/okhttp/UrlComponentEncodingTester.java +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/UrlComponentEncodingTester.java @@ -168,7 +168,7 @@ class UrlComponentEncodingTester { } private final Map encodings; - private final StringBuilder skipForUri = new StringBuilder(); + private final StringBuilder uriEscapedCodePoints = new StringBuilder(); public UrlComponentEncodingTester() { this.encodings = new LinkedHashMap<>(defaultEncodings); @@ -186,7 +186,7 @@ public UrlComponentEncodingTester override(Encoding encoding, int... codePoints) * That class is more strict than the others. */ public UrlComponentEncodingTester skipForUri(int... codePoints) { - skipForUri.append(new String(codePoints, 0, codePoints.length)); + uriEscapedCodePoints.append(new String(codePoints, 0, codePoints.length)); return this; } @@ -202,9 +202,10 @@ public UrlComponentEncodingTester test(Component component) { testToUrl(codePoint, encoding, component); testFromUrl(codePoint, encoding, component); - if (skipForUri.indexOf(Encoding.IDENTITY.encode(codePoint)) == -1) { - testToUri(codePoint, encoding, component); - testFromUri(codePoint, encoding, component); + if (codePoint != '%') { + boolean uriEscaped = uriEscapedCodePoints.indexOf( + Encoding.IDENTITY.encode(codePoint)) != -1; + testUri(codePoint, encoding, component, uriEscaped); } } return this; @@ -261,21 +262,29 @@ private void testFromUrl(int codePoint, Encoding encoding, Component component) } } - private void testToUri(int codePoint, Encoding encoding, Component component) { + private void testUri( + int codePoint, Encoding encoding, Component component, boolean uriEscaped) { + String string = new String(new int[] { codePoint }, 0, 1); String encoded = encoding.encode(codePoint); HttpUrl httpUrl = HttpUrl.parse(component.urlString(encoded)); URI uri = httpUrl.uri(); - if (!uri.toString().equals(uri.toString())) { - fail(String.format("Encoding %s %#x using %s", component, codePoint, encoding)); - } - } - - private void testFromUri(int codePoint, Encoding encoding, Component component) { - String encoded = encoding.encode(codePoint); - HttpUrl httpUrl = HttpUrl.parse(component.urlString(encoded)); - HttpUrl toAndFromUri = HttpUrl.get(httpUrl.uri()); - if (!toAndFromUri.equals(httpUrl)) { - fail(String.format("Encoding %s %#x using %s", component, codePoint, encoding)); + HttpUrl toAndFromUri = HttpUrl.get(uri); + if (uriEscaped) { + // The URI has more escaping than the HttpURL. Check that the decoded values still match. + if (uri.toString().equals(httpUrl.toString())) { + fail(String.format("Encoding %s %#x using %s", component, codePoint, encoding)); + } + if (!component.get(toAndFromUri).equals(string)) { + fail(String.format("Encoding %s %#x using %s", component, codePoint, encoding)); + } + } else { + // Check that the URI and HttpURL have the exact same escaping. + if (!toAndFromUri.equals(httpUrl)) { + fail(String.format("Encoding %s %#x using %s", component, codePoint, encoding)); + } + if (!uri.toString().equals(httpUrl.toString())) { + fail(String.format("Encoding %s %#x using %s", component, codePoint, encoding)); + } } } @@ -358,10 +367,11 @@ public enum Component { return query.substring(1, query.length() - 1); } @Override public void set(HttpUrl.Builder builder, String value) { - builder.query(value); + builder.query("a" + value + "z"); } @Override public String get(HttpUrl url) { - return url.query(); + String query = url.query(); + return query.substring(1, query.length() - 1); } }, FRAGMENT { @@ -373,10 +383,11 @@ public enum Component { return fragment.substring(1, fragment.length() - 1); } @Override public void set(HttpUrl.Builder builder, String value) { - builder.fragment(value); + builder.fragment("a" + value + "z"); } @Override public String get(HttpUrl url) { - return url.fragment(); + String fragment = url.fragment(); + return fragment.substring(1, fragment.length() - 1); } }; diff --git a/okhttp/src/main/java/com/squareup/okhttp/HttpUrl.java b/okhttp/src/main/java/com/squareup/okhttp/HttpUrl.java index a5b9876..76764c2 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/HttpUrl.java +++ b/okhttp/src/main/java/com/squareup/okhttp/HttpUrl.java @@ -258,10 +258,13 @@ public final class HttpUrl { static final String USERNAME_ENCODE_SET = " \"':;<=>@[]^`{}|/\\?#"; static final String PASSWORD_ENCODE_SET = " \"':;<=>@[]^`{}|/\\?#"; static final String PATH_SEGMENT_ENCODE_SET = " \"<>^`{}|/\\?#"; + static final String PATH_SEGMENT_ENCODE_SET_URI = "[]"; static final String QUERY_ENCODE_SET = " \"'<>#"; static final String QUERY_COMPONENT_ENCODE_SET = " \"'<>#&="; + static final String QUERY_COMPONENT_ENCODE_SET_URI = "\\^`{|}"; static final String FORM_ENCODE_SET = " \"':;<=>@[]^`{}|/\\?#&!$(),~"; static final String FRAGMENT_ENCODE_SET = ""; + static final String FRAGMENT_ENCODE_SET_URI = " \"#<>\\^`{|}"; /** Either "http" or "https". */ private final String scheme; @@ -324,19 +327,17 @@ public URL url() { } /** - * Attempt to convert this URL to a {@link URI java.net.URI}. This method throws an unchecked - * {@link IllegalStateException} if the URL it holds isn't valid by URI's overly-stringent - * standard. For example, URI rejects paths containing the '[' character. Consult that class for - * the exact rules of what URLs are permitted. + * Returns this URL as a {@link URI java.net.URI}. Because {@code URI} forbids certain characters + * like {@code [} and {@code |}, the returned URI may escape more characters than this URL. + * + *

This method throws an unchecked {@link IllegalStateException} if it cannot be converted to a + * URI even after escaping forbidden characters. In particular, URLs that contain malformed + * percent escapes like {@code http://host/%xx} will trigger this exception. */ public URI uri() { try { - String uriUserInfo = username + ":" + password; - if (uriUserInfo.equals(":")) uriUserInfo = null; - final int uriPort = port == defaultPort(scheme) ? -1 : port; // Don't include default port - StringBuilder path = new StringBuilder(); - pathSegmentsToString(path, pathSegments); - return new URI(scheme, uriUserInfo, host, uriPort, path.toString(), query(), fragment); + String uri = newBuilder().reencodeForUri().toString(); + return new URI(uri); } catch (URISyntaxException e) { throw new IllegalStateException("not valid as a java.net.URI: " + url); } @@ -590,12 +591,8 @@ public Builder newBuilder() { result.encodedUsername = encodedUsername(); result.encodedPassword = encodedPassword(); result.host = host; - // If we're set to a default port, unset it, in case of a scheme change. - if (port == defaultPort(scheme)) { - result.port = -1; - } else { - result.port = port; - } + // If we're set to a default port, unset it in case of a scheme change. + result.port = port != defaultPort(scheme) ? port : -1; result.encodedPathSegments.clear(); result.encodedPathSegments.addAll(encodedPathSegments()); result.encodedQuery(encodedQuery()); @@ -880,6 +877,31 @@ public Builder encodedFragment(String encodedFragment) { return this; } + /** + * Re-encodes the components of this URL so that it satisfies (obsolete) RFC 2396, which is + * particularly strict for certain components. + */ + Builder reencodeForUri() { + for (int i = 0, size = encodedPathSegments.size(); i < size; i++) { + String pathSegment = encodedPathSegments.get(i); + encodedPathSegments.set(i, + canonicalize(pathSegment, PATH_SEGMENT_ENCODE_SET_URI, true, false)); + } + if (encodedQueryNamesAndValues != null) { + for (int i = 0, size = encodedQueryNamesAndValues.size(); i < size; i++) { + String component = encodedQueryNamesAndValues.get(i); + if (component != null) { + encodedQueryNamesAndValues.set(i, + canonicalize(component, QUERY_COMPONENT_ENCODE_SET_URI, true, true)); + } + } + } + if (encodedFragment != null) { + encodedFragment = canonicalize(encodedFragment, FRAGMENT_ENCODE_SET_URI, true, false); + } + return this; + } + public HttpUrl build() { if (scheme == null) throw new IllegalStateException("scheme == null"); if (host == null) throw new IllegalStateException("host == null"); @@ -1376,8 +1398,6 @@ private static String domainToAscii(String input) { String result = IDN.toASCII(input).toLowerCase(Locale.US); if (result.isEmpty()) return null; - if (result == null) return null; - // Confirm that the IDN ToASCII result doesn't contain any illegal characters. if (containsInvalidHostnameAsciiCodes(result)) { return null;