Read the channel's data encoder live when encoding annotations - #2231
Draft
SimonWoolf wants to merge 1 commit into
Draft
Read the channel's data encoder live when encoding annotations#2231SimonWoolf wants to merge 1 commit into
SimonWoolf wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
github-actions
Bot
temporarily deployed
to
staging/pull/2231/markdown-api-reference
July 29, 2026 10:13
Inactive
ARTRestAnnotationsInternal and ARTRealtimeAnnotationsInternal each captured _channel.dataEncoder into an ivar at construction. ARTChannel replaces its _dataEncoder with a new instance whenever setOptions is called (recreateDataEncoderWith:), and there is no hook to refresh the annotations objects' copies, so after a setOptions that added or changed cipher params the annotations objects kept using the encoder built at channel-creation time. Annotation data would then be published and decoded with the stale cipher config (typically none at all) while messages, which read the channel's dataEncoder property live, correctly picked up the new cipher. Read _channel.dataEncoder at each encode/decode site instead of caching it, matching ARTChannel's encodeMessageIfNeeded: and ARTRealtimePresence's publish path. The REST decode path already read it live. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SimonWoolf
force-pushed
the
annotation-cipher-setoptions
branch
from
July 29, 2026 10:54
1c2ba67 to
b91ad99
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ARTRestAnnotationsInternalandARTRealtimeAnnotationsInternaleach captured_channel.dataEncoderinto an ivar at construction time. ButARTChannelreplaces its_dataEncoderwith a brand-new instance wheneversetOptionsis called (recreateDataEncoderWith:,ARTChannel.m:63), and there is no hook to refresh the annotations objects' copies.So after a
setOptionsthat added or changed cipher params, annotation data was encoded and decoded with the encoder built at channel-creation time — typically one with no cipher at all — while messages correctly picked up the new cipher, becauseencodeMessageIfNeeded:reads thedataEncoderproperty live (ARTChannel.m:252).ARTRealtimePresence's publish path also reads it live (ARTRealtimePresence.m:596), deliberately bypassing its own cached ivar.Fix: read
_channel.dataEncoderat each encode/decode site rather than caching it. Four sites; the REST decode path (ARTRestAnnotations.m:268) already read it live, which was itself an inconsistency within that file.Scope
This is not the same bug as the one being fixed in ably-js (ably-js#2277), where annotation data was never encrypted at all. ably-cocoa is correct on the normal path: a channel constructed with cipher options encrypts annotation data properly, because construction order guarantees the captured encoder is already cipher-bearing.
This is the narrower latent case — a channel reconfigured via
setOptionsafter creation. It also fails symmetrically (annotations stay internally consistent, just encrypted under the wrong policy — in practice not encrypted), so unlike the ably-js bug it wouldn't corrupt a round trip within a single client. It surfaces as annotation data sitting in plaintext on the service, and as garbling against a client that constructed its channel with the cipher up front.Found while auditing the other SDKs for the ably-js bug. Java is unaffected; Python has the ably-js bug and worse (its decode path is broken too).
Test
test__annotation_data_uses_a_cipher_added_by_setOptions_after_channel_creationinRealtimeAnnotationsTests.swiftcreates a channel with no cipher, adds one viasetOptions, then publishes and receives an annotation with a data payload and asserts it round-trips.Before the fix the annotation is published as plaintext and arrives as ciphertext, so the assertion fails.
Worth noting there was no existing test coverage for annotations on encrypted channels at all — neither
RealtimeAnnotationsTests.swiftnorRestAnnotationsTests.swiftcontained a single reference tocipherorencrypt. That gap is what let this sit unnoticed.CI status
The new test passes on all three platforms (macOS, iOS, tvOS), and the SDK compiles cleanly everywhere. EditorConfig passes.
The three
checkjobs are nonetheless red, on a pre-existing flaky failure unrelated to this change:test__annotations_subscribe_should_implicitly_attach_the_channel_if_options_attachOnSubscribe_is_true, failing withError 80017 - connection broken before receiving publishing acknowledgment. That test never publishes an annotation and never configures a cipher — it only exercises attach/detach lifecycle — so it cannot be affected by this change. It also passed on an earlier run of this same branch. tvOS additionally failed a presence test (test__050__Presence__PresenceMap__...).For reference, the last three
Integration Testruns onmaineach failed a different test (Presence.test__038,Presence.test__039,RestClientStats.test__002) — none repeating, which is the signature of sandbox/network flakiness rather than a regression.Note the REST path has no equivalent test; the realtime one covers the shared mechanism.
An earlier revision of this test used
reaction:distinct.v1and failed withError 40000 - Aggregation method not allowed for anonymous client publish; allowed methods are: [multiple.v1 total.v1]. Now usesreaction:multiple.v1, matching the other tests in the file.🤖 Generated with Claude Code