name: sms # Phase 0 of the SMS remediation plan: make the SMS stack verifiable. # # Before this file existed, `crates/robius-sms` (2,019 LOC) and # `crates/apps/nigig-sms` (5,706 LOC) had NO CI of any kind and two tests, # both of which assert derived trait impls and neither of which mentions # SMS. That is 7,725 lines of untested code whose job is to spend the # user's money by sending real, billable, irreversible messages. # # Enforces: # 1. Both crates compile on the host AND on aarch64-linux-android. # The host build only ever compiles sys/linux.rs -- a stub whose # every function returns PermanentlyUnavailable. ALL of the real # logic (~600 lines of JNI in sys/android/) is behind # #[cfg(target_os = "android")] and is invisible to a host build. # A host-only gate would be close to worthless here. # 2. Clippy stays clean for code these two crates OWN. Warnings from # path dependencies (nigig-core, nigig-uikit, matrix_client) are # pre-existing and out of scope for this workflow; they are # filtered by package id rather than muted, so they still show in # the log and a future workflow can gate them. # 3. Tests pass. # 4. Two defect classes that are invisible in review and have already # shipped here, as source-scanning gates. # # `cargo fmt --check` is deliberately not a gate, matching the reasoning # already recorded in doc-engine.yml: these crates predate the pinned # toolchain's rustfmt style and reformatting them wholesale would # conflict with the remediation work. A step that always fails gets # ignored, which is worse than no step. on: push: paths: - 'crates/robius-sms/**' - 'crates/apps/nigig-sms/**' - 'Cargo.lock' - 'Cargo.toml' - 'rust-toolchain.toml' - '.forgejo/workflows/sms.yml' pull_request: paths: - 'crates/robius-sms/**' - 'crates/apps/nigig-sms/**' - 'Cargo.lock' - 'Cargo.toml' - 'rust-toolchain.toml' - '.forgejo/workflows/sms.yml' jobs: # --------------------------------------------------------------------- # Source-scanning gates. These need no toolchain, so they run first and # fail fast. # --------------------------------------------------------------------- gates: runs-on: ubuntu-latest timeout-minutes: 10 steps: - uses: actions/checkout@v4 # A poisoned mutex turns every later `.lock().unwrap()` into a # panic. The contact cache in conversations_list.rs is written from # a spawned worker thread, so one panic anywhere under the lock # permanently bricks contact resolution for the process lifetime, # and it surfaces far from the cause. # # The codebase already knows the right pattern -- companies_list.rs # uses `if let Ok(mut s) = ..lock()` -- it is just applied # inconsistently. This keeps the good pattern from regressing. # # A6 is FIXED: all 25 call sites now go through # LockRecover::lock_recover() (sms_frame/lock_ext.rs), which # recovers the guard via PoisonError::into_inner() instead of # panicking. This was a ratchet at 19 during Phase 0; it is now a # hard gate, because production code is at zero and any new # occurrence is a regression. # # lock_ext.rs itself is excluded: it documents the old pattern in # comments and its tests deliberately poison a mutex, which # requires a real .lock().unwrap() to observe the Err. - name: No .lock().unwrap() in the SMS crates run: | set -euo pipefail if grep -rn --include='*.rs' '\.lock()\s*\.unwrap()' \ crates/robius-sms/src crates/apps/nigig-sms/src \ --exclude=lock_ext.rs \ | sed 's/^[^:]*:[0-9]*://' \ | grep -vE '^[[:space:]]*(//|/\*|\*)'; then echo echo "ERROR: the lock acquisition(s) above panic on a poisoned" echo "mutex. One panic while the lock is held bricks the cache" echo "for the rest of the process, and the panic surfaces far" echo "from the cause. Use:" echo " use crate::chats::sms_frame::lock_ext::LockRecover;" echo " MUTEX.lock_recover()" exit 1 fi echo "OK" # `&s[..n]` where n is a byte offset panics with "byte index N is # not a char boundary" the moment n lands inside a multi-byte # character. truncate_preview() in sms_utils.rs did exactly this # against a 120-BYTE constant, and it runs per visible row per # frame -- so one inbound SMS containing emoji, Swahili or Arabic # text panicked the whole conversation list on every frame. That is # a remote denial of service triggerable by anyone who knows the # victim's number. # # Slicing must go through char_indices()/char_boundary logic. # # E3: scheduled message bodies must never be stored in the clear. # # Pending schedules have to outlive the process so SmsAlarmReceiver # can send them when the alarm fires, so recipient and body go to # SharedPreferences. MODE_PRIVATE is UID-scoped but not encrypted, # and the file is swept into cloud backup by default # (THREAT_MODEL.md T-I4). They are enveloped with an AndroidKeyStore # key; writing them raw again would be silent. - name: Scheduled SMS payloads must be encrypted run: | set -euo pipefail sched=crates/robius-sms/src/sys/android/schedule.rs if grep -qE 'put_string\(env, &editor, &\(prefix\.clone\(\) \+ "(recipient|body)"\), &request\.' "$sched"; then echo "ERROR: schedule_sms writes recipient/body without encrypting." echo "Route them through encrypt_value() (SmsScheduleCrypto)." echo "See THREAT_MODEL.md T-I4." exit 1 fi for fn in encrypt_value decrypt_value; do grep -q "fn $fn" "$sched" || { echo "ERROR: $fn missing from $sched"; exit 1; } done echo "OK" # E7: the tracked send path must attach real PendingIntents. # # Both intents used to be null, so nothing could report back and # `Ok` was indistinguishable from delivered. If make_result_intent # disappears, or send_with_intents stops taking them, the UI is # lying again and no test would notice. - name: Tracked sends must carry send/delivery intents run: | set -euo pipefail grep -q 'fn make_result_intent' \ crates/robius-sms/src/sys/android/send_status.rs || { echo "ERROR: make_result_intent missing (E7)"; exit 1; } grep -q 'JValueGen::Object(sent_intent)' \ crates/robius-sms/src/sys/android/send.rs || { echo "ERROR: single-part send no longer passes sentIntent (E7)" exit 1; } grep -q 'JValueGen::Object(&sent_list)' \ crates/robius-sms/src/sys/android/send.rs || { echo "ERROR: multipart send no longer passes sentIntents (E7)" exit 1; } echo "OK" # E1: SMS message bodies must never be persisted. # # SMS carries OTPs and banking codes. THREAT_MODEL.md classifies # raw SMS text as a sensitive asset and records T-I2 -- raw SMS # persisted to disk -- as fixed in Phase 0 for the payment crate. # The offline store then re-introduced exactly that defect at # larger scale: every inbox body, plaintext JSON, uncapped. # # OfflineSmsMessage.body is #[serde(skip)]. Removing that attribute # silently starts writing plaintext bodies again, and nothing else # would fail. - name: SMS bodies must not be serialised to disk run: | set -euo pipefail store=crates/nigig-core/src/persistence/offline_store.rs if ! grep -B1 'pub body: String,' "$store" | grep -q 'serde(skip)'; then echo "ERROR: OfflineSmsMessage.body has lost its #[serde(skip)]." echo echo "That writes every inbox message body to" echo "app_data_dir/offline_store/sms_messages.json in plaintext." echo "See THREAT_MODEL.md T-I2b. The device SMS provider is the" echo "system of record; the cache only needs metadata." exit 1 fi echo "OK" # D1/D2: draw_walk must not perform blocking platform I/O. # # fetch_from_device() used to call robius_sms::list_messages() # -- a blocking cross-process ContentProvider query over Binder -- # straight from draw_walk, on a 5-second wall-clock timer that # re-armed itself via redraw(). The app never idled, and each cycle # also rewrote the whole offline JSON store and deep-cloned every # conversation. It ran whether or not the SMS tab was visible. # # The read now happens on a worker thread and is applied through a # results queue. This gate stops the direct call coming back. - name: No blocking SMS provider calls in draw_walk run: | set -euo pipefail if awk ' /fn draw_walk/ { in_draw = 1 } in_draw && /robius_sms::(list_messages|list_threads|list_thread_messages|send_sms|send_bulk_sms)\(/ { print FILENAME ":" FNR ": " $0 found = 1 } in_draw && /^ }$/ { in_draw = 0 } END { exit(found ? 0 : 1) } ' $(find crates/apps/nigig-sms/src -name '*.rs'); then echo echo "ERROR: the call(s) above run blocking platform I/O inside" echo "draw_walk. list_messages() is a Binder round trip to the" echo "SMS ContentProvider; on a populated inbox that is a" echo "multi-hundred-millisecond stall on the render thread." echo "Spawn a worker and deliver the result via SignalToUI," echo "as fetch_from_device/drain_pending_sms_fetch do." exit 1 fi echo "OK" # A3 is FIXED. truncate_preview() now counts characters and takes # its offsets from char_indices()/rfind(), which cannot land # mid-character. # # The two slices it still contains are proven-safe, and a textual # grep cannot tell a proven-safe offset from an arbitrary one -- # so this step is no longer the real guarantee. That job now # belongs to `#![deny(clippy::string_slice)]` at the top of # sms_utils.rs plus the ten unit tests in that module, including a # sweep that walks the cut offset through every possible position # inside a multi-byte character. # # This gate is kept, pinned at those two known-safe sites, purely # so a THIRD slice cannot appear in the module without someone # deliberately editing this number. # # The pattern deliberately does NOT require a leading `&`. The # first version of this gate did, and it missed # if let Some(pos) = trimmed[..end].rfind(..) # one line above the `&trimmed[..end]` it did catch -- an # autoref'd slice panics identically, and being the earlier of the # two it is the one that actually fired first. A gate that matches # only the second of two adjacent instances of the same bug is # worse than none, because it reports a number that looks audited. - name: No new byte-offset string slicing in SMS text helpers run: | set -euo pipefail BASELINE=2 PATTERN='[A-Za-z_][A-Za-z0-9_]*\[(\.\.=?[A-Za-z0-9_]+|[A-Za-z0-9_]+\.\.)[A-Za-z0-9_]*\]' count=$(grep -rnE --include='*.rs' "$PATTERN" \ crates/apps/nigig-sms/src \ | sed 's/^[^:]*:[0-9]*://' \ | grep -vcE '^[[:space:]]*(//|/\*|\*)' || true) echo "found $count, baseline $BASELINE" if [ "$count" -gt "$BASELINE" ]; then grep -rnE --include='*.rs' "$PATTERN" \ crates/apps/nigig-sms/src || true echo echo "ERROR: $count byte-offset slice(s), up from $BASELINE." echo "These index a &str by BYTE offset and panic when the" echo "offset is not a char boundary -- reachable from any" echo "inbound SMS containing emoji or non-Latin text. Use" echo "char_indices() to find a real boundary first." exit 1 fi if [ "$count" -lt "$BASELINE" ]; then echo echo "Good: down to $count. Lower BASELINE in this file to $count." exit 1 fi echo "OK" # --------------------------------------------------------------------- # robius-sms: the platform layer. Cheap -- no GUI stack. # --------------------------------------------------------------------- robius-sms: runs-on: ubuntu-latest timeout-minutes: 30 steps: - uses: actions/checkout@v4 # polkit/gio are pulled in ONLY by the Linux backend, whose every # function returns PermanentlyUnavailable. They are why # deny-nigig-build.toml carries RUSTSEC-2024-0370 and # RUSTSEC-2024-0429 plus an open LGPL-2.1 question. Phase E9 of the # remediation plan removes the backend and these packages with it; # until then CI needs the headers to build the stub. - name: Install native dependencies run: | sudo apt-get update -qq sudo apt-get install -y -qq \ pkg-config libpolkit-gobject-1-dev libpolkit-agent-1-dev \ libglib2.0-dev libsqlite3-dev - name: Host check run: cargo check --locked -p robius-sms - name: Host clippy run: cargo clippy --locked -p robius-sms --all-targets -- -D warnings - name: Host tests run: cargo test --locked -p robius-sms # --------------------------------------------------------------------- # The Android target. This is the job that matters: everything in # sys/android/ is invisible to every other job in this file. # --------------------------------------------------------------------- android: runs-on: ubuntu-latest timeout-minutes: 40 steps: - uses: actions/checkout@v4 # d8 (build-tools 34) rejects class file major version > 61, i.e. # anything newer than Java 17. A runner defaulting to JDK 21 makes # build.rs fail inside d8 with "Unsupported class file major # version 65", which reads like a toolchain bug rather than a JDK # mismatch. Pin the JDK so that failure cannot recur here. - uses: actions/setup-java@v4 with: distribution: temurin java-version: '17' - uses: android-actions/setup-android@v3 - name: Install Android SDK packages run: sdkmanager "platforms;android-34" "build-tools;34.0.0" - name: Add Rust Android target run: rustup target add aarch64-linux-android # robius-sms/src/build.rs compiles two .java files with javac and # dexes them with d8, so this step exercises the Java toolchain and # the runtime-dex-loading path, not just the Rust. - name: Android check run: cargo check --locked -p robius-sms --target aarch64-linux-android - name: Android clippy run: | cargo clippy --locked -p robius-sms \ --target aarch64-linux-android -- -D warnings # --------------------------------------------------------------------- # nigig-sms: the UI layer. Pulls the whole Makepad stack, so it is the # slow job and needs the GUI system libraries. # --------------------------------------------------------------------- nigig-sms: runs-on: ubuntu-latest timeout-minutes: 60 steps: - uses: actions/checkout@v4 - name: Install native dependencies run: | sudo apt-get update -qq sudo apt-get install -y -qq \ pkg-config libwayland-dev libxcursor-dev libxrandr-dev \ libxi-dev libx11-dev libgl1-mesa-dev libasound2-dev \ libpolkit-gobject-1-dev libpolkit-agent-1-dev \ libglib2.0-dev libssl-dev libsqlite3-dev libudev-dev \ libpulse-dev libxkbcommon-dev - name: Check run: cargo check --locked -p nigig-sms - name: Test run: cargo test --locked -p nigig-sms # `-D warnings` cannot be applied to the whole `-p nigig-sms` build: # it also compiles nigig-core, nigig-uikit and matrix_client, which # carry ~89 pre-existing warnings that are not this workflow's to # fix. Muting them with --no-deps does not work either -- they are # workspace members, not registry deps, so clippy still reports # them. # # So: take clippy's JSON, keep only diagnostics whose package id is # nigig-sms, and count those. Dependency warnings stay visible in # the log above but do not fail the build. When nigig-core and # nigig-uikit are cleaned up, this can collapse to a plain # `-- -D warnings`. # # RATCHET at 49. Every one is mechanical (unused imports, dead # code, map->for_each, needless borrows) and none is a logic # change, but `cargo clippy --fix` cannot apply them: the crate is # built around Makepad's script_mod! proc macro and rustfix # refuses to edit through it. They therefore have to be fixed by # hand, which is Phase F work. # # 50 -> 49 when Phase C5 deleted the hand-rolled date helpers. # 49 -> 32 in Phase F: deleting the two dead compose paths, the # duplicate contact subsystem in sms_screen.rs and the duplicated # Desktop/Mobile page tree removed the code those lints were # reporting on, and orphaned a further eight imports. # # This was briefly 52: Phase 0.7 turned on clippy::string_slice, # which surfaced the two byte-offset slices behind bug A3. A3 is # now fixed and that lint is deny-with-a-scoped-allow in # sms_utils.rs, so the count is back to 50. # # 34 of the 50 are dead-code reports for the duplicate contact # subsystem in inbox/sms_screen.rs -- an entire second copy of the # cache, its lookup helpers and try_load_contacts(), none of it # reachable. Deleting that file's dead half clears most of this # number in one commit. # G: the SMS surface must keep a real test suite. # # This crate shipped with TWO tests, both asserting derived trait # impls, neither mentioning SMS -- for 7,725 lines whose job is to # spend the user's money. That is the defect that produced every # other defect in the remediation plan, because nothing could prove # a change was safe. # # A floor, not a ratchet: tests should only ever go up, and unlike # the clippy count there is no reason to want this number to fall. - name: The SMS test suite must not shrink run: | set -euo pipefail FLOOR=100 total=0 for spec in "-p robius-sms" "-p nigig-sms"; do n=$(cargo test --locked $spec 2>&1 \ | grep -oE '^test result: ok\. [0-9]+ passed' \ | grep -oE '[0-9]+' | awk '{s+=$1} END {print s+0}') echo " $spec -> $n" total=$((total + n)) done n=$(cargo test --locked -p nigig-core --test sms_store 2>&1 \ | grep -oE '^test result: ok\. [0-9]+ passed' \ | grep -oE '[0-9]+' | awk '{s+=$1} END {print s+0}') echo " sms_store -> $n" total=$((total + n)) echo "total: $total (floor $FLOOR)" if [ "$total" -lt "$FLOOR" ]; then echo echo "ERROR: the SMS test suite has shrunk to $total, below $FLOOR." echo "Tests were deleted or a test binary stopped running." exit 1 fi echo "OK" - name: Clippy ratchet (nigig-sms-owned diagnostics only) run: | set -euo pipefail BASELINE=32 cargo clippy --locked -p nigig-sms --all-targets \ --message-format=json > /tmp/clippy-sms.json 2>/tmp/clippy-sms.err || true # Surface the human-readable log for debugging. cat /tmp/clippy-sms.err || true BASELINE="$BASELINE" python3 - <<'PY' import json, os, sys baseline = int(os.environ['BASELINE']) owned, seen = [], set() with open('/tmp/clippy-sms.json') as fh: for line in fh: try: m = json.loads(line) except ValueError: continue if m.get('reason') != 'compiler-message': continue if 'nigig-sms' not in m.get('package_id', ''): continue msg = m['message'] if msg.get('level') not in ('warning', 'error'): continue # --all-targets compiles lib and lib-test, duplicating # every diagnostic; dedupe on rendered text. key = msg.get('rendered', '') if key in seen: continue seen.add(key) owned.append(msg) n = len(owned) print("found %d nigig-sms diagnostics, baseline %d" % (n, baseline)) if n > baseline: for msg in owned: sys.stdout.write(msg.get('rendered', '')) print() print("ERROR: %d diagnostics, up from %d." % (n, baseline)) sys.exit(1) if n < baseline: print() print("Good: down to %d. Lower BASELINE in this file to %d " "so the progress cannot be undone." % (n, n)) sys.exit(1) print("OK") PY # --------------------------------------------------------------------- # Supply chain. Mirrors the gates already in nigig-build.yml so the SMS # crates cannot drift from the rest of the workspace. # --------------------------------------------------------------------- supply-chain: runs-on: ubuntu-latest timeout-minutes: 15 steps: - uses: actions/checkout@v4 # A lockfile that changes during CI means the committed one was # stale. This is only enforceable because Phase 0.1 committed the # root Cargo.lock -- before that it was gitignored and every build # silently re-resolved. - name: Lockfile must be committed and current run: | set -euo pipefail test -f Cargo.lock || { echo "missing Cargo.lock at workspace root"; exit 1; } cargo metadata --locked --format-version 1 > /dev/null git diff --exit-code -- Cargo.lock - name: Reject whitespace errors run: git diff --check