From a235e5b02ce72e2fd43d03ac817586476ea572fa Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sun, 13 Sep 2026 10:21:17 +0330 Subject: [PATCH 1/5] fix(diag): a bundle identifier is redacted by the key that names it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #65 and #63. #65 asked for an independent read of 3001e6d, which merged after the last hosted review of #55 and so was never read. The prior was that a change of that shape carries a leak. It held: five defects, and the first is the worst kind. Two identities could share one placeholder. The ordinal was derived by COUNTING order entries of a kind, so a refusal — which advances the token but appends one entry — vacated a number and handed it to the next value. `profile-1` refused its way to `profile-2`, then `alpha` counted one entry and was given `profile-2` as well. One token, two servers, and a legend reading "2 distinct profile names -> profile-2 ... profile-2". The counter is monotonic now, a candidate is refused if it is any value the bundle carries, and a discarded pass rolls the counter back with the rest. The legend no longer renders a range across a hole. A refused ordinal makes the tokens non-consecutive, and a range promises everything between its ends — the same lie the 1..n rendering told, moved to the interior. Range when they run consecutively, list when they do not. placeholderRe had drifted: the iface kind was added with its own mint path and the pattern was not. It is generated from one list of kinds now, and that matters because it became a guard rather than a comment. knownNames could launder its own tokens, which json.go asserted it could not. The replay sorts longest-first, so a value SPELLED like a token had the pair (profile-1 -> profile-2) applied to text the replay had just written. A value matching placeholderRe is skipped; leaving it verbatim costs nothing, since by its spelling it names nobody. ifaceAttrRe also matched `tunnel=`, which nothing in the tree writes. #63 was the gap #55 left open: Text ran no literal-name pass, so rules-preview.txt and log.txt had no coverage of names the bundle had already replaced. The issue proposed fixing it at the source. That is necessary and not sufficient — the entries that leak are a rendered pf/nft ruleset and slog records, and `pass out quick on { nordlynx }` cannot carry an interface as anything but a bare word. So both halves: knownNames now replays interface names, which nothing did. `nordlynx` was shipping verbatim in a default bundle from the rendered rulesets, doctor's prose, state.json's tunnel detail and the log. reserved gains vpn, tunnel and wireguard first — keepIface keeps only a generic stem plus digits, so an interface can be called those, and the replay would have rewritten dezhban's own words. Text is now the replay plus the shape passes, and shapes is what a caller says when it wants the shape passes alone; every external caller wanted the replay, so a second door would only have left the wrong one reachable. Guarding the mint rather than each pass: with the replay running first, a shape pass is handed a token, and placeholder refuses to mint for one. And at the source, doctor.json's details carry the identifier as a field under a key the walk already knows, with both renderers composing the sentence. The discover check names the connected VPN once, as data, instead of after every line. state.json publishes one entry per tunnel interface — a comma-joined name minted ONE token for the pair and stopped keepIface seeing the generic half as the kernel's. ADR-0016 records why the prose keeps its identifiers where we do not author it, so the next round does not re-litigate it from the issue comments. Verified: task check, GOOS=linux/windows go vet, swift test (265), and a real `dezhban report` over a fixture carrying a vendor interface, a single-label endpoint and a profile called `profile-1` — nothing leaks, rules-preview.txt reads `on { iface-1 utun4 }`, every JSON entry parses, the legend names only tokens that are in the bundle, and --include-network still returns all of it. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 44 +++ cmd/dezhban/doctor_test.go | 174 +++++++-- cmd/dezhban/doctor_unattended_test.go | 45 ++- cmd/dezhban/main.go | 332 ++++++++++++------ cmd/dezhban/report_test.go | 46 ++- ...6-bundle-identifiers-are-data-not-prose.md | 109 ++++++ docs/adr/README.md | 1 + docs/contribute/architecture.md | 1 + docs/contribute/testing.md | 26 +- docs/usage/cli.md | 52 ++- .../Sources/DezhbanCore/DoctorReport.swift | 49 ++- .../Sources/DezhbanMenu/DiagnosticsView.swift | 22 +- .../DezhbanCoreTests/DoctorReportTests.swift | 49 ++- internal/redact/json.go | 126 +++++-- internal/redact/json_test.go | 135 +++++++ internal/redact/redact.go | 229 ++++++++++-- internal/redact/redact_test.go | 114 ++++++ internal/runner/runner.go | 45 ++- internal/runner/tunnelsnapshot_test.go | 82 +++++ 19 files changed, 1455 insertions(+), 226 deletions(-) create mode 100644 docs/adr/0016-bundle-identifiers-are-data-not-prose.md create mode 100644 internal/runner/tunnelsnapshot_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 52bc2ed..7deaf51 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,50 @@ current as you land changes. ## [Unreleased] +### Fixed + +- **A bundle could give two different servers the same placeholder.** A profile + can legitimately be called `profile-1`, and the redactor skips a number that + would produce the name it is replacing — but it then handed that number to the + next name instead of leaving it behind. Two identities shared one token, the + legend said "2 distinct profile names → profile-2 … profile-2", and a bundle + that reports two servers as one looks like a working bundle until someone tries + to diagnose with it. A placeholder is now never a name the bundle carries, and + never stands for more than one thing. +- **The README's legend no longer advertises tokens that are not in the bundle.** + When a number is skipped the tokens stop being consecutive, and a range implies + everything between its ends. The legend now prints a range only when the tokens + really do run consecutively, and lists them otherwise. +- **Tunnel interface names your VPN client created now go from the prose, the + rendered rulesets and the log — not only from the fields that name them.** + `nordlynx` in `pass out quick on { nordlynx }`, in a doctor finding, in + `state.json`'s tunnel detail and in `detail="nordlynx up"` were all shipping + verbatim in a default, redacted bundle: a name with no dot is invisible to a + hostname shape, and the pass that reaches such names was not running over the + bundle's text entries at all. Every `utun*`, `en0` and `lo0` still survives — + they identify nobody, and hiding them would make a ruleset unreadable. +- **A single-label server name quoted inside a log error no longer survives.** + The daemon logs it twice — `host=mullvad err="lookup mullvad: no such host"` — + and only the first copy was being replaced. +- **`state.json` no longer reports two tunnel interfaces as one.** Multiple + interfaces were joined with a comma into a single name, which the redactor then + replaced with one placeholder for the pair — and stopped recognising the + generic half as a name worth keeping. + +### Changed + +- **`dezhban doctor --json` carries its identifiers as data.** Each entry of a + check's `details` is now an object, `{iface?, profile?, endpoint?, text}`, + instead of a pre-composed string, and a check may carry `profiles` and + `connectedVPN`. `dezhban doctor` and the app's Diagnostics pane print the same + sentences as before; what changed is that the identifier inside them is a field + the bundle's redactor can recognise rather than a word it has to hunt for. The + discover section names the connected VPN once, above its findings, instead of + after each one. See [ADR-0016](docs/adr/0016-bundle-identifiers-are-data-not-prose.md). +- **`status --json` and `state.json` publish one entry per tunnel interface**, + each naming exactly one, and a tunnel's `detail` no longer repeats the name + already in its `name`. + ## [0.14.0] - 2026-09-10 ### Added diff --git a/cmd/dezhban/doctor_test.go b/cmd/dezhban/doctor_test.go index fdf6c0d..50dd1f2 100644 --- a/cmd/dezhban/doctor_test.go +++ b/cmd/dezhban/doctor_test.go @@ -1,10 +1,13 @@ package main import ( + "bytes" + "encoding/json" "errors" "io" "log/slog" "net/netip" + "slices" "strings" "testing" "time" @@ -27,7 +30,7 @@ func TestBuildTunnelsCheck(t *testing.T) { if c.Status != checkWarn { t.Errorf("status = %q, want %q", c.Status, checkWarn) } - if len(c.Details) != 1 || c.Details[0] != "(none — set vpn.tunnelInterfaces or vpn.autoDetect)" { + if len(c.Details) != 1 || c.Details[0].line() != "(none — set vpn.tunnelInterfaces or vpn.autoDetect)" { t.Errorf("details = %v", c.Details) } }) @@ -39,7 +42,7 @@ func TestBuildTunnelsCheck(t *testing.T) { t.Errorf("status = %q, want %q", c.Status, checkOK) } want := "utun4 — 10.0.0.0/24" - if len(c.Details) != 1 || c.Details[0] != want { + if len(c.Details) != 1 || c.Details[0].line() != want { t.Errorf("details = %v, want [%q]", c.Details, want) } }) @@ -50,7 +53,7 @@ func TestBuildTunnelsCheck(t *testing.T) { t.Errorf("status = %q, want %q (informational only)", c.Status, checkOK) } want := "utun9 — no subnet (interface down or absent?)" - if len(c.Details) != 1 || c.Details[0] != want { + if len(c.Details) != 1 || c.Details[0].line() != want { t.Errorf("details = %v, want [%q]", c.Details, want) } }) @@ -62,7 +65,7 @@ func TestBuildTunnelsCheck(t *testing.T) { } c := buildTunnelsCheck([]string{"utun4"}, nets) want := "utun4 — 10.0.0.0/24, fe80::/64" - if len(c.Details) != 1 || c.Details[0] != want { + if len(c.Details) != 1 || c.Details[0].line() != want { t.Errorf("details = %v, want [%q]", c.Details, want) } }) @@ -95,7 +98,7 @@ func TestBuildLivenessCheck(t *testing.T) { if c.Status != checkWarn { t.Errorf("status = %q, want %q", c.Status, checkWarn) } - if len(c.Details) != 1 || !strings.Contains(c.Details[0], "re-applied 2 time(s)") { + if len(c.Details) != 1 || !strings.Contains(c.Details[0].line(), "re-applied 2 time(s)") { t.Errorf("details = %v", c.Details) } }) @@ -111,7 +114,7 @@ func TestBuildLivenessCheck(t *testing.T) { if c.Status != checkOK { t.Errorf("status = %q, want %q", c.Status, checkOK) } - if len(c.Details) != 1 || !strings.Contains(c.Details[0], "Exit IP last changed at") { + if len(c.Details) != 1 || !strings.Contains(c.Details[0].line(), "Exit IP last changed at") { t.Errorf("details = %v", c.Details) } }) @@ -154,7 +157,7 @@ func TestBuildLivenessCheck(t *testing.T) { if c.Status != checkWarn { t.Errorf("status = %q, want %q", c.Status, checkWarn) } - if len(c.Details) != 1 || !strings.Contains(c.Details[0], "apply: exit status 1") { + if len(c.Details) != 1 || !strings.Contains(c.Details[0].line(), "apply: exit status 1") { t.Errorf("details = %v", c.Details) } if !strings.Contains(c.Summary, "could not confirm") { @@ -219,7 +222,7 @@ func TestBuildControlCheck(t *testing.T) { if len(c.Fixes) != 0 { t.Errorf("fixes = %v, want none — there is no portable add-to-group command to badge", c.Fixes) } - if !strings.Contains(strings.Join(c.Details, "\n"), "passwordless.md") { + if !strings.Contains(detailText(c), "passwordless.md") { t.Errorf("details = %v, want the doc pointer", c.Details) } }) @@ -271,7 +274,7 @@ func TestBuildControlCheck(t *testing.T) { cfg := base() cfg.Control.Group = "" c := buildControlCheck(&cfg, control.Response{}, errors.New("dial: no such file")) - if len(c.Details) == 0 || !strings.Contains(c.Details[0], "no group") { + if len(c.Details) == 0 || !strings.Contains(c.Details[0].line(), "no group") { t.Errorf("details = %v, want a note about the missing group", c.Details) } }) @@ -304,7 +307,7 @@ func TestBuildControlCheck(t *testing.T) { cfg.Control.AllowSwitchOps = false cfg.Control.AllowConfigOps = false c := buildControlCheck(&cfg, control.Response{OK: true}, nil) - joined := strings.Join(c.Details, "\n") + joined := detailText(c) if !strings.Contains(joined, "allowSwitchOps=false") { t.Errorf("details = %v, want the switch gate named", c.Details) } @@ -323,7 +326,7 @@ func TestBuildEndpointsCheck(t *testing.T) { if c.Status != checkWarn { t.Errorf("status = %q, want %q", c.Status, checkWarn) } - if len(c.Details) != 1 || c.Details[0] != "(none resolved)" { + if len(c.Details) != 1 || c.Details[0].line() != "(none resolved)" { t.Errorf("details = %v", c.Details) } }) @@ -335,7 +338,7 @@ func TestBuildEndpointsCheck(t *testing.T) { t.Errorf("status = %q, want %q", c.Status, checkOK) } want := "203.0.113.9 — ok (assumed reachable on the physical interface)" - if len(c.Details) != 1 || c.Details[0] != want { + if len(c.Details) != 1 || c.Details[0].line() != want { t.Errorf("details = %v, want [%q]", c.Details, want) } if len(c.Fixes) != 0 { @@ -351,9 +354,9 @@ func TestBuildEndpointsCheck(t *testing.T) { if c.Status != checkFail { t.Errorf("status = %q, want %q", c.Status, checkFail) } - wantBad := "10.0.0.1 — MISCONFIGURED: inside utun4's subnet 10.0.0.0/24" + wantBad := "10.0.0.1 — MISCONFIGURED: inside its subnet 10.0.0.0/24 (utun4)" wantOK := "203.0.113.9 — ok (assumed reachable on the physical interface)" - if len(c.Details) != 2 || c.Details[0] != wantBad || c.Details[1] != wantOK { + if len(c.Details) != 2 || c.Details[0].line() != wantBad || c.Details[1].line() != wantOK { t.Errorf("details = %v, want [%q %q]", c.Details, wantBad, wantOK) } wantFix := "the endpoint marked MISCONFIGURED above is a tunnel-internal address; set\n" + @@ -397,7 +400,7 @@ func TestBuildLockoutCheck(t *testing.T) { if c.Summary != "dezhban will refuse to start" { t.Errorf("summary = %q", c.Summary) } - if len(c.Details) == 0 || c.Details[0] != "The VPN guard is on and utun4 is up, but no server address is known." { + if len(c.Details) == 0 || c.Details[0].line() != "utun4 is up and the VPN guard is on, but no server address is known." { t.Errorf("details[0] = %v", c.Details) } if len(c.Fixes) != 3 { @@ -468,10 +471,16 @@ func TestPrintDoctorMatchesKnownLayout(t *testing.T) { OK: false, Checks: []doctorCheck{ {Name: "config", Status: checkOK, Summary: "OK (loaded and validated)"}, - {Name: "tunnels", Status: checkOK, Details: []string{"utun4 — 10.0.0.0/24"}}, + {Name: "tunnels", Status: checkOK, Details: []doctorDetail{ + {Iface: "utun4", Text: "— 10.0.0.0/24"}, + }}, { Name: "endpoints", Status: checkFail, - Details: []string{"10.0.0.1 — MISCONFIGURED: inside utun4's subnet 10.0.0.0/24"}, + Details: []doctorDetail{{ + Endpoint: "10.0.0.1", + Iface: "utun4", + Text: "— MISCONFIGURED: inside its subnet 10.0.0.0/24", + }}, Fixes: []string{ "10.0.0.1 is a tunnel-internal address (inside utun4 10.0.0.0/24); set vpn.endpoints to\n" + " your VPN server's PUBLIC IP from your VPN client config.", @@ -487,7 +496,7 @@ func TestPrintDoctorMatchesKnownLayout(t *testing.T) { " utun4 — 10.0.0.0/24\n" + "\n" + "endpoints (resolved: literals + hostnames + discovery):\n" + - " 10.0.0.1 — MISCONFIGURED: inside utun4's subnet 10.0.0.0/24\n" + + " 10.0.0.1 — MISCONFIGURED: inside its subnet 10.0.0.0/24 (utun4)\n" + "\n" + "fixes:\n" + " - 10.0.0.1 is a tunnel-internal address (inside utun4 10.0.0.0/24); set vpn.endpoints to\n" + @@ -510,19 +519,24 @@ func TestPrintDoctorLayoutForLockoutTouchIDAndDiscover(t *testing.T) { OK: false, Checks: []doctorCheck{ {Name: "config", Status: checkOK, Summary: "OK (loaded and validated)"}, - {Name: "tunnels", Status: checkOK, Details: []string{"utun4 — 10.0.0.0/24"}}, - {Name: "endpoints", Status: checkWarn, Details: []string{"(none resolved)"}}, + {Name: "tunnels", Status: checkOK, Details: []doctorDetail{ + {Iface: "utun4", Text: "— 10.0.0.0/24"}, + }}, + {Name: "endpoints", Status: checkWarn, Details: details("(none resolved)")}, buildLockoutCheck([]string{"utun4"}), { Name: "touchID", Status: checkWarn, Summary: "not configured for sudo — privileged ops will ask for a password.", - Details: []string{"To authenticate with a fingerprint instead (survives OS updates):"}, + Details: details("To authenticate with a fingerprint instead (survives OS updates):"), Fixes: []string{"echo 'auth sufficient pam_tid.so' | sudo tee /etc/pam.d/sudo_local"}, }, { Name: "discover", Status: checkOK, - Details: []string{"198.51.100.7:51820 [wg0] <- not in vpn.endpoints"}, - Fixes: []string{"add any missing server IP to vpn.endpoints and drop stale entries."}, + ConnectedVPN: "wg0", + Details: []doctorDetail{ + {Endpoint: "198.51.100.7", Text: ":51820 <- not in vpn.endpoints"}, + }, + Fixes: []string{"add any missing server IP to vpn.endpoints and drop stale entries."}, }, }, } @@ -537,7 +551,7 @@ func TestPrintDoctorLayoutForLockoutTouchIDAndDiscover(t *testing.T) { " (none resolved)\n" + "\n" + "LOCKOUT RISK — dezhban will refuse to start:\n" + - " The VPN guard is on and utun4 is up, but no server address is known.\n" + + " utun4 is up and the VPN guard is on, but no server address is known.\n" + " The guard would block the tunnel's own transport and cut ALL traffic.\n" + "\n" + " Auto-discovery reads CONNECTED sockets. WireGuard (and other\n" + @@ -555,7 +569,8 @@ func TestPrintDoctorLayoutForLockoutTouchIDAndDiscover(t *testing.T) { " echo 'auth sufficient pam_tid.so' | sudo tee /etc/pam.d/sudo_local\n" + "\n" + "discover (best-effort, macOS):\n" + - " 198.51.100.7:51820 [wg0] <- not in vpn.endpoints\n" + + " via wg0\n" + + " 198.51.100.7:51820 <- not in vpn.endpoints\n" + " add any missing server IP to vpn.endpoints and drop stale entries.\n" got := captureStdout(t, func() { printDoctor(r) }) if got != want { @@ -600,8 +615,8 @@ func TestPrintDoctorDropsNoCheck(t *testing.T) { Checks: []doctorCheck{ {Name: "config", Status: checkOK, Summary: "first"}, {Name: "config", Status: checkFail, Summary: "second, same name"}, - {Name: "tunnels", Status: checkOK, Details: []string{"utun4"}}, - {Name: "endpoints", Status: checkOK, Details: []string{"1.2.3.4"}}, + {Name: "tunnels", Status: checkOK, Details: []doctorDetail{{Iface: "utun4"}}}, + {Name: "endpoints", Status: checkOK, Details: []doctorDetail{{Endpoint: "1.2.3.4"}}}, {Name: "brandNew", Status: checkWarn, Summary: "a check with no section yet", Fixes: []string{"do the thing"}}, }, @@ -638,3 +653,108 @@ func TestDoctorChecksHaveUniqueNames(t *testing.T) { seen[c.Name] = true } } + +// A finding's identifier is a FIELD, not a word inside its prose. That is what +// lets the diagnostic bundle's redactor see it by key (internal/redact's value +// switch) instead of hunting for it in free text — the pass that has produced a +// defect in every round it has been touched in. +func TestADetailLineCarriesItsIdentifierAsAField(t *testing.T) { + t.Run("tunnels", func(t *testing.T) { + nets := []netdetect.TunnelNet{{Iface: "nordlynx", Subnet: netip.MustParsePrefix("10.0.0.0/24")}} + c := buildTunnelsCheck([]string{"nordlynx"}, nets) + if len(c.Details) != 1 || c.Details[0].Iface != "nordlynx" { + t.Fatalf("details = %+v, want the interface carried in Iface", c.Details) + } + if strings.Contains(c.Details[0].Text, "nordlynx") { + t.Errorf("text %q still names the interface", c.Details[0].Text) + } + }) + + t.Run("endpoints", func(t *testing.T) { + bad := netip.MustParseAddr("10.0.0.1") + route := netdetect.EndpointRoute{Endpoint: bad, Iface: "nordlynx", Subnet: netip.MustParsePrefix("10.0.0.0/24")} + c := buildEndpointsCheck([]netip.Addr{bad}, []netdetect.EndpointRoute{route}) + if len(c.Details) != 1 { + t.Fatalf("details = %+v, want one", c.Details) + } + d := c.Details[0] + if d.Endpoint != "10.0.0.1" || d.Iface != "nordlynx" { + t.Errorf("details[0] = %+v, want both identifiers carried as fields", d) + } + if strings.Contains(d.Text, "nordlynx") || strings.Contains(d.Text, "10.0.0.1") { + t.Errorf("text %q still names an identifier", d.Text) + } + }) + + t.Run("lockout", func(t *testing.T) { + c := buildLockoutCheck([]string{"utun4", "nordlynx"}) + var carried []string + for _, d := range c.Details { + if d.Iface != "" { + carried = append(carried, d.Iface) + } + if strings.Contains(d.Text, "nordlynx") { + t.Errorf("text %q still names the interface", d.Text) + } + } + // One line per interface, each naming exactly one: a comma-joined list + // in a single field mints ONE token for the pair and stops keepIface + // recognising the generic half as the kernel's vocabulary. + want := []string{"utun4", "nordlynx"} + if !slices.Equal(carried, want) { + t.Errorf("carried %v, want %v", carried, want) + } + }) +} + +// encoding/json emits fields in declaration order and the redactor's walk visits +// an object's keys in document order, so an identifier declared after Text would +// be minted only after the prose that might repeat it had already been walked. +func TestAnIdentifierFieldIsMarshalledBeforeItsText(t *testing.T) { + b, err := json.Marshal(doctorDetail{Iface: "a", Profile: "b", Endpoint: "c", Text: "d"}) + if err != nil { + t.Fatal(err) + } + text := bytes.Index(b, []byte(`"text"`)) + for _, key := range []string{`"iface"`, `"profile"`, `"endpoint"`} { + if i := bytes.Index(b, []byte(key)); i < 0 || i > text { + t.Errorf("%s is marshalled at %d, after %q at %d: %s", key, i, "text", text, b) + } + } + + c, err := json.Marshal(doctorCheck{Profiles: []string{"p"}, ConnectedVPN: "v", Summary: "s"}) + if err != nil { + t.Fatal(err) + } + summary := bytes.Index(c, []byte(`"summary"`)) + for _, key := range []string{`"profiles"`, `"connectedVPN"`} { + if i := bytes.Index(c, []byte(key)); i < 0 || i > summary { + t.Errorf("%s is marshalled after summary: %s", key, c) + } + } +} + +// The VPN's friendly service name is read once per scan and is the same for +// every candidate, so repeating it as a suffix on each line only put a provider +// name where no key could see it. It belongs to the check. +func TestTheDiscoverCheckDoesNotNameTheVPNClientInAnyDetailLine(t *testing.T) { + cands := []netdetect.Candidate{ + {VPN: "Mullvad VPN", Server: netip.MustParseAddr("198.51.100.7"), Port: 51820}, + {VPN: "Mullvad VPN", Server: netip.MustParseAddr("198.51.100.8"), Port: 51820}, + } + c := buildDiscoverCheck(cands, nil, nil) + if c.ConnectedVPN != "Mullvad VPN" { + t.Errorf("connectedVPN = %q, want the service name carried as a field", c.ConnectedVPN) + } + for _, d := range c.Details { + if strings.Contains(d.Text, "Mullvad") { + t.Errorf("detail text %q names the VPN client", d.Text) + } + if d.Endpoint == "" { + t.Errorf("detail %+v does not carry its server address as a field", d) + } + } + if len(c.Details) != 2 { + t.Errorf("details = %+v, want one per candidate", c.Details) + } +} diff --git a/cmd/dezhban/doctor_unattended_test.go b/cmd/dezhban/doctor_unattended_test.go index 5ac0aa0..851d294 100644 --- a/cmd/dezhban/doctor_unattended_test.go +++ b/cmd/dezhban/doctor_unattended_test.go @@ -25,7 +25,16 @@ func hasFix(c doctorCheck, substr string) bool { return slices.ContainsFunc(c.Fixes, func(f string) bool { return strings.Contains(f, substr) }) } -func detailText(c doctorCheck) string { return strings.Join(c.Details, "\n") } +func detailText(c doctorCheck) string { return strings.Join(detailLines(c), "\n") } + +// detailLines composes each finding the way both renderers do. +func detailLines(c doctorCheck) []string { + out := make([]string, 0, len(c.Details)) + for _, d := range c.Details { + out = append(out, d.line()) + } + return out +} func TestBuildServiceCheck(t *testing.T) { const path = "/Library/LaunchDaemons/dezhban.plist" @@ -280,3 +289,37 @@ func TestEveryCheckHasASection(t *testing.T) { } } } + +// The retention check names EVERY entry — one detail line each — so Profiles has +// to carry every one of them, not only the stale or rotating ones the Summary +// happens to mention. The carrier is what makes the coverage stop depending on +// config.json and learned.json being collected before doctor.json. +func TestTheRetentionCheckCarriesEveryEntryName(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + const ttl = 24 * time.Hour + store := &learned.Store{Entries: []learned.Entry{ + {Name: "work-nord", Endpoints: []learned.Endpoint{ + {Addr: "203.0.113.7", FirstSeen: now.Add(-time.Hour), LastSeen: now.Add(-time.Hour)}, + }}, + {Name: "home-wg", Endpoints: []learned.Endpoint{ + {Addr: "203.0.113.8", FirstSeen: now.Add(-90 * time.Hour), LastSeen: now.Add(-90 * time.Hour)}, + }}, + }} + c := buildEndpointRetentionCheck(store, nil, ttl, 8, 0, now) + + want := []string{"work-nord", "home-wg"} + if !slices.Equal(c.Profiles, want) { + t.Errorf("profiles = %v, want %v — every entry, not just the ones the summary names", c.Profiles, want) + } + for _, d := range c.Details { + for _, name := range want { + if strings.Contains(d.Text, name) { + t.Errorf("detail text %q names an entry that should be carried as a field", d.Text) + } + } + } + // The per-entry line still READS as one, with the name composed back in. + if got := detailText(c); !strings.Contains(got, "work-nord — 1 stored") { + t.Errorf("composed details = %q, want the entry named in its own line", got) + } +} diff --git a/cmd/dezhban/main.go b/cmd/dezhban/main.go index 75f3be0..fcb5209 100644 --- a/cmd/dezhban/main.go +++ b/cmd/dezhban/main.go @@ -2153,21 +2153,97 @@ const ( checkFail checkStatus = "fail" ) +// doctorDetail is one of a check's findings: the identifier it is ABOUT, carried +// as data, plus the prose that qualifies it. Both renderers compose the line — +// printDoctor for the terminal, the GUI's checkRow for Diagnostics — and neither +// has to reparse the other's sentence. +// +// The identifier is a field rather than a word inside Text because the diagnostic +// bundle's redactor (internal/redact) works by KEY: a value under `iface`, +// `profile` or `endpoint` is recognised for what it is and replaced with a stable +// token, while the same word inside prose is reachable only by a literal +// word-replacement pass over free text — the pass that has produced a defect in +// every round it has been touched in. These key names are exactly the ones that +// redactor's value switch already knows; do not invent new spellings for them. +// +// DECLARATION ORDER IS LOAD-BEARING. encoding/json emits fields in declaration +// order and the redactor's walk visits an object's keys in document order, so an +// identifier declared after Text would be minted only after the prose that might +// repeat it had already been walked. +// +// An empty Text with no identifier is a PARAGRAPH BREAK, not a finding — the only +// piece of layout this contract carries, and it is here because both renderers +// need it: printDoctor emits a blank line, and the GUI's checkRow emits vertical +// space. A renderer that treats it as an ordinary line gets a stray empty row. +// Anything more elaborate than a break belongs in Fixes. +type doctorDetail struct { + Iface string `json:"iface,omitempty"` + Profile string `json:"profile,omitempty"` + Endpoint string `json:"endpoint,omitempty"` + Text string `json:"text"` +} + +// line is the composed human form: the finding's subject, its prose, and — when +// a finding is about an endpoint AND the interface it is misrouted onto — the +// interface in parentheses after it. Both renderers use this shape, so the CLI +// and the GUI cannot disagree about what a check found. +func (d doctorDetail) line() string { + subject := d.Endpoint + if subject == "" { + subject = d.Profile + } + if subject == "" { + subject = d.Iface + } + var b strings.Builder + b.WriteString(subject) + switch { + case d.Text == "": + case subject == "": + b.WriteString(d.Text) + case strings.HasPrefix(d.Text, ":"): + // A port joins its address with no separator: `1.2.3.4:51820`. + b.WriteString(d.Text) + default: + b.WriteString(" " + d.Text) + } + if d.Iface != "" && d.Iface != subject { + b.WriteString(" (" + d.Iface + ")") + } + return b.String() +} + +// detail is a finding with no identifier of its own — prose, or the empty +// paragraph break. +func detail(text string) doctorDetail { return doctorDetail{Text: text} } + +// details converts a run of identifier-free lines. +func details(texts ...string) []doctorDetail { + out := make([]doctorDetail, 0, len(texts)) + for _, t := range texts { + out = append(out, detail(t)) + } + return out +} + // doctorCheck is one section of `doctor`'s output, structured. Details/Fixes -// are the exact lines printDoctor prints under the check's header — kept as -// data so a second renderer (the GUI's Diagnostics pane, over --json) never -// has to reparse human prose to find out what's wrong. +// are the lines printDoctor prints under the check's header — kept as data so a +// second renderer (the GUI's Diagnostics pane, over --json) never has to reparse +// human prose to find out what's wrong. type doctorCheck struct { - Name string `json:"name"` - Status checkStatus `json:"status"` - Summary string `json:"summary"` - // Details are the check's findings, one line each. An EMPTY string is a - // paragraph break, not a finding — the only piece of layout this contract - // carries, and it is here because both renderers need it: printDoctor emits - // a blank line, and the GUI's checkRow emits vertical space. A renderer that - // treats it as an ordinary line gets a stray empty row, so new consumers - // must handle it. Anything more elaborate than a break belongs in Fixes. - Details []string `json:"details,omitempty"` + Name string `json:"name"` + Status checkStatus `json:"status"` + + // Identifier carriers for the prose in Summary, which is one sentence and + // stays one sentence. Declared above it for the ordering reason given on + // doctorDetail. Profiles reuses the redactor's `profile` key; ConnectedVPN is + // the VPN's friendly service name, which names the provider as plainly as a + // server address does. + Profiles []string `json:"profiles,omitempty"` + ConnectedVPN string `json:"connectedVPN,omitempty"` + + Summary string `json:"summary"` + Details []doctorDetail `json:"details,omitempty"` // Fixes are the commands or actions that resolve the check, never prose // about them — the GUI badges each one, so a sentence dressed as a fix // reads as a command the user should run. @@ -2190,7 +2266,7 @@ func buildTunnelsCheck(tunnels []string, nets []netdetect.TunnelNet) doctorCheck c := doctorCheck{Name: "tunnels", Status: checkOK} if len(tunnels) == 0 { c.Status = checkWarn - c.Details = []string{"(none — set vpn.tunnelInterfaces or vpn.autoDetect)"} + c.Details = details("(none — set vpn.tunnelInterfaces or vpn.autoDetect)") return c } subsByIface := map[string][]string{} @@ -2199,9 +2275,9 @@ func buildTunnelsCheck(tunnels []string, nets []netdetect.TunnelNet) doctorCheck } for _, t := range tunnels { if subs := subsByIface[t]; len(subs) > 0 { - c.Details = append(c.Details, fmt.Sprintf("%s — %s", t, strings.Join(subs, ", "))) + c.Details = append(c.Details, doctorDetail{Iface: t, Text: "— " + strings.Join(subs, ", ")}) } else { - c.Details = append(c.Details, fmt.Sprintf("%s — no subnet (interface down or absent?)", t)) + c.Details = append(c.Details, doctorDetail{Iface: t, Text: "— no subnet (interface down or absent?)"}) } } return c @@ -2216,7 +2292,7 @@ func buildEndpointsCheck(endpoints []netip.Addr, bad []netdetect.EndpointRoute) c := doctorCheck{Name: "endpoints", Status: checkOK} if len(endpoints) == 0 { c.Status = checkWarn - c.Details = []string{"(none resolved)"} + c.Details = details("(none resolved)") return c } internal := map[string]netdetect.EndpointRoute{} @@ -2225,9 +2301,16 @@ func buildEndpointsCheck(endpoints []netip.Addr, bad []netdetect.EndpointRoute) } for _, ep := range endpoints { if b, ok := internal[ep.String()]; ok { - c.Details = append(c.Details, fmt.Sprintf("%s — MISCONFIGURED: inside %s's subnet %s", ep, b.Iface, b.Subnet)) + c.Details = append(c.Details, doctorDetail{ + Endpoint: ep.String(), + Iface: b.Iface, + Text: fmt.Sprintf("— MISCONFIGURED: inside its subnet %s", b.Subnet), + }) } else { - c.Details = append(c.Details, fmt.Sprintf("%s — ok (assumed reachable on the physical interface)", ep)) + c.Details = append(c.Details, doctorDetail{ + Endpoint: ep.String(), + Text: "— ok (assumed reachable on the physical interface)", + }) } } if len(bad) > 0 { @@ -2260,25 +2343,35 @@ func plural(n int, one, many string) string { // buildLockoutCheck formats the "guard would block its own tunnel's transport" // warning. Pure — the caller decides whether the lockout condition holds. func buildLockoutCheck(tunnels []string) doctorCheck { - return doctorCheck{ + c := doctorCheck{ Name: "lockout", Status: checkFail, Summary: "dezhban will refuse to start", - Details: []string{ - fmt.Sprintf("The VPN guard is on and %s is up, but no server address is known.", strings.Join(tunnels, ", ")), - "The guard would block the tunnel's own transport and cut ALL traffic.", - "", - "Auto-discovery reads CONNECTED sockets. WireGuard (and other", - "NetworkExtension clients) send from an UNCONNECTED UDP socket, so they", - "never appear as a connected flow — discovery cannot find them. Name the", - "server explicitly:", - }, - Fixes: []string{ - "dezhban vpn import # reads the endpoint from it", - "dezhban vpn add --endpoint ", - "sudo dezhban config set vpn.endpoints=", - }, } + // One line per interface, each naming exactly one, so the redactor sees each + // name under its own key. A comma-joined list in a single field mints one + // token for the lot and stops keepIface recognising the generic ones as the + // kernel's vocabulary. + for _, t := range tunnels { + c.Details = append(c.Details, doctorDetail{ + Iface: t, + Text: "is up and the VPN guard is on, but no server address is known.", + }) + } + c.Details = append(c.Details, details( + "The guard would block the tunnel's own transport and cut ALL traffic.", + "", + "Auto-discovery reads CONNECTED sockets. WireGuard (and other", + "NetworkExtension clients) send from an UNCONNECTED UDP socket, so they", + "never appear as a connected flow — discovery cannot find them. Name the", + "server explicitly:", + )...) + c.Fixes = []string{ + "dezhban vpn import # reads the endpoint from it", + "dezhban vpn add --endpoint ", + "sudo dezhban config set vpn.endpoints=", + } + return c } // buildServiceCheck answers "will dezhban be there after I reboot". Pure — the @@ -2302,7 +2395,7 @@ func buildServiceCheck(unit svc.BootUnit, daemonLive bool) doctorCheck { // may exist but could not be read. Guessing between them is how a // correctly-installed user gets told to reinstall. c.Summary = "cannot tell without asking the service manager." - c.Details = []string{"Nothing readable here says what happens at boot. Ask it directly:"} + c.Details = details("Nothing readable here says what happens at boot. Ask it directly:") c.Fixes = []string{"dezhban status"} return c } @@ -2311,34 +2404,34 @@ func buildServiceCheck(unit svc.BootUnit, daemonLive bool) doctorCheck { case !unit.Present: c.Status = checkWarn c.Summary = "not registered to start at boot." - c.Details = []string{ + c.Details = details( fmt.Sprintf("No service unit at %s, so nothing", unit.Path), "arms the guard after a reboot until you start dezhban by hand.", - } + ) if daemonLive { - c.Details = append(c.Details, + c.Details = append(c.Details, details( "", "dezhban IS enforcing right now — this is about reboots, not about", - "the guard being off today.") + "the guard being off today.")...) } c.Fixes = []string{"sudo dezhban install"} case !unit.AtBoot: c.Status = checkWarn c.Summary = "installed, but not set to start at boot." - c.Details = []string{ + c.Details = details( fmt.Sprintf("%s exists but does not ask", unit.Path), "the service manager to start dezhban at boot, so `start` works and", "every reboot comes up unguarded. Reinstalling rewrites the unit:", - } + ) c.Fixes = []string{"sudo dezhban install"} case !daemonLive: c.Status = checkWarn c.Summary = "set to start at boot, but nothing is enforcing right now." - c.Details = []string{ + c.Details = details( "The next reboot will arm the guard. Until then this host is unguarded.", - } + ) c.Fixes = []string{"sudo dezhban start"} default: @@ -2346,10 +2439,10 @@ func buildServiceCheck(unit svc.BootUnit, daemonLive bool) doctorCheck { // The point of saying this out loud: it rules out the enforcement // explanation for "I have to turn it on after every reboot" and leaves // only the presentation one, which has an entirely different fix. - c.Details = []string{ + c.Details = details( "If the menubar app is missing after a login, that is a login-item", "question — the guard is already up without it.", - } + ) } return c } @@ -2397,21 +2490,21 @@ func buildControlCheck(cfg *config.Config, resp control.Response, probeErr error case errors.Is(probeErr, control.ErrForbidden): c.Status = checkWarn c.Summary = fmt.Sprintf("reachable (%s), but you are not in the %q group — routine ops need sudo.", path, cfg.Control.Group) - c.Details = []string{ + c.Details = details( fmt.Sprintf("Add your account to %q the normal way for this OS, then log out and back in — group membership is read at login, not live.", cfg.Control.Group), seeDoc, - } + ) case probeErr != nil || !resp.OK: c.Status = checkWarn c.Summary = fmt.Sprintf("unreachable (%s) — dezhban is not running; routine ops need sudo.", path) if cfg.Control.Group == "" { - c.Details = []string{"no group is configured either — once running, an unprivileged caller would still need sudo.", seeDoc} + c.Details = details("no group is configured either — once running, an unprivileged caller would still need sudo.", seeDoc) c.Fixes = []string{setGroupFix()} } case cfg.Control.Group == "": c.Status = checkWarn c.Summary = fmt.Sprintf("reachable (%s), but no group is configured — routine ops need sudo.", path) - c.Details = []string{seeDoc} + c.Details = details(seeDoc) c.Fixes = []string{setGroupFix()} default: c.Summary = fmt.Sprintf("reachable (%s, group %q) — routine ops need no password.", path, cfg.Control.Group) @@ -2429,10 +2522,10 @@ func buildControlCheck(cfg *config.Config, resp control.Response, probeErr error } if len(gated) > 0 { if len(c.Details) > 0 { - c.Details = append(c.Details, "") + c.Details = append(c.Details, details("")...) } - c.Details = append(c.Details, "Forced back to sudo regardless of group membership:") - c.Details = append(c.Details, gated...) + c.Details = append(c.Details, details("Forced back to sudo regardless of group membership:")...) + c.Details = append(c.Details, details(gated...)...) } return c } @@ -2456,25 +2549,25 @@ func buildArmAtBootCheck(armAtBoot bool, haveTunnel bool, rec *armed.Record, loa if loadErr != nil { c.Status = checkWarn c.Summary = "the arm-at-boot record could not be read; boot will fall back to standby." - c.Details = []string{ + c.Details = details( loadErr.Error(), "", "dezhban treats an unreadable record as \"no tunnel has ever been up\",", "which is safe but means the next reboot waits for a live tunnel instead", "of arming straight away. dezhban rewrites it the next time a tunnel", "comes up.", - } + ) return c } if !armAtBoot { c.Status = checkWarn c.Summary = "off — after a reboot the guard waits for a live tunnel before arming." - c.Details = []string{ + c.Details = details( "That leaves a gap between boot and the VPN connecting, during which", "traffic uses your real address. Turning it on closes the gap on a host", "whose VPN has already worked once.", - } + ) c.Fixes = []string{"sudo dezhban config set vpn.armAtBoot=true"} return c } @@ -2482,28 +2575,28 @@ func buildArmAtBootCheck(armAtBoot bool, haveTunnel bool, rec *armed.Record, loa if !rec.TunnelEverUp { c.Status = checkWarn c.Summary = "on, but no tunnel has been observed up yet, so it cannot arm." - c.Details = []string{ + c.Details = details( fmt.Sprintf("The record at %s has not seen a tunnel come up on this host.", path), "Arm-at-boot needs that observation — arming without it would fail closed", "on a machine that has never had a working VPN, which is a lockout by", "design rather than a guard.", "", - } + ) if haveTunnel { - c.Details = append(c.Details, - "Connect your VPN once with dezhban running and this becomes permanent.") + c.Details = append(c.Details, details( + "Connect your VPN once with dezhban running and this becomes permanent.")...) } else { - c.Details = append(c.Details, - "Configure a tunnel first, then connect it once with dezhban running.") + c.Details = append(c.Details, details( + "Configure a tunnel first, then connect it once with dezhban running.")...) } return c } c.Summary = "on — the next reboot arms the guard without waiting for a tunnel." - c.Details = []string{ + c.Details = details( fmt.Sprintf("A tunnel was first seen up %s and last seen %s.", rec.FirstUp.Local().Format(time.RFC1123), rec.LastUp.Local().Format(time.RFC1123)), - } + ) return c } @@ -2586,7 +2679,7 @@ func buildLivenessCheck(snap state.Snapshot, daemonLive bool) doctorCheck { "country would show here even though the blocked-country check saw no change).", snap.ExitIPChangedAt.Local().Format(time.RFC1123))) } - c.Details = lines + c.Details = details(lines...) return c } @@ -2607,7 +2700,7 @@ func buildEndpointRetentionCheck(store *learned.Store, loadErr error, ttl time.D if loadErr != nil { c.Status = checkWarn c.Summary = "the learned-endpoint store could not be read; every drop starts from nothing." - c.Details = []string{loadErr.Error()} + c.Details = details(loadErr.Error()) return c } @@ -2622,10 +2715,10 @@ func buildEndpointRetentionCheck(store *learned.Store, loadErr error, ttl time.D } c.Status = checkWarn c.Summary = "nothing learned, and no server address configured either." - c.Details = []string{ + c.Details = details( "A drop has no known address to redial through, so it needs a window", "every time. Naming the server once removes the interaction entirely.", - } + ) c.Fixes = []string{"dezhban vpn add --endpoint "} return c } @@ -2646,8 +2739,11 @@ func buildEndpointRetentionCheck(store *learned.Store, loadErr error, ttl time.D recentlyNew++ } } - c.Details = append(c.Details, fmt.Sprintf("%s — %d stored, %d within the %s retention window", - e.Name, len(e.Endpoints), fresh, ttl)) + c.Profiles = append(c.Profiles, e.Name) + c.Details = append(c.Details, doctorDetail{ + Profile: e.Name, + Text: fmt.Sprintf("— %d stored, %d within the %s retention window", len(e.Endpoints), fresh, ttl), + }) switch { case fresh == 0: @@ -2661,21 +2757,21 @@ func buildEndpointRetentionCheck(store *learned.Store, loadErr error, ttl time.D case len(staleOnly) > 0: c.Status = checkWarn c.Summary = fmt.Sprintf("every learned address for %s has aged out.", strings.Join(staleOnly, ", ")) - c.Details = append(c.Details, "", + c.Details = append(c.Details, details("", "They were learned and then discarded, so the next drop redials with", "nothing known and needs a window. Retaining them for longer removes", - "that interaction.") + "that interaction.")...) c.Fixes = []string{"sudo dezhban config set vpn.advanced.learnedEndpointTTL=720h"} case len(rotating) > 0: c.Status = checkWarn c.Summary = fmt.Sprintf("%s looks like it rotates its server address.", strings.Join(rotating, ", ")) - c.Details = append(c.Details, "", + c.Details = append(c.Details, details("", "The store is full and most of what is in it was seen for the first time", "recently, which means the address is rarely the same twice. Retaining", "more of them only delays the problem — a hostname is the real fix,", "because dezhban re-resolves it on vpn.endpointRefresh and follows the", - "rotation instead of chasing it.") + "rotation instead of chasing it.")...) c.Fixes = []string{ "dezhban vpn add --endpoint ", "sudo dezhban config set vpn.advanced.learnedMaxPerProfile=32", @@ -2770,7 +2866,7 @@ func runDoctor(cfg *config.Config, log *slog.Logger, discover bool) doctorReport Name: "touchID", Status: checkWarn, Summary: "not configured for sudo — privileged ops will ask for a password.", - Details: []string{"To authenticate with a fingerprint instead (survives OS updates):"}, + Details: details("To authenticate with a fingerprint instead (survives OS updates):"), // The command is a Fix, not a Detail: it is the thing to run, so it // belongs where every other runnable line lives (and where the GUI // badges it) rather than as a detail line the CLI had to indent @@ -2780,36 +2876,8 @@ func runDoctor(cfg *config.Config, log *slog.Logger, discover bool) doctorReport } if discover { - discoverCheck := doctorCheck{Name: "discover", Status: checkOK} cands, err := netdetect.DiscoverEndpoints() - switch { - // Summary only, never also as a Detail: the GUI renders Summary in the - // row's title and Details beneath it, so setting both printed the same - // sentence twice. - case err != nil: - discoverCheck.Status = checkWarn - discoverCheck.Summary = err.Error() - case len(cands) == 0: - discoverCheck.Status = checkWarn - discoverCheck.Summary = "no physical-side public transport sockets found — is the VPN connected?" - default: - configured := map[string]bool{} - for _, ep := range endpoints { - configured[ep.String()] = true - } - for _, c := range cands { - line := fmt.Sprintf("%s:%d", c.Server, c.Port) - if c.VPN != "" { - line += " [" + c.VPN + "]" - } - if !configured[c.Server.String()] { - line += " <- not in vpn.endpoints" - } - discoverCheck.Details = append(discoverCheck.Details, line) - } - discoverCheck.Fixes = []string{"add any missing server IP to vpn.endpoints and drop stale entries."} - } - checks = append(checks, discoverCheck) + checks = append(checks, buildDiscoverCheck(cands, err, endpoints)) } // A diagnostic that reports a guaranteed blackout and still exits 0 is one @@ -2819,6 +2887,48 @@ func runDoctor(cfg *config.Config, log *slog.Logger, discover bool) doctorReport return doctorReport{Checks: checks, OK: !(lockout || len(bad) > 0)} } +// buildDiscoverCheck formats the discovered transport sockets (already looked up +// by the caller — this function does no I/O) into a doctorCheck. Pure, so the +// no-candidates, error and listing branches are directly testable without a live +// VPN, matching every other builder above. +func buildDiscoverCheck(cands []netdetect.Candidate, err error, endpoints []netip.Addr) doctorCheck { + c := doctorCheck{Name: "discover", Status: checkOK} + switch { + // Summary only, never also as a Detail: the GUI renders Summary in the + // row's title and Details beneath it, so setting both printed the same + // sentence twice. + case err != nil: + c.Status = checkWarn + c.Summary = err.Error() + case len(cands) == 0: + c.Status = checkWarn + c.Summary = "no physical-side public transport sockets found — is the VPN connected?" + default: + // The VPN's friendly service name is the same for every candidate — it is + // read once per scan, not per socket — so it belongs to the CHECK, not to + // each line. Carrying it as a field rather than interpolating it into the + // prose is what lets the bundle's redactor see it by key; repeated inside + // a detail line, it is a bare word no shape can find. + c.ConnectedVPN = cands[0].VPN + configured := map[string]bool{} + for _, ep := range endpoints { + configured[ep.String()] = true + } + for _, cand := range cands { + d := doctorDetail{ + Endpoint: cand.Server.String(), + Text: fmt.Sprintf(":%d", cand.Port), + } + if !configured[cand.Server.String()] { + d.Text += " <- not in vpn.endpoints" + } + c.Details = append(c.Details, d) + } + c.Fixes = []string{"add any missing server IP to vpn.endpoints and drop stale entries."} + } + return c +} + // unattendedSections are the checks that answer "will dezhban need me again" — // after a reboot, or after the next drop. Grouped because they read as one // question and print as one block. @@ -2957,6 +3067,12 @@ func printDoctor(r doctorReport) { if len(discover.Details) == 0 && discover.Summary != "" { fmt.Printf(" %s\n", discover.Summary) } + // Once, above the sockets, rather than repeated as a suffix on each one: + // it is read once per scan and is the same for every candidate. Saying it + // once is also what lets it be a field the bundle's redactor can see. + if discover.ConnectedVPN != "" { + fmt.Printf(" via %s\n", discover.ConnectedVPN) + } printDetails(discover.Details) for _, f := range discover.Fixes { fmt.Printf(" %s\n", f) @@ -2982,9 +3098,11 @@ func printDoctor(r doctorReport) { } // printDetails prints a check's Details at the standard two-space indent, -// honouring the empty-string paragraph break (see doctorCheck.Details). -func printDetails(details []string) { - for _, line := range details { +// composing each finding's identifier with its prose (doctorDetail.line) and +// honouring the empty paragraph break (see doctorDetail). +func printDetails(found []doctorDetail) { + for _, d := range found { + line := d.line() if line == "" { fmt.Println() continue diff --git a/cmd/dezhban/report_test.go b/cmd/dezhban/report_test.go index 72e62f7..2c7084c 100644 --- a/cmd/dezhban/report_test.go +++ b/cmd/dezhban/report_test.go @@ -93,16 +93,26 @@ func TestTheBundleIsRedactedAndPrivateEndToEnd(t *testing.T) { // A single-label endpoint: valid by config's grammar, invisible to a // hostname SHAPE, and so the case only the field-aware pass reaches. const bareHost = "acmevpn" + // A provider-created interface. It reaches the bundle as a BARE WORD in a + // rendered pf/nft ruleset (`pass out quick on { … }`) and in doctor's prose — + // no key, no dot, no attr — which is the shape only the name replay catches. + const vendorIface = "nordlynx" + // A profile legitimately CALLED like a placeholder. Config accepts + // [A-Za-z0-9._-], and this is what made one token stand for two identities. + const tokenShaped = "profile-1" cfg := `{ "pollInterval": "5s", "blockedCountries": [], "hysteresis": 1, "providers": ["https://ipinfo.io/json"], "vpn": { - "tunnelInterfaces": ["utun9"], + "tunnelInterfaces": ["utun9", "` + vendorIface + `"], "endpoints": ["` + endpoint + `", "` + bareHost + `"], "autoDetect": false, - "profiles": [{"name": "` + profile + `", "endpoints": ["` + endpoint + `"]}] + "profiles": [ + {"name": "` + profile + `", "endpoints": ["` + endpoint + `"]}, + {"name": "` + tokenShaped + `", "endpoints": ["` + endpoint + `"]} + ] }, "providerQuorum": false, "logLevel": "error" @@ -155,7 +165,7 @@ func TestTheBundleIsRedactedAndPrivateEndToEnd(t *testing.T) { t.Fatalf("%s: %v", f.Name, err) } bodies[f.Name] = string(body) - for _, leak := range []string{endpoint, profile, bareHost} { + for _, leak := range []string{endpoint, profile, bareHost, vendorIface} { if strings.Contains(string(body), leak) { t.Errorf("%s leaked %q", f.Name, leak) } @@ -220,24 +230,46 @@ func TestTheBundleIsRedactedAndPrivateEndToEnd(t *testing.T) { // ordinals actually minted, and an ordinal is skipped when it would have // produced the value it replaces — so the first one is not always `-1`, and // an assertion that assumed it was tested only the first token of each kind. - legend := regexp.MustCompile(`(\d+) distinct [a-z ]+ → ([a-z]+-\d+)(?: … ([a-z]+-\d+))?`) + // Two forms now: a RANGE when the kind's tokens run consecutively, and a + // comma-separated LIST when one was refused and left a hole. The fixture + // carries a profile literally called `profile-1`, so the list form is + // exercised on every run rather than only on an unusual host. + legend := regexp.MustCompile(`(\d+) distinct [A-Za-z ]+ → ((?:[a-z]+-\d+)(?:(?: … |, )[a-z]+-\d+)*)`) rows := legend.FindAllStringSubmatch(bodies["README.txt"], -1) if len(rows) == 0 && strings.Contains(bodies["README.txt"], "What was replaced") { t.Error("the legend rendered rows this test cannot parse") } + tokenRe := regexp.MustCompile(`[a-z]+-\d+`) + claimed := map[string]string{} for _, m := range rows { count, err := strconv.Atoi(m[1]) if err != nil { t.Fatalf("legend count %q: %v", m[1], err) } - for _, token := range []string{m[2], m[3]} { - if token != "" && !strings.Contains(all, token) { + named := tokenRe.FindAllString(m[2], -1) + for _, token := range named { + if !strings.Contains(all, token) { t.Errorf("the legend names %s, which appears nowhere in the bundle", token) } + // One token, one identity. A refused ordinal used to be vacated and + // reissued, so two distinct originals could land on the same token — + // invisible from outside except that the legend then names it twice. + if prev, dup := claimed[token]; dup { + t.Errorf("%s is claimed by two legend rows (%q and %q)", token, prev, m[0]) + } + claimed[token] = m[0] } - if count == 1 && m[3] != "" { + if count == 1 && len(named) != 1 { t.Errorf("a count of one rendered as a range: %q", m[0]) } + // The list form spells every token out, so it must spell out as many as + // it counted. The range form names only its two ends. + if strings.Contains(m[2], ", ") && len(named) != count { + t.Errorf("legend row %q lists %d tokens for a count of %d", m[0], len(named), count) + } + if strings.Contains(m[2], ", ") && strings.Contains(m[2], " … ") { + t.Errorf("legend row %q mixes the range and list forms", m[0]) + } } } diff --git a/docs/adr/0016-bundle-identifiers-are-data-not-prose.md b/docs/adr/0016-bundle-identifiers-are-data-not-prose.md new file mode 100644 index 0000000..a136504 --- /dev/null +++ b/docs/adr/0016-bundle-identifiers-are-data-not-prose.md @@ -0,0 +1,109 @@ +# ADR-0016: A bundle identifier is redacted by the key that names it + +**Date**: 2026-09-13 +**Status**: accepted, implemented +**Deciders**: Behnam RK + +## Context + +`dezhban report` writes a diagnostic bundle meant to be pasted into a public +issue, so `internal/redact` must replace every identifier in it with a stable +placeholder. It has two ways to find one, and they are not peers. + +The **key-aware** pass parses a JSON document and rewrites a value because of +the key above it: a string under `endpoint`, `tunnelInterfaces` or +`activeProfile` is an identifier by definition, whatever it happens to spell. +The **literal-name replay** (`knownNames`) is the fallback: it takes the names +already replaced elsewhere and substitutes them wherever they appear as words in +free text. It is what reaches a name no shape and no key can see — a +single-label host inside a resolver's error, an interface name in a rendered pf +rule. + +The replay is also where every defect in this package has come from. Across the +review loop on #55 it claimed the posture and mode strings when a profile shared +their name, double-minted host-shaped names under two kinds, mangled doctor's +fix commands, and needed a reserved-word list to stop claiming dezhban's own +vocabulary. Issue #63 was left open for exactly that reason, with the note that +the cleaner fix was probably "at the source" — stop interpolating identifiers +into prose at all. + +Implementing it showed the source-side fix cannot stand alone. The entries that +actually leaked were `rules-preview.txt` and `log.txt`: a rendered pf/nft +ruleset and slog records. A pf rule carries an interface as `pass out quick on +{ nordlynx }`, because that is what a pf rule *is*, and an OS error string is +written by the OS. Neither can be restructured into fields by us. + +## Decision + +An identifier in a bundle is redacted **by the key that names it**. Where we +author the data — `doctor.json`, `state.json` — the identifier is carried as a +field under a key the redactor's value switch already knows, and the prose that +mentions it is composed by each renderer at display time. The literal-name +replay stays, is widened to cover interface names, and is the **fallback** for +text we do not author: rendered rulesets, log records, OS error strings. + +## Alternatives considered + +### Alternative 1: widen the replay and leave the prose alone + +- **Pros**: one small change; no struct, renderer or Swift changes; closes every + observed leak on its own. +- **Cons**: makes the pass with this package's worst defect history the primary + mechanism for the whole bundle, including entries whose prose we control and + could simply have kept the identifier out of. Every word dezhban writes becomes + a word the replay might claim, and the only defence is a hand-maintained + reserved list that has needed extending in every round it has existed. +- **Why not**: it is the shape that keeps failing, applied to more surface. + +### Alternative 2: remove identifiers from prose entirely, no replay + +- **Pros**: kills the class; nothing to replay because nothing repeats a name. +- **Why not**: impossible for the entries that actually leak. `rules-preview.txt` + is firewall syntax, `log.txt` is slog output, and `render.Display`'s detail + quotes `EnforcementErr` and `LookupErr` verbatim from pfctl, nft and the + resolver. Adopting this alone would have closed #63 in the two files that were + already covered and in neither of the two that were not. + +### Alternative 3: retype `doctorCheck.Details` as a template plus fields, with no composed line + +- **Pros**: the identifier is unambiguously data. +- **Why not**: `Details` is documented as the lines both renderers print, and the + macOS app decodes them. A renderer that cannot compose the sentence shows a + line with a hole. Keeping a `line()` on both sides — Go and Swift, mirrored — + gets the field without giving up the contract. + +## Consequences + +### Positive + +- An identifier we author is recognised for what it is, whatever it spells, and + cannot be missed because it lacked a dot or a boundary character. +- The replay's blast radius shrinks to text we do not write, where over-claiming + a word is noise rather than a rewritten diagnosis. +- One rule for a reader: if a value is an identity, it has a key. `doctor --json` + and `status --json` are more useful to the app for the same reason. + +### Negative + +- `doctor.json`'s `details` became objects, a coordinated Go and Swift change. + The two ship in one `.pkg`, so the skew window is the dev loop, not a release. +- Two places now compose the same sentence (`doctorDetail.line` in Go, + `DoctorDetail.line` in Swift). They must stay in step; both are tested against + the same expected string. + +### Risks + +- **The replay still has to be right for what it now covers.** Mitigated by + refusing any value that is spelled like a minted token (which is how it used to + launder its own output), by the reserved list gaining `vpn`, `tunnel` and + `wireguard` — the words an interface can legitimately be called — and by + minting only names that fail `keepIface`, so the kernel's vocabulary never + enters the replay at all. +- **The bundle's collection order stays load-bearing.** The replay can only + replace what some earlier entry taught it, so `config.json` and `learned.json` + must still be collected before `doctor.json` and `state.json`. Pinned by + `TestTheBundleCollectsNameSourcesFirst`. +- **An interface known only to the live host** — autodetect, no daemon, so no + `state.json` — is taught to the redactor by nothing, and the replay has nothing + to replay. Stated in `internal/redact`'s package comment rather than implied, + because a redactor must never claim coverage it does not have. diff --git a/docs/adr/README.md b/docs/adr/README.md index 4e29fd3..1999423 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -27,6 +27,7 @@ New records use [template.md](template.md) and take the next free number. | [0013](0013-geo-provider-pass-opt-out.md) | The geo-provider pass gets an opt-out, not a redesign | accepted, implemented | | [0014](0014-login-item-launch-marker.md) | The login item carries an explicit launch marker | accepted, implemented | | [0015](0015-complete-purge-semantics.md) | What a complete purge removes, and what it deliberately does not | accepted, implemented | +| [0016](0016-bundle-identifiers-are-data-not-prose.md) | A bundle identifier is redacted by the key that names it | accepted, implemented | > **0006 is the one to read first if you are touching the geo lookup.** It records why > the obvious implementation silently defeats the exit-country check, and it exists diff --git a/docs/contribute/architecture.md b/docs/contribute/architecture.md index a867c24..10d0507 100644 --- a/docs/contribute/architecture.md +++ b/docs/contribute/architecture.md @@ -309,6 +309,7 @@ failure they were built to prevent. | Guard model | **Always-on interface guard — the only model** | A VPN drop is cut instantly, with a zero leak window. A reactive poller leaks for one poll interval, which is why the country-blocklist fallback was removed rather than kept as a peer ([ADR-0001](../adr/0001-single-guard-mode.md)) | | Resting posture | **STANDBY — no rules until a tunnel is observed** | A guard with no tunnel blocks everything, which is a blackout rather than security. This is the safety job `vpn.enabled: false` was quietly doing ([ADR-0002](../adr/0002-standby-no-tunnel-posture.md)) | | Recovery | **Wait for the VPN to return to an allowed country** | While full-blocked, observe the exit through a time-windowed probe and restore the guard once the exit is allowed again | +| Bundle redaction | **By the KEY that names an identifier**, with a literal-name replay as the fallback | A value under `endpoint` or `tunnelInterfaces` is an identity whatever it spells; hunting for the same word in free text is the pass that has produced a defect in every round it was touched in, so it covers only text dezhban does not author — rendered rulesets, log records, OS errors ([ADR-0016](../adr/0016-bundle-identifiers-are-data-not-prose.md)) | Two of these were revisited during the build and are worth naming as *deviations*, since the reasoning is not obvious from the code: diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index 9a4e15d..e1e829c 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -1394,6 +1394,24 @@ end up typing a password. rule can see. Confirm the structure survived: `/Users/user-1/Downloads/…` still reads as a Downloads folder, and the same profile is the same `profile-N` token in config.json and state.json. +- [ ] **The interface name goes from the PROSE and the RULESET, not only from the + fields that name it.** If your VPN client made its own interface + (`nordlynx`, `proton`, `gpd`), grep the whole bundle for it — and make sure + you look in `rules-preview.txt` and `log.txt`, not just the JSON. A rendered + pf or nft rule carries it as a bare word (`pass out quick on { … }`, + `oifname { … }`), doctor's tunnels and lockout checks write it into a + finding, and the daemon logs `detail="… up"` — none of which any shape can + see. **None may appear.** Then confirm the other direction: every + `utun*`/`en0`/`lo0` is still there, or the ruleset has become unreadable for + no gain. Same check for the VPN's service name as Network settings shows it. +- [ ] **One token, one identity.** Two different servers must be two different + `ip-N`, and two different profiles two different `profile-N`. The README's + legend is where a collision shows: a row naming the same token twice, or two + rows claiming one token, means the bundle reports two things as one — which + reads as a working bundle right up until someone tries to diagnose with it. + A legend row rendered as a **list** ("2 distinct profile names → profile-1, + profile-3") rather than a range is not a bug: a number is skipped whenever + it would have produced a name the bundle actually carries. - [ ] **Nothing dezhban ships is redacted.** The other direction, and it fails just as badly: a bundle that hides the diagnosis has thrown away the answer and hidden no identity. In the same bundle confirm dezhban's OWN @@ -1412,9 +1430,11 @@ end up typing a password. bundle until someone tries to use it. - [ ] **The bundle is 0600.** `ls -l dezhban-report-*.zip` — an `--include-network` bundle must not be readable by other local accounts. -- [ ] **The README never leaks.** Its legend reports counts - ("23 distinct IP addresses → ip-1 … ip-23", "2 distinct profile names → - profile-1 … profile-2") and no originals. +- [ ] **The README never leaks.** Its legend reports counts and the tokens they + cover — a range when they run consecutively ("23 distinct IP addresses → + ip-1 … ip-23"), a list when one was skipped ("2 distinct profile names → + profile-1, profile-3") — and no originals. Every token it names must be + findable somewhere in the bundle. - [ ] **The opt-out is loud.** With "Turn redaction off: include my real server addresses, exit IP, VPN profile names and account name" ticked, the bundle contains all of those AND says so at the top of its README; the CLI prints diff --git a/docs/usage/cli.md b/docs/usage/cli.md index bfe8e0b..fd6b956 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -286,9 +286,18 @@ one of them (a rendered ruleset is firewall syntax, not JSON), and so is fixed path the config file does not move. Each exits 2 and says which flag to drop. `doctor --json` prints the identical findings `doctor` reports in prose — `{checks: -[{name, status, summary, details, fixes}], ok}` — for a consumer (the macOS -app's Diagnostics pane) that needs to render them itself rather than parse -text. See [config.md](config.md) for the full field reference and +[{name, status, profiles?, connectedVPN?, summary, details, fixes}], ok}` — for +a consumer (the macOS app's Diagnostics pane) that needs to render them itself +rather than parse text. Each entry of `details` is an object, `{iface?, +profile?, endpoint?, text}`: the identifier the finding is **about**, carried as +data, plus the prose that qualifies it. A renderer composes the line — the +identifier, its text, and the interface in parentheses when a finding is about +an endpoint *and* the interface it is misrouted onto. An entry whose `text` is +empty and that carries no identifier is a **paragraph break**, not a finding. +`profiles` and `connectedVPN` carry the identifiers named by the one-sentence +`summary`. The identifiers are fields rather than words inside the prose so that +`dezhban report` can redact them by key; a consumer that does not need them can +ignore them. See [config.md](config.md) for the full field reference and [troubleshooting.md](troubleshooting.md) for the lockout-recovery runbook. `detect-vpn --json` is the machine-readable VPN inventory the app's @@ -470,13 +479,36 @@ unreadable, and the geo-provider hostnames dezhban ships are kept for the same reason. Hostname redaction works from an **allow-list**, so a name nobody anticipated is redacted rather than leaked. -Two identifiers in a bundle are not address-shaped and get placeholders of their -own kind: the **profile names** you chose (`vpn.profiles[].name`, -`state.json`'s `activeProfile`, and a profile's `tunnelHint` — these are called -"mullvad-de" and "nordlynx", so they name the provider as plainly as a server -address does) and the **account name** in any home-directory path. A file you -imported is redacted too, because a `.conf` or `.ovpn` is named after the VPN it -configures. +Several identifiers in a bundle are not address-shaped and are just as telling. +Each gets placeholders of its own kind, so the legend can say how many of each +were replaced: + +- `ip-N` — a public IP address. +- `host-N` — a hostname, including a single-label one like `mullvad` that no + shape can recognise. +- `profile-N` — the **profile names** you chose (`vpn.profiles[].name`, + `state.json`'s `activeProfile`, and the names `learned.json` files endpoints + under). They are called "mullvad-de", so they name the provider as plainly as + a server address does. +- `hint-N` — a profile's `tunnelHint`, unless it is one of the generic prefixes + every host has (`utun`, `tun`, `tap`, `wg`, `ipsec`), which name nobody. +- `iface-N` — a **tunnel interface name** your VPN client created, like + `nordlynx` or `proton`. The kernel's own names — `utun4`, `en0`, `lo0`, `wg0` + — are kept: every host has them, they identify nobody, and hiding them would + make a ruleset unreadable. +- `vpn-N` — the VPN's service name as your OS network settings show it. +- `user-N` — the **account name** in any home-directory path. A file you + imported is redacted too, because a `.conf` or `.ovpn` is named after the VPN + it configures. + +The README's legend reports counts and the tokens they cover — a range +("23 distinct IP addresses → ip-1 … ip-23") when a kind's tokens run +consecutively, and a list ("2 distinct profile names → profile-1, profile-3") +when one number was skipped. A number is skipped whenever it would have produced +a name this bundle actually carries: a profile may legitimately be *called* +`profile-1`, and a placeholder that is also a real name is no redaction at all. +That is the same rule read twice — a token is never the spelling of a name in +the bundle, and no two identities ever share a token. The bundle is written **0600**, so the `--include-network` version is not readable by other accounts on the machine. `--include-network` produces that diff --git a/gui/macos/Sources/DezhbanCore/DoctorReport.swift b/gui/macos/Sources/DezhbanCore/DoctorReport.swift index f5af0d9..4802090 100644 --- a/gui/macos/Sources/DezhbanCore/DoctorReport.swift +++ b/gui/macos/Sources/DezhbanCore/DoctorReport.swift @@ -1,5 +1,50 @@ import Foundation +/// Mirrors Go's `doctorDetail` (cmd/dezhban). One of a check's findings: the +/// identifier it is ABOUT, carried as data, plus the prose that qualifies it. +/// +/// The identifier is a field rather than a word inside `text` because the +/// diagnostic bundle's redactor works by KEY — a value under `iface`, `profile` +/// or `endpoint` is recognised for what it is and replaced with a stable token, +/// while the same word inside prose is reachable only by a literal +/// word-replacement pass over free text. Composing the sentence is this view's +/// job, which is why `line` lives here and mirrors Go's `doctorDetail.line`. +/// +/// An empty `text` with no identifier is a PARAGRAPH BREAK, not a finding. +public struct DoctorDetail: Codable, Hashable { + public let iface: String? + public let profile: String? + public let endpoint: String? + public let text: String + + public init(iface: String? = nil, profile: String? = nil, endpoint: String? = nil, text: String) { + self.iface = iface + self.profile = profile + self.endpoint = endpoint + self.text = text + } + + /// The composed human form, identical to what `dezhban doctor` prints: the + /// finding's subject, its prose, and — when a finding is about an endpoint + /// AND the interface it is misrouted onto — the interface in parentheses. + public var line: String { + let subject = endpoint ?? profile ?? iface ?? "" + var out = subject + if !text.isEmpty { + if subject.isEmpty || text.hasPrefix(":") { + // A port joins its address with no separator: `1.2.3.4:51820`. + out += text + } else { + out += " " + text + } + } + if let iface, !iface.isEmpty, iface != subject { + out += " (\(iface))" + } + return out + } +} + /// Mirrors Go's `doctorCheck` (cmd/dezhban's `dezhban doctor --json`). `status` /// is one of "ok"/"warn"/"fail" — kept as the raw string (not an enum) so an /// unrecognised future value decodes rather than failing the whole report. @@ -8,7 +53,9 @@ public struct DoctorCheck: Codable, Identifiable { public let name: String public let status: String public let summary: String - public let details: [String]? + public let profiles: [String]? + public let connectedVPN: String? + public let details: [DoctorDetail]? public let fixes: [String]? } diff --git a/gui/macos/Sources/DezhbanMenu/DiagnosticsView.swift b/gui/macos/Sources/DezhbanMenu/DiagnosticsView.swift index 7c559ca..071fb65 100644 --- a/gui/macos/Sources/DezhbanMenu/DiagnosticsView.swift +++ b/gui/macos/Sources/DezhbanMenu/DiagnosticsView.swift @@ -526,11 +526,25 @@ struct DiagnosticsView: View { Label(rowTitle(check), systemImage: symbol(for: check.status)) .foregroundStyle(color(for: check.status)) .font(.body.weight(.semibold)) - ForEach(Array((check.details ?? []).enumerated()), id: \.offset) { _, line in + // Said once, above the findings, because it is read once per scan + // and is the same for every one of them — and because saying it once + // is what lets it be a field the report bundle's redactor can see. + if let vpn = check.connectedVPN, !vpn.isEmpty { + Text("via \(vpn)") + .font(.callout) + .foregroundStyle(.secondary) + .textSelection(.enabled) + } + ForEach(Array((check.details ?? []).enumerated()), id: \.offset) { _, found in // An empty detail is a paragraph break, not a finding (see - // doctorCheck.Details in cmd/dezhban/main.go) — rendering it as - // a Text would leave a stray blank row where the CLI puts a - // blank line. + // doctorDetail in cmd/dezhban/main.go) — rendering it as a Text + // would leave a stray blank row where the CLI puts a blank line. + // + // `line` composes the identifier with its prose, the same way + // printDoctor does: the identifier is carried as a FIELD so the + // bundle's redactor can see it by key, so the sentence is this + // view's to build. + let line = found.line if line.isEmpty { Spacer().frame(height: 4) } else { diff --git a/gui/macos/Tests/DezhbanCoreTests/DoctorReportTests.swift b/gui/macos/Tests/DezhbanCoreTests/DoctorReportTests.swift index 2d65018..2716201 100644 --- a/gui/macos/Tests/DezhbanCoreTests/DoctorReportTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/DoctorReportTests.swift @@ -8,7 +8,8 @@ struct DoctorReportTests { { "checks": [ {"name": "config", "status": "ok", "summary": "OK (loaded and validated)"}, - {"name": "tunnels", "status": "ok", "summary": "", "details": ["utun4 — 10.0.0.0/24"]} + {"name": "tunnels", "status": "ok", "summary": "", + "details": [{"iface": "utun4", "text": "— 10.0.0.0/24"}]} ], "ok": true } @@ -17,7 +18,7 @@ struct DoctorReportTests { let report = try #require(DoctorReport.decode(json)) #expect(report.ok) #expect(report.checks.count == 2) - #expect(report.checks[1].details == ["utun4 — 10.0.0.0/24"]) + #expect(report.checks[1].details?.first?.line == "utun4 — 10.0.0.0/24") } @Test func decodesFailingCheckWithFixes() throws { @@ -28,7 +29,8 @@ struct DoctorReportTests { "name": "endpoints", "status": "fail", "summary": "", - "details": ["10.0.0.1 — MISCONFIGURED: inside utun4's subnet 10.0.0.0/24"], + "details": [{"endpoint": "10.0.0.1", "iface": "utun4", + "text": "— MISCONFIGURED: inside its subnet 10.0.0.0/24"}], "fixes": ["10.0.0.1 is a tunnel-internal address..."] } ], @@ -40,6 +42,47 @@ struct DoctorReportTests { #expect(!report.ok) #expect(report.checks[0].status == "fail") #expect(report.checks[0].fixes?.count == 1) + // The identifier is a field, and composing it back is this side's job — + // the same sentence the CLI prints. + #expect(report.checks[0].details?.first?.line + == "10.0.0.1 — MISCONFIGURED: inside its subnet 10.0.0.0/24 (utun4)") + } + + /// Additive keys must not fail the decode. Every carrier this report gained + /// is optional for exactly this reason: JSONDecoder fails the WHOLE + /// DoctorReport on a shape it cannot read, so `decode` returns nil, the + /// Diagnostics pane falls back to its error state, and DoctorAttention stops + /// badging the sidebar — an app that silently goes quiet about a warning. + @Test func unknownAndNewCheckFieldsDoNotFailTheDecode() throws { + let json = """ + { + "checks": [{ + "name": "discover", "status": "ok", "summary": "", + "profiles": ["work"], + "connectedVPN": "Mullvad", + "somethingAddedLater": 7, + "details": [{"endpoint": "198.51.100.7", "text": ":51820"}] + }], + "ok": true + } + """.data(using: .utf8)! + + let report = try #require(DoctorReport.decode(json)) + #expect(report.checks[0].connectedVPN == "Mullvad") + #expect(report.checks[0].profiles == ["work"]) + #expect(report.checks[0].details?.first?.line == "198.51.100.7:51820") + } + + /// A detail with no identifier and no text is the paragraph break, and both + /// renderers depend on it staying distinguishable from a finding. + @Test func anEmptyDetailIsAParagraphBreak() throws { + let json = """ + { "checks": [{"name": "config", "status": "ok", "summary": "", + "details": [{"text": ""}]}], "ok": true } + """.data(using: .utf8)! + + let report = try #require(DoctorReport.decode(json)) + #expect(report.checks[0].details?.first?.line.isEmpty == true) } @Test func decodesCheckWithNoDetailsOrFixes() throws { diff --git a/internal/redact/json.go b/internal/redact/json.go index 8f058fa..b2b609d 100644 --- a/internal/redact/json.go +++ b/internal/redact/json.go @@ -54,7 +54,7 @@ func (r *Redactor) JSON(s string) string { // the fallback re-reads the ORIGINAL text: without this the legend would // count tokens that appear nowhere in the bundle and push every real token's // ordinal past them. - mark := len(r.order) + mark := r.checkpoint() var buf bytes.Buffer enc := json.NewEncoder(&buf) enc.SetIndent("", " ") @@ -69,13 +69,41 @@ func (r *Redactor) JSON(s string) string { return buf.String() } -// rollback forgets every placeholder minted since mark, for a pass whose output -// was thrown away. -func (r *Redactor) rollback(mark int) { - for _, key := range r.order[mark:] { +// checkpoint captures everything placeholder mutates, so a pass whose output is +// thrown away can be undone. `order` alone is not enough: the ordinal counter is +// monotonic by design (see placeholder), so truncating `order` no longer restores +// it the way the old count-derived ordinal did. +type checkpoint struct { + order int + next map[string]int +} + +func (r *Redactor) checkpoint() checkpoint { + next := make(map[string]int, len(r.next)) + for k, v := range r.next { + next[k] = v + } + return checkpoint{order: len(r.order), next: next} +} + +// rollback forgets every placeholder minted since c, for a pass whose output was +// thrown away. +func (r *Redactor) rollback(c checkpoint) { + for _, key := range r.order[c.order:] { delete(r.seen, key) } - r.order = r.order[:mark] + r.order = r.order[:c.order] + r.next = c.next + // values is rebuilt rather than pruned per key: one value can be keyed under + // two kinds, so deleting it for the discarded key would forget it for the + // surviving one. This runs at most once per bundle entry. + r.values = make(map[string]bool, len(r.order)) + r.minted = make(map[string]bool, len(r.order)) + for _, key := range r.order { + _, value, _ := strings.Cut(key, ":") + r.values[value] = true + r.minted[r.seen[key]] = true + } } // object preserves an object's key ORDER, which encoding/json's map does not. @@ -227,7 +255,13 @@ func (r *Redactor) value(s string, path []string) string { key = path[len(path)-1] } switch key { - case "activeProfile", "profile": + case "activeProfile", "profile", "profiles": + // `profiles` is doctor.json's carrier for the entry names its Summary + // names in prose, and it reuses this kind deliberately: a learned entry + // and the config profile it belongs to must be the SAME token, or the + // bundle shows two identities where the host has one. It cannot misfire + // on config.json, where `profiles` holds objects — walk never calls value + // on an object. return r.name(s, "profile") case "tunnelHint": return r.name(s, "hint") @@ -252,13 +286,20 @@ func (r *Redactor) value(s string, path []string) string { // the scoping above exists to prevent, walking back in through the // literal-name pass. Prose still gets knownNames, where mangling a // common word is noise and the leak it closes is not. - return r.Text(s) + return r.shapes(s) case "tunnelInterfaces", "iface": // learned.json records the interface it observed under `iface`. The same // value is redacted under tunnelInterfaces and tunnels[].name, so leaving // this key out meant one document redacted a provider-named interface and // the one beside it did not. return r.ifaceName(s) + case "connectedVPN": + // The VPN's friendly service name, from the OS network settings and from + // doctor's discover check. It is chosen by the user and usually names the + // provider, so it is an identity — but not a tunnelHint, which is an + // interface-name PREFIX whose allow-list would keep `wg` or `utun` and + // whose legend noun would call this a tunnel hint. + return r.name(s, "vpn") case "addr", "endpoint", "endpoints", "ip", "ipv6": return r.endpointOrFree(s) case "fixes": @@ -274,7 +315,7 @@ func (r *Redactor) value(s string, path []string) string { // needs, and they are what catches the next interpolation. return r.addresses(s) default: - return r.free(s) + return r.Text(s) } } @@ -296,7 +337,7 @@ func (r *Redactor) endpointOrFree(s string) string { return red + ":" + port } } - return r.free(s) + return r.Text(s) } // splitPort separates a trailing :port from a value that is not itself an @@ -313,18 +354,9 @@ func splitPort(s string) (host, port string, ok bool) { return host, port, true } -// free rewrites a string that no key identifies: the shape passes, then the -// names this bundle has already replaced elsewhere. -func (r *Redactor) free(s string) string { - // knownNames FIRST. Its values are known identities; hostRe is a guess. Run - // the guess first and a profile called `nord.vpn` is minted as `host-N` in - // prose and `profile-N` in the config — one identifier, two tokens, two - // legend lines, and no way for a reader to see they are the same server. - return r.Text(r.knownNames(s)) -} - -// knownNames replaces profile and hint names this Redactor has already minted, -// wherever they appear as words in free text. +// knownNames replaces the names this Redactor has already minted — profile, hint, +// host and interface names, see replayKinds — wherever they appear as words in +// free text. // // This is what reaches the names no shape and no key can see: the doctor writes // learned entry names — which are profile names — into its Details and Summary @@ -349,14 +381,15 @@ func (r *Redactor) knownNames(s string) string { var names []named for _, key := range r.order { kind, value, _ := strings.Cut(key, ":") - if (kind == "profile" || kind == "hint" || kind == "host") && value != "" { + if replayKinds[kind] && value != "" { names = append(names, named{value, r.seen[key]}) } } // Longest first, so a name that is a prefix of another cannot claim it. sort.Slice(names, func(i, j int) bool { return len(names[i].value) > len(names[j].value) }) for _, n := range names { - if reserved[n.value] || !strings.Contains(strings.ToLower(s), n.value) { + if reserved[n.value] || placeholderRe.MatchString(n.value) || + !strings.Contains(strings.ToLower(s), n.value) { continue } // Not `\b`. config accepts a profile name of [A-Za-z0-9._-] (config.go's @@ -386,16 +419,53 @@ func (r *Redactor) knownNames(s string) string { // Failing to redact one profile whose name collides with dezhban's vocabulary // is noise in a bundle; rewriting the vocabulary is a broken diagnosis. // -// This pass cannot launder its own tokens, but that is `free`'s ordering rather -// than anything here: knownNames runs on the value's ORIGINAL text, before Text -// has minted a placeholder into it, so there is never a token present for a name -// to match inside. +// This pass cannot launder its own tokens, and the GUARD is what makes that true +// — ordering alone did not. knownNames runs on a value's original text, so a +// token it minted is not there to be matched. But a real value can be SPELLED +// like a token: `a-very-long-profile-name` mints `profile-1`, a later profile +// literally called `profile-1` mints `profile-2`, and because the replay runs +// longest-value-first it wrote `profile-1` into the prose and then matched its +// own output — two identities collapsed onto one token, and the long name's real +// token appearing nowhere. A value matching placeholderRe is therefore skipped. +// Leaving such a name verbatim costs nothing a reader can use: by its spelling it +// names nobody. +// `vpn`, `tunnel` and `wireguard` are here because keepIface only keeps a generic +// stem followed by digits, so an interface literally called one of them MINTS — +// and once iface joined the replay, that would have rewritten dezhban's own prose: +// the lockout check's "WireGuard (and other…", the runner's tunnel warnings, and +// every "vpn guard active" line in the log. Note what reserved does and does not +// cost: it suppresses only the free-text replay. The key-aware pass still redacts +// the value where a key names it, so nothing leaks by being listed here. var reserved = map[string]bool{ "guard": true, "full-block": true, "switch-window": true, "standby": true, "stopped": true, "fullblock": true, "switch": true, "pause": true, "config": true, "tunnels": true, "endpoints": true, "lockout": true, "service": true, "liveness": true, "control": true, "armatboot": true, "endpointretention": true, + "vpn": true, "tunnel": true, "wireguard": true, +} + +// replayKinds are the kinds whose values knownNames replays into free text. +// +// `iface` is in here because a provider-created interface is named as plainly as +// a server is, and no shape can see a single-label word: state.json's +// tunnels[].detail is literally " up", doctor's tunnels and lockout checks +// write the name as an ordinary word, applied-rules.json carries the whole +// rendered ruleset under one key, and the daemon logs `detail=" up"`. Only +// names that FAIL keepIface ever mint, so the values here are vendor names +// (`nordlynx`, `proton`, `gpd`) or host-structural stems (`wlan0`, `enp3s0`) — +// the first is the point, the second is noise, and the words that would have been +// damage are in reserved above. +// +// `user`, `ip` and `vpn` are deliberately absent, and that is a decision rather +// than an oversight. Account names are routinely generic words (`admin`, `dev`) +// and the home-directory pass already reaches the only place one appears; an +// address is found everywhere by shape, so replaying it adds risk and no +// coverage; and a VPN service name is whatever the user typed into Network +// settings, which is as often `Work` or `Home` as it is a provider — the same +// hazard as an account name, and it appears in exactly one field. +var replayKinds = map[string]bool{ + "profile": true, "hint": true, "host": true, "iface": true, } // within reports whether key appears anywhere in the path. diff --git a/internal/redact/json_test.go b/internal/redact/json_test.go index c93db93..9f08c2a 100644 --- a/internal/redact/json_test.go +++ b/internal/redact/json_test.go @@ -2,6 +2,7 @@ package redact import ( "encoding/json" + "strconv" "strings" "testing" ) @@ -401,3 +402,137 @@ func TestATruncatedInterfaceArrayIsStillRedacted(t *testing.T) { t.Errorf("got %q — the attr form leaked", got) } } + +// The replay sorts longest-value-first, so a value SPELLED like a token gets the +// pair (`profile-1` → `profile-2`) applied to text the replay itself just wrote: +// the long name became `profile-1` and was then rewritten to `profile-2`. Two +// identities onto one token, and the long name's real token nowhere in the file. +func TestTheNameReplayNeverLaundersATokenIntoAnother(t *testing.T) { + r := New(true) + r.JSON(`{"vpn":{"profiles":[{"name":"a-very-long-profile-name"},{"name":"profile-1"}]}}`) + got := r.JSON(`{"checks":[{"summary":"every learned address for a-very-long-profile-name has aged out."}]}`) + if strings.Contains(got, "a-very-long-profile-name") { + t.Fatalf("the name survived: %q", got) + } + if !strings.Contains(got, "profile-1") || strings.Contains(got, "profile-2") { + t.Errorf("got %q — the prose carries a token minted for a different identity", got) + } +} + +// An interface can be CALLED `vpn` or `wireguard`: keepIface only keeps a generic +// stem followed by digits. Once iface joined the replay, such a name would have +// rewritten dezhban's own prose everywhere it says those words. +func TestTheReplayNeverClaimsAWordDezhbanWrites(t *testing.T) { + r := New(true) + r.JSON(`{"vpn":{"tunnelInterfaces":["vpn"]}}`) + const prose = "The vpn guard is on. WireGuard clients send from an unconnected socket." + got := r.JSON(`{"checks":[{"summary":` + strconv.Quote(prose) + `}]}`) + if !strings.Contains(got, "vpn guard is on") || !strings.Contains(got, "WireGuard clients") { + t.Errorf("got %q — the replay claimed dezhban's own vocabulary", got) + } +} + +// state.json pairs tunnels[].name, which the walk redacts by key, with a detail +// that repeats the same word one key over — where only the replay can reach it. +func TestAnInterfaceNameTheBundleAlreadyKnowsIsReplacedInProse(t *testing.T) { + got := New(true).JSON(`{"tunnels":[{"name":"nordlynx","detail":"nordlynx up"}]}`) + if strings.Contains(got, "nordlynx") { + t.Fatalf("the interface name survived: %q", got) + } + if strings.Count(got, "iface-1") != 2 { + t.Errorf("got %q — the two copies are not the same token", got) + } +} + +// rules-preview.txt is a rendered ruleset, and a pf or nft rule carries an +// interface name as a bare word — no key, no dot, no attr. Only the replay can +// see it, and Text did not run the replay. +func TestARenderedRulesetLosesTheProviderInterface(t *testing.T) { + r := New(true) + r.JSON(`{"vpn":{"tunnelInterfaces":["nordlynx","utun4"]}}`) + got := r.Text("pass out quick on { nordlynx utun4 } all no state\n" + + "oifname { \"nordlynx\" } accept\n" + + "pass out quick on lo0 all\n") + if strings.Contains(got, "nordlynx") { + t.Fatalf("the provider interface survived the ruleset: %q", got) + } + for _, keep := range []string{"utun4", "lo0", "pass out quick", "accept"} { + if !strings.Contains(got, keep) { + t.Errorf("got %q — %q is structural and must survive", got, keep) + } + } +} + +// The daemon logs a single-label endpoint twice: once as an attr, once inside +// the resolver's error text. A name with no dot is invisible to hostRe, so the +// copy in the error stood while the attr went. +func TestASingleLabelHostSurvivesNowhereInALogLine(t *testing.T) { + r := New(true) + r.JSON(`{"vpn":{"endpoints":["mullvad"]}}`) + got := r.Text(`level=ERROR msg="resolve failed" host=mullvad err="lookup mullvad: no such host"`) + if strings.Contains(got, "mullvad") { + t.Fatalf("the host survived: %q", got) + } + if strings.Count(got, "host-1") != 2 { + t.Errorf("got %q — the attr and the error text are not the same token", got) + } +} + +// doctor.json carries its identifiers as fields so the walk sees them by key. +// The keys are the ones this switch already knows; a new spelling would fall to +// the default branch and get only the treatment the prose already had. +func TestADoctorDetailsIdentifierIsRedactedByItsKey(t *testing.T) { + got := New(true).JSON(`{"checks":[{"name":"tunnels","details":[` + + `{"iface":"nordlynx","text":"— 10.0.0.0/24"},` + + `{"iface":"utun4","text":"— 10.1.0.0/24"},` + + `{"endpoint":"198.51.100.7","text":":51820"}` + + `]}]}`) + if strings.Contains(got, "nordlynx") { + t.Errorf("the interface name survived: %q", got) + } + if !strings.Contains(got, "utun4") { + t.Errorf("got %q — a generic interface is structural and must survive", got) + } + if strings.Contains(got, "198.51.100.7") { + t.Errorf("the endpoint survived: %q", got) + } + if !strings.Contains(got, `"text": "— 10.0.0.0/24"`) { + t.Errorf("got %q — the private subnet is deliberately kept", got) + } +} + +// A doctor check's profiles field must share the config's token, or the bundle +// shows two identities where the host has one. And it must not fire on +// config.json's own `profiles`, which holds objects rather than names. +func TestADoctorChecksProfileFieldSharesTheConfigsToken(t *testing.T) { + r := New(true) + cfg := r.JSON(`{"vpn":{"profiles":[{"name":"work-nord"}]}}`) + if !strings.Contains(cfg, "profile-1") || strings.Contains(cfg, "work-nord") { + t.Fatalf("config = %q", cfg) + } + got := r.JSON(`{"checks":[{"name":"endpointRetention","profiles":["work-nord"]}]}`) + if strings.Contains(got, "work-nord") { + t.Fatalf("the entry name survived: %q", got) + } + if !strings.Contains(got, "profile-1") { + t.Errorf("got %q — the same profile got a second token", got) + } +} + +// The connected VPN's service name is chosen by the user and usually names the +// provider. It is its own kind: a tunnelHint is an interface-name PREFIX whose +// allow-list would keep `wg`, and whose legend noun would mislabel this. +func TestAConnectedVPNNameIsRedacted(t *testing.T) { + r := New(true) + got := r.JSON(`{"checks":[{"name":"discover","connectedVPN":"Mullvad VPN"}]}`) + if strings.Contains(got, "Mullvad") { + t.Fatalf("the service name survived: %q", got) + } + if !strings.Contains(got, "vpn-1") { + t.Errorf("got %q, want a vpn-N token", got) + } + legend := r.Legend() + if len(legend) != 1 || !strings.Contains(legend[0], "VPN service name") { + t.Errorf("legend = %v — the kind has no noun of its own", legend) + } +} diff --git a/internal/redact/redact.go b/internal/redact/redact.go index d587638..de694ce 100644 --- a/internal/redact/redact.go +++ b/internal/redact/redact.go @@ -7,10 +7,13 @@ // ruleset carries both, because the whole point of the ruleset is which // addresses may be reached. // -// Two identifiers in a bundle are not address-shaped and are just as telling: -// the PROFILE NAMES the user chose (they are called "mullvad-de", and a tunnel -// hint is "nordlynx"), and the ACCOUNT NAME in every home-directory path. Both -// get placeholders of their own kind. +// Several identifiers in a bundle are not address-shaped and are just as +// telling, and each gets placeholders of its own kind: the PROFILE NAMES the +// user chose (they are called "mullvad-de"), a profile's TUNNEL HINT, the +// TUNNEL INTERFACE NAME the VPN client created ("nordlynx" names the provider +// as plainly as a server does, while the kernel's own `utun4`/`en0` name +// nobody), the VPN's SERVICE NAME from the OS network settings, and the ACCOUNT +// NAME in every home-directory path. // // **Stable** placeholders, not `[redacted]`: the same address becomes the same // placeholder everywhere it appears, so the bundle stays diagnosable. "The rules @@ -23,6 +26,26 @@ // address-shaped and hostname-shaped text everywhere, in every file, rather than // by knowing which fields of which struct to blank. Anything it is unsure about // is redacted. +// +// TWO RESIDUALS, stated rather than implied, because the rule above means the +// limits have to be written down as plainly as the coverage. +// +// 1. A real identifier SPELLED like an already-minted token, arriving after that +// mint, keeps its spelling — see placeholder. Nothing can move a token that +// earlier entries already carry. The bound: only `-` can +// collide, and that spelling names no provider and no person. +// +// 2. An interface known ONLY to the live host — autodetect with no +// vpn.tunnelInterfaces, and no daemon, so no state.json — is minted by +// nothing before rules-preview.txt and doctor.json are walked, and the name +// replay has nothing to replay. With the daemon running, state.json's +// tunnels[].name closes it, which is why the bundle's collection order +// (cmd/dezhban/report.go) is load-bearing. Closing it outright means either a +// field-aware pass over the three rendering grammars dezhban emits +// (`on { … }`, `oifname { … }`, `-InterfaceAlias …`) or seeding this +// Redactor from the resolved policy — the second risks minting tokens for +// values no entry ends up carrying, which is the legend overcount +// redactEntry's comment warns about. package redact import ( @@ -136,7 +159,13 @@ var ( // untouched. ifacesJSONRe = regexp.MustCompile(`("tunnelInterfaces"\s*:\s*\[)([^\[\]{}]*)(\]?)`) ifaceJSONRe = regexp.MustCompile(`("iface"\s*:\s*")((?:[^"\\]|\\.)+)(")`) - ifaceAttrRe = regexp.MustCompile(`\b(iface|tunnel)=("(?:[^"\\]|\\.)*"|[^\s]+)`) + // Only `iface=`. `tunnel=` was in here with no producer anywhere in the + // tree — the Windows preview writes `-DisplayName 'dezhban-tunnel'`, not an + // attr — and a pattern matching a key the daemon never writes is untested + // surface that can only mint a token for a word it should not have claimed. + // The producers that justify this one: internal/runner/runner.go's bad-route + // warning and internal/netdetect/resolve.go's tunnel-internal drop. + ifaceAttrRe = regexp.MustCompile(`\biface=("(?:[^"\\]|\\.)*"|[^\s]+)`) jsonStringRe = regexp.MustCompile(`"(?:[^"\\]|\\.)*"`) // A home directory names the account, which names the person. The segment // after /Users or /home is the only identifying part — the rest of the path @@ -166,16 +195,62 @@ type Redactor struct { seen map[string]string order []string + + // next is the highest ordinal ever OFFERED for a kind, whether or not it was + // used. Monotonic, because an ordinal derived by counting `order` reissues + // one that a skip vacated — see placeholder. + next map[string]int + // values is every value keyed so far, ACROSS ALL KINDS. A token must not be + // the spelling of a name this bundle carries, and knownNames replays values + // without regard to kind, so the check cannot be per-kind either. + values map[string]bool + // minted is every token this Redactor has produced. A pass that runs after + // another one can be handed one — see placeholder. + minted map[string]bool } // New returns a Redactor. enabled false is the explicit opt-out: everything // passes through untouched. func New(enabled bool) *Redactor { - return &Redactor{Enabled: enabled, seen: map[string]string{}} + return &Redactor{ + Enabled: enabled, + seen: map[string]string{}, + next: map[string]int{}, + values: map[string]bool{}, + minted: map[string]bool{}, + } } -// Text rewrites every address and hostname in s. +// Text redacts one TEXTUAL entry of a bundle: the names this bundle has already +// replaced elsewhere, then the shape passes. +// +// knownNames FIRST, deliberately. Its values are known identities; hostRe is a +// guess. Run the guess first and a profile called `nord.vpn` is minted as +// `host-N` in prose and `profile-N` in the config — one identifier, two tokens, +// two legend lines, and no way for a reader to see they are the same server. +// +// Every external caller wants this order, which is why it is what `Text` means +// rather than a second exported method beside it. `rules-preview.txt`, `log.txt` +// and the README's notes can each quote a name another entry taught the redactor, +// and a two-door API whose wrong door under-redacts silently is the one shape +// this package may not have. Inside the package, a caller that wants the shape +// passes ALONE says so by calling shapes. +// +// The residual: a name first learned in the same entry that quotes it — the +// daemon's `host=x err="lookup x: no such host"` when `x` appears in no earlier +// file — is not replayed within that entry, because the replay runs ahead of the +// mint by design. Entry order (see cmd/dezhban/report.go) is what keeps that rare. func (r *Redactor) Text(s string) string { + if !r.Enabled { + return s + } + return r.shapes(r.knownNames(s)) +} + +// shapes rewrites every address and hostname in s. No literal-name replay: a +// caller reaching a value whose KEY already identified it must not have the +// bundle's other names spliced into it. +func (r *Redactor) shapes(s string) string { if !r.Enabled { return s } @@ -258,15 +333,15 @@ func (r *Redactor) Text(s string) string { }) s = ifaceAttrRe.ReplaceAllStringFunc(s, func(m string) string { g := ifaceAttrRe.FindStringSubmatch(m) - v := jsonBody(strings.Trim(g[2], `"`)) + v := jsonBody(strings.Trim(g[1], `"`)) red := r.ifaceName(v) if red == v { return m } - if strings.HasPrefix(g[2], `"`) { + if strings.HasPrefix(g[1], `"`) { red = `"` + red + `"` } - return g[1] + "=" + red + return "iface=" + red }) s = endpointAttrRe.ReplaceAllStringFunc(s, func(m string) string { g := endpointAttrRe.FindStringSubmatch(m) @@ -381,10 +456,19 @@ func (r *Redactor) homeDirs(s string) string { }) } +// placeholderKinds is every kind this package mints. It is a list rather than a +// literal alternation because the alternation drifted: the `iface` kind was added +// with its own mint path and placeholderRe was left as it was, so the pattern +// stopped matching every token the package produces — which is the one thing it +// promises. kindNoun keeps its own switch (nouns are prose, not identity) and a +// test pins the two together. +var placeholderKinds = []string{"ip", "host", "profile", "hint", "user", "iface", "vpn"} + // placeholderRe matches a token this package has already minted. Any pass that // can run after another one needs it: replacing a placeholder produces a token -// in no legend and splits one identifier across two. -var placeholderRe = regexp.MustCompile(`^(?:ip|host|profile|hint|user)-\d+$`) +// in no legend and splits one identifier across two. knownNames relies on it to +// refuse a value that is spelled like a token — see json.go. +var placeholderRe = regexp.MustCompile(`^(?:` + strings.Join(placeholderKinds, "|") + `)-\d+$`) // address replaces one address-shaped match, keeping any /prefix — the prefix // length is structural (it says "this is a subnet rule"), not identifying. @@ -580,29 +664,67 @@ var keptHints = map[string]bool{ const unattributed = "_unattributed" // placeholder returns the stable token for one value, minting it on first sight. +// +// A candidate ordinal is REFUSED for two reasons, and the second is the one that +// is easy to get wrong. +// +// 1. It would produce the value itself. Config accepts `[A-Za-z0-9._-]`, so a +// profile can legitimately be CALLED `profile-1` — and minting `profile-1` +// for it left the name verbatim in the bundle while the legend said it had +// been replaced, which is the one failure this package must never have: +// advertising a safety it did not deliver. +// +// 2. It is a value this bundle already carries, under ANY kind. A token that is +// also a real name is indistinguishable from that name to a reader, and +// knownNames — which replays values without regard to kind — would rewrite +// the token as if it were the name. +// +// The counter is r.next, not a count of `order` entries, and that distinction is +// the whole bug this replaced: a refusal advances the token but appends ONE +// order entry, so a count-derived ordinal handed the vacated number to the next +// value. `profile-1` refused its way to `profile-2`, then `alpha` counted one +// entry and was also given `profile-2` — two identities, one token, and a legend +// reading `2 distinct profile names → profile-2 … profile-2`. +// +// The residual a streaming redactor cannot close: a real identifier spelled like +// a token ALREADY minted, arriving after that mint. Its own mint is safe, since +// tokens are never reissued, but the entries carrying that token are already +// written and nothing can move it retroactively. The bound that makes this +// acceptable is that the only colliding spellings are `-`, which +// name no provider and no person. func (r *Redactor) placeholder(value, kind string) string { + // A token this Redactor already produced is not a name. Text replays known + // names BEFORE the shape passes run, so `profile=mullvad-de` becomes + // `profile=profile-1` and profileAttrRe then offers `profile-1` here as if it + // were a value — minting a second token for the same identity, in no legend, + // splitting one server across two. This is the guard placeholderRe was + // written for; it belongs at the mint, the one place every pass funnels + // through, rather than at each pass. + // + // Membership, not shape: a real profile CALLED `profile-1` that this bundle + // has not already used as a token must still be replaced, which is what the + // refusal loop below is for. + if r.minted[value] { + return value + } key := kind + ":" + value if p, ok := r.seen[key]; ok { return p } - n := 0 - for _, k := range r.order { - if strings.HasPrefix(k, kind+":") { - n++ - } - } - // Skip an ordinal that would produce the value itself. Config accepts - // `[A-Za-z0-9._-]`, so a profile can legitimately be CALLED `profile-1` — - // and minting `profile-1` for it left the name verbatim in the bundle while - // the legend said it had been replaced, which is the one failure this - // package must never have: advertising a safety it did not deliver. - p := fmt.Sprintf("%s-%d", kind, n+1) - for p == value { + n := r.next[kind] + var p string + for { n++ - p = fmt.Sprintf("%s-%d", kind, n+1) + p = fmt.Sprintf("%s-%d", kind, n) + if p != value && !r.values[p] { + break + } } + r.next[kind] = n r.seen[key] = p r.order = append(r.order, key) + r.values[value] = true + r.minted[p] = true return p } @@ -641,12 +763,61 @@ func (r *Redactor) Legend() []string { out = append(out, fmt.Sprintf("1 distinct %s → %s", kindNoun(kind, n), t[0])) continue } - out = append(out, fmt.Sprintf("%d distinct %s → %s … %s", - n, kindNoun(kind, n), t[0], t[n-1])) + // A RANGE promises everything between its ends. placeholder refuses an + // ordinal whenever it would collide, so the tokens of one kind need not + // be consecutive — and `profile-1 … profile-3` for two profiles + // advertises a `profile-2` that is nowhere in the bundle, which is the + // same lie the 1..n rendering told. When there is a hole, list them. + // + // Listing, rather than dropping to a bare count: the token SPELLING is + // what a reader greps the bundle with, and teaching that spelling is + // what the range was for. The list is not unbounded in practice — a hole + // needs an identifier literally spelled `-`, impossible for + // `ip` (values are parsed addresses) and vanishingly rare for `host`, so + // this branch belongs to the small-count kinds. It discloses one thing, + // and only one: that some identifier here is spelled like a placeholder, + // a spelling that names nobody. + if consecutive(t) { + out = append(out, fmt.Sprintf("%d distinct %s → %s … %s", + n, kindNoun(kind, n), t[0], t[n-1])) + continue + } + out = append(out, fmt.Sprintf("%d distinct %s → %s", + n, kindNoun(kind, n), strings.Join(t, ", "))) } return out } +// consecutive reports whether tokens run without a gap. They arrive in mint +// order, which placeholder guarantees is increasing ordinal order for one kind, +// so only the ends have to be read. An unparseable token answers false, which +// sends the caller to the enumeration — the honest direction, since a range +// nobody can verify is exactly what this guards against. +func consecutive(tokens []string) bool { + first, ok := ordinal(tokens[0]) + if !ok { + return false + } + last, ok := ordinal(tokens[len(tokens)-1]) + if !ok { + return false + } + return last-first+1 == len(tokens) +} + +// ordinal is the number a minted token ends in. +func ordinal(token string) (int, bool) { + i := strings.LastIndexByte(token, '-') + if i < 0 { + return 0, false + } + n, err := strconv.Atoi(token[i+1:]) + if err != nil { + return 0, false + } + return n, true +} + func kindNoun(kind string, n int) string { singular, plural := "IP address", "IP addresses" switch kind { @@ -660,6 +831,8 @@ func kindNoun(kind string, n int) string { singular, plural = "interface name", "interface names" case "user": singular, plural = "account name", "account names" + case "vpn": + singular, plural = "VPN service name", "VPN service names" } if n == 1 { return singular diff --git a/internal/redact/redact_test.go b/internal/redact/redact_test.go index 7cdedfd..47d51c9 100644 --- a/internal/redact/redact_test.go +++ b/internal/redact/redact_test.go @@ -565,3 +565,117 @@ func TestAnIPv6ZoneIsRedactedWhenItNamesAProvider(t *testing.T) { t.Errorf("got %q, want a generic zone left alone", g) } } + +// Two identities must never share a token. The ordinal used to be derived by +// COUNTING order entries of a kind, so a refusal — which advances the token but +// appends one entry — vacated a number and handed it to the next value. +// `profile-1` refused its way to `profile-2`, then `alpha` counted one entry and +// was given `profile-2` as well: one token, two servers, and a legend reading +// "2 distinct profile names → profile-2 … profile-2". +func TestTwoIdentitiesNeverSharePlaceholder(t *testing.T) { + r := New(true) + first := r.placeholder("profile-1", "profile") + second := r.placeholder("alpha", "profile") + if first == second { + t.Fatalf("both values minted %q — one token for two identities", first) + } + legend := r.Legend() + if len(legend) != 1 { + t.Fatalf("legend = %v, want one line", legend) + } + for _, want := range []string{first, second} { + if !strings.Contains(legend[0], want) { + t.Errorf("legend %q does not name %q", legend[0], want) + } + } +} + +// A token must not be the spelling of a name this bundle carries. Otherwise a +// reader cannot tell the placeholder from the real profile beside it, and the +// name replay rewrites the placeholder as if it were that name. +func TestAPlaceholderIsNeverAValueTheBundleAlreadyCarries(t *testing.T) { + r := New(true) + r.placeholder("profile-3", "profile") + for _, v := range []string{"alpha", "beta", "gamma"} { + if got := r.placeholder(v, "profile"); got == "profile-3" { + t.Fatalf("%q minted %q, which is a real profile name in this bundle", v, got) + } + } +} + +// A RANGE promises everything between its ends. A refused ordinal leaves a hole, +// and "profile-1 … profile-3" for two profiles advertises a profile-2 that is +// nowhere in the bundle — the same lie the old 1..n rendering told. +func TestTheLegendNeverImpliesATokenThatWasNotMinted(t *testing.T) { + r := New(true) + r.placeholder("alpha", "profile") // profile-1 + r.placeholder("profile-2", "profile") // refuses profile-2, takes profile-3 + legend := r.Legend() + if len(legend) != 1 { + t.Fatalf("legend = %v, want one line", legend) + } + const want = "2 distinct profile names → profile-1, profile-3" + if legend[0] != want { + t.Errorf("legend[0] = %q, want %q", legend[0], want) + } + if strings.Contains(legend[0], "…") { + t.Errorf("legend %q renders a range across a hole", legend[0]) + } +} + +// placeholderRe promises it matches every token this package mints, and a pass +// that runs after another one relies on that. The alternation drifted once +// already: the iface kind was added with its own mint path and the pattern was +// left as it was. +func TestEveryKindTheRedactorMintsIsRecognisedAsAPlaceholder(t *testing.T) { + nouns := map[string]bool{} + for _, kind := range placeholderKinds { + token := kind + "-1" + if !placeholderRe.MatchString(token) { + t.Errorf("placeholderRe does not match %q", token) + } + noun := kindNoun(kind, 2) + if nouns[noun] { + t.Errorf("kind %q reuses the legend noun %q — a kind with no noun of its own falls to the default", kind, noun) + } + nouns[noun] = true + } +} + +// Text replays known names BEFORE the shape passes, so a shape pass is handed a +// token it must not treat as a value. Guarding at the mint covers every pass at +// once; guarding at each pass is how one gets missed. +// +// This guards the mechanism Text's new ordering introduced rather than an older +// defect — it passes against the shape-only Text, where no pass could ever see a +// token. It is here because the ordering is what makes the leak reachable, and +// removing the guard must not be silent. +func TestAShapePassNeverReMintsAToken(t *testing.T) { + r := New(true) + r.JSON(`{"activeProfile":"mullvad-de"}`) + got := r.Text(`profile=mullvad-de and profile=mullvad-de`) + if strings.Count(got, "profile-1") != 2 { + t.Errorf("got %q, want the same token twice", got) + } + if len(r.Legend()) != 1 || !strings.Contains(r.Legend()[0], "1 distinct") { + t.Errorf("legend = %v — the replayed token was minted a second time", r.Legend()) + } +} + +// A discarded pass must leave the ordinal counter where it was, or the legend +// counts tokens appearing nowhere in the bundle and pushes every real token's +// ordinal past them. The counter is monotonic now, so truncating order no longer +// restores it on its own. +func TestADiscardedPassLeavesTheNextOrdinalWhereItWas(t *testing.T) { + r := New(true) + mark := r.checkpoint() + r.placeholder("alpha", "profile") + r.placeholder("beta", "profile") + r.rollback(mark) + if got := r.placeholder("gamma", "profile"); got != "profile-1" { + t.Errorf("after rollback the next token is %q, want profile-1", got) + } + if legend := r.Legend(); len(legend) != 1 || !strings.Contains(legend[0], "profile-1") { + t.Errorf("legend = %v, want only the surviving token", legend) + } +} diff --git a/internal/runner/runner.go b/internal/runner/runner.go index 14e6501..905a6c3 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -420,15 +420,46 @@ type Options struct { AllowConfigOps bool } -// tunnelSnapshot maps a watcher edge to the published tunnel state. Name comes -// from the interface the watcher identified; on a down/unknown edge it carries -// no name, so fall back to the configured tunnel(s) the guard is watching. +// tunnelSnapshot maps a watcher edge to the published tunnel state: ONE entry +// per interface, each naming exactly one. Names come from the interfaces the +// watcher identified; on a down/unknown edge it identified none, so fall back to +// the configured tunnel(s) the guard is watching. +// +// One entry per interface, rather than one entry naming them all, because +// `tunnels[].name` is a redacted field in the diagnostic bundle +// (internal/redact) and a comma-joined value defeats that twice over: keepIface +// cannot see `utun4,nordlynx` as the kernel vocabulary it half is, so the +// structural half stops being kept, and the whole pair mints ONE token, so the +// bundle reports two interfaces as one identity. +// +// Detail does not repeat the names. It is the same word one key over, landing +// where only a literal pass can reach it, and Name already carries it. The +// watcher's own Detail is untouched — it is documented as a short human reason +// for logs, and the runner logs it. func tunnelSnapshot(st netdetect.TunnelState, tunnels []string) []state.Tunnel { - name := st.Name - if name == "" { - name = strings.Join(tunnels, ",") + names := st.Names + if len(names) == 0 && st.Name != "" { + names = []string{st.Name} + } + if len(names) == 0 { + names = tunnels + } + if len(names) == 0 { + // Nothing configured and nothing observed: keep one entry so a consumer + // reading tunnels[0] still sees the up/down answer. + return []state.Tunnel{{Up: st.Up, Detail: st.Detail}} } - return []state.Tunnel{{Name: name, Up: st.Up, Detail: st.Detail}} + // Up: "up". Down: the watcher's reason, which names no interface + // ("no configured tunnel is up"). + detail := st.Detail + if st.Up { + detail = "up" + } + out := make([]state.Tunnel, 0, len(names)) + for _, n := range names { + out = append(out, state.Tunnel{Name: n, Up: st.Up, Detail: detail}) + } + return out } // postureName maps (blocked, window, standby) to the snapshot's posture string. diff --git a/internal/runner/tunnelsnapshot_test.go b/internal/runner/tunnelsnapshot_test.go new file mode 100644 index 0000000..938fd78 --- /dev/null +++ b/internal/runner/tunnelsnapshot_test.go @@ -0,0 +1,82 @@ +package runner + +import ( + "strings" + "testing" + + "github.com/behnam-rk/dezhban/internal/netdetect" +) + +// state.json's tunnels[].name is a field the diagnostic bundle redacts BY KEY, +// and a comma-joined value defeats that twice over: keepIface cannot see +// "utun4,nordlynx" as the kernel vocabulary it half is, so the structural half +// stops being kept, and the whole pair mints ONE token, so the bundle reports +// two interfaces as one identity. +func TestEachPublishedTunnelNamesExactlyOneInterface(t *testing.T) { + for _, tc := range []struct { + name string + st netdetect.TunnelState + cfg []string + want []string + }{ + { + name: "a down edge falls back to every configured tunnel", + st: netdetect.TunnelState{Up: false, Detail: "no configured tunnel is up"}, + cfg: []string{"utun4", "nordlynx"}, + want: []string{"utun4", "nordlynx"}, + }, + { + name: "an up edge publishes every interface the watcher saw", + st: netdetect.TunnelState{ + Up: true, Name: "nordlynx", Names: []string{"nordlynx", "utun4"}, + Detail: "nordlynx,utun4 up", + }, + cfg: []string{"utun4", "nordlynx"}, + want: []string{"nordlynx", "utun4"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + got := tunnelSnapshot(tc.st, tc.cfg) + if len(got) != len(tc.want) { + t.Fatalf("published %d entries, want %d: %+v", len(got), len(tc.want), got) + } + for i, tun := range got { + if strings.ContainsAny(tun.Name, ", ") { + t.Errorf("entry %d names more than one interface: %q", i, tun.Name) + } + if tun.Name != tc.want[i] { + t.Errorf("entry %d name = %q, want %q", i, tun.Name, tc.want[i]) + } + } + }) + } +} + +// Detail used to repeat the interface name that Name already carries — the same +// word one key over, landing where only a literal pass over free text can reach +// it. The watcher's own Detail is untouched; this is about what gets published. +func TestAPublishedTunnelDoesNotRepeatItsOwnNameInItsDetail(t *testing.T) { + st := netdetect.TunnelState{ + Up: true, Name: "nordlynx", Names: []string{"nordlynx"}, Detail: "nordlynx up", + } + for _, tun := range tunnelSnapshot(st, []string{"nordlynx"}) { + if strings.Contains(tun.Detail, "nordlynx") { + t.Errorf("detail %q repeats the interface name", tun.Detail) + } + if tun.Detail == "" { + t.Error("the up/down answer was dropped along with the name") + } + } +} + +// A host with nothing configured and nothing observed still gets one entry, so a +// consumer reading tunnels[0] sees the up/down answer rather than an empty list. +func TestATunnelSnapshotIsNeverEmpty(t *testing.T) { + got := tunnelSnapshot(netdetect.TunnelState{Up: false, Detail: "no tunnel interface is up"}, nil) + if len(got) != 1 { + t.Fatalf("published %d entries, want 1: %+v", len(got), got) + } + if got[0].Name != "" || got[0].Up { + t.Errorf("entry = %+v, want an unnamed down entry", got[0]) + } +} From 69410026fd28aa2386ed927ef5697f4c5c97135b Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sun, 13 Sep 2026 10:40:27 +0330 Subject: [PATCH 2/5] fix(diag): harden the redactor's mint guard and pin what the loop changed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review loop over the previous commit. Three defects in its own new code, none in the branch's behaviour, plus two tests that did not bite. rollback assigned the checkpoint's own map, so the Redactor mutated it on the next mint and the checkpoint stopped describing the state it was taken at — a second rollback from it restored whatever had happened since. Copied now. The Overview row paired the up/down word with a tunnel detail that no longer repeats the interface name, so it rendered "up (up)", and it read `.first`, which hides every interface but one now that state.json publishes one entry each. PostureUI.tunnelSummary owns that reading, in Core where it is testable. TestTheInterfaceAttrKeepsItsKeyAndItsQuoting passed against the old two-group pattern too, so the `tunnel=` removal was pinned by nothing. Re-adding the alternative now fails by name: `tunnel=something-else` comes back `iface=iface-2`, which is both the over-claim and the silently wrong key. The mint guard stays CROSS-KIND, and that is now written down where someone about to "tighten" it will read it. Scoping it by kind was proposed and is a regression: a name that is both a profile and an endpoint is replayed as `profile-1` into `host=`, and a kind-scoped guard mints `host-2` for it — a token standing for a token, and a legend counting a hostname that is nowhere in the bundle. TestAReplayedTokenIsNotReMintedUnderAnotherKind fails against it. Also: the residual in the package comment now says the collision holds across kinds, which is what made it read as covering only one; rollback's rebuild states the order/seen invariant and refuses an empty token, since "" in minted would make placeholder return early and ship the next empty value unredacted; and doctorDetail.line is pinned against Swift's DoctorDetail.line by the same ten-case table on both sides, so two renderers cannot drift. Verified: task check, swift test (270, was 265), and each new test run against the code it guards to confirm it fails there. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/dezhban/doctor_test.go | 27 +++++++++++ gui/macos/Sources/DezhbanCore/PostureUI.swift | 21 +++++++++ .../Sources/DezhbanMenu/OverviewView.swift | 9 +++- .../DezhbanCoreTests/DoctorReportTests.swift | 26 ++++++++++ .../DezhbanCoreTests/PostureUITests.swift | 33 +++++++++++++ internal/redact/json.go | 18 ++++++- internal/redact/json_test.go | 28 +++++++++++ internal/redact/redact.go | 22 +++++++-- internal/redact/redact_test.go | 47 +++++++++++++++++++ 9 files changed, 223 insertions(+), 8 deletions(-) diff --git a/cmd/dezhban/doctor_test.go b/cmd/dezhban/doctor_test.go index 50dd1f2..6283a1b 100644 --- a/cmd/dezhban/doctor_test.go +++ b/cmd/dezhban/doctor_test.go @@ -758,3 +758,30 @@ func TestTheDiscoverCheckDoesNotNameTheVPNClientInAnyDetailLine(t *testing.T) { t.Errorf("details = %+v, want one per candidate", c.Details) } } + +// Two renderers compose this sentence — printDoctor here, DoctorDetail.line in +// gui/macos/Sources/DezhbanCore/DoctorReport.swift — and they must never +// disagree about what a check found. The table is duplicated verbatim in +// DoctorReportTests.swift; changing one without the other is the drift this +// pins. +func TestTheComposedDetailLineIsTheSameOnBothSides(t *testing.T) { + for _, tc := range []struct { + d doctorDetail + want string + }{ + {doctorDetail{Text: ""}, ""}, + {doctorDetail{Text: "plain prose"}, "plain prose"}, + {doctorDetail{Iface: "nordlynx", Text: "— no subnet"}, "nordlynx — no subnet"}, + {doctorDetail{Iface: "nordlynx"}, "nordlynx"}, + {doctorDetail{Profile: "work-nord", Text: "— 2 stored"}, "work-nord — 2 stored"}, + {doctorDetail{Endpoint: "1.2.3.4", Text: ":51820"}, "1.2.3.4:51820"}, + {doctorDetail{Iface: "utun4", Endpoint: "1.2.3.4", Text: "— MISCONFIGURED"}, "1.2.3.4 — MISCONFIGURED (utun4)"}, + {doctorDetail{Iface: "utun4", Profile: "p", Endpoint: "1.2.3.4", Text: "— x"}, "1.2.3.4 — x (utun4)"}, + {doctorDetail{Iface: "utun4", Profile: "p", Text: "— x"}, "p — x (utun4)"}, + {doctorDetail{Iface: "utun4", Text: ":51820"}, "utun4:51820"}, + } { + if got := tc.d.line(); got != tc.want { + t.Errorf("line(%+v) = %q, want %q", tc.d, got, tc.want) + } + } +} diff --git a/gui/macos/Sources/DezhbanCore/PostureUI.swift b/gui/macos/Sources/DezhbanCore/PostureUI.swift index 0f9e0c1..32333c4 100644 --- a/gui/macos/Sources/DezhbanCore/PostureUI.swift +++ b/gui/macos/Sources/DezhbanCore/PostureUI.swift @@ -159,6 +159,27 @@ public enum PostureUI { return false } + /// The Overview's one-line tunnel summary: every interface named, with its + /// state, in the order the daemon published them. + /// + /// Here rather than in the view because there is one right reading of the + /// list and it is easy to get two. `state.json` publishes ONE ENTRY PER + /// INTERFACE, so a view taking `.first` names one and silently hides the + /// rest on a multi-tunnel host; and a tunnel's `detail` no longer repeats + /// the name that its own `name` carries, so pairing the two rendered as + /// "up (up)". + /// + /// An entry with no name is what a host with nothing configured and nothing + /// observed publishes — it still carries the up/down answer, which is the + /// part this row exists for. + public static func tunnelSummary(_ tunnels: [Tunnel]) -> String { + tunnels.map { t in + let state = t.up ? "up" : "down" + guard let name = t.name, !name.isEmpty else { return state } + return "\(name) — \(state)" + }.joined(separator: ", ") + } + /// SwiftUI accent for a brand state — used where the bundled bitmap isn't /// (SF Symbol fallback, text highlights). public static func color(for state: String) -> Color { diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index 80b8081..71839da 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -231,8 +231,13 @@ struct OverviewView: View { if let preset = state.presetLabel { row("Strictness", preset) } - if let t = s.tunnels?.first { - row("Tunnel", "\(t.up ? "up" : "down")\(t.detail.map { " (\($0))" } ?? "")") + if let tuns = s.tunnels, !tuns.isEmpty { + // The NAME, then the state. Each entry names exactly one + // interface now (state.json publishes one per interface), so + // `.first` would hide the rest on a multi-tunnel host — and + // `detail` no longer repeats the name, so pairing it with the + // up/down word read as "up (up)". + row("Tunnel", PostureUI.tunnelSummary(tuns)) } if let app = state.vpnInventory?.connectedName { row("VPN app", app) diff --git a/gui/macos/Tests/DezhbanCoreTests/DoctorReportTests.swift b/gui/macos/Tests/DezhbanCoreTests/DoctorReportTests.swift index 2716201..a2dba71 100644 --- a/gui/macos/Tests/DezhbanCoreTests/DoctorReportTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/DoctorReportTests.swift @@ -99,3 +99,29 @@ struct DoctorReportTests { #expect(DoctorReport.decode("not json".data(using: .utf8)!) == nil) } } + +extension DoctorReportTests { + /// The same table as Go's TestTheComposedDetailLineIsTheSameOnBothSides in + /// cmd/dezhban/doctor_test.go. Two renderers compose this sentence and they + /// must never disagree about what a check found; changing one side without + /// the other is the drift this pins. + @Test func theComposedDetailLineIsTheSameOnBothSides() { + let cases: [(DoctorDetail, String)] = [ + (DoctorDetail(text: ""), ""), + (DoctorDetail(text: "plain prose"), "plain prose"), + (DoctorDetail(iface: "nordlynx", text: "— no subnet"), "nordlynx — no subnet"), + (DoctorDetail(iface: "nordlynx", text: ""), "nordlynx"), + (DoctorDetail(profile: "work-nord", text: "— 2 stored"), "work-nord — 2 stored"), + (DoctorDetail(endpoint: "1.2.3.4", text: ":51820"), "1.2.3.4:51820"), + (DoctorDetail(iface: "utun4", endpoint: "1.2.3.4", text: "— MISCONFIGURED"), + "1.2.3.4 — MISCONFIGURED (utun4)"), + (DoctorDetail(iface: "utun4", profile: "p", endpoint: "1.2.3.4", text: "— x"), + "1.2.3.4 — x (utun4)"), + (DoctorDetail(iface: "utun4", profile: "p", text: "— x"), "p — x (utun4)"), + (DoctorDetail(iface: "utun4", text: ":51820"), "utun4:51820"), + ] + for (detail, want) in cases { + #expect(detail.line == want) + } + } +} diff --git a/gui/macos/Tests/DezhbanCoreTests/PostureUITests.swift b/gui/macos/Tests/DezhbanCoreTests/PostureUITests.swift index 4a63660..d3bf96c 100644 --- a/gui/macos/Tests/DezhbanCoreTests/PostureUITests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/PostureUITests.swift @@ -272,3 +272,36 @@ struct PostureUITests { #expect(PostureUI.agoString(125) == "2m ago") } } + +extension PostureUITests { + /// state.json publishes one entry per interface, so a summary taking only + /// the first names one and hides the rest. + @Test func theTunnelSummaryNamesEveryInterface() { + let tuns = [ + Tunnel(name: "utun4", up: true, detail: "up"), + Tunnel(name: "nordlynx", up: true, detail: "up"), + ] + #expect(PostureUI.tunnelSummary(tuns) == "utun4 — up, nordlynx — up") + } + + /// A tunnel's detail no longer repeats the name its own `name` carries, so + /// pairing the two rendered as "up (up)". + @Test func theTunnelSummaryNeverRepeatsTheState() { + let s = PostureUI.tunnelSummary([Tunnel(name: "utun4", up: false, detail: "no configured tunnel is up")]) + #expect(s == "utun4 — down") + } + + /// The view guards on `!tuns.isEmpty`, but this is a public helper and the + /// guard is one caller's choice — an empty list must produce an empty string + /// rather than a stray separator, so a future caller that skips the guard + /// renders nothing rather than something wrong. + @Test func anEmptyTunnelListSummarisesToNothing() { + #expect(PostureUI.tunnelSummary([]).isEmpty) + } + + /// Nothing configured and nothing observed still carries the up/down answer, + /// which is the part the row exists for. + @Test func anUnnamedTunnelStillReportsItsState() { + #expect(PostureUI.tunnelSummary([Tunnel(name: nil, up: false, detail: "no tunnel interface is up")]) == "down") + } +} diff --git a/internal/redact/json.go b/internal/redact/json.go index b2b609d..7c20ee2 100644 --- a/internal/redact/json.go +++ b/internal/redact/json.go @@ -93,16 +93,30 @@ func (r *Redactor) rollback(c checkpoint) { delete(r.seen, key) } r.order = r.order[:c.order] - r.next = c.next + // COPIED, not aliased. Assigning the checkpoint's own map would leave this + // Redactor mutating it on the next mint, so the checkpoint would no longer + // describe the state it was taken at — and a second rollback from it would + // restore whatever had happened since. + r.next = make(map[string]int, len(c.next)) + for k, v := range c.next { + r.next[k] = v + } // values is rebuilt rather than pruned per key: one value can be keyed under // two kinds, so deleting it for the discarded key would forget it for the // surviving one. This runs at most once per bundle entry. + // Every key in order has a seen entry — placeholder appends to one and writes + // the other in the same breath — so the lookup below cannot come back empty. + // Said out loud because an empty token here would poison minted with "", and + // placeholder returns early on anything minted: the next empty value would + // come back unredacted. r.values = make(map[string]bool, len(r.order)) r.minted = make(map[string]bool, len(r.order)) for _, key := range r.order { _, value, _ := strings.Cut(key, ":") r.values[value] = true - r.minted[r.seen[key]] = true + if token := r.seen[key]; token != "" { + r.minted[token] = true + } } } diff --git a/internal/redact/json_test.go b/internal/redact/json_test.go index 9f08c2a..c9d2e88 100644 --- a/internal/redact/json_test.go +++ b/internal/redact/json_test.go @@ -2,6 +2,7 @@ package redact import ( "encoding/json" + "slices" "strconv" "strings" "testing" @@ -536,3 +537,30 @@ func TestAConnectedVPNNameIsRedacted(t *testing.T) { t.Errorf("legend = %v — the kind has no noun of its own", legend) } } + +// The mint guard is CROSS-KIND, and a review proposed scoping it to the kind as +// a tightening. It is the opposite, and this is the case that shows it. +// +// `work-nord` is both a profile name and an endpoint, so it holds two tokens. +// Text replays one of them into the log line, and endpointAttrRe then offers +// that token back under the `host` kind. Cross-kind, the guard recognises it and +// the line keeps the token it was given. Kind-scoped, `host` has not minted that +// spelling, so a SECOND token is minted for the first one — the bundle carries a +// token standing for a token, and the legend counts a hostname that is nowhere +// in it. +func TestAReplayedTokenIsNotReMintedUnderAnotherKind(t *testing.T) { + r := New(true) + r.JSON(`{"vpn":{"profiles":[{"name":"work-nord"}],"endpoints":["work-nord"]}}`) + before := append([]string(nil), r.Legend()...) + + got := r.Text(`level=WARN host=work-nord msg="resolve failed"`) + if strings.Contains(got, "work-nord") { + t.Fatalf("the name survived: %q", got) + } + if strings.Contains(got, "host-2") { + t.Errorf("got %q — a token was minted for a token", got) + } + if after := r.Legend(); !slices.Equal(before, after) { + t.Errorf("the legend grew replaying a name it already knew:\n before: %v\n after: %v", before, after) + } +} diff --git a/internal/redact/redact.go b/internal/redact/redact.go index de694ce..0e8c35b 100644 --- a/internal/redact/redact.go +++ b/internal/redact/redact.go @@ -31,9 +31,12 @@ // limits have to be written down as plainly as the coverage. // // 1. A real identifier SPELLED like an already-minted token, arriving after that -// mint, keeps its spelling — see placeholder. Nothing can move a token that -// earlier entries already carry. The bound: only `-` can -// collide, and that spelling names no provider and no person. +// mint, keeps its spelling — see placeholder. This holds ACROSS KINDS, not +// only within one: an interface literally called `profile-1` ships as +// `profile-1` once that token exists. Nothing can move a token earlier +// entries already carry, and the alternative is worse — see the guard in +// placeholder, which must stay cross-kind. The bound: only `-` +// can collide, and that spelling names no provider and no person. // // 2. An interface known ONLY to the live host — autodetect with no // vpn.tunnelInterfaces, and no daemon, so no state.json — is minted by @@ -701,9 +704,20 @@ func (r *Redactor) placeholder(value, kind string) string { // written for; it belongs at the mint, the one place every pass funnels // through, rather than at each pass. // + // CROSS-KIND, and it must stay that way. Scoping it to the kind looks tighter + // and is the opposite: the replay writes a token without regard to kind, and + // the pass that then offers it back is whichever pass owns the field it + // landed in. A name that is both a profile and an endpoint is replayed as + // `profile-1` into `host=…`, where endpointAttrRe offers it under `host` — a + // kind-scoped guard mints `host-2` for it, so the bundle carries a token + // standing for a token and the legend counts a hostname that exists nowhere. + // Pinned by TestAReplayedTokenIsNotReMintedUnderAnotherKind. + // // Membership, not shape: a real profile CALLED `profile-1` that this bundle // has not already used as a token must still be replaced, which is what the - // refusal loop below is for. + // refusal loop below is for. The reverse — a real name spelled like a token + // that DOES already exist — keeps its spelling, and is residual 1 in this + // package's doc comment rather than a case this can close. if r.minted[value] { return value } diff --git a/internal/redact/redact_test.go b/internal/redact/redact_test.go index 47d51c9..26d4c3f 100644 --- a/internal/redact/redact_test.go +++ b/internal/redact/redact_test.go @@ -679,3 +679,50 @@ func TestADiscardedPassLeavesTheNextOrdinalWhereItWas(t *testing.T) { t.Errorf("legend = %v, want only the surviving token", legend) } } + +// A checkpoint has to keep describing the state it was taken at. rollback used +// to assign the checkpoint's own map, so the Redactor then mutated it on every +// later mint and a second rollback restored whatever had happened since. +func TestACheckpointIsNotMutatedByWhatFollowsIt(t *testing.T) { + r := New(true) + mark := r.checkpoint() + r.placeholder("alpha", "profile") + r.rollback(mark) + + r.placeholder("beta", "profile") + r.placeholder("gamma", "profile") + r.rollback(mark) + + if got := r.placeholder("delta", "profile"); got != "profile-1" { + t.Errorf("after rolling back to the same mark twice the next token is %q, want profile-1", got) + } +} + +// The attr pass carries exactly one capture group now that `tunnel=` is gone, +// and getting the renumbering half-right compiles: the old indexing against the +// new pattern reads a group that is not there. Pin the exact output, including +// that quotes survive only when the original had them and that the kernel's own +// names are left alone. +// +// And pin the REMOVAL. `tunnel=` had no producer anywhere in the tree, so this +// pass no longer claims it — without a case saying so, dropping the alternative +// is invisible to the suite and adding it back is free. A name in a +// `tunnel=` attr this bundle already knows is still replaced, by the name +// replay rather than by a pattern matching a key nothing writes. +func TestTheInterfaceAttrKeepsItsKeyAndItsQuoting(t *testing.T) { + r := New(true) + for _, tc := range []struct{ in, want string }{ + {`iface="nordlynx"`, `iface="iface-1"`}, + {`iface=nordlynx`, `iface=iface-1`}, + {`iface="utun4"`, `iface="utun4"`}, + {`iface=utun4`, `iface=utun4`}, + // Not an attr this daemon writes, so the pattern does not claim it. + {`tunnel=something-else`, `tunnel=something-else`}, + // But a name the bundle already knows still goes, via the replay. + {`tunnel=nordlynx`, `tunnel=iface-1`}, + } { + if got := r.Text(tc.in); got != tc.want { + t.Errorf("Text(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} From 091f9619938ccc35b3a8663bfcfb8810b45688f3 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sun, 13 Sep 2026 10:41:00 +0330 Subject: [PATCH 3/5] docs(diag): record why the mint guard is cross-kind, and the Overview fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The decision the review loop settled, in the record rather than only in a code comment — it reads as a thing to tighten, and the next reader will otherwise re-derive the regression from scratch. Plus the CHANGELOG bullet for the Overview row, which is user-visible and was missing. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 4 +++- docs/adr/0016-bundle-identifiers-are-data-not-prose.md | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7deaf51..a76acfc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,7 +54,9 @@ current as you land changes. after each one. See [ADR-0016](docs/adr/0016-bundle-identifiers-are-data-not-prose.md). - **`status --json` and `state.json` publish one entry per tunnel interface**, each naming exactly one, and a tunnel's `detail` no longer repeats the name - already in its `name`. + already in its `name`. The app's Overview names every tunnel interface and its + state — it used to show only the first, and to pair the state with a detail + that repeated it ("up (up)"). ## [0.14.0] - 2026-09-10 diff --git a/docs/adr/0016-bundle-identifiers-are-data-not-prose.md b/docs/adr/0016-bundle-identifiers-are-data-not-prose.md index a136504..0ee5c98 100644 --- a/docs/adr/0016-bundle-identifiers-are-data-not-prose.md +++ b/docs/adr/0016-bundle-identifiers-are-data-not-prose.md @@ -99,6 +99,16 @@ text we do not author: rendered rulesets, log records, OS error strings. `wireguard` — the words an interface can legitimately be called — and by minting only names that fail `keepIface`, so the kernel's vocabulary never enters the replay at all. +- **The mint guard that makes the replay safe is cross-kind, and looks wrong.** + A pass that runs after the replay can be handed a token, so `placeholder` + returns a value unchanged when that value is a token it already minted — and + it does so regardless of which kind is asking. Scoping it to the kind reads as + a tightening and is a regression: a name that is both a profile and an + endpoint is replayed as `profile-1` into `host=…`, where the endpoint pass + offers it back under `host`, and a kind-scoped guard mints `host-2` for it — + a token standing for a token, and a legend counting a hostname that is nowhere + in the bundle. This was proposed during review and rejected on that evidence. + Pinned by `TestAReplayedTokenIsNotReMintedUnderAnotherKind`. - **The bundle's collection order stays load-bearing.** The replay can only replace what some earlier entry taught it, so `config.json` and `learned.json` must still be collected before `doctor.json` and `state.json`. Pinned by From 60ec5b06ede8e7f8ddf5cde09b003f7173ea84a0 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sun, 13 Sep 2026 10:49:30 +0330 Subject: [PATCH 4/5] fix(diag): stop a rollback guard from doubting an invariant that holds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 of the review loop, over the slice the loop itself wrote after round 2 read the tree. No defect in the branch; two in the loop's own output. The rebuild in rollback claimed the seen lookup "cannot come back empty" and then guarded against exactly that. Both cannot be true. placeholder is the only place that appends to order and it writes seen in the same breath, so the invariant is absolute and the guard was dead code reading as doubt about it — and it took the wrong branch for the case it imagined: skipping a token leaves it out of minted, and a token not in minted is one a later pass mints a second token for. That is the split-identifier failure placeholderRe exists to prevent. Nothing covered the minted/values half of the rebuild at all. Deleting it now fails by name: the surviving token is re-minted as profile-2 and the legend counts three identities where the bundle carries two. And the `tunnel=nordlynx` case passed only because an earlier row of its own table had minted iface-1 first — reordering the rows would have changed what it asserted without changing how it read. Split out, with the mint seeded explicitly and the fresh-Redactor half added, so the removal of `tunnel=` is pinned from both directions. Not fixed, deliberately: name("") mints a token for an empty string when called directly. Unreachable through JSON — the walk leaves empty values alone — and pre-existing, since name is untouched here. A round that grows its own scope is the failure this loop guards against. Verified: task check, swift test (270), and each test run against the code it guards to confirm it fails there. Co-Authored-By: Claude Opus 5 (1M context) --- internal/redact/json.go | 16 +++++------ internal/redact/redact_test.go | 50 ++++++++++++++++++++++++++++++++-- 2 files changed, 56 insertions(+), 10 deletions(-) diff --git a/internal/redact/json.go b/internal/redact/json.go index 7c20ee2..09bc25e 100644 --- a/internal/redact/json.go +++ b/internal/redact/json.go @@ -104,19 +104,19 @@ func (r *Redactor) rollback(c checkpoint) { // values is rebuilt rather than pruned per key: one value can be keyed under // two kinds, so deleting it for the discarded key would forget it for the // surviving one. This runs at most once per bundle entry. - // Every key in order has a seen entry — placeholder appends to one and writes - // the other in the same breath — so the lookup below cannot come back empty. - // Said out loud because an empty token here would poison minted with "", and - // placeholder returns early on anything minted: the next empty value would - // come back unredacted. + // Every key in order has a seen entry, unconditionally: placeholder is the + // ONLY place that appends to order, and it writes seen in the same breath. + // So the lookup cannot come back empty, and there is deliberately no guard + // against it — a guard here would read as doubt about an invariant that is + // absolute, and it would take the wrong branch if it ever were not: skipping + // a token leaves it out of minted, and a token not in minted is one a later + // pass will mint a second token for. r.values = make(map[string]bool, len(r.order)) r.minted = make(map[string]bool, len(r.order)) for _, key := range r.order { _, value, _ := strings.Cut(key, ":") r.values[value] = true - if token := r.seen[key]; token != "" { - r.minted[token] = true - } + r.minted[r.seen[key]] = true } } diff --git a/internal/redact/redact_test.go b/internal/redact/redact_test.go index 26d4c3f..f711bce 100644 --- a/internal/redact/redact_test.go +++ b/internal/redact/redact_test.go @@ -718,11 +718,57 @@ func TestTheInterfaceAttrKeepsItsKeyAndItsQuoting(t *testing.T) { {`iface=utun4`, `iface=utun4`}, // Not an attr this daemon writes, so the pattern does not claim it. {`tunnel=something-else`, `tunnel=something-else`}, - // But a name the bundle already knows still goes, via the replay. - {`tunnel=nordlynx`, `tunnel=iface-1`}, } { if got := r.Text(tc.in); got != tc.want { t.Errorf("Text(%q) = %q, want %q", tc.in, got, tc.want) } } } + +// Dropping `tunnel=` from the pattern is not a coverage loss, because a name the +// bundle already knows is reached by the name replay rather than by a pattern +// matching a key nothing writes. +// +// Its own test, with the mint SEEDED explicitly, because the precondition is the +// whole point: as one more row of the table above it passed only because an +// earlier row happened to have minted first, and reordering the rows would have +// changed what it asserted without changing what it looked like. +func TestAKnownInterfaceInAnUnwrittenAttrIsStillReplaced(t *testing.T) { + r := New(true) + r.JSON(`{"vpn":{"tunnelInterfaces":["nordlynx"]}}`) + + if got := r.Text(`tunnel=nordlynx`); got != `tunnel=iface-1` { + t.Errorf("Text(`tunnel=nordlynx`) = %q, want `tunnel=iface-1`", got) + } + // And with nothing minted, it is left alone — the pattern really is gone. + if got := New(true).Text(`tunnel=nordlynx`); got != `tunnel=nordlynx` { + t.Errorf("Text(`tunnel=nordlynx`) on a fresh Redactor = %q, want it untouched", got) + } +} + +// rollback restores every map placeholder writes, not just the counter. `minted` +// is the one that matters most: a token missing from it is one a later pass will +// mint a second token for, which is the split-identifier failure placeholderRe +// exists to prevent — and it would show up nowhere until a bundle carried both. +func TestADiscardedPassRestoresTheMintedTokens(t *testing.T) { + r := New(true) + kept := r.placeholder("alpha", "profile") + + mark := r.checkpoint() + discarded := r.placeholder("beta", "profile") + r.rollback(mark) + + // The surviving token is still recognised, so a pass handed it back does not + // mint again. + if got := r.placeholder(kept, "profile"); got != kept { + t.Errorf("placeholder(%q) = %q — the surviving token was re-minted", kept, got) + } + // The discarded one is forgotten: its value may be keyed afresh, and its + // ordinal is free again. + if got := r.placeholder("beta", "profile"); got != discarded { + t.Errorf("after rollback, %q minted %q, want the ordinal back at %q", "beta", got, discarded) + } + if legend := r.Legend(); len(legend) != 1 || !strings.Contains(legend[0], "2 distinct") { + t.Errorf("legend = %v, want exactly the two surviving identities", legend) + } +} From fa06a341a8baae9871a71009cba4b6a8def3ed83 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sun, 13 Sep 2026 11:02:25 +0330 Subject: [PATCH 5/5] docs(diag): stop the bundle's own page promising a safety it does not deliver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 of the review loop, over a dimension the first three never touched: whether the prose this branch ships is true of the code it ships. One finding, in the page that is compiled into the macOS app and read offline. docs/usage/cli.md claimed "a token is never the spelling of a name in the bundle, and no two identities ever share a token". The second half is enforced. The first is false in the case this package already documents as a residual: a name spelled like a token ALREADY MINTED keeps its spelling, because the token is in entries that are written. An interface called `profile-1`, on a host where `profile-1` is some profile's placeholder, ships verbatim — and the legend beside it says `profile-1` is a redacted profile name, so the one thing a reader can do with it is read an interface name as a redaction. That is worse than saying nothing, and it is exactly the failure this package's own rule names: it must never claim to have redacted something it did not. The page now states the guarantees that hold — no two identities share a token, and a token is never the name it replaced — and then states the case that is not covered, including what to do about it (do not name things `kind-number`). Pinned by TestANameSpelledLikeAnExistingTokenKeepsItsSpelling, so the residual cannot drift away from the two documents that now describe it. Pinned rather than only written down because the fix that suggests itself — scoping the mint guard by kind — is the regression an earlier round already rejected. Verified: task check, swift test (270 — this round added a Go test, not a Swift one), go test ./... clean. Co-Authored-By: Claude Opus 5 (1M context) --- docs/usage/cli.md | 14 +++++++++++--- internal/redact/json_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/docs/usage/cli.md b/docs/usage/cli.md index fd6b956..ce8ca47 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -505,10 +505,18 @@ The README's legend reports counts and the tokens they cover — a range ("23 distinct IP addresses → ip-1 … ip-23") when a kind's tokens run consecutively, and a list ("2 distinct profile names → profile-1, profile-3") when one number was skipped. A number is skipped whenever it would have produced -a name this bundle actually carries: a profile may legitimately be *called* +a name this bundle already carries: a profile may legitimately be *called* `profile-1`, and a placeholder that is also a real name is no redaction at all. -That is the same rule read twice — a token is never the spelling of a name in -the bundle, and no two identities ever share a token. +Two things follow, and they are guarantees: no two identities ever share a token, +and a token is never the name it replaced. + +One case is **not** covered, and it is stated rather than implied. A name that is +spelled like a token *already minted* keeps its spelling — an interface called +`profile-1`, on a host where `profile-1` is already some profile's placeholder, +stays `profile-1` in the bundle. Nothing can move a token that earlier entries +already carry. It is left this way because the only spellings that can collide +are `kind-number`, which name no provider and no person; if you have deliberately +named something that way, know that it reads as a placeholder. The bundle is written **0600**, so the `--include-network` version is not readable by other accounts on the machine. `--include-network` produces that diff --git a/internal/redact/json_test.go b/internal/redact/json_test.go index c9d2e88..5ff8dcd 100644 --- a/internal/redact/json_test.go +++ b/internal/redact/json_test.go @@ -564,3 +564,28 @@ func TestAReplayedTokenIsNotReMintedUnderAnotherKind(t *testing.T) { t.Errorf("the legend grew replaying a name it already knew:\n before: %v\n after: %v", before, after) } } + +// The one case the redaction does NOT cover, pinned so it stays true of the docs +// that now state it (docs/usage/cli.md, and residual 1 in this package's doc +// comment). A name spelled like a token ALREADY MINTED keeps its spelling: the +// token is in entries that are already written, and nothing can move it. +// +// Pinned rather than merely documented, because this is the shape of thing that +// gets "fixed" by someone who has not read why — and the fix that suggests +// itself, making the mint guard kind-scoped, is the regression +// TestAReplayedTokenIsNotReMintedUnderAnotherKind rejects. +func TestANameSpelledLikeAnExistingTokenKeepsItsSpelling(t *testing.T) { + r := New(true) + r.JSON(`{"vpn":{"profiles":[{"name":"alpha"}]}}`) // mints profile-1 + + got := r.JSON(`{"vpn":{"tunnelInterfaces":["profile-1"]}}`) + if !strings.Contains(got, `"profile-1"`) { + t.Errorf("got %q — the documented residual no longer holds; docs/usage/cli.md says it does", got) + } + // And the bundle does not grow a second identity for it: the legend still + // names exactly the one profile. + legend := r.Legend() + if len(legend) != 1 || !strings.Contains(legend[0], "1 distinct profile name") { + t.Errorf("legend = %v, want the single profile and no interface kind", legend) + } +}