Fix regression in the HTTP request line
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)
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 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 @@
@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 @@
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 @@
.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 @@
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 @@
}
/**
- * 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 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 @@
/** 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 @@
* 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 @@
* @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 @@
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 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 @@
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 @@
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 @@
* 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 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;
}
