Revised TransportHandlingSockJsService for defensive transport checking and consistent logging

Issue: SPR-13545

Author: jhoeller

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

@@ -26,7 +26,6 @@
 import java.util.List;
 import java.util.Random;
 import java.util.concurrent.TimeUnit;
-
 import javax.servlet.http.HttpServletRequest;
 
 import org.apache.commons.logging.Log;
@@ -279,16 +278,13 @@ public boolean isWebSocketEnabled() {
 	 * Configure allowed {@code Origin} header values. This check is mostly
 	 * designed for browsers. There is nothing preventing other types of client
 	 * to modify the {@code Origin} header value.
-	 *
 	 * <p>When SockJS is enabled and origins are restricted, transport types
 	 * that do not allow to check request origin (JSONP and Iframe based
 	 * transports) are disabled. As a consequence, IE 6 to 9 are not supported
 	 * when origins are restricted.
-	 *
 	 * <p>Each provided allowed origin must have a scheme, and optionally a port
 	 * (e.g. "http://example.org", "http://example.org:9090"). An allowed origin
 	 * string may also be "*" in which case all origins are allowed.
-	 *
 	 * @since 4.1.2
 	 * @see <a href="https://tools.ietf.org/html/rfc6454">RFC 6454: The Web Origin Concept</a>
 	 * @see <a href="https://github.com/sockjs/sockjs-client#supported-transports-by-browser-html-served-from-http-or-https">SockJS supported transports by browser</a>
@@ -325,6 +321,7 @@ public boolean shouldSuppressCors() {
 		return this.suppressCors;
 	}
 
+
 	/**
 	 * This method determines the SockJS path and handles SockJS static URLs.
 	 * Session URLs and raw WebSocket requests are delegated to abstract methods.
@@ -348,68 +345,84 @@ public final void handleRequest(ServerHttpRequest request, ServerHttpResponse re
 			// As per SockJS protocol content-type can be ignored (it's always json)
 		}
 
-		String requestInfo = logger.isDebugEnabled() ? request.getMethod() + " " + request.getURI() : "";
+		String requestInfo = (logger.isDebugEnabled() ? request.getMethod() + " " + request.getURI() : null);
 		try {
 			if (sockJsPath.equals("") || sockJsPath.equals("/")) {
-				logger.debug(requestInfo);
+				if (requestInfo != null) {
+					logger.debug("Processing transport request: " + requestInfo);
+				}
 				response.getHeaders().setContentType(new MediaType("text", "plain", UTF8_CHARSET));
 				response.getBody().write("Welcome to SockJS!\n".getBytes(UTF8_CHARSET));
 			}
 			else if (sockJsPath.equals("/info")) {
-				logger.debug(requestInfo);
+				if (requestInfo != null) {
+					logger.debug("Processing transport request: " + requestInfo);
+				}
 				this.infoHandler.handle(request, response);
 			}
 			else if (sockJsPath.matches("/iframe[0-9-.a-z_]*.html")) {
 				if (!this.allowedOrigins.isEmpty() && !this.allowedOrigins.contains("*")) {
-					if (logger.isDebugEnabled()) {
-						logger.debug("Iframe support is disabled when an origin check is required, ignoring " +
-										requestInfo);
+					if (requestInfo != null) {
+						logger.debug("Iframe support is disabled when an origin check is required. " +
+								"Ignoring transport request: " + requestInfo);
 					}
 					response.setStatusCode(HttpStatus.NOT_FOUND);
 					return;
 				}
 				if (this.allowedOrigins.isEmpty()) {
 					response.getHeaders().add(XFRAME_OPTIONS_HEADER, "SAMEORIGIN");
 				}
-				logger.debug(requestInfo);
+				if (requestInfo != null) {
+					logger.debug("Processing transport request: " + requestInfo);
+				}
 				this.iframeHandler.handle(request, response);
 			}
 			else if (sockJsPath.equals("/websocket")) {
 				if (isWebSocketEnabled()) {
-					logger.debug(requestInfo);
+					if (requestInfo != null) {
+						logger.debug("Processing transport request: " + requestInfo);
+					}
 					handleRawWebSocketRequest(request, response, wsHandler);
 				}
-				else if (logger.isDebugEnabled()) {
-					logger.debug("WebSocket disabled, ignoring " + requestInfo);
+				else if (requestInfo != null) {
+					logger.debug("WebSocket disabled. Ignoring transport request: " + requestInfo);
 				}
 			}
 			else {
 				String[] pathSegments = StringUtils.tokenizeToStringArray(sockJsPath.substring(1), "/");
 				if (pathSegments.length != 3) {
 					if (logger.isWarnEnabled()) {
-						logger.warn("Ignoring invalid transport request " + requestInfo);
+						logger.warn("Invalid SockJS path '" + sockJsPath + "' - required to have 3 path segments");
+					}
+					if (requestInfo != null) {
+						logger.debug("Ignoring transport request: " + requestInfo);
 					}
 					response.setStatusCode(HttpStatus.NOT_FOUND);
 					return;
 				}
+
 				String serverId = pathSegments[0];
 				String sessionId = pathSegments[1];
 				String transport = pathSegments[2];
 
 				if (!isWebSocketEnabled() && transport.equals("websocket")) {
-					if (logger.isDebugEnabled()) {
-						logger.debug("WebSocket transport is disabled, ignoring " + requestInfo);
+					if (requestInfo != null) {
+						logger.debug("WebSocket disabled. Ignoring transport request: " + requestInfo);
 					}
 					response.setStatusCode(HttpStatus.NOT_FOUND);
 					return;
 				}
 				else if (!validateRequest(serverId, sessionId, transport)) {
-					if (logger.isWarnEnabled()) {
-						logger.warn("Ignoring transport request " + requestInfo);
+					if (requestInfo != null) {
+						logger.debug("Ignoring transport request: " + requestInfo);
 					}
 					response.setStatusCode(HttpStatus.NOT_FOUND);
 					return;
 				}
+
+				if (requestInfo != null) {
+					logger.debug("Processing transport request: " + requestInfo);
+				}
 				handleTransportRequest(request, response, wsHandler, sessionId, transport);
 			}
 			response.close();
@@ -421,14 +434,16 @@ else if (!validateRequest(serverId, sessionId, transport)) {
 
 	protected boolean validateRequest(String serverId, String sessionId, String transport) {
 		if (!StringUtils.hasText(serverId) || !StringUtils.hasText(sessionId) || !StringUtils.hasText(transport)) {
-			logger.warn("No server, session, or transport path segment");
+			logger.warn("No server, session, or transport path segment in SockJS request.");
 			return false;
 		}
+
 		// Server and session id's must not contain "."
 		if (serverId.contains(".") || sessionId.contains(".")) {
 			logger.warn("Either server or session contains a \".\" which is not allowed by SockJS protocol.");
 			return false;
 		}
+
 		return true;
 	}
 
@@ -445,6 +460,7 @@ protected abstract void handleRawWebSocketRequest(ServerHttpRequest request,
 	protected abstract void handleTransportRequest(ServerHttpRequest request, ServerHttpResponse response,
 			WebSocketHandler webSocketHandler, String sessionId, String transport) throws SockJsException;
 
+
 	protected boolean checkOrigin(ServerHttpRequest request, ServerHttpResponse response,
 			HttpMethod... httpMethods) throws IOException {
 
@@ -454,7 +470,9 @@ protected boolean checkOrigin(ServerHttpRequest request, ServerHttpResponse resp
 
 		if (!WebUtils.isValidOrigin(request, this.allowedOrigins)) {
 			String origin = request.getHeaders().getOrigin();
-			logger.debug("Request rejected, Origin header value " + origin + " not allowed");
+			if (logger.isWarnEnabled()) {
+				logger.warn("Origin header value '" + origin + "' not allowed.");
+			}
 			response.setStatusCode(HttpStatus.FORBIDDEN);
 			return false;
 		}

Diff for: spring-websocket/src/main/java/org/springframework/web/socket/sockjs/support/SockJsHttpRequestHandler.java

@@ -17,7 +17,6 @@
 package org.springframework.web.socket.sockjs.support;
 
 import java.io.IOException;
-
 import javax.servlet.ServletContext;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
@@ -66,8 +65,8 @@ public class SockJsHttpRequestHandler
 	 * @param webSocketHandler the websocket handler
 	 */
 	public SockJsHttpRequestHandler(SockJsService sockJsService, WebSocketHandler webSocketHandler) {
-		Assert.notNull(sockJsService, "sockJsService must not be null");
-		Assert.notNull(webSocketHandler, "webSocketHandler must not be null");
+		Assert.notNull(sockJsService, "SockJsService must not be null");
+		Assert.notNull(webSocketHandler, "WebSocketHandler must not be null");
 		this.sockJsService = sockJsService;
 		this.webSocketHandler =
 				new ExceptionWebSocketHandlerDecorator(new LoggingWebSocketHandlerDecorator(webSocketHandler));
@@ -95,10 +94,6 @@ public void setServletContext(ServletContext servletContext) {
 		}
 	}
 
-	@Override
-	public boolean isRunning() {
-		return this.running;
-	}
 
 	@Override
 	public void start() {
@@ -120,6 +115,11 @@ public void stop() {
 		}
 	}
 
+	@Override
+	public boolean isRunning() {
+		return this.running;
+	}
+
 
 	@Override
 	public void handleRequest(HttpServletRequest servletRequest, HttpServletResponse servletResponse)
@@ -139,13 +139,13 @@ public void handleRequest(HttpServletRequest servletRequest, HttpServletResponse
 	private String getSockJsPath(HttpServletRequest servletRequest) {
 		String attribute = HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE;
 		String path = (String) servletRequest.getAttribute(attribute);
-		return ((path.length() > 0) && (path.charAt(0) != '/')) ? "/" + path : path;
+		return (path.length() > 0 && path.charAt(0) != '/' ? "/" + path : path);
 	}
 
 	@Override
 	public CorsConfiguration getCorsConfiguration(HttpServletRequest request) {
-		if (sockJsService instanceof CorsConfigurationSource) {
-			return ((CorsConfigurationSource)sockJsService).getCorsConfiguration(request);
+		if (this.sockJsService instanceof CorsConfigurationSource) {
+			return ((CorsConfigurationSource) this.sockJsService).getCorsConfiguration(request);
 		}
 		return null;
 	}

Diff for: spring-websocket/src/main/java/org/springframework/web/socket/sockjs/transport/TransportHandlingSockJsService.java

@@ -60,8 +60,7 @@
  * @author Sebastien Deleuze
  * @since 4.0
  */
-public class TransportHandlingSockJsService extends AbstractSockJsService
-		implements SockJsServiceConfig, Lifecycle {
+public class TransportHandlingSockJsService extends AbstractSockJsService implements SockJsServiceConfig, Lifecycle {
 
 	private static final boolean jackson2Present = ClassUtils.isPresent(
 			"com.fasterxml.jackson.databind.ObjectMapper", TransportHandlingSockJsService.class.getClassLoader());
@@ -154,10 +153,6 @@ public List<HandshakeInterceptor> getHandshakeInterceptors() {
 		return this.interceptors;
 	}
 
-	@Override
-	public boolean isRunning() {
-		return this.running;
-	}
 
 	@Override
 	public void start() {
@@ -183,6 +178,11 @@ public void stop() {
 		}
 	}
 
+	@Override
+	public boolean isRunning() {
+		return this.running;
+	}
+
 
 	@Override
 	protected void handleRawWebSocketRequest(ServerHttpRequest request, ServerHttpResponse response,
@@ -322,13 +322,21 @@ else if (transportType.supportsCors()) {
 
 	@Override
 	protected boolean validateRequest(String serverId, String sessionId, String transport) {
-		if (!getAllowedOrigins().contains("*") && !TransportType.fromValue(transport).supportsOrigin()) {
-			if (logger.isWarnEnabled()) {
-				logger.warn("Origin check has been enabled, but transport " + transport + " does not support it");
-			}
+		if (!super.validateRequest(serverId, sessionId, transport)) {
 			return false;
 		}
-		return super.validateRequest(serverId, sessionId, transport);
+
+		if (!getAllowedOrigins().contains("*")) {
+			TransportType transportType = TransportType.fromValue(transport);
+			if (transportType == null || !transportType.supportsOrigin()) {
+				if (logger.isWarnEnabled()) {
+					logger.warn("Origin check enabled but transport '" + transport + "' does not support it.");
+				}
+				return false;
+			}
+		}
+
+		return true;
 	}
 
 	private SockJsSession createSockJsSession(String sessionId, SockJsSessionFactory sessionFactory,

Diff for: spring-websocket/src/main/java/org/springframework/web/socket/sockjs/transport/TransportType.java

@@ -50,13 +50,8 @@ public enum TransportType {
 	HTML_FILE("htmlfile", HttpMethod.GET, "cors", "jsessionid", "no_cache");
 
 
-	private final String value;
-
-	private final HttpMethod httpMethod;
-
-	private final List<String> headerHints;
-
 	private static final Map<String, TransportType> TRANSPORT_TYPES;
+
 	static {
 		Map<String, TransportType> transportTypes = new HashMap<String, TransportType>();
 		for (TransportType type : values()) {
@@ -65,8 +60,19 @@ public enum TransportType {
 		TRANSPORT_TYPES = Collections.unmodifiableMap(transportTypes);
 	}
 
+	public static TransportType fromValue(String value) {
+		return TRANSPORT_TYPES.get(value);
+	}
+
+
+	private final String value;
+
+	private final HttpMethod httpMethod;
 
-	private TransportType(String value, HttpMethod httpMethod, String... headerHints) {
+	private final List<String> headerHints;
+
+
+	TransportType(String value, HttpMethod httpMethod, String... headerHints) {
 		this.value = value;
 		this.httpMethod = httpMethod;
 		this.headerHints = Arrays.asList(headerHints);
@@ -77,9 +83,6 @@ public String value() {
 		return this.value;
 	}
 
-	/**
-	 * The HTTP method for this transport.
-	 */
 	public HttpMethod getHttpMethod() {
 		return this.httpMethod;
 	}
@@ -88,6 +91,10 @@ public boolean sendsNoCacheInstruction() {
 		return this.headerHints.contains("no_cache");
 	}
 
+	public boolean sendsSessionCookie() {
+		return this.headerHints.contains("jsessionid");
+	}
+
 	public boolean supportsCors() {
 		return this.headerHints.contains("cors");
 	}
@@ -96,13 +103,6 @@ public boolean supportsOrigin() {
 		return this.headerHints.contains("cors") || this.headerHints.contains("origin");
 	}
 
-	public boolean sendsSessionCookie() {
-		return this.headerHints.contains("jsessionid");
-	}
-
-	public static TransportType fromValue(String value) {
-		return TRANSPORT_TYPES.get(value);
-	}
 
 	@Override
 	public String toString() {