From b44044e36f4a670006c803795b20a62c836f2975 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Mon, 4 May 2015 11:02:55 -0400 Subject: [PATCH] Allow "ws" and "wss" for isValidCorsOrigin checks Issue: SPR-12956 --- .../web/util/UriComponentsBuilder.java | 23 ++++- .../springframework/web/util/WebUtils.java | 24 ++--- .../web/util/WebUtilsTests.java | 95 ++++++++----------- .../support/OriginHandshakeInterceptor.java | 5 - .../sockjs/support/AbstractSockJsService.java | 8 -- .../OriginHandshakeInterceptorTests.java | 20 +--- .../handler/DefaultSockJsServiceTests.java | 25 +---- 7 files changed, 76 insertions(+), 124 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/util/UriComponentsBuilder.java b/spring-web/src/main/java/org/springframework/web/util/UriComponentsBuilder.java index e93f73860f..0b4fa36015 100644 --- a/spring-web/src/main/java/org/springframework/web/util/UriComponentsBuilder.java +++ b/spring-web/src/main/java/org/springframework/web/util/UriComponentsBuilder.java @@ -272,7 +272,7 @@ public class UriComponentsBuilder implements Cloneable { * @return the URI components of the URI * @since 4.1.5 */ - public static UriComponentsBuilder fromHttpRequest(HttpRequest request) { + public static UriComponentsBuilder fromHttpRequest(HttpRequest request) { URI uri = request.getURI(); UriComponentsBuilder builder = UriComponentsBuilder.fromUri(uri); @@ -316,6 +316,27 @@ public class UriComponentsBuilder implements Cloneable { return builder; } + /** + * Create an instance by parsing the "origin" header of an HTTP request. + */ + public static UriComponentsBuilder fromOriginHeader(String origin) { + UriComponentsBuilder builder = UriComponentsBuilder.newInstance(); + if (StringUtils.hasText(origin)) { + int schemaIdx = origin.indexOf("://"); + String schema = (schemaIdx != -1 ? origin.substring(0, schemaIdx) : "http"); + builder.scheme(schema); + String hostString = (schemaIdx != -1 ? origin.substring(schemaIdx + 3) : origin); + if (hostString.contains(":")) { + String[] hostAndPort = StringUtils.split(hostString, ":"); + builder.host(hostAndPort[0]); + builder.port(Integer.parseInt(hostAndPort[1])); + } + else { + builder.host(hostString); + } + } + return builder; + } // build methods diff --git a/spring-web/src/main/java/org/springframework/web/util/WebUtils.java b/spring-web/src/main/java/org/springframework/web/util/WebUtils.java index 572e457ae3..c5c5177d5e 100644 --- a/spring-web/src/main/java/org/springframework/web/util/WebUtils.java +++ b/spring-web/src/main/java/org/springframework/web/util/WebUtils.java @@ -38,6 +38,7 @@ import org.apache.commons.logging.LogFactory; import org.springframework.http.HttpRequest; import org.springframework.util.Assert; +import org.springframework.util.CollectionUtils; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; import org.springframework.util.StringUtils; @@ -790,21 +791,10 @@ public abstract class WebUtils { if (origin == null || allowedOrigins.contains("*")) { return true; } - else if (allowedOrigins.isEmpty()) { - UriComponents originComponents; - try { - originComponents = UriComponentsBuilder.fromHttpUrl(origin).build(); - } - catch (IllegalArgumentException ex) { - if (logger.isWarnEnabled()) { - logger.warn("Failed to parse Origin header value [" + origin + "]"); - } - return false; - } - UriComponents requestComponents = UriComponentsBuilder.fromHttpRequest(request).build(); - int originPort = getPort(originComponents); - int requestPort = getPort(requestComponents); - return (originComponents.getHost().equals(requestComponents.getHost()) && originPort == requestPort); + else if (CollectionUtils.isEmpty(allowedOrigins)) { + UriComponents actualUrl = UriComponentsBuilder.fromHttpRequest(request).build(); + UriComponents originUrl = UriComponentsBuilder.fromOriginHeader(origin).build(); + return (actualUrl.getHost().equals(originUrl.getHost()) && getPort(actualUrl) == getPort(originUrl)); } else { return allowedOrigins.contains(origin); @@ -814,10 +804,10 @@ public abstract class WebUtils { private static int getPort(UriComponents component) { int port = component.getPort(); if (port == -1) { - if ("http".equals(component.getScheme())) { + if ("http".equals(component.getScheme()) || "ws".equals(component.getScheme())) { port = 80; } - else if ("https".equals(component.getScheme())) { + else if ("https".equals(component.getScheme()) || "wss".equals(component.getScheme())) { port = 443; } } diff --git a/spring-web/src/test/java/org/springframework/web/util/WebUtilsTests.java b/spring-web/src/test/java/org/springframework/web/util/WebUtilsTests.java index be8a2cec53..97c2852198 100644 --- a/spring-web/src/test/java/org/springframework/web/util/WebUtilsTests.java +++ b/spring-web/src/test/java/org/springframework/web/util/WebUtilsTests.java @@ -16,8 +16,10 @@ package org.springframework.web.util; -import java.util.ArrayList; +import static org.junit.Assert.*; + import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -30,8 +32,6 @@ import org.springframework.http.server.ServletServerHttpRequest; import org.springframework.mock.web.test.MockHttpServletRequest; import org.springframework.util.MultiValueMap; -import static org.junit.Assert.*; - /** * @author Juergen Hoeller * @author Arjen Poutsma @@ -106,60 +106,45 @@ public class WebUtilsTests { } @Test - public void isValidOrigin() { - List allowedOrigins = new ArrayList<>(); + public void isValidOriginSuccess() { + + List allowed = Collections.emptyList(); + assertTrue(checkOrigin("mydomain1.com", -1, "http://mydomain1.com", allowed)); + assertTrue(checkOrigin("mydomain1.com", -1, "http://mydomain1.com:80", allowed)); + assertTrue(checkOrigin("mydomain1.com", 443, "https://mydomain1.com", allowed)); + assertTrue(checkOrigin("mydomain1.com", 443, "https://mydomain1.com:443", allowed)); + assertTrue(checkOrigin("mydomain1.com", 123, "http://mydomain1.com:123", allowed)); + assertTrue(checkOrigin("mydomain1.com", -1, "ws://mydomain1.com", allowed)); + assertTrue(checkOrigin("mydomain1.com", 443, "wss://mydomain1.com", allowed)); + + allowed = Collections.singletonList("*"); + assertTrue(checkOrigin("mydomain1.com", -1, "http://mydomain2.com", allowed)); + + allowed = Collections.singletonList("http://mydomain1.com"); + assertTrue(checkOrigin("mydomain2.com", -1, "http://mydomain1.com", allowed)); + } + + @Test + public void isValidOriginFailure() { + + List allowed = Collections.emptyList(); + assertFalse(checkOrigin("mydomain1.com", -1, "http://mydomain2.com", allowed)); + assertFalse(checkOrigin("mydomain1.com", -1, "https://mydomain1.com", allowed)); + assertFalse(checkOrigin("mydomain1.com", -1, "invalid-origin", allowed)); + + allowed = Collections.singletonList("http://mydomain1.com"); + assertFalse(checkOrigin("mydomain2.com", -1, "http://mydomain3.com", allowed)); + } + + private boolean checkOrigin(String serverName, int port, String originHeader, List allowed) { MockHttpServletRequest servletRequest = new MockHttpServletRequest(); ServerHttpRequest request = new ServletServerHttpRequest(servletRequest); - - servletRequest.setServerName("mydomain1.com"); - request.getHeaders().set(HttpHeaders.ORIGIN, "http://mydomain1.com"); - assertTrue(WebUtils.isValidOrigin(request, allowedOrigins)); - - servletRequest.setServerName("mydomain1.com"); - request.getHeaders().set(HttpHeaders.ORIGIN, "http://mydomain1.com:80"); - assertTrue(WebUtils.isValidOrigin(request, allowedOrigins)); - - servletRequest.setServerName("mydomain1.com"); - servletRequest.setServerPort(443); - request.getHeaders().set(HttpHeaders.ORIGIN, "https://mydomain1.com"); - assertTrue(WebUtils.isValidOrigin(request, allowedOrigins)); - - servletRequest.setServerName("mydomain1.com"); - servletRequest.setServerPort(443); - request.getHeaders().set(HttpHeaders.ORIGIN, "https://mydomain1.com:443"); - assertTrue(WebUtils.isValidOrigin(request, allowedOrigins)); - - servletRequest.setServerName("mydomain1.com"); - servletRequest.setServerPort(123); - request.getHeaders().set(HttpHeaders.ORIGIN, "http://mydomain1.com:123"); - assertTrue(WebUtils.isValidOrigin(request, allowedOrigins)); - - servletRequest.setServerName("mydomain1.com"); - request.getHeaders().set(HttpHeaders.ORIGIN, "http://mydomain2.com"); - assertFalse(WebUtils.isValidOrigin(request, allowedOrigins)); - - servletRequest.setServerName("mydomain1.com"); - request.getHeaders().set(HttpHeaders.ORIGIN, "https://mydomain1.com"); - assertFalse(WebUtils.isValidOrigin(request, allowedOrigins)); - - servletRequest.setServerName("invalid-origin"); - request.getHeaders().set(HttpHeaders.ORIGIN, "invalid-origin"); - assertFalse(WebUtils.isValidOrigin(request, allowedOrigins)); - - allowedOrigins = Arrays.asList("*"); - servletRequest.setServerName("mydomain1.com"); - request.getHeaders().set(HttpHeaders.ORIGIN, "http://mydomain2.com"); - assertTrue(WebUtils.isValidOrigin(request, allowedOrigins)); - - allowedOrigins = Arrays.asList("http://mydomain1.com"); - servletRequest.setServerName("mydomain2.com"); - request.getHeaders().set(HttpHeaders.ORIGIN, "http://mydomain1.com"); - assertTrue(WebUtils.isValidOrigin(request, allowedOrigins)); - - allowedOrigins = Arrays.asList("http://mydomain1.com"); - servletRequest.setServerName("mydomain2.com"); - request.getHeaders().set(HttpHeaders.ORIGIN, "http://mydomain3.com"); - assertFalse(WebUtils.isValidOrigin(request, allowedOrigins)); + servletRequest.setServerName(serverName); + if (port != -1) { + servletRequest.setServerPort(port); + } + request.getHeaders().set(HttpHeaders.ORIGIN, originHeader); + return WebUtils.isValidOrigin(request, allowed); } } diff --git a/spring-websocket/src/main/java/org/springframework/web/socket/server/support/OriginHandshakeInterceptor.java b/spring-websocket/src/main/java/org/springframework/web/socket/server/support/OriginHandshakeInterceptor.java index 178fe69882..b58c357064 100644 --- a/spring-websocket/src/main/java/org/springframework/web/socket/server/support/OriginHandshakeInterceptor.java +++ b/spring-websocket/src/main/java/org/springframework/web/socket/server/support/OriginHandshakeInterceptor.java @@ -76,11 +76,6 @@ public class OriginHandshakeInterceptor implements HandshakeInterceptor { */ public void setAllowedOrigins(Collection allowedOrigins) { Assert.notNull(allowedOrigins, "Allowed origin Collection must not be null"); - for (String allowedOrigin : allowedOrigins) { - Assert.isTrue(allowedOrigin.equals("*") || allowedOrigin.startsWith("http://") || - allowedOrigin.startsWith("https://"), "Invalid allowed origin provided: \"" + - allowedOrigin + "\". It must start with \"http://\", \"https://\" or be \"*\""); - } this.allowedOrigins.clear(); this.allowedOrigins.addAll(allowedOrigins); } 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 9ed0cb0b14..caf3538c2f 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 @@ -289,14 +289,6 @@ public abstract class AbstractSockJsService implements SockJsService { */ public void setAllowedOrigins(List allowedOrigins) { Assert.notNull(allowedOrigins, "Allowed origin List must not be null"); - for (String allowedOrigin : allowedOrigins) { - Assert.isTrue( - allowedOrigin.equals("*") || allowedOrigin.startsWith("http://") || - allowedOrigin.startsWith("https://"), - "Invalid allowed origin provided: \"" + - allowedOrigin + - "\". It must start with \"http://\", \"https://\" or be \"*\""); - } this.allowedOrigins.clear(); this.allowedOrigins.addAll(allowedOrigins); } diff --git a/spring-websocket/src/test/java/org/springframework/web/socket/server/support/OriginHandshakeInterceptorTests.java b/spring-websocket/src/test/java/org/springframework/web/socket/server/support/OriginHandshakeInterceptorTests.java index 866e9569dc..86c1121c4d 100644 --- a/spring-websocket/src/test/java/org/springframework/web/socket/server/support/OriginHandshakeInterceptorTests.java +++ b/spring-websocket/src/test/java/org/springframework/web/socket/server/support/OriginHandshakeInterceptorTests.java @@ -16,13 +16,14 @@ package org.springframework.web.socket.server.support; +import static org.junit.Assert.*; + import java.util.Arrays; import java.util.HashMap; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentSkipListSet; -import static org.junit.Assert.*; import org.junit.Test; import org.mockito.Mockito; @@ -38,25 +39,10 @@ import org.springframework.web.socket.WebSocketHandler; public class OriginHandshakeInterceptorTests extends AbstractHttpRequestTests { @Test(expected = IllegalArgumentException.class) - public void nullAllowedOriginList() { + public void invalidInput() { new OriginHandshakeInterceptor(null); } - @Test(expected = IllegalArgumentException.class) - public void invalidAllowedOrigin() { - new OriginHandshakeInterceptor(Arrays.asList("domain.com")); - } - - @Test - public void emtpyAllowedOriginList() { - new OriginHandshakeInterceptor(Arrays.asList()); - } - - @Test - public void validAllowedOrigins() { - new OriginHandshakeInterceptor(Arrays.asList("http://domain.com", "https://domain.com", "*")); - } - @Test public void originValueMatch() throws Exception { Map attributes = new HashMap(); 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 dc1dce9609..9fe323bee9 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,11 +16,13 @@ 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; -import org.hamcrest.Matchers; import org.junit.Before; import org.junit.Test; import org.mockito.Mock; @@ -39,9 +41,6 @@ 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}. * @@ -123,26 +122,10 @@ public class DefaultSockJsServiceTests extends AbstractHttpRequestTests { } @Test(expected = IllegalArgumentException.class) - public void nullAllowedOriginList() { + public void invalidInput() { this.service.setAllowedOrigins(null); } - @Test - public void emptyAllowedOriginList() { - this.service.setAllowedOrigins(Arrays.asList()); - assertThat(this.service.getAllowedOrigins(), Matchers.empty()); - } - - @Test(expected = IllegalArgumentException.class) - public void invalidAllowedOrigin() { - this.service.setAllowedOrigins(Arrays.asList("domain.com")); - } - - @Test - public void validAllowedOrigins() { - this.service.setAllowedOrigins(Arrays.asList("http://domain.com", "https://domain.com", "*")); - } - @Test public void customizedTransportHandlerList() { TransportHandlingSockJsService service = new TransportHandlingSockJsService(