diff --git a/CLAUDE.md b/CLAUDE.md index e7af1910..2e5025b9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -424,6 +424,7 @@ Working code-review checklist (adapted from [Lukaszuk's clean-code summary](http - **Core modules must not import from UI components.** Stores (`src/stores/`) and core (`src/core/`) are the shared backend; they must never import from `src/components/`. If a store needs a UI side effect, use an event, a shared utility in `src/utils/`/`src/core/`, or let the UI react to store state changes. - **Pull-before-mutate invariant**: Any flow that reads remote state and then modifies local state must fetch the remote data **before** any destructive local op (`deleteDatabase`, `tryDeleteServerVault`). The recovery flow calls `pullVault()` first, then `initFresh(skipServerCleanup: true)`. Otherwise you destroy the vault you're trying to recover. Workflows with destructive + read operations on shared remote state need integration tests; mocked unit tests can't catch temporal coupling across module boundaries. - **No-auto-destroy invariant**: No automated code path may delete server-side vault data. `destroy()` (`src/core/storage/key-manager.ts`) has exactly one sanctioned caller — `useAppStore.getState().resetApp`, which must be invoked from an explicit user-confirmation UI (the "Wipe and start over" `` on `InvalidKeysScreen` or the equivalent Settings reset). Boot-time canary failures route to `recoveryMode: "invalid-keys"` instead of `destroy()`. Issue #117 root-caused a chain of silent vault deletion to a boot-time auto-destroy cascade; ADR 018 is the durable rule. The runtime check `assertKeyDataCoupling()` is called at the end of every key-touching flow (`initFresh`, `applyCloudVault`, `restore`) to enforce the key-data coupling invariant mechanically rather than by convention. +- **API handlers answer JSON on every path; clients never treat an unparseable body as a network error.** A handler that throws returns no response, so the platform substitutes an HTML error page — and a client calling `res.json()` in the same `try` as its `fetch` reports that server crash as a *connection* failure, hiding the status code that identifies the real layer. Narrow untrusted payload fields to their expected type (`readTrimmedString` in `src/core/feedback/feedback-handler.ts`) instead of reaching for `.trim()` on whatever `JSON.parse` returned — `null`, arrays, and `{"message": 42}` are all valid JSON. On the client, only a rejected `fetch` may say "check your connection"; every answered request reports the server's own `error`, falling back to the status code (`submitFeedback` in `src/components/feedback/feedback-dialog.tsx`). - **Shared mutable headers leak Content-Length**: `@hono/node-server` mutates the `headers` record passed to `new Response(body, { headers })` by appending the computed `Content-Length`. A `const HEADERS = {...}` shared across responses lets a small response's `Content-Length` leak into a large response's headers, truncating the body at the receiver. The shared-state pattern is now a code-review smell — see `apiHeaders()` in `src/core/sync/sync-handler.ts` for the correct pattern (function returning a fresh object per call). This was the proximate cause of issue #117's `JSON.parse: unterminated string` reports. - **Test-only adapters are branded; resolvers refuse them in production.** Every in-memory adapter (`createMemoryAdapter`, `createMemoryCatalogAdapter`, `MemoryLicenseStorage`, `MemorySeenEventStore`) is branded via `markTestOnly()` from `src/core/test-only-brand.ts`. The four resolvers call `assertNotTestOnlyInProduction()` at the point they hand the adapter back; a misconfigured production deploy throws at module-load instead of silently routing writes to a per-cold-start `Map`. New backends MUST follow the same pattern: brand the memory implementation, guard the fallthrough in the resolver. Without this, the next "credential silently missing → adapter silently swapped" incident reads exactly like the 2026-05-12 sync regression and the 2026-05-14 stats-always-zero one. - **Feature gating is honor-system open-core**: Client-side tier gates live in `src/core/features/feature-gates.ts` and consume tier from `useLicenseStore` (`src/stores/license-store.ts`). React components call `useFeatureGate(feature)`; store actions call `enforceFeature(feature)` / `isFeatureEnabled(feature)` from `src/stores/enforce-feature.ts` for defense-in-depth. Self-hosters bypass via `VITE_SELF_HOSTED=1` at build time. Coming-soon features stay locked regardless. See ADR 012. diff --git a/docs/architecture.md b/docs/architecture.md index cd80a88c..ad4720c3 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -156,6 +156,24 @@ Endpoints: - `POST /api/checkout/create-session` — Stripe Checkout Session creation with priceId allowlist. Injects a server-controlled 30-day free trial via `subscription_data.trial_period_days` — see [ADR 015](decisions/015-stripe-side-trial.md). - `GET /api/health`, `GET /api/icon`, `POST /api/feedback`, `GET /api/favicon` — Operational endpoints. +#### JSON on every path, including the ones nobody calls + +A handler must answer JSON for *every* outcome it can produce — validation +failures, wrong methods, malformed payloads — and must never let an exception +escape. The two rules are the same rule: a handler that throws does not return +a response at all, so the platform substitutes its own HTML error page. + +Clients read the server's `error` field to tell the user what went wrong. Any +response they cannot parse collapses into a generic message and takes the +status code — the only clue identifying which layer broke — with it. This is +not hypothetical: `/api/feedback` read `body.message.trim()` straight off the +parsed payload, so a body of `null`, an array, or `{"message": 42}` (all valid +JSON) threw a `TypeError` out of the handler, and the feedback dialog reported +the resulting HTML 500 to the user as *"check your connection"* — sending them +to inspect the one part of the system that was working. Narrow untrusted +fields to their expected type before using them, and return `jsonResponse` from +the method guard too. + ### Production data layer: Upstash KV Per [ADR 008](decisions/008-upstash-as-production-data-layer.md), five distinct server-side concerns share one Upstash REST KV instance with non-overlapping key prefixes (`license:*`, `customer:*`, `vault:*`, `seen-event:*`, `catalog:*`, `ratelimit:*`). The credential cascade `UPSTASH_REDIS_REST_URL/TOKEN` → `KV_REST_API_URL/TOKEN` → memory fallback is shared by every adapter, so an operator configures Upstash once and all five subsystems pick it up. diff --git a/src/components/feedback/feedback-dialog.tsx b/src/components/feedback/feedback-dialog.tsx index 276755ab..25735274 100644 --- a/src/components/feedback/feedback-dialog.tsx +++ b/src/components/feedback/feedback-dialog.tsx @@ -33,6 +33,68 @@ import { toast } from "sonner"; const MAX_LENGTH = 2000; const GITHUB_ISSUES_URL = "https://github.com/forcingfx/feedzero/issues"; +interface FeedbackPayload { + message: string; + email?: string; +} + +type SubmitOutcome = { sent: true } | { sent: false; error: string }; + +/** + * Parse a response body as JSON, or `null` when it isn't JSON at all. + * + * The endpoint answers JSON on every path it controls, but the layers in front + * of it do not: a crashed function, a gateway timeout or a misrouted request + * come back as an HTML error page, and `res.json()` throws on those. + */ +async function readJsonBody( + res: Response, +): Promise<{ ok?: boolean; error?: string } | null> { + try { + return (await res.json()) as { ok?: boolean; error?: string }; + } catch { + return null; + } +} + +/** + * POST the note and turn every ending — sent, rejected, unreachable — into a + * message the user can act on. + * + * Only a rejected `fetch` is a connection problem. This used to catch the + * rejected `fetch` and a failed `res.json()` in one block, so a server that + * answered with an HTML error page (the shape of a crashed serverless + * function) was reported as a broken network: the user was sent to check the + * one part of the system that demonstrably worked, and the status code that + * would have identified the real fault never reached them. + */ +async function submitFeedback( + payload: FeedbackPayload, +): Promise { + let res: Response; + try { + res = await fetch("/api/feedback", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(payload), + }); + } catch { + return { + sent: false, + error: "Could not send feedback. Check your connection.", + }; + } + + const data = await readJsonBody(res); + if (res.ok && data?.ok) return { sent: true }; + + return { + sent: false, + error: + data?.error ?? `Could not send feedback (server error ${res.status}).`, + }; +} + interface FeedbackDialogProps { open: boolean; onOpenChange: (open: boolean) => void; @@ -49,30 +111,20 @@ export function FeedbackDialog({ open, onOpenChange }: FeedbackDialogProps) { if (!trimmedMessage) return; const trimmedEmail = email.trim(); - const payload: { message: string; email?: string } = { - message: trimmedMessage, - }; + const payload: FeedbackPayload = { message: trimmedMessage }; if (trimmedEmail) payload.email = trimmedEmail; setIsSending(true); try { - const res = await fetch("/api/feedback", { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify(payload), - }); - const data = await res.json(); - - if (data.ok) { - toast.success("Thanks for your feedback!"); - setMessage(""); - setEmail(""); - onOpenChange(false); - } else { - toast.error(data.error || "Could not send feedback"); + const outcome = await submitFeedback(payload); + if (!outcome.sent) { + toast.error(outcome.error); + return; } - } catch { - toast.error("Could not send feedback. Check your connection."); + toast.success("Thanks for your feedback!"); + setMessage(""); + setEmail(""); + onOpenChange(false); } finally { setIsSending(false); } diff --git a/src/core/feedback/feedback-handler.ts b/src/core/feedback/feedback-handler.ts index 833403ba..39914824 100644 --- a/src/core/feedback/feedback-handler.ts +++ b/src/core/feedback/feedback-handler.ts @@ -11,11 +11,6 @@ * before the user types it — see . */ -interface FeedbackBody { - message?: string; - email?: string; -} - const MAX_MESSAGE_LENGTH = 2000; const MAX_EMAIL_LENGTH = 254; @@ -30,7 +25,7 @@ export async function handleFeedbackRequest( request: Request, ): Promise { if (request.method !== "POST") { - return new Response("Method not allowed", { status: 405 }); + return jsonResponse({ ok: false, error: "Method not allowed" }, 405); } const token = process.env.GITHUB_FEEDBACK_TOKEN; @@ -43,14 +38,14 @@ export async function handleFeedbackRequest( ); } - let body: FeedbackBody; + let payload: unknown; try { - body = await request.json(); + payload = await request.json(); } catch { return jsonResponse({ ok: false, error: "Invalid JSON" }, 400); } - const message = body.message?.trim(); + const message = readTrimmedString(payload, "message"); if (!message) { return jsonResponse({ ok: false, error: "Message is required" }, 400); } @@ -64,7 +59,7 @@ export async function handleFeedbackRequest( // Email is optional. Reject obviously malformed values so a stray copy-paste // doesn't end up in the public issue body, but keep validation permissive — // the maintainer is the one who'll actually try replying. - const email = body.email?.trim(); + const email = readTrimmedString(payload, "email"); if (email) { if (email.length > MAX_EMAIL_LENGTH || !email.includes("@")) { return jsonResponse( @@ -112,6 +107,27 @@ export async function handleFeedbackRequest( } } +/** + * Read one field of the request payload as a trimmed string, or `undefined`. + * + * Every value `JSON.parse` accepts arrives here: `null`, arrays, bare strings + * and numbers are all valid JSON documents, and a well-formed object can still + * carry a number where a string belongs. Reaching straight for `.trim()` threw + * a TypeError that escaped the handler, and a rejected handler promise is not + * a response — the serverless platform answered with its own HTML 500 page. + * The dialog cannot parse HTML, so it reported a *server crash* as a *network + * failure* and pointed the user at their connection. Narrowing to string here + * keeps every malformed payload on the 400-JSON path. + */ +function readTrimmedString( + payload: unknown, + field: string, +): string | undefined { + if (typeof payload !== "object" || payload === null) return undefined; + const value = (payload as Record)[field]; + return typeof value === "string" ? value.trim() : undefined; +} + function jsonResponse(body: unknown, status = 200): Response { return new Response(JSON.stringify(body), { status, diff --git a/tests/components/feedback/feedback-dialog.test.tsx b/tests/components/feedback/feedback-dialog.test.tsx index d017f596..cf74d7d1 100644 --- a/tests/components/feedback/feedback-dialog.test.tsx +++ b/tests/components/feedback/feedback-dialog.test.tsx @@ -209,6 +209,58 @@ describe("FeedbackDialog", () => { }); }); + // A crashed serverless function, a gateway timeout or a misrouted request + // answers with an HTML error page, not JSON. Reporting that as a connection + // problem sends the user to check the one thing that is working; the status + // code is the only clue they can pass on to support. + it("reports the HTTP status when the server answers with a non-JSON error page", async () => { + mockFetch.mockResolvedValue( + new Response("

500: INTERNAL_SERVER_ERROR

", { + status: 500, + headers: { "Content-Type": "text/html" }, + }), + ); + + render(); + + await userEvent.type( + screen.getByPlaceholderText("What's on your mind?"), + "hello", + ); + await userEvent.click(screen.getByRole("button", { name: /send/i })); + + await vi.waitFor(() => { + expect(mockToast.error).toHaveBeenCalledWith( + expect.stringContaining("500"), + ); + }); + expect(mockToast.error).not.toHaveBeenCalledWith( + expect.stringMatching(/connection/i), + ); + }); + + it("keeps the dialog open with the message intact when submission fails", async () => { + const onOpenChange = vi.fn(); + mockFetch.mockResolvedValue( + new Response("

502

", { + status: 502, + headers: { "Content-Type": "text/html" }, + }), + ); + + render(); + + const textarea = screen.getByPlaceholderText("What's on your mind?"); + await userEvent.type(textarea, "hello"); + await userEvent.click(screen.getByRole("button", { name: /send/i })); + + await vi.waitFor(() => { + expect(mockToast.error).toHaveBeenCalled(); + }); + expect(onOpenChange).not.toHaveBeenCalledWith(false); + expect(textarea).toHaveValue("hello"); + }); + it("falls back to a connection error toast when fetch throws", async () => { mockFetch.mockRejectedValue(new TypeError("network down")); diff --git a/tests/core/feedback/feedback-handler.test.ts b/tests/core/feedback/feedback-handler.test.ts index 07c994a5..c8f4a46a 100644 --- a/tests/core/feedback/feedback-handler.test.ts +++ b/tests/core/feedback/feedback-handler.test.ts @@ -44,6 +44,51 @@ describe("handleFeedbackRequest", () => { expect(res.status).toBe(405); }); + it("answers a non-POST with a JSON body the client can parse", async () => { + const res = await handleFeedbackRequest( + new Request(ENDPOINT, { method: "GET" }), + ); + expect(res.headers.get("content-type")).toBe("application/json"); + await expect(res.json()).resolves.toMatchObject({ ok: false }); + }); + + // Every entry below is accepted by JSON.parse, so the handler reaches the + // field reads with a value it did not expect. Each one used to throw a + // TypeError that escaped the handler entirely: the serverless platform + // turned the rejected promise into an HTML 500 page, which the dialog could + // not parse, so the user was told to check their connection. + describe.each([ + ["a null body", null], + ["an array body", []], + ["a string body", "just text"], + ["a number body", 42], + ["a non-string message", { message: 42 }], + ["an object message", { message: { nested: true } }], + ])("malformed payload: %s", (_label, payload) => { + it("answers 400 JSON instead of throwing", async () => { + const res = await handleFeedbackRequest(postJson(payload)); + + expect(res.status).toBe(400); + expect(res.headers.get("content-type")).toBe("application/json"); + await expect(res.json()).resolves.toMatchObject({ ok: false }); + }); + }); + + it("treats a non-string email as absent rather than throwing", async () => { + const fetchMock = vi + .fn() + .mockResolvedValue(new Response(null, { status: 201 })); + vi.stubGlobal("fetch", fetchMock); + + const res = await handleFeedbackRequest( + postJson({ message: "Hi", email: 7 }), + ); + + expect(res.status).toBe(200); + const sent = JSON.parse(fetchMock.mock.calls[0][1].body); + expect(sent.body).toBe("Hi"); + }); + it("returns 503 when GITHUB_FEEDBACK_TOKEN is missing", async () => { delete process.env.GITHUB_FEEDBACK_TOKEN; const res = await handleFeedbackRequest(postJson({ message: "Hi" })); diff --git a/tests/smoke/feedback.test.ts b/tests/smoke/feedback.test.ts new file mode 100644 index 00000000..187820d4 --- /dev/null +++ b/tests/smoke/feedback.test.ts @@ -0,0 +1,71 @@ +// @vitest-environment node +import { describe, it, expect } from "vitest"; + +/** + * Smoke test: /api/feedback against the live deployment. + * + * Why this exists: the in-app feedback form was the only production endpoint + * with neither a smoke test nor an E2E, and it went three months without + * producing a single issue before a human noticed. Every check below is + * chosen to be provable without creating a real GitHub issue — each request + * is rejected by validation before the handler reaches the GitHub API. + * + * The load-bearing assertion is `Content-Type: application/json` on the + * failure paths. The dialog reads the server's own `error` field to tell the + * user what went wrong; anything that isn't JSON (a crashed function, a + * gateway page, a missing route) collapses into an unhelpful generic error + * and hides which layer actually broke. + * + * Skipped by default. Run with `SMOKE_TESTS=1 npx vitest run tests/smoke/`. + */ + +const SKIP = !process.env.SMOKE_TESTS; +const BASE_URL = process.env.SMOKE_BASE_URL ?? "https://my.feedzero.app"; + +function postFeedback(body: string): Promise { + return fetch(`${BASE_URL}/api/feedback`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body, + }); +} + +describe.skipIf(SKIP)("production /api/feedback (live)", () => { + it("is deployed and configured with GitHub credentials", async () => { + // An empty message is rejected at validation, so this never files an + // issue. A 503 here means GITHUB_FEEDBACK_TOKEN / GITHUB_REPO are missing + // from the deployment and every real submission is being dropped. + const res = await postFeedback(JSON.stringify({ message: "" })); + + expect(res.status, "endpoint missing from the deployment").not.toBe(404); + expect(res.status, "feedback credentials missing in production").not.toBe( + 503, + ); + expect(res.status).toBe(400); + }, 15_000); + + it("answers validation failures as JSON the dialog can render", async () => { + const res = await postFeedback(JSON.stringify({ message: "" })); + + expect(res.headers.get("content-type")).toContain("application/json"); + await expect(res.json()).resolves.toMatchObject({ ok: false }); + }, 15_000); + + it("rejects a malformed payload without crashing the function", async () => { + // `null` is valid JSON. Reading fields off it used to throw inside the + // handler, and a rejected handler promise becomes the platform's own HTML + // 500 page — which the dialog reported to the user as a network failure. + const res = await postFeedback("null"); + + expect(res.status).toBe(400); + expect(res.headers.get("content-type")).toContain("application/json"); + await expect(res.json()).resolves.toMatchObject({ ok: false }); + }, 15_000); + + it("answers a non-POST as JSON rather than plain text", async () => { + const res = await fetch(`${BASE_URL}/api/feedback`, { method: "GET" }); + + expect(res.status).toBe(405); + expect(res.headers.get("content-type")).toContain("application/json"); + }, 15_000); +});