11 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
|
25f32f7870 |
test(sms): build a real test suite (Phase G)
Some checks failed
doc-engine / engine (push) Waiting to run
doc-engine / consumer (push) Waiting to run
nigig-map / test (push) Waiting to run
sms / gates (push) Waiting to run
sms / robius-sms (push) Waiting to run
sms / android (push) Waiting to run
sms / nigig-sms (push) Waiting to run
sms / supply-chain (push) Waiting to run
nigig-build (CAD) / supply-chain (push) Has been cancelled
nigig-build (CAD) / cad-module (push) Has been cancelled
nigig-build (CAD) / full-crate-check (push) Has been cancelled
repo hygiene / hygiene (push) Has been cancelled
50 tests -> 102, and the two that were there at the start of this work
are deleted.
Where this started: robius-sms had ZERO tests, and nigig-sms had two --
bulk_sub_tab_default_is_contacts and bulk_sub_tab_variants_distinct.
Both asserted a derived Default and a derived PartialEq. Neither
mentioned SMS. Neither could fail short of the compiler breaking. That
is the defect that produced every other defect in this plan: nothing
could prove a change was safe, so nothing was ever deleted and every
bug survived contact with review.
Property tests (proptest, new dev-dependency)
Seven over truncate_preview, format_timestamp, badge_text, and five
more over segment_count, the rate limiter and ScheduleRequest.
These are the ones that matter, because the hand-written cases in this
repo all encode a bug someone had ALREADY found. proptest searches the
space instead. I verified that by reinstating the original byte-slicing
truncate_preview and confirming
prop_truncate_preview_survives_mixed_scripts and
prop_truncate_preview_respects_the_char_limit both fail against it --
they would have caught A3 before it shipped.
prop_rate_limiter_respects_capacity models the window independently
and asserts the invariant across random clock sequences, rather than
re-implementing the limiter's own arithmetic in the assertion.
Integration tests (2 new files, public API only)
robius-sms/tests/sms_pipeline.rs and nigig-core/tests/sms_store.rs go
through the public surface the application actually uses. The unit
tests inside src/ can see private helpers; these cannot, which is the
point -- they catch a refactor that keeps every unit test green while
breaking the caller-visible contract.
Two of them are privacy canaries. e1_message_bodies_are_never_persisted
and e1_no_body_text_reaches_the_serialised_store fail if anyone removes
#[serde(skip)] from OfflineSmsMessage.body. Verified by removing it:
both fail, the other six pass. Nothing else in the tree would have
noticed the inbox silently going back to plaintext on disk.
Named regression tests
One per defect, named for it -- c1_*, d1_*, d3_*, e1_*, e7_*, a4_*,
c3_*, c7_* -- so a future reader goes from a failing test straight to
the bug it guards rather than to a git archaeology session.
New coverage for logic that had none
- build_timeline_items / build_filtered_timeline_items: date-divider
placement and the message indices the draw loop uses to index
conv_data.messages. An off-by-one there renders the wrong body in
the wrong bubble; it had no test at all.
- kind_to_offline / kind_from_offline round-trip: the only thing
stopping a cached Sent message reappearing as Inbox after a restart,
which would flip the bubble to the wrong side of the screen.
- normalize_number: what C1 groups on, across five formatting variants
plus short codes and alphanumeric senders.
MessageKind::from_android_type / to_android_type were hoisted out of
sys/android/inbox.rs onto the type, the same way ScheduleRequest::validate
was in A4, so the provider mapping is testable off-device. An
unrecognised TYPE value is preserved verbatim in Unknown rather than
defaulted, and there is a property test asserting the round trip is
total over every i32.
CI: a test-count FLOOR at 100. A floor rather than a ratchet -- unlike
the clippy count, there is no reason to ever want this number to fall.
Deliberately NOT faked: the JNI cursor loop, the keystore round-trip and
broadcast delivery still need an emulator. A mock returning what I expect
would test my expectations, not Android. Those remain called out in the
Phase A and E commit messages.
Verified: 11/11 checks. 48 robius-sms + 46 nigig-sms + 8 sms_store = 102.
clippy -D warnings clean on host and aarch64-linux-android; nigig-sms
ratchet holds at 32 (my first draft added an orphaned `use super::*`,
caught by the ratchet and removed rather than baselined).
|
||
|
|
1670ddf49c |
refactor(sms): delete the dead code and the duplication (Phase F)
Some checks failed
doc-engine / engine (push) Has been cancelled
doc-engine / consumer (push) Has been cancelled
nigig-build (CAD) / supply-chain (push) Has been cancelled
nigig-build (CAD) / cad-module (push) Has been cancelled
nigig-build (CAD) / full-crate-check (push) Has been cancelled
repo hygiene / hygiene (push) Has been cancelled
nigig-map / test (push) Has been cancelled
sms / gates (push) Has been cancelled
sms / robius-sms (push) Has been cancelled
sms / android (push) Has been cancelled
sms / nigig-sms (push) Has been cancelled
sms / supply-chain (push) Has been cancelled
Net -596 lines. No behaviour change except F10, which replaces a label that was lying. F2 -- four copies of one stub backend. apple.rs, linux.rs and windows.rs were BYTE-IDENTICAL 66-line files, and unsupported.rs was the same again. That duplication is what let them drift: Phase C3 had to fix `Error::Unknown` in exactly one of the four, because only one had it wrong. Collapsed into sys/stub.rs, which each platform module invokes. 268 lines become 35 plus one shared definition. The module is cfg'd out on Android, which has a real implementation and would otherwise report the macro as unused under -D warnings. F3 -- TWO dead compose implementations. SmsComposePage (189 lines) was registered in the VM and instantiated nowhere. Separately, the FAB and its compose overlay were left in the DSL as `visible: false` with a comment saying "FAB removed: SMS compose/inbox navigation now lives in SmsActionBar" -- but 102 lines of DSL and 53 lines of handler stayed behind, wired to a button no user can reach. Deleted both, and send_reply() with them: it existed only to serve the unreachable overlay. Compose navigation is SmsActionBar's, as the comment already said. F4 -- a whole second contact subsystem, unreachable. sms_screen.rs carried its own CONTACTS_CACHE, contacts_loaded(), load_contacts_into_cache(), display_name_for_number(), normalize_number(), try_load_contacts() and a contacts_load_attempted field. Nothing called any of it -- the live implementation is in conversations_list.rs. Worth noting the dead copy was also the WRONG one: its display_name_for_number did an O(n) linear scan of the whole phone book per lookup, where the live version is O(1) because cache_contact_number inserts under both the raw and normalised key. F5 -- the page tree was written out twice. sms_bulk_page, sms_schedule_page and sms_more_page were each declared under Desktop AND under Mobile, byte-identical apart from indentation. Any change to a page header had to be made in both places or the layouts silently diverged. Now three named widgets plus a shared SmsPageHeader, referenced from both variants. F8 -- serde, serde_json and robius-location were declared by nigig-sms and referenced nowhere in its sources. F9 -- was_scrolling was read twice per frame from the same portal list; the copy in handle_event was bound and never used. F10 -- the character counter was a hardcoded lie. The old compose page rendered "0 / 160 characters" and never updated it. It died with F3, but the bulk composer -- where the money actually goes -- had no cost indication at all. It now shows live segment count as you type, using segment_count() from Phase A5, because segments are the billing unit and "160" is only right for GSM-7: one emoji forces UCS-2 and drops the limit to 70. This is the only user-visible change in the commit. F1 and F7 were already done, in Phase A (shared cursor.rs) and Phase D1 (I/O out of draw_walk). The deletions orphaned eight imports, which are also removed. Together that takes the nigig-sms clippy ratchet from 49 to 32 -- these were not suppressed, the code they reported on is gone. Verified: 10/10 checks. clippy -D warnings clean on host AND aarch64-linux-android, 28 robius-sms tests, 22 nigig-sms tests, nigig-build still builds, metadata --locked clean. |
||
|
|
737a3e5d5d |
security(sms): encrypt scheduled payloads, report real send status (E3, E7)
Some checks failed
sms / gates (push) Has been cancelled
sms / robius-sms (push) Has been cancelled
sms / android (push) Has been cancelled
sms / nigig-sms (push) Has been cancelled
sms / supply-chain (push) Has been cancelled
repo hygiene / hygiene (push) Has been cancelled
Closes the two Phase E items I had left open and documented as open.
E3 -- scheduled message bodies were plaintext on disk.
Pending schedules must outlive the process so SmsAlarmReceiver can
send them when the alarm fires and so they survive a reboot, so
recipient and body go to SharedPreferences. MODE_PRIVATE is the right
primitive -- the file is UID-scoped -- but the contents were in the
clear, readable by anything running as the same UID and swept into
cloud backup by default. Same asset class as the inbox
(THREAT_MODEL.md T-I4).
Adds SmsScheduleCrypto: AES-256-GCM, fresh IV per value, key generated
inside the platform AndroidKeyStore and non-exportable. An attacker
with the prefs file but not the keystore gets ciphertext.
Deliberately NOT androidx.security.EncryptedSharedPreferences: that is
a Gradle dependency, and this crate compiles its Java with bare javac
against android.jar (see build.rs), so using it would mean a Gradle
build or a vendored jar. AndroidKeyStore and javax.crypto are both in
android.jar and give the property that matters.
The key is deliberately NOT user-authentication-bound: an alarm fires
while the device may be locked and the receiver must decrypt with no
user present. This protects against another app and against an
extracted backup, which is the threat in scope -- not against someone
holding an unlocked handset.
Fails CLOSED. If the keystore is unavailable, encrypt returns null and
schedule_sms errors rather than writing plaintext. A row that cannot
be decrypted -- wrong key after a reinstall, tampering, or written by
an older build -- is treated exactly like a missing row and skipped;
sending a garbled body would be worse than not sending.
E7 -- "sent" was a guess.
Both the sentIntent and deliveryIntent arguments were null, so nothing
could report back. send_sms returning Ok meant "the JNI call
returned", not that the radio accepted the message and certainly not
that it arrived -- and the UI rendered that as a tick. A send rejected
for no service, no SIM or a throttled radio was indistinguishable from
a delivered one.
Adds send_sms_tracked, which attaches real PendingIntents and returns
a correlating token, plus SmsSentReceiver to collect the platform
result and SendOutcome/SendReport to express it: Sent (radio accepted)
is now a different value from Delivered (handset acknowledged), and
failures carry the RESULT_ERROR_* code.
Three details worth recording:
- multipart takes ArrayList<PendingIntent>, one entry per part, so
the intent is repeated part_count times. Passing null here, as
before, meant no status for exactly the messages most likely to
fail: the long ones.
- the request code is derived from (token, kind), or the two intents
collide and the delivery report overwrites the send report.
- the broadcast is package-scoped and the receiver registered
NOT_EXPORTED, so another app cannot forge a delivery report.
If the receiver class is unavailable the send still goes out with null
intents: losing the status report is much better than losing the
message.
Also fixes A5 on the scheduled path. SmsAlarmReceiver still called
sendTextMessage directly, so a scheduled message over 160 GSM-7
characters -- or 70 with any emoji -- was silently truncated by the
carrier. It now divides and sends multipart, as the Rust send path has
since Phase A.
CI: two gates, both negative-tested by reverting the fix and confirming
they fail. One asserts schedule_sms never writes request.recipient or
request.body directly; the other asserts the send path still passes
sent/delivery intents in both the single-part and multipart calls.
THREAT_MODEL.md T-I4 and the delivery-confirmation row move from open to
fixed, with the residual risk stated: callers may still use the
untracked send_sms, which remains honest about meaning only "handed to
the platform".
Tests: robius-sms 25 -> 28.
Verified: 13/13 CI checks, clippy -D warnings clean on host and
aarch64-linux-android, both new Java classes javac-compile and dex.
NOT verified on a device. The keystore round-trip, the broadcast
delivery and the token correlation all need an emulator or handset;
there is still no CI runner on this repo.
|
||
|
|
cbe47a5f6c |
perf(sms): close the outstanding Phase B and D items
Some checks failed
doc-engine / engine (push) Has been cancelled
doc-engine / consumer (push) Has been cancelled
nigig-map / test (push) Has been cancelled
sms / gates (push) Has been cancelled
sms / robius-sms (push) Has been cancelled
sms / android (push) Has been cancelled
sms / nigig-sms (push) Has been cancelled
sms / supply-chain (push) Has been cancelled
repo hygiene / hygiene (push) Has been cancelled
nigig-build (CAD) / supply-chain (push) Has been cancelled
nigig-build (CAD) / cad-module (push) Has been cancelled
nigig-build (CAD) / full-crate-check (push) Has been cancelled
An audit of Phase 0 through D against the tree found three tasks marked
done in prose but absent from the code. This closes them.
D5 -- bulk send still blocked the UI thread.
The send loop ran synchronously in handle_send_bulk.
SmsManager.sendTextMessage queues to the radio and rate-limits, so a
200-recipient batch was a multi-minute ANR with no progress and no
way to tell whether anything was happening. Phase E8's throttle
bounded the worst case at 30 sends, but bounded blocking is still
blocking -- and I said as much when deferring it.
Now uses the worker pattern D1 established: spawn, publish progress
through a mutex-guarded slot, SignalToUI, drain on the UI thread. The
status line counts up ("Sending 12/200…") instead of freezing.
BULK_SEND_IN_FLIGHT prevents two overlapping batches.
D2 -- the ContentObserver, the half I left open.
Phase D removed the 5-second poll that re-armed itself via redraw()
and stopped the app ever idling. That fixed the busy loop but left a
gap I documented rather than closed: a message arriving while the app
was open did not surface until the next Resume or manual pull.
Adds SmsInboxObserver.java -- a ContentObserver on content://sms,
registered with a main-Looper Handler, idempotent so onResume can call
it freely -- compiled and dexed by the existing build.rs pipeline and
loaded through the same in-memory dex loader as the receivers.
onChange calls into Rust, which does two cheap things: set an atomic,
and invoke a registered waker. The waker matters. robius-sms has no UI
dependency and cannot call SignalToUI itself, so without it the flag
would only be observed on the next event-loop turn that happened for
some other reason -- which, with the poll gone, might be never while
the app sits idle. The app registers SignalToUI::set_ui_signal, so
this is a genuine push.
Native binding is dynamic, not #[no_mangle], for the same reason as
E6: the class comes from an in-memory dex and is not on the JVM's
search path.
B0 (wider) -- 23 crates declared robius-sms and never called it.
Phase B removed the three declarations that put RUSTSEC advisories on
nigig-build and explicitly flagged the rest as "the same latent
problem, sweep separately". This is that sweep: every crate with zero
references to robius_sms in its sources loses the dependency.
nigig-mpesa, nigig-pay and nigig-sms keep it -- they are the only real
users. nigig-system-prefs only mentions robius-sms in its package
description, so its manifest is untouched.
Cargo.lock loses another 21 lines.
Verified: 13/13 CI checks. clippy -D warnings clean on host and
aarch64-linux-android; the Android build compiles, javac-builds and
dexes the new observer class. cargo deny still "advisories ok, bans ok,
licenses ok, sources ok". clippy ratchet holds at 49. Sampled four of
the 23 stripped crates plus all five I edited; all build.
Pre-existing and unrelated: nigig-map fails to compile on pristine
origin/main (12 errors in view.rs, a Script/Widget derive problem), so
pageflipnav and anything else reaching it cannot be checked here. I
touched no files under crates/apps/map.
NOT verified on a device. The observer's registration, the onChange
callback and the waker all need an emulator or handset with a live SMS
provider; this sandbox has neither, and there is still no CI runner.
|
||
|
|
a38c41c00a |
security(sms): stop persisting message bodies, throttle sends (Phase E)
Some checks failed
repo hygiene / hygiene (push) Has been cancelled
doc-engine / engine (push) Has been cancelled
doc-engine / consumer (push) Has been cancelled
nigig-build (CAD) / supply-chain (push) Has been cancelled
nigig-build (CAD) / cad-module (push) Has been cancelled
nigig-build (CAD) / full-crate-check (push) Has been cancelled
nigig-map / test (push) Has been cancelled
sms / gates (push) Has been cancelled
sms / robius-sms (push) Has been cancelled
sms / android (push) Has been cancelled
sms / nigig-sms (push) Has been cancelled
sms / supply-chain (push) Has been cancelled
E1 -- the inbox was written to disk in plaintext. offline_store wrote every SMS body to app_data_dir/offline_store/sms_messages.json as pretty-printed JSON. SMS is the transport for OTPs, banking codes and M-Pesa confirmations, so that file was the user's complete authentication history sitting in app-private storage -- readable by anything running as the same UID, and included in backups. This repository already knew the answer. THREAT_MODEL.md T-I2 records "Raw SMS persisted to PSV file" as fixed in Phase 0, with raw_message omitted from save_to_disk() so it "lives in memory only". The SMS app then re-introduced the same defect at larger scale: the entire inbox rather than just M-Pesa messages, and with no retention limit until D6. OfflineSmsMessage.body is now #[serde(skip)]. Dropping the field rather than encrypting it is the deliberate choice: every consumer already reads the device provider FIRST and writes the cache second (nigig-sms fetch_from_device, and the mpesa and pay transaction pages), so the provider is the system of record and no body needs to survive a restart. Encryption would keep the plaintext reachable to anything holding the key. Not writing it removes the asset. Two consequences handled: sms_key() no longer hashes the body, since a reloaded row has an empty one and dedupe would otherwise never match its own cached entry and grow a duplicate per refresh; and the cached first paint shows a neutral placeholder rather than a blank preview for the instant before the provider read lands. E9 -- the Linux backend's dependencies were pure cost. robius-sms declared polkit =0.17.0 and gio =0.17.0 for target_os = "linux". sys/linux.rs references neither: all twelve functions return Err(PermanentlyUnavailable). Those two crates dragged in glib and proc-macro-error and were the origin of RUSTSEC-2024-0370 and RUSTSEC-2024-0429 for every consumer of this crate. Deleting the block removes 340 lines from Cargo.lock. polkit, gio, glib and proc-macro-error no longer appear in the workspace at all, which also closes the LGPL-2.1 linkage question outright rather than routing around it as Phase B did for nigig-build alone. E4 -- ROBIUS_SMS_BOOT_LIB was a code-injection vector. build.rs interpolated that environment variable straight into a Java string literal, which is then compiled, dexed and loaded at runtime with the app's full permissions. A value containing a quote closes the literal and injects arbitrary Java that runs on the device at boot. Build-time environment is not trusted input. Now validated against [A-Za-z0-9_]+ and the build fails loudly otherwise. Tested both ways: an exec payload is rejected, a legitimate name builds. E5 -- undefined behaviour in the dex loader. new_direct_byte_buffer was handed RECEIVER_BYTECODE.as_ptr() as *mut u8 -- a &'static [u8] in .rodata cast to a mutable pointer, when the API is documented as taking writable memory and InMemoryDexClassLoader may write through it. Now copies into an owned allocation and leaks it, which is correct rather than lazy: the buffer backs a ClassLoader cached in a OnceLock for the process lifetime. E6 -- two bindings for one native method. rustRestoreSchedules was both exported #[no_mangle] and registered dynamically via register_native_methods. Which one won was unspecified. Kept the dynamic one, because the class is loaded from an in-memory dex and is not on the JVM's search path, so symbol binding is not guaranteed to find it. E8 -- no send rate limiting. Nothing capped send rate, and the bulk UI exists to blast a scraped directory. Android's practical throttle is ~30 messages per 30 minutes per app, past which sends are silently dropped -- so an unthrottled batch both overspends and fails opaquely. Adds SendRateLimiter, a pure token bucket taking an explicit clock so it is unit-testable without sleeping, wired into the bulk sender. A 200-recipient blast now stops at 30 and says why. E10 -- robius-sms carried no license field, so cargo-deny needed a [[licenses.clarify]] override asserting one. Stated in the manifest; override removed. E2 was already satisfied by D6 (retention capped at 5,000). E7/E11 are documented rather than fixed, which is the honest status: delivery confirmation needs real PendingIntents plumbed through (A8 documents that Ok != delivered), and sender validation cannot be solved client-side. Both are now rows in THREAT_MODEL.md instead of findings in a markdown report -- along with T-I2b for E1 and T-I4 for E3, which is NOT done: scheduled message bodies are still plaintext in SharedPreferences. CI: adds a gate asserting OfflineSmsMessage.body keeps #[serde(skip)]. Removing that attribute silently resumes writing plaintext and nothing else would fail. Negative-tested. deny-nigig-build.toml drops to 2 exemptions (from 5 before Phase B). Tests: robius-sms 21 -> 25. Verified: 12/12 CI jobs, clippy -D warnings clean on host and aarch64-linux-android, cargo deny "advisories ok, bans ok, licenses ok, sources ok", clippy ratchet holds at 49. |
||
|
|
0614a70888 |
fix(sms): correctness pass on grouping, errors, dates and iOS (Phase C)
Some checks failed
repo hygiene / hygiene (push) Has been cancelled
sms / gates (push) Has been cancelled
sms / robius-sms (push) Has been cancelled
sms / android (push) Has been cancelled
sms / nigig-sms (push) Has been cancelled
sms / supply-chain (push) Has been cancelled
C1 -- one contact appeared as several conversations.
populate_display_messages() keyed the group HashMap on the RAW
provider address. A Kenyan inbox routinely carries three forms of the
same person -- +254712345678, 0712345678, 254712345678 -- depending on
whether they were on-net, roaming, or saved in contacts. Each became a
separate thread holding part of the history, and a reply went to
whichever one happened to be open, so the user's own messages
scattered across the duplicates.
normalize_number() (last 9 digits) already existed and was already
used for contact-NAME lookup; it just was not used for grouping.
get_conversation() and insert_sent_message() now match the same way,
so a reply joins the existing thread. Alphanumeric senders
("Safaricom", "MPESA") normalise to empty and fall back to the raw
address, so they are not all merged into one bucket.
C3 -- every JNI failure said "Unknown error".
From<jni::errors::Error> mapped everything to Error::Unknown,
discarding the payload. A failed send read identically whether the
cause was a missing method, a mismatched descriptor, a Java exception
or an unreachable JVM. Field diagnosis was impossible -- and this is
how the setRepeating signature bug (fixed in A4) stayed invisible.
Adds Error::Jni { kind: JniFailure, detail } and classifies. Also
fixes sys/unsupported.rs, which returned Unknown for all twelve
functions where every other stub backend returns
PermanentlyUnavailable, so unsupported targets reported "Unknown
error" instead of "not supported here".
C4 -- dead branch in show_conversation().
Two blocks; the second ran unconditionally and re-applied the
no-search behaviour, making the search branch above it dead. Opening a
conversation with an active filter scrolled to the bottom of the
UNFILTERED timeline instead of the top of the matches. A merge
artifact, invisible unless you read the control flow rather than the
surrounding "Mirror Robrix" comments.
C5 -- timestamps were wrong outside East Africa.
~160 lines of hand-rolled civil-date arithmetic with a hardcoded
UTC+3. The desktop branch attempted to read $TZ but stripped the
alphabetic characters and parsed the remainder, so "America/New_York"
became "/New_York", failed, and fell through to +3 as well: it could
never have worked for any named zone.
Deleted in favour of chrono, which was ALREADY a dependency of this
crate and already used correctly in conversation_screen.rs. Verified
green under TZ=UTC, Africa/Nairobi, America/New_York and Asia/Tokyo.
C6 -- iOS thread ids were from the wrong namespace.
read_modern_db selected m.handle_id -- a PARTICIPANT id -- and wrote
it into SmsMessage.thread_id. But list_thread_messages() filters on
chat_message_join.chat_id and read_modern_threads() reports
chat.ROWID. Three namespaces, so an id handed out on read never
matched the id expected on query: thread navigation on iOS was broken
by construction. Now joins chat_message_join and selects the chat id.
apple_date_to_ms() also assumed nanoseconds unconditionally; older
chat.db revisions store whole seconds in some columns, which divided
by 10^9 and rendered as 1970. Now disambiguates by magnitude.
C7 -- the bulk path skipped its validation.
The app never called send_bulk_sms; it wrote its own loop over
send_sms so it could report per-recipient success. That also skipped
validate_bulk_send_request, the only check that rejects a
whitespace-only recipient inside a batch -- a stray blank line in the
recipients box. Rather than give up the per-recipient reporting, the
rule moves onto BulkSendRequest::validate() and the caller runs it
explicitly.
C2 was already fixed in Phase A (A10, persisted id counter).
Tests: robius-sms 15 -> 21, nigig-sms 15 -> 19.
CI: nigig-sms clippy ratchet 50 -> 49 (C5 removed the dead helpers).
Verified: clippy -D warnings clean on host and aarch64-linux-android,
nigig-build still builds, cargo deny still passes.
|
||
|
|
0ed64c0435 |
fix(sms): send long messages whole, and confirm before bulk (A5, A7, A8)
Some checks failed
repo hygiene / hygiene (push) Has been cancelled
sms / gates (push) Has been cancelled
sms / robius-sms (push) Has been cancelled
sms / android (push) Has been cancelled
sms / nigig-sms (push) Has been cancelled
sms / supply-chain (push) Has been cancelled
A5 -- long messages were silently truncated.
send_text_message() called SmsManager.sendTextMessage
unconditionally. That API is only defined for a body that fits one
PDU: 160 GSM-7 characters, or 70 once anything forces UCS-2. Past
that the behaviour is carrier- and OEM-dependent -- silent
truncation, silent failure, or an exception -- and the UI reported
"Sent" regardless, because a JNI call returning cleanly only means
the call was made.
Now uses divideMessage() + sendMultipartTextMessage() when the body
needs more than one part, so the recipient's handset reassembles it.
Adds segment_count(), which computes encoding and segment count on
any host so it is unit-testable and usable by UI. Correct GSM-7
modelling matters here and is not intuitive:
- the extended characters ^ { } \ [ ] ~ | € cost TWO septets each
- concatenation costs 7 septets per part, so the limit drops from
160 to 153 (and 70 to 67 for UCS-2)
- non-BMP characters such as emoji are surrogate pairs and cost two
UTF-16 units
- e/a/o/n/u with diacritics ARE in GSM-7. My first version of the
boundary test assumed é forced UCS-2 and failed; Swahili, French
and German text still bills at the 160-char rate. Arabic and
emoji do not.
A7 -- bulk send had no confirmation.
One tap on "Send SMS to Selected", one tap on Send, and N real,
billed, irreversible messages went out. For a feature whose purpose
is mass-messaging a scraped business directory, that is a
financial-harm defect rather than a UX gap.
Send now arms a confirmation quoting SEGMENTS, not messages, because
segments are what the user pays for and the number is surprising: one
emoji in the body forces UCS-2 and can turn "200 messages" into 800
paid segments. Editing the body or the recipient list re-arms the
prompt, so a confirmation cannot be inherited by different content.
A8 -- SCHEDULE_EXACT_ALARM was neither requested nor documented.
Android 12+ refuses exact alarms without it, and it is a
special-access permission: a runtime prompt cannot grant it, the user
must enable "Alarms & reminders" in Settings. Without it scheduled
sends are subject to Doze batching. Documented in the crate docs and
README, along with two things that were not written down anywhere:
Ok from send_sms means "handed to the platform", NOT delivered (both
PendingIntents are null, so nothing can report back); and callers are
billed per segment.
Verified: robius-sms 15 tests pass (8 new for segmentation), nigig-sms
15 pass, clippy -D warnings clean on host and aarch64-linux-android,
nigig-sms clippy ratchet holds at 50, gates and supply-chain green.
|
||
|
|
6bb135872a |
fix(sms): stop the cursor loop aborting the process (A1, A2)
Three defects in the content-provider read path, all of which abort
rather than return an error, and all of which were duplicated because
inbox.rs and thread.rs each carried their own byte-identical copy of
the column helpers (~75 lines).
A1 -- local reference table overflow.
Every get_*_column() called env.new_string(name) to resolve the
column by name, and getString returned another local ref: ~10 JNI
local references per message row. Local refs are not freed when the
Rust value drops; they live until the native method returns to the
JVM, which here is the end of the whole cursor loop. ART's default
local reference capacity is 512, so the table overflowed at roughly
45-50 messages -- and overflow is a hard abort ("local reference
table overflow"), not an Err.
Any real inbox has hundreds to thousands of messages. I consider this
the most likely single source of field crashes in this crate.
Fixed twice over: column indices are resolved ONCE per query into a
struct instead of per row per column, which removes the new_string
churn entirely; and each row is read inside env.with_local_frame(),
so whatever a row does allocate is released when that row finishes.
A2 -- pending exceptions were never cleared.
My assessment said this crate does no exception checking. That was
WRONG, and the correction matters: jni-rs 0.21 expands every checked
call through check_exception!, which calls ExceptionCheck and returns
Err(Error::JavaException).
The actual defect is that nothing ever CLEARS it. ExceptionClear
appears in exactly one place in jni-rs -- the public
exception_clear() -- and this crate never called it. So the Err
propagated while the exception stayed pending on the thread, and the
next JNI call made on that thread aborted the process.
A SecurityException from a revoked READ_SMS, or an
IllegalStateException from a stale cursor, therefore produced a tidy
Err and then killed the app somewhere unrelated. Every error path out
of these functions now runs clear_pending_exception(), which
describes the exception to logcat and leaves the thread clean.
Cursor leak -- `?` inside the loop skipped the close() at the end of
the function, leaking the Cursor and its CursorWindow (typically a
2 MB ashmem region) on every error. CursorGuard closes it on all
paths.
Also removes the duplicated helpers: both files now share
sys/android/cursor.rs, so a fix here cannot land in one copy and miss
the other.
Verified: check and clippy -D warnings clean on host and
aarch64-linux-android.
NOT verified on a device. The abort threshold, the frame capacity and
the exception paths need an emulator or handset with a populated inbox
to confirm, and there is still no CI runner registered on this repo.
|
||
|
|
d96f8301ef |
fix(sms): make scheduling actually work (A4, A9, A10)
Some checks failed
repo hygiene / hygiene (push) Has been cancelled
sms / gates (push) Has been cancelled
sms / robius-sms (push) Has been cancelled
sms / android (push) Has been cancelled
sms / nigig-sms (push) Has been cancelled
sms / supply-chain (push) Has been cancelled
Scheduled SMS could never have sent a single message. Four independent defects in one code path, all in code with no tests. A4 -- every request was rejected before reaching the platform. `ScheduleRequest.interval_ms` was a bare i64 and the validator required `interval_ms > 0`, but "send once" is the only mode the UI offers, so sms_schedule_page.rs passed `interval_ms: 0`. Result: 100% of scheduled messages failed with InvalidInput. The user saw "Scheduled: 0, Failed: N" and no reason. interval_ms is now Option<i64>: None = send once, Some(n>0) = repeat. The validator moves onto ScheduleRequest::validate() so it can be tested on any host -- the old copy was behind #[cfg(target_os = "android")], which is part of why a defect this total went unnoticed. Wrong JNI signature -- found while fixing the above, not in the original audit. setRepeating was invoked with the descriptor "(IJLandroid/app/PendingIntent;)V": three parameters, four arguments. AlarmManager.setRepeating is (int, long, long, PendingIntent), i.e. "(IJJLandroid/app/PendingIntent;)V". Even had A4 not rejected everything first, this would have thrown NoSuchMethodError at the JNI boundary -- and with no exception checking (A2, still open) that is a process abort, not an Err. A9 -- alarms did not wake the device. The type argument was hardcoded to 0 (AlarmManager.RTC). RTC does not fire while the device sleeps, so a 03:00 schedule waited until the user next picked up the phone. Now RTC_WAKEUP (1), as a named constant. A10 -- batches silently destroyed each other. Ids were `1000 + index`, so a second batch reused 1000..1000+n. PendingIntent.getBroadcast matches on request code and the crate passes FLAG_UPDATE_CURRENT, so the new alarm replaced the old one; store_schedule keys SharedPreferences on the same id, so the stored recipient and body went too. next_schedule_id() now allocates from a counter persisted under app_data_dir, because the collision has to be avoided across restarts, not just within a run. Persistence: SharedPreferences has no null long, so None is stored as ONE_SHOT_SENTINEL (0) and mapped back on load. Anything <= 0 reads back as None rather than being trusted as an interval -- a 0 interval would make setRepeating fire in a tight loop. Tests: robius-sms had NO tests. It now has 7, covering the validator's whole contract. accepts_the_one_shot_request_the_app_actually_builds constructs the exact shape sms_schedule_page.rs sends and fails against the old validator. Verified: robius-sms 7 tests pass, nigig-sms 15 pass, clippy -D warnings clean on host AND aarch64-linux-android, gates and supply-chain green. Still open on this path: A2 (no JNI exception checking) and SCHEDULE_EXACT_ALARM is neither requested nor documented, so exact delivery is not guaranteed on Android 12+. |
||
|
|
7db530b374 |
fix(sms): clear the three clippy errors on the Android target
These are only visible when cross-compiling: a host build of
robius-sms compiles sys/linux.rs, a stub whose every function returns
PermanentlyUnavailable, so none of sys/android/ is type-checked at
all. Host clippy was already clean; the Android target was not.
receivers.rs unnecessary `unsafe` block. The block wrapped nothing
that needs it -- taking `&mut env` and calling a safe
Rust fn -- inside an already-`extern "C"` function.
Removing it does not change what the JNI entry point
does; it stops the block from implying the body has
been audited for an invariant it does not have.
schedule.rs redundant closure |env, activity| load_schedules(env,
thread.rs activity) -> load_schedules.
All three are mechanical. No behaviour change.
Verified with the pinned toolchain (1.97.1), JDK 17, SDK 34:
cargo clippy -p robius-sms --all-targets -- -D warnings OK
cargo clippy -p robius-sms --target aarch64-linux-android
-- -D warnings OK
|
||
| e9616c3288 | added missing crates |