From 3cd95820daf6f14fdc7779ade7bd746a1d3e02ac Mon Sep 17 00:00:00 2001 From: thetechnologist1911 Date: Thu, 17 Sep 2026 18:39:49 -0400 Subject: [PATCH 1/4] fix(agent): stop restored database contents leaving the machine RedactForTransport was a no-op on every message the agent can actually produce. The pattern was anchored with (?m)^...$, but tail() joins command output with " / " before the message is built, so there are no line starts to match and DETAIL/CONTEXT segments passed through untouched. What that leaked: a failed database load is the exact case this product exists to detect, and Postgres puts the offending value in DETAIL and the whole failing row in CONTEXT. Those reached the control plane, were stored in test_runs.results, and were lifted into alert bodies sent to Telegram, Discord, ntfy and email. Segment the message on either joiner and match per segment instead. Also covers psql's "LINE n:" echo, which quotes the failing statement text and was not in the pattern at all. RunResult.Error gets the same treatment. It carried only the credential scrub, so a restore failure shipped an unbounded list of paths out of the user's snapshot; the server accepts 4000 characters where check messages are capped at 500. The old unit test passed throughout because it fed a message containing real newlines, a shape nothing generates. Tests now use captured psql and mysql output, and a new test in the recipes package runs tail() output through RedactForTransport so the two cannot drift apart again: the bug lived in the seam between them, not in either function. --- .../recipes/transport_redaction_test.go | 58 +++++++++++++++++++ agent/internal/report/client.go | 14 +++-- agent/internal/report/redact_test.go | 57 +++++++++++++++--- agent/internal/report/report.go | 32 +++++++--- 4 files changed, 141 insertions(+), 20 deletions(-) create mode 100644 agent/internal/recipes/transport_redaction_test.go diff --git a/agent/internal/recipes/transport_redaction_test.go b/agent/internal/recipes/transport_redaction_test.go new file mode 100644 index 0000000..0657af9 --- /dev/null +++ b/agent/internal/recipes/transport_redaction_test.go @@ -0,0 +1,58 @@ +package recipes + +import ( + "strings" + "testing" + + "github.com/restorable-dev/restorable/agent/internal/report" +) + +// The redaction bug this guards against was not in either function on its own. +// tail() joins output with " / " and report's filter was anchored to line +// starts, so each half was individually reasonable and the seam between them +// leaked every row value a failed database load echoes. Unit tests on either +// side passed throughout. +// +// This asserts the two agree, using output captured verbatim from psql and +// mysql, so changing either joiner or pattern in isolation fails here. +func TestTailOutputSurvivesTransportRedaction(t *testing.T) { + tests := []struct { + name string + raw string + mustHide []string + }{ + { + name: "postgres unique violation echoes the key", + raw: "psql:/tmp/dump:5: ERROR: duplicate key value violates unique constraint \"users_pkey\"\n" + + "DETAIL: Key (email)=(alice@example.com) already exists.\n" + + "CONTEXT: COPY users, line 2", + mustHide: []string{"alice@example.com", "COPY users"}, + }, + { + name: "postgres syntax error echoes the statement", + raw: "psql:/tmp/dump:1: ERROR: syntax error at or near \")\"\n" + + "LINE 1: ...email) VALUES ('a@example.com'), ('b@example.com')\n" + + " ^", + mustHide: []string{"a@example.com", "b@example.com"}, + }, + { + name: "mysql detail line", + raw: "ERROR 1062 (23000) at line 3: Duplicate entry 'dave@example.com' for key 'users.email'\n" + + "DETAIL: row 3 rejected", + mustHide: []string{"row 3 rejected"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Exactly what the database checks do before building a message. + joined := tail(tt.raw, 2) + got := report.RedactForTransport(joined) + for _, secret := range tt.mustHide { + if strings.Contains(got, secret) { + t.Errorf("row data survived redaction: %q\n joined: %s\n sent: %s", secret, joined, got) + } + } + }) + } +} diff --git a/agent/internal/report/client.go b/agent/internal/report/client.go index 9251365..b9b3379 100644 --- a/agent/internal/report/client.go +++ b/agent/internal/report/client.go @@ -138,11 +138,15 @@ func (c *Client) SubmitRun(ctx context.Context, r *RunResult) error { fingerprint = Fingerprint(r.Repo) // fallback for old restic without a repo ID } payload := runPayload{ - RepoFingerprint: fingerprint, - RepoLabel: r.Repo, // already scrubbed at result construction - SnapshotID: r.SnapshotID, - Status: r.Status, - Error: r.Error, + RepoFingerprint: fingerprint, + RepoLabel: r.Repo, // already scrubbed at result construction + SnapshotID: r.SnapshotID, + Status: r.Status, + // Same treatment as check messages. A failed restore enumerates paths + // out of the user's snapshot, so this field needs the DB-detail strip + // and the length bound too, not just the credential scrub it already + // carries from result construction. + Error: RedactForTransport(r.Error), StartedAt: r.StartedAt, FinishedAt: r.FinishedAt, RestoreDurationMS: r.RestoreDurationMS, diff --git a/agent/internal/report/redact_test.go b/agent/internal/report/redact_test.go index 2394311..4f2cf89 100644 --- a/agent/internal/report/redact_test.go +++ b/agent/internal/report/redact_test.go @@ -31,15 +31,58 @@ func TestScrubTrickyPasswords(t *testing.T) { // Check messages that leave the machine must not carry DB row data (Postgres // DETAIL lines echo actual values from the user's backup). +// +// Every case here is the shape the recipe engine actually produces. An earlier +// version of this test used only the newline-joined form, which the agent +// never generates: the engine joins command output with " / ", so the +// line-anchored pattern this test was guarding never fired in production and +// the test passed against a message that could not occur. func TestRedactForTransportStripsDBDetail(t *testing.T) { - msg := "dump failed to load into postgres: ERROR: duplicate key value violates unique constraint\n" + - "DETAIL: Key (email)=(alice@example.com) already exists." - got := RedactForTransport(msg) - if strings.Contains(got, "alice@example.com") || strings.Contains(got, "DETAIL") { - t.Errorf("RedactForTransport leaked DB detail: %q", got) + tests := []struct { + name string + msg string + mustHide []string + mustKeep string + }{ + { + name: "slash-joined, the shape tail() produces", + msg: `dump "db/dump.sql" failed to load into postgres: psql:/tmp/dump:5: ERROR: duplicate key value violates unique constraint "users_pkey"` + + ` / DETAIL: Key (email)=(alice@example.com) already exists.` + + ` / CONTEXT: COPY users, line 2`, + mustHide: []string{"alice@example.com", "DETAIL", "CONTEXT", "COPY users"}, + mustKeep: "failed to load", + }, + { + name: "newline-joined", + msg: "dump failed to load into postgres: ERROR: duplicate key\nDETAIL: Key (email)=(bob@example.com) already exists.", + mustHide: []string{"bob@example.com", "DETAIL"}, + mustKeep: "failed to load", + }, + { + name: "psql LINE echo carries the statement text", + msg: `dump failed: ERROR: syntax error at or near ")" / LINE 1: ...email) VALUES ('a@example.com'), ('b@example.com'), ('c@exam`, + mustHide: []string{"a@example.com", "b@example.com", "LINE 1"}, + mustKeep: "dump failed", + }, + { + name: "mysql HINT", + msg: `load failed / HINT: row 4 value "carol@example.com" is invalid`, + mustHide: []string{"carol@example.com", "HINT"}, + mustKeep: "load failed", + }, } - if !strings.Contains(got, "failed to load") { - t.Errorf("RedactForTransport over-redacted, lost the error class: %q", got) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := RedactForTransport(tt.msg) + for _, secret := range tt.mustHide { + if strings.Contains(got, secret) { + t.Errorf("leaked %q\n got: %s", secret, got) + } + } + if !strings.Contains(got, tt.mustKeep) { + t.Errorf("over-redacted, lost %q\n got: %s", tt.mustKeep, got) + } + }) } } diff --git a/agent/internal/report/report.go b/agent/internal/report/report.go index 46df161..fe8464b 100644 --- a/agent/internal/report/report.go +++ b/agent/internal/report/report.go @@ -66,18 +66,34 @@ func Scrub(s string) string { return credRe.ReplaceAllString(s, "${1}***@") } -// pgDetailRe matches Postgres/MySQL DETAIL/HINT/CONTEXT lines, which routinely -// echo actual row values (emails, names) from the user's backup. -var pgDetailRe = regexp.MustCompile(`(?im)^\s*(DETAIL|HINT|CONTEXT|Key \()[^\n]*$`) +// dbDetailRe matches the start of a message segment that echoes row values out +// of the user's restored database: Postgres and MySQL put real column data in +// DETAIL, HINT and CONTEXT, and psql's "LINE n:" echoes the offending +// statement text. +// +// This is deliberately not anchored with (?m)^...$. The recipe engine joins +// command output with " / " before the message is ever built, so a +// line-anchored pattern matches nothing on a real message and the filter +// silently does nothing. Segment the message first, then match per segment. +var dbDetailRe = regexp.MustCompile(`(?i)^\s*(DETAIL|HINT|CONTEXT|LINE\s+\d+|Key\s*\()`) + +// segmentRe splits a check message back into the pieces command output was +// joined from, whichever joiner produced it. +var segmentRe = regexp.MustCompile(`\n| / `) // RedactForTransport prepares a check message to leave the machine. Check // messages are the one field that can embed raw command output from the -// user's restored databases (Postgres error DETAIL lines echo row values), so -// anything sent to the control plane is scrubbed of URL credentials, stripped -// of DB detail lines, and length-bounded. Local stdout keeps the full text. +// user's restored databases, so anything sent to the control plane is stripped +// of DB detail segments, scrubbed of URL credentials, and length-bounded. +// Local stdout keeps the full text. func RedactForTransport(msg string) string { - msg = pgDetailRe.ReplaceAllString(msg, "[redacted]") - msg = Scrub(msg) + segs := segmentRe.Split(msg, -1) + for i, seg := range segs { + if dbDetailRe.MatchString(seg) { + segs[i] = "[redacted]" + } + } + msg = Scrub(strings.Join(segs, " / ")) const max = 500 if len(msg) > max { msg = msg[:max] + "…" From cd3a6c45730d6eec40ecfca13412364293d7e7c8 Mon Sep 17 00:00:00 2001 From: thetechnologist1911 Date: Thu, 17 Sep 2026 18:39:59 -0400 Subject: [PATCH 2/4] fix(web): make alert_channels.verified server-only alert_channels carried a table-level insert/update grant to `authenticated` and RLS policies that only check user_id, so any signed-in user could POST straight to PostgREST with {"verified": true} for an address they do not own. Verified channels receive every alert, and both halves of an alert are submitter controlled: the title carries repo_label and the body carries the check message. With SES out of sandbox at 50k/day and no cap on agents per account, that is a working path to sending attacker-worded mail from our own domain. `verified` asserts that a message actually arrived, which is decided in Node by sendToChannel returning. No policy or user-callable function can enforce that, since either would be equally spoofable, so the privilege is revoked from `authenticated` and the server flips the flag with the service role after a real send. Creating a channel stays a client insert, so the RLS policy and the plan-limit gate still govern it; `verified` simply keeps its default. Editing config stays a client update. Telegram connect now inserts unverified and flips separately rather than inserting pre-verified, which keeps channel creation under the same gate as every other type. Verified against the local stack: insert and update of `verified` both 403 as an authenticated user, normal create returns 201 with verified=false, config edits still work, and the service role can still flip it. alerts.sh was performing this exact bypass in its own setup and now uses the service key, mirroring the app. All four web e2e suites pass. --- web/e2e/alerts.sh | 5 +- web/src/app/dashboard/alerts/actions.ts | 51 +++++++++++++++---- ...00001_alert_channel_verified_privilege.sql | 31 +++++++++++ 3 files changed, 77 insertions(+), 10 deletions(-) create mode 100644 web/supabase/migrations/20260917000001_alert_channel_verified_privilege.sql diff --git a/web/e2e/alerts.sh b/web/e2e/alerts.sh index 4f2e67c..e99dfd5 100755 --- a/web/e2e/alerts.sh +++ b/web/e2e/alerts.sh @@ -90,8 +90,11 @@ curl -sf -X POST "$SUPABASE_URL/rest/v1/alert_channels" \ -H "Prefer: return=representation" \ -d '{"user_id":"'$USER_ID'","type":"telegram","config":{"chat_id":"42"}}' > "$WORK/channel.json" CHANNEL_ID="$(python3 -c 'import json,sys; print(json.load(open(sys.argv[1]))[0]["id"])' "$WORK/channel.json")" +# Service role, because `authenticated` has no privilege on `verified` (see +# migration 20260917000001). This mirrors the app: a user creates the channel, +# and only the server marks it verified once a send has actually succeeded. curl -sf -X PATCH "$SUPABASE_URL/rest/v1/alert_channels?id=eq.$CHANNEL_ID" \ - -H "apikey: $ANON_KEY" -H "Authorization: Bearer $JWT" -H "Content-Type: application/json" \ + -H "apikey: $SERVICE_KEY" -H "Authorization: Bearer $SERVICE_KEY" -H "Content-Type: application/json" \ -d '{"verified":true}' log "building fixture: repo whose recipe requires a missing path" diff --git a/web/src/app/dashboard/alerts/actions.ts b/web/src/app/dashboard/alerts/actions.ts index dc68028..08b0ee6 100644 --- a/web/src/app/dashboard/alerts/actions.ts +++ b/web/src/app/dashboard/alerts/actions.ts @@ -5,9 +5,13 @@ import { revalidatePath } from "next/cache"; import { channelTypeSchema, parseChannelConfig, type ChannelType } from "@/lib/alerts/config"; import { sendToChannel } from "@/lib/alerts/send"; import { getLimits } from "@/lib/billing/entitlements"; +import { createAdminClient } from "@/lib/supabase/admin"; import { createClient } from "@/lib/supabase/server"; -// All actions use the user's own client: RLS scopes every read and write. +// Actions use the user's own client, so RLS scopes every read and write. The +// one exception is setting alert_channels.verified, which `authenticated` has +// no privilege on: it records that a message actually reached the destination, +// so only the code that performed the send may assert it. export async function createAlertChannel( _prev: unknown, @@ -76,6 +80,14 @@ export async function deleteAlertChannel(id: string): Promise { // only verified channels receive real alerts. export async function testAlertChannel(id: string): Promise<{ error?: string }> { const supabase = await createClient(); + const { + data: { user }, + } = await supabase.auth.getUser(); + if (!user) { + return { error: "not signed in" }; + } + // RLS scopes this read to the caller, so a channel coming back at all is + // proof of ownership. const { data: channel } = await supabase .from("alert_channels") .select("id, type, config") @@ -96,7 +108,15 @@ export async function testAlertChannel(id: string): Promise<{ error?: string }> } catch (err) { return { error: err instanceof Error ? err.message : "send failed" }; } - await supabase.from("alert_channels").update({ verified: true }).eq("id", id); + // `verified` is not writable by `authenticated` (see migration + // 20260917000001): it asserts that a message actually arrived, which only + // this code path knows. sendToChannel returned, so flip it with the service + // role, scoped to the owner since that bypasses RLS. + await createAdminClient() + .from("alert_channels") + .update({ verified: true }) + .eq("id", id) + .eq("user_id", user.id); revalidatePath("/dashboard/alerts"); return {}; } @@ -169,15 +189,28 @@ export async function pollTelegramConnect( } } - const { error: insertErr } = await supabase.from("alert_channels").insert({ - user_id: user.id, - type: "telegram", - config: { chat_id: link.chat_id }, - verified: true, // connecting via the bot proves the chat is reachable - }); - if (insertErr) { + // Insert unverified through the user's own client, so the RLS insert policy + // (and with it the plan-limit gate) still governs channel creation. + const { data: created, error: insertErr } = await supabase + .from("alert_channels") + .insert({ + user_id: user.id, + type: "telegram", + config: { chat_id: link.chat_id }, + }) + .select("id") + .single(); + if (insertErr || !created) { return { error: "could not save the Telegram channel" }; } + // Connecting via the bot is itself proof the chat is reachable, so this is + // the one place a channel is born verified. `verified` is not writable by + // `authenticated` (migration 20260917000001), hence the service role. + await createAdminClient() + .from("alert_channels") + .update({ verified: true }) + .eq("id", created.id) + .eq("user_id", user.id); await supabase .from("telegram_links") .update({ consumed_at: new Date().toISOString() }) diff --git a/web/supabase/migrations/20260917000001_alert_channel_verified_privilege.sql b/web/supabase/migrations/20260917000001_alert_channel_verified_privilege.sql new file mode 100644 index 0000000..5839f4b --- /dev/null +++ b/web/supabase/migrations/20260917000001_alert_channel_verified_privilege.sql @@ -0,0 +1,31 @@ +-- alert_channels.verified must not be client-writable. +-- +-- The table carried a table-level `grant insert, update ... to authenticated` +-- and RLS policies that only check user_id, so any signed-in user could POST +-- straight to PostgREST with {"verified": true} and skip verification +-- entirely. A channel is what makes the product send a message, so that turned +-- any account into a sender of attacker-worded email, from our own SES domain, +-- to an address nobody confirmed. The alert title and body are submitter +-- controlled (repo_label and the check message), and nothing caps agents per +-- account. +-- +-- `verified` means "a message was successfully delivered to this destination". +-- That is decided in Node, by sendToChannel actually succeeding, so it cannot +-- be enforced by a policy or by a function the user is able to call directly: +-- either would be equally spoofable. The privilege is therefore removed from +-- `authenticated` outright, and the application flips the flag with the +-- service role only after a real send returns. +-- +-- SELECT and DELETE stay table-level: reading and removing your own channels +-- is already fully governed by the existing RLS policies. + +revoke insert, update on public.alert_channels from authenticated; + +-- Creating a channel stays a client operation, so the RLS insert policy (and +-- with it the plan-limit gate) remains the thing that governs it. `verified` +-- is simply not in the list, so it keeps its `default false`. +grant insert (user_id, type, config) on public.alert_channels to authenticated; + +-- Editing a channel's destination is allowed; re-verification is triggered by +-- the application, which flips `verified` back through the service role. +grant update (config) on public.alert_channels to authenticated; From e9fa630c8c7f076063b7ae9cb66c00418bdead11 Mon Sep 17 00:00:00 2001 From: thetechnologist1911 Date: Thu, 17 Sep 2026 18:48:22 -0400 Subject: [PATCH 3/4] fix(agent): never verify data from outside the restored snapshot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolve() checked containment lexically and then called os.Stat, which follows symlinks. restic restores symlinks as symlinks, so the common homelab shape — /srv/app backed up while `data` links to /var/lib/app — restored a link pointing at the live host copy, and every check read the running system instead of the backup. That is a silent false PASS on the one question this tool exists to answer. Demonstrated against the shipped binary with a real restic repository and a healthy host database behind the link: old: PASS, exit 0 ✓ "data/app.db" passes integrity_check new: FAIL, exit 1 ✗ database "data/app.db" is a symlink leading outside the restored snapshot, so its target was never backed up Resolve both sides before comparing. Comparing a resolved path against an unresolved root would reject everything on macOS, where the sandbox sits under /var/folders and /var is itself a link to /private/var. An escape is reported as its own condition rather than as "not found", because it is a real finding: the symlink was backed up and its target was not, which is a backup gap worth naming. resolve() returns an error instead of a bool to carry that, and the five call sites now interpolate it. Symlinks pointing within the snapshot still resolve normally. --- agent/internal/recipes/dockerapp.go | 8 +- agent/internal/recipes/files.go | 46 ++++++++-- agent/internal/recipes/mysql.go | 6 +- agent/internal/recipes/postgres.go | 6 +- agent/internal/recipes/security_test.go | 2 +- agent/internal/recipes/sqlite.go | 6 +- agent/internal/recipes/symlink_escape_test.go | 89 +++++++++++++++++++ 7 files changed, 142 insertions(+), 21 deletions(-) create mode 100644 agent/internal/recipes/symlink_escape_test.go diff --git a/agent/internal/recipes/dockerapp.go b/agent/internal/recipes/dockerapp.go index 915e5e4..21980fe 100644 --- a/agent/internal/recipes/dockerapp.go +++ b/agent/internal/recipes/dockerapp.go @@ -87,10 +87,10 @@ func (c *DockerAppCheck) Run(ctx context.Context, t *Target) (report.Status, str } hostDir = t.Roots[0].Dir } else { - var ok bool - hostDir, ok = resolve(t.Roots, restored) - if !ok { - return report.StatusFail, fmt.Sprintf("mount path %q not found in restored snapshot", c.Mount.Restored) + var err error + hostDir, err = resolve(t.Roots, restored) + if err != nil { + return report.StatusFail, fmt.Sprintf("mount path %q %v", c.Mount.Restored, err) } } diff --git a/agent/internal/recipes/files.go b/agent/internal/recipes/files.go index a3ad153..8a7c23e 100644 --- a/agent/internal/recipes/files.go +++ b/agent/internal/recipes/files.go @@ -65,9 +65,9 @@ func (f *FilesCheck) Run(ctx context.Context, t *Target) (report.Status, string) for _, req := range f.Require { rel := strings.TrimSuffix(req.Path, "/") - abs, ok := resolve(t.Roots, rel) - if !ok { - issues = append(issues, fmt.Sprintf("required path %q not found in restored snapshot", req.Path)) + abs, err := resolve(t.Roots, rel) + if err != nil { + issues = append(issues, fmt.Sprintf("required path %q %v", req.Path, err)) continue } if req.MinFiles > 0 { @@ -124,20 +124,52 @@ func validateRelPath(p string) error { return nil } +// errPathMissing means nothing exists at the requested path. +var errPathMissing = errors.New("not found in restored snapshot") + +// errPathEscapes means the path exists but leads out of the restored data. For +// a backup tool this is a finding in its own right, not a lookup failure: the +// symlink was backed up and whatever it points at was not. +var errPathEscapes = errors.New( + "is a symlink leading outside the restored snapshot, so its target was never backed up") + // resolve finds rel under one of the restored roots, enforcing containment: // even if a malformed path slips past validation, the resolved location must // stay inside a root directory (defense in depth against traversal). -func resolve(roots []Root, rel string) (string, bool) { +func resolve(roots []Root, rel string) (string, error) { + escaped := false for _, root := range roots { abs := filepath.Join(root.Dir, rel) if !withinRoot(root.Dir, abs) { continue } - if _, err := os.Stat(abs); err == nil { - return abs, true + if _, err := os.Stat(abs); err != nil { + continue } + // os.Stat follows symlinks. Without the check below, a restored + // symlink pointing out of the sandbox resolves to the live copy on the + // host, so the check verifies the running system and reports the + // backup healthy — the exact failure this tool exists to catch. + // + // Both sides are resolved before comparing because the sandbox itself + // routinely sits under a symlinked prefix: /tmp is /private/tmp on + // macOS, so comparing a resolved path against an unresolved root would + // reject every legitimate path there. + realRoot, rootErr := filepath.EvalSymlinks(root.Dir) + realAbs, absErr := filepath.EvalSymlinks(abs) + if rootErr != nil || absErr != nil { + continue + } + if !withinRoot(realRoot, realAbs) { + escaped = true + continue + } + return abs, nil + } + if escaped { + return "", errPathEscapes } - return "", false + return "", errPathMissing } // withinRoot reports whether abs is inside dir (after cleaning), blocking diff --git a/agent/internal/recipes/mysql.go b/agent/internal/recipes/mysql.go index b7dc06b..1d7fd85 100644 --- a/agent/internal/recipes/mysql.go +++ b/agent/internal/recipes/mysql.go @@ -47,9 +47,9 @@ func (c *MySQLCheck) validate() error { // Run implements Check. func (c *MySQLCheck) Run(ctx context.Context, t *Target) (report.Status, string) { - dumpPath, ok := resolve(t.Roots, strings.TrimSuffix(c.Dump, "/")) - if !ok { - return report.StatusFail, fmt.Sprintf("dump file %q not found in restored snapshot", c.Dump) + dumpPath, err := resolve(t.Roots, strings.TrimSuffix(c.Dump, "/")) + if err != nil { + return report.StatusFail, fmt.Sprintf("dump file %q %v", c.Dump, err) } runner, err := t.Docker(ctx) if err != nil { diff --git a/agent/internal/recipes/postgres.go b/agent/internal/recipes/postgres.go index 6c42e3f..ba97eb8 100644 --- a/agent/internal/recipes/postgres.go +++ b/agent/internal/recipes/postgres.go @@ -84,9 +84,9 @@ func (c *PostgresCheck) validate() error { // Run implements Check. func (c *PostgresCheck) Run(ctx context.Context, t *Target) (report.Status, string) { - dumpPath, ok := resolve(t.Roots, strings.TrimSuffix(c.Dump, "/")) - if !ok { - return report.StatusFail, fmt.Sprintf("dump file %q not found in restored snapshot", c.Dump) + dumpPath, err := resolve(t.Roots, strings.TrimSuffix(c.Dump, "/")) + if err != nil { + return report.StatusFail, fmt.Sprintf("dump file %q %v", c.Dump, err) } runner, err := t.Docker(ctx) if err != nil { diff --git a/agent/internal/recipes/security_test.go b/agent/internal/recipes/security_test.go index 69139df..fbf7df3 100644 --- a/agent/internal/recipes/security_test.go +++ b/agent/internal/recipes/security_test.go @@ -36,7 +36,7 @@ func TestPathTraversalRejectedEverywhere(t *testing.T) { // slips past validation (defense in depth). func TestResolveContainment(t *testing.T) { roots := []Root{{Dir: t.TempDir(), SnapPath: "/srv"}} - if _, ok := resolve(roots, "../../etc/passwd"); ok { + if _, err := resolve(roots, "../../etc/passwd"); err == nil { t.Error("resolve returned a path escaping the root") } } diff --git a/agent/internal/recipes/sqlite.go b/agent/internal/recipes/sqlite.go index 5b69e87..443d07d 100644 --- a/agent/internal/recipes/sqlite.go +++ b/agent/internal/recipes/sqlite.go @@ -36,9 +36,9 @@ func (c *SQLiteCheck) validate() error { // database, integrity errors — is a verification failure: the backup does // not contain a working database. func (c *SQLiteCheck) Run(ctx context.Context, t *Target) (report.Status, string) { - abs, ok := resolve(t.Roots, strings.TrimSuffix(c.Path, "/")) - if !ok { - return report.StatusFail, fmt.Sprintf("database %q not found in restored snapshot", c.Path) + abs, err := resolve(t.Roots, strings.TrimSuffix(c.Path, "/")) + if err != nil { + return report.StatusFail, fmt.Sprintf("database %q %v", c.Path, err) } // immutable=1 guarantees the check never writes (no WAL, no journal diff --git a/agent/internal/recipes/symlink_escape_test.go b/agent/internal/recipes/symlink_escape_test.go new file mode 100644 index 0000000..8bc1a39 --- /dev/null +++ b/agent/internal/recipes/symlink_escape_test.go @@ -0,0 +1,89 @@ +package recipes + +import ( + "errors" + "os" + "path/filepath" + "testing" +) + +// restic restores symlinks as symlinks, so a backup of /srv/app where `data` +// is a link to /var/lib/app restores a link pointing at the live host copy. +// os.Stat follows it, so before this was fixed the check read the running +// system and reported the backup healthy — a silent false PASS on exactly the +// question this tool exists to answer. +// +// Note t.TempDir() sits under /var/folders on macOS, and /var is itself a +// symlink to /private/var, so these cases also cover the sandbox living under +// a symlinked prefix. A containment check that resolved only one side would +// reject every legitimate path here. +func TestResolveSymlinkContainment(t *testing.T) { + sandbox := t.TempDir() + outside := t.TempDir() + + if err := os.WriteFile(filepath.Join(outside, "live.db"), []byte("LIVE HOST DATA"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(sandbox, "real.db"), []byte("restored"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(filepath.Join(sandbox, "sub"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(sandbox, "sub", "inner.db"), []byte("restored"), 0o600); err != nil { + t.Fatal(err) + } + // The dangerous one: points out of the restored tree entirely. + if err := os.Symlink(filepath.Join(outside, "live.db"), filepath.Join(sandbox, "escape.db")); err != nil { + t.Fatal(err) + } + // The legitimate one: a relative link the backup itself contained. + if err := os.Symlink("sub/inner.db", filepath.Join(sandbox, "inside.db")); err != nil { + t.Fatal(err) + } + + roots := []Root{{Dir: sandbox, SnapPath: "/srv/app"}} + + tests := []struct { + name string + rel string + wantErr error + }{ + {"plain restored file resolves", "real.db", nil}, + {"symlink within the snapshot resolves", "inside.db", nil}, + {"symlink out of the snapshot is refused", "escape.db", errPathEscapes}, + {"absent path is still just missing", "nope.db", errPathMissing}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := resolve(roots, tt.rel) + if !errors.Is(err, tt.wantErr) { + t.Fatalf("resolve(%q) err = %v, want %v", tt.rel, err, tt.wantErr) + } + if tt.wantErr == nil && got == "" { + t.Fatalf("resolve(%q) returned no path", tt.rel) + } + }) + } +} + +// The failure mode that matters is not the error value, it is that a check +// must never read the host copy. This asserts the data itself never surfaces. +func TestResolveNeverReachesHostData(t *testing.T) { + sandbox := t.TempDir() + outside := t.TempDir() + secret := filepath.Join(outside, "live.db") + if err := os.WriteFile(secret, []byte("LIVE HOST DATA"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(secret, filepath.Join(sandbox, "data.db")); err != nil { + t.Fatal(err) + } + + got, err := resolve([]Root{{Dir: sandbox, SnapPath: "/srv/app"}}, "data.db") + if err == nil { + content, _ := os.ReadFile(got) + t.Fatalf("resolved a path outside the sandbox (%s) reading %q", got, content) + } +} From 35eb2377b210125eef2922b1f4494f5d679c6973 Mon Sep 17 00:00:00 2001 From: thetechnologist1911 Date: Thu, 17 Sep 2026 18:48:30 -0400 Subject: [PATCH 4/4] fix(agent): bound repo_label so long repo paths still reach the dashboard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The agent sent the full repo string while the control plane caps repo_label at 200. Past that the whole submission was rejected, so the repo row was never created, the run never appeared on the dashboard, and stale detection could never fire for that repo — it needs an existing row. Meanwhile the run printed PASS and exited 0, and the only backstop was a 24h agent-silent alert blaming the wrong thing. Long B2 and S3 URLs and deep local paths cross 200 characters easily, and the failure is silent in the direction that matters: the user believes a repo is covered when the control plane has never heard of it. Truncate in the middle, keeping the scheme and host at the head and the path that distinguishes two repos on the same host at the tail. The label is display only — repos are identified by fingerprint — so shortening costs nothing. Cuts land on rune boundaries, and bounding bytes is sufficient because the server counts UTF-16 units, which is never more than the UTF-8 byte count. --- agent/internal/report/client.go | 36 ++++++++++++++++- agent/internal/report/repolabel_test.go | 54 +++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 agent/internal/report/repolabel_test.go diff --git a/agent/internal/report/client.go b/agent/internal/report/client.go index b9b3379..9651dfc 100644 --- a/agent/internal/report/client.go +++ b/agent/internal/report/client.go @@ -14,6 +14,7 @@ import ( "path/filepath" "strings" "time" + "unicode/utf8" ) // ErrNoCredentials means the agent is not registered with a control plane — @@ -120,6 +121,39 @@ type runPayload struct { Checks []CheckResult `json:"checks"` } +// maxRepoLabelBytes mirrors the control plane's bound on repo_label. Keeping +// the agent inside it matters more than it looks: the server rejected the +// whole submission when a label ran long, so the repo row was never created, +// the run never appeared on the dashboard, and stale detection could never +// fire for that repo — while the agent still printed PASS and exited 0. Long +// B2 and S3 URLs reach 200 characters easily. +// +// Bounding bytes is enough: the server counts UTF-16 units, which is never +// more than the UTF-8 byte count. +const maxRepoLabelBytes = 200 + +// truncateRepoLabel keeps both ends of an over-long repo string. The head +// carries the scheme and host, the tail carries the path that tells two repos +// on the same host apart, so cutting either end alone loses the half that +// makes the label worth showing. The label is display only; repos are +// identified by fingerprint, so shortening it costs nothing. +func truncateRepoLabel(s string) string { + if len(s) <= maxRepoLabelBytes { + return s + } + const ellipsis = "…" + budget := maxRepoLabelBytes - len(ellipsis) + head := budget / 2 + tailStart := len(s) - (budget - head) + for head > 0 && !utf8.RuneStart(s[head]) { + head-- + } + for tailStart < len(s) && !utf8.RuneStart(s[tailStart]) { + tailStart++ + } + return s[:head] + ellipsis + s[tailStart:] +} + // SubmitRun reports one run result. Only pass/fail metadata leaves the // machine: the repo is reduced to a fingerprint plus its scrubbed label, and // every string in the result was scrubbed when the result was built. @@ -139,7 +173,7 @@ func (c *Client) SubmitRun(ctx context.Context, r *RunResult) error { } payload := runPayload{ RepoFingerprint: fingerprint, - RepoLabel: r.Repo, // already scrubbed at result construction + RepoLabel: truncateRepoLabel(r.Repo), // scrubbed at result construction SnapshotID: r.SnapshotID, Status: r.Status, // Same treatment as check messages. A failed restore enumerates paths diff --git a/agent/internal/report/repolabel_test.go b/agent/internal/report/repolabel_test.go new file mode 100644 index 0000000..78678aa --- /dev/null +++ b/agent/internal/report/repolabel_test.go @@ -0,0 +1,54 @@ +package report + +import ( + "strings" + "testing" + "unicode/utf8" +) + +// A repo string longer than the control plane's bound used to fail the whole +// submission, so the repo row was never created, nothing appeared on the +// dashboard, and stale detection could never fire for it — while the run +// printed PASS and exited 0. Silence was the entire bug, so what matters here +// is that nothing the agent sends can exceed the bound. +func TestTruncateRepoLabel(t *testing.T) { + tests := []struct { + name string + in string + }{ + {"short path is untouched", "/srv/backups/restic"}, + {"exactly at the bound", strings.Repeat("a", maxRepoLabelBytes)}, + {"one past the bound", strings.Repeat("a", maxRepoLabelBytes+1)}, + {"long b2 url", "b2:my-bucket-with-a-long-name:" + strings.Repeat("nested/", 60) + "repo"}, + {"long s3 url", "s3:https://s3.eu-central-1.amazonaws.com/" + strings.Repeat("deep/", 70) + "restic"}, + {"multibyte runes at the cut points", strings.Repeat("ünïcödé-påth/", 40)}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := truncateRepoLabel(tt.in) + if len(got) > maxRepoLabelBytes { + t.Errorf("result is %d bytes, over the %d bound", len(got), maxRepoLabelBytes) + } + if !utf8.ValidString(got) { + t.Errorf("truncation split a rune: %q", got) + } + if len(tt.in) <= maxRepoLabelBytes && got != tt.in { + t.Errorf("modified a label that already fit:\n in: %q\n got: %q", tt.in, got) + } + }) + } +} + +// Truncating from either end alone would drop the half that makes the label +// readable, so both survive. +func TestTruncateRepoLabelKeepsBothEnds(t *testing.T) { + in := "s3:https://s3.amazonaws.com/" + strings.Repeat("x/", 200) + "prod-repo" + got := truncateRepoLabel(in) + if !strings.HasPrefix(got, "s3:https://s3.amazonaws.com/") { + t.Errorf("lost the scheme and host: %q", got) + } + if !strings.HasSuffix(got, "prod-repo") { + t.Errorf("lost the distinguishing tail: %q", got) + } +}