From 6651a9c15e06e8d1829aa00f970998be2f33b225 Mon Sep 17 00:00:00 2001 From: jwilson Date: Sun, 29 Jan 2017 13:02:08 -0500 Subject: [PATCH] Change MediaType's failure mode to not crash on charset problems. As-is it throws unchecked exceptions on unexpected charsets. This is a problem because it can cause a misbehaving webserver to crash the client. I don't expect this to break existing clients; returning 'null' has always been a possibility; it's just returned in more cases. --- .../logging/HttpLoggingInterceptor.java | 11 +----- .../logging/HttpLoggingInterceptorTest.java | 10 +++--- .../src/test/java/okhttp3/MediaTypeTest.java | 35 ++++--------------- okhttp/src/main/java/okhttp3/MediaType.java | 14 +++++--- 4 files changed, 21 insertions(+), 49 deletions(-) diff --git a/okhttp-logging-interceptor/src/main/java/okhttp3/logging/HttpLoggingInterceptor.java b/okhttp-logging-interceptor/src/main/java/okhttp3/logging/HttpLoggingInterceptor.java index c5217b16c956..173d88caffb5 100644 --- a/okhttp-logging-interceptor/src/main/java/okhttp3/logging/HttpLoggingInterceptor.java +++ b/okhttp-logging-interceptor/src/main/java/okhttp3/logging/HttpLoggingInterceptor.java @@ -18,7 +18,6 @@ import java.io.EOFException; import java.io.IOException; import java.nio.charset.Charset; -import java.nio.charset.UnsupportedCharsetException; import java.util.concurrent.TimeUnit; import okhttp3.Connection; import okhttp3.Headers; @@ -241,15 +240,7 @@ public Level getLevel() { Charset charset = UTF8; MediaType contentType = responseBody.contentType(); if (contentType != null) { - try { - charset = contentType.charset(UTF8); - } catch (UnsupportedCharsetException e) { - logger.log(""); - logger.log("Couldn't decode the response body; charset is likely malformed."); - logger.log("<-- END HTTP"); - - return response; - } + charset = contentType.charset(UTF8); } if (!isPlaintext(buffer)) { diff --git a/okhttp-logging-interceptor/src/test/java/okhttp3/logging/HttpLoggingInterceptorTest.java b/okhttp-logging-interceptor/src/test/java/okhttp3/logging/HttpLoggingInterceptorTest.java index caa6cd72bdd0..c631899fb343 100644 --- a/okhttp-logging-interceptor/src/test/java/okhttp3/logging/HttpLoggingInterceptorTest.java +++ b/okhttp-logging-interceptor/src/test/java/okhttp3/logging/HttpLoggingInterceptorTest.java @@ -563,7 +563,7 @@ private void bodyGetNoBody(int code) throws IOException { server.enqueue(new MockResponse() .setHeader("Content-Type", "text/html; charset=0") - .setBody("Ignore This")); + .setBody("Body with unknown charset")); Response response = client.newCall(request().build()).execute(); response.body().close(); @@ -578,8 +578,8 @@ private void bodyGetNoBody(int code) throws IOException { .assertLogEqual("Content-Type: text/html; charset=0") .assertLogMatch("Content-Length: \\d+") .assertLogMatch("") - .assertLogEqual("Couldn't decode the response body; charset is likely malformed.") - .assertLogEqual("<-- END HTTP") + .assertLogEqual("Body with unknown charset") + .assertLogEqual("<-- END HTTP (25-byte body)") .assertNoMoreLogs(); applicationLogs @@ -589,8 +589,8 @@ private void bodyGetNoBody(int code) throws IOException { .assertLogEqual("Content-Type: text/html; charset=0") .assertLogMatch("Content-Length: \\d+") .assertLogEqual("") - .assertLogEqual("Couldn't decode the response body; charset is likely malformed.") - .assertLogEqual("<-- END HTTP") + .assertLogEqual("Body with unknown charset") + .assertLogEqual("<-- END HTTP (25-byte body)") .assertNoMoreLogs(); } diff --git a/okhttp-tests/src/test/java/okhttp3/MediaTypeTest.java b/okhttp-tests/src/test/java/okhttp3/MediaTypeTest.java index 5e4a14f4841f..b2d3f396fa1f 100644 --- a/okhttp-tests/src/test/java/okhttp3/MediaTypeTest.java +++ b/okhttp-tests/src/test/java/okhttp3/MediaTypeTest.java @@ -17,15 +17,12 @@ package okhttp3; import java.nio.charset.Charset; -import java.nio.charset.IllegalCharsetNameException; -import java.nio.charset.UnsupportedCharsetException; import okhttp3.internal.Util; import org.junit.Test; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; /** * Test MediaType API and parsing. @@ -123,29 +120,17 @@ public class MediaTypeTest { } @Test public void testMultipleCharsets() { - try { - MediaType.parse("text/plain; charset=utf-8; charset=utf-16"); - fail(); - } catch (IllegalArgumentException expected) { - } + assertNull(MediaType.parse("text/plain; charset=utf-8; charset=utf-16")); } @Test public void testIllegalCharsetName() { MediaType mediaType = MediaType.parse("text/plain; charset=\"!@#$%^&*()\""); - try { - mediaType.charset(); - fail(); - } catch (IllegalCharsetNameException expected) { - } + assertNull(mediaType.charset()); } @Test public void testUnsupportedCharset() { MediaType mediaType = MediaType.parse("text/plain; charset=utf-wtf"); - try { - mediaType.charset(); - fail(); - } catch (UnsupportedCharsetException expected) { - } + assertNull(mediaType.charset()); } /** @@ -159,20 +144,12 @@ public class MediaTypeTest { @Test public void testCharsetNameIsDoubleQuotedAndSingleQuoted() throws Exception { MediaType mediaType = MediaType.parse("text/plain;charset=\"'utf-8'\""); - try { - mediaType.charset(); - fail(); - } catch (IllegalCharsetNameException expected) { - } + assertNull(mediaType.charset()); } @Test public void testCharsetNameIsDoubleQuotedSingleQuote() throws Exception { MediaType mediaType = MediaType.parse("text/plain;charset=\"'\""); - try { - mediaType.charset(); - fail(); - } catch (IllegalCharsetNameException expected) { - } + assertNull(mediaType.charset()); } @Test public void testDefaultCharset() throws Exception { @@ -189,7 +166,7 @@ public class MediaTypeTest { MediaType mediaType = MediaType.parse("text/plain;"); assertEquals("text", mediaType.type()); assertEquals("plain", mediaType.subtype()); - assertEquals(null, mediaType.charset()); + assertNull(mediaType.charset()); assertEquals("text/plain;", mediaType.toString()); } diff --git a/okhttp/src/main/java/okhttp3/MediaType.java b/okhttp/src/main/java/okhttp3/MediaType.java index 77b943e5892b..d171f06c1bea 100644 --- a/okhttp/src/main/java/okhttp3/MediaType.java +++ b/okhttp/src/main/java/okhttp3/MediaType.java @@ -73,7 +73,7 @@ public static MediaType parse(String string) { charsetParameter = parameter.group(3); } if (charset != null && !charsetParameter.equalsIgnoreCase(charset)) { - throw new IllegalArgumentException("Multiple different charsets: " + string); + return null; // Multiple different charsets! } charset = charsetParameter; } @@ -100,15 +100,19 @@ public String subtype() { * Returns the charset of this media type, or null if this media type doesn't specify a charset. */ public Charset charset() { - return charset != null ? Charset.forName(charset) : null; + return charset(null); } /** - * Returns the charset of this media type, or {@code defaultValue} if this media type doesn't - * specify a charset. + * Returns the charset of this media type, or {@code defaultValue} if either this media type + * doesn't specify a charset, of it its charset is unsupported by the current runtime. */ public Charset charset(Charset defaultValue) { - return charset != null ? Charset.forName(charset) : defaultValue; + try { + return charset != null ? Charset.forName(charset) : defaultValue; + } catch (IllegalArgumentException e) { + return defaultValue; // This charset is invalid or unsupported. Give up. + } } /**