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
(cherry picked from commit 9b3319b)

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 @@ package org.springframework.web.socket;
 
 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 @@ public abstract class AbstractHttpRequestTests {
 	}
 
 	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 class WebMvcStompWebSocketEndpointRegistrationTests {
 		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 class WebSocketHandlerRegistrationTests {
 		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();

spring-websocket/src/test/java/org/springframework/web/socket/server/support/OriginHandshakeInterceptorTests.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.
@@ -35,7 +35,25 @@ import org.springframework.web.socket.WebSocketHandler;
  *
  * @author Sebastien Deleuze
  */
-public class AllowedOriginsInterceptorTests extends AbstractHttpRequestTests {
+public class OriginHandshakeInterceptorTests extends AbstractHttpRequestTests {
+
+	@Test(expected = IllegalArgumentException.class)
+	public void nullAllowedOriginList() {
+		OriginHandshakeInterceptor interceptor = new OriginHandshakeInterceptor();
+		interceptor.setAllowedOrigins(null);
+	}
+
+	@Test(expected = IllegalArgumentException.class)
+	public void invalidAllowedOrigin() {
+		OriginHandshakeInterceptor interceptor = new OriginHandshakeInterceptor();
+		interceptor.setAllowedOrigins(Arrays.asList("domain.com"));
+	}
+
+	@Test
+	public void validAllowedOrigins() {
+		OriginHandshakeInterceptor interceptor = new OriginHandshakeInterceptor();
+		interceptor.setAllowedOrigins(Arrays.asList("http://domain.com", "https://domain.com", "*"));
+	}
 
 	@Test
 	public void originValueMatch() throws Exception {
@@ -82,24 +100,27 @@ public class AllowedOriginsInterceptorTests extends AbstractHttpRequestTests {
 	}
 
 	@Test
-	public void noOriginNoMatchWithNullHostileCollection() throws Exception {
+	public void originNoMatchWithNullHostileCollection() throws Exception {
 		Map<String, Object> attributes = new HashMap<String, Object>();
 		WebSocketHandler wsHandler = Mockito.mock(WebSocketHandler.class);
+		setOrigin("http://mydomain4.com");
+		OriginHandshakeInterceptor interceptor = new OriginHandshakeInterceptor();
 		Set<String> allowedOrigins = new ConcurrentSkipListSet<String>();
 		allowedOrigins.add("http://mydomain1.com");
-		OriginHandshakeInterceptor interceptor = new OriginHandshakeInterceptor();
 		interceptor.setAllowedOrigins(allowedOrigins);
 		assertFalse(interceptor.beforeHandshake(request, response, wsHandler, attributes));
 		assertEquals(servletResponse.getStatus(), HttpStatus.FORBIDDEN.value());
 	}
 
 	@Test
-	 public void noOriginNoMatch() throws Exception {
+	public void originMatchAll() throws Exception {
 		Map<String, Object> attributes = new HashMap<String, Object>();
 		WebSocketHandler wsHandler = Mockito.mock(WebSocketHandler.class);
+		setOrigin("http://mydomain1.com");
 		OriginHandshakeInterceptor interceptor = new OriginHandshakeInterceptor();
-		assertFalse(interceptor.beforeHandshake(request, response, wsHandler, attributes));
-		assertEquals(servletResponse.getStatus(), HttpStatus.FORBIDDEN.value());
+		interceptor.setAllowedOrigins(Arrays.asList("*"));
+		assertTrue(interceptor.beforeHandshake(request, response, wsHandler, attributes));
+		assertNotEquals(servletResponse.getStatus(), HttpStatus.FORBIDDEN.value());
 	}
 
 }

spring-websocket/src/test/java/org/springframework/web/socket/sockjs/support/SockJsServiceTests.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,7 +18,6 @@ package org.springframework.web.socket.sockjs.support;
 
 import java.io.IOException;
 import java.util.Arrays;
-import java.util.List;
 import javax.servlet.ServletOutputStream;
 import javax.servlet.http.HttpServletResponse;
 
@@ -103,13 +102,13 @@ public class SockJsServiceTests extends AbstractHttpRequestTests {
 				body.substring(body.indexOf(',')));
 
 		this.service.setAllowedOrigins(Arrays.asList("http://mydomain1.com"));
-		resetResponseAndHandleRequest("GET", "/echo/info", HttpStatus.FORBIDDEN);
+		resetResponseAndHandleRequest("GET", "/echo/info", HttpStatus.OK);
 		assertNull(this.servletResponse.getHeader("Access-Control-Allow-Origin"));
 		assertNull(this.servletResponse.getHeader("Access-Control-Allow-Credentials"));
 		assertNull(this.servletResponse.getHeader("Vary"));
 	}
 
-	@Test  // SPR-12226
+	@Test  // SPR-12226 and SPR-12660
 	public void handleInfoGetWithOrigin() throws Exception {
 		setOrigin("http://mydomain2.com");
 		resetResponseAndHandleRequest("GET", "/echo/info", HttpStatus.OK);
@@ -125,12 +124,6 @@ public class SockJsServiceTests extends AbstractHttpRequestTests {
 		assertEquals(",\"origins\":[\"*:*\"],\"cookie_needed\":true,\"websocket\":true}",
 				body.substring(body.indexOf(',')));
 
-		this.service.setAllowedOrigins(null);
-		resetResponseAndHandleRequest("GET", "/echo/info", HttpStatus.FORBIDDEN);
-		assertNull(this.servletResponse.getHeader("Access-Control-Allow-Origin"));
-		assertNull(this.servletResponse.getHeader("Access-Control-Allow-Credentials"));
-		assertNull(this.servletResponse.getHeader("Vary"));
-
 		this.service.setAllowedOrigins(Arrays.asList("http://mydomain1.com"));
 		resetResponseAndHandleRequest("GET", "/echo/info", HttpStatus.FORBIDDEN);
 		assertNull(this.servletResponse.getHeader("Access-Control-Allow-Origin"));
@@ -168,7 +161,7 @@ public class SockJsServiceTests extends AbstractHttpRequestTests {
 		verify(mockResponse, times(1)).getOutputStream();
 	}
 
-	@Test
+	@Test  // SPR-12660
 	public void handleInfoOptions() throws Exception {
 		this.servletRequest.addHeader("Access-Control-Request-Headers", "Last-Modified");
 		resetResponseAndHandleRequest("OPTIONS", "/echo/info", HttpStatus.NO_CONTENT);
@@ -182,19 +175,19 @@ public class SockJsServiceTests extends AbstractHttpRequestTests {
 		assertEquals("Origin", this.servletResponse.getHeader("Vary"));
 
 		this.service.setAllowedOrigins(Arrays.asList("http://mydomain1.com"));
-		resetResponseAndHandleRequest("OPTIONS", "/echo/info", HttpStatus.FORBIDDEN);
+		resetResponseAndHandleRequest("OPTIONS", "/echo/info", HttpStatus.NO_CONTENT);
 		assertNull(this.servletResponse.getHeader("Access-Control-Allow-Origin"));
 		assertNull(this.servletResponse.getHeader("Access-Control-Allow-Credentials"));
 		assertNull(this.servletResponse.getHeader("Access-Control-Allow-Headers"));
 		assertNull(this.servletResponse.getHeader("Access-Control-Allow-Methods"));
 		assertNull(this.servletResponse.getHeader("Access-Control-Max-Age"));
-		assertNull(this.servletResponse.getHeader("Vary"));
+		assertEquals("Origin", this.servletResponse.getHeader("Vary"));
 	}
 
-	@Test  // SPR-12226
+	@Test  // SPR-12226 and SPR-12660
 	public void handleInfoOptionsWithOrigin() throws Exception {
 		setOrigin("http://mydomain2.com");
-		this.servletRequest.addHeader("Access-Control-Request-Headers", "Last-Modified");
+		this.request.getHeaders().add("Access-Control-Request-Headers", "Last-Modified");
 		resetResponseAndHandleRequest("OPTIONS", "/echo/info", HttpStatus.NO_CONTENT);
 		this.response.flush();
 		assertEquals("http://mydomain2.com", this.servletResponse.getHeader("Access-Control-Allow-Origin"));
@@ -204,16 +197,6 @@ public class SockJsServiceTests extends AbstractHttpRequestTests {
 		assertEquals("31536000", this.servletResponse.getHeader("Access-Control-Max-Age"));
 		assertEquals("Origin", this.servletResponse.getHeader("Vary"));
 
-		this.service.setAllowedOrigins(null);
-		resetResponseAndHandleRequest("OPTIONS", "/echo/info", HttpStatus.FORBIDDEN);
-		this.response.flush();
-		assertNull(this.servletResponse.getHeader("Access-Control-Allow-Origin"));
-		assertNull(this.servletResponse.getHeader("Access-Control-Allow-Credentials"));
-		assertNull(this.servletResponse.getHeader("Access-Control-Allow-Headers"));
-		assertNull(this.servletResponse.getHeader("Access-Control-Allow-Methods"));
-		assertNull(this.servletResponse.getHeader("Access-Control-Max-Age"));
-		assertNull(this.servletResponse.getHeader("Vary"));
-
 		this.service.setAllowedOrigins(Arrays.asList("http://mydomain1.com"));
 		resetResponseAndHandleRequest("OPTIONS", "/echo/info", HttpStatus.FORBIDDEN);
 		this.response.flush();
@@ -236,8 +219,9 @@ public class SockJsServiceTests extends AbstractHttpRequestTests {
 	}
 
 	@Test  // SPR-12283
-	public void handleInfoOptionsWithOriginAndCorsDisabled() throws Exception {
+	public void handleInfoOptionsWithOriginAndCorsHeadersDisabled() throws Exception {
 		setOrigin("http://mydomain2.com");
+		this.service.setAllowedOrigins(Arrays.asList("*"));
 		this.service.setSuppressCors(true);
 
 		this.servletRequest.addHeader("Access-Control-Request-Headers", "Last-Modified");
@@ -278,7 +262,7 @@ public class SockJsServiceTests extends AbstractHttpRequestTests {
 		assertEquals("text/html;charset=UTF-8", this.servletResponse.getContentType());
 		assertTrue(this.servletResponse.getContentAsString().startsWith("<!DOCTYPE html>\n"));
 		assertEquals(490, this.servletResponse.getContentLength());
-		assertEquals("public, max-age=31536000", this.response.getHeaders().getCacheControl());
+		assertEquals("no-store, no-cache, must-revalidate, max-age=0", this.response.getHeaders().getCacheControl());
 		assertEquals("\"06b486b3208b085d9e3220f456a6caca4\"", this.response.getHeaders().getETag());
 	}
 

spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/DefaultSockJsServiceTests.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,9 +18,9 @@ package org.springframework.web.socket.sockjs.transport.handler;
 
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.List;
 import java.util.Map;
 
+import org.hamcrest.Matchers;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.Mock;
@@ -56,8 +56,6 @@ public class DefaultSockJsServiceTests extends AbstractHttpRequestTests {
 
 	private static final String sessionUrlPrefix = "/server1/" + sessionId + "/";
 
-	private static final List<String> origins = Arrays.asList("http://mydomain1.com", "http://mydomain2.com");
-
 
 	@Mock private SessionCreatingTransportHandler xhrHandler;
 
@@ -124,6 +122,31 @@ public class DefaultSockJsServiceTests extends AbstractHttpRequestTests {
 		assertSame(xhrHandler, handlers.get(xhrHandler.getTransportType()));
 	}
 
+	@Test
+	public void defaultAllowedOrigin() {
+		assertThat(this.service.getAllowedOrigins(), Matchers.contains("*"));
+	}
+
+	@Test(expected = IllegalArgumentException.class)
+	public void nullAllowedOriginList() {
+		this.service.setAllowedOrigins(null);
+	}
+
+	@Test(expected = IllegalArgumentException.class)
+	public void emptyAllowedOriginList() {
+		this.service.setAllowedOrigins(Arrays.asList());
+	}
+
+	@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(
@@ -148,27 +171,16 @@ public class DefaultSockJsServiceTests extends AbstractHttpRequestTests {
 		assertNull(this.response.getHeaders().getFirst("Access-Control-Allow-Credentials"));
 	}
 
-	@Test  // SPR-12226
-	public void handleTransportRequestXhrAllowNullOrigin() throws Exception {
-		String sockJsPath = sessionUrlPrefix + "xhr";
-		setRequest("POST", sockJsPrefix + sockJsPath);
-		this.service.setAllowedOrigins(null);
-		this.service.handleRequest(this.request, this.response, sockJsPath, this.wsHandler);
-
-		assertNull(this.response.getHeaders().getFirst("Access-Control-Allow-Origin"));
-		assertNull(this.response.getHeaders().getFirst("Access-Control-Allow-Credentials"));
-	}
-
 	@Test  // SPR-12226
 	public void handleTransportRequestXhrAllowedOriginsMatch() throws Exception {
 		String sockJsPath = sessionUrlPrefix + "xhr";
 		setRequest("POST", sockJsPrefix + sockJsPath);
-		setOrigin(origins.get(0));
-		this.service.setAllowedOrigins(origins);
+		this.service.setAllowedOrigins(Arrays.asList("http://mydomain1.com", "http://mydomain2.com"));
+		setOrigin("http://mydomain1.com");
 		this.service.handleRequest(this.request, this.response, sockJsPath, this.wsHandler);
 
 		assertEquals(200, this.servletResponse.getStatus());
-		assertEquals(origins.get(0), this.response.getHeaders().getFirst("Access-Control-Allow-Origin"));
+		assertEquals("http://mydomain1.com", this.response.getHeaders().getFirst("Access-Control-Allow-Origin"));
 		assertEquals("true", this.response.getHeaders().getFirst("Access-Control-Allow-Credentials"));
 	}
 
@@ -176,8 +188,8 @@ public class DefaultSockJsServiceTests extends AbstractHttpRequestTests {
 	public void handleTransportRequestXhrAllowedOriginsNoMatch() throws Exception {
 		String sockJsPath = sessionUrlPrefix + "xhr";
 		setRequest("POST", sockJsPrefix + sockJsPath);
+		this.service.setAllowedOrigins(Arrays.asList("http://mydomain1.com", "http://mydomain2.com"));
 		setOrigin("http://mydomain3.com");
-		this.service.setAllowedOrigins(origins);
 		this.service.handleRequest(this.request, this.response, sockJsPath, this.wsHandler);
 
 		assertEquals(403, this.servletResponse.getStatus());
@@ -197,19 +209,6 @@ public class DefaultSockJsServiceTests extends AbstractHttpRequestTests {
 		assertNull(this.response.getHeaders().getFirst("Access-Control-Allow-Methods"));
 	}
 
-	@Test  // SPR-12226
-	public void handleTransportRequestXhrOptionsAllowNullOrigin() throws Exception {
-		String sockJsPath = sessionUrlPrefix + "xhr";
-		setRequest("OPTIONS", sockJsPrefix + sockJsPath);
-		this.service.setAllowedOrigins(null);
-		this.service.handleRequest(this.request, this.response, sockJsPath, this.wsHandler);
-
-		assertEquals(403, this.servletResponse.getStatus());
-		assertNull(this.response.getHeaders().getFirst("Access-Control-Allow-Origin"));
-		assertNull(this.response.getHeaders().getFirst("Access-Control-Allow-Credentials"));
-		assertNull(this.response.getHeaders().getFirst("Access-Control-Allow-Methods"));
-	}
-
 	@Test
 	public void handleTransportRequestNoSuitableHandler() throws Exception {
 		String sockJsPath = sessionUrlPrefix + "eventsource";
@@ -279,12 +278,6 @@ public class DefaultSockJsServiceTests extends AbstractHttpRequestTests {
 		setRequest("GET", sockJsPrefix + sockJsPath);
 		jsonpService.handleRequest(this.request, this.response, sockJsPath, this.wsHandler);
 		assertEquals(404, this.servletResponse.getStatus());
-
-		resetRequestAndResponse();
-		jsonpService.setAllowedOrigins(null);
-		setRequest("GET", sockJsPrefix + sockJsPath);
-		jsonpService.handleRequest(this.request, this.response, sockJsPath, this.wsHandler);
-		assertEquals(404, this.servletResponse.getStatus());
 	}
 
 	@Test
@@ -311,6 +304,21 @@ public class DefaultSockJsServiceTests extends AbstractHttpRequestTests {
 		assertEquals(403, this.servletResponse.getStatus());
 	}
 
+	@Test
+	public void handleTransportRequestIframe() throws Exception {
+		String sockJsPath = "/iframe.html";
+		setRequest("GET", sockJsPrefix + sockJsPath);
+		this.service.handleRequest(this.request, this.response, sockJsPath, this.wsHandler);
+		assertNotEquals(404, this.servletResponse.getStatus());
+		assertNull(this.servletResponse.getHeader("X-Frame-Options"));
+
+		resetRequestAndResponse();
+		setRequest("GET", sockJsPrefix + sockJsPath);
+		this.service.setAllowedOrigins(Arrays.asList("http://mydomain1.com"));
+		this.service.handleRequest(this.request, this.response, sockJsPath, this.wsHandler);
+		assertEquals(404, this.servletResponse.getStatus());
+	}
+
 
 	interface SessionCreatingTransportHandler extends TransportHandler, SockJsSessionFactory {
 	}