From 4ac23cc1f2d2099cc09c56e7c0bbca4b9c4b56b2 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Wed, 29 Jul 2026 01:06:54 +0330 Subject: [PATCH 1/5] feat: coverage gate, smart installer, and sudo-less CLI path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three-part hardening pass: - Test quality + coverage gate: a checked-in .testcoverage.yml enforced by `task test:cover` and CI, poll-based tests replacing time.Sleep, t.Parallel across the pure packages, an in-process CLI harness, and blockPlan/ parseOverrides extracted so privileged commands have a testable pure core. - Smart installer: scripts/install.sh gains a TTY-gated interactive menu (state-aware: fresh/upgrade/reinstall), [n/N] progress, and a full uninstall flow. `dezhban upgrade can-activate` closes a real gap where the installer's upgrade path could restart a running daemon through FULL BLOCK, violating ADR-0007 — no override, matching `sudo dezhban restart` as the deliberate escape hatch. Piped installs (`curl | sudo bash`) are unchanged. - Sudo-less CLI path: docs/usage/passwordless.md walks through pointing control.group at the host's existing admin group so day-to-day ops (block/unblock/switch/pause/resume/hold) need no password once configured; `dezhban doctor` gained a control check; the page is bundled into the macOS app's Help pane; and sudo is trimmed from read-only/socket-routed command examples across the user-facing docs (frozen docs, contributor checklists, and curl | sudo bash lines are untouched by design). Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 8 + .testcoverage.yml | 38 +++ CHANGELOG.md | 55 ++++ Taskfile.yml | 12 + cmd/dezhban/doctor_test.go | 94 ++++++ cmd/dezhban/harness_test.go | 501 +++++++++++++++++++++++++++++ cmd/dezhban/main.go | 156 +++++++-- cmd/dezhban/setup.go | 2 +- cmd/dezhban/upgrade.go | 36 ++- docs/contribute/testing.md | 32 ++ docs/usage/cli.md | 44 +-- docs/usage/config.md | 2 +- docs/usage/getting-started.md | 4 +- docs/usage/install.md | 34 +- docs/usage/passwordless.md | 117 +++++++ docs/usage/troubleshooting.md | 19 ++ docs/usage/upgrade.md | 9 + internal/armed/armed_test.go | 4 + internal/config/config_test.go | 38 +++ internal/config/docdrift_test.go | 2 + internal/config/pause_test.go | 4 + internal/config/preset_test.go | 11 + internal/config/reload_test.go | 8 + internal/config/schema_test.go | 9 + internal/config/unknown_test.go | 17 + internal/control/control_test.go | 5 + internal/decision/decision_test.go | 8 + internal/help/manifest.go | 4 + internal/learned/learned_test.go | 9 + internal/redial/redial_test.go | 22 ++ internal/render/render_test.go | 5 + internal/runner/control_test.go | 60 +++- internal/runner/recovery_test.go | 58 +++- internal/update/gate.go | 6 +- internal/vocab/lint_test.go | 3 + scripts/install.sh | 312 ++++++++++++++++-- 36 files changed, 1617 insertions(+), 131 deletions(-) create mode 100644 .testcoverage.yml create mode 100644 cmd/dezhban/harness_test.go create mode 100644 docs/usage/passwordless.md diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f307d1b..b6921bf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -48,6 +48,14 @@ jobs: # loop with a recovery probe — catch data races there. - if: matrix.os == 'ubuntu-latest' run: go test -race ./... + # Coverage gate once (ubuntu): enforces .testcoverage.yml's per-package + # floors so coverage cannot regress. See docs/contribute/testing.md's + # Unit test policy for what counts. + - if: matrix.os == 'ubuntu-latest' + run: | + go test ./... -coverprofile=cover.out + go install github.com/vladopajic/go-test-coverage/v2@latest + "$(go env GOPATH)/bin/go-test-coverage" --config=.testcoverage.yml # Keep the windows backend compiling even though we don't test it yet (see # the matrix comment). Vet, not just build: it type-checks the _windows.go # files that no runner in the matrix ever touches. diff --git a/.testcoverage.yml b/.testcoverage.yml new file mode 100644 index 0000000..8a74227 --- /dev/null +++ b/.testcoverage.yml @@ -0,0 +1,38 @@ +# go-test-coverage config (github.com/vladopajic/go-test-coverage). Dev-only +# tool, installed on demand by `task test:cover` and CI — never added to +# go.mod. See docs/contribute/testing.md's "Unit test policy". +# +# Thresholds below are today's real, measured coverage per package (`go test +# ./... -cover`), rounded down for margin — not an aspirational number. The +# point of the gate is to stop coverage from REGRESSING, not to pretend every +# package is already where it should be. Raise a package's own threshold +# whenever a change pushes its real coverage up; that is the intended way +# this file grows over time. +profile: cover.out +threshold: + package: 60 + total: 55 +override: + # OS-boundary packages: what they DO is verified by + # docs/contribute/testing.md's privileged, on-host checklist, not go test. + # Forcing these toward the default package threshold would mean asserting on + # exact pfctl/nft/launchd argv strings — change detectors that break on + # harmless refactors and prove nothing about correctness. + - path: ^internal/firewall$ # shells out to pfctl/nft/netsh + threshold: 35 + - path: ^internal/svc$ # wraps kardianos/service -> launchd/systemd/Windows SCM + threshold: 5 + - path: ^internal/netdetect$ # reads real network interfaces and routes + threshold: 50 + # cmd/dezhban mixes a testable read-only CLI surface (see + # cmd/dezhban/harness_test.go's runCLI, and the plan functions pulled out of + # cmdBlock/cmdRun) with privileged commands that re-exec sudo, install an OS + # service, or call firewall.Apply directly. Those stay uncovered by design. + - path: ^cmd/dezhban$ + threshold: 35 +exclude: + # Unlike override (package paths, exact match), exclude matches FILE paths — + # a trailing $ here would never match a real .go file, so these are prefixes. + paths: + - ^internal/privilege/ # Geteuid wrapper + a Windows syscall + - ^tools/ # build/dev tooling, never shipped diff --git a/CHANGELOG.md b/CHANGELOG.md index 05a4f97..8a60d45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,61 @@ current as you land changes. ## [Unreleased] +### Added + +- **A checked-in test-quality gate.** `task test:cover` (and CI, once per run) + now enforces `.testcoverage.yml`'s per-package coverage floors via + `go-test-coverage`, pinned to today's real measured coverage rather than an + aspirational number — the point is to stop regressions, not to pretend every + package is already where it should be. See docs/contribute/testing.md's new + "Unit test policy" section for the rules a new test is expected to follow. +- `cmd/dezhban/harness_test.go` exercises the CLI's read-only command surface + in-process (`run()` itself was previously untested), and `blockPlan`/ + `parseOverrides` pull pure decision logic out of `cmdBlock`/`cmdRun` so it is + testable without root or a firewall backend. +- **`scripts/install.sh` is now interactive at a real terminal.** Piped + (`curl | sudo bash`) it is unchanged — no prompt, today's exact defaults. + Run directly instead, it asks: which components to install on a fresh + machine (menubar app, service registration), and on a machine that already + has dezhban, upgrade / reinstall / **uninstall** (typed confirmation, + keep-config prompt, defaults to keeping `/etc/dezhban`). Progress now shows + `[n/N]` step counters, with curl's own progress bar on a real terminal. +- **`dezhban upgrade can-activate [--json]`** — a read-only, no-root + subcommand reporting whether a restart could activate right now (the same + gate `upgrade apply` uses). See "Fixed" below for why `install.sh` needed it. +- **[docs/usage/passwordless.md](docs/usage/passwordless.md)** — a task-oriented + tutorial for pointing `control.group` at your distro's existing admin group + (`sudo`/`wheel`/`admin`) so day-to-day ops (`block`, `unblock`, `switch`, + `pause`, `resume`, `hold`) no longer prompt for a password. Grants no new + authority — if you can already `sudo`, you're already a member. Bundled into + the macOS app's Help pane. +- `dezhban doctor` gained a **`control:`** check: socket reachability, the + configured group, whether the caller is in it, and which ops + `allowSwitchOps`/`allowPauseOps`/`allowConfigOps` force back to `sudo`. + +### Fixed + +- Replaced six blind `time.Sleep`-and-hope waits in `internal/runner` and + `internal/control`'s tests with bounded polling (or, in one case, an + explicitly documented exception where the sleep IS the scenario under test). +- **`scripts/install.sh`'s upgrade path could restart a running daemon through + an unsafe posture.** It stopped and restarted the service unconditionally, + with no regard for ADR-0007's activation gate — including through FULL + BLOCK, which would have briefly lifted a block on a forbidden-country exit, + the one thing this tool exists to prevent. It now checks `dezhban upgrade + can-activate` before stopping anything: the new binary always installs (safe + — a running process keeps its old inode), but the stop/restart is skipped on + refusal, leaving the old build enforcing until `sudo dezhban restart` + succeeds on its own. No override, matching `upgrade apply`'s own rule. +- **Docs and `doctor`/`setup` hints no longer model `sudo` on commands that + don't need it.** `status` never needed root — its `doctor` fix hint wrongly + said otherwise. `block`, `unblock`, `switch`, `pause`, `resume` route over + the control socket before ever needing root; `sudo` in front of them was + actively counterproductive; it re-execs as root, which then pays the very + password prompt the socket exists to avoid. Trimmed from README.md and the + user-facing `docs/usage/` and `docs/concepts/` pages; ADRs, contributor + docs, and every `curl | sudo bash` install line are untouched by design. + ## [0.8.0] - 2026-07-28 ### Changed diff --git a/Taskfile.yml b/Taskfile.yml index 01b7681..dcfcb0c 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -314,6 +314,18 @@ tasks: cmds: - go test ./... + test:cover: + desc: 'Run the suite with coverage and enforce .testcoverage.yml' + cmds: + - go test ./... -coverprofile=cover.out + - | + gobin="$(go env GOPATH)/bin" + if [ ! -x "$gobin/go-test-coverage" ]; then + echo "installing go-test-coverage..." + go install github.com/vladopajic/go-test-coverage/v2@latest + fi + "$gobin/go-test-coverage" --config=.testcoverage.yml + lint: cmds: - | diff --git a/cmd/dezhban/doctor_test.go b/cmd/dezhban/doctor_test.go index 9e11705..3b535bc 100644 --- a/cmd/dezhban/doctor_test.go +++ b/cmd/dezhban/doctor_test.go @@ -1,6 +1,7 @@ package main import ( + "errors" "io" "log/slog" "net/netip" @@ -8,6 +9,7 @@ import ( "testing" "github.com/behnam-rk/dezhban/internal/config" + "github.com/behnam-rk/dezhban/internal/control" "github.com/behnam-rk/dezhban/internal/netdetect" ) @@ -64,6 +66,98 @@ func TestBuildTunnelsCheck(t *testing.T) { }) } +func TestBuildControlCheck(t *testing.T) { + base := func() config.Config { + cfg := config.Default() + cfg.Control.Enabled = true + cfg.Control.Group = "admin" + return cfg + } + + t.Run("disabled", func(t *testing.T) { + cfg := base() + cfg.Control.Enabled = false + c := buildControlCheck(&cfg, control.Response{}, nil) + if c.Status != checkWarn { + t.Errorf("status = %q, want %q", c.Status, checkWarn) + } + if !strings.Contains(c.Summary, "disabled") { + t.Errorf("summary = %q, want it to say disabled", c.Summary) + } + }) + + t.Run("forbidden — reachable but not in the group", func(t *testing.T) { + cfg := base() + c := buildControlCheck(&cfg, control.Response{}, control.ErrForbidden) + if c.Status != checkWarn { + t.Errorf("status = %q, want %q", c.Status, checkWarn) + } + if !strings.Contains(c.Summary, "not in the") || !strings.Contains(c.Summary, "admin") { + t.Errorf("summary = %q, want it to name the group", c.Summary) + } + }) + + t.Run("unreachable — no daemon running", func(t *testing.T) { + cfg := base() + c := buildControlCheck(&cfg, control.Response{}, errors.New("dial: no such file")) + if c.Status != checkWarn { + t.Errorf("status = %q, want %q", c.Status, checkWarn) + } + if !strings.Contains(c.Summary, "unreachable") { + t.Errorf("summary = %q, want it to say unreachable", c.Summary) + } + }) + + t.Run("unreachable and no group configured names both problems", func(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") { + t.Errorf("details = %v, want a note about the missing group", c.Details) + } + }) + + t.Run("reachable, no group configured", func(t *testing.T) { + cfg := base() + cfg.Control.Group = "" + c := buildControlCheck(&cfg, control.Response{OK: true}, nil) + if c.Status != checkWarn { + t.Errorf("status = %q, want %q", c.Status, checkWarn) + } + if !strings.Contains(c.Summary, "no group") { + t.Errorf("summary = %q, want it to say no group is configured", c.Summary) + } + }) + + t.Run("reachable and a member — the happy path", func(t *testing.T) { + cfg := base() + c := buildControlCheck(&cfg, control.Response{OK: true}, nil) + if c.Status != checkOK { + t.Errorf("status = %q, want %q", c.Status, checkOK) + } + if !strings.Contains(c.Summary, "need no password") { + t.Errorf("summary = %q, want it to confirm no password is needed", c.Summary) + } + }) + + t.Run("gated ops are named regardless of reachability", func(t *testing.T) { + cfg := base() + cfg.Control.AllowSwitchOps = false + cfg.Control.AllowConfigOps = false + c := buildControlCheck(&cfg, control.Response{OK: true}, nil) + joined := strings.Join(c.Details, "\n") + if !strings.Contains(joined, "allowSwitchOps=false") { + t.Errorf("details = %v, want the switch gate named", c.Details) + } + if !strings.Contains(joined, "allowConfigOps=false") { + t.Errorf("details = %v, want the config-write gate named", c.Details) + } + if strings.Contains(joined, "allowPauseOps=false") { + t.Errorf("details = %v, want the pause gate NOT named (it wasn't disabled)", c.Details) + } + }) +} + func TestBuildEndpointsCheck(t *testing.T) { t.Run("no endpoints resolved", func(t *testing.T) { c := buildEndpointsCheck(nil, nil) diff --git a/cmd/dezhban/harness_test.go b/cmd/dezhban/harness_test.go new file mode 100644 index 0000000..f55bc69 --- /dev/null +++ b/cmd/dezhban/harness_test.go @@ -0,0 +1,501 @@ +package main + +import ( + "encoding/json" + "io" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/behnam-rk/dezhban/internal/config" + "github.com/behnam-rk/dezhban/internal/control" + "github.com/behnam-rk/dezhban/internal/firewall" +) + +// runCLI runs the CLI's run() entry point in-process, capturing stdout and +// stderr separately, and returns them with the exit code. Global flag state +// (-v, --no-sudo, --no-daemon) is reset first so tests stay order-independent +// regardless of what ran before them — run() itself never resets it, since a +// real process only calls it once. +func runCLI(t *testing.T, args ...string) (stdout, stderr string, code int) { + t.Helper() + verbose, noSudo, noDaemonFlag = false, false, false + + oldOut, oldErr := os.Stdout, os.Stderr + outR, outW, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + errR, errW, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + os.Stdout, os.Stderr = outW, errW + defer func() { os.Stdout, os.Stderr = oldOut, oldErr }() + + code = run(args) + + _ = outW.Close() + _ = errW.Close() + outData, _ := io.ReadAll(outR) + errData, _ := io.ReadAll(errR) + return string(outData), string(errData), code +} + +// testConfigPath writes a valid, self-contained config to a temp file and +// returns its path. config.Default()'s own provider/endpoint set is empty, so +// this touches no network by default; a case that needs to exercise a real +// network-bound path (there are none in this file) would have to opt in +// explicitly. +func testConfigPath(t *testing.T, mutate func(*config.Config)) string { + t.Helper() + cfg := config.Default() + if mutate != nil { + mutate(&cfg) + } + p := filepath.Join(t.TempDir(), "dezhban.json") + if err := config.Save(p, &cfg); err != nil { + t.Fatal(err) + } + return p +} + +// The bare dispatch table: no args, help, and an unknown command. These are +// what a user sees before any subcommand-specific flag parsing happens, and +// were entirely uncovered — run() itself was at 0%. +func TestRunDispatchBasics(t *testing.T) { + cases := []struct { + name string + args []string + wantCode int + wantStderr string + wantStdout string + }{ + {"no args", nil, 2, "Usage:", ""}, + {"help", []string{"help"}, 0, "", "Usage:"}, + {"--help", []string{"--help"}, 0, "", "Usage:"}, + {"-h", []string{"-h"}, 0, "", "Usage:"}, + {"unknown command", []string{"bogus-command"}, 2, `unknown command "bogus-command"`, ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + stdout, stderr, code := runCLI(t, c.args...) + if code != c.wantCode { + t.Errorf("code = %d, want %d (stdout=%q stderr=%q)", code, c.wantCode, stdout, stderr) + } + if c.wantStderr != "" && !strings.Contains(stderr, c.wantStderr) { + t.Errorf("stderr = %q, want it to contain %q", stderr, c.wantStderr) + } + if c.wantStdout != "" && !strings.Contains(stdout, c.wantStdout) { + t.Errorf("stdout = %q, want it to contain %q", stdout, c.wantStdout) + } + }) + } +} + +func TestCmdVersion(t *testing.T) { + stdout, _, code := runCLI(t, "version") + if code != 0 { + t.Fatalf("version exited %d, want 0", code) + } + if !strings.Contains(stdout, "dezhban") { + t.Errorf("version output = %q, want it to mention dezhban", stdout) + } + + // -v adds the build detail lines; it can appear before the subcommand + // because stripVerbose scans the whole arg list before dispatch. + verboseOut, _, code := runCLI(t, "-v", "version") + if code != 0 { + t.Fatalf("version -v exited %d, want 0", code) + } + if !strings.Contains(verboseOut, "go:") { + t.Errorf("verbose version output = %q, want a %q line", verboseOut, "go:") + } + if len(verboseOut) <= len(stdout) { + t.Errorf("verbose output (%d bytes) should be longer than plain (%d bytes)", len(verboseOut), len(stdout)) + } +} + +func TestCmdValidate(t *testing.T) { + good := testConfigPath(t, func(c *config.Config) { + c.BlockedCountries = []string{"IR", "CN"} + }) + stdout, _, code := runCLI(t, "validate", "--config", good) + if code != 0 { + t.Fatalf("validate exited %d, want 0 (stdout=%q)", code, stdout) + } + if !strings.Contains(stdout, "config OK") || !strings.Contains(stdout, "IR, CN") { + t.Errorf("validate output = %q, want it to confirm the config and list the blocked countries", stdout) + } + + bad := filepath.Join(t.TempDir(), "bad.json") + if err := os.WriteFile(bad, []byte("{not valid json"), 0o600); err != nil { + t.Fatal(err) + } + _, stderr, code := runCLI(t, "validate", "--config", bad) + if code != 1 { + t.Fatalf("validate on a broken file exited %d, want 1", code) + } + if !strings.Contains(stderr, "config invalid") { + t.Errorf("stderr = %q, want it to say the config is invalid", stderr) + } +} + +func TestCmdPrintRules(t *testing.T) { + cfgPath := testConfigPath(t, nil) + + for _, mode := range []string{"guard", "fullblock", "switch"} { + t.Run(mode, func(t *testing.T) { + stdout, stderr, code := runCLI(t, "print-rules", "--config", cfgPath, "--mode", mode) + if code != 0 { + t.Fatalf("print-rules --mode %s exited %d, want 0 (stderr=%q)", mode, code, stderr) + } + if strings.TrimSpace(stdout) == "" { + t.Error("print-rules produced no ruleset") + } + }) + } + + // docs/adr/0001: `legacy` was deliberately removed, not silently rendered + // as something else — this pins that it still errors by name. + _, stderr, code := runCLI(t, "print-rules", "--config", cfgPath, "--mode", "legacy") + if code == 0 { + t.Fatal("print-rules --mode legacy succeeded, want a refusal (docs/adr/0001)") + } + if !strings.Contains(stderr, "0001") { + t.Errorf("stderr = %q, want it to point at ADR-0001", stderr) + } + + _, stderr, code = runCLI(t, "print-rules", "--config", cfgPath, "--mode", "bogus") + if code == 0 { + t.Fatal("print-rules --mode bogus succeeded, want a refusal") + } + if !strings.Contains(stderr, `"bogus"`) { + t.Errorf("stderr = %q, want it to name the bad mode", stderr) + } +} + +func TestCmdDetectVPN(t *testing.T) { + // Host-dependent output (tunnels may or may not be present), but the exit + // code and the fact that it prints something are not. + stdout, _, code := runCLI(t, "detect-vpn") + if code != 0 { + t.Fatalf("detect-vpn exited %d, want 0", code) + } + if strings.TrimSpace(stdout) == "" { + t.Error("detect-vpn produced no output") + } +} + +// TestCmdMonitorNoProviders exercises monitor's config/provider validation +// without a network call: an unrecognized provider URL matches nothing in +// ProvidersFromURLs, so this fails before any lookup is attempted. +func TestCmdMonitorNoProviders(t *testing.T) { + cfgPath := testConfigPath(t, func(c *config.Config) { + c.Providers = []string{"https://provider.invalid/not-a-known-geo-api"} + }) + _, stderr, code := runCLI(t, "monitor", "--config", cfgPath, "--once") + if code != 1 { + t.Fatalf("monitor with no usable providers exited %d, want 1 (stderr=%q)", code, stderr) + } + if !strings.Contains(stderr, "no usable geo providers configured") { + t.Errorf("stderr = %q, want the no-providers refusal", stderr) + } +} + +func TestCmdCompletion(t *testing.T) { + for _, shell := range []string{"bash", "zsh", "fish"} { + t.Run(shell, func(t *testing.T) { + stdout, _, code := runCLI(t, "completion", shell) + if code != 0 { + t.Fatalf("completion %s exited %d, want 0", shell, code) + } + if strings.TrimSpace(stdout) == "" { + t.Error("completion produced no script") + } + }) + } + + _, _, code := runCLI(t, "completion") + if code != 2 { + t.Errorf("completion with no shell exited %d, want 2", code) + } + _, stderr, code := runCLI(t, "completion", "powershell") + if code != 2 { + t.Errorf("completion powershell exited %d, want 2", code) + } + if !strings.Contains(stderr, `"powershell"`) { + t.Errorf("stderr = %q, want it to name the unsupported shell", stderr) + } +} + +func TestCmdConfigShowPathSchema(t *testing.T) { + cfgPath := testConfigPath(t, func(c *config.Config) { + c.LogLevel = "debug" + }) + + stdout, _, code := runCLI(t, "config", "path", "--config", cfgPath) + if code != 0 { + t.Fatalf("config path exited %d, want 0", code) + } + if strings.TrimSpace(stdout) != cfgPath { + t.Errorf("config path = %q, want %q", strings.TrimSpace(stdout), cfgPath) + } + + stdout, _, code = runCLI(t, "config", "show", "--config", cfgPath) + if code != 0 { + t.Fatalf("config show exited %d, want 0", code) + } + var shown map[string]any + if err := json.Unmarshal([]byte(stdout), &shown); err != nil { + t.Fatalf("config show did not print valid JSON: %v\n%s", err, stdout) + } + if shown["logLevel"] != "debug" { + t.Errorf("config show logLevel = %v, want %q", shown["logLevel"], "debug") + } + + for _, args := range [][]string{{"config", "schema"}, {"config", "schema", "--json"}} { + t.Run(strings.Join(args, " "), func(t *testing.T) { + stdout, _, code := runCLI(t, args...) + if code != 0 { + t.Fatalf("%v exited %d, want 0", args, code) + } + if strings.TrimSpace(stdout) == "" { + t.Error("schema produced no output") + } + }) + } +} + +// TestBlockPlan covers the pure decision extracted from cmdBlock's default +// branch: no root, no firewall backend, no daemon. AutoDetect is off in every +// case so tunnel resolution never touches this machine's real interfaces — +// the two error cases must fire on config content alone, not on host state. +func TestBlockPlan(t *testing.T) { + log := newLogger(&config.Config{LogLevel: "error"}) + + t.Run("no tunnels configured", func(t *testing.T) { + cfg := config.Default() + cfg.VPN.AutoDetect = false + _, err := blockPlan(&cfg, log, false) + if err == nil { + t.Fatal("blockPlan succeeded with no tunnel interfaces configured, want an error") + } + if !strings.Contains(err.Error(), "tunnel interfaces") { + t.Errorf("err = %q, want it to name the missing tunnels", err) + } + }) + + t.Run("no endpoints resolvable", func(t *testing.T) { + cfg := config.Default() + cfg.VPN.AutoDetect = false + cfg.VPN.TunnelInterfaces = []string{"dzh-test-tun0"} // pinned, never expected to exist + cfg.VPN.AutoDiscoverEndpoints = false + _, err := blockPlan(&cfg, log, false) + if err == nil { + t.Fatal("blockPlan succeeded with no endpoints configured or discoverable, want an error") + } + if !strings.Contains(err.Error(), "endpoint") { + t.Errorf("err = %q, want it to name the missing endpoint", err) + } + }) + + baseCfg := func() config.Config { + cfg := config.Default() + cfg.VPN.AutoDetect = false + cfg.VPN.TunnelInterfaces = []string{"dzh-test-tun0"} + cfg.VPN.Endpoints = []string{"203.0.113.9"} + return cfg + } + + t.Run("full block", func(t *testing.T) { + cfg := baseCfg() + d, err := blockPlan(&cfg, log, false) + if err != nil { + t.Fatalf("blockPlan: %v", err) + } + if d.Policy.Mode != firewall.ModeFullBlock { + t.Errorf("Mode = %v, want ModeFullBlock", d.Policy.Mode) + } + }) + + t.Run("guard", func(t *testing.T) { + cfg := baseCfg() + d, err := blockPlan(&cfg, log, true) + if err != nil { + t.Fatalf("blockPlan: %v", err) + } + if d.Policy.Mode != firewall.ModeGuard { + t.Errorf("Mode = %v, want ModeGuard", d.Policy.Mode) + } + if len(d.Tunnels) != 1 || d.Tunnels[0] != "dzh-test-tun0" { + t.Errorf("Tunnels = %v, want [dzh-test-tun0]", d.Tunnels) + } + }) +} + +// TestParseOverrides covers cmdRun's flag-validation step in isolation. +// assembleOptions and runDryRun, cmdRun's other two extracted pieces, are +// deliberately not covered here: both read hardcoded, unconfigurable system +// paths (defaultStatePath, defaultLearnedPath, defaultArmedPath, +// defaultCommandPath, defaultTokenPath) and assembleOptions calls +// state.EnsureDir on the real state directory — calling either from a test +// would touch this machine's real dezhban installation state, which no test +// may do. +func TestParseOverrides(t *testing.T) { + ov, err := parseOverrides(" ir ", "") + if err != nil { + t.Fatalf("parseOverrides: %v", err) + } + if ov.simCountry != "ir" { + t.Errorf("simCountry = %q, want %q (trimmed, case preserved)", ov.simCountry, "ir") + } + if ov.tunnelDownSet { + t.Error("tunnelDownSet = true with no --simulate-tunnel-down given") + } + + ov, err = parseOverrides("", "8s") + if err != nil { + t.Fatalf("parseOverrides: %v", err) + } + if !ov.tunnelDownSet || ov.tunnelDownAfter != 8*time.Second { + t.Errorf("tunnelDownSet/After = %v/%v, want true/8s", ov.tunnelDownSet, ov.tunnelDownAfter) + } + + if _, err := parseOverrides("", "not-a-duration"); err == nil { + t.Fatal("parseOverrides accepted a malformed --simulate-tunnel-down, want an error") + } +} + +// TestCmdDoctorJSON only checks that --json produces valid JSON. The exit +// code and the report's actual findings are host-dependent (real tunnel, +// service, and lockout checks), so this does not assert on either. +func TestCmdDoctorJSON(t *testing.T) { + cfgPath := testConfigPath(t, nil) + stdout, stderr, code := runCLI(t, "doctor", "--config", cfgPath, "--json") + if code != 0 && code != 1 { + t.Fatalf("doctor --json exited %d, want 0 or 1 (stderr=%q)", code, stderr) + } + var report map[string]any + if err := json.Unmarshal([]byte(stdout), &report); err != nil { + t.Fatalf("doctor --json did not print valid JSON: %v\n%s", err, stdout) + } +} + +// TestCmdUpgradeCanActivate only checks the JSON shape and that the exit code +// agrees with "ok" — the verdict itself depends on defaultStatePath(), a real +// unconfigurable system path this test must not assume anything about. +func TestCmdUpgradeCanActivate(t *testing.T) { + stdout, _, code := runCLI(t, "upgrade", "can-activate", "--json") + var res struct { + OK bool `json:"ok"` + Reason string `json:"reason"` + } + if err := json.Unmarshal([]byte(stdout), &res); err != nil { + t.Fatalf("upgrade can-activate --json did not print valid JSON: %v\n%s", err, stdout) + } + if res.Reason == "" { + t.Error("upgrade can-activate --json printed no reason") + } + wantCode := 1 + if res.OK { + wantCode = 0 + } + if code != wantCode { + t.Errorf("code = %d, want %d for ok=%v", code, wantCode, res.OK) + } +} + +// TestCmdVPNList only asserts on the profile/default lines this test itself +// controls via cfg — it must NOT assert on the "learned" or "active state" +// sections, which read the real (host-dependent, possibly populated on a dev +// machine) /var/db/dezhban files with no config override. +func TestCmdVPNList(t *testing.T) { + cfgPath := testConfigPath(t, func(c *config.Config) { + c.VPN.Profiles = []config.Profile{ + {Name: "home", Endpoints: []string{"203.0.113.9"}, TunnelHint: "wg"}, + } + }) + stdout, _, code := runCLI(t, "vpn", "list", "--config", cfgPath) + if code != 0 { + t.Fatalf("vpn list exited %d, want 0", code) + } + if !strings.Contains(stdout, "home") || !strings.Contains(stdout, "203.0.113.9") { + t.Errorf("vpn list = %q, want it to list the configured profile", stdout) + } + + _, stderr, code := runCLI(t, "vpn") + if code != 2 { + t.Errorf("vpn with no subcommand exited %d, want 2", code) + } + if strings.TrimSpace(stderr) == "" { + t.Error("vpn with no subcommand printed no usage") + } +} + +// TestCmdTokenStatus only checks the exit code (always 0) — the message +// itself depends on real, unconfigurable host state +// (defaultTokenPath/stateDir have no config override), which this test must +// not assume anything about. +func TestCmdTokenStatus(t *testing.T) { + _, _, code := runCLI(t, "token", "status") + if code != 0 { + t.Fatalf("token status exited %d, want 0", code) + } +} + +// controlStatus is fully config-driven (Control.Socket overrides the default +// path), so all three branches are deterministic and hermetic. +func TestControlStatus(t *testing.T) { + t.Run("disabled", func(t *testing.T) { + cfg := config.Default() + cfg.Control.Enabled = false + got := controlStatus(&cfg) + if !strings.Contains(got, "disabled") { + t.Errorf("controlStatus = %q, want it to say disabled", got) + } + }) + + t.Run("unreachable", func(t *testing.T) { + cfg := config.Default() + cfg.Control.Enabled = true + cfg.Control.Socket = filepath.Join(t.TempDir(), "no-daemon-here.sock") + got := controlStatus(&cfg) + if !strings.Contains(got, "unreachable") { + t.Errorf("controlStatus = %q, want it to say unreachable", got) + } + }) + + t.Run("reachable", func(t *testing.T) { + // A short dir, not t.TempDir(): that path nests under a per-test name + // long enough to overrun the platform's unix socket sun_path limit. + dir, err := os.MkdirTemp("", "dzh") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.RemoveAll(dir) }) + sockPath := filepath.Join(dir, "c.sock") + srv, err := control.New(sockPath, "", newLogger(&config.Config{LogLevel: "error"})) + if err != nil { + t.Fatalf("control.New: %v", err) + } + srv.Start(t.Context()) + defer srv.Stop() + go func() { + for req := range srv.Requests() { + req.Reply <- control.Response{OK: true} + } + }() + + cfg := config.Default() + cfg.Control.Enabled = true + cfg.Control.Socket = sockPath + cfg.Control.Group = "admin" + got := controlStatus(&cfg) + if !strings.Contains(got, "reachable") || !strings.Contains(got, "admin") { + t.Errorf("controlStatus = %q, want it to say reachable and name the group", got) + } + }) +} diff --git a/cmd/dezhban/main.go b/cmd/dezhban/main.go index 9100928..994090f 100644 --- a/cmd/dezhban/main.go +++ b/cmd/dezhban/main.go @@ -9,6 +9,7 @@ package main import ( "context" "encoding/json" + "errors" "flag" "fmt" "log/slog" @@ -779,6 +780,49 @@ func runDryRun(cfg *config.Config, log *slog.Logger, ov runOverrides) int { return 0 } +// blockDecision is the outcome of blockPlan: the policy to apply, plus the +// tunnels/endpoints it was built from — cmdBlock logs those alongside the +// apply, and recomputing them a second time would repeat the DNS/discovery +// work resolveEndpointsOnce already did. +type blockDecision struct { + Policy firewall.Policy + Tunnels []string + Endpoints []netip.Addr +} + +// blockPlan builds the VPN-mode firewall policy `block`/`block --guard` would +// apply — the pure decision pulled out of cmdBlock's default branch (`--force` +// stays a manual override built directly in cmdBlock; it has no VPN state to +// resolve) so it can be tested without root or a firewall backend. Built +// through the same firewall.PolicyInput constructor the daemon and +// print-rules use, so this manual override can never drift from what the run +// loop would actually install — in particular, it must NOT carry a physical +// dst-IP allowlist: a VPN posture opens the tunnel endpoint, never a +// destination allowlist. +func blockPlan(cfg *config.Config, log *slog.Logger, guard bool) (blockDecision, error) { + tunnels := resolveTunnels(cfg, log) + if len(tunnels) == 0 { + return blockDecision{}, fmt.Errorf("vpn mode needs tunnel interfaces (vpn.tunnelInterfaces or vpn.autoDetect)") + } + endpoints := resolveEndpointsOnce(cfg, log, tunnels) + if len(endpoints) == 0 { + return blockDecision{}, fmt.Errorf("vpn mode needs at least one reachable endpoint (vpn.endpoints as IP/hostname, or vpn.autoDiscoverEndpoints with the VPN connected)") + } + in := firewall.PolicyInput{ + Tunnels: tunnels, + Endpoints: endpoints, + AllowPhysicalDNS: cfg.VPN.AllowPhysicalDNS, + AllowLocalNetwork: cfg.VPN.AllowLocalNetwork, + WindowProtos: cfg.VPN.Advanced.WindowProtocols, + WindowPorts: cfg.VPN.Advanced.WindowPorts, + } + pol := in.FullBlock() + if guard { + pol = in.Guard() + } + return blockDecision{Policy: pol, Tunnels: tunnels, Endpoints: endpoints}, nil +} + func cmdBlock(args []string) int { fs := flag.NewFlagSet("block", flag.ExitOnError) cfgPath := fs.String("config", "", "path to config file (JSON)") @@ -831,41 +875,20 @@ func cmdBlock(args []string) int { default: // `--guard` installs the always-on interface guard (tunnel stays open, // physical egress locked to the endpoint); a plain `block` is a full block - // that cuts the tunnel too. Built through the same firewall.PolicyInput - // constructor the daemon and print-rules use, so this manual override can - // never drift from what the run loop would actually install — in - // particular, it must NOT carry a physical dst-IP allowlist: a VPN posture - // opens the tunnel endpoint, never a destination allowlist. - tunnels := resolveTunnels(cfg, log) - if len(tunnels) == 0 { - log.Error("vpn mode needs tunnel interfaces (vpn.tunnelInterfaces or vpn.autoDetect)") - return 1 - } - endpoints := resolveEndpointsOnce(cfg, log, tunnels) - if len(endpoints) == 0 { - log.Error("vpn mode needs at least one reachable endpoint (vpn.endpoints as IP/hostname, or vpn.autoDiscoverEndpoints with the VPN connected)") + // that cuts the tunnel too. See blockPlan for how the policy is built. + d, err := blockPlan(cfg, log, *guard) + if err != nil { + log.Error(err.Error()) return 1 } - in := firewall.PolicyInput{ - Tunnels: tunnels, - Endpoints: endpoints, - AllowPhysicalDNS: cfg.VPN.AllowPhysicalDNS, - AllowLocalNetwork: cfg.VPN.AllowLocalNetwork, - WindowProtos: cfg.VPN.Advanced.WindowProtocols, - WindowPorts: cfg.VPN.Advanced.WindowPorts, - } - pol := in.FullBlock() - if *guard { - pol = in.Guard() - } - if err := fw.Apply(pol); err != nil { + if err := fw.Apply(d.Policy); err != nil { log.Error("block failed", "err", err) return 1 } if *guard { - log.Info("vpn guard active", "tunnels", tunnels, "endpoints", len(endpoints)) + log.Info("vpn guard active", "tunnels", d.Tunnels, "endpoints", len(d.Endpoints)) } else { - log.Info("network full-blocked (vpn)", "tunnels", tunnels) + log.Info("network full-blocked (vpn)", "tunnels", d.Tunnels) } } return 0 @@ -1647,8 +1670,8 @@ 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 (needs root):"} - c.Fixes = []string{"sudo dezhban status"} + c.Details = []string{"Nothing readable here says what happens at boot. Ask it directly:"} + c.Fixes = []string{"dezhban status"} return c } @@ -1699,6 +1722,62 @@ func buildServiceCheck(unit svc.BootUnit, daemonLive bool) doctorCheck { return c } +// buildControlCheck reports whether routine ops (block/unblock/switch/pause/ +// resume/hold) will ask for a password over the control socket — the +// passwordless path's own diagnostic. Pure: resp/probeErr are the result of +// probeControl(cfg), already run by the caller, so the branches are directly +// testable without a real socket. Mirrors controlStatus's own three-way +// distinction (disabled / forbidden / unreachable / reachable) rather than +// reusing its prose, so this stays a structured doctorCheck instead of a +// second renderer reparsing a sentence to guess a status. +func buildControlCheck(cfg *config.Config, resp control.Response, probeErr error) doctorCheck { + c := doctorCheck{Name: "control", Status: checkOK} + path := controlSocketPath(cfg) + + switch { + case !cfg.Control.Enabled: + c.Status = checkWarn + c.Summary = "disabled (control.enabled=false) — routine ops need sudo." + c.Fixes = []string{"dezhban config set control.enabled true"} + 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.Fixes = []string{"see docs/usage/passwordless.md"} + 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."} + c.Fixes = []string{"see docs/usage/passwordless.md"} + } + case cfg.Control.Group == "": + c.Status = checkWarn + c.Summary = fmt.Sprintf("reachable (%s), but no group is configured — routine ops need sudo.", path) + c.Fixes = []string{"see docs/usage/passwordless.md"} + default: + c.Summary = fmt.Sprintf("reachable (%s, group %q) — routine ops need no password.", path, cfg.Control.Group) + } + + var gated []string + if !cfg.Control.AllowSwitchOps { + gated = append(gated, "switch (control.allowSwitchOps=false)") + } + if !cfg.Control.AllowPauseOps { + gated = append(gated, "pause/resume (control.allowPauseOps=false)") + } + if !cfg.Control.AllowConfigOps { + gated = append(gated, "config set (control.allowConfigOps=false)") + } + if len(gated) > 0 { + if len(c.Details) > 0 { + c.Details = append(c.Details, "") + } + c.Details = append(c.Details, "Forced back to sudo regardless of group membership:") + c.Details = append(c.Details, gated...) + } + return c +} + // buildArmAtBootCheck reports whether the NEXT boot arms the guard immediately // or opens into standby until a live tunnel probe succeeds. Pure. // @@ -1928,6 +2007,9 @@ func runDoctor(cfg *config.Config, log *slog.Logger, discover bool) doctorReport cfg.VPN.Advanced.LearnedEndpointTTL, cfg.VPN.Advanced.LearnedMaxPerProfile, len(cfg.VPN.Endpoints), now)) + controlResp, controlErr := probeControl(cfg) + checks = append(checks, buildControlCheck(cfg, controlResp, controlErr)) + // Touch ID discoverability (macOS): privileged ops (start/stop/panic, GUI // actions) authenticate through sudo, and sudo only offers Touch ID when // pam_tid is opted in via /etc/pam.d/sudo_local. Informational only — never @@ -2003,7 +2085,7 @@ var unattendedSections = []struct{ name, heading string }{ var sectionedChecks = []string{ "config", "tunnels", "endpoints", "lockout", "service", "armAtBoot", "endpointRetention", - "touchID", "discover", + "control", "touchID", "discover", } // printDoctor renders a doctorReport in the text layout `doctor` has always @@ -2092,6 +2174,18 @@ func printDoctor(r doctorReport) { fmt.Println() } + if ctl, ok := get("control"); ok { + fmt.Printf("control: %s\n", ctl.Summary) + printDetails(ctl.Details) + if len(ctl.Fixes) > 0 { + fmt.Println() + for _, f := range ctl.Fixes { + fmt.Printf(" %s\n", f) + } + } + fmt.Println() + } + if touchID, ok := get("touchID"); ok { fmt.Printf("touch id: %s\n", touchID.Summary) printDetails(touchID.Details) diff --git a/cmd/dezhban/setup.go b/cmd/dezhban/setup.go index 499c244..5da5688 100644 --- a/cmd/dezhban/setup.go +++ b/cmd/dezhban/setup.go @@ -175,7 +175,7 @@ func cmdSetup(args []string) int { fmt.Println("later, enable it with: sudo dezhban install && sudo dezhban start") } if answers.Bool("configureVPN") { - fmt.Println("to connect a brand-new VPN whose server isn't known yet: sudo dezhban switch, then connect it.") + fmt.Println("to connect a brand-new VPN whose server isn't known yet: dezhban switch, then connect it.") } return 0 } diff --git a/cmd/dezhban/upgrade.go b/cmd/dezhban/upgrade.go index 3e3d18f..56d3d60 100644 --- a/cmd/dezhban/upgrade.go +++ b/cmd/dezhban/upgrade.go @@ -29,9 +29,10 @@ import ( const upgradeUsage = `usage: dezhban upgrade - check Ask GitHub for the latest release and report if one is newer (no root) - download Fetch and verify the latest .pkg, staged for apply (root — see below) - apply Install the staged .pkg and, unless --no-activate, restart into it (root) + check Ask GitHub for the latest release and report if one is newer (no root) + download Fetch and verify the latest .pkg, staged for apply (root — see below) + apply Install the staged .pkg and, unless --no-activate, restart into it (root) + can-activate Report whether a restart could activate right now (no root) download needs root too, not just apply: its staging directory is root-owned on purpose. A writable-by-anyone staging area would let a local user swap the @@ -65,6 +66,8 @@ func cmdUpgrade(args []string) int { return cmdUpgradeDownload(rest) case "apply": return cmdUpgradeApply(rest) + case "can-activate": + return cmdUpgradeCanActivate(rest) case "help", "-h", "--help": fmt.Println(upgradeUsage) return 0 @@ -117,6 +120,33 @@ func cmdUpgradeCheck(args []string) int { return 0 } +// cmdUpgradeCanActivate wraps update.CanActivate for callers outside the Go +// process — namely scripts/install.sh, whose upgrade path used to restart a +// running daemon unconditionally, including from FULL BLOCK, which ADR-0007 +// says a restart must never do. It reads the daemon's own state snapshot, so +// it needs no root — same posture cmdUpgradeApply's activation step already +// gates on, just exposed as its own read-only check. +func cmdUpgradeCanActivate(args []string) int { + fs := flag.NewFlagSet("upgrade can-activate", flag.ExitOnError) + jsonOut := fs.Bool("json", false, "print machine-readable JSON") + _ = fs.Parse(args) + + res := update.CanActivate(defaultStatePath()) + + if *jsonOut { + data, _ := json.MarshalIndent(res, "", " ") + fmt.Println(string(data)) + } else if res.OK { + fmt.Printf("ok — a restart could activate now (posture: %s)\n", res.Posture) + } else { + fmt.Printf("refused: %s\n", res.Reason) + } + if !res.OK { + return 1 + } + return 0 +} + func cmdUpgradeDownload(args []string) int { fs := flag.NewFlagSet("upgrade download", flag.ExitOnError) versionFlag := fs.String("version", "", "exact version to fetch (default: latest)") diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index d61f14c..22973ff 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -29,6 +29,38 @@ decoding, posture→icon derivation, settings-field batching). `DezhbanMenu` itself (the AppKit/SwiftUI executable, elevation, CLI shell-out) has no test target — see Known gaps. +## Unit test policy + +Rules for `go test ./...` — the part of the suite that runs in CI, with no +root and no real firewall. `task test:cover` enforces the coverage floors in +`.testcoverage.yml`; these rules keep what counts toward them honest. + +- **No `time.Sleep` in a test.** Poll for the condition with a bounded + deadline, or inject a clock the way `internal/redial` and `internal/decision` + already do. A sleep either wastes wall-clock waiting past the real moment or + races it — neither proves the behaviour happened. **Exception:** a test whose + whole point is a wall-clock duration itself — e.g. proving a reply survives + arriving *after* an internal deadline has passed + (`TestSlowRunLoopStillGetsItsReplyThrough`) — may sleep past that real + duration, because the delay is the scenario, not a wait for one. Comment why + when you reach for this. +- **`t.Parallel()` by default.** Add it to every new test unless the test uses + `t.Setenv` or otherwise mutates process-global state (`t.Setenv` itself + already fails if paired with `t.Parallel()`, which is the tell). +- **Table-driven once there are ≥3 similar cases; a plain `t.Run` below that.** + A table with one or two rows is indirection with nothing to show for it. +- **Assert observable behaviour, not call arguments.** Prefer "the firewall + policy now blocks egress" over "`Apply` was called with these exact flags" — + the latter breaks on refactors and proves nothing about correctness. +- **Every gate or refusal needs a negative case.** If code can say no + (`CanActivate`, `canElevate`, a disabled window, a policy switch), a test + must prove it actually says no, not just that it says yes when allowed. +- **Invariant-pin tests are named contracts — extend them, never restructure.** + `TestWindowDisableMatrix`, `TestEveryLinkGoesSomewhere`, + `TestPauseMaxDefaultAndDisableSentinel`, and others like them each pin one + specific past bug. Add a case to cover new ground; don't rewrite them to "clean + up" — the name and the shape are the point. + ## Enforcement — all platforms - [ ] **Block cuts egress.** `sudo dezhban block` → general egress dies diff --git a/docs/usage/cli.md b/docs/usage/cli.md index 436fd5c..995945c 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -44,7 +44,9 @@ need no path at all. ## Do I need a password? Mostly, no. Once the daemon is running, the commands you use day to day go **to the -daemon** over its control socket and need no password at all: +daemon** over its control socket and need no password at all — provided +`control.group` is set, which it is by default on macOS but not on Linux; see +[passwordless.md](passwordless.md) to turn it on there: | Command | Needs a password? | |---|---| @@ -57,7 +59,7 @@ daemon** over its control socket and need no password at all: | `config show`/`path`/`schema`, `config preset list`/`show`/`diff`, `setup --questions` | **No** — read-only; they report the config (or what the wizard would ask), they don't change it. | | `token status` | **No** — reports whether a control token is enrolled; the answer is not itself a secret. | | `token enroll`, `token forget` | Yes — the token's hash lives in the daemon's root-owned state dir, because anything that could rewrite it could nominate its own token. Once, at setup. | -| `upgrade check` | **No** — read-only, no root. | +| `upgrade check`, `upgrade can-activate` | **No** — read-only, no root. `can-activate` reports whether a restart could activate right now — the same gate `apply`'s activation step uses (see [upgrade.md](upgrade.md)); `scripts/install.sh` checks it before restarting a running daemon. | | `upgrade download`, `upgrade apply` | Yes — root, macOS only. `download`'s staging directory is root-owned on purpose: a writable-by-anyone staging area would let a local user swap the verified `.pkg` before `apply` installs it. | `dezhban status` prints a `control socket:` line saying which mode you're in. @@ -157,9 +159,9 @@ dezhban run --dry-run # poll & print country, no fir sudo dezhban run --config /etc/dezhban/dezhban.json # manual block / override -sudo dezhban block --config configs/dezhban.example.json -sudo dezhban block --force # cut ALL egress, ignore detection -sudo dezhban unblock +dezhban block --config configs/dezhban.example.json +dezhban block --force # cut ALL egress, ignore detection +dezhban unblock sudo dezhban panic # standalone teardown, no daemon needed ``` @@ -373,9 +375,9 @@ dezhban vpn import ~/wg0.conf # WireGuard .conf / OpenVPN .ovpn / V2Ray dezhban vpn list # profiles + learned endpoints + active state # A brand-new VPN whose server dezhban has never seen: -sudo dezhban switch # open a window (5s default); connect it in its app now -sudo dezhban switch --for 90s --name windscribe # custom duration + attribution -sudo dezhban switch --cancel # close the window early +dezhban switch # open a window (5s default); connect it in its app now +dezhban switch --for 90s --name windscribe # custom duration + attribution +dezhban switch --cancel # close the window early dezhban switch --status # is a window open? sudo dezhban vpn promote # make a learned endpoint permanent (see: vpn list) sudo dezhban vpn forget # drop a learned endpoint @@ -404,8 +406,8 @@ service that refuses a foreign VPN exit: ```sh dezhban pause --list # the offered lengths, and what each is for -sudo dezhban pause 15m # real IP for 15 minutes, capped by vpn.pauseMax -sudo dezhban resume # end it early +dezhban pause 15m # real IP for 15 minutes, capped by vpn.pauseMax +dezhban resume # end it early ``` Unlike `switch`, this doesn't wait for a VPN — it just opens egress for the given @@ -491,20 +493,24 @@ while blocked, the rules persist by design (a kill switch must not fail open); u ## Upgrade ```sh -dezhban upgrade check # no root — is a newer release out? +dezhban upgrade check # no root — is a newer release out? +dezhban upgrade can-activate --json # no root — could a restart activate right now? sudo dezhban upgrade download # macOS only — fetch + verify the .pkg sudo dezhban upgrade apply # macOS only — install it, then activate sudo dezhban upgrade apply --no-activate # install without restarting ``` -`check` works on every platform and is read-only. `download`/`apply` are -macOS only — Linux and Windows package managers own their own upgrade path. -`apply` installs the `.pkg` (zero enforcement gap) and then, unless -`--no-activate`, restarts into it — but only when the daemon's posture makes -that safe (healthy `guard` or `standby`; never `full-block` or an open -switch window). See [docs/upgrade.md](upgrade.md) for the full design: why -it's split this way, the activation gate, rollback, and the menubar app's -**About → Updates** panel. +`check` and `can-activate` work on every platform and are read-only. +`download`/`apply` are macOS only — Linux and Windows package managers own +their own upgrade path. `apply` installs the `.pkg` (zero enforcement gap) +and then, unless `--no-activate`, restarts into it — but only when the +daemon's posture makes that safe (healthy `guard` or `standby`; never +`full-block` or an open switch window). `can-activate` reports that same +verdict without applying anything — `scripts/install.sh`'s upgrade path +checks it before restarting a service that was already running, so it can +never lift a block by accident. See [docs/upgrade.md](upgrade.md) for the +full design: why it's split this way, the activation gate, rollback, and the +menubar app's **About → Updates** panel. ## macOS app diff --git a/docs/usage/config.md b/docs/usage/config.md index aef5421..3b03809 100644 --- a/docs/usage/config.md +++ b/docs/usage/config.md @@ -286,7 +286,7 @@ freely — is served by two mechanisms: snaps shut: ```sh - sudo dezhban switch # opens a window (5s default), watches for the new tunnel + server + dezhban switch # opens a window (5s default), watches for the new tunnel + server # …connect your VPN in its app… sudo dezhban vpn promote # make the learned endpoint permanent (see: dezhban vpn list) ``` diff --git a/docs/usage/getting-started.md b/docs/usage/getting-started.md index 4a9c01d..48c7f4e 100644 --- a/docs/usage/getting-started.md +++ b/docs/usage/getting-started.md @@ -138,9 +138,9 @@ A brand-new VPN server can't complete its handshake through a closed guard, so open a short window for it: ```sh -sudo dezhban switch # opens a window — connect it in its own app now +dezhban switch # opens a window — connect it in its own app now dezhban switch --status # is one open? -sudo dezhban switch --cancel # close it early +dezhban switch --cancel # close it early ``` The window closes itself as soon as a good exit is confirmed, and expires on its diff --git a/docs/usage/install.md b/docs/usage/install.md index 4e6e6ad..fca950a 100644 --- a/docs/usage/install.md +++ b/docs/usage/install.md @@ -22,6 +22,15 @@ sudo dezhban setup # choose your settings sudo dezhban start # arm it ``` +Piped like that, `scripts/install.sh` never prompts — stdin is the script +text itself, so there's nowhere to read a question from, and it takes exactly +the defaults above. Save it to a file and run it directly at a real terminal +(`sudo bash install.sh`, not piped) and it asks a few questions instead: which +components to install on a fresh machine, and — on a machine that already has +dezhban — upgrade, reinstall, or **uninstall**, with a typed confirmation +before anything is removed. `DEZHBAN_ASSUME_YES=1` forces the non-interactive +defaults even at a real terminal. + Everything below is why this is the recommended path, what else exists, and how to verify what you downloaded. @@ -70,8 +79,8 @@ checksums](../contribute/releasing.md#unsigned-artifacts-signed-checksums). 1. Detects OS/arch, resolves the requested version (`VERSION=X.Y.Z` env var pins one; otherwise the latest release — which is never a `-rc` build). -2. Downloads the platform binary (+ the app bundle on macOS) and - `SHA256SUMS`. +2. Downloads the platform binary (+ the app bundle on macOS, unless you opted + out at the component prompt) and `SHA256SUMS`. 3. **Verifies the checksum. A mismatch aborts the install outright** — this is not a warning, it's a hard stop. This is what actually protects you on this path: HTTPS gets the bytes to you unmodified in transit, and the @@ -86,10 +95,23 @@ checksums](../contribute/releasing.md#unsigned-artifacts-signed-checksums). `packaging/linux/uninstall.sh`, from the **same tag** just installed) to `/usr/local/share/dezhban/uninstall.sh`. -Re-running either script is safe: it stops the service first only if it was -already running, replaces the binary, and restarts only if it was running — -never touching `/etc/dezhban/` (your config) or `/var/db/dezhban/` (learned -endpoints, state) either way. +Re-running either script upgrades or reinstalls: it replaces the binary, and +if a service was already running, stops it first and restarts it after — but +only when a restart is actually safe right now (`dezhban upgrade +can-activate`, the same rule `dezhban upgrade apply` honors — never through +FULL BLOCK or an open switch window, see [upgrade.md](upgrade.md)). Refused: +the new binary is installed and the old daemon keeps enforcing on it until you +run `sudo dezhban restart` yourself, once the posture clears. Either way, +`/etc/dezhban/` (your config) and `/var/db/dezhban/` (learned endpoints, +state) are never touched. + +**Uninstalling.** At a real terminal, an existing install offers "Uninstall" +in its menu — shows exactly what will be removed, asks whether to keep your +config (default: yes), and requires typing `uninstall` to confirm. Or run it +directly any time: `sudo sh /usr/local/share/dezhban/uninstall.sh` (add +`KEEP_CONFIG=1` to keep `/etc/dezhban`). It always runs `panic` first — removes +the firewall rules even with no daemon running — before touching anything +else, so you're never left locked out mid-removal. ## Other ways to install diff --git a/docs/usage/passwordless.md b/docs/usage/passwordless.md new file mode 100644 index 0000000..9d06f16 --- /dev/null +++ b/docs/usage/passwordless.md @@ -0,0 +1,117 @@ +# Using the CLI without sudo + +Most day-to-day commands — `block`, `unblock`, `switch`, `pause`, `resume`, +`hold` — don't actually need root once dezhban is running. They ask the +running daemon over its **control socket** instead, and the daemon performs +them. Whether that path is open for you depends on one setting: +`control.group`. + +## Why this isn't on by default + +By default `control.group` is empty, which means the socket is root-only +(mode `0600`). Every routine command then falls back to `sudo`, which is why +a fresh install still asks for a password for things that feel like they +shouldn't need it. Turning this on is one line. + +## Turn it on + +First, identify your system's existing administrators group — the same one +that already lets you run `sudo` at all: + +| System | Group | +|---|---| +| Debian, Ubuntu | `sudo` | +| Fedora, RHEL, Arch, openSUSE | `wheel` | +| macOS | `admin` | + +Point `control.group` at it: + +```sh +sudo dezhban config set control.group sudo # or wheel / admin +``` + +Confirm you're actually a member (you almost certainly already are, since +this is the same group sudo itself uses): + +```sh +id -nG +``` + +Confirm dezhban agrees: + +```sh +dezhban doctor +``` + +Look for the `control:` line. `reachable (…, group "sudo") — routine ops +need no password` means it worked. Anything else names exactly what's +still missing — see [troubleshooting](#troubleshooting) below. + +That's it. `dezhban block`, `unblock`, `switch`, `pause`, `resume`, and `hold` +now run with no password prompt, from your own account. + +## What this actually grants — and what it doesn't + +**No new authority is created here.** Membership in your distro's admin group +already lets you run any command as root via `sudo` — including +`sudo dezhban unblock` today. Pointing `control.group` at that same group +doesn't hand out a new capability; it removes a password prompt for +capabilities those accounts already had. + +This is a different model from a tool that creates its **own** dedicated +group for this purpose — Docker's postinstall does exactly that, and Docker's +own documentation is direct about the cost: membership in the `docker` group +is "equivalent to root" and "grants root-level privileges." dezhban never +creates a group, never runs `usermod`, and never asks you to add an +unprivileged account to anything. If you can already `sudo`, you're already a +member of whatever group you point this at — that's the whole point. + +Be clear-eyed about what membership *does* let someone do without a password: +**relax the kill switch** — open a switch/redial window, pause enforcement, +or lift a block. That's real, and it's why this setting must only ever point +at a group whose members could already do the equivalent thing through +`sudo` — never a group created just for this, and never a group that +includes accounts you wouldn't otherwise trust with root. + +## What still needs root, and why + +A handful of commands intentionally stay outside the control socket +entirely, no matter how `control.group` is set: + +- `panic` — the lockout escape hatch. It removes every firewall rule with no + daemon involved at all, deliberately independent of the very socket it + would otherwise need to work. +- `install`, `uninstall`, `start`, `stop`, `restart` — a daemon can't manage + its own service lifecycle. +- `vpn add`, `vpn remove`, `vpn promote`, `vpn forget`, `vpn import` — these + write to files the daemon owns. +- `token enroll`, `token forget` — the control token that lets a *macOS Touch + ID* prompt substitute for `sudo` on config writes; enrolling one is itself + a privileged, once-per-setup action. +- `upgrade download`, `upgrade apply` — the staging directory is root-owned + on purpose, so a local account can't swap the verified package before it's + installed. + +All of these auto-elevate under `sudo` on their own when you're at a real +terminal (see [cli.md § Do I need a password?](cli.md#do-i-need-a-password)) — +this page is only about the routine ops that don't have to. + +## Troubleshooting + +- **`disabled (control.enabled=false)`** — the socket itself is off: + `dezhban config set control.enabled true`. +- **`unreachable`** — no daemon is listening yet. Start one + (`sudo dezhban start`), or check that `control.socket` (if you've set a + custom path) actually exists. +- **`reachable, but you are not in the "" group`** — `id -nG` doesn't + list it. Add your account to that group the normal way for your OS (the + same step that already lets you `sudo`), then log out and back in — group + membership is read at login, not live. +- **`reachable, but no group is configured`** — `control.group` is still + empty; follow [Turn it on](#turn-it-on) above. +- **A specific op still asks for a password** — check `control.allowSwitchOps` + and `control.allowPauseOps`. Either can independently force `switch`/`pause` + ops back to `sudo` without touching the socket itself; `dezhban doctor`'s + `control:` section names which ones are set that way. + +See [config.md](config.md#control-block) for the full `control.*` reference. diff --git a/docs/usage/troubleshooting.md b/docs/usage/troubleshooting.md index 2377d65..ec7ee9c 100644 --- a/docs/usage/troubleshooting.md +++ b/docs/usage/troubleshooting.md @@ -293,6 +293,25 @@ sudo chmod 755 /var/db/dezhban The open directory leaks nothing: the sensitive files inside it (`command.json`, `pf.state`) are `0600`. +## The installer upgraded the binary but the service is still running the old version + +Expected when the daemon's posture wasn't safe to restart through at the +moment `scripts/install.sh` ran: FULL BLOCK, a guard holding a downed tunnel, +or an open switch/redial/pause window. The new binary is on disk and installed +either way — only the *restart* was skipped, so the old build keeps enforcing +uninterrupted rather than the install racing a restart through an unsafe +posture (see [upgrade.md](upgrade.md) for why that specific gap matters). + +```sh +dezhban upgrade can-activate # names the current refusal reason, if any +dezhban status # confirm the posture that's blocking it +sudo dezhban restart # once the posture clears +``` + +There is no override — this is the same rule `dezhban upgrade apply` enforces +for its own restart, and `sudo dezhban restart` is already the deliberate, +by-name escape hatch for an operator who wants to force it anyway. + ## Preview rules before applying them Never find out what a block does by getting locked out — render the exact diff --git a/docs/usage/upgrade.md b/docs/usage/upgrade.md index 2dd757c..3a8f1fb 100644 --- a/docs/usage/upgrade.md +++ b/docs/usage/upgrade.md @@ -82,6 +82,15 @@ the previous version is still running normally. retry activation later with: sud Retry with `sudo dezhban restart` once the posture clears, whenever that is. +`dezhban upgrade can-activate [--json]` reports this same verdict on demand, +with no root and without applying anything — the CLI-exposed form of the +gate above, for a caller that isn't `dezhban upgrade apply` itself. +`scripts/install.sh` calls it before restarting a service that was already +running: install the new binary either way (the running daemon keeps +enforcing on its old inode), but only restart into it when the gate says +it's safe. There is deliberately no override — an operator who wants to +force it already has `sudo dezhban restart`, typed by name. + ## The restart window is disclosed, not hidden or blocked A deliberate choice: rather than adding a *second* firewall mechanism (a diff --git a/internal/armed/armed_test.go b/internal/armed/armed_test.go index b27e087..bad815d 100644 --- a/internal/armed/armed_test.go +++ b/internal/armed/armed_test.go @@ -9,6 +9,7 @@ import ( ) func TestLoadMissingFileIsNeverArmed(t *testing.T) { + t.Parallel() r, err := Load(filepath.Join(t.TempDir(), "nope.json")) if err != nil { t.Fatalf("Load missing: %v", err) @@ -19,6 +20,7 @@ func TestLoadMissingFileIsNeverArmed(t *testing.T) { } func TestLoadCorruptIsNeverArmedNotFatal(t *testing.T) { + t.Parallel() p := filepath.Join(t.TempDir(), "armed.json") if err := os.WriteFile(p, []byte("{not json"), 0o644); err != nil { t.Fatal(err) @@ -33,6 +35,7 @@ func TestLoadCorruptIsNeverArmedNotFatal(t *testing.T) { } func TestMarkUpRoundTrip(t *testing.T) { + t.Parallel() p := filepath.Join(t.TempDir(), "sub", "armed.json") t1 := time.Date(2026, 7, 7, 10, 0, 0, 0, time.UTC) if err := MarkUp(p, t1); err != nil { @@ -82,6 +85,7 @@ func TestMarkUpRoundTrip(t *testing.T) { } func TestMarkUpOverCorruptFileRecovers(t *testing.T) { + t.Parallel() p := filepath.Join(t.TempDir(), "armed.json") if err := os.WriteFile(p, []byte("{not json"), 0o644); err != nil { t.Fatal(err) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 9d8e290..7487734 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -16,6 +16,7 @@ import ( ) func TestLoadMissingPathReturnsDefaults(t *testing.T) { + t.Parallel() cfg, err := Load(filepath.Join(t.TempDir(), "does-not-exist.json")) if err != nil { t.Fatalf("Load: %v", err) @@ -26,6 +27,7 @@ func TestLoadMissingPathReturnsDefaults(t *testing.T) { } func TestLoadOverlaysAndNormalizes(t *testing.T) { + t.Parallel() path := filepath.Join(t.TempDir(), "cfg.json") body := `{ "pollInterval": "5s", @@ -58,6 +60,7 @@ func TestLoadOverlaysAndNormalizes(t *testing.T) { } func TestLoadVPNBlock(t *testing.T) { + t.Parallel() path := filepath.Join(t.TempDir(), "cfg.json") body := `{ "vpn": { @@ -86,6 +89,7 @@ func TestLoadVPNBlock(t *testing.T) { } func TestLoadVPNHostnamesAndDefaults(t *testing.T) { + t.Parallel() path := filepath.Join(t.TempDir(), "cfg.json") body := `{ "vpn": { @@ -119,6 +123,7 @@ func TestLoadVPNHostnamesAndDefaults(t *testing.T) { } func TestLoadVPNCadenceDefaults(t *testing.T) { + t.Parallel() path := filepath.Join(t.TempDir(), "cfg.json") body := `{"vpn": {"enabled": true, "tunnelInterfaces": ["utun4"], "endpoints": ["1.2.3.4"]}}` if err := os.WriteFile(path, []byte(body), 0o600); err != nil { @@ -138,6 +143,7 @@ func TestLoadVPNCadenceDefaults(t *testing.T) { // Auto-discovery with no hand-typed endpoint is a valid zero-config setup. func TestLoadVPNAutoDiscoverNoEndpoints(t *testing.T) { + t.Parallel() path := filepath.Join(t.TempDir(), "cfg.json") body := `{"vpn": {"enabled": true, "tunnelInterfaces": ["utun4"], "autoDiscoverEndpoints": true}}` if err := os.WriteFile(path, []byte(body), 0o600); err != nil { @@ -149,6 +155,7 @@ func TestLoadVPNAutoDiscoverNoEndpoints(t *testing.T) { } func TestClassifyTarget(t *testing.T) { + t.Parallel() cases := map[string]targetKind{ "203.0.113.5": kindIP, "2001:db8::1": kindIP, @@ -172,6 +179,7 @@ func TestClassifyTarget(t *testing.T) { } func TestValidateErrors(t *testing.T) { + t.Parallel() cases := map[string]string{ "bad interval": `{"pollInterval": "0s"}`, "bad hyst": `{"hysteresis": 0}`, @@ -208,6 +216,7 @@ func TestValidateErrors(t *testing.T) { // Save then Load must reproduce the same validated Config. Both configs pass // through the Load pipeline so nil/empty-slice representations match. func TestSaveLoadRoundTrip(t *testing.T) { + t.Parallel() cases := map[string]string{ "legacy": `{ "pollInterval": "5s", @@ -290,6 +299,7 @@ func TestSaveLoadRoundTrip(t *testing.T) { // otherwise re-enabling the guard later would fail validation or lock the host // out. Regression test for toFileConfig dropping the vpn block when !Enabled. func TestSavePreservesVPNFieldsWhenDisabled(t *testing.T) { + t.Parallel() cfg := Default() cfg.VPN = VPN{ TunnelInterfaces: []string{"utun4"}, @@ -319,6 +329,7 @@ func TestSavePreservesVPNFieldsWhenDisabled(t *testing.T) { // normalization lives in config.Normalize and runs on every write path, so // callers (config set, the setup wizard) don't each re-implement it. func TestSaveNormalizesCountries(t *testing.T) { + t.Parallel() cfg := Default() cfg.BlockedCountries = []string{" ir ", "IR", "ru"} path := filepath.Join(t.TempDir(), "cfg.json") @@ -336,6 +347,7 @@ func TestSaveNormalizesCountries(t *testing.T) { // Save must reject an invalid Config rather than persist it. func TestSaveValidates(t *testing.T) { + t.Parallel() bad := Default() bad.PollInterval = 0 if err := Save(filepath.Join(t.TempDir(), "x.json"), &bad); err == nil { @@ -344,6 +356,7 @@ func TestSaveValidates(t *testing.T) { } func TestLoadInvalidJSON(t *testing.T) { + t.Parallel() path := filepath.Join(t.TempDir(), "cfg.json") if err := os.WriteFile(path, []byte("{not json"), 0o600); err != nil { t.Fatal(err) @@ -354,6 +367,7 @@ func TestLoadInvalidJSON(t *testing.T) { } func TestLoadVPNProfilesAndSwitchWindow(t *testing.T) { + t.Parallel() path := filepath.Join(t.TempDir(), "cfg.json") body := `{ "vpn": { @@ -394,6 +408,7 @@ func TestLoadVPNProfilesAndSwitchWindow(t *testing.T) { // A config with no vpn.advanced block gets every knob defaulted; an explicit // block overrides only the fields it sets. func TestLoadVPNAdvancedDefaultsAndOverride(t *testing.T) { + t.Parallel() dir := t.TempDir() defPath := filepath.Join(dir, "def.json") if err := os.WriteFile(defPath, []byte(`{"vpn":{"enabled":true,"endpoints":["1.2.3.4"]}}`), 0o600); err != nil { @@ -440,6 +455,7 @@ func TestLoadVPNAdvancedDefaultsAndOverride(t *testing.T) { } func TestEffectiveEndpoints(t *testing.T) { + t.Parallel() cfg := Default() cfg.VPN.Endpoints = []string{"1.1.1.1", "dup.example.com"} cfg.VPN.Profiles = []Profile{ @@ -456,6 +472,7 @@ func TestEffectiveEndpoints(t *testing.T) { // A profiles-only config (no flat endpoints, no autoDiscover) is valid because // the union has endpoints. func TestValidateProfilesOnlyValid(t *testing.T) { + t.Parallel() path := filepath.Join(t.TempDir(), "cfg.json") body := `{"vpn":{"enabled":true,"profiles":[{"name":"a","endpoints":["1.2.3.4"]}]}}` if err := os.WriteFile(path, []byte(body), 0o600); err != nil { @@ -470,6 +487,7 @@ func TestValidateProfilesOnlyValid(t *testing.T) { // confusing derived switchWindow range: the advanced block is validated before // switchWindow is bounded against it. func TestValidateAdvancedBeforeSwitchWindow(t *testing.T) { + t.Parallel() path := filepath.Join(t.TempDir(), "cfg.json") body := `{"vpn":{"enabled":true,"endpoints":["1.2.3.4"],"switchWindow":"2m","advanced":{"switchWindowMax":"5s"}}}` if err := os.WriteFile(path, []byte(body), 0o600); err != nil { @@ -487,6 +505,7 @@ func TestValidateAdvancedBeforeSwitchWindow(t *testing.T) { // Normalize must canonicalize windowProtocols (trim + lowercase) so the pf/nft/WFP // renderers, which emit the strings verbatim, never receive " UDP" or "Tcp". func TestNormalizeWindowProtocols(t *testing.T) { + t.Parallel() cfg := Default() cfg.VPN.Advanced.WindowProtocols = []string{" UDP", "Tcp"} Normalize(&cfg) @@ -503,6 +522,7 @@ func TestNormalizeWindowProtocols(t *testing.T) { // --- automatic redial window config --- func TestRedialWindowDefaultsAndDisable(t *testing.T) { + t.Parallel() dir := t.TempDir() write := func(name, body string) string { p := filepath.Join(dir, name) @@ -562,6 +582,7 @@ func TestRedialWindowDefaultsAndDisable(t *testing.T) { // endpointGrace and autoArm must survive a save/load round-trip — a saved // config silently dropping them is how the GUI "reset to zero" bug happened. func TestSavePreservesEndpointGraceAndAutoArm(t *testing.T) { + t.Parallel() path := filepath.Join(t.TempDir(), "cfg.json") body := `{"vpn": {"enabled": true, "tunnelInterfaces": ["utun4"], "endpoints": ["1.2.3.4"], "endpointGrace": "42m", "autoArm": true}}` if err := os.WriteFile(path, []byte(body), 0o600); err != nil { @@ -593,6 +614,7 @@ func TestSavePreservesEndpointGraceAndAutoArm(t *testing.T) { // silently coerced back to a default is a security setting accepted, discarded, // and never reported. func TestPauseMaxDefaultAndDisableSentinel(t *testing.T) { + t.Parallel() path := filepath.Join(t.TempDir(), "cfg.json") body := `{"vpn": {"tunnelInterfaces": ["utun4"], "endpoints": ["1.2.3.4"]}}` if err := os.WriteFile(path, []byte(body), 0o600); err != nil { @@ -635,6 +657,7 @@ func TestPauseMaxDefaultAndDisableSentinel(t *testing.T) { // key must agree — see docs/adr/0008-arm-at-boot.md), and an explicit false // must survive a save/load round-trip rather than being silently reset. func TestArmAtBootDefaultTrueAndRoundTrips(t *testing.T) { + t.Parallel() if !Default().VPN.ArmAtBoot { t.Error("Default().VPN.ArmAtBoot = false, want true") } @@ -680,6 +703,7 @@ func TestArmAtBootDefaultTrueAndRoundTrips(t *testing.T) { // An absent endpointGrace now normalizes to the effective 15m default so // observers (GUI, config show) see the real value instead of 0. func TestEndpointGraceDefaultVisible(t *testing.T) { + t.Parallel() path := filepath.Join(t.TempDir(), "cfg.json") body := `{"vpn": {"enabled": true, "tunnelInterfaces": ["utun4"], "endpoints": ["1.2.3.4"]}}` if err := os.WriteFile(path, []byte(body), 0o600); err != nil { @@ -701,6 +725,7 @@ func TestEndpointGraceDefaultVisible(t *testing.T) { // sentinel; this asserts the pair now behaves the same and that disabling one // never disables the other. func TestWindowDisableMatrix(t *testing.T) { + t.Parallel() cases := []struct { name string body string @@ -737,6 +762,7 @@ func TestWindowDisableMatrix(t *testing.T) { // values well under the old 10s/5s floors must validate, right up to their // (now independent) caps. func TestWindowNoFloor(t *testing.T) { + t.Parallel() path := filepath.Join(t.TempDir(), "cfg.json") body := `{"vpn":{"endpoints":["1.2.3.4"],"switchWindow":"3s","redialWindow":"1s"}}` if err := os.WriteFile(path, []byte(body), 0o600); err != nil { @@ -758,6 +784,7 @@ func TestWindowNoFloor(t *testing.T) { // is a deliberate "block nothing" and must never be overridden. Both must // survive a save/load round-trip. func TestBlockedCountriesDefaultVsExplicitEmpty(t *testing.T) { + t.Parallel() dir := t.TempDir() absent := filepath.Join(dir, "absent.json") @@ -801,6 +828,7 @@ func TestBlockedCountriesDefaultVsExplicitEmpty(t *testing.T) { // A disabled switchWindow must round-trip as "0", not as the default it would // have been coerced to. func TestDisabledSwitchWindowRoundTrips(t *testing.T) { + t.Parallel() src := filepath.Join(t.TempDir(), "in.json") if err := os.WriteFile(src, []byte(`{"vpn":{"endpoints":["1.2.3.4"],"switchWindow":"0"}}`), 0o600); err != nil { t.Fatal(err) @@ -827,6 +855,7 @@ func TestDisabledSwitchWindowRoundTrips(t *testing.T) { // file behind their back. Silently accepting a discarded security setting is // the failure mode this whole mechanism exists to prevent. func TestLegacyConfigMigrates(t *testing.T) { + t.Parallel() path := filepath.Join(t.TempDir(), "legacy.json") body := `{ "pollInterval": "30s", @@ -922,6 +951,7 @@ func TestLegacyConfigMigrates(t *testing.T) { // it wrong in the "explicit false" direction silently re-opens the LAN for // someone who deliberately closed it on an untrusted network. func TestAllowLocalNetworkTriState(t *testing.T) { + t.Parallel() cases := []struct { name string body string @@ -952,6 +982,7 @@ func TestAllowLocalNetworkTriState(t *testing.T) { // An explicit false must also survive a save/reload round trip, or `config set` // on any unrelated key would quietly re-open the LAN. func TestAllowLocalNetworkFalseRoundTrips(t *testing.T) { + t.Parallel() src := filepath.Join(t.TempDir(), "in.json") if err := os.WriteFile(src, []byte(`{"vpn":{"endpoints":["1.2.3.4"],"allowLocalNetwork":false}}`), 0o600); err != nil { t.Fatal(err) @@ -984,6 +1015,7 @@ func TestAllowLocalNetworkFalseRoundTrips(t *testing.T) { // have to mean "no limit" — the opposite of what "0" means on every other key — // and there is no value that expresses it. Saying so is the only honest answer. func TestRedialBudgetZeroInTheFileIsRefused(t *testing.T) { + t.Parallel() for _, key := range []string{"redialBudget", "redialBudgetWindow"} { for _, val := range []string{"0", "0s", "-1m"} { t.Run(key+"="+val, func(t *testing.T) { @@ -1014,6 +1046,7 @@ func TestRedialBudgetZeroInTheFileIsRefused(t *testing.T) { // the parsed value: an ABSENT key is the ordinary case for both of these, and // must still take its default rather than trip the refusal above. func TestAbsentRedialBudgetTakesTheDefault(t *testing.T) { + t.Parallel() p := filepath.Join(t.TempDir(), "c.json") body := `{"vpn":{"enabled":true,"endpoints":["1.2.3.4"],"advanced":{"commandFreshness":"15s"}}}` if err := os.WriteFile(p, []byte(body), 0o600); err != nil { @@ -1038,6 +1071,7 @@ func TestAbsentRedialBudgetTakesTheDefault(t *testing.T) { // exact silent-disable this rule exists to prevent, reintroduced by a constant // nobody thought to grep for. func TestMinRedialGrantMatchesTheLedger(t *testing.T) { + t.Parallel() if minRedialGrant != redial.MinGrant { t.Fatalf("minRedialGrant = %s but redial.MinGrant = %s; the validation rule "+ "and the ledger disagree about the shortest window worth opening", @@ -1050,6 +1084,7 @@ func TestMinRedialGrantMatchesTheLedger(t *testing.T) { // on. Turning it off is fine — `vpn.redialWindow: "0"` is there for that — but // it has to be a decision, not a rounding error, so this is refused by name. func TestABudgetTooSmallToEverOpenIsRefused(t *testing.T) { + t.Parallel() for _, budget := range []time.Duration{time.Second, redial.MinGrant - time.Nanosecond} { cfg := Default() cfg.VPN.TunnelInterfaces = []string{"utun4"} @@ -1071,6 +1106,7 @@ func TestABudgetTooSmallToEverOpenIsRefused(t *testing.T) { // one shortest window, so it is a legitimate — if severe — choice and must pass. // A rule that also refused this would be tightening past what it can justify. func TestABudgetAtTheFloorIsAccepted(t *testing.T) { + t.Parallel() cfg := Default() cfg.VPN.TunnelInterfaces = []string{"utun4"} cfg.VPN.RedialWindow = 30 * time.Second @@ -1086,6 +1122,7 @@ func TestABudgetAtTheFloorIsAccepted(t *testing.T) { // that does not apply would refuse a working config while stating a reason that // is not true of it. func TestTheBudgetFloorFollowsAShortConfiguredWindow(t *testing.T) { + t.Parallel() cfg := Default() cfg.VPN.TunnelInterfaces = []string{"utun4"} cfg.VPN.RedialWindow = 3 * time.Second // below redial.MinGrant, honoured as-is @@ -1109,6 +1146,7 @@ func TestTheBudgetFloorFollowsAShortConfiguredWindow(t *testing.T) { // with vpn.redialWindow disabled the budget is inert, so complaining about its // size would block a config that has no automatic window to break. func TestTheBudgetFloorIsMootWhenTheWindowIsDisabled(t *testing.T) { + t.Parallel() cfg := Default() cfg.VPN.TunnelInterfaces = []string{"utun4"} cfg.VPN.RedialWindow = Disabled diff --git a/internal/config/docdrift_test.go b/internal/config/docdrift_test.go index 0d2e578..ee7623e 100644 --- a/internal/config/docdrift_test.go +++ b/internal/config/docdrift_test.go @@ -39,6 +39,7 @@ var docDefaultIsProse = map[string]string{ // config reference that disagrees with the shipped default is worse than no // documentation: a reader acts on it. func TestDocumentedDefaultsMatchTheCode(t *testing.T) { + t.Parallel() documented := parseDocDefaults(t, docConfigRef) for _, tun := range Tunables() { @@ -88,6 +89,7 @@ func TestDocumentedDefaultsMatchTheCode(t *testing.T) { // assertion. What must never happen is an example naming a key that no longer // takes effect. func TestExampleConfigsUseOnlyKnownKeys(t *testing.T) { + t.Parallel() paths, err := filepath.Glob(filepath.Join(exampleDir, "*.json")) if err != nil { t.Fatalf("glob %s: %v", exampleDir, err) diff --git a/internal/config/pause_test.go b/internal/config/pause_test.go index ed93ff1..8a30466 100644 --- a/internal/config/pause_test.go +++ b/internal/config/pause_test.go @@ -10,6 +10,7 @@ import ( // to `dezhban pause`, so a Value that does not parse back to its own Duration // would silently grant a different pause than the label promised. func TestPauseOptionValuesRoundTrip(t *testing.T) { + t.Parallel() c := Default() Normalize(&c) for _, o := range PauseOptions(&c) { @@ -30,6 +31,7 @@ func TestPauseOptionValuesRoundTrip(t *testing.T) { // Over-cap options are LISTED and explained, never hidden and never shortened. // Hiding them teaches the user their cap is something other than it is. func TestPauseOptionsMarkOverCapWithoutHiding(t *testing.T) { + t.Parallel() c := Default() Normalize(&c) c.VPN.PauseMax = 20 * time.Minute @@ -56,6 +58,7 @@ func TestPauseOptionsMarkOverCapWithoutHiding(t *testing.T) { // With pausing off, every option is unavailable and says why — the reason is // the disabled setting, not the length. func TestPauseOptionsWhenPausingIsDisabled(t *testing.T) { + t.Parallel() c := Default() Normalize(&c) c.VPN.PauseMax = Disabled @@ -73,6 +76,7 @@ func TestPauseOptionsWhenPausingIsDisabled(t *testing.T) { // PauseRefusal is where "clamp nothing silently" lives: an over-cap request is // refused and explained rather than quietly shortened. func TestPauseRefusal(t *testing.T) { + t.Parallel() c := Default() Normalize(&c) c.VPN.PauseMax = 30 * time.Minute diff --git a/internal/config/preset_test.go b/internal/config/preset_test.go index f386532..77c1f62 100644 --- a/internal/config/preset_test.go +++ b/internal/config/preset_test.go @@ -7,6 +7,7 @@ import ( ) func TestPresetsAreWellFormed(t *testing.T) { + t.Parallel() for _, p := range Presets() { t.Run(p.Name, func(t *testing.T) { if p.Summary == "" { @@ -31,6 +32,7 @@ func TestPresetsAreWellFormed(t *testing.T) { // config — the round-trip a preset must survive before it's ever offered to // a user. func TestPresetApplyValidates(t *testing.T) { + t.Parallel() base := Default() base.VPN.Endpoints = []string{"203.0.113.9"} // a config with a known endpoint validates cleanly Normalize(&base) @@ -50,6 +52,7 @@ func TestPresetApplyValidates(t *testing.T) { } func TestStrictPresetDisablesAllThreeWindowsAndSurvivesNormalize(t *testing.T) { + t.Parallel() base := Default() strict, _ := PresetByName("strict") cfg, err := strict.apply(base) @@ -72,6 +75,7 @@ func TestStrictPresetDisablesAllThreeWindowsAndSurvivesNormalize(t *testing.T) { // TestBalancedPresetMatchesDefault pins that the shipped defaults and the // middle preset can never silently disagree. func TestBalancedPresetMatchesDefault(t *testing.T) { + t.Parallel() def := Default() Normalize(&def) @@ -85,6 +89,7 @@ func TestBalancedPresetMatchesDefault(t *testing.T) { } func TestPresetDriftEmptyForExactMatch(t *testing.T) { + t.Parallel() base := Default() relaxed, _ := PresetByName("relaxed") cfg, err := relaxed.apply(base) @@ -99,6 +104,7 @@ func TestPresetDriftEmptyForExactMatch(t *testing.T) { } func TestPresetDriftNamesExactlyTheDivergentKeys(t *testing.T) { + t.Parallel() base := Default() balanced, _ := PresetByName("balanced") cfg, err := balanced.apply(base) @@ -120,6 +126,7 @@ func TestPresetDriftNamesExactlyTheDivergentKeys(t *testing.T) { } func TestMatchPresetReportsCustomWhenDrifted(t *testing.T) { + t.Parallel() base := Default() balanced, _ := PresetByName("balanced") cfg, err := balanced.apply(base) @@ -135,6 +142,7 @@ func TestMatchPresetReportsCustomWhenDrifted(t *testing.T) { } func TestMatchPresetFindsExactMatch(t *testing.T) { + t.Parallel() def := Default() Normalize(&def) if name, exact := MatchPreset(&def); !exact || name != "balanced" { @@ -143,6 +151,7 @@ func TestMatchPresetFindsExactMatch(t *testing.T) { } func TestPresetByNameIsCaseInsensitive(t *testing.T) { + t.Parallel() if _, ok := PresetByName("STRICT"); !ok { t.Error("PresetByName(\"STRICT\") not found") } @@ -158,6 +167,7 @@ func TestPresetByNameIsCaseInsensitive(t *testing.T) { // time. And the preset must NOT resolve it by raising the cap: the cap is the // operator's own ceiling on a sanctioned relaxation of the guard. func TestPresetConflictsAgainstLoweredAdvancedCaps(t *testing.T) { + t.Parallel() base := Default() Normalize(&base) if got := PresetConflicts(&base, mustPreset(t, "relaxed")); len(got) != 0 { @@ -195,6 +205,7 @@ func TestPresetConflictsAgainstLoweredAdvancedCaps(t *testing.T) { // or applying a "strictness" macro would silently raise a ceiling the operator // set by hand. func TestPresetsNeverSetTheAdvancedCaps(t *testing.T) { + t.Parallel() for _, k := range presetKeys { if k == "vpn.advanced.switchWindowMax" || k == "vpn.advanced.redialWindowMax" { t.Errorf("presetKeys contains %q — a preset must never widen a cap the operator set", k) diff --git a/internal/config/reload_test.go b/internal/config/reload_test.go index ca109ec..4443ba3 100644 --- a/internal/config/reload_test.go +++ b/internal/config/reload_test.go @@ -20,6 +20,7 @@ func changeFor(t *testing.T, changes []Change, key string) Change { // reload would report a pile of phantom edits and the user could never tell a // real one from noise. func TestChangesEmptyForIdenticalConfigs(t *testing.T) { + t.Parallel() a, b := Default(), Default() if got := Changes(&a, &b); len(got) != 0 { t.Fatalf("Changes on identical configs = %v, want none", got) @@ -29,6 +30,7 @@ func TestChangesEmptyForIdenticalConfigs(t *testing.T) { // The two halves of the answer: what changed, and whether the running daemon can // actually adopt it. func TestChangesClassifiesLiveAndRestartRequired(t *testing.T) { + t.Parallel() old := Default() cur := Default() cur.PollInterval = 42 * time.Second // live: the run loop owns the geo ticker @@ -69,6 +71,7 @@ func TestChangesClassifiesLiveAndRestartRequired(t *testing.T) { // as a raw duration it renders "-1ns", which tells a user nothing and looks like // corruption in a reload report. func TestKeyValuesRendersDisabledWindowsAsOff(t *testing.T) { + t.Parallel() c := Default() c.VPN.SwitchWindow = Disabled c.VPN.RedialWindow = Disabled @@ -85,6 +88,7 @@ func TestKeyValuesRendersDisabledWindowsAsOff(t *testing.T) { // Turning a window off is a security-relevant edit, so it must show up as a // change like any other rather than being swallowed. func TestChangesReportsDisablingAWindow(t *testing.T) { + t.Parallel() old := Default() cur := Default() cur.VPN.RedialWindow = Disabled @@ -102,6 +106,7 @@ func TestChangesReportsDisablingAWindow(t *testing.T) { // defaults to restart-required, which is safe but wrong to leave in place — this // is the test that makes someone decide. func TestEveryKeyIsClassifiedExactlyOnce(t *testing.T) { + t.Parallel() c := Default() for key := range KeyValues(&c) { _, restart := restartReasons[key] @@ -128,6 +133,7 @@ func TestEveryKeyIsClassifiedExactlyOnce(t *testing.T) { // Every restart reason is shown to a user, so an empty one would render as a // blank explanation next to a setting that silently did not take effect. func TestEveryRestartReasonExplainsItself(t *testing.T) { + t.Parallel() for key, reason := range restartReasons { if reason == "" { t.Errorf("key %q is restart-required with no reason given", key) @@ -140,6 +146,7 @@ func TestEveryRestartReasonExplainsItself(t *testing.T) { // from what it is actually enforcing, and the key would stop being reported as // pending — the user would never learn a restart was still owed. func TestMergeLiveMovesOnlyLiveKeys(t *testing.T) { + t.Parallel() base := Default() cur := Default() @@ -172,6 +179,7 @@ func TestMergeLiveMovesOnlyLiveKeys(t *testing.T) { // every live key must actually be moved. This is what keeps the field-by-field // copy above honest against the liveKeys table beside it. func TestMergeLiveCoversExactlyTheLiveKeys(t *testing.T) { + t.Parallel() base := Default() // A config that differs from base in every single key. diff --git a/internal/config/schema_test.go b/internal/config/schema_test.go index e47bfbc..cb3778b 100644 --- a/internal/config/schema_test.go +++ b/internal/config/schema_test.go @@ -16,6 +16,7 @@ import ( // no help, and no default for any surface to show; a Tunable for a key KeyValues // does not carry would describe something nobody can set. func TestTunablesCoverEverySettableKey(t *testing.T) { + t.Parallel() c := Default() Normalize(&c) settable := KeyValues(&c) @@ -44,6 +45,7 @@ func TestTunablesCoverEverySettableKey(t *testing.T) { // If Tunables ever grew a hand-written Default, this is what would catch it // disagreeing with what you actually get by setting nothing. func TestTunableDefaultsMatchANormalizedDefaultConfig(t *testing.T) { + t.Parallel() c := Default() Normalize(&c) want := KeyValues(&c) @@ -61,6 +63,7 @@ func TestTunableDefaultsMatchANormalizedDefaultConfig(t *testing.T) { // path reports it as restart-required is the same failure as claiming a change // applied when the old value is still being enforced. func TestTunableRestartClassificationMatchesReload(t *testing.T) { + t.Parallel() for _, tun := range Tunables() { want := restartReasonFor(tun.Key) if tun.RestartReason != want { @@ -75,6 +78,7 @@ func TestTunableRestartClassificationMatchesReload(t *testing.T) { // TestTunableCapKeysResolve — a cap is a key rather than a number precisely so a // surface can read the live ceiling, which only works if the key exists. func TestTunableCapKeysResolve(t *testing.T) { + t.Parallel() for _, tun := range Tunables() { if tun.CapKey == "" { continue @@ -100,6 +104,7 @@ func TestTunableCapKeysResolve(t *testing.T) { // that promise is only honest if the negative sentinel actually survives // Normalize instead of being coerced back to the default. func TestDisablableKeysSurviveNormalize(t *testing.T) { + t.Parallel() // Field accessors kept test-local: the production key→field table lives in // cmd/dezhban's configFields, and internal/config must not depend on it. fields := map[string]func(*Config) *time.Duration{ @@ -143,6 +148,7 @@ func TestDisablableKeysSurviveNormalize(t *testing.T) { // NOT marked Disablable must be one where Normalize really does restore the // default, so no surface offers an "Off" that would silently do nothing. func TestNonDisablableDurationsCoerceZeroToTheirDefault(t *testing.T) { + t.Parallel() for _, tun := range Tunables() { if tun.Kind != KindDuration || tun.Disablable { continue @@ -156,6 +162,7 @@ func TestNonDisablableDurationsCoerceZeroToTheirDefault(t *testing.T) { // TestTunableMetadataIsComplete — every field a surface relies on is populated, // and the ones that are meaningful for only one Kind stay empty elsewhere. func TestTunableMetadataIsComplete(t *testing.T) { + t.Parallel() for _, tun := range Tunables() { if tun.Label == "" { t.Errorf("%s: no Label", tun.Key) @@ -192,6 +199,7 @@ func TestTunableMetadataIsComplete(t *testing.T) { // TestTunableKeysAreSorted — TunableKeys promises a stable order to CLI help and // tests, which is only useful if it is actually stable. func TestTunableKeysAreSorted(t *testing.T) { + t.Parallel() keys := TunableKeys() if len(keys) != len(tunables) { t.Fatalf("TunableKeys returned %d keys, want %d", len(keys), len(tunables)) @@ -206,6 +214,7 @@ func TestTunableKeysAreSorted(t *testing.T) { // TestTunablesReturnsACopy — a surface that mutates what it is handed must not // change what the next caller sees. func TestTunablesReturnsACopy(t *testing.T) { + t.Parallel() first := Tunables() first[0].Label = "mutated" if Tunables()[0].Label == "mutated" { diff --git a/internal/config/unknown_test.go b/internal/config/unknown_test.go index 97f139a..49108d7 100644 --- a/internal/config/unknown_test.go +++ b/internal/config/unknown_test.go @@ -36,6 +36,7 @@ func retiredKeys(cfg *Config) []string { // default. For a window that someone deliberately disabled, that quietly // re-enables a relaxation of the guard. func TestUnknownKeysAreReported(t *testing.T) { + t.Parallel() cfg := loadFromJSON(t, `{ "pollInterval": "20s", "notAKey": true, @@ -53,6 +54,7 @@ func TestUnknownKeysAreReported(t *testing.T) { // A renamed key has to say what replaced it. "not recognised" sends someone // hunting through docs for a key that simply moved. func TestRenamedKeysPointAtTheirReplacement(t *testing.T) { + t.Parallel() cfg := loadFromJSON(t, `{"vpn": {"reconnectWindow": "0"}}`) var found bool @@ -77,6 +79,7 @@ func TestRenamedKeysPointAtTheirReplacement(t *testing.T) { // class of failure as silently discarding one, and the more dangerous // direction: they stop looking while the value is in force. func TestMiscasedAutodetectIsReportedAsHavingTakenEffect(t *testing.T) { + t.Parallel() cfg := loadFromJSON(t, `{"vpn": {"autodetect": false}}`) // The value is live: vpn.autoDetect defaults to TRUE, so a silently @@ -110,6 +113,7 @@ func TestMiscasedAutodetectIsReportedAsHavingTakenEffect(t *testing.T) { // miscased key has to be reported: the report is the only thing that tells an // operator their config has two keys fighting over one setting. func TestBothAutoDetectSpellingsPresentIsReported(t *testing.T) { + t.Parallel() for _, body := range []string{ `{"vpn": {"autodetect": true, "autoDetect": false}}`, `{"vpn": {"autoDetect": false, "autodetect": true}}`, @@ -127,6 +131,7 @@ func TestBothAutoDetectSpellingsPresentIsReported(t *testing.T) { // failure this file exists to prevent, just one JSON container type away from // where it was already caught for vpn/vpn.advanced/control. func TestUnknownKeysInsideProfilesAreReported(t *testing.T) { + t.Parallel() cfg := loadFromJSON(t, `{ "vpn": { "profiles": [ @@ -151,6 +156,7 @@ func TestUnknownKeysInsideProfilesAreReported(t *testing.T) { // ifaceHint set on ANY profile must be told so, with the index that pinpoints // which entry. func TestOldIfaceHintInsideAProfileIsReportedAsRenamed(t *testing.T) { + t.Parallel() cfg := loadFromJSON(t, `{ "vpn": { "profiles": [ @@ -180,6 +186,8 @@ func TestOldIfaceHintInsideAProfileIsReportedAsRenamed(t *testing.T) { // one map entry rather than needing one per index. Uses a synthetic entry // (saved/restored) rather than depending on any real rename existing. func TestRenamedKeyInsideAnArrayElementIsNormalised(t *testing.T) { + // No t.Parallel(): mutates the package-level renamedKeys map directly, which + // races with any parallel sibling test reading it via lookupRenamed. const oldKey, newKey = "vpn.profiles[].syntheticOld", "vpn.profiles[].syntheticNew" renamedKeys[oldKey] = newKey t.Cleanup(func() { delete(renamedKeys, oldKey) }) @@ -200,6 +208,7 @@ func TestRenamedKeyInsideAnArrayElementIsNormalised(t *testing.T) { // setting is running, which is the lie this whole file exists to prevent, // reached from the one direction the case-folding branch didn't cover. func TestMiscasedRetiredKeyIsNotReportedAsLive(t *testing.T) { + t.Parallel() cfg := loadFromJSON(t, `{ "FailClosed": true, "vpn": { "tunnelInterfaces": ["utun4"], "endpoints": ["1.2.3.4"], "Enabled": false } @@ -219,6 +228,7 @@ func TestMiscasedRetiredKeyIsNotReportedAsLive(t *testing.T) { // outcomes, which is exactly the accident TestRenameHintSurvivesAMiscasedParent // Block pins for the neighbouring lookup. func TestMiscasedRetiredKeyUnderAMiscasedParentIsNotReportedAsLive(t *testing.T) { + t.Parallel() cfg := loadFromJSON(t, `{"VPN": {"Enabled": true}}`) assertReportedRetired(t, cfg, "VPN.Enabled") } @@ -248,6 +258,7 @@ func assertReportedRetired(t *testing.T, cfg *Config, miscased string) { // schema's own name — a leaf-only fix would still emit "VPN.profiles", which // exists nowhere and leaves the one actionable part of the line wrong. func TestCanonicalSpellingIsSchemaCasedAtEveryLevel(t *testing.T) { + t.Parallel() cfg := loadFromJSON(t, `{"VPN": {"Profiles": [{"name": "w", "endpoints": ["1.2.3.4"]}]}}`) want := map[string]string{"VPN": `"vpn"`, "VPN.Profiles": `"vpn.profiles"`} @@ -277,6 +288,7 @@ func TestCanonicalSpellingIsSchemaCasedAtEveryLevel(t *testing.T) { // while the value is in force. Both halves are asserted together on purpose: // the value landing, and the report admitting it. func TestMiscasedKeysTakeEffectAndAreReportedAsSuch(t *testing.T) { + t.Parallel() cfg := loadFromJSON(t, `{ "pollinterval": "1h", "vpn": { "PAUSEMAX": "2h", "advanced": { "RedialMinUptime": "0s" } } @@ -312,6 +324,7 @@ func TestMiscasedKeysTakeEffectAndAreReportedAsSuch(t *testing.T) { // vpn, so a typo nested under it is just as inert as one under the correctly // spelled block, and just as much worth reporting. func TestTyposUnderAMiscasedBlockAreStillReported(t *testing.T) { + t.Parallel() cfg := loadFromJSON(t, `{"VPN": {"tunnelInterfaces": ["utun4"], "nonsenseKey": 1}}`) got := retiredKeys(cfg) @@ -325,6 +338,7 @@ func TestTyposUnderAMiscasedBlockAreStillReported(t *testing.T) { // A valid config must stay quiet, or the report becomes noise nobody reads. func TestKnownKeysAreNotReportedAsUnknown(t *testing.T) { + t.Parallel() cfg := loadFromJSON(t, `{ "pollInterval": "20s", "hysteresis": 2, @@ -347,6 +361,7 @@ func TestKnownKeysAreNotReportedAsUnknown(t *testing.T) { // report's line order stops matching the file's, and "the third one" no longer // means the third line. See sortKey. func TestArrayIndexedKeysSortNumerically(t *testing.T) { + t.Parallel() var profiles []string for i := range 12 { profiles = append(profiles, `{"name":"p`+strconv.Itoa(i)+`","endpoints":["198.51.100.1"],"ifaceHint":"wg"}`) @@ -381,6 +396,7 @@ func TestArrayIndexedKeysSortNumerically(t *testing.T) { // both spellings — a renamed key has no struct field for the decoder's // case-insensitive fallback to land on — so the hint is right either way. func TestRenameHintSurvivesAMiscasedParentBlock(t *testing.T) { + t.Parallel() cases := []string{ "vpn.profiles[1].ifaceHint", // exact "VPN.profiles[1].ifaceHint", // parent miscased @@ -400,6 +416,7 @@ func TestRenameHintSurvivesAMiscasedParentBlock(t *testing.T) { // End to end through the loader: the note a user actually reads for a miscased // parent block must carry the replacement name. func TestMiscasedParentBlockStillReportsTheRename(t *testing.T) { + t.Parallel() cfg := loadFromJSON(t, `{"VPN": {"profiles": [{"name":"a","endpoints":["1.2.3.4"],"ifaceHint":"wg"}]}}`) var found bool for _, r := range cfg.Retired { diff --git a/internal/control/control_test.go b/internal/control/control_test.go index 55fa4f5..0bf2d3b 100644 --- a/internal/control/control_test.go +++ b/internal/control/control_test.go @@ -263,6 +263,11 @@ func TestSlowRunLoopStillGetsItsReplyThrough(t *testing.T) { // Answer from the run loop only after the old connection-wide deadline would // have fired. The reply must still arrive intact. + // + // Deliberate exception to the no-sleep test policy: the delay past + // connDeadline IS the scenario under test, not a wait for one — there is + // no condition to poll for, and shortening it would silently stop + // exercising the regression this test exists to catch. go func() { select { case cr := <-srv.Requests(): diff --git a/internal/decision/decision_test.go b/internal/decision/decision_test.go index baf5085..97760c9 100644 --- a/internal/decision/decision_test.go +++ b/internal/decision/decision_test.go @@ -39,6 +39,7 @@ func assertSeq(t *testing.T, got, want []Verdict) { // hysteresis=1 → behaves like the pure Phase-3 mapping (immediate toggle). func TestNoHysteresisImmediateToggle(t *testing.T) { + t.Parallel() d := New([]string{"IR", "ru"}, 1) // mixed case → normalized got := feed(d, []monitor.Result{ ok("US"), // allow @@ -52,6 +53,7 @@ func TestNoHysteresisImmediateToggle(t *testing.T) { } func TestHysteresisRequiresConsecutiveAgreement(t *testing.T) { + t.Parallel() d := New([]string{"IR"}, 3) got := feed(d, []monitor.Result{ ok("IR"), // streak 1 → still Allow @@ -68,6 +70,7 @@ func TestHysteresisRequiresConsecutiveAgreement(t *testing.T) { // A reading that agrees with the committed state resets a pending flip, so an // alternating sequence never flaps the firewall. func TestFlapResetsStreak(t *testing.T) { + t.Parallel() d := New([]string{"IR"}, 3) got := feed(d, []monitor.Result{ ok("IR"), // streak 1 toward Block @@ -87,6 +90,7 @@ func TestFlapResetsStreak(t *testing.T) { // would cut the tunnel's own egress and livelock the redial that could fix // the lookup in the first place. func TestErrorsNeverCommitABlock(t *testing.T) { + t.Parallel() d := New([]string{"IR"}, 3) got := feed(d, []monitor.Result{ fail(), // neutral @@ -105,6 +109,7 @@ func TestErrorsNeverCommitABlock(t *testing.T) { // retired fail-open path did the latter, which let an exit that fails lookups // every other tick postpone its block indefinitely. func TestErrorMidStreakDoesNotCancelPendingFlip(t *testing.T) { + t.Parallel() d := New([]string{"IR"}, 3) got := feed(d, []monitor.Result{ ok("IR"), // streak 1 @@ -117,18 +122,21 @@ func TestErrorMidStreakDoesNotCancelPendingFlip(t *testing.T) { // The mirror case: once blocked, errors must not lift the block either. func TestErrorsDoNotLiftACommittedBlock(t *testing.T) { + t.Parallel() d := New([]string{"IR"}, 1) got := feed(d, []monitor.Result{ok("IR"), fail(), fail()}) assertSeq(t, got, []Verdict{Block, Block, Block}) } func TestEmptyBlocklistAllowsEverything(t *testing.T) { + t.Parallel() d := New(nil, 1) got := feed(d, []monitor.Result{ok("IR"), ok("KP"), fail()}) assertSeq(t, got, []Verdict{Allow, Allow, Allow}) } func TestHysteresisFloorIsOne(t *testing.T) { + t.Parallel() d := New([]string{"IR"}, 0) // clamped to 1 got := feed(d, []monitor.Result{ok("IR")}) assertSeq(t, got, []Verdict{Block}) diff --git a/internal/help/manifest.go b/internal/help/manifest.go index aa2f8a8..5e8c45f 100644 --- a/internal/help/manifest.go +++ b/internal/help/manifest.go @@ -62,6 +62,10 @@ var Pages = []Page{ Source: "usage/cli.md", Title: "Command reference", Summary: "Every command and flag, and which of them need root.", }, + { + Source: "usage/passwordless.md", Title: "Using the CLI without sudo", + Summary: "Turn on control.group so routine ops need no password.", + }, { Source: "concepts/glossary.md", Title: "Glossary", Summary: "One word per concept — the words this app, the CLI, and the docs all use.", diff --git a/internal/learned/learned_test.go b/internal/learned/learned_test.go index 84444b1..d04fb0d 100644 --- a/internal/learned/learned_test.go +++ b/internal/learned/learned_test.go @@ -20,6 +20,7 @@ func addr(t *testing.T, s string) netip.Addr { } func TestLoadMissingFileIsEmpty(t *testing.T) { + t.Parallel() s, err := Load(filepath.Join(t.TempDir(), "nope.json")) if err != nil { t.Fatalf("Load missing: %v", err) @@ -30,6 +31,7 @@ func TestLoadMissingFileIsEmpty(t *testing.T) { } func TestLoadCorruptIsEmptyNotFatal(t *testing.T) { + t.Parallel() p := filepath.Join(t.TempDir(), "learned.json") if err := os.WriteFile(p, []byte("{not json"), 0o644); err != nil { t.Fatal(err) @@ -44,6 +46,7 @@ func TestLoadCorruptIsEmptyNotFatal(t *testing.T) { } func TestRecordAndSaveRoundTrip(t *testing.T) { + t.Parallel() p := filepath.Join(t.TempDir(), "sub", "learned.json") now := time.Date(2026, 7, 7, 10, 0, 0, 0, time.UTC) s := &Store{} @@ -74,6 +77,7 @@ func TestRecordAndSaveRoundTrip(t *testing.T) { } func TestRecordRefreshesLastSeenAndDedupes(t *testing.T) { + t.Parallel() t0 := time.Date(2026, 7, 7, 10, 0, 0, 0, time.UTC) t1 := t0.Add(time.Hour) s := &Store{} @@ -92,6 +96,7 @@ func TestRecordRefreshesLastSeenAndDedupes(t *testing.T) { // handling), so "Proton" and "proton" merge into one entry rather than bloating // the store with case-variant duplicates. func TestRecordMergesCaseInsensitively(t *testing.T) { + t.Parallel() t0 := time.Date(2026, 7, 7, 10, 0, 0, 0, time.UTC) s := &Store{} s.Record("Proton", "", "discovery", []netip.Addr{addr(t, "1.2.3.4")}, 16, t0) @@ -112,6 +117,7 @@ func TestRecordMergesCaseInsensitively(t *testing.T) { } func TestRecordEnforcesPerEntryCap(t *testing.T) { + t.Parallel() base := time.Date(2026, 7, 7, 10, 0, 0, 0, time.UTC) s := &Store{} // 5 addresses, cap 3 → the 3 most-recently-seen survive. @@ -131,6 +137,7 @@ func TestRecordEnforcesPerEntryCap(t *testing.T) { } func TestPruneDropsExpiredAndEmptyEntries(t *testing.T) { + t.Parallel() now := time.Date(2026, 7, 7, 10, 0, 0, 0, time.UTC) s := &Store{} s.Record("old", "", "discovery", []netip.Addr{addr(t, "1.1.1.1")}, 16, now.Add(-48*time.Hour)) @@ -145,6 +152,7 @@ func TestPruneDropsExpiredAndEmptyEntries(t *testing.T) { } func TestForgetAndAddrs(t *testing.T) { + t.Parallel() now := time.Date(2026, 7, 7, 10, 0, 0, 0, time.UTC) s := &Store{} s.Record("a", "", "", []netip.Addr{addr(t, "3.3.3.3"), addr(t, "1.1.1.1")}, 16, now) @@ -169,6 +177,7 @@ func TestForgetAndAddrs(t *testing.T) { } func TestSaveWritesSchemaVersion(t *testing.T) { + t.Parallel() p := filepath.Join(t.TempDir(), "learned.json") if err := (&Store{}).Save(p); err != nil { t.Fatal(err) diff --git a/internal/redial/redial_test.go b/internal/redial/redial_test.go index 97d4a81..ea6e0ec 100644 --- a/internal/redial/redial_test.go +++ b/internal/redial/redial_test.go @@ -22,6 +22,7 @@ var t0 = time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) // The ordinary drop: a tunnel that was up long enough gets the whole window and // arms no cooldown. This is the case the budget must not make worse. func TestHealthyDropGetsTheFullWindow(t *testing.T) { + t.Parallel() s := defaults() b := New() @@ -41,6 +42,7 @@ func TestHealthyDropGetsTheFullWindow(t *testing.T) { // — is not flapping, however short the uptime. Treating it as a flap would back // off from a VPN that is working. func TestConfirmedExitIsNotAFlap(t *testing.T) { + t.Parallel() s := defaults() b := New() @@ -54,6 +56,7 @@ func TestConfirmedExitIsNotAFlap(t *testing.T) { // The old anti-flap gate refused outright, which pushed the user onto the manual // path at exactly the moment the automatic one was most useful. func TestFastDropIsShortenedNotRefused(t *testing.T) { + t.Parallel() s := defaults() b := New() @@ -72,6 +75,7 @@ func TestFastDropIsShortenedNotRefused(t *testing.T) { // Consecutive fast drops halve the window and lengthen the cooldown, so a // pathological flap decays toward "cut and holding" instead of chaining windows. func TestConsecutiveFastDropsBackOff(t *testing.T) { + t.Parallel() s := defaults() b := New() @@ -113,6 +117,7 @@ func TestConsecutiveFastDropsBackOff(t *testing.T) { // A healthy uptime clears the backoff. Without this a single bad afternoon would // keep shortening windows for a VPN that had since recovered. func TestHealthyUptimeResetsTheBackoff(t *testing.T) { + t.Parallel() s := defaults() b := New() @@ -139,6 +144,7 @@ func TestHealthyUptimeResetsTheBackoff(t *testing.T) { // there is not a short wait — a refusal is only re-decided on the next // tunnel-down edge, so it stands until the operator opens a window by hand. func TestARecoveredTunnelIsNotHeldByTheCooldown(t *testing.T) { + t.Parallel() s := defaults() // window 30s, minUptime 15s b := New() @@ -183,6 +189,7 @@ func TestARecoveredTunnelIsNotHeldByTheCooldown(t *testing.T) { // threshold with no confirmed exit is exactly the flap it rations, and clearing // it on evidence must not amount to clearing it on arrival. func TestTheCooldownStillHoldsWithoutEvidence(t *testing.T) { + t.Parallel() s := defaults() b := New() @@ -216,6 +223,7 @@ func TestTheCooldownStillHoldsWithoutEvidence(t *testing.T) { // the run loop's retry timer and internal/render lean on this instant being real // — one arms against it, the other prints it ("dezhban tries again at …"). func TestACooldownRefusalAlsoAnswersForTheBudget(t *testing.T) { + t.Parallel() // A budget that affords the first window and then almost nothing: 17s buys // the 15s first grant and leaves 2s, below the 5s floor. s := Settings{ @@ -256,6 +264,7 @@ func TestACooldownRefusalAlsoAnswersForTheBudget(t *testing.T) { // The bound the ADR exists to add: total open time inside the rolling interval // cannot exceed the budget, however many drops occur. func TestBudgetIsExhaustedAndHolds(t *testing.T) { + t.Parallel() s := defaults() s.MinUptime = 0 // isolate the budget from the backoff b := New() @@ -297,6 +306,7 @@ func TestBudgetIsExhaustedAndHolds(t *testing.T) { // costs. Charging the offer would punish the successful redial — the exact // outcome the window exists to produce. func TestEarlyCloseCreditsTheUnusedRemainder(t *testing.T) { + t.Parallel() s := defaults() s.MinUptime = 0 b := New() @@ -320,6 +330,7 @@ func TestEarlyCloseCreditsTheUnusedRemainder(t *testing.T) { // An open window is committed, not free. Reporting it as available would let a // surface promise room that is already claimed. func TestAnOpenWindowCountsAtItsFullGrant(t *testing.T) { + t.Parallel() s := defaults() s.MinUptime = 0 b := New() @@ -335,6 +346,7 @@ func TestAnOpenWindowCountsAtItsFullGrant(t *testing.T) { // is merely busy gets help back promptly instead of waiting for a full period of // silence. func TestBudgetRefillsPerEpisode(t *testing.T) { + t.Parallel() s := defaults() s.MinUptime = 0 b := New() @@ -374,6 +386,7 @@ func TestBudgetRefillsPerEpisode(t *testing.T) { // leaving a client time to hand-shake. Refusing keeps the remainder for a drop // that can use it. func TestASliverIsRefusedRatherThanSpent(t *testing.T) { + t.Parallel() s := Settings{Window: 30 * time.Second, Budget: 33 * time.Second, Interval: 15 * time.Minute} b := New() @@ -394,6 +407,7 @@ func TestASliverIsRefusedRatherThanSpent(t *testing.T) { // A budget that can afford something, but less than a full window, opens for // what it has — truncation is honest, and the client may well redial inside it. func TestAPartialBudgetTruncatesTheWindow(t *testing.T) { + t.Parallel() s := Settings{Window: 30 * time.Second, Budget: 50 * time.Second, Interval: 15 * time.Minute} b := New() @@ -415,6 +429,7 @@ func TestAPartialBudgetTruncatesTheWindow(t *testing.T) { // asked — because MinGrant said so — would be the mirror image of silently // discarding a security setting. func TestAShortConfiguredWindowIsHonoured(t *testing.T) { + t.Parallel() s := Settings{Window: 2 * time.Second, Budget: time.Minute, Interval: 15 * time.Minute, MinUptime: 15 * time.Second} b := New() @@ -427,6 +442,7 @@ func TestAShortConfiguredWindowIsHonoured(t *testing.T) { // sentinel, so "0" must mean every qualifying drop gets a full window until the // budget itself runs out. func TestBackoffDisabledByZeroMinUptime(t *testing.T) { + t.Parallel() s := defaults() s.MinUptime = 0 b := New() @@ -444,6 +460,7 @@ func TestBackoffDisabledByZeroMinUptime(t *testing.T) { // Close with nothing open is a no-op, so the run loop's close paths need no // "was this an automatic window" bookkeeping of their own. func TestCloseWithNothingOpenIsHarmless(t *testing.T) { + t.Parallel() s := defaults() b := New() b.Close(t0) @@ -459,6 +476,7 @@ func TestCloseWithNothingOpenIsHarmless(t *testing.T) { // but it is the only way to reach the case, and "the longest windows are free" // is not an acceptable answer to it. func TestAnOpenEpisodeIsNeverAgedOut(t *testing.T) { + t.Parallel() s := Settings{Window: 30 * time.Second, Budget: 2 * time.Minute, Interval: 10 * time.Second} b := New() @@ -485,6 +503,7 @@ func TestAnOpenEpisodeIsNeverAgedOut(t *testing.T) { // ledger rolls over and the guard keeps holding on a cooldown built entirely out // of drops it declined to assist with. func TestARefusedDropDoesNotDeepenTheBackoff(t *testing.T) { + t.Parallel() s := defaults() b := New() @@ -524,6 +543,7 @@ func TestARefusedDropDoesNotDeepenTheBackoff(t *testing.T) { // above would read as "the backoff never engages", which is the behaviour // ADR-0009 replaced arriving by the back door. func TestAGrantedFastDropStillCommitsTheBackoff(t *testing.T) { + t.Parallel() s := defaults() b := New() @@ -545,6 +565,7 @@ func TestAGrantedFastDropStillCommitsTheBackoff(t *testing.T) { // refusal that looks like one while quietly holding a slot the next Close would // settle in place of a real window. func TestADisabledWindowIsRefusedWithoutTouchingTheLedger(t *testing.T) { + t.Parallel() s := defaults() s.Window = 0 b := New() @@ -573,6 +594,7 @@ func TestADisabledWindowIsRefusedWithoutTouchingTheLedger(t *testing.T) { // keeps the guarantee inside this package rather than several hundred lines away // in the caller. func TestGrantSettlesAnOrphanedEpisode(t *testing.T) { + t.Parallel() s := defaults() b := New() diff --git a/internal/render/render_test.go b/internal/render/render_test.go index bcf8d4a..572fe83 100644 --- a/internal/render/render_test.go +++ b/internal/render/render_test.go @@ -8,6 +8,7 @@ import ( ) func TestText(t *testing.T) { + t.Parallel() until := time.Date(2026, 7, 25, 15, 4, 0, 0, time.UTC) // A minute before the window's deadline, so a case carrying both can tell // the drop time and the close time apart. @@ -369,6 +370,7 @@ func TestText(t *testing.T) { // posture string this package does not recognise — the error is more urgent // than any posture prose, recognised or not. func TestTextEnforcementErrWinsOverUnknownPosture(t *testing.T) { + t.Parallel() got := Text(state.Snapshot{Posture: "some-future-posture", EnforcementErr: "backend refused"}) if got.Key != KeyWarning || got.Headline != "Enforcement failed" || got.Detail != "backend refused" { t.Errorf("got %+v, want the enforcement-failed display regardless of posture", got) @@ -381,6 +383,7 @@ func TestTextEnforcementErrWinsOverUnknownPosture(t *testing.T) { // sentence, right before "(profile …)"), and no appended lookup/hysteresis // note (which would land between that sentence and the caller's clause). func TestPostureIgnoresEnforcementErrAndNotes(t *testing.T) { + t.Parallel() until := time.Date(2026, 7, 25, 15, 4, 0, 0, time.UTC) snap := state.Snapshot{ Posture: PostureSwitchWindow, @@ -404,6 +407,7 @@ func TestPostureIgnoresEnforcementErrAndNotes(t *testing.T) { } func TestStaleThreshold(t *testing.T) { + t.Parallel() cases := []struct { name string poll int @@ -424,6 +428,7 @@ func TestStaleThreshold(t *testing.T) { } func TestIsStale(t *testing.T) { + t.Parallel() now := time.Date(2026, 7, 25, 12, 0, 0, 0, time.UTC) fresh := state.Snapshot{Time: now.Add(-30 * time.Second), PollIntervalSeconds: 15} diff --git a/internal/runner/control_test.go b/internal/runner/control_test.go index 5a91447..32b1d5c 100644 --- a/internal/runner/control_test.go +++ b/internal/runner/control_test.go @@ -3,9 +3,12 @@ package runner import ( "context" "errors" + "log/slog" "net/netip" "os" "path/filepath" + "strings" + "sync/atomic" "testing" "time" @@ -15,6 +18,39 @@ import ( "github.com/behnam-rk/dezhban/internal/firewall" ) +// pollUntil polls cond every 5ms until it returns true or timeout elapses, at +// which point it fails the test with msg. +func pollUntil(t *testing.T, timeout time.Duration, cond func() bool, msg string) { + t.Helper() + deadline := time.Now().Add(timeout) + for !cond() { + if time.Now().After(deadline) { + t.Fatal(msg) + } + time.Sleep(5 * time.Millisecond) + } +} + +// countingHandler counts slog records whose message contains substr, so a +// test can poll for a specific number of log-visible events instead of +// sleeping over a fixed wall-clock window. +type countingHandler struct { + substr string + count *int64 +} + +func (h countingHandler) Enabled(context.Context, slog.Level) bool { return true } + +func (h countingHandler) Handle(_ context.Context, r slog.Record) error { + if strings.Contains(r.Message, h.substr) { + atomic.AddInt64(h.count, 1) + } + return nil +} + +func (h countingHandler) WithAttrs([]slog.Attr) slog.Handler { return h } +func (h countingHandler) WithGroup(string) slog.Handler { return h } + // controlSocket returns a short socket path (see internal/control: t.TempDir() // overruns the platform sun_path limit). func controlSocket(t *testing.T) string { @@ -46,7 +82,9 @@ func startControlledWith(t *testing.T, o Options, tune func(*control.Server)) st tune(srv) } o.Control = srv - o.Log = discardLog() + if o.Log == nil { + o.Log = discardLog() + } ctx, cancel := context.WithCancel(context.Background()) done := make(chan error, 1) @@ -64,13 +102,7 @@ func startControlledWith(t *testing.T, o Options, tune func(*control.Server)) st }) // Wait for the loop to install its startup posture and start serving. - deadline := time.Now().Add(3 * time.Second) - for !control.Ping(path) { - if time.Now().After(deadline) { - t.Fatal("control socket never became reachable") - } - time.Sleep(5 * time.Millisecond) - } + pollUntil(t, 3*time.Second, func() bool { return control.Ping(path) }, "control socket never became reachable") return path } @@ -136,6 +168,8 @@ func TestControlManualBlockHeldAcrossGeoTicks(t *testing.T) { be := &fakeBackend{} o := vpnOpts(be) o.Interval = 5 * time.Millisecond // geo ticks fire continuously + var skipped int64 + o.Log = slog.New(countingHandler{substr: "manual block held", count: &skipped}) path := startControlled(t, o) if resp := do(t, path, control.Request{Op: control.OpBlock}); !resp.OK { @@ -143,9 +177,11 @@ func TestControlManualBlockHeldAcrossGeoTicks(t *testing.T) { } callsAfterBlock := len(be.calls) - // Let many geo ticks pass. A running state machine would probe (apply-guard + + // Wait for many geo ticks to actually be suspended by the manual block — not + // merely idle. A running state machine would probe (apply-guard + // apply-fullblock) and then lift the block on the steady "US" reading. - time.Sleep(150 * time.Millisecond) + pollUntil(t, 3*time.Second, func() bool { return atomic.LoadInt64(&skipped) >= 10 }, + "geo ticks were not suspended by the manual block in time") resp := do(t, path, control.Request{Op: control.OpStatus}) if !resp.Blocked || resp.Posture != "full-block" { @@ -324,9 +360,7 @@ func TestControlSocketRemovedOnShutdown(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) done := make(chan error, 1) go func() { done <- Run(ctx, o) }() - for !control.Ping(path) { - time.Sleep(5 * time.Millisecond) - } + pollUntil(t, 3*time.Second, func() bool { return control.Ping(path) }, "control socket never became reachable") cancel() if err := <-done; err != nil { t.Fatalf("Run: %v", err) diff --git a/internal/runner/recovery_test.go b/internal/runner/recovery_test.go index 423c006..5cd5682 100644 --- a/internal/runner/recovery_test.go +++ b/internal/runner/recovery_test.go @@ -4,6 +4,7 @@ import ( "context" "net/netip" "sync" + "sync/atomic" "testing" "time" @@ -84,10 +85,12 @@ func TestNoPendingFlipWhenSettled(t *testing.T) { } // countingMonitor reports a fixed country and counts how many lookups it served, -// which is how the tests below observe the probe cadence. +// which is how the tests below observe the probe cadence. n is accessed from +// both the runner's goroutine (Once) and the test goroutine (polling for it), +// hence atomic. type countingMonitor struct { cc string - n int + n atomic.Int64 } func (m *countingMonitor) Poll(ctx context.Context) <-chan monitor.Result { @@ -97,7 +100,7 @@ func (m *countingMonitor) Poll(ctx context.Context) <-chan monitor.Result { } func (m *countingMonitor) Once(ctx context.Context) (monitor.Reading, error) { - m.n++ + m.n.Add(1) return monitor.Reading{CountryCode: m.cc, IP: netip.MustParseAddr("203.0.113.9")}, nil } @@ -189,15 +192,24 @@ func TestNoAccelerationWhenProbingWouldHaveToLiftTheGuard(t *testing.T) { t.Fatal("never reached FULL BLOCK") } - before := mon.n + before := mon.n.Load() mon.cc = "US" tun.send(netdetect.TunnelState{Up: false, Detail: "dropped"}) tun.send(netdetect.TunnelState{Up: true, Names: []string{"utun4"}, Detail: "redialed"}) - time.Sleep(300 * time.Millisecond) // many fastProbeIntervals, were it accelerating - if extra := mon.n - before; extra > 0 { - t.Fatalf("%d lookup(s) ran after the tunnel-up edge with no tunnel-scoped provider pass; "+ - "each would have lifted the guard, so this must fall back to the configured cadence", extra) + // Absence assertion: no provider pass means acceleration must not start. + // Poll the window an accelerated cadence would have used, failing as soon + // as an extra lookup appears rather than only after the full wait. + deadline := time.Now().Add(300 * time.Millisecond) // many fastProbeIntervals, were it accelerating + for { + if extra := mon.n.Load() - before; extra > 0 { + t.Fatalf("%d lookup(s) ran after the tunnel-up edge with no tunnel-scoped provider pass; "+ + "each would have lifted the guard, so this must fall back to the configured cadence", extra) + } + if time.Now().After(deadline) { + break + } + time.Sleep(5 * time.Millisecond) } cancel() <-done @@ -206,14 +218,18 @@ func TestNoAccelerationWhenProbingWouldHaveToLiftTheGuard(t *testing.T) { // scriptedWatcher drives tunnel edges from a test by holding a state the // sampler keeps returning. // -// `send` deliberately BLOCKS for several sample intervals. netdetect.Watcher -// debounces a down edge over DownDebounce consecutive samples, so a state -// visible for only one tick is swallowed — which is correct for a real -// interface (it stops a flapping redial churning rule reloads) and would make a -// fake that flipped states instantaneously silently emit no edges at all. +// `send` deliberately BLOCKS until the watcher has actually sampled the new +// state enough times to register. netdetect.Watcher debounces a down edge +// over DownDebounce (default 2) consecutive samples, so a state visible for +// only one tick is swallowed — which is correct for a real interface (it +// stops a flapping redial churning rule reloads) and would make a fake that +// flipped states instantaneously silently emit no edges at all. Polling the +// watcher's own sample count — rather than sleeping a fixed multiple of the +// interval — ties the wait to what the fake actually observed. type scriptedWatcher struct { - mu sync.Mutex - last netdetect.TunnelState + mu sync.Mutex + last netdetect.TunnelState + samples atomic.Int64 } const scriptedInterval = 5 * time.Millisecond @@ -223,6 +239,7 @@ func (s *scriptedWatcher) watcher() *netdetect.Watcher { Tunnels: []string{"utun4"}, Interval: scriptedInterval, Sample: func([]string) netdetect.TunnelState { + s.samples.Add(1) s.mu.Lock() defer s.mu.Unlock() return s.last @@ -234,8 +251,15 @@ func (s *scriptedWatcher) send(st netdetect.TunnelState) { s.mu.Lock() s.last = st s.mu.Unlock() - // Long enough to clear the down debounce and be observed as a real edge. - time.Sleep(10 * scriptedInterval) + + // netdetect's default DownDebounce is 2; wait for a few more samples of + // the new state to clear it and register as a real edge, bounded by a + // generous deadline for a slow CI runner. + target := s.samples.Load() + 3 + deadline := time.Now().Add(2 * time.Second) + for s.samples.Load() < target && time.Now().Before(deadline) { + time.Sleep(time.Millisecond) + } } // waitFor drains snapshots until one satisfies pred, or the deadline passes. diff --git a/internal/update/gate.go b/internal/update/gate.go index 25a8c07..18730ec 100644 --- a/internal/update/gate.go +++ b/internal/update/gate.go @@ -10,9 +10,9 @@ import ( // GateResult explains an activation gate decision. type GateResult struct { - OK bool - Reason string // human-readable, always set - Posture string // the snapshot's posture, "" if no snapshot was read at all + OK bool `json:"ok"` + Reason string `json:"reason"` // human-readable, always set + Posture string `json:"posture,omitempty"` // the snapshot's posture, "" if no snapshot was read at all } // CanActivate reports whether restarting into a newly-applied version is safe diff --git a/internal/vocab/lint_test.go b/internal/vocab/lint_test.go index feef17b..91b6f6a 100644 --- a/internal/vocab/lint_test.go +++ b/internal/vocab/lint_test.go @@ -103,6 +103,7 @@ var docExempt = []string{"docs/concepts/glossary.md"} // fails as "the glossary changed shape", not as "zero violations found". A lint // that silently checks an empty list is worse than no lint: it reports success. func TestTheGlossaryStillParses(t *testing.T) { + t.Parallel() terms, err := Load(glossary()) if err != nil { t.Fatal(err) @@ -138,6 +139,7 @@ func TestTheGlossaryStillParses(t *testing.T) { // the lint reported success over a rule it was not applying. A term that cannot // find itself in its own text can never find itself in anyone else's. func TestEveryTermMatchesItself(t *testing.T) { + t.Parallel() terms, err := Load(glossary()) if err != nil { t.Fatal(err) @@ -161,6 +163,7 @@ func TestEveryTermMatchesItself(t *testing.T) { // authority depends on: "when user-facing copy and this page disagree, the copy // is wrong" was true only as an intention until something verified it. func TestUserFacingCopyUsesTheGlossary(t *testing.T) { + t.Parallel() terms, err := Load(glossary()) if err != nil { t.Fatal(err) diff --git a/scripts/install.sh b/scripts/install.sh index fd0c281..ae66fac 100644 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -13,6 +13,13 @@ # curl -fsSL https://raw.githubusercontent.com/Behnam-RK/dezhban/main/scripts/install.sh | sudo bash # curl -fsSL .../install.sh | sudo VERSION=0.2.0 bash # pin an exact version # +# On a real terminal (curl saved to a file, then run — NOT piped, since piping +# makes stdin the script text itself) this asks a few questions: which +# components to install on a fresh machine, and upgrade/reinstall/uninstall on +# a machine that already has dezhban. Piped or non-interactive, it takes +# today's exact defaults with no prompt at all — DEZHBAN_ASSUME_YES=1 forces +# that behavior even on a real terminal. +# # Must run as root: it installs to /usr/local and /etc, and registers a system # service. Written for bash 3.2 — that is what macOS ships at /bin/bash with no # Homebrew on PATH, and a fresh machine with nothing else installed is exactly @@ -27,6 +34,91 @@ note() { echo "==> $*"; } [ "$(id -u)" -eq 0 ] || die "run as root — e.g. curl -fsSL .../install.sh | sudo bash" +# --- interactive discipline ----------------------------------------------- +# stdin IS the script text itself when piped (`curl | sudo bash`), so any +# prompt reads /dev/tty directly, and only when one exists, stdout is a real +# terminal, and the caller hasn't opted out. Every function below degrades +# silently with no prompt at all when this is 0 — that is the load-bearing +# property: a provisioner, CI job, or `curl | sudo bash` must see byte-for-byte +# today's non-interactive behavior. +interactive=0 +if [ -r /dev/tty ] && [ -t 1 ] && [ "${DEZHBAN_ASSUME_YES:-0}" != "1" ]; then + interactive=1 +fi + +# ask VAR PROMPT DEFAULT — sets the (bash 3.2 has no namerefs, hence eval) +# variable named VAR to a line read from /dev/tty, or DEFAULT verbatim when +# not interactive or the line was empty. Always succeeds. +ask() { + if [ "$interactive" != 1 ]; then + eval "$1=\$3" + return 0 + fi + printf '%s' "$2" > /dev/tty + IFS= read -r _ask_reply < /dev/tty || _ask_reply="" + [ -n "$_ask_reply" ] || _ask_reply="$3" + eval "$1=\$_ask_reply" +} + +# confirm PROMPT DEFAULT("Y"|"N") — prints PROMPT with a [Y/n]/[y/N] suffix on +# /dev/tty and returns 0 for yes, 1 for no. Not interactive: DEFAULT wins with +# no prompt shown at all. +confirm() { + suffix="[Y/n]" + [ "$2" = N ] && suffix="[y/N]" + ask _confirm_reply "$1 $suffix " "$2" + # shellcheck disable=SC2154 # ask() assigns this indirectly via eval "$1=..." + case "$_confirm_reply" in + [Yy]*) return 0 ;; + [Nn]*) return 1 ;; + *) [ "$2" != N ] ;; + esac +} + +# menu PROMPT VAR OPTION... — prints PROMPT and a numbered OPTION list to +# /dev/tty, reads a choice into VAR (1-based). Not interactive: VAR is always +# 1 — every menu below puts the today-equivalent action first, so this is the +# same rule confirm/ask follow, just for a multi-way choice. +menu() { + _menu_prompt="$1"; _menu_var="$2"; shift 2 + if [ "$interactive" != 1 ]; then + eval "$_menu_var=1" + return 0 + fi + printf '%s\n' "$_menu_prompt" > /dev/tty + _menu_i=1 + for _menu_opt in "$@"; do + printf ' %d) %s\n' "$_menu_i" "$_menu_opt" > /dev/tty + _menu_i=$((_menu_i + 1)) + done + printf 'choice [1]: ' > /dev/tty + IFS= read -r _menu_reply < /dev/tty || _menu_reply="" + [ -n "$_menu_reply" ] || _menu_reply=1 + eval "$_menu_var=\$_menu_reply" +} + +# --- step counter ----------------------------------------------------------- +# [n/N] progress lines. total is computed once every conditional step is known +# (component choice, whether a service is already running) and before any of +# them run — see "compute step total" below. +step_n=0 +step_total=0 +step() { + step_n=$((step_n + 1)) + note "[$step_n/$step_total] $*" +} + +# dl NAME — fetches release asset NAME into $tmp/NAME. Shows curl's own +# progress bar on a real terminal; quiet everywhere else (unchanged from +# before — a piped install must not spew a progress bar into a log). +dl() { + if [ "$interactive" = 1 ]; then + curl -fL --progress-bar -o "$tmp/$1" "$GH/releases/download/$tag/$1" + else + curl -fsSL -o "$tmp/$1" "$GH/releases/download/$tag/$1" + fi +} + # --- 1. detect platform ------------------------------------------------------- # Only the 4 unix release targets. Windows has no curl-pipe-bash story — see # scripts/install.ps1 — and anything else isn't a supported build target at all @@ -95,6 +187,123 @@ else note "no existing installation found — this is a first-time install" fi +# Whether a service is already running, checked here (read-only — this just +# asks the OLD binary) so both the step total below and the stop/restart +# sequence later can rely on one answer. A FRESH install never touches this: +# was_running stays 0, so enforcement is never armed here. That is the same +# invariant the .pkg's postinstall holds: a kill switch must not arm itself +# during install. +was_running=0 +if [ -x /usr/local/bin/dezhban ] \ + && /usr/local/bin/dezhban status --json 2>/dev/null | grep -q '"service": *"installed, running"'; then + was_running=1 +fi + +# --- 2c. menu: what to do ------------------------------------------------- +# Every branch's option 1 is exactly what a non-interactive run already does — +# menu()'s own non-interactive default is "1", so this whole block is a no-op +# in effect when interactive=0. + +install_app=0 +[ "$goos" = darwin ] && install_app=1 +install_service=1 + +if [ "$mode" = fresh ]; then + fresh_desc="CLI + register the service, not started" + [ "$goos" = darwin ] && fresh_desc="CLI + menubar app + register the service, not started" + fresh_choice=1 + menu "Install dezhban $version:" fresh_choice \ + "Install now ($fresh_desc)" \ + "Choose components" \ + "Cancel" + case "$fresh_choice" in + 3) note "cancelled — nothing changed"; exit 0 ;; + 2) + if [ "$goos" = darwin ]; then + confirm "Install the menubar app (Dezhban.app)?" Y && install_app=1 || install_app=0 + fi + confirm "Register dezhban as a system service now? (installed, not started)" Y && install_service=1 || install_service=0 + ;; + esac +else + if [ "$mode" = upgrade ]; then + upgrade_choice=1 + menu "dezhban $prev_version is installed; $version is available." upgrade_choice \ + "Upgrade to $version" \ + "Uninstall dezhban" \ + "Cancel" + else + upgrade_choice=1 + menu "dezhban $version is already installed." upgrade_choice \ + "Reinstall $version" \ + "Uninstall dezhban" \ + "Cancel" + fi + case "$upgrade_choice" in + 2) do_uninstall=1 ;; + 3) note "cancelled — nothing changed"; exit 0 ;; + esac +fi + +if [ "${do_uninstall:-0}" = 1 ]; then + SHARE_DIR=/usr/local/share/dezhban + uninstaller="$SHARE_DIR/uninstall.sh" + if [ ! -x "$uninstaller" ]; then + # Fetch the uninstaller matching what's actually installed (prev_version), + # never the newer $tag just resolved above — this run may not even proceed + # with that version. "unknown" (an unreadable prior binary) has no real tag + # to ask for, so fall back to the version this run resolved. + fetch_tag="v$prev_version" + [ "$prev_version" = "unknown" ] && fetch_tag="$tag" + note "no installed uninstaller found — fetching the one matching $fetch_tag" + uninstall_src="packaging/macos/uninstall.sh" + [ "$goos" = linux ] && uninstall_src="packaging/linux/uninstall.sh" + mkdir -p "$SHARE_DIR" + curl -fsSL -o "$uninstaller" "https://raw.githubusercontent.com/$REPO/$fetch_tag/$uninstall_src" \ + || die "could not fetch the uninstaller for $fetch_tag" + chmod +x "$uninstaller" + fi + + echo + echo "This will remove:" + echo " - dezhban's firewall rules (panic teardown)" + echo " - the CLI, and the menubar app if installed" + echo " - the system service" + echo " - daemon state (learned VPN endpoints, live posture)" + echo + + keep_config=1 + confirm "Keep your config in /etc/dezhban?" Y && keep_config=1 || keep_config=0 + + echo + ask confirm_word "Type \"uninstall\" to confirm: " "" + # shellcheck disable=SC2154 # ask() assigns this indirectly via eval "$1=..." + [ "$confirm_word" = "uninstall" ] || die "confirmation did not match \"uninstall\" — nothing was removed" + + if [ "$keep_config" = 1 ]; then + KEEP_CONFIG=1 sh "$uninstaller" + else + sh "$uninstaller" + fi + exit 0 +fi + +# --- 2d. compute step total ------------------------------------------------ +# Every conditional below mirrors a real step further down: keep this in sync +# with the sequence, or the [n/N] count will drift from what's actually +# happening. +# Base, unconditional steps: download CLI, verify checksums, install CLI, +# fetch uninstaller. Platform/mode detection and version resolution print +# their own "==>" lines but aren't part of this count. +step_total=4 +[ "$install_app" = 1 ] && step_total=$((step_total + 2)) # download + install app +[ "$install_service" = 1 ] && step_total=$((step_total + 1)) # register service +# Upper bound: was_running, not restart_after — the activation gate (checked +# later, right before the stop) may still refuse and skip both, in which case +# the printed count simply falls short of this total. That's the honest +# tradeoff: the gate must be checked as late as possible, not this early. +[ "$was_running" = 1 ] && step_total=$((step_total + 2)) + # --- 3. download + verify ------------------------------------------------------ # Checksum verification is mandatory and aborts on mismatch. This is deliberately # NOT an ed25519 signature check: a bare macOS system's /usr/bin/openssl is @@ -109,12 +318,11 @@ fi tmp="$(mktemp -d)" trap 'rm -rf "$tmp"' EXIT -dl() { curl -fsSL -o "$tmp/$1" "$GH/releases/download/$tag/$1"; } - -note "downloading $asset $tag" +step "downloading $asset $tag" dl "$asset" -dl "SHA256SUMS" -if [ "$goos" = darwin ]; then +curl -fsSL -o "$tmp/SHA256SUMS" "$GH/releases/download/$tag/SHA256SUMS" +if [ "$install_app" = 1 ]; then + step "downloading Dezhban-macos.app.zip" dl "Dezhban-macos.app.zip" fi @@ -168,26 +376,38 @@ verify() { || die "checksum mismatch for $1 — aborting install. This may mean a bad mirror or a tampered download; do not retry blindly." } -note "verifying checksums" +step "verifying checksums" verify "$asset" -[ "$goos" = darwin ] && verify "Dezhban-macos.app.zip" +[ "$install_app" = 1 ] && verify "Dezhban-macos.app.zip" # --- 4. install ----------------------------------------------------------- -# If a service is already running (an upgrade over a live install), stop it -# before replacing the binary and restart after — restoring exactly the state -# it was already in. A FRESH install never touches this: was_running stays 0, -# so enforcement is never armed here. That is the same invariant the .pkg's -# postinstall holds: a kill switch must not arm itself during install. +# If a service is already running (an upgrade over a live install), the new +# binary is installed regardless — overwriting a running executable's file is +# safe on unix; the old daemon keeps enforcing on its old inode until it +# actually restarts. But STOPPING/RESTARTING it is gated on the same +# activation rule `dezhban upgrade apply` honours (docs/adr/0007): a restart +# must never happen through FULL BLOCK or an open switch window, since that +# would lift a block on a forbidden-country exit — the one thing this tool +# exists to prevent. Checked as late as possible, right before the stop, not +# back when was_running was first read — the posture can change during the +# download above. No override: an operator who wants to force it already has +# `sudo dezhban restart`, typed by name. +restart_after=0 +if [ "$was_running" = 1 ]; then + if gate_msg="$(/usr/local/bin/dezhban upgrade can-activate 2>&1)"; then + restart_after=1 + else + note "not restarting the running service: $gate_msg" + note "the currently running dezhban keeps enforcing on the old build; retry later with: sudo dezhban restart" + fi +fi -was_running=0 -if [ -x /usr/local/bin/dezhban ] \ - && /usr/local/bin/dezhban status --json 2>/dev/null | grep -q '"service": *"installed, running"'; then - was_running=1 - note "existing installation is running — stopping for the upgrade" +if [ "$restart_after" = 1 ]; then + step "stopping the running service for the upgrade" /usr/local/bin/dezhban --no-sudo stop || true fi -note "installing the CLI to /usr/local/bin/dezhban" +step "installing the CLI to /usr/local/bin/dezhban" install -m 0755 "$tmp/$asset" /usr/local/bin/dezhban.new mv -f /usr/local/bin/dezhban.new /usr/local/bin/dezhban @@ -197,8 +417,8 @@ mv -f /usr/local/bin/dezhban.new /usr/local/bin/dezhban # silently never comes up. Cheap no-op when the flag was never set. [ "$goos" = darwin ] && { xattr -d com.apple.quarantine /usr/local/bin/dezhban 2>/dev/null || true; } -if [ "$goos" = darwin ]; then - note "installing the menubar app to /Applications/Dezhban.app" +if [ "$install_app" = 1 ]; then + step "installing the menubar app to /Applications/Dezhban.app" rm -rf /Applications/Dezhban.app ditto -xk "$tmp/Dezhban-macos.app.zip" /Applications @@ -231,23 +451,26 @@ fi CONFIG_DIR=/etc/dezhban mkdir -p "$CONFIG_DIR" -note "registering the service (not starting it — see 'next steps' below)" -# Absolute path, never a bare `dezhban`: /usr/local/bin is not necessarily first -# on root's PATH — on Apple Silicon, Homebrew's /opt/homebrew/bin usually is, -# and this repo now ships a Homebrew formula that puts a dezhban there. Resolving -# through PATH could register the service using a DIFFERENT build than the one -# just installed two lines above. -/usr/local/bin/dezhban --no-sudo install --config "$CONFIG_DIR/dezhban.json" \ - || die "could not register the service; the CLI is installed at /usr/local/bin/dezhban — retry with 'sudo dezhban install'" +if [ "$install_service" = 1 ]; then + step "registering the service (not starting it — see 'next steps' below)" + # Absolute path, never a bare `dezhban`: /usr/local/bin is not necessarily first + # on root's PATH — on Apple Silicon, Homebrew's /opt/homebrew/bin usually is, + # and this repo now ships a Homebrew formula that puts a dezhban there. Resolving + # through PATH could register the service using a DIFFERENT build than the one + # just installed two lines above. + /usr/local/bin/dezhban --no-sudo install --config "$CONFIG_DIR/dezhban.json" \ + || die "could not register the service; the CLI is installed at /usr/local/bin/dezhban — retry with 'sudo dezhban install'" +fi -if [ "$was_running" = 1 ]; then - note "restarting the service" +if [ "$restart_after" = 1 ]; then + step "restarting the service" /usr/local/bin/dezhban --no-sudo start fi # The uninstaller comes from the SAME tag being installed — same guarantee the # .pkg gives (it bakes in whichever uninstall.sh existed when that tag was # built), just fetched instead of embedded in a payload. +step "fetching the uninstaller" SHARE_DIR=/usr/local/share/dezhban mkdir -p "$SHARE_DIR" uninstall_src="packaging/macos/uninstall.sh" @@ -262,10 +485,25 @@ fi echo if [ "$mode" = fresh ]; then echo "dezhban $version installed." - echo echo "next steps:" - echo " sudo dezhban setup # configure: VPN, tunnel interfaces, blocked countries" - echo " sudo dezhban start # arm the kill switch" + # Explicitly gated on interactive, not just confirm()'s own default: a + # piped/non-interactive run must never launch an interactive wizard, no + # matter what a "default yes" would otherwise imply. + setup_now=0 + if [ "$interactive" = 1 ]; then + confirm "Run the setup wizard now? (VPN, tunnel interfaces, blocked countries)" Y && setup_now=1 + fi + if [ "$setup_now" = 1 ]; then + /usr/local/bin/dezhban setup < /dev/tty + else + echo " dezhban setup # configure: VPN, tunnel interfaces, blocked countries" + fi + if [ "$install_service" = 1 ]; then + echo " sudo dezhban start # arm the kill switch" + else + echo " sudo dezhban install # register the service (skipped above)" + echo " sudo dezhban start # then arm the kill switch" + fi else if [ "$mode" = upgrade ]; then echo "dezhban upgraded: $prev_version -> $version." @@ -276,9 +514,13 @@ else echo # Deliberately no `setup` here: an existing user has a config already, and # re-running the wizard would walk them through replacing it. - if [ "$was_running" = 1 ]; then + if [ "$restart_after" = 1 ]; then echo "The service was running and has been restarted on the new build." - echo " sudo dezhban status # confirm the posture came back as expected" + echo " dezhban status # confirm the posture came back as expected" + elif [ "$was_running" = 1 ]; then + echo "The new build is installed, but the running service was NOT restarted" + echo "(see the activation-gate note above) — it is still enforcing on the old build." + echo " sudo dezhban restart # once the posture clears" else echo "The service was not running, so it was left stopped." echo " sudo dezhban start # arm the kill switch" From 17b25efaba3604fd5be7842e48cf02d091361fc1 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Wed, 29 Jul 2026 01:22:48 +0330 Subject: [PATCH 2/5] fix(review): close six findings from the PR #38 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - install.sh: gate interactivity on `-t 0` (stdin), not `-t 1` (stdout). A piped `curl | sudo bash` still has the terminal as its STDOUT, so the old test made the advertised one-liner install stop at a menu — the exact opposite of what the script header, install.md, and the changelog all promise. stdin is the only stream that distinguishes the two invocations. - install.sh: install the new CLI BEFORE asking `upgrade can-activate`, and ask the newly-installed binary. `can-activate` ships for the first time in this same change, so asking the previously-installed binary meant every upgrade from an older release answered "unknown subcommand", refused the gate, and silently never restarted a running daemon. Installing first is safe and already documented as such (the old daemon keeps its inode) — this also makes upgrade.md's description of the sequence literally true. - passwordless.md: `control.group` is NOT empty by default on macOS, where it is "admin". Said the opposite, on a page bundled into the macOS app's own Help pane, and contradicted cli.md in the same PR. - harness_test.go: drain the captured stdout/stderr pipes from goroutines started before run(), not with ReadAll after it returns. A pipe holds only a fixed kernel buffer, so any command printing more than that would have hung the test forever. Also closes the read ends. - ci.yml / Taskfile.yml: pin go-test-coverage to v2.19.0 instead of @latest — the gate decides whether CI goes red, so upstream must not be able to change that verdict with no commit here. Install unconditionally, so the pin isn't silently defeated by a stale binary already on PATH. - CHANGELOG: the sudo trim touched neither README.md nor docs/concepts/; say what it actually changed. Co-Authored-By: Claude Opus 5 --- .claude/settings.json | 16 +++++++-- .github/workflows/ci.yml | 7 ++-- CHANGELOG.md | 8 +++-- Taskfile.yml | 18 ++++++---- cmd/dezhban/harness_test.go | 27 +++++++++++++-- docs/usage/passwordless.md | 31 +++++++++++------ scripts/install.sh | 69 +++++++++++++++++++++++-------------- 7 files changed, 121 insertions(+), 55 deletions(-) diff --git a/.claude/settings.json b/.claude/settings.json index 448b46f..aafc822 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -4,9 +4,19 @@ "on this machine read it — unlike settings.local.json (gitignored) and", "~/.claude/settings.json (per-profile, keyed by CLAUDE_CONFIG_DIR).", "Subagents inherit these, so approvals no longer have to be re-granted per agent.", - "Plan mode DOES honour this allow-list for read-only tools — it only blocks", - "mutations unconditionally. So anything read-only that prompts during planning", - "is simply missing from the list below; add it here rather than re-approving.", + "Plan mode blocks mutations unconditionally, and for an MCP tool it is the", + "SERVER that decides what counts: Claude Code trusts the tool's own", + "annotations.readOnlyHint and consults no allow-list at all in that mode.", + "So an MCP tool declaring readOnlyHint:false re-prompts forever no matter", + "what you add here — the wildcards below cannot fix it, and adding more", + "entries is a dead end. Verified 2026-07-28 against Claude Code 2.1.220.", + "Concretely, context-mode declares ctx_search/ctx_stats/ctx_doctor read-only", + "but ctx_execute/ctx_execute_file/ctx_batch_execute/ctx_index NOT read-only.", + "A PreToolUse hook returning permissionDecision:allow does NOT override the", + "gate either — built, installed and tested live 2026-07-28. Nor does Claude", + "Code expose any plan-mode tool allow-list setting. The only fix that works", + "is not calling those tools while planning: see the plan-mode paragraph in", + "~/.claude/CLAUDE.md, which routes planning through jcodemunch/serena/Read.", "Caveat: Bash patterns match the whole command string, so a novel compound", "command (a for-loop, an unlisted pipe) still prompts even when every program", "inside it is listed. Prefer one allow-listed command per call." diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b6921bf..c5be5c1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,11 +50,14 @@ jobs: run: go test -race ./... # Coverage gate once (ubuntu): enforces .testcoverage.yml's per-package # floors so coverage cannot regress. See docs/contribute/testing.md's - # Unit test policy for what counts. + # Unit test policy for what counts. The tool version is PINNED, never + # @latest — this gate decides whether CI goes red, so an upstream release + # must not be able to change that verdict with no commit here. Keep in + # sync with Taskfile.yml's test:cover. - if: matrix.os == 'ubuntu-latest' run: | go test ./... -coverprofile=cover.out - go install github.com/vladopajic/go-test-coverage/v2@latest + go install github.com/vladopajic/go-test-coverage/v2@v2.19.0 "$(go env GOPATH)/bin/go-test-coverage" --config=.testcoverage.yml # Keep the windows backend compiling even though we don't test it yet (see # the matrix comment). Vet, not just build: it type-checks the _windows.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a60d45..c13d1bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,9 +63,11 @@ current as you land changes. said otherwise. `block`, `unblock`, `switch`, `pause`, `resume` route over the control socket before ever needing root; `sudo` in front of them was actively counterproductive; it re-execs as root, which then pays the very - password prompt the socket exists to avoid. Trimmed from README.md and the - user-facing `docs/usage/` and `docs/concepts/` pages; ADRs, contributor - docs, and every `curl | sudo bash` install line are untouched by design. + password prompt the socket exists to avoid. Trimmed from the user-facing + `docs/usage/` pages; ADRs, contributor docs, and every `curl | sudo bash` + install line are untouched by design, and the `sudo` still shown on `panic`, + `setup`, `run`, and the service lifecycle is correct — those genuinely need + root. ## [0.8.0] - 2026-07-28 diff --git a/Taskfile.yml b/Taskfile.yml index dcfcb0c..0a78c4c 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -316,15 +316,19 @@ tasks: test:cover: desc: 'Run the suite with coverage and enforce .testcoverage.yml' + vars: + # Pinned, never @latest: the gate decides whether CI goes red, so a new + # upstream release must not be able to change that verdict without a + # commit here. Bump deliberately. Keep in sync with .github/workflows/ci.yml. + COVERAGE_TOOL: github.com/vladopajic/go-test-coverage/v2@v2.19.0 cmds: - go test ./... -coverprofile=cover.out - - | - gobin="$(go env GOPATH)/bin" - if [ ! -x "$gobin/go-test-coverage" ]; then - echo "installing go-test-coverage..." - go install github.com/vladopajic/go-test-coverage/v2@latest - fi - "$gobin/go-test-coverage" --config=.testcoverage.yml + # Unconditional install, not "skip if the binary exists": `go install` is + # a fast no-op once the module is cached, and skipping would silently keep + # whatever version a developer installed first — which is exactly what the + # pin above exists to prevent. + - go install {{.COVERAGE_TOOL}} + - '"$(go env GOPATH)/bin/go-test-coverage" --config=.testcoverage.yml' lint: cmds: diff --git a/cmd/dezhban/harness_test.go b/cmd/dezhban/harness_test.go index f55bc69..bc3e064 100644 --- a/cmd/dezhban/harness_test.go +++ b/cmd/dezhban/harness_test.go @@ -19,6 +19,15 @@ import ( // (-v, --no-sudo, --no-daemon) is reset first so tests stay order-independent // regardless of what ran before them — run() itself never resets it, since a // real process only calls it once. +// +// Because it swaps the process-global os.Stdout/os.Stderr, no test in this +// file may call t.Parallel(). +// +// Each pipe is drained by its own goroutine STARTED BEFORE run(), never read +// after it returns: a pipe holds only a fixed kernel buffer (64 KiB on Linux, +// smaller on macOS), so a command that prints more than that — `config +// schema`, `completion zsh`, `print-rules` on a large config — would block +// forever inside run() with nothing reading the other end. func runCLI(t *testing.T, args ...string) (stdout, stderr string, code int) { t.Helper() verbose, noSudo, noDaemonFlag = false, false, false @@ -32,16 +41,28 @@ func runCLI(t *testing.T, args ...string) (stdout, stderr string, code int) { if err != nil { t.Fatal(err) } + + drain := func(r *os.File) <-chan string { + ch := make(chan string, 1) + go func() { + data, _ := io.ReadAll(r) + _ = r.Close() + ch <- string(data) + }() + return ch + } + outCh, errCh := drain(outR), drain(errR) + os.Stdout, os.Stderr = outW, errW defer func() { os.Stdout, os.Stderr = oldOut, oldErr }() code = run(args) + // Closing the write ends is what ends each ReadAll — do it before + // receiving, or the drains never see EOF. _ = outW.Close() _ = errW.Close() - outData, _ := io.ReadAll(outR) - errData, _ := io.ReadAll(errR) - return string(outData), string(errData), code + return <-outCh, <-errCh, code } // testConfigPath writes a valid, self-contained config to a temp file and diff --git a/docs/usage/passwordless.md b/docs/usage/passwordless.md index 9d06f16..e300373 100644 --- a/docs/usage/passwordless.md +++ b/docs/usage/passwordless.md @@ -6,23 +6,32 @@ running daemon over its **control socket** instead, and the daemon performs them. Whether that path is open for you depends on one setting: `control.group`. -## Why this isn't on by default - -By default `control.group` is empty, which means the socket is root-only -(mode `0600`). Every routine command then falls back to `sudo`, which is why -a fresh install still asks for a password for things that feel like they -shouldn't need it. Turning this on is one line. +## Is it already on? + +It depends on your OS. `control.group` defaults to `"admin"` on macOS, where +that group exists on every install and is exactly the set of accounts that can +already `sudo` — so **on macOS this is on out of the box** and there is +nothing to do. Off macOS it defaults to empty, because there is no single +portable name for "the admins" (Debian calls it `sudo`, Fedora calls it +`wheel`), and empty means the socket is root-only (mode `0600`). Every routine +command then falls back to `sudo`, which is why a fresh Linux install still +asks for a password for things that feel like they shouldn't need it. Turning +it on is one line. + +Either way, `dezhban doctor`'s `control:` line is the authority on which state +you're actually in — skip to [Turn it on](#turn-it-on)'s verification steps if +you just want to check. ## Turn it on First, identify your system's existing administrators group — the same one that already lets you run `sudo` at all: -| System | Group | -|---|---| -| Debian, Ubuntu | `sudo` | -| Fedora, RHEL, Arch, openSUSE | `wheel` | -| macOS | `admin` | +| System | Group | Default | +|---|---|---| +| Debian, Ubuntu | `sudo` | not set | +| Fedora, RHEL, Arch, openSUSE | `wheel` | not set | +| macOS | `admin` | **already set** | Point `control.group` at it: diff --git a/scripts/install.sh b/scripts/install.sh index ae66fac..8955ac6 100644 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -36,13 +36,17 @@ note() { echo "==> $*"; } # --- interactive discipline ----------------------------------------------- # stdin IS the script text itself when piped (`curl | sudo bash`), so any -# prompt reads /dev/tty directly, and only when one exists, stdout is a real -# terminal, and the caller hasn't opted out. Every function below degrades -# silently with no prompt at all when this is 0 — that is the load-bearing -# property: a provisioner, CI job, or `curl | sudo bash` must see byte-for-byte -# today's non-interactive behavior. +# prompt reads /dev/tty directly. But the DECISION to prompt at all keys on +# `-t 0`, not `-t 1`: a piped install still has this terminal as its stdout, +# so testing stdout would make `curl | sudo bash` interactive — the exact +# thing every comment, doc, and changelog entry here promises it is not. +# stdin is the only stream that actually distinguishes the two invocations. +# /dev/tty must also be readable and the caller must not have opted out. +# Every function below degrades silently with no prompt at all when this is +# 0 — that is the load-bearing property: a provisioner, CI job, or +# `curl | sudo bash` must see byte-for-byte today's non-interactive behavior. interactive=0 -if [ -r /dev/tty ] && [ -t 1 ] && [ "${DEZHBAN_ASSUME_YES:-0}" != "1" ]; then +if [ -t 0 ] && [ -r /dev/tty ] && [ "${DEZHBAN_ASSUME_YES:-0}" != "1" ]; then interactive=1 fi @@ -79,6 +83,11 @@ confirm() { # /dev/tty, reads a choice into VAR (1-based). Not interactive: VAR is always # 1 — every menu below puts the today-equivalent action first, so this is the # same rule confirm/ask follow, just for a multi-way choice. +# +# Unrecognised input is NOT re-prompted: each caller's `case` simply falls +# through to option 1. That is safe by construction because option 1 is always +# the non-destructive install/upgrade — a typo can never land on "uninstall", +# which additionally requires typing the word out in full. menu() { _menu_prompt="$1"; _menu_var="$2"; shift 2 if [ "$interactive" != 1 ]; then @@ -381,17 +390,35 @@ verify "$asset" [ "$install_app" = 1 ] && verify "Dezhban-macos.app.zip" # --- 4. install ----------------------------------------------------------- -# If a service is already running (an upgrade over a live install), the new -# binary is installed regardless — overwriting a running executable's file is -# safe on unix; the old daemon keeps enforcing on its old inode until it -# actually restarts. But STOPPING/RESTARTING it is gated on the same -# activation rule `dezhban upgrade apply` honours (docs/adr/0007): a restart -# must never happen through FULL BLOCK or an open switch window, since that -# would lift a block on a forbidden-country exit — the one thing this tool -# exists to prevent. Checked as late as possible, right before the stop, not -# back when was_running was first read — the posture can change during the -# download above. No override: an operator who wants to force it already has +# The CLI lands FIRST, before anything is stopped. If a service is already +# running (an upgrade over a live install) that is safe: overwriting a running +# executable's file is safe on unix, and the old daemon keeps enforcing on its +# old inode until it actually restarts. +step "installing the CLI to /usr/local/bin/dezhban" +install -m 0755 "$tmp/$asset" /usr/local/bin/dezhban.new +mv -f /usr/local/bin/dezhban.new /usr/local/bin/dezhban + +# Same enforcement as the .app below, and it matters just as much here: a +# quarantined bare executable is refused on exec too (not only bundles), so a +# flagged binary would fail as a launchd-started daemon — i.e. the kill switch +# silently never comes up. Cheap no-op when the flag was never set. +[ "$goos" = darwin ] && { xattr -d com.apple.quarantine /usr/local/bin/dezhban 2>/dev/null || true; } + +# STOPPING/RESTARTING a live daemon, unlike installing the file, is gated on +# the same activation rule `dezhban upgrade apply` honours (docs/adr/0007): a +# restart must never happen through FULL BLOCK or an open switch window, since +# that would lift a block on a forbidden-country exit — the one thing this tool +# exists to prevent. No override: an operator who wants to force it already has # `sudo dezhban restart`, typed by name. +# +# Asked as late as possible — the posture can change during the download above, +# so this must not reuse the answer from when was_running was first read. It is +# asked of the NEWLY INSTALLED binary, deliberately: `upgrade can-activate` +# ships for the first time with this very change, so the previously-installed +# binary would answer "unknown subcommand" on every upgrade from an older +# release and no live daemon would ever be restarted again. The new binary +# reads the same daemon-written state snapshot the old one does, so asking it +# is both correct and the only version-independent option. restart_after=0 if [ "$was_running" = 1 ]; then if gate_msg="$(/usr/local/bin/dezhban upgrade can-activate 2>&1)"; then @@ -407,16 +434,6 @@ if [ "$restart_after" = 1 ]; then /usr/local/bin/dezhban --no-sudo stop || true fi -step "installing the CLI to /usr/local/bin/dezhban" -install -m 0755 "$tmp/$asset" /usr/local/bin/dezhban.new -mv -f /usr/local/bin/dezhban.new /usr/local/bin/dezhban - -# Same enforcement as the .app below, and it matters just as much here: a -# quarantined bare executable is refused on exec too (not only bundles), so a -# flagged binary would fail as a launchd-started daemon — i.e. the kill switch -# silently never comes up. Cheap no-op when the flag was never set. -[ "$goos" = darwin ] && { xattr -d com.apple.quarantine /usr/local/bin/dezhban 2>/dev/null || true; } - if [ "$install_app" = 1 ]; then step "installing the menubar app to /Applications/Dezhban.app" rm -rf /Applications/Dezhban.app From 5069e724b128344eea8293ac19247c4f427f1754 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Wed, 29 Jul 2026 11:42:36 +0330 Subject: [PATCH 3/5] fix(review): close nine findings from the PR #38 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit doctor's new control: check violated doctorCheck.Fixes' own contract ("commands or actions ... never prose about them — the GUI badges each one") by putting "see docs/usage/passwordless.md" in Fixes on three branches, and dropped sudo from `config set control.enabled true` — a command this PR's own policy says must keep it. The doc pointer moves to Details, the group branches get a real runnable `config set control.group` line chosen by GOOS, and the forbidden branch carries prose with no Fix at all, since add-to-group is usermod on Linux and dseditgroup on macOS. A table-driven case now checks every branch, not just the exercised ones. Also: runDoctor probed the socket even with control.enabled=false, where buildControlCheck discards the result — controlReachable and controlStatus already short-circuit the same way. docs/usage/passwordless.md's "what still needs root" omitted config set/edit/preset apply — the very command the page opens with — so a reader would read its own `sudo` as a typo. CI ran the full suite three times on ubuntu (plain, race, coverage). The profile is now produced by the matrix-wide run and only GATED on ubuntu; -race stays the one uninstrumented run. .testcoverage.yml now records that its floors are LINUX numbers, since internal/firewall and internal/netdetect are build-tagged and a Mac measures different files than the gate does. Test-comment corrections: config.Default() does carry geo providers (the claim that it doesn't is contradicted by TestCmdMonitorNoProviders in the same file), and runCLI's os.Stdout swap forbids t.Parallel() package-wide, not just in harness_test.go. scriptedWatcher.send now fails by name instead of silently giving up at its deadline. Installer prints "next steps:" after the setup wizard rather than above it, and says why SHA256SUMS bypasses dl(). Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 15 ++++++++-- .testcoverage.yml | 9 ++++++ cmd/dezhban/doctor_test.go | 48 ++++++++++++++++++++++++++++++ cmd/dezhban/harness_test.go | 19 ++++++++---- cmd/dezhban/main.go | 51 +++++++++++++++++++++++++++----- docs/usage/passwordless.md | 9 +++++- internal/runner/recovery_test.go | 27 ++++++++++++----- scripts/install.sh | 13 ++++++-- 8 files changed, 162 insertions(+), 29 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c5be5c1..f327062 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,9 +43,13 @@ jobs: with: go-version-file: go.mod - run: go vet ./... - - run: go test ./... + # The profile is written on every OS but GATED on one (below): running the + # suite a third time just to produce it would triple ubuntu's test cost + # for a file only ubuntu reads. + - run: go test ./... -coverprofile=cover.out # Race detector once (ubuntu): the runner toggles posture from a ticker - # loop with a recovery probe — catch data races there. + # loop with a recovery probe — catch data races there. Also the one + # uninstrumented run of the whole suite. - if: matrix.os == 'ubuntu-latest' run: go test -race ./... # Coverage gate once (ubuntu): enforces .testcoverage.yml's per-package @@ -54,9 +58,14 @@ jobs: # @latest — this gate decides whether CI goes red, so an upstream release # must not be able to change that verdict with no commit here. Keep in # sync with Taskfile.yml's test:cover. + # + # Gated to ubuntu on purpose, and that is also why the floors in + # .testcoverage.yml are LINUX numbers: the backends are build-tagged, so + # macOS compiles pf_darwin.go/discover_darwin.go where Linux compiles + # nft_linux.go/discover_other.go. Same package name, different files, + # legitimately different coverage. - if: matrix.os == 'ubuntu-latest' run: | - go test ./... -coverprofile=cover.out go install github.com/vladopajic/go-test-coverage/v2@v2.19.0 "$(go env GOPATH)/bin/go-test-coverage" --config=.testcoverage.yml # Keep the windows backend compiling even though we don't test it yet (see diff --git a/.testcoverage.yml b/.testcoverage.yml index 8a74227..4026946 100644 --- a/.testcoverage.yml +++ b/.testcoverage.yml @@ -8,6 +8,15 @@ # package is already where it should be. Raise a package's own threshold # whenever a change pushes its real coverage up; that is the intended way # this file grows over time. +# +# THE NUMBERS ARE LINUX NUMBERS, because the gate runs only on ubuntu (see +# .github/workflows/ci.yml). Two packages are build-tagged and therefore +# genuinely different bodies of code per OS — internal/firewall compiles +# pf_darwin.go on macOS and nft_linux.go on Linux, internal/netdetect compiles +# discover_darwin.go vs the much smaller discover_other.go. `task test:cover` +# on a Mac measures those files, not the ones CI will. If a local run passes +# and CI's gate fails (or vice versa) on one of those two, that is why, and +# the number to record here is CI's. profile: cover.out threshold: package: 60 diff --git a/cmd/dezhban/doctor_test.go b/cmd/dezhban/doctor_test.go index 3b535bc..182564c 100644 --- a/cmd/dezhban/doctor_test.go +++ b/cmd/dezhban/doctor_test.go @@ -84,6 +84,13 @@ func TestBuildControlCheck(t *testing.T) { if !strings.Contains(c.Summary, "disabled") { t.Errorf("summary = %q, want it to say disabled", c.Summary) } + // `config set` is in the privileged set — it needs an enrolled control + // token, not just group membership. A fix modelled without sudo tells + // the user to run something that fails, on the one check whose whole + // subject is which commands need a password. + if len(c.Fixes) != 1 || !strings.HasPrefix(c.Fixes[0], "sudo dezhban config set control.enabled") { + t.Errorf("fixes = %v, want a single `sudo dezhban config set control.enabled true`", c.Fixes) + } }) t.Run("forbidden — reachable but not in the group", func(t *testing.T) { @@ -95,6 +102,47 @@ func TestBuildControlCheck(t *testing.T) { if !strings.Contains(c.Summary, "not in the") || !strings.Contains(c.Summary, "admin") { t.Errorf("summary = %q, want it to name the group", c.Summary) } + // Adding an account to a unix group is usermod on Linux and dseditgroup + // on macOS — no portable command to offer, so this branch must explain + // rather than invent one. + 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") { + t.Errorf("details = %v, want the doc pointer", c.Details) + } + }) + + // doctorCheck.Fixes is documented as "commands or actions ... never prose + // about them — the GUI badges each one". A sentence in that slot renders as + // a command the user should type, so every branch is checked, not just the + // ones a case above happens to exercise. + t.Run("no branch puts prose in Fixes", func(t *testing.T) { + for _, tc := range []struct { + name string + mutate func(*config.Config) + resp control.Response + probeErr error + }{ + {"disabled", func(c *config.Config) { c.Control.Enabled = false }, control.Response{}, nil}, + {"forbidden", nil, control.Response{}, control.ErrForbidden}, + {"unreachable", nil, control.Response{}, errors.New("dial: no such file")}, + {"unreachable, no group", func(c *config.Config) { c.Control.Group = "" }, control.Response{}, errors.New("dial: no such file")}, + {"reachable, no group", func(c *config.Config) { c.Control.Group = "" }, control.Response{OK: true}, nil}, + {"reachable and a member", nil, control.Response{OK: true}, nil}, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := base() + if tc.mutate != nil { + tc.mutate(&cfg) + } + for _, f := range buildControlCheck(&cfg, tc.resp, tc.probeErr).Fixes { + if !strings.HasPrefix(f, "dezhban ") && !strings.HasPrefix(f, "sudo dezhban ") { + t.Errorf("fix %q is not a runnable command — prose belongs in Details", f) + } + } + }) + } }) t.Run("unreachable — no daemon running", func(t *testing.T) { diff --git a/cmd/dezhban/harness_test.go b/cmd/dezhban/harness_test.go index bc3e064..4593767 100644 --- a/cmd/dezhban/harness_test.go +++ b/cmd/dezhban/harness_test.go @@ -20,8 +20,11 @@ import ( // regardless of what ran before them — run() itself never resets it, since a // real process only calls it once. // -// Because it swaps the process-global os.Stdout/os.Stderr, no test in this -// file may call t.Parallel(). +// Because it swaps the process-global os.Stdout/os.Stderr, no test in package +// main may call t.Parallel() — not just the ones in this file. A parallel test +// anywhere in the package runs concurrently with these, and would both race on +// those two variables and have its own output swallowed by whichever pipe was +// installed at the time. // // Each pipe is drained by its own goroutine STARTED BEFORE run(), never read // after it returns: a pipe holds only a fixed kernel buffer (64 KiB on Linux, @@ -66,10 +69,14 @@ func runCLI(t *testing.T, args ...string) (stdout, stderr string, code int) { } // testConfigPath writes a valid, self-contained config to a temp file and -// returns its path. config.Default()'s own provider/endpoint set is empty, so -// this touches no network by default; a case that needs to exercise a real -// network-bound path (there are none in this file) would have to opt in -// explicitly. +// returns its path. +// +// config.Default() carries no endpoints, but it DOES carry real geo providers +// (ipinfo.io and friends) — so "no network" holds because none of the commands +// exercised here performs a country lookup, not because the config is inert. +// `monitor` is the one that would, which is why TestCmdMonitorNoProviders +// overrides Providers with an unrecognized URL rather than relying on the +// default. Any new case reaching a lookup path must do the same. func testConfigPath(t *testing.T, mutate func(*config.Config)) string { t.Helper() cfg := config.Default() diff --git a/cmd/dezhban/main.go b/cmd/dezhban/main.go index 994090f..a631710 100644 --- a/cmd/dezhban/main.go +++ b/cmd/dezhban/main.go @@ -1722,38 +1722,65 @@ func buildServiceCheck(unit svc.BootUnit, daemonLive bool) doctorCheck { return c } +// setGroupFix is the runnable `config set` line that points control.group at +// this host's existing administrators group — the one fix for every "no group +// is configured" branch below. It keeps `sudo`: config writes are in the +// privileged set (they need an enrolled control token, not just group +// membership), and a check that exists to explain a password prompt must not +// model a command that would fail without one. +func setGroupFix() string { + if runtime.GOOS == "darwin" { + return "sudo dezhban config set control.group admin" + } + return "sudo dezhban config set control.group sudo # or wheel, whichever your distro uses" +} + // buildControlCheck reports whether routine ops (block/unblock/switch/pause/ // resume/hold) will ask for a password over the control socket — the // passwordless path's own diagnostic. Pure: resp/probeErr are the result of // probeControl(cfg), already run by the caller, so the branches are directly -// testable without a real socket. Mirrors controlStatus's own three-way +// testable without a real socket. Mirrors controlStatus's own four-way // distinction (disabled / forbidden / unreachable / reachable) rather than // reusing its prose, so this stays a structured doctorCheck instead of a -// second renderer reparsing a sentence to guess a status. +// second renderer reparsing a sentence to guess a status — splitting +// "reachable" once more, since a reachable socket with no group configured is +// still a password prompt and has its own fix. +// +// The doc pointer is a Detail, never a Fix: Fixes are runnable commands the GUI +// badges as such, so "see docs/usage/passwordless.md" dressed as one reads as a +// command to type. Where a real command exists it is the Fix; where it doesn't +// — adding yourself to a unix group is usermod on Linux and dseditgroup on +// macOS, neither safe to hand someone unprompted — the branch carries prose and +// no Fix at all. func buildControlCheck(cfg *config.Config, resp control.Response, probeErr error) doctorCheck { c := doctorCheck{Name: "control", Status: checkOK} path := controlSocketPath(cfg) + const seeDoc = "To turn the passwordless path on, see docs/usage/passwordless.md." switch { case !cfg.Control.Enabled: c.Status = checkWarn c.Summary = "disabled (control.enabled=false) — routine ops need sudo." - c.Fixes = []string{"dezhban config set control.enabled true"} + c.Fixes = []string{"sudo dezhban config set control.enabled true"} 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.Fixes = []string{"see docs/usage/passwordless.md"} + c.Details = []string{ + 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."} - c.Fixes = []string{"see docs/usage/passwordless.md"} + c.Details = []string{"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.Fixes = []string{"see docs/usage/passwordless.md"} + c.Details = []string{seeDoc} + c.Fixes = []string{setGroupFix()} default: c.Summary = fmt.Sprintf("reachable (%s, group %q) — routine ops need no password.", path, cfg.Control.Group) } @@ -2007,7 +2034,15 @@ func runDoctor(cfg *config.Config, log *slog.Logger, discover bool) doctorReport cfg.VPN.Advanced.LearnedEndpointTTL, cfg.VPN.Advanced.LearnedMaxPerProfile, len(cfg.VPN.Endpoints), now)) - controlResp, controlErr := probeControl(cfg) + // Probe only when the socket is supposed to exist. buildControlCheck's + // disabled branch discards resp/probeErr anyway, and controlReachable and + // controlStatus short-circuit the same way rather than dialing a path the + // config says nothing should be listening on. + var controlResp control.Response + var controlErr error + if cfg.Control.Enabled { + controlResp, controlErr = probeControl(cfg) + } checks = append(checks, buildControlCheck(cfg, controlResp, controlErr)) // Touch ID discoverability (macOS): privileged ops (start/stop/panic, GUI diff --git a/docs/usage/passwordless.md b/docs/usage/passwordless.md index e300373..cd981c9 100644 --- a/docs/usage/passwordless.md +++ b/docs/usage/passwordless.md @@ -94,6 +94,12 @@ entirely, no matter how `control.group` is set: its own service lifecycle. - `vpn add`, `vpn remove`, `vpn promote`, `vpn forget`, `vpn import` — these write to files the daemon owns. +- `config set`, `config edit`, `config preset apply` — including the very + `sudo dezhban config set control.group …` this page opens with. Group + membership is not enough for a config write: that path wants an enrolled + control token, so without one it falls back to `sudo` no matter what + `control.group` says. Reading the config (`config show`/`path`/`schema`, + `preset list`/`show`/`diff`) needs nothing. - `token enroll`, `token forget` — the control token that lets a *macOS Touch ID* prompt substitute for `sudo` on config writes; enrolling one is itself a privileged, once-per-setup action. @@ -108,7 +114,8 @@ this page is only about the routine ops that don't have to. ## Troubleshooting - **`disabled (control.enabled=false)`** — the socket itself is off: - `dezhban config set control.enabled true`. + `sudo dezhban config set control.enabled true`. Config writes keep their + `sudo` on purpose — see [What still needs root](#what-still-needs-root-and-why). - **`unreachable`** — no daemon is listening yet. Start one (`sudo dezhban start`), or check that `control.socket` (if you've set a custom path) actually exists. diff --git a/internal/runner/recovery_test.go b/internal/runner/recovery_test.go index 5cd5682..fedd736 100644 --- a/internal/runner/recovery_test.go +++ b/internal/runner/recovery_test.go @@ -146,15 +146,15 @@ func TestTunnelUpWhileBlockedProbesUntilTheGuardIsRestored(t *testing.T) { done := make(chan error, 1) go func() { done <- Run(ctx, o) }() - tun.send(netdetect.TunnelState{Up: true, Names: []string{"utun4"}, Detail: "connected"}) + tun.send(t, netdetect.TunnelState{Up: true, Names: []string{"utun4"}, Detail: "connected"}) if !waitFor(t, snaps, func(s state.Snapshot) bool { return s.Posture == "full-block" }) { t.Fatal("never reached FULL BLOCK on a blocked exit") } // The VPN redials onto an allowed exit: drop, then up. mon.cc = "US" - tun.send(netdetect.TunnelState{Up: false, Detail: "dropped"}) - tun.send(netdetect.TunnelState{Up: true, Names: []string{"utun4"}, Detail: "redialed"}) + tun.send(t, netdetect.TunnelState{Up: false, Detail: "dropped"}) + tun.send(t, netdetect.TunnelState{Up: true, Names: []string{"utun4"}, Detail: "redialed"}) if !waitFor(t, snaps, func(s state.Snapshot) bool { return s.Posture == "guard" }) { t.Fatal("the guard was never restored; a tunnel-up edge in FULL BLOCK must probe for recovery " + @@ -187,15 +187,15 @@ func TestNoAccelerationWhenProbingWouldHaveToLiftTheGuard(t *testing.T) { done := make(chan error, 1) go func() { done <- Run(ctx, o) }() - tun.send(netdetect.TunnelState{Up: true, Names: []string{"utun4"}, Detail: "connected"}) + tun.send(t, netdetect.TunnelState{Up: true, Names: []string{"utun4"}, Detail: "connected"}) if !waitFor(t, snaps, func(s state.Snapshot) bool { return s.Posture == "full-block" }) { t.Fatal("never reached FULL BLOCK") } before := mon.n.Load() mon.cc = "US" - tun.send(netdetect.TunnelState{Up: false, Detail: "dropped"}) - tun.send(netdetect.TunnelState{Up: true, Names: []string{"utun4"}, Detail: "redialed"}) + tun.send(t, netdetect.TunnelState{Up: false, Detail: "dropped"}) + tun.send(t, netdetect.TunnelState{Up: true, Names: []string{"utun4"}, Detail: "redialed"}) // Absence assertion: no provider pass means acceleration must not start. // Poll the window an accelerated cadence would have used, failing as soon @@ -247,7 +247,8 @@ func (s *scriptedWatcher) watcher() *netdetect.Watcher { } } -func (s *scriptedWatcher) send(st netdetect.TunnelState) { +func (s *scriptedWatcher) send(t *testing.T, st netdetect.TunnelState) { + t.Helper() s.mu.Lock() s.last = st s.mu.Unlock() @@ -255,9 +256,19 @@ func (s *scriptedWatcher) send(st netdetect.TunnelState) { // netdetect's default DownDebounce is 2; wait for a few more samples of // the new state to clear it and register as a real edge, bounded by a // generous deadline for a slow CI runner. + // + // The counter is read AFTER the unlock on purpose: Sample increments before + // it takes the mutex, so every sample counted from here on acquires the + // lock after this write and therefore observes the new state. Timing out is + // a hard failure, not a silent pass — a send that never registered leaves + // the test asserting on an edge that was never delivered, and the resulting + // failure names the wrong thing. target := s.samples.Load() + 3 deadline := time.Now().Add(2 * time.Second) - for s.samples.Load() < target && time.Now().Before(deadline) { + for s.samples.Load() < target { + if time.Now().After(deadline) { + t.Fatalf("watcher never sampled the new tunnel state (up=%v) enough times to clear the down debounce", st.Up) + } time.Sleep(time.Millisecond) } } diff --git a/scripts/install.sh b/scripts/install.sh index 8955ac6..f973765 100644 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -329,6 +329,9 @@ trap 'rm -rf "$tmp"' EXIT step "downloading $asset $tag" dl "$asset" +# Not dl(): SHA256SUMS is a few hundred bytes, and dl's interactive branch would +# flash a progress bar that finishes before it can be read. Same URL shape, +# always quiet. curl -fsSL -o "$tmp/SHA256SUMS" "$GH/releases/download/$tag/SHA256SUMS" if [ "$install_app" = 1 ]; then step "downloading Dezhban-macos.app.zip" @@ -502,7 +505,6 @@ fi echo if [ "$mode" = fresh ]; then echo "dezhban $version installed." - echo "next steps:" # Explicitly gated on interactive, not just confirm()'s own default: a # piped/non-interactive run must never launch an interactive wizard, no # matter what a "default yes" would otherwise imply. @@ -510,11 +512,16 @@ if [ "$mode" = fresh ]; then if [ "$interactive" = 1 ]; then confirm "Run the setup wizard now? (VPN, tunnel interfaces, blocked countries)" Y && setup_now=1 fi + # The wizard runs BEFORE the "next steps" heading, never under it: it is + # pages of its own interactive output, so a heading printed first has + # scrolled off by the time it exits, and the steps that follow it would + # read as part of the wizard. if [ "$setup_now" = 1 ]; then /usr/local/bin/dezhban setup < /dev/tty - else - echo " dezhban setup # configure: VPN, tunnel interfaces, blocked countries" + echo fi + echo "next steps:" + [ "$setup_now" = 1 ] || echo " dezhban setup # configure: VPN, tunnel interfaces, blocked countries" if [ "$install_service" = 1 ]; then echo " sudo dezhban start # arm the kill switch" else From 223d345d91d2f64b4d8caa713926372c22a51b10 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Wed, 29 Jul 2026 12:03:13 +0330 Subject: [PATCH 4/5] fix(review): close five findings from the second PR #38 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit install.sh's `start` after an upgrade stop was unguarded: under `set -e` a failure aborted the script silently, after the stop had already torn down every firewall rule — a completed run left the host unprotected with no message and no reason to suspect it. It now dies with a warning naming the exposure and the fix. This one predates the PR (it is on main too), hence the CHANGELOG entry. The optional setup wizard was unguarded the same way, so cancelling it swallowed the "next steps" footer and the uninstall hint; a cancelled wizard now also restores the `dezhban setup` line to that footer. The installer decides whether to restart a daemon by grepping `status --json` for a string owned by internal/svc — a cross-language contract with no compiler behind it, whose silent failure mode is telling the user "the service was not running" while their old build keeps running. Extracted svc.StatusInstalledRunning and pinned both sides with a test (verified it fails when either side drifts). Nits: recovery_test.go's send now uses pollUntil, the helper this same PR added to the same package, instead of a hand-rolled deadline loop; cli.md's link label said docs/upgrade.md for docs/usage/upgrade.md. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 8 ++++++ docs/usage/cli.md | 2 +- internal/runner/recovery_test.go | 10 ++----- internal/svc/program.go | 11 +++++++- internal/svc/status_contract_test.go | 41 ++++++++++++++++++++++++++++ scripts/install.sh | 26 ++++++++++++++++-- 6 files changed, 87 insertions(+), 11 deletions(-) create mode 100644 internal/svc/status_contract_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index c13d1bf..5540dbe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,14 @@ current as you land changes. — a running process keeps its old inode), but the stop/restart is skipped on refusal, leaving the old build enforcing until `sudo dezhban restart` succeeds on its own. No override, matching `upgrade apply`'s own rule. +- **`scripts/install.sh` could finish with the kill switch disarmed and say + nothing.** When an upgrade restarted a running daemon, the `start` was + unguarded: under `set -e` a failure aborted the script with no message at + all — after the `stop` had already run, so every firewall rule was gone. + The install looked like it had merely "failed" while the host was in fact + left unprotected. It now stops with an explicit warning that names the + exposure and the command that ends it. Relatedly, cancelling the optional + setup wizard on a fresh install no longer swallows the "next steps" footer. - **Docs and `doctor`/`setup` hints no longer model `sudo` on commands that don't need it.** `status` never needed root — its `doctor` fix hint wrongly said otherwise. `block`, `unblock`, `switch`, `pause`, `resume` route over diff --git a/docs/usage/cli.md b/docs/usage/cli.md index 995945c..2a31a57 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -508,7 +508,7 @@ daemon's posture makes that safe (healthy `guard` or `standby`; never `full-block` or an open switch window). `can-activate` reports that same verdict without applying anything — `scripts/install.sh`'s upgrade path checks it before restarting a service that was already running, so it can -never lift a block by accident. See [docs/upgrade.md](upgrade.md) for the +never lift a block by accident. See [docs/usage/upgrade.md](upgrade.md) for the full design: why it's split this way, the activation gate, rollback, and the menubar app's **About → Updates** panel. diff --git a/internal/runner/recovery_test.go b/internal/runner/recovery_test.go index fedd736..0698255 100644 --- a/internal/runner/recovery_test.go +++ b/internal/runner/recovery_test.go @@ -2,6 +2,7 @@ package runner import ( "context" + "fmt" "net/netip" "sync" "sync/atomic" @@ -264,13 +265,8 @@ func (s *scriptedWatcher) send(t *testing.T, st netdetect.TunnelState) { // the test asserting on an edge that was never delivered, and the resulting // failure names the wrong thing. target := s.samples.Load() + 3 - deadline := time.Now().Add(2 * time.Second) - for s.samples.Load() < target { - if time.Now().After(deadline) { - t.Fatalf("watcher never sampled the new tunnel state (up=%v) enough times to clear the down debounce", st.Up) - } - time.Sleep(time.Millisecond) - } + pollUntil(t, 2*time.Second, func() bool { return s.samples.Load() >= target }, + fmt.Sprintf("watcher never sampled the new tunnel state (up=%v) enough times to clear the down debounce", st.Up)) } // waitFor drains snapshots until one satisfies pred, or the deadline passes. diff --git a/internal/svc/program.go b/internal/svc/program.go index f1be235..20bc460 100644 --- a/internal/svc/program.go +++ b/internal/svc/program.go @@ -153,6 +153,15 @@ func Run(build Builder, baseLog *slog.Logger, level, configPath string, persist return s.Run() } +// StatusInstalledRunning is what Status returns for a live service, and it is +// not only a display string: scripts/install.sh greps `dezhban status --json` +// for it to decide whether an upgrade has a running daemon to restart. That +// makes it a cross-language contract with no compiler behind it — rename the +// literal and the installer silently stops restarting daemons, then tells the +// user "The service was not running, so it was left stopped", which is false. +// TestInstallerGrepsTheLiveServiceString pins the two sides together. +const StatusInstalledRunning = "installed, running" + // Status reports whether the service is installed and running, as a short string // for `dezhban status`. Querying status may require privilege on some platforms; // any error is surfaced rather than hidden. @@ -168,7 +177,7 @@ func Status() string { case err != nil: return "unknown: " + err.Error() case st == service.StatusRunning: - return "installed, running" + return StatusInstalledRunning case st == service.StatusStopped: return "installed, stopped" default: diff --git a/internal/svc/status_contract_test.go b/internal/svc/status_contract_test.go new file mode 100644 index 0000000..9777d7f --- /dev/null +++ b/internal/svc/status_contract_test.go @@ -0,0 +1,41 @@ +package svc + +import ( + "os" + "strings" + "testing" +) + +// installScript is read, not executed: this asserts a text contract, and the +// script itself needs root, a network, and a real service manager. +const installScript = "../../scripts/install.sh" + +// The installer decides whether an upgrade has a live daemon to restart by +// grepping `dezhban status --json` for the string Status returns for a running +// service. Nothing else connects those two files — no import, no shared type, +// no build error — so a rename on the Go side leaves the installer matching a +// string that can no longer appear. +// +// The failure is silent and actively misleading rather than loud: was_running +// stays 0, the stop/restart is skipped, and the footer tells the user "The +// service was not running, so it was left stopped" while their daemon is in +// fact still running the OLD build. They have no reason to run `dezhban +// restart`, so the upgrade never actually takes effect. +// +// Asserting on the constant, not a second copy of the literal, is the point: +// this test can only pass when both sides say the same thing. +func TestInstallerGrepsTheLiveServiceString(t *testing.T) { + t.Parallel() + + data, err := os.ReadFile(installScript) + if err != nil { + t.Fatalf("read %s: %v", installScript, err) + } + if !strings.Contains(string(data), StatusInstalledRunning) { + t.Errorf("%s no longer matches svc.StatusInstalledRunning (%q).\n"+ + "These two must agree: the installer greps `dezhban status --json` for that exact string to\n"+ + "decide whether to restart a running daemon after an upgrade. Update the grep in install.sh's\n"+ + "was_running check, or revert the constant.", + installScript, StatusInstalledRunning) + } +} diff --git a/scripts/install.sh b/scripts/install.sh index f973765..2d498d6 100644 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -484,7 +484,17 @@ fi if [ "$restart_after" = 1 ]; then step "restarting the service" - /usr/local/bin/dezhban --no-sudo start + # The one step in this script whose failure leaves the host LESS protected + # than when it started: the stop above already ran, so the daemon's deferred + # Cleanup removed every rule. Bare, `set -e` would abort here silently — no + # message, no footer, exit 1 — and the user would be left with a disarmed + # kill switch and no reason to suspect it. Say so explicitly instead, and + # name the one command that fixes it. + /usr/local/bin/dezhban --no-sudo start || die \ +"the service was stopped for the upgrade but did NOT come back — dezhban is not enforcing right now. + The new build is installed correctly; only the restart failed. Start it with: + sudo dezhban start + If that also fails, check 'sudo dezhban doctor' and the service log." fi # The uninstaller comes from the SAME tag being installed — same guarantee the @@ -516,8 +526,20 @@ if [ "$mode" = fresh ]; then # pages of its own interactive output, so a heading printed first has # scrolled off by the time it exits, and the steps that follow it would # read as part of the wizard. + # + # A wizard the user cancels (Ctrl-C, or any non-zero exit) must not take the + # rest of this script with it: under `set -e` a bare call would abort before + # "next steps" and the uninstall hint ever print, so a completed install + # would look like a failed one and the user would never be told to run + # `dezhban start`. Clearing setup_now also puts the `dezhban setup` line back + # into the footer below, which is exactly right — the config still isn't + # written. if [ "$setup_now" = 1 ]; then - /usr/local/bin/dezhban setup < /dev/tty + if ! /usr/local/bin/dezhban setup < /dev/tty; then + setup_now=0 + echo + note "the setup wizard did not finish — dezhban itself installed fine; nothing was configured" + fi echo fi echo "next steps:" From d091ed06cf4aa4be9098e2bc07f6fa53e76075e9 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Wed, 29 Jul 2026 12:14:56 +0330 Subject: [PATCH 5/5] fix(review): close three findings from the third PR #38 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Enforce the "no t.Parallel() in package main" invariant instead of only documenting it above runCLI. It is the one rule in that package a compiler cannot keep, and this PR made it easier to break both ways: it added a new test file there and it wrote the unit-test policy that now asks for t.Parallel() BY DEFAULT. A violation races two goroutines on the global os.Stdout/os.Stderr and silently redirects one test's output into another's pipe — nondeterministic, and it lands on whoever runs the suite next rather than on the author. TestNoTestInPackageMainIsParallel walks each test file's AST (not a text scan, which would flag the files that merely describe the rule) and names file:line. Verified to fail on an injected violation. - Name that exception in docs/contribute/testing.md's t.Parallel() bullet. The policy already exempts "process-global state" in the abstract; the one package where that actually bites should say so, especially since — unlike t.Setenv, whose own panic is the tell — nothing here fails on its own. - countingHandler now holds an *atomic.Int64 rather than a *int64 driven by atomic.AddInt64/LoadInt64, matching countingMonitor and scriptedWatcher in the same package (both added by this PR). The typed form is the one that cannot be read non-atomically by accident. Also avoided parser.ParseDir in the new guard: it is deprecated in favour of golang.org/x/tools/go/packages, and a test does not get to add a dependency. Verified: build + vet (darwin/linux/windows), go test ./... 755 passed in 26 packages, same under -race, coverage gate PASS (total 60.2%). Co-Authored-By: Claude Opus 5 --- cmd/dezhban/harness_test.go | 51 +++++++++++++++++++++++++++++++++ docs/contribute/testing.md | 4 +++ internal/runner/control_test.go | 11 ++++--- 3 files changed, 62 insertions(+), 4 deletions(-) diff --git a/cmd/dezhban/harness_test.go b/cmd/dezhban/harness_test.go index 4593767..6b966ba 100644 --- a/cmd/dezhban/harness_test.go +++ b/cmd/dezhban/harness_test.go @@ -2,6 +2,9 @@ package main import ( "encoding/json" + "go/ast" + "go/parser" + "go/token" "io" "os" "path/filepath" @@ -68,6 +71,54 @@ func runCLI(t *testing.T, args ...string) (stdout, stderr string, code int) { return <-outCh, <-errCh, code } +// The invariant runCLI's doc comment states, enforced instead of merely +// documented. It is the one rule in this package a compiler cannot keep: adding +// `t.Parallel()` to a test here is legal Go, passes review as "following the +// unit test policy" (docs/contribute/testing.md now asks for it BY DEFAULT), and +// breaks nothing visibly — it just races two goroutines on os.Stdout/os.Stderr +// and silently sends one test's output into the other's pipe. That failure is +// nondeterministic and lands on whoever runs the suite next, not on the author. +// +// Parsed rather than grepped so the rule reads actual calls: the sentence above, +// the string in the failure message, and runCLI's own comment all contain the +// spelling, and a text scan would flag this file for describing the rule. That +// is also why this test needs no self-exemption and can cover its own file. +func TestNoTestInPackageMainIsParallel(t *testing.T) { + // ParseFile over a glob, not parser.ParseDir: that one is deprecated in + // favour of golang.org/x/tools/go/packages, and this repo does not take a + // dependency to satisfy a test (see CLAUDE.md's Conventions). + files, err := filepath.Glob("*_test.go") + if err != nil { + t.Fatalf("glob package main's test files: %v", err) + } + if len(files) == 0 { + t.Fatal("no test files found — this guard would pass vacuously") + } + + fset := token.NewFileSet() + for _, name := range files { + file, err := parser.ParseFile(fset, name, nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", name, err) + } + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Parallel" { + return true + } + t.Errorf("%s calls t.Parallel(); no test in package main may.\n"+ + "runCLI swaps the process-global os.Stdout/os.Stderr, so a parallel test anywhere in\n"+ + "this package races it and has its own output swallowed. See runCLI's doc comment.", + fset.Position(call.Pos())) + return true + }) + } +} + // testConfigPath writes a valid, self-contained config to a temp file and // returns its path. // diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index 22973ff..7653f8d 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -47,6 +47,10 @@ root and no real firewall. `task test:cover` enforces the coverage floors in - **`t.Parallel()` by default.** Add it to every new test unless the test uses `t.Setenv` or otherwise mutates process-global state (`t.Setenv` itself already fails if paired with `t.Parallel()`, which is the tell). + **`cmd/dezhban` is the standing exception — no test there may be parallel**, + because `runCLI` swaps the process-global `os.Stdout`/`os.Stderr` and a + parallel test anywhere in the package races it. Unlike `t.Setenv` nothing + fails on its own here, so `TestNoTestInPackageMainIsParallel` enforces it. - **Table-driven once there are ≥3 similar cases; a plain `t.Run` below that.** A table with one or two rows is indirection with nothing to show for it. - **Assert observable behaviour, not call arguments.** Prefer "the firewall diff --git a/internal/runner/control_test.go b/internal/runner/control_test.go index 32b1d5c..91b4394 100644 --- a/internal/runner/control_test.go +++ b/internal/runner/control_test.go @@ -34,16 +34,19 @@ func pollUntil(t *testing.T, timeout time.Duration, cond func() bool, msg string // countingHandler counts slog records whose message contains substr, so a // test can poll for a specific number of log-visible events instead of // sleeping over a fixed wall-clock window. +// atomic.Int64, not a *int64 driven by atomic.AddInt64: the same as +// countingMonitor and scriptedWatcher in this package, and the typed form is +// the one that cannot be read non-atomically by accident. type countingHandler struct { substr string - count *int64 + count *atomic.Int64 } func (h countingHandler) Enabled(context.Context, slog.Level) bool { return true } func (h countingHandler) Handle(_ context.Context, r slog.Record) error { if strings.Contains(r.Message, h.substr) { - atomic.AddInt64(h.count, 1) + h.count.Add(1) } return nil } @@ -168,7 +171,7 @@ func TestControlManualBlockHeldAcrossGeoTicks(t *testing.T) { be := &fakeBackend{} o := vpnOpts(be) o.Interval = 5 * time.Millisecond // geo ticks fire continuously - var skipped int64 + var skipped atomic.Int64 o.Log = slog.New(countingHandler{substr: "manual block held", count: &skipped}) path := startControlled(t, o) @@ -180,7 +183,7 @@ func TestControlManualBlockHeldAcrossGeoTicks(t *testing.T) { // Wait for many geo ticks to actually be suspended by the manual block — not // merely idle. A running state machine would probe (apply-guard + // apply-fullblock) and then lift the block on the steady "US" reading. - pollUntil(t, 3*time.Second, func() bool { return atomic.LoadInt64(&skipped) >= 10 }, + pollUntil(t, 3*time.Second, func() bool { return skipped.Load() >= 10 }, "geo ticks were not suspended by the manual block in time") resp := do(t, path, control.Request{Op: control.OpStatus})