The guard is `{ id: { in: [] } }`, which is only safe because Prisma
renders an empty IN list as `WHERE 1=0` instead of dropping the filter.
A regression there would delete every workspace on a self-hosted
deployment, which is too expensive to leave resting on that assumption.
The expired-owner filter expressed "no billing access" as
NOT: buildBillingAccessWhereInput(now). Prisma renders that as
NOT (status IN ('ACTIVE','TRIALING') OR "trialEndsAt" > $1 OR
"stripeCurrentPeriodEnd" > $2), and a SQL comparison against NULL is
unknown rather than false, so for a row with both dates empty the OR is
NULL and NOT NULL is NULL: the row is never returned.
Both dates empty is exactly what a canceled subscriber looks like, since
markSubscriptionCanceledByCustomerId clears trialEndsAt and Stripe no
longer reports current_period_end on the subscription. The scheduled
cleanup therefore matched nobody at all while reporting success, and
media of owners fifteen days past their grace period stayed in Bunny and
R2 indefinitely.
Each branch now names NULL explicitly. Disabling Stripe also selects
nobody instead of falling through to NOT {}, which Prisma drops
entirely: that left a filter keyed on the grace period alone, so a
self-hosted deployment running the cleanup would delete workspaces of
users it never charged.
The unit tests could not catch this, because as an object the old filter
reads correctly and no SQL is produced. The new coverage lives in
tests/api and runs against Postgres.
The suite that landed in #43/#44 was written against existing behaviour, so a
number of tests pinned bugs rather than asserting correct behaviour. This fixes
the production code and moves each of those tests onto the fixed behaviour in
the same change.
Security:
- project-download: derive the archive entry extension from the last path
segment and restrict it to a short alphanumeric run, so an extensionless
allowlisted url can no longer contribute a path separator; validate the r2
branch against the strict proxy-path pattern instead of a `startsWith`, which
let `/api/upload/video/clip.mp4/../../etc/passwd` through verbatim.
- rate-limit: hash a key or action wider than its column instead of skipping the
query. Both the guard and the failing INSERT used to answer "allowed", so the
limit stopped applying entirely. Warn at startup when TRUSTED_PROXY_MODE is
unset in production.
- video uploads: the file name decides the content type; a client-declared video
mime no longer makes `payload.exe` acceptable.
- email templates: escape in the helpers rather than relying on every caller,
with an explicit `rawEmailHtml()` opt-out for the one call site that builds
markup. `escapeHtml` now covers the single quote.
- CSP: allow loopback object storage outside production only.
- route-access: reach the billing redirect only for the workspace owner. Keying
it off the owner's billing status alone made the redirect target an oracle for
whose subscription had lapsed, and sent members to a page they cannot act on.
- search: carry the same billing condition every other read path carries.
- logger: check `err.name` as well as `err.constructor.name`, so a re-thrown,
deserialised or minified Prisma error is still redacted.
- upload tokens: resolve the signing secret outside the try, so a server booted
without one fails loudly instead of reporting every grant as a forgery.
- invitations: never downgrade an existing membership, and report a scoped
invitation that points at nothing as not_found rather than accepted.
- auth: resolve the workspace role for every signed-in caller, so
checkProjectAccess and computeProjectAccess stop disagreeing about the owner
who also owns the workspace. The `intent` option is gone with it.
- r2-media-proxy: validate the object key inside the proxy so the guard travels
with the function; delete the unused, unanchored `mediaUrlToR2Key`.
- r2: sign the content type into presigned PUT grants.
Correctness:
- frame rate snapping picks the nearest standard, not the first within
tolerance, so 24, 30 and 60 fps are reachable at all.
- a version upload registers its Bunny cleanup as soon as bunny-init answers, so
a failed tus upload no longer leaves a billed video behind.
- deleting videos clears storage before the rows, so a refused DELETE leaves a
retryable row rather than an orphaned object.
- an expired upload session can be cancelled, which is what releases its quota.
- `voice/` joins the delete allowlist, so a voice note can be removed by the
module that wrote it.
- a failed CORS write propagates instead of being mistaken for an empty config
and replacing the bucket's rules.
- filtering projects by workspace no longer hides projects the unfiltered call
returns.
- upload retries skip aborts and permanent 4xx; progress no longer divides by
zero.
- reply edits no longer clear the comment's tag; optimistic resolve rolls back
to the state it replaced; the delete snapshot is captured once.
- assorted UI fixes: duplicate React keys, double-click guards reading stale
closures, the tag list fetched twice per load, a failed member list rendering
as an empty one, a stale "Initializing upload..." beside a failure, and a
registration banner pointing at an email that never arrives.
Consistency and access:
- the two download routes answer 404 for an id belonging to another tenant, as
the comment export route already did. A caller who does belong still gets 403.
- accessible names for the share-link password field, the guest name gates, the
version dialog inputs and the comment-tag controls.
Repository health:
- the runner image installs production dependencies only.
- a setup file for the unit project restores stubbed env centrally.
- native tsconfig path resolution replaces vite-tsconfig-paths.
- `uploadBytesWithProgress` exists once.
- admin stats bill Bunny storage to the workspace owner like every other
quota, gate on the configured flag, wire up the single-flight guard and count
the statuses that belonged to no bucket.
- `r2Client.destroy()` releases the presign client too.
- `prepare` tolerates a production install, where husky is absent.
Node ships an experimental Web Storage global now, which evaluates to
`undefined` unless the process was started with `--localstorage-file`. Vitest
leaves an already-present global alone when it copies jsdom's window onto
globalThis, so jsdom's own localStorage never lands and Node's empty one wins.
sessionStorage has no counterpart in Node and comes through untouched, which is
what makes the asymmetry visible.
Every test in guest-gate.test.tsx therefore failed on `localStorage.clear()` on
a developer machine, while CI stayed green on its pinned Node 22 and the
container stayed green with no node at all. That is also why the branch had to
be pushed with --no-verify once: the pre-push hook runs on the host.
The in-memory stand-in only installs when nothing else provides localStorage,
so where jsdom's implementation is in scope it is left alone.
bun loads a plain `.env` into process.env before anything runs, and
tests/helpers/env.ts read that as a deliberate export, so it beat `.env.test`
outright. `scripts/test.sh api` therefore pointed the api suites at whatever
deployment `.env` describes: `prisma db push --accept-data-loss` for the
schema, then a truncate of every table between tests. The e2e half was worse,
because playwright.config.ts built and started the app with that DATABASE_URL
and those R2 credentials, then wrote fixtures into it. CI never saw any of
this: a runner has no `.env`.
Three changes, in order of what each one catches:
- helpers/dev-env.ts drops the values bun copied out of a development env file,
leaving `.env.test` to fill them. Only values that match the file
character-for-character go, so a real export still wins and the per-suite
databases of a parallel api run keep working.
- helpers/test-database.ts refuses a DATABASE_URL whose database name is not
marked as a test one, at the single point every path into the setup passes
through. This is the backstop, not the fix.
- playwright.config.ts blanks the variables a development env file defines and
the config does not. Dropping them from process.env is not enough there:
`next build` and `next start` run @next/env themselves and read the files
again. That is also why a local e2e run could not build at all (a set
DISABLE_RATE_LIMIT throws in lib/rate-limit.ts under NODE_ENV=production) and
why auth.spec.ts failed on a machine with SMTP configured.
`scripts/test.sh` now creates `.env.test` from the committed example instead of
asking for a one-line copy, so the guard above is something nobody has to meet.
`.dockerignore` excludes `tests`, so the production build context carries
scripts/test-db-bootstrap.ts without the tests/setup/db-global module it
imports. tsconfig includes `**/*.ts`, so the `prebuild` typecheck fails on the
missing module and every deploy since the test suites landed has died there.
CI never saw it because tests/ exists on a runner.
The script is test-only, so it belongs in the tree that is already ignored.
`failure-recovery.spec.ts` hardcoded `http://minio-test:9000/**`, which is the
compose hostname. CI publishes MinIO on localhost, so the pattern matched
nothing there: the PUT went through, the upload succeeded, and the test sat
waiting for an error message that was never going to appear. It passed locally
and failed on CI for a reason the diagnostic did not name.
The glob now comes from R2_ENDPOINT, and the test counts the PUTs it actually
refused and asserts the count is not zero. A pattern that matches nothing is now
a failure that says so, rather than a failure that blames the error message.
Recorded in AGENTS.md as the third way a test can be worthless, alongside a
note to run a new spec under CI conditions and not only locally.
Second pass over the suite, driven by the inventory in the gaps document. Nine
agents wrote suites in parallel against private databases, then a tenth read all
of it adversarially and five of its findings were fixed.
unit + component 2076 -> 2079 (+888 over the round)
api 647 -> 1015
e2e 18 -> 29
What was closed:
- lib/route-access.ts, the page-level authorization layer, went from zero tests
to 48. Every API route was guarded and none of the pages were.
- The five media proxy routes now have a real 2xx beside every 403. The blocker
was the positive control, solved by stubbing r2Client.send() and leaving
lib/r2-media-proxy.ts itself real.
- Every remaining server-side lib module: invitations, email verification, the
upload tokens, the logger, request origin, the whole R2 and Bunny lifecycle,
notifications and admin stats.
- Six video-page hooks, and the chunking arithmetic extracted out of
lib/client/r2-video-upload.ts as a pure module.
- Five end-to-end flows: workspace members, bulk operations, the admin area,
player interaction and failure recovery.
Three things about the harness itself turned out to be wrong:
- Two @/lib/r2 stubs in tests/setup/api.ts had the wrong return shape, so every
route reaching finalizeR2VideoUpload silently took the "not a valid video"
branch and no test noticed.
- The auth matrix asserted only "not 2xx", which two entries satisfied without
their guard existing. It now requires 401 or 403, which makes both
load-bearing, and all 60 routes pass the stricter form.
- Both admin API routes had no positive control anywhere: replacing their guard
with an unconditional refusal left the entire suite green. Found by the
adversarial review, now covered.
Process:
- bun run test:mutation runs StrykerJS over the authorization and validation
modules. Diagnostic, not a gate, weekly in CI rather than on a push.
- playwright.config.ts gains an opt-in webkit project for the player spec.
- AGENTS.md now requires a batch of new tests to be reviewed by somebody who
did not write them.
Only two production files change, both deliberate: lib/auth.ts loses a verbatim
copy of its own permission formulas, and lib/client/r2-video-upload.ts calls the
extracted arithmetic. No behaviour change in either.
The login page derives callbackUrl from its own default when the
parameter is absent, and on CI it arrives as
/login?registered=true&callbackUrl=%2Fdashboard. Anchoring the pattern
with $ made that a deterministic CI-only failure while the suite passed
locally on every run, including with CI=1.
What the register flow promises is the login page plus registered=true.
The rest of the query string is not part of that contract, so the
pattern now tolerates extra parameters in any order.
The page snapshot that diagnosed this also turned up a product bug,
recorded in the findings notes rather than fixed here: the success
banner tells every new user to check their email for a verification
link, including in the self-hosted default where SMTP is unset, email
verification is off, and the account is already usable.
The repo had no automated tests. Every change was verified by hand.
Adds four layers, 2023 tests in total, runnable with one command:
- 1191 unit tests over the pure logic in lib/, including the full
computeProjectAccess permission matrix and the billing gate
- 167 component and hook tests in jsdom, covering the hooks that hold
real logic rather than presentational wrappers
- 647 API integration tests against a real Postgres, with only auth()
mocked, including a data-driven sweep asserting that none of the 60
route modules answers 2xx to an unauthenticated caller
- 18 Playwright specs driving a real browser against a real build
Infrastructure: vitest.config.ts with three projects, a disposable
Postgres and MinIO in docker-compose.test.yml, factories and helpers
under tests/, scripts/test.sh as the single entry point, a pre-push
hook running bun run verify, and CI split into check, test and e2e jobs.
The test database is built with prisma db push plus a replay of the
hand-written SQL, because prisma migrate deploy cannot build this schema
from empty: the migration history has no captured baseline. This mirrors
what scripts/docker-db-bootstrap.ts already does in production, and
tests/setup/db-global.ts carries a drift guard so a new migration fails
the run until someone reviews it.
Production code is unchanged apart from one pure-function extraction out
of use-video-player.ts, which was too large to test in jsdom.
Several tests pin behaviour that looks wrong, each marked KNOWN BUG in
place. TESTING.md section 12 records where the plan turned out to be
wrong, and AGENTS.md now states which layer a change needs a test in.
yusufipk