From 2f082cf9a2b6ef001340b059a2087488d5773568 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Fri, 25 Apr 2014 13:26:41 +0200 Subject: [PATCH] Adding support for echoing the request origin for CORS. Motivation: When CORS has been configured to allow "*" origin, and at the same time is allowing credentials/cookies, this causes an error from the browser because when the response 'Access-Control-Allow-Credentials' header is true, the 'Access-Control-Allow-Origin' must be an actual origin. Modifications: Changed CorsHandler setOrigin method to check for the combination of "*" origin and allowCredentials, and if the check matches echo the CORS request's 'Origin' value. Result: This addition enables the echoing of the request 'Origin' value as the 'Access-Control-Allow-Origin' value when the server has been configured to allow any origin in combination with allowCredentials. This allows client requests to succeed when expecting the server to be able to handle "*" origin and at the same time be able to send cookies by setting 'xhr.withCredentials=true'. A concrete example of this is the SockJS protocol which expects behaviour. --- .../handler/codec/http/cors/CorsHandler.java | 26 ++++++++++++++++--- .../codec/http/cors/CorsHandlerTest.java | 16 ++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/cors/CorsHandler.java b/codec-http/src/main/java/io/netty/handler/codec/http/cors/CorsHandler.java index abb08fbe52..9c5f716f2f 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/cors/CorsHandler.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/cors/CorsHandler.java @@ -89,11 +89,17 @@ public class CorsHandler extends ChannelDuplexHandler { return true; } if (config.isAnyOriginSupported()) { - setAnyOrigin(response); + if (config.isCredentialsAllowed()) { + echoRequestOrigin(response); + setVaryHeader(response); + } else { + setAnyOrigin(response); + } return true; } if (config.origins().contains(origin)) { - response.headers().set(ACCESS_CONTROL_ALLOW_ORIGIN, origin); + setOrigin(response, origin); + setVaryHeader(response); return true; } logger.debug("Request origin [" + origin + "] was not among the configured origins " + config.origins()); @@ -101,8 +107,20 @@ public class CorsHandler extends ChannelDuplexHandler { return false; } - private static void setAnyOrigin(final HttpResponse response) { - response.headers().set(ACCESS_CONTROL_ALLOW_ORIGIN, "*"); + private void echoRequestOrigin(final HttpResponse response) { + setOrigin(response, request.headers().get(ORIGIN)); + } + + private void setVaryHeader(final HttpResponse response) { + response.headers().set(VARY, ORIGIN); + } + + private void setAnyOrigin(final HttpResponse response) { + setOrigin(response, "*"); + } + + private void setOrigin(final HttpResponse response, final String origin) { + response.headers().set(ACCESS_CONTROL_ALLOW_ORIGIN, origin); } private void setAllowCredentials(final HttpResponse response) { diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/cors/CorsHandlerTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/cors/CorsHandlerTest.java index 45a91fdcd7..5f718c2122 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/cors/CorsHandlerTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/cors/CorsHandlerTest.java @@ -82,6 +82,7 @@ public class CorsHandlerTest { final HttpResponse response = preflightRequest(config, "http://localhost:8888", "content-type, xheader1"); assertThat(response.headers().get(ACCESS_CONTROL_ALLOW_ORIGIN), is("http://localhost:8888")); assertThat(response.headers().getAll(ACCESS_CONTROL_ALLOW_METHODS), hasItems("GET", "DELETE")); + assertThat(response.headers().get(VARY), equalTo(ORIGIN.toString())); } @Test @@ -94,6 +95,7 @@ public class CorsHandlerTest { assertThat(response.headers().get(ACCESS_CONTROL_ALLOW_ORIGIN), is("http://localhost:8888")); assertThat(response.headers().getAll(ACCESS_CONTROL_ALLOW_METHODS), hasItems("OPTIONS", "GET")); assertThat(response.headers().getAll(ACCESS_CONTROL_ALLOW_HEADERS), hasItems("content-type", "xheader1")); + assertThat(response.headers().get(VARY), equalTo(ORIGIN.toString())); } @Test @@ -102,6 +104,7 @@ public class CorsHandlerTest { final HttpResponse response = preflightRequest(config, "http://localhost:8888", "content-type, xheader1"); assertThat(response.headers().get(CONTENT_LENGTH), is("0")); assertThat(response.headers().get(DATE), is(notNullValue())); + assertThat(response.headers().get(VARY), equalTo(ORIGIN.toString())); } @Test @@ -111,6 +114,7 @@ public class CorsHandlerTest { .build(); final HttpResponse response = preflightRequest(config, "http://localhost:8888", "content-type, xheader1"); assertThat(response.headers().get("CustomHeader"), equalTo("somevalue")); + assertThat(response.headers().get(VARY), equalTo(ORIGIN.toString())); } @Test @@ -120,6 +124,7 @@ public class CorsHandlerTest { .build(); final HttpResponse response = preflightRequest(config, "http://localhost:8888", "content-type, xheader1"); assertThat(response.headers().getAll("CustomHeader"), hasItems("value1", "value2")); + assertThat(response.headers().get(VARY), equalTo(ORIGIN.toString())); } @Test @@ -129,6 +134,7 @@ public class CorsHandlerTest { .build(); final HttpResponse response = preflightRequest(config, "http://localhost:8888", "content-type, xheader1"); assertThat(response.headers().getAll("CustomHeader"), hasItems("value1", "value2")); + assertThat(response.headers().get(VARY), equalTo(ORIGIN.toString())); } @Test @@ -142,6 +148,7 @@ public class CorsHandlerTest { }).build(); final HttpResponse response = preflightRequest(config, "http://localhost:8888", "content-type, xheader1"); assertThat(response.headers().get("GenHeader"), equalTo("generatedValue")); + assertThat(response.headers().get(VARY), equalTo(ORIGIN.toString())); } @Test @@ -190,6 +197,15 @@ public class CorsHandlerTest { assertThat(response.headers().contains(ACCESS_CONTROL_ALLOW_CREDENTIALS), is(false)); } + @Test + public void anyOriginAndAllowCredentialsShouldEchoRequestOrigin() { + final CorsConfig config = CorsConfig.withAnyOrigin().allowCredentials().build(); + final HttpResponse response = simpleRequest(config, "http://localhost:7777", ""); + assertThat(response.headers().get(ACCESS_CONTROL_ALLOW_CREDENTIALS), equalTo("true")); + assertThat(response.headers().get(ACCESS_CONTROL_ALLOW_ORIGIN), equalTo("http://localhost:7777")); + assertThat(response.headers().get(VARY), equalTo(ORIGIN.toString())); + } + @Test public void simpleRequestExposeHeaders() { final CorsConfig config = CorsConfig.withAnyOrigin().exposeHeaders("one", "two").build();