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

Jump to bottom

Fix preflight failure on unknown router paths #11376

Open
wants to merge 8 commits into
base: 4.7.x
Choose a base branch
from
+10 −38
Open

Fix preflight failure on unknown router paths #11376

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cecf207
fix: remove check on known routes to decide in a preflight request is ok
Nov 23, 2024
e7fbe9a
test: preflight should not fail on unknown router paths
Nov 23, 2024
3ae35be
test: previous test was failing for wrong reasons (unknown path, not …
Nov 23, 2024
b5ad5af
test: preflight should not fail for unknown routes
Nov 23, 2024
473a290
test: aligns test with its description
Nov 23, 2024
38f0a89
test: fixes test since preflight should not fail for unknown routes
Nov 23, 2024
7bb04d7
test: removes redundant test
Nov 23, 2024
4d8101b
refactor: simplify conditions in the CORS preflight validation
ThomasIommi Nov 24, 2024
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
19 changes: 2 additions & 17 deletions http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsFilterSpec.groovy
View file
Open in desktop
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
5 changes: 2 additions & 3 deletions http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsVersionSpec.groovy
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/NettyCorsSpec.groovy
View file
Open in desktop
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
9 changes: 2 additions & 7 deletions http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java
View file
Open in desktop
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,20 +494,15 @@ private MutableHttpResponse<?> handlePreflightRequest(@NonNull HttpRequest<?> re

@Nullable
private boolean validatePreflightRequest(@NonNull HttpRequest<?> request,
@NonNull CorsOriginConfiguration config) {
@NonNull CorsOriginConfiguration config) {
Optional<HttpMethod> methodToMatchOptional = validateMethodToMatch(request, config);
if (methodToMatchOptional.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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)) {
return false;
Expand Down