41-add-cors-filter-to-enable-cross-origin-requests - #120
Merged
Conversation
HttpResponse was immutable, which prevented filters from adding headers or modifying status/body. Since our filter chain relies on mutating the shared response object, immutability blocked correct CORS implementation. Added setter methods and removed unmodifiable headers wrapper. HttpResponseWriter remains unaffected.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new CorsFilter implementing CORS logic (whitelists http://localhost:3000, handles preflight OPTIONS) and updates HttpResponse to support case-insensitive header lookup and mutable headers; includes JUnit 5 tests exercising the filter against a local server. Changes
Sequence DiagramsequenceDiagram
participant Client
participant CorsFilter
participant FilterChain
participant ServerResponse as Response
Client->>CorsFilter: HTTP Request (with/without Origin)
alt Origin missing or not whitelisted
CorsFilter->>FilterChain: forward request
FilterChain->>Response: produce response
Response->>Client: response (no CORS headers)
else Origin whitelisted
alt OPTIONS (preflight)
CorsFilter->>Response: set AC-Allow-Origin, AC-Allow-Methods
CorsFilter->>Response: set AC-Allow-Headers (mirror or Content-Type)
CorsFilter->>Response: set AC-Max-Age: 3600
CorsFilter->>Client: 204 No Content (empty)
else Non-OPTIONS
CorsFilter->>Response: set AC-Allow-Origin, Vary: Origin
CorsFilter->>FilterChain: forward request
FilterChain->>Response: produce response
Response->>Client: response (with CORS headers)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/juv25d/http/HttpResponse.java (1)
30-32:⚠️ Potential issue | 🟠 Major
headers()exposes the internal mutable map — encapsulation leak.Before this PR the map was wrapped in
Collections.unmodifiableMap, preventing external callers from bypassingsetHeader. The new accessor returns the rawLinkedHashMapdirectly, so any caller can callresponse.headers().put(...)/response.headers().remove(...), sidestepping the null-validation insetHeaderand making mutation semantics inconsistent.🛡️ Proposed fix
public Map<String, String> headers() { - return headers; + return Collections.unmodifiableMap(headers); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/http/HttpResponse.java` around lines 30 - 32, The headers() accessor in HttpResponse currently returns the internal mutable LinkedHashMap (headers), leaking encapsulation; change headers() to return an unmodifiable view or a defensive copy so callers cannot mutate the internal map directly and bypass setHeader's validation. Specifically, update the HttpResponse.headers() method to return Collections.unmodifiableMap(headers) (or new LinkedHashMap<>(headers)) and keep setHeader(...) as the only sanctioned mutator to preserve null-checks and consistent mutation semantics.
🧹 Nitpick comments (1)
src/main/java/org/juv25d/filter/CorsFilter.java (1)
13-15: Hardcoded development origin is not production-ready.
http://localhost:3000is the only permitted origin. Any production deployment will silently deny all browser cross-origin requests. Consider accepting the allowed-origins list via constructor injection or an externalized configuration property so the filter is reusable without code changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/CorsFilter.java` around lines 13 - 15, Replace the hardcoded ALLOWED_ORIGINS Set in CorsFilter with a configurable source: add a constructor or setter on class CorsFilter that accepts a Collection<String> (e.g., allowedOrigins) or read from an external config property, remove the static Set.of(...) usage, and use the injected allowedOrigins inside the filter logic; ensure existing tests/usage instantiate CorsFilter with the desired origins (e.g., from application properties or environment) so production deployments do not rely on the hardcoded "http://localhost:3000".
🤖 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/filter/CorsFilter.java`:
- Around line 45-50: The else branch in CorsFilter currently overwrites the
Access-Control-Allow-Origin header with the literal "Content-Type"; update the
fallback to set Access-Control-Allow-Headers instead so the origin set earlier
remains untouched. Specifically, in the block that reads requestedHeaders and
handles the null/blank case, replace the
res.setHeader("Access-Control-Allow-Origin", "Content-Type") call with
res.setHeader("Access-Control-Allow-Headers", "Content-Type") so
Access-Control-Allow-Origin set by the earlier code remains correct and the
preflight response includes a default allowed header.
In `@src/test/java/org/juv25d/filter/CorsFilterTest.java`:
- Around line 19-24: The `@BeforeAll` method ensureServerIsRunning is currently a
no-op causing ConnectException for tests; implement server lifecycle here by
starting the test HTTP server (e.g., call your HTTP server start method such as
Server.start(3000) or equivalent) before tests and stop it in a corresponding
`@AfterAll` teardown (e.g., Server.stop()), or switch the test class to per-class
lifecycle with `@TestInstance`(Lifecycle.PER_CLASS) if you need non-static
setup/teardown; ensure the started server binds to port 3000 so the tests that
hit localhost:3000 succeed.
- Around line 55-80: Add a new test that exercises the CorsFilter fallback by
sending an OPTIONS preflight request WITHOUT the Access-Control-Request-Headers
header (mirror the existing shouldHandlePreflightOptionsRequest pattern) and
assert the response status is 204 and that
response.headers().firstValue("Access-Control-Allow-Headers") equals
"Content-Type"; this will hit the CorsFilter fallback branch (the logic around
line 49) and verify the fallback header value instead of the mirrored path.
---
Outside diff comments:
In `@src/main/java/org/juv25d/http/HttpResponse.java`:
- Around line 30-32: The headers() accessor in HttpResponse currently returns
the internal mutable LinkedHashMap (headers), leaking encapsulation; change
headers() to return an unmodifiable view or a defensive copy so callers cannot
mutate the internal map directly and bypass setHeader's validation.
Specifically, update the HttpResponse.headers() method to return
Collections.unmodifiableMap(headers) (or new LinkedHashMap<>(headers)) and keep
setHeader(...) as the only sanctioned mutator to preserve null-checks and
consistent mutation semantics.
---
Nitpick comments:
In `@src/main/java/org/juv25d/filter/CorsFilter.java`:
- Around line 13-15: Replace the hardcoded ALLOWED_ORIGINS Set in CorsFilter
with a configurable source: add a constructor or setter on class CorsFilter that
accepts a Collection<String> (e.g., allowedOrigins) or read from an external
config property, remove the static Set.of(...) usage, and use the injected
allowedOrigins inside the filter logic; ensure existing tests/usage instantiate
CorsFilter with the desired origins (e.g., from application properties or
environment) so production deployments do not rely on the hardcoded
"http://localhost:3000".
fmazmz
reviewed
Feb 23, 2026
fmazmz
left a comment
fmazmz
left a comment
Member
There was a problem hiding this comment.
Very nice, just one thought below regarding where we put and read the properties from.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/juv25d/filter/CorsFilter.java (1)
44-50:⚠️ Potential issue | 🔴 CriticalFix the preflight fallback header assignment.
Line 49 overwrites
Access-Control-Allow-Originwith"Content-Type", which drops the real origin and omitsAccess-Control-Allow-Headerswhen noAccess-Control-Request-Headersis present. This breaks the preflight response in that branch.🐛 Proposed fix
- } else { - res.setHeader("Access-Control-Allow-Origin", "Content-Type"); - } + } else { + res.setHeader("Access-Control-Allow-Headers", "Content-Type"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/CorsFilter.java` around lines 44 - 50, In CorsFilter (the preflight handling block that uses header(req.headers(), "Access-Control-Request-Headers")), the else branch mistakenly overwrites Access-Control-Allow-Origin with "Content-Type"; change that branch to set Access-Control-Allow-Headers to a sensible default (e.g. "Content-Type") instead, so the real origin header is not clobbered and the preflight response still includes Access-Control-Allow-Headers when no Access-Control-Request-Headers is present.
🧹 Nitpick comments (1)
src/main/java/org/juv25d/filter/CorsFilter.java (1)
37-38: Avoid clobbering existingVaryvalues.If another filter or handler sets
Vary, this overwrites it. Consider appendingOrigininstead.♻️ Suggested tweak
- res.setHeader("Vary", "Origin"); + String vary = res.headers().get("Vary"); + if (vary == null || vary.isBlank()) { + res.setHeader("Vary", "Origin"); + } else if (!vary.toLowerCase().contains("origin")) { + res.setHeader("Vary", vary + ", Origin"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/CorsFilter.java` around lines 37 - 38, In CorsFilter, avoid overwriting an existing Vary header: read the current Vary value from the response (res.getHeader("Vary")), and if it's null or empty call res.setHeader("Vary", "Origin"), otherwise if it doesn't already contain "Origin" append ", Origin" and call res.setHeader with the combined value; update the code around the existing res.setHeader("Vary", "Origin") lines so you preserve any previously set Vary values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/juv25d/filter/CorsFilter.java`:
- Around line 44-50: In CorsFilter (the preflight handling block that uses
header(req.headers(), "Access-Control-Request-Headers")), the else branch
mistakenly overwrites Access-Control-Allow-Origin with "Content-Type"; change
that branch to set Access-Control-Allow-Headers to a sensible default (e.g.
"Content-Type") instead, so the real origin header is not clobbered and the
preflight response still includes Access-Control-Allow-Headers when no
Access-Control-Request-Headers is present.
---
Nitpick comments:
In `@src/main/java/org/juv25d/filter/CorsFilter.java`:
- Around line 37-38: In CorsFilter, avoid overwriting an existing Vary header:
read the current Vary value from the response (res.getHeader("Vary")), and if
it's null or empty call res.setHeader("Vary", "Origin"), otherwise if it doesn't
already contain "Origin" append ", Origin" and call res.setHeader with the
combined value; update the code around the existing res.setHeader("Vary",
"Origin") lines so you preserve any previously set Vary values.
fmazmz
approved these changes
Feb 25, 2026
fmazmz
deleted the
41-add-cors-filter-to-enable-cross-origin-requests
branch
February 25, 2026 15:59
Merged
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
3 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.
Add CORS filter with preflight support
Description
This PR adds CORS support to the server.
Made HttpResponse mutable to allow filters to modify status, headers, and body.
Implemented a whitelist-based CorsFilter.
Added proper handling of OPTIONS preflight requests (returns 204 with required CORS headers).
Added integration tests covering:
GET with/without Origin
OPTIONS preflight
Disallowed origin
Summary by CodeRabbit
New Features
Tests