fix(EncryptedSession): add strict mode to reject non-encrypted items - #4376
fix(EncryptedSession): add strict mode to reject non-encrypted items#4376SashaMIT wants to merge 1 commit into
Conversation
EncryptedSession wraps an untrusted-at-rest store with Fernet AEAD, but the read path returned any item lacking the __enc__ envelope verbatim, so unauthenticated plaintext could enter model context with no signal. Add allow_plaintext_passthrough (default True, preserving current behavior for plaintext-to-encrypted migration). When set to False, non-envelope items are dropped with a warning log on both the get_items and pop_item read paths.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a99cfd9ec0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not self._allow_plaintext_passthrough: | ||
| logger.warning( | ||
| "EncryptedSession dropping non-encrypted item from session store " | ||
| "(allow_plaintext_passthrough=False)." | ||
| ) | ||
| return None |
There was a problem hiding this comment.
When strict mode encounters a plaintext tail, returning None here causes the existing pop_item() loop to discard that record and continue until it also removes and returns an older encrypted record. A single pop can therefore delete multiple persisted items; more critically, if a plaintext record is appended concurrently after Runner verifies an attempt-owned suffix, retry rewind silently deletes that unowned record and still reports success after receiving the expected encrypted item. Strict rejection must not destructively scan past an unowned tail.
AGENTS.md reference: AGENTS.md:L155-L156
Useful? React with 👍 / 👎.
| logger.warning( | ||
| "EncryptedSession dropping non-encrypted item from session store " | ||
| "(allow_plaintext_passthrough=False)." | ||
| ) |
There was a problem hiding this comment.
When strict mode is used with a positive history limit and rejected items prevent the first fetch window from satisfying that limit, get_items() doubles the window and runs _unwrap_valid_items() over the entire suffix again. Emitting the warning inside _unwrap() therefore logs the same plaintext record on every expansion; a plaintext-heavy or attacker-controlled store can generate roughly twice as many warning records as stored items on every model turn, repeatedly consuming logging I/O and storage. Emit a bounded warning once per public read, optionally with a rejection count, rather than once per unwrap attempt.
Useful? React with 👍 / 👎.
|
Thanks for the thoughtful hardening proposal. The current plaintext passthrough is real, but this PR does not establish a supported scenario where |
|
Thanks for the careful read, that makes sense. The pop scan-past behavior was the part I was least sure of, and you're right that a writable store can delete or reorder regardless, so the flag only ever covered a slice of the integrity question. Appreciate the quick review either way. |
2 participants
Summary
EncryptedSessionwraps an underlying session store with Fernet AEAD so stored conversation items are encrypted and integrity-protected at rest. The read path, however, currently returns any stored item that lacks the__enc__envelope completely verbatim:_unwrappasses non-envelope items straight through, soget_itemsandpop_itemfeed unauthenticated plaintext into model context with no signal to the operator.That passthrough is useful when migrating a plaintext store onto encrypted storage, so this PR keeps it as the default and adds an opt-in strict mode rather than changing behavior:
allow_plaintext_passthrough(defaultTrue, behavior identical to today).False, non-envelope items are dropped with a warning log on both read paths (get_itemsvia_unwrap_valid_items, and thepop_itemretry loop), so only authenticated ciphertext reaches the model.This is a hardening / fail-closed option, not a vulnerability fix: planting items already requires write access to the underlying store, but operators who treat the store as untrusted at rest now have a way to enforce the integrity the wrapper otherwise implies.
A minimum-length check for raw-string
encryption_keyvalues was considered but left out to avoid breaking existing deployments. Happy to add a short note todocs/sessions/encrypted_session.mdif you would like that in this PR.Test plan
pytest tests/extensions/memory/test_encrypt_session.py: 24 passed (4 new)pop_itemskips non-envelope itemsruff check,ruff format --check, andmypyclean on touched filesMade with Cursor