Skip to content

fix(#532): gate checkpoint attempts on entitlements, not enrollments - #535

Merged
guillermoscript merged 1 commit into
masterfrom
fix/532-checkpoint-attempt-entitlement-gate
Jul 25, 2026
Merged

fix(#532): gate checkpoint attempts on entitlements, not enrollments#535
guillermoscript merged 1 commit into
masterfrom
fix/532-checkpoint-attempt-entitlement-gate

Conversation

@guillermoscript

Copy link
Copy Markdown
Owner

What

The lesson-checkpoint attempt API route now gates on entitlements (via resolveCourseAccessState()) instead of on an enrollments row, the two sibling admin-client media routes get the same check as defence-in-depth, and a migration stops an enrollments row from being usable as an access grant at the database level.

Closes #532

Why

app/api/lesson-checkpoints/[checkpointId]/attempt/route.ts reads through createAdminClient(), so the RLS backstop shipped in #509 (migration 20260724150000) cannot apply to it, and the only gate it carried was an active enrollments row.

That row has not meant "has access" since migration 20260516150000:

  • Nothing revokes it. Refunds revoke entitlements (lib/payments/webhook-dispatch.ts), and tenants.access_cutoff_at (Non-payment produces no access enforcement — over-limit tenants keep full access indefinitely #494) is read exclusively inside has_course_access().
  • It was self-issuable. The INSERT policy's WITH CHECK was (auth.uid() = user_id) AND (tenant_id = get_tenant_id()) — no entitlement predicate — so any tenant member could POST /rest/v1/enrollments for any course in their tenant and manufacture the proof this route wanted. The same EXISTS also backed lesson_checkpoint_attempts_students_insert_own_rows, so the RLS insert the route deferred to was equally satisfiable.

Three populations therefore reached paid checkpoint content: students past their tenant's access cutoff (#494), students whose entitlement was revoked by a refund (#498/#515), and anyone who self-inserted an enrollment row. What they got was AI evaluation with per-question feedback and score, plus writes to lesson_checkpoint_attempts, exercise_evaluations, exercise_completions and the XP ledger — unpaid consumption of the school's AI budget rather than a raw content dump, but the same root cause as #509: admin client plus the wrong gate.

Changes

  • Route gateresolveCourseAccessState() replaces the enrollments lookup. Both refusals are 403; the payload distinguishes a tenant suspension (accessSuspended) from a missing entitlement (accessDenied) so the checkpoint UI can say which, in translated copy (en + es) instead of the server's raw English.
  • Sibling routesapi/exercises/media/analyze and api/exercises/media/signed-url were ownership + tenant checked only. Neither is a live hole (both are reachable only behind the gated upload-url route), but one spends AI credits and the other mints a storage URL off the admin client, and an entitlement can be revoked — or the tenant cutoff can pass — between the upload and the call. Both now call hasCourseAccess().
  • Migration 20260725160000 — the enrollments INSERT policy and the lesson_checkpoint_attempts INSERT policy both now require has_course_access(). Safe for every legitimate writer: enroll_user(), self_enroll_subscription_course(), grant_free_entitlement() and issue_certificate_if_eligible() are all SECURITY DEFINER (verified in pg_proc) so RLS does not apply to them, and admin/seed paths use the service-role client. No client-side enrollments INSERT exists in the app. The policy is tightened rather than dropped, so any caller that genuinely holds access keeps working.
  • Docsdocs/MONETIZATION.md claimed the enforcement layer covered "the certificate, AI tutor and exercise-submission APIs"; it now lists the API-route layer accurately and states that an enrollments row is not an access grant. Same note added to docs/DATABASE_SCHEMA.md and as a COMMENT ON TABLE, so the next reader does not reuse it as one.
  • Four pre-existing no-explicit-any errors in the media analyze route were fixed rather than bypassed with --no-verify, since lint-staged lints whole files.

How to QA

Requires a dev server (npm run dev) and the seeded local database.

Automated (the fastest check):

  1. npx playwright test checkpoint-access --workers=1 --project=desktop-chromium — three tests, all green. To see them catch the bug, git stash the route change and re-run: the two refusal tests fail with 500 instead of 403.

Manual, as student@e2etest.com / password123 on lvh.me:3000 (Default School, course 1001):

  1. Open a lesson with a checkpoint and submit an answer — it grades and records normally.
  2. Revoke access without touching the enrollment row:
    update entitlements set status='revoked' where user_id='a1000000-0000-0000-0000-000000000001' and course_id=1001;
    Submit again — the checkpoint shows "You don't have access to this course" and the API returns 403. Confirm the enrollments row is still there and still active; that is the point.
  3. Restore the entitlement (status='active'), then simulate the Non-payment produces no access enforcement — over-limit tenants keep full access indefinitely #494 cutoff:
    update tenants set access_cutoff_at = now() - interval '1 minute' where id='00000000-0000-0000-0000-000000000001';
    Submit again — the copy now reads "Your school's access is currently suspended". Reset access_cutoff_at to null afterwards.
  4. Prove the enrollment row is no longer self-issuable — as an authenticated role with no entitlement for the course, INSERT INTO enrollments (...) is rejected with "new row violates row-level security policy", while the same insert succeeds with the entitlement intact.
  5. Regression: npx playwright test entitlements-overlap enrollment-flows --workers=1 --project=desktop-chromium — 11 passed.

Screenshots / GIF

None — this is an API/RLS change. The only user-visible difference is the error copy in the checkpoint card for students who no longer have access, which QA step 3/4 above exercises directly.

Checklist

  • npm run typecheck and npm run test:unit pass — typecheck clean, 342 unit tests passed (29 files)
  • npm run build passes
  • Every new tenant-scoped query filters by tenant_id (or the table genuinely has no such column) — no new queries; the changed reads keep their existing tenant checks
  • Tested with every relevant role (student / teacher / admin) — student is the only role that reaches these routes (proxy.ts keeps staff out of /dashboard/student/*, and has_course_access() has no staff branch by design); teacher-side checkpoint code reads attempts and never posts them
  • Loading and error states handled — the new 403s render through the existing error path in checkpoint-exercise-renderer.tsx
  • New UI strings added to both messages/en.json and messages/es.json
  • Migration applies cleanly on npm run db:reset, has RLS policies, and lib/database.types.ts was regenerated

On the last box: the migration was applied to the local database and both resulting policies re-read from pg_policies to confirm they landed as written, but a full npm run db:reset was not run — another session was working against the same local database at the time and a reset would have destroyed its state. The migration is DDL-only (DROP POLICY / CREATE POLICY / COMMENT), adds no columns or tables, so lib/database.types.ts is unchanged by design. Worth a db:reset on a quiet database before merge.

The migration has not been applied to the cloud project — that is a deliberate hold, since it tightens a policy in production. Say the word and I'll apply it.

…nts (#532)

The lesson-checkpoint attempt route reads through the service-role client, so
the RLS backstop added in #509 never applied to it, and the only gate it had
was an active `enrollments` row. That row has not meant "has access" since
migration 20260516150000: nothing revokes it (refunds revoke entitlements,
and tenants.access_cutoff_at is read only inside has_course_access), and its
RLS INSERT policy let any tenant member issue one for any course in their
tenant. Three populations could therefore spend the school's AI grading
budget and write progress records for content they had no right to: students
past their tenant's access cutoff (#494), students whose entitlement was
revoked by a refund (#498/#515), and anyone who self-inserted an enrollment.

- Route now calls resolveCourseAccessState() and returns a 403 that tells a
  tenant suspension apart from a missing entitlement, so the checkpoint UI can
  explain which one it is (new translated copy, en + es).
- Same check added to the two sibling admin-client media routes (analyze,
  signed-url), which were ownership-checked only. Not live holes — both sit
  behind the gated upload path — but both spend AI credits or mint storage
  URLs, and an entitlement can be revoked after the upload was authorized.
- Migration 20260725160000 stops enrollments being an access grant at the
  database level: its INSERT policy and the checkpoint-attempt INSERT policy
  now both require has_course_access(). Every legitimate writer (enroll_user,
  self_enroll_subscription_course, grant_free_entitlement,
  issue_certificate_if_eligible) is SECURITY DEFINER, so none are affected.
- docs/MONETIZATION.md now describes the API-route layer accurately instead of
  claiming page gates cover it, and both docs state that an enrollment row is
  not an access grant.

New E2E spec pins the three states (entitled 200, revoked 403 accessDenied,
cutoff 403 accessSuspended); its two refusal cases fail against the old code.

The four pre-existing `no-explicit-any` errors in the media analyze route are
fixed rather than bypassed, since lint-staged lints the whole file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019QSWA8GwmjsYHUS7ov5NG3
@guillermoscript guillermoscript added bug Something isn't working security Security vulnerability or hardening severity:high High severity security finding Sub-task labels Jul 25, 2026
@guillermoscript guillermoscript self-assigned this Jul 25, 2026
@guillermoscript
guillermoscript marked this pull request as ready for review July 25, 2026 18:17
@guillermoscript
guillermoscript merged commit 4b2cb16 into master Jul 25, 2026
2 checks passed
@guillermoscript
guillermoscript deleted the fix/532-checkpoint-attempt-entitlement-gate branch July 25, 2026 18:25
@guillermoscript

Copy link
Copy Markdown
Owner Author

Merged as 4b2cb16 (squash). Shipped exactly as described in the body — no scope changed during review.

What landed

  • app/api/lesson-checkpoints/[checkpointId]/attempt/route.ts gates on resolveCourseAccessState() instead of an enrollments row, returning a 403 that distinguishes a tenant suspension from a missing entitlement, with translated copy in messages/en.json and messages/es.json.
  • api/exercises/media/analyze and api/exercises/media/signed-url gained the same hasCourseAccess() check as defence-in-depth.
  • Migration 20260725160000_entitlement_gated_enrollment_inserts.sql requires has_course_access() in the enrollments INSERT policy and in lesson_checkpoint_attempts_students_insert_own_rows.
  • docs/MONETIZATION.md, docs/DATABASE_SCHEMA.md and a COMMENT ON TABLE public.enrollments all now state that an enrollment row is a progress record, not an access grant.
  • tests/playwright/checkpoint-access.spec.ts pins entitled → 200, revoked → 403 accessDenied, tenant past cutoff → 403 accessSuspended.

Verified before merge: npm run typecheck clean · npm run test:unit 342 passed · npm run build passed · new spec 3/3 · entitlements-overlap + enrollment-flows 11/11 · both CI verify runs green. The two refusal cases in the new spec were confirmed to fail (500 instead of 403) against the pre-fix code, so they are a real regression guard rather than a tautology.

Outstanding — the migration is not on the cloud project yet. It was applied to the local database only, deliberately: it tightens a policy in production and that is not a change to make unattended. Until supabase db push runs against the cloud project, the fix is app-level only there — the route gate holds, but the database-level backstop (a self-issued enrollments row being refused) does not.

Also worth a follow-up run on a quiet local database: npm run db:reset was not executed here because another session was working against the same local Postgres. The migration is DDL-only (DROP POLICY / CREATE POLICY / COMMENT) and both resulting policies were re-read from pg_policies to confirm they landed as written, so this is a formality rather than a suspected problem.

guillermoscript added a commit that referenced this pull request Jul 26, 2026
…ble rows (#543) (#554)

* fix(security): gate the write side on entitlements, not on self-issuable 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

* fix(security): drop the out-of-scope course_id predicate on practice_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

---------

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

bug Something isn't working security Security vulnerability or hardening severity:high High severity security finding Sub-task

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lesson-checkpoint attempt route gates on a self-issuable enrollment row, bypassing entitlements and the access cutoff (#509 follow-up)

1 participant