refactor(android): forbid unsafe workspace-wide, quarantine JNI in app (#91)

Addresses #91: the #90 posture (`deny(unsafe_code)` + three scattered
`#![allow(unsafe_code)]` across solitaire_data and solitaire_engine) punched
unsafe holes into otherwise-pure logic crates. Replace it by *reducing* the
unsafe rather than relocating it, then forbidding it everywhere it can be
forbidden.

Changes:
- solitaire_data: new safe `android_jni` bridge owning the cached `JavaVM`
  and activity `GlobalRef`; exposes `with_env` / `with_activity_env` so
  keystore/clipboard/safe-area never touch a raw handle.
- keystore: drop the `JavaVM::from_raw` init path (now in the app) and
  replace the three `unsafe { JByteArray::from_raw(x.into_raw()) }` casts
  with the safe `JByteArray::from(JObject)` conversion jni 0.21 provides.
- engine clipboard + safe_area: route through the bridge; remove their
  `#![allow(unsafe_code)]` and all `from_raw` calls.
- solitaire_app: becomes the single owner of FFI unsafe. `android_main`
  reconstructs the raw `JavaVM` / activity once (it must, as the cdylib that
  exports `#[unsafe(no_mangle)]`) and registers the safe wrappers. It opts to
  its own `deny`-level lints with two scoped `#[allow(unsafe_code)]`.
- workspace: `unsafe_code` is now `forbid`. Every crate except the app entry
  point is fully unsafe-free.

Net: 7 unsafe sites across three crates collapse to 3 at the OS boundary in
one crate. Verified with host `clippy --workspace -- -D warnings` and an
`aarch64-linux-android` clippy build of solitaire_app (transitively engine +
data); also fixed two latent android-only `collapsible_if` warnings surfaced
in the keystore by the cross-target check.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Cargo.lock

@@ -7306,6 +7306,7 @@ name = "solitaire_app"
 version = "0.1.0"
 dependencies = [
  "bevy",
+ "jni 0.21.1",
  "keyring",
  "solitaire_data",
  "solitaire_engine",

Cargo.toml

@@ -19,13 +19,14 @@ license = "MIT"
 rust-version = "1.95"
 
 # Pedantic correctness lints applied across every member crate via
-# `[lints] workspace = true`. `unsafe_code` is "deny" rather than "forbid"
-# so the three Android JNI FFI modules can opt back in with a scoped
-# `#![allow(unsafe_code)]` — `forbid` cannot be locally overridden, which
-# would break the Android build. Pure crates (core, sync) carry no `unsafe`
-# and so remain effectively forbidden in practice.
+# `[lints] workspace = true`.
 [workspace.lints.rust]
-unsafe_code = "deny"
+# Workspace-wide ban on `unsafe`. The sole exception is `solitaire_app`,
+# which sets its own `deny`-level lints (see its Cargo.toml) because the
+# Android cdylib entry point must reconstruct raw JNI handles. Every other
+# crate reaches Android JNI through the safe `solitaire_data::android_jni`
+# bridge and stays fully unsafe-free.
+unsafe_code = "forbid"
 single_use_lifetimes = "warn"
 trivial_casts = "warn"
 unused_lifetimes = "warn"

solitaire_app/Cargo.toml

@@ -22,6 +22,13 @@ bevy             = { workspace = true }
 solitaire_engine = { workspace = true }
 solitaire_data   = { workspace = true }
 
+# Android-only: the entry point reconstructs the raw `JavaVM` / activity
+# handles and registers the safe `solitaire_data::android_jni` bridge. This
+# is the one crate in the workspace that performs `unsafe` FFI, so it is also
+# the only one that depends on `jni` directly at the app layer.
+[target.'cfg(target_os = "android")'.dependencies]
+jni = { workspace = true }
+
 # Desktop-only deps. `keyring`'s default-store init only matters on
 # platforms with a real keychain backend (Linux Secret Service,
 # macOS Keychain, Windows Credential Store), and its transitive
@@ -100,5 +107,17 @@ icon = "@mipmap/ic_launcher"
 # enabling auto-rotate.
 orientation = "portrait"
 
-[lints]
-workspace = true
+# `solitaire_app` is the one crate that cannot inherit the workspace
+# `forbid(unsafe_code)`: as the Android cdylib it must export the
+# `#[unsafe(no_mangle)]` entry point and reconstruct the raw JNI handles
+# there (a `no_mangle` symbol cannot live in a dependency rlib). It mirrors
+# the workspace lints but at `deny`, so the two `#[allow(unsafe_code)]`
+# scopes in the Android entry point are the only unsafe in the whole tree.
+[lints.rust]
+unsafe_code = "deny"
+single_use_lifetimes = "warn"
+trivial_casts = "warn"
+unused_lifetimes = "warn"
+unused_qualifications = "warn"
+variant_size_differences = "warn"
+unexpected_cfgs = "warn"

solitaire_app/src/lib.rs

@@ -363,16 +363,57 @@ fn set_window_icon(
 /// works on a function named `main`; our shared entry point is `run`, so
 /// we emit the equivalent expansion manually.
 #[cfg(target_os = "android")]
+#[allow(unsafe_code)]
 #[unsafe(no_mangle)]
 fn android_main(android_app: bevy::android::android_activity::AndroidApp) {
-    let vm_ptr = android_app.vm_as_ptr().cast();
-    if let Err(e) = solitaire_data::init_android_jvm(vm_ptr) {
-        eprintln!("warn: could not initialise Android Keystore JNI ({e})");
+    if let Err(e) = init_android_jni(&android_app) {
+        eprintln!("warn: could not initialise Android JNI bridge ({e})");
     }
     let _ = bevy::android::ANDROID_APP.set(android_app);
     run();
 }
 
+/// Reconstructs the raw `JavaVM` / `NativeActivity` handles handed over by the
+/// Android runtime and registers safe wrappers with `solitaire_data`.
+///
+/// This is the *only* place in the workspace that performs `unsafe` FFI handle
+/// reconstruction. Every other crate consumes the safe
+/// [`solitaire_data::android_jni`] bridge and stays `forbid(unsafe_code)`;
+/// `solitaire_app` opts down to `deny` with a narrowly scoped allow on this
+/// function and the `#[unsafe(no_mangle)]` entry point above.
+#[cfg(target_os = "android")]
+#[allow(unsafe_code)]
+fn init_android_jni(
+    android_app: &bevy::android::android_activity::AndroidApp,
+) -> Result<(), String> {
+    use jni::JavaVM;
+    use jni::objects::JObject;
+
+    let vm_ptr = android_app.vm_as_ptr();
+    if vm_ptr.is_null() {
+        return Err("JavaVM pointer is null".into());
+    }
+    // SAFETY: `vm_as_ptr()` returns the process-wide JavaVM* established by the
+    // Android runtime; it is valid for the lifetime of the process.
+    let vm = unsafe { JavaVM::from_raw(vm_ptr.cast()) }.map_err(|e| format!("JavaVM: {e}"))?;
+
+    let env = vm
+        .attach_current_thread_permanently()
+        .map_err(|e| format!("attach_current_thread: {e}"))?;
+
+    // SAFETY: `activity_as_ptr()` returns the NativeActivity jobject pointer,
+    // valid for the lifetime of the process. Promote it to a global reference
+    // so the safe bridge can hand it to any thread.
+    let activity = unsafe { JObject::from_raw(android_app.activity_as_ptr().cast()) };
+    let activity_ref = env
+        .new_global_ref(&activity)
+        .map_err(|e| format!("activity global ref: {e}"))?;
+
+    solitaire_data::android_jni::set_jvm(vm);
+    solitaire_data::android_jni::set_activity(activity_ref);
+    Ok(())
+}
+
 /// Wraps the default panic hook with one that also appends a crash log
 /// to `<data_dir>/crash.log` (next to `settings.json`). The default hook
 /// still runs afterwards, so stderr output and debugger integration are

solitaire_data/src/android_jni.rs

@@ -0,0 +1,65 @@
+//! Safe JNI bridge for Android platform integration.
+//!
+//! Reconstructing the raw `JavaVM` / `NativeActivity` handles handed over by
+//! the Android runtime requires `unsafe` FFI. That single reconstruction lives
+//! in `solitaire_app`'s entry point ([`solitaire_app::android_main`]); this
+//! module only ever stores the resulting *safe* [`JavaVM`] and activity
+//! [`GlobalRef`] and hands callers an attached [`JNIEnv`].
+//!
+//! As a result every consumer of Android JNI — the keystore, clipboard, and
+//! safe-area subsystems — goes through the safe functions here and stays
+//! `forbid(unsafe_code)`. The only crate carrying `unsafe` is `solitaire_app`.
+//!
+//! Only compiled and linked on `target_os = "android"`.
+
+use jni::objects::{GlobalRef, JObject};
+use jni::{JNIEnv, JavaVM};
+use std::sync::OnceLock;
+
+static ANDROID_JVM: OnceLock<JavaVM> = OnceLock::new();
+static ANDROID_ACTIVITY: OnceLock<GlobalRef> = OnceLock::new();
+
+/// Store the process-wide [`JavaVM`]. Called once from Android startup
+/// (`solitaire_app::android_main`); subsequent calls are ignored.
+pub fn set_jvm(vm: JavaVM) {
+    let _ = ANDROID_JVM.set(vm);
+}
+
+/// Store a global reference to the `NativeActivity`. Called once from Android
+/// startup; subsequent calls are ignored.
+pub fn set_activity(activity: GlobalRef) {
+    let _ = ANDROID_ACTIVITY.set(activity);
+}
+
+/// Run `f` with a [`JNIEnv`] attached to the current thread.
+///
+/// Returns an error string if the bridge has not been initialised yet or the
+/// thread cannot be attached. The closure's JNI errors are surfaced through the
+/// same `String` channel so callers have a single error type to map.
+pub fn with_env<F, R>(f: F) -> Result<R, String>
+where
+    F: for<'local> FnOnce(&mut JNIEnv<'local>) -> jni::errors::Result<R>,
+{
+    let vm = ANDROID_JVM
+        .get()
+        .ok_or_else(|| "Android JavaVM not initialised".to_string())?;
+    let mut env = vm
+        .attach_current_thread_permanently()
+        .map_err(|e| format!("attach_current_thread: {e}"))?;
+    f(&mut env).map_err(|e| format!("JNI: {e}"))
+}
+
+/// Run `f` with an attached [`JNIEnv`] and the `NativeActivity` object.
+///
+/// Like [`with_env`] but also resolves the cached activity global reference, so
+/// callers that need to invoke instance methods on the activity (clipboard,
+/// window insets) never touch a raw handle.
+pub fn with_activity_env<F, R>(f: F) -> Result<R, String>
+where
+    F: for<'local> FnOnce(&mut JNIEnv<'local>, &JObject<'local>) -> jni::errors::Result<R>,
+{
+    let activity = ANDROID_ACTIVITY
+        .get()
+        .ok_or_else(|| "Android activity not initialised".to_string())?;
+    with_env(|env| f(env, activity.as_obj()))
+}

solitaire_data/src/android_keystore.rs

@@ -1,8 +1,3 @@
-// JNI FFI requires `unsafe` to reconstruct `JavaVM` / `JByteArray` handles
-// from raw pointers handed over by the Android runtime. Scoped to this
-// module so the rest of the workspace stays `deny(unsafe_code)`.
-#![allow(unsafe_code)]
-
 /// Android Keystore token storage via JNI.
 ///
 /// Tokens are serialised to JSON, encrypted with AES-256/GCM/NoPadding using a
@@ -19,19 +14,16 @@
 ///
 /// Only compiled and linked on `target_os = "android"`.
 use jni::{
-    JNIEnv, JavaVM,
+    JNIEnv,
     objects::{JByteArray, JObject, JObjectArray, JValue, JValueOwned},
 };
 use serde::{Deserialize, Serialize};
 use std::collections::HashMap;
-use std::ffi::c_void;
 use std::path::PathBuf;
-use std::sync::OnceLock;
 
 use crate::auth_tokens::TokenError;
 
 const KEY_ALIAS: &str = "ferrous_solitaire_token_key";
-static ANDROID_JVM: OnceLock<JavaVM> = OnceLock::new();
 
 #[derive(Serialize, Deserialize)]
 struct TokenBlob {
@@ -44,43 +36,15 @@ struct TokenBlob {
 // JVM helper
 // ---------------------------------------------------------------------------
 
-/// Initialise Android Keystore access with the process-wide `JavaVM*`.
-///
-/// This is called by `solitaire_app` from Android startup code. Keeping the
-/// raw JVM pointer here avoids making `solitaire_data` depend on the app or
-/// engine layer just to reach platform startup state.
-pub fn init_android_jvm(vm_ptr: *mut c_void) -> Result<(), TokenError> {
-    if vm_ptr.is_null() {
-        return Err(TokenError::KeychainUnavailable(
-            "JavaVM pointer is null".into(),
-        ));
-    }
-    if ANDROID_JVM.get().is_some() {
-        return Ok(());
-    }
-
-    // SAFETY: `vm_ptr` is supplied by Android startup code and must be the
-    // process-wide JavaVM* for this app. `OnceLock` keeps the wrapper alive for
-    // the process lifetime.
-    let vm = unsafe { JavaVM::from_raw(vm_ptr.cast()) }
-        .map_err(|e| TokenError::Keyring(format!("JavaVM: {e}")))?;
-    let _ = ANDROID_JVM.set(vm);
-    Ok(())
-}
-
+/// Run `f` with an attached `JNIEnv`, delegating thread attach and the
+/// `JavaVM` handle to the safe [`crate::android_jni`] bridge. The bridge is
+/// initialised once from Android startup, so the keystore never touches a raw
+/// pointer and this module stays `forbid(unsafe_code)`.
 fn with_jvm<F, R>(f: F) -> Result<R, TokenError>
 where
     F: for<'env> FnOnce(&mut JNIEnv<'env>) -> Result<R, jni::errors::Error>,
 {
-    let vm = ANDROID_JVM
-        .get()
-        .ok_or_else(|| TokenError::KeychainUnavailable("Android JavaVM not initialised".into()))?;
-
-    let mut env = vm
-        .attach_current_thread_permanently()
-        .map_err(|e| TokenError::Keyring(format!("attach: {e}")))?;
-
-    f(&mut env).map_err(|e| TokenError::Keyring(format!("JNI: {e}")))
+    crate::android_jni::with_env(f).map_err(TokenError::Keyring)
 }
 
 // ---------------------------------------------------------------------------
@@ -235,9 +199,10 @@ fn encrypt_gcm(
     .v()?;
 
     // IV is generated by Android's provider; read it back after init.
+    // `getIV()` returns `[B`; the safe `From<JObject>` reinterprets the
+    // returned object reference as a typed byte array.
     let iv_jobj = env.call_method(&cipher, "getIV", "()[B", &[])?.l()?;
-    // SAFETY: the method signature guarantees a byte array return.
-    let iv_arr = unsafe { JByteArray::from_raw(iv_jobj.into_raw()) };
+    let iv_arr = JByteArray::from(iv_jobj);
     let iv = env.convert_byte_array(&iv_arr)?;
 
     let pt_arr = env.byte_array_from_slice(plaintext)?;
@@ -245,8 +210,7 @@ fn encrypt_gcm(
     let ct_jobj = env
         .call_method(&cipher, "doFinal", "([B)[B", &[pt_val.borrow()])?
         .l()?;
-    // SAFETY: doFinal([B) returns [B.
-    let ct_arr = unsafe { JByteArray::from_raw(ct_jobj.into_raw()) };
+    let ct_arr = JByteArray::from(ct_jobj);
     let ciphertext = env.convert_byte_array(&ct_arr)?;
 
     let mut out = Vec::with_capacity(iv.len() + ciphertext.len());
@@ -297,8 +261,7 @@ fn decrypt_gcm(
     let pt_jobj = env
         .call_method(&cipher, "doFinal", "([B)[B", &[ct_val.borrow()])?
         .l()?;
-    // SAFETY: doFinal([B) returns [B.
-    let pt_arr = unsafe { JByteArray::from_raw(pt_jobj.into_raw()) };
+    let pt_arr = JByteArray::from(pt_jobj);
     env.convert_byte_array(&pt_arr)
 }
 
@@ -385,8 +348,9 @@ fn read_map() -> Result<HashMap<String, TokenBlob>, TokenError> {
     }
 
     // --- 2. Legacy path migration ---
-    if let Some(ref lpath) = legacy_path {
-        if lpath.exists() {
+    if let Some(ref lpath) = legacy_path
+        && lpath.exists()
+    {
         let data = read_file_bytes_from(lpath).map_err(|e| match e {
             TokenError::NotFound(_) => TokenError::NotFound(String::new()),
             other => other,
@@ -408,7 +372,6 @@ fn read_map() -> Result<HashMap<String, TokenBlob>, TokenError> {
         }
         // Legacy file corrupt or unrecognised — treat as empty.
     }
-    }
 
     // --- 3. No file found ---
     Ok(HashMap::new())
@@ -496,12 +459,12 @@ pub fn delete_tokens(username: &str) -> Result<(), TokenError> {
 
     if map.is_empty() {
         // No more users — remove the file and the Keystore key.
-        if let Some(path) = token_file_path() {
-            if path.exists() {
+        if let Some(path) = token_file_path()
+            && path.exists()
+        {
             std::fs::remove_file(&path)
                 .map_err(|e| TokenError::Keyring(format!("delete auth_tokens.bin: {e}")))?;
         }
-        }
 
         // Remove the Keystore key so a future re-login generates a fresh key.
         with_jvm(|env| {

solitaire_data/src/auth_tokens.rs

@@ -19,8 +19,9 @@
 //! `keyring-core` cannot compile for the android target (its `rpassword`
 //! transitive dep uses `libc::__errno_location`, which Android's bionic
 //! doesn't expose). On Android this module delegates to an Android Keystore
-//! JNI backend. `solitaire_app` must call `solitaire_data::init_android_jvm`
-//! from Android startup before token operations can succeed.
+//! JNI backend. `solitaire_app` must initialise the safe
+//! [`crate::android_jni`] bridge (via `set_jvm` / `set_activity`) from Android
+//! startup before token operations can succeed.
 //!
 //! # Note: no unit tests — requires live OS keychain.
 

solitaire_data/src/lib.rs

@@ -144,9 +144,10 @@ pub use settings::{
 };
 
 #[cfg(target_os = "android")]
-mod android_keystore;
+pub mod android_jni;
+
 #[cfg(target_os = "android")]
-pub use android_keystore::init_android_jvm;
+mod android_keystore;
 
 #[cfg(not(target_arch = "wasm32"))]
 pub mod auth_tokens;

solitaire_engine/src/android_clipboard.rs

@@ -1,42 +1,19 @@
-// JNI FFI requires `unsafe` to reconstruct `JavaVM` / `JObject` handles from
-// raw pointers handed over by the Android runtime. Scoped to this module so
-// the rest of the workspace stays `deny(unsafe_code)`.
-#![allow(unsafe_code)]
-
 /// Android clipboard bridge via JNI.
 ///
 /// Writes text to the system clipboard by calling into `ClipboardManager`
-/// through the JNI. Only compiled and linked on `target_os = "android"`.
+/// through the safe [`solitaire_data::android_jni`] bridge. Only compiled and
+/// linked on `target_os = "android"`.
 #[cfg(target_os = "android")]
 pub fn set_text(text: &str) -> Result<(), String> {
-    use bevy::android::ANDROID_APP;
-    use jni::{
-        JavaVM,
-        objects::{JObject, JValueOwned},
-    };
+    use jni::objects::JValueOwned;
+    use solitaire_data::android_jni;
 
-    let app = ANDROID_APP
-        .get()
-        .ok_or_else(|| "ANDROID_APP not initialized".to_string())?;
-
-    // SAFETY: vm_as_ptr() returns the raw JavaVM* set up by the Android runtime.
-    let vm = unsafe { JavaVM::from_raw(app.vm_as_ptr().cast()) }
-        .map_err(|e| format!("JavaVM::from_raw: {e}"))?;
-
-    let mut env = vm
-        .attach_current_thread_permanently()
-        .map_err(|e| format!("attach_current_thread: {e}"))?;
-
-    // SAFETY: activity_as_ptr() is the NativeActivity jobject pointer —
-    // valid for the lifetime of the process.
-    let activity = unsafe { JObject::from_raw(app.activity_as_ptr() as _) };
-
-    (|| -> jni::errors::Result<()> {
+    android_jni::with_activity_env(|env, activity| {
         // ClipboardManager cm = activity.getSystemService("clipboard")
         let svc_name = JValueOwned::from(env.new_string("clipboard")?);
         let cm = env
             .call_method(
-                &activity,
+                activity,
                 "getSystemService",
                 "(Ljava/lang/String;)Ljava/lang/Object;",
                 &[svc_name.borrow()],
@@ -65,6 +42,5 @@ pub fn set_text(text: &str) -> Result<(), String> {
             &[clip_val.borrow()],
         )?
         .v()
-    })()
-    .map_err(|e| format!("clipboard JNI: {e}"))
+    })
 }

solitaire_engine/src/safe_area.rs

@@ -1,10 +1,5 @@
 //! Safe-area insets.
 //!
-// JNI FFI (Android only) requires `unsafe` to reconstruct `JavaVM` /
-// `JObject` handles from raw pointers handed over by the runtime. Scoped to
-// this module so the rest of the workspace stays `deny(unsafe_code)`.
-#![allow(unsafe_code)]
-//!
 //! Reports the OS-reserved regions around the playable surface (status
 //! bar at the top, gesture / navigation bar at the bottom on Android,
 //! display cutouts, etc.) so UI anchored to a screen edge can avoid
@@ -296,30 +291,12 @@ mod android {
     }
 
     fn query_insets() -> Result<SafeAreaInsets, String> {
-        use bevy::android::ANDROID_APP;
-        use jni::{JavaVM, objects::JObject};
+        use solitaire_data::android_jni;
 
-        let app = ANDROID_APP
-            .get()
-            .ok_or_else(|| "ANDROID_APP not initialized".to_string())?;
-
-        // SAFETY: `vm_as_ptr()` returns the JavaVM* set up by the Android
-        // runtime; valid for the lifetime of the process.
-        let vm = unsafe { JavaVM::from_raw(app.vm_as_ptr().cast()) }
-            .map_err(|e| format!("JavaVM::from_raw: {e}"))?;
-
-        let mut env = vm
-            .attach_current_thread_permanently()
-            .map_err(|e| format!("attach_current_thread: {e}"))?;
-
-        // SAFETY: `activity_as_ptr()` returns the NativeActivity jobject
-        // pointer — valid for the lifetime of the process.
-        let activity = unsafe { JObject::from_raw(app.activity_as_ptr() as _) };
-
-        (|| -> jni::errors::Result<SafeAreaInsets> {
+        android_jni::with_activity_env(|env, activity| {
             // Window window = activity.getWindow();
             let window = env
-                .call_method(&activity, "getWindow", "()Landroid/view/Window;", &[])?
+                .call_method(activity, "getWindow", "()Landroid/view/Window;", &[])?
                 .l()?;
 
             // View decor = window.getDecorView();
@@ -371,8 +348,7 @@ mod android {
                 left,
                 right,
             })
-        })()
-        .map_err(|e| format!("safe-area JNI: {e}"))
+        })
     }
 }