Fix issue with parsing media types

Invalid Content-Type or Accept header values previously resulted in the
IllegalArgumentException getting propagated. After this change such
errors are detected and generally treated as a "no match", which
may for example result in a 406 in the case of the Accept header.

Issue: SPR-9142
Backport-Issue: SPR-9148
Backport-Commit: ca8b98e

Author: rstoyanchev

build-spring-framework/resources/changelog.txt

@@ -15,6 +15,7 @@ Changes in version 3.1.2 (2012-06-??)
 * EhCacheFactoryBean applies listeners and enabled/disabled flags to existing cache regions as well
 * ServletContextResource's getFile implementation falls back to getRealPath for non-existent files
 * fixed StandardServletMultipartResolver compatibility with Resin (only deleting actual file parts)
+* fix issue with parsing invalid Content-Type or Accept headers
 
 
 Changes in version 3.1.1 (2012-02-16)

org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/AbstractMediaTypeExpression.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2011 the original author or authors.
+ * Copyright 2002-2012 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -18,19 +18,23 @@
 
 import javax.servlet.http.HttpServletRequest;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 import org.springframework.http.MediaType;
 import org.springframework.web.bind.annotation.RequestMapping;
 
 /**
  * Supports media type expressions as described in:
  * {@link RequestMapping#consumes()} and {@link RequestMapping#produces()}.
- * 
+ *
  * @author Arjen Poutsma
  * @author Rossen Stoyanchev
  * @since 3.1
  */
 abstract class AbstractMediaTypeExpression implements Comparable<AbstractMediaTypeExpression>, MediaTypeExpression {
 
+	protected final Log logger = LogFactory.getLog(getClass());
+
 	private final MediaType mediaType;
 
 	private final boolean isNegated;
@@ -60,8 +64,16 @@ public boolean isNegated() {
 	}
 
 	public final boolean match(HttpServletRequest request) {
-		boolean match = matchMediaType(request);
-		return !isNegated ? match : !match;
+		try {
+			boolean match = matchMediaType(request);
+			return !isNegated ? match : !match;
+		}
+		catch (IllegalArgumentException ex) {
+			if (logger.isDebugEnabled()) {
+				logger.debug("Could not parse media type header: " + ex.getMessage());
+			}
+			return false;
+		}
 	}
 
 	protected abstract boolean matchMediaType(HttpServletRequest request);

org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2011 the original author or authors.
+ * Copyright 2002-2012 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -41,7 +41,7 @@
 import org.springframework.web.servlet.HandlerMapping;
 
 /**
- * Extends {@link AbstractMessageConverterMethodArgumentResolver} with the ability to handle method return 
+ * Extends {@link AbstractMessageConverterMethodArgumentResolver} with the ability to handle method return
  * values by writing to the response with {@link HttpMessageConverter}s.
  *
  * @author Arjen Poutsma
@@ -103,7 +103,7 @@ protected <T> void writeWithMessageConverters(T returnValue,
 
 		List<MediaType> acceptableMediaTypes = getAcceptableMediaTypes(inputMessage);
 		List<MediaType> producibleMediaTypes = getProducibleMediaTypes(inputMessage.getServletRequest(), returnValueClass);
-		
+
 		Set<MediaType> compatibleMediaTypes = new LinkedHashSet<MediaType>();
 		for (MediaType a : acceptableMediaTypes) {
 			for (MediaType p : producibleMediaTypes) {
@@ -115,10 +115,10 @@ protected <T> void writeWithMessageConverters(T returnValue,
 		if (compatibleMediaTypes.isEmpty()) {
 			throw new HttpMediaTypeNotAcceptableException(allSupportedMediaTypes);
 		}
-		
+
 		List<MediaType> mediaTypes = new ArrayList<MediaType>(compatibleMediaTypes);
 		MediaType.sortBySpecificity(mediaTypes);
-		
+
 		MediaType selectedMediaType = null;
 		for (MediaType mediaType : mediaTypes) {
 			if (mediaType.isConcrete()) {
@@ -130,7 +130,7 @@ else if (mediaType.equals(MediaType.ALL) || mediaType.equals(MEDIA_TYPE_APPLICAT
 				break;
 			}
 		}
-		
+
 		if (selectedMediaType != null) {
 			for (HttpMessageConverter<?> messageConverter : messageConverters) {
 				if (messageConverter.canWrite(returnValueClass, selectedMediaType)) {
@@ -166,7 +166,7 @@ else if (!allSupportedMediaTypes.isEmpty()) {
                 if (converter.canWrite(returnValueClass, null)) {
                 	result.addAll(converter.getSupportedMediaTypes());
                 }
-            }			
+            }
 			return result;
 		}
 		else {
@@ -175,8 +175,16 @@ else if (!allSupportedMediaTypes.isEmpty()) {
 	}
 
 	private List<MediaType> getAcceptableMediaTypes(HttpInputMessage inputMessage) {
-		List<MediaType> result = inputMessage.getHeaders().getAccept();
-		return result.isEmpty() ? Collections.singletonList(MediaType.ALL) : result;
+		try {
+			List<MediaType> result = inputMessage.getHeaders().getAccept();
+			return result.isEmpty() ? Collections.singletonList(MediaType.ALL) : result;
+		}
+		catch (IllegalArgumentException ex) {
+			if (logger.isDebugEnabled()) {
+				logger.debug("Could not parse Accept header: " + ex.getMessage());
+			}
+			return Collections.emptyList();
+		}
 	}
 
 	/**

org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/condition/ConsumesRequestConditionTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2011 the original author or authors.
+ * Copyright 2002-2012 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -44,7 +44,7 @@ public void consumesMatch() {
 
 		assertNotNull(condition.getMatchingCondition(request));
 	}
-	
+
 	@Test
 	public void negatedConsumesMatch() {
 		ConsumesRequestCondition condition = new ConsumesRequestCondition("!text/plain");
@@ -60,7 +60,7 @@ public void getConsumableMediaTypesNegatedExpression() {
 		ConsumesRequestCondition condition = new ConsumesRequestCondition("!application/xml");
 		assertEquals(Collections.emptySet(), condition.getConsumableMediaTypes());
 	}
-	
+
 	@Test
 	public void consumesWildcardMatch() {
 		ConsumesRequestCondition condition = new ConsumesRequestCondition("text/*");
@@ -91,6 +91,26 @@ public void consumesSingleNoMatch() {
 		assertNull(condition.getMatchingCondition(request));
 	}
 
+	@Test
+	public void consumesParseError() {
+		ConsumesRequestCondition condition = new ConsumesRequestCondition("text/plain");
+
+		MockHttpServletRequest request = new MockHttpServletRequest();
+		request.setContentType("01");
+
+		assertNull(condition.getMatchingCondition(request));
+	}
+
+	@Test
+	public void consumesParseErrorWithNegation() {
+		ConsumesRequestCondition condition = new ConsumesRequestCondition("!text/plain");
+
+		MockHttpServletRequest request = new MockHttpServletRequest();
+		request.setContentType("01");
+
+		assertNull(condition.getMatchingCondition(request));
+	}
+
 	@Test
 	public void compareToSingle() {
 		MockHttpServletRequest request = new MockHttpServletRequest();
@@ -128,7 +148,7 @@ public void combine() {
 		ConsumesRequestCondition result = condition1.combine(condition2);
 		assertEquals(condition2, result);
 	}
-	
+
 	@Test
 	public void combineWithDefault() {
 		ConsumesRequestCondition condition1 = new ConsumesRequestCondition("text/plain");

org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/condition/ProducesRequestConditionTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2011 the original author or authors.
+ * Copyright 2002-2012 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -34,7 +34,7 @@
  * @author Rossen Stoyanchev
  */
 public class ProducesRequestConditionTests {
-	
+
 	@Test
 	public void producesMatch() {
 		ProducesRequestCondition condition = new ProducesRequestCondition("text/plain");
@@ -44,7 +44,7 @@ public void producesMatch() {
 
 		assertNotNull(condition.getMatchingCondition(request));
 	}
-	
+
 	@Test
 	public void negatedProducesMatch() {
 		ProducesRequestCondition condition = new ProducesRequestCondition("!text/plain");
@@ -60,7 +60,7 @@ public void getProducibleMediaTypesNegatedExpression() {
 		ProducesRequestCondition condition = new ProducesRequestCondition("!application/xml");
 		assertEquals(Collections.emptySet(), condition.getProducibleMediaTypes());
 	}
-	
+
 	@Test
 	public void producesWildcardMatch() {
 		ProducesRequestCondition condition = new ProducesRequestCondition("text/*");
@@ -91,6 +91,26 @@ public void producesSingleNoMatch() {
 		assertNull(condition.getMatchingCondition(request));
 	}
 
+	@Test
+	public void producesParseError() {
+		ProducesRequestCondition condition = new ProducesRequestCondition("text/plain");
+
+		MockHttpServletRequest request = new MockHttpServletRequest();
+		request.addHeader("Accept", "bogus");
+
+		assertNull(condition.getMatchingCondition(request));
+	}
+
+	@Test
+	public void producesParseErrorWithNegation() {
+		ProducesRequestCondition condition = new ProducesRequestCondition("!text/plain");
+
+		MockHttpServletRequest request = new MockHttpServletRequest();
+		request.addHeader("Accept", "bogus");
+
+		assertNull(condition.getMatchingCondition(request));
+	}
+
 	@Test
 	public void compareTo() {
 		ProducesRequestCondition html = new ProducesRequestCondition("text/html");
@@ -126,12 +146,12 @@ public void compareTo() {
 		assertTrue(html.compareTo(xml, request) > 0);
 		assertTrue(xml.compareTo(html, request) < 0);
 	}
-	
+
 	@Test
 	public void compareToWithSingleExpression() {
 		MockHttpServletRequest request = new MockHttpServletRequest();
 		request.addHeader("Accept", "text/plain");
-		
+
 		ProducesRequestCondition condition1 = new ProducesRequestCondition("text/plain");
 		ProducesRequestCondition condition2 = new ProducesRequestCondition("text/*");
 
@@ -188,7 +208,7 @@ public void compareToMultipleExpressionsAndMultipeAcceptHeaderValues() {
 	@Test
 	public void compareToMediaTypeAll() {
 		MockHttpServletRequest request = new MockHttpServletRequest();
-		
+
 		ProducesRequestCondition condition1 = new ProducesRequestCondition();
 		ProducesRequestCondition condition2 = new ProducesRequestCondition("application/json");
 
@@ -202,7 +222,7 @@ public void compareToMediaTypeAll() {
 
 		assertTrue(condition1.compareTo(condition2, request) < 0);
 		assertTrue(condition2.compareTo(condition1, request) > 0);
-	
+
 		request.addHeader("Accept", "*/*");
 
 		condition1 = new ProducesRequestCondition();
@@ -255,7 +275,7 @@ public void combine() {
 		ProducesRequestCondition result = condition1.combine(condition2);
 		assertEquals(condition2, result);
 	}
-	
+
 	@Test
 	public void combineWithDefault() {
 		ProducesRequestCondition condition1 = new ProducesRequestCondition("text/plain");

org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2011 the original author or authors.
+ * Copyright 2002-2012 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -56,7 +56,7 @@
 
 /**
  * Test fixture with {@link HttpEntityMethodProcessor} and mock {@link HttpMessageConverter}.
- * 
+ *
  * @author Arjen Poutsma
  * @author Rossen Stoyanchev
  */
@@ -106,7 +106,7 @@ public void setUp() throws Exception {
 		returnTypeResponseEntityProduces = new MethodParameter(getClass().getMethod("handle4"), -1);
 
 		mavContainer = new ModelAndViewContainer();
-		
+
 		servletRequest = new MockHttpServletRequest();
 		servletResponse = new MockHttpServletResponse();
 		webRequest = new ServletWebRequest(servletRequest, servletResponse);
@@ -219,7 +219,7 @@ public void handleReturnValueNotAcceptable() throws Exception {
 
 		fail("Expected exception");
 	}
-	
+
 	@Test(expected = HttpMediaTypeNotAcceptableException.class)
 	public void handleReturnValueNotAcceptableProduces() throws Exception {
 		String body = "Foo";
@@ -238,6 +238,17 @@ public void handleReturnValueNotAcceptableProduces() throws Exception {
 		fail("Expected exception");
 	}
 
+	// SPR-9142
+
+	@Test(expected=HttpMediaTypeNotAcceptableException.class)
+	public void handleReturnValueNotAcceptableParseError() throws Exception {
+		ResponseEntity<String> returnValue = new ResponseEntity<String>("Body", HttpStatus.ACCEPTED);
+		servletRequest.addHeader("Accept", "01");
+
+		processor.handleReturnValue(returnValue, returnTypeResponseEntity, mavContainer, webRequest);
+		fail("Expected exception");
+	}
+
 	@Test
 	public void responseHeaderNoBody() throws Exception {
 		HttpHeaders headers = new HttpHeaders();
@@ -269,7 +280,7 @@ public void responseHeaderAndBody() throws Exception {
 		assertEquals("headerValue", outputMessage.getValue().getHeaders().get("header").get(0));
 		verify(messageConverter);
 	}
-	
+
 	public ResponseEntity<String> handle1(HttpEntity<String> httpEntity, ResponseEntity<String> responseEntity, int i) {
 		return responseEntity;
 	}