Repository navigation
Conversation
…as and the audit trail A second audit sweep, aimed away from the ground the previous eight already burnt: six subsystems none of them touched (authentication, MCP, email, the embed SDK, first-run setup, audit logs) and six cross-cutting lenses rather than subsystems. 42 candidates, 30 confirmed by two independent refuters, 12 refuted and dropped. Nothing came back critical, which is the honest result after eight sweeps. The ones worth reading: - The unauthenticated link-survey email action took `email: z.string()`, and nodemailer parses a string `to` as an address list, so one request mailed every address in a comma-separated string from the operator's own relay. The same action let the caller supply the survey name the mail renders, so the attacker wrote the copy as well. Rate limiting counted requests, not recipients. - `sendToPipeline` retried its enqueue three times with no job id, so one Valkey blip between the Lua script running and the reply arriving doubled every side effect of a submission: two follow-up emails, two notification emails, a duplicate row in every connected sheet, two POSTs to every webhook. The idempotency seam already existed and no caller used it. - Quota admission counted, decided in JS, then inserted, under Read Committed with nothing serialising it, so concurrent responses both passed a full quota and it committed over its limit. - Three embed-SDK lifecycle defects left `isSurveyRunning` out of step with reality: a blocked reCAPTCHA script wedged it true for the whole page session, a fired timeout was never pruned so an unrelated route change released it under a live survey, and a `setUserId` did not cancel a pending survey, which then rendered for and was attributed to the next identity. - Audit events for teams, 2FA and contact attribute keys recorded `target.id: "unknown"`, and an invite role change recorded no change at all — so an escalation from member to owner left no trace. The rest are bounded here: a Notion OAuth callback that stored the error body as credentials, an Airtable refresh whose safety comment had been falsified six hours earlier, three tests that could not fail, a Stripe path that could give a paying organization a second customer, and two zod/Prisma drifts.
`@forma/database#test` declared no dependency on `@forma/database#build`, while `@forma/web#test` does — so `turbo run test` was free to run them at the same time. The build's script starts with `clean-generated-prisma.mjs`, which deletes `generated/prisma/` before Prisma rewrites it, so a concurrent database test could import `generated/prisma/client.ts` in the window where the entry exists and `internal/prismaNamespace.ts` does not. Latent until this branch: nothing under `packages/database` loaded the generated client, because the only tests there were scripts and the adapter. The two new zod schema tests do — `zod/users.ts` reads `OrganizationRole` at runtime — which is what turned a dormant graph gap into a red CI job that no local run reproduces.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No ticket — a second repository-wide audit, aimed away from the ground the previous eight sweeps already covered.
What & why
Was: one request to the public link-survey email action could mail an attacker-chosen list of addresses, with attacker-written copy, from the operator's own relay; a single Valkey blip doubled every side effect of a submission; quotas committed over their limit under concurrency; a blocked reCAPTCHA script stopped the embed SDK showing any survey for the rest of the page session.
Now: each of those is closed, with a test that fails on
main.Six unswept subsystems (auth, MCP, email, embed SDK, first-run setup, audit logs) and six cross-cutting lenses instead of a subsystem split. 42 candidates → 30 confirmed by two independent refuters, 12 refuted and dropped; two of the 30 were the same defect, so 29 fixes. Nothing came back critical, which is the honest result after eight prior sweeps.
Where to look
packages/types/email.ts+link/actions.ts— the mail-amplification pairlib/pipelines.ts— deterministic job id; runs outside the retry loop'stry, so it must not throwquotas/lib/utils.ts— the only raw SQL in the diff;ORDER BY "id"is the deadlock guardturbo.json— a graph gap this branch tripped over, explained belowBreaking changes
None. The one externally-visible tightening is that the link-survey email action now rejects a non-email
emailfield with a validation error; the shipped client already validated it withz.email(), so no real caller changes.Migrations & env
How this was tested
Rerun:
pnpm format:check && pnpm lint && pnpm typecheck && pnpm testCoverage
packages/types/email.test.ts; list-parsing confirmed against the vendored nodemailerlink/actions.test.tslib/pipelines.test.tsFOR UPDATEinquotas/lib/utils.ts→ redaudit-logs/lib/handler.test.ts.tsxand comment-only fixes verified by readingFull run
typecheck27/27 ·lint0 errors + all four repo gates ·test28/28 tasks, 9053 passed in@forma/web, 706 files.Production diff is 776 lines; tests are 1393 and are excluded from the size check.
Open gaps
?error=it redirects with, so the user still sees a bare "not connected".deleteInviteAction's audit snapshot gainedrolein its select, but its test mocks Prisma, so nothing pins it.docs/api-v2-reference/openapi.ymlis stale on the two zod drifts. It was already stale onmain, no CI gate reads it, and it wants a centralgenerate-api-specsrun rather than a hand edit here.One defect this branch found the hard way
@forma/database#testdeclared no dependency on@forma/database#build, while@forma/web#testdoes, soturbo run testcould run them together. The build's script begins by deletinggenerated/prisma/, so a concurrent database test could importclient.tsin the window beforeinternal/prismaNamespace.tswas rewritten. Dormant until now — nothing underpackages/databaseloaded the generated client until the two new zod tests, one of which readsOrganizationRoleat runtime. It reproduced only on CI, never locally, including after a clean regenerate.Risks
SurveyQuota, no@@map), empty-array guarded, ordered by id.del, and releasing a lock you may no longer hold admits a third refresher. It expires instead; the comment says so.getEnqueueJobIdoptional-chainsjob.response, because it runs outside the retry loop'stryand that function must never throw.Note
AI model used —
claude-opus-5[1m](Claude Code), reasoning effortunknown.