fix(client): keep zone keys out of the debug output of a snapshot

crates/orbyn-client/src/lib.rs

@@ -360,8 +360,9 @@ pub struct RefusedEnvelope {
 }
 
 /// The engine's state for the host to store. It holds zone keys: a host must
-/// seal it before it leaves the process (`orbyn-wasm` does).
-#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)]
+/// seal it before it leaves the process (`orbyn-wasm` does). Its `Debug` prints the account, the
+/// device and the number of zones, and never a key or a record.
+#[derive(Clone, Default, PartialEq, Serialize, Deserialize)]
 pub struct Snapshot {
     pub account_id: String,
     pub device_id: String,
@@ -386,7 +387,9 @@ pub struct Snapshot {
     pub zones: BTreeMap<String, ZoneSnapshot>,
 }
 
-#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)]
+/// One zone of a [`Snapshot`]: its keys (base64url), its records and its upload state. Its
+/// `Debug` prints counts only, never a key or a record.
+#[derive(Clone, Default, PartialEq, Serialize, Deserialize)]
 pub struct ZoneSnapshot {
     /// Key id to key, both base64url.
     pub keys: BTreeMap<String, String>,
@@ -406,6 +409,25 @@ pub struct ZoneSnapshot {
     refetch_restarts: u8,
 }
 
+impl std::fmt::Debug for Snapshot {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("Snapshot")
+            .field("account_id", &self.account_id)
+            .field("device_id", &self.device_id)
+            .field("zones", &self.zones.len())
+            .finish_non_exhaustive()
+    }
+}
+
+impl std::fmt::Debug for ZoneSnapshot {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("ZoneSnapshot")
+            .field("keys", &self.keys.len())
+            .field("records", &self.records.len())
+            .finish_non_exhaustive()
+    }
+}
+
 /// One device's sync engine.
 pub struct Engine {
     keys: DeviceKeys,

crates/orbyn-client/tests/snapshot_debug.rs

@@ -0,0 +1,97 @@
+//! A snapshot holds zone keys and records. Printing it with `{:?}` or `{:#?}` must never show
+//! either, in any encoding (the app's threat model, control 4).
+
+use base64::Engine as _;
+use orbyn_client::{DeviceKeys, Engine, Fields, Snapshot};
+use serde_json::json;
+
+const ZONE: &str = "inbox";
+const SECRET_NOTE: &str = "a private note that must not be printed";
+
+/// Every encoding a key could be printed in.
+fn encodings(key: &[u8]) -> Vec<String> {
+    let hex: String = key.iter().map(|b| format!("{b:02x}")).collect();
+    let upper = hex.to_uppercase();
+    let std = base64::engine::general_purpose::STANDARD.encode(key);
+    let std_nopad = base64::engine::general_purpose::STANDARD_NO_PAD.encode(key);
+    let url = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(key);
+    let url_pad = base64::engine::general_purpose::URL_SAFE.encode(key);
+    let decimals: Vec<String> = key.iter().map(u8::to_string).collect();
+    let array = format!("[{}]", decimals.join(", "));
+    let pretty = format!("[\n    {},\n]", decimals.join(",\n    "));
+    let compact = decimals.join(",");
+    vec![
+        hex, upper, std, std_nopad, url, url_pad, array, pretty, compact,
+    ]
+}
+
+fn snapshot_with_a_key_and_a_record() -> (Snapshot, Vec<u8>) {
+    let mut engine = Engine::new(
+        DeviceKeys::generate().expect("keys"),
+        "0a0a0a0a-0000-4000-8000-000000000001",
+        "0000000a-0000-4000-8000-00000000000a",
+    )
+    .expect("engine");
+    let key_id = engine.create_zone_key(ZONE).expect("zone key");
+    let stamp = engine.tick(1_700_000_000_000);
+    let mut fields = Fields::default();
+    fields.set("note", json!(SECRET_NOTE), stamp);
+    engine
+        .stage(ZONE, "person_settings", "person", &fields)
+        .expect("stage");
+    let snapshot = engine.snapshot();
+    // The key as the snapshot stores it (base64url), decoded to its bytes.
+    let stored = &snapshot.zones[ZONE].keys[&key_id];
+    let bytes = base64::engine::general_purpose::URL_SAFE_NO_PAD
+        .decode(stored)
+        .expect("base64url");
+    assert_eq!(bytes.len(), 32);
+    (snapshot, bytes)
+}
+
+#[test]
+fn debug_of_a_snapshot_never_shows_a_zone_key_or_a_record() {
+    let (snapshot, key) = snapshot_with_a_key_and_a_record();
+    let zone = &snapshot.zones[ZONE];
+    let printed = [
+        format!("{snapshot:?}"),
+        format!("{snapshot:#?}"),
+        format!("{:?}", Some(&snapshot)),
+        format!("{:#?}", Some(snapshot.clone())),
+        format!("{zone:?}"),
+        format!("{zone:#?}"),
+        format!("{:?}", snapshot.zones),
+        format!("{:#?}", snapshot.zones),
+    ];
+    for text in &printed {
+        for encoding in encodings(&key) {
+            assert!(
+                !text.contains(&encoding),
+                "a key shows as {encoding}: {text}"
+            );
+        }
+        assert!(!text.contains(SECRET_NOTE), "a record shows: {text}");
+    }
+    // It still says whose snapshot it is and how big.
+    let text = format!("{snapshot:?}");
+    assert!(
+        text.contains("0a0a0a0a-0000-4000-8000-000000000001"),
+        "{text}"
+    );
+    assert!(
+        text.contains("0000000a-0000-4000-8000-00000000000a"),
+        "{text}"
+    );
+    assert!(text.contains("zones: 1"), "{text}");
+}
+
+#[test]
+fn the_check_finds_a_key_when_one_is_printed() {
+    // A derived Debug of the stored map would print the key: the encodings above catch it.
+    let (snapshot, key) = snapshot_with_a_key_and_a_record();
+    let leaked = format!("{:?}", snapshot.zones[ZONE].keys);
+    assert!(
+        encodings(&key).iter().any(|e| leaked.contains(e)),
+        "the encodings must match what a derive would print: {leaked}"
+    );
+}

crates/orbyn-device-client/src/client.rs

@@ -4,6 +4,7 @@
 use crate::api::{DeviceApi, Method, Request, Response};
 use crate::clock::Clock;
 use crate::error::DeviceError;
+use crate::password::Password;
 use crate::status::SyncStatus;
 use crate::store::{KeyStore, StoredIdentity, StoredSession};
 use base64::Engine as _;
@@ -40,9 +41,6 @@ struct SecondFactorBody<'a> {
     recovery_code: Option<&'a str>,
 }
 
-/// A password. Wiped when dropped.
-pub type Password = Zeroizing<String>;
-
 /// What the host says about this device.
 #[derive(Clone, Debug)]
 pub struct ClientConfig {

crates/orbyn-device-client/src/lib.rs

@@ -9,6 +9,7 @@ mod client;
 mod clock;
 mod error;
 mod notices;
+mod password;
 mod signup;
 mod status;
 mod store;
@@ -17,12 +18,13 @@ mod sync;
 pub use api::{DeviceApi, HttpApi, Method, Request, Response, is_loopback_literal};
 pub use approvals::{ApproverApproval, ApproverState, JoinedApproval};
 pub use client::{
-    ClientConfig, DeviceClient, Password, SecondFactor, SecondFactorChallenge, SessionInfo,
-    SignInOutcome, SignUpRequest,
+    ClientConfig, DeviceClient, SecondFactor, SecondFactorChallenge, SessionInfo, SignInOutcome,
+    SignUpRequest,
 };
 pub use clock::{Clock, SystemClock};
 pub use error::{DeviceError, StoreError};
 pub use notices::{Notice, NoticeConfig, Notices};
+pub use password::Password;
 pub use signup::PendingSignUp;
 pub use status::{SyncState, SyncStatus};
 pub use store::{KeyStore, MemoryStore, StoredIdentity, StoredSession};

crates/orbyn-device-client/src/password.rs

@@ -0,0 +1,38 @@
+//! The password type.
+
+use zeroize::Zeroizing;
+
+/// A password. Wiped when dropped, and its `Debug` prints `Password(..)` whatever version of
+/// `zeroize` a build resolves (older ones print the contents of a bare `Zeroizing<String>`).
+#[derive(Clone)]
+pub struct Password(Zeroizing<String>);
+
+impl Password {
+    pub fn new(text: impl Into<String>) -> Self {
+        Self(Zeroizing::new(text.into()))
+    }
+
+    pub fn as_str(&self) -> &str {
+        &self.0
+    }
+
+    pub fn as_bytes(&self) -> &[u8] {
+        self.0.as_bytes()
+    }
+
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
+impl From<String> for Password {
+    fn from(text: String) -> Self {
+        Self::new(text)
+    }
+}
+
+impl std::fmt::Debug for Password {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.write_str("Password(..)")
+    }
+}

crates/orbyn-device-client/tests/client.rs

@@ -6,7 +6,7 @@
 use orbyn_contracts::{DeviceApp, DevicePlatform, DeviceState};
 use orbyn_device_client::{
     ClientConfig, Clock, DeviceApi, DeviceClient, DeviceError, HttpApi, KeyStore, MemoryStore,
-    Method, Request, Response, SecondFactor, SignInOutcome, SignUpRequest, StoreError,
+    Method, Password, Request, Response, SecondFactor, SignInOutcome, SignUpRequest, StoreError,
     StoredIdentity,
 };
 use std::sync::Arc;
@@ -42,8 +42,8 @@
     )
 }
 
-fn password(text: &str) -> Zeroizing<String> {
-    Zeroizing::new(text.to_owned())
+fn password(text: &str) -> Password {
+    Password::new(text)
 }
 
 fn request(invite: Option<&str>) -> SignUpRequest {
@@ -214,13 +214,19 @@
         "nothing is kept before the factor"
     );
     assert_eq!(
-        b.complete_second_factor(&challenge, SecondFactor::Code(password("000000")))
-            .await
-            .err(),
+        b.complete_second_factor(
+            &challenge,
+            SecondFactor::Code(Zeroizing::new("000000".to_owned()))
+        )
+        .await
+        .err(),
         Some(DeviceError::SecondFactorFailed)
     );
     let outcome = b
-        .complete_second_factor(&challenge, SecondFactor::Code(password("123456")))
+        .complete_second_factor(
+            &challenge,
+            SecondFactor::Code(Zeroizing::new("123456".to_owned())),
+        )
         .await
         .expect("the right code");
     assert!(
@@ -633,6 +639,26 @@
     )
 }
 
+#[test]
+fn debug_of_a_password_never_shows_the_text() {
+    let secret = "correct horse battery staple";
+    let password = Password::new(secret);
+    for text in [
+        format!("{password:?}"),
+        format!("{password:#?}"),
+        format!("{:?}", Some(&password)),
+        format!("{:?}", vec![password.clone()]),
+    ] {
+        assert!(!text.contains(secret), "{text}");
+        assert!(!text.contains("horse"), "{text}");
+        assert!(text.contains("Password(..)"), "{text}");
+    }
+    // It still works as a password.
+    assert_eq!(password.as_str(), secret);
+    assert_eq!(password.as_bytes(), secret.as_bytes());
+    assert!(!password.is_empty() && Password::from(String::new()).is_empty());
+}
+
 #[tokio::test(flavor = "multi_thread")]
 async fn a_failed_save_after_the_code_is_spent_keeps_the_keys_and_retry_save_finishes() {
     let engine = FakeEngine::new();

crates/orbyn-device-client/tests/local_stack.rs

@@ -12,12 +12,11 @@
 
 use orbyn_contracts::{DeviceApp, DevicePlatform, DeviceState};
 use orbyn_device_client::{
-    ClientConfig, DeviceClient, DeviceError, HttpApi, MemoryStore, SignInOutcome, SignUpRequest,
-    SystemClock,
+    ClientConfig, DeviceClient, DeviceError, HttpApi, MemoryStore, Password, SignInOutcome,
+    SignUpRequest, SystemClock,
 };
 use std::process::Command;
 use std::time::Duration;
-use zeroize::Zeroizing;
 
 fn script(args: &[&str]) -> String {
     let path = concat!(env!("CARGO_MANIFEST_DIR"), "/../../scripts/local.sh");
@@ -51,7 +50,7 @@ async fn sign_up_second_device_re_login_and_sign_out_everywhere() {
         eprintln!("skipped: set ORBYN_LOCAL_STACK=1 with the local stack up (scripts/local.sh up)");
         return;
     }
-    let password = Zeroizing::new("correct horse battery staple".to_owned());
+    let password = Password::new("correct horse battery staple");
     let email = format!("e2e-{}@example.test", uuid::Uuid::new_v4().simple());
     let invite = script(&["invite-code", "device-client"]);
     assert_eq!(invite.len(), 19, "an invite code: {invite:?}");
@@ -94,7 +93,7 @@ async fn sign_up_second_device_re_login_and_sign_out_everywhere() {
     let third = client("Third");
     assert_eq!(
         third
-            .sign_in(&email, &Zeroizing::new("wrong horse".to_owned()))
+            .sign_in(&email, &Password::new("wrong horse"))
             .await
             .err(),
         Some(DeviceError::LoginFailed)

crates/orbyn-device-client/tests/single_crate.rs

@@ -4,9 +4,9 @@
 
 use orbyn_device_client::{
     Applied, ClientConfig, ClientError, Device, DeviceApp, DeviceKeys, DeviceList, DevicePlatform,
-    DevicePublic, DeviceRemoval, DeviceState, Engine, Fields, KeyStore, Local, MemoryStore, Peer,
-    PendingApproval, Quarantined, RefusedEnvelope, RejectedFields, Snapshot, Stamp, StoredIdentity,
-    SyncReport, Trashed, Zeroizing, ZoneReport,
+    DevicePublic, DeviceRemoval, DeviceState, Engine, Fields, KeyStore, Local, MemoryStore,
+    Password, Peer, PendingApproval, Quarantined, RefusedEnvelope, RejectedFields, Snapshot, Stamp,
+    StoredIdentity, SyncReport, Trashed, Zeroizing, ZoneReport,
 };
 
 /// A host's `ClientConfig` is written with the re-exported platform and app types.
@@ -87,6 +87,12 @@ fn a_host_drives_the_engine_and_a_key_store_with_the_re_exported_types() {
     assert!(store.load().expect("load").is_some());
 }
 
+#[test]
+fn a_host_builds_a_password_from_this_crate_alone() {
+    let password = Password::new("hunter2 but longer");
+    assert!(!format!("{password:?}").contains("hunter2"));
+}
+
 #[test]
 fn this_file_never_names_another_engine_crate() {
     let source = include_str!("single_crate.rs");

crates/orbyn-server/tests/device_client.rs

@@ -9,7 +9,8 @@ use orbyn_core::accounts::{AuthSecrets, Limit, Limits};
 use orbyn_core::{Accounts, Database};
 use orbyn_device_client::{
     ApproverState, ClientConfig, DeviceApi, DeviceClient, DeviceError, HttpApi, KeyStore,
-    MemoryStore, Notice, NoticeConfig, SignInOutcome, SignUpRequest, SyncState, SystemClock,
+    MemoryStore, Notice, NoticeConfig, Password, SignInOutcome, SignUpRequest, SyncState,
+    SystemClock,
 };
 use orbyn_server::{AppState, Readiness, router};
 use orbyn_store::{Hub, Store};
@@ -113,8 +114,8 @@ impl Server {
 
 type Client = DeviceClient<HttpApi, MemoryStore, SystemClock>;
 
-fn password() -> Zeroizing<String> {
-    Zeroizing::new(PASSWORD.to_owned())
+fn password() -> Password {
+    Password::new(PASSWORD)
 }
 
 fn email() -> String {

docs/device-client.md

@@ -131,12 +131,16 @@ The app may not depend on or name `orbyn-contracts` (rule D6). Every type of ano
 | `orbyn-client` | `Engine`, `Snapshot`, `Applied`, `RejectedFields`, `RefusedEnvelope`, `ClientError`, `Peer`, `Local`, `Quarantined`, `Trashed` | `open_engine`, `sync_all`, `SyncReport` and the engine's own return types |
 | `orbyn-sync` (through `orbyn-client`) | `Fields`, `Stamp` | staging and reading records |
 | `orbyn-crypto` (through `orbyn-client`) | `DeviceKeys`, `DevicePublic` | `StoredIdentity::keys()`, device keys |
-| `zeroize` | `Zeroizing` | `Password`, `StoredIdentity` |
+| `zeroize` | `Zeroizing` | `StoredIdentity`, `SecondFactor` |
 
 `orbyn-client` re-exports, besides those, the other contract types its `Engine` methods take or return (`ZoneList`, `UploadChanges`, `UploadResult`, `RemoveDevice`, `DeleteZone`, `KeyEnvelope`, `NewKeyEnvelope`, `KeyRotation`, `ApprovalNonce`, `CreateApproval`, `ConfirmApproval`, `ProtectionChain`, `ProtectionLink`, `ProtectionLevel`, `SetProtection`, `Record`, `RecordChanged`, `ZoneChanges`).
 
 Left out on purpose: the planner's `Item` and `ItemError` (planner only); the request and response bodies this crate sends and reads internally (`RegisterFinish`, `EmailVerify` and the like) and `orbyn-auth-client`, which no public signature uses; `ApproverPairing` and `DevicePairing`, which `ApproverApproval` and `JoinedApproval` wrap. `DeviceError` carries only strings, numbers and a duration. `tests/single_crate.rs` here and `orbyn-client/tests/reexports.rs` use these types through the one crate, and a check fails if the first names another Engine crate.
 
+### Debug prints no secret
+
+A type that can hold secret material has a `Debug` that prints none: `Snapshot` (account, device and the number of zones) and `ZoneSnapshot` (counts) are manual, as are `StoredIdentity`, `StoredSession`, `SecondFactorChallenge`, `PendingSignUp`, `SymmetricKey` and `RecoveryKey`; `DeviceKeys`, `Engine`, `ApproverPairing`, `DevicePairing`, `SecondFactor`, `ApproverApproval` and `JoinedApproval` have no `Debug` at all. `Password` is **its own type** (`Password::new(text)`, `as_str`, `as_bytes`, `is_empty`, `From<String>`; wiped on drop) whose `Debug` prints `Password(..)` and nothing else, so it does not depend on the `zeroize` version a build resolves: `zeroize` 1.9 prints `Zeroizing { .. }` for a bare `Zeroizing<String>`, but 1.8 derives `Debug` and prints the contents. A host must still not format a bare `Zeroizing<String>` (the second-factor code inside `SecondFactor::Code` is one, and `SecondFactor` has no `Debug`). `orbyn-client/tests/snapshot_debug.rs` checks that `{:?}` and `{:#?}` of a snapshot show neither a zone key (hex, base64 or an array) nor a record, and `tests/client.rs` that a `Password` never shows its text. `Local`, `Trashed` and `Quarantined` show the record they cache by design: it is content, not key material.
+
 ### Tests
 
 `orbyn-server/tests/device_client.rs` runs the client over a real loopback socket against the real server on Postgres: two devices, the zone key, a sealed record that the server holds only as ciphertext, a conflict merged both ways, the snapshot round trip, a pending device refused, the whole approval through the client API (digits compared, confirmed and finished), a deny from either side, wrong digits typed, a relabelled listing refused by the server's commitment check, a removal and the last device that stays, offline work kept and sent later, and the notices stream hearing another device's change and being rejected when the session is revoked. `tests/notices.rs` serves the stream by hand to check the sign-in message, the pong, the reconnect and the stop.