fix(crypto): require a trusted cross-signature before showing a device as verified

.changeset/fix-device-verified-badge.md

@@ -0,0 +1,5 @@
+---
+default: patch
+---
+
+Fix devices showing as verified in Sable while other clients still see them as unverified

src-tauri/src/matrix_crypto/devices.rs

@@ -19,6 +19,8 @@ fn device_id(args: &Value, method: &str) -> Result<OwnedDeviceId, String> {
     Ok(str_arg(args, method, "deviceId")?.into())
 }
 
+pub(super) const IDENTITY_QUERY_WAIT: Duration = Duration::from_secs(1);
+
 fn timeout(args: &Value) -> Option<Duration> {
     args.get("timeoutSecs")
         .and_then(Value::as_f64)
@@ -139,7 +141,7 @@ async fn identity_for(
 ) -> Result<Option<UserIdentity>, String> {
     let user = user_id(args, method, "userId")?;
     machine
-        .get_identity(&user, timeout(args))
+        .get_identity(&user, timeout(args).or(Some(IDENTITY_QUERY_WAIT)))
         .await
         .map_err(|e| format!("{method} failed: {e}"))
 }
@@ -189,7 +191,7 @@ fn query_keys_for_users(machine: &OlmMachine, args: &Value, method: &str) -> Res
         "body": super::requests::keys_query_body(
             &request.timeout,
             &json!(request.device_keys),
-        ),
+        )?,
     }))
 }
 

src-tauri/src/matrix_crypto/dispatch.rs

@@ -132,7 +132,13 @@ fn processed_to_device_event_json(
         }
     };
 
-    let mut value = serde_json::to_value(snapshot).ok()?;
+    let mut value = match serde_json::to_value(snapshot) {
+        Ok(value) => value,
+        Err(e) => {
+            log::warn!("Dropping an incoming to-device event we could not serialize: {e}");
+            return None;
+        }
+    };
     if let Some(request) = verification_request {
         value["verificationRequest"] = request;
     }

src-tauri/src/matrix_crypto/mod.rs

@@ -173,7 +173,8 @@ fn store_dir(
 pub(super) static OPEN_GUARD: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(());
 
 /// Opens a store and registers its machine, replacing any machine already open for the
-/// account. Tauri-free so a cold push can open the same store without an `AppHandle`.
+/// account. Callers holding `OPEN_GUARD` themselves use [`open_machine_locked`].
+#[cfg(test)]
 pub async fn open_machine(
     dir: &Path,
     passphrase: Option<&str>,
@@ -244,16 +245,20 @@ pub async fn engine_open(
         None => store_dir(&app, &user_id, &device_id)?,
     };
 
-    let (machine, info) = open_machine(&dir, passphrase.as_deref(), &user_id, &device_id).await?;
-
     let account = account_key(&user_id, &device_id);
+
+    let guard = OPEN_GUARD.lock().await;
+    let (machine, info) =
+        open_machine_locked(&dir, passphrase.as_deref(), &user_id, &device_id).await?;
     let listeners = events::spawn(&app, &machine, account.clone());
-    if let Some(displaced) = engines()
+    let displaced = engines()
         .listeners
         .lock()
         .map_err(|e| e.to_string())?
-        .insert(account, listeners)
-    {
+        .insert(account, listeners);
+    drop(guard);
+
+    if let Some(displaced) = displaced {
         for handle in displaced {
             handle.abort();
         }

src-tauri/src/matrix_crypto/requests.rs

@@ -32,23 +32,23 @@ pub(super) struct KeysClaimBody<'a> {
 pub(super) fn keys_claim_body(
     timeout: &Option<std::time::Duration>,
     one_time_keys: &Value,
-) -> String {
+) -> Result<String, String> {
     serde_json::to_string(&KeysClaimBody {
         timeout,
         one_time_keys,
     })
-    .unwrap_or_default()
+    .map_err(|e| format!("serializing a /keys/claim body failed: {e}"))
 }
 
 pub(super) fn keys_query_body(
     timeout: &Option<std::time::Duration>,
     device_keys: &Value,
-) -> String {
+) -> Result<String, String> {
     serde_json::to_string(&KeysQueryBody {
         timeout,
         device_keys,
     })
-    .unwrap_or_default()
+    .map_err(|e| format!("serializing a /keys/query body failed: {e}"))
 }
 
 /// `device_keys` is omitted when absent; an explicit `null` is not the same thing.
@@ -93,12 +93,12 @@ pub async fn outgoing_requests(machine: &OlmMachine) -> Result<Value, String> {
                     AnyOutgoingRequest::KeysQuery(req) => json!({
                         "type": request_type::KEYS_QUERY,
                         "className": "KeysQueryRequest",
-                        "body": keys_query_body(&req.timeout, &json!(req.device_keys)),
+                        "body": keys_query_body(&req.timeout, &json!(req.device_keys))?,
                     }),
                     AnyOutgoingRequest::KeysClaim(req) => json!({
                         "type": request_type::KEYS_CLAIM,
                         "className": "KeysClaimRequest",
-                        "body": keys_claim_body(&req.timeout, &json!(req.one_time_keys)),
+                        "body": keys_claim_body(&req.timeout, &json!(req.one_time_keys))?,
                     }),
                     AnyOutgoingRequest::ToDeviceRequest(req) => json!({
                         "type": request_type::TO_DEVICE,
@@ -122,9 +122,9 @@ pub async fn outgoing_requests(machine: &OlmMachine) -> Result<Value, String> {
                     }),
                 };
                 entry["id"] = Value::String(id);
-                entry
+                Ok(entry)
             })
-            .collect(),
+            .collect::<Result<Vec<_>, String>>()?,
     ))
 }
 

src-tauri/src/matrix_crypto/rooms.rs

@@ -97,8 +97,10 @@ fn collect_strategy(value: Option<&Value>, method: &str) -> Result<CollectStrate
                 Ok(CollectStrategy::OnlyTrustedDevices)
             } else if flag("errorOnVerifiedUserProblem") {
                 Ok(CollectStrategy::ErrorOnVerifiedUserProblem)
-            } else {
+            } else if flag("allDevices") {
                 Ok(CollectStrategy::AllDevices)
+            } else {
+                Err(format!("{method}: unknown sharing strategy {value:?}"))
             }
         }
         Some(_) => Err(format!(
@@ -268,15 +270,18 @@ pub async fn invoke(
                 .get_missing_sessions(users.iter().map(AsRef::as_ref))
                 .await
             {
-                Ok(Some((txn_id, request))) => Ok(json!({
-                    "type": request_type::KEYS_CLAIM,
-                    "className": "KeysClaimRequest",
-                    "id": txn_id.to_string(),
-                    "body": super::requests::keys_claim_body(
-                        &request.timeout,
-                        &json!(request.one_time_keys),
-                    ),
-                })),
+                Ok(Some((txn_id, request))) => super::requests::keys_claim_body(
+                    &request.timeout,
+                    &json!(request.one_time_keys),
+                )
+                .map(|body| {
+                    json!({
+                        "type": request_type::KEYS_CLAIM,
+                        "className": "KeysClaimRequest",
+                        "id": txn_id.to_string(),
+                        "body": body,
+                    })
+                }),
                 Ok(None) => Ok(Value::Null),
                 Err(e) => Err(format!("getMissingSessions failed: {e}")),
             }

src-tauri/src/matrix_crypto/verification.rs

@@ -77,7 +77,7 @@ async fn identity(
 ) -> Result<UserIdentity, String> {
     let user = user_arg(args, method)?;
     machine
-        .get_identity(&user, None)
+        .get_identity(&user, Some(super::devices::IDENTITY_QUERY_WAIT))
         .await
         .map_err(|e| format!("{method}: cannot load the identity of {user}: {e}"))?
         .ok_or_else(|| format!("{method}: no cross-signing identity for {user}"))
@@ -466,25 +466,12 @@ pub async fn invoke(
                 .map(verification_state)
                 .unwrap_or(Value::Null))
         }
-        "verificationRequest.accept" => {
-            request(machine, args, method).and_then(|request| {
-                match args.get("methods").and_then(Value::as_array) {
-                    Some(codes) => {
-                        let methods = codes
-                            .iter()
-                            .filter_map(Value::as_u64)
-                            .map(|code| {
-                                method_from_code(code).ok_or_else(|| {
-                                    format!("{method}: unknown verification method {code}")
-                                })
-                            })
-                            .collect::<Result<Vec<_>, _>>()?;
-                        Ok(optional_outgoing(request.accept_with_methods(methods)))
-                    }
-                    None => Ok(optional_outgoing(request.accept())),
-                }
+        "verificationRequest.accept" => request(machine, args, method).and_then(|request| {
+            Ok(match methods_arg(args, method)? {
+                Some(methods) => optional_outgoing(request.accept_with_methods(methods)),
+                None => optional_outgoing(request.accept()),
             })
-        }
+        }),
         "verificationRequest.cancel" => {
             request(machine, args, method).map(|request| optional_outgoing(request.cancel()))
         }

src/app/components/BackupRestore.test.tsx

@@ -107,7 +107,6 @@ describe('BackupRestoreTile', () => {
     );
     expect(crypto.processDeviceLists).toHaveBeenCalledWith({ changed: ['@me:example.org'] });
     expect(crypto.bootstrapCrossSigning).toHaveBeenCalled();
-    expect(crypto.setDeviceVerified).toHaveBeenCalledWith('@me:example.org', 'DEVICE');
     expect(crypto.loadSessionBackupPrivateKeyFromSecretStorage).toHaveBeenCalled();
   });
 

src/app/components/BackupRestore.tsx

@@ -73,12 +73,6 @@ function BackupKeyRecovery({
         await cryptoBackend.bootstrapCrossSigning({});
         await cryptoBackend.bootstrapSecretStorage({});
 
-        const deviceId = mx.getDeviceId();
-        if (!deviceId) {
-          throw new Error('Unexpected Error! Current device ID not found.');
-        }
-        await cryptoBackend.setDeviceVerified(mx.getSafeUserId(), deviceId);
-
         // Emits KeyBackupDecryptionKeyCached, which drives the restore.
         await crypto.loadSessionBackupPrivateKeyFromSecretStorage();
       },

src/app/components/ManualVerification.test.tsx

@@ -11,7 +11,6 @@ const storePrivateKey = vi.hoisted(() => vi.fn<() => void>());
 const processDeviceLists = vi.hoisted(() => vi.fn<() => Promise<void>>());
 const bootstrapCrossSigning = vi.hoisted(() => vi.fn<() => Promise<void>>());
 const bootstrapSecretStorage = vi.hoisted(() => vi.fn<() => Promise<void>>());
-const setDeviceVerified = vi.hoisted(() => vi.fn<() => Promise<void>>());
 const loadSessionBackupPrivateKeyFromSecretStorage = vi.hoisted(() => vi.fn<() => Promise<void>>());
 
 vi.mock('$types/matrix-sdk', () => ({ decodeRecoveryKey }));
@@ -26,7 +25,6 @@ vi.mock('$hooks/useMatrixClient', () => ({
         processDeviceLists,
         bootstrapCrossSigning,
         bootstrapSecretStorage,
-        setDeviceVerified,
         loadSessionBackupPrivateKeyFromSecretStorage,
       }) as unknown as CryptoApi,
   }),
@@ -59,7 +57,6 @@ describe('ManualVerificationTile', () => {
     processDeviceLists.mockResolvedValue(undefined);
     bootstrapCrossSigning.mockResolvedValue(undefined);
     bootstrapSecretStorage.mockResolvedValue(undefined);
-    setDeviceVerified.mockResolvedValue(undefined);
     loadSessionBackupPrivateKeyFromSecretStorage.mockResolvedValue(undefined);
   });
 
@@ -72,8 +69,9 @@ describe('ManualVerificationTile', () => {
     expect(storePrivateKey).toHaveBeenCalledWith(KEY_ID, recoveryKey);
     expect(processDeviceLists).toHaveBeenCalledWith({ changed: ['@me:example.org'] });
     expect(bootstrapCrossSigning).toHaveBeenCalledAfter(processDeviceLists);
-    expect(setDeviceVerified).toHaveBeenCalledWith('@me:example.org', 'DEVICE');
-    expect(setDeviceVerified).toHaveBeenCalledAfter(bootstrapCrossSigning);
+    expect(loadSessionBackupPrivateKeyFromSecretStorage).toHaveBeenCalledAfter(
+      bootstrapCrossSigning
+    );
   });
 
   it('invalidates the cached verification status after bootstrapping', async () => {

src/app/components/ManualVerification.tsx

@@ -133,12 +133,6 @@ export function ManualVerificationTile({
       await crypto.bootstrapCrossSigning({});
       await crypto.bootstrapSecretStorage({});
 
-      const deviceId = mx.getDeviceId();
-      if (!deviceId) {
-        throw new Error('Unexpected Error! Current device ID not found.');
-      }
-      await crypto.setDeviceVerified(mx.getSafeUserId(), deviceId);
-
       await crypto.loadSessionBackupPrivateKeyFromSecretStorage();
 
       refreshVerificationStatus();

src/app/crypto/engineCrypto/EngineCrypto.ts

@@ -129,7 +129,7 @@
 
   const entries = Object.entries(value as Record<string, unknown>)
     .filter(([, item]) => item !== undefined)
     .sort(([a], [b]) => (a < b ? -1 : 1));
   return `{${entries.map(([key, item]) => `${JSON.stringify(key)}:${canonicalJson(item)}`).join(',')}}`;
 };
 
@@ -1555,11 +1555,19 @@
     });
   }
 
+  async #queryOwnKeys(): Promise<void> {
+    await this.#sendTracked(
+      await this.#call('queryKeysForUsers', { users: [this.#identity.userId] })
+    );
+    await this.#flushOutgoingRequests();
+  }
+
   async crossSignDevice(deviceId: string): Promise<void> {
     await this.#sendTracked(
       await this.#call('device.verify', { userId: this.#identity.userId, deviceId })
     );
     await this.#flushOutgoingRequests();
+    await this.#queryOwnKeys();
   }
 
   async isCrossSigningReady(): Promise<boolean> {
@@ -1621,6 +1629,8 @@
   }
 
   async #importCrossSigningKeys(keys: Record<string, string>): Promise<void> {
+    await this.#queryOwnKeys();
+
     const status = (await this.#call('importCrossSigningKeys', keys)) as {
       hasMaster: boolean;
       hasSelfSigning: boolean;
@@ -1634,7 +1644,10 @@
       userId: this.#identity.userId,
       deviceId: this.#identity.deviceId,
     })) as OutgoingRequest | null;
-    if (request) await sendOutgoingRequest(this.#mx, request);
+    if (request) {
+      await sendOutgoingRequest(this.#mx, request);
+      await this.#queryOwnKeys();
+    }
   }
 
   async #exportCrossSigningKeysToStorage(): Promise<void> {

src/app/crypto/engineCrypto/bootstrapCrossSigning.test.ts

@@ -88,6 +88,44 @@ describe('bootstrapCrossSigning', () => {
     expect(requestsTo(authedRequest, '/_matrix/client/v3/keys/signatures/upload')).toHaveLength(1);
   });
 
+  it('refreshes the published keys on both sides of a secret storage import', async () => {
+    engine((method) => {
+      if (method === 'crossSigningStatus') return noKeys;
+      if (method === 'queryKeysForUsers')
+        return { id: 'q1', type: 1, className: 'KeysQueryRequest', body: '{}' };
+      if (method === 'importCrossSigningKeys') return allKeys;
+      if (method === 'device.verify') return { id: null, type: 4, body: '{}' };
+      return undefined;
+    });
+    const { mx, authedRequest } = clientStub({ storage: STORED_KEYS });
+
+    await new EngineCrypto(mx, { userId: '@me:e.org', deviceId: 'D' }).bootstrapCrossSigning({});
+
+    const methods = mockInvoke.mock.calls.map(([, name]) => name);
+    expect(methods.indexOf('queryKeysForUsers')).toBeLessThan(
+      methods.indexOf('importCrossSigningKeys')
+    );
+    expect(methods.lastIndexOf('queryKeysForUsers')).toBeGreaterThan(
+      methods.indexOf('device.verify')
+    );
+    expect(requestsTo(authedRequest, '/_matrix/client/v3/keys/query')).toHaveLength(2);
+  });
+
+  it('refreshes the published keys after blindly cross-signing a device', async () => {
+    engine((method) => {
+      if (method === 'queryKeysForUsers')
+        return { id: 'q1', type: 1, className: 'KeysQueryRequest', body: '{}' };
+      if (method === 'device.verify') return { id: null, type: 4, body: '{}' };
+      return undefined;
+    });
+    const { mx, authedRequest } = clientStub();
+
+    await new EngineCrypto(mx, { userId: '@me:e.org', deviceId: 'D' }).crossSignDevice('OTHER');
+
+    expect(requestsTo(authedRequest, '/_matrix/client/v3/keys/signatures/upload')).toHaveLength(1);
+    expect(requestsTo(authedRequest, '/_matrix/client/v3/keys/query')).toHaveLength(1);
+  });
+
   it('refuses a secret storage import the engine did not actually apply', async () => {
     engine((method) => {
       if (method === 'crossSigningStatus') return noKeys;

src/app/crypto/verification/verifier.ts

@@ -122,7 +122,7 @@ abstract class EngineVerifier<TState>
   private finishCancelled(flow: Record<string, unknown>, error: Error): void {
     if (this.hasBeenCancelled) return;
     this.markCancelled();
-    void this.call(this.cancelMethod, flow);
+    void this.call(this.cancelMethod, flow).catch(() => undefined);
     this.completion.reject(error);
     this.emit(VerifierEvent.Cancel, error);
   }