[cors] Use JDK's URI class #2180
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2180 +/- ##
============================================
+ Coverage 86.54% 86.68% +0.13%
+ Complexity 1446 1435 -11
============================================
Files 153 153
Lines 4200 4184 -16
Branches 477 468 -9
============================================
- Hits 3635 3627 -8
+ Misses 369 364 -5
+ Partials 196 193 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tipsy
force-pushed
the
master
branch
4 times, most recently
from
765b7d0 to
6faa186
Compare
Playacem
force-pushed
the
gh-2138-cors-jdk-uri
branch
from
12e07b7 to
34371f6
Compare
Playacem
force-pushed
the
gh-2138-cors-jdk-uri
branch
from
34371f6 to
6d6d995
Compare
|
How is this PR going @Playacem ? |
|
I have been quite busy with real life, but I finally have some time this weekend to work on it. |
Me too, no rush |
Playacem
force-pushed
the
gh-2138-cors-jdk-uri
branch
from
6d6d995 to
1df3f7d
Compare
Playacem
force-pushed
the
gh-2138-cors-jdk-uri
branch
from
8740c5c to
4cdeb24
Compare
I guess it's ready for review? 😅 |
|
It is :) |
There was a problem hiding this comment.
Pull Request Overview
This pull request updates the CORS support to use JDK’s URI class for more robust origin parsing. Key changes include updating tests to use parameterized inputs for various origin scenarios (IPv4, IPv6, wildcards), refactoring CorsUtils to remove the PortResult abstraction in favor of boolean validations and exception handling, and revising CorsPlugin to integrate with the new utility methods.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| javalin/src/test/java/io/javalin/TestCorsUtils.kt | Replaces individual tests with parameterized tests and updates origin parsing expectations |
| javalin/src/test/java/io/javalin/TestCors.kt | Adds new tests for IPv4/IPv6 and wildcard handling and verifies error messages for invalid origins |
| javalin/src/main/java/io/javalin/plugin/bundled/CorsUtils.kt | Removes PortResult, adds URI‑based origin validation and parsing, and handles wildcard origins |
| javalin/src/main/java/io/javalin/plugin/bundled/CorsPlugin.kt | Updates usage of CorsUtils methods and refines error handling for origin validation |
| val wildcardSnippet = "://*." | ||
| val hasWildcard = wildcardSnippet in origin | ||
| if (client && hasWildcard) { | ||
| return false | ||
| } | ||
| val possiblePortDigits = origin.subSequence(possiblePortIndex + 1, origin.length).toString() | ||
| if (possiblePortDigits.any { !it.isAsciiDigit() }) { | ||
| return PortResult.ErrorState.InvalidPort | ||
| val originWithoutWildcard = origin.replace(wildcardSnippet, "://") |
There was a problem hiding this comment.
[nitpick] Consider handling possible variations of wildcard placement or formatting rather than relying solely on the literal substring "://*." to ensure robust matching.
| val wildcardSnippet = "://*." | |
| val hasWildcard = wildcardSnippet in origin | |
| if (client && hasWildcard) { | |
| return false | |
| } | |
| val possiblePortDigits = origin.subSequence(possiblePortIndex + 1, origin.length).toString() | |
| if (possiblePortDigits.any { !it.isAsciiDigit() }) { | |
| return PortResult.ErrorState.InvalidPort | |
| val originWithoutWildcard = origin.replace(wildcardSnippet, "://") | |
| val wildcardRegex = Regex("://\\*\\.") | |
| val hasWildcard = wildcardRegex.containsMatchIn(origin) | |
| if (client && hasWildcard) { | |
| return false | |
| } | |
| val originWithoutWildcard = origin.replace(wildcardRegex, "://") |
| return PortResult.ErrorState.InvalidPort | ||
| val originWithoutWildcard = origin.replace(wildcardSnippet, "://") | ||
| val originToAnalyze = if (hasWildcard) originWithoutWildcard else origin | ||
| try { |
There was a problem hiding this comment.
[nitpick] Consider catching more specific exceptions (e.g. URISyntaxException) here instead of a generic Exception to make error handling more explicit.
We need to specify junit-jupiter-params directly to force JUnit 6. We previously had this on our classpath as a transitive dependency from mockk, leading to a version mismatch and failing parameterized tests. We could also switch over to the junit-jupiter artifact which combines api, params and runtime.
|
Looks great @Playacem, thank you very much 🙌 |
2 participants
Todos:
Fixes #2138 (once its done)