-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: 4.7.x
Are you sure you want to change the base?
Changes from all commits
cecf207
e7fbe9a
3ae35be
b5ad5af
473a290
38f0a89
7bb04d7
4d8101b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"() { | ||
|
@@ -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)} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what the reason of "x-api-version" here? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Before the proposed changes, I believe the test passed for the wrong reason — the I think the issue is related to the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this feature.
There was a problem hiding this comment.
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.