diff --git a/CHANGELOG.md b/CHANGELOG.md index 531b867..e55e8f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,21 @@ current as you land changes. ## [Unreleased] +### Fixed + +- **A redacted bundle no longer replaces dezhban's own file paths with a hostname + token.** `doctor`'s control check names the socket it probed, and that path came + back as `/var/db/dezhban/host-1` — the answer replaced, and no identity hidden, + which is the wrong direction twice over. The run lock's path had the same + problem wherever a startup failure carried it into the log. Both survive now, + and the legend no longer counts a hostname that stands for a filename. + Those two names exactly, not the file endings: `control.socket` is a config + key, so a socket you renamed — `/var/run/nordvpn.sock` — is still redacted, + because that name is yours and it states the provider. The bundle's own `.zip` + name is still redacted for the same family of reason: `.zip` is a real + top-level domain, so admitting it as a file ending would wave a genuine + hostname through. + ## [0.15.0] - 2026-09-13 ### Fixed diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index e1e829c..5f8d165 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -1419,7 +1419,10 @@ end up typing a password. `doctor.json`'s check names (`config`, `tunnels`, `endpoints`, `lockout`, …), the shipped geo-provider hostnames, the posture strings (`guard`, `full-block`, `switch-window`, `standby`), the mode names in - rules-preview.txt, and every `utun*`/`lo0`. If the check names read as + rules-preview.txt, every `utun*`/`lo0`, and the paths of dezhban's OWN + files — the control check names the socket it probed + (`…/control.sock`), and a path reading as `…/host-N` has had the answer + replaced rather than an identity. If the check names read as `profile-N`, the redactor has replaced the answer rather than the identity, and the legend is overcounting to match. - [ ] **Every JSON entry still opens.** `for f in *.json; do python3 -m json.tool diff --git a/internal/redact/redact.go b/internal/redact/redact.go index 0e8c35b..7a6c89b 100644 --- a/internal/redact/redact.go +++ b/internal/redact/redact.go @@ -533,7 +533,7 @@ func (r *Redactor) host(m string) string { // merely look like hostnames. func keepHost(m string) bool { lower := strings.ToLower(m) - if allowedHosts[lower] { + if allowedHosts[lower] || keptNames[lower] { return true } for _, suffix := range keptSuffixes { @@ -582,12 +582,35 @@ var allowedHosts = map[string]bool{ // This is a deny-list embedded in an allow-listed matcher, so it is kept as // short as those two rules allow. Redacting a filename is noise; keeping one is // a leak. +// `.zip` is NOT here and must not be added, though the report bundle's own +// filename ends in it: `.zip` is a delegated gTLD, so rule 1 refuses it. A bundle +// name quoted in a note is redacted, and that is the correct trade — this rule +// exists precisely to stop a suffix that looks like a file extension from waving a +// real host through. var keptSuffixes = []string{ ".json", ".log", ".txt", ".plist", ".dezhban", // Reserved by RFC 2606 / RFC 6761: never delegated, so never a real host. ".arpa", ".invalid", ".test", } +// keptNames are dezhban's own filenames, matched EXACTLY rather than by suffix. +// +// Exactly, because the part in front is only dezhban's for these two spellings. +// `control.socket` is a config key, so a `.sock` suffix rule would keep whatever +// the user pointed it at — `/var/run/nordvpn.sock` names the provider as plainly +// as a server does, which is rule 2 on keptSuffixes and the reason `.conf` and +// `.ovpn` are kept out. A user who renames the socket gets it redacted; the +// default, which is dezhban's own name, survives. +// +// Both reach a bundle. `doctor`'s control check quotes the socket as the answer to +// "which socket did it probe", and the run lock's path rides a startup failure +// into the log. Replacing either with `host-N` throws the diagnosis away and hides +// nothing, and counts a hostname in the legend that stands for a filename. +var keptNames = map[string]bool{ + "control.sock": true, // controlSocketPath's default basename + "dezhban.lock": true, // runLockName, a constant +} + // replaceProfileNames rewrites every `"name": "..."` in body. func (r *Redactor) replaceProfileNames(body string) string { return profileNameRe.ReplaceAllStringFunc(body, func(n string) string { diff --git a/internal/redact/redact_test.go b/internal/redact/redact_test.go index f711bce..3fff4b8 100644 --- a/internal/redact/redact_test.go +++ b/internal/redact/redact_test.go @@ -89,11 +89,61 @@ func TestShippedGeoProvidersAreKept(t *testing.T) { // would be hard to read for no gain. func TestDezhbanFilenamesAreNotHostnames(t *testing.T) { r := New(true) - for _, name := range []string{"learned.json", "dezhban.log", "README.txt"} { + // control.sock and dezhban.lock were missing, and both reach a bundle: the + // control check quotes the socket as the answer to "which socket did it + // probe", and a startup failure carries the lock's path into the log. Turning + // either into host-N throws away the diagnosis and hides nothing, which is + // the direction docs/contribute/testing.md calls out by name. + for _, name := range []string{ + "learned.json", "dezhban.log", "README.txt", "control.sock", "dezhban.lock", + } { if got := r.Text("wrote " + name); !strings.Contains(got, name) { t.Errorf("%s was treated as a hostname: %q", name, got) } } + // The whole path survives, not just the basename — the control check's value + // is that it names WHERE the socket is. + const path = "/var/db/dezhban/control.sock" + if got := r.Text("reachable (" + path + ")"); !strings.Contains(got, path) { + t.Errorf("the socket path was redacted: %q", got) + } + // And the legend does not gain a hostname that stands for a filename. + if legend := r.Legend(); len(legend) != 0 { + t.Errorf("legend = %v, want nothing minted for dezhban's own filenames", legend) + } +} + +// The socket is a CONFIG KEY, so only its default basename is dezhban's. A suffix +// rule would have kept whatever the user pointed `control.socket` at — and a +// socket named after the VPN it sits beside states the provider as plainly as a +// server address does. That is rule 2 on keptSuffixes, and the reason `.conf` and +// `.ovpn` are kept off that list. +func TestASocketTheUserNamedIsStillRedacted(t *testing.T) { + r := New(true) + for _, name := range []string{"nordvpn.sock", "mullvad.sock", "proton.lock"} { + if got := r.Text(name); got == name { + t.Errorf("%s survived — a user-named socket is not one of dezhban's files", name) + } + } + // While dezhban's own two still do. + for _, name := range []string{"control.sock", "dezhban.lock"} { + if got := r.Text(name); got != name { + t.Errorf("%s was redacted: %q", name, got) + } + } +} + +// `.zip` must never join keptSuffixes, however much the bundle's own filename +// ends in it: it is a delegated gTLD, so a suffix rule admitting it would wave a +// real host straight through — rule 1 on that list, and the reason the list is +// kept as short as its two rules allow. +func TestADelegatedTLDIsNeverTreatedAsAFileExtension(t *testing.T) { + for _, host := range []string{"dezhban-report-20260913.zip", "mullvad.zip", "vpn.sh", "server.md"} { + r := New(true) + if got := r.Text(host); got == host { + t.Errorf("%s survived — a delegated TLD was treated as one of dezhban's file endings", host) + } + } } // The user's OWN filenames are the opposite case, and this is the direction the