Skip to content

Commit

Permalink
Merge pull request square#1785 from square/jwilson_0803_strict_headers
Browse files Browse the repository at this point in the history 
Be strict on invalid characters in request headers.
  • Loading branch information
@JakeWharton
JakeWharton committed Aug 3, 2015
2 parents 7cf6363 + a57aa43 commit bf69c55
Show file tree
Hide file tree
Showing 5 changed files with 157 additions and 31 deletions.
20 changes: 20 additions & 0 deletions okhttp-tests/src/test/java/com/squareup/okhttp/CallTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1822,6 +1822,26 @@ private InetSocketAddress startNullServer() throws IOException {
.assertSuccessful();
}

/** We forbid non-ASCII characters in outgoing request headers, but accept UTF-8. */
@Test public void responseHeaderParsingIsLenient() throws Exception {
Headers headers = new Headers.Builder()
.add("Content-Length", "0")
.addLenient("a\tb: c\u007fd")
.addLenient(": ef")
.addLenient("\ud83c\udf69: \u2615\ufe0f")
.build();
server.enqueue(new MockResponse().setHeaders(headers));

Request request = new Request.Builder()
.url(server.url("/"))
.build();

executeSynchronously(request)
.assertHeader("a\tb", "c\u007fd")
.assertHeader("\ud83c\udf69", "\u2615\ufe0f")
.assertHeader("", "ef");
}

private RecordedResponse executeSynchronously(Request request) throws IOException {
Response response = client.newCall(request).execute();
return new RecordedResponse(request, response, null, response.body().string(), null);
Expand Down
1 change: 1 addition & 0 deletions okhttp-tests/src/test/java/com/squareup/okhttp/HttpUrlTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ public final class HttpUrlTest {
@Test public void port() throws Exception {
assertEquals(HttpUrl.parse("http://host/"), HttpUrl.parse("http://host:80/"));
assertEquals(HttpUrl.parse("http://host:99/"), HttpUrl.parse("http://host:99/"));
assertEquals(HttpUrl.parse("http://host/"), HttpUrl.parse("http://host:/"));
assertEquals(65535, HttpUrl.parse("http://host:65535/").port());
assertEquals(null, HttpUrl.parse("http://host:0/"));
assertEquals(null, HttpUrl.parse("http://host:65536/"));
Expand Down
60 changes: 59 additions & 1 deletion okhttp-tests/src/test/java/com/squareup/okhttp/RequestTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.fail;

public final class RequestTest {
@Test public void string() throws Exception {
Expand Down Expand Up @@ -131,7 +132,8 @@ public final class RequestTest {
Request requestWithCache = new Request.Builder().url("http://localhost/api").build();
// cache url object
requestWithCache.url();
Request builtRequestWithCache = requestWithCache.newBuilder().url("http://localhost/api/foo").build();
Request builtRequestWithCache = requestWithCache.newBuilder().url(
"http://localhost/api/foo").build();
assertEquals(new URL("http://localhost/api/foo"), builtRequestWithCache.url());
}

Expand All @@ -152,6 +154,62 @@ public final class RequestTest {
assertEquals(Collections.<String>emptyList(), request.headers("Cache-Control"));
}

@Test public void headerAcceptsPermittedCharacters() throws Exception {
Request.Builder builder = new Request.Builder();
builder.header("AZab09 ~", "AZab09 ~");
builder.addHeader("AZab09 ~", "AZab09 ~");
}

@Test public void emptyNameForbidden() throws Exception {
Request.Builder builder = new Request.Builder();
try {
builder.header("", "Value");
fail();
} catch (IllegalArgumentException expected) {
}
try {
builder.addHeader("", "Value");
fail();
} catch (IllegalArgumentException expected) {
}
}

@Test public void headerForbidsControlCharacters() throws Exception {
assertForbiddenHeader(null);
assertForbiddenHeader("\u0000");
assertForbiddenHeader("\r");
assertForbiddenHeader("\n");
assertForbiddenHeader("\t");
assertForbiddenHeader("\u001f");
assertForbiddenHeader("\u007f");
assertForbiddenHeader("\u0080");
assertForbiddenHeader("\ud83c\udf69");
}

private void assertForbiddenHeader(String s) {
Request.Builder builder = new Request.Builder();
try {
builder.header(s, "Value");
fail();
} catch (IllegalArgumentException expected) {
}
try {
builder.addHeader(s, "Value");
fail();
} catch (IllegalArgumentException expected) {
}
try {
builder.header("Name", s);
fail();
} catch (IllegalArgumentException expected) {
}
try {
builder.addHeader("Name", s);
fail();
} catch (IllegalArgumentException expected) {
}
}

private String bodyToHex(RequestBody body) throws IOException {
Buffer buffer = new Buffer();
body.writeTo(buffer);
Expand Down
78 changes: 54 additions & 24 deletions ...http/internal/http/URLConnectionTest.java → ...om/squareup/okhttp/URLConnectionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,8 @@
* limitations under the License.
*/

package com.squareup.okhttp.internal.http;

import com.squareup.okhttp.Cache;
import com.squareup.okhttp.Challenge;
import com.squareup.okhttp.ConnectionPool;
import com.squareup.okhttp.ConnectionSpec;
import com.squareup.okhttp.Credentials;
import com.squareup.okhttp.DelegatingServerSocketFactory;
import com.squareup.okhttp.DelegatingSocketFactory;
import com.squareup.okhttp.FallbackTestClientSocketFactory;
import com.squareup.okhttp.Headers;
import com.squareup.okhttp.Interceptor;
import com.squareup.okhttp.OkHttpClient;
import com.squareup.okhttp.OkUrlFactory;
import com.squareup.okhttp.Protocol;
import com.squareup.okhttp.Response;
import com.squareup.okhttp.TlsVersion;
package com.squareup.okhttp;

import com.squareup.okhttp.internal.Internal;
import com.squareup.okhttp.internal.RecordingAuthenticator;
import com.squareup.okhttp.internal.RecordingOkAuthenticator;
Expand Down Expand Up @@ -67,7 +52,7 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.EnumSet;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Random;
Expand Down Expand Up @@ -150,8 +135,8 @@ public final class URLConnectionTest {
assertEquals("f", connection.getRequestProperty("D"));
assertEquals("f", connection.getRequestProperty("d"));
Map<String, List<String>> requestHeaders = connection.getRequestProperties();
assertEquals(newSet("e", "f"), new HashSet<String>(requestHeaders.get("D")));
assertEquals(newSet("e", "f"), new HashSet<String>(requestHeaders.get("d")));
assertEquals(newSet("e", "f"), new LinkedHashSet<>(requestHeaders.get("D")));
assertEquals(newSet("e", "f"), new LinkedHashSet<>(requestHeaders.get("d")));
try {
requestHeaders.put("G", Arrays.asList("h"));
fail("Modified an unmodifiable view.");
Expand Down Expand Up @@ -222,8 +207,8 @@ public final class URLConnectionTest {
assertEquals("HTTP/1.0 200 Fantastic", connection.getHeaderField(null));
Map<String, List<String>> responseHeaders = connection.getHeaderFields();
assertEquals(Arrays.asList("HTTP/1.0 200 Fantastic"), responseHeaders.get(null));
assertEquals(newSet("c", "e"), new HashSet<String>(responseHeaders.get("A")));
assertEquals(newSet("c", "e"), new HashSet<String>(responseHeaders.get("a")));
assertEquals(newSet("c", "e"), new LinkedHashSet<>(responseHeaders.get("A")));
assertEquals(newSet("c", "e"), new LinkedHashSet<>(responseHeaders.get("a")));
try {
responseHeaders.put("N", Arrays.asList("o"));
fail("Modified an unmodifiable view.");
Expand Down Expand Up @@ -2498,7 +2483,7 @@ private void testFlushAfterStreamTransmitted(TransferKind transferKind) throws I
fail();
} catch (NullPointerException expected) {
}
assertNull(connection.getContent(new Class[]{getClass()}));
assertNull(connection.getContent(new Class[] { getClass() }));
}

@Test public void getOutputStreamOnGetFails() throws Exception {
Expand Down Expand Up @@ -2798,6 +2783,51 @@ private void reusedConnectionFailsWithPost(TransferKind transferKind, int reques
assertEquals("A", connection.getHeaderField(""));
}

@Test public void requestHeaderValidationIsStrict() throws Exception {
connection = client.open(server.getUrl("/"));
try {
connection.addRequestProperty("a\tb", "Value");
fail();
} catch (IllegalArgumentException expected) {
}
try {
connection.addRequestProperty("Name", "c\u007fd");
fail();
} catch (IllegalArgumentException expected) {
}
try {
connection.addRequestProperty("", "Value");
fail();
} catch (IllegalArgumentException expected) {
}
try {
connection.addRequestProperty("\ud83c\udf69", "Value");
fail();
} catch (IllegalArgumentException expected) {
}
try {
connection.addRequestProperty("Name", "\u2615\ufe0f");
fail();
} catch (IllegalArgumentException expected) {
}
}

@Test public void responseHeaderParsingIsLenient() throws Exception {
Headers headers = new Headers.Builder()
.add("Content-Length", "0")
.addLenient("a\tb: c\u007fd")
.addLenient(": ef")
.addLenient("\ud83c\udf69: \u2615\ufe0f")
.build();
server.enqueue(new MockResponse().setHeaders(headers));

connection = client.open(server.getUrl("/"));
connection.getResponseCode();
assertEquals("c\u007fd", connection.getHeaderField("a\tb"));
assertEquals("\u2615\ufe0f", connection.getHeaderField("\ud83c\udf69"));
assertEquals("ef", connection.getHeaderField(""));
}

@Test @Ignore public void deflateCompression() {
fail("TODO");
}
Expand Down Expand Up @@ -3160,7 +3190,7 @@ private void assertContent(String expected, HttpURLConnection connection) throws
}

private Set<String> newSet(String... elements) {
return new HashSet<String>(Arrays.asList(elements));
return new LinkedHashSet<>(Arrays.asList(elements));
}

enum TransferKind {
Expand Down
29 changes: 23 additions & 6 deletions okhttp/src/main/java/com/squareup/okhttp/Headers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,11 +242,7 @@ public Builder add(String line) {

/** Add a field with the specified value. */
public Builder add(String name, String value) {
if (name == null) throw new IllegalArgumentException("name == null");
if (value == null) throw new IllegalArgumentException("value == null");
if (name.length() == 0 || name.indexOf('\0') != -1 || value.indexOf('\0') != -1) {
throw new IllegalArgumentException("Unexpected header: " + name + ": " + value);
}
checkNameAndValue(name, value);
return addLenient(name, value);
}

Expand Down Expand Up @@ -276,11 +272,32 @@ public Builder removeAll(String name) {
* added. If the field is found, the existing values are replaced.
*/
public Builder set(String name, String value) {
checkNameAndValue(name, value);
removeAll(name);
add(name, value);
addLenient(name, value);
return this;
}

private void checkNameAndValue(String name, String value) {
if (name == null) throw new IllegalArgumentException("name == null");
if (name.isEmpty()) throw new IllegalArgumentException("name is empty");
for (int i = 0, length = name.length(); i < length; i++) {
char c = name.charAt(i);
if (c <= '\u001f' || c >= '\u007f') {
throw new IllegalArgumentException(String.format(
"Unexpected char %#04x at %d in header name: %s", (int) c, i, name));
}
}
if (value == null) throw new IllegalArgumentException("value == null");
for (int i = 0, length = value.length(); i < length; i++) {
char c = value.charAt(i);
if (c <= '\u001f' || c >= '\u007f') {
throw new IllegalArgumentException(String.format(
"Unexpected char %#04x at %d in header value: %s", (int) c, i, value));
}
}
}

/** Equivalent to {@code build().get(name)}, but potentially faster. */
public String get(String name) {
for (int i = namesAndValues.size() - 2; i >= 0; i -= 2) {
Expand Down

0 comments on commit bf69c55

Please sign in to comment.