diff --git a/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/support/AbstractSockJsService.java b/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/support/AbstractSockJsService.java index caf3538c2f..43aff0628d 100644 --- a/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/support/AbstractSockJsService.java +++ b/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/support/AbstractSockJsService.java @@ -272,17 +272,16 @@ public abstract class AbstractSockJsService implements SockJsService { } /** - * Configure allowed {@code Origin} header values. This check is mostly designed for - * browser clients. There is nothing preventing other types of client to modify the - * {@code Origin} header value. - * - *

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. - * - *

Each provided allowed origin must start by "http://", "https://" or be "*" - * (means that all origins are allowed). - * + * 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. + *

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. + *

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 RFC 6454: The Web Origin Concept * @see SockJS supported transports by browser @@ -319,6 +318,7 @@ public abstract class AbstractSockJsService implements SockJsService { 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. @@ -342,22 +342,29 @@ public abstract class AbstractSockJsService implements SockJsService { // 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; @@ -365,45 +372,59 @@ public abstract class AbstractSockJsService implements SockJsService { 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(); @@ -415,14 +436,16 @@ public abstract class AbstractSockJsService implements SockJsService { 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; } @@ -455,7 +478,9 @@ public abstract class AbstractSockJsService implements SockJsService { } if (!WebUtils.isValidOrigin(request, this.allowedOrigins)) { - 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; } @@ -535,8 +560,7 @@ public abstract class AbstractSockJsService implements SockJsService { } } else if (HttpMethod.OPTIONS.equals(request.getMethod())) { - if (checkAndAddCorsHeaders(request, response, HttpMethod.OPTIONS, - HttpMethod.GET)) { + if (checkAndAddCorsHeaders(request, response, HttpMethod.OPTIONS, HttpMethod.GET)) { addCacheHeaders(response); response.setStatusCode(HttpStatus.NO_CONTENT); } diff --git a/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/support/SockJsHttpRequestHandler.java b/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/support/SockJsHttpRequestHandler.java index 01ab07449b..d1b0a16bfb 100644 --- a/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/support/SockJsHttpRequestHandler.java +++ b/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/support/SockJsHttpRequestHandler.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. @@ -56,8 +56,8 @@ public class SockJsHttpRequestHandler implements HttpRequestHandler { * @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)); @@ -97,7 +97,7 @@ public class SockJsHttpRequestHandler implements HttpRequestHandler { 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); } } diff --git a/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/transport/TransportHandlingSockJsService.java b/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/transport/TransportHandlingSockJsService.java index c8050b519c..058b7e6058 100644 --- a/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/transport/TransportHandlingSockJsService.java +++ b/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/transport/TransportHandlingSockJsService.java @@ -288,13 +288,21 @@ public class TransportHandlingSockJsService extends AbstractSockJsService implem @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 --git a/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/transport/TransportType.java b/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/transport/TransportType.java index e3da359f91..78b844240e 100644 --- a/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/transport/TransportType.java +++ b/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 headerHints; - private static final Map TRANSPORT_TYPES; + static { Map transportTypes = new HashMap(); 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 TransportType(String value, HttpMethod httpMethod, String... headerHints) { + + private final String value; + + private final HttpMethod httpMethod; + + private final List headerHints; + + + TransportType(String value, HttpMethod httpMethod, String... headerHints) { this.value = value; this.httpMethod = httpMethod; this.headerHints = Arrays.asList(headerHints); @@ -77,9 +83,6 @@ public enum TransportType { return this.value; } - /** - * The HTTP method for this transport. - */ public HttpMethod getHttpMethod() { return this.httpMethod; } @@ -88,6 +91,10 @@ public enum TransportType { 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 enum TransportType { 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() { diff --git a/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/DefaultSockJsServiceTests.java b/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/DefaultSockJsServiceTests.java index 9fe323bee9..36aa94567c 100644 --- a/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/DefaultSockJsServiceTests.java +++ b/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/DefaultSockJsServiceTests.java @@ -16,9 +16,6 @@ package org.springframework.web.socket.sockjs.transport.handler; -import static org.junit.Assert.*; -import static org.mockito.BDDMockito.*; - import java.util.Arrays; import java.util.Collections; import java.util.Map; @@ -28,6 +25,7 @@ import org.junit.Test; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import org.springframework.http.HttpHeaders; import org.springframework.scheduling.TaskScheduler; import org.springframework.web.socket.AbstractHttpRequestTests; import org.springframework.web.socket.WebSocketHandler; @@ -41,11 +39,15 @@ import org.springframework.web.socket.sockjs.transport.TransportType; import org.springframework.web.socket.sockjs.transport.session.StubSockJsServiceConfig; import org.springframework.web.socket.sockjs.transport.session.TestSockJsSession; +import static org.junit.Assert.*; +import static org.mockito.BDDMockito.*; + /** * Test fixture for {@link org.springframework.web.socket.sockjs.transport.handler.DefaultSockJsService}. * * @author Rossen Stoyanchev * @author Sebastien Deleuze + * @author Ben Kiefer */ public class DefaultSockJsServiceTests extends AbstractHttpRequestTests { @@ -176,6 +178,18 @@ public class DefaultSockJsServiceTests extends AbstractHttpRequestTests { assertNull(this.response.getHeaders().getFirst("Access-Control-Allow-Credentials")); } + @Test // SPR-13545 + public void handleInvalidTransportType() throws Exception { + String sockJsPath = sessionUrlPrefix + "invalid"; + setRequest("POST", sockJsPrefix + sockJsPath); + this.service.setAllowedOrigins(Arrays.asList("http://mydomain1.com")); + this.servletRequest.addHeader(HttpHeaders.ORIGIN, "http://mydomain2.com"); + this.servletRequest.setServerName("mydomain2.com"); + this.service.handleRequest(this.request, this.response, sockJsPath, this.wsHandler); + + assertEquals(404, this.servletResponse.getStatus()); + } + @Test public void handleTransportRequestXhrOptions() throws Exception { String sockJsPath = sessionUrlPrefix + "xhr"; diff --git a/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/HttpReceivingTransportHandlerTests.java b/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/HttpReceivingTransportHandlerTests.java index c85a6cf4b9..070f8d7816 100644 --- a/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/HttpReceivingTransportHandlerTests.java +++ b/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/HttpReceivingTransportHandlerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2013 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,7 +16,6 @@ package org.springframework.web.socket.sockjs.transport.handler; -import org.junit.Before; import org.junit.Test; import org.springframework.http.MediaType; @@ -37,14 +36,7 @@ import static org.mockito.BDDMockito.*; * * @author Rossen Stoyanchev */ -public class HttpReceivingTransportHandlerTests extends AbstractHttpRequestTests { - - - @Override - @Before - public void setUp() { - super.setUp(); - } +public class HttpReceivingTransportHandlerTests extends AbstractHttpRequestTests { @Test public void readMessagesXhr() throws Exception { @@ -73,9 +65,7 @@ public class HttpReceivingTransportHandlerTests extends AbstractHttpRequestTest assertEquals("ok", this.servletResponse.getContentAsString()); } - // SPR-10621 - - @Test + @Test // SPR-10621 public void readMessagesJsonpFormEncodedWithEncoding() throws Exception { this.servletRequest.setContent("d=[\"x\"]".getBytes("UTF-8")); this.servletRequest.setContentType("application/x-www-form-urlencoded;charset=UTF-8"); @@ -102,9 +92,7 @@ public class HttpReceivingTransportHandlerTests extends AbstractHttpRequestTest @Test public void delegateMessageException() throws Exception { - StubSockJsServiceConfig sockJsConfig = new StubSockJsServiceConfig(); - this.servletRequest.setContent("[\"x\"]".getBytes("UTF-8")); WebSocketHandler wsHandler = mock(WebSocketHandler.class); @@ -126,7 +114,6 @@ public class HttpReceivingTransportHandlerTests extends AbstractHttpRequestTest private void handleRequest(AbstractHttpReceivingTransportHandler transportHandler) throws Exception { - WebSocketHandler wsHandler = mock(WebSocketHandler.class); AbstractSockJsSession session = new TestHttpSockJsSession("1", new StubSockJsServiceConfig(), wsHandler, null); @@ -138,7 +125,6 @@ public class HttpReceivingTransportHandlerTests extends AbstractHttpRequestTest } private void handleRequestAndExpectFailure() throws Exception { - resetResponse(); WebSocketHandler wsHandler = mock(WebSocketHandler.class); diff --git a/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/HttpSendingTransportHandlerTests.java b/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/HttpSendingTransportHandlerTests.java index 7747fa815e..bc78068840 100644 --- a/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/HttpSendingTransportHandlerTests.java +++ b/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/HttpSendingTransportHandlerTests.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. @@ -62,9 +62,9 @@ public class HttpSendingTransportHandlerTests extends AbstractHttpRequestTests setRequest("POST", "/"); } + @Test public void handleRequestXhr() throws Exception { - XhrPollingTransportHandler transportHandler = new XhrPollingTransportHandler(); transportHandler.initialize(this.sockJsConfig); @@ -91,7 +91,6 @@ public class HttpSendingTransportHandlerTests extends AbstractHttpRequestTests @Test public void jsonpTransport() throws Exception { - JsonpPollingTransportHandler transportHandler = new JsonpPollingTransportHandler(); transportHandler.initialize(this.sockJsConfig); PollingSockJsSession session = transportHandler.createSession("1", this.webSocketHandler, null); @@ -114,7 +113,6 @@ public class HttpSendingTransportHandlerTests extends AbstractHttpRequestTests @Test public void handleRequestXhrStreaming() throws Exception { - XhrStreamingTransportHandler transportHandler = new XhrStreamingTransportHandler(); transportHandler.initialize(this.sockJsConfig); AbstractSockJsSession session = transportHandler.createSession("1", this.webSocketHandler, null); @@ -128,7 +126,6 @@ public class HttpSendingTransportHandlerTests extends AbstractHttpRequestTests @Test public void htmlFileTransport() throws Exception { - HtmlFileTransportHandler transportHandler = new HtmlFileTransportHandler(); transportHandler.initialize(this.sockJsConfig); StreamingSockJsSession session = transportHandler.createSession("1", this.webSocketHandler, null); @@ -151,7 +148,6 @@ public class HttpSendingTransportHandlerTests extends AbstractHttpRequestTests @Test public void eventSourceTransport() throws Exception { - EventSourceTransportHandler transportHandler = new EventSourceTransportHandler(); transportHandler.initialize(this.sockJsConfig); StreamingSockJsSession session = transportHandler.createSession("1", this.webSocketHandler, null); @@ -165,7 +161,6 @@ public class HttpSendingTransportHandlerTests extends AbstractHttpRequestTests @Test public void frameFormats() throws Exception { - this.servletRequest.setQueryString("c=callback"); this.servletRequest.addParameter("c", "callback"); diff --git a/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/SockJsWebSocketHandlerTests.java b/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/SockJsWebSocketHandlerTests.java index 8bc7bc679d..1d8d0efe8d 100644 --- a/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/SockJsWebSocketHandlerTests.java +++ b/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/SockJsWebSocketHandlerTests.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. @@ -31,11 +31,11 @@ import static org.mockito.Mockito.*; /** * Unit tests for {@link SockJsWebSocketHandler}. + * * @author Rossen Stoyanchev */ public class SockJsWebSocketHandlerTests { - @Test public void getSubProtocols() throws Exception { SubscribableChannel channel = mock(SubscribableChannel.class);