Protect against RFD exploits

Issue: SPR-13548

spring-web/src/main/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverter.java

@@ -95,6 +95,7 @@ public class MappingJackson2HttpMessageConverter extends AbstractJackson2HttpMes
 		String jsonpFunction =
 				(object instanceof MappingJacksonValue ? ((MappingJacksonValue) object).getJsonpFunction() : null);
 		if (jsonpFunction != null) {
+			generator.writeRaw("/**/");
 			generator.writeRaw(jsonpFunction + "(");
 		}
 	}

spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManager.java

@@ -120,6 +120,16 @@ public class ContentNegotiationManager implements ContentNegotiationStrategy,
 		return new ArrayList<String>(result);
 	}
 
+	/**
+	 * {@inheritDoc}
+	 * <p>At startup this method returns extensions explicitly registered with
+	 * either {@link PathExtensionContentNegotiationStrategy} or
+	 * {@link ParameterContentNegotiationStrategy}. At runtime if there is a
+	 * "path extension" strategy and its
+	 * {@link PathExtensionContentNegotiationStrategy#setUseJaf(boolean)
+	 * useJaf} property is set to "true", the list of extensions may
+	 * increase as file extensions are resolved via JAF and cached.
+	 */
 	@Override
 	public List<String> getAllFileExtensions() {
 		Set<String> result = new LinkedHashSet<String>();

spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java

@@ -178,6 +178,10 @@ public class ContentNegotiationManagerFactoryBean
 		this.useJaf = useJaf;
 	}
 
+	private boolean isUseJafTurnedOff() {
+		return (this.useJaf != null && !this.useJaf);
+	}
+
 	/**
 	 * Whether a request parameter ("format" by default) should be used to
 	 * determine the requested media type. For this option to work you must
@@ -240,7 +244,7 @@ public class ContentNegotiationManagerFactoryBean
 
 		if (this.favorPathExtension) {
 			PathExtensionContentNegotiationStrategy strategy;
-			if (this.servletContext != null) {
+			if (this.servletContext != null && !isUseJafTurnedOff()) {
 				strategy = new ServletPathExtensionContentNegotiationStrategy(
 						this.servletContext, this.mediaTypes);
 			}
@@ -272,7 +276,6 @@ public class ContentNegotiationManagerFactoryBean
 		this.contentNegotiationManager = new ContentNegotiationManager(strategies);
 	}
 
-
 	@Override
 	public ContentNegotiationManager getObject() {
 		return this.contentNegotiationManager;

spring-web/src/main/java/org/springframework/web/accept/PathExtensionContentNegotiationStrategy.java

@@ -66,7 +66,8 @@ public class PathExtensionContentNegotiationStrategy
 		PATH_HELPER.setUrlDecode(false);
 	}
 
-	private boolean useJaf = JAF_PRESENT;
+
+	private boolean useJaf = true;
 
 	private boolean ignoreUnknownExtensions = true;
 
@@ -89,8 +90,7 @@ public class PathExtensionContentNegotiationStrategy
 
 	/**
 	 * Whether to use the Java Activation Framework to look up file extensions.
-	 * <p>By default if this property is not set JAF is present on the
-	 * classpath it will be used.
+	 * <p>By default this is set to "true" but depends on JAF being present.
 	 */
 	public void setUseJaf(boolean useJaf) {
 		this.useJaf = useJaf;
@@ -123,7 +123,7 @@ public class PathExtensionContentNegotiationStrategy
 	protected MediaType handleNoMatch(NativeWebRequest webRequest, String extension)
 			throws HttpMediaTypeNotAcceptableException {
 
-		if (this.useJaf) {
+		if (this.useJaf && JAF_PRESENT) {
 			MediaType mediaType = JafMediaTypeFactory.getMediaType("file." + extension);
 			if (mediaType != null && !MediaType.APPLICATION_OCTET_STREAM.equals(mediaType)) {
 				return mediaType;

spring-web/src/main/java/org/springframework/web/util/UrlPathHelper.java

@@ -438,7 +438,7 @@ public class UrlPathHelper {
 	 * @see java.net.URLDecoder#decode(String)
 	 */
 	public String decodeRequestString(HttpServletRequest request, String source) {
-		if (this.urlDecode) {
+		if (this.urlDecode && source != null) {
 			return decodeInternal(request, source);
 		}
 		return source;

spring-web/src/main/java/org/springframework/web/util/WebUtils.java

@@ -723,20 +723,23 @@ public abstract class WebUtils {
 	}
 
 	/**
-	 * Extract the full URL filename (including file extension) from the given request URL path.
-	 * Correctly resolves nested paths such as "/products/view.html" as well.
+	 * Extract the full URL filename (including file extension) from the given
+	 * request URL path. Correctly resolve nested paths such as
+	 * "/products/view.html" and remove any path and or query parameters.
 	 * @param urlPath the request URL path (e.g. "/products/index.html")
 	 * @return the extracted URI filename (e.g. "index.html")
 	 */
 	public static String extractFullFilenameFromUrlPath(String urlPath) {
-		int end = urlPath.indexOf(';');
+		int end = urlPath.indexOf('?');
 		if (end == -1) {
-			end = urlPath.indexOf('?');
+			end = urlPath.indexOf('#');
 			if (end == -1) {
 				end = urlPath.length();
 			}
 		}
 		int begin = urlPath.lastIndexOf('/', end) + 1;
+		int paramIndex = urlPath.indexOf(';', begin);
+		end = (paramIndex != -1 && paramIndex < end ? paramIndex : end);
 		return urlPath.substring(begin, end);
 	}
 

spring-web/src/test/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverterTests.java

@@ -291,7 +291,7 @@ public class MappingJackson2HttpMessageConverterTests {
 		MockHttpOutputMessage outputMessage = new MockHttpOutputMessage();
 		this.converter.writeInternal(jacksonValue, null, outputMessage);
 
-		assertEquals("callback(\"foo\");", outputMessage.getBodyAsString(Charset.forName("UTF-8")));
+		assertEquals("/**/callback(\"foo\");", outputMessage.getBodyAsString(Charset.forName("UTF-8")));
 	}
 
 	@Test
@@ -308,7 +308,7 @@ public class MappingJackson2HttpMessageConverterTests {
 		this.converter.writeInternal(jacksonValue, null, outputMessage);
 
 		String result = outputMessage.getBodyAsString(Charset.forName("UTF-8"));
-		assertThat(result, startsWith("callback("));
+		assertThat(result, startsWith("/**/callback("));
 		assertThat(result, endsWith(");"));
 		assertThat(result, containsString("\"withView1\":\"with\""));
 		assertThat(result, not(containsString("\"withView2\":\"with\"")));

spring-web/src/test/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBeanTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2015 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.
@@ -15,7 +15,6 @@
  */
 package org.springframework.web.accept;
 
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
@@ -25,11 +24,13 @@ import org.junit.Test;
 
 import org.springframework.http.MediaType;
 import org.springframework.mock.web.test.MockHttpServletRequest;
+import org.springframework.mock.web.test.MockServletContext;
+import org.springframework.util.StringUtils;
 import org.springframework.web.HttpMediaTypeNotAcceptableException;
 import org.springframework.web.context.request.NativeWebRequest;
 import org.springframework.web.context.request.ServletWebRequest;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
 
 /**
  * Test fixture for {@link ContentNegotiationManagerFactoryBean} tests.
@@ -46,7 +47,10 @@ public class ContentNegotiationManagerFactoryBeanTests {
 
 	@Before
 	public void setup() {
-		this.servletRequest = new MockHttpServletRequest();
+		TestServletContext servletContext = new TestServletContext();
+		servletContext.getMimeTypes().put("foo", "application/foo");
+
+		this.servletRequest = new MockHttpServletRequest(servletContext);
 		this.webRequest = new ServletWebRequest(this.servletRequest);
 
 		this.factoryBean = new ContentNegotiationManagerFactoryBean();
@@ -62,7 +66,7 @@ public class ContentNegotiationManagerFactoryBeanTests {
 		this.servletRequest.setRequestURI("/flower.gif");
 
 		assertEquals("Should be able to resolve file extensions by default",
-				Arrays.asList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest));
+				Collections.singletonList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest));
 
 		this.servletRequest.setRequestURI("/flower.xyz");
 
@@ -79,26 +83,46 @@ public class ContentNegotiationManagerFactoryBeanTests {
 		this.servletRequest.addHeader("Accept", MediaType.IMAGE_GIF_VALUE);
 
 		assertEquals("Should resolve Accept header by default",
-				Arrays.asList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest));
+				Collections.singletonList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest));
 	}
 
 	@Test
-	public void addMediaTypes() throws Exception {
-		Map<String, MediaType> mediaTypes = new HashMap<>();
-		mediaTypes.put("json", MediaType.APPLICATION_JSON);
-		this.factoryBean.addMediaTypes(mediaTypes);
+	public void favorPath() throws Exception {
+		this.factoryBean.setFavorPathExtension(true);
+		this.factoryBean.addMediaTypes(Collections.singletonMap("bar", new MediaType("application", "bar")));
+		this.factoryBean.afterPropertiesSet();
+		ContentNegotiationManager manager = this.factoryBean.getObject();
+
+		this.servletRequest.setRequestURI("/flower.foo");
+		assertEquals(Collections.singletonList(new MediaType("application", "foo")),
+				manager.resolveMediaTypes(this.webRequest));
+
+		this.servletRequest.setRequestURI("/flower.bar");
+		assertEquals(Collections.singletonList(new MediaType("application", "bar")),
+				manager.resolveMediaTypes(this.webRequest));
+
+		this.servletRequest.setRequestURI("/flower.gif");
+		assertEquals(Collections.singletonList(MediaType.IMAGE_GIF), manager.resolveMediaTypes(this.webRequest));
+	}
 
+	@Test
+	public void favorPathWithJafTurnedOff() throws Exception {
+		this.factoryBean.setFavorPathExtension(true);
+		this.factoryBean.setUseJaf(false);
 		this.factoryBean.afterPropertiesSet();
 		ContentNegotiationManager manager = this.factoryBean.getObject();
 
-		this.servletRequest.setRequestURI("/flower.json");
-		assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest));
+		this.servletRequest.setRequestURI("/flower.foo");
+		assertEquals(Collections.emptyList(), manager.resolveMediaTypes(this.webRequest));
+
+		this.servletRequest.setRequestURI("/flower.gif");
+		assertEquals(Collections.emptyList(), manager.resolveMediaTypes(this.webRequest));
 	}
 
 	// SPR-10170
 
 	@Test(expected = HttpMediaTypeNotAcceptableException.class)
-	public void favorPathExtensionWithUnknownMediaType() throws Exception {
+	public void favorPathWithIgnoreUnknownPathExtensionTurnedOff() throws Exception {
 		this.factoryBean.setFavorPathExtension(true);
 		this.factoryBean.setIgnoreUnknownPathExtensions(false);
 		this.factoryBean.afterPropertiesSet();
@@ -124,7 +148,8 @@ public class ContentNegotiationManagerFactoryBeanTests {
 		this.servletRequest.setRequestURI("/flower");
 		this.servletRequest.addParameter("format", "json");
 
-		assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest));
+		assertEquals(Collections.singletonList(MediaType.APPLICATION_JSON),
+				manager.resolveMediaTypes(this.webRequest));
 	}
 
 	// SPR-10170
@@ -159,26 +184,48 @@ public class ContentNegotiationManagerFactoryBeanTests {
 		this.factoryBean.afterPropertiesSet();
 		ContentNegotiationManager manager = this.factoryBean.getObject();
 
-		assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest));
+		assertEquals(Collections.singletonList(MediaType.APPLICATION_JSON),
+				manager.resolveMediaTypes(this.webRequest));
 
 		// SPR-10513
 
 		this.servletRequest.addHeader("Accept", MediaType.ALL_VALUE);
 
-		assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest));
+		assertEquals(Collections.singletonList(MediaType.APPLICATION_JSON),
+				manager.resolveMediaTypes(this.webRequest));
 	}
 
 	// SPR-12286
+
 	@Test
 	public void setDefaultContentTypeWithStrategy() throws Exception {
 		this.factoryBean.setDefaultContentTypeStrategy(new FixedContentNegotiationStrategy(MediaType.APPLICATION_JSON));
 		this.factoryBean.afterPropertiesSet();
 		ContentNegotiationManager manager = this.factoryBean.getObject();
 
-		assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest));
+		assertEquals(Collections.singletonList(MediaType.APPLICATION_JSON),
+				manager.resolveMediaTypes(this.webRequest));
 
 		this.servletRequest.addHeader("Accept", MediaType.ALL_VALUE);
-		assertEquals(Arrays.asList(MediaType.APPLICATION_JSON), manager.resolveMediaTypes(this.webRequest));
+		assertEquals(Collections.singletonList(MediaType.APPLICATION_JSON),
+				manager.resolveMediaTypes(this.webRequest));
+	}
+
+
+	private static class TestServletContext extends MockServletContext {
+
+		private final Map<String, String> mimeTypes = new HashMap<>();
+
+
+		public Map<String, String> getMimeTypes() {
+			return this.mimeTypes;
+		}
+
+		@Override
+		public String getMimeType(String filePath) {
+			String extension = StringUtils.getFilenameExtension(filePath);
+			return getMimeTypes().get(extension);
+		}
 	}
 
 }

spring-web/src/test/java/org/springframework/web/util/WebUtilsTests.java

@@ -70,9 +70,17 @@ public class WebUtilsTests {
 		assertEquals("index.html", WebUtils.extractFullFilenameFromUrlPath("index.html"));
 		assertEquals("index.html", WebUtils.extractFullFilenameFromUrlPath("/index.html"));
 		assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html"));
+		assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html#/a"));
+		assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html#/path/a"));
+		assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html#/path/a.do"));
 		assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=a"));
 		assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a"));
 		assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a.do"));
+		assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a#/path/a"));
+		assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products/view.html?param=/path/a.do#/path/a.do"));
+		assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products;q=11/view.html?param=/path/a.do"));
+		assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products;q=11/view.html;r=22?param=/path/a.do"));
+		assertEquals("view.html", WebUtils.extractFullFilenameFromUrlPath("/products;q=11/view.html;r=22;s=33?param=/path/a.do"));
 	}
 
 	@Test

spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractJsonpResponseBodyAdvice.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2015 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.
@@ -16,8 +16,12 @@
 
 package org.springframework.web.servlet.mvc.method.annotation;
 
+import java.util.regex.Pattern;
 import javax.servlet.http.HttpServletRequest;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+
 import org.springframework.core.MethodParameter;
 import org.springframework.http.MediaType;
 import org.springframework.http.converter.json.MappingJacksonValue;
@@ -44,6 +48,14 @@ import org.springframework.util.ObjectUtils;
  */
 public abstract class AbstractJsonpResponseBodyAdvice extends AbstractMappingJacksonResponseBodyAdvice {
 
+	/**
+	 * Pattern for validating jsonp callback parameter values.
+	 */
+	private static final Pattern CALLBACK_PARAM_PATTERN = Pattern.compile("[0-9A-Za-z_\\.]*");
+
+
+	private final Log logger = LogFactory.getLog(getClass());
+
 	private final String[] jsonpQueryParamNames;
 
 
@@ -62,14 +74,31 @@ public abstract class AbstractJsonpResponseBodyAdvice extends AbstractMappingJac
 		for (String name : this.jsonpQueryParamNames) {
 			String value = servletRequest.getParameter(name);
 			if (value != null) {
+				if (!isValidJsonpQueryParam(value)) {
+					if (logger.isDebugEnabled()) {
+						logger.debug("Ignoring invalid jsonp parameter value: " + value);
+					}
+					continue;
+				}
 				MediaType contentTypeToUse = getContentType(contentType, request, response);
 				response.getHeaders().setContentType(contentTypeToUse);
 				bodyContainer.setJsonpFunction(value);
-				return;
+				break;
 			}
 		}
 	}
 
+	/**
+	 * Validate the jsonp query parameter value. The default implementation
+	 * returns true if it consists of digits, letters, or "_" and ".".
+	 * Invalid parameter values are ignored.
+	 * @param value the query param value, never {@code null}
+	 * @since 4.1.8
+	 */
+	protected boolean isValidJsonpQueryParam(String value) {
+		return CALLBACK_PARAM_PATTERN.matcher(value).matches();
+	}
+
 	/**
 	 * Return the content type to set the response to.
 	 * This implementation always returns "application/javascript".

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

@@ -19,15 +19,19 @@ package org.springframework.web.servlet.mvc.method.annotation;
 import java.io.IOException;
 import java.lang.reflect.Type;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Locale;
 import java.util.Set;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
 import org.springframework.core.MethodParameter;
 import org.springframework.http.HttpEntity;
+import org.springframework.http.HttpHeaders;
 import org.springframework.http.HttpOutputMessage;
 import org.springframework.http.MediaType;
 import org.springframework.http.converter.GenericHttpMessageConverter;
@@ -36,12 +40,14 @@ import org.springframework.http.converter.HttpMessageNotWritableException;
 import org.springframework.http.server.ServletServerHttpRequest;
 import org.springframework.http.server.ServletServerHttpResponse;
 import org.springframework.util.CollectionUtils;
+import org.springframework.util.StringUtils;
 import org.springframework.web.HttpMediaTypeNotAcceptableException;
 import org.springframework.web.accept.ContentNegotiationManager;
 import org.springframework.web.context.request.NativeWebRequest;
 import org.springframework.web.context.request.ServletWebRequest;
 import org.springframework.web.method.support.HandlerMethodReturnValueHandler;
 import org.springframework.web.servlet.HandlerMapping;
+import org.springframework.web.util.UrlPathHelper;
 
 /**
  * Extends {@link AbstractMessageConverterMethodArgumentResolver} with the ability to handle
@@ -56,24 +62,52 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe
 
 	private static final MediaType MEDIA_TYPE_APPLICATION = new MediaType("application");
 
+	private static final UrlPathHelper RAW_URL_PATH_HELPER = new UrlPathHelper();
+
+	private static final UrlPathHelper DECODING_URL_PATH_HELPER = new UrlPathHelper();
+
+	static {
+		RAW_URL_PATH_HELPER.setRemoveSemicolonContent(false);
+		RAW_URL_PATH_HELPER.setUrlDecode(false);
+	}
+
+	/* Extensions associated with the built-in message converters */
+	private static final Set<String> WHITELISTED_EXTENSIONS = new HashSet<String>(Arrays.asList(
+			"txt", "text", "json", "xml", "atom", "rss", "png", "jpe", "jpeg", "jpg", "gif", "wbmp", "bmp"));
+
+
 	private final ContentNegotiationManager contentNegotiationManager;
 
+	private final Set<String> safeExtensions = new HashSet<String>();
 
+
+	/**
+	 * Constructor with list of converters only.
+	 */
 	protected AbstractMessageConverterMethodProcessor(List<HttpMessageConverter<?>> converters) {
 		this(converters, null);
 	}
 
+	/**
+	 * Constructor with list of converters and ContentNegotiationManager.
+	 */
 	protected AbstractMessageConverterMethodProcessor(List<HttpMessageConverter<?>> converters,
 			ContentNegotiationManager contentNegotiationManager) {
 
 		this(converters, contentNegotiationManager, null);
 	}
 
+	/**
+	 * Constructor with list of converters and ContentNegotiationManager as well
+	 * as request/response body advice instances.
+	 */
 	protected AbstractMessageConverterMethodProcessor(List<HttpMessageConverter<?>> converters,
 			ContentNegotiationManager manager, List<Object> requestResponseBodyAdvice) {
 
 		super(converters, requestResponseBodyAdvice);
 		this.contentNegotiationManager = (manager != null ? manager : new ContentNegotiationManager());
+		this.safeExtensions.addAll(this.contentNegotiationManager.getAllFileExtensions());
+		this.safeExtensions.addAll(WHITELISTED_EXTENSIONS);
 	}
 
 
@@ -164,6 +198,7 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe
 								(Class<? extends HttpMessageConverter<?>>) messageConverter.getClass(),
 								inputMessage, outputMessage);
 						if (returnValue != null) {
+							addContentDispositionHeader(inputMessage, outputMessage);
 							((GenericHttpMessageConverter<T>) messageConverter).write(returnValue,
 									returnValueType, selectedMediaType, outputMessage);
 							if (logger.isDebugEnabled()) {
@@ -179,6 +214,7 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe
 							(Class<? extends HttpMessageConverter<?>>) messageConverter.getClass(),
 							inputMessage, outputMessage);
 					if (returnValue != null) {
+						addContentDispositionHeader(inputMessage, outputMessage);
 						((HttpMessageConverter<T>) messageConverter).write(returnValue,
 								selectedMediaType, outputMessage);
 						if (logger.isDebugEnabled()) {
@@ -225,7 +261,7 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe
 	/**
 	 * @see #getProducibleMediaTypes(HttpServletRequest, Class, Type)
 	 */
-	@SuppressWarnings("unchecked")
+	@SuppressWarnings({"unchecked", "unused"})
 	protected List<MediaType> getProducibleMediaTypes(HttpServletRequest request, Class<?> returnValueClass) {
 		return getProducibleMediaTypes(request, returnValueClass, null);
 	}
@@ -278,4 +314,48 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe
 		return (MediaType.SPECIFICITY_COMPARATOR.compare(acceptType, produceTypeToUse) <= 0 ? acceptType : produceTypeToUse);
 	}
 
+	/**
+	 * Check if the path has a file extension and whether the extension is either
+	 * {@link #WHITELISTED_EXTENSIONS whitelisted} or
+	 * {@link ContentNegotiationManager#getAllFileExtensions() explicitly
+	 * registered}. If not add a 'Content-Disposition' header with a safe
+	 * attachment file name ("f.txt") to prevent RFD exploits.
+	 */
+	private void addContentDispositionHeader(ServletServerHttpRequest request,
+			ServletServerHttpResponse response) {
+
+		HttpHeaders headers = response.getHeaders();
+		if (headers.containsKey(HttpHeaders.CONTENT_DISPOSITION)) {
+			return;
+		}
+
+		HttpServletRequest servletRequest = request.getServletRequest();
+		String requestUri = RAW_URL_PATH_HELPER.getOriginatingRequestUri(servletRequest);
+
+		int index = requestUri.lastIndexOf('/') + 1;
+		String filename = requestUri.substring(index);
+		String pathParams = "";
+
+		index = filename.indexOf(';');
+		if (index != -1) {
+			pathParams = filename.substring(index);
+			filename = filename.substring(0, index);
+		}
+
+		filename = DECODING_URL_PATH_HELPER.decodeRequestString(servletRequest, filename);
+		String ext = StringUtils.getFilenameExtension(filename);
+
+		pathParams = DECODING_URL_PATH_HELPER.decodeRequestString(servletRequest, pathParams);
+		String extInPathParams = StringUtils.getFilenameExtension(pathParams);
+
+		if (!isSafeExtension(ext) || !isSafeExtension(extInPathParams)) {
+			headers.add(HttpHeaders.CONTENT_DISPOSITION, "attachment;filename=f.txt");
+		}
+	}
+
+	private boolean isSafeExtension(String extension) {
+		return (!StringUtils.hasText(extension) ||
+				this.safeExtensions.contains(extension.toLowerCase(Locale.ENGLISH)));
+	}
+
 }

spring-webmvc/src/main/java/org/springframework/web/servlet/view/json/MappingJackson2JsonView.java

@@ -23,6 +23,7 @@ import java.util.HashMap;
 import java.util.LinkedHashSet;
 import java.util.Map;
 import java.util.Set;
+import java.util.regex.Pattern;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
@@ -70,6 +71,12 @@ public class MappingJackson2JsonView extends AbstractJackson2View {
 	 */
 	public static final String DEFAULT_JSONP_CONTENT_TYPE = "application/javascript";
 
+	/**
+	 * Pattern for validating jsonp callback parameter values.
+	 */
+	private static final Pattern CALLBACK_PARAM_PATTERN = Pattern.compile("[0-9A-Za-z_\\.]*");
+
+
 	private String jsonPrefix;
 
 	private Set<String> modelKeys;
@@ -170,14 +177,32 @@ public class MappingJackson2JsonView extends AbstractJackson2View {
 		if (this.jsonpParameterNames != null) {
 			for (String name : this.jsonpParameterNames) {
 				String value = request.getParameter(name);
-				if (!StringUtils.isEmpty(value)) {
-					return value;
+				if (StringUtils.isEmpty(value)) {
+					continue;
+				}
+				if (!isValidJsonpQueryParam(value)) {
+					if (logger.isDebugEnabled()) {
+						logger.debug("Ignoring invalid jsonp parameter value: " + value);
+					}
+					continue;
 				}
+				return value;
 			}
 		}
 		return null;
 	}
 
+	/**
+	 * Validate the jsonp query parameter value. The default implementation
+	 * returns true if it consists of digits, letters, or "_" and ".".
+	 * Invalid parameter values are ignored.
+	 * @param value the query param value, never {@code null}
+	 * @since 4.1.8
+	 */
+	protected boolean isValidJsonpQueryParam(String value) {
+		return CALLBACK_PARAM_PATTERN.matcher(value).matches();
+	}
+
 	/**
 	 * Filter out undesired attributes from the given model.
 	 * The return value can be either another {@link Map} or a single value object.
@@ -228,6 +253,7 @@ public class MappingJackson2JsonView extends AbstractJackson2View {
 			jsonpFunction = ((MappingJacksonValue) object).getJsonpFunction();
 		}
 		if (jsonpFunction != null) {
+			generator.writeRaw("/**/");
 			generator.writeRaw(jsonpFunction + "(" );
 		}
 	}

spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapterTests.java

@@ -19,6 +19,7 @@ package org.springframework.web.servlet.mvc.method.annotation;
 import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -53,7 +54,8 @@ import org.springframework.web.servlet.DispatcherServlet;
 import org.springframework.web.servlet.FlashMap;
 import org.springframework.web.servlet.ModelAndView;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 /**
  * Unit tests for {@link RequestMappingHandlerAdapter}.
@@ -128,7 +130,7 @@ public class RequestMappingHandlerAdapterTests {
 		HandlerMethodReturnValueHandler viewHandler = new ViewNameMethodReturnValueHandler();
 
 		this.handlerAdapter.setArgumentResolvers(Arrays.asList(redirectAttributesResolver, modelResolver));
-		this.handlerAdapter.setReturnValueHandlers(Arrays.asList(viewHandler));
+		this.handlerAdapter.setReturnValueHandlers(Collections.singletonList(viewHandler));
 		this.handlerAdapter.setIgnoreDefaultModelOnRedirect(true);
 		this.handlerAdapter.afterPropertiesSet();
 
@@ -143,7 +145,7 @@ public class RequestMappingHandlerAdapterTests {
 	@Test
 	public void setCustomArgumentResolvers() throws Exception {
 		HandlerMethodArgumentResolver resolver = new ServletRequestMethodArgumentResolver();
-		this.handlerAdapter.setCustomArgumentResolvers(Arrays.asList(resolver));
+		this.handlerAdapter.setCustomArgumentResolvers(Collections.singletonList(resolver));
 		this.handlerAdapter.afterPropertiesSet();
 
 		assertTrue(this.handlerAdapter.getArgumentResolvers().contains(resolver));
@@ -153,7 +155,7 @@ public class RequestMappingHandlerAdapterTests {
 	@Test
 	public void setArgumentResolvers() throws Exception {
 		HandlerMethodArgumentResolver resolver = new ServletRequestMethodArgumentResolver();
-		this.handlerAdapter.setArgumentResolvers(Arrays.asList(resolver));
+		this.handlerAdapter.setArgumentResolvers(Collections.singletonList(resolver));
 		this.handlerAdapter.afterPropertiesSet();
 
 		assertMethodProcessorCount(1, INIT_BINDER_RESOLVER_COUNT, HANDLER_COUNT);
@@ -162,7 +164,7 @@ public class RequestMappingHandlerAdapterTests {
 	@Test
 	public void setInitBinderArgumentResolvers() throws Exception {
 		HandlerMethodArgumentResolver resolver = new ServletRequestMethodArgumentResolver();
-		this.handlerAdapter.setInitBinderArgumentResolvers(Arrays.<HandlerMethodArgumentResolver>asList(resolver));
+		this.handlerAdapter.setInitBinderArgumentResolvers(Collections.singletonList(resolver));
 		this.handlerAdapter.afterPropertiesSet();
 
 		assertMethodProcessorCount(RESOLVER_COUNT, 1, HANDLER_COUNT);
@@ -171,7 +173,7 @@ public class RequestMappingHandlerAdapterTests {
 	@Test
 	public void setCustomReturnValueHandlers() {
 		HandlerMethodReturnValueHandler handler = new ViewNameMethodReturnValueHandler();
-		this.handlerAdapter.setCustomReturnValueHandlers(Arrays.asList(handler));
+		this.handlerAdapter.setCustomReturnValueHandlers(Collections.singletonList(handler));
 		this.handlerAdapter.afterPropertiesSet();
 
 		assertTrue(this.handlerAdapter.getReturnValueHandlers().contains(handler));
@@ -181,7 +183,7 @@ public class RequestMappingHandlerAdapterTests {
 	@Test
 	public void setReturnValueHandlers() {
 		HandlerMethodReturnValueHandler handler = new ModelMethodProcessor();
-		this.handlerAdapter.setReturnValueHandlers(Arrays.asList(handler));
+		this.handlerAdapter.setReturnValueHandlers(Collections.singletonList(handler));
 		this.handlerAdapter.afterPropertiesSet();
 
 		assertMethodProcessorCount(RESOLVER_COUNT, INIT_BINDER_RESOLVER_COUNT, 1);
@@ -240,20 +242,37 @@ public class RequestMappingHandlerAdapterTests {
 		this.handlerAdapter.setMessageConverters(converters);
 
 		this.webAppContext.registerSingleton("rba", ResponseCodeSuppressingAdvice.class);
-		this.webAppContext.registerSingleton("ja", JsonpAdvice.class);
 		this.webAppContext.refresh();
 
 		this.request.addHeader("Accept", MediaType.APPLICATION_JSON_VALUE);
 		this.request.setParameter("c", "callback");
 
-		HandlerMethod handlerMethod = handlerMethod(new SimpleController(), "handleWithResponseEntity");
+		HandlerMethod handlerMethod = handlerMethod(new SimpleController(), "handleBadRequest");
 		this.handlerAdapter.afterPropertiesSet();
 		this.handlerAdapter.handle(this.request, this.response, handlerMethod);
 
 		assertEquals(200, this.response.getStatus());
-		assertEquals("callback({\"status\":400,\"message\":\"body\"});", this.response.getContentAsString());
+		assertEquals("{\"status\":400,\"message\":\"body\"}", this.response.getContentAsString());
 	}
 
+	@Test
+	public void jsonpResponseBodyAdvice() throws Exception {
+
+		List<HttpMessageConverter<?>> converters = new ArrayList<>();
+		converters.add(new MappingJackson2HttpMessageConverter());
+		this.handlerAdapter.setMessageConverters(converters);
+
+		this.webAppContext.registerSingleton("jsonpAdvice", JsonpAdvice.class);
+		this.webAppContext.refresh();
+
+		testJsonp("callback", true);
+		testJsonp("_callback", true);
+		testJsonp("_Call.bAcK", true);
+		testJsonp("0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_.", true);
+
+		testJsonp("<script>", false);
+		testJsonp("!foo!bar", false);
+	}
 
 	private HandlerMethod handlerMethod(Object handler, String methodName, Class<?>... paramTypes) throws Exception {
 		Method method = handler.getClass().getDeclaredMethod(methodName, paramTypes);
@@ -266,6 +285,26 @@ public class RequestMappingHandlerAdapterTests {
 		assertEquals(handlerCount, this.handlerAdapter.getReturnValueHandlers().size());
 	}
 
+	private void testJsonp(String value, boolean validValue) throws Exception {
+
+		this.request = new MockHttpServletRequest("GET", "/");
+		this.request.addHeader("Accept", MediaType.APPLICATION_JSON_VALUE);
+		this.request.setParameter("c", value);
+		this.response = new MockHttpServletResponse();
+
+		HandlerMethod handlerMethod = handlerMethod(new SimpleController(), "handleWithResponseEntity");
+		this.handlerAdapter.afterPropertiesSet();
+		this.handlerAdapter.handle(this.request, this.response, handlerMethod);
+
+		assertEquals(200, this.response.getStatus());
+		if (validValue) {
+			assertEquals("/**/" + value + "({\"foo\":\"bar\"});", this.response.getContentAsString());
+		}
+		else {
+			assertEquals("{\"foo\":\"bar\"}", this.response.getContentAsString());
+		}
+	}
+
 
 	@SuppressWarnings("unused")
 	private static class SimpleController {
@@ -279,9 +318,15 @@ public class RequestMappingHandlerAdapterTests {
 			return null;
 		}
 
-		public ResponseEntity<String> handleWithResponseEntity() {
+		public ResponseEntity<Map<String, String>> handleWithResponseEntity() {
+			return new ResponseEntity<Map<String, String>>(Collections.singletonMap(
+					"foo", "bar"), HttpStatus.OK);
+		}
+
+		public ResponseEntity<String> handleBadRequest() {
 			return new ResponseEntity<String>("body", HttpStatus.BAD_REQUEST);
 		}
+
 	}
 
 
@@ -355,12 +400,12 @@ public class RequestMappingHandlerAdapterTests {
 		}
 	}
 
-@ControllerAdvice
-private static class JsonpAdvice extends AbstractJsonpResponseBodyAdvice {
+	@ControllerAdvice
+	private static class JsonpAdvice extends AbstractJsonpResponseBodyAdvice {
 
-	public JsonpAdvice() {
-		super("c");
+		public JsonpAdvice() {
+			super("c");
+		}
 	}
-}
 
 }

spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestResponseBodyMethodProcessorTests.java

@@ -16,8 +16,6 @@
 
 package org.springframework.web.servlet.mvc.method.annotation;
 
-import static org.junit.Assert.*;
-
 import java.io.ByteArrayOutputStream;
 import java.io.OutputStream;
 import java.io.Serializable;
@@ -56,6 +54,7 @@ import org.springframework.mock.web.test.MockHttpServletRequest;
 import org.springframework.mock.web.test.MockHttpServletResponse;
 import org.springframework.util.MultiValueMap;
 import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean;
+import org.springframework.web.accept.ContentNegotiationManagerFactoryBean;
 import org.springframework.web.bind.WebDataBinder;
 import org.springframework.web.bind.annotation.RequestBody;
 import org.springframework.web.bind.annotation.RequestMapping;
@@ -68,6 +67,13 @@ import org.springframework.web.method.HandlerMethod;
 import org.springframework.web.method.support.ModelAndViewContainer;
 import org.springframework.web.servlet.ModelAndView;
 import org.springframework.web.servlet.view.json.MappingJackson2JsonView;
+import org.springframework.web.util.WebUtils;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 
 /**
  * Test fixture for a {@link RequestResponseBodyMethodProcessor} with
@@ -326,6 +332,37 @@ public class RequestResponseBodyMethodProcessorTests {
 		processor.writeWithMessageConverters(new ByteArrayOutputStream(), returnType, this.webRequest);
 	}
 
+	@Test
+	public void addContentDispositionHeader() throws Exception {
+
+		ContentNegotiationManagerFactoryBean factory = new ContentNegotiationManagerFactoryBean();
+		factory.addMediaType("pdf", new MediaType("application", "pdf"));
+		factory.afterPropertiesSet();
+
+		RequestResponseBodyMethodProcessor processor = new RequestResponseBodyMethodProcessor(
+				Collections.singletonList(new StringHttpMessageConverter()),
+				factory.getObject());
+
+		assertContentDisposition(processor, false, "/hello.json", "whitelisted extension");
+		assertContentDisposition(processor, false, "/hello.pdf", "registered extension");
+		assertContentDisposition(processor, true, "/hello.dataless", "uknown extension");
+
+		// path parameters
+		assertContentDisposition(processor, false, "/hello.json;a=b", "path param shouldn't cause issue");
+		assertContentDisposition(processor, true, "/hello.json;a=b;setup.dataless", "uknown ext in path params");
+		assertContentDisposition(processor, true, "/hello.dataless;a=b;setup.json", "uknown ext in filename");
+		assertContentDisposition(processor, false, "/hello.json;a=b;setup.json", "whitelisted extensions");
+
+		// encoded dot
+		assertContentDisposition(processor, true, "/hello%2Edataless;a=b;setup.json", "encoded dot in filename");
+		assertContentDisposition(processor, true, "/hello.json;a=b;setup%2Edataless", "encoded dot in path params");
+		assertContentDisposition(processor, true, "/hello.dataless%3Bsetup.bat", "encoded dot in path params");
+
+		this.servletRequest.setAttribute(WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE, "/hello.bat");
+		assertContentDisposition(processor, true, "/bonjour", "forwarded URL");
+		this.servletRequest.removeAttribute(WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE);
+	}
+
 	@Test
 	public void supportsReturnTypeResponseBodyOnType() throws Exception {
 		Method method = ResponseBodyController.class.getMethod("handle");
@@ -598,6 +635,27 @@ public class RequestResponseBodyMethodProcessorTests {
 		assertTrue(content.contains("\"name\":\"bar\""));
 	}
 
+	private void assertContentDisposition(RequestResponseBodyMethodProcessor processor,
+			boolean expectContentDisposition, String requestURI, String comment) throws Exception {
+
+		this.servletRequest.setRequestURI(requestURI);
+		processor.handleReturnValue("body", this.returnTypeString, this.mavContainer, this.webRequest);
+
+		String header = servletResponse.getHeader("Content-Disposition");
+		if (expectContentDisposition) {
+			assertEquals("Expected 'Content-Disposition' header. Use case: '" + comment + "'",
+					"attachment;filename=f.txt", header);
+		}
+		else {
+			assertNull("Did not expect 'Content-Disposition' header. Use case: '" + comment + "'", header);
+		}
+
+		this.servletRequest = new MockHttpServletRequest();
+		this.servletResponse = new MockHttpServletResponse();
+		this.webRequest = new ServletWebRequest(servletRequest, servletResponse);
+	}
+
+
 
 	String handle(
 			@RequestBody List<SimpleBean> list,

spring-webmvc/src/test/java/org/springframework/web/servlet/view/json/MappingJackson2JsonViewTests.java

@@ -324,43 +324,14 @@ public class MappingJackson2JsonViewTests {
 	}
 
 	@Test
-	public void renderWithJsonpDefaultParameterName() throws Exception {
-		Map<String, Object> model = new HashMap<String, Object>();
-		model.put("foo", "bar");
-		request.addParameter("otherparam", "value");
-		request.addParameter("jsonp", "jsonpCallback");
-
-		view.render(model, request, response);
-
-		String content = response.getContentAsString();
-		assertEquals("jsonpCallback({\"foo\":\"bar\"});", content);
-	}
-
-	@Test
-	public void renderWithCallbackDefaultParameterName() throws Exception {
-		Map<String, Object> model = new HashMap<String, Object>();
-		model.put("foo", "bar");
-		request.addParameter("otherparam", "value");
-		request.addParameter("callback", "jsonpCallback");
-
-		view.render(model, request, response);
-
-		String content = response.getContentAsString();
-		assertEquals("jsonpCallback({\"foo\":\"bar\"});", content);
-	}
-
-	@Test
-	 public void renderWithCustomJsonpParameterName() throws Exception {
-		Map<String, Object> model = new HashMap<String, Object>();
-		model.put("foo", "bar");
-		request.addParameter("otherparam", "value");
-		request.addParameter("custom", "jsonpCallback");
-		view.setJsonpParameterNames(new LinkedHashSet(Arrays.asList("jsonp", "callback", "custom")));
-
-		view.render(model, request, response);
-
-		String content = response.getContentAsString();
-		assertEquals("jsonpCallback({\"foo\":\"bar\"});", content);
+	public void renderWithJsonp() throws Exception {
+		testJsonp("jsonp", "callback", true);
+		testJsonp("jsonp", "_callback", true);
+		testJsonp("jsonp", "_Call.bAcK", true);
+		testJsonp("jsonp", "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_.", true);
+
+		testJsonp("jsonp", "<script>", false);
+		testJsonp("jsonp", "!foo!bar", false);
 	}
 
 	private void validateResult() throws Exception {
@@ -376,6 +347,26 @@ public class MappingJackson2JsonViewTests {
 		assertEquals("application/json", response.getContentType());
 	}
 
+	private void testJsonp(String paramName, String paramValue, boolean validValue) throws Exception {
+		Map<String, Object> model = new HashMap<String, Object>();
+		model.put("foo", "bar");
+
+		this.request = new MockHttpServletRequest();
+		this.request.addParameter("otherparam", "value");
+		this.request.addParameter(paramName, paramValue);
+		this.response = new MockHttpServletResponse();
+
+		this.view.render(model, this.request, this.response);
+
+		String content = this.response.getContentAsString();
+		if (validValue) {
+			assertEquals("/**/" + paramValue + "({\"foo\":\"bar\"});", content);
+		}
+		else {
+			assertEquals("{\"foo\":\"bar\"}", content);
+		}
+	}
+
 
 	public interface MyJacksonView1 {
 	}

spring-websocket/src/main/java/org/springframework/web/socket/sockjs/support/AbstractSockJsService.java

@@ -417,7 +417,7 @@ public abstract class AbstractSockJsService implements SockJsService, CorsConfig
 					response.setStatusCode(HttpStatus.NOT_FOUND);
 					return;
 				}
-				else if (!validateRequest(serverId, sessionId, transport)) {
+				else if (!validateRequest(serverId, sessionId, transport) || !validatePath(request)) {
 					if (requestInfo != null) {
 						logger.debug("Ignoring transport request: " + requestInfo);
 					}
@@ -452,6 +452,21 @@ public abstract class AbstractSockJsService implements SockJsService, CorsConfig
 		return true;
 	}
 
+	/**
+	 * Ensure the path does not contain a file extension, either in the filename
+	 * (e.g. "/jsonp.bat") or possibly after path parameters ("/jsonp;Setup.bat")
+	 * which could be used for RFD exploits.
+	 * <p>Since the last part of the path is expected to be a transport type, the
+	 * presence of an extension would not work. All we need to do is check if
+	 * there are any path parameters, which would have been removed from the
+	 * SockJS path during request mapping, and if found reject the request.
+	 */
+	private boolean validatePath(ServerHttpRequest request) {
+		String path = request.getURI().getPath();
+		int index = path.lastIndexOf('/') + 1;
+		String filename = path.substring(index);
+		return filename.indexOf(';') == -1;
+	}
 
 	/**
 	 * Handle request for raw WebSocket communication, i.e. without any SockJS message framing.

spring-websocket/src/main/java/org/springframework/web/socket/sockjs/transport/handler/AbstractHttpSendingTransportHandler.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2015 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,10 +18,12 @@ package org.springframework.web.socket.sockjs.transport.handler;
 
 import java.io.IOException;
 import java.io.UnsupportedEncodingException;
+import java.util.regex.Pattern;
 
 import org.springframework.http.MediaType;
 import org.springframework.http.server.ServerHttpRequest;
 import org.springframework.http.server.ServerHttpResponse;
+import org.springframework.util.Assert;
 import org.springframework.util.MultiValueMap;
 import org.springframework.util.StringUtils;
 import org.springframework.web.socket.WebSocketHandler;
@@ -43,6 +45,12 @@ import org.springframework.web.util.UriUtils;
 public abstract class AbstractHttpSendingTransportHandler extends AbstractTransportHandler
 		implements SockJsSessionFactory {
 
+	/**
+	 * Pattern for validating jsonp callback parameter values.
+	 */
+	private static final Pattern CALLBACK_PARAM_PATTERN = Pattern.compile("[0-9A-Za-z_\\.]*");
+
+
 	@Override
 	public final void handleRequest(ServerHttpRequest request, ServerHttpResponse response,
 			WebSocketHandler wsHandler, SockJsSession wsSession) throws SockJsException {
@@ -109,8 +117,12 @@ public abstract class AbstractHttpSendingTransportHandler extends AbstractTransp
 		String query = request.getURI().getQuery();
 		MultiValueMap<String, String> params = UriComponentsBuilder.newInstance().query(query).build().getQueryParams();
 		String value = params.getFirst("c");
+		if (StringUtils.isEmpty(value)) {
+			return null;
+		}
 		try {
-			return (!StringUtils.isEmpty(value) ? UriUtils.decode(value, "UTF-8") : null);
+			String result = UriUtils.decode(value, "UTF-8");
+			return (CALLBACK_PARAM_PATTERN.matcher(result).matches() ? result : null);
 		}
 		catch (UnsupportedEncodingException ex) {
 			// should never happen

spring-websocket/src/main/java/org/springframework/web/socket/sockjs/transport/handler/JsonpPollingTransportHandler.java

@@ -84,7 +84,7 @@ public class JsonpPollingTransportHandler extends AbstractHttpSendingTransportHa
 		// We already validated the parameter above...
 		String callback = getCallbackParam(request);
 
-		return new DefaultSockJsFrameFormat(callback + "(\"%s\");\r\n") {
+		return new DefaultSockJsFrameFormat("/**/" + callback + "(\"%s\");\r\n") {
 			@Override
 			protected String preProcessContent(String content) {
 				return JavaScriptUtils.javaScriptEscape(content);

spring-websocket/src/test/java/org/springframework/web/socket/sockjs/support/SockJsServiceTests.java

@@ -76,6 +76,7 @@ public class SockJsServiceTests extends AbstractHttpRequestTests {
 		resetResponseAndHandleRequest("GET", "/echo/server/session/", HttpStatus.NOT_FOUND);
 		resetResponseAndHandleRequest("GET", "/echo/s.erver/session/websocket", HttpStatus.NOT_FOUND);
 		resetResponseAndHandleRequest("GET", "/echo/server/s.ession/websocket", HttpStatus.NOT_FOUND);
+		resetResponseAndHandleRequest("GET", "/echo/server/session/jsonp;Setup.pl", HttpStatus.NOT_FOUND);
 	}
 
 	@Test

spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/HttpSendingTransportHandlerTests.java

@@ -24,6 +24,7 @@ import org.junit.Test;
 import org.springframework.scheduling.TaskScheduler;
 import org.springframework.web.socket.AbstractHttpRequestTests;
 import org.springframework.web.socket.WebSocketHandler;
+import org.springframework.web.socket.sockjs.SockJsTransportFailureException;
 import org.springframework.web.socket.sockjs.frame.SockJsFrame;
 import org.springframework.web.socket.sockjs.frame.SockJsFrameFormat;
 import org.springframework.web.socket.sockjs.transport.session.AbstractSockJsSession;
@@ -31,8 +32,13 @@ import org.springframework.web.socket.sockjs.transport.session.PollingSockJsSess
 import org.springframework.web.socket.sockjs.transport.session.StreamingSockJsSession;
 import org.springframework.web.socket.sockjs.transport.session.StubSockJsServiceConfig;
 
-import static org.junit.Assert.*;
-import static org.mockito.Mockito.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
 
 /**
  * Test fixture for {@link AbstractHttpSendingTransportHandler} and sub-classes.
@@ -91,24 +97,45 @@ public class HttpSendingTransportHandlerTests  extends AbstractHttpRequestTests
 
 	@Test
 	public void jsonpTransport() throws Exception {
+		testJsonpTransport(null, false);
+		testJsonpTransport("_jp123xYz", true);
+		testJsonpTransport("A..B__3..4", true);
+		testJsonpTransport("!jp!abc", false);
+		testJsonpTransport("<script>", false);
+		testJsonpTransport("0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_.", true);
+	}
+
+	private void testJsonpTransport(String callbackValue, boolean expectSuccess) throws Exception {
 		JsonpPollingTransportHandler transportHandler = new JsonpPollingTransportHandler();
 		transportHandler.initialize(this.sockJsConfig);
 		PollingSockJsSession session = transportHandler.createSession("1", this.webSocketHandler, null);
 
-		transportHandler.handleRequest(this.request, this.response, this.webSocketHandler, session);
-
-		assertEquals(500, this.servletResponse.getStatus());
-		assertEquals("\"callback\" parameter required", this.servletResponse.getContentAsString());
-
 		resetRequestAndResponse();
 		setRequest("POST", "/");
-		this.servletRequest.setQueryString("c=callback");
-		this.servletRequest.addParameter("c", "callback");
-		transportHandler.handleRequest(this.request, this.response, this.webSocketHandler, session);
 
-		assertEquals("application/javascript;charset=UTF-8", this.response.getHeaders().getContentType().toString());
-		assertFalse("Polling request should complete after open frame", this.servletRequest.isAsyncStarted());
-		verify(this.webSocketHandler).afterConnectionEstablished(session);
+		if (callbackValue != null) {
+			this.servletRequest.setQueryString("c=" + callbackValue);
+			this.servletRequest.addParameter("c", callbackValue);
+		}
+
+		try {
+			transportHandler.handleRequest(this.request, this.response, this.webSocketHandler, session);
+		}
+		catch (SockJsTransportFailureException ex) {
+			if (expectSuccess) {
+				throw new AssertionError("Unexpected transport failure", ex);
+			}
+		}
+
+		if (expectSuccess) {
+			assertEquals(200, this.servletResponse.getStatus());
+			assertEquals("application/javascript;charset=UTF-8", this.response.getHeaders().getContentType().toString());
+			verify(this.webSocketHandler).afterConnectionEstablished(session);
+		}
+		else {
+			assertEquals(500, this.servletResponse.getStatus());
+			verifyNoMoreInteractions(this.webSocketHandler);
+		}
 	}
 
 	@Test
@@ -184,7 +211,7 @@ public class HttpSendingTransportHandlerTests  extends AbstractHttpRequestTests
 
 		format = new JsonpPollingTransportHandler().getFrameFormat(this.request);
 		formatted = format.format(frame);
-		assertEquals("callback(\"" + frame.getContent() + "\");\r\n", formatted);
+		assertEquals("/**/callback(\"" + frame.getContent() + "\");\r\n", formatted);
 	}
 
 }