diff options
author | Neil Fuller <nfuller@google.com> | 2016-07-12 14:32:15 +0100 |
---|---|---|
committer | gitbuildkicker <android-build@google.com> | 2016-07-13 20:29:55 -0700 |
commit | c6817fbea3bc321e3816492c18f98c45895630f6 (patch) | |
tree | ffb7efd54c800aca65133bc158031c3b1c98ef97 | |
parent | 2abd308f1aae9e9eb28a3aa6d1386be446fc276b (diff) | |
download | okhttp-nougat-bugfix-release.tar.gz |
Fix regression in the HTTP request lineandroid-7.0.0_r9android-7.0.0_r8android-7.0.0_r7android-7.0.0_r6android-7.0.0_r5android-7.0.0_r4android-7.0.0_r36android-7.0.0_r35android-7.0.0_r34android-7.0.0_r33android-7.0.0_r32android-7.0.0_r31android-7.0.0_r30android-7.0.0_r3android-7.0.0_r29android-7.0.0_r28android-7.0.0_r27android-7.0.0_r24android-7.0.0_r21android-7.0.0_r19android-7.0.0_r17android-7.0.0_r15android-7.0.0_r14android-7.0.0_r13android-7.0.0_r12android-7.0.0_r11android-7.0.0_r10android-7.0.0_r1afw-test-harness-2.1nougat-releasenougat-mr0.5-releasenougat-bugfix-release
Adds a test that demonstrates a regression from M in
the HTTP request line when a proxied connection
is made. It only affects URLs with empty paths (i.e. ones
without even a '/' after the host / authority). For
example, on M the request line for a request to
new URL("http://myhost").openConnection() would be:
GET http://myhost HTTP/1.1
but on N, without this change, it would be:
GET http://myhost/ HTTP/1.1
This change reverts to the M behavior.
Bug: 29983827
Test: CtsLibcoreOkHttpTestCases and CtsLibcoreTestCases
Change-Id: I117bca1371e5311b85e0cee0d3d2528191ade182
(cherry picked from commit cd57d9eb83acc7a13ee576bfa22d4c490316ac18)
4 files changed, 199 insertions, 18 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 71deb6c..d45ac10 100644 --- a/okhttp-tests/src/test/java/com/squareup/okhttp/HttpUrlTest.java +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/HttpUrlTest.java @@ -29,6 +29,7 @@ import org.junit.Test; import static java.util.Collections.singletonList; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; import static org.junit.Assert.fail; @@ -110,7 +111,11 @@ public final class HttpUrlTest { @Test public void resolveNoScheme() throws Exception { HttpUrl base = HttpUrl.parse("http://host/a/b"); - assertEquals(HttpUrl.parse("http://host2/"), base.resolve("//host2")); + // ANDROID-BEGIN: http://b/29983827 + // assertEquals(HttpUrl.parse("http://host2/"), base.resolve("//host2")); + assertEquals(HttpUrl.parse("http://host2"), base.resolve("//host2")); + // ANDROID-END: http://b/29983827 + assertEquals(HttpUrl.parse("http://host2"), base.resolve("//host2")); assertEquals(HttpUrl.parse("http://host/path"), base.resolve("/path")); assertEquals(HttpUrl.parse("http://host/a/path"), base.resolve("path")); assertEquals(HttpUrl.parse("http://host/a/b?query"), base.resolve("?query")); @@ -119,6 +124,98 @@ public final class HttpUrlTest { assertEquals(HttpUrl.parse("http://host/path"), base.resolve("\\path")); } + // ANDROID-BEGIN: http://b/29983827 + @Test public void encodedPath_pathVariations() throws Exception { + assertEquals("", HttpUrl.parse("http://example.com").encodedPath()); + assertEquals("", HttpUrl.parse("http://example.com#fragment").encodedPath()); + assertEquals("", HttpUrl.parse("http://example.com?arg=value").encodedPath()); + assertEquals("/", HttpUrl.parse("http://example.com/").encodedPath()); + assertEquals("/", HttpUrl.parse("http://example.com/#fragment").encodedPath()); + assertEquals("/", HttpUrl.parse("http://example.com/?arg=value").encodedPath()); + assertEquals("/foo", HttpUrl.parse("http://example.com/foo").encodedPath()); + assertEquals("/foo/", HttpUrl.parse("http://example.com/foo/").encodedPath()); + } + + @Test public void newBuilder_pathVariations() throws Exception { + assertNewBuilderRoundtrip("http://example.com"); + assertNewBuilderRoundtrip("http://example.com/"); + assertNewBuilderRoundtrip("http://example.com/foo"); + assertNewBuilderRoundtrip("http://example.com/foo/"); + } + + private void assertNewBuilderRoundtrip(String urlString) { + HttpUrl url = HttpUrl.parse(urlString); + assertEquals(url, url.newBuilder().build()); + } + + @Test public void equals_emptyPathNotSameAsSlash() throws Exception { + assertFalse(HttpUrl.parse("http://example.com").equals(HttpUrl.parse("http://example.com/"))); + } + + @Test public void parse_pathVariations() throws Exception { + parseThenAssertToStringEquals("http://example.com"); + parseThenAssertToStringEquals("https://example.com"); + parseThenAssertToStringEquals("http://example.com?value=42"); + parseThenAssertToStringEquals("http://example.com:3434"); + parseThenAssertToStringEquals("http://example.com#hello"); + + parseThenAssertToStringEquals("http://example.com/"); + parseThenAssertToStringEquals("https://example.com/"); + parseThenAssertToStringEquals("http://example.com/foo/bar"); + parseThenAssertToStringEquals("http://example.com/foo/bar/"); + parseThenAssertToStringEquals("http://example.com/foo/bar?value=100"); + parseThenAssertToStringEquals("http://example.com/?value=200"); + + { + HttpUrl httpUrl = HttpUrl.parse("http://example.com/foo/.."); + assertEquals("http://example.com/", httpUrl.toString()); + } + + { + HttpUrl httpUrl = HttpUrl.parse("http://example.com/.."); + assertEquals("http://example.com/", httpUrl.toString()); + } + } + + private static void parseThenAssertToStringEquals(String url) { + HttpUrl httpUrl = HttpUrl.parse(url); + assertEquals(url, httpUrl.toString()); + } + + @Test public void resolve_pathVariations() throws Exception { + HttpUrl baseWithoutSlash = HttpUrl.parse("http://example.com"); + assertResolveResult("http://example.com#section", baseWithoutSlash, "#section"); + assertResolveResult("http://example.com?attitude=friendly", baseWithoutSlash, + "?attitude=friendly"); + assertResolveResult("http://example.com", baseWithoutSlash, ""); + assertResolveResult("http://example.com/", baseWithoutSlash, "/"); + assertResolveResult("http://example.com/foo", baseWithoutSlash, "/foo"); + assertResolveResult("http://example.com/foo", baseWithoutSlash, "foo"); + + assertResolveResult("http://other.com", baseWithoutSlash, "http://other.com"); + assertResolveResult("http://other.com/", baseWithoutSlash, "http://other.com/"); + assertResolveResult("http://other.com/foo", baseWithoutSlash, "http://other.com/foo"); + + HttpUrl baseWithSlash = HttpUrl.parse("http://example.com/"); + assertResolveResult("http://example.com/#section", baseWithSlash, "#section"); + assertResolveResult("http://example.com/?attitude=friendly", baseWithSlash, + "?attitude=friendly"); + assertResolveResult("http://example.com/", baseWithSlash, ""); + assertResolveResult("http://example.com/", baseWithSlash, "/"); + assertResolveResult("http://example.com/foo", baseWithSlash, "/foo"); + assertResolveResult("http://example.com/foo", baseWithSlash, "foo"); + + assertResolveResult("http://other.com", baseWithSlash, "http://other.com"); + assertResolveResult("http://other.com/", baseWithSlash, "http://other.com/"); + assertResolveResult("http://other.com/foo", baseWithSlash, "http://other.com/foo"); + } + + private void assertResolveResult(String expected, HttpUrl base, String link) { + assertEquals(HttpUrl.parse(expected), base.resolve(link)); + assertEquals(expected, base.resolve(link).toString()); + } + // ANDROID-END: http://b/29983827 + @Test public void resolveUnsupportedScheme() throws Exception { HttpUrl base = HttpUrl.parse("http://a/"); assertEquals(null, base.resolve("ftp://b")); @@ -939,8 +1036,12 @@ public final class HttpUrlTest { .removePathSegment(0) .removePathSegment(0) .build(); - assertEquals(Arrays.asList(""), url.pathSegments()); - assertEquals("/", url.encodedPath()); + // ANDROID-BEGIN: http://b/29983827 Behavior changed. Test name is now incorrect. + // assertEquals(Arrays.asList(""), url.pathSegments()); + // assertEquals("/", url.encodedPath()); + assertEquals(Collections.emptyList(), url.pathSegments()); + assertEquals("http://host", url.toString()); + // ANDROID-END: http://b/29983827 } @Test public void removePathSegmentOutOfBounds() throws Exception { 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 3598da0..7c948b8 100644 --- a/okhttp-tests/src/test/java/com/squareup/okhttp/URLConnectionTest.java +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/URLConnectionTest.java @@ -680,19 +680,29 @@ public final class URLConnectionTest { testConnectViaProxy(ProxyConfig.HTTP_PROXY_SYSTEM_PROPERTY); } + // ANDROID-BEGIN: http://b/29983827 private void testConnectViaProxy(ProxyConfig proxyConfig) throws Exception { + testConnectViaProxy(proxyConfig, "http://android.com/foo", "android.com"); + testConnectViaProxy(proxyConfig, "http://android.com/", "android.com"); + testConnectViaProxy(proxyConfig, "http://android.com", "android.com"); + testConnectViaProxy(proxyConfig, "http://mms", "mms"); + } + + private void testConnectViaProxy(ProxyConfig proxyConfig, String urlString, String expectedHost) + throws Exception { MockResponse mockResponse = new MockResponse().setBody("this response comes via a proxy"); server.enqueue(mockResponse); - URL url = new URL("http://android.com/foo"); + URL url = new URL(urlString); connection = proxyConfig.connect(server, client, url); assertContent("this response comes via a proxy", connection); assertTrue(connection.usingProxy()); RecordedRequest request = server.takeRequest(); - assertEquals("GET http://android.com/foo HTTP/1.1", request.getRequestLine()); - assertEquals("android.com", request.getHeader("Host")); + assertEquals("GET " + urlString + " HTTP/1.1", request.getRequestLine()); + assertEquals(expectedHost, request.getHeader("Host")); } + // ANDROID-END: http://b/29983827 @Test public void contentDisagreesWithContentLengthHeaderBodyTooLong() throws IOException { server.enqueue(new MockResponse().setBody("abc\r\nYOU SHOULD NOT SEE THIS") diff --git a/okhttp/src/main/java/com/squareup/okhttp/HttpUrl.java b/okhttp/src/main/java/com/squareup/okhttp/HttpUrl.java index beabeca..8e75e3f 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/HttpUrl.java +++ b/okhttp/src/main/java/com/squareup/okhttp/HttpUrl.java @@ -442,11 +442,18 @@ public final class HttpUrl { } /** - * Returns the entire path of this URL, encoded for use in HTTP resource resolution. The - * returned path is always nonempty and is prefixed with {@code /}. + * Returns the entire path of this URL, encoded for use in HTTP resource resolution. + // ANDROID-BEGIN: http://b/29983827 + // * The returned path is always nonempty and is prefixed with {@code /}. + // ANDROID-END: http://b/29983827 */ public String encodedPath() { int pathStart = url.indexOf('/', scheme.length() + 3); // "://".length() == 3. + // ANDROID-BEGIN: http://b/29983827 + if (pathStart == -1) { + return ""; + } + // ANDROID-END: http://b/29983827 int pathEnd = delimiterOffset(url, pathStart, url.length(), "?#"); return url.substring(pathStart, pathEnd); } @@ -460,6 +467,12 @@ public final class HttpUrl { public List<String> encodedPathSegments() { int pathStart = url.indexOf('/', scheme.length() + 3); + // ANDROID-BEGIN: http://b/29983827 + if (pathStart == -1) { + return new ArrayList<>(); + } + // ANDROID-END: http://b/29983827 + int pathEnd = delimiterOffset(url, pathStart, url.length(), "?#"); List<String> result = new ArrayList<>(); for (int i = pathStart; i < pathEnd; ) { @@ -590,13 +603,19 @@ public final class HttpUrl { /** Returns the URL that would be retrieved by following {@code link} from this URL. */ public HttpUrl resolve(String link) { - Builder builder = new Builder(); + // ANDROID-BEGIN: http://b/29983827 + // Builder builder = new Builder(); + Builder builder = new Builder(false); + // ANDROID-END: http://b/29983827 Builder.ParseResult result = builder.parse(this, link); return result == Builder.ParseResult.SUCCESS ? builder.build() : null; } public Builder newBuilder() { - Builder result = new Builder(); + // ANDROID-BEGIN: http://b/29983827 + // Builder builder = new Builder(); + Builder result = new Builder(false); + // ANDROID-END: http://b/29983827 result.scheme = scheme; result.encodedUsername = encodedUsername(); result.encodedPassword = encodedPassword(); @@ -615,7 +634,10 @@ public final class HttpUrl { * URL, or null if it isn't. */ public static HttpUrl parse(String url) { - Builder builder = new Builder(); + // ANDROID-BEGIN: http://b/29983827 + // Builder builder = new Builder(); + Builder builder = new Builder(false); + // ANDROID-END: http://b/29983827 Builder.ParseResult result = builder.parse(null, url); return result == Builder.ParseResult.SUCCESS ? builder.build() : null; } @@ -636,7 +658,10 @@ public final class HttpUrl { * @throws UnknownHostException if the host was invalid */ static HttpUrl getChecked(String url) throws MalformedURLException, UnknownHostException { - Builder builder = new Builder(); + // ANDROID-END: http://b/29983827 + // Builder builder = new Builder(); + Builder builder = new Builder(false); + // ANDROID-END: http://b/29983827 Builder.ParseResult result = builder.parse(null, url); switch (result) { case SUCCESS: @@ -677,10 +702,22 @@ public final class HttpUrl { List<String> encodedQueryNamesAndValues; String encodedFragment; + // ANDROID-BEGIN: http://b/29983827 + // public Builder() { + // encodedPathSegments.add(""); // The default path is '/' which needs a trailing space. + // } + public Builder() { - encodedPathSegments.add(""); // The default path is '/' which needs a trailing space. + this(true); // // The default path is '/' which needs a trailing space. } + private Builder(boolean startWithSlash) { + if (startWithSlash) { + encodedPathSegments.add(""); + } + } + // ANDROID-END: http://b/29983827 + public Builder scheme(String scheme) { if (scheme == null) { throw new IllegalArgumentException("scheme == null"); @@ -782,9 +819,12 @@ public final class HttpUrl { public Builder removePathSegment(int index) { encodedPathSegments.remove(index); - if (encodedPathSegments.isEmpty()) { - encodedPathSegments.add(""); // Always leave at least one '/'. - } + // ANDROID-BEGIN: http://b/29983827. Note this method only used from tests. + // Only changed for consistency. + // if (encodedPathSegments.isEmpty()) { + // encodedPathSegments.add(""); // Always leave at least one '/'. + // } + // ANDROID-END: http://b/29983827 - only used from tests return this; } @@ -1112,8 +1152,14 @@ public final class HttpUrl { encodedPathSegments.add(""); pos++; } else { - // Relative path: clear everything after the last '/'. - encodedPathSegments.set(encodedPathSegments.size() - 1, ""); + // ANDROID-BEGIN: http://b/29983827 + // // Relative path: clear everything after the last '/'. + // encodedPathSegments.set(encodedPathSegments.size() - 1, ""); + // Relative path: clear everything after the last '/' (if there is one). + if (!encodedPathSegments.isEmpty()) { + encodedPathSegments.set(encodedPathSegments.size() - 1, ""); + } + // ANDROID-END: http://b/29983827 } // Read path segments. @@ -1138,6 +1184,15 @@ public final class HttpUrl { pop(); return; } + + // ANDROID-BEGIN: http://b/29983827 + // If the encodedPathSegments doesn't even include "/" then add the leading "/" before + // pushing more segments or modifying existing segments. + if (encodedPathSegments.isEmpty()) { + encodedPathSegments.add(""); + } + // ANDROID-END: http://b/29983827 + if (encodedPathSegments.get(encodedPathSegments.size() - 1).isEmpty()) { encodedPathSegments.set(encodedPathSegments.size() - 1, segment); } else { @@ -1170,6 +1225,14 @@ public final class HttpUrl { * to ["a", "b", ""]. */ private void pop() { + // ANDROID-BEGIN: http://b/29983827 + // Cannot pop() if there isn't even a "/". Leave the path as is. This method is only used + // from push(). push() handles the empty case explicitly. + if (encodedPathSegments.isEmpty()) { + return; + } + // ANDROID-END: http://b/29983827 + String removed = encodedPathSegments.remove(encodedPathSegments.size() - 1); // Make sure the path ends with a '/' by either adding an empty string or clearing a segment. diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/RequestLine.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/RequestLine.java index d22be27..89a3922 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/RequestLine.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/RequestLine.java @@ -46,6 +46,13 @@ public final class RequestLine { */ public static String requestPath(HttpUrl url) { String path = url.encodedPath(); + // ANDROID-BEGIN: http://b/29983827 - Now path can be empty, which is forbidden in relative + // paths so we must handle it here. + if (path.isEmpty()) { + path = "/"; + } + // ANDROID-END: http://b/29983827 + String query = url.encodedQuery(); return query != null ? (path + '?' + query) : path; } |