allow using both white and blacklist at the same time with a configurable default response - #99
Merged
Merged
Conversation
…able default response 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 (1)
📝 WalkthroughWalkthroughRefactors IpFilter to centralize access logic into a new public isAllowed(String) gate, adds internal whitelist/blacklist state and an allowByDefault flag with a new three-argument constructor, updates doFilter to delegate to isAllowed, adapts tests to the new constructor and shared fixtures, and updates App to call the new constructor. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 (4)
src/main/java/org/juv25d/filter/IpFilter.java (2)
49-49: Consider narrowingisAllowedvisibility to package-private.
isAllowedis an implementation detail of the filter; all existing tests reach it throughdoFilter. Declaring itpublicpermanently widens the API contract ofIpFilter.♻️ Proposed change
- public boolean isAllowed(String ip) { + boolean isAllowed(String ip) {🤖 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` at line 49, The method isAllowed in class IpFilter is unnecessarily public; change its visibility to package-private (remove the public modifier) so it becomes an implementation detail, keep its signature otherwise, and ensure any external callers use doFilter (tests already do), updating any direct calls to IpFilter.isAllowed to go through doFilter or move tests into the same package if they must access it.
18-36: Delegate the 2-arg constructor to the 3-arg one to eliminate duplication.Both constructors repeat identical null-check and
addAlllogic.♻️ Proposed refactor
public IpFilter(Set<String> whitelist, Set<String> blacklist) { - if (whitelist != null) { - this.whitelist.addAll(whitelist); - } - if (blacklist != null) { - this.blacklist.addAll(blacklist); - } - this.allowByDefault = true; + this(whitelist, blacklist, true); }🤖 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 18 - 36, The two IpFilter constructors duplicate the same null-check and addAll logic; refactor the 2-arg constructor to delegate to the 3-arg one by calling the three-argument constructor (IpFilter(Set<String>, Set<String>, boolean)) with allowByDefault=true so all initialization (whitelist.addAll, blacklist.addAll, and allowByDefault assignment) is centralized in the single constructor and duplication is removed.src/test/java/org/juv25d/filter/IpFilterTest.java (2)
17-91: Minor naming inconsistency across test methods.Test names mix
Ip(camelCase) andIP(all-caps), andinBothList(singular) vsinBothLists(plural). Suggest picking one convention and applying it consistently — e.g. alwaysIpto stay idiomatic with Java's camelCase convention.🤖 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 17 - 91, Rename the test methods in IpFilterTest to use a consistent naming convention (prefer Java camelCase with "Ip" and plural "Lists" where appropriate); for example, rename allowsIP_inNeitherList_noChangeDefault -> allowsIp_inNeitherLists_noChangeDefault, blocksIP_inBothList_ChangeDefault -> blocksIp_inBothLists_changeDefault, blocksIP_inNeitherList_ChangeDefault -> blocksIp_inNeitherLists_changeDefault, and adjust any references to these methods in the class so all test names consistently use "Ip" (not "IP") and "Lists" (not mixed singular/plural).
17-91: Two important coverage gaps after the removal ofwhitelist_blocksIpNotInList.The test suite no longer covers:
- Whitelist provided, IP not in the list, default constructor — confirms the new allow-by-default semantic that replaces the old blocking behavior. Without this, the behavioral change from the 2-arg constructor is not explicitly documented in tests.
- Whitelist provided, IP not in the list,
allowByDefault=false— the "strict whitelist" mode that existing callers may expect from the old constructor.✅ Suggested additional test cases
`@Test` void allowsIP_notInWhitelist_noChangeDefault() throws IOException { // 2-arg constructor: allowByDefault=true → IP not in whitelist should now be ALLOWED IpFilter filter = new IpFilter(Set.of("192.168.1.1"), null); when(req.remoteIp()).thenReturn("127.0.0.1"); filter.doFilter(req, res, chain); verify(chain).doFilter(req, res); assertEquals(200, res.statusCode()); } `@Test` void blocksIP_notInWhitelist_ChangeDefault() throws IOException { // 3-arg constructor with allowByDefault=false: strict whitelist mode IpFilter filter = new IpFilter(Set.of("192.168.1.1"), null, false); when(req.remoteIp()).thenReturn("127.0.0.1"); filter.doFilter(req, res, chain); verify(chain, never()).doFilter(req, res); assertEquals(403, res.statusCode()); assertEquals("Forbidden", res.statusText()); }🤖 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 17 - 91, The test suite is missing coverage for the "IP not in whitelist" cases introduced by the new IpFilter semantics; add two tests in IpFilterTest targeting the IpFilter constructors: 1) a test (e.g., allowsIP_notInWhitelist_noChangeDefault) that constructs IpFilter with new IpFilter(Set.of("192.168.1.1"), null) and asserts the request is allowed (chain.doFilter called, statusCode 200) when req.remoteIp() returns "127.0.0.1"; 2) a test (e.g., blocksIP_notInWhitelist_ChangeDefault) that constructs IpFilter with new IpFilter(Set.of("192.168.1.1"), null, false) and asserts the request is blocked (chain.doFilter never called, statusCode 403 and statusText "Forbidden") when req.remoteIp() returns "127.0.0.1". Ensure both tests use the same mocked req/res/chain setup as other tests.
🤖 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 18-26: The two-arg constructor IpFilter(Set<String> whitelist,
Set<String> blacklist) currently sets allowByDefault = true causing a breaking
semantic change; revert to the original strict-whitelist default by delegating
to or setting the three-arg behavior: call or mimic the three-arg constructor
(IpFilter(Set<String>, Set<String>, boolean)) with allowByDefault = false (or
explicitly set this.allowByDefault = false) so that IPs not in either list are
denied unless the caller opts into permissive behavior; update the constructor
body that manipulates whitelist/blacklist to reuse the same initialization logic
as the 3-arg constructor (or delegate to it) to avoid duplication.
---
Nitpick comments:
In `@src/main/java/org/juv25d/filter/IpFilter.java`:
- Line 49: The method isAllowed in class IpFilter is unnecessarily public;
change its visibility to package-private (remove the public modifier) so it
becomes an implementation detail, keep its signature otherwise, and ensure any
external callers use doFilter (tests already do), updating any direct calls to
IpFilter.isAllowed to go through doFilter or move tests into the same package if
they must access it.
- Around line 18-36: The two IpFilter constructors duplicate the same null-check
and addAll logic; refactor the 2-arg constructor to delegate to the 3-arg one by
calling the three-argument constructor (IpFilter(Set<String>, Set<String>,
boolean)) with allowByDefault=true so all initialization (whitelist.addAll,
blacklist.addAll, and allowByDefault assignment) is centralized in the single
constructor and duplication is removed.
In `@src/test/java/org/juv25d/filter/IpFilterTest.java`:
- Around line 17-91: Rename the test methods in IpFilterTest to use a consistent
naming convention (prefer Java camelCase with "Ip" and plural "Lists" where
appropriate); for example, rename allowsIP_inNeitherList_noChangeDefault ->
allowsIp_inNeitherLists_noChangeDefault, blocksIP_inBothList_ChangeDefault ->
blocksIp_inBothLists_changeDefault, blocksIP_inNeitherList_ChangeDefault ->
blocksIp_inNeitherLists_changeDefault, and adjust any references to these
methods in the class so all test names consistently use "Ip" (not "IP") and
"Lists" (not mixed singular/plural).
- Around line 17-91: The test suite is missing coverage for the "IP not in
whitelist" cases introduced by the new IpFilter semantics; add two tests in
IpFilterTest targeting the IpFilter constructors: 1) a test (e.g.,
allowsIP_notInWhitelist_noChangeDefault) that constructs IpFilter with new
IpFilter(Set.of("192.168.1.1"), null) and asserts the request is allowed
(chain.doFilter called, statusCode 200) when req.remoteIp() returns "127.0.0.1";
2) a test (e.g., blocksIP_notInWhitelist_ChangeDefault) that constructs IpFilter
with new IpFilter(Set.of("192.168.1.1"), null, false) and asserts the request is
blocked (chain.doFilter never called, statusCode 403 and statusText "Forbidden")
when req.remoteIp() returns "127.0.0.1". Ensure both tests use the same mocked
req/res/chain setup as other tests.
SandraNelj
previously approved these changes
Feb 19, 2026
SandraNelj
left a comment
SandraNelj
left a comment
There was a problem hiding this comment.
This looks good! The ability to configure default behavior for whitelist/blacklist makes the filter more flexible. The updated tests also help verify the behavior clearly!
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
bamsemats
previously approved these changes
Feb 20, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
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-29:⚠️ Potential issue | 🔴 CriticalStale 2-arg
IpFilterconstructor call will fail to compile after the conflict is resolved.The 2-arg
IpFilter(Set, Set)constructor was removed in this PR (only the 3-arg form exists now). Line 29 must be deleted — its role is superseded by the 3-arg instantiation inside the conflict block (lines 44–48).🐛 Proposed fix — remove the stale registration
- pipeline.addGlobalFilter(new IpFilter(Set.of(), Set.of()), 2); -🤖 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 29, Remove the stale 2-arg registration call to IpFilter by deleting the line invoking pipeline.addGlobalFilter(new IpFilter(Set.of(), Set.of()), 2); because the codebase now only provides the 3-arg IpFilter constructor and the filter is already registered in the conflict block via the 3-arg instantiation (lines around the conflict). Ensure only the 3-arg IpFilter(...) registration remains and no other references to the removed 2-arg constructor are left.
🧹 Nitpick comments (2)
src/test/java/org/juv25d/filter/IpFilterTest.java (1)
43-66: Good new coverage; add the symmetricallowByDefaultcounterparts.The two new tests cover
(both-lists, default=true)and(neither-list, default=false). The symmetric cases —(both-lists, default=false)→ blocked, and(neither-list, default=true)→ allowed — are equally non-obvious and currently untested.✅ Suggested additional tests
`@Test` void blocksIP_inBothList_defaultFalse() throws IOException { IpFilter filter = new IpFilter(Set.of("127.0.0.1"), Set.of("127.0.0.1"), false); when(req.remoteIp()).thenReturn("127.0.0.1"); filter.doFilter(req, res, chain); verify(chain, never()).doFilter(req, res); assertEquals(403, res.statusCode()); } `@Test` void allowsIP_inNeitherList_defaultTrue() throws IOException { IpFilter filter = new IpFilter(null, null, true); when(req.remoteIp()).thenReturn("127.0.0.1"); filter.doFilter(req, res, chain); verify(chain).doFilter(req, res); assertEquals(200, res.statusCode()); }🤖 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 43 - 66, Add two symmetric unit tests for IpFilter to cover the missing allowByDefault variants: create a test method blocksIP_inBothList_defaultFalse that constructs new IpFilter(Set.of("127.0.0.1"), Set.of("127.0.0.1"), false), stubs req.remoteIp() to "127.0.0.1", invokes filter.doFilter(req, res, chain), then verify(chain, never()).doFilter(req, res) and assert res.statusCode() is 403; and create allowsIP_inNeitherList_defaultTrue that constructs new IpFilter(null, null, true), stubs req.remoteIp(), invokes doFilter, then verify(chain).doFilter(req, res) and assert res.statusCode() is 200.src/main/java/org/juv25d/filter/IpFilter.java (1)
41-50:isAllowedlogic is correct; consider documenting null-IP behavior.The four-case precedence (both →
allowByDefault, whitelist-only →true, blacklist-only →false, neither →allowByDefault) is well-defined and aligns with the tests. One edge case worth a brief comment: ifgetClientIpever returnsnull(e.g., behind a proxy),HashSet.contains(null)returnsfalse, so the call quietly falls through toallowByDefault. This is safe but silent.💡 Optional: guard or document the null-IP fallback
public boolean isAllowed(String ip) { + // null IP (e.g., missing X-Forwarded-For) defers to allowByDefault if (whitelist.contains(ip) && blacklist.contains(ip)) return allowByDefault;🤖 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 41 - 50, The isAllowed method's behavior when ip is null should be made explicit: inside isAllowed(String ip) (which currently checks whitelist, blacklist and allowByDefault), either add an early null check (e.g., if (ip == null) return allowByDefault) or add a concise comment above the method documenting that getClientIp may return null and that contains(null) will be treated as "neither" and thus fall back to allowByDefault; update the isAllowed method or its javadoc accordingly and reference whitelist, blacklist and allowByDefault in the comment.
🤖 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/App.java`:
- Around line 39-50: Remove the raw merge conflict markers and the duplicated
RedirectFilter registration: delete the conflict blocks (<<<<<<<, =======,
>>>>>>>) and the extra pipeline.addGlobalFilter(new
RedirectFilter(redirectRules), 0) introduced by the fix branch, leaving only the
pipeline.addGlobalFilter(new IpFilter(Set.of(), Set.of(), true), 0) call; ensure
the remaining pipeline.addGlobalFilter usages (including the existing
RedirectFilter at position 4) remain intact and compile.
In `@src/test/java/org/juv25d/filter/IpFilterTest.java`:
- Line 45: The test contains a stray double semicolon after constructing the
IpFilter instance in IpFilterTest; remove the extra semicolon so the line using
new IpFilter(Set.of("127.0.0.1"), Set.of("127.0.0.1"), true) ends with a single
semicolon to fix the syntax error.
---
Outside diff comments:
In `@src/main/java/org/juv25d/App.java`:
- Line 29: Remove the stale 2-arg registration call to IpFilter by deleting the
line invoking pipeline.addGlobalFilter(new IpFilter(Set.of(), Set.of()), 2);
because the codebase now only provides the 3-arg IpFilter constructor and the
filter is already registered in the conflict block via the 3-arg instantiation
(lines around the conflict). Ensure only the 3-arg IpFilter(...) registration
remains and no other references to the removed 2-arg constructor are left.
---
Nitpick comments:
In `@src/main/java/org/juv25d/filter/IpFilter.java`:
- Around line 41-50: The isAllowed method's behavior when ip is null should be
made explicit: inside isAllowed(String ip) (which currently checks whitelist,
blacklist and allowByDefault), either add an early null check (e.g., if (ip ==
null) return allowByDefault) or add a concise comment above the method
documenting that getClientIp may return null and that contains(null) will be
treated as "neither" and thus fall back to allowByDefault; update the isAllowed
method or its javadoc accordingly and reference whitelist, blacklist and
allowByDefault in the comment.
In `@src/test/java/org/juv25d/filter/IpFilterTest.java`:
- Around line 43-66: Add two symmetric unit tests for IpFilter to cover the
missing allowByDefault variants: create a test method
blocksIP_inBothList_defaultFalse that constructs new
IpFilter(Set.of("127.0.0.1"), Set.of("127.0.0.1"), false), stubs req.remoteIp()
to "127.0.0.1", invokes filter.doFilter(req, res, chain), then verify(chain,
never()).doFilter(req, res) and assert res.statusCode() is 403; and create
allowsIP_inNeitherList_defaultTrue that constructs new IpFilter(null, null,
true), stubs req.remoteIp(), invokes doFilter, then verify(chain).doFilter(req,
res) and assert res.statusCode() is 200.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/juv25d/App.javasrc/main/java/org/juv25d/filter/IpFilter.javasrc/test/java/org/juv25d/filter/IpFilterTest.java
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
bamsemats
approved these changes
Feb 23, 2026
FionaSprinkles
approved these changes
Feb 23, 2026
This was referenced Feb 25, 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
4 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.
Added a field for default response and a new constructor to configure it, if the other constructor is used it is set to true.
Created a sub issue #98 to expand on this functionality but limiting this PR to the first issue.
Updated the tests to check the new logic and moved repeated fields out of the methods.
Summary by CodeRabbit
New Features
Tests