Fix preflight failure on unknown router paths #11376

ThomasIommi · 2024-11-23T18:52:39Z

Hi!

I've tried to propose a fix for this issue #11375 and to update tests.
Let me know if there are any suggestions or feedback. I'm open to any improvements or discussions.

Thanks!

CLAassistant · 2024-11-23T18:52:45Z

All committers have signed the CLA.

…cors config)

glorrian · 2024-11-23T23:05:02Z

what the motivation for this pr? i think server shouldn't response 200 on non existing route

ThomasIommi · 2024-11-24T02:12:54Z

Hi!

In the current implementation, OPTIONS requests are being blocked with 403 Forbidden errors, even when the routes do exist but are not explicitly known by the router. This occurs, for example, in scenarios where the server acts as a proxy using a @Filter, as outlined in the official documentation ((https://docs.micronaut.io/latest/guide/#proxyClient)).

I believe (though I might be mistaken) that it’s not the responsibility of the CorsFilter to block such requests at this stage. The original CORS request should be blocked if the server fails to handle it, not the preflight OPTIONS request.

I encountered this issue after upgrading from Micronaut 4.6.X to 4.7.X and traced it back to this PR: #11097.

Let me know if you disagree. If there’s another approach to account for routes handled by @Filters, I’d be happy to adjust this PR accordingly.

glorrian · 2024-11-24T14:13:39Z

I am confused by the tests that you deleted "A preflight request is rejected for a non-existing route" and "A preflight request is rejected for a route that does exist but doesn't handle the requested HTTP Method" because they must pass. if you fix some functionality, you should at least leave all previous tests valid, but it's better to add new ones

ThomasIommi · 2024-11-24T14:42:47Z

Have you reviewed the PR I mentioned (#11097)?

I believe it introduced a regression. Additionally, there were changes to the tests in the PR itself, and I don't think I can make those tests pass since I'm essentially saying they don't represent the correct behavior.

I'm specifically referring to parts like this one:

Sorry for the screenshot I couldn't figure out how to link to this diff 😅.

glorrian · 2024-11-24T16:25:33Z

yeah, thanks to the link on pr, i got you.

glorrian · 2024-11-24T16:25:48Z

http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsVersionSpec.groovy

@@ -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)}


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

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.

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

glorrian · 2024-11-24T16:36:45Z

http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java

@@ -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()) {


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

yawkat · 2024-12-03T07:35:10Z

http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsFilterSpec.groovy

-
-    @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"() {


We need this feature.

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.

yawkat · 2024-12-03T08:06:25Z

I'm not sure I like this change. It is not against spec from what I can tell, but it does weaken the filtering we have.

A simple workaround would be to make your filter pre-routing as well, since you clearly don't need route information.

Other options for this PR would be to either add a config option to disable CORS preflight handling for some paths, or a config option to disable the route requirement entirely like you do in this PR.

dstepanov · 2024-12-03T09:54:40Z

I think we should extract the is route exists logic to some interface and allow the addition of an extra checker.

ThomasIommi · 2024-12-03T09:58:25Z

@yawkat
Doesn't the workaround you propose still invalidate the use of @Filter to implement a gateway proxy?

In a similar scenario to this use case (https://docs.micronaut.io/latest/guide/#proxyClient), if I move the proxy filter before the CORS filter and find a way to bypass it, I believe I would then need to handle CORS in every backend behind the gateway. That doesn’t seem ideal. I hope this makes sense, and that I'm not overlooking something.

I still think the responsibility of the CORS filter for preflight requests is to simply validate the CORS configuration, not the routes. Of course, I could be wrong, but I thought we were on the same page based on the comments in the PR I'm trying to address; did I misunderstand, or has something changed your perspective?

Just curious! Maybe I'm missing some background. 😛

ThomasIommi · 2024-12-03T10:20:02Z

I like the idea of using a configuration to enforce or disable route requirements. I can try to update the code accordingly as soon as possible.

dstepanov · 2024-12-03T12:13:33Z

I would not disable validation; I would add a new check to validate the route's validity.

Thomas Iommi added 7 commits November 23, 2024 20:02

fix: remove check on known routes to decide in a preflight request is ok

cecf207

test: preflight should not fail on unknown router paths

e7fbe9a

test: previous test was failing for wrong reasons (unknown path, not …

3ae35be

…cors config)

test: preflight should not fail for unknown routes

b5ad5af

test: aligns test with its description

473a290

test: fixes test since preflight should not fail for unknown routes

38f0a89

test: removes redundant test

7bb04d7

ThomasIommi force-pushed the fix-preflight-failure-on-unknown-router-paths branch from a3f933d to 7bb04d7 Compare November 23, 2024 19:03

glorrian reviewed Nov 24, 2024

View reviewed changes

refactor: simplify conditions in the CORS preflight validation

4d8101b

glorrian approved these changes Nov 24, 2024

View reviewed changes

graemerocher requested a review from yawkat November 25, 2024 13:04

sdelamo requested a review from dstepanov November 27, 2024 11:25

yawkat reviewed Dec 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix preflight failure on unknown router paths #11376

Fix preflight failure on unknown router paths #11376

ThomasIommi commented Nov 23, 2024

CLAassistant commented Nov 23, 2024 •

edited

Loading

glorrian commented Nov 23, 2024

ThomasIommi commented Nov 24, 2024 •

edited

Loading

glorrian commented Nov 24, 2024

ThomasIommi commented Nov 24, 2024

glorrian commented Nov 24, 2024

glorrian Nov 24, 2024

ThomasIommi Nov 24, 2024

glorrian Nov 24, 2024

glorrian Nov 24, 2024

yawkat Dec 3, 2024

ThomasIommi Dec 3, 2024

yawkat commented Dec 3, 2024

dstepanov commented Dec 3, 2024

ThomasIommi commented Dec 3, 2024 •

edited

Loading

ThomasIommi commented Dec 3, 2024

dstepanov commented Dec 3, 2024

Fix preflight failure on unknown router paths #11376

Are you sure you want to change the base?

Fix preflight failure on unknown router paths #11376

Conversation

ThomasIommi commented Nov 23, 2024

CLAassistant commented Nov 23, 2024 • edited Loading

glorrian commented Nov 23, 2024

ThomasIommi commented Nov 24, 2024 • edited Loading

glorrian commented Nov 24, 2024

ThomasIommi commented Nov 24, 2024

glorrian commented Nov 24, 2024

glorrian Nov 24, 2024

Choose a reason for hiding this comment

ThomasIommi Nov 24, 2024

Choose a reason for hiding this comment

glorrian Nov 24, 2024

Choose a reason for hiding this comment

glorrian Nov 24, 2024

Choose a reason for hiding this comment

yawkat Dec 3, 2024

Choose a reason for hiding this comment

ThomasIommi Dec 3, 2024

Choose a reason for hiding this comment

yawkat commented Dec 3, 2024

dstepanov commented Dec 3, 2024

ThomasIommi commented Dec 3, 2024 • edited Loading

ThomasIommi commented Dec 3, 2024

dstepanov commented Dec 3, 2024

CLAassistant commented Nov 23, 2024 •

edited

Loading

ThomasIommi commented Nov 24, 2024 •

edited

Loading

ThomasIommi commented Dec 3, 2024 •

edited

Loading