logs.gokuls.in

11 pull requests merged across 1 repo

Offline-Protocol/offline-protocol-sdk

What

Adds a README.md to each of the eight publishable crates, and guards the omission from recurring.

Why

None of the crates had a README, so all eight crates.io pages show nothing but their one-line description.

This is fixable only *forward*. A crates.io version is immutable — the README is extracted from the .crate archive at publish time, there is no edit-metadata API, and docs.rs builds from the same frozen tarball. The pages fill in when the next release publishes; no already-published version's page changes.

The reason it went unnoticed is worth naming, because it is the same trap for the next crate: cargo infers readme = "README.md" when the file sits beside Cargo.toml, and infers nothing — silently — when it does not. No warning at package time, no failure at publish time, and the omission is visible only on the rendered page afterwards.

Per-crate text, not the root README shared eight ways

offline-protocol gets the fullest page, including the "the crates are I/O-free, you must implement the platform bridge" caveat from the root README, since it is the crate someone actually reaches for with cargo add.

The internal layers get short pages that say what the layer is and point at offline-protocol and the RN/Python bindings — the question someone landing on offline-protocol-core actually has. Sharing one README across all eight would put an SDK-wide quick-start on pages where it is misleading.

All links are absolute GitHub URLs; relative links do not resolve on crates.io.

offline-protocol-bench is publish = false and is correctly skipped.

Guards

A crate added later inherits nothing from these, and the consequence lands after an immutable publish — the same shape as the existing per-crate LICENSE assertion, so it is enforced the same way:

  • scripts/check-crate-readmes.sh (new) — asserts a README exists for every crate not marked publish = false. Follows an explicit readme = "..." key if one is set rather than checking a file cargo may not be reading, and rejects readme = false on a publishable crate.
  • scripts/check-license-consistency.sh (extended, +8 lines) — asserts each crate README carries the copyright notice. These now ship inside redistributed archives, which is the same reason the root and per-binding READMEs already must, and that script already owns the notice check.

Both run in CI's existing License Consistency job.

Verification

  • cargo metadata confirms all eight crates now resolve readme='README.md' (auto-detection, no manifest change needed).
  • cargo package --list confirms README.md is actually inside all eight tarballs.
  • Both guards were negative-controlled — hid a README, stripped a notice — and each fails with the intended message rather than passing vacuously.

No Rust code changes; nothing compiles differently.

Release cut for v0.21.0 — the identity release. First release under the coupled versioning scheme (#357): the tag, npm, pyproject, Cargo workspace, and crates.io all carry 0.21.0, and release.yml's version gate verifies the manifests against the tag before either publish job runs.

Version bumps

  • Cargo.toml[workspace.package].version + the seven internal-dependency requirements in [workspace.dependencies] (member crates inherit via version.workspace = true); Cargo.lock re-locked
  • bindings/python/pyproject.toml → 0.21.0
  • bindings/react-native/package.json + lockfile via npm version (podspec reads from package.json)
  • THIRD-PARTY-NOTICES.md ×3 regenerated (they list our own crates by version; the drift gate fails on stale copies)

Prose

  • CHANGELOG: [Unreleased][0.21.0] — 2026-08-13. A coverage audit of all 55 commits since v0.20.1 found four missing user-visible clusters, now added: mesh forwarding (#324), the breaking BLE identity cutover (#330), the iOS/Android BLE main-thread fixes (#338/#341), and the telemetry reason classification (#353/#354/#358). Also amended the #335 entry that still claimed RELAY_ADDRESS_DECLARATION_REFUSED carries the relay's reason text verbatim — #358 changed exactly that — and added a CI/CD line for the SHA-pinned release workflow (#355).
  • SECURITY.md: supported table → 0.21.x current / ≤ 0.20.x unsupported
  • UPGRADING.md: current line → v0.21.x; the "only one build-breaking change since v0.17.0" intro corrected (v0.21.0 breaks every surface via §14); §14 stamped (v0.21.0) with all anchor references renamed across four docs; new §11.2 for the one compile-fine-but-behaves-differently change (secure_session_failed can now fire while the session stays live)

Verified

  • cargo fmt --all -- --check, scripts/check-license-consistency.sh, npx tsc --noEmit (RN) — all green locally
  • clippy + cargo test --workspace --lib running; only manifest/docs changed vs. main, where CI is green

After merge: tag v0.21.0 on main fires release.yml (npm + crates.io + GitHub release). Optionally rehearse with a v0.21.0-rc.1 tag first — it runs every gate and publishes npm under next without burning the crates.io number.

Closes #351.

The last two open leaks from the #346 audit. The telemetry scrubber hashes an event's named actor fields and ships free-text reason/detail verbatim by design — content scrubbing is deliberately deferred to a future emit_content knob — so the fix for a leak here is always at the producer. This follows the shape #353 and #354 settled on: classify to a fixed local vocabulary, return &'static str, keep the wording in a device log.

#350 — SecurityWarning.reason

The control-gate arm reproduced the #345 decoy exactly. One refusal fires two events: verify_sender_derivation emits a carefully identifier-free SenderAddressMismatch, and the gate then rendered the returned Err into ControlSignatureInvalid beside it — naming the claimed sender *and* the address the signing key really derives to. The claimed sender is that event's peer_id, so the cleartext copy sat next to its own hash; the derived address is a second identity the sink never had.

Four arms fixed, plus one gap: the unparseable-sender sibling had no sanitized event of its own and reported only through the gate's rendered catch-all. It now reports SenderAddressMismatch like the arm it belongs beside — identifier-free for the stronger reason that what it would echo is arbitrary wire input, not merely an address. The two relay-binding arms rendered declared, which is *also* the event's hashed peer_id; the fourth quoted the relay's own refusal text.

#351 — transport and relay send-failure text

Relay-sourced over the FFI (recipient_unreachable: <relay text>) reached MessageUndeliverable.reason, ConnectionRequestUndeliverable.reason and the persisted WelcomeSendFailed.transport_error. Locally raised errors interpolate the recipient (PeerNotReachable("… link to {peer_id}")), so a routine send failure shipped the counterparty's address — and MessageDeferred had no hashed field at all, making that address the only identity in the record. It now carries a typed recipient.

The recipient_unreachable token is load-bearing, not cosmetic. The bridges hardcode it and core prefix-matches it to fast-fail connection requests and park DMs. The classification *is* the bare token, so every protocol decision survives and only the prose is dropped — the test asserts the park still fires for that reason.

Three things the issues did not name

  • A fourth feeder. try_send_welcome stored Some(err.to_string()) — a locally rendered transport error — into the same persisted field. Pushing &'static str into the Event constructors made it a compile error rather than something to spot.
  • A latent classification bug. send_via_transport flattened every transport error into Error::Other(format!(…)), discarding the variant SessionStateError::classify and is_no_carrier_error key on. On that path they answered Unknown/false and the rendered string was the only surviving signal — precisely the string that must stop being read. Un-flattened.
  • An existing test asserted the opposite of this change — that the relay's refusal text must reach the event so an operator can tell causes apart. The premise did not survive: reason goes to sinks, so "reaches the operator" and "reaches a remote sink" were the same act. Rewritten to pin the new contract and say why.

Nothing app-visible is lost

No example app reads these fields (repo-wide grep for recipient_unreachable in TS: zero hits), the docs promised only the prefix, the raw wording stays in the device log, and on the internet path the bridges already deliver it to apps on the diagnostic channel.

Testing

Eight new integration tests plus five classifier unit tests. Every test is premise-guarded — the forged-frame one asserts the error really *does* render both addresses, and that both events fired, since covering only one is the decoy itself — and asserts absence of a junk marker as well as of off1, because half of what these arms carry is sender-chosen text that need not look like an address.

The guards did real work. The MessageDeferred premise proved the DORS path collapses to SendFailed("All transports failed") and names no peer, so the test was retargeted to the forced-transport path where the backend's own error survives. And a mutation run showed a fourth test passing that should not have: it was pinning the writer, not the reader. Tracing it established that all five writers overwrite the field before their emit, so a legacy persisted record *cannot* carry raw text into an event — the emit-site classification is a backstop on a persisted field with five writers, and is now documented as exactly that rather than as the legacy path.

All were mutation-checked by restoring the interpolations. The failure output is the defect stated better than prose manages:

an address reached an event reason verbatim: SecurityWarning {
  peer_id: "[REDACTED]", reason_code: ControlSignatureInvalid,
  reason: "Control message rejected: Sender address mismatch: 'off1qywq…' claimed, key derives to 'off1qyvh…'" }

cargo test --package offline-protocol --lib: 1301 passed. cargo fmt --all and tsc --noEmit clean. Full workspace test run skipped per request; CI covers it.

Contract notes for review

  • MessageDeferred gains recipient (additive JSON, mirrored in types.ts).
  • Four Event constructors now take &'static str instead of String — a Rust-API break for anyone constructing SDK events, which nothing outside the crate does. It is what makes interpolation unrepresentable rather than merely discouraged.
  • No UDL, bridge, or migration changes.

What

Publishes the Rust workspace to crates.io, and makes the workspace version *the* release version.

A publish-crates job runs beside the npm publish, behind the same build/test gates, and ships all 8 crates in dependency order. Nothing publishes to either registry unless every build and test job is green and the repo and the tag agree on what version this is.

The version becomes one number

[workspace.package].version moves from the decoupled internal 0.2.0 to 0.20.1, in lockstep with the git tag and the npm package. Two numbers for one release is a fine trade while nothing is published; it's a bad one once cargo add offline-protocol is how people consume the SDK, because crates.io would carry a version nothing else in the project answers to. bindings/python/pyproject.toml moves with it — it already tracked the Cargo version, and leaving it out is how it quietly becomes a third scheme.

The version is verified, not written. The npm job sets its version from the tag; the same trick would be wrong here. Cargo.lock pins the workspace members' own versions, so rewriting manifests at release time invalidates it (and --locked verification stops working), and the published .crate would no longer match the tag it claims to come from — the exact property the release attestations exist to make checkable. So the bump is part of the release cut, now written down in CONTRIBUTING.md under "Cutting a Release", and CI's job is to refuse a tag that disagrees.

The gate is a shared job, not a step

version-gate is its own job that both publish jobs depend on. This is the correctness core of the PR, and the first draft got it wrong by putting the check inside publish-crates.

The two publish jobs are independent siblings behind an identical needs list, so a check living in one has no hold over the other. For every other way publish-crates can fail — missing token, rate limit, dropped connection — that's harmless, because all of those are recoverable by re-running against the same tag. A forgotten version bump is the exception, and it's also the mistake this design invites: every release through v0.20.1 was cut without touching Cargo.toml at all. Checked inside the crates job, it fails there in seconds while the npm job publishes the number and cuts the GitHub release anyway — and nothing recovers that. Re-running checks out the same unbumped source; moving the tag onto a bumped commit is the force-move CONTRIBUTING now forbids. The number is burned and the release split permanently.

As a shared prerequisite, the same mistake fails everything with nothing published anywhere, within a minute of the tag push (the job needs nothing, so it doesn't queue behind the build fan-out). That also makes the error message honest — it says *delete and re-push the tag*, which is correct only because the gate runs first.

The CARGO_REGISTRY_TOKEN presence check deliberately stays inside publish-crates: it *is* re-runnable on the same tag, so it has no business blocking npm.

What can reach a real crates.io upload

REAL_PUBLISH requires a v* tag ref, not a dry run, and no - or + anywhere in the tag.

The suffix test is blanket rather than the release job's -alpha/-beta/-rc list, because the gate compares release *cores*: a suffix the list misses (-pre.1, a typo like -cr.1) would clear it, and v0.21.0-pre.1 against a 0.21.0 workspace would really publish the final, immutable 0.21.0 from prerelease code — after which the eventual v0.21.0 tag no-ops on crates.io while npm and the GitHub release ship the real thing. + is the same hole one character over (build metadata is stripped before comparing, and + is legal in a refname). Nothing is lost by the broader test: a workspace version carrying either character can never match a core-compared tag.

Dry runs and prerelease tags still pass the gate and run the full packaging verification — they just stop short of uploading. That makes a vX.Y.Z-rc.N tag a genuine full rehearsal, which is worth having.

Publisher mechanics

  • cargo-workspaces@0.4.2 (publish --publish-as-is --no-verify --yes) — topological order, so there's no hand-maintained crate list to drift when a member is added, and it unconditionally skips versions already on the registry, which is what makes a partially-failed run resumable. Native cargo publish --workspace gives neither.
  • Verification runs first, separatelycargo publish --workspace --locked --dry-run, unconditional, with no token in the environment. crates.io uploads are immutable and not atomic, so a packaging defect discovered at crate six leaves five published at a version the run can never complete; since all eight version in lockstep, the whole workspace would have to skip to the next number with one crate half-published forever. Skip-already-published rescues *transient* faults, never deterministic ones.
  • Supply-chain posture matches the release job — every action SHA-pinned (the top-of-file rationale now covers this job and checkout's role in deciding what gets packaged), no rust-cache, no cached cargo-workspaces binary, and --no-verify on the publish itself: all three decline the same vector, which is executing third-party build scripts in a step holding the registry token.

Traps found and documented in the job

1. --locked must stay off the cargo-workspaces invocation. It temporarily strips [dev-dependencies] from crates whose dev-deps name workspace siblings with versions; the shrunken dep set needs a lockfile update, and --locked refuses. Reproduced on a synthetic workspace: seven crates published, death on the eighth. The dry-run pre-flight keeps --locked and is where lock drift gets caught.

2. cargo-workspaces' own dry-run mode exits 0 even when publishes fail (downgrades them to warnings) — which is why the pre-flight uses native cargo. Real-mode failures return a proper error.

3. --from-git (as used elsewhere) is a hidden clap alias of --publish-as-is; the job uses the documented spelling.

Also fixed here

Prereleases no longer hijack the npm latest dist-tag (own commit). npm publish tags every upload latest unless told otherwise, so a vX.Y.Z-rc.N tag — or a branch dispatch, which falls back to 0.0.0-dev — became what a plain npm install resolves to. Latent until now because we never cut rc tags; this PR makes cutting one a reasonable thing to do, so it stops being latent. Prerelease versions go to next, derived from the resolved semver-validated version rather than the ref (plenty of our branch names have hyphens).

Verification

  • Job graph parses and resolves: version-gate is in both publish jobs' needs; 13 jobs, no dangling dependencies.
  • Gate script executed against the real repo: matching tag passes, v0.20.1-rc.1 passes on core comparison, v0.21.0 (the forgotten-bump case) fails with the annotation.
  • Dist-tag derivation checked across 0.21.0latest, 0.21.0-rc.1 / -pre.1 / 0.0.0-devnext.
  • Every run: block in the workflow shellchecked at -S warning; the three findings are pre-existing and in jobs this PR doesn't touch.
  • cargo publish --workspace --dry-run packages all 8 crates cleanly in dependency order (bench auto-skipped via publish = false).
  • Skip-already-published confirmed in cargo-workspaces 0.4.2 source (is_publishedcontinue); real-mode failure returns Err(Error::Publish).
  • All 8 crate names are already reserved on crates.io at 0.0.0 under the maintainers team, so these are new-*version* publishes and the strict brand-new-crate rate limit doesn't apply.

Before the next tag

Add the CARGO_REGISTRY_TOKEN repository secret (crates.io API token from an owner account; publish-update scope suffices since all names exist). The job fails fast with instructions if it's missing — and that failure is re-runnable on the same tag, which is why it isn't in the shared gate.

Known pre-existing loose end (unchanged by this PR): the repo-relative drift-guard tests panic under cargo test inside a published tarball (noted in #273 review) — doesn't affect publishing, since verify builds don't run tests.

What

Pins the four unpinned third-party actions in release.yml to immutable commit SHAs, and folds in Dependabot #340 so the one action that *was* already pinned lands current.

actionbeforeafter
dtolnay/rust-toolchain ×7@stable (a branch)4360b52 # stable
Swatinem/rust-cache ×5@v26323deb # v2.9.2
nttld/setup-ndk ×1@v1ed92fe6 # v1.6.0
softprops/action-gh-release ×1@v33d0d988 # v3.0.2
taiki-e/install-action ×143aecc8 # v2cb33e69 # v2.85.8

Functionally a no-op. Every SHA is the commit its mutable ref resolves to *right now*, verified against upstream:

$ gh api repos/<owner>/<repo>/commits/<sha> --jq .commit.message
dtolnay/rust-toolchain       4360b52  ->  "toolchain: stable"   (= stable branch head)
Swatinem/rust-cache          6323deb  ->  "2.9.2"               (= v2 tag head)
nttld/setup-ndk              ed92fe6  ->  "1.6.0"               (= v1 tag head)
softprops/action-gh-release  3d0d988  ->  "release 3.0.2 (#818)" (= v3 tag head)
taiki-e/install-action       cb33e69  ->  "Release 2.85.8"

Why

taiki-e/install-action already carried the comment *"Pinned because this job later receives release credentials."* The reasoning was right; it just wasn't applied to the other four — one of which runs in that same job.

A tag is a mutable pointer the upstream owner can re-point at any commit. An upstream account compromise therefore lands in the next release build with no change on our side and nothing in the diff to review. dtolnay/rust-toolchain was worse than a tag: stable is a *branch*.

The blast radius grew when #314 added attestations:

  • The release job holds contents: write, id-token: write, attestations: write, and reads NPM_TOKEN. softprops/action-gh-release@v3 runs there. Anything executing in that job can mint GitHub attestations and npm provenance signed by this workflow's own identity — forging exactly the signal we tell consumers to trust over SHA256SUMS.txt ("whoever can replace an asset can replace the manifest in the same operation").
  • The build jobs hold no credentials, but they produce the .a/.so/.dylib/.dll the release job then attests. Tampering there launders a backdoor *through* our provenance rather than around it.

Notes for review

**1. actions/* deliberately left on major tags.** GitHub's own org, served from the same trust root as the runner and the token. This matches cla.yml, which pins its one third-party action and nothing else. Say the word if you'd rather pin everything.

2. Every dtolnay/rust-toolchain site gains an explicit toolchain: stable. This is a necessary companion to the pin, not scope creep. The action reads its default toolchain from the *branch's own* action.yml (stable branch → default: stable), so the ref used to carry that information and an opaque SHA does not. Without the input, re-pinning from a version branch such as 1.87.0 — plausible here, since ci.yml pins exactly that for MSRV — would silently change the toolchain rather than fail loudly.

Pinning does not freeze the Rust version: rustup still resolves stable at run time. Only the action's code is frozen.

3. dependabot.yml comment corrected. Its rationale for ignoring dtolnay/rust-toolchain was *"Our four other uses of this action are @stable, which Dependabot does not version-bump, so ignoring the action outright costs us nothing."* The conclusion still holds but the stated reason no longer does. The comment now gives the real one: dtolnay/rust-toolchain publishes no per-release tags at all, so there is nothing for Dependabot to compare against in either file, and release.yml's pins are bumped by hand (procedure is in the new header comment).

The other four are semver-tagged, so Dependabot will keep those pins current automatically via the existing github-actions ecosystem entry — including its 7-day cooldown.

4. Supersedes #340. That PR bumps taiki-e/install-action 2.83.2 → 2.85.8 on a line this PR also touches. I verified cb33e69 is exactly the commit tagged v2.85.8 and folded it in. Both make the identical one-line change, so this merges cleanly regardless of ordering.

Out of scope

ci.yml has the same unpinned third-party actions (dtolnay/rust-toolchain, Swatinem/rust-cache, android-actions/setup-android, EmbarkStudios/cargo-deny-action, gradle/actions/setup-gradle). It runs with permissions: contents: read and publishes nothing, so the exposure is materially lower and the audit item was scoped to release.yml. Happy to do it as a follow-up.

Testing

YAML-parse-checked (release.yml, ci.yml, cla.yml, dependabot.yml), and every pinned SHA re-resolved against upstream from the edited file. No Rust code touched, so no test run.

The real proof is a workflow_dispatch dry run (dry_run: true skips only the publish steps) — worth doing before the next tag.

The bug, confirmed

__GROUP_ERROR__ is a relay answer. Its reason was copied straight onto an event:

state.emit_event(Event::group_error(payload.reason));   // message_dispatch.rs:1874

and the scrubber had nothing to do, because the event had no hashed field at all:

Event::GroupError { reason: _ } => {}                   // scrub_event.rs:546

Reachability is as the issue describes, with one correction worth stating precisely: the control gate's exemption is *narrower* than "unsigned" — is_unsignable_relay_answer requires all three of the prefix, Internet arrival, and no transport peer identity (security.rs:437). So the no-key-material path is the relay ingest shape specifically. A mesh peer needs a valid signature — but the handler is deliberately not Internet-gated (message_dispatch.rs:1845), so any signed peer reaches it too. Either way the text is remote-chosen.

Two findings from tracing it that the issue did not have:

  • The honest relay leaks through this field too. websocket.rs:2213 sends format!("Not a member of group {}", group_id) — a group id rendered into prose, past a scrubber that hashes group_id *fields*. This is not only an attacker story.
  • Nothing downstream wanted the text. The sync revocation keys off payload.group_id; the bridges' admin-denial matching runs on the raw relay frame *before* injection; and both bridges already dual-emit the raw frame on the server-message channel (InternetManager.swift:1897, InternetManager.kt). Apps that need the relay's exact wording still have it.

The fix

Producer-side, per the issue's first option and the #346 / PR #353 precedent — a scrubber-side regex would contradict that file's own architecture.

  • GroupErrorPayload::classify_reason() -> &'static str maps the wire text to not_found / sync_denied / error. The return type is load-bearing: it makes interpolating wire input unrepresentable, not merely discouraged. Matching is exact and the fallback closed, so a relay rewording an error degrades to error — never back to shipping text.
  • The structure the prose carried comes back as a typed field. Event::GroupError gains group_id: Option<String> (additive, skip_serializing_if), which the scrubber hashes like every other group id. Dropping the text without this would have lost real information; smuggling it inside prose was the actual defect.
  • The relay's wording stays in the device warn!, bounded (bounded_wire_text, 200 bytes, char-boundary safe — note str::get(..n) returns None mid-codepoint, so the usual .unwrap_or(text) fallback would log the whole untruncated string).

I checked that tracing has no bridge to TelemetrySink before keeping the raw text in the log — there is none, so it stays device-local.

The general rule the issue asked about

> Worth deciding alongside: whether an event with no scrubbed field at all should be able to carry free text sourced from the wire, as a general rule.

Settled in the scrubber's own policy doc, where the rest of the policy lives: an event field never carries text chosen by a remote party — not shortened, not sanitized in place, but classified, with remote wording kept bounded in a device log if it is worth keeping. Plus the two habits: return &'static str, and when the dropped text was carrying structure, add it back as a typed field.

Blast radius

Wire-shape additive and the type tag is unchanged, so the types.ts tag guard is unaffected and no UDL/binding regeneration is needed (events cross UniFFI as JSON). reason changes meaning from relay prose to a fixed code — RN GroupErrorEvent.reason is now a union; no producer of that type exists in the package, and neither fernweh app consumes group_error at all (the two example apps display/log it and degrade to showing a code).

Verification

cargo fmt --all, cargo clippy --workspace -- -D warnings, cargo test --workspace --lib all clean. (--all-targets clippy reports ~1445 pre-existing unwrap_used findings in test code on the base commit too — unrelated.)

New tests: test_group_error_reason_is_classified_not_quoted_from_the_wire, test_group_error_known_relay_wordings_map_to_distinct_codes, group_error_hashes_its_group_id_and_leaves_the_code_alone, test_group_error_event_wire_shape.

The producer test is premise-guarded — it asserts the injected frame really does carry the marker and the address, because otherwise "the event contains neither" passes vacuously. Mutation-checked by restoring the verbatim emit; the failure states the defect better than prose could:

assertion `left == right` failed: unrecognized relay wording must classify to the closed fallback
  left: "LEAKMARKER-9f3: off1q9f9vz4hxl7qd8dd8u6ymseyay29hz509ytuwzpx was denied in group secret-group-42 (see audit)"
 right: "error"

Not in scope

#350–#352 (the other sinks from the same audit). Moving relay answers off the message plane onto dedicated FFI entry points — the fix for the *authentication* half — remains the follow-up prefixes.rs already names; it would not remove the need for this change, since the honest relay interpolates identifiers too. A machine-readable code field on the relay's GroupError is the durable upgrade, and belongs in the relay-server repo.

The defect

handle_welcome_message filled error_reason with e.to_string() on both session-Welcome failure arms, and error_reason becomes secure_session_failed.reason, which the telemetry scrubber ships verbatim — it hashes only peer_id (scrub_event.rs:387), and that is deliberate policy for reason fields. So the problem was never the scrubber; it was what these arms put in one.

Two reachable variants render identifiers:

  • WelcomeIdentityMismatch — raised by verify_welcome_slot *before* the Welcome blob is deserialized, so it is reachable by anyone who can put a frame on the wire: no key material, no valid Welcome, no prior contact. #346 did not name this one; it is the cheapest repro and the one the first test below uses.
  • WelcomeGroupIdMismatch — raised by verify_staged_group_id. Its embedded field is from_utf8_lossy over raw GroupContext bytes, so unlike the wire field it passes through neither charset validation nor GroupId::MAX_LEN.

Each renders a session:<a>:<b> slot — two addresses, one of which may be a third party with no part in the exchange — plus a string the sender chose. The mutation run makes it concrete; with the fix reverted, the test fails with:

SecureSessionFailed { peer_id: "[REDACTED]", reason: "Welcome session-slot mismatch: inviter maps to slot
'session:off1qyvh9kjj…:off1qy60a3pu…', Welcome claims 'session:off1q9s8p83d…:off1qxmdt9v3…'" }

peer_id redacted, four raw addresses beside it.

The fix, and why it is not a third special case

#345 fixed this class on the two *identity-refusal* arms by substituting a fixed string, and argued explicitly against generalizing: "every other join failure here is a fault rather than an accusation and names nobody, so those keep the raw error — please don't turn this into a blanket sanitize." That premise was false, which is what #346 establishes. Patching a third arm would leave the same default in place for the fourth.

So the rule is inverted, as the issue asks. MlsError::privacy_safe_reason() -> &'static str (error.rs) maps every variant to a fixed classified string. Two properties are load-bearing:

1. The return type is &'static str, so a caller cannot interpolate a value into it.

2. The match is exhaustive, which is legal despite #[non_exhaustive] only because it lives in the defining crate — so a newly added variant fails to compile *there*, forcing the privacy decision where variants are written. There is deliberately no _ => arm; adding one restores the per-site opt-in this replaces.

The #345 arms keep their hand-written string: it describes the refusal in the app's terms ("a session invite was declined") where the generic classification speaks about ratchet-tree leaves. Both are identifier-free.

A third feeder, found by the audit

#346 also asked whether other reason sinks have the same defect. Auditing every unscrubbed field turned up a third feeder of *this same event*, which the issue did not mention: the persist-confirmation Err arm rendered a storage error that interpolates the record's id, i.e. the peer's raw address. It now emits a fixed string, with the error in a warn! beside it.

Coverage

  • identifier_bearing_variants_classify_without_their_payload (mls crate) — every identifier-bearing variant, with a premise guard asserting each one really does render its payload first; without that the assertion passes vacuously on an empty variant.
  • test_welcome_naming_a_foreign_session_slot_is_refused_without_naming_it — the pre-deserialization path, end to end through process_internal_message.
  • test_welcome_embedding_a_foreign_session_slot_is_refused_without_naming_it — the staged-group-id path.

Both end-to-end tests were mutation-checked: reverting the arms to e.to_string() fails both. The adopt-path arm is covered by construction (same call) rather than by an artificial degraded-state fixture — it is shadowed by the has_session duplicate check for every error that leaves the session intact.

cargo fmt --all --check, cargo clippy --workspace -- -D warnings, RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps and cargo test --workspace --lib all pass.

Behaviour change for apps

secure_session_failed.reason now carries a failure *class* rather than a rendered error. The event doc already stated the field is diagnostic text that must not be parsed, and events cross UniFFI as opaque JSON, so there is no UDL regen and no binding change. Full errors remain in the device log at each refusal site.

Out of scope, filed separately

The audit found leaks of the same class outside this seam. None are touched here:

  • #349 — GroupError.reason ships unsigned, relay-chosen wire text verbatim, on an event with no hashed field at all. Arguably worse than this issue.
  • #350 — SecurityWarning.reason interpolates addresses on the control-gate and relay-binding arms, de-anonymizing the hashed peer_id in the same record. The control-gate one reproduces the #345 decoy exactly: one sanitized event, one leaking event, same refusal.
  • #351 — transport/relay error text reaching MessageDeferred / MessageUndeliverable / ConnectionRequestUndeliverable / WelcomeSendFailed.
  • #352 — policy question: wire-supplied service-plane fields classified as if the SDK minted them.

One correction to #346 for the record: ReservedSessionNamespace is raised only in MlsManager::join_group (the group-Welcome path) and is not reachable from these two session arms. It is covered by the mapping regardless.

Three claims in shipped artifacts did not survive being checked against the code. Each was verified at HEAD before changing anything, and each is the kind of line a skeptical reviewer tests first.

The overclaims

"No routing tag has a computable private half" was false. routing_tag_for_address is SHA-256(address) → scalar → x-only pubkey, so anyone holding an address can reconstruct the entire keypair behind that address's tag. The same sentence also described *both* derivations as domain-separated when only the seal key is (record_seal_keypair_for_address uses HKDF; the tag is a bare SHA-256). What the addressing migration actually bought is narrower and is now what gets claimed: nothing seals to the tag any more, so reconstructing one decrypts nothing. Recorded on routing_tag_for_address itself, since that's where the derivation is read.

The gift-wrap anonymity set is not "every NIP-17 conversation on the relay". Reusing kind 1059 defeats a scrape *by kind*; it does not make a tag anonymous to a relay you subscribe on. NOSTR_GIFT_WRAP_KIND and docs/nostr.md §"What a relay can see" now enumerate three distinguishers, none of which has to break a seal:

  • our own REQ names our routing tag to every relay we connect to — decisive, and unavoidable while a recipient subscribes on a stable tag;
  • kinds:[4, 1059] under one #p is a shape a NIP-17-only client does not have;
  • kind-30443 records sit at our own tag signed by the install's real Nostr key, which a relay can join to it.

None of the three reveals content. Two prose restatements of the same overclaim (CHANGELOG.md:883, and a docs/nostr.md paragraph that understated it as requiring an observer who "already holds" the address) are corrected alongside.

The 160-bit second-preimage figure now names its collision bound. ~2^160 is the cost of aiming at an address that already exists. The birthday bound on the same truncation is ~2^80, which buys two identity keys under one address rather than a chosen peer's — enough to equivocate, and enough to defeat the "one identity cannot hold two leaves" property the MLS leaf binding otherwise inherits from signature-key uniqueness. Stated on Address::HASH_LEN as the deliberate trade against BLE frame budget that it is, rather than left for reviewers to derive.

Stale TOFU vocabulary

SECURITY.md invited reports on "TOFU key management bypasses" — a mechanism deleted in this release. It now names the sender-address derivation gate that replaced it, in both its control-frame and MLS-leaf shapes. CLAUDE.md likewise stops calling the control gate "Ed25519+TOFU". The historical UPGRADING.md/CHANGELOG.md references are left alone: they are past-tense migration notes describing the removal, and are correct.

Codename scrub

The internal downstream codename survived in the two npm-shipped Swift files, the historical cleartext envelope reproduced in CHANGELOG.md and docs/nostr.md, and — not previously reported — as the AppId fixture in the transport's sealed-payload leak test, i.e. in shipped Rust and asserted against as a leak string. Renamed to example-app, whose hyphen sits outside both the hex and bech32 charsets, so it cannot collide with that test's chance-substring concern. git grep -i over tracked files is now empty.

The fix/docs-examples-fernweh-scrub branch was not cherry-picked: its other targets are already clean on main, so only this residue remained.

The alice_real_username / bob_real_username placeholders are kept deliberately — they are synthetic, and their naming does the rhetorical work of the sentence that follows them ("Both usernames … were readable by every relay").

Rustdoc CI gate

A new Rustdoc job builds the workspace under RUSTDOCFLAGS: -D warnings. It caught six broken intra-doc links, not the one reported: four public items linking to private ones — which resolve to nothing for every reader not building the crate themselves — and two simply wrong paths. All six are fixed, since a gate that does not pass is not a gate.

Deliberately *without* --document-private-items: that flag would have suppressed four of the six rather than surfaced them.

The gate is negative-controlled — an injected bad link fails the build with error: could not document, so it is not vacuous.

Verification

  • cargo fmt --all -- --check — clean
  • cargo clippy --workspace --locked -- -D warnings — clean
  • cargo test --workspace --locked — 2088 tests, 0 failures
  • RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps --locked — clean

No behaviour change. The only non-comment source edit is the test fixture rename.

The gap

NostrTransport::get_next_message — the generic Transport whole-message poll — implemented itself by popping the send queue, serializing the bare Message, and returning it. It never sealed, signed, or wrapped. The result was the whole protocol envelope with both endpoints on it, so anything that published it put in front of every relay exactly the cleartext gift wrapping exists to prevent — and did so regardless of nostr_sealing_enabled, which this path never consulted.

It has carried a doc comment warning callers off it since #288 sealed the transport. A doc comment is not a control.

Why it was reachable

Two commits set this up between them:

  • c5d2888 moved the method from an inherent method onto the Transport trait, so it no longer required a downcast.
  • b272352 (#288) sealed the transport, hardened the ingress seam — on_data_received refuses a still-sealed frame — and for the egress seam rewrote the doc comment while leaving the body byte-identical.

So reaching the cleartext was a composition of two public methods, no downcast and no unsafe:

\\\`rust

OfflineProtocol::transport_manager() // pub, config_accessors.rs:51

.get_transport(TransportType::Nostr) // pub, transport_manager.rs:1201 -> Arc<dyn Transport>

.get_next_message() // cleartext envelope, both endpoints

\\\`

A secondary hazard rode along: because the old body popped the queue and inserted into pending_confirmation, a caller landing there didn't just leak — it stole the frame out from under the sealed drain.

The change

get_next_message now returns Error::ConfigurationError naming get_next_signed_event as the drain. The refusal returns before the send queue or pending_confirmation are read, so it cannot strand the frame it declines to hand over: the message stays queued and the next get_next_signed_event serves it.

Blast radius

No bundled bridge or UniFFI entry is affected. nostrGetNextMessage routes to get_next_signed_event (uniffi/src/lib.rs:4730), and both bridges call it (\NostrManager.swift:1117\, \NostrManager.kt:807\). The three FFI callers of the trait method are type-pinned to Internet, WiFi Direct and Reticulum. No UDL change, no binding regeneration, no wire-format change.

Only a Rust embedder polling Nostr through \dyn Transport\ changes behavior — and such an embedder was publishing cleartext, so failing loudly is the correct outcome.

Choices worth reviewing

  • \ConfigurationError\ over \SendFailed\ (special-cased by \map_send_error\, and this is an integration bug rather than a delivery failure) and over \TransportNotAvailable\ (which would lie about transport health). No new variant added — \Error\ is \#[non_exhaustive]\ so one would have been non-breaking, but a single call site doesn't earn it.
  • \Err\ over deleting the override (which would inherit the trait's \Ok(None)\, matching BLE's precedent and the documented default calibration). Rejected because this seam already failed once by being quiet: \Ok(None)\ makes a misused drain look like an idle queue, whereas the mistake being guarded against is precisely "a bridge author believes this is the drain."
  • \test_send_receive\ deleted, not converted. Its assertions are each already covered — the queue-to-pending transition by \test_get_next_signed_event\, the content round trip by \test_sealed_frame_round_trips_through_the_recipients_transport\ (which also asserts the sender). A tombstone comment at its old location points at both.

Tests

Six tests used the method only as a fixture step to move a frame into \pending_confirmation\; they now use \get_next_signed_event\, which performs the identical transition from identical setup.

\test_generic_transport_poll_refuses_rather_than_leaking_cleartext\ dispatches through \&dyn Transport\ on purpose — that is the shape the leak had, and a concrete-typed call would not pin the vtable entry that mattered. It asserts the refusal, that the frame stays queued, that nothing enters the confirmation loop, and that the sealed drain still serves that same frame.

Verification

GateResult
\cargo test -p offline-protocol-transport --lib nostr\92 passed, 0 failed
\cargo test --workspace --lib\2075 passed, 0 failed
\cargo clippy --workspace -- -D warnings\clean
\cargo fmt --all -- --check\clean

Mutation-checked: restoring the old body fails the new test, and the panic output decodes to the cleartext envelope — \"sender":"off1qy4a…"\, \"recipient":"off1qxqm…"\, \"content":"Test message"\ — which is exactly what would have reached the relay.

A group member — or anyone whose invite you accept — could claim another

peer's off1… address and be attributed as them.

The trust collapse (#329) made an identity claim provable by re-deriving it

from the key that signed for it, and wired that check to the two planes where

a claim was known to arrive: control frames, and key packages this device

supplies (import_key_package, get_contact_key_package, add_group_member).

The MLS ratchet tree is a third, and it was never enumerated. A leaf arriving

in a Welcome's tree, or in an Add another member commits, was accepted on its

credential alone — which RFC 9420 calls "a bare assertion of an identity".

SEC-M1 rests on that credential. So decrypt_message was comparing a wire

sender the attacker chose against a credential the attacker also chose. On the

__GROUP_MSG__ path — data-plane, exempt from the signature gate precisely

*because* "authentication happens after the gate instead, via SEC-M1" — that

needs no signature from anyone.

Confirmed with a proof of concept before any fix: forged roster entry, and a

forged message delivered to the app as the victim. It is now a regression test.

The fix is the Authentication Service RFC 9420 §5.3.1 assigns to the

application, which for this SDK is one derivation — an address *is*

bech32m(0x01 ‖ SHA-256(signature_key)[..20]). OpenMLS does not perform it and

says so ("This MUST be checked by the application"). Three seams:

SeamWhereCovers
Welcome treeverify_staged_welcome_leaves, before into_groupthe whole tree an inviter picks
Staged commitverify_staged_commit_leaves, pre-merge, before the admin checkall four sources in credentials_to_verify
Message senderverify_sender_leaf at the SEC-M1 seam, O(1)any leaf however it arrived, incl. a tampered store

The third is not redundant with the first two: it is the same import-time plus

use-time pairing get_contact_key_package already documents, and the only one

that holds for a leaf the entry gates never saw.

Unconditional, unlike enforce_admin_commits beside it, and the difference

is the argument for it. That check reads the best-effort-replicated admin

overlay, so two honest members can legitimately disagree and partition each

other. This verdict is computed from the commit's own bytes, so every honest

member reaches the same answer — a refusal forks the *attacker* off a group

that stays consistent.

Related issues

None filed — found during a review of the identity layer.

Type of change

  • fix — bug fix

Checklist

  • cargo fmt --all -- --check passes
  • cargo clippy --workspace -- -D warnings passes
  • cargo test --workspace passes (1278 protocol + 92 MLS + the rest)
  • cargo-deny is satisfied (advisories, bans, licenses, sources all ok)
  • Commits follow Conventional Commits
  • Docs / CHANGELOG.md updated (CLAUDE.md architecture bullet + Unreleased ▸ Security)
  • No new unsafe
  • UDL unchanged — the new SecurityWarningCode crosses UniFFI as a JSON string, so no binding regeneration. bindings/react-native/src/types.ts is the hand-mirrored half and is updated; tsc --noEmit clean.

Breaking changes

GroupManager::remove_member now takes &[LeafNodeIndex] instead of one

index, so it can remove every leaf an identity holds in a single commit.

Public on the offline-protocol-mls crate; no in-tree caller outside the one

updated here.

Behaviourally, a Welcome or commit carrying a leaf that cannot prove its own

identity is now refused where it was previously accepted. Nothing this SDK

produces is affected — see the note on key rotation below.

The new MlsError variants are additive (#[non_exhaustive]).

Notes for reviewers

**The two places the plan was wrong, both found by building it — worth the

scrutiny:**

1. **The commit walk follows OpenMLS's credentials_to_verify (four sources),

not the OpenMLS book's "add & update proposals".** Validating only Add

proposals — the obvious reading, and where this started — leaves a *cheaper*

attack open: a member renames their own leaf to a peer's address through the

commit update path. No new leaf, no key package, no invite. That is

test_member_renaming_their_own_leaf_to_a_peers_address_is_refused, and it

exists because a mutation test survived without it.

2. The remove_group_member first-match issue is not reachable. The test

written to prove it failed with `Duplicate signature key in proposals and

group` — MLS requires unique signature keys, so once credentials are bound

to keys, a duplicate credential is refused twice over. The loop stays as

defence and the comment now says that, rather than asserting a bug it does

not fix. test_one_identity_cannot_hold_two_leaves_in_a_group pins the

invariant it depends on, which lives in OpenMLS.

Why this is safe for honest peers. Nothing in this SDK rotates a leaf

signature key or credential independently of the identity key: update_keys

passes LeafNodeParameters::default(), and self_update_with_new_signer is

never called. If that ever changes, every member starts refusing every honest

key rotation — so it is pinned by

test_key_rotation_and_ordinary_traffic_still_pass_the_binding.

Mutation-checked, after #329's own lesson that its first cut passed 1249

tests with the derivation stubbed to Ok(()) because every fixture signs

honestly. Five sabotages, each failing at least one test: stubbed binding,

waved-through underivable keys, dropped Welcome walk, dropped commit update

path, dropped sender-leaf check. Two of those survived the first pass and each

bought a test.

Judgement calls, flagged rather than buried:

  • Non-Member senders (external joiners/proposals, ExternalSenders

extensions) are now refused outright rather than skipped. This SDK issues

none of them, so nothing honest is lost — but it is a behaviour change

beyond the minimum fix.

  • get_group_info skips unbound leaves with a warn! (as list_groups does

for invalid stored ids) rather than failing. Post-fix such a leaf means a

tampered store; keeping it out of the roster matters because the roster

addresses the per-member fan-out, feeds the rich-payload gate, and supplies

the address-ordered tiebreakers.

  • Shipped as one commit rather than the three the plan proposed: a partial

application leaves the vulnerability half-closed. Happy to split on request.

Deliberate follow-up, not in scope here. RFC 9420 §5.3.1 also requires the

AS to verify a new credential is a valid *successor* to the old one. The

binding already blocks impersonation via Update (forging a leaf named for

someone else needs their private key to sign it); what remains is a member

swapping to a new identity they genuinely control, which confuses the roster

and the user-id-keyed admin overlay. That is roster stability rather than

this vulnerability, and belongs in its own change.

What

scripts/generate-bindings.sh generates Swift, Kotlin and Python from the one UDL, in one run. Every other path delegates to it:

  • bindings/react-native/scripts/generate-bindings.sh (so npm run generate:bindings now covers Python too)
  • bindings/python/scripts/build-desktop.sh (still builds the cdylib pytest needs; only the codegen moved)
  • build-uniffi-ios.sh / build-uniffi-android.sh
  • build-all.sh / build-ios.sh / build-android.sh — now thin wrappers over the build-uniffi-* scripts
  • release.yml's Python and Kotlin steps, so what ships is produced the way CI's gate verified it

Why

A UDL change obligates three regenerations, and the checklist item everyone reads — npm run generate:bindings — covered two of them. The three generated files are one artifact set: they carry the FFI checksums of the library they were generated against, so a partial regeneration fails nothing at build time and fails the app at its first call across the boundary. #330 shipped with that checklist ticked and the Python file stale; the Python drift gate then failed alone in an otherwise-green run, which reads like flake and isn't.

The script also refuses to run when uniffi-bindgen disagrees with the crate's uniffi pin to the minor version — same class of failure, silent until runtime — and reads the pin out of Cargo.toml rather than repeating it. Patch drift is deliberately tolerated: uniffi's FFI contract version is not patch-scoped.

It passes --no-format, which is load-bearing rather than tidiness: uniffi-bindgen post-processes with ktlint and swiftformat when they happen to be on PATH, so without it the committed bytes depend on what each developer has installed, and an Android or iOS contributor with those tools would regenerate correctly and still trip the drift gate.

Two holes this closed along the way

Both were found by review after the first commit, and both are the failure this PR is about, reached from a direction the first version did not cover:

  • release.yml generated Kotlin directly. That artifact is downloaded *over* the committed Android bindings in the publish job, so it — not the committed file — is what ships to npm, produced by a path the guard could not see and the version check never gated. The first version of this PR waved it off as "an ephemeral release directory"; it is not ephemeral, it is the released artifact.
  • npm run build:all regenerated nothing. build-all.sh/build-ios.sh/build-android.sh were near-copies of the build-uniffi-* trio minus the codegen, so they paired fresh native artifacts with whatever was committed — on every run, and six example READMEs open with npm run build:all. Collapsing them into wrappers also fixed three unrelated regressions the copies had drifted into: no check-elf-alignment.py (Google Play's 16 KB requirement, which release.yml enforces), stale NDK toolchain resolution, and a missing release/deps staticlib fallback.

The iOS and Android scripts no longer warn-and-continue when uniffi-bindgen is missing. Pairing a freshly built native library with the previously committed bindings *is* the ABI mismatch this rule exists to prevent, and it surfaces at the app's first FFI call rather than at the build — so failing is strictly better than continuing.

CI

One regeneration, one gate over all three paths, replacing two half-gates that a half-updated commit could satisfy one of. The generated files are deleted before regenerating, because git diff --exit-code on a path nothing wrote to reports no diff — without the delete, "up to date" and "never regenerated" are indistinguishable, and a mistyped output path would sail through green while every later UDL change shipped stale bindings.

Plus scripts/tests/test-generate-bindings.sh, which runs first (no bindgen, no toolchain, under a second). It asserts the rule *behaviorally*: it runs the real generator against a stub uniffi-bindgen and checks the languages, output paths, UDL and --no-format it asked for, then scans every tracked file by content for a direct invocation. Scanning only *.sh would have left the release workflow — the path that ships — exempt.

Verification

  • Regeneration is byte-identical to what was committed — the drift gate passes on a clean CI runner. This changes how they're produced, not what they say.
  • All 15 CI checks green, including the full Python Bindings chain: guard → delete → regenerate → drift gate → cdylib build → pytest.
  • The guard is mutation-tested both ways: a one-character output-path typo, a dropped language, a dropped --no-format, a gutted version gate, a gutted CI delete step, a path dropped from the drift gate, and a build script that stops routing are all caught; a refactor to a loop, --language/--out-dir reordering, --flag=value form, and direct-exec delegation are all tolerated.
  • Its scan is negative-controlled against 10 invocation shapes (inline, line-continuation, --config before the subcommand, cargo run --bin, Gradle Kotlin and Groovy commandLine, subprocess.run, spawnSync, YAML list) and 4 benign ones, with zero false positives across every tracked file.
  • Two vacuous-pass bugs *in the guard itself* were found and fixed by review: it accused itself in three of four invocation forms, and its version-gate control silently did nothing under a patch-level pin. Both now have explicit fixture-exists assertions so "setup never ran" can no longer read as "check passed".
  • shellcheck --severity=warning clean on everything touched; both workflows parse; the guard runs under macOS bash 3.2.
  • release.yml's Kotlin artifact shape simulated end to end — identical to what the publish job's download path expects.

Not executed locally: the iOS/Android platform builds (need Xcode targets / NDK) — those edits are shellcheck- and syntax-verified only.

Known gap (follow-up, not introduced here)

The drift gate compares tracked files, so a future uniffi upgrade that emits a fourth generated file would produce it, leave it untracked, and pass. Worth a git ls-files --others check; independent of this change.