Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update OptionsFilter to not emit duplicate headers #10126

Merged
merged 2 commits into from
Nov 21, 2023
Merged

Update OptionsFilter to not emit duplicate headers #10126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb02ef0
Update OptionsFilter to not emit duplicate headers
jeremyg484 Nov 15, 2023
1d3b109
Fix OptionsFilterSpec
jeremyg484 Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions ...ient-jdk/src/test/groovy/io/micronaut/http/client/jdk/OptionsRequestAttributesSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package io.micronaut.http.client.jdk

import io.micronaut.context.ApplicationContext
import io.micronaut.context.annotation.Requires
import io.micronaut.core.type.Argument
import io.micronaut.core.util.StringUtils
import io.micronaut.http.HttpAttributes
import io.micronaut.http.HttpHeaders
Expand Down Expand Up @@ -52,11 +53,15 @@ class OptionsRequestAttributesSpec extends Specification {
then:
noExceptionThrown()
response.status == HttpStatus.OK
response.getHeaders().getAll(HttpHeaders.ALLOW)
3 == response.getHeaders().getAll(HttpHeaders.ALLOW).size()
response.getHeaders().getAll(HttpHeaders.ALLOW).contains('GET')
response.getHeaders().getAll(HttpHeaders.ALLOW).contains('OPTIONS')
response.getHeaders().getAll(HttpHeaders.ALLOW).contains('HEAD')

when:
List<String> allowedMethods = response.getHeaders().get(HttpHeaders.ALLOW, Argument.of(List.class, Argument.of(String.class))).orElse(new ArrayList<>())

then:
3 == allowedMethods.size()
allowedMethods.contains('GET')
allowedMethods.contains('OPTIONS')
allowedMethods.contains('HEAD')

cleanup:
ctx.close()
Expand Down
22 changes: 16 additions & 6 deletions ...-netty/src/test/groovy/io/micronaut/http/server/netty/OptionsRequestAttributesSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,14 @@ package io.micronaut.http.server.netty

import io.micronaut.context.ApplicationContext
import io.micronaut.context.annotation.Requires
import io.micronaut.core.type.Argument
import io.micronaut.core.util.StringUtils
import io.micronaut.http.*
import io.micronaut.http.HttpAttributes
import io.micronaut.http.HttpHeaders
import io.micronaut.http.HttpRequest
import io.micronaut.http.HttpResponse
import io.micronaut.http.HttpStatus
import io.micronaut.http.MutableHttpResponse
import io.micronaut.http.annotation.Controller
import io.micronaut.http.annotation.Filter
import io.micronaut.http.annotation.Get
Expand Down Expand Up @@ -47,11 +53,15 @@ class OptionsRequestAttributesSpec extends Specification {
then:
noExceptionThrown()
response.status == HttpStatus.OK
response.getHeaders().getAll(HttpHeaders.ALLOW)
3 == response.getHeaders().getAll(HttpHeaders.ALLOW).size()
response.getHeaders().getAll(HttpHeaders.ALLOW).contains('GET')
response.getHeaders().getAll(HttpHeaders.ALLOW).contains('OPTIONS')
response.getHeaders().getAll(HttpHeaders.ALLOW).contains('HEAD')

when:
List<String> allowedMethods = response.getHeaders().get(HttpHeaders.ALLOW, Argument.of(List.class, Argument.of(String.class))).orElse(new ArrayList<>())

then:
3 == allowedMethods.size()
allowedMethods.contains('GET')
allowedMethods.contains('OPTIONS')
allowedMethods.contains('HEAD')

cleanup:
ctx.close()
Expand Down
22 changes: 15 additions & 7 deletions ...ck/src/main/java/io/micronaut/http/server/tck/tests/filter/options/OptionsFilterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@
import io.micronaut.http.HttpMethod;
import io.micronaut.http.HttpRequest;
import io.micronaut.http.HttpStatus;
import io.micronaut.http.annotation.*;
import io.micronaut.http.annotation.Controller;
import io.micronaut.http.annotation.Get;
import io.micronaut.http.annotation.Options;
import io.micronaut.http.annotation.Post;
import io.micronaut.http.annotation.Status;
import io.micronaut.http.tck.AssertionUtils;
import io.micronaut.http.tck.HttpResponseAssertion;
import io.micronaut.http.tck.ServerUnderTest;
Expand All @@ -30,9 +34,12 @@

import java.io.IOException;
import java.util.Collections;
import java.util.List;
import java.util.function.BiConsumer;

import static org.junit.jupiter.api.Assertions.*;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;

@SuppressWarnings({
"java:S5960", // We're allowed assertions, as these are used in tests only
Expand Down Expand Up @@ -87,11 +94,12 @@ public void optionsTest() throws IOException {
.assertResponse(httpResponse -> {
assertNotNull(httpResponse.getHeaders().get(HttpHeaders.ALLOW));
assertNotNull(httpResponse.getHeaders().getAll(HttpHeaders.ALLOW));
assertEquals(4, httpResponse.getHeaders().getAll(HttpHeaders.ALLOW).size());
assertTrue(httpResponse.getHeaders().getAll(HttpHeaders.ALLOW).stream().anyMatch(v -> v.equals(HttpMethod.GET.toString())));
assertTrue(httpResponse.getHeaders().getAll(HttpHeaders.ALLOW).stream().anyMatch(v -> v.equals(HttpMethod.POST.toString())));
assertTrue(httpResponse.getHeaders().getAll(HttpHeaders.ALLOW).stream().anyMatch(v -> v.equals(HttpMethod.OPTIONS.toString())));
assertTrue(httpResponse.getHeaders().getAll(HttpHeaders.ALLOW).stream().anyMatch(v -> v.equals(HttpMethod.HEAD.toString())));
List<String> allowedMethods = httpResponse.getHeaders().get(HttpHeaders.ALLOW, List.class).orElseThrow();
assertEquals(4, allowedMethods .size());
assertTrue(allowedMethods.contains(HttpMethod.GET.toString()));
assertTrue(allowedMethods.contains(HttpMethod.POST.toString()));
assertTrue(allowedMethods.contains(HttpMethod.OPTIONS.toString()));
assertTrue(allowedMethods.contains(HttpMethod.HEAD.toString()));
})
.build()));
}
Expand Down
49 changes: 25 additions & 24 deletions http-server/src/main/java/io/micronaut/http/server/OptionsFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,29 @@
import io.micronaut.core.annotation.Nullable;
import io.micronaut.core.order.Ordered;
import io.micronaut.core.util.StringUtils;
import io.micronaut.http.*;
import io.micronaut.http.annotation.RequestFilter;
import io.micronaut.http.HttpAttributes;
import io.micronaut.http.HttpHeaders;
import io.micronaut.http.HttpMethod;
import io.micronaut.http.HttpRequest;
import io.micronaut.http.HttpResponse;
import io.micronaut.http.HttpStatus;
import io.micronaut.http.MutableHttpResponse;
import io.micronaut.http.annotation.ResponseFilter;
import io.micronaut.http.annotation.ServerFilter;
import io.micronaut.http.server.cors.CorsUtil;
import io.micronaut.web.router.Router;
import io.micronaut.web.router.UriRouteMatch;
import io.micronaut.web.router.RouteMatch;
import io.micronaut.web.router.UriRouteMatch;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import static io.micronaut.http.annotation.Filter.MATCH_ALL_PATTERN;
import static io.micronaut.http.server.cors.CorsFilter.CORS_FILTER_ORDER;

/**
* This Filter intercepts HTTP OPTIONS requests which are not CORS Preflight requests.
* It responds with a NO_CONTENT(204) response, and it populates the Allow HTTP Header with the supported HTTP methods for the request URI.
* It responds with an OK(200) response, and it populates the Allow HTTP Header with the supported HTTP methods for the request URI.
* @author Sergio del Amo
* @since 4.2.0
*/
Expand All @@ -45,20 +54,10 @@ public final class OptionsFilter implements Ordered {
@SuppressWarnings("WeakerAccess")
public static final String PREFIX = HttpServerConfiguration.PREFIX + ".dispatch-options-requests";

private final Router router;

/**
*
* @param router Router
*/
public OptionsFilter(Router router) {
this.router = router;
}

@RequestFilter
@ResponseFilter
@Nullable
@Internal
public HttpResponse<?> filterRequest(HttpRequest<?> request) {
public HttpResponse<?> filterResponse(HttpRequest<?> request, MutableHttpResponse<?> response) {
if (request.getMethod() != HttpMethod.OPTIONS) {
return null; // proceed
}
Expand All @@ -68,13 +67,15 @@ public HttpResponse<?> filterRequest(HttpRequest<?> request) {
if (hasOptionsRouteMatch(request)) {
return null; // proceed
}
MutableHttpResponse<?> mutableHttpResponse = HttpResponse.status(HttpStatus.OK);
router.findAny(request.getUri().toString(), request)
.map(UriRouteMatch::getHttpMethod)
.map(HttpMethod::toString)
.forEach(allow -> mutableHttpResponse.header(HttpHeaders.ALLOW, allow));
mutableHttpResponse.header(HttpHeaders.ALLOW, HttpMethod.OPTIONS.toString());
return mutableHttpResponse;
if (HttpStatus.METHOD_NOT_ALLOWED.equals(response.getStatus())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at this point might as well move the entire logic into RequestLifecycle or wherever METHOD_NOT_ALLOWED is thrown...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yawkat Yes, I had that same thought. It happens in RequestLifecycle#onRouteMiss.

My main concern, and reason for not doing that yet, is that onRouteMiss executes before runWithFilters and the original intention of this filter to avoid conflicting with the CORS filter.

That said, it occurs to me now that the onRouteMiss logic must be getting executed anyway (and the Allow header getting populated) even in the case of CORS being enabled. Thus in a Servlet environment, we are likely sending the Allow header even in response to a CORS pre-flight request (alongside all the other headers that CORS adds) because of the same issue of not being able to create a new ServletResponse in the middle of request processing.

List<String> allowedMethods = response.getHeaders().get(HttpHeaders.ALLOW, String[].class)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we have a getAll method to simplify this

Copy link
Contributor Author

@jeremyg484 jeremyg484 Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, getAll makes it more complicated because it does not do any parsing of the values, it just returns the raw value strings. So in this case, the header has already been written in RequestLifecycle as something like Allow: GET,POST,HEAD. getAll then returns a list with a single String value of "GET,POST,HEAD". Thus to produce this same result we would need conditional logic to check whether that String is empty to determine whether to add a comma before appending OPTIONS to it.

.map(allow -> new ArrayList<>(Arrays.asList(allow))).orElse(new ArrayList<>());
allowedMethods.add(HttpMethod.OPTIONS.toString());
response.getHeaders().remove(HttpHeaders.ALLOW);
response.getHeaders().allowGeneric(allowedMethods);
response.status(HttpStatus.OK);
}
return response;
}

private boolean hasOptionsRouteMatch(HttpRequest<?> request) {
Expand Down
3 changes: 1 addition & 2 deletions http-server/src/test/groovy/io/micronaut/http/server/OptionsFilterSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@ import io.micronaut.core.order.OrderUtil
import io.micronaut.core.order.Ordered
import io.micronaut.http.server.cors.CorsFilter
import io.micronaut.http.server.util.HttpHostResolver
import io.micronaut.web.router.Router
import spock.lang.Specification

class OptionsFilterSpec extends Specification {

void "OptionsFilter after CorsFilter"() {
given:
OptionsFilter optionsFilter = new OptionsFilter(Mock(Router))
OptionsFilter optionsFilter = new OptionsFilter()
CorsFilter corsFilter = new CorsFilter(Mock(HttpServerConfiguration.CorsConfiguration), Mock(HttpHostResolver))

when:
Expand Down
Loading