From c1dc837deb6e5cc5c1044694d72a8798bd68b22f Mon Sep 17 00:00:00 2001 From: thetechnologist1911 Date: Sat, 19 Sep 2026 09:46:57 -0400 Subject: [PATCH 1/2] fix(agent): stop damaging things outside our own scope Three defects that all share a shape: the agent leaving a mess somewhere it does not own. **Stale repository locks.** exec.CommandContext SIGKILLs on cancellation, so Ctrl-C during a restore killed restic before it could release its lock. The user's next `restic forget` or `prune` then failed with "repository is already locked by PID ...". A tool that sells itself on being read-only against your repository should not break your retention job. Interrupt instead, with a WaitDelay grace period, so restic unwinds the way it would under a real Ctrl-C. Measured, interrupting inside the restore window, five trials each: before: LOCKED LOCKED LOCKED clean LOCKED after: clean clean clean clean clean **Orphaned sandboxes.** Destroy is deferred and survives panics, failures and signals, but nothing survives SIGKILL, an OOM kill or a power cut. Those left a complete copy of the user's restored data on disk with no owner, and no later run ever reclaimed it: a disk leak and a privacy problem on the same facts. New now sweeps abandoned sandboxes first. Deliberately conservative, since it deletes directories: only the prefix this package creates, only directly under the configured base dir, and only past an age no legitimate restore reaches, so a concurrent agent's live sandbox is never a candidate. Failures are ignored, because housekeeping must never stop a verification run. **The account name in the Docker socket path.** Docker client errors embed the daemon endpoint, which on a desktop install spells out the operator's username, and that travelled to the control plane. Scrub now masks it. Matching on the docker.sock suffix keeps it from touching any other URL. --- agent/internal/report/redact_test.go | 22 +++++++ agent/internal/report/report.go | 21 +++++-- agent/internal/restic/restic.go | 8 +++ agent/internal/sandbox/reap_test.go | 87 ++++++++++++++++++++++++++++ agent/internal/sandbox/sandbox.go | 40 +++++++++++++ 5 files changed, 173 insertions(+), 5 deletions(-) create mode 100644 agent/internal/sandbox/reap_test.go 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 } From 0a55e3469d453fa3282281f15bb02fd60e1d0671 Mon Sep 17 00:00:00 2001 From: thetechnologist1911 Date: Sat, 19 Sep 2026 09:47:03 -0400 Subject: [PATCH 2/2] fix(web): reject a restore longer than the run that contained it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The schema refined finished_at >= started_at but had no equivalent for the restore being a phase inside the run. A one-second run claiming a ten-minute restore was accepted, and the dashboard rendered it verbatim as "1s · restore 10m · verify 0s" under the heading "verified recovery time". That number is the product's headline claim, so it should be the hardest field to put nonsense into, not the easiest. One second of slack covers clock granularity between the two measurements; anything beyond that is a bug in an agent or a forged submission, and neither should reach the dashboard. --- web/src/lib/api/schemas.test.ts | 31 +++++++++++++++++++++++++++++++ web/src/lib/api/schemas.ts | 12 ++++++++++++ 2 files changed, 43 insertions(+) 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;