Uh oh!
There was an error while loading. Please reload this page.
fix(security): encrypt syllabus-extracted assignment notes at write boundary (#126 #4) - #226
Conversation
…oundary (#126#4) insert_new_assignments wrote "notes": a.get("notes") with no encrypt_if_present, so uploading a syllabus persisted assignment notes as PLAINTEXT in the encrypted assignments.notes column — defeating column encryption and spamming decrypt_if_present fallback warnings on read. Every other writer (calendar.py, gradebook.py) encrypts. This is the single write site behind both callers (calendar save flow and the documents upload pipeline, via save_assignments_to_db). Wrap the write in encrypt_if_present. Test asserts the inserted value is ciphertext that decrypts back to the original (fails pre-fix: stored plaintext). NOTE: this stops NEW plaintext writes only. Rows already written in plaintext remain in the clear at rest and need a backfill migration — surfaced for a separate decision, NOT silently re-encrypted here.
Warning Review limit reached
More reviews will be available in 46 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| it asserts the value handed to the DB insert is ciphertext, and that it | ||
| round-trips back to the original plaintext via decrypt. | ||
| """ | ||
| from unittest.mock import MagicMock, patch |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend | 538478f | Commit Preview URL Branch Preview URL | Jun 13 2026, 05:09 AM |
Part of #126 (finding #4, HIGH). Split per request: this is the HIGH plaintext-write fix; the two LOW response-decrypt findings (#18/#19) follow in a separate PR.
Vulnerability
insert_new_assignments(services/calendar_service.py) wrote"notes": a.get("notes")with noencrypt_if_present.assignments.notesis an encrypted column; every other writer (calendar.py:128,gradebook.py:252) encrypts. Uploading a syllabus persisted assignment notes as plaintext at rest, defeating column encryption and spammingdecrypt_if_present fallbackwarnings on read. This single function is the write site behind both callers — the calendar save flow and the documents upload pipeline (viasave_assignments_to_db→insert_new_assignments).Fix
Wrap the write in
encrypt_if_present(one line + import). Test asserts the value handed to the insert is ciphertext thatdecrypt()s back to the original plaintext;NonestaysNone. Fails on pre-fix code (stored plaintext).This stops new plaintext writes. Rows already written in plaintext remain exposed at rest. Evidence they exist:
services/encryption.pyROLLOUT NOTES explicitly state the decrypt fallback exists so "legacy plaintext rows continue to load while a backfill is pending," and "a backfill script is required" — and none has run.I could not get a live count: the only Supabase project reachable via MCP is
lajrjnjyvbpaaspzgpvh, which is the unrelated GTM-Builds production DB (not Sapling), and the read was correctly denied as an unapproved prod query. Re-encrypting existing rows is a data migration (read eachassignments.notes, attemptdecrypt, re-encrypton failure, write back) — exactly why the #197 migration framework matters. Surfacing for a separate decision; not silently re-encrypting here. See my message for the detection query + migration shape.