Skip to content

fix(sessions): preserve ciphertext on wrong-key pop - #5018

Open
harshitethic wants to merge 1 commit into
openai:mainfrom
harshitethic:fix/encrypted-session-wrong-key-pop
Open

harshitethic wants to merge 1 commit into
openai:mainfrom
harshitethic:fix/encrypted-session-wrong-key-pop

Conversation

@harshitethic

Copy link
Copy Markdown

Summary

  • distinguish an expired Fernet token from ciphertext that cannot authenticate with the configured key when EncryptedSession.pop_item() unwraps the backend's atomic pop result
  • keep the existing behavior of discarding expired items
  • restore the exact popped encrypted envelope and stop when authentication fails, preventing a wrong key from draining the rest of the recoverable session history
  • add a SQLite-backed regression test proving a wrong-key pop leaves ciphertext intact and the correct key can still read all original messages

Verification

  • git diff --check
  • Python compile check for the modified session module and regression test
  • branch reduced to a single focused commit with no workflow/helper files in the final diff

Fixes #5005

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7366a7f3a8

ℹ️ 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".

Comment on lines +237 to +240
try:
self.cipher.decrypt(token)
except InvalidToken:
return None, True

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not classify corrupt tokens as wrong-key ciphertext

When the newest envelope has a damaged, truncated, or tampered payload, the second decrypt raises the same InvalidToken as a wrong key, so this classifies the corrupt record as recoverable. pop_item() then restores it and returns None, permanently hiding any valid older items behind that corrupt tail instead of isolating it; the implementation needs an independent key identifier or another mechanism that can distinguish a key mismatch from record corruption.

AGENTS.md reference: AGENTS.md:L115-L115

Useful? React with 👍 / 👎.

Comment on lines +324 to +328
await _call_session_method(
self.underlying_session.add_items,
[cast(TResponseInputItem, enc)],
wrapper=wrapper,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restore popped ciphertext atomically

If another writer appends after the backend pop completes but before this add_items call, the old ciphertext is appended after the new item, so a correct-key reader observes reordered history and the next pop targets the wrong tail; if restoration fails, the ciphertext is lost outright. The pop and conditional restoration therefore need a backend-level atomic/transactional operation rather than two independently awaited mutations.

AGENTS.md reference: AGENTS.md:L117-L117

Useful? React with 👍 / 👎.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant