From dd262b51f17f28bf7fc548d7bdaa8af8e6b41f11 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sun, 13 Sep 2026 11:39:36 +0330 Subject: [PATCH 1/2] fix(diag): keep dezhban's own file paths out of the hostname pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `doctor`'s control check names the socket it probed, and a default bundle reported it as `/var/db/dezhban/host-1`. The path is the answer to "which socket" — replacing it throws the diagnosis away and hides nothing, the direction docs/contribute/testing.md names by itself. It also counted a hostname in the legend that stands for a filename. `control.sock` satisfies hostRe (dotted, alphabetic last label) and keptSuffixes did not list `.sock`. Nor `.lock`, which has the same problem wherever a startup failure carries the run lock's path into the log. Both pass the two rules that list states: neither is a delegated TLD, and both names are dezhban's. Pre-existing — reproduced against v0.14.0's internal/redact, so it is not a regression from #67; found while probing the redactor after that release. `.zip` is NOT added and the comment now says why, because the bundle's own filename ends in it and that makes it the obvious next thing to add: `.zip` is a delegated gTLD, so rule 1 refuses it. TestADelegatedTLDIsNeverTreatedAsAFileExtension fails on `mullvad.zip` if anyone tries — a real provider name waved through by a suffix that looks like a file extension, which is the whole reason that rule exists. `.pkg` is left out for want of a root-zone check rather than a decision. Verified: task check, and a real `dezhban report` whose doctor.json now reads `unreachable (/var/db/dezhban/control.sock)` with nothing else leaked. Closes #68 Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 12 ++++++++++++ docs/contribute/testing.md | 5 ++++- internal/redact/redact.go | 15 ++++++++++++++- internal/redact/redact_test.go | 32 +++++++++++++++++++++++++++++++- 4 files changed, 61 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 531b867..9768f7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,18 @@ 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. The + bundle's own `.zip` name is still redacted, deliberately: `.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..6a61994 100644 --- a/internal/redact/redact.go +++ b/internal/redact/redact.go @@ -582,8 +582,21 @@ 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. +// Two of dezhban's own filenames were missing and were being replaced with +// `host-N`: the control socket, which `doctor`'s control check quotes as the +// answer to "which socket did it probe", and the run lock, whose path a startup +// failure carries into the log. Neither is a delegated TLD and both are named by +// dezhban, so both rules hold. +// +// `.zip` is NOT here and must not be added, though the 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 — the rule exists precisely to +// stop a suffix that looks like a file extension waving a real host through. +// `.pkg` is left out for want of a check against the root zone rather than a +// decision; it reaches a bundle from one error string, so the cost of leaving it +// is one mangled word in a failure nobody sees twice. var keptSuffixes = []string{ - ".json", ".log", ".txt", ".plist", ".dezhban", + ".json", ".log", ".txt", ".plist", ".dezhban", ".sock", ".lock", // Reserved by RFC 2606 / RFC 6761: never delegated, so never a real host. ".arpa", ".invalid", ".test", } diff --git a/internal/redact/redact_test.go b/internal/redact/redact_test.go index f711bce..de01bc1 100644 --- a/internal/redact/redact_test.go +++ b/internal/redact/redact_test.go @@ -89,11 +89,41 @@ 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) + } +} + +// `.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 From 01a72d61cf1e1a1a37bf664f2eb652edb4c4f457 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sun, 13 Sep 2026 11:53:19 +0330 Subject: [PATCH 2/2] fix(diag): keep dezhban's two filenames by NAME, not by file ending MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of the previous commit, before anyone else read it. It added `.sock` and `.lock` to keptSuffixes, and that breaks rule 2 on that very list — the part in front has to be dezhban's, not the user's. `control.socket` is a config key. A suffix rule keeps whatever it points at, so a socket sitting beside the VPN it serves — `/var/run/nordvpn.sock` — would have shipped verbatim in a default bundle. That names the provider as plainly as a server address does, and it is exactly the trap `.conf` and `.ovpn` are kept off the list for. So: an exact-name map instead. `control.sock` is controlSocketPath's default basename and `dezhban.lock` is a constant; both are ours, neither is configurable, and a socket the user renamed goes back to being redacted. TestASocketTheUserNamedIsStillRedacted fails against the suffix version on nordvpn.sock, mullvad.sock and proton.lock. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 7 ++++-- internal/redact/redact.go | 40 +++++++++++++++++++++------------- internal/redact/redact_test.go | 20 +++++++++++++++++ 3 files changed, 50 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9768f7e..e55e8f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,8 +19,11 @@ current as you land changes. 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. The - bundle's own `.zip` name is still redacted, deliberately: `.zip` is a real + 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. diff --git a/internal/redact/redact.go b/internal/redact/redact.go index 6a61994..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,25 +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. -// Two of dezhban's own filenames were missing and were being replaced with -// `host-N`: the control socket, which `doctor`'s control check quotes as the -// answer to "which socket did it probe", and the run lock, whose path a startup -// failure carries into the log. Neither is a delegated TLD and both are named by -// dezhban, so both rules hold. -// -// `.zip` is NOT here and must not be added, though the 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 — the rule exists precisely to -// stop a suffix that looks like a file extension waving a real host through. -// `.pkg` is left out for want of a check against the root zone rather than a -// decision; it reaches a bundle from one error string, so the cost of leaving it -// is one mangled word in a failure nobody sees twice. +// `.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", ".sock", ".lock", + ".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 de01bc1..3fff4b8 100644 --- a/internal/redact/redact_test.go +++ b/internal/redact/redact_test.go @@ -113,6 +113,26 @@ func TestDezhbanFilenamesAreNotHostnames(t *testing.T) { } } +// 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