Repository: aurora Updated Branches: refs/heads/master 59f66ffcd -> c69ccb9f1
Make charset parsing in HTTP headers case insensitve Users have reported that the UI does not load in Firefox. Investigating the issue shows that Chrome and Safari will format charset into an uppercase `UTF-8` which is accepted by the servlet. However, Mozilla will leave the charset as lowercase `utf-8` which causes a 415 response. Charsets should be case-insensitive but the default Java `MediaType` class does not take this into account when parsing/comparing. I propose switching to Guava's `MediaType` class which does smarter comparisons. Reviewed at https://reviews.apache.org/r/65690/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/c69ccb9f Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/c69ccb9f Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/c69ccb9f Branch: refs/heads/master Commit: c69ccb9f13acdb99f25385a6c783c71376839599 Parents: 59f66ff Author: Jordan Ly <[email protected]> Authored: Sun Feb 18 23:26:15 2018 -0800 Committer: Jordan Ly <[email protected]> Committed: Sun Feb 18 23:26:15 2018 -0800 ---------------------------------------------------------------------- .../aurora/scheduler/http/api/ApiModule.java | 28 +++++++++++--------- .../http/api/TContentAwareServlet.java | 7 ++--- .../apache/aurora/scheduler/http/api/ApiIT.java | 26 +++++++++++------- 3 files changed, 36 insertions(+), 25 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/c69ccb9f/src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java b/src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java index 2820cda..09950a7 100644 --- a/src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java +++ b/src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java @@ -13,12 +13,13 @@ */ package org.apache.aurora.scheduler.http.api; +import java.nio.charset.Charset; import javax.inject.Singleton; -import javax.ws.rs.core.MediaType; import com.beust.jcommander.Parameter; import com.beust.jcommander.Parameters; import com.google.common.collect.ImmutableMap; +import com.google.common.net.MediaType; import com.google.inject.Provides; import com.google.inject.servlet.ServletModule; @@ -34,17 +35,17 @@ import org.apache.thrift.protocol.TJSONProtocol; import org.eclipse.jetty.servlet.DefaultServlet; import org.eclipse.jetty.util.resource.Resource; -import static javax.ws.rs.core.MediaType.APPLICATION_JSON_TYPE; - public class ApiModule extends ServletModule { public static final String API_PATH = "/api"; - private static final MediaType GENERIC_THRIFT = new MediaType("application", "x-thrift"); - private static final MediaType THRIFT_JSON = - new MediaType("application", "vnd.apache.thrift.json"); - private static final MediaType THRIFT_JSON_UTF_8 = - new MediaType("application", "vnd.apache.thrift.json", "UTF-8"); - private static final MediaType THRIFT_BINARY = - new MediaType("application", "vnd.apache.thrift.binary"); + private static final MediaType GENERIC_JSON = MediaType.JSON_UTF_8.withoutParameters(); + private static final MediaType GENERIC_THRIFT = MediaType.create("application", "x-thrift"); + private static final MediaType THRIFT_JSON = MediaType + .create("application", "vnd.apache.thrift.json"); + private static final MediaType THRIFT_JSON_UTF_8 = MediaType + .create("application", "vnd.apache.thrift.json") + .withCharset(Charset.forName("UTF-8")); + private static final MediaType THRIFT_BINARY = MediaType + .create("application", "vnd.apache.thrift.binary"); @Parameters(separators = "=") public static class Options { @@ -112,18 +113,19 @@ public class ApiModule extends ServletModule { // Which factory to use based on the Content-Type header of the request for reading the request. InputConfig inputConfig = new InputConfig(GENERIC_THRIFT, ImmutableMap.of( + GENERIC_JSON, jsonFactory, GENERIC_THRIFT, jsonFactory, THRIFT_JSON, jsonFactory, THRIFT_JSON_UTF_8, jsonFactory, - APPLICATION_JSON_TYPE, jsonFactory, THRIFT_BINARY, binFactory )); // Which factory to use based on the Accept header of the request for the response. - OutputConfig outputConfig = new OutputConfig(APPLICATION_JSON_TYPE, ImmutableMap.of( - APPLICATION_JSON_TYPE, jsonFactory, + OutputConfig outputConfig = new OutputConfig(GENERIC_JSON, ImmutableMap.of( + GENERIC_JSON, jsonFactory, GENERIC_THRIFT, jsonFactory, THRIFT_JSON, jsonFactory, + THRIFT_JSON_UTF_8, jsonFactory, THRIFT_BINARY, binFactory )); http://git-wip-us.apache.org/repos/asf/aurora/blob/c69ccb9f/src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java b/src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java index d9df968..3a9ae43 100644 --- a/src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java +++ b/src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java @@ -22,7 +22,8 @@ import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import javax.ws.rs.core.MediaType; + +import com.google.common.net.MediaType; import org.apache.thrift.TException; import org.apache.thrift.TProcessor; @@ -120,7 +121,7 @@ public class TContentAwareServlet extends HttpServlet { throws ServletException, IOException { Optional<ContentFactoryPair> factoryOptional = inputConfig - .getFactory(Optional.ofNullable(request.getContentType()).map(MediaType::valueOf)); + .getFactory(Optional.ofNullable(request.getContentType()).map(MediaType::parse)); if (!factoryOptional.isPresent()) { response.setStatus(HttpServletResponse.SC_UNSUPPORTED_MEDIA_TYPE); @@ -138,7 +139,7 @@ public class TContentAwareServlet extends HttpServlet { Optional<MediaType> acceptType = Optional.empty(); if (acceptHeader.isPresent()) { try { - acceptType = acceptHeader.map(MediaType::valueOf); + acceptType = acceptHeader.map(MediaType::parse); } catch (IllegalArgumentException e) { // Thrown if the Accept header contains more than one type or something else we can't // parse, we just treat is as no header (which will pick up the default value). http://git-wip-us.apache.org/repos/asf/aurora/blob/c69ccb9f/src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java b/src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java index bfd117b..910af28 100644 --- a/src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java +++ b/src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java @@ -19,9 +19,9 @@ import java.util.function.Function; import javax.servlet.ServletContext; import javax.ws.rs.core.HttpHeaders; -import javax.ws.rs.core.MediaType; import com.google.common.collect.ImmutableList; +import com.google.common.net.MediaType; import com.google.common.primitives.Bytes; import com.google.inject.AbstractModule; import com.google.inject.Module; @@ -96,22 +96,30 @@ public class ApiIT extends AbstractJettyTest { @Test public void testThriftJsonUtf8Accepted() throws Exception { - expect(thrift.getRoleSummary()).andReturn(new Response()); + expect(thrift.getRoleSummary()).andReturn(new Response()).times(2); replayAndStart(); - // TODO(rdelvalle): If UTF-8 is not in caps, the accept method doesn't register a charset - // when building the request. The servlet accepts the charset in lower caps for now but - // might be worth resisting this oddity in the future to prevent regressions. - ClientResponse response = getPlainRequestBuilder(ApiModule.API_PATH) + // We also want to ensure charset parsing is case-insensitive because different browsers have + // different default behaviors (Chrome and Safari will change charset to all uppercase, while + // Firefox may leave it lowercase. + ClientResponse upperCaseUTF = getPlainRequestBuilder(ApiModule.API_PATH) .type("application/vnd.apache.thrift.json; charset=UTF-8") .accept("application/vnd.apache.thrift.json; charset=UTF-8") .post(ClientResponse.class, JSON_FIXTURE); + assertEquals(SC_OK, upperCaseUTF.getStatus()); + assertEquals( + "application/vnd.apache.thrift.json", + upperCaseUTF.getHeaders().getFirst(CONTENT_TYPE)); - assertEquals(SC_OK, response.getStatus()); + ClientResponse lowerCaseUTF = getPlainRequestBuilder(ApiModule.API_PATH) + .type("application/vnd.apache.thrift.json; charset=utf-8") + .accept("application/vnd.apache.thrift.json; charset=utf-8") + .post(ClientResponse.class, JSON_FIXTURE); + assertEquals(SC_OK, lowerCaseUTF.getStatus()); assertEquals( "application/vnd.apache.thrift.json", - response.getHeaders().getFirst(CONTENT_TYPE)); + lowerCaseUTF.getHeaders().getFirst(CONTENT_TYPE)); } @Test @@ -119,7 +127,7 @@ public class ApiIT extends AbstractJettyTest { replayAndStart(); ClientResponse response = getRequestBuilder(ApiModule.API_PATH) - .type(MediaType.TEXT_HTML_TYPE) + .type(MediaType.PLAIN_TEXT_UTF_8.toString()) .post(ClientResponse.class, JSON_FIXTURE); assertEquals(SC_UNSUPPORTED_MEDIA_TYPE, response.getStatus());
