Skip to content

EncryptedSession.pop_item drains recoverable ciphertext when configured with the wrong key #5005

Description

@kaluli123123

Please read this first

  • Have you read the docs? Yes, the encrypted-session guide and session correction examples.
  • Have you searched for related issues? Yes, searched EncryptedSession and pop_item. This report excludes TTL cleanup, which is covered intentionally by existing tests.

Describe the bug

With an incorrect encryption key, a single EncryptedSession.pop_item() deletes every ciphertext item in the underlying session and returns None. Restoring the correct key afterward cannot recover the conversation. Before the pop, all messages remain readable with the correct key.

A concrete application scenario is an undo/regenerate action after a deployment has loaded the wrong encryption secret. The configuration mistake initially hides history without deleting it (get_items() returns an empty list); one correction action then removes the entire recoverable conversation.

This is not a claim that replacing the key transparently rotates existing ciphertext, or that expired items must remain indefinitely. The reproduction uses freshly written, unexpired items and intentionally demonstrates a configuration error. I am asking whether a destructive operation should distinguish authentication failure from ordinary TTL expiry rather than drain both identically.

Debug information

  • SDK checkout: fbf59a40 (0.22.2).
  • Python: 3.13.14; macOS arm64.
  • Backend: real SQLiteSession in a temporary on-disk database.
  • No model, API key, network, concurrency, timing manipulation, or fabricated backend.
  • Frequency: deterministic.
  • Latest packaged release was not separately installed for this reproduction.

Repro steps

import asyncio
import tempfile
from pathlib import Path
from agents import SQLiteSession
from agents.extensions.memory import EncryptedSession

async def main():
    with tempfile.TemporaryDirectory() as tmp:
        store = SQLiteSession('conversation', str(Path(tmp) / 'history.db'))
        try:
            correct = EncryptedSession(session_id='conversation', underlying_session=store, encryption_key='example-correct-key', ttl=3600)
            wrong = EncryptedSession(session_id='conversation', underlying_session=store, encryption_key='example-wrong-key', ttl=3600)
            messages = [{'role':'user','content':'hello'}, {'role':'assistant','content':'hi'}, {'role':'user','content':'follow-up'}]
            await correct.add_items(messages)
            assert await correct.get_items() == messages
            print('recoverable with correct key before:', len(await correct.get_items()))
            print('wrong-key get_items:', await wrong.get_items())
            print('stored ciphertext after get_items:', len(await store.get_items()))
            print('wrong-key pop_item:', await wrong.pop_item())
            print('stored ciphertext after one pop:', len(await store.get_items()))
            print('recoverable with correct key after:', len(await correct.get_items()))
        finally:
            store.close()

asyncio.run(main())

Observed output:

recoverable with correct key before: 3
wrong-key get_items: []
stored ciphertext after get_items: 3
wrong-key pop_item: None
stored ciphertext after one pop: 0
recoverable with correct key after: 0

Expected behavior / scope clarification

I would expect an incorrect key not to turn a one-item correction into deletion of all recoverable history. An explicit authentication error or a non-destructive refusal would make the configuration problem recoverable. The intended retention policy for this case is not explicitly established in the current docs, so maintainer confirmation of that policy would be useful before choosing a patch.

_unwrap() catches Fernet InvalidToken and returns None for both expiry and authentication failure. pop_item() destructively pops first, then loops whenever _unwrap() returns None (src/agents/extensions/memory/encrypt_session.py:207-216,280-295).

A naive get_items(1) followed by pop_item() is not proposed as a fix: those are separate backend operations and can address different items if another caller mutates the session. A fix should preserve the existing expiry behavior and the underlying pop operation's concurrency guarantees.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions