98 extend ipfilter functionality with subnets#152
Merged
jesperlarsson1910 merged 9 commits intomainfrom Feb 28, 2026
Merged
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>
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
|
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 (3)
📝 WalkthroughWalkthroughThe IP filter class is enhanced with CIDR subnet support, thread-safe collections for whitelist/blacklist management, proxy header trust configuration, dynamic list management methods, and state getter methods. Tests are updated to align with new constructor signatures. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant IpFilter as IpFilter
participant SubnetUtils as SubnetUtils
participant Logger as Logger
Client->>IpFilter: doFilter(request)
IpFilter->>IpFilter: getClientIp(request)
Note over IpFilter: Extract IP from request or proxy headers
IpFilter->>IpFilter: isAllowed(clientIp)
alt IP in whitelist IPs
IpFilter->>Logger: Log approved (whitelist)
IpFilter-->>Client: Allow request
else IP in whitelist subnets
IpFilter->>SubnetUtils: Check CIDR containment
SubnetUtils-->>IpFilter: Containment result
IpFilter->>Logger: Log approved (subnet)
IpFilter-->>Client: Allow request
else IP in blacklist IPs
IpFilter->>Logger: Log blocked (blacklist)
IpFilter-->>Client: forbidden() - 403 response
else IP in blacklist subnets
IpFilter->>SubnetUtils: Check CIDR containment
SubnetUtils-->>IpFilter: Containment result
IpFilter->>Logger: Log blocked (subnet)
IpFilter-->>Client: forbidden() - 403 response
else allowByDefault flag
alt True
IpFilter-->>Client: Allow request
else False
IpFilter->>Logger: Log blocked (default deny)
IpFilter-->>Client: forbidden() - 403 response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 1
🧹 Nitpick comments (3)
src/main/java/org/juv25d/config/IpFilterConfig.java (1)
22-23: Consider consistent formatting for getter.The
trustProxyHeaders()getter uses single-line formatting whileallowByDefault()uses multi-line. Minor inconsistency.♻️ Consistent formatting
- public boolean trustProxyHeaders() {return trustProxyHeaders;} + public boolean trustProxyHeaders() { + return trustProxyHeaders; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/config/IpFilterConfig.java` around lines 22 - 23, The getter formatting is inconsistent: change the single-line method public boolean trustProxyHeaders() {return trustProxyHeaders;} to match the multi-line style used by allowByDefault() by expanding it into a conventional multiline getter (use the same brace/indent style and spacing as allowByDefault()); locate the trustProxyHeaders() method in IpFilterConfig and update its formatting only (no logic changes).src/test/java/org/juv25d/filter/IpFilterTest.java (1)
94-107: Minor naming convention inconsistency.Method name
Ip_inNeitherList_followsDefaultstarts with a capitalI, which is inconsistent with other test methods that uselowercase_camelCasepattern (e.g.,whitelist_allowsIp,blacklist_blocksIp).♻️ Consistent naming
- void Ip_inNeitherList_followsDefault(boolean allowByDefault) throws IOException { + void ip_inNeitherList_followsDefault(boolean allowByDefault) throws IOException {🤖 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 94 - 107, Rename the test method Ip_inNeitherList_followsDefault in class IpFilterTest to follow lowercase_camelCase (e.g., ip_inNeitherList_followsDefault or ipInNeitherList_followsDefault) to match other tests like whitelist_allowsIp and blacklist_blocksIp; update any references to this method (test runner or annotations if present) so the test continues to execute under the new name.src/main/java/org/juv25d/filter/IpFilter.java (1)
69-82: Consider constructor delegation to reduce duplication.The 4-argument constructor duplicates initialization logic from the 3-argument constructor. Using
this(...)delegation improves maintainability.♻️ Refactor using constructor delegation
- public IpFilter(`@Nullable` Set<String> whitelist, `@Nullable` Set<String> blacklist, boolean allowByDefault, boolean trustProxyHeaders) { - if (whitelist != null) { - for (String entry : whitelist) { - addToWhitelist(entry); - } - } - if (blacklist != null) { - for (String entry : blacklist) { - addToBlacklist(entry); - } - } - this.allowByDefault = allowByDefault; - this.trustProxyHeaders = trustProxyHeaders; - } + public IpFilter(`@Nullable` Set<String> whitelist, `@Nullable` Set<String> blacklist, boolean allowByDefault, boolean trustProxyHeaders) { + this.allowByDefault = allowByDefault; + this.trustProxyHeaders = trustProxyHeaders; + if (whitelist != null) { + whitelist.forEach(this::addToWhitelist); + } + if (blacklist != null) { + blacklist.forEach(this::addToBlacklist); + } + }Then update the 3-argument constructor to delegate:
public IpFilter(`@Nullable` Set<String> whitelist, `@Nullable` Set<String> blacklist, boolean allowByDefault) { - if (whitelist != null) { - for (String entry : whitelist) { - addToWhitelist(entry); - } - } - if (blacklist != null) { - for (String entry : blacklist) { - addToBlacklist(entry); - } - } - this.allowByDefault = allowByDefault; - this.trustProxyHeaders = false; + this(whitelist, blacklist, allowByDefault, false); }🤖 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 69 - 82, Refactor the IpFilter constructors to delegate: replace the current 4-arg constructor body with a call to the existing 3-arg constructor using this(whitelist, blacklist, allowByDefault) as the first statement, then set this.trustProxyHeaders = trustProxyHeaders; this removes the duplicated whitelist/blacklist initialization while keeping the existing addToWhitelist/addToBlacklist logic in the 3-arg constructor (refer to the IpFilter( Set<String>, Set<String>, boolean ) and IpFilter( Set<String>, Set<String>, boolean, boolean ) constructors).
🤖 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 36-59: The Javadoc for the IpFilter(Set<String>, Set<String>,
boolean) constructor wrongly states it sets trustProxyHeaders = true while the
constructor actually assigns this.trustProxyHeaders = false; update the Javadoc
to reflect the actual behavior (change the sentence "This constructor sets
{`@code` trustProxyHeaders = true}" to indicate it sets trustProxyHeaders to
false) or alternatively change the constructor assignment if the intended
behavior was to trust proxy headers; locate the IpFilter constructor and the
trustProxyHeaders field to make the correction so docs and implementation match.
---
Nitpick comments:
In `@src/main/java/org/juv25d/config/IpFilterConfig.java`:
- Around line 22-23: The getter formatting is inconsistent: change the
single-line method public boolean trustProxyHeaders() {return
trustProxyHeaders;} to match the multi-line style used by allowByDefault() by
expanding it into a conventional multiline getter (use the same brace/indent
style and spacing as allowByDefault()); locate the trustProxyHeaders() method in
IpFilterConfig and update its formatting only (no logic changes).
In `@src/main/java/org/juv25d/filter/IpFilter.java`:
- Around line 69-82: Refactor the IpFilter constructors to delegate: replace the
current 4-arg constructor body with a call to the existing 3-arg constructor
using this(whitelist, blacklist, allowByDefault) as the first statement, then
set this.trustProxyHeaders = trustProxyHeaders; this removes the duplicated
whitelist/blacklist initialization while keeping the existing
addToWhitelist/addToBlacklist logic in the 3-arg constructor (refer to the
IpFilter( Set<String>, Set<String>, boolean ) and IpFilter( Set<String>,
Set<String>, boolean, boolean ) constructors).
In `@src/test/java/org/juv25d/filter/IpFilterTest.java`:
- Around line 94-107: Rename the test method Ip_inNeitherList_followsDefault in
class IpFilterTest to follow lowercase_camelCase (e.g.,
ip_inNeitherList_followsDefault or ipInNeitherList_followsDefault) to match
other tests like whitelist_allowsIp and blacklist_blocksIp; update any
references to this method (test runner or annotations if present) so the test
continues to execute under the new name.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pom.xmlsrc/main/java/org/juv25d/config/IpFilterConfig.javasrc/main/java/org/juv25d/filter/IpFilter.javasrc/test/java/org/juv25d/filter/IpFilterTest.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/org/juv25d/filter/IpFilterTest.java (1)
34-43: Inconsistent constructor usage compared to updated tests.This test uses the 4-arg constructor
new IpFilter(Set.of("127.0.0.1"), null, false, false), but the test doesn't exercise proxy header functionality. The parallel testwhitelist_allowsIpInRangeat line 48 was updated to use the 3-arg constructor.For consistency across the test suite, consider updating this line as well.
♻️ Suggested change for consistency
`@Test` `@DisplayName`("Allow ip only in whitelist") void whitelist_allowsIp() throws IOException { - IpFilter filter = new IpFilter(Set.of("127.0.0.1"), null, false, false); + IpFilter filter = new IpFilter(Set.of("127.0.0.1"), null, false); filter.doFilter(req, res, chain);🤖 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 34 - 43, The test whitelist_allowsIp uses the 4-arg IpFilter constructor but should match the updated tests (like whitelist_allowsIpInRange) that use the 3-arg constructor; update the instantiation in method whitelist_allowsIp to call new IpFilter(Set.of("127.0.0.1"), null, false) (i.e., remove the extra boolean argument) so constructor usage is consistent across tests and proxy-header behavior isn't accidentally enabled.
🤖 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/test/java/org/juv25d/filter/IpFilterTest.java`:
- Around line 34-43: The test whitelist_allowsIp uses the 4-arg IpFilter
constructor but should match the updated tests (like whitelist_allowsIpInRange)
that use the 3-arg constructor; update the instantiation in method
whitelist_allowsIp to call new IpFilter(Set.of("127.0.0.1"), null, false) (i.e.,
remove the extra boolean argument) so constructor usage is consistent across
tests and proxy-header behavior isn't accidentally enabled.
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
OskarLundqvist33
approved these changes
Feb 28, 2026
OskarLundqvist33
left a comment
OskarLundqvist33
left a comment
There was a problem hiding this comment.
This significantly enhances the IP filtering functionality with CIDR range support, proxy header handling, and dynamic list updates — all of which are valuable upgrades and well implemented. The tests align well with the expected behaviour. It looks great, nice work!
johanbriger
approved these changes
Feb 28, 2026
jesperlarsson1910
deleted the
98-extend-ipfilter-functionality-with-subnets
branch
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.
Missing last commit from #140
Summary by CodeRabbit