Extend ipfilter functionality with subnets - #140
Merged
jesperlarsson1910 merged 5 commits intoFeb 27, 2026
Conversation
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
📝 WalkthroughWalkthroughAdds Apache Commons Net dependency and extends IpFilter with thread-safe collections, CIDR/subnet support, runtime add/remove APIs, proxy-header-aware client IP extraction, improved forbidden response construction, logging, accessors, and expanded tests. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Proxy
participant Filter as IpFilter
participant Subnet as SubnetUtils
participant Chain as FilterChain
Client->>Filter: HTTP Request (headers + remote IP)
activate Filter
Filter->>Filter: getClientIp() (trustProxyHeaders ? headers : remote)
alt header contains X-Forwarded-For
Filter-->>Filter: use first valid forwarded IP
end
Filter->>Filter: isAllowed(ip)
alt IP in Whitelist
Filter-->>Filter: allow
else IP in Whitelist Subnets
Filter->>Subnet: check CIDR membership
Subnet-->>Filter: result
Filter-->>Filter: allow/deny
else IP in Blacklist
Filter-->>Filter: deny
else IP in Blacklist Subnets
Filter->>Subnet: check CIDR membership
Subnet-->>Filter: result
Filter-->>Filter: deny/allow
else Default Policy
Filter-->>Filter: allowByDefault
end
deactivate Filter
alt Allowed
Filter->>Chain: chain.doFilter()
Chain-->>Client: request proceeds
else Denied
Filter->>Client: 403 Forbidden (full response)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 3
🧹 Nitpick comments (4)
src/main/java/org/juv25d/filter/IpFilter.java (2)
75-89: No validation for plain IP addresses.When adding a non-CIDR entry, the IP is added without validation. Invalid entries like
"not-an-ip"or"999.999.999.999"will be stored and never match anything, silently failing to filter.Consider validating plain IPs before adding:
🔧 Example validation
private static final java.util.regex.Pattern IP_PATTERN = java.util.regex.Pattern.compile( "^((25[0-5]|(2[0-4]|1\\d|[1-9]|)\\d)\\.?\\b){4}$"); public void addToWhitelist(String ipOrCidr) { if (ipOrCidr == null || ipOrCidr.isBlank()) return; if (ipOrCidr.contains("/")) { // existing CIDR handling... } else { if (!IP_PATTERN.matcher(ipOrCidr).matches()) { logger.warning("Invalid IP format for whitelist: " + ipOrCidr); return; } whitelist.add(ipOrCidr); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/IpFilter.java` around lines 75 - 89, The addToWhitelist method currently accepts non-CIDR strings without validation, so invalid plain IPs like "not-an-ip" are stored silently; add input validation for the non-CIDR branch by defining a reusable IP_PATTERN (e.g., a static java.util.regex.Pattern) and use it in addToWhitelist before adding to whitelist: if the pattern does not match, log a warning via logger (including the invalid value) and return instead of adding; preserve existing CIDR handling that populates whitelistSubnets and its error logging for IllegalArgumentException.
25-29: Thread-safety note: compound checks inisAllowed()are not atomic.While individual collection operations are thread-safe, the compound check (whitelist → blacklist → default) in
isAllowed()is not atomic. If lists are modified concurrently during a check, an IP could theoretically slip through inconsistently.For most IP filtering use cases this is acceptable (eventual consistency), but if strict consistency is required during runtime updates, consider using a read-write lock or snapshotting the lists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/IpFilter.java` around lines 25 - 29, The isAllowed() method's compound checks over whitelist, blacklist, whitelistSubnets, and blacklistSubnets are not atomic, so concurrent modifications can cause inconsistent decisions; to fix, introduce a ReentrantReadWriteLock (or similar) as a field and acquire the read lock at the start of isAllowed() and release at the end, and acquire the write lock in all mutating methods that add/remove entries from whitelist, blacklist, whitelistSubnets, and blacklistSubnets so checks and updates are properly synchronized.src/test/java/org/juv25d/filter/IpFilterTest.java (2)
94-94: Minor: Inconsistent method naming convention.The method
Ip_inNeitherList_followsDefaultstarts with uppercase, while other test methods use lowercase (e.g.,whitelist_allowsIp). Consider renaming toip_inNeitherList_followsDefaultfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/filter/IpFilterTest.java` at line 94, Rename the test method Ip_inNeitherList_followsDefault to use lowercase-style naming for consistency with other tests (e.g., ip_inNeitherList_followsDefault); update any references to the method name (test runner annotations or calls) and ensure the method signature void ip_inNeitherList_followsDefault(boolean allowByDefault) throws IOException remains otherwise unchanged.
220-261: Consider adding security-focused edge case tests for proxy headers.The proxy header tests cover the happy paths well. However, since
X-Forwarded-ForandX-Real-IPcan be spoofed by clients, consider adding tests for:
- Malformed IP addresses in headers (e.g.,
"not-an-ip, 1.2.3.4")- IPv6 addresses in mixed scenarios
- Whitespace variations (e.g.,
" 1.2.3.4 ")These edge cases would help ensure robust handling of potentially malicious input.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/filter/IpFilterTest.java` around lines 220 - 261, Add security-focused edge-case tests for IpFilter to ensure proxy headers are robust: create new test methods (e.g., malformedXForwardedFor_rejectsOrIgnores, ipv6MixedAddresses_handledCorrectly, xForwardedFor_withWhitespace_trimmed) that construct an IpFilter (as in existing tests), stub req.headers() with cases like "X-Forwarded-For" => "not-an-ip, 1.2.3.4", an IPv6+IPv4 mix, and entries with extra whitespace, then call filter.doFilter(req, res, chain) and assert the expected behavior (verify chain.doFilter called only when the resolved client IP is allowed, and check res.statusCode()/response handling when it's not); reference the existing test patterns (xForwardedFor_takesFirstIp, xRealIp_overridesRemoteIp, xForwardedFor_priorityOverXRealIp) for setup and verification to keep assertions consistent.
🤖 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/IpFilter.java`:
- Around line 242-256: The getClientIp(HttpRequest req) method currently trusts
X-Forwarded-For and X-Real-IP unconditionally, allowing header spoofing; add a
configurable guard (e.g., a boolean field like trustProxyHeaders or a
trustedProxy check) and only read those headers when trustProxyHeaders is true
or when req.remoteIp() is a known/trusted proxy, otherwise return
req.remoteIp(); update getClientIp to consult this flag/check before parsing
headers and document the assumption in JavaDoc for the IpFilter/getClientIp
method.
- Around line 172-186: The doFilter method currently catches Exception, causing
IOException to be swallowed and leaking internal state ("error") to clients;
change the catch to only handle unchecked failures (catch RuntimeException |
Error as appropriate) and rethrow IOException so it can propagate, log the full
exception (use logger.log with the Throwable) for
getClientIp/isAllowed/chain.doFilter failures, and call forbidden(res,
clientFacingMessage) with a generic message (e.g., "IP not allowed") instead of
"error"; ensure chain.doFilter(invocations) still happen for allowed IPs and
unexpected exceptions are not silently swallowed.
In `@src/test/java/org/juv25d/filter/IpFilterTest.java`:
- Around line 166-169: The test method removeIpOrRange_blacklist has an
incorrect `@DisplayName` mentioning "whitelist" while the test actually removes
entries from the blacklist; update the `@DisplayName` string to accurately
describe the test (e.g., "Fall back on blacklist/default after removing IP or
CIDR range from blacklist") so it reflects the behavior under test in the
removeIpOrRange_blacklist parameterized test.
---
Nitpick comments:
In `@src/main/java/org/juv25d/filter/IpFilter.java`:
- Around line 75-89: The addToWhitelist method currently accepts non-CIDR
strings without validation, so invalid plain IPs like "not-an-ip" are stored
silently; add input validation for the non-CIDR branch by defining a reusable
IP_PATTERN (e.g., a static java.util.regex.Pattern) and use it in addToWhitelist
before adding to whitelist: if the pattern does not match, log a warning via
logger (including the invalid value) and return instead of adding; preserve
existing CIDR handling that populates whitelistSubnets and its error logging for
IllegalArgumentException.
- Around line 25-29: The isAllowed() method's compound checks over whitelist,
blacklist, whitelistSubnets, and blacklistSubnets are not atomic, so concurrent
modifications can cause inconsistent decisions; to fix, introduce a
ReentrantReadWriteLock (or similar) as a field and acquire the read lock at the
start of isAllowed() and release at the end, and acquire the write lock in all
mutating methods that add/remove entries from whitelist, blacklist,
whitelistSubnets, and blacklistSubnets so checks and updates are properly
synchronized.
In `@src/test/java/org/juv25d/filter/IpFilterTest.java`:
- Line 94: Rename the test method Ip_inNeitherList_followsDefault to use
lowercase-style naming for consistency with other tests (e.g.,
ip_inNeitherList_followsDefault); update any references to the method name (test
runner annotations or calls) and ensure the method signature void
ip_inNeitherList_followsDefault(boolean allowByDefault) throws IOException
remains otherwise unchanged.
- Around line 220-261: Add security-focused edge-case tests for IpFilter to
ensure proxy headers are robust: create new test methods (e.g.,
malformedXForwardedFor_rejectsOrIgnores, ipv6MixedAddresses_handledCorrectly,
xForwardedFor_withWhitespace_trimmed) that construct an IpFilter (as in existing
tests), stub req.headers() with cases like "X-Forwarded-For" => "not-an-ip,
1.2.3.4", an IPv6+IPv4 mix, and entries with extra whitespace, then call
filter.doFilter(req, res, chain) and assert the expected behavior (verify
chain.doFilter called only when the resolved client IP is allowed, and check
res.statusCode()/response handling when it's not); reference the existing test
patterns (xForwardedFor_takesFirstIp, xRealIp_overridesRemoteIp,
xForwardedFor_priorityOverXRealIp) for setup and verification to keep assertions
consistent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pom.xmlsrc/main/java/org/juv25d/filter/IpFilter.javasrc/test/java/org/juv25d/filter/IpFilterTest.java
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/org/juv25d/filter/IpFilter.java (2)
175-187:⚠️ Potential issue | 🟠 MajorDo not swallow
IOExceptionand unexpected failures as 403.Line 184 catches all exceptions, which masks internal failures and turns them into
403 Forbiddenresponses ("error"on Line 186). This changes failure semantics and hides real server issues.Proposed fix
`@Override` public void doFilter(HttpRequest req, HttpResponse res, FilterChain chain) throws IOException { - try { - String clientIp = getClientIp(req); - - if (isAllowed(clientIp)) { - chain.doFilter(req, res); - } else { - logger.fine("IP blocked: " + clientIp); - forbidden(res, clientIp); - } - } catch (Exception e) { - logger.severe("Error in IP filter: " + e.getMessage()); - forbidden(res, "error"); - } + String clientIp = getClientIp(req); + if (isAllowed(clientIp)) { + chain.doFilter(req, res); + } else { + logger.fine("IP blocked: " + clientIp); + forbidden(res, clientIp); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/IpFilter.java` around lines 175 - 187, The current catch-all (catch (Exception e)) in the IP filter swallows IO/Servlet errors and converts them to 403; change this so IO and Servlet exceptions are not converted to Forbidden. Specifically, in the block using getClientIp, isAllowed, chain.doFilter and forbidden, remove or narrow the catch-all: rethrow IOException and ServletException (or don't catch them at all) so the container handles them, and only handle unexpected runtime errors by logging via logger.severe and returning a 500 (e.g. res.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR)) instead of calling forbidden; ensure you reference and preserve getClientIp, isAllowed, chain.doFilter and forbidden when making these changes.
244-257:⚠️ Potential issue | 🟠 MajorGuard proxy-header trust to prevent IP spoofing.
Lines 247-255 trust
X-Forwarded-For/X-Real-IPunconditionally. If traffic can reach this service without a trusted proxy boundary, attackers can spoof headers and bypass IP filtering.Proposed fix
+private final boolean trustProxyHeaders; + +public IpFilter(`@Nullable` Set<String> whitelist, `@Nullable` Set<String> blacklist, boolean allowByDefault, boolean trustProxyHeaders) { + // existing init... + this.allowByDefault = allowByDefault; + this.trustProxyHeaders = trustProxyHeaders; +} + private String getClientIp(HttpRequest req) { - Map<String, String> headers = req.headers(); - - String ip = headers.get("X-Forwarded-For"); - if (ip != null && !ip.isBlank()) { - return ip.split(",")[0].trim(); - } - - ip = headers.get("X-Real-IP"); - if (ip != null && !ip.isBlank()) { - return ip.trim(); + if (trustProxyHeaders) { + Map<String, String> headers = req.headers(); + String ip = headers.get("X-Forwarded-For"); + if (ip != null && !ip.isBlank()) { + return ip.split(",")[0].trim(); + } + ip = headers.get("X-Real-IP"); + if (ip != null && !ip.isBlank()) { + return ip.trim(); + } } - return req.remoteIp(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/IpFilter.java` around lines 244 - 257, The getClientIp method in IpFilter currently trusts X-Forwarded-For and X-Real-IP unconditionally; change it to only use those headers when the request comes from a trusted proxy or when a configuration flag indicates the app is behind a proxy. Update getClientIp(HttpRequest req) (and any call sites in IpFilter) to first verify req.remoteIp() is in a configured trusted proxy list or that a boolean config (e.g., isBehindProxy/trustProxy) is true; if not trusted, ignore X-Forwarded-For/X-Real-IP and return req.remoteIp() directly. Ensure the trusted proxies/configuration is plumbed into IpFilter (constructor or DI) so the method can validate the remote IP before honoring proxy headers.
🤖 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/IpFilter.java`:
- Around line 35-43: The JavaDoc for the IpFilter constructor incorrectly
declares it throws IllegalArgumentException; update the constructor JavaDoc (on
IpFilter#IpFilter) to remove the `@throws` tag and instead document the actual
behavior: invalid CIDR entries passed to the constructor are handled inside
addToWhitelist/addToBlacklist (they are caught, logged, and ignored), and the
constructor will not throw for invalid CIDRs. Reference IpFilter#addToWhitelist
and IpFilter#addToBlacklist in the doc so readers know where invalid entries are
processed.
- Around line 78-90: Normalize the ipOrCidr input by trimming whitespace before
any add/remove/check operations so entries like " 10.0.0.0/8 " don't get
stored/removed inconsistently; in IpFilter.java, update every block that reads
the ipOrCidr parameter (the branches that populate whitelistSubnets/whitelist
and the corresponding blacklistSubnets/blacklist code paths) to first do
something like ipOrCidr = ipOrCidr == null ? null : ipOrCidr.trim(), then keep
the existing null/isBlank checks and use the trimmed value when constructing
SubnetUtils, calling
whitelist.add(...)/whitelistSubnets.put(...)/blacklist.add(...)/blacklistSubnets.put(...),
and when removing entries so all comparisons use the normalized string.
---
Duplicate comments:
In `@src/main/java/org/juv25d/filter/IpFilter.java`:
- Around line 175-187: The current catch-all (catch (Exception e)) in the IP
filter swallows IO/Servlet errors and converts them to 403; change this so IO
and Servlet exceptions are not converted to Forbidden. Specifically, in the
block using getClientIp, isAllowed, chain.doFilter and forbidden, remove or
narrow the catch-all: rethrow IOException and ServletException (or don't catch
them at all) so the container handles them, and only handle unexpected runtime
errors by logging via logger.severe and returning a 500 (e.g.
res.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR)) instead of calling
forbidden; ensure you reference and preserve getClientIp, isAllowed,
chain.doFilter and forbidden when making these changes.
- Around line 244-257: The getClientIp method in IpFilter currently trusts
X-Forwarded-For and X-Real-IP unconditionally; change it to only use those
headers when the request comes from a trusted proxy or when a configuration flag
indicates the app is behind a proxy. Update getClientIp(HttpRequest req) (and
any call sites in IpFilter) to first verify req.remoteIp() is in a configured
trusted proxy list or that a boolean config (e.g., isBehindProxy/trustProxy) is
true; if not trusted, ignore X-Forwarded-For/X-Real-IP and return req.remoteIp()
directly. Ensure the trusted proxies/configuration is plumbed into IpFilter
(constructor or DI) so the method can validate the remote IP before honoring
proxy headers.
jesperlarsson1910
marked this pull request as draft
February 26, 2026 11:55
jesperlarsson1910
marked this pull request as ready for review
February 26, 2026 13:39
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/IpFilter.java`:
- Around line 262-271: The header lookup for client IP in IpFilter is
case-sensitive and can miss headers; update the logic that uses the headers Map
(variable headers and the checks for "X-Forwarded-For" and "X-Real-IP") to
perform case-insensitive key matching (e.g., find the first map entry whose
key.equalsIgnoreCase("X-Forwarded-For") or key.equalsIgnoreCase("X-Real-IP") and
use its value), falling back to the next header or default if absent; ensure
trimming and splitting logic remains the same once the header value is obtained.
- Around line 44-57: The new 4-arg IpFilter constructor removed the prior 3-arg
API; restore backwards compatibility by adding the original 3-argument
constructor IpFilter(`@Nullable` Set<String> whitelist, `@Nullable` Set<String>
blacklist, boolean allowByDefault) that delegates to the 4-arg constructor
passing a sensible default for trustProxyHeaders (e.g., false). Ensure both
constructors still populate whitelist/blacklist via
addToWhitelist/addToBlacklist and set the allowByDefault and trustProxyHeaders
fields as the existing 4-arg constructor does.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/juv25d/config/IpFilterConfig.javasrc/main/java/org/juv25d/filter/IpFilter.javasrc/test/java/org/juv25d/filter/IpFilterTest.java
johanbriger
approved these changes
Feb 27, 2026
johanbriger
left a comment
johanbriger
left a comment
There was a problem hiding this comment.
Great work! Solid improvements to the IpFilter
Approved
jesperlarsson1910
deleted the
98-extend-ipfilter-functionality-with-subnets
branch
February 27, 2026 12:26
jesperlarsson1910
restored the
98-extend-ipfilter-functionality-with-subnets
branch
February 27, 2026 22:59
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.
Enhanced IP filtering with CIDR subnet support, proxy awareness, and thread-safety improvements
New Features
192.168.1.0/24or10.0.0.0/8X-Forwarded-ForandX-Real-IPheadersaddToWhitelist(),removeFromBlacklist(), etc.getWhitelistIps(),getBlacklistSubnets(), etc.Improvements
ConcurrentHashMapfor safe concurrent accessDocumentation
Testing
Should be fully backwards compatible.
Summary by CodeRabbit
New Features
Tests
Chores