From 22578ea4b6988e4e1d0b7c9992d15ffb21743ba4 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Thu, 24 Sep 2026 22:58:11 +0200 Subject: [PATCH] fix: Correctly percent-encode addresses in securejoin invite codes (#8747) This PR fixes how addresses in securejoin invite codes (i.e. QR codes and invite links) are percent-encoded. Before this PR, - `@` in the addresses was percent-encoded. This makes the invite code harder to read, and is not necessary; at least, every software we tested correctly handled links that contain `@` (Signal, WhatsApp, Telegram, Thunderbird, Delta Chat on Android, iOS, and Desktop) - _But_, the first address in the `r=` parameter of a securejoin link was not percent-encoded at all. This was a sneaky bug caused by using the `reduce` function; I always find it hard to follow code that uses `reduce` (and similar functions like `fold`), and apparently others have the same problem since neither @j-g00da nor @link2xt noticed the problem when implementing & reviewing the PR that introduced the bug. With this PR: - `@` is allowed in addresses in securejoin invite codes - all addresses are percent-encoded - `reduced` is not used anymore --------- Co-authored-by: l --- src/securejoin.rs | 19 ++++++++------- src/securejoin/securejoin_tests.rs | 38 ++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 9 deletions(-) diff --git a/src/securejoin.rs b/src/securejoin.rs index 4e4ce9b31..d709e1326 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -33,7 +33,7 @@ pub(crate) use qrinvite::QrInvite; use crate::token::Namespace; -const DISALLOWED_CHARACTERS: &AsciiSet = &NON_ALPHANUMERIC_WITHOUT_DOT.remove(b'_'); +const DISALLOWED_CHARACTERS: &AsciiSet = &NON_ALPHANUMERIC_WITHOUT_DOT.remove(b'_').remove(b'@'); fn inviter_progress( context: &Context, @@ -124,18 +124,19 @@ pub async fn get_securejoin_qr(context: &Context, chat: Option) -> Resul let self_addr = context.get_primary_self_addr().await?; let self_addr_urlencoded = utf8_percent_encode(&self_addr, DISALLOWED_CHARACTERS).to_string(); - let r_param = context + let encoded_extra_relays: Vec = context .get_self_addrs() .await? .into_iter() .filter(|addr| *addr != self_addr) - .reduce(|acc, addr| { - format!( - "{acc},{}", - utf8_percent_encode(&addr, DISALLOWED_CHARACTERS) - ) - }) - .map_or(String::default(), |addrs| format!("&r={addrs}")); + .map(|addr| utf8_percent_encode(&addr, DISALLOWED_CHARACTERS).to_string()) + .collect(); + + let r_param = if encoded_extra_relays.is_empty() { + "".to_string() + } else { + format!("&r={}", encoded_extra_relays.join(",")) + }; let self_name = context .get_config(Config::Displayname) diff --git a/src/securejoin/securejoin_tests.rs b/src/securejoin/securejoin_tests.rs index 5be9a1177..5600a4b08 100644 --- a/src/securejoin/securejoin_tests.rs +++ b/src/securejoin/securejoin_tests.rs @@ -13,6 +13,7 @@ use crate::test_utils::{ AVATAR_64x64_BYTES, AVATAR_64x64_DEDUPLICATED, TestContext, TestContextManager, TimeShiftFalsePositiveNote, get_chat_msg, sync, }; +use crate::transport::add_pseudo_transport; #[derive(PartialEq)] enum SetupContactCase { @@ -1142,6 +1143,43 @@ async fn test_get_securejoin_qr_name_is_last() -> Result<()> { Ok(()) } +/// Test that addresses in QR codes are percent-encoded. +/// `@` should not be encoded unnecessarily, +/// since this would just make the QR code longer. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_get_securejoin_qr_encoding() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + + // `@` in email addresses must not be percent-encoded: + add_pseudo_transport(alice, "asdf@example.org").await?; + // But `%` does need percent-encoding: + add_pseudo_transport(alice, "jk%l@example.net").await?; + + let qr = get_securejoin_qr(alice, None).await?; + assert!( + qr.contains("a=alice@example.org"), + "{qr} doesn't contain 'a=alice@example.org'" + ); + assert!( + qr.contains("r=jk%25l@example.net,asdf@example.org"), + "{qr} doesn't contain 'r=jk%25l@example.net,asdf@example.org'" + ); + + let qr = check_qr(bob, &qr).await?; + let Qr::AskVerifyContact { mut addrs, .. } = qr else { + unreachable!() + }; + addrs.sort(); + assert_eq!( + addrs, + vec!["alice@example.org", "asdf@example.org", "jk%l@example.net",] + ); + + Ok(()) +} + /// QR codes should not get arbitrary big because of long names. /// The truncation, however, should not let the url end with a `.`, which is a call for trouble in linkfiers. #[tokio::test(flavor = "multi_thread", worker_threads = 2)]