From 5ed33639881e2db4528f5521559784d80cc38a34 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 30 Sep 2026 16:57:14 +0530 Subject: [PATCH 01/12] ci(release): tell a failing npm view from a staging publish The provenance verify step read every empty answer as staging, so a DNS failure or a broken .npmrc polled for twenty minutes and then blamed the registry. Each poll now keeps npm's stderr. Nothing, a warning or an E404 is staging; any other npm error is printed, and the timeout names the last one. --- .github/workflows/release.yml | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 6bdfae3bd..6d66e50b0 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -1112,24 +1112,48 @@ jobs: set -euo pipefail PKG="@offline-protocol/mesh-sdk@$VERSION" + # A version that is not there yet prints nothing, or an E404. An npm + # error of any other kind (a DNS failure, a bad .npmrc, a registry + # outage) is not staging, and reading it as staging would spend the + # whole budget and then blame the registry. So each poll keeps what + # npm said, and the failure below prints the last error. Warnings + # (`npm warn`) are not errors and read as staging. + ERR="$(mktemp)" + LAST_ERROR="" visible=false for attempt in $(seq 1 "$VISIBLE_POLLS"); do - if [ "$(npm view "$PKG" version 2>/dev/null || true)" = "$VERSION" ]; then + SEEN="$(npm view "$PKG" version 2>"$ERR" || true)" + if [ "$SEEN" = "$VERSION" ]; then visible=true echo "$PKG is readable on the registry (poll $attempt of $VISIBLE_POLLS)." break fi - echo "$PKG not readable yet, probably still staging (poll $attempt of $VISIBLE_POLLS)." + if ! grep -qE '^npm (ERR!|error)' "$ERR" || grep -q 'E404' "$ERR"; then + # A missing version after an error means the error passed. + LAST_ERROR="" + echo "$PKG not readable yet, probably still staging (poll $attempt of $VISIBLE_POLLS)." + else + # One line: a workflow annotation ends at the first newline. + # `%` would start an escape in a workflow command. + LAST_ERROR="$(grep -E '^npm (ERR!|error)' "$ERR" | tail -n 5 | tr '\n' ' ' | sed 's/%/%25/g')" + echo "npm view failed, not a staging answer (poll $attempt of $VISIBLE_POLLS): $LAST_ERROR" + fi sleep "$POLL_SECONDS" done if [ "$visible" != true ]; then - echo "::error title=Published version not visible::$PKG was accepted by the \ + if [ -n "$LAST_ERROR" ]; then + echo "::error title=npm view failed::$PKG could not be read back for \ + $((VISIBLE_POLLS * POLL_SECONDS)) seconds, and npm reported an error rather than \ + a missing version, so this may not be staging at all. Last error: $LAST_ERROR" + else + echo "::error title=Published version not visible::$PKG was accepted by the \ publish step but is still not readable after $((VISIBLE_POLLS * POLL_SECONDS)) \ seconds. The registry stages large uploads and can take this long; check \ 'npm view $PKG dist.attestations' by hand before doing anything else. A re-run \ is safe (the publish step skips a version that is already there) but only \ waits again." + fi exit 1 fi From 1932d30639476d373e2cdfc6efbfe56af0050c8c Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 30 Sep 2026 16:57:29 +0530 Subject: [PATCH 02/12] ci: key the iOS Rust caches on the podspec's deployment target cargo does not rebuild a Rust crate when IPHONEOS_DEPLOYMENT_TARGET changes, so a lowered target restored objects stamped for the old one and the minimum-OS gate failed on them. The Swift Package job and the release's iOS job now add the podspec's hash to their cache key. --- .github/workflows/ci.yml | 7 ++++++- .github/workflows/release.yml | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ab4f43704..7d4d12163 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -821,7 +821,12 @@ jobs: - uses: Swatinem/rust-cache@v2 with: - shared-key: ci-swift-package + # The deployment target is the podspec's, and cargo does not rebuild + # a Rust crate when IPHONEOS_DEPLOYMENT_TARGET changes. Without the + # hash a lowered target restores objects stamped for the old one, and + # the minimum-OS gate fails on them with no cargo clean to reach. It + # goes in shared-key: rust-cache ignores `key` when shared-key is set. + shared-key: ci-swift-package-${{ hashFiles('bindings/react-native/MeshSdk.podspec') }} # One simulator slice, for the architecture of this runner. The release # builds the device slice and both simulator architectures. diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 6d66e50b0..1b9aad45e 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -112,7 +112,12 @@ jobs: - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 with: - shared-key: release-ios + # The deployment target is the podspec's, and cargo does not rebuild + # a Rust crate when IPHONEOS_DEPLOYMENT_TARGET changes. Without the + # hash a lowered target restores objects stamped for the old one, and + # the minimum-OS gate fails on them with no cargo clean to reach. It + # goes in shared-key: rust-cache ignores `key` when shared-key is set. + shared-key: release-ios-${{ hashFiles('bindings/react-native/MeshSdk.podspec') }} - name: Cache uniffi-bindgen id: cache-uniffi From 127b3d1224a5762e82e35afb495b103b4aca5e1d Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 30 Sep 2026 16:58:51 +0530 Subject: [PATCH 03/12] fix(bindings): check the deployment target inside package_xcframework The minimum-OS gate was three calls in the build script, pinned by greps on the order of its lines, and four of six edits that disabled it kept the order. package_xcframework now takes the deployment target and checks every archive before it packages anything, and the gate test drives the function with stand-ins for otool, lipo and xcodebuild: a refused archive packages nothing, and each slice is held to its own ceiling. Six mutants of the function and the call are each caught. --- .github/workflows/ci.yml | 5 +- .../react-native/scripts/build-uniffi-ios.sh | 10 +- .../scripts/shared/xcframework.sh | 27 +++++- docs/bridges/swift.md | 23 +++-- scripts/tests/test-ios-min-os.sh | 97 +++++++++++++++---- 5 files changed, 124 insertions(+), 38 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7d4d12163..44a3bf446 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -555,8 +555,9 @@ jobs: # The gate reads a real archive with otool, so it runs where the iOS # library is built: in a release, on all three archives, and in the # Swift Package job below, on the simulator archive of every pull - # request. This drives everything in it that is text handling, with a - # stand-in for otool, on a runner that has none. + # request. This drives everything in it that is text handling, and the + # packaging function the gate sits inside, with stand-ins for otool, + # lipo and xcodebuild, on a runner that has none of them. - name: Test the iOS deployment-target gate run: bash scripts/tests/test-ios-min-os.sh diff --git a/bindings/react-native/scripts/build-uniffi-ios.sh b/bindings/react-native/scripts/build-uniffi-ios.sh index ba26d2588..3e0a81299 100755 --- a/bindings/react-native/scripts/build-uniffi-ios.sh +++ b/bindings/react-native/scripts/build-uniffi-ios.sh @@ -61,12 +61,7 @@ static_lib_for() { fi } -echo "Checking the deployment target of every object..." -assert_archive_min_os "$(static_lib_for aarch64-apple-ios)" "$IOS_DEPLOYMENT_TARGET" -assert_archive_min_os "$(static_lib_for aarch64-apple-ios-sim)" \ - "$(arm64_simulator_ceiling "$IOS_DEPLOYMENT_TARGET")" -assert_archive_min_os "$(static_lib_for x86_64-apple-ios)" "$IOS_DEPLOYMENT_TARGET" - +# Packaging checks every archive against the deployment target first. echo "Packaging the XCFramework..." # Why an XCFramework, and why both slices share one archive basename: see @@ -75,7 +70,8 @@ package_xcframework \ "$OUTPUT_DIR" \ "$(static_lib_for aarch64-apple-ios)" \ "$(static_lib_for aarch64-apple-ios-sim)" \ - "$(static_lib_for x86_64-apple-ios)" + "$(static_lib_for x86_64-apple-ios)" \ + "$IOS_DEPLOYMENT_TARGET" # Generate bindings — all languages, not just Swift: they are one artifact set # off one UDL (see scripts/generate-bindings.sh). diff --git a/bindings/react-native/scripts/shared/xcframework.sh b/bindings/react-native/scripts/shared/xcframework.sh index 3d35b1d4e..e3aa6f8e3 100644 --- a/bindings/react-native/scripts/shared/xcframework.sh +++ b/bindings/react-native/scripts/shared/xcframework.sh @@ -202,15 +202,36 @@ assert_archive_min_os() { esac } -# package_xcframework +# package_xcframework # -# Stages the device archive and a fat simulator archive under one shared -# basename, then builds /offline_protocol_uniffi.xcframework. +# Checks every archive against the deployment target, then stages the device +# archive and a fat simulator archive under one shared basename and builds +# /offline_protocol_uniffi.xcframework. +# +# The gate is here, not beside the call, so the pod's XCFramework cannot be +# packaged without it: a gate the caller has to remember was pinned by the +# order of lines in the build script, and most edits that broke it kept the +# order. scripts/package-swiftpm-xcframework.sh, which packages the Swift +# package's XCFramework, does not come through here; the Swift Package CI +# job calls assert_archive_min_os on its archive itself. +# scripts/tests/test-ios-min-os.sh drives this function with stand-ins for +# otool, lipo and xcodebuild. package_xcframework() { local output_dir="$1" local device_lib="$2" local sim_arm64_lib="$3" local sim_x86_64_lib="$4" + local deployment_target="${5:-}" + + if [ -z "$deployment_target" ]; then + echo "ERROR: package_xcframework needs the deployment target to check the archives against" >&2 + return 1 + fi + + echo "Checking the deployment target of every object..." + assert_archive_min_os "$device_lib" "$deployment_target" || return 1 + assert_archive_min_os "$sim_arm64_lib" "$(arm64_simulator_ceiling "$deployment_target")" || return 1 + assert_archive_min_os "$sim_x86_64_lib" "$deployment_target" || return 1 local xcframework="$output_dir/offline_protocol_uniffi.xcframework" diff --git a/docs/bridges/swift.md b/docs/bridges/swift.md index 8bbc4378c..3726199b9 100644 --- a/docs/bridges/swift.md +++ b/docs/bridges/swift.md @@ -259,17 +259,22 @@ Three things hold the invariant: - `bindings/react-native/scripts/build-uniffi-ios.sh` reads the target out of the podspec and exports it before it builds. The number is written once, in the podspec. The SDK's own Rust code is compiled for it too. -- After the build, the script reads every object in every slice and refuses to - package one that is too new. An archive it could not read fails the same - way: one with no stamp in it, and one `otool` read part of before giving - up. +- `package_xcframework` reads every object in every slice before it packages + anything, and refuses an archive that holds one too new. An archive it + could not read fails the same way: one with no stamp in it, and one + `otool` read part of before giving up. The check is inside the packaging + function, not beside the call, so the pod's XCFramework cannot be packaged + without it. The Swift package's XCFramework is built by + `scripts/package-swiftpm-xcframework.sh`, which does not check; the + `Swift Package` job runs the check on its archive as a step of its own. - The gate needs `otool`, so in CI it runs where the library is built on a Mac: in a release, on all three archives, and in the `Swift Package` job, on the simulator archive of every pull request. `scripts/tests/test-ios-min-os.sh`, at the repository root, runs on every pull request on Linux. It covers what can go wrong without `otool`: the - parser, the podspec reader, a failing `otool`, and that the build script - still calls the gate on all three archives. + parser, the podspec reader, a failing `otool`, and the packaging function + itself, driven with stand-ins for `otool`, `lipo` and `xcodebuild`: each + archive is held to its own ceiling, and a refused one packages nothing. The podspec has one reader, `ios_deployment_target` in `bindings/react-native/scripts/shared/xcframework.sh`. @@ -281,8 +286,10 @@ the release would have built for a number the package refused. A target directory built for one deployment target keeps its Rust objects when the target changes, because cargo does not rebuild a crate for a new value of the variable. Only the C compiled through `cc-rs` is redone. A -lowered target therefore fails the gate on Rust objects until `cargo clean`, -locally or in a CI cache. +lowered target therefore fails the gate on Rust objects until `cargo clean`. +Both CI caches that hold iOS objects, the `Swift Package` job's and the +release's, add the podspec's hash to their key, so a changed target starts +from a cold cache there. ## Testing diff --git a/scripts/tests/test-ios-min-os.sh b/scripts/tests/test-ios-min-os.sh index ba338ba55..eb41ba60b 100755 --- a/scripts/tests/test-ios-min-os.sh +++ b/scripts/tests/test-ios-min-os.sh @@ -22,7 +22,7 @@ source "$RN_SCRIPTS/shared/xcframework.sh" FAILURES=0 ASSERTIONS=0 # A test that stops asserting passes. Raise this with every check added. -EXPECTED_ASSERTIONS=59 +EXPECTED_ASSERTIONS=69 WORK="$(mktemp -d)" trap 'rm -rf "$WORK"' EXIT @@ -232,17 +232,88 @@ gate "an archive with no stamp" "not otool output" 0 13.0 1 "found no minimum OS # up. The ones it never reached are the ones nothing looked at. gate "an archive otool read part of" "$GOOD" 1 13.0 1 "otool could not read all of" +echo "the packaging" + +# The gate lives inside package_xcframework, so the release cannot package an +# archive it did not check. These drive the function itself, with stand-ins +# for otool (a fixture per archive), lipo and xcodebuild (which log their +# calls), so a gate that is dropped, skipped or given the wrong ceiling fails +# here whatever the build script around it looks like. +cat >"$STUBS/otool" <<'STUB' +#!/usr/bin/env bash +cat "$2.otool" +STUB +cat >"$STUBS/lipo" <<'STUB' +#!/usr/bin/env bash +echo "lipo $*" >>"$PACKAGING_LOG" +while [ "$#" -gt 0 ]; do + if [ "$1" = "-output" ]; then : >"$2"; fi + shift +done +STUB +cat >"$STUBS/xcodebuild" <<'STUB' +#!/usr/bin/env bash +echo "xcodebuild $*" >>"$PACKAGING_LOG" +STUB +chmod +x "$STUBS/otool" "$STUBS/lipo" "$STUBS/xcodebuild" + +# archive : an archive on disk and the otool dump the stand-in +# prints for it. +archive() { + : >"$WORK/$1" + object "$1.o" LC_BUILD_VERSION minos "$2" >"$WORK/$1.otool" +} + +# package +# . The function runs in a subshell because it sets and clears +# an EXIT trap of its own, which would otherwise replace this script's. +package() { + local name="$1" status=0 + archive device.a "$2" + archive sim_arm64.a "$3" + archive sim_x86_64.a "$4" + export PACKAGING_LOG="$WORK/packaging.log" + : >"$PACKAGING_LOG" + rm -rf "$WORK/out" + mkdir -p "$WORK/out" + ( + PATH="$STUBS:$PATH" + package_xcframework "$WORK/out" "$WORK/device.a" "$WORK/sim_arm64.a" \ + "$WORK/sim_x86_64.a" "$5" + ) >"$WORK/package.out" 2>&1 || status=$? + equal "$name: exit status" "$status" "$6" + if [ "$6" != 0 ]; then + equal "$name: nothing is packaged" "$(wc -l <"$PACKAGING_LOG" | tr -d ' ')" "0" + fi +} + +package "every archive at the target" 13.0 14.0 13.0 13.0 0 +equal "one simulator archive is made" "$(grep -c '^lipo -create' "$PACKAGING_LOG")" "1" +equal "one XCFramework is made" "$(grep -c '^xcodebuild -create-xcframework' "$PACKAGING_LOG")" "1" +equal "of two slices" "$(grep -o -- '-library' "$PACKAGING_LOG" | wc -l | tr -d ' ')" "2" + +package "a device archive built for a newer iOS" 26.5 14.0 13.0 13.0 1 +# The arm64 simulator slice is held to 14.0, never higher. +package "an arm64 simulator archive above its floor" 13.0 14.5 13.0 13.0 1 +# The floor is the arm64 simulator's alone: the Intel slice keeps the target. +package "an Intel simulator archive at the arm64 floor" 13.0 14.0 14.0 13.0 1 +package "no deployment target" 13.0 14.0 13.0 "" 1 +if grep -qF "needs the deployment target" "$WORK/package.out"; then + pass "no deployment target: says it needs one" +else + fail "no deployment target: does not say it needs one: $(tr '\n' ' ' <"$WORK/package.out")" +fi + echo "the wiring" -# The gate runs only in a release, so nothing else notices it being removed -# from the script that calls it. +# The build script reads the target, exports it before building, and hands it +# to the packaging, which checks against it. BUILD="$RN_SCRIPTS/build-uniffi-ios.sh" line_of() { grep -nF -- "$1" "$BUILD" | head -1 | cut -d: -f1; } EXPORT_LINE="$(line_of 'export IPHONEOS_DEPLOYMENT_TARGET="$IOS_DEPLOYMENT_TARGET"')" READ_LINE="$(line_of 'IOS_DEPLOYMENT_TARGET="$(ios_deployment_target ')" BUILD_LINE="$(line_of 'cargo build --release --target "$arch"')" -PACKAGE_LINE="$(line_of 'package_xcframework \')" if [ -n "$READ_LINE" ] && [ -n "$EXPORT_LINE" ] && [ "$READ_LINE" -lt "$EXPORT_LINE" ]; then pass "the target is read from the podspec, then exported" @@ -256,21 +327,11 @@ else fail "it is not exported before the build" fi -for arch in aarch64-apple-ios aarch64-apple-ios-sim x86_64-apple-ios; do - GATE_LINE="$(line_of "assert_archive_min_os \"\$(static_lib_for $arch)\"")" - if [ -n "$GATE_LINE" ] && [ -n "$PACKAGE_LINE" ] && - [ "$BUILD_LINE" -lt "$GATE_LINE" ] && [ "$GATE_LINE" -lt "$PACKAGE_LINE" ]; then - pass "$arch is checked after it is built and before it is packaged" - else - fail "$arch is not checked between the build and the packaging" - fi -done - -if grep -A1 -F 'assert_archive_min_os "$(static_lib_for aarch64-apple-ios-sim)"' "$BUILD" | - grep -qF 'arm64_simulator_ceiling "$IOS_DEPLOYMENT_TARGET"'; then - pass "the simulator slice is held to a ceiling worked out from the target" +if awk '/^package_xcframework /{p=1} p{print} p&&!/\\$/{exit}' "$BUILD" | + grep -qF '"$IOS_DEPLOYMENT_TARGET"'; then + pass "the packaging is handed the target it checks against" else - fail "the simulator slice is held to a ceiling that is written down" + fail "the packaging is not handed the target" fi if [ "$ASSERTIONS" != "$EXPECTED_ASSERTIONS" ]; then From 5c4117ba1f39bd58d19360c377c7a4c9ecb25415 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 30 Sep 2026 17:04:14 +0530 Subject: [PATCH 04/12] fix(bindings): say where the Kotlin timestamp parser parts from Instant Four sentences said the parser answers what java.time.Instant answered. It does for every timestamp a relay sends, and not at the edges: Instant accepted hour 24, an empty fraction, lowercase t and z, and a leap second, and this refuses all four. The header, the Swift test comment and the changelog now say so. The pattern matches ASCII digits: on Android \d is ICU's and matches any Unicode digit, which a JVM test cannot see, so a Rust guard pins it. Two tests pin every field's bound, which mutation showed no test did. --- CHANGELOG.md | 8 +++-- .../com/offlineprotocol/RelayTimestamps.kt | 21 ++++++++---- .../offlineprotocol/RelayTimestampsTest.kt | 32 +++++++++++++++++++ .../ios/tests/RelayTimestampsTests.swift | 2 +- crates/offline-protocol-uniffi/src/lib.rs | 20 ++++++++++++ 5 files changed, 73 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd68c6de7..8d48823f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -177,9 +177,11 @@ archived by series under [docs/changelog/](docs/changelog/); see the `NoClassDefFoundError`, which the surrounding `catch (e: Exception)` does not catch, for a legacy relay message and an ISO-8601 last-seen timestamp. An application that enables core library desugaring was not affected. The - timestamp is now parsed without `java.time`, with the answers `Instant` - gave, and a Rust guard refuses a new `java.time` call in the bridge. Not - run on an Android 7 device. + timestamp is now parsed without `java.time`, as RFC 3339, and gives the + answers `Instant` gave for every timestamp a relay sends. At the edges it + is stricter than `Instant`: hour 24, a fraction with no digits, lowercase + `t` or `z` and a leap second are refused. A Rust guard refuses a new + `java.time` call in the bridge. Not run on an Android 7 device. - **An opted-in mesh wake with no wake service stops at once.** A sticky restart that wakes JavaScript started the wake service by name, and `startService` returns null rather than throwing when no such service is diff --git a/bindings/react-native/android/src/main/java/com/offlineprotocol/RelayTimestamps.kt b/bindings/react-native/android/src/main/java/com/offlineprotocol/RelayTimestamps.kt index 3c1f0252b..6083709c9 100644 --- a/bindings/react-native/android/src/main/java/com/offlineprotocol/RelayTimestamps.kt +++ b/bindings/react-native/android/src/main/java/com/offlineprotocol/RelayTimestamps.kt @@ -11,12 +11,17 @@ package com.offlineprotocol * API 24 or 25 the call throws `NoClassDefFoundError`, which is an `Error` * and passes through a `catch (e: Exception)`. Desugaring would have to be * switched on by every application, not by this library, so the parse is - * written out here and behaves the same on every API level. It answers what - * `Instant.parse` answered for every input the relay sends. + * written out here and behaves the same on every API level. * - * Stricter than the Swift twin, as `Instant` was: Foundation's formatter - * rolls 2023-02-29 over into March and accepts 24:00 and a `+0530` offset, - * and this refuses all three. + * It reads RFC 3339 internet date-time and answers what `Instant.parse` + * answered for every timestamp the relay sends. It is not `Instant` at the + * edges. It refuses hour 24 and a `.` with no digits after it, which RFC 3339 + * refuses too, and lowercase `t` or `z` and a leap second `:60`, which RFC + * 3339 allows. `Instant.parse` accepted all four. No relay sends any of them. + * + * Stricter than the Swift twin: Foundation's formatter rolls 2023-02-29 over + * into March and accepts 24:00 and a `+0530` offset, and this refuses all + * three. */ object RelayTimestamps { /** @@ -30,9 +35,13 @@ object RelayTimestamps { * RFC 3339 internet date-time, what the Swift twin's * `ISO8601DateFormatter` accepts with `.withInternetDateTime`: a `T`, an * optional fraction, and `Z` or a numeric offset. + * + * `[0-9]`, never `\d`: on Android `\d` is ICU's and matches every Unicode + * decimal digit, so `٢٠٢٤` would match here and then parse as 2024. The + * JVM the unit tests run on reads `\d` as ASCII and cannot tell. */ private val INTERNET_DATE_TIME = Regex( - """(\d{4})-(\d{2})-(\d{2})T(\d{2}):(\d{2}):(\d{2})(?:\.(\d{1,9}))?(Z|([+-])(\d{2}):(\d{2}))""" + """([0-9]{4})-([0-9]{2})-([0-9]{2})T([0-9]{2}):([0-9]{2}):([0-9]{2})(?:\.([0-9]{1,9}))?(Z|([+-])([0-9]{2}):([0-9]{2}))""" ) fun parseToMsOrNull(timestampStr: String): Long? { diff --git a/bindings/react-native/android/src/test/java/com/offlineprotocol/RelayTimestampsTest.kt b/bindings/react-native/android/src/test/java/com/offlineprotocol/RelayTimestampsTest.kt index 64837c9db..0c5dbf7f0 100644 --- a/bindings/react-native/android/src/test/java/com/offlineprotocol/RelayTimestampsTest.kt +++ b/bindings/react-native/android/src/test/java/com/offlineprotocol/RelayTimestampsTest.kt @@ -88,6 +88,38 @@ class RelayTimestampsTest { assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:00+0530")) } + /** Each field's own bound, one at a time: every other field is valid. */ + @Test + fun refusesAFieldOutOfItsRange() { + assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T00:60:00Z")) + assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:60Z")) + assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:00+24:00")) + assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:00+05:60")) + // The largest value of each is still read. + assertEquals( + 1704153599000L, + RelayTimestamps.parseToMsOrNull("2024-01-01T23:59:59Z") + ) + assertEquals( + 1704067200000L - (23 * 3600L + 59 * 60L) * 1000L, + RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:00+23:59") + ) + } + + /** + * Where this parts from `Instant.parse`, which accepted each of these. RFC 3339 + * refuses hour 24 and an empty fraction, and allows lowercase `t`/`z` and + * a leap second; this refuses all of them, and no relay sends any. + */ + @Test + fun refusesWhatInstantAcceptedAndNoRelaySends() { + assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T24:00:00Z")) + assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:00.Z")) + assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01t00:00:00Z")) + assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:00z")) + assertNull(RelayTimestamps.parseToMsOrNull("2016-12-31T23:59:60Z")) + } + @Test fun epochSecondsAreScaledToMilliseconds() { // ~2024-07 as epoch seconds; without the heuristic this would render diff --git a/bindings/react-native/ios/tests/RelayTimestampsTests.swift b/bindings/react-native/ios/tests/RelayTimestampsTests.swift index 040c40900..331f500e5 100644 --- a/bindings/react-native/ios/tests/RelayTimestampsTests.swift +++ b/bindings/react-native/ios/tests/RelayTimestampsTests.swift @@ -50,7 +50,7 @@ final class RelayTimestampsTests: XCTestCase { // No zone: a local time, which names no instant. The Kotlin twin also // refuses an impossible date, 24:00 and an offset without a colon, which - // Foundation's formatter accepts. That difference predates both suites. + // Foundation's formatter accepts. No relay sends any of the three. func testRefusesATimeWithNoZone() { XCTAssertNil(RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:00")) } diff --git a/crates/offline-protocol-uniffi/src/lib.rs b/crates/offline-protocol-uniffi/src/lib.rs index 4e60c11bd..4d2e153ad 100644 --- a/crates/offline-protocol-uniffi/src/lib.rs +++ b/crates/offline-protocol-uniffi/src/lib.rs @@ -15154,6 +15154,26 @@ mod tests { ); } + /// The Android relay timestamp parser matches ASCII digits only. + /// + /// On Android a regex `\d` is ICU's and matches every Unicode decimal + /// digit, and `toInt()` then reads them as their values, so a timestamp + /// written in Arabic-Indic digits would parse. The unit suite runs on a + /// JVM, where `\d` is ASCII, so no Kotlin test can see the difference. + #[test] + fn android_relay_timestamps_match_ascii_digits_only() { + let code = + rn_source_code_only("android/src/main/java/com/offlineprotocol/RelayTimestamps.kt"); + assert!( + code.contains("[0-9]{4}"), + "RelayTimestamps.kt: expected the date-time pattern spelled with [0-9]" + ); + assert!( + !code.contains("\\d"), + "RelayTimestamps.kt: `\\d` is any Unicode digit on Android; use [0-9]" + ); + } + /// Nothing that blocks on the network shares the thread `stop()` waits on. /// /// A background caller's `runSync` wait is deliberately unbounded — it is From e83f137f34917ec6bfb3e48f0567f2aa683cbac9 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 30 Sep 2026 17:04:21 +0530 Subject: [PATCH 05/12] test(bindings): pin a mesh wake whose service does not start dispatchWake treats a null from startService as a failed wake, and no test could reach that branch: Robolectric's startService never returns null. The start now goes through an internal seam, and a test makes it answer null and checks the keep-alive stops at once. Removing the branch fails it. --- .../offlineprotocol/MeshForegroundService.kt | 11 ++++++++++- .../MeshForegroundServiceTest.kt | 17 +++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/bindings/react-native/android/src/main/java/com/offlineprotocol/MeshForegroundService.kt b/bindings/react-native/android/src/main/java/com/offlineprotocol/MeshForegroundService.kt index 46b56011b..499881398 100644 --- a/bindings/react-native/android/src/main/java/com/offlineprotocol/MeshForegroundService.kt +++ b/bindings/react-native/android/src/main/java/com/offlineprotocol/MeshForegroundService.kt @@ -5,6 +5,7 @@ import android.app.NotificationChannel import android.app.NotificationManager import android.app.PendingIntent import android.app.Service +import android.content.ComponentName import android.content.Context import android.content.Intent import android.content.pm.PackageManager @@ -191,6 +192,14 @@ class MeshForegroundService : Service() { /** The armed watchdog, or null when no wake is outstanding. */ private var wakeWatchdog: Runnable? = null + /** + * How [dispatchWake] starts the wake service: [startService], and a test + * seam only. `startService` answers null when it starts nothing, and + * Robolectric's never does, so the null branch below is reachable from a + * test only through here. + */ + internal var startWakeService: (Intent) -> ComponentName? = { startService(it) } + inner class LocalBinder : Binder() { fun getService(): MeshForegroundService = this@MeshForegroundService } @@ -396,7 +405,7 @@ class MeshForegroundService : Service() { "declared in this application; stopping instead", ) false - } else if (startService(intent) == null) { + } else if (startWakeService(intent) == null) { Log.w(TAG, "The mesh wake service did not start; stopping instead") false } else { diff --git a/bindings/react-native/android/src/test/java/com/offlineprotocol/MeshForegroundServiceTest.kt b/bindings/react-native/android/src/test/java/com/offlineprotocol/MeshForegroundServiceTest.kt index 4b3499024..51fff1919 100644 --- a/bindings/react-native/android/src/test/java/com/offlineprotocol/MeshForegroundServiceTest.kt +++ b/bindings/react-native/android/src/test/java/com/offlineprotocol/MeshForegroundServiceTest.kt @@ -416,6 +416,23 @@ class MeshForegroundServiceTest { assertTrue(shadowOf(service).isStoppedBySelf) } + @Test + fun `a wake service that resolves but does not start stops at once`() { + // The service is declared, so it resolves, and the platform still + // starts nothing: `startService` answers null. Robolectric's never + // does, so the start goes through the seam. + optInToWake() + declareWakeService() + + val service = createService() + service.startWakeService = { null } + val result = service.onStartCommand(null, 0, 1) + + assertEquals(Service.START_NOT_STICKY, result) + assertTrue(shadowOf(service).isForegroundStopped) + assertTrue(shadowOf(service).isStoppedBySelf) + } + @Test fun `the wake watchdog stops the keep-alive when no host arrives`() { optInToWake() From c6d203d42f038d68a5d1e9623d9ef534bb452a53 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 30 Sep 2026 17:04:21 +0530 Subject: [PATCH 06/12] ci(bindings): lint the Android library for calls newer than minSdk The java.time guard reads source text for one package, and java.time on Android 7 was one instance of a class: any API above minSdk compiles, passes every JVM test, and throws NoClassDefFoundError on an older phone. The library build now runs lint's NewApi check alone, failing the build, and the Android Library job runs it. The generated bindings are left out: their one newer call, Cleaner, sits behind a Class.forName. A planted java.time call fails the step. --- .github/workflows/ci.yml | 7 +++++++ .../offline-protocol-android/build.gradle.kts | 13 +++++++++++++ bindings/kotlin/offline-protocol-android/lint.xml | 11 +++++++++++ crates/offline-protocol-uniffi/src/lib.rs | 9 +++++---- 4 files changed, 36 insertions(+), 4 deletions(-) create mode 100644 bindings/kotlin/offline-protocol-android/lint.xml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 44a3bf446..b63bb4f0d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -995,6 +995,13 @@ jobs: :offline-protocol-android:testDebugUnitTest :offline-protocol-android:publishAllPublicationsToStagingRepository + # A call to an API newer than minSdk passes every JVM test and throws on + # an older phone. Lint's NewApi check is the only thing that sees the + # class, and the library build runs it alone (see its lint block). + - name: Lint the library for calls newer than minSdk + working-directory: bindings/kotlin + run: gradle --no-daemon --stacktrace -PVERSION_NAME=0.0.0 :offline-protocol-android:lintDebug + # Gradle succeeds on a test task that found no test. The sources are # read from another directory, so that is one wrong path away. - name: Check that the suite ran diff --git a/bindings/kotlin/offline-protocol-android/build.gradle.kts b/bindings/kotlin/offline-protocol-android/build.gradle.kts index 9a1490d27..552b32b93 100644 --- a/bindings/kotlin/offline-protocol-android/build.gradle.kts +++ b/bindings/kotlin/offline-protocol-android/build.gradle.kts @@ -117,6 +117,19 @@ android { withSourcesJar() } } + + // A call to an API newer than minSdk compiles, passes every unit test on + // the JVM, and throws NoClassDefFoundError on an older phone, which a + // `catch (e: Exception)` does not catch. java.time on Android 7 was one. + // Lint's NewApi check is the class of that defect, so it runs alone and + // fails the build: the other checks report findings this library has not + // decided on. lint.xml leaves out the generated bindings, which guard + // their one such call behind a Class.forName. + lint { + checkOnly += "NewApi" + abortOnError = true + lintConfig = file("lint.xml") + } } // A source set is a list of directories and cannot leave a file out, so the diff --git a/bindings/kotlin/offline-protocol-android/lint.xml b/bindings/kotlin/offline-protocol-android/lint.xml new file mode 100644 index 000000000..058567493 --- /dev/null +++ b/bindings/kotlin/offline-protocol-android/lint.xml @@ -0,0 +1,11 @@ + + + + + + + diff --git a/crates/offline-protocol-uniffi/src/lib.rs b/crates/offline-protocol-uniffi/src/lib.rs index 4d2e153ad..5d88ee187 100644 --- a/crates/offline-protocol-uniffi/src/lib.rs +++ b/crates/offline-protocol-uniffi/src/lib.rs @@ -15135,11 +15135,12 @@ mod tests { /// /// `minSdk` is 24 and `java.time` exists from API 26. On API 24 or 25 a /// call throws `NoClassDefFoundError`, an `Error`, which passes through - /// the `catch (e: Exception)` these call sites are written with. Nothing - /// else would say so: the unit suite runs on a JVM that has `java.time`, - /// the build does not run Android lint, and desugaring would be every + /// the `catch (e: Exception)` these call sites are written with. The unit + /// suite runs on a JVM that has `java.time`, and desugaring would be every /// application's switch to throw, not this library's. `RelayTimestamps` - /// parses ISO-8601 by hand for exactly this reason. + /// parses ISO-8601 by hand for exactly this reason. The Android Library + /// CI job runs lint's NewApi check, which covers every newer API; this + /// guard keeps the one that already shipped from coming back without it. #[test] fn android_bridge_sources_never_call_java_time() { let callers: Vec = rn_android_kotlin_sources() From ca0a994c038ba2bad3c02d7318d4f147d7a5d3c5 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 30 Sep 2026 17:05:31 +0530 Subject: [PATCH 07/12] test(leaf): pin the pointer guard's tokenizer, and name what it cannot see Four tokenizer behaviours had no pin and survived mutation: string escapes, nested block comments, raw identifiers and raw byte strings. Each now has a case that fails when it is removed. The module doc names the three routes a one-file token scan cannot follow, a module that re-exports alloc, a glob over one, and a path a macro assembles, where it had said the guard refuses any path. --- crates/offline-protocol-leaf/src/shared.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/crates/offline-protocol-leaf/src/shared.rs b/crates/offline-protocol-leaf/src/shared.rs index be13d8444..98706fd27 100644 --- a/crates/offline-protocol-leaf/src/shared.rs +++ b/crates/offline-protocol-leaf/src/shared.rs @@ -18,6 +18,12 @@ //! glob over `alloc`, and any rename of `alloc`. The `embedded-core` CI job //! refuses a direct import on the targets. //! +//! The scan reads one file's tokens and cannot see three routes: an import +//! through a module of this crate that re-exports `alloc`, a glob over such a +//! module, and a path a macro assembles. None of them is written in this +//! crate, and a new one would be, visibly, a module that re-exports `alloc` +//! or a macro that pastes a path together. +//! //! # Building a handle //! //! [`shared_store`] is the portable way. The replacement pointer cannot turn @@ -388,6 +394,10 @@ mod tests { "use crate::alloc as heap;", "extern crate alloc as heap;", "pub(crate) use alloc as mem;", + // A raw identifier is the same name. + "use alloc::r#sync::Arc;", + // A raw byte string ends at its quote and hashes, not the first quote. + "const B: &[u8] = br#\"a \" b\"#; use alloc::sync::Arc;", ] { assert!(!counted_pointer_paths(code).is_empty(), "missed: {code}"); } @@ -406,6 +416,10 @@ mod tests { "mod sync { pub struct Arc; } use self::sync::Arc;", "extern crate alloc; extern crate alloc as _;", "let c = '\\''; let d = '\\\\'; let e = b'\\\\';", + // An escaped quote does not end a string. + "const S: &str = \"\\\" use alloc::sync::Arc; \\\"\";", + // A block comment ends at its own close, not the first `*/`. + "/* a /* b */ use alloc::sync::Arc; */", ] { assert_eq!( counted_pointer_paths(code), From 1a5646b83ce19b6da623536775aa4cba781f5857 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 30 Sep 2026 17:08:59 +0530 Subject: [PATCH 08/12] fix(protocol): tell an unreadable sealed record from one that does not open open_record folded a read error into 'does not authenticate', so a permission or disk error was logged as a record sealed under another key, and the lost-check probe counted it towards WrongStoreKey: the right key over one unreadable record was refused as a wrong one. The probe now stops with an I/O error naming the record, and the listing warns with the real cause. The listing's warn-once memo also forgets a path when the file there is written or removed, so a record damaged again is reported again. Five mutants are each caught. --- CHANGELOG.md | 6 +- crates/offline-protocol/src/file_store/mls.rs | 329 ++++++++++++++++-- 2 files changed, 299 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d48823f3..7f40d5343 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,8 +70,10 @@ archived by series under [docs/changelog/](docs/changelog/); see the record proves the key; a damaged check over an empty store fails with `FileStoreError::KeyCheckUnverifiable`, naming the file to remove. The sealed store never deletes a record on a read; a damaged one is reported - as `CorruptedData` naming its file, and one the listing skips is logged, - once per store, as a warning naming its file. Each store holds an + as `CorruptedData` naming its file, and one the listing skips is logged + as a warning naming its file, once until that file is written or removed. + A record that cannot be read is reported as a read failure, never as one + sealed under another key, and never counts towards refusing the store key. Each store holds an exclusive lock on its directory while open, so a second engine over the same directories fails with `FileStoreError::InUse` instead of silently diverging the MLS state. `close()` on either store releases its directory diff --git a/crates/offline-protocol/src/file_store/mls.rs b/crates/offline-protocol/src/file_store/mls.rs index 63d792bb2..6522144f6 100644 --- a/crates/offline-protocol/src/file_store/mls.rs +++ b/crates/offline-protocol/src/file_store/mls.rs @@ -122,6 +122,19 @@ enum Probe { NoneOpened, } +/// What one record file turned out to be. +/// +/// A file that could not be read is not a file that did not open. Folding +/// the two together sent a permission or disk error to the operator as "does +/// not open under this store key", and counted it towards refusing the key. +enum Opened { + Record(String, String), + /// Removed between the listing and the read. + Absent, + /// Read, and does not authenticate under this key and name. + DoesNotOpen, +} + impl SealedFileMlsStorage { /// Opens (creating if needed) the store at `//`, sealed /// under the key `provider` supplies. @@ -346,16 +359,40 @@ impl SealedFileMlsStorage { .map(Zeroizing::new) } - /// Opens one record file into its `(key_type, key_id)`, or `None` when it - /// does not authenticate under this key and name. The value is not + /// Opens one record file into its `(key_type, key_id)`. An error is a + /// read that failed, never a record that does not open. The value is not /// copied: the only plaintext is the zeroizing buffer, dropped here. - fn open_record(&self, path: &Path, name: &str) -> Option<(String, String)> { - let BoundedRead::Bytes(raw) = records::read_bounded(path, MAX_SEALED_BYTES).ok()? else { - return None; + fn open_record(&self, path: &Path, name: &str) -> std::io::Result { + let raw = match records::read_bounded(path, MAX_SEALED_BYTES)? { + BoundedRead::Bytes(raw) => raw, + BoundedRead::Absent => return Ok(Opened::Absent), + BoundedRead::Oversized => return Ok(Opened::DoesNotOpen), }; - let plain = self.open_sealed(name, &raw)?; - let header = records::parse_header(&plain)?; - Some((header.key_type, header.key_id)) + let opened = self + .open_sealed(name, &raw) + .and_then(|plain| records::parse_header(&plain)) + .map_or(Opened::DoesNotOpen, |header| { + Opened::Record(header.key_type, header.key_id) + }); + Ok(opened) + } + + /// Whether this is the first listing to warn about `path` since the file + /// there was last written or removed. + fn first_listing_warning(&self, path: &Path) -> bool { + self.listing_warned + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .insert(path.to_path_buf()) + } + + /// Forgets that a listing warned about `path`, once the file there has + /// been written or removed: a record damaged again later is news. + fn forget_listing_warning(&self, path: &Path) { + self.listing_warned + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .remove(path); } /// Proves the store key against the check file, minting the file on a @@ -397,7 +434,7 @@ impl SealedFileMlsStorage { // path. A check that does not open with no records to prove anything // stays a refusal; there is nothing sealed to lose, and the operator // can remove the check file to start the store afresh. - match self.probe_existing_records().map_err(io)? { + match self.probe_existing_records()? { Probe::Opened => { if check_present { tracing::warn!( @@ -422,11 +459,23 @@ impl SealedFileMlsStorage { records::write_atomic(&self.directory, &path, &sealed).map_err(io) } - fn probe_existing_records(&self) -> std::io::Result { + /// Tries up to [`KEY_CHECK_PROBE_LIMIT`] records against the store key. + /// + /// A record that cannot be read says nothing about the key, so it is + /// skipped rather than counted against it, and a readable record later in + /// the walk can still prove the key. Only when none opens is a read + /// failure the answer, naming the record: refusing the key as wrong over + /// a permission error would send the operator after the wrong fault. + fn probe_existing_records(&self) -> Result { + let directory_error = |source| FileStoreError::Io { + path: self.directory.clone(), + source, + }; let mut probe = Probe::NoRecords; let mut tried = 0usize; - for type_entry in std::fs::read_dir(&self.directory)? { - let type_entry = type_entry?; + let mut unreadable: Option = None; + for type_entry in std::fs::read_dir(&self.directory).map_err(directory_error)? { + let type_entry = type_entry.map_err(directory_error)?; let is_type_dir = type_entry .file_name() .to_str() @@ -435,27 +484,48 @@ impl SealedFileMlsStorage { if !is_type_dir { continue; } + let type_directory = type_entry.path(); records::for_each_entry( - &type_entry.path(), + &type_directory, ENTRY_PREFIX, KEY_CHECK_PROBE_LIMIT, |path, name| { if matches!(probe, Probe::Opened) || tried >= KEY_CHECK_PROBE_LIMIT { return; } - tried += 1; - probe = if self.open_record(path, name).is_some() { - Probe::Opened - } else { - Probe::NoneOpened - }; + match self.open_record(path, name) { + Ok(Opened::Record(..)) => { + tried += 1; + probe = Probe::Opened; + } + Ok(Opened::DoesNotOpen) => { + tried += 1; + probe = Probe::NoneOpened; + } + // Removed meanwhile: not a record. + Ok(Opened::Absent) => {} + Err(source) => { + unreadable.get_or_insert(FileStoreError::Io { + path: path.to_path_buf(), + source, + }); + } + } }, - )?; + ) + .map_err(|source| FileStoreError::Io { + path: type_directory.clone(), + source, + })?; if matches!(probe, Probe::Opened) || tried >= KEY_CHECK_PROBE_LIMIT { break; } } - Ok(probe) + match (probe, unreadable) { + (Probe::Opened, _) => Ok(Probe::Opened), + (_, Some(err)) => Err(err), + (probe, None) => Ok(probe), + } } } @@ -495,8 +565,11 @@ impl MlsStorage for SealedFileMlsStorage { records::private_mkdir(&directory) .map_err(|err| StorageError::StoreFailed(err.to_string()))?; self.swept.sweep_once(&directory); - records::write_atomic(&directory, &directory.join(&name), &sealed) - .map_err(|err| StorageError::StoreFailed(err.to_string())) + let path = directory.join(&name); + records::write_atomic(&directory, &path, &sealed) + .map_err(|err| StorageError::StoreFailed(err.to_string()))?; + self.forget_listing_warning(&path); + Ok(()) } fn load(&self, key_type: &str, key_id: &str) -> StorageResult>> { @@ -537,9 +610,10 @@ impl MlsStorage for SealedFileMlsStorage { .lock .open() .ok_or_else(|| StorageError::DeleteFailed(self.closed()))?; - records::remove(&self.entry_path(key_type, key_id)) - .map(|_| ()) - .map_err(|err| StorageError::DeleteFailed(err.to_string())) + let path = self.entry_path(key_type, key_id); + records::remove(&path).map_err(|err| StorageError::DeleteFailed(err.to_string()))?; + self.forget_listing_warning(&path); + Ok(()) } fn list_keys(&self, key_type: &str) -> StorageResult> { @@ -553,21 +627,30 @@ impl MlsStorage for SealedFileMlsStorage { ENTRY_PREFIX, records::MAX_LISTED_ENTRIES, |path, name| match self.open_record(path, name) { - Some((record_type, record_id)) if record_type == key_type => { + Ok(Opened::Record(record_type, record_id)) if record_type == key_type => { keys.insert(record_id); } + // Deleted since the directory was read: not a record. + Ok(Opened::Absent) => {} + // Left out of the listing, and named with the real cause. A + // listing that failed outright on one unreadable file would + // stop an engine from finding any of its groups. + Err(err) => { + if self.first_listing_warning(path) { + tracing::warn!( + path = %path.display(), + error = %err, + "sealed MLS record could not be read; left out of the listing" + ); + } + } // The listing is how the engine finds its groups, so a record // skipped here is one the engine never learns it had. Same // level and same path as `load` reports: names are keyed // digests, and the path is the only way to find the file. // Once per file per store: listings are frequent. - _ => { - let first = self - .listing_warned - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .insert(path.to_path_buf()); - if first { + Ok(Opened::Record(..) | Opened::DoesNotOpen) => { + if self.first_listing_warning(path) { tracing::warn!( path = %path.display(), "sealed MLS record does not open under this store key; \ @@ -956,6 +1039,184 @@ mod tests { .contains(&damaged)); } + /// A record written or removed after a listing warned about it is a new + /// file: damaged again later, it is warned about again. + #[test] + fn a_record_rewritten_after_a_warning_is_warned_about_again() { + let root = TempRoot::new("mls-list-rewarn"); + let store = open(&root, 1); + store.store("group_state", "a", b"one").expect("store"); + let damaged = store.entry_path("group_state", "a"); + let damage = || { + let mut bytes = std::fs::read(&damaged).expect("read"); + bytes[SEALED_MAGIC.len() + NONCE_BYTES] ^= 1; + std::fs::write(&damaged, bytes).expect("write"); + }; + damage(); + + let _callsite = listing_warning_callsite(); + let warnings = Warnings::default(); + let _default = tracing::dispatcher::set_default(&tracing::Dispatch::new(warnings.clone())); + tracing::callsite::rebuild_interest_cache(); + let path = damaged.display().to_string(); + let named = || { + warnings + .0 + .lock() + .expect("warnings") + .iter() + .filter(|w| w.contains(&path)) + .count() + }; + + assert!(store.list_keys("group_state").expect("list").is_empty()); + assert_eq!(named(), 1); + + store.store("group_state", "a", b"again").expect("store"); + damage(); + assert!(store.list_keys("group_state").expect("list").is_empty()); + assert_eq!( + named(), + 2, + "a rewritten record damaged again was not reported" + ); + + store.delete("group_state", "a").expect("delete"); + assert!( + !store + .listing_warned + .lock() + .expect("memo") + .contains(&damaged), + "a removed record's warning is still remembered" + ); + } + + /// Makes `path` unreadable, or answers false where the test runs with + /// the power to read it anyway. + #[cfg(unix)] + fn make_unreadable(path: &Path) -> bool { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o000)).expect("chmod"); + std::fs::read(path).is_err() + } + + #[cfg(unix)] + fn make_readable(path: &Path) { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)).expect("chmod"); + } + + /// A record that cannot be read is not evidence against the store key. + /// Counted as one that does not open, a permission error on the only + /// record refused the right key as a wrong one. + #[cfg(unix)] + #[test] + fn an_unreadable_record_is_an_io_error_not_a_wrong_key() { + let root = TempRoot::new("mls-unreadable-probe"); + let (check, record) = { + let store = open(&root, 1); + store.store("group_state", "a", b"one").expect("store"); + ( + store.directory().join(KEY_CHECK_FILE), + store.entry_path("group_state", "a"), + ) + }; + std::fs::remove_file(&check).expect("remove check"); + if !make_unreadable(&record) { + return; + } + let outcome = SealedFileMlsStorage::open(root.path(), &namespace(), &key(1)); + make_readable(&record); + match outcome { + Err(FileStoreError::Io { path, .. }) => { + assert_eq!(path, record, "the error does not name the record"); + } + Err(other) => panic!("expected an I/O error naming the record, got {other}"), + Ok(_) => panic!("an unreadable record proved the key"), + } + assert!( + !check.exists(), + "no key check was minted on an unread record" + ); + open(&root, 1); + assert!(check.exists(), "once readable, the record proves the key"); + } + + /// An unreadable record does not stand in the way of a readable one that + /// proves the key, whichever the directory lists first. + /// + /// File names are keyed digests, so the order the probe meets records in + /// is not the test's to choose. Four of five are unreadable, so a probe + /// that stopped at the first unreadable record fails here on most runs. + #[cfg(unix)] + #[test] + fn an_unreadable_record_does_not_hide_one_that_proves_the_key() { + let root = TempRoot::new("mls-unreadable-beside-good"); + let ids = ["a", "b", "c", "d", "e"]; + let (check, unreadable) = { + let store = open(&root, 1); + for id in ids { + store.store("group_state", id, b"value").expect("store"); + } + ( + store.directory().join(KEY_CHECK_FILE), + ids[1..] + .iter() + .map(|id| store.entry_path("group_state", id)) + .collect::>(), + ) + }; + std::fs::remove_file(&check).expect("remove check"); + if !unreadable.iter().all(|path| make_unreadable(path)) { + unreadable.iter().for_each(|path| make_readable(path)); + return; + } + let outcome = SealedFileMlsStorage::open(root.path(), &namespace(), &key(1)); + unreadable.iter().for_each(|path| make_readable(path)); + outcome.expect("a readable record proves the key"); + assert!(check.exists(), "the key check is rebuilt"); + } + + /// The listing names a read failure as one, not as a record sealed under + /// another key, and still returns the records it could read. + #[cfg(unix)] + #[test] + fn an_unreadable_record_is_listed_as_unreadable() { + let root = TempRoot::new("mls-unreadable-list"); + let store = open(&root, 1); + store.store("group_state", "a", b"one").expect("store"); + store.store("group_state", "b", b"two").expect("store"); + let record = store.entry_path("group_state", "a"); + if !make_unreadable(&record) { + return; + } + + let _callsite = listing_warning_callsite(); + let warnings = Warnings::default(); + let _default = tracing::dispatcher::set_default(&tracing::Dispatch::new(warnings.clone())); + tracing::callsite::rebuild_interest_cache(); + + let listed = store.list_keys("group_state"); + make_readable(&record); + assert_eq!(listed.expect("list"), vec!["b"]); + let path = record.display().to_string(); + let said: Vec = warnings + .0 + .lock() + .expect("warnings") + .iter() + .filter(|w| w.contains(&path)) + .cloned() + .collect(); + assert_eq!(said.len(), 1, "{said:?}"); + assert!( + said[0].contains("could not be read") && !said[0].contains("store key"), + "the warning blames the key for a read failure: {}", + said[0] + ); + } + /// Nothing to write down in a record's name, so the error must carry the /// path: file names are keyed digests, and an operator cannot find the /// damaged file any other way. From f9e93838e6b79fbc7a0ca18359f5a973ae5cb340 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 30 Sep 2026 17:10:10 +0530 Subject: [PATCH 09/12] test(protocol): open the file stores over a relative root in its own binary A one-component relative root once failed its first open, because the parent of 'keys' is the empty path and flushing it is 'not found'. flush_target fixes that, and its unit test pins the function, but bypassing it at its call site passed every test: each store test opens an absolute temporary root. This opens a pair over 'keys' and 'state' under a temporary working directory, in its own test binary because it changes the process's current directory. --- .../tests/file_store_relative_root.rs | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 crates/offline-protocol/tests/file_store_relative_root.rs diff --git a/crates/offline-protocol/tests/file_store_relative_root.rs b/crates/offline-protocol/tests/file_store_relative_root.rs new file mode 100644 index 000000000..aeb3d30c4 --- /dev/null +++ b/crates/offline-protocol/tests/file_store_relative_root.rs @@ -0,0 +1,45 @@ +//! The built-in file stores open over a relative root on the first try. +//! +//! A store creates its directories and flushes each one's parent so the new +//! entry survives a crash. For a one-component relative root such as `keys`, +//! `Path::parent` is the empty path, and flushing that fails with "not +//! found": the first open failed and the second, finding the directory +//! there, succeeded. Every unit test opens an absolute temporary root, so +//! none of them reaches it. +//! +//! This is its own test binary because it changes the process's working +//! directory, which every other test in a shared binary would see. + +#![cfg(feature = "file-store")] + +use offline_protocol::file_store::{account_storage_namespace, FileStorePair, StaticStoreKey}; + +#[test] +fn a_pair_opens_over_one_component_relative_roots_on_the_first_try() { + let base = std::env::temp_dir().join(format!( + "offline-protocol-relative-root-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|elapsed| elapsed.as_nanos()) + .unwrap_or(0) + )); + std::fs::create_dir_all(&base).expect("create the working directory"); + let previous = std::env::current_dir().expect("current directory"); + std::env::set_current_dir(&base).expect("enter the working directory"); + + let namespace = account_storage_namespace("com.example.relative", "alice"); + let outcome = FileStorePair::open("keys", "state", &namespace, &StaticStoreKey::new([7; 32])); + let created = (base.join("keys").is_dir(), base.join("state").is_dir()); + + std::env::set_current_dir(&previous).expect("leave the working directory"); + let pair = outcome.expect("the first open over relative roots"); + pair.close(); + let _ = std::fs::remove_dir_all(&base); + + assert_eq!( + created, + (true, true), + "both roots are made under the working directory" + ); +} From 502af57b95df1a31bde24c7c4673f3d6f437dc9e Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 30 Sep 2026 17:13:33 +0530 Subject: [PATCH 10/12] fix(protocol): let a sealed record that opens outrank a pairing-id mismatch A pairing-id mismatch refused the state root outright, even when its sealed records opened under this MLS store's record key, which only this identity's records do. Lose the MLS pairing file, rebind beside a fresh state root, put the first one back, and the same identity was refused. A mismatch now asks the records, and only one that opens admits the root. Two tests pin branches that survived mutation: a state store with no record beside another bound MLS store, and a state root made again beside a bound one. Three sentences said the MLS store takes the pairing id first; the state store does, which is the crash-safe order, and the docs now say so. --- CHANGELOG.md | 5 +- .../src/offline_protocol.udl | 18 +- .../offline-protocol/src/file_store/pair.rs | 154 ++++++++++++++++-- docs/mls-integration.md | 10 +- 4 files changed, 159 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f40d5343..f6975c896 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -122,8 +122,9 @@ archived by series under [docs/changelog/](docs/changelog/); see the failed nor a first write that failed can disarm it); a state root that holds records and belongs with another MLS store (an identity is in place, but not the one that wrote the state), decided by the pairing ids - the two stores' first open wrote, and for a state root never bound by - whether its sealed records open under this MLS store's record key; an + the two stores' first open wrote, and for a state root never bound, or + bound to another id, by whether its sealed records open under this MLS + store's record key, which only this identity's do; an account directory that cannot be read, which is never taken for empty; and roots that are one directory or one inside the other, compared as spelled before anything is created diff --git a/crates/offline-protocol-uniffi/src/offline_protocol.udl b/crates/offline-protocol-uniffi/src/offline_protocol.udl index 8117af5d8..85ea4d22f 100644 --- a/crates/offline-protocol-uniffi/src/offline_protocol.udl +++ b/crates/offline-protocol-uniffi/src/offline_protocol.udl @@ -1618,19 +1618,21 @@ interface OfflineProtocol { // (the message names the path), a wrong key, or a key check that cannot // prove the key. After both opened: protocol state that belongs with // another MLS store. The first open of a pair binds the two with one - // pairing id, and later opens compare the ids; when either store holds - // no id, the state root is asked whether its sealed records open under - // this MLS store's record key. That is another identity's state, and the engine deletes what its - // restore reads of it, queued and parked messages among them. A refusal + // pairing id, and later opens compare the ids. When either store holds + // no id, or the ids differ, the state root is asked whether its sealed + // records open under this MLS store's record key, and one that opens + // admits it. State that belongs with another MLS store is refused + // because the engine deletes what its restore reads of it, queued and + // parked messages among them. A refusal // after the MLS store opened leaves its directory, lock file and key // check, and no record, so the first refusal above still applies to a // retry. // // A damaged record key is not refused, on the launch that finds it or - // on any launch after while the MLS root keeps its pairing file. Nothing opens the state sealed under it again, so - // a refusal would save no record, and the engine's new key and its - // settlement of that state as failed is how the application learns - // which messages were lost. What that launch cannot delete stays sealed + // on any launch after while the MLS root keeps its pairing file. + // Nothing opens the state sealed under it again, so a refusal would save + // no record, and the engine's new key and its settlement of that state + // as failed is how the application learns which messages were lost. What that launch cannot delete stays sealed // under the lost key, and the pairing ids, not those records, say the // state is still this identity's. // diff --git a/crates/offline-protocol/src/file_store/pair.rs b/crates/offline-protocol/src/file_store/pair.rs index 958c2179d..10aed8f59 100644 --- a/crates/offline-protocol/src/file_store/pair.rs +++ b/crates/offline-protocol/src/file_store/pair.rs @@ -8,17 +8,25 @@ //! merely somewhere else. //! //! Ownership is recorded, not inferred. The first open of a pair writes one -//! random pairing id into both stores, the MLS store first. A later open -//! compares the two ids, and nothing the engine does to its record key -//! changes them. Asking the sealed records instead -//! ([`FileProtocolStateStorage::sealed_state`]) cannot tell another -//! identity's state from this identity's state under a record key the engine -//! had to regenerate: records sealed under the lost key outlive the launch -//! that regenerated it (documents are read lazily, and each restore walk -//! stops at its delete allowance), and the next launch finds records that do -//! not open under a usable key. That reading refused the same identity over -//! its own roots. It remains only as the fallback for a state store that was -//! never bound. +//! random pairing id into both stores, the protocol-state store first (see +//! [`bind`] for why that order). A later open compares the two ids, and +//! nothing the engine does to its record key changes them. +//! +//! The records alone ([`FileProtocolStateStorage::sealed_state`]) cannot +//! tell another identity's state from this identity's state under a record +//! key the engine had to regenerate: records sealed under the lost key +//! outlive the launch that regenerated it (documents are read lazily, and +//! each restore walk stops at its delete allowance), and the next launch +//! finds records that do not open under a usable key. That reading refused +//! the same identity over its own roots, so it decides only where the ids +//! cannot: a store that holds no readable id, and ids that differ. +//! +//! Where the ids differ, only a sealed record that opens under this MLS +//! store's record key admits the state, since only this identity's records +//! open under it. That is the MLS store that lost its pairing file and was +//! bound again beside another state root. The state root then takes the +//! MLS store's id, so two state roots can carry one id; each still belongs +//! to that identity. use std::path::{Component, Path, PathBuf}; use std::sync::Arc; @@ -164,8 +172,11 @@ impl FileStorePair { /// Whether the state store belongs with the MLS store. /// /// Decided by the pairing ids when both stores hold one: equal ids belong -/// together, and a state store bound to another id belongs with another MLS -/// store. When the MLS store holds no readable id (never bound, restored from +/// together. A state store bound to another id belongs with another MLS +/// store unless one of its sealed records opens under this MLS store's +/// record key, which only this identity's records do: an MLS store that lost +/// its pairing file and was bound again beside another state root still +/// owns the state root it started with. When the MLS store holds no readable id (never bound, restored from /// a backup older than its binding, the file removed or damaged), or the /// state store holds none, the sealed records decide: another identity's /// records still open under no key this store holds. @@ -183,7 +194,15 @@ fn ownership( ) -> Result { match (state.pairing()?, pairing) { (Some(bound), StoredPairing::Present(ours)) if bound == *ours => Ok(SealedState::Opens), - (Some(_), StoredPairing::Present(_)) => Ok(SealedState::Foreign), + // A record that opens is proof no id can overrule. Anything short of + // it stays another MLS store's: records that do not open, a damaged + // record key, and unsealed records alone, which prove nothing. + (Some(_), StoredPairing::Present(_)) => Ok(match state.sealed_state(secure)? { + SealedState::Opens => SealedState::Opens, + SealedState::Empty | SealedState::Foreign | SealedState::KeyDamaged => { + SealedState::Foreign + } + }), (Some(_), StoredPairing::Absent | StoredPairing::Damaged) | (None, _) => { state.sealed_state(secure) } @@ -393,7 +412,7 @@ mod tests { } } - /// The first open binds both stores to one id, the MLS store first, and + /// The first open binds both stores to one id, the state store first, and /// neither pairing file is a record: the probes that decide whether a new /// identity may start still see an empty pair. #[test] @@ -554,6 +573,111 @@ mod tests { open(&roots).expect("the next open admits the pair and binds it"); } + /// Gives the MLS store a record key and seals one record under it in the + /// state store, as the engine does. + fn seal_one_record(pair: &FileStorePair, key: [u8; 32]) { + use crate::protocol::storage_keys::{STATE_RECORD_KEY, STATE_RECORD_KEY_ID}; + pair.secure + .store(STATE_RECORD_KEY, STATE_RECORD_KEY_ID, &key) + .expect("a record key"); + let key_type = crate::protocol::sealed_state_key_types() + .next() + .expect("a sealed category"); + let sealed = crate::protocol::state_crypto::StateRecordCipher::new(&key) + .seal(key_type, "parked", b"ours") + .expect("seal"); + pair.state + .store(key_type, "parked", &sealed) + .expect("a sealed record"); + } + + /// An id mismatch is not the last word when a record opens. The MLS + /// store lost its pairing file and was bound again beside a fresh state + /// root; its first state root, put back, holds records only this + /// identity's record key opens, and it is admitted and bound again. + #[test] + fn a_state_root_whose_records_open_is_admitted_over_an_id_mismatch() { + let ours = roots("pair-mismatch-opens"); + let pair = open(&ours).expect("open"); + pair.secure + .store("identity", "self", b"ours") + .expect("an identity"); + seal_one_record(&pair, [5; 32]); + let pairing_file = pair.secure.directory().join(PAIRING_FILE); + pair.close(); + std::fs::remove_file(&pairing_file).expect("lose the MLS pairing file"); + + let elsewhere = TempRoot::new("pair-mismatch-elsewhere"); + let rebound = FileStorePair::open( + ours.keys.path(), + elsewhere.path(), + &namespace(), + &StaticStoreKey::new([4; 32]), + ) + .expect("bound beside a fresh state root"); + let StoredPairing::Present(new_id) = rebound.secure.pairing().unwrap() else { + panic!("bound again"); + }; + rebound.close(); + assert_ne!( + FileProtocolStateStorage::open(ours.state.path(), &namespace()) + .and_then(|state| { + let id = state.pairing(); + state.close(); + id + }) + .expect("the first state root"), + Some(new_id), + "the first state root still carries the old id" + ); + + let pair = open(&ours).expect("its own records open under its record key"); + assert_eq!(pair.state.pairing().unwrap(), Some(new_id), "bound again"); + } + + /// A state store with no record is admitted beside an MLS store bound to + /// another id, and takes that id: the ids alone would call it another + /// MLS store's, and it has nothing to lose to anyone. + #[test] + fn a_state_store_with_no_record_is_rebound_beside_another_bound_mls_store() { + let theirs = roots("pair-empty-bound-theirs"); + open(&theirs).expect("their pair").close(); + let ours = roots("pair-empty-bound-ours"); + open(&ours).expect("our pair").close(); + + let pair = FileStorePair::open( + theirs.keys.path(), + ours.state.path(), + &namespace(), + &StaticStoreKey::new([4; 32]), + ) + .expect("no record to lose"); + let StoredPairing::Present(id) = pair.secure.pairing().unwrap() else { + panic!("bound"); + }; + assert_eq!(pair.state.pairing().unwrap(), Some(id)); + } + + /// A reinstall: the state root is gone and made again beside an MLS root + /// that is still bound. The new state store takes the MLS store's id, or + /// every later open would judge the pair by its records alone. + #[test] + fn a_state_root_made_again_takes_the_bound_mls_id() { + let roots = roots("pair-reinstall"); + let pair = open(&roots).expect("open"); + pair.secure + .store("identity", "self", b"ours") + .expect("an identity"); + let StoredPairing::Present(id) = pair.secure.pairing().unwrap() else { + panic!("bound"); + }; + pair.close(); + std::fs::remove_dir_all(roots.state.path()).expect("remove the state root"); + + let pair = open(&roots).expect("reopen over a new state root"); + assert_eq!(pair.state.pairing().unwrap(), Some(id)); + } + /// A pairing file that no longer opens names no state store. The pair /// is judged by its records, and bound again once it passes. #[test] diff --git a/docs/mls-integration.md b/docs/mls-integration.md index 1b2ca0053..ae8c5241f 100644 --- a/docs/mls-integration.md +++ b/docs/mls-integration.md @@ -428,9 +428,13 @@ and binds the two so a later open knows they belong together (see a store sealed under another identity's record key. `FileStorePair` records which MLS store a state store belongs with: its first open writes one random pairing id into both (`mls-store.pair`, sealed under the store - key, and `state-store.pair`, in the clear), the MLS store first, and a - later open compares them. Neither file is a record, and a binding's - provider never reads them. + key, and `state-store.pair`, in the clear), the state store first, so a + crash between the two writes leaves the MLS store with no id rather than + two stores on different ones. A later open compares them. Where they + differ, a sealed record that opens under this MLS store's record key + still admits the state root, since only this identity's records open under + it; nothing less does. Neither file is a record, and a binding's provider + never reads them. - **The records alone cannot settle ownership.** `FileProtocolStateStorage::sealed_state` asks the sealed records whether they open under the MLS store's record key, without changing either From 36e9963154219b860d2bbb864e2f3ce04bb4630e Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 30 Sep 2026 17:17:54 +0530 Subject: [PATCH 11/12] fix(protocol,mls): check key-type isolation before the state suite deletes The protocol-state conformance suite, the one exposed over the FFI and run against real providers, cleaned up by listing a probe key type and deleting what it listed before any check ran. On a backend that merges key types that listing names real records, and the cleanup deleted them; the UDL called the suite safe against a live store. It now checks isolation first, with point writes and deletes, and stops there on a failure, as the MLS suite already did. Both suites also write a probe type that ends like another: the SDK's key types are related by suffix and the probes only by prefix, so a backend listing by suffix was green on both. The count and names of the checks are unchanged. --- CHANGELOG.md | 9 ++ .../src/storage_conformance.rs | 44 ++++++- .../src/offline_protocol.udl | 5 +- .../src/protocol/tests/data_layer.rs | 97 ++++++++++++++ .../src/storage_conformance.rs | 120 ++++++++++++++---- docs/bridges/README.md | 11 +- 6 files changed, 256 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f6975c896..bfb8a09aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -174,6 +174,15 @@ archived by series under [docs/changelog/](docs/changelog/); see the ### Fixed +- **The storage conformance suite no longer deletes a merging backend's + records.** `runStorageConformance` cleaned up its probe records by listing + a probe key type and deleting what it listed, before any check had run. + On a backend that merges key types, or lists them by the tail of the name, + that listing names real records too, and the cleanup deleted them. The + suite now checks key-type isolation first, with point writes and point + deletes only, and a failure ends the run there with that one check + reported. A run that passes it still has thirteen checks, under the same + names. - **Relay timestamps parse on Android 7.** The Android bridge parsed them with `java.time.Instant`, which exists from Android 8 (API 26), while the SDK supports Android 7 (API 24). On Android 7 the call threw diff --git a/crates/offline-protocol-mls/src/storage_conformance.rs b/crates/offline-protocol-mls/src/storage_conformance.rs index fa0754247..a1e16963d 100644 --- a/crates/offline-protocol-mls/src/storage_conformance.rs +++ b/crates/offline-protocol-mls/src/storage_conformance.rs @@ -49,6 +49,14 @@ pub const CONFORMANCE_KEY_TYPE_OTHER: &str = "mls_storage_conformance_probe_othe /// untouched category is empty and not an error. const CONFORMANCE_KEY_TYPE_UNUSED: &str = "mls_storage_conformance_probe_unused"; +/// A key type that ends with [`CONFORMANCE_KEY_TYPE`], the way the SDK's +/// own key types are related: `key_package` ends `contact_key_package` and +/// `openmls_key_package`, and `group_state` ends `openmls_group_state`. The +/// two probe types above are related by prefix only, so a backend that lists +/// by suffix passed every check and then listed every OpenMLS record as a +/// group. The isolation check writes its listing probe here too. +const CONFORMANCE_KEY_TYPE_SUFFIXED: &str = "suffixed_mls_storage_conformance_probe"; + /// The id the isolation check writes under both probe key types. Nothing /// else uses it, so a point write or delete of it cannot touch a real /// record even on a backend that ignores the key type. @@ -532,6 +540,9 @@ fn key_types_are_isolated(storage: &dyn MlsStorage) -> Result<(), String> { .and_then(|()| { storage.store(CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_LISTING_PROBE_ID, b"x") }) + .and_then(|()| { + storage.store(CONFORMANCE_KEY_TYPE_SUFFIXED, ISOLATION_LISTING_PROBE_ID, b"x") + }) .map_err(|err| { format!("store failed, so isolation could not be checked and nothing was deleted: {err}") })?; @@ -555,6 +566,7 @@ fn key_types_are_isolated(storage: &dyn MlsStorage) -> Result<(), String> { (CONFORMANCE_KEY_TYPE, ISOLATION_PROBE_ID), (CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_PROBE_ID), (CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_LISTING_PROBE_ID), + (CONFORMANCE_KEY_TYPE_SUFFIXED, ISOLATION_LISTING_PROBE_ID), ] { let _ = storage.delete(key_type, key_id); } @@ -736,6 +748,7 @@ mod tests { AbsentIsAnError, DeleteOfAbsentFails, IgnoresKeyType, + ListsBySuffix, ListReturnsFirstPageOnly, ListOfUnknownTypeFails, KeepsIdUpToFirstColon, @@ -852,7 +865,12 @@ mod tests { .lock() .unwrap() .keys() - .filter(|(kt, _)| *kt == key_type) + .filter(|(kt, _)| match self.fault { + // Matching the tail of the name, which the SDK's own + // key types share (see CONFORMANCE_KEY_TYPE_SUFFIXED). + Fault::ListsBySuffix => kt.ends_with(key_type.as_str()), + _ => *kt == key_type, + }) .map(|(_, id)| id.clone()) .collect(); match self.fault { @@ -899,6 +917,7 @@ mod tests { // nothing, so each check is shown failing the defect it exists for. let cases = [ (Fault::IgnoresKeyType, "key_types_are_separate_namespaces"), + (Fault::ListsBySuffix, "key_types_are_separate_namespaces"), (Fault::CorruptsValues, "store_then_load"), (Fault::StringTypedValues, "binary_values_round_trip"), (Fault::EmptyReadsAsMissing, "empty_value_is_not_missing"), @@ -964,6 +983,29 @@ mod tests { .expect("the check failed"); assert!(failure.detail.contains("KeyNotFound"), "{}", failure.detail); } + #[test] + fn a_backend_that_lists_by_suffix_stops_the_run_before_any_delete() { + // Listing one key type by the tail of its name reaches every type + // that ends the same way. The run stops at the isolation check, and + // a real record in such a type is untouched. + let storage = Faulty::new(Fault::ListsBySuffix); + storage + .store("prefixed_mls_storage_conformance_probe", "real", b"kept") + .unwrap(); + let report = run(&storage); + assert!(report.passed.is_empty(), "{:?}", report.passed); + assert_eq!( + report.failures[0].check, + "key_types_are_separate_namespaces" + ); + assert_eq!( + storage + .load("prefixed_mls_storage_conformance_probe", "real") + .unwrap(), + Some(b"kept".to_vec()) + ); + } + #[test] fn a_backend_that_merges_key_types_stops_the_run_before_any_delete() { // The isolation check runs first because every listed delete after diff --git a/crates/offline-protocol-uniffi/src/offline_protocol.udl b/crates/offline-protocol-uniffi/src/offline_protocol.udl index 85ea4d22f..eaa701a3e 100644 --- a/crates/offline-protocol-uniffi/src/offline_protocol.udl +++ b/crates/offline-protocol-uniffi/src/offline_protocol.udl @@ -61,7 +61,10 @@ namespace offline_protocol { // — swappability that cannot be verified is a promise, not a property. // // The suite writes only under its own probe key types and deletes - // everything it wrote, so it is safe against a live store. + // everything it wrote. Key-type isolation is checked first, with point + // writes and deletes, and a failure ends the run before anything is + // listed and deleted, so a backend that merges key types keeps its real + // records. Probe a new backend on a scratch instance until it is green. string run_storage_conformance(ProtocolStateStorageProvider storage); }; diff --git a/crates/offline-protocol/src/protocol/tests/data_layer.rs b/crates/offline-protocol/src/protocol/tests/data_layer.rs index f3a3dd035..bffd06653 100644 --- a/crates/offline-protocol/src/protocol/tests/data_layer.rs +++ b/crates/offline-protocol/src/protocol/tests/data_layer.rs @@ -743,6 +743,103 @@ fn the_conformance_suite_catches_a_broken_backend() { ); } +/// A backend whose key types bleed into each other, one of two ways. +#[derive(Clone, Copy)] +enum KeyTypeFault { + /// Every key type is one namespace. + Merged, + /// A listing matches the tail of the key type's name. + ListsBySuffix, +} + +struct KeyTypeFaultStorage { + fault: KeyTypeFault, + records: Mutex>>, +} + +impl KeyTypeFaultStorage { + fn new(fault: KeyTypeFault) -> Self { + Self { + fault, + records: Mutex::default(), + } + } + + fn key(&self, key_type: &str, key_id: &str) -> (String, String) { + let key_type = match self.fault { + KeyTypeFault::Merged => "", + KeyTypeFault::ListsBySuffix => key_type, + }; + (key_type.to_string(), key_id.to_string()) + } +} + +impl ProtocolStateStorage for KeyTypeFaultStorage { + fn store(&self, key_type: &str, key_id: &str, data: &[u8]) -> ProtocolStateResult<()> { + let key = self.key(key_type, key_id); + self.records + .lock() + .expect("lock") + .insert(key, data.to_vec()); + Ok(()) + } + fn load(&self, key_type: &str, key_id: &str) -> ProtocolStateResult>> { + let key = self.key(key_type, key_id); + Ok(self.records.lock().expect("lock").get(&key).cloned()) + } + fn delete(&self, key_type: &str, key_id: &str) -> ProtocolStateResult<()> { + let key = self.key(key_type, key_id); + self.records.lock().expect("lock").remove(&key); + Ok(()) + } + fn list_keys(&self, key_type: &str) -> ProtocolStateResult> { + let (wanted, _) = self.key(key_type, ""); + Ok(self + .records + .lock() + .expect("lock") + .keys() + .filter(|(kt, _)| match self.fault { + KeyTypeFault::Merged => *kt == wanted, + KeyTypeFault::ListsBySuffix => kt.ends_with(wanted.as_str()), + }) + .map(|(_, id)| id.clone()) + .collect()) + } +} + +/// The suite is exposed over the FFI and run against real providers, so a +/// backend that merges key types must not lose a record to it: listing a +/// probe type names every record there, and the cleanup deletes what it +/// lists. The isolation check runs first and ends the run before any +/// listed delete. The same for a backend that lists by the tail of a name, +/// which a real category ending like a probe type would meet. +#[test] +fn the_conformance_suite_stops_before_deleting_from_a_backend_whose_key_types_bleed() { + for (fault, real_type) in [ + (KeyTypeFault::Merged, "outbox"), + ( + KeyTypeFault::ListsBySuffix, + "prefixed_storage_conformance_probe", + ), + ] { + let storage = KeyTypeFaultStorage::new(fault); + storage.store(real_type, "parked", b"kept").expect("seed"); + let report = crate::storage_conformance::run(&storage); + assert!(report.passed.is_empty(), "{:?}", report.passed); + assert_eq!(report.failures.len(), 1, "{}", report.summary()); + assert_eq!( + report.failures[0].check, + "key_types_are_separate_namespaces" + ); + assert_eq!( + storage.load(real_type, "parked").expect("load"), + Some(b"kept".to_vec()), + "{real_type}: the suite deleted a real record" + ); + } +} + #[test] fn the_mls_storage_conformance_suite_is_reachable_from_the_engine() { // The engine re-exports the MLS suite beside the state suite, so a Rust diff --git a/crates/offline-protocol/src/storage_conformance.rs b/crates/offline-protocol/src/storage_conformance.rs index c1d94a87d..b1b03048e 100644 --- a/crates/offline-protocol/src/storage_conformance.rs +++ b/crates/offline-protocol/src/storage_conformance.rs @@ -12,7 +12,16 @@ //! The suite drives the trait directly rather than going through the //! sealing chokepoint, because what is under test is the adapter's contract: //! bytes in, the same bytes out, addressed by `(key_type, key_id)`. It -//! writes only under its own key type, and deletes everything it wrote. +//! writes only under its own key types, and deletes everything it wrote. +//! +//! Key-type isolation is checked first, with point writes and point deletes +//! of ids nothing else uses, and a failure ends the run there. Every listed +//! delete after it trusts that listing a probe key type names only probe +//! records; on a backend that merges key types it names real records too, +//! and the cleanup would delete them. That makes the suite safe to run +//! against a live store whatever the backend does with key types. It is not +//! a promise for every defect: probe a new backend on a scratch instance +//! until it is green. use crate::protocol_state_storage::{ ProtocolStateError, ProtocolStateStorage, MAX_PROTOCOL_STATE_RECORD_TRANSFER_BYTES, @@ -27,6 +36,20 @@ pub const CONFORMANCE_KEY_TYPE: &str = "storage_conformance_probe"; /// A second key type, used to prove categories do not bleed into each other. pub const CONFORMANCE_KEY_TYPE_OTHER: &str = "storage_conformance_probe_other"; +/// A key type that ends with [`CONFORMANCE_KEY_TYPE`]. The second probe type +/// is related to the first by prefix only, and a backend that lists by the +/// tail of a name is caught only by one related by suffix. +const CONFORMANCE_KEY_TYPE_SUFFIXED: &str = "suffixed_storage_conformance_probe"; + +/// The id the isolation check writes under two probe key types. Nothing else +/// uses it, so a point write or delete of it cannot reach a real record even +/// on a backend that ignores the key type. +const ISOLATION_PROBE_ID: &str = "storage_conformance_isolation"; + +/// The id the isolation check writes under the other probe key types only, +/// to catch a listing that crosses key types. +const ISOLATION_LISTING_PROBE_ID: &str = "storage_conformance_isolation_listing"; + /// The report types are the MLS suite's, re-exported, so the two suites /// render one JSON shape and one reader serves both. pub use offline_protocol_mls::storage_conformance::{ConformanceFailure, ConformanceReport}; @@ -56,18 +79,31 @@ fn expect_load( pub fn run(storage: &dyn ProtocolStateStorage) -> ConformanceReport { let mut report = ConformanceReport::new(); + // 1. Key types are separate namespaces, checked before anything is + // deleted. The data layer keeps documents, their delta logs and space + // indexes in three of them; a backend that merges them would serve a + // delta where a document was asked for. It is also the one defect that + // turns this suite destructive (see the module doc), so a failure ends + // the run here. + let isolation = key_types_are_isolated(storage); + let isolated = isolation.is_ok(); + report.check("key_types_are_separate_namespaces", isolation); + if !isolated { + return report; + } + // Anything left from an interrupted earlier run would make the listing // checks report failures that are not the adapter's fault. cleanup(storage); - // 1. The basic contract. + // 2. The basic contract. let outcome = storage .store(CONFORMANCE_KEY_TYPE, "basic", b"hello") .map_err(|err| format!("store failed: {err}")) .and_then(|()| expect_load(storage, "basic", b"hello")); report.check("store_then_load", outcome); - // 2. Values are bytes, not text. A backend that round-trips through a + // 3. Values are bytes, not text. A backend that round-trips through a // string type mangles these, and the failure is invisible until a // sealed record (which is ciphertext) comes back wrong. let binary: Vec = (0u8..=255).collect(); @@ -77,14 +113,14 @@ pub fn run(storage: &dyn ProtocolStateStorage) -> ConformanceReport { .and_then(|()| expect_load(storage, "binary", &binary)); report.check("binary_values_round_trip", outcome); - // 3. An empty value is a value, and must not read back as absent. + // 4. An empty value is a value, and must not read back as absent. let outcome = storage .store(CONFORMANCE_KEY_TYPE, "empty", b"") .map_err(|err| format!("store failed: {err}")) .and_then(|()| expect_load(storage, "empty", b"")); report.check("empty_value_is_not_missing", outcome); - // 4. A second store replaces; it does not append or refuse. + // 5. A second store replaces; it does not append or refuse. let outcome = storage .store(CONFORMANCE_KEY_TYPE, "overwrite", b"first") .and_then(|()| storage.store(CONFORMANCE_KEY_TYPE, "overwrite", b"second")) @@ -92,7 +128,7 @@ pub fn run(storage: &dyn ProtocolStateStorage) -> ConformanceReport { .and_then(|()| expect_load(storage, "overwrite", b"second")); report.check("store_overwrites", outcome); - // 5. A key that was never written reads as missing, not as an error. + // 6. A key that was never written reads as missing, not as an error. let outcome = match storage.load(CONFORMANCE_KEY_TYPE, "never_written") { Ok(None) => Ok(()), Ok(Some(_)) => Err("a key that was never written returned a value".to_string()), @@ -103,7 +139,7 @@ pub fn run(storage: &dyn ProtocolStateStorage) -> ConformanceReport { }; report.check("absent_key_loads_as_none", outcome); - // 6. Delete removes, and deleting again is not an error. The data layer + // 7. Delete removes, and deleting again is not an error. The data layer // deletes folded delta records after a crash may already have taken // them, so a delete that fails on an absent key turns a clean recovery // into a spurious failure. @@ -124,25 +160,12 @@ pub fn run(storage: &dyn ProtocolStateStorage) -> ConformanceReport { ); report.check("delete_removes_and_is_idempotent", outcome); - // 7. Key types are separate namespaces. The data layer keeps documents, - // their delta logs and space indexes in three of them; a backend that - // merges them would serve a delta where a document was asked for. - let outcome = storage - .store(CONFORMANCE_KEY_TYPE, "shared_id", b"mine") - .and_then(|()| storage.store(CONFORMANCE_KEY_TYPE_OTHER, "shared_id", b"theirs")) - .map_err(|err| format!("store failed: {err}")) - .and_then(|()| expect_load(storage, "shared_id", b"mine")) - .and_then( - |()| match storage.load(CONFORMANCE_KEY_TYPE_OTHER, "shared_id") { - Ok(Some(value)) if value == b"theirs" => Ok(()), - Ok(_) => Err("the same key id in two key types collided".to_string()), - Err(err) => Err(format!("load failed: {err}")), - }, - ); - report.check("key_types_are_separate_namespaces", outcome); - // 8. Listing names what is there, and only within its own key type. let outcome = (|| -> std::result::Result<(), String> { + storage + .store(CONFORMANCE_KEY_TYPE, "shared_id", b"mine") + .and_then(|()| storage.store(CONFORMANCE_KEY_TYPE_OTHER, "shared_id", b"theirs")) + .map_err(|err| format!("store failed: {err}"))?; let keys = storage .list_keys(CONFORMANCE_KEY_TYPE) .map_err(|err| format!("list_keys failed: {err}"))?; @@ -240,6 +263,55 @@ pub fn run(storage: &dyn ProtocolStateStorage) -> ConformanceReport { report } +/// The isolation check: the same id under two key types, and an id under +/// two others that must not appear in the first one's listing. +/// +/// Only a collision fails it. A value that comes back wrong in another way +/// is the round-trip checks' to report, and a listing that fails is the +/// listing checks'; neither is evidence that a listed delete would reach +/// outside the probe key types. The probe records are removed with point +/// deletes whatever the outcome. +fn key_types_are_isolated(storage: &dyn ProtocolStateStorage) -> std::result::Result<(), String> { + let outcome = (|| -> std::result::Result<(), String> { + storage + .store(CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_PROBE_ID, b"theirs") + .and_then(|()| storage.store(CONFORMANCE_KEY_TYPE, ISOLATION_PROBE_ID, b"mine")) + .and_then(|()| { + storage.store(CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_LISTING_PROBE_ID, b"x") + }) + .and_then(|()| { + storage.store(CONFORMANCE_KEY_TYPE_SUFFIXED, ISOLATION_LISTING_PROBE_ID, b"x") + }) + .map_err(|err| { + format!("store failed, so isolation could not be checked and nothing was deleted: {err}") + })?; + if let Ok(Some(other)) = storage.load(CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_PROBE_ID) { + if other == b"mine" { + return Err( + "the same key id in two key types collided: a write under one \ + replaced the other" + .to_string(), + ); + } + } + if let Ok(listed) = storage.list_keys(CONFORMANCE_KEY_TYPE) { + if listed.iter().any(|key| key == ISOLATION_LISTING_PROBE_ID) { + return Err("list_keys named a record written under another key type".to_string()); + } + } + Ok(()) + })(); + for (key_type, key_id) in [ + (CONFORMANCE_KEY_TYPE, ISOLATION_PROBE_ID), + (CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_PROBE_ID), + (CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_LISTING_PROBE_ID), + (CONFORMANCE_KEY_TYPE_SUFFIXED, ISOLATION_LISTING_PROBE_ID), + ] { + let _ = storage.delete(key_type, key_id); + } + outcome +} + /// Remove every record the suite writes, in both key types. fn cleanup(storage: &dyn ProtocolStateStorage) { for key_type in [CONFORMANCE_KEY_TYPE, CONFORMANCE_KEY_TYPE_OTHER] { diff --git a/docs/bridges/README.md b/docs/bridges/README.md index c0518f994..da19a73c5 100644 --- a/docs/bridges/README.md +++ b/docs/bridges/README.md @@ -411,10 +411,13 @@ binary and empty values, overwrite semantics, delete idempotence, key-type isolation, listing accuracy, composed and long key ids, large records, and delete completeness. Each check exists because that defect is invisible until data is missing. The suite writes only under its own probe key types -and deletes everything it wrote, which keeps a live store intact only if the -backend keeps key types apart: on one that merges them, listing a probe key -type names real records too, and the suite's cleanup deletes what it lists. -Probe a new backend on a scratch instance until it is green. +and deletes everything it wrote. That keeps a live store intact only if the +backend keeps key types apart, since on one that merges them, listing a +probe key type names real records too. So isolation is checked first, with +point writes and point deletes of ids nothing else uses, including a probe +type that ends like another (the SDK's own key types are related that way), +and a failure ends the run before anything is listed and deleted. Probe a +new backend on a scratch instance until it is green all the same. Green on either suite is not a persistence test. Every check runs against one open instance, so a backend that forgets everything on reopen, or that From 84fbbe04faeae2f842953fe33f9593facd7bdd77 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 30 Sep 2026 17:20:54 +0530 Subject: [PATCH 12/12] fix(bindings): two orderings in the peer-stream managers' stop and regroup iOS stop() ended every link while the session was still set, so a .connected already queued passed the session check and created a link in the emptied table. start() never clears it, and a remote that kept its MCPeerID met a stale refused link after a restart: no preamble, a refusal at its deadline, one wasted round. The session is now cleared first. Android never reset the redial delay on a new group, so a new group's first failed dial waited out the old group's backoff; a new connection after a disconnect now resets it. Both are pinned by a source guard, since neither manager runs in CI. --- .../com/offlineprotocol/WifiDirectManager.kt | 13 +++++++ .../react-native/ios/WifiDirectManager.swift | 14 +++++-- crates/offline-protocol-uniffi/src/lib.rs | 37 +++++++++++++++++++ 3 files changed, 61 insertions(+), 3 deletions(-) diff --git a/bindings/react-native/android/src/main/java/com/offlineprotocol/WifiDirectManager.kt b/bindings/react-native/android/src/main/java/com/offlineprotocol/WifiDirectManager.kt index 84f03eb1d..2226a82de 100644 --- a/bindings/react-native/android/src/main/java/com/offlineprotocol/WifiDirectManager.kt +++ b/bindings/react-native/android/src/main/java/com/offlineprotocol/WifiDirectManager.kt @@ -584,8 +584,21 @@ class WifiDirectManager( if (connected && hasRequiredPermissions()) { wifiP2pManager?.requestConnectionInfo(channel) { info -> info?.let { + val previousOwner = groupOwnerAddress isGroupOwner = it.isGroupOwner groupOwnerAddress = it.groupOwnerAddress?.hostAddress + // A new connection starts its redial ladder from the + // bottom. The delay doubles on each failed dial and was + // reset only by a proved stream or stop(), so a new group's + // first failed dial waited out the old group's backoff. + // Group owners on Android nearly always take the same + // address, so this compares against the null the + // disconnect branch below leaves, not one owner with + // another: it resets on every connection after a + // disconnect, the same group rejoined included. + if (groupOwnerAddress != previousOwner) { + reconnectDelayMs.set(RECONNECT_INITIAL_DELAY_MS) + } emitDiagnostic("info", "Connection info", mapOf( "isGroupOwner" to isGroupOwner, diff --git a/bindings/react-native/ios/WifiDirectManager.swift b/bindings/react-native/ios/WifiDirectManager.swift index c49fb90fe..afb9b7176 100644 --- a/bindings/react-native/ios/WifiDirectManager.swift +++ b/bindings/react-native/ios/WifiDirectManager.swift @@ -274,6 +274,16 @@ public class WifiDirectManager: NSObject, TransportManager { advertiser?.stopAdvertisingPeer() advertiser = nil + // The session is forgotten first. The state-change and data callbacks + // check `self.session === session` on linkQueue, so from here a .connected + // that was already queued finds nothing to attach to. Cleared after + // endAll(), such a callback created a link in the just-emptied table, + // start() never clears it, and a remote that kept its MCPeerID met a + // stale refused link after a restart: no preamble, a refusal at its + // deadline, and one wasted round before the disconnect cleared it. + let old = session + session = nil + // Report every proved peer lost while the core still holds its link, // on linkQueue so no delivery lands after a loss report. The session's // own .notConnected callbacks then find nothing left to report. @@ -281,9 +291,7 @@ public class WifiDirectManager: NSObject, TransportManager { peers.endAll() } - // Disconnect session - session?.disconnect() - session = nil + old?.disconnect() // Notify protocol try? protocolInstance.wifiDirectStatusChanged(isConnected: false) diff --git a/crates/offline-protocol-uniffi/src/lib.rs b/crates/offline-protocol-uniffi/src/lib.rs index 5d88ee187..e0b5d276a 100644 --- a/crates/offline-protocol-uniffi/src/lib.rs +++ b/crates/offline-protocol-uniffi/src/lib.rs @@ -16691,6 +16691,43 @@ mod tests { ); } + /// Two orderings in the peer-stream managers that no test can run. + /// + /// iOS `stop()` forgets the session before it ends every link. The + /// state-change and data callbacks check `self.session === session` on + /// the link queue, so a `.connected` already queued then finds nothing; forgotten after + /// `endAll()`, it created a link in the emptied table that `start()` + /// never clears, and a remote that kept its MCPeerID met a stale refused + /// link on the next session. + /// + /// Android resets the redial delay on a new connection after a + /// disconnect (group owners nearly always share one address, so the + /// comparison is with the null the disconnect leaves). The delay doubles + /// per failed dial, so without the reset a new group's first failed dial + /// waited out the previous group's backoff. + #[test] + fn react_native_peer_stream_managers_forget_the_session_first_and_reset_on_a_new_group() { + let swift = rn_source_code_only("ios/WifiDirectManager.swift"); + assert!( + swift.contains( + "let old = session session = nil onLinkQueueSync { peers.endAll() } \ + old?.disconnect()" + ), + "ios/WifiDirectManager.swift: stop() must clear the session before endAll()" + ); + let kotlin = + rn_source_code_only("android/src/main/java/com/offlineprotocol/WifiDirectManager.kt"); + assert!( + kotlin.contains( + "val previousOwner = groupOwnerAddress isGroupOwner = it.isGroupOwner \ + groupOwnerAddress = it.groupOwnerAddress?.hostAddress \ + if (groupOwnerAddress != previousOwner) { \ + reconnectDelayMs.set(RECONNECT_INITIAL_DELAY_MS) }" + ), + "android/.../WifiDirectManager.kt: a new connection must reset the redial delay" + ); + } + /// Wi-Fi Direct hands the core only an address a preamble proved. /// /// Both managers used to pass a transport-level string, a TCP endpoint on