Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 0
docs(adr): 0025 — encrypt course_chunks.chunk_text (#484)#503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,96 @@ | ||||||||||||||||||||||
| # 0025: Encrypt `course_chunks.chunk_text` — restore the encryption boundary, but the embedding stays plaintext | ||||||||||||||||||||||
| - Status: accepted | ||||||||||||||||||||||
| - Date: 2026-07-31 | ||||||||||||||||||||||
| - Relates to: #484 (this decision), #483 (blocked on it), #482 (RAG hardening), | ||||||||||||||||||||||
| #231 (storage/RLS lockdown), migration 0030 (`documents.extracted_text`), | ||||||||||||||||||||||
| migration 0039 (the vector store) | ||||||||||||||||||||||
| - Supersedes: none | ||||||||||||||||||||||
| ## Context | ||||||||||||||||||||||
| #484 names a real asymmetry. `documents.extracted_text` is encrypted (0030) | ||||||||||||||||||||||
| because it is student-uploaded content; `course_chunks.chunk_text` holds *the | ||||||||||||||||||||||
| same text, chunked*, in plaintext. One column is treated as PII and the other | ||||||||||||||||||||||
| isn't, for no reason anyone wrote down. | ||||||||||||||||||||||
| **The issue's stated premise is wrong, and it matters.** It assumes "pgvector | ||||||||||||||||||||||
| similarity can't run over ciphertext." `match_course_chunks` (0039) computes | ||||||||||||||||||||||
| `1 - (c.embedding <=> query_embedding)`, orders by `c.embedding <=> | ||||||||||||||||||||||
| query_embedding`, and filters `WHERE c.embedding IS NOT NULL`. `chunk_text` is | ||||||||||||||||||||||
| only ever SELECTed as payload — the ranking never reads it. Nothing in the | ||||||||||||||||||||||
| codebase queries it by content either (no `ILIKE` / `LIKE` / FTS; verified | ||||||||||||||||||||||
| across `services/`, `routes/`, `scripts/`). So encryption does not block | ||||||||||||||||||||||
| retrieval at all, and this decision is far cheaper than the issue implies. | ||||||||||||||||||||||
| But the correction cuts both ways, and this is the part worth recording: **the | ||||||||||||||||||||||
| reason it's cheap is the reason it's partial.** The `embedding` column cannot | ||||||||||||||||||||||
| be encrypted — pgvector must compute distance over it — and an embedding is a | ||||||||||||||||||||||
| lossy but real representation of its source text; embedding-inversion | ||||||||||||||||||||||
| techniques recover substantial content from vectors alone. Encrypting | ||||||||||||||||||||||
| `chunk_text` therefore does *not* make the row opaque. | ||||||||||||||||||||||
| Threat model, for calibration: the backend connects with the service-role key | ||||||||||||||||||||||
| and RLS locks out `anon`/`authenticated` (#231), so direct table reads imply a | ||||||||||||||||||||||
| Supabase credential compromise or an insider. `ENCRYPTION_KEY` is a separate | ||||||||||||||||||||||
| secret held in the app environment, so column encryption genuinely raises the | ||||||||||||||||||||||
| bar against a database-only compromise — the same bar every other encrypted | ||||||||||||||||||||||
| column is already set at. | ||||||||||||||||||||||
| ## Decision | ||||||||||||||||||||||
| Encrypt `chunk_text` for **every** row in `course_chunks` — document *and* | ||||||||||||||||||||||
| catalog — through the standard `encrypt_if_present` / `decrypt_if_present` | ||||||||||||||||||||||
| helpers, and compute content-addressed ids on the **plaintext, before | ||||||||||||||||||||||
| encryption**. | ||||||||||||||||||||||
| Uniform rather than scoped to document chunks, even though catalog chunks are | ||||||||||||||||||||||
| public BU course-catalog text with nothing to protect: | ||||||||||||||||||||||
| - One invariant — "`chunk_text` is always ciphertext" — is assertable by the | ||||||||||||||||||||||
| existing `ciphertext` e2e oracle. A per-category rule is not. | ||||||||||||||||||||||
| - `decrypt_if_present` returns the **raw value** when it cannot decrypt. In a | ||||||||||||||||||||||
| mixed table that makes a genuine decrypt failure indistinguishable from a | ||||||||||||||||||||||
| legitimately-plaintext catalog row, which is precisely the kind of silent | ||||||||||||||||||||||
| degradation #482 was about. | ||||||||||||||||||||||
Comment on lines
+52
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🏁 Script executed: #!/bin/bashset -euo pipefail
rg -n -C 12 \
-e 'def (encrypt_if_present|decrypt_if_present)' \
-e '\bdecrypt_if_present\b' \
-e '\bplan_migration\b' \
-e 'backfill_encryption' \
backend ||trueRepository: SaplingLearn/Sapling Length of output: 50379 🏁 Script executed: #!/bin/bashset -euo pipefail
echo"== relevant file sizes =="
wc -l backend/services/encryption.py backend/scripts/dedupe_course_chunks.py backend/scripts/backfill_document_chunks.py backend/db/backfill_encryption.py docs/decisions/0025-encrypt-rag-chunk-text.md
echoecho"== backend/services/encryption.py =="
cat -n backend/services/encryption.py
echoecho"== backend/scripts/dedupe_course_chunks.py =="
cat -n backend/scripts/dedupe_course_chunks.py
echoecho"== backend/scripts/backfill_document_chunks.py relevant section =="
cat -n backend/scripts/backfill_document_chunks.py | sed -n '1,180p'echoecho"== backend/db/backfill_encryption.py relevant section =="
cat -n backend/db/backfill_encryption.py | sed -n '1,220p'echoecho"== docs/decisions/0025-encrypt-rag-chunk-text.md relevant section =="
cat -n docs/decisions/0025-encrypt-rag-chunk-text.md | sed -n '1,90p'echoecho"== tests mentioning chunk dedupe or encryption migration =="
rg -n -C 4 'dedupe_course_chunks|decrypt_if_present|backfill_document_chunks|chunk_hash|chunkid|chunk_id' backend/tests backend/scripts backend/db docs ||trueRepository: SaplingLearn/Sapling Length of output: 50377 🏁 Script executed: #!/bin/bashset -euo pipefail
python3 - <<'PY'import base64from cryptography.hazmat.primitives.ciphers.aead import AESGCMkey = bytes.fromhex("00"*32)aesgcm = AESGCM(key)plaintext = "hello world"nonce = b"0" * 12ciphertext = aesgcm.encrypt(nonce, plaintext.encode(), None)stored = base64.b64encode(nonce + ciphertext).decode()wrong_aesgcm = AESGCM(bytes.fromhex("11"*32))try: # This ciphertext is valid-looking base64 + AES nonce length for the current helper wrong_aesgcm.decrypt(nonce, ciphertext, None) decryption_succeeds = Trueexcept Exception as exc: decryption_succeeds = False exc_type = type(exc).__name__is_encrypted_fingerprint = stored.isascii() and base64.b64decode(stored)print({ "wrong_key_decrypt_succeeds": decryption_succeeds, "wrong_key_exception_type": exc_type if not decryption_succeeds else None, "ciphertext_base64_len": len(stored), "ciphertext_base64_is_ascii": stored.isascii(),})PYechoecho"== targeted encryption-related tests and scripts =="
rg -n -C 6 'decrypt_if_present.*fallback|fallback.*raw|ENCRYPTION_KEY|ciphertext|course_chunks|backfill_encryption|dedupe_course_chunks|plan_migration|extracted_text' \
backend tests frontend docs/dev \
--glob '!backend/prompts/**' \
--glob '!backend/db/backfill_encryption.py' \
--glob '!backend/services/encryption.py' \
| head -240Repository: SaplingLearn/Sapling Length of output: 283 🌐 Web query:
💡 Result: In the Python cryptography library, the AESGCM.decrypt method raises a cryptography.exceptions.InvalidTag exception when decryption fails [1][2]. This exception is not specific only to an incorrect key; it is raised whenever the authentication tag cannot be validated [1][2]. Because AES-GCM is an authenticated encryption mode, the authentication tag is verified during decryption to ensure the integrity and authenticity of the ciphertext [3]. The verification will fail—triggering the InvalidTag exception—if any of the following are incorrect [1][2]: * The decryption key * The nonce (initialization vector) * The associated data (if any was provided during encryption) * The ciphertext itself (if it has been tampered with or corrupted) The library does not provide a distinct exception for an incorrect key because the nature of authenticated encryption means that from the perspective of the decryption process, any mismatch in these parameters results in an invalid authentication tag [3]. If you encounter this exception, ensure that the key, nonce, and associated data used for decryption match exactly those used during encryption [1][2]. Citations:
Make raw fallback fail closed during migration.
🤖 Prompt for AI Agents | ||||||||||||||||||||||
| - The cost is ~5 AES-GCM decrypts per retrieval (`k=5`). Immaterial. | ||||||||||||||||||||||
| Ids stay keyed on plaintext because AES-GCM uses a random nonce per call: the | ||||||||||||||||||||||
| same plaintext encrypts to different ciphertext every time, so ciphertext can | ||||||||||||||||||||||
| never be a dedup key. `chunk_id(course, text)` remains the merge key, and | ||||||||||||||||||||||
| re-uploads of identical content still converge on one row. | ||||||||||||||||||||||
Comment on lines
+58
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Preserve the separate document and catalog ID namespaces. The existing contract is not one Proposed clarification-Ids stay keyed on plaintext because AES-GCM uses a random nonce per call: the-same plaintext encrypts to different ciphertext every time, so ciphertext can-never be a dedup key. `chunk_id(course, text)` remains the merge key, and-re-uploads of identical content still converge on one row.+Ids stay keyed on plaintext because AES-GCM uses a random nonce per call: the+same plaintext encrypts to different ciphertext every time, so ciphertext can+never be a dedup key. Preserve the existing per-kind namespaces: document rows+use `chunk_id(course, text)` and catalog rows use `sha256(course::text)`.+Encryption occurs after ID derivation, so identical content still converges+within each row kind.This follows the ID contract documented in 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents | ||||||||||||||||||||||
| **This ADR does not claim chunk confidentiality.** It restores boundary | ||||||||||||||||||||||
| consistency and removes trivially-readable plaintext. The embedding remains, | ||||||||||||||||||||||
| and it is the residual exposure. | ||||||||||||||||||||||
| ## Consequences | ||||||||||||||||||||||
| - (+) The encryption boundary is consistent: the same student text is | ||||||||||||||||||||||
| protected in `documents.extracted_text` and in the chunks derived from it. | ||||||||||||||||||||||
| - (+) Retrieval is unaffected — ranking never touched `chunk_text`. | ||||||||||||||||||||||
| - (+) Unblocks #483. Notes indexing would otherwise write decrypted note | ||||||||||||||||||||||
| bodies (`notes.body` is encrypted) into a plaintext column, deepening the | ||||||||||||||||||||||
| very asymmetry this closes. | ||||||||||||||||||||||
| - (+) A uniform invariant the `ciphertext` oracle can assert, so a future | ||||||||||||||||||||||
| regression fails a lane instead of sitting unnoticed. | ||||||||||||||||||||||
| - (−) **Residual exposure: the embedding stays plaintext and is partially | ||||||||||||||||||||||
| invertible.** This is defense in depth, not confidentiality. Anyone reading | ||||||||||||||||||||||
| this ADR to answer "is chunk content protected?" must read this line. | ||||||||||||||||||||||
| - (−) `scripts/dedupe_course_chunks.py:70` re-derives ids from the **stored** | ||||||||||||||||||||||
| `chunk_text`. Run against encrypted rows it would hash ciphertext and | ||||||||||||||||||||||
| destroy content-addressing. It must decrypt before hashing, or be retired — | ||||||||||||||||||||||
| its own docstring calls it a one-time migration. | ||||||||||||||||||||||
| - (−) A backfill is required for existing rows (the | ||||||||||||||||||||||
| `db/backfill_encryption.py` precedent). Until it completes the table is | ||||||||||||||||||||||
| mixed, carried by `decrypt_if_present`'s raw-value fallback. | ||||||||||||||||||||||
Comment on lines
+75
to
+86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win Define the ciphertext-oracle rollout order. The ADR says the oracle should assert that every 🤖 Prompt for AI Agents | ||||||||||||||||||||||
| - (−) Catalog chunks pay encryption cost for no privacy benefit. Accepted as | ||||||||||||||||||||||
| the price of the uniform invariant. | ||||||||||||||||||||||
| ## Implementation (not done here) | ||||||||||||||||||||||
| Write sites: `services/rag_service.py::index_document_chunks` and | ||||||||||||||||||||||
| `scripts/ingest_catalog.py`. Read site: | ||||||||||||||||||||||
| `services/rag_service.py::retrieve_chunks`, decrypting each returned chunk | ||||||||||||||||||||||
| before `format_rag_context`. Plus the dedupe-script fix above, a backfill, and | ||||||||||||||||||||||
| extending the `ciphertext` oracle to cover the column. | ||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the issue reference so Markdownlint can parse the paragraph.
Line 12 starts with
#484without a space. Markdownlint reports MD018 because it parses this as an invalid ATX heading. Replace it withIssue#484names a real asymmetry.Proposed fix
📝 Committable suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 12-12: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Source: Linters/SAST tools