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.

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

Fix preflight failure on unknown router paths #11376

Open
wants to merge 8 commits into
base: 4.7.x
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ class CorsFilterSpec extends Specification {

@Property(name = "micronaut.server.cors.configurations.foo.allowed-origins", value = "http://www.foo.com")
@Property(name = "micronaut.server.cors.configurations.foo.allowed-methods", value = "GET")
void "A preflight request is rejected for a non-existing route"() {
void "A preflight request is NOT rejected for a non-existing route if CORS configuration is valid"() {
given:
HttpRequest request = HttpRequest.OPTIONS("/doesnt-exists-route")
.header(HttpHeaders.ORIGIN, 'http://www.foo.com')
Expand All @@ -311,22 +311,7 @@ class CorsFilterSpec extends Specification {
HttpResponse<?> response = execute(request)

then:
HttpStatus.FORBIDDEN == response.status()
}

@Property(name = "micronaut.server.cors.configurations.foo.allowed-origins", value = "http://www.foo.com")
@Property(name = "micronaut.server.cors.configurations.foo.exposed-headers", value = "Foo-Header,Bar-Header")
void "A preflight request is rejected for a route that does exist but doesn't handle the requested HTTP Method"() {
Copy link
Member

Choose a reason for hiding this comment

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

We need this feature.

Copy link
Author

Choose a reason for hiding this comment

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

Perhaps we could handle it as a special case: if the route exists, we perform an additional check on the HTTP methods it supports. However, if the route is not found, we make no assumptions. Honestly, I find this approach a bit trickier. For me, it’s simpler to treat it as if the route it is not known / found.

given:
HttpRequest request = HttpRequest.OPTIONS("/example")
.header(HttpHeaders.ORIGIN, 'http://www.foo.com')
.header(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, 'POST')

when:
HttpResponse<?> response = execute(request)

then:
HttpStatus.FORBIDDEN == response.status()
HttpStatus.OK == response.status()
}

@Requires(property = "spec.name", value = "CorsFilterSpec")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,7 @@ class CorsVersionSpec extends Specification {
client.exchange(request)

then:
HttpClientResponseException ex = thrown()
ex.status == HttpStatus.FORBIDDEN
noExceptionThrown()
}

void "preflight for version routed from private network"() {
Expand All @@ -113,7 +112,7 @@ class CorsVersionSpec extends Specification {

when:
request = HttpRequest.OPTIONS("/new-not-allowed-from-private")
preflightHeaders(null, true).each { k, v -> request.header(k, v)}
preflightHeaders("x-api-version", true).each { k, v -> request.header(k, v)}
Copy link
Contributor

Choose a reason for hiding this comment

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

what the reason of "x-api-version" here?

Copy link
Author

Choose a reason for hiding this comment

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

While debugging, I discovered that without the "Access-Control-Request-Headers" header, the test failed to target the correct TestController endpoint (annotated with @CrossOrigin(allowPrivateNetwork = false) and @Version("2")).

Before the proposed changes, I believe the test passed for the wrong reason — the 403 FORBIDDEN response occurred because the route was not found, not due to the @CrossOrigin(allowPrivateNetwork = false) configuration.

I think the issue is related to the @Version("2") annotation since temporarily removing this annotation resolved the problem, but I then decided not to remove the annotation and add the header instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

ive seen to the code. this behaviour happens by the reason that matchesOrigin method at CorsFilter returns false if we defined Version annotation but request will not support this header cause accessControlRequestHeaders is null and then it responce 200 cause all cors requests return 200 by this pr. so i guess it test right, thanks

client.exchange(request)

then:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ class NettyCorsSpec extends AbstractMicronautSpec {
}).blockFirst()

expect:
response.code() == HttpStatus.FORBIDDEN.code
response.code() == HttpStatus.OK.code
}

void "test control headers are applied to error response routes"() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public class CorsFilter implements Ordered, ConditionalFilter {
/**
* @param corsConfiguration The {@link CorsOriginConfiguration} instance
* @param httpHostResolver HTTP Host resolver
* @deprecated use {@link CorsFilter(HttpServerConfiguration, HttpHostResolver, Router)} instead.
* @deprecated use {@link CorsFilter(HttpServerConfiguration, HttpHostResolver, Router)} instead.
*/
@Deprecated(since = "4.7", forRemoval = true)
public CorsFilter(HttpServerConfiguration.CorsConfiguration corsConfiguration,
Expand Down Expand Up @@ -494,22 +494,10 @@ private MutableHttpResponse<?> handlePreflightRequest(@NonNull HttpRequest<?> re

@Nullable
private boolean validatePreflightRequest(@NonNull HttpRequest<?> request,
@NonNull CorsOriginConfiguration config) {
Optional<HttpMethod> methodToMatchOptional = validateMethodToMatch(request, config);
if (methodToMatchOptional.isEmpty()) {
return false;
}
HttpMethod methodToMatch = methodToMatchOptional.get();

if (!CorsUtil.isPreflightRequest(request)) {
return false;
}
List<HttpMethod> availableHttpMethods = router.findAny(request).stream().map(UriRouteMatch::getHttpMethod).toList();
if (availableHttpMethods.stream().noneMatch(method -> method.equals(methodToMatch))) {
return false;
}

if (!hasAllowedHeaders(request, config)) {
@NonNull CorsOriginConfiguration config) {
if (validateMethodToMatch(request, config).isEmpty() ||
!CorsUtil.isPreflightRequest(request) ||
!hasAllowedHeaders(request, config)) {
return false;
}

Expand Down
Loading