fix(mls): keep a synced key package's record so expiry still owns it - #500
Merged
Merged
Conversation
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.
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.
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.
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.
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.
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.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
What this does
mls_mark_key_package_synceddeleted a key package's record and left its private init key in the MLS provider. Keeping the key was right, because a Welcome built from the uploaded copy has to open. Deleting the record was not, because the record is the only thing that carries expiry. A published package nobody claimed kept its key for the life of the install (#367).syncedon it. Nothing has ever set that field, which has been on the bundle since the first MLS commit. The package now expires, and its key is destroyed past the 7-day grace window like any other.The second commit fixes a separate bug on the same methods. The React Native wrappers read
createdAtandisSynced, but both native bridges sendcreatedAtMs,expiresAtMsandsynced. JavaScript has always receivedundefinedfor both fields. The three key package wrappers now share one mapper, andMlsKeyPackagegainsexpiresAt.ADR 0012's gap section, the session lifecycle state machine, the MLS integration guide, the React Native method table and the changelog are updated.
Verification
cargo clippy --workspace -- -D warnings,cargo test --workspace --lib,cargo fmt --all -- --checkand rustdoc under-D warningspass. Doctests were not run locally.npm run test:jspass, including the newkey-package-mapping.test.js.Not in this PR
mesh-sdk/methods.mdxstill describes the old upload loop.Closes #367.