Encode enough to make URI happy.

squarejesse · squarejesse · commit 080e7536ab8c · 2015-11-01T12:32:27.000-05:00
Plus a bunch of test cases around this unfortunate case. Closes square#1872
diff --git 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 {
@@ -930,19 +939,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
@@ -395,7 +395,7 @@ private void testRequestBodySurvivesRetries(TransferKind transferKind) throws Ex
     try {
       connection.connect();
       fail();
-    } catch (IllegalStateException expected) {
+    } catch (UnknownHostException expected) {
     }
   }
 
@@ -2427,7 +2427,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
@@ -168,7 +168,7 @@ class UrlComponentEncodingTester {
   }
 
   private final Map<Integer, Encoding> 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
@@ -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.
+   *
+   * <p>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);
     }
@@ -577,12 +578,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());
@@ -867,6 +864,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");
@@ -1363,8 +1385,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;

Original file line number	Diff line number	Diff line change
`@@ -395,7 +395,7 @@ private void testRequestBodySurvivesRetries(TransferKind transferKind) throws Ex`
`395`	`395`	`try {`
`396`	`396`	`connection.connect();`
`397`	`397`	`fail();`
`398`		`- } catch (IllegalStateException expected) {`
	`398`	`+ } catch (UnknownHostException expected) {`
`399`	`399`	`}`
`400`	`400`	`}`
`401`	`401`
`@@ -2427,7 +2427,7 @@ private void testFlushAfterStreamTransmitted(TransferKind transferKind) throws I`
`2427`	`2427`	`}`
`2428`	`2428`
`2429`	`2429`	`@Test public void malformedUrlThrowsUnknownHostException() throws IOException {`
`2430`		`- connection = client.open(new URL("http:///foo.html"));`
	`2430`	`+ connection = client.open(new URL("http://./foo.html"));`
`2431`	`2431`	`try {`
`2432`	`2432`	`connection.connect();`
`2433`	`2433`	`fail();`