89 implement securityheadersfilter to harden http responses - #91
Conversation
📝 WalkthroughWalkthroughAdds a new global Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
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/App.java (1)
29-48:⚠️ Potential issue | 🟠 MajorSecurity headers won't be applied to redirect and IP-block responses due to filter short-circuiting.
All five filters share priority
0, so execution follows insertion order.RedirectFilterandIpFilterboth run beforeSecurityHeadersFilter. WhenRedirectFiltermatches a redirect rule (lines 50–57), it callsperformRedirect()and returns without callingchain.doFilter(), stopping the pipeline. Similarly,IpFilterreturns without callingchain.doFilter()when an IP is blocked (lines 33–35). SinceSecurityHeadersFilter.doFilter()is never invoked when the chain is short-circuited, those 301/302 and 403 responses are sent withoutX-Frame-Options,X-XSS-Protection,Referrer-Policy, orX-Content-Type-Options— security headers that should protect these browser-visible responses.The fix is to register
SecurityHeadersFilterbefore the other filters so it wraps the entire downstream chain:🛡️ Proposed fix — register SecurityHeadersFilter first
Pipeline pipeline = new Pipeline(); + pipeline.addGlobalFilter(new SecurityHeadersFilter(), 0); + // Configure redirect rules List<RedirectRule> redirectRules = List.of( new RedirectRule("/old-page", "/new-page", 301), new RedirectRule("/temp", "https://example.com/temporary", 302), new RedirectRule("/docs/*", "/documentation/", 301) ); pipeline.addGlobalFilter(new RedirectFilter(redirectRules), 0); pipeline.addGlobalFilter(new IpFilter( Set.of(), Set.of() ), 0); pipeline.addGlobalFilter(new LoggingFilter(), 0); - pipeline.addGlobalFilter(new SecurityHeadersFilter(), 0); if (config.isRateLimitingEnabled()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/App.java` around lines 29 - 48, The SecurityHeadersFilter is added after RedirectFilter and IpFilter so short-circuiting in RedirectFilter.performRedirect() and IpFilter when blocking IPs prevents SecurityHeadersFilter.doFilter() from running; fix by registering SecurityHeadersFilter earlier (before RedirectFilter and IpFilter) using the same pipeline.addGlobalFilter call pattern so it wraps the downstream chain and always adds headers even when later filters return without calling chain.doFilter().
🧹 Nitpick comments (4)
src/main/java/org/juv25d/filter/SecurityHeadersFilter.java (2)
15-23: Wrapchain.doFilterin atry/finallyto guarantee security headers are set on all response paths.If
chain.doFilterthrows, the headers on lines 18–21 are never applied. While an exception path typically produces no usable response, wrapping intry/finallyis the correct defensive pattern for response-decoration filters.♻️ Proposed refactor
public void doFilter(HttpRequest req, HttpResponse res, FilterChain chain) throws IOException { - chain.doFilter(req, res); - - res.setHeader("X-Content-Type-Options", "nosniff"); - res.setHeader("X-Frame-Options", "DENY"); - res.setHeader("X-XSS-Protection", "1; mode=block"); - res.setHeader("Referrer-Policy", "no-referrer"); + try { + chain.doFilter(req, res); + } finally { + res.setHeader("X-Content-Type-Options", "nosniff"); + res.setHeader("X-Frame-Options", "DENY"); + res.setHeader("X-XSS-Protection", "0"); + res.setHeader("Referrer-Policy", "no-referrer"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/SecurityHeadersFilter.java` around lines 15 - 23, The doFilter implementation in SecurityHeadersFilter currently calls chain.doFilter(req, res) before setting headers so if chain.doFilter throws the security headers (X-Content-Type-Options, X-Frame-Options, X-XSS-Protection, Referrer-Policy) are never applied; change SecurityHeadersFilter.doFilter to call chain.doFilter(req, res) inside a try block and move the res.setHeader(...) calls into a finally block so headers are always added (the exception should be allowed to propagate after finally).
8-24: Top-level class should not be indented.The entire class body (Javadoc +
public class SecurityHeadersFilter) is indented four spaces as if it were nested. This is syntactically valid Java but non-standard and will confuse formatters/IDEs.🔧 Proposed fix
- /** - * Filter som lägger till säkerhetsheaders till varje HTTP-svar. - * Detta hjälper till att skydda mot attacker som Clickjacking och MIME-sniffing. - */ - public class SecurityHeadersFilter implements Filter { +/** + * Filter som lägger till säkerhetsheaders till varje HTTP-svar. + * Detta hjälper till att skydda mot attacker som Clickjacking och MIME-sniffing. + */ +public class SecurityHeadersFilter implements Filter { - `@Override` - public void doFilter(HttpRequest req, HttpResponse res, FilterChain chain) throws IOException { + `@Override` + public void doFilter(HttpRequest req, HttpResponse res, FilterChain chain) throws IOException { ... - } - } + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/SecurityHeadersFilter.java` around lines 8 - 24, The class declaration and its Javadoc are indented as if nested; move the Javadoc and the public class SecurityHeadersFilter (and its closing brace) to the top-level (left-most column) so the class is not indented, keeping the existing doFilter(HttpRequest req, HttpResponse res, FilterChain chain) implementation and its res.setHeader calls intact and ensuring braces align with the class declaration.src/main/java/org/juv25d/App.java (2)
3-3: Consider preferring explicit imports over the wildcard.Wildcard imports obscure which types are actually used, can cause naming conflicts if another package exports the same simple name, and most Java style guides (Google, Oracle) recommend explicit imports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/App.java` at line 3, Replace the wildcard import "org.juv25d.filter.*" in App.java with explicit imports for each type from that package actually referenced in this file: scan App.java for all classes/interfaces used from org.juv25d.filter (e.g., any Filter implementations or helper types referenced in method bodies or fields) and add one import statement per type instead of the wildcard, removing the wildcard import afterward.
6-8: Remove stale// New importcomments.These annotations are developer noise and should be dropped before merging.
🔧 Proposed fix
-import org.juv25d.plugin.NotFoundPlugin; // New import +import org.juv25d.plugin.NotFoundPlugin; import org.juv25d.plugin.StaticFilesPlugin; -import org.juv25d.router.SimpleRouter; // New import +import org.juv25d.router.SimpleRouter;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/App.java` around lines 6 - 8, Remove the inline developer annotations "// New import" from the import statements so they are clean; specifically edit the import lines referencing NotFoundPlugin and SimpleRouter (keep the imports org.juv25d.plugin.NotFoundPlugin and org.juv25d.router.SimpleRouter and other imports like StaticFilesPlugin unchanged) and delete the trailing "// New import" comments to eliminate developer noise.
🤖 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/SecurityHeadersFilter.java`:
- Line 20: Replace the deprecated insecure X-XSS-Protection value in
SecurityHeadersFilter: locate the res.setHeader call that sets
"X-XSS-Protection" (in the filter class/method where response headers are
applied) and either remove that header entirely or change its value to "0" to
explicitly disable the legacy browser XSS filter (preferred: set to "0"); ensure
any related comments/tests expecting "1; mode=block" are updated accordingly and
keep the rest of the security header logic intact.
---
Outside diff comments:
In `@src/main/java/org/juv25d/App.java`:
- Around line 29-48: The SecurityHeadersFilter is added after RedirectFilter and
IpFilter so short-circuiting in RedirectFilter.performRedirect() and IpFilter
when blocking IPs prevents SecurityHeadersFilter.doFilter() from running; fix by
registering SecurityHeadersFilter earlier (before RedirectFilter and IpFilter)
using the same pipeline.addGlobalFilter call pattern so it wraps the downstream
chain and always adds headers even when later filters return without calling
chain.doFilter().
---
Nitpick comments:
In `@src/main/java/org/juv25d/App.java`:
- Line 3: Replace the wildcard import "org.juv25d.filter.*" in App.java with
explicit imports for each type from that package actually referenced in this
file: scan App.java for all classes/interfaces used from org.juv25d.filter
(e.g., any Filter implementations or helper types referenced in method bodies or
fields) and add one import statement per type instead of the wildcard, removing
the wildcard import afterward.
- Around line 6-8: Remove the inline developer annotations "// New import" from
the import statements so they are clean; specifically edit the import lines
referencing NotFoundPlugin and SimpleRouter (keep the imports
org.juv25d.plugin.NotFoundPlugin and org.juv25d.router.SimpleRouter and other
imports like StaticFilesPlugin unchanged) and delete the trailing "// New
import" comments to eliminate developer noise.
In `@src/main/java/org/juv25d/filter/SecurityHeadersFilter.java`:
- Around line 15-23: The doFilter implementation in SecurityHeadersFilter
currently calls chain.doFilter(req, res) before setting headers so if
chain.doFilter throws the security headers (X-Content-Type-Options,
X-Frame-Options, X-XSS-Protection, Referrer-Policy) are never applied; change
SecurityHeadersFilter.doFilter to call chain.doFilter(req, res) inside a try
block and move the res.setHeader(...) calls into a finally block so headers are
always added (the exception should be allowed to propagate after finally).
- Around line 8-24: The class declaration and its Javadoc are indented as if
nested; move the Javadoc and the public class SecurityHeadersFilter (and its
closing brace) to the top-level (left-most column) so the class is not indented,
keeping the existing doFilter(HttpRequest req, HttpResponse res, FilterChain
chain) implementation and its res.setHeader calls intact and ensuring braces
align with the class declaration.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/org/juv25d/filter/SecurityHeadersFilter.java (2)
8-12: Top-level class is indented 4 spaces.The Javadoc and
public classdeclaration are indented as though they sit inside an outer class, but this is a top-level type. Java ignores indentation so it compiles, but it's misleading.♻️ Proposed fix
- /** - * Filter som lägger till säkerhetsheaders till varje HTTP-svar. - * Detta hjälper till att skydda mot attacker som Clickjacking och MIME-sniffing. - */ - public class SecurityHeadersFilter implements Filter { +/** + * Filter som lägger till säkerhetsheaders till varje HTTP-svar. + * Detta hjälper till att skydda mot attacker som Clickjacking och MIME-sniffing. + */ +public class SecurityHeadersFilter implements Filter {(The method body inside should be dedented consistently by 4 spaces as well.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/SecurityHeadersFilter.java` around lines 8 - 12, The file's top-level Javadoc and class declaration for SecurityHeadersFilter are indented 4 spaces as if nested; dedent the Javadoc block, the "public class SecurityHeadersFilter implements Filter {" line, and all of the class's members (constructor/methods/fields) so the top-level type starts at column 0; ensure every method (e.g., doFilter, init, destroy) and their bodies are consistently dedented the same amount to match standard top-level formatting.
18-21: Consider addingContent-Security-PolicyandStrict-Transport-Security.
Content-Security-Policyis the modern, actively maintained replacement for the now-deprecated XSS filter and provides far stronger XSS mitigations.Strict-Transport-Security(HSTS) is the standard way to enforce HTTPS if the server supports TLS. Both are widely recommended by OWASP and the MDN HTTP security guidance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/SecurityHeadersFilter.java` around lines 18 - 21, The SecurityHeadersFilter currently sets several headers but is missing Content-Security-Policy and Strict-Transport-Security; update the filter (e.g., inside the same method where res.setHeader(...) is called in class SecurityHeadersFilter) to add a Content-Security-Policy header with a safe baseline policy (for example: default-src 'self'; script-src 'self'; object-src 'none'; base-uri 'self'; frame-ancestors 'none') and add a Strict-Transport-Security header when TLS is supported (e.g., max-age=31536000; includeSubDomains; preload) so browsers enforce HTTPS; ensure the HSTS header is only sent for secure requests if your environment may serve HTTP.
🤖 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/SecurityHeadersFilter.java`:
- Around line 18-21: Update the X-XSS-Protection header in SecurityHeadersFilter
to the OWASP-recommended value by ensuring the call in the filter that sets the
header (res.setHeader("X-XSS-Protection", ...)) uses "0" instead of the
deprecated "1; mode=block"; confirm this change is present in the class (e.g.,
SecurityHeadersFilter.doFilter or the method where res.setHeader is invoked) and
leave the other headers (X-Content-Type-Options, X-Frame-Options,
Referrer-Policy) unchanged.
---
Nitpick comments:
In `@src/main/java/org/juv25d/filter/SecurityHeadersFilter.java`:
- Around line 8-12: The file's top-level Javadoc and class declaration for
SecurityHeadersFilter are indented 4 spaces as if nested; dedent the Javadoc
block, the "public class SecurityHeadersFilter implements Filter {" line, and
all of the class's members (constructor/methods/fields) so the top-level type
starts at column 0; ensure every method (e.g., doFilter, init, destroy) and
their bodies are consistently dedented the same amount to match standard
top-level formatting.
- Around line 18-21: The SecurityHeadersFilter currently sets several headers
but is missing Content-Security-Policy and Strict-Transport-Security; update the
filter (e.g., inside the same method where res.setHeader(...) is called in class
SecurityHeadersFilter) to add a Content-Security-Policy header with a safe
baseline policy (for example: default-src 'self'; script-src 'self'; object-src
'none'; base-uri 'self'; frame-ancestors 'none') and add a
Strict-Transport-Security header when TLS is supported (e.g., max-age=31536000;
includeSubDomains; preload) so browsers enforce HTTPS; ensure the HSTS header is
only sent for secure requests if your environment may serve HTTP.
kristinaxm
left a comment
There was a problem hiding this comment.
Since we introduced annotations on filters I'd suggest to just add this line:
@global(order = 0)
kristinaxm
left a comment
There was a problem hiding this comment.
Nice work on the filter! :) Approved
mattknatt
left a comment
There was a problem hiding this comment.
The filter looks good, but it is added twice in the pipeline,
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/java/org/juv25d/filter/SecurityHeadersFilter.java (3)
20-28: Inconsistent indentation inside thefinallyblock.The four
setHeadercalls are indented one extra level compared to the enclosingfinallyblock, and the closing brace is misaligned. Standard 4-space Java indentation expects the body at 12 spaces (method body = 8, block content = 12) and the closing brace at 8.✏️ Proposed fix
- } finally { - - res.setHeader("X-Content-Type-Options", "nosniff"); - res.setHeader("X-Frame-Options", "DENY"); - res.setHeader("X-XSS-Protection", "0"); - res.setHeader("Referrer-Policy", "no-referrer"); - - } - } + } finally { + res.setHeader("X-Content-Type-Options", "nosniff"); + res.setHeader("X-Frame-Options", "DENY"); + res.setHeader("X-XSS-Protection", "0"); + res.setHeader("Referrer-Policy", "no-referrer"); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/SecurityHeadersFilter.java` around lines 20 - 28, Adjust the indentation in the finally block inside SecurityHeadersFilter (the block that calls res.setHeader(...)) so the four setHeader calls are indented to match standard 4-space Java style (body at 12 spaces under the method body) and align the closing brace with the method block (8 spaces); update the finally block in the doFilter/doFilterInternal method accordingly so the setHeader lines and the closing brace are consistently aligned.
9-12: Javadoc only partially describes the headers being set.The comment covers Clickjacking and MIME sniffing but omits the purpose of
X-XSS-Protection: 0(disabling the legacy browser XSS filter) andReferrer-Policy: no-referrer.✏️ Proposed update
/** * Filter that adds security headers to every HTTP response. - * This helps protect against attacks such as Clickjacking and MIME sniffing. + * Sets the following headers on every response: + * <ul> + * <li>X-Content-Type-Options: nosniff — prevents MIME-type sniffing</li> + * <li>X-Frame-Options: DENY — prevents Clickjacking</li> + * <li>X-XSS-Protection: 0 — disables the legacy browser XSS filter</li> + * <li>Referrer-Policy: no-referrer — suppresses the Referer header</li> + * </ul> */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/SecurityHeadersFilter.java` around lines 9 - 12, Javadoc for SecurityHeadersFilter should be expanded to list and describe all headers it sets: explicitly mention X-Frame-Options (prevent clickjacking), X-Content-Type-Options (prevent MIME sniffing), X-XSS-Protection: 0 (disables legacy browser XSS filter to avoid incorrect mitigation and inline-script issues), and Referrer-Policy: no-referrer (never send referrer info); update the class-level comment in SecurityHeadersFilter (and/or the doFilter method Javadoc if present) to include these purposes and a short rationale for each header.
22-25: Consider addingContent-Security-PolicyandStrict-Transport-Securityheaders.Two commonly expected security headers are absent:
Content-Security-Policy—X-XSS-Protection: 0disables the legacy browser XSS filter; the modern mitigation is CSP. Without at least a baseline policy (e.g.,default-src 'self'), settingX-XSS-Protection: 0removes a safeguard without replacing it. The PR description's stated goal of hardening against XSS is only partially met.Strict-Transport-Security— if this server is HTTPS-only, HSTS (max-age=31536000; includeSubDomains) is a critical header. Its absence allows protocol-downgrade attacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/SecurityHeadersFilter.java` around lines 22 - 25, The SecurityHeadersFilter currently sets X-Content-Type-Options, X-Frame-Options, X-XSS-Protection, and Referrer-Policy but omits CSP and HSTS; update the header-setting code in SecurityHeadersFilter (where res.setHeader(...) is called, e.g., in doFilter/doFilterInternal) to add a baseline Content-Security-Policy (for example "default-src 'self'") and, when the app is served over HTTPS, add Strict-Transport-Security (for example "max-age=31536000; includeSubDomains"); ensure these headers are applied alongside the existing ones and consider making their values configurable if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/juv25d/filter/SecurityHeadersFilter.java`:
- Around line 20-28: Adjust the indentation in the finally block inside
SecurityHeadersFilter (the block that calls res.setHeader(...)) so the four
setHeader calls are indented to match standard 4-space Java style (body at 12
spaces under the method body) and align the closing brace with the method block
(8 spaces); update the finally block in the doFilter/doFilterInternal method
accordingly so the setHeader lines and the closing brace are consistently
aligned.
- Around line 9-12: Javadoc for SecurityHeadersFilter should be expanded to list
and describe all headers it sets: explicitly mention X-Frame-Options (prevent
clickjacking), X-Content-Type-Options (prevent MIME sniffing), X-XSS-Protection:
0 (disables legacy browser XSS filter to avoid incorrect mitigation and
inline-script issues), and Referrer-Policy: no-referrer (never send referrer
info); update the class-level comment in SecurityHeadersFilter (and/or the
doFilter method Javadoc if present) to include these purposes and a short
rationale for each header.
- Around line 22-25: The SecurityHeadersFilter currently sets
X-Content-Type-Options, X-Frame-Options, X-XSS-Protection, and Referrer-Policy
but omits CSP and HSTS; update the header-setting code in SecurityHeadersFilter
(where res.setHeader(...) is called, e.g., in doFilter/doFilterInternal) to add
a baseline Content-Security-Policy (for example "default-src 'self'") and, when
the app is served over HTTPS, add Strict-Transport-Security (for example
"max-age=31536000; includeSubDomains"); ensure these headers are applied
alongside the existing ones and consider making their values configurable if
needed.
lindaeskilsson
left a comment
There was a problem hiding this comment.
Nice addition! The filter makes sense and fits well in the pipeline! 👍🏼
* 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
…sponse defaults (#87) * fix(httpresponse): add safe defaults, null-safe headers and defensive body handling * test(httpresponse): verify default values and null-safe behaviour * refactor(httpresponse): enforce non-null statusText in constructor * implement securityheadersfilter to harden http responses (#91) * 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 * Added IpFilterTest class with unit test verifying IpFilter allows whi… (#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 * Refactor ConfigLoader to accept InputStream and add unit tests (#72) * 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> --------- Co-authored-by: KM <kristina0x7@gmail.com> Co-authored-by: johanbriger <johanbriger@gmail.com> Co-authored-by: SandraNelj <229708855+SandraNelj@users.noreply.github.com> Co-authored-by: Simon Forsberg <simon.co.forsberg@gmail.com> Co-authored-by: mattknatt <mattiashagstrommusic@gmail.com>
This PR introduces a new global filter, SecurityHeadersFilter, to the server's pipeline. The purpose is to strengthen the security of all outgoing HTTP responses by injecting standard security headers.
Changes:
SecurityHeadersFilter: A new filter class that adds protection against common web vulnerabilities.
Pipeline Integration: Registered as a global filter in App.java to ensure it covers all routes.
Headers Added:
X-Content-Type-Options: nosniff – Prevents MIME-type sniffing.
X-Frame-Options: DENY – Protects against Clickjacking.
X-XSS-Protection: 0 – Enables the browser's cross-site scripting filter.
Referrer-Policy: no-referrer – Protects privacy by limiting referrer information.
Summary by CodeRabbit
New Features
Refactor