From b806efa0965e4f06ca8b8572589efca9d8e54508 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Mon, 27 Apr 2026 11:22:21 +0200 Subject: [PATCH] refactor: Make `Fingerprint` not implement `Display` (#8177) Currently, the Fingerprint type implements Display, but this doesn't get you the canonical fingerprint representation, but something human-readable. This is confusing, and back when I first used `Fingerprint`, I immediately wrote a bug because of this. So, instead, make a function `human_readable()` on Fingerprint. This comes from the discussion at https://github.com/chatmail/core/pull/8174#discussion_r3143130722. --- deltachat-jsonrpc/src/api/types/qr.rs | 18 +++++++------- src/chat.rs | 3 ++- src/contact.rs | 4 +-- src/key.rs | 35 +++++++++++++-------------- src/qr.rs | 2 +- src/qr/qr_tests.rs | 2 +- src/securejoin.rs | 3 ++- 7 files changed, 34 insertions(+), 33 deletions(-) diff --git a/deltachat-jsonrpc/src/api/types/qr.rs b/deltachat-jsonrpc/src/api/types/qr.rs index e3b055306..6a4f4cd7b 100644 --- a/deltachat-jsonrpc/src/api/types/qr.rs +++ b/deltachat-jsonrpc/src/api/types/qr.rs @@ -238,7 +238,7 @@ impl From for QrObject { is_v3, } => { let contact_id = contact_id.to_u32(); - let fingerprint = fingerprint.to_string(); + let fingerprint = fingerprint.human_readable(); QrObject::AskVerifyContact { contact_id, fingerprint, @@ -257,7 +257,7 @@ impl From for QrObject { is_v3, } => { let contact_id = contact_id.to_u32(); - let fingerprint = fingerprint.to_string(); + let fingerprint = fingerprint.human_readable(); QrObject::AskVerifyGroup { grpname, grpid, @@ -278,7 +278,7 @@ impl From for QrObject { is_v3, } => { let contact_id = contact_id.to_u32(); - let fingerprint = fingerprint.to_string(); + let fingerprint = fingerprint.human_readable(); QrObject::AskJoinBroadcast { name, grpid, @@ -321,7 +321,7 @@ impl From for QrObject { authcode, } => { let contact_id = contact_id.to_u32(); - let fingerprint = fingerprint.to_string(); + let fingerprint = fingerprint.human_readable(); QrObject::WithdrawVerifyContact { contact_id, fingerprint, @@ -338,7 +338,7 @@ impl From for QrObject { authcode, } => { let contact_id = contact_id.to_u32(); - let fingerprint = fingerprint.to_string(); + let fingerprint = fingerprint.human_readable(); QrObject::WithdrawVerifyGroup { grpname, grpid, @@ -357,7 +357,7 @@ impl From for QrObject { authcode, } => { let contact_id = contact_id.to_u32(); - let fingerprint = fingerprint.to_string(); + let fingerprint = fingerprint.human_readable(); QrObject::WithdrawJoinBroadcast { name, grpid, @@ -374,7 +374,7 @@ impl From for QrObject { authcode, } => { let contact_id = contact_id.to_u32(); - let fingerprint = fingerprint.to_string(); + let fingerprint = fingerprint.human_readable(); QrObject::ReviveVerifyContact { contact_id, fingerprint, @@ -391,7 +391,7 @@ impl From for QrObject { authcode, } => { let contact_id = contact_id.to_u32(); - let fingerprint = fingerprint.to_string(); + let fingerprint = fingerprint.human_readable(); QrObject::ReviveVerifyGroup { grpname, grpid, @@ -410,7 +410,7 @@ impl From for QrObject { authcode, } => { let contact_id = contact_id.to_u32(); - let fingerprint = fingerprint.to_string(); + let fingerprint = fingerprint.human_readable(); QrObject::ReviveJoinBroadcast { name, grpid, diff --git a/src/chat.rs b/src/chat.rs index 2fd8b000a..51090c6de 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -1210,7 +1210,8 @@ SELECT id, rfc724_mid, pre_rfc724_mid, timestamp, ?, 1 FROM msgs WHERE chat_id=? ); let fingerprint = contact .fingerprint() - .context("Contact does not have a fingerprint in encrypted chat")?; + .context("Contact does not have a fingerprint in encrypted chat")? + .human_readable(); if let Some(public_key) = contact.public_key(context).await? { if let Some(relay_addrs) = addresses_from_public_key(&public_key) { let relays = relay_addrs.join(","); diff --git a/src/contact.rs b/src/contact.rs index 976fedd3e..310600c96 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -1396,7 +1396,7 @@ WHERE addr=? let Some(fingerprint_other) = contact.fingerprint() else { return Ok(stock_str::encr_none(context)); }; - let fingerprint_other = fingerprint_other.to_string(); + let fingerprint_other = fingerprint_other.human_readable(); let stock_message = if contact.public_key(context).await?.is_some() { stock_str::messages_are_e2ee(context) @@ -1410,7 +1410,7 @@ WHERE addr=? let fingerprint_self = load_self_public_key(context) .await? .dc_fingerprint() - .to_string(); + .human_readable(); if addr < contact.addr { cat_fingerprint( &mut ret, diff --git a/src/key.rs b/src/key.rs index 500302914..c16b847d2 100644 --- a/src/key.rs +++ b/src/key.rs @@ -1,7 +1,7 @@ //! Cryptographic key module. use std::collections::BTreeMap; -use std::fmt; +use std::fmt::{self, Write as _}; use std::io::Cursor; use anyhow::{Context as _, Result, bail, ensure}; @@ -583,6 +583,21 @@ impl Fingerprint { pub fn hex(&self) -> String { hex::encode_upper(&self.0) } + + /// Make a human-readable fingerprint. + pub fn human_readable(&self) -> String { + let mut f = String::new(); + // Split key into chunks of 4 with space and newline at 20 chars + for (i, c) in self.hex().chars().enumerate() { + if i > 0 && i % 20 == 0 { + writeln!(&mut f).ok(); + } else if i > 0 && i % 4 == 0 { + write!(&mut f, " ").ok(); + } + write!(&mut f, "{c}").ok(); + } + f + } } impl From for Fingerprint { @@ -599,22 +614,6 @@ impl fmt::Debug for Fingerprint { } } -/// Make a human-readable fingerprint. -impl fmt::Display for Fingerprint { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - // Split key into chunks of 4 with space and newline at 20 chars - for (i, c) in self.hex().chars().enumerate() { - if i > 0 && i % 20 == 0 { - writeln!(f)?; - } else if i > 0 && i % 4 == 0 { - write!(f, " ")?; - } - write!(f, "{c}")?; - } - Ok(()) - } -} - /// Parse a human-readable or otherwise formatted fingerprint. impl std::str::FromStr for Fingerprint { type Err = anyhow::Error; @@ -890,7 +889,7 @@ i8pcjGO+IZffvyZJVRWfVooBJmWWbPB1pueo3tx8w3+fcuzpxz+RLFKaPyqXO+dD 1, 2, 4, 8, 16, 32, 64, 128, 255, 1, 2, 4, 8, 16, 32, 64, 128, 255, 19, 20, ]); assert_eq!( - fp.to_string(), + fp.human_readable(), "0102 0408 1020 4080 FF01\n0204 0810 2040 80FF 1314" ); } diff --git a/src/qr.rs b/src/qr.rs index c204c194c..d4531442c 100644 --- a/src/qr.rs +++ b/src/qr.rs @@ -645,7 +645,7 @@ async fn decode_openpgp(context: &Context, qr: &str) -> Result { } } else { Ok(Qr::FprWithoutAddr { - fingerprint: fingerprint.to_string(), + fingerprint: fingerprint.human_readable(), }) } } diff --git a/src/qr/qr_tests.rs b/src/qr/qr_tests.rs index 4c0d1f5ac..71c631f90 100644 --- a/src/qr/qr_tests.rs +++ b/src/qr/qr_tests.rs @@ -388,7 +388,7 @@ async fn test_decode_openpgp_fingerprint() -> Result<()> { bob, &format!( "OPENPGP4FPR:{}#a=alice@example.org", - alice_contact.fingerprint().unwrap() + alice_contact.fingerprint().unwrap().hex() ), ) .await?; diff --git a/src/securejoin.rs b/src/securejoin.rs index 77db42473..3df9b6b09 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -861,7 +861,8 @@ fn encrypted_and_signed( } else { warn!( context, - "Message does not match expected fingerprint {expected_fingerprint}.", + "Message does not match expected fingerprint {}.", + expected_fingerprint.human_readable() ); false }