Fix SockJS origin check

This commit introduces the following changes:
 - Requests without Origin header are not rejected anymore
 - Disable Iframe when allowedOrigins is not empty and not equals to *
 - The Iframe is not cached anymore in order to have a reliable origin check
 - allowedOrigins must not be null or empty
 - allowedOrigins format is now validated (should be * or start by http(s)://)

Issue: SPR-12660

Author: sdeleuze

spring-websocket/src/main/java/org/springframework/web/socket/config/annotation/AbstractWebSocketHandlerRegistration.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.
@@ -89,6 +89,7 @@ public WebSocketHandlerRegistration addInterceptors(HandshakeInterceptor... inte
 
 	@Override
 	public WebSocketHandlerRegistration setAllowedOrigins(String... origins) {
+		Assert.notEmpty(origins, "No allowed origin specified");
 		this.allowedOrigins.clear();
 		if (!ObjectUtils.isEmpty(origins)) {
 			this.allowedOrigins.addAll(Arrays.asList(origins));

spring-websocket/src/main/java/org/springframework/web/socket/config/annotation/StompWebSocketEndpointRegistration.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.
@@ -43,16 +43,20 @@ public interface StompWebSocketEndpointRegistration {
 	StompWebSocketEndpointRegistration addInterceptors(HandshakeInterceptor... interceptors);
 
 	/**
-	 * Configure allowed {@code Origin} header values. This check is mostly designed for browser
-	 * clients. There is noting preventing other types of client to modify the Origin header value.
+	 * 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.
 	 *
-	 * <p>When SockJS is enabled and allowed origins are restricted, transport types that do not
-	 * use {@code Origin} headers for cross origin requests (jsonp-polling, iframe-xhr-polling,
-	 * iframe-eventsource and iframe-htmlfile) are disabled. As a consequence, IE6/IE7 won't be
-	 * supported anymore and IE8/IE9 will only be supported without cookies.
+	 * <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 start by "http://", "https://" or be "*"
+	 * (means that all origins are allowed). Empty allowed origin list is not supported.
+	 * By default, all origins are allowed.
 	 *
-	 * <p>By default, 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>
 	 */
 	StompWebSocketEndpointRegistration setAllowedOrigins(String... origins);

spring-websocket/src/main/java/org/springframework/web/socket/config/annotation/WebMvcStompWebSocketEndpointRegistration.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.
@@ -86,10 +86,9 @@ public StompWebSocketEndpointRegistration addInterceptors(HandshakeInterceptor..
 
 	@Override
 	public StompWebSocketEndpointRegistration setAllowedOrigins(String... origins) {
+		Assert.notEmpty(origins, "No allowed origin specified");
 		this.allowedOrigins.clear();
-		if (!ObjectUtils.isEmpty(origins)) {
-			this.allowedOrigins.addAll(Arrays.asList(origins));
-		}
+		this.allowedOrigins.addAll(Arrays.asList(origins));
 		return this;
 	}
 

spring-websocket/src/main/java/org/springframework/web/socket/config/annotation/WebSocketHandlerRegistration.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.
@@ -45,17 +45,20 @@ public interface WebSocketHandlerRegistration {
 	WebSocketHandlerRegistration addInterceptors(HandshakeInterceptor... interceptors);
 
 	/**
-	 * Configure allowed {@code Origin} header values. This check is mostly designed for browser
-	 * clients. There is noting preventing other types of client to modify the Origin header value.
+	 * 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.
 	 *
-	 * <p>When SockJS is enabled and allowed origins are restricted, transport types that do not
-	 * use {@code Origin} headers for cross origin requests (jsonp-polling, iframe-xhr-polling,
-	 * iframe-eventsource and iframe-htmlfile) are disabled. As a consequence, IE6/IE7 won't be
-	 * supported anymore and IE8/IE9 will only be supported without cookies.
+	 * <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>By default, all origins are allowed.
+	 * <p>Each provided allowed origin must start by "http://", "https://" or be "*"
+	 * (means that all origins are allowed). Empty allowed origin list is not supported.
+	 * By default, 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>
 	 */
 	WebSocketHandlerRegistration setAllowedOrigins(String... origins);

spring-websocket/src/main/java/org/springframework/web/socket/server/support/OriginHandshakeInterceptor.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,6 +18,7 @@
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 
@@ -27,6 +28,7 @@
 import org.springframework.http.HttpStatus;
 import org.springframework.http.server.ServerHttpRequest;
 import org.springframework.http.server.ServerHttpResponse;
+import org.springframework.util.Assert;
 import org.springframework.web.socket.WebSocketHandler;
 import org.springframework.web.socket.server.HandshakeInterceptor;
 
@@ -52,13 +54,32 @@ public OriginHandshakeInterceptor() {
 	}
 
 	/**
-	 * Use this property to define a collection of allowed origins.
+	 * 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.
+	 *
+	 * <p>Each provided allowed origin must start by "http://", "https://" or be "*"
+	 * (means that all origins are allowed).
+	 *
+	 * @see <a href="https://tools.ietf.org/html/rfc6454">RFC 6454: The Web Origin Concept</a>
 	 */
 	public void setAllowedOrigins(Collection<String> allowedOrigins) {
-		this.allowedOrigins.clear();
-		if (allowedOrigins != null) {
-			this.allowedOrigins.addAll(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);
+	}
+
+	/**
+	 * @see #setAllowedOrigins(Collection)
+	 * @since 4.1.5
+	 */
+	public Collection<String> getAllowedOrigins() {
+		return Collections.unmodifiableList(this.allowedOrigins);
 	}
 
 	@Override
@@ -76,7 +97,14 @@ public boolean beforeHandshake(ServerHttpRequest request, ServerHttpResponse res
 	}
 
 	protected boolean isValidOrigin(ServerHttpRequest request) {
-		return this.allowedOrigins.contains(request.getHeaders().getOrigin());
+		String origin = request.getHeaders().getOrigin();
+		if (origin == null) {
+			return true;
+		}
+		if (this.allowedOrigins.contains("*")) {
+			return true;
+		}
+		return this.allowedOrigins.contains(origin);
 	}
 
 	@Override

spring-websocket/src/main/java/org/springframework/web/socket/sockjs/support/AbstractSockJsService.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.
@@ -266,32 +266,42 @@ public boolean isWebSocketEnabled() {
 	}
 
 	/**
-	 * Configure allowed {@code Origin} header values. This check is mostly designed for browser
-	 * clients. There is noting preventing other types of client to modify the Origin header value.
+	 * 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.
 	 *
-	 * <p>When SockJS is enabled and allowed origins are restricted, transport types that do not
-	 * use {@code Origin} headers for cross origin requests (jsonp-polling, iframe-xhr-polling,
-	 * iframe-eventsource and iframe-htmlfile) are disabled. As a consequence, IE6/IE7 won't be
-	 * supported anymore and IE8/IE9 will only be supported without cookies.
+	 * <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>By default, all origins are allowed.
+	 * <p>Each provided allowed origin must start by "http://", "https://" or be "*"
+	 * (means that all origins are allowed). Empty allowed origin list is not supported.
+	 * By default, 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>
 	 */
 	public void setAllowedOrigins(List<String> allowedOrigins) {
-		this.allowedOrigins.clear();
-		if (allowedOrigins != null) {
-			this.allowedOrigins.addAll(allowedOrigins);
+		Assert.notEmpty(allowedOrigins, "Allowed origin List must not be empty");
+		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);
 	}
 
 	/**
 	 * @since 4.1.2
 	 * @see #setAllowedOrigins(List)
 	 */
 	public List<String> getAllowedOrigins() {
-		return Collections.unmodifiableList(allowedOrigins);
+		return Collections.unmodifiableList(this.allowedOrigins);
 	}
 
 	/**
@@ -345,6 +355,11 @@ else if (sockJsPath.equals("/info")) {
 				this.infoHandler.handle(request, response);
 			}
 			else if (sockJsPath.matches("/iframe[0-9-.a-z_]*.html")) {
+				if (!this.allowedOrigins.isEmpty() && !this.allowedOrigins.contains("*")) {
+					logger.debug("Iframe support is disabled when an origin check is required, ignoring " + requestInfo);
+					response.setStatusCode(HttpStatus.NOT_FOUND);
+					return;
+				}
 				logger.debug(requestInfo);
 				this.iframeHandler.handle(request, response);
 			}
@@ -423,8 +438,13 @@ protected boolean checkAndAddCorsHeaders(ServerHttpRequest request, ServerHttpRe
 		HttpHeaders requestHeaders = request.getHeaders();
 		HttpHeaders responseHeaders = response.getHeaders();
 		String origin = requestHeaders.getOrigin();
+		String host = requestHeaders.getFirst(HttpHeaders.HOST);
+
+		if (origin == null) {
+			return true;
+		}
 
-		if (!this.allowedOrigins.contains("*") && (origin == null || !this.allowedOrigins.contains(origin))) {
+		if (!this.allowedOrigins.contains("*") && !this.allowedOrigins.contains(origin)) {
 			logger.debug("Request rejected, Origin header value " + origin + " not allowed");
 			response.setStatusCode(HttpStatus.FORBIDDEN);
 			return false;
@@ -439,7 +459,7 @@ protected boolean checkAndAddCorsHeaders(ServerHttpRequest request, ServerHttpRe
 			// See SPR-11919 and https://issues.jboss.org/browse/WFLY-3474
 		}
 
-		if (!this.suppressCors && origin != null && !hasCorsResponseHeaders) {
+		if (!this.suppressCors && !hasCorsResponseHeaders) {
 			addCorsHeaders(request, response, httpMethods);
 		}
 		return true;
@@ -561,7 +581,8 @@ public void handle(ServerHttpRequest request, ServerHttpResponse response) throw
 			response.getHeaders().setContentType(new MediaType("text", "html", UTF8_CHARSET));
 			response.getHeaders().setContentLength(contentBytes.length);
 
-			addCacheHeaders(response);
+			// No cache in order to check every time if IFrame are authorized
+			addNoCacheHeaders(response);
 			response.getHeaders().setETag(etagValue);
 			response.getBody().write(contentBytes);
 		}

spring-websocket/src/test/java/org/springframework/web/socket/AbstractHttpRequestTests.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,6 +18,7 @@
 
 import org.junit.Before;
 
+import org.springframework.http.HttpHeaders;
 import org.springframework.http.server.ServerHttpAsyncRequestControl;
 import org.springframework.http.server.ServerHttpRequest;
 import org.springframework.http.server.ServerHttpResponse;
@@ -55,7 +56,7 @@ protected void setRequest(String method, String requestUri) {
 	}
 
 	protected void setOrigin(String origin) {
-		this.servletRequest.addHeader("Origin", origin);
+		this.request.getHeaders().add(HttpHeaders.ORIGIN, origin);
 	}
 
 	protected void resetRequestAndResponse() {

spring-websocket/src/test/java/org/springframework/web/socket/config/annotation/WebMvcStompWebSocketEndpointRegistrationTests.java

@@ -90,6 +90,12 @@ public void allowedOrigins() {
 		assertEquals(OriginHandshakeInterceptor.class, requestHandler.getHandshakeInterceptors().get(0).getClass());
 	}
 
+	@Test(expected = IllegalArgumentException.class)
+	public void noAllowedOrigin() {
+		WebMvcStompWebSocketEndpointRegistration registration = new WebMvcStompWebSocketEndpointRegistration(new String[] {"/foo"}, this.handler, this.scheduler);
+		registration.setAllowedOrigins();
+	}
+
 	@Test
 	public void allowedOriginsWithSockJsService() {
 		WebMvcStompWebSocketEndpointRegistration registration =

spring-websocket/src/test/java/org/springframework/web/socket/config/annotation/WebSocketHandlerRegistrationTests.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.
@@ -93,6 +93,11 @@ public void interceptors() {
 		assertArrayEquals(new HandshakeInterceptor[] {interceptor}, mapping.interceptors);
 	}
 
+	@Test(expected = IllegalArgumentException.class)
+	public void noAllowedOrigin() {
+		this.registration.addHandler(Mockito.mock(WebSocketHandler.class), "/foo").setAllowedOrigins();
+	}
+
 	@Test
 	public void interceptorsWithAllowedOrigins() {
 		WebSocketHandler handler = new TextWebSocketHandler();