diff --git a/internal/jobs/propagation_runner.go b/internal/jobs/propagation_runner.go index 552e7ee..3b6ff8a 100644 --- a/internal/jobs/propagation_runner.go +++ b/internal/jobs/propagation_runner.go @@ -71,6 +71,7 @@ import ( "go.opentelemetry.io/otel" commonv1 "instant.dev/proto/common/v1" + "instant.dev/worker/internal/logsafe" "instant.dev/worker/internal/metrics" ) @@ -1020,7 +1021,16 @@ func (w *PropagationRunnerWorker) insertPropagationAuditRow(ctx context.Context, // truncatePropagationError caps the persisted last_error at // propagationLastErrorMax bytes. Avoids unbounded growth from a chatty // gRPC error string. +// +// SEC-WORKER FINDING-6 (2026-05-29): the truncated string persists into +// pending_propagations.last_error (DB) and audit_log.metadata.last_error +// (JSON, surfaced to operators). A provisioner gRPC error can embed +// connection URIs with userinfo from inner driver errors. Scrub via +// logsafe.ScrubURL BEFORE truncation so the scrub does not chop a +// half-URI mid-userinfo (a half-stripped URI is worse than a fully +// stripped one). func truncatePropagationError(s string) string { + s = logsafe.ScrubURL(s) if len(s) <= propagationLastErrorMax { return s } diff --git a/internal/jobs/provisioner_reconciler.go b/internal/jobs/provisioner_reconciler.go index ef6d641..3ecacc2 100644 --- a/internal/jobs/provisioner_reconciler.go +++ b/internal/jobs/provisioner_reconciler.go @@ -382,11 +382,23 @@ func nullableTeamID(v sql.NullString) any { // probeErrString defends against nil-error inputs from ProbeUnreachable. // Per prober.go's contract, ProbeUnreachable comes with a non-nil err, but // belt-and-braces — a misbehaving prober shouldn't crash the sweep. +// +// SEC-WORKER FINDING-3 (2026-05-29): the returned string flows into three +// persistent surfaces: +// 1. resources.degraded_reason (DB column, surfaced in dashboard banner) +// 2. audit_log.metadata.error (JSON column, visible to admins) +// 3. slog.Error (shipped to New Relic Logs) +// +// Driver errors — especially mongo-driver and redis — can embed the full +// connection URI including userinfo (user:password@) in the error string. +// We run the output through logsafe.ScrubURL so any such embedded URI gets +// its userinfo stripped before persistence. Conservative: only strips +// `scheme://userinfo@` shapes; leaves everything else untouched. func probeErrString(err error) string { if err == nil { return "probe returned unreachable but no error message" } - return err.Error() + return logsafe.ScrubURL(err.Error()) } // truncateReason caps a probe error string at 500 chars so the audit_log diff --git a/internal/logsafe/logsafe.go b/internal/logsafe/logsafe.go index 973571c..9e3f8ee 100644 --- a/internal/logsafe/logsafe.go +++ b/internal/logsafe/logsafe.go @@ -1,5 +1,17 @@ // Package logsafe provides log-safe redactions for PII / credentials. // +// SEC-WORKER FINDING-3 + FINDING-6 (2026-05-29): driver / gRPC errors that +// propagate up to slog / audit_log / persisted error columns can embed the +// underlying connection URI verbatim. The MongoDB driver in particular +// surfaces `mongodb://user:secret@host/...` in `options.ApplyURI` parse +// errors; lib/pq's connection-refused errors include host/port but not +// usually password; redis.ParseURL can return URLs with embedded auth. +// `ScrubURL` strips the userinfo (`user:password@`) component from any +// `scheme://userinfo@host/...` substring it finds anywhere in the input. +// +// Conservative: matches only well-defined RFC 3986 syntax. Will never +// double-scrub. Idempotent. Safe to call in hot per-row loops. +// // T21 P1-2 (BugBash 2026-05-20): the worker logs resource bearer tokens // (inst_live_… / customer UUID tokens) raw at INFO/WARN/ERROR in ~20 // sites — `worker/internal/jobs/quota_infra.go` alone has 12. The @@ -20,6 +32,42 @@ // log dashboards / alerts can rely on a stable format. package logsafe +import "regexp" + +// urlUserinfoRE matches `scheme://userinfo@host` sequences and captures +// the scheme + host so the userinfo can be replaced with `***`. The scheme +// list is conservative: connection URIs we care about (postgres, redis, +// mongodb, amqp, http, https, s3, nats). Matching is case-insensitive on +// the scheme to absorb provider quirks (Postgres://, MongoDB+SRV://, ...). +// +// Why a regexp instead of net/url: +// 1. The input is typically an ERROR MESSAGE with the URI embedded, not a +// standalone URI — net/url.Parse on `error: failed to connect to +// mongodb://u:p@host` would fail. +// 2. We can apply over the whole string in one pass and catch every +// embedded URI even when the error wraps multiple. +// +// The regexp is compiled once at package init. +var urlUserinfoRE = regexp.MustCompile( + `(?i)([a-z][a-z0-9+.-]*://)([^/@\s]+@)`, +) + +// ScrubURL returns s with every `scheme://userinfo@host` sequence rewritten +// to `scheme://***@host`. Idempotent — applying twice is a no-op. Safe on +// strings with no embedded URI (returned unchanged). +// +// Examples: +// "mongo: failed mongodb://u:p@h/d" → "mongo: failed mongodb://***@h/d" +// "postgres://doadmin:abc@host:25060/db" → "postgres://***@host:25060/db" +// "redis: dial error redis://:pw@127/0" → "redis: dial error redis://***@127/0" +// "nothing to scrub here" → "nothing to scrub here" +func ScrubURL(s string) string { + if s == "" { + return s + } + return urlUserinfoRE.ReplaceAllString(s, "${1}***@") +} + // Token returns a log-safe rendering of a resource bearer token. // // "inst_live_aB3xY9..." → "inst_liv*** (len=42)" diff --git a/internal/logsafe/logsafe_test.go b/internal/logsafe/logsafe_test.go index 3579f82..8a0f404 100644 --- a/internal/logsafe/logsafe_test.go +++ b/internal/logsafe/logsafe_test.go @@ -79,3 +79,63 @@ func TestToken_NoLeakBeyondPrefix(t *testing.T) { t.Errorf("Token(%q) = %q — expected prefix `inst_liv`", token, out) } } + +// TestScrubURL pins the credential-redaction algorithm for the SEC-WORKER +// FINDING-3 / FINDING-6 fix: any `scheme://userinfo@host` embedded in an +// error message or persisted column gets its userinfo stripped to `***`. +// +// This is THE regression guard against secrets leaking into NR Logs, the +// dashboard degraded-banner reason, audit_log.metadata, and +// pending_propagations.last_error. +func TestScrubURL(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"empty", "", ""}, + {"no_url", "nothing to scrub", "nothing to scrub"}, + {"postgres_userpass", "postgres://doadmin:abc123@host:25060/db?sslmode=require", + "postgres://***@host:25060/db?sslmode=require"}, + {"mongodb_userpass_in_error", + "mongo: ping: error connecting mongodb://u:secret@cluster.svc/d", + "mongo: ping: error connecting mongodb://***@cluster.svc/d"}, + {"redis_password_only", "redis: dial error redis://:pw@127.0.0.1:6379/0", + "redis: dial error redis://***@127.0.0.1:6379/0"}, + {"mongodb_srv", "mongo: connect mongodb+srv://u:p@cluster.mongodb.net/?retryWrites=true", + "mongo: connect mongodb+srv://***@cluster.mongodb.net/?retryWrites=true"}, + {"https_url_without_userinfo", "GET https://api.razorpay.com/v1/subscriptions/sub_xx", + "GET https://api.razorpay.com/v1/subscriptions/sub_xx"}, + {"two_urls_in_one_error", + "failed: postgres://a:b@h1/d; retry: postgres://a:b@h2/d", + "failed: postgres://***@h1/d; retry: postgres://***@h2/d"}, + {"case_insensitive_scheme", "Postgres://Doadmin:P@host/db", + "Postgres://***@host/db"}, + {"already_scrubbed_idempotent", "postgres://***@host/db", + "postgres://***@host/db"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got := ScrubURL(c.in) + if got != c.want { + t.Errorf("ScrubURL(%q)\n got: %q\n want: %q", c.in, got, c.want) + } + }) + } +} + +// TestScrubURL_NoLeakOfSecretSuffix is the substantive regression guard: +// after scrubbing, the literal password substring must not appear anywhere +// in the output. Catches a regex regression that over-scopes the userinfo +// group and accidentally preserves the trailing password chars. +func TestScrubURL_NoLeakOfSecretSuffix(t *testing.T) { + const secret = "ZZZ_PASSWORD_MUST_NEVER_APPEAR_ZZZ" + in := "connection failed: postgres://admin:" + secret + "@host:5432/db" + out := ScrubURL(in) + if strings.Contains(out, secret) { + t.Errorf("ScrubURL leaked the password substring %q in output %q", secret, out) + } + if !strings.Contains(out, "postgres://***@host:5432/db") { + t.Errorf("ScrubURL output missing expected scrubbed shape: %q", out) + } +}