Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds IPv6 overlay support and a DisableIPv6 toggle across client, device, DNS, netflow, SSH, status, UI, and protobuf schemas; introduces compact netip encoding utilities; migrates WireGuard iface/address types to carry IPv6; and adds targeted nolint suppressions for a backward-compatible PeerIP field. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Engine
participant Device
participant Management
participant DNS
Client->>Engine: Start with EngineConfig{WgAddr: wgaddr.Address, DisableIPv6: bool}
Engine->>Device: Create/Update interface with wgaddr.Address (may include IPv6)
Device-->>Engine: Report assigned v4/v6 (or clear IPv6 on failure)
Engine->>Management: Login/register with System Info (Flags.DisableIPv6, Capabilities)
Management-->>Client: Send PeerConfig (address_v6, sourcePrefixes, capabilities)
Client->>DNS: Generate reverse zones for v4 and v6 based on Engine.WgAddr
Client->>Engine: Apply peer AllowedIPs filtered by local IPv6 availability
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
lixmal
changed the title
lixmal
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shared/management/proto/management.proto (1)
338-340: Proto field addition looks good.The
address_v6field appropriately extendsPeerConfigfor dual-stack peer support.Minor documentation consideration: The comment says "16 bytes IP + 1 byte prefix length" which matches the compact format, but for a peer's overlay address the prefix length is typically implied (e.g., always
/128for IPv6). Consider clarifying whether the prefix length byte is meaningful here or if it could be simplified to just 16 bytes for the address sinceDecodeAddr(fromnetiputil) discards the prefix length anyway.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/proto/management.proto` around lines 338 - 340, The comment for the new bytes field address_v6 on PeerConfig is ambiguous about the prefix byte; either explicitly document that the extra prefix-length byte is ignored by DecodeAddr (netiputil) or remove it and change the wire-format expectation to a plain 16-byte IPv6 address. Update the proto comment for address_v6 to state the intended format (e.g., "16 bytes IPv6 address, no prefix byte" or "16 bytes IP + 1 byte prefix (prefix is meaningful)"), and if you choose to remove the prefix byte, update any decoding/encoding logic that reads/writes address_v6 (search for DecodeAddr and usages in netiputil and consumers of PeerConfig) to handle the 16-byte-only form. Ensure the comment and code for address_v6, PeerConfig, and DecodeAddr/netiputil are 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 `@shared/netiputil/compact.go`:
- Around line 24-42: The Gosec G602 false positives in DecodePrefix arise from
slice access checks that are actually guarded by the switch on len(b); suppress
them by adding a nolint directive: annotate the relevant slice-access lines in
DecodePrefix (the cases handling length 5 and 17, i.e., the uses of b[:4], b[4],
b[:16], and b[16]) with "//nolint:gosec // G602 false positive: length is
checked by switch case" so the analyzer ignores these safe accesses while
preserving the existing guard logic.
---
Nitpick comments:
In `@shared/management/proto/management.proto`:
- Around line 338-340: The comment for the new bytes field address_v6 on
PeerConfig is ambiguous about the prefix byte; either explicitly document that
the extra prefix-length byte is ignored by DecodeAddr (netiputil) or remove it
and change the wire-format expectation to a plain 16-byte IPv6 address. Update
the proto comment for address_v6 to state the intended format (e.g., "16 bytes
IPv6 address, no prefix byte" or "16 bytes IP + 1 byte prefix (prefix is
meaningful)"), and if you choose to remove the prefix byte, update any
decoding/encoding logic that reads/writes address_v6 (search for DecodeAddr and
usages in netiputil and consumers of PeerConfig) to handle the 16-byte-only
form. Ensure the comment and code for address_v6, PeerConfig, and
DecodeAddr/netiputil are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 172f0e99-f851-4579-a9e4-d40f6310a23d
⛔ Files ignored due to path filters (1)
shared/management/proto/management.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
client/internal/acl/manager.goclient/internal/debug/debug.gomanagement/internals/shared/grpc/conversion.gomanagement/server/peer_test.goshared/management/proto/management.protoshared/netiputil/compact.goshared/netiputil/compact_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shared/netiputil/compact.go (1)
23-42: Consider validating prefix length bounds for robustness.The decoded prefix length (last byte) is not validated against valid bounds. For IPv4, valid bits are 0-32; for IPv6, 0-128. If the input contains out-of-range values,
netip.PrefixFromreturns an invalid prefix silently, requiring callers to checkIsValid().For a wire protocol decoder, explicit validation with a clear error message improves debuggability when malformed data is received.
🛡️ Proposed fix to validate prefix length
func DecodePrefix(b []byte) (netip.Prefix, error) { switch len(b) { case 5: var ip4 [4]byte copy(ip4[:], b) - return netip.PrefixFrom(netip.AddrFrom4(ip4), int(b[len(b)-1])), nil + bits := int(b[len(b)-1]) + if bits > 32 { + return netip.Prefix{}, fmt.Errorf("invalid IPv4 prefix length %d (max 32)", bits) + } + return netip.PrefixFrom(netip.AddrFrom4(ip4), bits), nil case 17: var ip6 [16]byte copy(ip6[:], b) addr := netip.AddrFrom16(ip6).Unmap() bits := int(b[len(b)-1]) + maxBits := 128 + if addr.Is4() { + maxBits = 32 + } + if bits > maxBits { + return netip.Prefix{}, fmt.Errorf("invalid prefix length %d (max %d)", bits, maxBits) + } if addr.Is4() && bits > 32 { bits = 32 } return netip.PrefixFrom(addr, bits), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/netiputil/compact.go` around lines 23 - 42, DecodePrefix does not validate the prefix length byte and can produce an invalid netip.Prefix; update DecodePrefix to read the trailing byte into bits and explicitly validate bounds: for the 5-byte (IPv4) case ensure 0 <= bits <= 32 (return a clear fmt.Errorf if not), and for the 17-byte (IPv6) case ensure 0 <= bits <= 128 (return error if out of range); keep the existing addr.Unmap() and the special case that an IPv6-mapped IPv4 address should not claim >32 bits (either validate before or after Unmap and return an error if inconsistent), and only call netip.PrefixFrom when bits are within the validated range so callers never receive silently invalid prefixes from DecodePrefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shared/netiputil/compact.go`:
- Around line 23-42: DecodePrefix does not validate the prefix length byte and
can produce an invalid netip.Prefix; update DecodePrefix to read the trailing
byte into bits and explicitly validate bounds: for the 5-byte (IPv4) case ensure
0 <= bits <= 32 (return a clear fmt.Errorf if not), and for the 17-byte (IPv6)
case ensure 0 <= bits <= 128 (return error if out of range); keep the existing
addr.Unmap() and the special case that an IPv6-mapped IPv4 address should not
claim >32 bits (either validate before or after Unmap and return an error if
inconsistent), and only call netip.PrefixFrom when bits are within the validated
range so callers never receive silently invalid prefixes from DecodePrefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c0facdd-fccf-46f0-b534-18bb5d96c612
📒 Files selected for processing (1)
shared/netiputil/compact.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shared/netiputil/compact.go (1)
16-21: Consider validating input prefix.The implementation is correct for valid prefixes. However, if an invalid/zero
netip.Prefixis passed,p.Bits()returns -1 (becomes 255 as byte) and a zeroAddrproduces a 1-byte result. Both would fail on decode, so it's fail-safe, but explicit validation could provide clearer error messages at the source.💡 Optional: Add validation for invalid prefixes
// EncodePrefix encodes a netip.Prefix into compact bytes. // The address is always unmapped before encoding. -func EncodePrefix(p netip.Prefix) []byte { +func EncodePrefix(p netip.Prefix) ([]byte, error) { + if !p.IsValid() { + return nil, fmt.Errorf("invalid prefix") + } addr := p.Addr().Unmap() - return append(addr.AsSlice(), byte(p.Bits())) + return append(addr.AsSlice(), byte(p.Bits())), nil }Note: This would require updating callers to handle the error. Given the current usage context where callers likely pass valid prefixes, keeping the current signature may be acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/netiputil/compact.go` around lines 16 - 21, Validate the input prefix in EncodePrefix by checking netip.Prefix validity and bit length before encoding: change EncodePrefix(p netip.Prefix) to return ([]byte, error), verify p.IsValid() and p.Bits() >= 0 (or other appropriate checks) and return a descriptive error when invalid instead of producing a malformed byte slice; update all callers to handle the error accordingly so decoding won't receive impossible values like 255 bits or a one-byte address.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shared/netiputil/compact.go`:
- Around line 16-21: Validate the input prefix in EncodePrefix by checking
netip.Prefix validity and bit length before encoding: change EncodePrefix(p
netip.Prefix) to return ([]byte, error), verify p.IsValid() and p.Bits() >= 0
(or other appropriate checks) and return a descriptive error when invalid
instead of producing a malformed byte slice; update all callers to handle the
error accordingly so decoding won't receive impossible values like 255 bits or a
one-byte address.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58f63604-5572-410f-90a1-859c6db3bd11
📒 Files selected for processing (2)
shared/netiputil/compact.goshared/netiputil/compact_test.go
✅ Files skipped from review due to trivial changes (1)
- shared/netiputil/compact_test.go
pascal-fischer
previously approved these changes
Mar 19, 2026
PeerCapability is reported in PeerSystemMeta.capabilities on login/sync. Management uses it instead of version gating to determine client features. disableIPv6 in Flags lets users opt out of IPv6 overlay.
lixmal
force-pushed
the
proto-ipv6-overlay
branch
from
b7be56f to
e2f7748
Compare
…anic, v6 validation
…anic, v6 validation
# Conflicts: # client/iface/wgaddr/address.go
…anic, v6 validation
# Conflicts: # client/iface/wgaddr/address.go
# Conflicts: # client/internal/dns/upstream_ios.go
This was referenced Mar 26, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shared/management/proto/management.proto (1)
587-604: Consider enhancing the deprecation comment for clarity.The current comment "Use sourcePrefixes instead" is accurate but could be more informative for developers maintaining this code. Given that the client code will continue using
PeerIPduring the transition period, a more explicit comment would help:- // Use sourcePrefixes instead. - string PeerIP = 1 [deprecated = true]; + // Deprecated: PeerIP is maintained for backward compatibility with older clients. + // New clients reporting PeerCapabilitySourcePrefixes will receive sourcePrefixes instead. + string PeerIP = 1 [deprecated = true];The staged rollout is confirmed: proto definitions are in place (including the
PeerCapabilitySourcePrefixescapability constant), but server/client implementations remain deferred to follow-up PRs, with capability gating ensuring backward compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/proto/management.proto` around lines 587 - 604, Update the deprecation comment on the PeerIP field to be more explicit about the migration path: mention that PeerIP is deprecated in favor of sourcePrefixes, that clients may continue to use PeerIP during the staged rollout, and reference the capability gating (PeerCapabilitySourcePrefixes) and the fact server/client implementations are deferred to follow-up PRs to ensure backward compatibility; locate and update the comment attached to the PeerIP definition (symbol: PeerIP) and optionally note the replacement field (symbol: sourcePrefixes) and the capability constant (symbol: PeerCapabilitySourcePrefixes) so maintainers understand the transition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shared/management/proto/management.proto`:
- Around line 587-604: Update the deprecation comment on the PeerIP field to be
more explicit about the migration path: mention that PeerIP is deprecated in
favor of sourcePrefixes, that clients may continue to use PeerIP during the
staged rollout, and reference the capability gating
(PeerCapabilitySourcePrefixes) and the fact server/client implementations are
deferred to follow-up PRs to ensure backward compatibility; locate and update
the comment attached to the PeerIP definition (symbol: PeerIP) and optionally
note the replacement field (symbol: sourcePrefixes) and the capability constant
(symbol: PeerCapabilitySourcePrefixes) so maintainers understand the transition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c106a4b2-a323-46cd-b32a-b1b620587839
⛔ Files ignored due to path filters (1)
shared/management/proto/management.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
client/internal/acl/manager.goclient/internal/debug/debug.gomanagement/internals/shared/grpc/conversion.gomanagement/server/peer_test.goshared/management/proto/management.protoshared/netiputil/compact.goshared/netiputil/compact_test.go
✅ Files skipped from review due to trivial changes (6)
- client/internal/acl/manager.go
- management/server/peer_test.go
- client/internal/debug/debug.go
- shared/netiputil/compact.go
- shared/netiputil/compact_test.go
- management/internals/shared/grpc/conversion.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/internal/debug/debug.go (1)
1268-1276:⚠️ Potential issue | 🟠 MajorAnonymize
SourcePrefixesalongside deprecatedPeerIP.Line 1274 still only scrubs the deprecated string field. After this proto change,
FirewallRule.SourcePrefixescan carry the real source IPs in compact bytes, so--anonymizeno longer covers firewall rules end to end.🔒 Proposed fix
import ( "net" "net/netip" "os" "path/filepath" @@ "github.com/netbirdio/netbird/client/internal/updater/installer" nbstatus "github.com/netbirdio/netbird/client/status" mgmProto "github.com/netbirdio/netbird/shared/management/proto" + "github.com/netbirdio/netbird/shared/netiputil" "github.com/netbirdio/netbird/util" ) @@ func anonymizeFirewallRule(rule *mgmProto.FirewallRule, anonymizer *anonymize.Anonymizer) { if rule == nil { return } //nolint:staticcheck // PeerIP used for backward compatibility if addr, err := netip.ParseAddr(rule.PeerIP); err == nil { rule.PeerIP = anonymizer.AnonymizeIP(addr).String() //nolint:staticcheck } + for i, raw := range rule.SourcePrefixes { + prefix, err := netiputil.DecodePrefix(raw) + if err != nil { + continue + } + anon := netip.PrefixFrom(anonymizer.AnonymizeIP(prefix.Addr()), prefix.Bits()) + rule.SourcePrefixes[i] = netiputil.EncodePrefix(anon) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/debug/debug.go` around lines 1268 - 1276, anonymizeFirewallRule currently only anonymizes the deprecated PeerIP string; update it to also scrub FirewallRule.SourcePrefixes by iterating over each entry in SourcePrefixes, decoding each compact byte entry into the appropriate netip type (netip.Addr or netip.Prefix), applying the anonymizer (e.g., anonymizer.AnonymizeIP for addresses or the anonymizer's prefix method if available), and writing the anonymized value back into SourcePrefixes in the same compact byte format; modify anonymizeFirewallRule to handle both PeerIP and SourcePrefixes so firewall rules are fully anonymized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shared/management/proto/management.proto`:
- Around line 353-354: The anonymization step currently only rewrites
PeerConfig.Address before emitting network_map.json; extend it to also scrub the
IPv6 field by rewriting PeerConfig.AddressV6 (generated from proto field
address_v6) in the same anonymize path in client/internal/debug/debug.go (the
routine around the existing Address rewrite at lines ~1162-1175). Update the
anonymizer to detect and replace AddressV6 with the same deterministic or
redacted value you use for Address so IPv6 overlay addresses are not emitted
when --anonymize is enabled.
- Around line 598-603: The conversion code that builds proto.FirewallRule is
only populating legacy fields and must also set the new customProtocol and
sourcePrefixes fields; update the builder in conversion.go (the function that
constructs proto.FirewallRule) to copy the internal firewall rule's custom
protocol value into proto.FirewallRule.customProtocol and to populate
proto.FirewallRule.sourcePrefixes from the internal compact prefix
representation (assign the byte slices directly if the internal struct already
holds compact [IP][prefix_len] entries, or serialize each internal prefix into
the 5- or 17-byte format before appending). Ensure field names referenced are
proto.FirewallRule.customProtocol and proto.FirewallRule.sourcePrefixes so sync
responses include the new values.
In `@shared/netiputil/compact_test.go`:
- Around line 72-80: The test TestEncodePrefixUnmaps currently uses
::ffff:10.1.2.3/32 which hides an encoder bug; update the test to include
explicit mapped-prefix cases that cover the problematic lengths (e.g.
::ffff:10.1.2.3/128 and ::ffff:10.1.2.3/120) so EncodePrefix's behavior is
exercised, and adjust expected lengths and decoded prefixes when calling
EncodePrefix and DecodePrefix accordingly (use EncodePrefix, DecodePrefix, and
assert/require on length and decoded netip.Prefix values to ensure /128 maps to
a full IPv6 handling failure or correct unmap to IPv4 and /120 maps to an IPv4
/24 as intended).
---
Outside diff comments:
In `@client/internal/debug/debug.go`:
- Around line 1268-1276: anonymizeFirewallRule currently only anonymizes the
deprecated PeerIP string; update it to also scrub FirewallRule.SourcePrefixes by
iterating over each entry in SourcePrefixes, decoding each compact byte entry
into the appropriate netip type (netip.Addr or netip.Prefix), applying the
anonymizer (e.g., anonymizer.AnonymizeIP for addresses or the anonymizer's
prefix method if available), and writing the anonymized value back into
SourcePrefixes in the same compact byte format; modify anonymizeFirewallRule to
handle both PeerIP and SourcePrefixes so firewall rules are fully anonymized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd25c3f3-a562-4224-94bd-607585bc6d19
⛔ Files ignored due to path filters (1)
shared/management/proto/management.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
client/internal/acl/manager.goclient/internal/debug/debug.gomanagement/internals/shared/grpc/conversion.gomanagement/server/peer_test.goshared/management/proto/management.protoshared/netiputil/compact.goshared/netiputil/compact_test.go
✅ Files skipped from review due to trivial changes (3)
- management/internals/shared/grpc/conversion.go
- client/internal/acl/manager.go
- management/server/peer_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shared/netiputil/compact.go
Comment on lines
+353
to
+354
| // IPv6 overlay address as compact bytes: 16 bytes IP + 1 byte prefix length. | ||
| bytes address_v6 = 9; |
Contributor
There was a problem hiding this comment.
Wire address_v6 into debug-bundle anonymization.
Line 353 adds a second peer-address carrier, but client/internal/debug/debug.go:1162-1175 still only rewrites PeerConfig.Address before network_map.json is emitted. With --anonymize, the real overlay IPv6 will still be included unless AddressV6 is scrubbed too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shared/management/proto/management.proto` around lines 353 - 354, The
anonymization step currently only rewrites PeerConfig.Address before emitting
network_map.json; extend it to also scrub the IPv6 field by rewriting
PeerConfig.AddressV6 (generated from proto field address_v6) in the same
anonymize path in client/internal/debug/debug.go (the routine around the
existing Address rewrite at lines ~1162-1175). Update the anonymizer to detect
and replace AddressV6 with the same deterministic or redacted value you use for
Address so IPv6 overlay addresses are not emitted when --anonymize is enabled.
Comment on lines
+598
to
+603
| // CustomProtocol is a custom protocol ID when Protocol is CUSTOM. | ||
| uint32 customProtocol = 8; | ||
|
|
||
| // Compact source IP prefixes for this rule, supersedes PeerIP. | ||
| // Each entry is 5 bytes (v4) or 17 bytes (v6): [IP bytes][1 byte prefix_len]. | ||
| repeated bytes sourcePrefixes = 9; |
Contributor
There was a problem hiding this comment.
Populate the new firewall fields in protocol conversion.
management/internals/shared/grpc/conversion.go:279-303 still builds proto.FirewallRule with only the legacy fields. Until it writes Line 599 and Line 603, sync responses never carry customProtocol or sourcePrefixes, so upgraded clients cannot consume compact firewall prefixes or custom protocol IDs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shared/management/proto/management.proto` around lines 598 - 603, The
conversion code that builds proto.FirewallRule is only populating legacy fields
and must also set the new customProtocol and sourcePrefixes fields; update the
builder in conversion.go (the function that constructs proto.FirewallRule) to
copy the internal firewall rule's custom protocol value into
proto.FirewallRule.customProtocol and to populate
proto.FirewallRule.sourcePrefixes from the internal compact prefix
representation (assign the byte slices directly if the internal struct already
holds compact [IP][prefix_len] entries, or serialize each internal prefix into
the 5- or 17-byte format before appending). Ensure field names referenced are
proto.FirewallRule.customProtocol and proto.FirewallRule.sourcePrefixes so sync
responses include the new values.
pappz
reviewed
Mar 31, 2026
| bits := int(b[len(b)-1]) | ||
| if addr.Is4() { | ||
| if bits > 32 { | ||
| bits = 32 |
Contributor
There was a problem hiding this comment.
It looks like in this case the input is invalid. If we force 32 then it produce incorrect address. so in case > 32 should return with error.
pappz
requested changes
Mar 31, 2026
[client] Add IPv6 support to SSH server, client config, and netflow logger
[client] Add IPv6 reverse DNS and host configurator support
[client] Add IPv6 overlay address support to WireGuard interface and engine
# Conflicts: # client/android/client.go # client/ssh/server/server.go # shared/management/proto/management.pb.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/internal/listener/network_change.go (1)
4-8:⚠️ Potential issue | 🟠 MajorThis is a breaking mobile SDK interface change.
client/ios/NetBirdSDK.NetworkChangeListenerembeds this interface, so existing app-side listeners that only implementOnNetworkChangedandSetInterfaceIPwill stop satisfying the exported API. If this break is intentional, it needs a migration note/version bump; otherwise keep IPv6 behind a separate optional interface and type-assert at the call site.💡 Backward-compatible shape
type NetworkChangeListener interface { // OnNetworkChanged invoke when network settings has been changed OnNetworkChanged(string) SetInterfaceIP(string) - SetInterfaceIPv6(string) } + +type NetworkChangeListenerIPv6 interface { + SetInterfaceIPv6(string) +}Then invoke
SetInterfaceIPv6only when the listener also satisfiesNetworkChangeListenerIPv6.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/listener/network_change.go` around lines 4 - 8, The new SetInterfaceIPv6 method added to the NetworkChangeListener interface is a breaking change; revert NetworkChangeListener to only include OnNetworkChanged and SetInterfaceIP, add a new optional interface named NetworkChangeListenerIPv6 that defines SetInterfaceIPv6, and update call sites that previously assumed SetInterfaceIPv6 to type-assert the listener to NetworkChangeListenerIPv6 before invoking SetInterfaceIPv6 (e.g., if l, ok := listener.(NetworkChangeListenerIPv6); ok { l.SetInterfaceIPv6(...) } ), leaving the exported API shape unchanged for existing consumers.client/status/status.go (1)
922-932:⚠️ Potential issue | 🟠 MajorPeer IPv4 addresses are still leaking through
--anonymize.This function now masks
peer.IPv6, butpeer.IPis still never rewritten. Any anonymized peer-detail output will keep exposing the IPv4 overlay addresses.Suggested fix
func anonymizePeerDetail(a *anonymize.Anonymizer, peer *PeerStateDetailOutput) { peer.FQDN = a.AnonymizeDomain(peer.FQDN) + peer.IP = a.AnonymizeIPString(peer.IP) if localIP, port, err := net.SplitHostPort(peer.IceCandidateEndpoint.Local); err == nil { peer.IceCandidateEndpoint.Local = fmt.Sprintf("%s:%s", a.AnonymizeIPString(localIP), port) } if remoteIP, port, err := net.SplitHostPort(peer.IceCandidateEndpoint.Remote); err == nil { peer.IceCandidateEndpoint.Remote = fmt.Sprintf("%s:%s", a.AnonymizeIPString(remoteIP), port) } peer.IPv6 = a.AnonymizeIPString(peer.IPv6)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/status/status.go` around lines 922 - 932, The anonymizePeerDetail function is missing masking for the IPv4 overlay address field; update anonymizePeerDetail to rewrite peer.IP (the IPv4 address on PeerStateDetailOutput) using the anonymizer (e.g., call a.AnonymizeIPString(peer.IP) and assign it back to peer.IP), similar to how peer.IPv6 and IceCandidateEndpoint fields are handled, and ensure you handle empty/invalid values the same way as the other anonymization calls.
🧹 Nitpick comments (2)
client/iface/wgaddr/address_test_helpers.go (1)
1-10: Move this test helper into a test-only file.
MustParseWGAddressis defined inaddress_test_helpers.go, which is not a*_test.gofile. This means the function is compiled into the production package and exported fromwgaddr, even though it is used only in test code. Move the function to a*_test.gofile (e.g.,address_test.go) or into an internal testutil package to keep the production API clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/iface/wgaddr/address_test_helpers.go` around lines 1 - 10, The helper MustParseWGAddress in address_test_helpers.go is being compiled into the production package; move it to a test-only file so it isn’t exported in builds. Create a new *_test.go file (e.g., address_test.go or address_test_helpers_test.go) or put it into a testutil package, copy the MustParseWGAddress(address string) Address function there, update any test imports/usages to point to the new location, and remove the original from address_test_helpers.go so only tests compile with it.client/ui/network.go (1)
195-203: Extract a shared default-route predicate to avoid drift.Both call sites now duplicate the same v4/v6 default-route check. A tiny helper keeps the behavior synchronized.
♻️ Proposed refactor
+const ( + defaultRouteV4 = "0.0.0.0/0" + defaultRouteV6 = "::/0" +) + +func isDefaultExitRoute(rangeCIDR string) bool { + return rangeCIDR == defaultRouteV4 || rangeCIDR == defaultRouteV6 +} + func getExitNodeNetworks(routes []*proto.Network) []*proto.Network { var filteredRoutes []*proto.Network for _, route := range routes { - if route.Range == "0.0.0.0/0" || route.Range == "::/0" { + if isDefaultExitRoute(route.Range) { filteredRoutes = append(filteredRoutes, route) } } return filteredRoutes } @@ var exitNodes []*proto.Network for _, network := range resp.Routes { - if network.Range == "0.0.0.0/0" || network.Range == "::/0" { + if isDefaultExitRoute(network.Range) { exitNodes = append(exitNodes, network) } }Also applies to: 490-496
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ui/network.go` around lines 195 - 203, The two call sites duplicate the IPv4/IPv6 default-route check; extract a small predicate (e.g., isDefaultRoute or isDefaultNetwork) that accepts a *proto.Network and returns true when Range == "0.0.0.0/0" || Range == "::/0", then update getExitNodeNetworks and the other duplicate site (the other function that currently repeats the same check) to call that predicate instead of inlining the string comparisons so both places share the same logic and avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/iface/device/device_windows.go`:
- Around line 93-102: The IPv6 MTU branch currently only logs warnings on
failure and leaves t.address as dual-stack, causing assignAddr() to proceed with
IPv6; modify the error branches (the errors from
luid.IPInterface(windows.AF_INET6) and nbiface6.Set()) to disable/clear the IPv6
overlay on the endpoint before returning (so assignAddr() won’t attempt IPv6).
Concretely, after the IPInterface error or Set error, call the address-clear
helper on t.address (e.g., t.address.ClearIPv6() or the equivalent method your
Address type provides) and return the error; ensure these changes are applied in
the block that surrounds luid.IPInterface(...) and nbiface6.Set() so failures
degrade to IPv4-only.
In `@client/internal/dns/host_darwin.go`:
- Around line 301-304: The current logic only assigns dnsSettings.ServerIP when
ip.Is4(), which breaks IPv6-only cases; change it to set dnsSettings.ServerIP to
the first valid nameserver regardless of v4/v6, but still prefer IPv4 by
allowing a later IPv4 address to override an earlier IPv6 assignment.
Concretely: when !dnsSettings.ServerIP.IsValid(), assign dnsSettings.ServerIP =
ip for any valid ip, and if you encounter an ip where ip.Is4() and you already
set an IPv6, override it so the first IPv4 wins; update the code referencing
dnsSettings.ServerIP and ip.Is4() accordingly.
In `@client/internal/dns/network_manager_unix.go`:
- Around line 113-121: The code that sets DNS entries in connSettings for the
chosen family (using networkManagerDbusIPv4Key / networkManagerDbusIPv6Key and
networkManagerDbusDNSKey) only overwrites the selected family's keys and leaves
stale DNS, dns-priority and dns-search entries for the opposite family; update
the logic in the block that writes to connSettings (and the analogous block
around the other branch at the referenced lines) to explicitly clear the
opposite family's entries by removing or setting empty variants for
networkManagerDbusDNSKey, networkManagerDbusDNSPriorityKey, and
networkManagerDbusDNSSearchKey on the other key (networkManagerDbusIPv4Key vs
networkManagerDbusIPv6Key) before assigning the new values so stale
resolver/search settings cannot persist after Reapply.
In `@client/internal/dns/upstream_ios.go`:
- Around line 72-90: The code currently allows falling back to the public client
when needsPrivate is true but no family-specific bind address was selected;
instead, when needsPrivate is true and you cannot derive a valid bindIP from
upstreamIP (using u.lIPv6, u.lIP and routeMatch/u.lNet/u.lNetV6), return an
error immediately rather than leaving client unmodified. Specifically, in the
block where bindIP is computed for upstreamIP (referencing needsPrivate, bindIP,
upstreamIP, u.lIPv6, u.lIP, u.lNet, u.lNetV6, routeMatch and GetClientPrivate),
if bindIP.IsValid() is false and needsPrivate is true, propagate a clear error
(e.g., "no suitable private bind address for upstream") instead of continuing to
use the default/public client.
In `@client/internal/peer/status_test.go`:
- Line 49: The test setup currently ignores the error returned by
status.AddPeer; capture its return value and fail the test on error instead of
discarding it. Replace occurrences like `_ = status.AddPeer(key, fqdn, ip, "")`
with `if err := status.AddPeer(key, fqdn, ip, ""); err != nil {
t.Fatalf("AddPeer failed: %v", err) }` (or use `require.NoError(t, err)` if
testify is available), updating both places where AddPeer is called in the test.
In `@client/ui/client_ui.go`:
- Around line 679-698: The reconnect goroutine started after sendConfigUpdate()
can be terminated when saveSettings() closes the window (one-shot
--settings/--networks mode), so make the config-apply sequence deterministic:
extract the goroutine logic into a function (e.g., applyConfigAndReconnect or
similar) that calls conn.Status/Down/Up with a proper context, then have
saveSettings() either call that function synchronously (block until it returns)
or wait on a returned channel/WaitGroup before closing the window; ensure you
keep the timeout behavior (context.WithTimeout) but extend it if needed and
handle/log errors as before. Use the existing symbols conn.Status, conn.Down,
conn.Up, saveSettings, and sendConfigUpdate to locate and replace the goroutine
with a synchronous or waitable call so the process doesn't exit before the
reconnect completes.
In `@client/wasm/cmd/main.go`:
- Around line 85-87: The JS option disableIPv6 is read via
jsOptions.Get("disableIPv6") and then used with disableIPv6.Bool() without
confirming its type, which can panic; update the logic in the main.go block
handling jsOptions.Get("disableIPv6") to first check that disableIPv6.Type() ==
js.TypeBoolean (and still check !IsNull() && !IsUndefined()), and only then call
disableIPv6.Bool() to assign options.DisableIPv6; keep the same surrounding
checks (jsOptions.Get, IsNull, IsUndefined) and add the Type() validation to
avoid a panic.
---
Outside diff comments:
In `@client/internal/listener/network_change.go`:
- Around line 4-8: The new SetInterfaceIPv6 method added to the
NetworkChangeListener interface is a breaking change; revert
NetworkChangeListener to only include OnNetworkChanged and SetInterfaceIP, add a
new optional interface named NetworkChangeListenerIPv6 that defines
SetInterfaceIPv6, and update call sites that previously assumed SetInterfaceIPv6
to type-assert the listener to NetworkChangeListenerIPv6 before invoking
SetInterfaceIPv6 (e.g., if l, ok := listener.(NetworkChangeListenerIPv6); ok {
l.SetInterfaceIPv6(...) } ), leaving the exported API shape unchanged for
existing consumers.
In `@client/status/status.go`:
- Around line 922-932: The anonymizePeerDetail function is missing masking for
the IPv4 overlay address field; update anonymizePeerDetail to rewrite peer.IP
(the IPv4 address on PeerStateDetailOutput) using the anonymizer (e.g., call
a.AnonymizeIPString(peer.IP) and assign it back to peer.IP), similar to how
peer.IPv6 and IceCandidateEndpoint fields are handled, and ensure you handle
empty/invalid values the same way as the other anonymization calls.
---
Nitpick comments:
In `@client/iface/wgaddr/address_test_helpers.go`:
- Around line 1-10: The helper MustParseWGAddress in address_test_helpers.go is
being compiled into the production package; move it to a test-only file so it
isn’t exported in builds. Create a new *_test.go file (e.g., address_test.go or
address_test_helpers_test.go) or put it into a testutil package, copy the
MustParseWGAddress(address string) Address function there, update any test
imports/usages to point to the new location, and remove the original from
address_test_helpers.go so only tests compile with it.
In `@client/ui/network.go`:
- Around line 195-203: The two call sites duplicate the IPv4/IPv6 default-route
check; extract a small predicate (e.g., isDefaultRoute or isDefaultNetwork) that
accepts a *proto.Network and returns true when Range == "0.0.0.0/0" || Range ==
"::/0", then update getExitNodeNetworks and the other duplicate site (the other
function that currently repeats the same check) to call that predicate instead
of inlining the string comparisons so both places share the same logic and avoid
drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d771277-7b42-4d55-bf2f-b5caffc4b480
⛔ Files ignored due to path filters (1)
client/proto/daemon.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (76)
.github/workflows/wasm-build-validation.ymlclient/android/client.goclient/android/peer_notifier.goclient/android/preferences.goclient/cmd/status.goclient/cmd/system.goclient/cmd/up.goclient/embed/embed.goclient/iface/device/device_darwin.goclient/iface/device/device_ios.goclient/iface/device/device_kernel_unix.goclient/iface/device/device_netstack.goclient/iface/device/device_usp_unix.goclient/iface/device/device_windows.goclient/iface/device/kernel_module.goclient/iface/device/kernel_module_freebsd.goclient/iface/device/kernel_module_nonlinux.goclient/iface/device/wg_link_freebsd.goclient/iface/device/wg_link_linux.goclient/iface/iface.goclient/iface/iface_new.goclient/iface/iface_new_android.goclient/iface/iface_new_darwin.goclient/iface/iface_new_freebsd.goclient/iface/iface_new_ios.goclient/iface/iface_new_js.goclient/iface/iface_new_linux.goclient/iface/iface_test.goclient/iface/netstack/tun.goclient/iface/wgaddr/address.goclient/iface/wgaddr/address_test_helpers.goclient/internal/auth/auth.goclient/internal/connect.goclient/internal/debug/debug.goclient/internal/dns.goclient/internal/dns/host_darwin.goclient/internal/dns/network_manager_unix.goclient/internal/dns/server_test.goclient/internal/dns/systemd_linux.goclient/internal/dns/upstream_ios.goclient/internal/dns_test.goclient/internal/engine.goclient/internal/engine_ssh.goclient/internal/engine_test.goclient/internal/iface_common.goclient/internal/listener/network_change.goclient/internal/netflow/conntrack/conntrack.goclient/internal/netflow/logger/logger.goclient/internal/netflow/logger/logger_test.goclient/internal/netflow/manager.goclient/internal/netflow/types/types.goclient/internal/peer/status.goclient/internal/peer/status_test.goclient/internal/profilemanager/config.goclient/internal/rosenpass/manager.goclient/internal/routemanager/client/client_bench_test.goclient/internal/routemanager/manager_test.goclient/internal/routemanager/systemops/systemops_generic_test.goclient/ios/NetBirdSDK/client.goclient/ios/NetBirdSDK/peer_notifier.goclient/ios/NetBirdSDK/preferences.goclient/proto/daemon.protoclient/server/server.goclient/server/setconfig_test.goclient/ssh/config/manager.goclient/ssh/config/manager_test.goclient/ssh/server/server.goclient/status/status.goclient/status/status_test.goclient/system/info.goclient/ui/client_ui.goclient/ui/network.goclient/wasm/cmd/main.goclient/wasm/internal/rdp/rdcleanpath.goclient/wasm/internal/ssh/client.goshared/management/client/grpc.go
💤 Files with no reviewable changes (4)
- client/iface/device/kernel_module.go
- client/iface/iface_new_darwin.go
- client/iface/device/kernel_module_freebsd.go
- client/iface/iface_new_freebsd.go
✅ Files skipped from review due to trivial changes (8)
- client/internal/auth/auth.go
- client/android/peer_notifier.go
- client/internal/netflow/types/types.go
- client/wasm/internal/ssh/client.go
- client/internal/debug/debug.go
- client/internal/rosenpass/manager.go
- client/wasm/internal/rdp/rdcleanpath.go
- client/cmd/system.go
Comment on lines
+93
to
+102
| if t.address.HasIPv6() { | ||
| nbiface6, err := luid.IPInterface(windows.AF_INET6) | ||
| if err != nil { | ||
| log.Warnf("failed to get IPv6 interface for MTU: %v", err) | ||
| } else { | ||
| nbiface6.NLMTU = uint32(t.mtu) | ||
| if err := nbiface6.Set(); err != nil { | ||
| log.Warnf("failed to set IPv6 interface MTU: %v", err) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fail closed when the IPv6 MTU step fails.
If the AF_INET6 lookup or nbiface6.Set() fails here, we only log a warning and still leave t.address dual-stack. assignAddr() then goes on to try IPv6 anyway, so the interface can come up with IPv6 enabled but without the intended MTU. Clear the IPv6 overlay on either error so this path reliably degrades to v4-only before address assignment.
🔧 Proposed fix
if t.address.HasIPv6() {
nbiface6, err := luid.IPInterface(windows.AF_INET6)
if err != nil {
log.Warnf("failed to get IPv6 interface for MTU: %v", err)
+ t.address.ClearIPv6()
} else {
nbiface6.NLMTU = uint32(t.mtu)
if err := nbiface6.Set(); err != nil {
log.Warnf("failed to set IPv6 interface MTU: %v", err)
+ t.address.ClearIPv6()
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if t.address.HasIPv6() { | |
| nbiface6, err := luid.IPInterface(windows.AF_INET6) | |
| if err != nil { | |
| log.Warnf("failed to get IPv6 interface for MTU: %v", err) | |
| } else { | |
| nbiface6.NLMTU = uint32(t.mtu) | |
| if err := nbiface6.Set(); err != nil { | |
| log.Warnf("failed to set IPv6 interface MTU: %v", err) | |
| } | |
| } | |
| if t.address.HasIPv6() { | |
| nbiface6, err := luid.IPInterface(windows.AF_INET6) | |
| if err != nil { | |
| log.Warnf("failed to get IPv6 interface for MTU: %v", err) | |
| t.address.ClearIPv6() | |
| } else { | |
| nbiface6.NLMTU = uint32(t.mtu) | |
| if err := nbiface6.Set(); err != nil { | |
| log.Warnf("failed to set IPv6 interface MTU: %v", err) | |
| t.address.ClearIPv6() | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/iface/device/device_windows.go` around lines 93 - 102, The IPv6 MTU
branch currently only logs warnings on failure and leaves t.address as
dual-stack, causing assignAddr() to proceed with IPv6; modify the error branches
(the errors from luid.IPInterface(windows.AF_INET6) and nbiface6.Set()) to
disable/clear the IPv6 overlay on the endpoint before returning (so assignAddr()
won’t attempt IPv6). Concretely, after the IPInterface error or Set error, call
the address-clear helper on t.address (e.g., t.address.ClearIPv6() or the
equivalent method your Address type provides) and return the error; ensure these
changes are applied in the block that surrounds luid.IPInterface(...) and
nbiface6.Set() so failures degrade to IPv4-only.
Comment on lines
+301
to
304
| // Prefer the first IPv4 server as ServerIP since our DNS listener is IPv4. | ||
| if !dnsSettings.ServerIP.IsValid() && ip.Is4() { | ||
| dnsSettings.ServerIP = ip | ||
| } |
Contributor
There was a problem hiding this comment.
Keep IPv6 fallback when no IPv4 nameserver is present.
Line 302 currently only assigns ServerIP for IPv4. On IPv6-only networks this can leave ServerIP invalid and skip local DNS restoration logic. Prefer IPv4, but preserve first valid (IPv6) fallback.
💡 Proposed fix
if ip, err := netip.ParseAddr(address); err == nil && !ip.IsUnspecified() {
ip = ip.Unmap()
serverAddresses = append(serverAddresses, ip)
- // Prefer the first IPv4 server as ServerIP since our DNS listener is IPv4.
- if !dnsSettings.ServerIP.IsValid() && ip.Is4() {
- dnsSettings.ServerIP = ip
- }
+ // Keep first valid server as fallback, but prefer IPv4 when available.
+ if !dnsSettings.ServerIP.IsValid() {
+ dnsSettings.ServerIP = ip
+ } else if dnsSettings.ServerIP.Is6() && ip.Is4() {
+ dnsSettings.ServerIP = ip
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Prefer the first IPv4 server as ServerIP since our DNS listener is IPv4. | |
| if !dnsSettings.ServerIP.IsValid() && ip.Is4() { | |
| dnsSettings.ServerIP = ip | |
| } | |
| // Keep first valid server as fallback, but prefer IPv4 when available. | |
| if !dnsSettings.ServerIP.IsValid() { | |
| dnsSettings.ServerIP = ip | |
| } else if dnsSettings.ServerIP.Is6() && ip.Is4() { | |
| dnsSettings.ServerIP = ip | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/internal/dns/host_darwin.go` around lines 301 - 304, The current logic
only assigns dnsSettings.ServerIP when ip.Is4(), which breaks IPv6-only cases;
change it to set dnsSettings.ServerIP to the first valid nameserver regardless
of v4/v6, but still prefer IPv4 by allowing a later IPv4 address to override an
earlier IPv6 assignment. Concretely: when !dnsSettings.ServerIP.IsValid(),
assign dnsSettings.ServerIP = ip for any valid ip, and if you encounter an ip
where ip.Is4() and you already set an IPv6, override it so the first IPv4 wins;
update the code referencing dnsSettings.ServerIP and ip.Is4() accordingly.
Comment on lines
+113
to
+121
| ipKey := networkManagerDbusIPv4Key | ||
| if config.ServerIP.Is6() { | ||
| ipKey = networkManagerDbusIPv6Key | ||
| raw := config.ServerIP.As16() | ||
| connSettings[ipKey][networkManagerDbusDNSKey] = dbus.MakeVariant([][]byte{raw[:]}) | ||
| } else { | ||
| convDNSIP := binary.LittleEndian.Uint32(config.ServerIP.AsSlice()) | ||
| connSettings[ipKey][networkManagerDbusDNSKey] = dbus.MakeVariant([]uint32{convDNSIP}) | ||
| } |
Contributor
There was a problem hiding this comment.
Clear the stale DNS settings on the opposite family.
If this profile previously had IPv4 DNS and ServerIP now switches to IPv6 (or the other way around), only the selected section is overwritten here. The old dns, dns-priority, and dns-search entries remain in connSettings, so NetworkManager can keep the stale resolver/search list active after Reapply.
🛠️ Suggested fix
ipKey := networkManagerDbusIPv4Key
+ staleIPKey := networkManagerDbusIPv6Key
if config.ServerIP.Is6() {
ipKey = networkManagerDbusIPv6Key
+ staleIPKey = networkManagerDbusIPv4Key
raw := config.ServerIP.As16()
connSettings[ipKey][networkManagerDbusDNSKey] = dbus.MakeVariant([][]byte{raw[:]})
} else {
convDNSIP := binary.LittleEndian.Uint32(config.ServerIP.AsSlice())
connSettings[ipKey][networkManagerDbusDNSKey] = dbus.MakeVariant([]uint32{convDNSIP})
}
+ for _, key := range []string{
+ networkManagerDbusDNSKey,
+ networkManagerDbusDNSPriorityKey,
+ networkManagerDbusDNSSearchKey,
+ } {
+ delete(connSettings[staleIPKey], key)
+ }Also applies to: 156-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/internal/dns/network_manager_unix.go` around lines 113 - 121, The code
that sets DNS entries in connSettings for the chosen family (using
networkManagerDbusIPv4Key / networkManagerDbusIPv6Key and
networkManagerDbusDNSKey) only overwrites the selected family's keys and leaves
stale DNS, dns-priority and dns-search entries for the opposite family; update
the logic in the block that writes to connSettings (and the analogous block
around the other branch at the referenced lines) to explicitly clear the
opposite family's entries by removing or setting empty variants for
networkManagerDbusDNSKey, networkManagerDbusDNSPriorityKey, and
networkManagerDbusDNSSearchKey on the other key (networkManagerDbusIPv4Key vs
networkManagerDbusIPv6Key) before assigning the new values so stale
resolver/search settings cannot persist after Reapply.
Comment on lines
72
to
90
| needsPrivate := u.lNet.Contains(upstreamIP) || | ||
| u.lNetV6.Contains(upstreamIP) || | ||
| (u.routeMatch != nil && u.routeMatch(upstreamIP)) | ||
| if needsPrivate { | ||
| log.Debugf("using private client to query %s via upstream %s", r.Question[0].Name, upstream) | ||
| client, err = GetClientPrivate(u.lIP, u.interfaceName, timeout) | ||
| if err != nil { | ||
| return nil, 0, fmt.Errorf("create private client: %s", err) | ||
| var bindIP netip.Addr | ||
| switch { | ||
| case upstreamIP.Is6() && u.lIPv6.IsValid(): | ||
| bindIP = u.lIPv6 | ||
| case upstreamIP.Is4() && u.lIP.IsValid(): | ||
| bindIP = u.lIP | ||
| } | ||
|
|
||
| if bindIP.IsValid() { | ||
| log.Debugf("using private client to query %s via upstream %s", r.Question[0].Name, upstream) | ||
| client, err = GetClientPrivate(bindIP, u.interfaceName, timeout) | ||
| if err != nil { | ||
| return nil, 0, fmt.Errorf("create private client: %s", err) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Don't fall back to the public client once needsPrivate is true.
If the overlay prefixes or routeMatch mark an upstream as private but the family-specific bind address is unavailable, this silently reuses the default client and sends the query on the wrong path. For private IPv6 resolvers that means query failures at best and DNS leakage at worst; return an error instead of falling through here.
Possible fix
if needsPrivate {
var bindIP netip.Addr
switch {
case upstreamIP.Is6() && u.lIPv6.IsValid():
bindIP = u.lIPv6
case upstreamIP.Is4() && u.lIP.IsValid():
bindIP = u.lIP
}
- if bindIP.IsValid() {
- log.Debugf("using private client to query %s via upstream %s", r.Question[0].Name, upstream)
- client, err = GetClientPrivate(bindIP, u.interfaceName, timeout)
- if err != nil {
- return nil, 0, fmt.Errorf("create private client: %s", err)
- }
- }
+ if !bindIP.IsValid() {
+ return nil, 0, fmt.Errorf("private upstream %s requires a local address of the same family", upstream)
+ }
+
+ log.Debugf("using private client to query %s via upstream %s", r.Question[0].Name, upstream)
+ client, err = GetClientPrivate(bindIP, u.interfaceName, timeout)
+ if err != nil {
+ return nil, 0, fmt.Errorf("create private client: %s", err)
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/internal/dns/upstream_ios.go` around lines 72 - 90, The code currently
allows falling back to the public client when needsPrivate is true but no
family-specific bind address was selected; instead, when needsPrivate is true
and you cannot derive a valid bindIP from upstreamIP (using u.lIPv6, u.lIP and
routeMatch/u.lNet/u.lNetV6), return an error immediately rather than leaving
client unmodified. Specifically, in the block where bindIP is computed for
upstreamIP (referencing needsPrivate, bindIP, upstreamIP, u.lIPv6, u.lIP,
u.lNet, u.lNetV6, routeMatch and GetClientPrivate), if bindIP.IsValid() is false
and needsPrivate is true, propagate a clear error (e.g., "no suitable private
bind address for upstream") instead of continuing to use the default/public
client.
| fqdn := "peer-a.netbird.local" | ||
| status := NewRecorder("https://mgm") | ||
| _ = status.AddPeer(key, fqdn, ip) | ||
| _ = status.AddPeer(key, fqdn, ip, "") |
Contributor
There was a problem hiding this comment.
Don’t ignore AddPeer errors in test setup.
These setup calls currently swallow failures, which can make test results misleading.
Proposed test fix
- _ = status.AddPeer(key, fqdn, ip, "")
+ assert.NoError(t, status.AddPeer(key, fqdn, ip, ""))- _ = status.AddPeer(key, "abc.netbird", ip, "")
+ assert.NoError(t, status.AddPeer(key, "abc.netbird", ip, ""))Also applies to: 88-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/internal/peer/status_test.go` at line 49, The test setup currently
ignores the error returned by status.AddPeer; capture its return value and fail
the test on error instead of discarding it. Replace occurrences like `_ =
status.AddPeer(key, fqdn, ip, "")` with `if err := status.AddPeer(key, fqdn, ip,
""); err != nil { t.Fatalf("AddPeer failed: %v", err) }` (or use
`require.NoError(t, err)` if testify is available), updating both places where
AddPeer is called in the test.
Comment on lines
+679
to
698
| // Reconnect if connected to apply the new settings. | ||
| // Use a background context so the reconnect outlives the settings window. | ||
| go func() { | ||
| status, err := conn.Status(s.ctx, &proto.StatusRequest{}) | ||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| defer cancel() | ||
| status, err := conn.Status(ctx, &proto.StatusRequest{}) | ||
| if err != nil { | ||
| log.Errorf("get service status: %v", err) | ||
| log.Errorf("failed to get service status: %v", err) | ||
| return | ||
| } | ||
| if status.Status == string(internal.StatusConnected) { | ||
| // run down & up | ||
| _, err = conn.Down(s.ctx, &proto.DownRequest{}) | ||
| if err != nil { | ||
| log.Errorf("down service: %v", err) | ||
| if _, err = conn.Down(ctx, &proto.DownRequest{}); err != nil { | ||
| log.Errorf("failed to stop service: %v", err) | ||
| } | ||
|
|
||
| _, err = conn.Up(s.ctx, &proto.UpRequest{}) | ||
| if err != nil { | ||
| log.Errorf("up service: %v", err) | ||
| return | ||
| // TODO: wait for the service to be idle before calling Up, or use a fresh connection | ||
| if _, err = conn.Up(ctx, &proto.UpRequest{}); err != nil { | ||
| log.Errorf("failed to start service: %v", err) | ||
| } | ||
| } | ||
| }() |
Contributor
There was a problem hiding this comment.
This reconnect can be lost in one-shot window mode.
saveSettings() closes the window immediately after sendConfigUpdate() returns, and in --settings / --networks mode that also ends the UI process. This goroutine is therefore only best-effort: it can be terminated before Status/Down/Up finish, leaving the config saved but not actually applied until the user reconnects manually.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/ui/client_ui.go` around lines 679 - 698, The reconnect goroutine
started after sendConfigUpdate() can be terminated when saveSettings() closes
the window (one-shot --settings/--networks mode), so make the config-apply
sequence deterministic: extract the goroutine logic into a function (e.g.,
applyConfigAndReconnect or similar) that calls conn.Status/Down/Up with a proper
context, then have saveSettings() either call that function synchronously (block
until it returns) or wait on a returned channel/WaitGroup before closing the
window; ensure you keep the timeout behavior (context.WithTimeout) but extend it
if needed and handle/log errors as before. Use the existing symbols conn.Status,
conn.Down, conn.Up, saveSettings, and sendConfigUpdate to locate and replace the
goroutine with a synchronous or waitable call so the process doesn't exit before
the reconnect completes.
Comment on lines
+85
to
+87
| if disableIPv6 := jsOptions.Get("disableIPv6"); !disableIPv6.IsNull() && !disableIPv6.IsUndefined() { | ||
| options.DisableIPv6 = disableIPv6.Bool() | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go's syscall/js package, does js.Value.Bool() panic when the underlying JavaScript value is not a boolean?
💡 Result:
Yes. In syscall/js, js.Value.Bool() panics if the underlying JavaScript value is not a boolean. [1]
🏁 Script executed:
# First, verify the code at lines 85-87
head -100 client/wasm/cmd/main.go | tail -30Repository: netbirdio/netbird
Length of output: 1046
🏁 Script executed:
# Check how many lines are in the file
wc -l client/wasm/cmd/main.goRepository: netbirdio/netbird
Length of output: 88
🏁 Script executed:
# Look for other .Bool() usages in the same file to see the pattern
rg '\.Bool\(\)' client/wasm/cmd/main.go -B 2 -A 1Repository: netbirdio/netbird
Length of output: 210
🏁 Script executed:
# Check for Type checks before .Bool() in the codebase to see if this is a pattern
rg 'Type\(\).*TypeBoolean' . -t go --max-count 10Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
# Verify that js.TypeBoolean is the correct constant to use
rg 'js\.TypeBoolean\|TypeBoolean' . -t go --max-count 5Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
# Check imports to see what js package is used
head -30 client/wasm/cmd/main.go | grep -A 20 "^import"Repository: netbirdio/netbird
Length of output: 642
🏁 Script executed:
# Search for any type checking in the entire codebase with js.Type pattern
rg 'Type\(\)' client/wasm/ -t go -B 1 -A 1Repository: netbirdio/netbird
Length of output: 2773
🏁 Script executed:
# Check if there are any panic guards or error handling for Bool/String elsewhere
rg 'Bool\(\)|String\(\)' client/wasm/ -t go --max-count 20Repository: netbirdio/netbird
Length of output: 1982
🏁 Script executed:
# Verify the js package documentation/constants - search for js imports and Type usage across repo
rg 'from syscall/js|import.*js' . -t go | head -5Repository: netbirdio/netbird
Length of output: 129
Add type validation before calling Bool() on disableIPv6.
The code at line 86 calls disableIPv6.Bool() without validating that the JavaScript value is actually a boolean. The syscall/js package's Bool() method panics if the value is not a boolean type. This is inconsistent with the type checking patterns used throughout the file for function parameters.
Proposed fix
if disableIPv6 := jsOptions.Get("disableIPv6"); !disableIPv6.IsNull() && !disableIPv6.IsUndefined() {
+ if disableIPv6.Type() != js.TypeBoolean {
+ return options, fmt.Errorf("disableIPv6 must be a boolean")
+ }
options.DisableIPv6 = disableIPv6.Bool()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if disableIPv6 := jsOptions.Get("disableIPv6"); !disableIPv6.IsNull() && !disableIPv6.IsUndefined() { | |
| options.DisableIPv6 = disableIPv6.Bool() | |
| } | |
| if disableIPv6 := jsOptions.Get("disableIPv6"); !disableIPv6.IsNull() && !disableIPv6.IsUndefined() { | |
| if disableIPv6.Type() != js.TypeBoolean { | |
| return options, fmt.Errorf("disableIPv6 must be a boolean") | |
| } | |
| options.DisableIPv6 = disableIPv6.Bool() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/wasm/cmd/main.go` around lines 85 - 87, The JS option disableIPv6 is
read via jsOptions.Get("disableIPv6") and then used with disableIPv6.Bool()
without confirming its type, which can panic; update the logic in the main.go
block handling jsOptions.Get("disableIPv6") to first check that
disableIPv6.Type() == js.TypeBoolean (and still check !IsNull() &&
!IsUndefined()), and only then call disableIPv6.Bool() to assign
options.DisableIPv6; keep the same surrounding checks (jsOptions.Get, IsNull,
IsUndefined) and add the Type() validation to avoid a panic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/ssh/server/server.go (1)
814-814: Consider adding parentheses for clarity.The logic is correct due to Go's operator precedence (
&&binds tighter than||), but explicit parentheses would make the intent immediately clear to readers:- if remoteIP == wgAddr.IP || wgAddr.IPv6.IsValid() && remoteIP == wgAddr.IPv6 { + if remoteIP == wgAddr.IP || (wgAddr.IPv6.IsValid() && remoteIP == wgAddr.IPv6) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ssh/server/server.go` at line 814, The conditional comparing remoteIP to wgAddr.IP and wgAddr.IPv6 is correct but unclear; update the if statement that uses remoteIP, wgAddr.IP, and wgAddr.IPv6 (and the IsValid check) to add explicit parentheses around the IPv6 sub-expression so the intent is obvious (e.g., group wgAddr.IPv6.IsValid() && remoteIP == wgAddr.IPv6), leaving the overall OR with remoteIP == wgAddr.IP unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/ssh/server/server.go`:
- Around line 331-339: Protect all accesses to s.extraListeners in Stop() with
the existing mutex s.mu: acquire s.mu before reading/iterating and clearing
s.extraListeners (mirroring AddListener's locking), then release the lock before
calling Close() on each listener to avoid holding the lock during I/O; after
collecting listeners under the lock, set s.extraListeners = nil while still
holding s.mu. Also remove the redundant assignments that set s.sshServer and
s.listener to nil outside the lock since they are already cleared inside the
locked section (keep only the locked clears).
---
Nitpick comments:
In `@client/ssh/server/server.go`:
- Line 814: The conditional comparing remoteIP to wgAddr.IP and wgAddr.IPv6 is
correct but unclear; update the if statement that uses remoteIP, wgAddr.IP, and
wgAddr.IPv6 (and the IsValid check) to add explicit parentheses around the IPv6
sub-expression so the intent is obvious (e.g., group wgAddr.IPv6.IsValid() &&
remoteIP == wgAddr.IPv6), leaving the overall OR with remoteIP == wgAddr.IP
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d5fa199-b90f-45de-818e-19c0cc6d03a0
⛔ Files ignored due to path filters (2)
shared/management/proto/management.pb.gois excluded by!**/*.pb.goshared/management/proto/management_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (16)
client/android/client.goclient/android/peer_notifier.goclient/embed/embed.goclient/internal/auth/auth.goclient/internal/connect.goclient/internal/connect_android_default.goclient/internal/engine.goclient/internal/engine_test.goclient/internal/profilemanager/config.goclient/server/server.goclient/ssh/server/server.goclient/system/info.goclient/ui/client_ui.goclient/ui/network.goshared/management/client/grpc.goshared/management/proto/management.proto
✅ Files skipped from review due to trivial changes (3)
- client/internal/auth/auth.go
- client/android/peer_notifier.go
- client/server/server.go
🚧 Files skipped from review as they are similar to previous changes (7)
- shared/management/client/grpc.go
- client/ui/network.go
- client/android/client.go
- client/system/info.go
- client/embed/embed.go
- client/internal/connect.go
- client/internal/engine_test.go
Comment on lines
+331
to
+339
| for _, ln := range s.extraListeners { | ||
| if err := ln.Close(); err != nil { | ||
| log.Debugf("close extra SSH listener: %v", err) | ||
| } | ||
| } | ||
| s.extraListeners = nil | ||
|
|
||
| s.sshServer = nil | ||
| s.listener = nil |
Contributor
There was a problem hiding this comment.
Data race: extraListeners accessed outside mutex protection.
s.extraListeners is read and cleared at lines 331-336 without holding s.mu, while AddListener modifies it under the lock (line 274). This creates a data race if Stop() and AddListener() execute concurrently.
Additionally, lines 338-339 redundantly set sshServer and listener to nil — they were already set inside the lock at lines 322-323.
Proposed fix
func (s *Server) Stop() error {
s.mu.Lock()
sshServer := s.sshServer
if sshServer == nil {
s.mu.Unlock()
return nil
}
s.sshServer = nil
s.listener = nil
+ extraListeners := s.extraListeners
+ s.extraListeners = nil
s.mu.Unlock()
// Close outside the lock: session handlers need s.mu for unregisterSession.
if err := sshServer.Close(); err != nil {
log.Debugf("close SSH server: %v", err)
}
- for _, ln := range s.extraListeners {
+ for _, ln := range extraListeners {
if err := ln.Close(); err != nil {
log.Debugf("close extra SSH listener: %v", err)
}
}
- s.extraListeners = nil
-
- s.sshServer = nil
- s.listener = nil
s.mu.Lock()
maps.Clear(s.sessions)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, ln := range s.extraListeners { | |
| if err := ln.Close(); err != nil { | |
| log.Debugf("close extra SSH listener: %v", err) | |
| } | |
| } | |
| s.extraListeners = nil | |
| s.sshServer = nil | |
| s.listener = nil | |
| func (s *Server) Stop() error { | |
| s.mu.Lock() | |
| sshServer := s.sshServer | |
| if sshServer == nil { | |
| s.mu.Unlock() | |
| return nil | |
| } | |
| s.sshServer = nil | |
| s.listener = nil | |
| extraListeners := s.extraListeners | |
| s.extraListeners = nil | |
| s.mu.Unlock() | |
| // Close outside the lock: session handlers need s.mu for unregisterSession. | |
| if err := sshServer.Close(); err != nil { | |
| log.Debugf("close SSH server: %v", err) | |
| } | |
| for _, ln := range extraListeners { | |
| if err := ln.Close(); err != nil { | |
| log.Debugf("close extra SSH listener: %v", err) | |
| } | |
| } | |
| s.mu.Lock() | |
| maps.Clear(s.sessions) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/ssh/server/server.go` around lines 331 - 339, Protect all accesses to
s.extraListeners in Stop() with the existing mutex s.mu: acquire s.mu before
reading/iterating and clearing s.extraListeners (mirroring AddListener's
locking), then release the lock before calling Close() on each listener to avoid
holding the lock during I/O; after collecting listeners under the lock, set
s.extraListeners = nil while still holding s.mu. Also remove the redundant
assignments that set s.sshServer and s.listener to nil outside the lock since
they are already cleared inside the locked section (keep only the locked
clears).
Contributor
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)
client/internal/engine.go (1)
1426-1433:⚠️ Potential issue | 🟠 MajorMake
toDNSConfignil-safe.This helper dereferences
protoDNSConfigimmediately, but Line 1813 passesnetMap.GetDNSConfig()straight through during Android bootstrap. A nilDNSConfigfrom management will panic there, even though the steady-state path already normalizes it.Proposed fix
func toDNSConfig(protoDNSConfig *mgmProto.DNSConfig, addr wgaddr.Address) nbdns.Config { network := addr.Network networkV6 := addr.IPv6Net + if protoDNSConfig == nil { + protoDNSConfig = &mgmProto.DNSConfig{} + } //nolint forwarderPort := uint16(protoDNSConfig.GetForwarderPort())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/engine.go` around lines 1426 - 1433, The toDNSConfig helper dereferences protoDNSConfig without checking for nil, which can panic when netMap.GetDNSConfig() returns nil; update toDNSConfig to handle a nil protoDNSConfig by treating it as an empty/default mgmProto.DNSConfig (or by returning a safe default nbdns.Config) before accessing fields, then proceed to compute network, networkV6, forwarderPort (using nbdns.ForwarderClientPort when zero) and other fields as before; ensure callers like netMap.GetDNSConfig() can pass nil safely and reference symbols: toDNSConfig, protoDNSConfig, netMap.GetDNSConfig(), nbdns.Config, nbdns.ForwarderClientPort.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/internal/engine.go`:
- Around line 1511-1526: splitAllowedIPs currently accepts only ourV6Net and
accepts any /32 as the peer IPv4 overlay address, which can mis-populate
state.IP; modify splitAllowedIPs to take an additional ourV4Net netip.Prefix
parameter and only accept a /32 IPv4 if ourV4Net.Contains(prefix.Addr()) (keep
the existing v6 guard using ourV6Net). Update all callers (e.g., where
splitAllowedIPs is invoked to compute v4,v6 for offlinePeer and peerConfig) to
pass the local IPv4 network (e.g., e.wgInterface.Address().Network) as the new
ourV4Net argument so v4 is only set when the /32 is inside our IPv4 overlay.
---
Outside diff comments:
In `@client/internal/engine.go`:
- Around line 1426-1433: The toDNSConfig helper dereferences protoDNSConfig
without checking for nil, which can panic when netMap.GetDNSConfig() returns
nil; update toDNSConfig to handle a nil protoDNSConfig by treating it as an
empty/default mgmProto.DNSConfig (or by returning a safe default nbdns.Config)
before accessing fields, then proceed to compute network, networkV6,
forwarderPort (using nbdns.ForwarderClientPort when zero) and other fields as
before; ensure callers like netMap.GetDNSConfig() can pass nil safely and
reference symbols: toDNSConfig, protoDNSConfig, netMap.GetDNSConfig(),
nbdns.Config, nbdns.ForwarderClientPort.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dda78081-1e72-426d-ab41-0c25c9e061e8
📒 Files selected for processing (2)
client/internal/dns/server_test.goclient/internal/engine.go
✅ Files skipped from review due to trivial changes (1)
- client/internal/dns/server_test.go
Comment on lines
+1511
to
+1526
| // splitAllowedIPs separates the peer's overlay v4 (/32) and v6 (/128) addresses | ||
| // from a list of AllowedIPs CIDRs. The v6 address is only matched if it falls | ||
| // within ourV6Net (the local overlay v6 subnet), to avoid confusing routed /128 | ||
| // prefixes with the peer's overlay address. | ||
| func splitAllowedIPs(allowedIPs []string, ourV6Net netip.Prefix) (v4, v6 string) { | ||
| for _, cidr := range allowedIPs { | ||
| prefix, err := netip.ParsePrefix(cidr) | ||
| if err != nil { | ||
| log.Warnf("failed to parse AllowedIP %q: %v", cidr, err) | ||
| continue | ||
| } | ||
| switch { | ||
| case prefix.Addr().Is4() && prefix.Bits() == 32 && v4 == "": | ||
| v4 = prefix.Addr().String() | ||
| case prefix.Addr().Is6() && prefix.Bits() == 128 && ourV6Net.Contains(prefix.Addr()) && v6 == "": | ||
| v6 = prefix.Addr().String() |
Contributor
There was a problem hiding this comment.
Don’t treat the first /32 as the peer’s IPv4 overlay address.
The IPv6 branch already guards against routed /128s with ourV6Net.Contains(...), but the IPv4 branch still accepts any /32. If a peer advertises a host route before its overlay address, Line 1497 and Line 1575 can write the wrong state.IP.
Proposed fix
-func splitAllowedIPs(allowedIPs []string, ourV6Net netip.Prefix) (v4, v6 string) {
+func splitAllowedIPs(allowedIPs []string, ourV4Net, ourV6Net netip.Prefix) (v4, v6 string) {
for _, cidr := range allowedIPs {
prefix, err := netip.ParsePrefix(cidr)
if err != nil {
log.Warnf("failed to parse AllowedIP %q: %v", cidr, err)
continue
}
switch {
- case prefix.Addr().Is4() && prefix.Bits() == 32 && v4 == "":
+ case prefix.Addr().Is4() && prefix.Bits() == 32 && ourV4Net.Contains(prefix.Addr()) && v4 == "":
v4 = prefix.Addr().String()
case prefix.Addr().Is6() && prefix.Bits() == 128 && ourV6Net.Contains(prefix.Addr()) && v6 == "":
v6 = prefix.Addr().String()
}v4, v6 := splitAllowedIPs(offlinePeer.GetAllowedIps(), e.wgInterface.Address().Network, e.wgInterface.Address().IPv6Net)
peerV4, peerV6 := splitAllowedIPs(peerConfig.GetAllowedIps(), e.wgInterface.Address().Network, e.wgInterface.Address().IPv6Net)Based on learnings: PeerByIP() intentionally checks only state.IP (IPv4), and its only caller is forwardingrules.go, so mis-populating state.IP here has functional impact beyond the UI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/internal/engine.go` around lines 1511 - 1526, splitAllowedIPs
currently accepts only ourV6Net and accepts any /32 as the peer IPv4 overlay
address, which can mis-populate state.IP; modify splitAllowedIPs to take an
additional ourV4Net netip.Prefix parameter and only accept a /32 IPv4 if
ourV4Net.Contains(prefix.Addr()) (keep the existing v6 guard using ourV6Net).
Update all callers (e.g., where splitAllowedIPs is invoked to compute v4,v6 for
offlinePeer and peerConfig) to pass the local IPv4 network (e.g.,
e.wgInterface.Address().Network) as the new ourV4Net argument so v4 is only set
when the /32 is inside our IPv4 overlay.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
management/server/store/sql_store.go (1)
4080-4083: Wording nit: avoid naming a single caller in the rationale.Line 4082 names only
UpdateAccountSettings, butSaveAccountSettingsis also called from other paths that do the same pre-check. Using “callers” keeps this comment accurate over time.Proposed comment tweak
- // caller (UpdateAccountSettings) already verified the account exists via - // GetAccountSettings with LockingStrengthUpdate. + // callers already verify the account exists (e.g. via + // GetAccountSettings with LockingStrengthUpdate) before this update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/store/sql_store.go` around lines 4080 - 4083, The comment about MySQL RowsAffected should avoid naming a single caller; update the comment to refer to callers in general (e.g., callers like SaveAccountSettings and UpdateAccountSettings) and state that the pre-check is performed via GetAccountSettings with LockingStrengthUpdate, so skipping the RowsAffected check is safe. Modify the comment near the MySQL note to use plural "callers" and optionally cite SaveAccountSettings and UpdateAccountSettings and GetAccountSettings(LockingStrengthUpdate) to make the reasoning accurate and future-proof.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@management/server/store/sql_store.go`:
- Around line 4080-4083: The comment about MySQL RowsAffected should avoid
naming a single caller; update the comment to refer to callers in general (e.g.,
callers like SaveAccountSettings and UpdateAccountSettings) and state that the
pre-check is performed via GetAccountSettings with LockingStrengthUpdate, so
skipping the RowsAffected check is safe. Modify the comment near the MySQL note
to use plural "callers" and optionally cite SaveAccountSettings and
UpdateAccountSettings and GetAccountSettings(LockingStrengthUpdate) to make the
reasoning accurate and future-proof.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 536a68c3-dd3c-48e5-96a4-0a92ef23d594
📒 Files selected for processing (1)
management/server/store/sql_store.go
|
lixmal
changed the title
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.
Describe your changes
This PR implements IPv6 overlay support in client and management.
The base branch adds the following changes:
addrFamilyabstraction encapsulating IPv4/IPv6 header offsets, address lengths, set key types, and ICMP protocol numbersip6 netbirdtable with its own router and ACL manager when the interface has IPv6UpdateSetprefixes by family for dynamic DNS route setsTestNftablesCreateIpSet_IPv6) andcalculateLastIPtests covering both familiesIssue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
netbirdio/docs#667
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests