Merge pull request square#3143 from square/jwilson.0129.bogus_charsets

Change MediaType's failure mode to not crash on charset problems.
hieronymus777 · Jan 30, 2017 · 3e12610 · 3e12610
2 parents 9690110 + 6651a9c
commit 3e12610
Show file tree

Hide file tree

Showing 4 changed files with 21 additions and 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
@@ -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
@@ -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
@@ -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
@@ -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.
+    }
   }
 
   /**