From 9c30ce9d3e7e4a4aaef379d1f4deb8e155f4fa87 Mon Sep 17 00:00:00 2001 From: Javi Aguilar <122741+itsjavi@users.noreply.github.com> Date: Tue, 25 Aug 2026 05:27:59 +0000 Subject: [PATCH 1/4] docs(agents): tell agents when an e2e test is warranted (#8938) Co-authored-by: Claude --- .coderabbit.yaml | 43 ++++++++--- .github/pull_request_template.md | 6 ++ AGENTS.md | 73 +++++++++++++++---- .../standards/qa/testing-methodology.mdx | 53 ++++++++++---- 4 files changed, 134 insertions(+), 41 deletions(-) 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/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/docs/development/standards/qa/testing-methodology.mdx b/docs/development/standards/qa/testing-methodology.mdx index 861eb16ab54f..56ecab66b69d 100644 --- a/docs/development/standards/qa/testing-methodology.mdx +++ b/docs/development/standards/qa/testing-methodology.mdx @@ -34,9 +34,13 @@ We use Vitest as our primary testing framework. All unit tests follow these conv }); ``` -3. **Coverage Requirements** - - Minimum 85% code coverage requirement - - Coverage is tracked using V8 provider +3. **Coverage** + - Coverage is measured, not targeted: confidence over coverage, and no test exists to move the + number (see `AGENTS.md`, "Testing Guidelines") + - The repo enforces exactly one Vitest threshold: `modules/auth/lib/**` at 80% + (`apps/web/vite.config.mts`). Everything else is reported — `pnpm test:coverage` emits lcov and + the SonarQube workflow uploads it, with the quality gate configured in SonarQube, not here + - Coverage is tracked using the V8 provider - Coverage reports include: - Text summaries - HTML reports @@ -44,25 +48,40 @@ We use Vitest as our primary testing framework. All unit tests follow these conv ### End-to-End Testing with Playwright -E2E tests are located in `apps/web/playwright/` and focus on critical user workflows. +E2E tests are located in `apps/web/playwright/` and focus on critical user workflows. `AGENTS.md` is the +source of truth for when one is warranted; the short version: + +1. **Level.** Prove a behavior at the cheapest level that can fail on it. A feature's happy path or a + journey across several surfaces is E2E. Business logic, invariants and anything pure is a unit test on + the `.ts`. A route's authorization, response shape or query scoping is a unit or integration test on + that route. +2. **Unit of coverage.** One spec per feature area, not per ticket and not per component. Every feature + area ships a happy-path spec, and a new spec file is for an area that has none. Inside an area that + already has one, the level still follows the behavior: journey behavior extends that spec, logic goes + to a `.ts` unit test, UI detail goes to manual QA (see 3) — never a second spec file. +3. **Neither is an option.** A granular UI detail inside a covered feature — a label, a breadcrumb, an + ARIA attribute, a keystroke inside one widget, a field's validation message — is verified manually and + recorded in the PR. It does not get a spec. +4. **Cost.** Every spec is paid on every PR by everyone, forever. The Playwright job is the critical path + of the PR gate and its wall clock can never drop below its slowest single test, so a new spec has to + buy risk coverage that nothing cheaper can. ## Testing Setup ### Configuration -Our Vitest configuration (`vite.config.ts`) includes: +The web app's Vitest configuration (`apps/web/vite.config.mts`) includes: ```typescript test: { -exclude: ['playwright/', 'node_modules/'], -setupFiles: ['../../packages/lib/vitestSetup.ts'], -coverage: { -provider: 'v8', -reporter: ['text', 'html', 'lcov'], -reportsDirectory: './coverage', -}, + exclude: ["playwright/**", "node_modules/**", ".next/**", "**/*.integration.test.ts"], + setupFiles: ["./vitestSetup.ts"], + coverage: { + provider: "v8", + reporter: ["text", "html", "lcov"], + reportsDirectory: "./coverage", + }, } - ``` ### Test Utilities @@ -124,6 +143,8 @@ Common test utilities are available in `vitestSetup.ts`: - Test results reporting 2. **New Features** - - Must include corresponding unit tests - - Must maintain or improve coverage metrics - - Must include relevant E2E tests for user-facing features + - Must include unit tests for the logic they add + - A new user-facing feature area must include one happy-path E2E spec. Inside an area that already + has one, the level follows the behavior: journey behavior extends that spec, logic goes to a unit + test, UI detail is verified manually + - Must not add tests whose only purpose is to move a coverage metric From abf1b0c2a97b4af82eb96233da8d2d5f17f11202 Mon Sep 17 00:00:00 2001 From: Tiago <1585571+xernobyl@users.noreply.github.com> Date: Tue, 25 Aug 2026 05:37:53 +0000 Subject: [PATCH 2/4] fix(sso): write the canonical Account.issuer so Google sign-in stops looping (#8951) --- .../credential-backfill.integration.test.ts | 28 ++- .../repair-account-issuer.integration.test.ts | 114 ++++++++++++ .../ee/sso/lib/account-linking.test.ts | 62 ++++++- .../web/modules/ee/sso/lib/account-linking.ts | 33 ++-- ...r-auth-recovery-signin.integration.test.ts | 76 ++++++++ apps/web/modules/ee/sso/lib/constants.test.ts | 165 ++++++++++++++++++ apps/web/modules/ee/sso/lib/constants.ts | 62 +++++-- .../migration.ts | 64 +++++++ 8 files changed, 566 insertions(+), 38 deletions(-) create mode 100644 apps/web/integration/repair-account-issuer.integration.test.ts create mode 100644 apps/web/modules/ee/sso/lib/constants.test.ts create mode 100644 packages/database/migration/20260821165535_repair_account_issuer/migration.ts 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/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/packages/database/migration/20260821165535_repair_account_issuer/migration.ts b/packages/database/migration/20260821165535_repair_account_issuer/migration.ts new file mode 100644 index 000000000000..1a588d827343 --- /dev/null +++ b/packages/database/migration/20260821165535_repair_account_issuer/migration.ts @@ -0,0 +1,64 @@ +import { logger } from "@formbricks/logger"; +import type { MigrationScript } from "../../src/scripts/migration-runner"; + +/** + * Repair `Account.issuer` rows that disagree with the canonical value for their provider (ENG-2555). + * + * The ENG-2343 backfill (20260812110000) set this correctly for every row that existed when it ran. What + * it could not cover is rows written AFTERWARDS by SSO recovery, which used a helper that returned the + * synthetic `local:oauth:` form for every provider — wrong for `google`, which declares its own + * `accountIssuer` upstream (`https://accounts.google.com`). + * + * Better Auth 1.7 keys accounts on `(issuer, accountId)`, so such a row is invisible at sign-in: the user + * is pushed back through verify-before-link on every attempt and never converges. This rewrites them. + * + * The `CASE` is byte-identical to the one in 20260812110000's `migration.sql` — including, + * deliberately, its missing percent-encoding on the `ELSE` arm: the TS helper encodes, the SQL cannot, + * and every provider id in use is encoding-neutral so the two coincide. Mirroring the flaw is the + * point; "fixing" one side is how ENG-2555 happened. `apps/web/modules/ee/sso/lib/constants.test.ts` + * parses every `CASE` copy in BOTH migrations (this file has two — `SET` and the self-excluding + * `WHERE`) and pins them against each other, the helper, and Better Auth's own exports, so no copy can + * drift silently. + * + * Safe by construction: + * - **No-op on an empty or already-correct database.** `IS DISTINCT FROM` matches only rows that are + * actually wrong (and, unlike `<>`, also catches `issuer IS NULL`). Re-running changes nothing. + * - **Cannot merge two identities.** A collision on `@@unique([issuer, providerAccountId])` would need + * two rows sharing a subject and mapping to one issuer. The provider→issuer map is injective and + * `@@unique([provider, providerAccountId])` already forbids two rows per provider+subject, so that is + * unreachable. If the reasoning is ever wrong the unique index aborts the migration, which is the + * outcome we want — no silent merge. + * - Only the `issuer` column is touched; `userId`, `provider` and `providerAccountId` are untouched, so + * no row can move between users or organizations. + */ +export const repairAccountIssuer: MigrationScript = { + type: "data", + id: "ghqt118bfqdjs8tpzv1dvbqi", + name: "20260821165535_repair_account_issuer", + run: async ({ tx }) => { + const repaired = await tx.$executeRaw` + UPDATE "Account" + SET "issuer" = CASE + WHEN "provider" = 'credential' THEN 'local:credential' + WHEN "provider" = 'google' THEN 'https://accounts.google.com' + ELSE 'local:oauth:' || "provider" + END + WHERE "issuer" IS DISTINCT FROM ( + CASE + WHEN "provider" = 'credential' THEN 'local:credential' + WHEN "provider" = 'google' THEN 'https://accounts.google.com' + ELSE 'local:oauth:' || "provider" + END + ) + `; + + if (repaired === 0) { + logger.info("Account.issuer: no rows disagreed with the canonical value; nothing to repair"); + return; + } + + // Worth logging loudly rather than silently: a non-zero count here means those users were stuck in + // the SSO verify-before-link loop until this ran. + logger.info({ repaired }, "Account.issuer: repaired rows that disagreed with the canonical value"); + }, +}; From be50fb9e54ba807fa56f4f3f6e4caf74cba97a54 Mon Sep 17 00:00:00 2001 From: Dhruwang Jariwala <67850763+Dhruwang@users.noreply.github.com> Date: Tue, 25 Aug 2026 07:37:53 +0000 Subject: [PATCH 3/4] perf(ci): halve the e2e test step and drop the dead docker build cache (#8940) --- .github/workflows/docker-build-validation.yml | 11 +- .../editor/components/add-element-button.tsx | 11 +- apps/web/playwright/storage-smoke.spec.ts | 9 +- apps/web/playwright/survey.spec.ts | 134 +--- apps/web/playwright/utils/helper.ts | 577 ++++++++++-------- 5 files changed, 367 insertions(+), 375 deletions(-) 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/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 `