test(e2e): module edit/delete + cross-tenant isolation - #43
Merged
Conversation
The l2 sweep only ever created records, so CrudPage's onUpdate/onDelete paths were untested — the PR #25 "form cannot submit" bug class in the edit dialog, and silent-corruption on update, had zero coverage. New e2e/l2/edit-delete.spec.ts drives both through the real UI on two representative CrudPage modules (risks, policies): open the created row's edit form via a scoped row-edit button, change the title, save, assert the new value in UI and Postgres and the old value gone; then create another, delete it via the row-delete button (accepting the window.confirm), and assert it is gone from UI and Postgres. The CrudPage flow (row action -> SchemaForm defaultValues -> update/delete -> refresh) is shared by all twelve modules, so two cover the mechanism. Product hooks: data-testid row-edit / row-delete on the RisksPage and PoliciesPage row action buttons (icon-only, previously unselectable). Full suite 81 passed + 1 skip; typecheck clean. Note surfaced (not fixed): PoliciesPage logs MISSING_MESSAGE for a status value that is not a known i18n key — an arbitrary status string breaks the row's translated render (same "string column with app-level enum" class as the numeric-500 finding).
…s data The suite ran as a single seeded company, so a dropped companyId filter in any ownership guard was structurally undetectable — the company-ending failure mode for a multi-tenant compliance product had zero coverage. New e2e/l3/cross-tenant.spec.ts provisions a real second tenant (Rival GmbH + an asset) via SQL, then from the authenticated Dev GmbH session attacks Rival's asset over the real tRPC endpoint: asset.list, asset.update and asset.delete by Rival's id. Ground truth is Postgres after the attack: Rival's asset must still exist with its original name. Non-vacuity is asserted so this cannot pass on a 404 or an unauthenticated no-op: asset.list returns the attacker's OWN seeded asset (Customer Database), and the update/delete endpoints answer (not 404). A dropped tenant filter in list/update/delete each fails a distinct assertion. Full suite 82 passed + 1 skip; typecheck clean.
Cross-tenant: add a positive control. The list attack was self-proving (returns the attacker's own asset), but update/delete only asserted the endpoint was not 404 — which cannot tell "tenant .where blocked it" from "the request never reached the resolver" (envelope/validation/activation break, or a 500 from any cause). The same session now updates and deletes its OWN asset first and asserts the DB actually changed, so the mutation path is proven live; a future break that silently stops the request fails here loudly instead of green-lighting the cross-tenant no-op. Edit spec: assert the edited row exists (>= 1) rather than exactly 1, so a reused local server (no per-run DB reset) with a prior run's edited row does not spuriously fail. Persistence is still proven; the old-title-gone check stays in the UI assertion. 6 specs pass; typecheck clean.
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.
Replaces #42 (auto-closed when its stacked base branch merged). Two review-driven coverage specs, now based on main.
Module edit + delete (
e2e/l2/edit-delete.spec.ts)The l2 sweep was create-only, so CrudPage's update/delete paths (the PR #25 bug class in the edit dialog, silent corruption on update) were untested. Drives both through the real UI on risks + policies: open a created row's edit form, change the title, assert the new value in UI and Postgres and the old value gone; then delete via the row button (accepting window.confirm) and assert gone from UI and Postgres. Shared CrudPage flow, so two modules prove the mechanism. Product hooks:
data-testid=row-edit/row-deleteon the two pages' row buttons.Cross-tenant isolation (
e2e/l3/cross-tenant.spec.ts)The highest-severity gap: the suite ran as one company, so a dropped
companyIdfilter in any ownership guard was undetectable. Provisions a real second tenant (Rival GmbH + an asset), then from the Dev GmbH session firesasset.list/update/deleteat Rival's asset over the real tRPC endpoint and asserts in Postgres that Rival's data is untouched. Non-vacuous: the list attack returns Dev GmbH's own seeded asset (proves it ran authenticated) and the mutation endpoints answer (not 404), so a dropped tenant filter in list, update, or delete each fails a distinct assertion.Verified
Full suite 82 passed + 1 skip; typecheck clean; rebased onto main (carries the #40 fix).
Note (not fixed): PoliciesPage logs
MISSING_MESSAGEfor a status value that isn't a known i18n key — an arbitrary status string breaks that row's translated render. Product follow-up.🤖 Generated with Claude Code
https://claude.ai/code/session_01De1NY1HDUJkkwetVKTbYtH