Initialize safe defaults in HttpResponse no-arg constructorFix/httpresponse defaults - #87
Merged
Merged
Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds safe defaults and null-safety to HttpResponse (final headers, default 200/OK, empty body), introduces SecurityHeadersFilter and registers it, makes ConfigLoader test-friendly (InputStream constructor), and adds unit tests for HttpResponse, IpFilter, and ConfigLoader. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as Router
participant SecFilter as SecurityHeadersFilter
participant Chain as FilterChain
participant Handler as Handler
participant Response as HttpResponse
Client->>Router: HTTP request
Router->>SecFilter: pass request to global filters
SecFilter->>Chain: chain.doFilter(req, res)
Chain->>Handler: invoke handler(s)
Handler-->>Response: write body/status/headers
Chain-->>SecFilter: return to filter
Note right of SecFilter: finally block ensures headers added
SecFilter-->>Response: add X-Content-Type-Options: nosniff
SecFilter-->>Response: add X-Frame-Options: DENY
SecFilter-->>Response: add X-XSS-Protection: 0
SecFilter-->>Response: add Referrer-Policy: no-referrer
SecFilter-->>Router: return response
Router-->>Client: HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 1
🧹 Nitpick comments (2)
src/test/java/org/juv25d/http/HttpResponseTest.java (2)
17-18: Chain the twobody()assertions to avoid a redundant array clone.
body()clones the underlyingbyte[]on every call. Two separate assertions allocate two clones for what is a single logical check.♻️ Proposed change
- assertThat(response.body()).isNotNull(); - assertThat(response.body()).isEmpty(); + assertThat(response.body()).isNotNull().isEmpty();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/http/HttpResponseTest.java` around lines 17 - 18, The two assertions call response.body() twice (which clones the byte[] both times); replace the two separate assertions in HttpResponseTest (the lines using assertThat(response.body()).isNotNull(); and assertThat(response.body()).isEmpty();) with a single chained assertion that calls response.body() once and asserts both conditions (e.g., assertThat(response.body()).isNotNull().isEmpty()), thereby avoiding the redundant array clone.
10-24: Add a test covering the parameterized constructor's null-headers fix.The PR description calls out null-safety for the parameterized constructor as an explicit objective, but there is no test for it.
new HttpResponse(200, "OK", null, null)should be exercised to confirmheaders()is non-null andbody()is empty.✅ Suggested additional test
`@Test` void parameterizedConstructor_withNullHeadersAndBody_hasSafeDefaults() { HttpResponse response = new HttpResponse(200, "OK", null, null); assertThat(response.headers()).isNotNull().isEmpty(); assertThat(response.body()).isNotNull().isEmpty(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/http/HttpResponseTest.java` around lines 10 - 24, Add a unit test that constructs HttpResponse via the parameterized constructor (new HttpResponse(200, "OK", null, null)) and asserts that headers() is non-null and empty and body() is non-null and empty; this verifies the null-headers/body fix in the HttpResponse constructor and mirrors the existing defaultConstructor test by exercising HttpResponse(HttpResponse(int,String,Map<String,String>,byte[])) and calling headers() and body() to confirm safe defaults.
🤖 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/http/HttpResponse.java`:
- Around line 24-29: The parameterized constructor HttpResponse(int statusCode,
String statusText, Map<String, String> headers, byte[] body) currently allows a
null statusText; update the constructor to enforce the same non-null contract as
setStatusText by either calling Objects.requireNonNull(statusText, "statusText
must not be null") or replacing null with a safe default (e.g., empty string)
before assigning to this.statusText, while keeping the existing null-safe
handling for headers and body.
---
Nitpick comments:
In `@src/test/java/org/juv25d/http/HttpResponseTest.java`:
- Around line 17-18: The two assertions call response.body() twice (which clones
the byte[] both times); replace the two separate assertions in HttpResponseTest
(the lines using assertThat(response.body()).isNotNull(); and
assertThat(response.body()).isEmpty();) with a single chained assertion that
calls response.body() once and asserts both conditions (e.g.,
assertThat(response.body()).isNotNull().isEmpty()), thereby avoiding the
redundant array clone.
- Around line 10-24: Add a unit test that constructs HttpResponse via the
parameterized constructor (new HttpResponse(200, "OK", null, null)) and asserts
that headers() is non-null and empty and body() is non-null and empty; this
verifies the null-headers/body fix in the HttpResponse constructor and mirrors
the existing defaultConstructor test by exercising
HttpResponse(HttpResponse(int,String,Map<String,String>,byte[])) and calling
headers() and body() to confirm safe defaults.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/org/juv25d/http/HttpResponse.java (2)
48-50:headers()returns the live internal map, allowing mutations that bypasssetHeader().Callers can invoke
response.headers().remove(key)orresponse.headers().clear()directly. SincesetHeadercurrently has no special validation logic, this is functionally equivalent today — but it couples external code to the internalMapAPI and bypasses any future hooks added tosetHeader/removeHeader.Consider wrapping the return in
Collections.unmodifiableMap(headers)and providing an explicitremoveHeader(String name)method to keep all mutation through the defined API. Given the intentionally mutable design, this is non-urgent.♻️ Proposed refactor
public Map<String, String> headers() { - return headers; + return Collections.unmodifiableMap(headers); } +public void removeHeader(String name) { + headers.remove(name); +}🤖 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 48 - 50, headers() currently exposes the internal mutable Map which allows callers to mutate it directly and bypass API methods; change headers() to return an unmodifiable view (e.g., wrap headers in Collections.unmodifiableMap) and add explicit mutation methods such as removeHeader(String name) (and keep setHeader(String name, String value)) so all modifications go through the class's API (update references to headers() consumers if they relied on direct mutations).
52-54:setHeaderhas no null guard onnameorvalue.
nullheader names and values are invalid per the HTTP spec (RFC 9110). WhileLinkedHashMap.putaccepts null keys without throwing, silently storing a null-named header would produce a malformed HTTP message.🛡️ Proposed fix
public void setHeader(String name, String value) { + Objects.requireNonNull(name, "header name must not be null"); + Objects.requireNonNull(value, "header value must not be null"); headers.put(name, value); }🤖 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 52 - 54, The setHeader method stores headers without validating inputs; add null guards in HttpResponse.setHeader to reject null header names or values (and optionally empty names) before calling headers.put: check the method parameters (name and value) and throw an IllegalArgumentException with a clear message if either is null (or name is empty), so the headers LinkedHashMap cannot be populated with invalid entries.
🤖 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/http/HttpResponse.java`:
- Around line 24-29: No change required: the HttpResponse(int statusCode, String
statusText, Map<String, String> headers, byte[] body) constructor already
enforces null-safety by calling Objects.requireNonNull on statusText and by
defaulting headers to Map.of() and defensively cloning body, so no further
modifications to statusText, headers or body handling are necessary.
---
Nitpick comments:
In `@src/main/java/org/juv25d/http/HttpResponse.java`:
- Around line 48-50: headers() currently exposes the internal mutable Map which
allows callers to mutate it directly and bypass API methods; change headers() to
return an unmodifiable view (e.g., wrap headers in Collections.unmodifiableMap)
and add explicit mutation methods such as removeHeader(String name) (and keep
setHeader(String name, String value)) so all modifications go through the
class's API (update references to headers() consumers if they relied on direct
mutations).
- Around line 52-54: The setHeader method stores headers without validating
inputs; add null guards in HttpResponse.setHeader to reject null header names or
values (and optionally empty names) before calling headers.put: check the method
parameters (name and value) and throw an IllegalArgumentException with a clear
message if either is null (or name is empty), so the headers LinkedHashMap
cannot be populated with invalid entries.
johanbriger
previously approved these changes
Feb 18, 2026
johanbriger
left a comment
johanbriger
left a comment
There was a problem hiding this comment.
Nice work! Solid improvement for null-safety!
Initializing defaults in the no-arg constructor and adding null-checks in the parameterized constructor is a great way to make the API more robust.
|
Looks good, I have also done the changes regarding the HttpResponse in my PR (#76), so we are going to have a conflict, but I don't think that it's going to be a big change! Just FYI 👍 |
SandraNelj
self-requested a review
February 18, 2026 09:05
SandraNelj
previously approved these changes
Feb 18, 2026
kristinaxm
dismissed stale reviews from SandraNelj and johanbriger
via
February 18, 2026 22:16
fe50c14
Tyreviel
previously approved these changes
Feb 19, 2026
SandraNelj
previously approved these changes
Feb 19, 2026
* Add SecurityHeadersFilter for hardened HTTP responses * Add SecurityHeadersFilter for hardened HTTP responses * Changed X-XSS-Protection value to recommended 0, * address code review feedback from CodeRabbit * Add @global annotation to SecurityHeadersFilter for automatic registration * Removed line of code in App.java
#76) * Added IpFilterTest class with unit test verifying IpFilter allows whitelisted IPs. * Fix IpFilterTest to verify response interaction instead of mock state * Added unit test for blocking IP that is not in the whitelist, results in 403 Forbidden response. Fixed HttpResponse construtors to always initialize headers and body to prevent NPE when filters call setHeader or setBody. * Update IpFilter whitelist allow test to use real HttpResponse * Assert expected status code in IpFilter whitelist allow test
* test(config-loader): add test skeleton for ConfigLoader * test(config-loader): add initial test for loading config * refactor(config-loader): extract configuration loading to InputStream constructor * test(config-loader): verify values are loaded from yaml input * test(config-loader): add test for default values when server keys missing * test(config-loader): add null-input error handling test * refactor(config-loader): add safe map casting and robust value parsing * fix: handle missing server config and keep original exception cause * fix(config-loader): handle empty yaml config safely * fix (config-loader): add default log level for consistent config values * Add missing curly bracket. * fix(config-loader): address review rabbit comments and improve tests --------- Co-authored-by: Simon Forsberg <simon.co.forsberg@gmail.com> Co-authored-by: mattknatt <mattiashagstrommusic@gmail.com>
lindaeskilsson
dismissed stale reviews from SandraNelj and Tyreviel
via
February 20, 2026 09:23
d0d75ce
johanbriger
approved these changes
Feb 20, 2026
SandraNelj
approved these changes
Feb 20, 2026
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
5 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 makes
HttpResponsesafe to use out of the box by initializing sensible defaults in the no-arg constructor.Changes
HttpResponse():statusCode = 200statusText = "OK"headersmapbodybyte arrayTests
setHeader()does not throw and stores the headerCloses #81
Summary by CodeRabbit
New Features
Bug Fixes
Tests