From ba71a16ae8f9ca8b74e84a7003951cc43ff0d310 Mon Sep 17 00:00:00 2001 From: Sergio del Amo Date: Fri, 9 Dec 2022 13:08:40 +0100 Subject: [PATCH 1/4] refactor: refactor CorsFilter --- .../server/netty/cors/CorsFilterSpec.groovy | 2 - .../http/server/cors/CorsFilter.java | 271 ++++++++++-------- 2 files changed, 155 insertions(+), 118 deletions(-) diff --git a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsFilterSpec.groovy b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsFilterSpec.groovy index 7fad9272fa0..8553e89adb1 100644 --- a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsFilterSpec.groovy +++ b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsFilterSpec.groovy @@ -16,7 +16,6 @@ package io.micronaut.http.server.netty.cors import io.micronaut.context.ApplicationContext -import io.micronaut.core.annotation.Nullable import io.micronaut.core.async.publisher.Publishers import io.micronaut.core.util.StringUtils import io.micronaut.http.* @@ -26,7 +25,6 @@ import io.micronaut.http.filter.ServerFilterChain import io.micronaut.http.server.HttpServerConfiguration import io.micronaut.http.server.cors.CorsFilter import io.micronaut.http.server.cors.CorsOriginConfiguration -import io.micronaut.http.server.util.HttpHostResolver import io.micronaut.runtime.server.EmbeddedServer import io.micronaut.web.router.RouteMatch import io.micronaut.web.router.Router diff --git a/http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java b/http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java index a7039180c88..8ec8de08336 100644 --- a/http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java +++ b/http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java @@ -15,6 +15,8 @@ */ package io.micronaut.http.server.cors; +import io.micronaut.core.annotation.NonNull; +import io.micronaut.core.annotation.Nullable; import io.micronaut.core.async.publisher.Publishers; import io.micronaut.core.convert.ArgumentConversionContext; import io.micronaut.core.convert.ConversionContext; @@ -31,11 +33,15 @@ import io.micronaut.http.filter.ServerFilterChain; import io.micronaut.http.filter.ServerFilterPhase; import io.micronaut.http.server.HttpServerConfiguration; +import io.micronaut.http.server.util.HttpHostResolver; +import jakarta.inject.Inject; +import org.jetbrains.annotations.NotNull; import org.reactivestreams.Publisher; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.util.ArrayList; import java.util.List; -import java.util.Map; import java.util.Optional; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -43,6 +49,7 @@ import static io.micronaut.http.HttpAttributes.AVAILABLE_HTTP_METHODS; import static io.micronaut.http.HttpHeaders.*; +import static io.micronaut.http.annotation.Filter.MATCH_ALL_PATTERN; /** * Responsible for handling CORS requests and responses. @@ -51,13 +58,14 @@ * @author Graeme Rocher * @since 1.0 */ -@Filter("/**") +@Filter(MATCH_ALL_PATTERN) public class CorsFilter implements HttpServerFilter { - + private static final Logger LOG = LoggerFactory.getLogger(CorsFilter.class); private static final ArgumentConversionContext CONVERSION_CONTEXT_HTTP_METHOD = ImmutableArgumentConversionContext.of(HttpMethod.class); protected final HttpServerConfiguration.CorsConfiguration corsConfiguration; + /** * @param corsConfiguration The {@link CorsOriginConfiguration} instance */ @@ -67,19 +75,23 @@ public CorsFilter(HttpServerConfiguration.CorsConfiguration corsConfiguration) { @Override public Publisher> doFilter(HttpRequest request, ServerFilterChain chain) { - boolean originHeaderPresent = request.getHeaders().getOrigin().isPresent(); - if (originHeaderPresent) { - MutableHttpResponse response = handleRequest(request).orElse(null); - if (response != null) { - return Publishers.just(response); - } else { - return Publishers.then(chain.proceed(request), mutableHttpResponse -> { - handleResponse(request, mutableHttpResponse); - }); - } - } else { + String origin = request.getHeaders().getOrigin().orElse(null); + if (origin == null) { + LOG.trace("Http Header " + HttpHeaders.ORIGIN + " not present. Proceeding with the request."); return chain.proceed(request); } + CorsOriginConfiguration corsOriginConfiguration = getConfiguration(origin).orElse(null); + if (corsOriginConfiguration != null) { + if (CorsUtil.isPreflightRequest(request)) { + return handlePreflightRequest(request, chain, corsOriginConfiguration); + } + if (!validateMethodToMatch(request, corsOriginConfiguration).isPresent()) { + return forbidden(); + } + return Publishers.then(chain.proceed(request), resp -> decorateResponseWithHeaders(request, resp, corsOriginConfiguration)); + } + LOG.trace("CORS configuration not found for {} origin", origin); + return chain.proceed(request); } @Override @@ -92,35 +104,10 @@ public int getOrder() { * * @param request The {@link HttpRequest} object * @param response The {@link MutableHttpResponse} object + * @deprecated not used */ + @Deprecated protected void handleResponse(HttpRequest request, MutableHttpResponse response) { - HttpHeaders headers = request.getHeaders(); - Optional originHeader = headers.getOrigin(); - originHeader.ifPresent(requestOrigin -> { - - Optional optionalConfig = getConfiguration(requestOrigin); - - if (optionalConfig.isPresent()) { - CorsOriginConfiguration config = optionalConfig.get(); - - if (CorsUtil.isPreflightRequest(request)) { - headers.getFirst(ACCESS_CONTROL_REQUEST_METHOD, CONVERSION_CONTEXT_HTTP_METHOD) - .ifPresent(result -> setAllowMethods(result, response)); - - Optional> allowedHeaders = headers.get(ACCESS_CONTROL_REQUEST_HEADERS, ConversionContext.LIST_OF_STRING); - allowedHeaders.ifPresent(val -> - setAllowHeaders(val, response) - ); - - setMaxAge(config.getMaxAge(), response); - } - - setOrigin(requestOrigin, response); - setVary(response); - setExposeHeaders(config.getExposedHeaders(), response); - setAllowCredentials(config, response); - } - }); } /** @@ -128,54 +115,22 @@ protected void handleResponse(HttpRequest request, MutableHttpResponse res * * @param request The {@link HttpRequest} object * @return An optional {@link MutableHttpResponse}. The request should proceed normally if empty + * @deprecated Not used any more. */ - protected Optional> handleRequest(HttpRequest request) { - HttpHeaders headers = request.getHeaders(); - Optional originHeader = headers.getOrigin(); - if (originHeader.isPresent()) { - - String requestOrigin = originHeader.get(); - boolean preflight = CorsUtil.isPreflightRequest(request); - - Optional optionalConfig = getConfiguration(requestOrigin); - - if (optionalConfig.isPresent()) { - CorsOriginConfiguration config = optionalConfig.get(); - - HttpMethod requestMethod = request.getMethod(); - - List allowedMethods = config.getAllowedMethods(); - HttpMethod methodToMatch = preflight ? headers.getFirst(ACCESS_CONTROL_REQUEST_METHOD, CONVERSION_CONTEXT_HTTP_METHOD).orElse(requestMethod) : requestMethod; - - if (!isAnyMethod(allowedMethods)) { - if (allowedMethods.stream().noneMatch(method -> method.equals(methodToMatch))) { - return Optional.of(HttpResponse.status(HttpStatus.FORBIDDEN)); - } - } - - Optional> availableHttpMethods = (Optional>) request.getAttribute(AVAILABLE_HTTP_METHODS, new ArrayList().getClass()); - - if (preflight && availableHttpMethods.isPresent() && availableHttpMethods.get().stream().anyMatch(method -> method.equals(methodToMatch))) { - Optional> accessControlHeaders = headers.get(ACCESS_CONTROL_REQUEST_HEADERS, ConversionContext.LIST_OF_STRING); - - List allowedHeaders = config.getAllowedHeaders(); - - if (!isAny(allowedHeaders) && accessControlHeaders.isPresent()) { - if (!accessControlHeaders.get().stream() - .allMatch(header -> allowedHeaders.stream() - .anyMatch(allowedHeader -> allowedHeader.equalsIgnoreCase(header.trim())))) { - return Optional.of(HttpResponse.status(HttpStatus.FORBIDDEN)); - } - } + @Deprecated + @NonNull + protected Optional> handleRequest(@NonNull HttpRequest request) { + return Optional.empty(); + } - MutableHttpResponse ok = HttpResponse.ok(); - handleResponse(request, ok); - return Optional.of(ok); - } - } + @NonNull + private Optional validateMethodToMatch(@NonNull HttpRequest request, + @NonNull CorsOriginConfiguration config) { + HttpMethod methodToMatch = methodToMatch(request); + if (!methodAllowed(config, methodToMatch)) { + return Optional.empty(); } - - return Optional.empty(); + return Optional.of(methodToMatch); } /** @@ -214,15 +169,17 @@ protected void setVary(MutableHttpResponse response) { * @param origin The origin * @param response The {@link MutableHttpResponse} object */ - protected void setOrigin(String origin, MutableHttpResponse response) { - response.header(ACCESS_CONTROL_ALLOW_ORIGIN, origin); + protected void setOrigin(@Nullable String origin, @NonNull MutableHttpResponse response) { + if (origin != null) { + response.header(ACCESS_CONTROL_ALLOW_ORIGIN, origin); + } } /** * @param method The {@link HttpMethod} object * @param response The {@link MutableHttpResponse} object */ - protected void setAllowMethods(HttpMethod method, MutableHttpResponse response) { + protected void setAllowMethods(HttpMethod method, MutableHttpResponse response) { response.header(ACCESS_CONTROL_ALLOW_METHODS, method); } @@ -230,7 +187,7 @@ protected void setAllowMethods(HttpMethod method, MutableHttpResponse response) * @param optionalAllowHeaders A list with optional allow headers * @param response The {@link MutableHttpResponse} object */ - protected void setAllowHeaders(List optionalAllowHeaders, MutableHttpResponse response) { + protected void setAllowHeaders(List optionalAllowHeaders, MutableHttpResponse response) { List allowHeaders = optionalAllowHeaders.stream().map(Object::toString).collect(Collectors.toList()); if (corsConfiguration.isSingleHeader()) { String headerValue = String.join(",", allowHeaders); @@ -249,38 +206,28 @@ protected void setAllowHeaders(List optionalAllowHeaders, MutableHttpResponse * @param maxAge The max age * @param response The {@link MutableHttpResponse} object */ - protected void setMaxAge(long maxAge, MutableHttpResponse response) { + protected void setMaxAge(long maxAge, MutableHttpResponse response) { if (maxAge > -1) { response.header(ACCESS_CONTROL_MAX_AGE, Long.toString(maxAge)); } } - private Optional getConfiguration(String requestOrigin) { - Map corsConfigurations = corsConfiguration.getConfigurations(); - for (Map.Entry config : corsConfigurations.entrySet()) { - List allowedOrigins = config.getValue().getAllowedOrigins(); - if (!allowedOrigins.isEmpty()) { - boolean matches = false; - if (isAny(allowedOrigins)) { - matches = true; - } - if (!matches) { - matches = allowedOrigins.stream().anyMatch(origin -> { - if (origin.equals(requestOrigin)) { - return true; - } - Pattern p = Pattern.compile(origin); - Matcher m = p.matcher(requestOrigin); - return m.matches(); - }); - } - - if (matches) { - return Optional.of(config.getValue()); - } - } + @NonNull + private Optional getConfiguration(@NonNull String requestOrigin) { + return corsConfiguration.getConfigurations().values().stream() + .filter(config -> { + List allowedOrigins = config.getAllowedOrigins(); + return !allowedOrigins.isEmpty() && (isAny(allowedOrigins) || allowedOrigins.stream().anyMatch(origin -> matchesOrigin(origin, requestOrigin))); + }).findFirst(); + } + + private boolean matchesOrigin(@NonNull String origin, @NonNull String requestOrigin) { + if (origin.equals(requestOrigin)) { + return true; } - return Optional.empty(); + Pattern p = Pattern.compile(origin); + Matcher m = p.matcher(requestOrigin); + return m.matches(); } private boolean isAny(List values) { @@ -290,4 +237,96 @@ private boolean isAny(List values) { private boolean isAnyMethod(List allowedMethods) { return allowedMethods == CorsOriginConfiguration.ANY_METHOD; } + + private boolean methodAllowed(@NonNull CorsOriginConfiguration config, + @NonNull HttpMethod methodToMatch) { + List allowedMethods = config.getAllowedMethods(); + return isAnyMethod(allowedMethods) || allowedMethods.stream().anyMatch(method -> method.equals(methodToMatch)); + } + + @NonNull + private HttpMethod methodToMatch(@NonNull HttpRequest request) { + HttpMethod requestMethod = request.getMethod(); + return CorsUtil.isPreflightRequest(request) ? request.getHeaders().getFirst(ACCESS_CONTROL_REQUEST_METHOD, CONVERSION_CONTEXT_HTTP_METHOD).orElse(requestMethod) : requestMethod; + } + + private boolean hasAllowedHeaders(@NonNull HttpRequest request, @NonNull CorsOriginConfiguration config) { + Optional> accessControlHeaders = request.getHeaders().get(ACCESS_CONTROL_REQUEST_HEADERS, ConversionContext.LIST_OF_STRING); + List allowedHeaders = config.getAllowedHeaders(); + return isAny(allowedHeaders) || ( + accessControlHeaders.isPresent() && + accessControlHeaders.get().stream().allMatch(header -> allowedHeaders.stream().anyMatch(allowedHeader -> allowedHeader.equalsIgnoreCase(header.trim()))) + ); + } + + @NotNull + private static Publisher> forbidden() { + return Publishers.just(HttpResponse.status(HttpStatus.FORBIDDEN)); + } + + @NonNull + private void decorateResponseWithHeadersForPreflightRequest(@NonNull HttpRequest request, + @NonNull MutableHttpResponse response, + @NonNull CorsOriginConfiguration config) { + HttpHeaders headers = request.getHeaders(); + headers.getFirst(ACCESS_CONTROL_REQUEST_METHOD, CONVERSION_CONTEXT_HTTP_METHOD) + .ifPresent(methods -> setAllowMethods(methods, response)); + headers.get(ACCESS_CONTROL_REQUEST_HEADERS, ConversionContext.LIST_OF_STRING) + .ifPresent(val -> setAllowHeaders(val, response)); + setMaxAge(config.getMaxAge(), response); + } + + @NonNull + private void decorateResponseWithHeaders(@NonNull HttpRequest request, + @NonNull MutableHttpResponse response, + @NonNull CorsOriginConfiguration config) { + HttpHeaders headers = request.getHeaders(); + setOrigin(headers.getOrigin().orElse(null), response); + setVary(response); + setExposeHeaders(config.getExposedHeaders(), response); + setAllowCredentials(config, response); + } + + @NonNull + private Publisher> handlePreflightRequest(@NonNull HttpRequest request, + @NonNull ServerFilterChain chain, + @NonNull CorsOriginConfiguration corsOriginConfiguration) { + Optional statusOptional = validatePreflightRequest(request, corsOriginConfiguration); + if (statusOptional.isPresent()) { + HttpStatus status = statusOptional.get(); + if (status.getCode() >= 400) { + return Publishers.just(HttpResponse.status(status)); + } + MutableHttpResponse resp = HttpResponse.status(status); + decorateResponseWithHeadersForPreflightRequest(request, resp, corsOriginConfiguration); + decorateResponseWithHeaders(request, resp, corsOriginConfiguration); + return Publishers.just(resp); + } + return Publishers.then(chain.proceed(request), resp -> { + decorateResponseWithHeadersForPreflightRequest(request, resp, corsOriginConfiguration); + decorateResponseWithHeaders(request, resp, corsOriginConfiguration); + }); + } + + + @NonNull + private Optional validatePreflightRequest(@NonNull HttpRequest request, + @NonNull CorsOriginConfiguration config) { + Optional methodToMatchOptional = validateMethodToMatch(request, config); + if (!methodToMatchOptional.isPresent()) { + return Optional.of(HttpStatus.FORBIDDEN); + } + HttpMethod methodToMatch = methodToMatchOptional.get(); + + Optional> availableHttpMethods = (Optional>) request.getAttribute(AVAILABLE_HTTP_METHODS, new ArrayList().getClass()); + if (CorsUtil.isPreflightRequest(request) && + availableHttpMethods.isPresent() && + availableHttpMethods.get().stream().anyMatch(method -> method.equals(methodToMatch))) { + if (!hasAllowedHeaders(request, config)) { + return Optional.of(HttpStatus.FORBIDDEN); + } + return Optional.of(HttpStatus.OK); + } + return Optional.empty(); + } } From eebf688a3cd58328a2a401b8534c0b9987cc0e44 Mon Sep 17 00:00:00 2001 From: Sergio del Amo Date: Fri, 9 Dec 2022 15:28:39 +0100 Subject: [PATCH 2/4] test: add enabled by default tests --- .../netty/cors/CorsFilterEnabledSpec.groovy | 19 +++++++++++++++++++ .../CorsOriginConverterEnabledSpec.groovy | 19 +++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsFilterEnabledSpec.groovy create mode 100644 http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsOriginConverterEnabledSpec.groovy diff --git a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsFilterEnabledSpec.groovy b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsFilterEnabledSpec.groovy new file mode 100644 index 00000000000..3a94796f892 --- /dev/null +++ b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsFilterEnabledSpec.groovy @@ -0,0 +1,19 @@ +package io.micronaut.http.server.netty.cors + +import io.micronaut.context.ApplicationContext +import io.micronaut.http.server.cors.CorsFilter +import spock.lang.AutoCleanup +import spock.lang.Shared +import spock.lang.Specification + +class CorsFilterEnabledSpec extends Specification { + + @AutoCleanup + @Shared + ApplicationContext applicationContext = ApplicationContext.run() + + void "CorsFilter is not enabled by default"() { + expect: + !applicationContext.containsBean(CorsFilter) + } +} diff --git a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsOriginConverterEnabledSpec.groovy b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsOriginConverterEnabledSpec.groovy new file mode 100644 index 00000000000..2f98a0bfca4 --- /dev/null +++ b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsOriginConverterEnabledSpec.groovy @@ -0,0 +1,19 @@ +package io.micronaut.http.server.netty.cors + +import io.micronaut.context.ApplicationContext +import io.micronaut.http.server.cors.CorsOriginConverter +import spock.lang.AutoCleanup +import spock.lang.Shared +import spock.lang.Specification + +class CorsOriginConverterEnabledSpec extends Specification { + + @AutoCleanup + @Shared + ApplicationContext applicationContext = ApplicationContext.run() + + void "CorsOriginConverter is not enabled by default"() { + expect: + !applicationContext.containsBean(CorsOriginConverter) + } +} From 72643bef628b618c952c8c242bb423a2b342627d Mon Sep 17 00:00:00 2001 From: Sergio del Amo Date: Fri, 9 Dec 2022 15:33:21 +0100 Subject: [PATCH 3/4] keep original method --- .../main/java/io/micronaut/http/server/cors/CorsFilter.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java b/http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java index 8ec8de08336..6d7d8fd9fcf 100644 --- a/http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java +++ b/http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java @@ -118,8 +118,7 @@ protected void handleResponse(HttpRequest request, MutableHttpResponse res * @deprecated Not used any more. */ @Deprecated - @NonNull - protected Optional> handleRequest(@NonNull HttpRequest request) { + protected Optional> handleRequest(HttpRequest request) { return Optional.empty(); } From 69f4b5c786d3ecb76d4aae3dd8e786a8c41ac2f9 Mon Sep 17 00:00:00 2001 From: Sergio del Amo Date: Fri, 9 Dec 2022 17:06:35 +0100 Subject: [PATCH 4/4] checkstyle: remove unused imports and blank lines --- .../main/java/io/micronaut/http/server/cors/CorsFilter.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java b/http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java index 6d7d8fd9fcf..ddc8d63a0e5 100644 --- a/http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java +++ b/http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java @@ -33,8 +33,6 @@ import io.micronaut.http.filter.ServerFilterChain; import io.micronaut.http.filter.ServerFilterPhase; import io.micronaut.http.server.HttpServerConfiguration; -import io.micronaut.http.server.util.HttpHostResolver; -import jakarta.inject.Inject; import org.jetbrains.annotations.NotNull; import org.reactivestreams.Publisher; import org.slf4j.Logger; @@ -65,7 +63,6 @@ public class CorsFilter implements HttpServerFilter { protected final HttpServerConfiguration.CorsConfiguration corsConfiguration; - /** * @param corsConfiguration The {@link CorsOriginConfiguration} instance */ @@ -307,7 +304,6 @@ private Publisher> handlePreflightRequest(@NonNull HttpRe }); } - @NonNull private Optional validatePreflightRequest(@NonNull HttpRequest request, @NonNull CorsOriginConfiguration config) {