diff --git a/.coderabbit.yaml b/.coderabbit.yaml index cdbe5780b8de..829f8cec571d 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -86,15 +86,22 @@ reviews: path_instructions: - path: "**/*.test.tsx" instructions: | - AGENTS.md forbids React component unit tests in this repo — components are covered by - Playwright specs in `apps/web/playwright/`. If this file is newly added, say so and ask for - the coverage to move to a Playwright `.spec.ts` instead. Do not raise this on edits to the - handful of `.test.tsx` files that already exist. + AGENTS.md forbids React component unit tests in this repo. If this file is newly added, say + so and ask for the logic under test to move into a `.ts` module with a unit test. Do not ask + for a Playwright spec instead: deleting a `.test.tsx` creates no E2E obligation, and a + component is not an E2E subject. Do not raise this on edits to the handful of `.test.tsx` + files that already exist. - path: "**/*.tsx" instructions: | - - Never suggest adding unit tests for `.tsx` files. AGENTS.md forbids them; UI behaviour is - covered by Playwright specs in `apps/web/playwright/`. Suggesting an E2E spec is fine. + - Never suggest adding unit tests for `.tsx` files — AGENTS.md forbids them, and their + absence is not a coverage gap to fill somewhere else. Ask for a Playwright spec only when + the diff changes a feature's happy path or a journey across several surfaces, and then ask + for it in that feature's existing spec — or, if the diff opens a feature area the suite does + not cover yet, for one happy-path spec named for that area. Never ask for one to cover + component detail (a + label, a breadcrumb, an ARIA attribute, a keystroke inside one widget, a field's + validation message) — that belongs in a `.ts` unit test or in manual QA. - New client data flows go through an `/api/v3` route with TanStack Query, not a new Server Action. Server data lives in the query cache only — flag it being mirrored into `useState` or Jotai, and flag `router.refresh()` used as a data-refresh mechanism. @@ -122,9 +129,27 @@ reviews: - path: "apps/web/playwright/**/*.spec.ts" instructions: | - - Flag timing hacks (`waitForTimeout`, arbitrary sleeps) and dependencies on data another - spec created — these are the repo's main source of flake. - - Specs must stay small and single-purpose; flag mega-specs. Slow suites need a `@slow` tag. + This suite is the critical path of every PR and every test is paid on every PR forever, so + cost is a review dimension here rather than a nit (AGENTS.md "Testing Guidelines"): + - A new spec file is a finding unless it opens a feature area the suite does not cover yet + (the spec filenames in `apps/web/playwright/` are the inventory — check before claiming an + area is uncovered); the default is assertions or a `test.step` added to that feature's + existing spec. A spec + that asserts component-level detail (a label, a breadcrumb, a sidebar's link list, an ARIA + attribute, a keystroke inside one widget, a field's validation message) is a finding + whatever file it lands in: ask for a `.ts` unit test or manual QA instead. + - Flag variant matrices. A second viewport, theme, locale, role or layout needs its own + stated reason; "the adjacent spec does it" is not one. A11y coverage for the rendered + survey extends `survey-accessibility.spec.ts`; elsewhere it belongs in that feature area's + own spec. Neither case is a per-ticket a11y spec. + - Flag state clicked into existence through the UI where Prisma or `/api/v3` could seed it + (`playwright/utils/accessibility.ts` is the pattern), and a per-test `users.create()` plus + login where a worker-scoped fixture would do. + - Flag timing hacks (`waitForTimeout`, arbitrary sleeps, `slowMo`) and dependencies on data + another spec created — these are the repo's main source of flake. + - Specs stay single-purpose per feature area: flag a spec that walks several unrelated + features, and a spec that exists for one detail of a covered one. `@slow` is metadata that + nothing reads, so a tag is not an answer to a cost finding. - path: "**/actions.ts" instructions: | diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 9056597bc2a9..0d610e6e2760 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -71,6 +71,12 @@ existed; `unit (mutation)` — only fails if you break the fix, because the code Any red-on-main or mutation row must carry the command or the mutated `file:line`, so a reviewer can rerun it instead of taking the claim on trust. --> + + | Behaviour | How | Outcome | | --- | --- | --- | | | unit (red on main) / unit (mutation) / unit (guard) / e2e / manual | | diff --git a/.github/workflows/docker-build-validation.yml b/.github/workflows/docker-build-validation.yml index c543ca424eef..40cac9788020 100644 --- a/.github/workflows/docker-build-validation.yml +++ b/.github/workflows/docker-build-validation.yml @@ -53,6 +53,15 @@ jobs: - name: Set up Docker Buildx uses: docker/setup-buildx-action@e468171a9de216ec08956ac3ada2f0791b6bd435 # v3.11.1 + # NOTE: deliberately no `cache-from`/`cache-to: type=gha`. Measured over four runs, exporting + # the cache cost 222-407s per run while the layers it could restore add up to 12s of work: the + # only steps that ever reported CACHED were the apk/corepack prelude, because `COPY . .` sits + # ahead of `pnpm install` in apps/web/Dockerfile and invalidates everything after it on any + # diff. Nothing populates a cache scope this job can read either — no workflow runs on pushes + # to main, and merge_group runs write to throwaway `gh-readonly-queue/...` scopes. Restoring + # the cache is worth revisiting only together with both halves of the fix: reorder the + # Dockerfile so the install layer survives a source change, and give the cache a producer PRs + # can read (a push-to-main build, or a registry cache, which is not branch-scoped). - name: Build Docker Image uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 # v6.18.0 env: @@ -63,8 +72,6 @@ jobs: push: false load: true tags: formbricks-test:${{ env.GITHUB_SHA }} - cache-from: type=gha - cache-to: type=gha,mode=max secrets: | database_url=${{ secrets.DUMMY_DATABASE_URL }} encryption_key=${{ secrets.DUMMY_ENCRYPTION_KEY }} diff --git a/AGENTS.md b/AGENTS.md index 344df1f10581..75eb3c2c1775 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -21,8 +21,8 @@ Every `packages/*` workspace therefore exposes the standard `lint` / `typecheck` `test:coverage` scripts (plus `build` where there is a compile step). Deliberate exceptions: `config-*` packages hold only config files (no scripts beyond `clean`); `types` has no runtime logic to test; `email`, `types`, and `vite-plugins` are consumed from source, so they have no `build`; -`apps/storybook` has no unit tests by policy (UI is covered by Playwright). Keep new packages on this -matrix or document the exception here. +`apps/storybook` has no unit tests by policy (its components are exercised by the feature journeys in +`apps/web/playwright`). Keep new packages on this matrix or document the exception here. ### Shared dependency versions (pnpm catalog) @@ -189,33 +189,74 @@ Always mark React component props as `Readonly<>` (e.g., `({ children }: Readonl Principles: - Confidence over coverage. Test behavior and outcomes; avoid brittle implementation-detail tests. +- Prove a behavior at the cheapest level that can fail on it. An E2E test is not a stronger unit test; it + has a different subject — the journey, not the logic. +- **An E2E test is paid on every PR, by everyone, forever.** The Playwright job is the critical path of the + PR gate (as of Aug 2026: a ~13 min job, of which ~6 min is the Playwright step itself — the rest is + install, build and boot — over ~110 tests and ~30 browser-minutes), and its wall clock can never drop + below its slowest single test. Weigh that before adding one — sometimes the right answer is no test + at this level. + +Which level, concretely: + +| The change | The level | +| ------------------------------------------------------------------------------- | --------------------------------------------- | +| A new feature area, or a journey across several surfaces | One happy-path E2E + unit tests for its logic | +| Business logic, invariants, validation, derivation, permissions — anything pure | Unit test on the `.ts` | +| A route's authorization, response shape, or query scoping | Unit or integration test on that route | +| A UI detail inside a feature that already has a happy-path spec | Neither — verify manually, say so in the PR | + +A journey across several surfaces means something like survey list → editor → public survey → response, +where the behavior only exists once browser, survey bundle, and server are wired together. + +The spec filenames in `apps/web/playwright/` are the inventory of covered areas — check there before +concluding an area has no spec. + +This raises a floor as well as lowering a ceiling. Every feature area ships a happy-path E2E, and an area +with none is a gap rather than a saving (Dashboards and Workflows are the current examples — ENG-2314). A +bug fix inside a feature that already has one almost never needs a second spec — the level still follows +the table above: journey behavior extends that spec, logic goes to a unit test, UI detail to manual QA. Do: -- E2E tests (Playwright): cover critical user flows and regression risks. Extend existing specs or add - focused new ones in `apps/web/playwright`, keep tests small and well-named, use descriptive filenames - such as `billing.spec.ts`, tag slow suites with `@slow`, and run the suite before opening a PR. +- E2E tests (Playwright): one spec per **feature area**, not per ticket and not per component. Default to + adding assertions or a `test.step` to that area's existing spec in `apps/web/playwright`; a new + `*.spec.ts` is for a feature area that has none, and it takes the area's name (`billing.spec.ts`). + Follow the suite's own patterns — seed state through Prisma or `/api/v3` instead of clicking it into + existence (`playwright/utils/accessibility.ts`), one journey per test with `test.step` phases + (`settings-tags.spec.ts`), assertions at feature level (`survey-overview.spec.ts`) — and run the suite + before opening a PR. - Unit tests: cover stable, high-value logic in `.ts` files, such as validators, transformers, evaluators, calculations, and edge cases. Keep assertions on inputs and outputs, colocate specs with the code they exercise (`utility.test.ts`), and mock network and storage boundaries through helpers from `@formbricks/*`. - Manual QA, especially for releases: verify on staging and file bugs. If a bug is critical, backport and - re-test. + re-test. For UI detail below the journey level, manual verification plus a screenshot in the PR is the + expected answer, not a new spec. - Run `pnpm test` before opening a PR and `pnpm test:coverage` when touching critical flows. +- Merging, narrowing, or deleting an E2E spec is legitimate work — record it in the PR's Coverage table + like any other change. Do not: -- Do not write component or UI unit tests for `.tsx` files; React components are covered by Playwright E2E - tests instead. +- Do not write component or UI unit tests for `.tsx` files. **This is not an instruction to write an E2E + test instead**: the absence of a component unit test creates no coverage obligation. If a component holds + logic worth proving, lift that logic into a `.ts` module and unit-test it there; the rendering is + exercised incidentally by the feature journeys that already cross it. +- Do not E2E a component. A language selector, a breadcrumb, a sidebar's link list, an ARIA attribute on + one widget, a keystroke inside one editor, a field's validation message, a search box filtering a list — + none of these justify a browser, a login, and a seeded tenant. +- Do not build a variant matrix. Cover the one case that carries the risk; a second viewport, theme, + locale, role, or layout needs its own stated reason, and "the adjacent spec does it" is not one. + Accessibility work on the rendered survey extends the existing axe gate + (`survey-accessibility.spec.ts`); elsewhere it becomes an assertion in that feature area's own spec. + Either way, not a per-ticket a11y spec. - Do not add coverage-driven or low-signal tests. -- Do not write tests that lock implementation details, markup, snapshots, or create churn. -- Do not create mega or flaky E2E tests; avoid timing hacks and unstable dependencies. - -Heuristic: - -- User journey risk: E2E. -- Pure logic or edge cases: unit test. -- Release readiness: manual QA plus bug/backport loop. +- Do not write tests that lock implementation details, markup, snapshots, or create churn — an assertion on + an exact list of nav labels is churn, not coverage. +- Do not create mega or flaky E2E tests; avoid timing hacks (`waitForTimeout`, `slowMo`) and unstable + dependencies. `@slow` is triage metadata only: nothing in `playwright.config.ts` or CI reads it, so + tagging a spec does not make its cost go away. ## Documentation (apps/docs) diff --git a/apps/web/integration/credential-backfill.integration.test.ts b/apps/web/integration/credential-backfill.integration.test.ts index e7370755ce0a..f8ad5e16d951 100644 --- a/apps/web/integration/credential-backfill.integration.test.ts +++ b/apps/web/integration/credential-backfill.integration.test.ts @@ -1,9 +1,9 @@ -import { createLocalAccountIssuer } from "@better-auth/core/db"; import { beforeEach, describe, expect, test } from "vitest"; import { prisma } from "@formbricks/database"; import { resetDb } from "@/integration/reset-db"; import { hashSecret } from "@/lib/crypto"; import { auth } from "@/modules/auth/lib/auth"; +import { canonicalAccountIssuer } from "@/modules/ee/sso/lib/constants"; // The cutover data migration under test (auto-discovered by the migration runner at the flip). import { backfillCredentialAccounts } from "../../../packages/database/migration/20260619120000_eng_1054_credential_account_backfill/migration"; @@ -12,15 +12,29 @@ import { backfillCredentialAccounts } from "../../../packages/database/migration * (data and schema migrations run in strict timestamp order), always runs BEFORE the schema migration * that adds that column — so it genuinely cannot set it, and its rows are inserted with issuer=NULL. * In real deployments that's fine: ENG-2343's schema migration runs immediately after this one and - * backfills every NULL-issuer credential row. A test calling this function standalone has to simulate - * that follow-up step itself before asserting a real Better Auth sign-in succeeds. + * backfills every NULL-issuer row. A test calling this function standalone has to simulate that + * follow-up step itself before asserting a real Better Auth sign-in succeeds — using the same canonical + * mapping production uses, so the fixture cannot drift from it (ENG-2555). */ -const applyEng2343IssuerBackfill = (): Promise<{ count: number }> => - prisma.account.updateMany({ - where: { provider: "credential", issuer: null }, - data: { issuer: createLocalAccountIssuer("credential") }, +const applyEng2343IssuerBackfill = async (): Promise => { + const rows = await prisma.account.findMany({ + where: { issuer: null }, + select: { id: true, provider: true }, }); + // Per row, because the real backfill is a CASE over `provider` — a single `updateMany` could only + // reproduce one arm of it. An earlier version of this helper did exactly that (credential only), which + // left the google row below at issuer=NULL and quietly diverged from what production data looks like. + await Promise.all( + rows.map((row) => + prisma.account.update({ + where: { id: row.id }, + data: { issuer: canonicalAccountIssuer(row.provider) }, + }) + ) + ); +}; + /** * Integration coverage for the cutover credential-account backfill (ENG-1054) against real Postgres. * Proves the scariest cutover guarantee: an existing NextAuth-era user (bcrypt hash on User.password, diff --git a/apps/web/integration/repair-account-issuer.integration.test.ts b/apps/web/integration/repair-account-issuer.integration.test.ts new file mode 100644 index 000000000000..2838d7cfd531 --- /dev/null +++ b/apps/web/integration/repair-account-issuer.integration.test.ts @@ -0,0 +1,114 @@ +import { beforeEach, describe, expect, test } from "vitest"; +import { prisma } from "@formbricks/database"; +import { resetDb } from "@/integration/reset-db"; +// The repair data migration under test (auto-discovered by the migration runner at deploy). +import { repairAccountIssuer } from "../../../packages/database/migration/20260821165535_repair_account_issuer/migration"; + +/** + * Integration coverage for the ENG-2555 `Account.issuer` repair against real Postgres — the one piece + * of that PR that was otherwise verified only by hand. The stakes are the same as the bug it repairs: + * this migration is an independent transcription of the canonical provider→issuer mapping, so a drift + * here (swap `IS DISTINCT FROM` for `<>`, drop the google arm) silently stops repairing rows while the + * unit suite stays green. `constants.test.ts` pins the SQL text; this proves the SQL's behaviour. + * + * The runner supplies `run` with its interactive transaction; here `prisma` stands in, which is the + * same shape the credential-backfill test uses for its migration. + */ +const runRepair = () => repairAccountIssuer.run!({ prisma, tx: prisma as never }); + +/** The seed matrix from the manual verification run — one row per repair class. */ +const seedMatrix = async (): Promise => { + const user = await prisma.user.create({ + data: { email: "issuer-matrix@example.com", name: "Matrix", emailVerified: true }, + }); + await prisma.account.createMany({ + data: [ + // the ENG-2555 shape: google stomped with the synthetic form + { + userId: user.id, + type: "oauth", + provider: "google", + providerAccountId: "g-sub", + issuer: "local:oauth:google", + }, + // already canonical — must stay byte-identical + { + userId: user.id, + type: "oauth", + provider: "github", + providerAccountId: "gh-sub", + issuer: "local:oauth:github", + }, + { + userId: user.id, + type: "credential", + provider: "credential", + providerAccountId: user.id, + issuer: "local:credential", + }, + // pre-backfill leftover: NULL issuer (IS DISTINCT FROM must catch it, `<>` would not) + { userId: user.id, type: "oauth", provider: "openid", providerAccountId: "oid-sub", issuer: null }, + ], + }); + return user.id; +}; + +const issuersByProvider = async (userId: string): Promise> => { + const rows = await prisma.account.findMany({ + where: { userId }, + select: { provider: true, issuer: true }, + }); + return Object.fromEntries(rows.map((r) => [r.provider, r.issuer])); +}; + +beforeEach(async () => { + await resetDb(); +}); + +describe("repairAccountIssuer data migration (real Postgres)", () => { + test("repairs the stomped google row and the NULL row, leaves canonical rows untouched", async () => { + const userId = await seedMatrix(); + + await runRepair(); + + expect(await issuersByProvider(userId)).toEqual({ + google: "https://accounts.google.com", + github: "local:oauth:github", + credential: "local:credential", + openid: "local:oauth:openid", + }); + }); + + test("is idempotent: a second run changes nothing", async () => { + const userId = await seedMatrix(); + await runRepair(); + const first = await issuersByProvider(userId); + + await runRepair(); + + expect(await issuersByProvider(userId)).toEqual(first); + }); + + test("is a no-op on a database with no Account rows", async () => { + await expect(runRepair()).resolves.toBeUndefined(); + expect(await prisma.account.count()).toBe(0); + }); + + /** + * The repaired row must be findable under Better Auth's own key — the property the whole ticket is + * about, asserted through the `@@unique([issuer, providerAccountId])` lookup 1.7 filters on. + */ + test("a repaired google row resolves by Better Auth's (issuer, accountId) key", async () => { + const userId = await seedMatrix(); + + await runRepair(); + + const found = await prisma.account.findUnique({ + where: { + issuer_providerAccountId: { issuer: "https://accounts.google.com", providerAccountId: "g-sub" }, + }, + select: { userId: true }, + }); + expect(found?.userId).toBe(userId); + }); +}); diff --git a/apps/web/modules/auth/lib/better-auth-observability.test.ts b/apps/web/modules/auth/lib/better-auth-observability.test.ts index 61ee954fdfb9..b82417502609 100644 --- a/apps/web/modules/auth/lib/better-auth-observability.test.ts +++ b/apps/web/modules/auth/lib/better-auth-observability.test.ts @@ -1,4 +1,7 @@ +import { BetterAuthError } from "@better-auth/core/error"; import * as Sentry from "@sentry/nextjs"; +import { betterAuth } from "better-auth"; +import { memoryAdapter } from "better-auth/adapters/memory"; import { APIError } from "better-auth/api"; import { beforeEach, describe, expect, test, vi } from "vitest"; import { prisma } from "@formbricks/database"; @@ -380,6 +383,213 @@ describe("betterAuthLogger (Sentry capture gating, ENG-2037)", () => { }); }); +/** + * ENG-2471: `BetterAuthError: State mismatch: verification not found` cleared the ENG-2037 gate and + * paged — 52 events in 14 days. It is a third shape ENG-2037 never enumerated: not a bare string code, + * not an `APIError`, but a `StateError extends BetterAuthError` carrying a stable `code`. + * + * Mirrors the real shape rather than importing it: `StateError` is internal to + * `better-auth/dist/state.mjs`, and it extends `BetterAuthError` adding only `code`/`details`/`errorURL` + * — so a subclass carrying `code` is exactly what the gate sees. The codes below are the five that + * module throws, read from its throw sites. + */ +class StateErrorLike extends BetterAuthError { + readonly code: string; + + constructor(message: string, code: string) { + super(message); + this.code = code; + } +} + +describe("betterAuthLogger — OAuth state errors (ENG-2471)", () => { + const log = betterAuthLogger.log!; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + // Client- or timing-caused: the verification record was purged or already consumed, or the parsed + // state is past its `expiresAt`. Nothing to act on, so these must not page. + test.each([ + ["state_mismatch", "State mismatch: verification not found"], + // Cookie-branch message, kept as a label only: the code is what the gate reads, and this variant + // is unreachable on our database strategy. + ["state_mismatch", "State mismatch: auth state cookie not found"], + ["state_mismatch", "Invalid state: request expired"], + ])("does not capture the client-caused %s (%s)", (code, message) => { + const stateError = new StateErrorLike(message, code); + + log("error", message, stateError); + + expect(Sentry.captureException).not.toHaveBeenCalled(); + // Still visible in the application log — suppression is Sentry-only. A burst of these is how a + // Redis eviction would present, since verification records live in Redis only. + expect(contextLoggerMock.error).toHaveBeenCalledWith(message); + }); + + /** + * The other four codes stay captured, and each for its own reason: + * - `state_generation_error` — the adapter could not write the verification row: a real fault. + * - `state_security_mismatch` — the state does not match the stored one, OR the signed state cookie + * fails verification. That second half is where a cross-replica `BETTER_AUTH_SECRET` divergence + * surfaces (a real outage) and also where the benign 5–10 minute cookie-expiry case lands, so it is + * mixed and ENG-2471 defers judging it until the ENG-2259 `auth.path` tag has sized the split. + * - `state_invalid` — cookie-branch only, so unreachable on this configuration. Kept because + * suppressing a code that never fires buys nothing. + * - `state_not_found` — a defensive entry rather than a live one. Verified against 1.7.0 that the + * callback route short-circuits a missing `state` before any `StateError` is thrown, and logs it + * with the OAuth `error` query param rather than an `Error` — so it reaches neither this gate nor + * Sentry, and the assertion below is a contract for a shape the callback route does not currently + * produce. Kept captured so that if upstream ever does route it through as a real `StateError`, it + * pages instead of being dropped by a stale allow-list. + */ + test.each([ + ["state_generation_error", "Unable to create verification"], + ["state_invalid", "State invalid: Failed to decrypt or parse auth state"], + ["state_security_mismatch", "State mismatch: OAuth state parameter does not match stored state"], + ["state_not_found", "State not found in OAuth callback"], + ])("still captures %s", (code, message) => { + const stateError = new StateErrorLike(message, code); + + log("error", message, stateError); + + expect(Sentry.captureException).toHaveBeenCalledWith(stateError, { + tags: { component: "better-auth" }, + }); + }); + + /** + * The log field is the whole justification for suppressing anything: a suppressed event survives only + * in the application log, so it has to be queryable by the same value that suppressed it. Without this + * test the field can be deleted and the suite stays green — verified, it did. + * + * Recorded for captured codes too, so one query covers the class rather than only its quiet half. + */ + test.each([ + ["state_mismatch", "suppressed"], + ["state_security_mismatch", "captured"], + ])("records %s on the log context (%s)", (code) => { + log("error", "State mismatch", new StateErrorLike("State mismatch", code)); + + expect(logger.withContext).toHaveBeenCalledWith({ + source: "better-auth", + stateErrorCode: code, + }); + }); + + // The raw `state` is a single-use credential for the in-flight flow and lives on the error's + // `details`. Only the code is ever read, so it cannot reach the log through this path. + test("never puts the raw state value on the log context", () => { + const stateError = new StateErrorLike("State mismatch: verification not found", "state_mismatch"); + Object.assign(stateError, { details: { state: "super-secret-state-value" } }); + + log("error", "State mismatch: verification not found", stateError); + + expect(JSON.stringify(vi.mocked(logger.withContext).mock.calls)).not.toContain( + "super-secret-state-value" + ); + }); + + // The gate fails CLOSED: anything it cannot positively identify as THE one suppressed code + // (`state_mismatch`) is still captured. A wrong answer should add noise, never silence a fault. + test("captures a BetterAuthError carrying no code", () => { + const bare = new BetterAuthError("something upstream broke"); + + log("error", "something upstream broke", bare); + + expect(Sentry.captureException).toHaveBeenCalledWith(bare, { + tags: { component: "better-auth" }, + }); + }); + + test("captures a look-alike that is not a BetterAuthError, even with a suppressed code", () => { + const impostor = Object.assign(new Error("State mismatch: verification not found"), { + name: "BetterAuthError", + code: "state_mismatch", + }); + + log("error", "State mismatch: verification not found", impostor); + + expect(Sentry.captureException).toHaveBeenCalledWith(impostor, { + tags: { component: "better-auth" }, + }); + }); +}); + +/** + * The contract test for the gate above, and the reason the `StateErrorLike` tests are not enough on + * their own: they assert against a stand-in we define here, so they would keep passing if the REAL + * `StateError` stopped satisfying the gate. `StateError` is internal to `better-auth/dist/state.mjs` + * and cannot be imported, so the only way to bind the real shape is to make Better Auth throw one. + * + * This drives a real `betterAuth` instance — configured with `betterAuthLogger` itself, so the whole + * production path runs — at an OAuth callback carrying a `state` with no verification record. That is + * exactly the reported failure (`State mismatch: verification not found`, FORMBRICKS-16G). If a future + * upgrade renames the code, changes the class, or stops routing it through the logger, this fails. + */ +describe("betterAuthLogger — the real Better Auth StateError (ENG-2471 contract)", () => { + const BASE_URL = "http://localhost:3000"; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + const provokeStateMismatch = async (): Promise => { + const auth = betterAuth({ + baseURL: BASE_URL, + secret: "eng-2471-contract-test-secret-0123456789abcdef", + // memoryAdapter declares its models up front; an empty `verification` table is the point — the + // state below resolves to no record, which is what throws. + database: memoryAdapter({ user: [], session: [], account: [], verification: [] }), + logger: betterAuthLogger, + socialProviders: { google: { clientId: "contract-test", clientSecret: "contract-test" } }, + }); + + await auth.handler( + new Request(`${BASE_URL}/api/auth/callback/google?state=no-such-state&code=irrelevant`) + ); + }; + + test("is logged, so this suite is exercising the path at all", async () => { + await provokeStateMismatch(); + + // Anti-vacuity: if Better Auth stopped routing this through our logger, the assertion below would + // pass for the wrong reason — nothing captured because nothing happened. + expect(contextLoggerMock.error).toHaveBeenCalled(); + }); + + // Pins the exact shape the gate depends on, so an upstream rename fails here loudly instead of + // quietly restoring the noise. Uses a capturing logger rather than `betterAuthLogger`, because the + // production logger deliberately forwards only the message to our logger, not the Error. + test("is a BetterAuthError carrying code state_mismatch", async () => { + const seen: unknown[] = []; + const auth = betterAuth({ + baseURL: BASE_URL, + secret: "eng-2471-contract-test-secret-0123456789abcdef", + database: memoryAdapter({ user: [], session: [], account: [], verification: [] }), + logger: { level: "error", log: (_level, _message, ...args) => seen.push(...args) }, + socialProviders: { google: { clientId: "contract-test", clientSecret: "contract-test" } }, + }); + + await auth.handler( + new Request(`${BASE_URL}/api/auth/callback/google?state=no-such-state&code=irrelevant`) + ); + + const stateError = seen.find((arg): arg is Error & { code?: unknown } => arg instanceof BetterAuthError); + expect(stateError).toBeDefined(); + expect(stateError?.name).toBe("BetterAuthError"); + expect(stateError?.code).toBe("state_mismatch"); + }); + + test("is NOT captured to Sentry", async () => { + await provokeStateMismatch(); + + expect(contextLoggerMock.error).toHaveBeenCalled(); + expect(Sentry.captureException).not.toHaveBeenCalled(); + }); +}); + // ENG-2259: Better Auth's router logs a non-APIError as `(e.name, e)` and drops the endpoint, so the // capture arrived with no transaction, URL or route and FORMBRICKS-183 could not be triaged at all. // The request context supplies the endpoint; these cases pin that it reaches Sentry AND the local log, diff --git a/apps/web/modules/auth/lib/better-auth-observability.ts b/apps/web/modules/auth/lib/better-auth-observability.ts index c7eee7c35143..76de82684bb3 100644 --- a/apps/web/modules/auth/lib/better-auth-observability.ts +++ b/apps/web/modules/auth/lib/better-auth-observability.ts @@ -1,4 +1,5 @@ import "server-only"; +import { BetterAuthError } from "@better-auth/core/error"; import * as Sentry from "@sentry/nextjs"; import type { BetterAuthOptions } from "better-auth"; import { isAPIError } from "better-auth/api"; @@ -129,6 +130,117 @@ const EMAIL_IN_MESSAGE = /[^\s@]{1,64}@([\w-]{1,63}(?:\.[\w-]{1,63}){1,8})/g; export const redactEmailsInLogMessage = (message: unknown): unknown => typeof message === "string" ? message.replace(EMAIL_IN_MESSAGE, "[redacted]@$1") : message; +/** + * `StateError` codes whose events are client- or timing-caused, and so are not actionable in Sentry + * (ENG-2471). `StateError extends BetterAuthError` and carries a stable `code`, which is what we match + * on — never the message, which is not a contract. + * + * Read from the throw sites in `better-auth/dist/state.mjs`, **for the strategy we actually run**. That + * matters more than it sounds: the file has two branches that throw different codes for the same + * underlying cause, and reading the wrong one inverts the conclusions. Better Auth picks the strategy + * from `!!options.database || !!options.secondaryStorage`, and `auth.ts` sets BOTH — so it resolves to + * `"database"` and the cookie branch never executes here. Noted as both deliberately: dropping Redis + * alone would not flip it back. + * + * | code | thrown when (database strategy) | actionable? | + * | --- | --- | --- | + * | `state_mismatch` | no verification record for this `state` — purged after its 10-minute TTL, already consumed, or never issued — or the parsed state is past `expiresAt` | no — **suppressed** | + * | `state_not_found` | the callback carried no `state` — **never reaches this gate on 1.7**, see below | kept | + * | `state_security_mismatch` | the state does not match the stored one, or the signed `state` cookie fails verification ("State not persisted correctly") | **yes**, kept | + * | `state_generation_error` | the adapter could not write the verification row | **yes**, kept — a real fault | + * | `state_invalid` | undecryptable state — **cookie branch only, unreachable on this config** | kept; suppressing a code that never fires buys nothing | + * + * `state_not_found` is kept out of the set, but on 1.7 that choice is **inert**, and the honest reason + * is worth recording because an earlier draft of this comment got it wrong in both directions. + * + * Verified live against 1.7.0: an OAuth callback with no `state` never produces a `StateError` at all. + * The callback route short-circuits before `parseGenericState` is reached + * (`better-auth/dist/api/routes/callback.mjs:72-76`): + * + * ```js + * if (!state) { c.context.logger.error("State not found", error); throw c.redirect(`…error=state_not_found`); } + * ``` + * + * That second argument is the OAuth `error` *query parameter*, not an `Error`, so our `cause` lookup + * finds nothing, the Sentry gate's `cause &&` is already false, and the event is logged without a + * `stateErrorCode`. It was therefore never captured — this code is not part of the over-capture problem, + * and the `StateError` carrying it (`state.mjs:90`) is unreachable from this route. Listing it here or + * omitting it changes nothing today; it stays omitted so that if upstream ever routes it through the + * logger as a real `StateError`, it pages rather than being silently dropped by a stale allow-list. + * + * Worth knowing separately, because it is a gap this PR does not close: that makes an IdP which stops + * echoing `state` — a provider-wide sign-in outage — produce **no Sentry event whatsoever**, only that + * log line. Independent of this change, and not fixable from this gate. + * + * `state_security_mismatch` carries the load this gate deliberately does not take on, and it is mixed: + * + * - The state cookie's `maxAge` is 300s while the verification record lives 10 minutes, so a user who + * spends 5–10 minutes at the identity provider loses the cookie first and lands here. Benign, and + * probably common — **so this change likely leaves residual noise behind**; it closes the reported + * `verification not found` shape (FORMBRICKS-16G), not the whole class. + * - It is also where a `BETTER_AUTH_SECRET` divergence across replicas surfaces, because the signed + * cookie cannot be verified with a different secret. That is a genuine outage and must keep paging. + * - And it is the shape a forged or mixed-up callback takes. + * + * Code cannot separate those three, which is exactly why ENG-2471 defers the decision to the `auth.path` + * tag from ENG-2259 rather than guessing. Suppressing a signal before measuring it is how ENG-2037 left + * this shape behind in the first place. + * + * One security-visible consequence, stated rather than left implicit: a *replayed* callback — the same + * `state` presented again after the legitimate flow consumed it — also lands on `state_mismatch`, so + * replay attempts stop being visible in Sentry. That is judged acceptable because the replay fails + * closed (the record is single-use, and the authorization code is single-use at the IdP too), so this is + * a loss of visibility into a *failed* attempt, not of a control. The events remain in the application + * log with their `stateErrorCode`, which is where a campaign would show up as volume. + * + * What suppression costs, stated narrowly. Two shapes produce `state_mismatch` for *real* login + * failures, and this gate hides both from Sentry: + * + * 1. **Runtime:** a Redis eviction, flush, or failover to an empty replica drops in-flight verification + * records. Arrives as a trickle or a burst, mid-operation. + * 2. **Deploy-time, and the worse of the two:** replicas that do not share one Redis. The record is + * written by the pod that started the flow and looked up by whichever pod serves the callback, and + * that lookup is the *first* check in the database branch — before the signed-cookie check. So a + * split-Redis deployment fails 100% of SSO sign-ins while the state cookie still verifies perfectly + * (the secret is shared even when the store is not), which keeps it off `state_security_mismatch` and + * squarely on the code we suppress. It also lands exactly when someone is watching Sentry rather than + * the logs. + * + * A *hard* Redis outage is not in this class — `secondary-storage.ts` rethrows on connect failure, which + * is not a `StateError`, so it still pages. + * + * The suppressed events stay at `error` in the application log with `stateErrorCode` and the request's + * `authPath`, so they are greppable — and `stateErrorCode` is load-bearing rather than convenient here, + * because upstream logs every state error under the same "Failed to parse state" message. What is + * missing is the alert: there is no log-based rule on that field today, so a burst is discoverable + * rather than announced, which for shape 2 means a total outage nobody is paged for. Tracked separately; + * this gate cannot fix it. + */ +const UNACTIONABLE_STATE_ERROR_CODES: ReadonlySet = new Set(["state_mismatch"]); + +/** + * The `StateError` code carried by a logged cause, or undefined when it is not one. + * + * Extracted once and used for BOTH the log context and the Sentry gate, so a suppressed event stays + * queryable by the same value that suppressed it — the log is the only place these survive, and an + * upstream message string is not something to build an alert on. + * + * Deliberately reads only `code`, never the error's `details`, which holds the raw `state` value: that + * is a single-use credential for the in-flight OAuth flow and has no business in a log line. + */ +const getStateErrorCode = (cause: Error | undefined): string | undefined => { + if (!(cause instanceof BetterAuthError)) return undefined; + const { code } = cause as { code?: unknown }; + return typeof code === "string" ? code : undefined; +}; + +/** + * Fails CLOSED on purpose: anything not positively identified as an unactionable code is still + * captured. A wrong answer here should add noise, never silence a genuine fault. + */ +const isUnactionableStateError = (code: string | undefined): boolean => + code !== undefined && UNACTIONABLE_STATE_ERROR_CODES.has(code); + /** * Route Better Auth's logger to @formbricks/logger and capture GENUINE internal faults to Sentry in * production — replaces auth.ts's placeholder logger (and the route's Sentry.captureException on auth @@ -158,21 +270,26 @@ export const betterAuthLogger: NonNullable = { // construction, a server-side `auth.api.*` call). Already reduced to a safe label — never a raw // path, which on `/reset-password/:token` would be a live credential (better-auth-path-label.ts). const request = getBetterAuthRequestContext(); + // BA usually passes the Error as a trailing arg, but a couple of sites pass it as `message`. + const cause = [...args, message].find((arg): arg is Error => arg instanceof Error); + const stateErrorCode = getStateErrorCode(cause); const contextLogger = logger.withContext({ source: "better-auth", // Self-hosters have no Sentry, so the label has to reach the application log too — otherwise // their copy of this fault stays as untriageable as FORMBRICKS-183 was. ...(request && { authPath: request.path, httpMethod: request.method }), + // The suppressed state rejections survive only in the log, so the code has to be queryable there + // (ENG-2471). Recorded for every state error, not only the suppressed ones. + ...(stateErrorCode && { stateErrorCode }), }); const safeMessage = redactEmailsInLogMessage(message); if (level === "error") { contextLogger.error(safeMessage); if (SENTRY_DSN && IS_PRODUCTION) { - // BA usually passes the Error as a trailing arg, but a couple of sites pass it as `message`. - const cause = [...args, message].find((arg): arg is Error => arg instanceof Error); - // Skip handled rejections: a bare string code (no Error) or a client-facing APIError. Capture - // only genuine internal faults so Sentry stays actionable (see the reason-split above). - if (cause && !isAPIError(cause)) { + // Skip handled rejections: a bare string code (no Error), a client-facing APIError, or a + // client/timing-caused OAuth `StateError` (ENG-2471). Capture only genuine internal faults so + // Sentry stays actionable (see the reason-split above and UNACTIONABLE_STATE_ERROR_CODES). + if (cause && !isAPIError(cause) && !isUnactionableStateError(stateErrorCode)) { // ENG-2259: Better Auth's router logs a non-APIError as `(e.name, e)` and discards the // endpoint (`better-auth/dist/api/index.mjs:210`), so a bare capture arrives with no // transaction, URL or route — which is why FORMBRICKS-183 sat at ~242 events untriageable. diff --git a/apps/web/modules/ee/sso/lib/account-linking.test.ts b/apps/web/modules/ee/sso/lib/account-linking.test.ts index 4135cf0e980f..fe88625a1ccb 100644 --- a/apps/web/modules/ee/sso/lib/account-linking.test.ts +++ b/apps/web/modules/ee/sso/lib/account-linking.test.ts @@ -31,7 +31,7 @@ describe("syncSsoIdentityForUser", () => { type: "oauth" as const, provider: "google", providerAccountId: "provider-account-1", - issuer: "local:oauth:google", + issuer: "https://accounts.google.com", access_token: "access-token", refresh_token: "refresh-token", scope: "openid email profile", @@ -101,10 +101,11 @@ describe("syncSsoIdentityForUser", () => { id: "account_1", }, data: { - // `issuer` on the token-refresh branch too (ENG-2343): the canonical row may predate the - // backfill window, and 1.7's account lookup filters on `(issuer, accountId)` — so leaving it - // NULL here would keep a recovered link invisible and re-trigger recovery on the next sign-in. - issuer: "local:oauth:google", + // `issuer` on the token-refresh branch too (ENG-2343, corrected in ENG-2555): the canonical row + // may predate the backfill window OR carry a wrong value written before the fix, and 1.7's + // account lookup filters on `(issuer, accountId)` — so leaving it alone here would keep a + // recovered link invisible and re-trigger recovery on the next sign-in, forever. + issuer: "https://accounts.google.com", access_token: "access-token", refresh_token: "refresh-token", scope: "openid email profile", @@ -138,7 +139,7 @@ describe("syncSsoIdentityForUser", () => { type: "oauth", provider: "google", providerAccountId: "provider-account-1", - issuer: "local:oauth:google", + issuer: "https://accounts.google.com", access_token: "access-token", refresh_token: "refresh-token", scope: "openid email profile", @@ -186,7 +187,10 @@ describe("syncSsoIdentityForUser", () => { where: { id: "account_1", }, + // `issuer` is written here as of ENG-2555 — this branch used to update tokens only, which is what + // stopped a row with a wrong issuer from ever healing. data: { + issuer: "https://accounts.google.com", access_token: "access-token", refresh_token: "refresh-token", scope: "openid email profile", @@ -222,7 +226,7 @@ describe("syncSsoIdentityForUser", () => { type: "oauth", provider: "google", providerAccountId: "provider-account-1", - issuer: "local:oauth:google", + issuer: "https://accounts.google.com", access_token: "access-token", refresh_token: "refresh-token", expires_at: 1234, @@ -233,4 +237,48 @@ describe("syncSsoIdentityForUser", () => { }); expect(mocks.userUpdate).toHaveBeenCalledOnce(); }); + + /** + * Every other assertion in this file uses google, which is exactly how ENG-2555 shipped: google is the + * one provider whose issuer is NOT the synthetic `local:oauth:` form, so a helper that always returned + * the synthetic form looked correct against a google-only suite. These two pin both arms. + */ + test("uses the provider's own declared issuer for google, not the synthetic form", async () => { + await syncSsoIdentityForUser({ userId: "user_1", provider: "google", account }); + + expect(mocks.accountCreate).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ issuer: "https://accounts.google.com" }), + }) + ); + }); + + test("uses the synthetic issuer for a provider that declares none", async () => { + await syncSsoIdentityForUser({ + userId: "user_1", + provider: "github", + account: { ...account, provider: "github", providerAccountId: "github-account-1" }, + }); + + expect(mocks.accountCreate).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ provider: "github", issuer: "local:oauth:github" }), + }) + ); + }); + + /** + * The branch that made the loop unbreakable (ENG-2555): with a canonical row already present and no + * legacy row, this used to update tokens only, so a row carrying a wrong issuer could never heal. + */ + test("repairs the issuer on an existing canonical row", async () => { + mocks.accountFindUnique.mockResolvedValue({ id: "account_1", userId: "user_1" }); + + await syncSsoIdentityForUser({ userId: "user_1", provider: "google", account }); + + expect(mocks.accountUpdate).toHaveBeenCalledWith({ + where: { id: "account_1" }, + data: expect.objectContaining({ issuer: "https://accounts.google.com" }), + }); + }); }); diff --git a/apps/web/modules/ee/sso/lib/account-linking.ts b/apps/web/modules/ee/sso/lib/account-linking.ts index bac495bf7d8e..e67867654ac3 100644 --- a/apps/web/modules/ee/sso/lib/account-linking.ts +++ b/apps/web/modules/ee/sso/lib/account-linking.ts @@ -1,7 +1,7 @@ import { prisma } from "@formbricks/database"; import type { IdentityProvider, Prisma } from "@formbricks/database/prisma"; import type { Account } from "@formbricks/types/auth"; -import { OAUTH_ACCOUNT_NOT_LINKED_ERROR, ssoAccountIssuer } from "@/modules/ee/sso/lib/constants"; +import { OAUTH_ACCOUNT_NOT_LINKED_ERROR, canonicalAccountIssuer } from "@/modules/ee/sso/lib/constants"; export const LINKED_SSO_LOOKUP_SELECT = { id: true, @@ -97,9 +97,10 @@ const syncSsoIdentityForUserWithTx = async ({ where: { id: existingCanonicalAccount.id, }, - // `issuer` too: the canonical row may predate the ENG-2343 backfill window, and leaving it NULL - // here would keep the recovered link invisible to 1.7's account lookup. - data: { issuer: ssoAccountIssuer(provider), ...getAccountTokenUpdate(account) }, + // `issuer` too, and the CANONICAL value (ENG-2555): the row may predate the ENG-2343 backfill + // window (NULL) or carry the synthetic form where the provider declares its own — either way + // 1.7's account lookup cannot see it until this write corrects it. + data: { issuer: canonicalAccountIssuer(provider), ...getAccountTokenUpdate(account) }, }); } else { await tx.account.update({ @@ -113,7 +114,7 @@ const syncSsoIdentityForUserWithTx = async ({ providerAccountId: account.providerAccountId, // Same reason as the create branch below: normalising a legacy row without setting `issuer` // leaves it unmatched by 1.7's account lookup (ENG-2343). - issuer: ssoAccountIssuer(provider), + issuer: canonicalAccountIssuer(provider), ...getAccountTokenUpdate(account), }, }); @@ -123,7 +124,13 @@ const syncSsoIdentityForUserWithTx = async ({ where: { id: existingCanonicalAccount.id, }, - data: getAccountTokenUpdate(account), + // `issuer` here too, and this branch is the one that matters most (ENG-2555). It is the branch + // every attempt after the first takes, so while it wrote only tokens a row created with a wrong + // or NULL issuer could never heal: sign-in could not see it, recovery ran again, and this update + // left the bad value untouched. Writing the canonical value makes the next sign-in repair it. + // The row's ownership is already asserted above, and the value derives from `provider`, so this + // cannot rebind the row to another identity. + data: { issuer: canonicalAccountIssuer(provider), ...getAccountTokenUpdate(account) }, }); } else { await tx.account.create({ @@ -133,12 +140,14 @@ const syncSsoIdentityForUserWithTx = async ({ provider, providerAccountId: account.providerAccountId, // 1.7 keys the account on `(issuer, accountId)` and `findAccountByKey` filters on `issuer`, so a - // row written without one is invisible to every later sign-in: the user completes - // verify-before-link, gets a session, and is then pushed back through recovery on the NEXT - // sign-in because `NULL !== 'local:oauth:'`. The migration cannot save them either — - // it runs once, before this row exists. Same value as the provider config and the backfill - // (ENG-2343); imported rather than re-spelled so the three cannot drift. - issuer: ssoAccountIssuer(provider), + // row written with a missing OR non-canonical issuer is invisible to every later sign-in: the + // user completes verify-before-link, gets a session, and is pushed back through recovery on the + // NEXT sign-in, forever. The migration cannot save them either — it runs once, before this row + // exists. NOT the same helper the provider config pins (`ssoAccountIssuer`): google's canonical + // issuer is the one upstream declares, not the synthetic form, which is exactly the bug that + // shipped here (ENG-2555). `canonicalAccountIssuer` mirrors the backfill's CASE and is pinned + // against both the SQL and upstream in constants.test.ts, so the sites cannot drift. + issuer: canonicalAccountIssuer(provider), ...getAccountTokenUpdate(account), }, }); diff --git a/apps/web/modules/ee/sso/lib/better-auth-recovery-signin.integration.test.ts b/apps/web/modules/ee/sso/lib/better-auth-recovery-signin.integration.test.ts index a13cd713717c..ff552ff0e5cb 100644 --- a/apps/web/modules/ee/sso/lib/better-auth-recovery-signin.integration.test.ts +++ b/apps/web/modules/ee/sso/lib/better-auth-recovery-signin.integration.test.ts @@ -4,6 +4,7 @@ import { resetDb } from "@/integration/reset-db"; import { WEBAPP_URL } from "@/lib/constants"; import { createToken } from "@/lib/jwt"; import { auth } from "@/modules/auth/lib/auth"; +import { syncSsoIdentityForUser } from "@/modules/ee/sso/lib/account-linking"; /** * Integration coverage for the SSO-recovery magic-link sign-in (ENG-1054, Phase 7) against real @@ -72,3 +73,78 @@ describe("SSO recovery sign-in (real Postgres)", () => { expect(await prisma.session.count({ where: { userId: user.id } })).toBe(2); }); }); + +/** + * ENG-2555. The unit tests for `syncSsoIdentityForUser` mock Prisma and assert the issuer *we* pass, so + * they can only ever confirm the value we chose — which is exactly how a wrong one shipped. These assert + * the property that actually matters: after recovery links an account, Better Auth can find it again. + * + * The lookup key is reproduced the way upstream builds it (a declared `accountIssuer` wins, else the + * synthetic form), so this fails if our write and upstream's read ever disagree — regardless of which + * side moved. + */ +describe("SSO recovery writes an issuer Better Auth can find (real Postgres)", () => { + const findByBetterAuthKey = (issuer: string, accountId: string) => + prisma.account.findUnique({ + where: { issuer_providerAccountId: { issuer, providerAccountId: accountId } }, + select: { userId: true, provider: true }, + }); + + test.each([ + // google declares its own issuer upstream — the case that broke + ["google", "google-sub-1", "https://accounts.google.com"], + // github declares none, so the synthetic form is correct for it + ["github", "github-id-1", "local:oauth:github"], + ] as const)("links %s under the key Better Auth looks it up by", async (provider, sub, expectedIssuer) => { + const user = await prisma.user.create({ + data: { email: `${provider}-link@example.com`, name: "Linked", emailVerified: true }, + }); + + await prisma.$transaction((tx) => + syncSsoIdentityForUser({ + userId: user.id, + provider, + account: { type: "oauth", provider, providerAccountId: sub }, + tx, + }) + ); + + const found = await findByBetterAuthKey(expectedIssuer, sub); + + expect(found).not.toBeNull(); + expect(found?.userId).toBe(user.id); + expect(found?.provider).toBe(provider); + }); + + /** + * The branch that made the bug unbreakable: a row already carrying a wrong issuer must be repaired by + * the next recovery, not left alone. Before the fix this update wrote tokens only. + */ + test("repairs a row that already carries a wrong issuer", async () => { + const user = await prisma.user.create({ + data: { email: "stomped@example.com", name: "Stomped", emailVerified: true }, + }); + await prisma.account.create({ + data: { + userId: user.id, + type: "oauth", + provider: "google", + providerAccountId: "google-sub-2", + issuer: "local:oauth:google", + }, + }); + + await prisma.$transaction((tx) => + syncSsoIdentityForUser({ + userId: user.id, + provider: "google", + account: { type: "oauth", provider: "google", providerAccountId: "google-sub-2" }, + tx, + }) + ); + + expect(await findByBetterAuthKey("https://accounts.google.com", "google-sub-2")).not.toBeNull(); + // and no duplicate was created in the process + expect(await prisma.account.count({ where: { userId: user.id } })).toBe(1); + }); +}); diff --git a/apps/web/modules/ee/sso/lib/constants.test.ts b/apps/web/modules/ee/sso/lib/constants.test.ts new file mode 100644 index 000000000000..a1911ceeabb9 --- /dev/null +++ b/apps/web/modules/ee/sso/lib/constants.test.ts @@ -0,0 +1,165 @@ +import { createLocalAccountIssuer, createOAuthAccountIssuer } from "@better-auth/core/db"; +import { github, google } from "@better-auth/core/social-providers"; +import { readFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { describe, expect, test } from "vitest"; +import { canonicalAccountIssuer, ssoAccountIssuer } from "./constants"; + +/** + * `Account.issuer` is spelled in four places that cannot import each other, and ENG-2555 happened + * because two of them disagreed about google: the SQL backfill had a `CASE` arm for it, the TypeScript + * helper did not, and every Google sign-in broke while the whole suite stayed green. + * + * So this pins `canonicalAccountIssuer` against the two sources it has to agree with: + * + * - the SQL literal, parsed out of the migration rather than restated here — restating it would just + * move the drift into this file; + * - Better Auth's own exports, which are what actually key the row at sign-in. + * + * The upstream leg is the one with a future in it: if a Better Auth release changes google's issuer, or + * gives github one it does not have today, this fails at `pnpm test` instead of at somebody's login. + */ +const MIGRATION_DIR = join( + dirname(fileURLToPath(import.meta.url)), + "../../../../../../packages/database/migration" +); + +/** + * The two SQL spellings of the canonical mapping. The ENG-2343 backfill has one `CASE`; the ENG-2555 + * repair restates it twice (`SET` and the self-excluding `WHERE`). All copies in all files must agree + * with each other and with `canonicalAccountIssuer` — a repair that drifts would *un-fix* rows through + * the migration meant to cure them. + */ +const ISSUER_MIGRATION_SOURCES = { + "eng-2343 backfill": "20260812110000_eng_2343_better_auth_17_resource_model/migration.sql", + "eng-2555 repair": "20260821165535_repair_account_issuer/migration.ts", +} as const; + +interface TParsedIssuerCase { + arms: Record; + elseTemplate: string; +} + +/** + * Every `"issuer" = CASE … END` block in a migration file, as `{ provider: issuer }` arms plus the + * `ELSE` template. Parsed, not transcribed — restating the values here would just move the drift into + * this file. + */ +const parseIssuerCases = (relativePath: string): TParsedIssuerCase[] => { + const source = readFileSync(join(MIGRATION_DIR, relativePath), "utf8"); + const blocks = [...source.matchAll(/"issuer" (?:= CASE|IS DISTINCT FROM \(\s*CASE)([\s\S]*?)END/g)]; + if (blocks.length === 0) throw new Error(`no issuer CASE found in ${relativePath} — moved or renamed?`); + + return blocks.map(([, body]) => { + const arms: Record = {}; + for (const [, provider, issuer] of body.matchAll(/WHEN "provider" = '([^']+)' THEN '([^']+)'/g)) { + arms[provider] = issuer; + } + const elseArm = /ELSE '([^']+)' \|\| "provider"/.exec(body); + if (!elseArm) throw new Error(`issuer CASE in ${relativePath} has no ELSE arm`); + return { arms, elseTemplate: elseArm[1] }; + }); +}; + +const parsedSources = Object.entries(ISSUER_MIGRATION_SOURCES).map(([label, path]) => ({ + label, + cases: parseIssuerCases(path), +})); + +// One canonical parse for the per-arm assertions below; the cross-copy equality tests prove every +// other copy is identical to it, so asserting against one is asserting against all. +const { arms, elseTemplate } = parsedSources[0].cases[0]; + +describe("canonicalAccountIssuer ↔ every SQL spelling of the mapping", () => { + // Guard the guard: if the regex silently matched nothing, every assertion below would pass against an + // empty object and prove precisely nothing. The repair file must contain exactly two copies (SET and + // the self-excluding WHERE) — a refactor that drops one would weaken its idempotency, and this is + // what notices. + test("both migrations parsed, with the expected number of CASE copies", () => { + expect(parsedSources.map(({ label, cases }) => [label, cases.length])).toEqual([ + ["eng-2343 backfill", 1], + ["eng-2555 repair", 2], + ]); + expect(Object.keys(arms).sort()).toEqual(["credential", "google"]); + expect(elseTemplate).toBe("local:oauth:"); + }); + + // The drift that shipped ENG-2555 was two spellings of this mapping disagreeing. Every copy in every + // migration must therefore be byte-equal to every other — including the repair's SET and WHERE pair, + // which could otherwise drift apart and make the repair non-idempotent. + test.each( + parsedSources.flatMap(({ label, cases }) => cases.map((c, i) => [`${label} copy ${i + 1}`, c] as const)) + )("%s is identical to the canonical parse", (_label, parsed) => { + expect(parsed.arms).toEqual(arms); + expect(parsed.elseTemplate).toBe(elseTemplate); + }); + + test.each(Object.entries(arms))("agrees with the SQL for %s", (provider, expected) => { + expect(canonicalAccountIssuer(provider)).toBe(expected); + }); + + test.each(["github", "azuread", "openid", "saml"])("agrees with the SQL ELSE arm for %s", (provider) => { + expect(canonicalAccountIssuer(provider)).toBe(`${elseTemplate}${provider}`); + }); + + /** + * Documents the one input class where the TS and SQL sides deliberately disagree: the SQL ELSE arm + * concatenates the raw provider id, while the helper percent-encodes. Identity for every provider id + * in use (all encoding-neutral) — but a future provider id carrying a reserved character would make + * the migration write a value the app never looks up. This pins the divergence as known and + * deliberate rather than letting it look like an oversight; enabling such a provider means adding an + * explicit arm to the SQL, per the ENG-2343 migration's own comment. + */ + test("the SQL ELSE arm cannot express an encoded provider id", () => { + expect(canonicalAccountIssuer("team/github")).toBe("local:oauth:team%2Fgithub"); + expect(canonicalAccountIssuer("team/github")).not.toBe(`${elseTemplate}team/github`); + }); +}); + +describe("canonicalAccountIssuer ↔ Better Auth's own account key", () => { + const providerOptions = { clientId: "pin-test", clientSecret: "pin-test" }; + + /** + * How upstream resolves the issuer (`better-auth/dist/oauth2/account-key.mjs`): a provider's declared + * `accountIssuer` wins, and only its absence falls back to the synthetic form. + */ + test("google declares its own issuer, and we use that rather than the synthetic form", () => { + const declared = google(providerOptions).accountIssuer; + + expect(declared).toBe("https://accounts.google.com"); + expect(canonicalAccountIssuer("google")).toBe(declared); + // The regression itself: this is the value that was written, and it is not the one BA looks under. + expect(canonicalAccountIssuer("google")).not.toBe(ssoAccountIssuer("google")); + }); + + /** + * Pins the *assumption* behind the fallback, not just the fallback. If a future release gives github a + * declared issuer, `local:oauth:github` silently becomes wrong for it in exactly the way it was wrong + * for google — and this is what notices. + */ + test("github declares none, so the synthetic fallback is the right answer for it", () => { + // `in` rather than reading the property: upstream's type for github has no `accountIssuer` at all, + // so this is checked at compile time as well as here. If a release adds one, the property appears + // and this fails — which is the notification we want. + expect("accountIssuer" in github(providerOptions)).toBe(false); + expect(canonicalAccountIssuer("github")).toBe(createOAuthAccountIssuer("github")); + }); + + test("credential matches Better Auth's local account issuer", () => { + expect(canonicalAccountIssuer("credential")).toBe(createLocalAccountIssuer("credential")); + }); +}); + +describe("ssoAccountIssuer stays the pinning helper", () => { + // It is still correct for the generic providers we pin `accountIssuer` on, and this records that the + // two helpers are deliberately different rather than one being a leftover. + test.each(["azuread", "openid", "saml"])("%s keeps the synthetic form", (provider) => { + expect(ssoAccountIssuer(provider)).toBe(`local:oauth:${provider}`); + expect(canonicalAccountIssuer(provider)).toBe(ssoAccountIssuer(provider)); + }); + + test("percent-encodes a provider id that needs it", () => { + expect(ssoAccountIssuer("team/github")).toBe("local:oauth:team%2Fgithub"); + }); +}); diff --git a/apps/web/modules/ee/sso/lib/constants.ts b/apps/web/modules/ee/sso/lib/constants.ts index c318ec27ca4f..41eaeaca24e3 100644 --- a/apps/web/modules/ee/sso/lib/constants.ts +++ b/apps/web/modules/ee/sso/lib/constants.ts @@ -2,20 +2,58 @@ export const OAUTH_ACCOUNT_NOT_LINKED_ERROR = "OAuthAccountNotLinked"; export const SSO_RECOVERY_COMPLETION_PATH = "/api/auth/sso/recovery/complete"; /** - * The synthetic `Account.issuer` for our generic-OAuth providers (ENG-2343). + * The synthetic `Account.issuer` for the providers we configure ourselves (ENG-2343). * - * Lives here, in a dependency-free module, because THREE places must produce byte-identical values and - * a literal repeated three times is a silent-divergence bug waiting to happen: + * This is the value we PIN via `accountIssuer` on the generic-OAuth providers in + * `better-auth-providers.ts` (azuread / openid / saml). Because it is pinned, Better Auth stores and + * looks up exactly what we hand it, so upstream's own format never enters the picture — which is why + * this is deliberately NOT `createOAuthAccountIssuer` from `@better-auth/core/db` even though the two + * are currently identical. Tracking upstream here would drift us away from rows already written, and + * the SQL backfill could not follow. * - * 1. `better-auth-providers.ts` — `accountIssuer`, what Better Auth writes and looks up. - * 2. `account-linking.ts` — the rows SSO recovery writes itself. - * 3. `migration/20260812110000_…` — the backfill, as a SQL literal (`'local:oauth:' || "provider"`), - * which cannot call TypeScript and is therefore the copy this one has to match. - * - * Deliberately NOT `createOAuthAccountIssuer` from `@better-auth/core/db`, even though it is public and - * currently identical: because `accountIssuer` is set explicitly, Better Auth stores and looks up - * whatever we hand it, so upstream's format never enters the picture — while the SQL literal in (3) - * cannot follow an upstream change. Tracking upstream would drift us away from rows already written. + * It is NOT the answer to "what issuer does an existing row for provider X have" — a built-in social + * provider can declare its own. Use `canonicalAccountIssuer` for that (ENG-2555). */ export const ssoAccountIssuer = (providerId: string): string => `local:oauth:${encodeURIComponent(providerId)}`; + +/** + * The canonical `Account.issuer` for a given `Account.provider` — the value Better Auth 1.7 actually + * keys the row on, and therefore the only value a write may use (ENG-2555). + * + * 1.7 keys accounts on `(issuer, accountId)` and filters every lookup on it, so a row written with the + * wrong issuer is invisible to sign-in. Google is the trap: it is a BUILT-IN social provider that + * declares its own `accountIssuer` upstream, so the synthetic `local:oauth:` form is wrong for it. + * Writing `local:oauth:google` is what broke Google sign-in on 5.4-rc — the link was created, the user + * got a session, and every subsequent sign-in bounced back through verify-before-link forever. + * + * This mirrors the `CASE` in `migration/20260812110000_…/migration.sql` — which got google right, and + * whose comment already warned that "getting google wrong would leave every existing Google user + * unmatched at sign-in". One deliberate asymmetry: the SQL `ELSE` concatenates the raw provider id + * while this helper percent-encodes it. Identity for every provider id in use (all encoding-neutral, + * per that migration's own comment); a provider id ever needing escaping must get an explicit `CASE` + * arm in SQL, and `constants.test.ts` documents the divergence so it reads as known, not an + * oversight. Four sites must agree and cannot import each other: + * + * 1. Better Auth itself — `provider.accountIssuer`, else `createOAuthAccountIssuer(provider.id)`. + * 2. `account-linking.ts` — the rows SSO recovery writes. + * 3. `migration/20260812110000_…` — the backfill, as a SQL literal. + * 4. `migration/20260821…_repair_account_issuer` — the repair for rows (2) got wrong. + * + * `constants.test.ts` pins this function against upstream's own exports AND every SQL copy in both + * migrations (the backfill's one, the repair's two), so a future Better Auth release that changes + * google's issuer — or gives github one — or any one SQL copy drifting fails `pnpm test` rather than + * production sign-in. + * + * Keyed on `Account.provider`, NOT `IdentityProvider`: the credential row's provider is `"credential"`, + * a value that enum does not contain. + */ +export const canonicalAccountIssuer = (provider: string): string => { + // Better Auth's own `createLocalAccountIssuer("credential")`. Unreachable from the SSO recovery + // caller (its provider is an `IdentityProvider`, which has no `credential` member) but kept so this + // stays a faithful mirror of the SQL, and so a future non-SSO caller cannot get it wrong. + if (provider === "credential") return "local:credential"; + // Declared upstream in `@better-auth/core/dist/social-providers/google.mjs`. + if (provider === "google") return "https://accounts.google.com"; + return ssoAccountIssuer(provider); +}; diff --git a/apps/web/modules/survey/editor/components/add-element-button.tsx b/apps/web/modules/survey/editor/components/add-element-button.tsx index ffe092c74beb..1e44becb40bf 100644 --- a/apps/web/modules/survey/editor/components/add-element-button.tsx +++ b/apps/web/modules/survey/editor/components/add-element-button.tsx @@ -92,7 +92,12 @@ export const AddElementButton = ({ addElement, workspace, isCxMode }: AddElement open ? "shadow-lg" : "shadow-md", "group w-full overflow-hidden rounded-lg border border-slate-300 bg-white duration-200 hover:cursor-pointer hover:bg-slate-50" )}> - + {/* Not `asChild` with a `div`: Radix forwards the click handler and aria state to the child but + adds no role and no tabIndex, so the control was unreachable by keyboard — a user could not + open the element picker at all. Letting Radix render its own `