From a299b6564c3b50e42f20e19b695cc989cf9ffc69 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Fri, 2 Oct 2026 12:58:36 +0530 Subject: [PATCH 1/6] fix(mls): keep a synced key package's record so expiry still owns it mls_mark_key_package_synced deleted the key package record and nothing else. The private init key stayed in the MLS provider, which is half right: a stranger who fetched the uploaded copy may still Welcome us, and only that Welcome consumes the key. The other half is the problem. The record is the *only* thing that carries expiry. Every withdrawal and every grace-window purge runs through the loader, and the loader starts from the record. Delete the record and an uploaded package nobody ever claimed keeps its init key for the life of the install. ADR 0012 already called this the leak most likely to come back. It never left. It turns out the getter made it worse. The pending list returned every live package, including the ones the push path had handed to a peer and the ones standing in the engine's own Nostr slots. An app following the documented upload loop would mark those synced too, and each time the engine minted a successor the old key was stranded. Let's do what the synced flag, which nothing has set since it was added, was always supposed to mean. Marking now sets it on the record and keeps the record, so the package expires and its key is destroyed past the grace window like any other. A synced package is withheld from both hand-out paths through the same predicate that withholds a slot package, because pointing a peer at a key a stranger may hold is the shared-key failure this whole ADR exists to prevent. The pending list shrinks to what the app may actually publish. Marking an unknown, expired or consumed id is a no-op. A source guard now refuses any new record-only delete of a key package outside the loader and the purge. The next person who writes three lines of storage.delete gets a test failure instead of a leak. Fixes #367. --- CHANGELOG.md | 18 ++ crates/offline-protocol-mls/src/manager.rs | 294 ++++++++++++++++++++- crates/offline-protocol-mls/src/types.rs | 23 +- docs/adr/0012-one-key-package-per-peer.md | 55 ++-- docs/mls-integration.md | 15 +- docs/react-native-integration.md | 4 +- docs/state-machines/session-lifecycle.md | 14 + 7 files changed, 394 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9057548dd..415650193 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -345,6 +345,24 @@ archived by series under [docs/changelog/](docs/changelog/); see the ### Fixed +- **A key package the application marks synced keeps its record, so its + private key is still destroyed when it expires.** + `mls_mark_key_package_synced` deleted the record and left the init key in + the MLS provider. Keeping the key was right, because a Welcome built from + the uploaded copy has to open, but the record is the only thing that carries + expiry, so a published package nobody claimed kept its key for the life of + the install. The mark now sets `synced` on the record instead. The package is + withdrawn at expiry and its key destroyed a week later like any other, and + it is never handed to a peer, because a stranger may already hold it. + `mls_get_pending_key_packages` now lists only packages the application may + publish: it used to include packages the engine had pushed to a peer or held + in its own Nostr slots, which the documented upload loop would then have + marked, stranding each old key as the engine minted a successor. Marking an + unknown or already used id does nothing. A source guard refuses any new + record-only delete of a key package + ([ADR 0012](docs/adr/0012-one-key-package-per-peer.md#a-package-the-application-publishes-keeps-its-record), + [#367](https://github.com/Offline-Protocol/offline-protocol-sdk/issues/367)). + - **`import offline_protocol_sdk` works on Windows.** `pyproject.toml` has never installed `bless` on Windows, where it has no backend, and the package imported it on the way in, so the import failed on every Windows diff --git a/crates/offline-protocol-mls/src/manager.rs b/crates/offline-protocol-mls/src/manager.rs index 3d05c98e8..81bd29d42 100644 --- a/crates/offline-protocol-mls/src/manager.rs +++ b/crates/offline-protocol-mls/src/manager.rs @@ -423,11 +423,12 @@ impl MlsManager { /// Gets an existing *unclaimed* key package or generates a new one. /// - /// Two kinds of package are skipped, for the same underlying reason — an + /// Three kinds of package are skipped, for the same underlying reason — an /// MLS init key is consumed by its first user, so two parties must never be /// pointed at one: /// - /// - packages reserved for publication slots, or a pushed-to peer and a + /// - packages reserved for publication slots, or synced by the application + /// ([`Self::mark_key_package_synced`]), or a pushed-to peer and a /// stranger who fetched the published record would race for it; /// - packages already claimed by a peer over the push path /// ([`Self::take_push_key_package`]), or this peer-less entry point would @@ -444,7 +445,7 @@ impl MlsManager { for package_id in packages { if let Some(bundle) = self.load_stored_key_package(&package_id)? { - if bundle.reserved_for_publication || bundle.assigned_peer.is_some() { + if bundle.withheld_from_hand_out() || bundle.assigned_peer.is_some() { continue; } return Ok(bundle); @@ -517,7 +518,9 @@ impl MlsManager { let Some(bundle) = self.load_stored_key_package(&package_id)? else { continue; }; - if bundle.reserved_for_publication { + // Before the count: a package somebody else holds is never handed + // out here, so it is no part of the pool the ceiling bounds. + if bundle.withheld_from_hand_out() { continue; } live += 1; @@ -817,7 +820,15 @@ impl MlsManager { Ok(()) } - /// Gets pending key packages. + /// Lists the live packages an application may publish itself. + /// + /// Only packages nobody has spoken for: unclaimed, unreserved and not yet + /// synced. A package the push path assigned to a peer, or one standing in + /// one of the engine's own publication slots, is the engine's to publish. + /// Listing them here invited an application following the documented + /// upload loop to mark them synced, which handed one init key to two + /// parties and, while marking deleted the record, stranded the old key + /// every time the engine minted a successor. pub fn get_pending_key_packages(&self) -> Result> { let key_type = StorageKeyType::KeyPackage.as_str(); let package_ids = self.storage.list_keys(key_type)?; @@ -825,6 +836,9 @@ impl MlsManager { let mut bundles = Vec::new(); for package_id in package_ids { if let Some(bundle) = self.load_stored_key_package(&package_id)? { + if bundle.withheld_from_hand_out() || bundle.assigned_peer.is_some() { + continue; + } bundles.push(bundle); } } @@ -832,10 +846,48 @@ impl MlsManager { Ok(bundles) } - /// Marks a key package as synced. + /// Records that the application has published a package itself. + /// + /// The record is kept, flagged, and withheld from both hand-out paths. It + /// is not deleted, and the private init key is not destroyed: + /// + /// - The key must outlive the mark, because a stranger who fetched the + /// uploaded copy may still Welcome us, and only that Welcome consumes it. + /// - The record must outlive the mark, because the record is the only + /// thing that carries expiry. Deleting it, which is what this used to + /// do, left the init key resident for the life of the install whenever + /// the uploaded copy was never claimed (issue 367). Kept, it is withdrawn + /// at expiry and its key destroyed past the grace window like any other. + /// + /// Marking an id that is unknown, expired or already consumed does + /// nothing: there is no live package left to publish. Marking a package + /// the push path already handed to a peer is allowed but logged, since + /// that peer and the uploaded copy now share an init key; the peer's held + /// copy stays openable, and a failed establishment is recovered by the + /// next push, which mints that peer a successor. pub fn mark_key_package_synced(&self, package_id: &str) -> Result<()> { - let key_type = StorageKeyType::KeyPackage.as_str(); - self.storage.delete(key_type, package_id)?; + let Some(mut bundle) = self.load_stored_key_package(package_id)? else { + debug!( + package_id = %package_id, + "No live key package to mark synced" + ); + return Ok(()); + }; + if bundle.synced { + return Ok(()); + } + if let Some(peer) = bundle.assigned_peer.as_deref() { + warn!( + package_id = %package_id, + peer_id = %peer, + "Marking a key package synced that the push path already handed to a peer" + ); + } + bundle.synced = true; + let serialized = + serde_json::to_vec(&bundle).map_err(|e| MlsError::Serialization(e.to_string()))?; + self.storage + .store(StorageKeyType::KeyPackage.as_str(), package_id, &serialized)?; debug!(package_id = %package_id, "Marked key package as synced"); Ok(()) } @@ -2552,6 +2604,232 @@ mod tests { assert!(unclaimed.assigned_peer.is_none()); } + /// Whether `package_id` still has a record in storage at all. + fn record_present(storage: &InMemoryStorage, package_id: &str) -> bool { + storage + .load(StorageKeyType::KeyPackage.as_str(), package_id) + .unwrap() + .is_some() + } + + /// Marking a package synced keeps its record and its init key. The key has + /// to outlive the mark so a Welcome built from the uploaded copy opens, and + /// the record has to, because it is the only thing that carries expiry. + #[test] + fn test_marking_synced_keeps_the_record_and_the_init_key() { + let (manager, storage) = create_test_manager_with_storage("alice"); + let bundle = manager.generate_key_package().unwrap(); + + manager.mark_key_package_synced(&bundle.package_id).unwrap(); + + assert!( + record_present(&storage, &bundle.package_id), + "marking synced deleted the record, so nothing can ever expire the key" + ); + assert!( + init_key_present(&manager, &bundle), + "marking synced destroyed the init key a Welcome from the upload needs" + ); + let reread = manager + .key_package_by_id(&bundle.package_id) + .unwrap() + .expect("a synced package is still live"); + assert!(reread.synced); + } + + /// The issue 367 leak: a published package nobody ever claimed. Past the + /// grace window its init key is destroyed like any other package's, + /// because the record that carries the expiry is still there. + #[test] + fn test_unclaimed_synced_package_has_its_init_key_destroyed_past_the_grace_window() { + let (manager, storage) = create_test_manager_with_storage("alice"); + let bundle = manager.generate_key_package().unwrap(); + manager.mark_key_package_synced(&bundle.package_id).unwrap(); + + expire_package( + &storage, + &bundle.package_id, + KEY_PACKAGE_PURGE_GRACE_SECS + 60, + ); + assert!(manager + .key_package_by_id(&bundle.package_id) + .unwrap() + .is_none()); + + assert!( + !init_key_present(&manager, &bundle), + "a synced package's init key outlived its grace window" + ); + assert!(!record_present(&storage, &bundle.package_id)); + } + + /// A synced package stands in a record a stranger may fetch, so handing it + /// to a peer too would point two parties at one init key. + #[test] + fn test_synced_package_is_withheld_from_both_hand_out_paths() { + let manager = create_test_manager("alice"); + let synced = manager.generate_key_package().unwrap(); + manager.mark_key_package_synced(&synced.package_id).unwrap(); + + let pushed = manager.take_push_key_package(&addr("bob")).unwrap(); + assert_ne!(pushed.bundle.package_id, synced.package_id); + assert!(!pushed.pool_exhausted); + + let peerless = manager.get_or_create_key_package().unwrap(); + assert_ne!(peerless.package_id, synced.package_id); + } + + /// The pending list is what an application publishes itself, so it holds + /// only packages nobody has spoken for. A slot package or a peer's package + /// listed here is one the documented upload loop would hand out twice. + #[test] + fn test_pending_list_holds_only_packages_the_application_may_publish() { + let manager = create_test_manager("alice"); + // Pushed first: the push path claims any unclaimed package, so pushing + // after `plain` is minted would make `plain` bob's. + let for_bob = manager.take_push_key_package(&addr("bob")).unwrap().bundle; + let plain = manager.generate_key_package().unwrap(); + let reserved = manager.generate_publication_key_package().unwrap(); + let synced = manager.generate_key_package().unwrap(); + manager.mark_key_package_synced(&synced.package_id).unwrap(); + + let pending: Vec = manager + .get_pending_key_packages() + .unwrap() + .into_iter() + .map(|b| b.package_id) + .collect(); + + assert!(pending.contains(&plain.package_id)); + assert!( + !pending.contains(&reserved.package_id), + "lists a slot package" + ); + assert!( + !pending.contains(&for_bob.package_id), + "lists a peer's package" + ); + assert!( + !pending.contains(&synced.package_id), + "lists a synced package" + ); + } + + /// The point of keeping the init key: a stranger who fetched the uploaded + /// copy can still establish with us. + #[test] + fn test_welcome_built_against_a_synced_package_still_opens() { + let alice = create_test_manager("alice"); + let bob = create_test_manager("bob"); + let uploaded = alice.generate_key_package().unwrap(); + alice.mark_key_package_synced(&uploaded.package_id).unwrap(); + + bob.import_key_package(&addr("alice"), &uploaded.key_package_data) + .unwrap(); + let welcome = bob.create_session(&addr("alice")).unwrap(); + + alice + .join_session(&welcome) + .expect("a Welcome against a synced package must open"); + assert!(alice.has_session(&addr("bob")).unwrap()); + } + + /// A record written before the bundle format is upgraded and marked, not + /// read as unknown and left unflagged. + #[test] + fn test_marking_a_legacy_record_upgrades_and_marks_it() { + let (manager, storage) = create_test_manager_with_storage("alice"); + let bundle = manager.generate_key_package().unwrap(); + storage + .store( + StorageKeyType::KeyPackage.as_str(), + &bundle.package_id, + &bundle.key_package_data, + ) + .unwrap(); + + manager.mark_key_package_synced(&bundle.package_id).unwrap(); + + let reread = manager + .key_package_by_id(&bundle.package_id) + .unwrap() + .expect("the legacy package is still live"); + assert!(reread.synced); + assert!(init_key_present(&manager, &bundle)); + } + + /// An unknown or already consumed id has no live package to publish, so + /// marking it is a no-op rather than an error the caller cannot act on. + #[test] + fn test_marking_an_unknown_or_consumed_package_is_a_no_op() { + let alice = create_test_manager("alice"); + alice.mark_key_package_synced("no-such-package").unwrap(); + + let bob = create_test_manager("bob"); + let advertised = alice.take_push_key_package(&addr("bob")).unwrap().bundle; + bob.import_key_package(&addr("alice"), &advertised.key_package_data) + .unwrap(); + let welcome = bob.create_session(&addr("alice")).unwrap(); + alice.join_session(&welcome).unwrap(); + + alice + .mark_key_package_synced(&advertised.package_id) + .unwrap(); + assert!(alice + .key_package_by_id(&advertised.package_id) + .unwrap() + .is_none()); + } + + /// Every record-only delete of one of this device's key packages has to be + /// one where the provider key is already gone or was never derivable. + /// Anywhere else it strands the init key with nothing left to expire it, + /// which is how `mark_key_package_synced` leaked (issue 367). + /// + /// Read per function: a body that names the key package storage type and + /// calls `delete(` on the storage is a record delete of ours, and only the + /// loader (consumed or unparseable records) and the purge (after the + /// provider key) may do that. + #[test] + fn only_the_known_sites_delete_a_key_package_record_alone() { + let source = include_str!("manager.rs"); + let production = &source[..source + .find("#[cfg(test)]\nmod tests") + .expect("manager.rs has a test module")]; + let allowed = ["load_stored_key_package", "purge_key_package_material"]; + + let mut offenders = Vec::new(); + let mut checked = 0usize; + for body in production + .split("\n fn ") + .chain(production.split("\n pub fn ")) + { + let name = body.split(['(', '<']).next().unwrap_or(""); + // Only the text up to the next function is this function's body. + let body = body.split("\n pub fn ").next().unwrap_or(body); + let body = body.split("\n fn ").next().unwrap_or(body); + if !body.contains("StorageKeyType::KeyPackage.as_str()") { + continue; + } + checked += 1; + if body.contains(".delete(") + && !body.contains("delete_key_package(") + && !allowed.contains(&name) + { + offenders.push(name.to_string()); + } + } + assert!( + checked >= 8, + "found only {checked} key package functions; the scan is broken, not the code" + ); + assert!( + offenders.is_empty(), + "record-only delete of a key package in {offenders:?}. Destroy the init \ + key too (purge_key_package_material), or keep the record" + ); + } + /// An unclaimed package — minted by the peer-less entry point, or by a /// build predating assignment — is claimed rather than left behind, so /// upgrading does not strand the package already in storage. diff --git a/crates/offline-protocol-mls/src/types.rs b/crates/offline-protocol-mls/src/types.rs index b929a483f..f7253997f 100644 --- a/crates/offline-protocol-mls/src/types.rs +++ b/crates/offline-protocol-mls/src/types.rs @@ -68,7 +68,18 @@ pub struct KeyPackageBundle { /// Timestamp when the key package expires (milliseconds since epoch). pub expires_at_ms: u64, - /// Whether this key package has been uploaded to a server. + /// Whether the application has published this package itself, through + /// [`MlsManager::mark_key_package_synced`](crate::MlsManager::mark_key_package_synced). + /// + /// A synced package stands in a record somebody else holds, so a stranger + /// may still build a Welcome against it. Two things follow, and both are + /// the single-use rule [`Self::reserved_for_publication`] already applies: + /// the package is withheld from both entry points that hand packages out, + /// or a pushed-to peer and whoever fetched the uploaded copy would race + /// for one init key; and the record is *kept*, because the record is the + /// only thing that carries expiry. Deleting it instead, which is what + /// marking used to do, left the private init key in the provider with no + /// path to destruction at all (issue 367). pub synced: bool, /// Whether this package is reserved for a publication slot. @@ -147,6 +158,16 @@ impl KeyPackageBundle { } } + /// Whether this package stands in a record somebody else holds, and so + /// must never be handed out by this device. + /// + /// One predicate rather than two checks at each hand-out site, so the push + /// path and the peer-less getter cannot come to disagree about which + /// packages are spoken for. + pub fn withheld_from_hand_out(&self) -> bool { + self.reserved_for_publication || self.synced + } + /// Checks if the key package has expired (local device's own packages only). /// /// This compares against the local clock and is valid because `created_at_ms` diff --git a/docs/adr/0012-one-key-package-per-peer.md b/docs/adr/0012-one-key-package-per-peer.md index 8b6296dd7..9f39e362c 100644 --- a/docs/adr/0012-one-key-package-per-peer.md +++ b/docs/adr/0012-one-key-package-per-peer.md @@ -83,21 +83,43 @@ Deletion also purges legacy records predating the bundle format: an unparseable record is read as the serialized key package so its provider reference is derivable. A record-only delete there is the exact stranding this rule removes. -### The one record-only delete that survives - -`mark_key_package_synced` is an exception, and an unresolved one. It deletes the -record and leaves the provider key in place. No engine path calls it; it exists -only on the FFI surface, for an application that publishes a package itself and -wants it out of the pending list. - -Retaining the provider key there is not obviously wrong. A published package -must stay openable, and only the peer's Welcome consumes the key. But the record -is what carries expiry, so a published-but-never-claimed package's key has no -path to destruction at all, which is the stranding this section otherwise -forbids. - -Treat it as a known gap rather than a pattern to copy: a caller that wants the -package withdrawn should expire it, not mark it synced. +### A package the application publishes keeps its record + +**Invariant: no key package record of ours is deleted while its provider key +is still resident, unless the provider key is destroyed in the same step.** The +only record-only deletes are the loader's, for a package whose provider key is +already gone or whose bytes name no key at all. +`only_the_known_sites_delete_a_key_package_record_alone` reads the source and +refuses a third. + +`mark_key_package_synced` is the case this used to miss. It exists only on the +FFI surface, for an application that uploads a package to its own key server, +and it deleted the record and left the provider key in place. Keeping the key +was right: a stranger who fetched the uploaded copy may still Welcome us, and +only that Welcome consumes it. Deleting the record was not, because the record +is what carries expiry, so a published package nobody claimed kept its init +key for the life of the install (issue 367). + +Marking now keeps the record and sets `synced` on it: + +1. **The record stays,** so the package is withdrawn at expiry and its key + destroyed past the grace window like every other package. +2. **The package is withheld from both hand-out paths,** through the same + predicate that withholds a slot package. A synced package is one somebody + else holds, and pointing a peer at it too is the shared-key failure this + ADR exists to prevent. +3. **The pending list holds only what the application may publish:** unclaimed, + unreserved and unsynced. It used to list every live package, so an + application following the documented upload loop marked the engine's own + slot and push packages synced. With the old delete, each one was stranded as + soon as the engine minted its successor. + +Marking an unknown, expired or consumed id does nothing. Marking a package the +push path already handed to a peer is allowed and logged: the two holders now +share an init key, the peer's copy stays openable, and if the uploaded copy is +spent first, the next push to that peer mints a successor. A package can still +be claimed by a push between the application's list call and its mark; the +window is one discovery event wide and the outcome is the same as this case. ## What would undo this @@ -106,3 +128,6 @@ skip assigned packages. The peerless escape hatch exists for FFI and tests, and it must skip both reserved and peer-assigned packages. Deleting a key package record without purging its provider key. + +A hand-out path that stops skipping synced packages, or a pending list that +shows a package the engine owns. diff --git a/docs/mls-integration.md b/docs/mls-integration.md index ae8c5241f..226879b32 100644 --- a/docs/mls-integration.md +++ b/docs/mls-integration.md @@ -904,8 +904,17 @@ DELETE /keys/{userId}/{pkgId} # Delete used key package ### Syncing Key Packages +The pending list holds only packages this app may publish. Packages the SDK +has already pushed to a peer, or published in its own slots, never appear in +it. Marking a package synced keeps it on the device, so a Welcome built from +the uploaded copy still opens, and stops the SDK handing it to anybody else. +It expires with the lifetime it was minted with (30 days), and its private key +is destroyed a week after that, used or not. Your server should drop its copy +by `expiresAtMs`: past it the package's own validity window has closed, and a +week later this device can no longer open a Welcome built from it. + ```swift -// Get pending key packages to upload +// Get the key packages this app may publish let pending = mesh.mlsGetPendingKeyPackages() for pkg in pending { @@ -1405,8 +1414,8 @@ Disabling the switch reverts to the legacy drop-and-ACK behaviour. | `mlsGenerateKeyPackage()` | Generate a new key package | | `mlsGetOrCreateKeyPackage()` | Get existing or generate new package | | `mlsImportKeyPackage(userId, data)` | Import a contact's key package | -| `mlsGetPendingKeyPackages()` | Get packages to upload | -| `mlsMarkKeyPackageSynced(packageId)` | Mark package as uploaded | +| `mlsGetPendingKeyPackages()` | Get packages this app may upload: unclaimed, not in the SDK's own slots, not yet synced | +| `mlsMarkKeyPackageSynced(packageId)` | Mark a package uploaded: kept until it expires, never handed to a peer | ### 1:1 Sessions diff --git a/docs/react-native-integration.md b/docs/react-native-integration.md index f02273eaa..1b5bf4c12 100644 --- a/docs/react-native-integration.md +++ b/docs/react-native-integration.md @@ -654,8 +654,8 @@ bytes. Worked code is in the | **isMlsInitialized** | `isMlsInitialized(): Promise` | Whether MLS is ready. | | **mlsGenerateKeyPackage** | `mlsGenerateKeyPackage(): Promise` | Generates a new key package. | | **mlsGetOrCreateKeyPackage** | `mlsGetOrCreateKeyPackage(): Promise` | Gets or creates key package. | -| **mlsGetPendingKeyPackages** | `mlsGetPendingKeyPackages(): Promise` | Pending key packages not yet synced. | -| **mlsMarkKeyPackageSynced** | `mlsMarkKeyPackageSynced(packageId): Promise` | Marks key package as synced. | +| **mlsGetPendingKeyPackages** | `mlsGetPendingKeyPackages(): Promise` | Key packages this app may publish itself: never one the SDK pushed to a peer or holds in its own slots, never one already synced. | +| **mlsMarkKeyPackageSynced** | `mlsMarkKeyPackageSynced(packageId): Promise` | Records that the app published a package. It stays on the device until it expires so a Welcome from the uploaded copy opens, and is never handed to a peer. | | **mlsImportKeyPackage** | `mlsImportKeyPackage(userId, keyPackageData: number[]): Promise` | Imports another user's key package. | | **mlsHasSession** | `mlsHasSession(otherUserId): Promise` | Whether an MLS session exists with that user. | | **hasPendingKeyPackage** | `hasPendingKeyPackage(peerId): Promise` | Whether a pending key package is available for the peer. | diff --git a/docs/state-machines/session-lifecycle.md b/docs/state-machines/session-lifecycle.md index 29f14f052..7847c659f 100644 --- a/docs/state-machines/session-lifecycle.md +++ b/docs/state-machines/session-lifecycle.md @@ -184,6 +184,20 @@ unparseable record is read as the serialized key package so its provider reference is derivable. A record-only delete there is the exact stranding this rule removes. +### A package the application publishes stays under expiry + +An application that uploads a package to its own key server marks it with +`mls_mark_key_package_synced`. The mark keeps the record and flags it, so the +two stages above still own it: withdrawn at expiry, destroyed past the grace +window, whether or not anybody used the uploaded copy. A synced package is +withheld from the push path and from the peer-less getter, as a slot package +is, because a stranger may already hold it. Deleting the record on the mark, +which is what it once did, stranded the init key with nothing left to expire it +([ADR 0012](../adr/0012-one-key-package-per-peer.md#a-package-the-application-publishes-keeps-its-record)). + +The pending list an application uploads from holds only packages nobody has +spoken for: never a peer's, never a slot's, never one already synced. + ## Desync and heal An **established** session whose two sides disagree on the MLS epoch yields an From d0be47e371ad021233301eed3e5dbce751b27f6a Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Fri, 2 Oct 2026 12:58:36 +0530 Subject: [PATCH 2/6] fix(bindings): read the key package fields the native bridges send Both native bridges have always sent a key package as createdAtMs, expiresAtMs and synced. The TypeScript wrappers read createdAt and isSynced. So every JavaScript caller got undefined for both, on all three key package methods, since the day they were written. Nothing noticed. The native record is typed any, so tsc is happy, and the Rust text guards can see that a key is present in a bridge but not that anybody reads it. Route all three wrappers through one mapper that reads the names the bridges actually send, keeping the plain names first the way the session and group mappers already do. While at it, expose expiresAt: it is the moment a key server should drop its copy, and the previous commit makes that moment matter. A harness test runs the compiled wrappers against a stubbed native module, and fails all three cases against the old mapping. --- CHANGELOG.md | 7 + bindings/react-native/js-ci-harness/README.md | 1 + .../js-ci-harness/key-package-mapping.test.js | 158 ++++++++++++++++++ bindings/react-native/package.json | 2 +- bindings/react-native/src/index.ts | 60 ++++--- bindings/react-native/src/types.ts | 13 +- 6 files changed, 214 insertions(+), 27 deletions(-) create mode 100644 bindings/react-native/js-ci-harness/key-package-mapping.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 415650193..f93a66e83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -363,6 +363,13 @@ archived by series under [docs/changelog/](docs/changelog/); see the ([ADR 0012](docs/adr/0012-one-key-package-per-peer.md#a-package-the-application-publishes-keeps-its-record), [#367](https://github.com/Offline-Protocol/offline-protocol-sdk/issues/367)). +- **React Native key packages carry their timestamps and synced flag.** The + wrappers read `createdAt` and `isSynced` from a native record that has only + ever sent `createdAtMs`, `expiresAtMs` and `synced`, so both were always + `undefined` in JavaScript. All three key package methods now go through one + mapper, and `MlsKeyPackage` gains `expiresAt`, the moment a key server + should drop its copy. + - **`import offline_protocol_sdk` works on Windows.** `pyproject.toml` has never installed `bless` on Windows, where it has no backend, and the package imported it on the way in, so the import failed on every Windows diff --git a/bindings/react-native/js-ci-harness/README.md b/bindings/react-native/js-ci-harness/README.md index 6c5b9763f..568f5a105 100644 --- a/bindings/react-native/js-ci-harness/README.md +++ b/bindings/react-native/js-ci-harness/README.md @@ -72,3 +72,4 @@ Two traps, both of which produce a test that passes while proving nothing: | `forward-priority.test.js` | The priority argument `forwardMessage` hands to native (`src/index.ts`): always a number and never `null`, because React Native refuses a null number argument before the Swift method runs and the promise then never settles (#417), and `MessagePriority.Low` is 0 so the default must be resolved with `??` rather than `||`. | | `rich-send-app-id.test.js` | The per-send `appId` on `sendMessage` and `sendMedia` (`src/index.ts`): an appId-only call must take the rich native method, because only it carries `app_id`, and the plain path would send under the configured id without a word; a rich call without one sends `null`. | | `relay-config.test.js` | The relay and DORS config payloads this layer hands to native: the whole `relay` section crossing at create time (not just `relayPriority`), the legacy `low`/`medium`/`high` spelling mapping to the engine vocabulary, and a runtime update naming only the fields it was given — which is what makes the native-side merge a partial update rather than a full overwrite. Its other half is the Rust guard `react_native_bridges_merge_dors_updates_from_the_live_config`. | +| `key-package-mapping.test.js` | The JS shape of a key package record (`src/index.ts`, `toMlsKeyPackage`): `createdAt`, `expiresAt` and `isSynced` read from the `createdAtMs`, `expiresAtMs` and `synced` both native bridges send. The wrappers read other names, so every caller got `undefined` for both, and neither the typecheck nor a text guard can see a key that is sent but never read. | diff --git a/bindings/react-native/js-ci-harness/key-package-mapping.test.js b/bindings/react-native/js-ci-harness/key-package-mapping.test.js new file mode 100644 index 000000000..e5a8c7159 --- /dev/null +++ b/bindings/react-native/js-ci-harness/key-package-mapping.test.js @@ -0,0 +1,158 @@ +#!/usr/bin/env node +/** + * Behavioral tests for the JS shape of a key package record (`src/index.ts`, + * `toMlsKeyPackage`). + * + * Both native bridges send `createdAtMs`, `expiresAtMs` and `synced`. The + * wrappers used to read `createdAt` and `isSynced`, so every caller received + * `undefined` for both. Nothing failed: the typecheck sees an `any`, and the + * Rust text guards cannot tell a key that is present from one that is read. + * + * See README.md for why the package has no other JS test setup. + */ +'use strict'; + +const assert = require('node:assert/strict'); +const { execFileSync } = require('node:child_process'); +const fs = require('node:fs'); +const Module = require('node:module'); +const os = require('node:os'); +const path = require('node:path'); + +const PACKAGE_DIR = path.resolve(__dirname, '..'); + +/** Compiles `src/` to a scratch dir. See one-shot-hold.test.js for why. */ +function compileSdk() { + const tsc = path.join(PACKAGE_DIR, 'node_modules', 'typescript', 'bin', 'tsc'); + if (!fs.existsSync(tsc)) { + throw new Error(`TypeScript not found at ${tsc} — run \`npm ci\` in ${PACKAGE_DIR} first.`); + } + const outDir = fs.mkdtempSync(path.join(os.tmpdir(), 'op-rn-kp-')); + execFileSync( + process.execPath, + [tsc, '--outDir', outDir, '--declaration', 'false', '--declarationMap', 'false'], + { cwd: PACKAGE_DIR, stdio: 'inherit' } + ); + return outDir; +} + +let nativeOverrides = {}; + +const nativeModule = new Proxy( + {}, + { + get(_target, method) { + if (typeof method !== 'string') return undefined; + return (...args) => { + const override = nativeOverrides[method]; + return override ? override(...args) : Promise.resolve(); + }; + }, + } +); + +class StubNativeEventEmitter { + addListener() { + return { remove: () => {} }; + } +} + +const realLoad = Module._load; +Module._load = function loadWithReactNativeStub(request) { + if (request === 'react-native') { + return { + NativeModules: { OfflineProtocolModule: nativeModule }, + NativeEventEmitter: StubNativeEventEmitter, + }; + } + return realLoad.apply(this, arguments); +}; + +const realConsole = { log: console.log, warn: console.warn, error: console.error }; + +function captureConsole() { + console.log = () => {}; + console.warn = () => {}; + console.error = () => {}; +} + +function releaseConsole() { + Object.assign(console, realConsole); +} + +const tests = []; +const test = (name, fn) => tests.push({ name, fn }); + +/** A record exactly as both native bridges build it. */ +function nativeRecord(overrides = {}) { + return { + packageId: 'pkg-1', + userId: 'off1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqa', + keyPackageData: [1, 2, 3], + createdAtMs: 1_700_000_000_000, + expiresAtMs: 1_702_592_000_000, + synced: true, + ...overrides, + }; +} + +let OfflineProtocol; + +function newSdk() { + return new OfflineProtocol({ appId: 'harness', profile: 'harness-profile' }); +} + +for (const method of ['mlsGenerateKeyPackage', 'mlsGetOrCreateKeyPackage']) { + test(`${method} reads the timestamps and the synced flag the bridges send`, async () => { + nativeOverrides[method] = () => Promise.resolve(nativeRecord()); + const pkg = await newSdk()[method](); + + assert.equal(pkg.createdAt, 1_700_000_000_000); + assert.equal(pkg.expiresAt, 1_702_592_000_000); + assert.equal(pkg.isSynced, true); + assert.equal(pkg.packageId, 'pkg-1'); + assert.deepEqual(pkg.keyPackageData, [1, 2, 3]); + }); +} + +test('mlsGetPendingKeyPackages maps every record the same way', async () => { + nativeOverrides.mlsGetPendingKeyPackages = () => + Promise.resolve([nativeRecord(), nativeRecord({ packageId: 'pkg-2', synced: false })]); + const pending = await newSdk().mlsGetPendingKeyPackages(); + + assert.equal(pending.length, 2); + assert.equal(pending[0].createdAt, 1_700_000_000_000); + assert.equal(pending[0].expiresAt, 1_702_592_000_000); + assert.equal(pending[1].packageId, 'pkg-2'); + assert.equal(pending[1].isSynced, false, 'false must stay false, not become undefined'); +}); + +(async () => { + const outDir = compileSdk(); + try { + ({ OfflineProtocol } = require(path.join(outDir, 'index.js'))); + + let failed = 0; + for (const { name, fn } of tests) { + nativeOverrides = { isMlsInitialized: () => Promise.resolve(true) }; + captureConsole(); + try { + await fn(); + releaseConsole(); + realConsole.log(` ✓ ${name}`); + } catch (error) { + failed += 1; + releaseConsole(); + realConsole.log(` ✗ ${name}\n ${error.message}`); + } + } + + realConsole.log( + failed === 0 ? `\n${tests.length} passed.` : `\n${failed} of ${tests.length} FAILED.` + ); + process.exitCode = failed === 0 ? 0 : 1; + } finally { + releaseConsole(); + fs.rmSync(outDir, { recursive: true, force: true }); + } +})(); diff --git a/bindings/react-native/package.json b/bindings/react-native/package.json index 2b9599263..55a5788e2 100644 --- a/bindings/react-native/package.json +++ b/bindings/react-native/package.json @@ -29,7 +29,7 @@ "scripts": { "prepare": "tsc", "build": "tsc", - "test:js": "node js-ci-harness/one-shot-hold.test.js && node js-ci-harness/local-address.test.js && node js-ci-harness/forward-priority.test.js && node js-ci-harness/rich-send-app-id.test.js && node js-ci-harness/relay-config.test.js && node js-ci-harness/data-config.test.js && node js-ci-harness/security-config.test.js && node js-ci-harness/telemetry-config.test.js && node js-ci-harness/custody-config.test.js", + "test:js": "node js-ci-harness/one-shot-hold.test.js && node js-ci-harness/local-address.test.js && node js-ci-harness/forward-priority.test.js && node js-ci-harness/rich-send-app-id.test.js && node js-ci-harness/relay-config.test.js && node js-ci-harness/data-config.test.js && node js-ci-harness/security-config.test.js && node js-ci-harness/telemetry-config.test.js && node js-ci-harness/custody-config.test.js && node js-ci-harness/key-package-mapping.test.js", "build:ios": "bash scripts/build-ios.sh", "build:android": "bash scripts/build-android.sh", "build:all": "bash scripts/build-all.sh", diff --git a/bindings/react-native/src/index.ts b/bindings/react-native/src/index.ts index 39a45ddca..01805d057 100644 --- a/bindings/react-native/src/index.ts +++ b/bindings/react-native/src/index.ts @@ -240,6 +240,25 @@ function sanitize(value: T | undefined | null): T | undefined return Object.fromEntries(cleanedEntries) as T; } +/** + * Maps a native key package record to the JS shape. + * + * Both native bridges send `createdAtMs`, `expiresAtMs` and `synced`. Reading + * `createdAt` and `isSynced` instead, which these wrappers did, gave every + * caller `undefined` for both, with no error anywhere. The plain names are + * still read first, the same tolerance the session and group mappers use. + */ +function toMlsKeyPackage(raw: any): MlsKeyPackage { + return { + packageId: raw.packageId, + userId: raw.userId, + keyPackageData: raw.keyPackageData, + createdAt: raw.createdAt ?? raw.createdAtMs ?? 0, + expiresAt: raw.expiresAt ?? raw.expiresAtMs ?? 0, + isSynced: raw.isSynced ?? raw.synced ?? false, + }; +} + /** * Main Offline Protocol class * @@ -2846,13 +2865,7 @@ export class OfflineProtocol { */ async mlsGenerateKeyPackage(): Promise { const result = await OfflineProtocolNativeModule.mlsGenerateKeyPackage(); - return { - packageId: result.packageId, - userId: result.userId, - keyPackageData: result.keyPackageData, - createdAt: result.createdAt, - isSynced: result.isSynced, - }; + return toMlsKeyPackage(result); } /** @@ -2863,34 +2876,33 @@ export class OfflineProtocol { */ async mlsGetOrCreateKeyPackage(): Promise { const result = await OfflineProtocolNativeModule.mlsGetOrCreateKeyPackage(); - return { - packageId: result.packageId, - userId: result.userId, - keyPackageData: result.keyPackageData, - createdAt: result.createdAt, - isSynced: result.isSynced, - }; + return toMlsKeyPackage(result); } /** - * Gets pending key packages that haven't been synced yet. + * Lists the key packages this app may publish itself, for example to its + * own key server. + * + * Only packages nobody has spoken for are listed: never one the SDK has + * already handed to a peer or one standing in its own publication slots, + * and never one already marked synced. * - * @returns Array of pending key packages + * @returns Array of key packages free to publish */ async mlsGetPendingKeyPackages(): Promise { const results = await OfflineProtocolNativeModule.mlsGetPendingKeyPackages(); - return results.map((r: any) => ({ - packageId: r.packageId, - userId: r.userId, - keyPackageData: r.keyPackageData, - createdAt: r.createdAt, - isSynced: r.isSynced, - })); + return results.map(toMlsKeyPackage); } /** - * Marks a key package as synced. + * Records that the app has published a key package itself. + * + * The package stays on the device so a Welcome built from the published + * copy still opens. It is no longer listed as pending and is never handed + * to a peer. It expires with the lifetime it was minted with, and its + * private key is destroyed after that, whether or not anybody used it. + * Marking an unknown, expired or already used package does nothing. * * @param packageId - Key package ID to mark * @throws Error if operation fails diff --git a/bindings/react-native/src/types.ts b/bindings/react-native/src/types.ts index 37a82ec1d..45fab5c6b 100644 --- a/bindings/react-native/src/types.ts +++ b/bindings/react-native/src/types.ts @@ -3092,9 +3092,18 @@ export interface MlsKeyPackage { userId: string; /** Raw key package data (bytes) */ keyPackageData: number[]; - /** Timestamp when this package was created */ + /** When this package was created, in milliseconds since the epoch */ createdAt: number; - /** Whether this package has been synced to a server */ + /** + * When this package expires, in milliseconds since the epoch. A key server + * holding a copy should drop it by then: the device withdraws it at this + * moment and destroys its private key a week later. + */ + expiresAt: number; + /** + * Whether the app has marked this package published with + * `mlsMarkKeyPackageSynced`. A synced package is never handed to a peer. + */ isSynced: boolean; } From 427abf8bbc07a2f9194913231fff2a27d3a8fc50 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Fri, 2 Oct 2026 13:20:05 +0530 Subject: [PATCH 3/6] test(mls): read the record delete guard line by line, not by \n The guard that refuses a record-only key package delete went red on the Windows runner and nowhere else. It turns out *.rs is not pinned to LF in .gitattributes, so a Windows checkout hands include_str! CRLF text, and a search for "#[cfg(test)]\nmod tests" finds nothing. The guard panicked before it checked a single function. The tokenizer had a second hole too. It split on " fn " and " pub fn " only, so a pub(crate) or async function was folded into whatever came before it. Put one right after the purge and its raw delete is attributed to an allowlisted name. Silently. Let's just walk lines. str::lines() drops the \r for free, and a header is recognised whatever its visibility or qualifiers. The scanner gets a test of its own on CRLF text with offenders hidden behind each header shape, so the next tokenizer bug fails here instead of waving a leak through. While at it, drop the exemption for any body that mentions delete_key_package(. Nothing needs it, and it exempted by name rather than by what the body actually does. --- crates/offline-protocol-mls/src/manager.rs | 139 +++++++++++++++++---- 1 file changed, 114 insertions(+), 25 deletions(-) diff --git a/crates/offline-protocol-mls/src/manager.rs b/crates/offline-protocol-mls/src/manager.rs index 81bd29d42..337973f1d 100644 --- a/crates/offline-protocol-mls/src/manager.rs +++ b/crates/offline-protocol-mls/src/manager.rs @@ -2790,39 +2790,24 @@ mod tests { /// calls `delete(` on the storage is a record delete of ours, and only the /// loader (consumed or unparseable records) and the purge (after the /// provider key) may do that. + /// + /// Read line by line, never by matching `\n`: `*.rs` is not pinned to LF, + /// so a Windows checkout hands `include_str!` CRLF text, and a `\n` pattern + /// finds nothing there (the first version of this guard failed only on the + /// Windows runner for that reason). #[test] fn only_the_known_sites_delete_a_key_package_record_alone() { - let source = include_str!("manager.rs"); - let production = &source[..source - .find("#[cfg(test)]\nmod tests") - .expect("manager.rs has a test module")]; let allowed = ["load_stored_key_package", "purge_key_package_material"]; + let (checked, offenders) = record_only_key_package_deletes(include_str!("manager.rs")); - let mut offenders = Vec::new(); - let mut checked = 0usize; - for body in production - .split("\n fn ") - .chain(production.split("\n pub fn ")) - { - let name = body.split(['(', '<']).next().unwrap_or(""); - // Only the text up to the next function is this function's body. - let body = body.split("\n pub fn ").next().unwrap_or(body); - let body = body.split("\n fn ").next().unwrap_or(body); - if !body.contains("StorageKeyType::KeyPackage.as_str()") { - continue; - } - checked += 1; - if body.contains(".delete(") - && !body.contains("delete_key_package(") - && !allowed.contains(&name) - { - offenders.push(name.to_string()); - } - } assert!( checked >= 8, "found only {checked} key package functions; the scan is broken, not the code" ); + let offenders: Vec<_> = offenders + .into_iter() + .filter(|name| !allowed.contains(&name.as_str())) + .collect(); assert!( offenders.is_empty(), "record-only delete of a key package in {offenders:?}. Destroy the init \ @@ -2830,6 +2815,110 @@ mod tests { ); } + /// The scanner behind the guard above, run on text it must flag: a CRLF + /// source, and offenders declared `pub(crate)`, `async` and at column 0, + /// each placed right after an allowed function so a header the tokenizer + /// missed would fold the offender into the allowed body. + #[test] + fn the_record_delete_scanner_sees_every_function_header_and_crlf() { + let source = [ + "impl MlsManager {", + " fn purge_key_package_material(&self) {", + " let t = StorageKeyType::KeyPackage.as_str();", + " self.storage.delete(t, id)?;", + " }", + " pub(crate) fn hidden_crate(&self) {", + " self.storage.delete(StorageKeyType::KeyPackage.as_str(), id)?;", + " }", + " fn load_stored_key_package(&self) {}", + " pub async fn hidden_async(&self) {", + " self.storage.delete(StorageKeyType::KeyPackage.as_str(), id)?;", + " }", + "}", + "fn hidden_free() {", + " storage.delete(StorageKeyType::KeyPackage.as_str(), id)?;", + "}", + "#[cfg(test)]", + "mod tests {", + " fn in_tests() {", + " storage.delete(StorageKeyType::KeyPackage.as_str(), id)?;", + " }", + "}", + ] + .join("\r\n"); + + let (checked, offenders) = record_only_key_package_deletes(&source); + + assert_eq!(checked, 4, "the test module is out of scope"); + assert_eq!( + offenders, + [ + "purge_key_package_material", + "hidden_crate", + "hidden_async", + "hidden_free" + ] + ); + } + + /// Scans the production half of `source` (everything before the + /// `#[cfg(test)]` test module) one function at a time. Returns how many + /// functions name the key package storage type, and the names of those + /// that also call `.delete(`. + fn record_only_key_package_deletes(source: &str) -> (usize, Vec) { + let lines: Vec<&str> = source.lines().collect(); + let test_module = lines + .windows(2) + .position(|pair| pair[0].trim() == "#[cfg(test)]" && pair[1].starts_with("mod tests")) + .expect("the source has a test module"); + + let mut functions: Vec<(String, String)> = Vec::new(); + for line in &lines[..test_module] { + if let Some(name) = function_header_name(line) { + functions.push((name.to_string(), String::new())); + } else if let Some((_, body)) = functions.last_mut() { + body.push_str(line); + body.push('\n'); + } + } + + let mut checked = 0; + let mut offenders = Vec::new(); + for (name, body) in functions { + if !body.contains("StorageKeyType::KeyPackage.as_str()") { + continue; + } + checked += 1; + if body.contains(".delete(") { + offenders.push(name); + } + } + (checked, offenders) + } + + /// The name a function header at column 0 or 4 declares, whatever its + /// visibility (`pub`, `pub(crate)`, `pub(super)`) and qualifiers. + fn function_header_name(line: &str) -> Option<&str> { + let trimmed = line.trim_start(); + if !matches!(line.len() - trimmed.len(), 0 | 4) { + return None; + } + let mut rest = trimmed; + loop { + if let Some(after) = rest.strip_prefix("pub(") { + rest = after.split_once(')')?.1.trim_start(); + } else if let Some(after) = ["pub ", "async ", "const ", "unsafe "] + .iter() + .find_map(|qualifier| rest.strip_prefix(qualifier)) + { + rest = after.trim_start(); + } else { + break; + } + } + rest.strip_prefix("fn ")?.split(['(', '<']).next() + } + /// An unclaimed package — minted by the peer-less entry point, or by a /// build predating assignment — is claimed rather than left behind, so /// upgrading does not strand the package already in storage. From 50e10b07eb8791c1c6d0db9a7640959fef2d5d84 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Fri, 2 Oct 2026 13:20:31 +0530 Subject: [PATCH 4/6] fix(mls): mark a key package synced under the write lock Marking is a load, a flag and a store of the package record. So is the push path's claim. The FFI took the manager's *read* lock for the mark, and the engine takes the read lock for the claim, so the two can interleave freely. If the claim's store lands second, the mark is gone. The package is on the application's key server and advertised to a peer, which is exactly the shared-key case the previous commit promised to log. It is not logged: the warning only fires when the mark *sees* the assignment, and here it never does. The package then falls out of the pending list because it is assigned, so the app has no way to notice its mark did not stick. Make the mark take &mut self. Every caller sharing the manager behind an RwLock now has to hold the write half, and the compiler says so, rather than one wrapper having to remember. The FFI drops the engine's inner lock before it touches the manager lock, and the push path never takes the inner lock while holding the manager one, so there is no ordering to invert. --- CHANGELOG.md | 3 ++- crates/offline-protocol-mls/src/manager.rs | 23 ++++++++++++++-------- crates/offline-protocol-uniffi/src/lib.rs | 4 ++-- docs/adr/0012-one-key-package-per-peer.md | 5 ++++- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f93a66e83..d1ad31d07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -358,7 +358,8 @@ archived by series under [docs/changelog/](docs/changelog/); see the publish: it used to include packages the engine had pushed to a peer or held in its own Nostr slots, which the documented upload loop would then have marked, stranding each old key as the engine minted a successor. Marking an - unknown or already used id does nothing. A source guard refuses any new + unknown or already used id does nothing, and marking takes the MLS manager's + write lock, so a concurrent push can no longer erase it. A source guard refuses any new record-only delete of a key package ([ADR 0012](docs/adr/0012-one-key-package-per-peer.md#a-package-the-application-publishes-keeps-its-record), [#367](https://github.com/Offline-Protocol/offline-protocol-sdk/issues/367)). diff --git a/crates/offline-protocol-mls/src/manager.rs b/crates/offline-protocol-mls/src/manager.rs index 337973f1d..d3fd190ee 100644 --- a/crates/offline-protocol-mls/src/manager.rs +++ b/crates/offline-protocol-mls/src/manager.rs @@ -865,7 +865,14 @@ impl MlsManager { /// that peer and the uploaded copy now share an init key; the peer's held /// copy stays openable, and a failed establishment is recovered by the /// next push, which mints that peer a successor. - pub fn mark_key_package_synced(&self, package_id: &str) -> Result<()> { + /// + /// Takes `&mut self` so a caller sharing the manager behind an `RwLock` + /// must hold the write half. The push path claims a package under the + /// read half, and both are a load, a change and a store of the same + /// record: under two read guards the claim's store can land second and + /// erase the mark, leaving the package uploaded *and* advertised to a + /// peer with nothing logged. + pub fn mark_key_package_synced(&mut self, package_id: &str) -> Result<()> { let Some(mut bundle) = self.load_stored_key_package(package_id)? else { debug!( package_id = %package_id, @@ -2617,7 +2624,7 @@ mod tests { /// the record has to, because it is the only thing that carries expiry. #[test] fn test_marking_synced_keeps_the_record_and_the_init_key() { - let (manager, storage) = create_test_manager_with_storage("alice"); + let (mut manager, storage) = create_test_manager_with_storage("alice"); let bundle = manager.generate_key_package().unwrap(); manager.mark_key_package_synced(&bundle.package_id).unwrap(); @@ -2642,7 +2649,7 @@ mod tests { /// because the record that carries the expiry is still there. #[test] fn test_unclaimed_synced_package_has_its_init_key_destroyed_past_the_grace_window() { - let (manager, storage) = create_test_manager_with_storage("alice"); + let (mut manager, storage) = create_test_manager_with_storage("alice"); let bundle = manager.generate_key_package().unwrap(); manager.mark_key_package_synced(&bundle.package_id).unwrap(); @@ -2667,7 +2674,7 @@ mod tests { /// to a peer too would point two parties at one init key. #[test] fn test_synced_package_is_withheld_from_both_hand_out_paths() { - let manager = create_test_manager("alice"); + let mut manager = create_test_manager("alice"); let synced = manager.generate_key_package().unwrap(); manager.mark_key_package_synced(&synced.package_id).unwrap(); @@ -2684,7 +2691,7 @@ mod tests { /// listed here is one the documented upload loop would hand out twice. #[test] fn test_pending_list_holds_only_packages_the_application_may_publish() { - let manager = create_test_manager("alice"); + let mut manager = create_test_manager("alice"); // Pushed first: the push path claims any unclaimed package, so pushing // after `plain` is minted would make `plain` bob's. let for_bob = manager.take_push_key_package(&addr("bob")).unwrap().bundle; @@ -2719,7 +2726,7 @@ mod tests { /// copy can still establish with us. #[test] fn test_welcome_built_against_a_synced_package_still_opens() { - let alice = create_test_manager("alice"); + let mut alice = create_test_manager("alice"); let bob = create_test_manager("bob"); let uploaded = alice.generate_key_package().unwrap(); alice.mark_key_package_synced(&uploaded.package_id).unwrap(); @@ -2738,7 +2745,7 @@ mod tests { /// read as unknown and left unflagged. #[test] fn test_marking_a_legacy_record_upgrades_and_marks_it() { - let (manager, storage) = create_test_manager_with_storage("alice"); + let (mut manager, storage) = create_test_manager_with_storage("alice"); let bundle = manager.generate_key_package().unwrap(); storage .store( @@ -2762,7 +2769,7 @@ mod tests { /// marking it is a no-op rather than an error the caller cannot act on. #[test] fn test_marking_an_unknown_or_consumed_package_is_a_no_op() { - let alice = create_test_manager("alice"); + let mut alice = create_test_manager("alice"); alice.mark_key_package_synced("no-such-package").unwrap(); let bob = create_test_manager("bob"); diff --git a/crates/offline-protocol-uniffi/src/lib.rs b/crates/offline-protocol-uniffi/src/lib.rs index 977c1f464..897ab5faf 100644 --- a/crates/offline-protocol-uniffi/src/lib.rs +++ b/crates/offline-protocol-uniffi/src/lib.rs @@ -6721,8 +6721,8 @@ impl OfflineProtocol { /// Mark a key package as synced pub fn mls_mark_key_package_synced(&self, package_id: String) -> Result<(), ProtocolError> { let manager = self.get_mls_manager()?; - let guard = manager - .read() + let mut guard = manager + .write() .map_err(|e| ProtocolError::LockPoisoned(format!("mls_manager: {}", e)))?; guard .mark_key_package_synced(&package_id) diff --git a/docs/adr/0012-one-key-package-per-peer.md b/docs/adr/0012-one-key-package-per-peer.md index 9f39e362c..454c35b68 100644 --- a/docs/adr/0012-one-key-package-per-peer.md +++ b/docs/adr/0012-one-key-package-per-peer.md @@ -117,7 +117,10 @@ Marking now keeps the record and sets `synced` on it: Marking an unknown, expired or consumed id does nothing. Marking a package the push path already handed to a peer is allowed and logged: the two holders now share an init key, the peer's copy stays openable, and if the uploaded copy is -spent first, the next push to that peer mints a successor. A package can still +spent first, the next push to that peer mints a successor. The mark takes +`&mut self`, so it holds the write half of the manager's lock while the push +path's claim holds the read half. Both rewrite the same record, and under two +read guards a claim stored second would erase the mark with nothing logged. A package can still be claimed by a push between the application's list call and its mark; the window is one discovery event wide and the outcome is the same as this case. From a6c24ac70bb3d45076c87606a2474f21ffd9ec91 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Fri, 2 Oct 2026 13:20:37 +0530 Subject: [PATCH 5/6] fix(mls): count only hand-out-able packages toward the push pool ensure_min_key_packages exists to keep the push path stocked, and it counted every live record to decide how many to mint. A slot package was already never handed to a peer, and since marking now keeps a synced package's record, that one is never handed out either. Each of them still counted toward the minimum. So the pool came up one package short for every package somebody else holds. Nothing in production calls this today, which is the only reason it is not a bug report. Count the same pool take_push_key_package counts against its ceiling. count_valid_key_packages keeps counting every live record, and its doc now says so instead of implying it is the push pool. --- crates/offline-protocol-mls/src/manager.rs | 39 +++++++++++++++++++--- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/crates/offline-protocol-mls/src/manager.rs b/crates/offline-protocol-mls/src/manager.rs index d3fd190ee..77a9fd3bf 100644 --- a/crates/offline-protocol-mls/src/manager.rs +++ b/crates/offline-protocol-mls/src/manager.rs @@ -1782,18 +1782,27 @@ impl MlsManager { /// ceiling stops it ever handing out, while holding the pool at capacity /// so every peer past the last claim is advertised a shared package. /// + /// Only packages the push path can hand out count toward `min`. A slot + /// package or one the application marked synced is held by somebody + /// else and never advertised to a peer, so counting it would leave the + /// pool short by exactly that many. + /// /// # Returns /// - /// Returns the total number of valid key packages after ensuring minimum. + /// Returns the number of live packages the push path can hand out after + /// ensuring the minimum. pub fn ensure_min_key_packages(&self, min: usize) -> Result { let min = min.min(MAX_PUSH_KEY_PACKAGES); let key_type = StorageKeyType::KeyPackage.as_str(); let package_ids = self.storage.list_keys(key_type)?; - // Count valid (non-expired) packages + // The same pool `take_push_key_package` counts against its ceiling. let mut valid_count = 0; for package_id in &package_ids { - if self.load_stored_key_package(package_id)?.is_some() { + if self + .load_stored_key_package(package_id)? + .is_some_and(|bundle| !bundle.withheld_from_hand_out()) + { valid_count += 1; } } @@ -1814,7 +1823,12 @@ impl MlsManager { Ok(valid_count) } - /// Returns the number of valid (non-expired) key packages available. + /// Returns the number of live key package records: unexpired, init key + /// resident. + /// + /// This includes slot packages and packages the application marked + /// synced, which are never handed to a peer. For the pool the push path + /// draws from, see [`Self::ensure_min_key_packages`]. pub fn count_valid_key_packages(&self) -> Result { let key_type = StorageKeyType::KeyPackage.as_str(); let package_ids = self.storage.list_keys(key_type)?; @@ -2722,6 +2736,23 @@ mod tests { ); } + /// A synced package is never advertised to a peer, so it is no part of the + /// pool `ensure_min_key_packages` keeps stocked. Counting it would leave + /// the push path one package short for every package the app published. + #[test] + fn test_ensure_min_does_not_count_a_synced_package_toward_the_pool() { + let mut manager = create_test_manager("alice"); + let synced = manager.generate_key_package().unwrap(); + manager.mark_key_package_synced(&synced.package_id).unwrap(); + + assert_eq!(manager.ensure_min_key_packages(1).unwrap(), 1); + assert_eq!( + manager.count_valid_key_packages().unwrap(), + 2, + "a replacement was minted beside the synced package" + ); + } + /// The point of keeping the init key: a stranger who fetched the uploaded /// copy can still establish with us. #[test] From 45c4646604a2f4e73781ae8e5be37ff134c26d5f Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Fri, 2 Oct 2026 13:20:46 +0530 Subject: [PATCH 6/6] docs(mls): say the app mints the packages its upload loop publishes The documented upload loop lists pending packages, uploads them and marks them. Since the pending list stopped returning the engine's own packages, nothing the SDK mints ever lands in it. On a fresh install the loop finds an empty list and uploads nothing, without a word. That is correct behaviour. Those packages were the engine's to hand out, and uploading them is what got one init key pointed at two parties. But an app that copied the old snippet goes from populating its key server to silently not, and the docs did not say why. Put the generate step in the loop and say it out loud, in the guide, on the React Native method and in the changelog. --- CHANGELOG.md | 4 +++- bindings/react-native/src/index.ts | 4 +++- docs/mls-integration.md | 14 +++++++++++--- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1ad31d07..f57b0f345 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -357,7 +357,9 @@ archived by series under [docs/changelog/](docs/changelog/); see the `mls_get_pending_key_packages` now lists only packages the application may publish: it used to include packages the engine had pushed to a peer or held in its own Nostr slots, which the documented upload loop would then have - marked, stranding each old key as the engine minted a successor. Marking an + marked, stranding each old key as the engine minted a successor. The SDK adds + none of its own packages to the list, so an application that publishes + packages mints them with `mls_generate_key_package` first. Marking an unknown or already used id does nothing, and marking takes the MLS manager's write lock, so a concurrent push can no longer erase it. A source guard refuses any new record-only delete of a key package diff --git a/bindings/react-native/src/index.ts b/bindings/react-native/src/index.ts index 01805d057..77da7ba57 100644 --- a/bindings/react-native/src/index.ts +++ b/bindings/react-native/src/index.ts @@ -2885,7 +2885,9 @@ export class OfflineProtocol { * * Only packages nobody has spoken for are listed: never one the SDK has * already handed to a peer or one standing in its own publication slots, - * and never one already marked synced. + * and never one already marked synced. The SDK adds none of its own, so + * the list is empty until the app mints packages with + * `mlsGenerateKeyPackage`. * * @returns Array of key packages free to publish */ diff --git a/docs/mls-integration.md b/docs/mls-integration.md index 226879b32..93514d4b4 100644 --- a/docs/mls-integration.md +++ b/docs/mls-integration.md @@ -904,9 +904,11 @@ DELETE /keys/{userId}/{pkgId} # Delete used key package ### Syncing Key Packages -The pending list holds only packages this app may publish. Packages the SDK -has already pushed to a peer, or published in its own slots, never appear in -it. Marking a package synced keeps it on the device, so a Welcome built from +The pending list holds only packages this app may publish, which means +packages it minted itself with `mlsGenerateKeyPackage` and has not marked yet. +The SDK mints its own packages for peers it pushes to and for its own slots, +and those never appear in the list, so on a fresh install it is empty until +the app generates what it intends to upload. Marking a package synced keeps it on the device, so a Welcome built from the uploaded copy still opens, and stops the SDK handing it to anybody else. It expires with the lifetime it was minted with (30 days), and its private key is destroyed a week after that, used or not. Your server should drop its copy @@ -914,6 +916,12 @@ by `expiresAtMs`: past it the package's own validity window has closed, and a week later this device can no longer open a Welcome built from it. ```swift +// Mint the packages this app intends to publish. The SDK never adds any of +// its own to the pending list. +for _ in 0..