Uh oh!
There was an error while loading. Please reload this page.
feat: AES-256-GCM column-level encryption for user PII - #65
Conversation
- auth_guard.get_session_user_id no longer falls back to query-param user_id - conftest installs autouse fixture stubbing require_self/admin/role for tests - routes/graph.py enforces require_self on every endpoint - routes/auth.py /me reads user_id from session token, not query param - ENCRYPTED LATER markers placed on every column slated for column-level encryption - adds the AES-256-GCM column encryption implementation plan Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Required by the upcoming services/encryption.py helper module that wraps AES-256-GCM for sensitive Supabase columns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds services/encryption.py providing encrypt/decrypt plus *_if_present and JSON/numeric helpers for sensitive Supabase columns. Key is loaded once at import time from ENCRYPTION_KEY (64 hex chars / 32 bytes); each encrypt() mints a fresh 12-byte nonce and stores base64(nonce || ct+tag). Reads use *_if_present helpers that fall back to the raw value with a warning, so legacy plaintext rows continue to load until the backfill runs. Tests cover round-trip (ASCII/unicode/long/empty), JSON helpers, numeric coercion, _if_present None passthrough, legacy plaintext fallback, nonce randomness, and tamper detection. conftest.py sets ENCRYPTION_KEY to a deterministic 32-byte zero key so the module imports cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire AES-256-GCM column encryption into the Google OAuth callback so user names, emails, and OAuth access/refresh tokens are encrypted before persistence. The plaintext locals stay in process memory long enough to run the @bu.edu domain check, split the display name, and build the redirect URL; encryption happens at the table boundary only. Also disables the legacy email-based account merge branch — random-nonce GCM ciphertext cannot be matched by an equality lookup on the plaintext email — and drops the `name` query param from the post-signin redirect so PII no longer leaks into HTTP logs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…arkers Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r tutor prompts Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…generation Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…pt build Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
assignments.points_possible/points_earned and documents.concept_notes / sessions.summary_json must store base64 ciphertext, not numeric/json. Casts existing plaintext rows in place; the legacy-plaintext fallback in services/encryption.py keeps reads working until backfill runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…json Closes the deferred-work items from the plan: - learn.py save_message/get_conversation_history encrypt+decrypt content - learn.py end_session encrypts summary_json via encrypt_json - flashcards.py _get_session_summary decrypts summary_json - social.py send/edit/get_room_messages encrypt+decrypt room_messages.text Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The redirect URL no longer carries the user's display name (PII leak). The Next.js callback now fetches it from /api/auth/me after the session cookie is set, restoring end-to-end sign-in. /me decrypts the encrypted name column before returning. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Walks every encrypted column, detects rows still stored as plaintext (decrypt() raises), and rewrites them through the encrypt helpers. Idempotent — re-runs are no-ops once every cell is valid ciphertext. Defaults to dry-run; --apply actually writes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend | 43d9015 | Commit Preview URL Branch Preview URL | May 03 2026, 07:57 PM |
📝 WalkthroughWalkthroughAdds AES-256-GCM column-level encryption: new encryption service, test coverage, env/docker wiring, a migration to TEXT for certain columns, a one-shot backfill CLI, and pervasive route changes to encrypt sensitive writes, decrypt reads, and enforce request-based authorization. ChangesEncryption rollout & route wiring
Sequence DiagramsequenceDiagram
participant Client
participant RouteHandler
participant AuthGuard
participant EncryptionSvc
participant Database
rect rgba(100, 150, 200, 0.5)
Note over Client,Database: Write path (encrypt before persist)
Client->>RouteHandler: POST /api/... (user_id, sensitive)
RouteHandler->>AuthGuard: require_self(user_id, request)
AuthGuard-->>RouteHandler: authorized
RouteHandler->>EncryptionSvc: encrypt(sensitive)
EncryptionSvc-->>RouteHandler: ciphertext
RouteHandler->>Database: INSERT/UPDATE (ciphertext)
Database-->>RouteHandler: OK
RouteHandler-->>Client: 200 (response with decrypted fields)
end
rect rgba(150, 200, 100, 0.5)
Note over Client,Database: Read path (decrypt on load)
Client->>RouteHandler: GET /api/.../{user_id}
RouteHandler->>AuthGuard: require_self(user_id, request)
AuthGuard-->>RouteHandler: authorized
RouteHandler->>Database: SELECT (ciphertext_field)
Database-->>RouteHandler: row with ciphertext
RouteHandler->>EncryptionSvc: decrypt_if_present(ciphertext)
EncryptionSvc-->>RouteHandler: plaintext or legacy value
RouteHandler-->>Client: 200 (plaintext)
end
rect rgba(200, 100, 100, 0.5)
Note over RouteHandler,EncryptionSvc: Legacy plaintext fallback
RouteHandler->>EncryptionSvc: decrypt_if_present(legacy_plaintext)
EncryptionSvc-->>RouteHandler: returns original plaintext (logs warning)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
| if user_id: | ||
| require_self(user_id, request) | ||
| else: | ||
| user_id = get_session_user_id(request) |
| def get_students(): | ||
| def get_students(request: Request): | ||
| """Return a lightweight profile for every user in the DB.""" | ||
| user_id = get_session_user_id(request) |
| from googleapiclient.discovery import build | ||
| from google.oauth2.credentials import Credentials | ||
| from google.auth.transport.requests import Request | ||
| from google.auth.transport.requests import Request as GoogleAuthRequest |
| The encryption key is set in conftest.py so the module imports cleanly. | ||
| """ | ||
| import json |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/routes/quiz.py (1)
190-203:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
student_nameagainst nulls before template replacement.
decrypt_if_present()can returnNone. Ifusers.nameis null, the.replace("{student_name}", student_name)call raisesTypeErrorafter the quiz attempt and mastery updates have already been persisted.Suggested fix
- student_name = decrypt_if_present(user_rows[0]["name"]) if user_rows else "Student"+ student_name = (+ decrypt_if_present(user_rows[0].get("name")) or "Student"+ if user_rows+ else "Student"+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/quiz.py` around lines 190 - 203, The student_name value used in the prompt can be None because decrypt_if_present(user_rows[0]["name"]) may return None; before calling .replace("{student_name}", student_name) ensure student_name is a non-null string (e.g., coerce to a fallback like "Student" or "" if None) so template replacement never receives None; update the code around where student_name is assigned (the decrypt_if_present call and subsequent use in ctx_prompt/_load_prompt) to coerce or default the value and use that sanitized variable in the .replace call.backend/routes/gradebook.py (1)
242-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn decrypted fields from
create_assignment().This route now writes encrypted
points_possible,points_earned, andnotes, then returnsinserted[0]verbatim. Any client that uses the POST response to update local state will receive ciphertext instead of the values it just submitted, while the read endpoints return plaintext.Suggested fix
inserted = table("assignments").insert({ "id": new_id, "user_id": body.user_id, "course_id": body.course_id, "title": body.title, "category_id": body.category_id, "points_possible": encrypt_if_present(body.points_possible), "points_earned": encrypt_if_present(body.points_earned), "due_date": body.due_date, "assignment_type": body.assignment_type, "notes": encrypt_if_present(body.notes), "source": "manual", }) - return {"assignment": inserted[0] if inserted else None}+ assignment = inserted[0] if inserted else None+ if assignment:+ assignment["points_possible"] = decrypt_numeric(assignment.get("points_possible"))+ assignment["points_earned"] = decrypt_numeric(assignment.get("points_earned"))+ assignment["notes"] = decrypt_if_present(assignment.get("notes"))+ return {"assignment": assignment}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/gradebook.py` around lines 242 - 255, The POST handler currently inserts encrypted values (using encrypt_if_present) and returns inserted[0] as-is, causing ciphertext to be returned; after the insert in create_assignment (the block that builds inserted = table("assignments").insert(...)), decrypt the stored fields before returning by running points_possible, points_earned, and notes through the corresponding decrypt helper (e.g., decrypt_if_present) and then return the modified assignment object (rather than the raw inserted[0]); update the return to return {"assignment": decrypted_assignment} so clients receive plaintext fields consistent with read endpoints.backend/routes/learn.py (1)
525-532:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMessages returned by resume_session are not decrypted.
When resuming a persisted session, the messages fetched from the database have encrypted
contentbut are returned directly without decryption. This is inconsistent withget_conversation_historywhich decrypts message content.🐛 Proposed fix
msgs = table("messages").select( "id,role,content,created_at", filters={"session_id": f"eq.{session_id}"}, order="created_at.asc", ) + for m in msgs:+ m["content"] = decrypt_if_present(m.get("content")) return { "session": session_rows[0], "messages": msgs, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/learn.py` around lines 525 - 532, The messages returned by resume_session are fetched into msgs via table("messages").select but not decrypted; update resume_session to reuse the decryption logic from get_conversation_history (or call get_conversation_history(session_id) instead of returning raw msgs) so each message's content is decrypted before returning. Locate the msgs variable and the return that currently returns session_rows[0] and msgs, then either map over msgs and replace each message's content with the decrypted value using the same helper used by get_conversation_history, or delegate to get_conversation_history to produce the decrypted messages, and return that result.backend/routes/social.py (1)
343-371:⚠️ Potential issue | 🟠 Major
user_nameis stored unencrypted in room_messages.The message
textis encrypted butbody.user_nameis stored in plaintext. This is inconsistent with the PR's goal of encrypting PII. User names are PII and should be encrypted at rest.This also requires updating
get_room_messagesto decryptuser_namein the message rows (line 290) and reply snippets (line 320).🔒 Proposed fix to encrypt user_name
row = table("room_messages").insert({ "room_id": room_id, "user_id": body.user_id, - "user_name": body.user_name,+ "user_name": encrypt_if_present(body.user_name), "text": encrypt_if_present(body.text or None), "image_url": body.image_url or None, "reply_to_id": body.reply_to_id or None, }) if row: row[0]["text"] = decrypt_if_present(row[0].get("text")) + row[0]["user_name"] = decrypt_if_present(row[0].get("user_name"))In
get_room_messages, decryptuser_namewhen retrieving message rows and reply snippets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/social.py` around lines 343 - 371, send_room_message currently stores body.user_name in plaintext; update the insert in send_room_message to pass user_name through encrypt_if_present (same pattern as text) so user_name is encrypted at rest. Then update get_room_messages to call decrypt_if_present on the message row's user_name and also on any reply snippet user_name (the reply_to handling at/around where reply snippets are built) to return decrypted names to clients; preserve null/None handling consistent with encrypt_if_present/decrypt_if_present usage.
🧹 Nitpick comments (8)
backend/services/auth_guard.py (1)
60-61: 💤 Low valueAdd exception chaining for better error tracing.
While catching a broad
Exceptionis acceptable here (any decode failure should yield 401), usingraise ... fromwould preserve the original exception context for debugging.♻️ Suggested improvement
- except Exception:- raise HTTPException(status_code=401, detail="Not authenticated")+ except Exception as exc:+ raise HTTPException(status_code=401, detail="Not authenticated") from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/auth_guard.py` around lines 60 - 61, The except block currently swallows the original exception; update the handler in auth_guard.py so the broad except Exception captures the original exception (e.g., except Exception as e:) and re-raise the HTTPException with exception chaining (raise HTTPException(status_code=401, detail="Not authenticated") from e) to preserve the original traceback for debugging while still returning 401 from the authentication function/handler.backend/routes/onboarding.py (1)
14-14: 💤 Low valueUnused
requestparameter insearch_courses.The
request: Requestparameter is accepted but never used in this function. If this is intentional for consistency or future auth guards, consider adding a comment. Otherwise, remove it to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/onboarding.py` at line 14, The function search_courses currently declares an unused parameter request: Request which causes confusion; either remove the request parameter from the function signature of search_courses (and any corresponding route decorator/handler registration) or, if it's intentionally reserved for future use (e.g., auth or middleware), add an inline comment above the signature explaining why request is unused (e.g., "# request kept for future auth guard") and prefix the variable with an underscore (request -> _request) to signal intentional non-use. Update any callers or route mappings that expect the old signature accordingly and ensure imports (Request) are cleaned up if removed.backend/db/backfill_encryption.py (2)
91-91: 🏗️ Heavy liftNo pagination — large tables may cause memory/timeout issues.
table(table_name).select(...)on line 91 (and similar calls) loads all rows into memory. For tables with millions of rows (e.g.,messages,room_messages), this could exhaust memory or timeout.Consider adding batching with
limitandoffset, or using cursor-based pagination:Example batch approach
BATCH_SIZE=1000offset=0whileTrue: rows=table(table_name).select( ",".join([pk, *columns]), limit=BATCH_SIZE, offset=offset, order=f"{pk}.asc" ) or [] ifnotrows: break# process rows...offset+=BATCH_SIZE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/db/backfill_encryption.py` at line 91, The current call rows = table(table_name).select(",".join([pk, *columns])) loads the entire table into memory; change the logic around table(table_name).select to iterate in batches (use limit/offset or a cursor) while selecting the same columns (referencing table_name, pk, columns and the table(...) call) so you fetch e.g. BATCH_SIZE rows at a time, process them, then advance the offset (or cursor) and repeat until no rows remain; ensure you preserve ordering (order by pk) to make pagination deterministic and avoid loading all rows into memory.
162-172: 💤 Low valueCorrupt JSON encrypted as raw string may cause decrypt mismatch.
When JSON parsing fails (line 164), the raw string is encrypted via
encrypt(v)(line 169). However, reads usedecrypt_json()which expectsjson.loads(decrypt(value)). Decrypting will succeed, butjson.loadswill fail on the corrupt JSON, triggering the fallback path.This is acceptable since the fallback handles it, but documenting this behavior would help future maintainers understand why some cells fail JSON parsing after decryption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/db/backfill_encryption.py` around lines 162 - 172, When JSON parsing fails in backfill_encryption.py (the _json.loads(v) exception path), we currently encrypt the raw string with encrypt(v) so the value can round-trip through decrypt_if_present/decrypt_json(); add a clear inline comment near the except block explaining that decrypt_json() will call json.loads(decrypt(value)) and therefore decrypted corrupt JSON will re-trigger the JSON parse fallback at read time, and that this is intentional because the runtime fallback will handle it; reference the functions _json.loads, encrypt, decrypt_json, and decrypt_if_present in the comment so future maintainers understand the round-trip behavior and why some cells may still fail JSON parsing after decryption.frontend/src/app/auth/callback/page.tsx (1)
70-77: 💤 Low valueSilent failure fallback masks
/api/auth/meerrors.When the
/api/auth/mefetch fails (line 75), the code defaultsonboardingCompleted = true, which could incorrectly skip onboarding for users who haven't completed it if the API is temporarily unavailable. Consider logging this case or retrying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/auth/callback/page.tsx` around lines 70 - 77, The catch block for the fetch('/api/auth/me') call in page.tsx silently sets onboardingCompleted = true which can incorrectly skip onboarding on transient API failures; update the catch to capture the error (e.g., catch (err)), log it (console.error or your app logger) and set onboardingCompleted conservatively (false or leave undefined) or implement a simple retry before deciding, referencing the fetch('/api/auth/me') call and the onboardingCompleted/local state update to locate the fix.backend/routes/flashcards.py (1)
128-137: 💤 Low valueSilent pass on decrypt failure is acceptable but consider logging.
The
try-except-passon lines 133-136 silently ignores decryption failures forconcept_notes. While this is intentional for legacy plaintext compatibility, the encryption module's docstring mentions "fallback logs a warning." Consider adding a logging statement here for consistency with the stated rollout observability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/flashcards.py` around lines 128 - 137, The code silently swallows exceptions when calling decrypt_json on d.get("concept_notes"); instead add a warning log inside the except to surface the failure (include context like the doc id/index and use exc_info=True or log the exception message) while preserving the fallback behavior of leaving concept_notes unchanged; reference the decrypt_json call and d["concept_notes"], and ensure you use the module logger (e.g., obtain logging.getLogger(__name__) or reuse an existing logger variable) before logging.backend/routes/learn.py (1)
231-239: ⚡ Quick winUse explicit
Noneunion type annotation.PEP 484 prohibits implicit
Optional. The type hint should use| Nonefor clarity.♻️ Proposed fix
-def save_message(session_id: str, role: str, content: str, graph_update: dict = None):+def save_message(session_id: str, role: str, content: str, graph_update: dict | None = None):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/learn.py` around lines 231 - 239, Update the function signature for save_message to use an explicit None union type for the graph_update parameter: replace the implicit Optional style "graph_update: dict = None" with the explicit union form "graph_update: dict | None = None" (referencing the save_message function and the graph_update parameter) so the type hint conforms to PEP 484; ensure no other code changes are required.backend/tests/test_encryption.py (1)
99-106: Use specific exception type for tamper detection test.The test currently catches
Exception, which is too broad. AES-GCM tamper detection raisescryptography.exceptions.InvalidTag, and using the specific exception type makes the test more precise and prevents accidentally passing if a different exception is raised.♻️ Proposed fix
def test_tampered_ciphertext_raises(): import base64 + from cryptography.exceptions import InvalidTag ct = encryption.encrypt("secret") raw = bytearray(base64.b64decode(ct)) raw[-1] ^= 0x01 # flip one bit in the tag tampered = base64.b64encode(bytes(raw)).decode() - with pytest.raises(Exception):+ with pytest.raises(InvalidTag): encryption.decrypt(tampered)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_encryption.py` around lines 99 - 106, The test test_tampered_ciphertext_raises should assert the specific AES-GCM tamper exception instead of catching all Exceptions: import InvalidTag from cryptography.exceptions and change the context manager to with pytest.raises(InvalidTag): calling encryption.decrypt(tampered). Keep the rest of the test (encryption.encrypt and bit-flip tampering) unchanged so it still exercises tamper detection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/db/migration_encryption_text_columns.sql`:
- Around line 13-27: The migration casts documents.concept_notes and
sessions.summary_json to TEXT but services/encryption.py's decrypt_json() lacks
the legacy-plaintext fallback, so pre-backfill rows will break; update
decrypt_json() (in backend/services/encryption.py) to detect and return
plaintext JSON when input isn't valid ciphertext (similar to the scalar
fallbacks used by decrypt_text()/decrypt_number()), or alternatively ensure the
DB backfill and enabling of readers are performed atomically so decrypt_json()
never sees legacy plaintext—modify either decrypt_json() to include a JSON
fallback helper or adjust the deployment/migration process to run the backfill
before enabling readers for documents.concept_notes and sessions.summary_json.
In `@backend/routes/auth.py`:
- Around line 228-229: The code is encrypting an empty string when
creds.refresh_token is None (calling encrypt("")), causing inconsistent handling
vs backfilled rows; change the assignment that sets "refresh_token" to either
call encrypt_if_present(creds.refresh_token) or keep a None value when
creds.refresh_token is falsy so empty strings are not encrypted—update the
object construction where "refresh_token" is set (referencing encrypt and
creds.refresh_token) to use encrypt_if_present or conditional preservation of
None to match the backfill behavior.
In `@backend/services/encryption.py`:
- Around line 116-117: decrypt_json currently assumes input is encrypted and
will raise if passed legacy plaintext JSON; update decrypt_json to mirror
decrypt_if_present by attempting to decrypt then json.loads, but on decryption
failure (or if decrypted result is invalid) fall back to json.loads(value) so
pre-encryption JSON still parses; locate the decrypt_json function and wrap its
call to decrypt(value) in a try/except (or check the decrypt result) and use
json.loads(value) as the fallback.
In `@backend/tests/conftest.py`:
- Around line 32-69: The current fixture only monkeypatches already-imported
route modules, leaving services.auth_guard itself untouched and causing
import-order dependent behavior; update the fixture to also monkeypatch the
functions on the services.auth_guard module (replace auth_guard._decode_session
with _decode_session_stub and auth_guard.get_session_user_id,
auth_guard.require_self, auth_guard.require_admin, and auth_guard.require_role
with the corresponding stubs/_require_role_stub) so any later-imported routes
that reference auth_guard will get the bypassed implementations; keep the
existing stubs (_decode_session_stub, _get_session_user_id_stub,
_require_self_stub, _require_admin_stub, _require_role_stub) and use
monkeypatch.setattr(auth_guard, "<name>", <stub>) for each symbol.
---
Outside diff comments:
In `@backend/routes/gradebook.py`:
- Around line 242-255: The POST handler currently inserts encrypted values
(using encrypt_if_present) and returns inserted[0] as-is, causing ciphertext to
be returned; after the insert in create_assignment (the block that builds
inserted = table("assignments").insert(...)), decrypt the stored fields before
returning by running points_possible, points_earned, and notes through the
corresponding decrypt helper (e.g., decrypt_if_present) and then return the
modified assignment object (rather than the raw inserted[0]); update the return
to return {"assignment": decrypted_assignment} so clients receive plaintext
fields consistent with read endpoints.
In `@backend/routes/learn.py`:
- Around line 525-532: The messages returned by resume_session are fetched into
msgs via table("messages").select but not decrypted; update resume_session to
reuse the decryption logic from get_conversation_history (or call
get_conversation_history(session_id) instead of returning raw msgs) so each
message's content is decrypted before returning. Locate the msgs variable and
the return that currently returns session_rows[0] and msgs, then either map over
msgs and replace each message's content with the decrypted value using the same
helper used by get_conversation_history, or delegate to get_conversation_history
to produce the decrypted messages, and return that result.
In `@backend/routes/quiz.py`:
- Around line 190-203: The student_name value used in the prompt can be None
because decrypt_if_present(user_rows[0]["name"]) may return None; before calling
.replace("{student_name}", student_name) ensure student_name is a non-null
string (e.g., coerce to a fallback like "Student" or "" if None) so template
replacement never receives None; update the code around where student_name is
assigned (the decrypt_if_present call and subsequent use in
ctx_prompt/_load_prompt) to coerce or default the value and use that sanitized
variable in the .replace call.
In `@backend/routes/social.py`:
- Around line 343-371: send_room_message currently stores body.user_name in
plaintext; update the insert in send_room_message to pass user_name through
encrypt_if_present (same pattern as text) so user_name is encrypted at rest.
Then update get_room_messages to call decrypt_if_present on the message row's
user_name and also on any reply snippet user_name (the reply_to handling
at/around where reply snippets are built) to return decrypted names to clients;
preserve null/None handling consistent with
encrypt_if_present/decrypt_if_present usage.
---
Nitpick comments:
In `@backend/db/backfill_encryption.py`:
- Line 91: The current call rows = table(table_name).select(",".join([pk,
*columns])) loads the entire table into memory; change the logic around
table(table_name).select to iterate in batches (use limit/offset or a cursor)
while selecting the same columns (referencing table_name, pk, columns and the
table(...) call) so you fetch e.g. BATCH_SIZE rows at a time, process them, then
advance the offset (or cursor) and repeat until no rows remain; ensure you
preserve ordering (order by pk) to make pagination deterministic and avoid
loading all rows into memory.
- Around line 162-172: When JSON parsing fails in backfill_encryption.py (the
_json.loads(v) exception path), we currently encrypt the raw string with
encrypt(v) so the value can round-trip through
decrypt_if_present/decrypt_json(); add a clear inline comment near the except
block explaining that decrypt_json() will call json.loads(decrypt(value)) and
therefore decrypted corrupt JSON will re-trigger the JSON parse fallback at read
time, and that this is intentional because the runtime fallback will handle it;
reference the functions _json.loads, encrypt, decrypt_json, and
decrypt_if_present in the comment so future maintainers understand the
round-trip behavior and why some cells may still fail JSON parsing after
decryption.
In `@backend/routes/flashcards.py`:
- Around line 128-137: The code silently swallows exceptions when calling
decrypt_json on d.get("concept_notes"); instead add a warning log inside the
except to surface the failure (include context like the doc id/index and use
exc_info=True or log the exception message) while preserving the fallback
behavior of leaving concept_notes unchanged; reference the decrypt_json call and
d["concept_notes"], and ensure you use the module logger (e.g., obtain
logging.getLogger(__name__) or reuse an existing logger variable) before
logging.
In `@backend/routes/learn.py`:
- Around line 231-239: Update the function signature for save_message to use an
explicit None union type for the graph_update parameter: replace the implicit
Optional style "graph_update: dict = None" with the explicit union form
"graph_update: dict | None = None" (referencing the save_message function and
the graph_update parameter) so the type hint conforms to PEP 484; ensure no
other code changes are required.
In `@backend/routes/onboarding.py`:
- Line 14: The function search_courses currently declares an unused parameter
request: Request which causes confusion; either remove the request parameter
from the function signature of search_courses (and any corresponding route
decorator/handler registration) or, if it's intentionally reserved for future
use (e.g., auth or middleware), add an inline comment above the signature
explaining why request is unused (e.g., "# request kept for future auth guard")
and prefix the variable with an underscore (request -> _request) to signal
intentional non-use. Update any callers or route mappings that expect the old
signature accordingly and ensure imports (Request) are cleaned up if removed.
In `@backend/services/auth_guard.py`:
- Around line 60-61: The except block currently swallows the original exception;
update the handler in auth_guard.py so the broad except Exception captures the
original exception (e.g., except Exception as e:) and re-raise the HTTPException
with exception chaining (raise HTTPException(status_code=401, detail="Not
authenticated") from e) to preserve the original traceback for debugging while
still returning 401 from the authentication function/handler.
In `@backend/tests/test_encryption.py`:
- Around line 99-106: The test test_tampered_ciphertext_raises should assert the
specific AES-GCM tamper exception instead of catching all Exceptions: import
InvalidTag from cryptography.exceptions and change the context manager to with
pytest.raises(InvalidTag): calling encryption.decrypt(tampered). Keep the rest
of the test (encryption.encrypt and bit-flip tampering) unchanged so it still
exercises tamper detection.
In `@frontend/src/app/auth/callback/page.tsx`:
- Around line 70-77: The catch block for the fetch('/api/auth/me') call in
page.tsx silently sets onboardingCompleted = true which can incorrectly skip
onboarding on transient API failures; update the catch to capture the error
(e.g., catch (err)), log it (console.error or your app logger) and set
onboardingCompleted conservatively (false or leave undefined) or implement a
simple retry before deciding, referencing the fetch('/api/auth/me') call and the
onboardingCompleted/local state update to locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c1bef18-e8e8-4608-9891-9e17ab2b8476
📒 Files selected for processing (29)
backend/.env.examplebackend/db/backfill_encryption.pybackend/db/migration_encryption_text_columns.sqlbackend/requirements.txtbackend/routes/admin.pybackend/routes/auth.pybackend/routes/calendar.pybackend/routes/documents.pybackend/routes/flashcards.pybackend/routes/gradebook.pybackend/routes/graph.pybackend/routes/learn.pybackend/routes/onboarding.pybackend/routes/profile.pybackend/routes/quiz.pybackend/routes/social.pybackend/routes/study_guide.pybackend/services/auth_guard.pybackend/services/encryption.pybackend/tests/conftest.pybackend/tests/test_encryption.pybackend/tests/test_flashcard_import_routes.pybackend/tests/test_learn_routes.pybackend/tests/test_onboarding_routes.pybackend/tests/test_shared_course_context.pybackend/tests/test_social_messages.pydocker-compose.ymldocs/superpowers/plans/2026-05-03-aes-256-gcm-column-encryption.mdfrontend/src/app/auth/callback/page.tsx
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
AndresL230
commented
May 3, 2026
Code reviewFound 2 issues:
Sapling/backend/routes/learn.py Lines 524 to 533 in ba27384
Sapling/frontend/src/middleware.ts Lines 43 to 62 in ba27384 Sapling/backend/services/auth_guard.py Lines 16 to 51 in ba27384 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
- learn.py resume_session: decrypt messages.content before returning - middleware.ts: forward sapling_session cookie to backend /me - session/route.ts: require authToken; remove dead unauthenticated /me fallback - encryption.py decrypt_json: fall back to plaintext JSON for legacy rows - documents.py / learn.py / flashcards.py: drop now-unnecessary try/except wrappers around decrypt_json - auth.py: use encrypt_if_present for refresh_token to preserve None - conftest.py: monkeypatch services.auth_guard so lazy route imports inherit the bypass; expose _real_* for tests that need the originals - test_flashcard_import_routes.py: import _real_require_self instead of the bypassed alias Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/app/api/auth/session/route.ts (1)
57-70:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftAlign the
sapling_sessionpayload with backendauth_guard.This route still mints the browser session through
signSession(verifiedUserId), butfrontend/src/lib/sessionToken.ts:32-41signs{ userId, exp }whilebackend/services/auth_guard.py:54-64looks forpayload["user_id"]. After this PR,/api/auth/meand the newly request-authenticated backend routes rely on that cookie, so the frontend will accept the session locally but the backend will 401 it and bounce approved users back to sign-in. Please make both sides read/write the same claim name, ideally with backward-compatible decoding during rollout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/api/auth/session/route.ts` around lines 57 - 70, The frontend is signing session tokens with a payload using the claim name userId while the backend auth_guard expects user_id; update the signer used by signSession (and the payload created in frontend's sessionToken.ts) to emit { user_id, exp } instead of { userId, exp } or add backward-compatible decoding in signSession so it also sets user_id when only userId exists, and ensure verifyAuthToken/signSession continue to produce the cookie name sapling_session with the same maxAge settings so backend(routes using auth_guard.py) will accept the cookie during rollout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/routes/auth.py`:
- Around line 225-230: The calendar token reader must decrypt persisted
ciphertext before building the Credentials object: update the code that
constructs Credentials(...) in backend/routes/calendar.py (where it currently
passes token_row["access_token"] and token_row.get("refresh_token")) to call
decrypt on access_token and decrypt_if_present on refresh_token (or handle
None/empty), and convert the stored expires_at ISO string back to a datetime for
expiry; ensure the same decrypt helper used for encrypt_if_present is referenced
so refresh/decode works after the Google callback writes encrypted tokens.
---
Outside diff comments:
In `@frontend/src/app/api/auth/session/route.ts`:
- Around line 57-70: The frontend is signing session tokens with a payload using
the claim name userId while the backend auth_guard expects user_id; update the
signer used by signSession (and the payload created in frontend's
sessionToken.ts) to emit { user_id, exp } instead of { userId, exp } or add
backward-compatible decoding in signSession so it also sets user_id when only
userId exists, and ensure verifyAuthToken/signSession continue to produce the
cookie name sapling_session with the same maxAge settings so backend(routes
using auth_guard.py) will accept the cookie during rollout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 01b8fe9a-7972-4263-9df1-4a7b944fcea4
📒 Files selected for processing (9)
backend/routes/auth.pybackend/routes/documents.pybackend/routes/flashcards.pybackend/routes/learn.pybackend/services/encryption.pybackend/tests/conftest.pybackend/tests/test_flashcard_import_routes.pyfrontend/src/app/api/auth/session/route.tsfrontend/src/middleware.ts
✅ Files skipped from review due to trivial changes (2)
- backend/tests/test_flashcard_import_routes.py
- backend/tests/conftest.py
| # Email-based account merge is disabled because emails are now encrypted | ||
| # with random nonces; equality lookups by plaintext email cannot match. | ||
| # New sign-ins for users without a google_id always create a fresh row. | ||
| user_id = f"user_{google_id}" | ||
| is_approved = False | ||
| table("users").insert({ | ||
| "id": user_id, | ||
| "name": encrypt_if_present(name), | ||
| "first_name": encrypt_if_present(first_name), | ||
| "last_name": encrypt_if_present(last_name), | ||
| "email": encrypt_if_present(email), | ||
| "google_id": google_id, | ||
| "avatar_url": avatar_url, | ||
| "auth_provider": "google", | ||
| }) |
There was a problem hiding this comment.
This will orphan pre-existing accounts that don't already have google_id.
When no google_id match exists, the new path always inserts user_{google_id} with is_approved = False. Any existing approved user created before google_id was populated will end up with a second account and lose access to their original courses, documents, graph, and approval state on next sign-in. This needs a transition strategy before merge, such as a one-time backfill of google_id or a temporary legacy-link path that can still find the old row.
| table("oauth_tokens").upsert( | ||
| { | ||
| "user_id": user_id, | ||
| "access_token": creds.token, | ||
| "refresh_token": creds.refresh_token or "", | ||
| "access_token": encrypt(creds.token), | ||
| "refresh_token": encrypt_if_present(creds.refresh_token), | ||
| "expires_at": creds.expiry.isoformat() if creds.expiry else "", |
There was a problem hiding this comment.
Update the calendar token reader in the same rollout.
These writes now persist ciphertext, but backend/routes/calendar.py:35-71 still passes token_row["access_token"] and token_row.get("refresh_token") directly into Credentials(...). After the first successful Google callback, calendar refresh/sync will start using encrypted strings and fail. Please land the matching decrypt-on-read change before shipping this write path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/routes/auth.py` around lines 225 - 230, The calendar token reader
must decrypt persisted ciphertext before building the Credentials object: update
the code that constructs Credentials(...) in backend/routes/calendar.py (where
it currently passes token_row["access_token"] and
token_row.get("refresh_token")) to call decrypt on access_token and
decrypt_if_present on refresh_token (or handle None/empty), and convert the
stored expires_at ISO string back to a datetime for expiry; ensure the same
decrypt helper used for encrypt_if_present is referenced so refresh/decode works
after the Google callback writes encrypted tokens.
Uh oh!
There was an error while loading. Please reload this page.
Summary
Adds AES-256-GCM column-level encryption for user PII, OAuth tokens, assignment notes/points, document summaries/concept notes, session summaries, and chat messages. Includes encrypt-on-write + decrypt-on-read across all affected routes, a schema migration that retypes encrypted columns to TEXT, and an idempotent backfill script for existing rows.
Rollout — three things you need to do
1. What to apply to Supabase
Run one SQL file in the Supabase SQL editor:
backend/db/migration_encryption_text_columns.sql. It contains fourALTER TABLE ... ALTER COLUMN ... TYPE TEXT USING ::TEXTstatements.Apply it once, before deploying the new code. The casts preserve existing rows as TEXT (e.g.,
87.5becomes the string"87.5"); thedecrypt_if_present/decrypt_numericfallbacks keep those legacy rows readable until you backfill.2. Values you need to set
Just one new env var.
ENCRYPTION_KEY= 64 hex chars (32 bytes). Generate it with:python -c "import secrets; print(secrets.token_hex(32))"Set it in:
backend/.envfor local dev (backend/.env.examplealready has the placeholder + the generation command).SUPABASE_SERVICE_KEYis set today).docker-compose.ymlalready passes it through frombackend/.env.Two non-negotiables:
The backend will refuse to boot if
ENCRYPTION_KEYis missing, not 64 chars, or not valid hex (intentional fail-fast).3. The backfill script
backend/db/backfill_encryption.py. For each encrypted column on each table (users.name/email/bio/...,oauth_tokens.access_token/refresh_token,assignments.notes/points_*,documents.summary/concept_notes,sessions.summary_json,messages.content,room_messages.text), it:decrypt(value)once.Prints counts per column. Idempotent — running it a second time changes nothing. Defaults to dry-run (counts only);
--applyto actually write.--table usersto do it in stages.How to actually run all of this, in order
After the backfill completes successfully, the
decrypt_if_presentfallback-to-raw-value warnings should stop appearing in logs. That's the signal it worked.Test plan
migration_encryption_text_columns.sqlto a Supabase branch DBENCRYPTION_KEY(64-hex) and confirm backend boots; confirm it refuses to boot when the var is missing/invalidcd backend && python -m pytest tests/ -q(includes newtest_encryption.py)users.name/emailwritten as ciphertext,/mereturns plaintextdocuments.summary/concept_notesciphertext at rest, decrypt on library/study-guide/flashcards readsoauth_tokens.access_token/refresh_tokenandassignments.notes/points_*encryptedroom_messages.textandmessages.contentencrypted--applyand re-run to confirm idempotency (zero changes)decrypt_if_presentfallback warnings disappear from logs after backfillSummary by CodeRabbit
New Features
Improvements
Chores
Tests & Docs