Skip to content

Encrypt annotation data payloads on encrypted channels - #680

Draft
SimonWoolf wants to merge 1 commit into
mainfrom
encrypted-annotation-data
Draft

Encrypt annotation data payloads on encrypted channels#680
SimonWoolf wants to merge 1 commit into
mainfrom
encrypted-annotation-data

Conversation

@SimonWoolf

@SimonWoolf SimonWoolf commented Jul 29, 2026

Copy link
Copy Markdown
Member

Annotation had no encrypt() method at all, so annotation data was published in plaintext even when the channel had a cipher configured — while Message (ably/types/message.py:235) and PresenceMessage (ably/types/presence.py:123) were correctly encrypted before serialisation at their publish sites (ably/rest/channel.py:80, ably/realtime/channel.py:415).

The REST get() path compounded it by hardcoding cipher=None when building its response handler, so it could not have decrypted an encrypted annotation either. The realtime receive path (ably/realtime/channel.py:768) already passed cipher=self.cipher correctly, so it was only the REST direction that was broken.

Changes

  • Add Annotation.encrypt(), mirroring Message.encrypt().
  • Call it from both publish paths (rest/annotations.py, realtime/annotations.py), guarded on channel.cipher like the message paths are.
  • Pass the channel's cipher when decoding REST get() responses.
  • Remove three docstrings asserting annotations are not encrypted (see below).

One deliberate difference from Message.encrypt(): annotations very commonly carry no data at all — a reaction is fully described by its name, and every existing annotation test passes data=None. Message.encrypt() would raise TypeError: Unexpected object type <class 'NoneType'> from TypedBuffer.from_obj() on that input; it just never gets called that way in practice. Annotation.encrypt() returns early instead. Worth noting Message.encrypt() has the same latent flaw if it is ever called with data=None, but I have left it alone.

🤖 Generated with Claude Code

Annotation had no encrypt() method, so annotation data was published in
plaintext even when the channel had a cipher configured, while Message and
PresenceMessage were correctly encrypted before serialisation.

The REST get() path compounded this by hardcoding cipher=None when building
its response handler, so it could not have decrypted an encrypted annotation
either. (The realtime receive path already passed the channel's cipher.)

Add Annotation.encrypt(), mirroring Message.encrypt(), and call it from both
publish paths. Pass the channel's cipher when decoding REST get() responses.
Unlike messages, annotations commonly carry no data at all, so encrypt()
returns early rather than failing in TypedBuffer.from_obj().

The docstrings claiming annotations are not encrypted because the server needs
to parse them for summarisation were correct for the original early-preview
API, where aggregation read JSON data payloads. That changed before public
release, when aggregation moved to the count field specifically so the server
would not have to read payloads.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d3595a84-cacd-4809-a1bd-079c0c266d52

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant