-
Notifications
You must be signed in to change notification settings - Fork 1.2k
CORS Plugin: Access-Control-Allow-Methods header does not include default methods (GET, POST, HEAD) #5104
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
base: main
Are you sure you want to change the base?
Conversation
…l-Allow-Methods header
WalkthroughUpdated CORS methods header construction to include all configured HTTP methods (including defaults) in a sorted, comma-separated list. Added a test to verify that preflight responses include default methods when an explicit method is allowed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ktor-server/ktor-server-tests/common/test/io/ktor/tests/server/plugins/CORSTest.kt (1)
1596-1622: LGTM + make tokenization resilient to formattingTest correctly covers the regression. Minor: split by comma and trim to avoid coupling to a specific spacing in the header.
- val allowMethods = response.headers[HttpHeaders.AccessControlAllowMethods]?.split(", ")?.toSet() + val allowMethods = response.headers[HttpHeaders.AccessControlAllowMethods] + ?.split(',') + ?.map { it.trim() } + ?.toSet()ktor-server/ktor-server-plugins/ktor-server-cors/common/src/io/ktor/server/plugins/cors/CORS.kt (1)
57-60: Good fix; drop redundant distinct()Including default methods in the header is correct.
methodsis already aSet, sodistinct()is redundant.- val methodsListHeaderValue = methods.distinct() - .map { it.value } + val methodsListHeaderValue = methods + .map { it.value } .sorted() .joinToString(", ")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ktor-server/ktor-server-plugins/ktor-server-cors/common/src/io/ktor/server/plugins/cors/CORS.kt(1 hunks)ktor-server/ktor-server-tests/common/test/io/ktor/tests/server/plugins/CORSTest.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{kt,kts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{kt,kts}: Follow Kotlin official style guide for all Kotlin source and build scripts
Use star imports for io.ktor.* packages
Max line length is 120 characters
Indent with 4 spaces in Kotlin code
Include a copyright header in new Kotlin files
Files:
ktor-server/ktor-server-tests/common/test/io/ktor/tests/server/plugins/CORSTest.ktktor-server/ktor-server-plugins/ktor-server-cors/common/src/io/ktor/server/plugins/cors/CORS.kt
**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.kt: Document all public Kotlin APIs, including parameters, return types, and exceptions
Annotate internal APIs with @internalapi
Follow Kotlin error-handling conventions and use specific Ktor exceptions
Files:
ktor-server/ktor-server-tests/common/test/io/ktor/tests/server/plugins/CORSTest.ktktor-server/ktor-server-plugins/ktor-server-cors/common/src/io/ktor/server/plugins/cors/CORS.kt
bjhham
left a comment
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.
Yep, this sounds like how it ought to work:
By default, the CORS plugin allows the GET, POST and HEAD HTTP methods. To add additional methods, use the allowMethod function.
Thanks for the fix.
Could you please run ./gradlew formatKotlin to get past the linter check?
Fixes #5103
Problem
In Ktor 3.3.0, the CORS plugin does not correctly include the default HTTP methods (GET, POST, HEAD) in the Access-Control-Allow-Methods response header.
Current implementation in CORSConfig builds the header value like this:
val methodsListHeaderValue = methods.filterNot { it in CorsDefaultMethods } .map { it.value } .sorted() .joinToString(", ")Since CorsDefaultMethods = { GET, POST, HEAD }, these methods are filtered out and not included in the final header.
This results in browsers seeing an incomplete Access-Control-Allow-Methods (often only OPTIONS), which breaks CORS preflight validation.
Solution
Replaced filtering logic with a distinct collection:
val methodsListHeaderValue = methods.distinct() .map { it.value } .sorted() .joinToString(", ")This ensures that the default methods (GET, POST, HEAD) are included along with any additional configured methods.
Tests
Added a new test testPreflightIncludesDefaultMethods to verify that:
GET, POST, and HEAD are always present in Access-Control-Allow-Methods
Custom allowed methods (e.g., PUT) are also included
Before the fix, this test fails. After the fix, it passes ✅
Environment
Ktor version: 3.3.0
Module: ktor-server-cors
JVM: 17
OS: Ubuntu 22.04 / Windows 11