Fix default methods filtering in CORS plugin (#5103)#5633
Fix default methods filtering in CORS plugin (#5103)#5633Hasan mohamed (HMH-3080) wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates CORS preflight header generation to format the plugin's full ChangesCORS Preflight Allow-Methods Header
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ktor-server/ktor-server-tests/common/test/io/ktor/tests/server/plugins/CORSTest.kt (1)
1596-1622: ⚡ Quick winStrengthen test coverage by verifying all default methods are included.
The test name suggests comprehensive validation of default methods (GET, POST, HEAD) being included in the
Access-Control-Allow-Methodsheader, but currently only verifies thatPOSTis present. Since the PR objective is to fix default methods being filtered out, the test should explicitly assert that all three default CORS methods appear in the header.🧪 Suggested enhancement
assertEquals(HttpStatusCode.OK, response.status) val allowMethodsHeader = response.headers[HttpHeaders.AccessControlAllowMethods] assertNotNull(allowMethodsHeader, "Access-Control-Allow-Methods header should not be null") +assertTrue( + allowMethodsHeader.contains("GET"), + "Expected Access-Control-Allow-Methods to include GET, but got: $allowMethodsHeader" +) +assertTrue( + allowMethodsHeader.contains("HEAD"), + "Expected Access-Control-Allow-Methods to include HEAD, but got: $allowMethodsHeader" +) assertTrue( allowMethodsHeader.contains("POST"), "Expected Access-Control-Allow-Methods to include POST, but got: $allowMethodsHeader" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ktor-server/ktor-server-tests/common/test/io/ktor/tests/server/plugins/CORSTest.kt` around lines 1596 - 1622, Update the testDefaultMethodsIncludedInAllowMethodsHeader test to assert that the Access-Control-Allow-Methods header contains all default CORS methods (GET, POST, HEAD) rather than only POST: after obtaining allowMethodsHeader from response.headers[HttpHeaders.AccessControlAllowMethods], split and trim the header values (or use contains checks) and add assertions that "GET", "POST", and "HEAD" are present; keep existing setup (install(CORS) { allowMethod(HttpMethod.Post); anyHost() }, routing/post handler, and OPTIONS request) and only extend the assertions in this test to verify all three methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@ktor-server/ktor-server-tests/common/test/io/ktor/tests/server/plugins/CORSTest.kt`:
- Around line 1596-1622: Update the
testDefaultMethodsIncludedInAllowMethodsHeader test to assert that the
Access-Control-Allow-Methods header contains all default CORS methods (GET,
POST, HEAD) rather than only POST: after obtaining allowMethodsHeader from
response.headers[HttpHeaders.AccessControlAllowMethods], split and trim the
header values (or use contains checks) and add assertions that "GET", "POST",
and "HEAD" are present; keep existing setup (install(CORS) {
allowMethod(HttpMethod.Post); anyHost() }, routing/post handler, and OPTIONS
request) and only extend the assertions in this test to verify all three
methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 325e4e76-d79d-4fab-82b7-5440b79e1f55
📒 Files selected for processing (2)
ktor-server/ktor-server-plugins/ktor-server-cors/common/src/io/ktor/server/plugins/cors/CORS.ktktor-server/ktor-server-tests/common/test/io/ktor/tests/server/plugins/CORSTest.kt
Bruce Hamilton (bjhham)
left a comment
Bruce Hamilton (bjhham)
left a comment
There was a problem hiding this comment.
Thanks!
Please run ./gradlew formatKotlin and submit the result to make the linter happy.
|
Bruce Hamilton (@bjhham) |
Bruce Hamilton (bjhham)
left a comment
Bruce Hamilton (bjhham)
left a comment
There was a problem hiding this comment.
I just noticed there was already a PR submitted for this a while ago 😅
|
Thanks for pointing that out! I looked into #5104 and noticed it’s been open since last year. Also, it introduces an unnecessary .sorted() call on every preflight request computation, which adds a tiny but avoidable overhead. My PR keeps the code simpler, includes a robust unit test for this case, and I've already formatted the code as you requested. I’d love it if we could move forward with this cleaner implementation if possible! 🙌 |
| .map { it.value } | ||
| .sorted() | ||
| .joinToString(", ") | ||
| val methodsListHeaderValue = methods.map { it.value }.sorted().joinToString(", ") |
There was a problem hiding this comment.
One thing I noticed in the other PR is they included a distinct() here - wondering if we'll end up with duplicates?
| assertTrue( | ||
| allowMethodsHeader.contains("POST"), | ||
| "Expected Access-Control-Allow-Methods to include POST, but got: $allowMethodsHeader" | ||
| ) |
There was a problem hiding this comment.
I think we should have an assertEquals here to ensure the default methods are included as well in a predictable way.
|
Bruce Hamilton (@bjhham) As for the predictability, I agree. I will add back the .sorted() call to ensure a consistent header value order, and update the test case to use assertEquals with the expected full string. Pushing the updates shortly |
2 participants
Fixes #5103
This PR ensures that default HTTP methods (
GET,POST,HEAD) are included explicitly in theAccess-Control-Allow-Methodsheader instead of being filtered out. Modern browsers strictly require these methods to be present in the preflight (OPTIONS) response header when custom configurations (like non-simple Content-Types) trigger a preflight validation.Changes :
.filterNot { it in CORSConfig.CorsDefaultMethods }frommethodsListHeaderValuecomputation.testDefaultMethodsIncludedInAllowMethodsHeaderto verify the fix.