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 <link2xt@testrun.org>

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<ChatId>) -> 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<String> = 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)

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)]