Conversation
…ods are now loaded from the application configuration file instead of being hardcoded. No more hardcoded CORS rules in code. Makes it easier to add or change allowed origins/methods without changing code. Improves flexibility and maintainability for future CORS changes.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces hardcoded CORS values with runtime-configured values: CorsFilter now reads allowed origins and methods from ConfigLoader, which parses a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as AppStart
participant Config as ConfigLoader
participant Filter as CorsFilter
participant Server as Server
participant Client as Client
App->>Config: load application-properties.yml (cors.allowed-origins, cors.allowed-methods)
Config-->>App: return allowedOrigins, allowedMethods
App->>Filter: new CorsFilter() (reads from ConfigLoader singleton)
Filter-->>App: initialized with dynamic CORS values
Client->>Server: HTTP request (with Origin header)
Server->>Filter: evaluate origin & method
Filter->>Server: set CORS response headers (Access-Control-Allow-Origin/Methods based on config)
Server-->>Client: response (with CORS headers)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/resources/application-properties.yml (1)
19-28: CORS configuration looks good for development; consider adding common options for completeness.The refactor to externalize CORS settings is a solid improvement. A few suggestions:
Missing common CORS options: Consider adding
allowed-headers(e.g.,Content-Type,Authorization),max-agefor preflight caching, andallow-credentialsif cookie-based auth is needed.Production readiness: The
http://localhost:3000origin is fine for local development. Ensure there's a strategy for production (e.g., separate profile or environment variable override).💡 Example with additional options
cors: allowed-origins: - http://localhost:3000 allowed-methods: - GET - POST - PUT - PATCH - DELETE - OPTIONS + allowed-headers: + - Content-Type + - Authorization + max-age: 3600🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/application-properties.yml` around lines 19 - 28, Add missing common CORS options to the YAML by extending the cors block: add cors.allowed-headers (e.g., Content-Type, Authorization), cors.allow-credentials (true/false) and cors.max-age (seconds) so preflight caching, credential handling and allowed headers are explicit, and update configuration loading so cors.allowed-origins can be overridden per environment (use a profile or environment-variable-based override) to replace the hardcoded http://localhost:3000 for production; update any code that reads cors.* keys to use these new properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Line 9: Remove the unused import by deleting the line importing
java.util.stream.Collectors from the ConfigLoader class; locate the import
statement at the top of ConfigLoader.java (the import of Collectors) and remove
it so Spotless/CI no longer flags the unused import and the build can proceed.
- Around line 127-141: The CORS parsing in ConfigLoader reads corsConfig entries
into allowedOrigins and allowedMethods but doesn't normalize or filter them,
which lets blank strings and mixed-case methods slip through; update the parsing
for the "allowed-origins" and "allowed-methods" keys (in class ConfigLoader,
methods/fields allowedOrigins and allowedMethods, and variable corsConfig) to:
trim each entry and filter out empty/blank values for origins, and for methods
trim, filter blanks and normalize to uppercase (e.g., map(String::trim) ->
filter(not blank) -> map(s -> s.toUpperCase())); also ensure that if the value
is not a List<?> you fall back to safe defaults (empty list for origins and the
current default methods list for methods).
---
Nitpick comments:
In `@src/main/resources/application-properties.yml`:
- Around line 19-28: Add missing common CORS options to the YAML by extending
the cors block: add cors.allowed-headers (e.g., Content-Type, Authorization),
cors.allow-credentials (true/false) and cors.max-age (seconds) so preflight
caching, credential handling and allowed headers are explicit, and update
configuration loading so cors.allowed-origins can be overridden per environment
(use a profile or environment-variable-based override) to replace the hardcoded
http://localhost:3000 for production; update any code that reads cors.* keys to
use these new properties.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/juv25d/filter/CorsFilter.javasrc/main/java/org/juv25d/util/ConfigLoader.javasrc/main/resources/application-properties.yml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/juv25d/util/ConfigLoader.java (1)
126-137:⚠️ Potential issue | 🟡 MinorCORS entries still need normalization/filtering before use.
This remains vulnerable to whitespace/null/casing mismatches and can fail runtime CORS matching.
Proposed fix
Object origins = corsConfig.get("allowed-origins"); if (origins instanceof List<?> list) { this.allowedOrigins = list.stream() + .filter(Objects::nonNull) .map(String::valueOf) + .map(String::trim) + .filter(s -> !s.isEmpty()) .toList(); } Object methods = corsConfig.get("allowed-methods"); if (methods instanceof List<?> methodList) { - this.allowedMethods = methodList.stream() + List<String> parsedMethods = methodList.stream() + .filter(Objects::nonNull) .map(String::valueOf) + .map(String::trim) + .filter(s -> !s.isEmpty()) + .map(s -> s.toUpperCase(Locale.ROOT)) .toList(); + this.allowedMethods = parsedMethods.isEmpty() + ? List.of("GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS") + : parsedMethods; } else { this.allowedMethods = List.of("GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/util/ConfigLoader.java` around lines 126 - 137, In ConfigLoader, normalize and filter CORS lists produced from corsConfig.get("allowed-origins") and corsConfig.get("allowed-methods") before assigning to allowedOrigins and allowedMethods: when converting the List<?> (the variables named origins/list and methods/methodList) map each entry to a String, trim whitespace, filter out null or empty strings, convert to a consistent case (e.g., toLowerCase for origins and methods or as appropriate for your matching logic), and remove duplicates before collecting to the final list; ensure this normalization happens inside the same conditional blocks that check instanceof List<?> to avoid runtime mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 57-58: ConfigLoader currently sets allowedMethods to an empty list
when the CORS section is missing, breaking preflight; update the default
initialization in the ConfigLoader constructor (replace this.allowedMethods =
List.of()) to a safe allow-list (e.g., include
"GET","POST","PUT","DELETE","OPTIONS","HEAD") so preflight works when cors is
absent, and make the same change for the other default assignment block
referenced around lines 121-141 so both initialization sites use the safe
default while preserving existing parsing logic that overrides these values when
cors is present.
---
Duplicate comments:
In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 126-137: In ConfigLoader, normalize and filter CORS lists produced
from corsConfig.get("allowed-origins") and corsConfig.get("allowed-methods")
before assigning to allowedOrigins and allowedMethods: when converting the
List<?> (the variables named origins/list and methods/methodList) map each entry
to a String, trim whitespace, filter out null or empty strings, convert to a
consistent case (e.g., toLowerCase for origins and methods or as appropriate for
your matching logic), and remove duplicates before collecting to the final list;
ensure this normalization happens inside the same conditional blocks that check
instanceof List<?> to avoid runtime mismatches.
…back from CodeRabbit. Ensures safe default HTTP methods are always set, even if cors is missing in YAML. Hardens parsing of allowed-origins and allowed-methods: Filters out null and blank values Trims whitespace Normalizes HTTP methods to uppercase
Johan-jans
previously approved these changes
Feb 27, 2026
Johan-jans
left a comment
Johan-jans
left a comment
There was a problem hiding this comment.
Nice improvement making CORS fully config-driven and more flexible
FionaSprinkles
approved these changes
Mar 1, 2026
FionaSprinkles
left a comment
FionaSprinkles
left a comment
There was a problem hiding this comment.
Great refactor for the maintainabillity of this project :)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR updates the CORS filter so that allowed origins and HTTP methods are now loaded from the application configuration file instead of being hardcoded.
No more hardcoded CORS rules in code.
Makes it easier to add or change allowed origins/methods without changing code.
Improves flexibility and maintainability for future CORS changes.
Summary by CodeRabbit