From 04c9fbbfb6d38cf5fede0967bc70c1e0557ba97d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 29 May 2026 23:27:39 +0530 Subject: [PATCH 1/2] sec(api): strip /healthz filename + drop env-var names from public 401s (API-21/90/217) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BUG-API-090 + BUG-API-217 — /healthz is anon-reachable and emits the raw migration filename, e.g. "063_forwarder_sent_audit_link.sql". That literally tells an attacker which feature shipped at which migration number — recon a public probe should not provide. Add migrations.State.PublicVersion() that strips to the numeric prefix ("063"). Canaries keep the commit_id+count+version drift tuple, attackers learn nothing about the schema. migration_count and migration_status unchanged. BUG-API-021 (plus siblings on webhook_secret_mismatch and webhook_signature_mismatch) — the public 401 envelope returned by /webhooks/brevo/ literally named the BREVO_WEBHOOK_SECRET env var in agent_action. Same recon-leak problem: anyone probing the public URL learned the exact env-var name. Drop env-var vocabulary from all three webhook-config error agent_actions; point at the docs page where the operator-side rotation procedure lives. Wire contract preserved: error codes, statuses, messages unchanged — only agent_action sentences are softened. No new endpoints, no new fields, no auth changes. Surface checklist (rule 22): - api/internal/migrations/state.go — PublicVersion helper + Strings import - api/internal/router/router.go — /healthz emits PublicVersion() - api/internal/router/healthz_test.go — pinned shape updated + new regression - api/internal/handlers/helpers.go — 3 agent_action sentences softened - api/internal/handlers/brevo_webhook_test.go — explicit assertion that BREVO_ env names never reach wire - OpenAPI / dashboard / marketing — no surface change (envelope shape unchanged) Coverage block: Symptom: /healthz.migration_version "063_forwarder_sent_audit_link.sql"; brevo 401 names BREVO_WEBHOOK_SECRET Enumeration: rg -F 'BREVO_WEBHOOK_SECRET' internal/handlers/helpers.go ; rg -F 'mstate.Filename' internal/router/ Sites found: 3 agent_action strings + 1 router emit site Sites touched: 4 / 4 Coverage test: TestHealthzMigrationVersionStripsFilenameSuffix (asserts no '_' or '.sql' in output across 7 cases); brevo_webhook_test BREVO_* + BREVO_WEBHOOK_SECRET assertion Live verified: pre-merge: curl https://api.instanode.dev/healthz returned "063_forwarder_sent_audit_link.sql"; curl https://api.instanode.dev/webhooks/brevo/x returned "BREVO_WEBHOOK_SECRET" in agent_action. post-merge: verify in PR comment. Local gate: - go build ./... PASS - go vet ./... PASS - go test ./internal/migrations/ PASS - go test ./internal/router/ PASS - go test -run 'Brevo' ./internal/handlers/ PASS Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/handlers/brevo_webhook_test.go | 15 +++++++- internal/handlers/helpers.go | 23 ++++++++++-- internal/migrations/state.go | 30 +++++++++++++++ internal/router/healthz_test.go | 49 +++++++++++++++++++++++-- internal/router/router.go | 10 ++++- 5 files changed, 117 insertions(+), 10 deletions(-) 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/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, }) From f6275991398c43646f838306fe58aafbc936f9bc Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 29 May 2026 23:40:33 +0530 Subject: [PATCH 2/2] test(migrations): unit-cover PublicVersion to satisfy 100% patch gate The package-external test in router_test exercises PublicVersion but diff-cover attributes coverage by package, so the migrations/state.go PublicVersion body landed at 0% for diff-cover. Add a package-internal test (state_public_version_test.go) iterating the same 7 cases so the patch-coverage gate sees migrations/state.go at 100%. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../migrations/state_public_version_test.go | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 internal/migrations/state_public_version_test.go 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)") + } +}