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) + } +} 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..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. @@ -138,11 +172,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: truncateRepoLabel(r.Repo), // 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/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) + } +} 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] + "…" 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;