Skip to content

Commit

Permalink
Be strict on invalid characters in request headers.
Browse files Browse the repository at this point in the history 
This includes potential security problems (newline characters) as well as
simple non-ASCII characters including international characters and emoji.

Closes #891
  • Loading branch information
@swankjesse
swankjesse committed Aug 3, 2015
1 parent 7cf6363 commit a57aa43
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 a57aa43

Please sign in to comment.