diff --git a/agent/internal/report/redact_test.go b/agent/internal/report/redact_test.go index 0d5b7ac..191f744 100644 --- a/agent/internal/report/redact_test.go +++ b/agent/internal/report/redact_test.go @@ -104,3 +104,25 @@ func TestRedactForTransportBoundsLength(t *testing.T) { t.Errorf("RedactForTransport did not bound length: %d chars", len(got)) } } + +// Docker client errors embed the daemon socket path, which on a desktop +// install contains the operator's account name. This repo is published under a +// pseudonym, so a username reaching the control plane is a real problem, not a +// cosmetic one. +func TestScrubDockerSocketPath(t *testing.T) { + in := `start container from mysql:8: Post "http://%2FUsers%2Fdbelle%2F.docker%2Frun%2Fdocker.sock/v1.48/containers/abc/start": terminated signal received` + got := Scrub(in) + if strings.Contains(got, "dbelle") { + t.Errorf("leaked the account name: %s", got) + } + for _, keep := range []string{"mysql:8", "terminated signal received"} { + if !strings.Contains(got, keep) { + t.Errorf("over-redacted, lost %q: %s", keep, got) + } + } + // A plain http URL that is not the docker socket must be untouched. + url := "checked https://example.com/v1/thing" + if Scrub(url) != url { + t.Errorf("rewrote an unrelated URL: %s", Scrub(url)) + } +} diff --git a/agent/internal/report/report.go b/agent/internal/report/report.go index e1309b7..4adcd82 100644 --- a/agent/internal/report/report.go +++ b/agent/internal/report/report.go @@ -58,12 +58,23 @@ type RunResult struct { // permits in rest:/s3: URLs) are fully masked, not partially. var credRe = regexp.MustCompile(`([a-zA-Z][a-zA-Z0-9+.-]*://)[^\s]+@`) -// Scrub masks credentials embedded in URLs anywhere in s. Repo strings and -// error strings must pass through Scrub before entering a RunResult. The -// regex is whitespace-bounded, so it can only ever over-mask within a token -// (safe), never leak. +// dockerSockRe matches the Docker daemon endpoint the client embeds in its +// errors, e.g. +// +// Post "http://%2FUsers%2Falice%2F.docker%2Frun%2Fdocker.sock/v1.48/…" +// +// That path contains the operator's account name, which has no business +// travelling to the control plane or sitting in a log. Matching on the +// docker.sock suffix keeps this from touching anything else. +var dockerSockRe = regexp.MustCompile(`(?i)https?://[^/"\s]*docker\.sock`) + +// Scrub masks credentials embedded in URLs anywhere in s, and the local Docker +// socket path. Repo strings and error strings must pass through Scrub before +// entering a RunResult. The credential regex is whitespace-bounded, so it can +// only ever over-mask within a token (safe), never leak. func Scrub(s string) string { - return credRe.ReplaceAllString(s, "${1}***@") + s = credRe.ReplaceAllString(s, "${1}***@") + return dockerSockRe.ReplaceAllString(s, "http://docker.sock") } // dbDetailRe matches the start of a message segment that echoes row values out diff --git a/agent/internal/restic/restic.go b/agent/internal/restic/restic.go index 43ebaef..b301fde 100644 --- a/agent/internal/restic/restic.go +++ b/agent/internal/restic/restic.go @@ -81,6 +81,14 @@ func (r *Runner) env() []string { func (r *Runner) run(ctx context.Context, sub subcommand, out io.Writer, args ...string) error { full := append([]string{string(sub), "--repo", r.repo}, args...) cmd := exec.CommandContext(ctx, r.bin, full...) + // Without this, cancellation SIGKILLs restic, which then never releases + // the repository lock it took. The user's own `restic forget`/`prune` + // afterwards fails with "repository is already locked by PID ...", so a + // Ctrl-C here silently breaks their retention job. Interrupt instead, the + // way a real Ctrl-C would, and give restic a moment to unlock before the + // runtime forces the issue. + cmd.Cancel = func() error { return cmd.Process.Signal(os.Interrupt) } + cmd.WaitDelay = 10 * time.Second var stderr bytes.Buffer cmd.Stdout = out cmd.Stderr = &stderr diff --git a/agent/internal/sandbox/reap_test.go b/agent/internal/sandbox/reap_test.go new file mode 100644 index 0000000..f032e09 --- /dev/null +++ b/agent/internal/sandbox/reap_test.go @@ -0,0 +1,87 @@ +package sandbox + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +// Destroy is deferred and survives panics and signals, but nothing survives +// SIGKILL or an OOM kill. Those leave the user's restored data on disk with no +// owner, and before this nothing ever reclaimed it. +func TestReapOrphans(t *testing.T) { + base := t.TempDir() + + mk := func(name string, age time.Duration) string { + p := filepath.Join(base, name) + if err := os.MkdirAll(p, 0o700); err != nil { + t.Fatal(err) + } + // Sandboxes hold restored data, so put something in it. + if err := os.WriteFile(filepath.Join(p, "restored.db"), []byte("customer data"), 0o600); err != nil { + t.Fatal(err) + } + when := time.Now().Add(-age) + if err := os.Chtimes(p, when, when); err != nil { + t.Fatal(err) + } + return p + } + + abandoned := mk("restorable-abandoned", 48*time.Hour) + live := mk("restorable-live", time.Minute) // a concurrent run + borderline := mk("restorable-recent", 23*time.Hour) // inside the window + foreign := mk("someone-elses-tempdir", 48*time.Hour) + + reapOrphans(base) + + for _, tc := range []struct { + path string + wantGone bool + why string + }{ + {abandoned, true, "old sandbox holding restored data must be reclaimed"}, + {live, false, "a concurrent run's sandbox must never be deleted"}, + {borderline, false, "inside the age window, still assume an owner"}, + {foreign, false, "only directories this package creates may be removed"}, + } { + _, err := os.Stat(tc.path) + gone := os.IsNotExist(err) + if gone != tc.wantGone { + t.Errorf("%s: gone=%v, want %v (%s)", filepath.Base(tc.path), gone, tc.wantGone, tc.why) + } + } +} + +// Reaping is housekeeping. It must never be the reason a run cannot start. +func TestReapOrphansToleratesUnreadableBase(t *testing.T) { + reapOrphans(filepath.Join(t.TempDir(), "does-not-exist")) +} + +// The reaper runs inside New, so a live sandbox created immediately after an +// abandoned one is reclaimed must still be usable. +func TestNewStillWorksAfterReaping(t *testing.T) { + base := t.TempDir() + old := filepath.Join(base, "restorable-old") + if err := os.MkdirAll(old, 0o700); err != nil { + t.Fatal(err) + } + when := time.Now().Add(-48 * time.Hour) + if err := os.Chtimes(old, when, when); err != nil { + t.Fatal(err) + } + + sb, err := New(base, 0) + if err != nil { + t.Fatalf("New after reaping: %v", err) + } + defer sb.Destroy() //nolint:errcheck // test cleanup + + if _, err := os.Stat(old); !os.IsNotExist(err) { + t.Error("abandoned sandbox survived New") + } + if _, err := os.Stat(sb.Dir()); err != nil { + t.Errorf("new sandbox unusable: %v", err) + } +} diff --git a/agent/internal/sandbox/sandbox.go b/agent/internal/sandbox/sandbox.go index 25176f4..d0fce37 100644 --- a/agent/internal/sandbox/sandbox.go +++ b/agent/internal/sandbox/sandbox.go @@ -7,6 +7,9 @@ package sandbox import ( "fmt" "os" + "path/filepath" + "strings" + "time" ) // Sandbox is a disposable directory a snapshot is restored into. @@ -33,6 +36,7 @@ func New(baseDir string, requiredBytes uint64) (*Sandbox, error) { "insufficient disk space in %s: %s free, %s required — aborting before restore", baseDir, humanBytes(free), humanBytes(requiredBytes)) } + reapOrphans(baseDir) dir, err := os.MkdirTemp(baseDir, "restorable-") if err != nil { return nil, fmt.Errorf("create sandbox dir: %w", err) @@ -40,6 +44,42 @@ func New(baseDir string, requiredBytes uint64) (*Sandbox, error) { return &Sandbox{dir: dir}, nil } +// orphanAge is how old a sandbox must be before it is assumed abandoned. A +// restore is bounded by disk and network, never by days, so this is far above +// any legitimate run while still well inside "the user would rather not keep +// their restored data lying around". +const orphanAge = 24 * time.Hour + +// reapOrphans removes sandboxes left behind by runs that died outright. +// +// Destroy is deferred and survives panics, failures and signals, but nothing +// survives SIGKILL, an OOM kill, or the power going out. Those leave a full +// copy of the user's restored data on disk with no owner, which is both a disk +// leak and a privacy problem, and no later run ever reclaimed it. +// +// Deliberately conservative, because this deletes directories: only entries +// directly under the configured base dir, only ones matching the prefix this +// package creates, and only after orphanAge, so a concurrent agent's live +// sandbox is never a candidate. Failures are ignored; reaping is housekeeping +// and must never be the reason a verification run cannot start. +func reapOrphans(baseDir string) { + entries, err := os.ReadDir(baseDir) + if err != nil { + return + } + cutoff := time.Now().Add(-orphanAge) + for _, e := range entries { + if !e.IsDir() || !strings.HasPrefix(e.Name(), "restorable-") { + continue + } + info, err := e.Info() + if err != nil || info.ModTime().After(cutoff) { + continue + } + _ = os.RemoveAll(filepath.Join(baseDir, e.Name())) + } +} + // Dir returns the sandbox directory path. func (s *Sandbox) Dir() string { return s.dir } diff --git a/web/src/lib/api/schemas.test.ts b/web/src/lib/api/schemas.test.ts index 9c70e4a..ca2c83e 100644 --- a/web/src/lib/api/schemas.test.ts +++ b/web/src/lib/api/schemas.test.ts @@ -73,3 +73,34 @@ describe("registerRequestSchema", () => { ).toBe(false); }); }); + +describe("restore_duration_ms sanity", () => { + const base = { + repo_fingerprint: "a".repeat(64), + repo_label: "/srv/backups/restic", + status: "pass" as const, + started_at: "2026-09-19T10:00:00.000Z", + finished_at: "2026-09-19T10:00:01.000Z", // a one-second run + agent_version: "v0.1.2", + checks: [], + }; + + it("rejects a restore longer than the run that contained it", () => { + // Previously accepted, and the dashboard rendered "1s · restore 10m" as + // the headline verified recovery time. + const r = runRequestSchema.safeParse({ ...base, restore_duration_ms: 600_000 }); + expect(r.success).toBe(false); + }); + + it("accepts a restore that fits inside the run", () => { + expect(runRequestSchema.safeParse({ ...base, restore_duration_ms: 800 }).success).toBe(true); + }); + + it("allows a second of slack for clock granularity", () => { + expect(runRequestSchema.safeParse({ ...base, restore_duration_ms: 1500 }).success).toBe(true); + }); + + it("still accepts runs that omit it", () => { + expect(runRequestSchema.safeParse(base).success).toBe(true); + }); +}); diff --git a/web/src/lib/api/schemas.ts b/web/src/lib/api/schemas.ts index 61a5075..0ec603e 100644 --- a/web/src/lib/api/schemas.ts +++ b/web/src/lib/api/schemas.ts @@ -46,6 +46,18 @@ export const runRequestSchema = z .refine( (r) => new Date(r.finished_at).getTime() >= new Date(r.started_at).getTime(), { message: "finished_at must not be before started_at" }, + ) + // The restore is a phase inside the run, so it cannot outlast it. Without + // this, a one-second run could claim a ten-minute restore and the dashboard + // would render exactly that as "verified recovery time", which is the number + // the whole product is sold on. One second of slack covers clock + // granularity between the two measurements. + .refine( + (r) => + r.restore_duration_ms == null || + r.restore_duration_ms <= + new Date(r.finished_at).getTime() - new Date(r.started_at).getTime() + 1000, + { message: "restore_duration_ms cannot exceed the run's wall-clock duration" }, ); export type RunRequest = z.infer;