fix: correctly filter IPs from CIDR during aggregation#684
fix: correctly filter IPs from CIDR during aggregation#684Mzack9999 merged 1 commit intoprojectdiscovery:mainfrom
Conversation
jjhwan-h
force-pushed
the
fix/ip-filtering
branch
4 times, most recently
from
b60c971 to
18d432c
Compare
WalkthroughAdds logic to expand CIDRs to individual IP CIDRs when both FilterIP and Aggregate are set, exclude filtered IPs before aggregation, and adjusts IPv4→IPv6 mapping to avoid double-mapping for already-mapped addresses. Also adds a unit test covering the filtering+aggregation scenario. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as mapcidr CLI
participant P as Processor
participant R as Ranger/Aggregator
U->>CLI: Run with -cl CIDRs, -fi FilterIP, -a
CLI->>P: Parse options and inputs
alt FilterIP && Aggregate
P->>P: enumerate IPs in each CIDR
P->>P: exclude IPs in FilterIP
loop for each remaining IP
P->>P: create single-IP CIDR (/32 or /128)
P->>R: add single-IP CIDR to aggregation set
end
R-->>CLI: perform aggregation/coalesce/sort
else Other modes
P->>R: existing path (coalesce/sort/shuffle/approx/count)
R-->>CLI: result
end
CLI-->>U: Output CIDRs
sequenceDiagram
autonumber
participant IP as Caller
participant N as ipNetToRange
IP->>N: firstIP, lastIP
alt firstIP is IPv4 and not already IPv4-mapped-IPv6
N->>N: map both ends to IPv4-mapped IPv6 form
else Already mapped or native IPv6
N->>N: leave addresses unchanged
end
N-->>IP: normalized range
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (3 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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)
ip.go (1)
282-285: Good guard against double IPv4-mapped IPv6 conversion; small symmetry nitThe check avoids double-mapping. For readability, consider using To4() for lastIP too.
- firstIP = append(v4Mappedv6Prefix, ip4...) - lastIP = append(v4Mappedv6Prefix, lastIP...) + firstIP = append(v4Mappedv6Prefix, ip4...) + lastIP = append(v4Mappedv6Prefix, lastIP.To4()...)cmd/mapcidr/main_test.go (1)
223-233: Regression covered — LGTMTest asserts the exact aggregates post-filter; matches the reported issue. Consider adding an IPv6 variant and a case where FilterIP contains a CIDR to harden coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/mapcidr/main.go(1 hunks)cmd/mapcidr/main_test.go(1 hunks)ip.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/mapcidr/main_test.go (1)
cmd/mapcidr/main.go (1)
Options(41-69)
jjhwan-h
force-pushed
the
fix/ip-filtering
branch
from
0767afd to
f844ff8
Compare
|
The issue was already resolved in #678. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ip.go (1)
282-285: MaplastIPviaTo4()as well for symmetry and future-proofing.Today this block only runs when the inputs aren’t already v4-mapped, so
lastIPshould be 4 bytes and the current code is safe. UsinglastIP.To4()explicitly makes the intent clear and resilient to upstream representation shifts.- if ip4 := firstIP.To4(); ip4 != nil && !(len(firstIP) == net.IPv6len && bytes.Equal(firstIP[:12], v4Mappedv6Prefix)) { - firstIP = append(v4Mappedv6Prefix, ip4...) - lastIP = append(v4Mappedv6Prefix, lastIP...) - } + if ip4 := firstIP.To4(); ip4 != nil && !(len(firstIP) == net.IPv6len && bytes.Equal(firstIP[:12], v4Mappedv6Prefix)) { + firstIP = append(v4Mappedv6Prefix, ip4...) + if ip4Last := lastIP.To4(); ip4Last != nil { + lastIP = append(v4Mappedv6Prefix, ip4Last...) + } + }Optionally, consider a tiny helper
isV4Mapped(ip net.IP) boolto reuse the prefix check across the file. Also, please add a test with an input like::ffff:10.0.0.0/120to ensure no re-wrapping occurs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/mapcidr/main.go(1 hunks)cmd/mapcidr/main_test.go(1 hunks)ip.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cmd/mapcidr/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/mapcidr/main_test.go
🔇 Additional comments (1)
ip.go (1)
282-285: Guard correctly avoids double-mapping IPv4-mapped IPv6.Nice catch. Because
To4()returns non-nil for v4-mapped IPv6 too, the previous logic could re-wrap already mapped addresses, leading to malformed 28-byte slices. The added prefix check prevents that.
|
@jjhwan-h Thanks for the PR and adding more test coverage! |
2 participants
Summary
Fixes a regression where filtered IPs (
-fi) were reintroduced after aggregation (-a).Closes #523
Fix
FilterIPis set:pCidrinto its IP list withgetIPListFilterIP/32(or/128) CIDRspCidrSummary by CodeRabbit