Encrypt annotation data payloads on encrypted channels - #680
Conversation
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>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Disabled knowledge base sources:
WalkthroughAnnotation payloads can now be encrypted for REST and realtime channels with ciphers. Responses pass the channel cipher for decryption, and unit tests cover supported payloads, idempotency, missing data, and cipher-less decoding. ChangesAnnotation encryption flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RestAnnotations
participant RealtimeAnnotations
participant Annotation
participant ChannelCipher
RestAnnotations->>Annotation: encrypt annotation payload
Annotation->>ChannelCipher: encrypt typed data
ChannelCipher-->>Annotation: return CipherData
RestAnnotations->>RestAnnotations: serialize encrypted annotation
RealtimeAnnotations->>Annotation: encrypt annotation payload
Annotation->>ChannelCipher: encrypt typed data
ChannelCipher-->>Annotation: return CipherData
RealtimeAnnotations->>RealtimeAnnotations: serialize encrypted annotation
Possibly related PRs
Poem
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
owenpearson
left a comment
There was a problem hiding this comment.
looks good! there are ci failures on every python version but they seem unrelated (all push admin timeouts)
Annotationhad noencrypt()method at all, so annotationdatawas published in plaintext even when the channel had a cipher configured — whileMessage(ably/types/message.py:235) andPresenceMessage(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 hardcodingcipher=Nonewhen building its response handler, so it could not have decrypted an encrypted annotation either. The realtime receive path (ably/realtime/channel.py:768) already passedcipher=self.ciphercorrectly, so it was only the REST direction that was broken.Changes
Annotation.encrypt(), mirroringMessage.encrypt().rest/annotations.py,realtime/annotations.py), guarded onchannel.cipherlike the message paths are.get()responses.One deliberate difference from
Message.encrypt(): annotations very commonly carry nodataat all — a reaction is fully described by itsname, and every existing annotation test passesdata=None.Message.encrypt()would raiseTypeError: Unexpected object type <class 'NoneType'>fromTypedBuffer.from_obj()on that input; it just never gets called that way in practice.Annotation.encrypt()returns early instead. Worth notingMessage.encrypt()has the same latent flaw if it is ever called withdata=None, but I have left it alone.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes