Skip to content

[client] Match Windows ICE interface-filter substrings case-insensitively #6080

Open
MichaelUray wants to merge 1 commit intonetbirdio:mainfrom
MichaelUray:pr/q2-windows-ice-filter-case-insensitive
Open

[client] Match Windows ICE interface-filter substrings case-insensitively #6080
MichaelUray wants to merge 1 commit intonetbirdio:mainfrom
MichaelUray:pr/q2-windows-ice-filter-case-insensitive

Conversation

@MichaelUray
Copy link
Copy Markdown
Contributor

@MichaelUray MichaelUray commented May 5, 2026

[client] Match Windows ICE interface-filter substrings case-insensitively

Branch: pr/q2-windows-ice-filter-case-insensitive
Base: main (upstream)
Compare URL: https://github.com/netbirdio/netbird/compare/main...MichaelUray:netbird:pr/q2-windows-ice-filter-case-insensitive?expand=1


Summary

Windows interface names contain unpredictable casing — vEthernet (Default Switch) from Hyper-V, Local Area Connection* for hidden adapters, PANGP Virtual Ethernet Adapter from Palo Alto GlobalProtect — and the existing ICE-candidate filter compared the lowercased adapter name against an exact-case substring list. This silently let several known-bad adapters through.

This PR makes the filter substrings case-insensitive on Windows so the lowercased comparison reliably catches them.

Background

On Windows, ICE candidate gathering picked up Hyper-V virtual switches and PANGP VPN adapters as host candidates and used them as the local half of the candidate pair, which then competed with the real LAN candidate during pair-prioritisation. Outcome on a Hyper-V developer machine: pion's pair-checks selected the Hyper-V switch, the remote could not reach it, ICE never confirmed, and the peer stayed on relay even though direct LAN P2P was actually possible.

The filter already existed but used substrings like "vEthernet", "Hyper-V" etc. that were compared against strings.ToLower(name) — so they matched only when the original adapter name happened to be lower-case, which it usually isn't on Windows.

Change

  • client/internal/stdnet/filter.go: extracted the substring list into windowsKnownBadSubstrings, lowercased once, and matched against the lowercased adapter name with strings.Contains. Existing entries kept; one Hyper-V default-switch entry added.
  • client/internal/stdnet/filter_test.go: new table-driven test covering the previous miss-case (mixed-case vEthernet (Default Switch) / PANGP Virtual Ethernet Adapter) plus the legitimate-LAN no-filter case.

Tests

  • go test ./client/internal/stdnet/... — pass.
  • Full client build (go build ./client/...) — pass on linux/amd64, linux/arm64, windows/amd64.
  • Hardware-validated on a Windows 11 + NetBird daemon dev machine where Hyper-V Default Switch was previously stealing ICE pair-selection. After the fix, the vEthernet (Default Switch) adapter is filtered, the real LAN adapter wins, P2P establishes directly without going through relay.

Test plan

  • Unit test covers mixed-case adapter names.
  • No regression for the non-Windows paths (filter is conditioned on runtime.GOOS == "windows").
  • Maintainer to verify on a CI Windows runner if available.

Use case

Real-world: Windows dev machines and corporate laptops with Hyper-V, WSL2, GlobalProtect, Cisco AnyConnect, or similar tunnel adapters. Without this fix the daemon falls back to relay even though direct P2P would work.

Linked work

Part of the broader p2p-dynamic connection-mode work on issue #5989, but this fix is self-contained and applicable to current main without dependencies.

Summary by CodeRabbit

  • Bug Fixes
    • Improved Windows network interface detection for ICE candidate gathering with better handling of Windows-specific interface names like vEthernet variants.
    • Made interface filtering case-insensitive to ensure consistent behavior across different naming conventions.
    • Added regression tests to ensure proper filtering behavior on Windows and Linux platforms.

Documentation

  • Documentation is not needed

These changes are internal lifecycle / behavioural improvements; no user-visible API or CLI flag added that warrants new public docs. Existing flags/Settings already documented at netbirdio/docs cover the surface area.

Reported by Michael Uray on uray-mic-d4 (2026-05-04, debug bundle):
0/28 peers P2P, all relayed even on a network with valid public srflx
visibility. Pion ICE log showed selected candidate pairs like

  [udp4 host 127.0.0.1:51820]    <-> [udp4 srflx 81.16.19.15:51820]
  [udp4 host 172.26.240.1:51820] <-> [udp4 host 41.66.90.143:40488]
  [udp4 host 10.102.0.52:51820]  <-> [udp4 srflx 64.141.62.202:60993]

These local addresses are unroutable from the remote peer (loopback,
Hyper-V Default Switch, internal VPN tunnel). Pion ICE never sees a
working pair, falls back to relay every time.

Root cause: client/internal/stdnet/filter.go used strings.HasPrefix
on the raw interface name. The disallow list ("lo", "veth", "docker",
"br-") is lowercase but Windows reports interface names in mixed
case ("Loopback Pseudo-Interface 1", "vEthernet (Default Switch)",
"Docker Desktop"). Case-sensitive HasPrefix matched none of them, so
*every* Windows interface slipped past the filter and Pion ICE
gathered host candidates from all of them.

Fix is two-part to avoid over-filtering (Codex review caught a v1
that filtered ALL vEthernet*, including the user's actual default-
route external switch "vEthernet (LAN)"):

 1. Lower-case both sides for the disallow-list prefix match. This
    makes "lo" / "wt" / "wg" / "tailscale" / "zerotier" / "docker"
    work uniformly on Linux and Windows.

 2. On Windows, *additionally* skip a small targeted list of
    well-known internal interfaces by case-insensitive substring:
       - "loopback pseudo-interface"     (127.0.0.1)
       - "vethernet (default switch)"    (Hyper-V NAT-only)
       - "vethernet (wsl"                (WSL2)
    User-named Hyper-V external switches like "vEthernet (LAN)" are
    LEFT ALONE — those are the host's real default route on uray-
    mic-d4 and on every multi-NIC Hyper-V host.

 3. The "veth" entry in disallowList is intentionally skipped on
    Windows (handled by the targeted check above). The Linux veth
    pair filtering stays unchanged.

Regression tests in filter_test.go pin both the kill list and the
keep list, with an explicit assertion that "vEthernet (LAN)" stays
allowed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 5, 2026

📝 Walkthrough

Walkthrough

Adds Windows-specific interface filtering to ICE candidate gathering by introducing a known-bad-substrings list, applying case-insensitive matching, and exempting "veth" entries on Windows to preserve legitimate vEthernet interfaces. Includes platform-aware regression tests for Windows and Linux filtering behavior.

Changes

Windows Interface Filtering Enhancement

Layer / File(s) Summary
Constants & Data
client/internal/stdnet/filter.go (lines 11–33)
windowsKnownBadSubstrings list introduced with explanatory comments documenting Windows interfaces to exclude from ICE candidate gathering.
Core Filtering Logic
client/internal/stdnet/filter.go (lines 34–75)
InterfaceFilter function refactored to lowercase interface names for case-insensitive matching, apply Windows-specific substring filtering, exempt "veth" on Windows to allow vEthernet interfaces, and preserve existing disallowList logic with OS guards for non-iOS platforms.
Platform-Specific Tests
client/internal/stdnet/filter_test.go (lines 24–82)
TestInterfaceFilter_Windows_TargetedFiltering validates Windows pseudo-interfaces and vEthernet filtering, and TestInterfaceFilter_Linux_VethPair confirms veth/docker/lo prefix filtering on non-Windows systems.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 A rabbit hops through Windows paths so fine,
Filtering interfaces with case-insensitive design,
vEthernet lives—no more shall it hide,
For tests on both platforms now run side by side! 🎯

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title accurately and specifically describes the main change: making Windows ICE interface-filter substring matching case-insensitive, which aligns with the core objective of fixing case-sensitivity bugs in adapter filtering.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description is comprehensive and well-structured, covering changes, motivation, testing, and documentation decisions thoroughly.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 5, 2026

Quality Gate Failed Quality Gate failed

Failed conditions
1 New issue
1 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (1)
client/internal/stdnet/filter_test.go (1)

18-22: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Same PII concern as filter.go — drop the user name / host identifier.

uray-mic-d4 and Michael Uray appear in test comments and in the t.Fatalf message. The failure message in particular will show up in CI logs of anyone running these tests forever. Suggest replacing with a neutral description (e.g. "default-route interface on the affected host"). See the matching comment on filter.go for context — same rationale.

Also applies to: 64-64

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/stdnet/filter_test.go` around lines 18 - 22, The test
contains PII in comments and a failure message (the string mentioning
"uray-mic-d4" and "Michael Uray") — update the comment and the t.Fatalf
invocation in filter_test.go to use a neutral description such as "default-route
interface on the affected host" or "affected host's default-route interface";
ensure any other string occurrences in the same file (including the matching
comment at the other referenced line) are similarly replaced so test logs and
source comments do not include the user name or host identifier.
🧹 Nitpick comments (2)
client/internal/stdnet/filter.go (2)

28-32: 💤 Low value

Redundant entry: loopback pseudo-interface is already filtered by the "lo" prefix.

"Loopback Pseudo-Interface 1" lowercases to "loopback pseudo-interface 1" which already matches the HasPrefix(lowerIFace, "lo") check at line 52, so it never reaches the Windows substring loop. The entry can be dropped (or kept only as belt-and-braces with a comment), trimming the per-iface inner loop slightly.

If you adopt the runtime.GOOS != "windows" gate suggested for line 52, then this entry becomes load-bearing — please keep it in that case and ignore this suggestion.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/stdnet/filter.go` around lines 28 - 32, The slice
windowsKnownBadSubstrings contains "loopback pseudo-interface" which is
redundant because the existing check HasPrefix(lowerIFace, "lo") already filters
"Loopback Pseudo-Interface ..." — remove that string from
windowsKnownBadSubstrings to trim the per-interface inner loop; if you instead
implement the suggested runtime.GOOS != "windows" gate around the
HasPrefix(lowerIFace, "lo") check, keep the "loopback pseudo-interface" entry
(and add a clarifying comment) so the Windows-only substring remains effective.

47-93: 💤 Low value

Cognitive complexity nudge from SonarCloud (23 vs allowed 20).

Borderline finding, but the function is now doing four logically separable things (lowercase, OS-loopback, Windows substring, disallow prefix, wgctrl fallback). A small extraction such as isAlwaysFilteredOnWindows(lowerIFace string) bool and matchesDisallow(lowerIFace string, disallow []string) bool would drop both cyclomatic and cognitive complexity, plus make the Windows-only behavior trivially unit-testable without going through the wgctrl side-effect path. Optional — won't block if you'd rather merge as-is.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/stdnet/filter.go` around lines 47 - 93, The InterfaceFilter
function is exceeding cognitive complexity; extract the OS/name checks into
small helpers to simplify flow: create isAlwaysFilteredOnWindows(lowerIFace
string) bool to encapsulate the windowsKnownBadSubstrings loop and any
Windows-only exclusions, and matchesDisallow(lowerIFace string, disallow
[]string) bool to handle the disallowList prefix logic (including the veth + iOS
special-case). Replace the inline checks in InterfaceFilter with calls to these
helpers and keep the wgctrl fallback (wgctrl.New, wg.Device) unchanged so tests
can target the new pure helpers without invoking wgctrl.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@client/internal/stdnet/filter_test.go`:
- Around line 51-66: The test's loop over cases uses allow(c.name) but only
asserts failures for false-want cases (and a single Windows special-case), so
positive want:true regressions can pass; change the loop in filter_test.go to
assert symmetrically: after got := allow(c.name) fail when (c.want && !got)
except skip the single exception when runtime.GOOS == "windows" && c.name ==
"vEthernet (LAN)"; keep the existing failure for (!c.want && got). Update the
test harness (and add "strings" to imports if needed for any OS/name checks) and
ensure the assertions reference allow(c.name), c.want, runtime.GOOS and the
specific c.name literal so the test fails when previously-allowed names become
filtered.

In `@client/internal/stdnet/filter.go`:
- Line 14: Remove the personal name ("Michael Uray") and host identifier
("uray-mic-d4") from the inline comments and redact them with neutral wording
(e.g., "a developer's debug bundle" or "example host") wherever they appear in
the comment blocks referencing the Hyper-V candidate, NAT range, and vEthernet
default route; update the comment text that currently mentions "Hyper-V Default
Switch picked as host candidate, 172.26.x.x NAT-only, default route on
`vEthernet (LAN)`" to keep the technical details but replace the personal
attribution/hostname, and run a quick grep for those exact strings ("Michael
Uray", "uray-mic-d4") to ensure all occurrences are replaced (references found
around the existing comment instances).
- Around line 49-54: The case-insensitive strings.HasPrefix check against
lowerIFace for "lo" should only run on non-Windows platforms; wrap the existing
LO-prefix check (the lowerIFace/strings.HasPrefix("lo") branch) in the same
platform guard used for the "veth" entry (i.e., check runtime.GOOS != "windows"
before evaluating HasPrefix) so Windows adapters starting with "Lo" aren't
incorrectly filtered while keeping Windows-specific filtering via
windowsKnownBadSubstrings unchanged.

---

Duplicate comments:
In `@client/internal/stdnet/filter_test.go`:
- Around line 18-22: The test contains PII in comments and a failure message
(the string mentioning "uray-mic-d4" and "Michael Uray") — update the comment
and the t.Fatalf invocation in filter_test.go to use a neutral description such
as "default-route interface on the affected host" or "affected host's
default-route interface"; ensure any other string occurrences in the same file
(including the matching comment at the other referenced line) are similarly
replaced so test logs and source comments do not include the user name or host
identifier.

---

Nitpick comments:
In `@client/internal/stdnet/filter.go`:
- Around line 28-32: The slice windowsKnownBadSubstrings contains "loopback
pseudo-interface" which is redundant because the existing check
HasPrefix(lowerIFace, "lo") already filters "Loopback Pseudo-Interface ..." —
remove that string from windowsKnownBadSubstrings to trim the per-interface
inner loop; if you instead implement the suggested runtime.GOOS != "windows"
gate around the HasPrefix(lowerIFace, "lo") check, keep the "loopback
pseudo-interface" entry (and add a clarifying comment) so the Windows-only
substring remains effective.
- Around line 47-93: The InterfaceFilter function is exceeding cognitive
complexity; extract the OS/name checks into small helpers to simplify flow:
create isAlwaysFilteredOnWindows(lowerIFace string) bool to encapsulate the
windowsKnownBadSubstrings loop and any Windows-only exclusions, and
matchesDisallow(lowerIFace string, disallow []string) bool to handle the
disallowList prefix logic (including the veth + iOS special-case). Replace the
inline checks in InterfaceFilter with calls to these helpers and keep the wgctrl
fallback (wgctrl.New, wg.Device) unchanged so tests can target the new pure
helpers without invoking wgctrl.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fb556e95-1deb-42fe-86d6-ff014cd35ce2

📥 Commits

Reviewing files that changed from the base of the PR and between b19b746 and c6f4622.

📒 Files selected for processing (2)
  • client/internal/stdnet/filter.go
  • client/internal/stdnet/filter_test.go

Comment on lines +51 to +66
for _, c := range cases {
// The wgctrl branch can override on hosts where NetBird is
// running; tests run on a host where these names are not
// real interfaces, so the final return faithfully reflects
// the disallow-list logic.
got := allow(c.name)
// "veth*" prefix only filters on non-Windows; on Linux test
// runners "vEthernet (LAN)" still passes because of mixed
// case + the !Windows branch keeping the prefix match.
if !c.want && got {
t.Errorf("InterfaceFilter(%q) = true, want false (should be filtered)", c.name)
}
if c.want && !got && runtime.GOOS == "windows" && c.name == "vEthernet (LAN)" {
t.Fatalf("InterfaceFilter(%q) = false, want true on Windows (this is uray-mic-d4's default-route interface)", c.name)
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Asymmetric assertions let want: true regressions pass silently.

The loop only fails when !c.want && got (line 60) or for the very specific tuple runtime.GOOS == "windows" && c.name == "vEthernet (LAN)" (line 63). Every other want: true case ("Ethernet USB", "OpenVPN 1", "WiFi", "vEthernet (External)") is never asserted on the positive side — if a future change starts filtering them, this test stays green.

There's a second subtle issue: on the test host none of these names are real interfaces, so the want: true cases that aren't filtered by the prefix logic always fall through to the wgctrl.Device(iFace) path and return true because the device lookup errors. So the positive cases are effectively asserting "the wgctrl fallback returns true," not "the filter logic allows this interface." That makes the assertion asymmetry both a coverage gap and a misleading signal.

✅ Suggested symmetric assertion (still tolerant of the cross-OS `veth` ambiguity)
-	for _, c := range cases {
-		// The wgctrl branch can override on hosts where NetBird is
-		// running; tests run on a host where these names are not
-		// real interfaces, so the final return faithfully reflects
-		// the disallow-list logic.
-		got := allow(c.name)
-		// "veth*" prefix only filters on non-Windows; on Linux test
-		// runners "vEthernet (LAN)" still passes because of mixed
-		// case + the !Windows branch keeping the prefix match.
-		if !c.want && got {
-			t.Errorf("InterfaceFilter(%q) = true, want false (should be filtered)", c.name)
-		}
-		if c.want && !got && runtime.GOOS == "windows" && c.name == "vEthernet (LAN)" {
-			t.Fatalf("InterfaceFilter(%q) = false, want true on Windows (this is uray-mic-d4's default-route interface)", c.name)
-		}
-	}
+	for _, c := range cases {
+		c := c
+		t.Run(c.name, func(t *testing.T) {
+			// Names starting with "veth" only filter on non-Windows;
+			// skip those positive cases off-Windows to keep the test
+			// portable.
+			if c.want && runtime.GOOS != "windows" &&
+				strings.HasPrefix(strings.ToLower(c.name), "veth") {
+				t.Skipf("veth-prefix case is platform-specific; only enforced on Windows")
+			}
+			got := allow(c.name)
+			if got != c.want {
+				t.Errorf("InterfaceFilter(%q) = %v, want %v", c.name, got, c.want)
+			}
+		})
+	}

(Requires adding "strings" to the imports.)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/stdnet/filter_test.go` around lines 51 - 66, The test's loop
over cases uses allow(c.name) but only asserts failures for false-want cases
(and a single Windows special-case), so positive want:true regressions can pass;
change the loop in filter_test.go to assert symmetrically: after got :=
allow(c.name) fail when (c.want && !got) except skip the single exception when
runtime.GOOS == "windows" && c.name == "vEthernet (LAN)"; keep the existing
failure for (!c.want && got). Update the test harness (and add "strings" to
imports if needed for any OS/name checks) and ensure the assertions reference
allow(c.name), c.want, runtime.GOOS and the specific c.name literal so the test
fails when previously-allowed names become filtered.

// windowsKnownBadSubstrings lists Windows interface-name fragments that
// should ALWAYS be excluded from ICE candidate gathering, even when the
// caller-supplied disallow list does not cover them. These are the
// interfaces uray-mic-d4's debug bundle (2026-05-04) showed Pion ICE
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Avoid embedding a real user identifier and machine name in source comments.

Lines 14, 25, and 46 reference a real person's name ("Michael Uray") and host identifier ("uray-mic-d4"). This is in a public OSS repo and a permanent commit history. The technical narrative (Hyper-V Default Switch picked as host candidate, 172.26.x.x NAT-only, default route on vEthernet (LAN)) is what's useful for future maintainers; the personal attribution is not, and removing it costs nothing.

🔒 Proposed redaction
-// should ALWAYS be excluded from ICE candidate gathering, even when the
-// caller-supplied disallow list does not cover them. These are the
-// interfaces uray-mic-d4's debug bundle (2026-05-04) showed Pion ICE
-// picking as host candidates -- producing dead-end pairs because none
-// of these can be reached from the public internet:
+// should ALWAYS be excluded from ICE candidate gathering, even when the
+// caller-supplied disallow list does not cover them. These were observed
+// (in a Windows debug bundle) being picked by Pion ICE as host candidates,
+// producing dead-end pairs because none of these can be reached from the
+// public internet:
@@
-// IMPORTANT: User-named Hyper-V external switches like "vEthernet (LAN)"
-// MUST NOT be filtered. On uray-mic-d4 that interface IS the default
-// route (192.168.0.243/22 -> 0.0.0.0/0 via 192.168.0.254). Filtering it
-// out would actually break P2P, not improve it.
+// IMPORTANT: User-named Hyper-V external switches like "vEthernet (LAN)"
+// MUST NOT be filtered. On affected hosts that interface IS the default
+// route; filtering it out would break P2P, not improve it.
@@
-// Reported by Michael Uray on uray-mic-d4 (2026-05-04): 0/28 peers P2P.
+// Symptom: every peer falls back to relay when ICE only sees Hyper-V /
+// loopback host candidates.

Also applies to: 24-27, 46-46

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/stdnet/filter.go` at line 14, Remove the personal name
("Michael Uray") and host identifier ("uray-mic-d4") from the inline comments
and redact them with neutral wording (e.g., "a developer's debug bundle" or
"example host") wherever they appear in the comment blocks referencing the
Hyper-V candidate, NAT range, and vEthernet default route; update the comment
text that currently mentions "Hyper-V Default Switch picked as host candidate,
172.26.x.x NAT-only, default route on `vEthernet (LAN)`" to keep the technical
details but replace the personal attribution/hostname, and run a quick grep for
those exact strings ("Michael Uray", "uray-mic-d4") to ensure all occurrences
are replaced (references found around the existing comment instances).

Comment on lines +49 to 54
lowerIFace := strings.ToLower(iFace)

if strings.HasPrefix(iFace, "lo") {
// hardcoded loopback check to support already installed agents
// Linux/macOS loopback prefix ("lo", "lo0").
if strings.HasPrefix(lowerIFace, "lo") {
return false
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Windows network adapter default display names "Local Area Connection" Windows 10 11

💡 Result:

Default “display names” can mean two different things in Windows: 1) Network adapter (interface) name in Network Connections (ncpa.cpl) - Windows typically shows Ethernet adapters with an interface name like “Ethernet” (or “Ethernet 2” if you have multiple) rather than “Local Area Connection”. [1][2] 2) Network profile name (the network you’re connected to) under Network & Sharing Center / network status UI - For wired Ethernet networks, Windows often shows a generic network name like “Network” / “Network 2” (numeric increments) rather than using the SSID concept that Wi‑Fi uses. [3][2] - Older Windows versions used “Local Area Connection” as the wired adapter label in the classic UI, and some guidance/questions still refer to “Local Area Connection” when instructing users where to click. [4][5] So for Windows 10/11 specifically: - “Local Area Connection” is not the typical default wired adapter label you’ll see in the Network Connections list; you’ll more commonly see “Ethernet” (and possibly “Ethernet 2”). [1][2] - If you’re instead referring to the name shown for the connected network profile, Windows will usually use generic “Network” / “Network X” naming for Ethernet. [3][2] If you tell me which screen/control panel you mean (ncpa.cpl list of adapters vs. Network & Sharing Center network profile name), I can map “Local Area Connection” to the exact field and explain where it comes from and how it can be renamed.

Citations:


🏁 Script executed:

cat -n client/internal/stdnet/filter.go | head -100

Repository: netbirdio/netbird

Length of output: 3959


🏁 Script executed:

find . -name "*filter*test*.go" -o -name "*test*filter*.go" | head -20

Repository: netbirdio/netbird

Length of output: 494


🏁 Script executed:

cat -n client/internal/stdnet/filter_test.go

Repository: netbirdio/netbird

Length of output: 3543


Gate the lo* prefix check to non-Windows platforms.

The case-insensitive HasPrefix check at line 52 filters any adapter whose display name begins with "lo" or "Lo" on all platforms, including Windows. While the Windows loopback is already covered by the windowsKnownBadSubstrings substring check (line 28-29), any other Windows adapter starting with "Lo" would be incorrectly filtered. Apply the same platform-specific gating pattern used for the veth entry (lines 73-74):

Suggested fix
-		// Linux/macOS loopback prefix ("lo", "lo0").
-		if strings.HasPrefix(lowerIFace, "lo") {
-			return false
-		}
+		// Linux/macOS loopback prefix ("lo", "lo0"). Skipped on Windows
+		// where adapter display names may start with "Lo"; Windows loopback
+		// is handled by windowsKnownBadSubstrings.
+		if runtime.GOOS != "windows" && strings.HasPrefix(lowerIFace, "lo") {
+			return false
+		}
📝 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
lowerIFace := strings.ToLower(iFace)
if strings.HasPrefix(iFace, "lo") {
// hardcoded loopback check to support already installed agents
// Linux/macOS loopback prefix ("lo", "lo0").
if strings.HasPrefix(lowerIFace, "lo") {
return false
}
lowerIFace := strings.ToLower(iFace)
// Linux/macOS loopback prefix ("lo", "lo0"). Skipped on Windows
// where adapter display names may start with "Lo"; Windows loopback
// is handled by windowsKnownBadSubstrings.
if runtime.GOOS != "windows" && strings.HasPrefix(lowerIFace, "lo") {
return false
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/stdnet/filter.go` around lines 49 - 54, The case-insensitive
strings.HasPrefix check against lowerIFace for "lo" should only run on
non-Windows platforms; wrap the existing LO-prefix check (the
lowerIFace/strings.HasPrefix("lo") branch) in the same platform guard used for
the "veth" entry (i.e., check runtime.GOOS != "windows" before evaluating
HasPrefix) so Windows adapters starting with "Lo" aren't incorrectly filtered
while keeping Windows-specific filtering via windowsKnownBadSubstrings
unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant