feat(signal): add typed legacy session interop - #1072
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds public legacy Signal SessionRecord V1 interoperability types and bidirectional conversion with canonical sessions. It also models closed receiver chains explicitly so persisted skipped-message keys remain decryptable after live chain-key bytes are absent. ChangesLegacy session interoperability
Closed receiver-chain decryption
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant LegacySessionRecordV1
participant V1Validation
participant SessionRecord
LegacySessionRecordV1->>V1Validation: validate indexed sessions and chains
V1Validation->>SessionRecord: build canonical SessionComponents
SessionRecord-->>LegacySessionRecordV1: return converted session record
sequenceDiagram
participant SessionCipher
participant SessionState
participant MessageKeyDerivation
SessionCipher->>SessionState: receiver_chain_state(sender)
SessionState-->>SessionCipher: Open or Closed(next_index)
SessionCipher->>MessageKeyDerivation: get_message_keys(counter)
MessageKeyDerivation-->>SessionCipher: persisted skipped-message key
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
| Filename | Overview |
|---|---|
| wacore/libsignal/src/protocol/legacy_session.rs | Adds the typed legacy model, validation, canonical import, operational projection, ordering, pruning, and conversion tests. |
| wacore/libsignal/src/protocol/session_cipher.rs | Continues archived-session search when a recognized chain lacks a skipped key and gives duplicate classification precedence over candidate MAC failures. |
| wacore/libsignal/src/protocol/state/session.rs | Extends canonical receiver-chain state to preserve and consume imported skipped-message keys. |
| wacore/derive/src/lib.rs | Generates strict numeric conversion and deserialization for integer wire enums without fallback variants. |
| wacore/libsignal/Cargo.toml | Adds the opt-in interoperability feature and derive dependency. |
| Cargo.toml | Forwards the interoperability feature through the root crate. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Decoded legacy session] --> B[Validate legacy state]
B --> C[Order and prune sessions]
C --> D[Build canonical SessionRecord]
D --> E[Canonical decrypt search]
D --> F[Operational legacy projection]
Reviews (4): Last reviewed commit: "fix(signal): log terminal duplicate clas..." | Re-trigger Greptile
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59d451c6e9
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/libsignal/src/protocol/legacy_session.rs`:
- Around line 2044-2049: Update debug_output_never_contains_key_material to
assert that the output does not contain a repeated byte value present in
reference_session(80)'s key material, such as 84, instead of 80. Keep the
existing <redacted> assertion and ensure the negative check would fail if key
material were emitted by Debug.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0dda9eae-5a7b-4ded-8a34-0cb5f9b3eb6a
📒 Files selected for processing (5)
wacore/libsignal/src/protocol/legacy_session.rswacore/libsignal/src/protocol/mod.rswacore/libsignal/src/protocol/session_cipher.rswacore/libsignal/src/protocol/state/mod.rswacore/libsignal/src/protocol/state/session.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 053e76bcff
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
wacore/libsignal/src/protocol/session_cipher.rs (1)
2130-2133: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRound-trip the record through persistence in this regression.
This rebuilds an in-memory
SessionRecord, but never serializes and reloads it. Add a serialization/reload round-trip before decryptingfirstto verify that the archived skipped key survives actual persisted state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/libsignal/src/protocol/session_cipher.rs` around lines 2130 - 2133, Update the regression setup around bob_sessions and SessionRecord::from_components to serialize the rebuilt record through the existing persistence mechanism, reload it, and use the reloaded record before decrypting first. Preserve the test’s existing session address and decryption flow while ensuring the archived skipped key is validated from persisted state.wacore/libsignal/src/protocol/legacy_session.rs (1)
415-416: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis error text lies about what actually went wrong. Fix it before it ships.
InvalidSignedPreKeyIdsays "is out of range," but per the library-context notes, out-of-range validation for the signed pre-key id was removed. The only remaining trigger (line 1137-1140) is a missingsigned_pre_key_id(None), not an out-of-range value. Ship a message that actually describes the failure — I don't want engineers chasing ghosts in the logs.🩹 Proposed fix
- #[error("legacy session {session} signed pre-key id is out of range")] + #[error("legacy session {session} is missing a signed pre-key id")] InvalidSignedPreKeyId { session: usize },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/libsignal/src/protocol/legacy_session.rs` around lines 415 - 416, Update the error message for InvalidSignedPreKeyId to describe a missing signed pre-key id rather than an out-of-range value. Keep the error variant and its session field unchanged, and align the text with the None-triggered failure at the existing call site.wacore/derive/src/lib.rs (1)
1117-1226: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMake
ConnectFailureReasonexplicit about missing fallback resilience
VideoStateandTempBanReasonstill have#[wire_fallback], which is appropriate.ConnectFailureReasonomits it, but that may intentionally hard-fail on unknown server codes before logging out. Make that contract explicit in the enum docs so a future refactoring does not accidentally re-add forward-compatibility where strict failure was meant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/derive/src/lib.rs` around lines 1117 - 1226, Update the documentation for ConnectFailureReason to explicitly state that it intentionally omits #[wire_fallback] and strictly rejects unknown server codes. Preserve the existing strict conversion behavior and do not add fallback handling; clarify that this hard failure is intentional before logout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/derive/src/lib.rs`:
- Around line 1188-1199: Remove the unwrap from the strict_from_arms
construction by matching directly on i.wire.as_ref() and handling the expected
VariantWire::Int case without re-destructuring. Preserve the existing literal
generation and Ok arm output for valid integer variants, while using the
established non-panicking handling for any unexpected wire variant.
In `@wacore/libsignal/src/protocol/session_cipher.rs`:
- Around line 1272-1287: Update StateDecryptError to derive thiserror::Error,
annotate Protocol for transparent source conversion from SignalProtocolError,
and add the corresponding transparent conversion for InvalidSessionError if
supported by the enum design. Remove the manual From<SignalProtocolError> and
From<InvalidSessionError> implementations while preserving the existing
MissingClosedChainMessageKey variant.
---
Outside diff comments:
In `@wacore/derive/src/lib.rs`:
- Around line 1117-1226: Update the documentation for ConnectFailureReason to
explicitly state that it intentionally omits #[wire_fallback] and strictly
rejects unknown server codes. Preserve the existing strict conversion behavior
and do not add fallback handling; clarify that this hard failure is intentional
before logout.
In `@wacore/libsignal/src/protocol/legacy_session.rs`:
- Around line 415-416: Update the error message for InvalidSignedPreKeyId to
describe a missing signed pre-key id rather than an out-of-range value. Keep the
error variant and its session field unchanged, and align the text with the
None-triggered failure at the existing call site.
In `@wacore/libsignal/src/protocol/session_cipher.rs`:
- Around line 2130-2133: Update the regression setup around bob_sessions and
SessionRecord::from_components to serialize the rebuilt record through the
existing persistence mechanism, reload it, and use the reloaded record before
decrypting first. Preserve the test’s existing session address and decryption
flow while ensuring the archived skipped key is validated from persisted state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ba762144-8634-4ce3-ab88-89a5bf47cab6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
wacore/derive/src/lib.rswacore/libsignal/Cargo.tomlwacore/libsignal/src/protocol/legacy_session.rswacore/libsignal/src/protocol/session_cipher.rs
A skipped key missing from a recognized open chain now falls through to the archived-session search, matching the closed-chain path, and the final classification prefers a recognized duplicate over candidate BadMac noise so replays cannot trigger retry receipts. The internal StateDecryptError wrapper is gone; both cases reuse DuplicatedMessage. The reverse projection rejects a pending pre-key base that differs from the session base key instead of emitting v1 state its own importer refuses, and previousCounter -1 from a never-used sending chain imports as zero while other out-of-range values fail typed. Also renames MissingSignedPreKeyId to describe its only trigger, drops unwraps from the derive int-mode arms, and reloads the rebuilt record through the wire format in the skipped-key regression tests.
The legacy-session-interop feature (default off, forwarded through wacore and the root crate) compiles the SessionRecord v1 typed model and its conversions out of native builds, so only migration consumers carry the interop surface. The session-search behavior stays ungated because canonical records reach the same states.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wacore/libsignal/src/protocol/session_cipher.rs (1)
1361-1392: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Closedbranch logic is sound — but where's the test for thecounter >= next_indexrejection?The bounds check at Lines 1373-1377 is a brand-new failure mode (
InvalidSessionStructure("receiver chain is closed")) for a message that's genuinely beyond recovery once the chain key bytes are gone. None of the three new regression tests exercise it — they all stay undernext_index. We don't ship security-critical ratchet code without a test for every new branch; add a case wherecounteris at or pastnext_indexon a closed chain and assert the error.🧪 Sketch of the missing test
#[test] fn closed_receiver_chain_rejects_a_counter_at_or_past_next_index() { let (mut tp, first, mut components) = setup_skipped_key_scenario(); let mut rng = rand::make_rng::<rand::rngs::StdRng>(); let current = components.current_session.as_mut().expect("current"); let receiver = receiver_chain_for(current, &first); receiver.chain_key.as_mut().expect("chain key").key = None; // Leave next_index as-is, but drop the skipped key for `first` so the // only remaining candidate for a counter >= next_index is rejected. receiver.message_keys.retain(|k| k.index != Some(first.counter())); install_record(&mut tp, components); futures::executor::block_on(async { let err = message_decrypt_signal( &first, &tp.alice_addr, &mut tp.bob_sessions, &mut tp.bob_identity, &mut rng, ) .await .expect_err("counter beyond a closed chain's next_index must fail"); assert!(matches!(err, SignalProtocolError::InvalidMessage(_, _))); }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/libsignal/src/protocol/session_cipher.rs` around lines 1361 - 1392, Add a regression test alongside the existing session-cipher tests for `decrypt_with_pending_state`’s closed receiver-chain branch, using the skipped-key setup to clear the chain key, remove the relevant message key, and submit a counter equal to or greater than `next_index`; assert decryption fails with the expected invalid-message-wrapped error while preserving the existing tests.wacore/derive/src/lib.rs (1)
1162-1199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood, the
.unwrap()from the last round is gone. Now let's stop writing the same match arm three times.
code_arms,from_arms, andstrict_from_armseach re-derive(id, lit)fromi.wire.as_ref()with an identicallet Some(VariantWire::Int(n)) = ... else { unreachable!() }guard — the only difference is the trailingquote!. Every time a field is added to this shape, three near-identical closures need to change in lockstep. Pull the(id, lit)pair once and map each output from it.♻️ Proposed consolidation
- let code_arms: Vec<_> = infos - .iter() - .filter(|i| !i.is_fallback) - .map(|i| { - let id = &i.ident; - let Some(VariantWire::Int(n)) = i.wire.as_ref() else { - unreachable!() - }; - let lit = proc_macro2::Literal::i32_suffixed(*n); - quote! { `#name`::`#id` => `#lit` } - }) - .collect(); - - let from_arms: Vec<_> = infos - .iter() - .filter(|i| !i.is_fallback) - .map(|i| { - let id = &i.ident; - let Some(VariantWire::Int(n)) = i.wire.as_ref() else { - unreachable!() - }; - let lit = proc_macro2::Literal::i32_suffixed(*n); - quote! { `#lit` => `#name`::`#id` } - }) - .collect(); - - let strict_from_arms: Vec<_> = infos - .iter() - .filter(|i| !i.is_fallback) - .map(|i| { - let id = &i.ident; - let Some(VariantWire::Int(n)) = i.wire.as_ref() else { - unreachable!() - }; - let lit = proc_macro2::Literal::i32_suffixed(*n); - quote! { `#lit` => ::core::result::Result::Ok(`#name`::`#id`) } - }) - .collect(); + let known: Vec<_> = infos + .iter() + .filter(|i| !i.is_fallback) + .map(|i| { + let Some(VariantWire::Int(n)) = i.wire.as_ref() else { + unreachable!() + }; + (&i.ident, proc_macro2::Literal::i32_suffixed(*n)) + }) + .collect(); + let code_arms: Vec<_> = known.iter().map(|(id, lit)| quote! { `#name`::`#id` => `#lit` }).collect(); + let from_arms: Vec<_> = known.iter().map(|(id, lit)| quote! { `#lit` => `#name`::`#id` }).collect(); + let strict_from_arms: Vec<_> = known + .iter() + .map(|(id, lit)| quote! { `#lit` => ::core::result::Result::Ok(`#name`::`#id`) }) + .collect();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/derive/src/lib.rs` around lines 1162 - 1199, Consolidate the repeated `(id, lit)` extraction shared by `code_arms`, `from_arms`, and `strict_from_arms` by deriving the non-fallback variant data once, including the existing `VariantWire::Int` validation, then generate each arm collection from that shared representation. Preserve the three distinct match-arm outputs and the current unreachable behavior for invalid wire values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/libsignal/src/protocol/session_cipher.rs`:
- Around line 1046-1060: Update the terminal non-Whisper DuplicatedMessage
branch in the session decryption match to call log_decryption_failure with the
ciphertext, current_state, and duplicate error before returning. Preserve the
existing session-state update and returned error behavior.
- Around line 1155-1167: Update the non-Whisper DuplicatedMessage branch in the
session decryption flow to log the decryption failure via log_decryption_failure
before restoring the session and returning the error. Keep the existing
restore_previous_session and error propagation behavior unchanged, matching the
logging behavior of the adjacent Whisper branch.
---
Outside diff comments:
In `@wacore/derive/src/lib.rs`:
- Around line 1162-1199: Consolidate the repeated `(id, lit)` extraction shared
by `code_arms`, `from_arms`, and `strict_from_arms` by deriving the non-fallback
variant data once, including the existing `VariantWire::Int` validation, then
generate each arm collection from that shared representation. Preserve the three
distinct match-arm outputs and the current unreachable behavior for invalid wire
values.
In `@wacore/libsignal/src/protocol/session_cipher.rs`:
- Around line 1361-1392: Add a regression test alongside the existing
session-cipher tests for `decrypt_with_pending_state`’s closed receiver-chain
branch, using the skipped-key setup to clear the chain key, remove the relevant
message key, and submit a counter equal to or greater than `next_index`; assert
decryption fails with the expected invalid-message-wrapped error while
preserving the existing tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 70f7685f-b376-4e13-99d5-3d60a3afec2e
📒 Files selected for processing (7)
Cargo.tomlwacore/Cargo.tomlwacore/derive/src/lib.rswacore/libsignal/Cargo.tomlwacore/libsignal/src/protocol/legacy_session.rswacore/libsignal/src/protocol/mod.rswacore/libsignal/src/protocol/session_cipher.rs
The non-Whisper duplicate arms returned without a trace while the adjacent Whisper arms log before continuing the search; a silently terminated pre-key replay left no audit trail.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
SessionRecordv1 data, behind the newlegacy-session-interopcargo feature (default off, forwarded throughwacoreand the root crate)SessionRecordand an explicit operational reverse projectionDesign
The new boundary accepts owned binary values and moves them into the canonical component model. It reuses the existing key validators,
SessionRecord::from_components,SessionMessageKeyMaterial::Seed, message-key derivation, and canonical record limits. It does not add textual serialization, transport-specific DTOs, dependencies, caches, schemas, or shadow state. The interop model compiles out of native builds; only migration consumers enable the feature.The reverse projection is intentionally explicit about information that the legacy format cannot represent exactly. Derived skipped-message keys, unsupported session state, ambiguous remote-ratchet state, and a pending pre-key base that differs from the session base key return typed deterministic errors instead of being discarded or inferred silently. Existing sender-key components remain the single model for sender-key records.
Decrypt-path behavior
These changes stay outside the feature gate because canonical records reach the same states natively (re-initiations reuse the peer's signed pre-key as a ratchet key, so sessions can legitimately share receiver ratchet keys):
BadMacnoise, so a replay is acknowledged as a duplicate instead of triggering a retry receipt for an already-processed messageCompatibility and safety
previousCounterof -1 (a ratchet step over a sending chain that never sent, the reference seed value) as zero; other out-of-range values fail typedDebugoutputThe reverse conversion is an operational projection, not a byte-exact round trip: legacy lifecycle timestamps and base-key lookup roles are reconstructed deterministically, while genuinely non-representable state is rejected.
Performance
Bytesand sorts collections in place+896bytes (about0.009%), now carried only by consumers enabling the feature.rodata,.data, or static stateValidation
cargo fmt --all -- --checkcargo clippy -p wacore-libsignal -p wacore-derive --all-targets -- -D warnings, with and withoutlegacy-session-interopcargo test -p wacore-libsignal, with and withoutlegacy-session-interopcargo test --workspace --exclude e2e-testsTests cover reference-shaped current and archived sessions, chain ordering and limits, checked counter conversion including the -1 floor, pending pre-keys, malformed and duplicate state, exact skipped-seed derivation, explicit reverse-projection failures including the pending-base mismatch, sender-key component round trips, serialization/reload of rebuilt records, real decryption using skipped keys from closed and open archived chains, and duplicate classification winning over candidate MAC failures.