diff --git a/internal/handlers/brevo_webhook_test.go b/internal/handlers/brevo_webhook_test.go index cfcfb67a..2efa94e8 100644 --- a/internal/handlers/brevo_webhook_test.go +++ b/internal/handlers/brevo_webhook_test.go @@ -201,8 +201,8 @@ func TestBrevoTxWebhook_SecretMismatch_AgentActionMentionsBrevo(t *testing.T) { if !strings.Contains(body, `"error":"brevo_secret_mismatch"`) { t.Errorf("body must carry error=brevo_secret_mismatch; got %s", body) } - // Agent action must mention Brevo / BREVO_WEBHOOK_SECRET — NOT - // "INSTANODE_TOKEN" or the generic login-recovery script. + // Agent action must mention Brevo — NOT "INSTANODE_TOKEN" or the + // generic login-recovery script. if !strings.Contains(strings.ToLower(body), "brevo") { t.Errorf("agent_action must mention Brevo; got %s", body) } @@ -212,6 +212,17 @@ func TestBrevoTxWebhook_SecretMismatch_AgentActionMentionsBrevo(t *testing.T) { if strings.Contains(body, "log in at https://instanode.dev/login to mint a new one") { t.Errorf("agent_action must NOT carry the user-login recovery script; got %s", body) } + // BUG-API-021 (QA 2026-05-29): the 401 envelope is reachable by any + // brute-forcer probing /webhooks/brevo/. The pre-fix copy + // literally named the BREVO_WEBHOOK_SECRET env var — recon a public + // 401 must not provide. Assert the env-var name (and BREVO_ prefix + // generally) never reaches the wire. + if strings.Contains(body, "BREVO_WEBHOOK_SECRET") { + t.Errorf("BUG-API-021: agent_action must NOT name BREVO_WEBHOOK_SECRET env var on a public 401; got %s", body) + } + if strings.Contains(body, "BREVO_") { + t.Errorf("BUG-API-021: agent_action must NOT carry any BREVO_-prefixed env-var name; got %s", body) + } } // ── 4. Closed-by-default: empty configured secret OR empty URL param diff --git a/internal/handlers/helpers.go b/internal/handlers/helpers.go index 496d6e23..f62e30c1 100644 --- a/internal/handlers/helpers.go +++ b/internal/handlers/helpers.go @@ -152,8 +152,18 @@ var codeToAgentAction = map[string]errorCodeMeta{ // dashboard webhook URL contains the configured BREVO_WEBHOOK_SECRET). // API-6 (QA 2026-05-29): give this error its own copy. Follows the U3 // contract — "Tell the user" opening, https://instanode.dev/ URL, < 280 chars. + // BUG-API-021 (QA 2026-05-29): the pre-fix agent_action literally + // named the BREVO_WEBHOOK_SECRET env var, which (a) is informational + // disclosure to a brute-forcer probing the public 401 surface — they + // learn the exact env-var name the operator must rotate — and (b) + // targets the operator using internal vocab the calling agent has no + // business surfacing to the end-user. The new copy drops the env-var + // name and points at the public docs page (which documents the + // rotation procedure for an operator that follows the link). Wire + // contract preserved: error code, status, message all unchanged — + // only the agent_action sentence is softened. "brevo_secret_mismatch": { - AgentAction: "Tell the user this is a Brevo-webhook config mismatch, not their auth. Operators must verify the Brevo dashboard webhook URL matches the configured BREVO_WEBHOOK_SECRET — see https://instanode.dev/docs/email.", + AgentAction: "Tell the user this is a Brevo-webhook configuration mismatch, not their auth — they have no action. Operators: rotate the Brevo webhook secret and update the api Deployment — see https://instanode.dev/docs/email.", }, // webhook_secret_mismatch is the generic per-provider webhook URL-path-token // or shared-secret mismatch surface. API-19/96/97/98 (QA 2026-05-29): the @@ -164,8 +174,12 @@ var codeToAgentAction = map[string]errorCodeMeta{ // distinguishes the secret-not-configured branch from the signature-mismatch // branch below. Operator must wire the corresponding env var // (BREVO_WEBHOOK_SECRET / SES_SNS_SUBSCRIPTION_ARN) before the route accepts. + // BUG-API-021 sibling: "set the corresponding webhook secret env var" + // pointed at an internal env-var by name. Same softening as + // brevo_secret_mismatch above — drop env-var vocab from the public + // 401 surface; the docs page covers operator-side wiring. "webhook_secret_mismatch": { - AgentAction: "Tell the user this is an email-webhook secret-config mismatch, not their auth. Operators must set the corresponding webhook secret env var in the api Deployment — see https://instanode.dev/docs/email.", + AgentAction: "Tell the user this is an email-webhook configuration mismatch, not their auth — they have no action. Operators: configure the webhook secret in the api Deployment — see https://instanode.dev/docs/email.", }, // webhook_signature_mismatch is the per-provider signature-verification // failure surface — the secret IS configured, the inbound payload's HMAC / @@ -174,8 +188,11 @@ var codeToAgentAction = map[string]errorCodeMeta{ // the secret yet" from "someone is sending bad signatures (or the provider // rotated keys)" without an operator hand-grepping log lines. Used by // /api/v1/email/webhook/brevo + /api/v1/email/webhook/ses. + // BUG-API-021 sibling: "the api Deployment's env var" leaked the + // internal wiring; soften to "the configured webhook secret" so the + // 401 stays self-explanatory without naming env-var keys. "webhook_signature_mismatch": { - AgentAction: "Tell the user the inbound email-webhook signature did not verify. Operators must confirm the dashboard webhook secret matches the api Deployment's env var and that the provider hasn't rotated signing keys — see https://instanode.dev/docs/email.", + AgentAction: "Tell the user the inbound email-webhook signature did not verify. Operators: confirm the dashboard webhook secret matches the configured value and the provider hasn't rotated signing keys — see https://instanode.dev/docs/email.", }, // webhook_method_not_allowed surfaces the GET-on-a-POST-only webhook URL // path (BUG-API-098). Brevo's dashboard sometimes sends a GET pre-flight to diff --git a/internal/migrations/state.go b/internal/migrations/state.go index 9d34b52f..be8e343d 100644 --- a/internal/migrations/state.go +++ b/internal/migrations/state.go @@ -22,6 +22,7 @@ package migrations import ( "context" "database/sql" + "strings" "sync" "time" ) @@ -43,6 +44,35 @@ type State struct { Count int // total rows in schema_migrations; 0 when unknown } +// PublicVersion returns the migration-version string safe to surface on +// the public /healthz envelope (BUG-API-090/217). The raw Filename leaks +// the table/feature names embedded in the migration filename ("e.g. +// 063_forwarder_sent_audit_link.sql" tells an attacker that a +// forwarder_sent → audit_log FK landed at migration 63), which is +// recon a reachable-by-anyone health probe should not provide. We strip +// the filename to its numeric prefix only ("063"), which preserves the +// canary contract (canaries compare commit_id+migration_count+ +// migration_version to a known-good tuple) without leaking domain +// knowledge. +// +// Empty filename (DB unreachable / pre-migration) returns "" — same +// shape the pre-fix wire emitted in the unknown branch. +func (s State) PublicVersion() string { + if s.Filename == "" { + return "" + } + // Split on first "_" — migration filenames are `<3-digit>_.sql`. + // We tolerate a missing "_" (treat the whole stem as the version) and + // a non-numeric prefix (return as-is) so a future renaming scheme + // degrades to "leak the prefix only" rather than panicking. The + // .sql suffix is stripped if present. + stem := strings.TrimSuffix(s.Filename, ".sql") + if idx := strings.IndexByte(stem, '_'); idx >= 0 { + return stem[:idx] + } + return stem +} + // Reader caches one State per process with a TTL. Safe for concurrent use. // Clock is injectable so tests can advance time without sleeping. type Reader struct { diff --git a/internal/migrations/state_public_version_test.go b/internal/migrations/state_public_version_test.go new file mode 100644 index 00000000..11615204 --- /dev/null +++ b/internal/migrations/state_public_version_test.go @@ -0,0 +1,46 @@ +package migrations_test + +// state_public_version_test.go — BUG-API-090/217 regression. Lives in +// the migrations package (not router_test) so go test's coverage tool +// attributes the hits against migrations/state.go for the 100%-patch +// gate. + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "instant.dev/internal/migrations" +) + +func TestPublicVersion_StripsFilenameSuffix(t *testing.T) { + cases := []struct { + filename string + want string + }{ + // The case that motivated the bug: BUG-API-090/217 — anyone + // hitting /healthz saw the embedded table/feature name. + {"063_forwarder_sent_audit_link.sql", "063"}, + {"022_schema_migrations.sql", "022"}, + {"001_init.sql", "001"}, + {"100_team_deletion_purge.sql", "100"}, + // No underscore — return stem only (strips .sql). + {"baseline.sql", "baseline"}, + // No extension — return up to first underscore. + {"063_anything", "063"}, + // Empty (DB unreachable / pre-migration). + {"", ""}, + } + for _, tc := range cases { + s := migrations.State{Filename: tc.filename} + got := s.PublicVersion() + require.Equal(t, tc.want, got, + "BUG-API-090: PublicVersion(%q) must strip to numeric prefix; got %q", + tc.filename, got) + // Sanity rail: no '_' or '.sql' escapes the helper, regardless of input. + require.NotContains(t, got, "_", + "BUG-API-090: PublicVersion must never contain '_' (would leak table/feature name)") + require.NotContains(t, got, ".sql", + "BUG-API-090: PublicVersion must never contain '.sql' (would leak filename)") + } +} diff --git a/internal/router/healthz_test.go b/internal/router/healthz_test.go index 3f756f73..c8b5af8b 100644 --- a/internal/router/healthz_test.go +++ b/internal/router/healthz_test.go @@ -51,7 +51,7 @@ func TestHealthzShape(t *testing.T) { "commit_id": buildinfo.GitSHA, "build_time": buildinfo.BuildTime, "version": buildinfo.Version, - "migration_version": m.Filename, + "migration_version": m.PublicVersion(), "migration_count": m.Count, "migration_status": m.Status, }) @@ -82,8 +82,13 @@ func TestHealthzShape(t *testing.T) { require.Equal(t, buildinfo.Version, got["version"]) // Migration contract — new fields the canary reads to detect drift - // between binary commit and DB schema state. - require.Equal(t, "022_schema_migrations.sql", got["migration_version"]) + // between binary commit and DB schema state. BUG-API-090/217: the + // public envelope emits the numeric prefix only ("022"), never the + // full filename ("022_schema_migrations.sql") which would leak the + // embedded table/feature names. + require.Equal(t, "022", got["migration_version"]) + require.NotContains(t, got["migration_version"], "schema_migrations", + "BUG-API-090: migration_version must not leak filename suffix to anon /healthz callers") require.Equal(t, float64(22), got["migration_count"]) // JSON numbers decode as float64 require.Equal(t, "ok", got["migration_status"]) @@ -110,7 +115,7 @@ func TestHealthzMigrationStatusUnknownWhenDBDown(t *testing.T) { m := reader.Get(c.UserContext()) return c.JSON(fiber.Map{ "ok": true, - "migration_version": m.Filename, + "migration_version": m.PublicVersion(), "migration_count": m.Count, "migration_status": m.Status, }) @@ -129,6 +134,42 @@ func TestHealthzMigrationStatusUnknownWhenDBDown(t *testing.T) { require.Equal(t, float64(0), got["migration_count"]) } +// TestHealthzMigrationVersionStripsFilenameSuffix is the BUG-API-090/217 +// regression. The pre-fix /healthz handler emitted the raw filename +// ("063_forwarder_sent_audit_link.sql") which leaks the table+feature +// names embedded in the migration to any anonymous probe. The public +// envelope now emits the numeric prefix only — canaries still get a +// drift signal, attackers don't learn the schema. +func TestHealthzMigrationVersionStripsFilenameSuffix(t *testing.T) { + cases := []struct { + filename string + want string + }{ + {"063_forwarder_sent_audit_link.sql", "063"}, + {"022_schema_migrations.sql", "022"}, + {"001_init.sql", "001"}, + {"100_team_deletion_purge.sql", "100"}, + // No underscore — return stem only (still strips .sql). + {"baseline.sql", "baseline"}, + // No extension — return as-is up to first underscore. + {"063_anything", "063"}, + // Empty (DB unreachable / pre-migration) — empty out. + {"", ""}, + } + for _, tc := range cases { + s := migrations.State{Filename: tc.filename} + got := s.PublicVersion() + require.Equal(t, tc.want, got, + "BUG-API-090: PublicVersion(%q) must strip to numeric prefix; got %q", + tc.filename, got) + // Sanity rail: no underscore or .sql escapes the helper. + require.NotContains(t, got, "_", + "BUG-API-090: PublicVersion must never contain '_' (would leak table/feature name)") + require.NotContains(t, got, ".sql", + "BUG-API-090: PublicVersion must never contain '.sql' (would leak filename)") + } +} + // TestHealthzMigrationVersionMatchesEmbeddedFile is the sanity rail — // whatever filename the DB reports must exist in the binary's embedded // migration set. If the running pod returns "099_phantom.sql" but no diff --git a/internal/router/router.go b/internal/router/router.go index 70957cb2..cde72e6b 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -349,13 +349,21 @@ func NewWithHooks(cfg *config.Config, db *sql.DB, rdb *redis.Client, geoDbs *mid migrationReader := migrations.NewReader(db, 0, nil) app.Get("/healthz", func(c *fiber.Ctx) error { mstate := migrationReader.Get(c.UserContext()) + // BUG-API-090 / BUG-API-217 (QA 2026-05-29): /healthz is a + // public probe (no auth, no rate-limit by token). Emitting the + // raw migration filename leaked the table/feature names embedded + // in it ("063_forwarder_sent_audit_link.sql" tells an attacker a + // forwarder_sent→audit_log FK landed at migration 63). Strip to + // the numeric prefix only via State.PublicVersion so canaries + // keep their commit_id+count+version tuple but no domain + // knowledge leaks. migration_count + migration_status unchanged. return c.JSON(fiber.Map{ "ok": true, "service": "instant.dev", "commit_id": buildinfo.GitSHA, "build_time": buildinfo.BuildTime, "version": buildinfo.Version, - "migration_version": mstate.Filename, + "migration_version": mstate.PublicVersion(), "migration_count": mstate.Count, "migration_status": mstate.Status, })