fix(security): gate exam questions, options and lesson comments through their parent (#542) - #553
Merged
Merged
Conversation
…gh their parent (#542) `exam_questions` and `question_options` were created `USING (true)` by 20260313152048 and left behind when #509 gated their parent `exams`, so any authenticated user of any tenant — holding no entitlement, past their school's `access_cutoff_at` — could read every exam's `correct_answer`, `grading_rubric`, `ai_grading_criteria` and `is_correct` straight over PostgREST. `lesson_comments` was the same shape with lower stakes. These tables have no `tenant_id` of their own, so the new predicates route through the FK to the parent that does, mirroring the #509 `exams` gate (tenant + staff/author/entitlement) plus the pre-existing super-admin policy via a shared `can_read_exam()` helper. Also tightened, beyond the four SELECT policies the issue lists: the sibling `FOR ALL` staff policies on both exam tables. They matched on `get_tenant_role() IN ('teacher','admin')` with no row predicate at all, and permissive policies OR together — so a teacher of tenant A kept reading and writing every other tenant's exam questions through that policy, and the SELECT fix alone would have bought nothing for staff accounts. And converged the #509 authenticated preview branch with the anon one by requiring the parent course to be published: a teacher drafting a course who marked a lesson preview + published was exposing that body to every signed-in member of the tenant while anonymous visitors correctly could not see it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xwf7RJcxHNRVXb3ufFuT8g
Owner
Author
Browser + live-session QA (local,
|
| Path | Client | Result |
|---|---|---|
Student exam taker (/courses/2001/exams/2001) as alice |
admin client + requireCourseAccess |
question text + True/False options render |
| Lesson page comment thread as alice | RLS-bound browser client | thread renders ("1 Comments", seeded body visible) |
Teacher exam editor (/teacher/courses/2001/exams/2001) as creator@codeacademy.com |
RLS-bound user client | "Questions (10)", 32 populated fields — question text, options, grading rubric, AI criteria |
Teacher grading screen (/submissions/1) as creator@codeacademy.com |
RLS-bound user client | all 10 questions, per-option Correct markers, student answers, Override controls |
The last two are the pages that went blank in #282 when a tenant_id filter was wrongly applied to these tables — they are the reason the staff branch is carried over.
Direct PostgREST probe with real user JWTs — the actual attack surface, against a seeded Code Academy exam whose course nobody holds an entitlement for (correct_answer = SECRET-ANSWER-KEY-542):
| Caller | gated answer key | all exam_questions |
all question_options |
all lesson_comments |
|---|---|---|---|---|
| Before — alice (same tenant, not entitled) | 🔴 SECRET-ANSWER-KEY-542 |
11 | — | — |
Before — student@e2etest.com (different tenant) |
🔴 SECRET-ANSWER-KEY-542 |
11 | — | — |
| After — alice (same tenant, not entitled) | ✅ 0 rows | 10 (only the exam she is entitled to) | 20 | 0 |
After — student@e2etest.com (different tenant) |
✅ 0 rows | 0 | 0 | 0 |
After — creator@codeacademy.com (tenant staff) |
✅ 1 row, key readable | 11 | 21 | 0 |
Staff keep full read of their own tenant; everyone else loses what they were never meant to have.
Gates: typecheck clean · test:unit 370/370 · build passes · exam-answer-key-rls 10/10 · AI grading E2E passes end to end with the new policies.
guillermoscript
marked this pull request as ready for review
July 26, 2026 01:29
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>
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
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.
What
Replaces the three
USING (true)SELECT policies onexam_questions,question_optionsandlesson_commentswith predicates that route through their parent (exams/lessons), tightens the two siblingFOR ALLstaff policies on the exam tables, and converges the #509 authenticated preview-lesson branch with the anon one.Closes #542
Why
20260313152048_rls_exam_child_tables.sqlcreatedexam_questionsandquestion_optionsasUSING (true). #509 replaced the parentexamspolicy with a tenant + staff/author +has_course_accessgate and stopped there, so any authenticated user of any tenant — holding no entitlement, past their school'saccess_cutoff_at— could read every exam'scorrect_answer,grading_rubric,ai_grading_criteria,expected_keywordsandquestion_options.is_correctstraight over PostgREST. Both the #509 exam gate and the #494 access cutoff were bypassable for the thing exams exist to protect.lesson_commentswas the same shape with lower stakes.Beyond the four policies the issue lists, this also tightens the sibling
FOR ALLstaff policies on both exam tables. They matched onget_tenant_role() IN ('teacher','admin')with no row predicate at all, and permissive policies OR together — so without this a teacher of tenant A keeps reading (and writing) every other tenant's exam questions through that policy, and the acceptance criterion "a member of tenant A gets zero rows" would fail for any staff account. Verified: before the change a caller presenting a tenant-Ateacherclaim read all of tenant B's questions; after, zero.The preview overshoot is a content leak introduced by the #509 hardening:
(is_preview AND status='published')omitted the parent-course-publishedEXISTSits anon counterpart carries, so a teacher drafting a course who marked a lesson preview + published exposed that body to every signed-in tenant member while anonymous visitors correctly could not see it.How it works
New
can_read_exam(_exam_id)helper (SECURITY INVOKER, soauth.uid()/get_tenant_id()/is_tenant_staff()resolve to the caller) mirrors the #509examsgate plus the pre-existing super-admin policy, and both exam child tables gate on it —question_optionstwo-hop viaexam_questions. Per CLAUDE.md these tables have notenant_id(adding one errors the query — the #282 teacher-grading regression), hence routing through the FK. The staff/author branches are carried over deliberately: the exam editor, teacher grading and the AI grading path inapp/actions/exam-grading.tsall read through RLS-bound user clients, so a missing branch breaks grading rather than failing safe.Also adds the three missing FK indexes (
exam_questions.exam_id,question_options.question_id,lesson_comments.lesson_id), which every new predicate looks rows up by.How to QA
On a fresh
npm run db:reset, thennpm run dev:npx playwright test exam-answer-key-rls --workers=1(10 tests; needs no dev server, it drives PostgREST directly). Covers: cross-tenant read → 0 rows; same-tenant-no-entitlement → 0 rows; unscoped whole-table sweep → 0 rows; staff still read rubrics +is_correct; entitled student still reads the exam they are sitting; comments cross-tenant → 0 / in-tenant → visible; preview lesson of a draft course hidden from both signed-in and anon, restored when the course is published.creator@codeacademy.comoncode-academy.lvh.me:3000, open a course exam in the editor and a submission's grading screen; questions, options and rubrics all render.npx playwright test exam-grading-test --workers=1asalice@student.com(needs her seeded entitlement to course 2001).Screenshots / GIF
n/a — no user-visible surface changes; this is an RLS-only change. Evidence is the policy dump in step 1 and the spec in step 2.
Verification
npm run typecheck— cleannpm run test:unit— 370/370 (matches the146a8cfabaseline)npm run build— passesnpx eslint tests/playwright/exam-answer-key-rls.spec.ts— clean (repo-wide lint has a large pre-existing baseline, unchanged here; SQL isn't linted)set_config('request.jwt.claims', …)) confirm each branch: cross-tenant student 0, non-entitled member 0, forged tenant-Ateacherclaim 0, owning-tenant staff full read, entitled student full read, super admin full readstudent-exams,evaluations-security,teacher-content— 40 passedexam-grading-test(AI grading end to end) — passes with the new policies activeTwo pre-existing failures, confirmed unrelated by re-running each with the old policies restored (identical failure):
full-student-journey.spec.ts— targets course9999/ lessons10000-10001, which no longer exist insupabase/seed.sql; the page 404s regardless of RLSexam-grading-testfailure wasalice@student.comhaving lost her seeded entitlement to course 2001 (deleted by a parallel job sharing this local database, not by this change); restoring the row makes it passNotes for the reviewer
pg_policiesassertion on cloud too — that is a deliberate separate step.lib/database.types.tsnot regenerated: this migration adds no tables or columns, only policies, one function and three indexes.FOR ALLstaff policies keepget_tenant_role()rather than swapping tois_tenant_staff()— the change there is purely the added row predicate, to keep the write gate itself untouched.Checklist
npm run typecheckandnpm run test:unitpassnpm run buildpassestenant_id(or the table genuinely has no such column — all three here genuinely have none, hence the FK routing)messages/en.jsonandmessages/es.json(n/a — no new strings)npm run db:reset, has RLS policies;lib/database.types.tsregeneration not needed (no schema shape change)🤖 Generated with Claude Code
https://claude.ai/code/session_01Xwf7RJcxHNRVXb3ufFuT8g