Skip to content

fix(security): gate the write side on entitlements, not on self-issuable rows (#543) - #554

Merged
guillermoscript merged 2 commits into
masterfrom
fix/write-side-rls-543
Jul 26, 2026
Merged

fix(security): gate the write side on entitlements, not on self-issuable rows (#543)#554
guillermoscript merged 2 commits into
masterfrom
fix/write-side-rls-543

Conversation

@guillermoscript

@guillermoscript guillermoscript commented Jul 26, 2026

Copy link
Copy Markdown
Owner

What

Closes #543 (EPIC #540 §1.3).

Adds a course-access predicate to four write-side policies that pinned who a row belonged to but never whether the writer had earned it, replaces the last enrollments join in the checkpoint feature, and gates /api/certificates/issue on entitlements.

Migration: 20260726110000_write_side_entitlement_gates.sql (local only — not applied to cloud).

Why

20260725160000 (#532) stamped the rule on enrollments: "Never gate content, APIs or RLS on the presence of an enrollment row." The generalisation — never let a client assert its own grade, completion, mastery or tenant — was never applied elsewhere.

Table Was Now
lesson_completions INSERT WITH CHECK (auth.uid() = user_id) — any authenticated user could record a completion for any lesson_id routed through lessons to has_course_access
exam_submissions identity + tenant, no access gate — a student with no entitlement could enqueue AI grading on the school's plan budget has_course_access on the exam's course
lesson_checkpoint_attempts identity + checkpoint consistency + access, but score/passed/completed/evaluator_type/attempt_number unconstrained server-write-only (the #538 pattern)
practice_attempts FOR ALL USING (user_id = auth.uid()) — no tenant predicate at all split SELECT/INSERT, active tenant_users row required, score/count CHECKs, source pinned, append-only

practice_attempts.course_id is deliberately not gated (see the second commit). It is an optional, model-supplied label on the MCP tool, not a server-derived value, so an access predicate on it would turn a mislabelled drill into a hard 42501 — and the cross-tenant attack is already closed by the membership check; course_id only scopes item_ratings within the caller's own tenant.

Two of these were more than theoretical:

  • The certificate was mintable. /api/certificates/issue checks auth, tenant and duplicates but never checked access; eligibility falls through to a raw lesson_completions count. Self-issuable completions ⇒ self-issuable credential.
  • practice_attempts crossed tenants. Two SECURITY DEFINER triggers fire on that fully caller-controlled row: handle_practice_attempt_elo writes the tenant-shared item_ratings anchors, and handle_practice_attempt_xp creates a gamification_profiles row for the named tenant — the exact eligibility source rollover_leagues scans, so a non-member surfaced in another school's league standings. 20260716120000's stated rationale ("a student session cannot write shared rating rows from application code") did not hold.

Bonus, same class: the lesson_checkpoints SELECT policy still authorized by joining enrollments. Refunds revoke entitlements and never enrollments, so a refunded student kept reading every enabled checkpoint on the course.

Shape

The three added predicates mirror the #509 content read policies: tenant match AND (staff OR course author OR has_course_access). Teachers hold no entitlement for courses they author, so the staff/author branches are what keep authoring and preview working; students only ever match the access branch. Preview lessons (#426) are deliberately not a branch on lesson_completions — reading a preview is fine, recording progress against an unbought course is the thing being stopped.

lesson_checkpoint_attempts could not be fixed with a predicate: every column but the identity ones is a grading output. The route already held an admin client and its own resolveCourseAccessState gate (#535), so the insert moved there and the authenticated INSERT/UPDATE/DELETE grants were revoked.

How to QA

On a fresh npm run db:reset, with npm run dev running:

npx playwright test tests/playwright/write-side-rls.spec.ts \
                    tests/playwright/checkpoint-access.spec.ts --workers=1

15 tests, one per acceptance criterion: a completion for an unreachable lesson, an exam submission without entitlement, a direct {passed: true, score: 100} checkpoint attempt, a practice_attempts row naming a foreign tenant, a wrong-answer attempt read back from the DB to prove the stored grade is the server's, /api/certificates/issue refused with completions present, and a refunded student reading zero checkpoints. Each negative has a positive control so a refusal can't pass for the wrong reason.

Manual, as student@e2etest.com on lvh.me:3000: complete a lesson, submit an exam, pass a checkpoint — unchanged. As owner@e2etest.com: teacher grading and authoring unchanged.

Test results

Gate Result
npm run typecheck pass
npm run test:unit 370/370 (baseline)
npm run build pass
npx eslint on changed files clean (also cleared 5 pre-existing anys in the certificates route that pre-commit lints whole-file)
#543 specs 15/15
student journey + student-courses/exams/features, certificate-issuance, enrollment-flows, evaluations-security 52 passed, 4 failed (pre-existing)
tenant-isolation 10/10
teacher-content 13/13
teacher-courses 13/13
admin-crud 14 passed, 3 failed (pre-existing)

Every failure above is pre-existing. The 3 admin-crud product-wizard failures (product-creation-wizard testid never renders) reproduce identically on 146a8cfa. The 4 student-side failures are — full-student-journey and certificate-issuance plus two student-features sidebar tests. Verified by re-running them on 146a8cfa with a fresh DB: identical failures. full-student-journey targets course 9999 / lessons 10000-10001, which supabase/seed.sql does not create.

One thing worth knowing for future E2E work: student-exams.spec.ts:58 deletes Alice's entitlements row for course 2001 in its cleanup and never restores it. That leak was harmless while nothing gated on entitlements; now it makes later specs in the same run fail. Not fixed here (out of scope), but it is why a long mixed run shows failures a per-spec run does not.

Screenshots / GIF

None — no user-visible surface changed. The one new user-facing string is the existing accessDenied / accessSuspended 403 shape from #535, reused verbatim by /api/certificates/issue.

Checklist

  • npm run typecheck and npm run test:unit pass
  • npm run build passes
  • Every new tenant-scoped query filters by tenant_id (or the table genuinely has no such column — lesson_completions has none, so the course is reached through lessons)
  • Tested with every relevant role (student / teacher / admin)
  • Loading and error states handled
  • New UI strings added to both messages/en.json and messages/es.json — n/a, no new strings
  • Migration applies cleanly on npm run db:reset; lib/database.types.ts needs no regeneration (policies and constraints only, no schema shape change)

🤖 Generated with Claude Code

https://claude.ai/code/session_011SbrtmdXw6ZwH2UeTCuT2v

…ble rows (#543)

Four write-side policies constrained who a row belonged to but never whether
the writer had earned it, so a client could assert its own completion, grade,
mastery or tenant:

- lesson_completions INSERT was `auth.uid() = user_id` and nothing else — any
  authenticated user could record a completion for any lesson id, and
  /api/certificates/issue counts exactly those rows to mint the school's
  credential. Now gated through `lessons` on has_course_access.
- exam_submissions INSERT had no access predicate, so a student with no
  entitlement (or one past their school's access_cutoff_at) could enqueue AI
  grading on the school's plan budget.
- lesson_checkpoint_attempts is now server-write-only, the #538 pattern: every
  column but the identity ones is a grading output, so no WITH CHECK could
  express the invariant. `{passed: true, score: 100}` satisfied any required
  checkpoint without answering, and rows tagged evaluator_type 'ai' billed the
  whole tenant's monthly AI allowance. The attempt route already held an admin
  client and its own access gate (#535); the insert moves there.
- practice_attempts had no tenant predicate at all, and two SECURITY DEFINER
  triggers fire on that fully caller-controlled row — writing another school's
  shared Elo anchors and creating a gamification profile there, which is the
  eligibility source league rollover scans. INSERT now requires an active
  tenant_users row for the named tenant plus access to the named course, the
  blanket FOR ALL is split so graded history is append-only, and score/count
  columns are range-checked.

Also replaces the last `enrollments` join in the checkpoint feature: the
lesson_checkpoints SELECT policy authorized by enrollment, which no refund
path ever removes, so a refunded student kept reading every enabled
checkpoint. And /api/certificates/issue now resolves course access at the top
of its self-serve branch rather than trusting the completion count.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011SbrtmdXw6ZwH2UeTCuT2v
@guillermoscript guillermoscript added security Security vulnerability or hardening db-migration Requires a Supabase migration labels Jul 26, 2026
…attempts (#543)

`course_id` on the record-practice MCP tool is an optional, model-supplied
label ("Related course, if any" — mcp-server/src/tools/practice.ts:810), not a
value the server derives from anything it verified. Gating the INSERT on it
turned a mislabelled drill into a hard 42501, with no MCP test harness to catch
the regression.

It also bought little. The cross-tenant attack this policy exists to stop —
driving another school's shared Elo anchors and appearing in its league
standings — is closed entirely by the tenant_users membership check;
`course_id` only scopes `item_ratings` rows within the caller's own tenant.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011SbrtmdXw6ZwH2UeTCuT2v
@guillermoscript
guillermoscript marked this pull request as ready for review July 26, 2026 01:51
@guillermoscript
guillermoscript merged commit 50537e6 into master Jul 26, 2026
2 checks passed
guillermoscript added a commit that referenced this pull request Jul 26, 2026
…elative (#541)

The two ledger checks compare cloud against the migrations in the CURRENT
checkout, so on a feature branch a migration another in-flight branch has
already applied reads as an orphan, and one on this branch not yet applied
reads as pending. Neither is drift.

Found while confirming #541: PRs #553 (#542) and #554 (#543) had applied
20260726013256, 20260726015858, 20260726100000 and 20260726110000 to cloud,
none of which exist on this branch — so verify:cloud run here reports four
orphans that are not drift at all.

Left as guidance rather than logic. Filtering by "is this stamp on some other
branch" would need a remote ref walk and would silently excuse the exact
condition the check exists to catch. The orphan detail line now names the
possibility and says to re-check on master; the payment-invariant checks are
branch-independent and meaningful anywhere.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HPFyHjqzSR6Ku1gyWjeQie
guillermoscript added a commit that referenced this pull request Jul 26, 2026
…ke migration drift detectable (#541) (#552)

* fix(security): apply the transactions INSERT lockdown to cloud and make drift detectable (#541)

The #538 lockdown (20260725180000) was never applied to the cloud project. Until
this commit, `authenticated` still held INSERT on `public.transactions` there and
the INSERT policy was the older #528 shape, so a caller could open a pending row
quoting `settlement_base: 1` against a priced product, pay that on-chain, and
have /api/payments/solana/verify — which verifies against the row's own
settlement_base — flip it to successful and grant the entitlement.

Applied to cloud (verified by querying the live catalog, not by reading files):

  - REVOKE INSERT ON transactions FROM authenticated, anon
    INSERT is now held only by postgres and service_role.
  - INSERT policy re-created with all four settlement_* IS NULL pins.

Nothing depended on the grant: PR #539 had already moved both user-scoped
inserts onto createAdminClient(). The two user-scoped .update() calls that
remain in the checkout route only touch provider_subscription_id and status,
both inside the #528 three-column UPDATE grant, so they are unaffected.

WHY IT WAS LOST, AND WHAT NOW CATCHES IT

Cloud migration stamps had drifted from repo filenames: four migrations were
applied through the MCP apply_migration tool, which stamps a fresh timestamp
instead of the filename. That makes "what is missing from cloud?" unanswerable —
the re-stamped four read as pending, and the genuinely missing fifth was
indistinguishable from them.

  npm run verify:cloud

queries the live database and exits non-zero on drift. It asserts ledger
integrity (every repo migration stamped under its own filename; no stamp
matching no file) and the #512/#528/#538 payment invariants against catalog
state rather than migration text, so a later re-widening fails it too.

The rules are split into scripts/lib/verify-cloud-schema-checks.ts and driven
from both healthy and drifted fixtures in tests/unit/verify-cloud-schema.test.ts
(13 tests). The healthy fixture is the exact state read back from cloud, so the
policy-pin matcher is tested against how Postgres actually renders the
expression. That is also how the "fails if you re-grant INSERT" criterion is
demonstrated: as a repeatable test, rather than by briefly re-opening a write
grant on the payments table of the only production database this project has.

docs/MIGRATIONS.md now states the push-only rule and why, with the incident
table, and reframes the Management API fallback so the schema_migrations stamp
reads as the thing that makes it safe rather than optional bookkeeping.

Gates: typecheck clean, test:unit 383 passed (370 baseline + 13), eslint clean
on new files, build succeeds.

Refs #540

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HPFyHjqzSR6Ku1gyWjeQie

* fix(db): repair the drifted cloud migration ledger (#541)

Applied to cloud. Re-stamps the four migrations that had been applied through
the MCP apply_migration tool (which stamps a fresh timestamp instead of the
migration's filename) and records 20260725180000, whose DDL was applied earlier
in this issue:

  20260721120000_add_binance_personal_provider        was 20260725213441
  20260725110000_transaction_split_snapshot_backstop  was 20260725213508
  20260725160000_entitlement_gated_enrollment_inserts was 20260725213610
  20260725170000_transactions_column_hardening        was 20260725192946
  20260725180000_transactions_insert_lockdown         was absent

Each of the four was confirmed genuinely live by querying what its DDL created
before being treated as a ledger-only repair, so this re-runs no DDL.

The file is committed under the stamp apply_migration assigned it
(20260726005843) so the repair is not itself an orphan — the ledger now matches
the repo exactly: 171 files, 171 stamps, zero pending, zero orphans.

Idempotent: on a fresh `supabase db reset` the four UPDATEs match nothing and
the INSERT no-ops, since the CLI has already stamped 20260725180000 by the time
this file runs.

verify:cloud now reports 10/10 PASS.

Gates: typecheck clean, test:unit 383 passed.

Refs #540

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HPFyHjqzSR6Ku1gyWjeQie

* docs(migrations): note that verify:cloud's ledger checks are branch-relative (#541)

The two ledger checks compare cloud against the migrations in the CURRENT
checkout, so on a feature branch a migration another in-flight branch has
already applied reads as an orphan, and one on this branch not yet applied
reads as pending. Neither is drift.

Found while confirming #541: PRs #553 (#542) and #554 (#543) had applied
20260726013256, 20260726015858, 20260726100000 and 20260726110000 to cloud,
none of which exist on this branch — so verify:cloud run here reports four
orphans that are not drift at all.

Left as guidance rather than logic. Filtering by "is this stamp on some other
branch" would need a remote ref walk and would silently excuse the exact
condition the check exists to catch. The orphan detail line now names the
possibility and says to re-check on master; the payment-invariant checks are
branch-independent and meaningful anywhere.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HPFyHjqzSR6Ku1gyWjeQie

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

db-migration Requires a Supabase migration security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lock the write side: grades, progress and practice rows are self-issuable

1 participant