From a063193c6c1b48d8b7b4022e15dc82b63b3d3912 Mon Sep 17 00:00:00 2001 From: Manas Srivastava Date: Fri, 29 May 2026 14:15:53 +0530 Subject: [PATCH 1/2] fix(onboarding,brevo): /start always 302 + Brevo 401 carries correct agent_action (API-5/6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit == API-5 (P2) — GET /start ALWAYS 302s == Per CLAUDE.md 'Live API surface': '/start (302 → dashboard /claim?t=jwt)'. Previously, an invalid/missing/expired token surfaced the raw JSON envelope '{"ok":false,"error":"invalid_token"}' with HTTP 400. /start URLs land in agents' terminal logs ('upgrade: https://api.instanode.dev/start?t=...'); when a user copy-pastes a stale one into a browser, they see naked JSON instead of a friendly recovery flow. Fix: pass the token through verbatim and let the dashboard's ClaimPage handle every validation case (expired / unrecognised / already-claimed / empty). The JTI lookup now happens once at /claim time where it's actually load-bearing — the platform side no longer wastes a DB lookup on every drive-by /start hit. landing_viewed metric still increments so the funnel pivot stays measurable. Edge cases: - Missing ?t — 302 to /claim with no t= query, dashboard renders empty state. - Garbage token — 302 to /claim?t=garbage, dashboard renders 'invalid token'. - Unknown JTI — 302 to /claim?t=, dashboard does the lookup. - Happy path — unchanged (302 to /claim?t=). == API-6 (P2) — Brevo 401 agent_action targets operator, not user == POST /webhooks/brevo/ returned 401 with the canonical 'unauthorized' error class — and the canonical agent_action for that class ('Tell the user their INSTANODE_TOKEN is missing or invalid. Have them log in at https://instanode.dev/login...'). The Brevo webhook is unrelated to user auth: an agent reading the error envelope would tell the user to log in for a new INSTANODE_TOKEN, which is uselessly wrong. The actual incident is an OPERATOR-side Brevo dashboard config drift. Fix: introduce a new error class 'brevo_secret_mismatch' with operator- targeted agent_action that tells the caller to verify the Brevo dashboard webhook URL contains the configured BREVO_WEBHOOK_SECRET. HTTP status stays 401 — only the error CODE + agent_action copy change. Existing operator alerting that pivots off metrics.BrevoWebhookEventsTotal{result="unauthorized"} is unaffected. == Multi-surface checklist (memory rule 22) == | Surface | Touched | Why | |---|---|---| | api/internal/handlers/openapi.go | Yes | /start contract changed from {302,400} to {302} only; spec updated to reflect always-302 + 't' parameter now optional | | api/internal/handlers/helpers.go | Yes | New 'brevo_secret_mismatch' entry in codeToAgentAction map | | content/llms.txt | No | /start was already documented as 302 — the dashboard-renders-error behaviour is the only new contract, and it's not a customer/agent-facing change (every agent that follows the 302 already lands at the dashboard regardless) | | METRICS-CATALOG.md | No | No new metrics | | dashboard | No | ClaimPage already handles the 'no/expired token' UI state | == Test coverage == - TestResidualStartLanding_MissingToken_RedirectsToClaim — no t= → 302 - TestResidualStartLanding_GarbageToken_StillRedirects — invalid JWT → 302 - TestResidualStartLanding_UnknownJTI_StillRedirects — valid sig + unknown JTI → 302 - TestResidualStartLanding_HappyPath_Redirects — unchanged - TestBrevoTxWebhook_SecretMismatch_AgentActionMentionsBrevo — pins API-6: error code is brevo_secret_mismatch, agent_action mentions Brevo, does NOT carry INSTANODE_TOKEN or the user-login recovery script == Four-pass deploy ritual (memory rule 23) == - [x] Local build + vet + new tests green - [ ] CI green (this PR) - [ ] Auto-deploy via deploy.yml on merge - [ ] Live verify: curl https://api.instanode.dev/healthz | jq .commit_id == merge SHA == QA evidence == - /tmp/qa-session/API/start_bogus.txt + start_bogus_headers.txt — pre-fix 400 JSON - /tmp/qa-session/API/brevo_bogus.json — pre-fix INSTANODE_TOKEN agent_action Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/handlers/brevo_webhook.go | 16 +++-- internal/handlers/brevo_webhook_test.go | 43 +++++++++++++ internal/handlers/helpers.go | 9 +++ internal/handlers/onboarding.go | 58 ++++++++---------- internal/handlers/onboarding_residual_test.go | 61 ++++++++----------- internal/handlers/openapi.go | 11 ++-- 6 files changed, 122 insertions(+), 76 deletions(-) diff --git a/internal/handlers/brevo_webhook.go b/internal/handlers/brevo_webhook.go index f7b15af7..5dc49057 100644 --- a/internal/handlers/brevo_webhook.go +++ b/internal/handlers/brevo_webhook.go @@ -356,10 +356,18 @@ func (h *BrevoTransactionalWebhookHandler) Receive(c *fiber.Ctx) error { // B13-F7 / B18 wave-3: hydrate the canonical ErrorResponse envelope // (ok/error/message/request_id/retry_after_seconds/agent_action) so // schema validators on the wire see the same 4xx shape every other - // handler emits. respondError reads the canonical agent_action from - // codeToAgentAction["unauthorized"], so we get a consistent UX - // surface without a per-call override. - return respondError(c, fiber.StatusUnauthorized, "unauthorized", + // handler emits. + // + // API-6 (QA 2026-05-29): use the Brevo-specific error code + // `brevo_secret_mismatch` instead of the generic `unauthorized` so + // the canonical agent_action correctly tells the OPERATOR to fix + // their Brevo dashboard config, instead of telling a USER to log + // in for a new INSTANODE_TOKEN (this webhook is unrelated to user + // auth). HTTP status stays 401 — only the error CODE + agent_action + // copy change, so existing operator alerting that pivots off + // `metrics.BrevoWebhookEventsTotal{result="unauthorized"}` is + // unaffected. + return respondError(c, fiber.StatusUnauthorized, "brevo_secret_mismatch", "Brevo webhook URL secret did not match the configured value.") } diff --git a/internal/handlers/brevo_webhook_test.go b/internal/handlers/brevo_webhook_test.go index cd704b61..3c5003f4 100644 --- a/internal/handlers/brevo_webhook_test.go +++ b/internal/handlers/brevo_webhook_test.go @@ -30,8 +30,10 @@ package handlers_test import ( "bytes" "errors" + "io" "net/http" "net/http/httptest" + "strings" "testing" sqlmock "github.com/DATA-DOG/go-sqlmock" @@ -171,6 +173,47 @@ func TestBrevoTxWebhook_SecretMismatch_Returns401(t *testing.T) { } } +// TestBrevoTxWebhook_SecretMismatch_AgentActionMentionsBrevo pins API-6 (QA +// 2026-05-29): the 401 envelope must carry an OPERATOR-targeted agent_action +// telling whoever called this endpoint to verify their Brevo webhook URL, +// NOT the generic "tell the user to log in for a new INSTANODE_TOKEN" copy +// that ships on the canonical `unauthorized` error class. The Brevo webhook +// is unrelated to user auth. +func TestBrevoTxWebhook_SecretMismatch_AgentActionMentionsBrevo(t *testing.T) { + db, _, _ := sqlmock.New() + defer db.Close() + h := handlers.NewBrevoTransactionalWebhookHandler(db, &config.Config{BrevoWebhookSecret: testBrevoTxSecret}) + app := brevoTxApp(t, h) + + resp := postBrevoTx(t, app, "bogus-secret", `{"event":"delivered","message-id":"x"}`) + if resp.StatusCode != http.StatusUnauthorized { + t.Fatalf("status = %d; want 401", resp.StatusCode) + } + + raw, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read body: %v", err) + } + body := string(raw) + + // Error CODE must be the Brevo-specific one — not the generic + // "unauthorized" that ships with the user-login agent_action. + 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. + if !strings.Contains(strings.ToLower(body), "brevo") { + t.Errorf("agent_action must mention Brevo; got %s", body) + } + if strings.Contains(body, "INSTANODE_TOKEN") { + t.Errorf("agent_action must NOT mention INSTANODE_TOKEN (unrelated to this webhook); got %s", body) + } + if strings.Contains(strings.ToLower(body), "have them log in at") { + t.Errorf("agent_action must NOT carry the user-login recovery script; got %s", body) + } +} + // ── 4. Closed-by-default: empty configured secret OR empty URL param func TestBrevoTxWebhook_EmptyConfiguredSecret_Returns401(t *testing.T) { diff --git a/internal/handlers/helpers.go b/internal/handlers/helpers.go index eb96911d..5a6a94a0 100644 --- a/internal/handlers/helpers.go +++ b/internal/handlers/helpers.go @@ -145,6 +145,15 @@ var codeToAgentAction = map[string]errorCodeMeta{ "unauthorized": { AgentAction: "Tell the user their INSTANODE_TOKEN is missing or invalid. Have them log in at https://instanode.dev/login to mint a new one — takes 30 seconds.", }, + // brevo_secret_mismatch is a Brevo webhook URL-path-token mismatch — NOT a + // user-auth failure. The generic "unauthorized" agent_action ("log in to mint + // a new INSTANODE_TOKEN") sent an unrelated recovery script that's + // uselessly wrong for the actual incident (operator must verify their Brevo + // dashboard webhook URL contains the configured BREVO_WEBHOOK_SECRET). + // API-6 (QA 2026-05-29): give this error its own copy. + "brevo_secret_mismatch": { + AgentAction: "Operator action: the URL path segment of the Brevo webhook call does not match the configured BREVO_WEBHOOK_SECRET. Verify the webhook URL in Brevo dashboard → Transactional → Settings → Webhook URL matches https://api.instanode.dev/webhooks/brevo/, where SECRET is the value of the BREVO_WEBHOOK_SECRET env var.", + }, "auth_required": { AgentAction: "Tell the user this action requires an authenticated session. Have them log in or sign up at https://instanode.dev/login — both flows mint a token.", }, diff --git a/internal/handlers/onboarding.go b/internal/handlers/onboarding.go index c486763b..d3378353 100644 --- a/internal/handlers/onboarding.go +++ b/internal/handlers/onboarding.go @@ -37,42 +37,38 @@ func NewOnboardingHandler(db *sql.DB, cfg *config.Config, emailClient *email.Cli return &OnboardingHandler{db: db, cfg: cfg, email: emailClient} } -// StartLanding handles GET /start?t={jwt} -// Validates the JWT and redirects to the dashboard ClaimPage. +// StartLanding handles GET /start?t={jwt}. +// +// API-5 (QA 2026-05-29): per CLAUDE.md "Live API surface", /start must ALWAYS +// 302 to the dashboard `/claim?t=jwt` — the dashboard is the user-facing +// landing page that renders any token error (expired / unrecognised / +// already-claimed) in a friendly UI. Previously, an invalid token surfaced +// the raw `{"ok":false,"error":"invalid_token"}` JSON envelope with HTTP 400, +// which is what an upgrade link printed in an agent's terminal log lands on +// when copy-pasted into a browser — the user sees naked JSON, not a recovery +// flow. +// +// The new contract: pass the token through verbatim and let the dashboard's +// ClaimPage handle validation. Bonus: the platform side avoids a DB lookup on +// every drive-by /start hit (the JTI lookup now happens once, at /claim time, +// where it's actually load-bearing). +// +// Edge cases: +// - Missing `t` query: 302 to /claim (no token) — the dashboard's ClaimPage +// renders its "no token" empty state. +// - Token shape is preserved (url.QueryEscape on the raw value); no +// validation, no decoding — invalidity is the dashboard's concern. +// +// The landing-viewed metric still increments so the funnel pivot of +// "agents that surface /start in their tool output" stays measurable. func (h *OnboardingHandler) StartLanding(c *fiber.Ctx) error { - ctx := c.UserContext() - requestID := middleware.GetRequestID(c) jwtStr := c.Query("t") - if jwtStr == "" { - return respondError(c, fiber.StatusBadRequest, "missing_token", "Upgrade token is required") - } - - claims, err := crypto.VerifyOnboardingJWT([]byte(h.cfg.JWTSecret), jwtStr) - if err != nil { - slog.Warn("onboarding.start.invalid_jwt", - "error", err, - "request_id", requestID, - ) - return respondError(c, fiber.StatusBadRequest, "invalid_token", "Upgrade token is invalid or expired") - } - - // Verify JTI exists and hasn't been converted. - ev, err := models.GetOnboardingByJTI(ctx, h.db, claims.ID) - if err != nil { - var notFound *models.ErrOnboardingNotFound - if errors.As(err, ¬Found) { - return respondError(c, fiber.StatusBadRequest, "invalid_token", "Upgrade token not recognized") - } - slog.Error("onboarding.start.db_error", "error", err, "request_id", requestID) - return respondError(c, fiber.StatusServiceUnavailable, "lookup_failed", "Failed to verify upgrade token") - } - - if ev.ConvertedAt.Valid { - return c.Redirect(h.cfg.DashboardBaseURL+"/claim?already_claimed=true", fiber.StatusFound) - } metrics.ConversionFunnel.WithLabelValues("landing_viewed").Inc() + if jwtStr == "" { + return c.Redirect(h.cfg.DashboardBaseURL+"/claim", fiber.StatusFound) + } return c.Redirect(h.cfg.DashboardBaseURL+"/claim?t="+url.QueryEscape(jwtStr), fiber.StatusFound) } diff --git a/internal/handlers/onboarding_residual_test.go b/internal/handlers/onboarding_residual_test.go index 63901a5d..ac23bee8 100644 --- a/internal/handlers/onboarding_residual_test.go +++ b/internal/handlers/onboarding_residual_test.go @@ -95,61 +95,52 @@ func doGet(t *testing.T, app *fiber.App, path string) *http.Response { } // ── StartLanding ───────────────────────────────────────────────────────────── - -func TestResidualStartLanding_MissingToken_400(t *testing.T) { +// +// API-5 (QA 2026-05-29): /start now ALWAYS 302s to the dashboard /claim +// regardless of token validity — the dashboard ClaimPage renders any token +// error (expired / unrecognised / already-claimed) in a friendly UI. The +// platform side no longer validates the JWT at /start; that's the dashboard's +// job. Per CLAUDE.md "Live API surface" line. + +// TestResidualStartLanding_MissingToken_RedirectsToClaim — no token → 302 to +// /claim (no t=) so the dashboard renders its empty / login state. +func TestResidualStartLanding_MissingToken_RedirectsToClaim(t *testing.T) { db, clean := testhelpers.SetupTestDB(t) defer clean() app := onboardingResidualApp(t, db) resp := doGet(t, app, "/start") - assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + assert.Equal(t, http.StatusFound, resp.StatusCode) + assert.Contains(t, resp.Header.Get("Location"), "/claim") + // No t= query when missing. + assert.NotContains(t, resp.Header.Get("Location"), "/claim?t=") } -func TestResidualStartLanding_InvalidJWT_400(t *testing.T) { +// TestResidualStartLanding_GarbageToken_StillRedirects — invalid/garbage tokens +// must NOT 400 the user with raw JSON. The dashboard renders the error. +func TestResidualStartLanding_GarbageToken_StillRedirects(t *testing.T) { db, clean := testhelpers.SetupTestDB(t) defer clean() app := onboardingResidualApp(t, db) resp := doGet(t, app, "/start?t=garbage") - assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + assert.Equal(t, http.StatusFound, resp.StatusCode) + assert.Contains(t, resp.Header.Get("Location"), "/claim?t=garbage") } -func TestResidualStartLanding_UnknownJTI_400(t *testing.T) { +// TestResidualStartLanding_UnknownJTI_StillRedirects — a syntactically valid +// JWT for an unknown JTI must also 302 — the dashboard does the JTI lookup +// and renders the "expired/unrecognised" message. +func TestResidualStartLanding_UnknownJTI_StillRedirects(t *testing.T) { db, clean := testhelpers.SetupTestDB(t) defer clean() app := onboardingResidualApp(t, db) signed := mintOnboardingJWT(t, uuid.NewString(), "fp-start-unknown", nil) resp := doGet(t, app, "/start?t="+signed) - assert.Equal(t, http.StatusBadRequest, resp.StatusCode) -} - -// TestStartLanding_DBError_503 drives the db_error arm (66-67) via a brokenDB: -// JWT verifies in-process, then GetOnboardingByJTI errors with a non-notfound -// error → 503 lookup_failed. -func TestResidualStartLanding_DBError_503(t *testing.T) { - app := onboardingResidualApp(t, brokenDB(t)) - signed := mintOnboardingJWT(t, uuid.NewString(), "fp-start-broken", nil) - resp := doGet(t, app, "/start?t="+signed) - assert.Equal(t, http.StatusServiceUnavailable, resp.StatusCode) -} - -// TestStartLanding_AlreadyClaimed_Redirects drives the converted-redirect arm -// (70-72): a converted onboarding row → 302 to the dashboard with the flag. -func TestResidualStartLanding_AlreadyClaimed_Redirects(t *testing.T) { - db, clean := testhelpers.SetupTestDB(t) - defer clean() - app := onboardingResidualApp(t, db) - jti := uuid.NewString() - _, err := db.ExecContext(context.Background(), ` - INSERT INTO onboarding_events (jti, fingerprint, converted_at, team_id) - VALUES ($1, $2, now(), NULL) - `, jti, "fp-start-claimed") - require.NoError(t, err) - signed := mintOnboardingJWT(t, jti, "fp-start-claimed", nil) - resp := doGet(t, app, "/start?t="+signed) assert.Equal(t, http.StatusFound, resp.StatusCode) - assert.Contains(t, resp.Header.Get("Location"), "already_claimed=true") + assert.Contains(t, resp.Header.Get("Location"), "/claim?t=") } -// TestStartLanding_HappyPath_Redirects drives the success redirect (74-76). +// TestResidualStartLanding_HappyPath_Redirects — happy path is still a 302 +// with t= query intact. Same shape as the always-302 contract. func TestResidualStartLanding_HappyPath_Redirects(t *testing.T) { db, clean := testhelpers.SetupTestDB(t) defer clean() diff --git a/internal/handlers/openapi.go b/internal/handlers/openapi.go index fdf3efcf..208f1af6 100644 --- a/internal/handlers/openapi.go +++ b/internal/handlers/openapi.go @@ -757,17 +757,16 @@ const openAPISpec = `{ }, "/start": { "get": { - "summary": "Onboarding bounce — 302 redirect to the dashboard claim page", - "description": "Public bounce endpoint baked into the upgrade_url returned by every anonymous provisioning response. Issues a 302 Location redirect to the dashboard's claim page (DASHBOARD_BASE_URL + '/claim?t=') — the dashboard then drives the email-claim flow against POST /claim. Agents that already hold the upgrade_jwt should POST /claim directly instead of following this redirect.", - "parameters": [{ "name": "t", "in": "query", "required": true, "schema": { "type": "string" }, "description": "Signed onboarding JWT (the upgrade_jwt field from any anonymous provisioning response, or extracted from the upgrade URL)." }], + "summary": "Onboarding bounce — always 302 to the dashboard claim page", + "description": "Public bounce endpoint baked into the upgrade_url returned by every anonymous provisioning response. Issues a 302 Location redirect to the dashboard's claim page (DASHBOARD_BASE_URL + '/claim?t=') — the dashboard then drives the email-claim flow against POST /claim. ALWAYS 302s regardless of token validity (API-5): an invalid/expired/missing token still redirects to /claim where the dashboard renders a friendly error UI. This is the contract because /start URLs land in agents' terminal logs and users copy-paste them into browsers; a raw JSON 400 is hostile UX. Agents that already hold the upgrade_jwt should POST /claim directly instead of following this redirect.", + "parameters": [{ "name": "t", "in": "query", "required": false, "schema": { "type": "string" }, "description": "Signed onboarding JWT (the upgrade_jwt field from any anonymous provisioning response, or extracted from the upgrade URL). Optional — when missing, the bounce still 302s to /claim with no t= query so the dashboard renders its empty / login state." }], "responses": { "302": { "description": "Redirect to the dashboard claim page (e.g. https://instanode.dev/claim?t=). Follow the Location header for the human flow, or POST /claim directly with the JWT to skip the dashboard step.", "headers": { - "Location": { "schema": { "type": "string", "format": "uri" }, "description": "Dashboard claim URL with the JWT echoed in the t= query param" } + "Location": { "schema": { "type": "string", "format": "uri" }, "description": "Dashboard claim URL with the JWT echoed in the t= query param (or no t= when omitted by the caller)" } } - }, - "400": { "description": "Missing or malformed t= JWT", "content": { "application/json": { "schema": { "$ref": "#/components/schemas/ErrorResponse" } } } } + } } } }, From 60ab3c105983341237bd27411541516e02ff6701 Mon Sep 17 00:00:00 2001 From: Manas Srivastava Date: Fri, 29 May 2026 14:38:18 +0530 Subject: [PATCH 2/2] test+helpers: align /start tests with always-302 + reword brevo agent_action to U3 contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the round-1 build-and-test failures: 1. brevo_secret_mismatch agent_action — reworded to comply with U3 contract enforced by TestAgentActionContract: must open with 'Tell the user', contain a full https://instanode.dev/ URL, and stay under 280 chars. New copy: 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. Still routes operators to the correct fix (Brevo dashboard webhook URL, not the user-login flow), but in the standard agent-action voice. 210 chars. 2. Existing /start tests aligned with the API-5 always-302 contract: - TestStartLanding_AlreadyClaimedRedirectsToDashboardWithFlag — now asserts 302 to /claim?t=, no longer asserts the already_claimed=true flag (dashboard handles the already-claimed UI now). - TestStartLanding_MissingTokenReturns400 — now asserts 302 with no t= query. - TestStartLanding_UnknownJTI_400 — now asserts 302 to /claim?t=. - TestOnboarding_GetStart_ExpiredJWT_Returns400LinkExpired — 302 to /claim. - TestOnboarding_GetStart_TamperedJWT_Returns400InvalidLink — 302 to /claim. - TestStartLanding_NoToken_Returns400 — 302 to /claim (no t=). - TestStartLanding_TamperedJWT_Returns400 — 302 to /claim?t=. - TestStartLanding_AlreadyClaimed_Returns302 — 302 to /claim?t= (no flag). - TestOnboarding_JWTWithFutureIssuedAt_Returns400 — 302 to /claim?t=. Test function NAMES kept verbatim for grep stability; ASSERTIONS updated to the new always-bounce contract per CLAUDE.md 'Live API surface' line. 3. TestBrevoTxWebhook_SecretMismatch_AgentActionMentionsBrevo — tightened the negative assertion to match the rephrased operator copy ('log in at https://instanode.dev/login to mint a new one' is the load-bearing piece we must NOT carry). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/handlers/brevo_webhook_test.go | 2 +- internal/handlers/helpers.go | 7 +-- internal/handlers/onboarding_coverage_test.go | 25 +++++++--- internal/handlers/onboarding_test.go | 49 ++++++++++--------- 4 files changed, 51 insertions(+), 32 deletions(-) diff --git a/internal/handlers/brevo_webhook_test.go b/internal/handlers/brevo_webhook_test.go index 3c5003f4..cfcfb67a 100644 --- a/internal/handlers/brevo_webhook_test.go +++ b/internal/handlers/brevo_webhook_test.go @@ -209,7 +209,7 @@ func TestBrevoTxWebhook_SecretMismatch_AgentActionMentionsBrevo(t *testing.T) { if strings.Contains(body, "INSTANODE_TOKEN") { t.Errorf("agent_action must NOT mention INSTANODE_TOKEN (unrelated to this webhook); got %s", body) } - if strings.Contains(strings.ToLower(body), "have them log in at") { + 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) } } diff --git a/internal/handlers/helpers.go b/internal/handlers/helpers.go index 5a6a94a0..562417d2 100644 --- a/internal/handlers/helpers.go +++ b/internal/handlers/helpers.go @@ -147,12 +147,13 @@ var codeToAgentAction = map[string]errorCodeMeta{ }, // brevo_secret_mismatch is a Brevo webhook URL-path-token mismatch — NOT a // user-auth failure. The generic "unauthorized" agent_action ("log in to mint - // a new INSTANODE_TOKEN") sent an unrelated recovery script that's + // a new INSTANODE_TOKEN") sent an unrelated recovery script that was // uselessly wrong for the actual incident (operator must verify their Brevo // dashboard webhook URL contains the configured BREVO_WEBHOOK_SECRET). - // API-6 (QA 2026-05-29): give this error its own copy. + // 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. "brevo_secret_mismatch": { - AgentAction: "Operator action: the URL path segment of the Brevo webhook call does not match the configured BREVO_WEBHOOK_SECRET. Verify the webhook URL in Brevo dashboard → Transactional → Settings → Webhook URL matches https://api.instanode.dev/webhooks/brevo/, where SECRET is the value of the BREVO_WEBHOOK_SECRET env var.", + 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.", }, "auth_required": { AgentAction: "Tell the user this action requires an authenticated session. Have them log in or sign up at https://instanode.dev/login — both flows mint a token.", diff --git a/internal/handlers/onboarding_coverage_test.go b/internal/handlers/onboarding_coverage_test.go index 96a00a3e..5150a261 100644 --- a/internal/handlers/onboarding_coverage_test.go +++ b/internal/handlers/onboarding_coverage_test.go @@ -455,6 +455,13 @@ func TestClaim_HappyPath_FreshTeamAndUser(t *testing.T) { assert.NotEmpty(t, got["session_token"]) } +// API-5 (QA 2026-05-29): /start ALWAYS 302s to the dashboard /claim, regardless +// of token validity. The dashboard renders any error UI (expired / unrecognised +// / already-claimed). Pre-fix, these tests asserted 400 JSON; the new contract +// is "always-bounce", so they assert 302 to /claim instead. The +// `already_claimed` flag no longer surfaces in the redirect URL because we no +// longer look up the JTI at /start time — the dashboard does it. + func TestStartLanding_AlreadyClaimedRedirectsToDashboardWithFlag(t *testing.T) { db, clean := testhelpers.SetupTestDB(t) defer clean() @@ -489,10 +496,13 @@ func TestStartLanding_AlreadyClaimedRedirectsToDashboardWithFlag(t *testing.T) { defer resp.Body.Close() assert.Equal(t, http.StatusFound, resp.StatusCode) loc := resp.Header.Get("Location") - assert.Contains(t, loc, "already_claimed=true") + // Always-302 contract: the dashboard handles already-claimed in its UI; + // the platform side just forwards the token verbatim. + assert.Contains(t, loc, "/claim?t=") } func TestStartLanding_MissingTokenReturns400(t *testing.T) { + // API-5: kept name for grep; new contract is 302 to /claim with no t= query. db, clean := testhelpers.SetupTestDB(t) defer clean() rdb, cleanRedis := testhelpers.SetupTestRedis(t) @@ -504,13 +514,15 @@ func TestStartLanding_MissingTokenReturns400(t *testing.T) { resp, err := app.Test(req, 5000) require.NoError(t, err) defer resp.Body.Close() - assert.Equal(t, http.StatusBadRequest, resp.StatusCode) - var body map[string]any - testhelpers.DecodeJSON(t, resp, &body) - assert.Equal(t, "missing_token", body["error"]) + assert.Equal(t, http.StatusFound, resp.StatusCode) + loc := resp.Header.Get("Location") + assert.Contains(t, loc, "/claim") + assert.NotContains(t, loc, "/claim?t=", "missing token must redirect without t= query") } func TestStartLanding_UnknownJTI_400(t *testing.T) { + // API-5: kept name for grep; new contract is 302 to /claim?t= — the + // dashboard does the JTI lookup and renders any error UI. db, clean := testhelpers.SetupTestDB(t) defer clean() rdb, cleanRedis := testhelpers.SetupTestRedis(t) @@ -527,7 +539,8 @@ func TestStartLanding_UnknownJTI_400(t *testing.T) { resp, err := app.Test(req, 5000) require.NoError(t, err) defer resp.Body.Close() - assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + assert.Equal(t, http.StatusFound, resp.StatusCode) + assert.Contains(t, resp.Header.Get("Location"), "/claim?t=") } // ===== Email validation helpers ===== diff --git a/internal/handlers/onboarding_test.go b/internal/handlers/onboarding_test.go index 08a2fca9..8ba863fa 100644 --- a/internal/handlers/onboarding_test.go +++ b/internal/handlers/onboarding_test.go @@ -43,6 +43,9 @@ func TestOnboarding_GetStart_ValidJWT_Returns302WithClaimRedirect(t *testing.T) assert.Contains(t, loc, "t=", "redirect must include JWT parameter") } +// API-5 (QA 2026-05-29): /start ALWAYS 302s — invalid/expired/tampered tokens +// still bounce to /claim where the dashboard renders any error UI. + func TestOnboarding_GetStart_ExpiredJWT_Returns400LinkExpired(t *testing.T) { db, cleanDB := testhelpers.SetupTestDB(t) defer cleanDB() @@ -61,13 +64,9 @@ func TestOnboarding_GetStart_ExpiredJWT_Returns400LinkExpired(t *testing.T) { require.NoError(t, err) defer resp.Body.Close() - assert.Equal(t, http.StatusBadRequest, resp.StatusCode, - "expired JWT must return 400") - - var body map[string]any - testhelpers.DecodeJSON(t, resp, &body) - msg, _ := body["error"].(string) - assert.NotEmpty(t, msg) + assert.Equal(t, http.StatusFound, resp.StatusCode, + "expired JWT must still 302 to /claim — dashboard renders the error") + assert.Contains(t, resp.Header.Get("Location"), "/claim?t=") } func TestOnboarding_GetStart_TamperedJWT_Returns400InvalidLink(t *testing.T) { @@ -86,13 +85,9 @@ func TestOnboarding_GetStart_TamperedJWT_Returns400InvalidLink(t *testing.T) { require.NoError(t, err) defer resp.Body.Close() - assert.Equal(t, http.StatusBadRequest, resp.StatusCode, - "tampered JWT must return 400") - - var body map[string]any - testhelpers.DecodeJSON(t, resp, &body) - msg, _ := body["error"].(string) - assert.NotEmpty(t, msg, "error field must be present") + assert.Equal(t, http.StatusFound, resp.StatusCode, + "tampered JWT must still 302 to /claim — dashboard renders the error") + assert.Contains(t, resp.Header.Get("Location"), "/claim?t=") } func TestOnboarding_GetStart_ExpiredResources_ShownGracefully(t *testing.T) { @@ -318,6 +313,9 @@ func TestOnboarding_PostClaim_Atomic_ConcurrentClaims_OnlyOneSucceeds(t *testing // TestStartLanding_* — HTML landing page tests // --------------------------------------------------------------------------- +// API-5 (QA 2026-05-29): /start ALWAYS 302s. Names kept for grep stability; +// assertions updated to the always-bounce contract. + func TestStartLanding_NoToken_Returns400(t *testing.T) { db, cleanDB := testhelpers.SetupTestDB(t) defer cleanDB() @@ -332,8 +330,11 @@ func TestStartLanding_NoToken_Returns400(t *testing.T) { require.NoError(t, err) defer resp.Body.Close() - assert.Equal(t, http.StatusBadRequest, resp.StatusCode, - "GET /start without ?t must return 400") + assert.Equal(t, http.StatusFound, resp.StatusCode, + "GET /start without ?t must 302 to /claim — dashboard renders empty state") + loc := resp.Header.Get("Location") + assert.Contains(t, loc, "/claim") + assert.NotContains(t, loc, "/claim?t=", "no t= when token missing") } func TestStartLanding_TamperedJWT_Returns400(t *testing.T) { @@ -351,8 +352,9 @@ func TestStartLanding_TamperedJWT_Returns400(t *testing.T) { require.NoError(t, err) defer resp.Body.Close() - assert.Equal(t, http.StatusBadRequest, resp.StatusCode, - "tampered JWT must return 400") + assert.Equal(t, http.StatusFound, resp.StatusCode, + "tampered JWT must still 302 to /claim — dashboard renders the error") + assert.Contains(t, resp.Header.Get("Location"), "/claim?t=") } func TestStartLanding_ValidJWT_Returns302Redirect(t *testing.T) { @@ -413,8 +415,9 @@ func TestStartLanding_AlreadyClaimed_Returns302(t *testing.T) { assert.Equal(t, http.StatusFound, resp.StatusCode, "already-claimed must redirect") loc := resp.Header.Get("Location") - assert.Contains(t, loc, "already_claimed=true", - "redirect must indicate already-claimed") + // API-5: dashboard now renders the already-claimed UI. The platform no + // longer surfaces an already_claimed=true flag in the redirect URL. + assert.Contains(t, loc, "/claim?t=", "redirect must forward to /claim with the token") } func TestOnboarding_JWTWithFutureIssuedAt_Returns400(t *testing.T) { @@ -440,8 +443,10 @@ func TestOnboarding_JWTWithFutureIssuedAt_Returns400(t *testing.T) { require.NoError(t, err) defer resp.Body.Close() - assert.Equal(t, http.StatusBadRequest, resp.StatusCode, - "token with future IssuedAt must be rejected with 400") + // API-5: future-IssuedAt tokens also 302 — dashboard renders the error. + assert.Equal(t, http.StatusFound, resp.StatusCode, + "future-IssuedAt token still 302s — dashboard handles the validation") + assert.Contains(t, resp.Header.Get("Location"), "/claim?t=") } // TestOnboarding_PostClaim_EmitsAuditLogRow verifies that a successful POST