From 29d8fbaefa29d33ee341854fc6c1c0c5ccd970ab Mon Sep 17 00:00:00 2001
From: Olivia Lee <olivia@computer.surgery>
Date: Mon, 20 Jan 2025 15:46:36 -0800
Subject: [PATCH] only validate canonical aliases that are new

Previously we required every alias in a canonical alias event sent by a
client to be valid, and would only validate local aliases. This
prevented clients from adding/removing canonical aliases if there were
existing remote or invalid aliases.
---
 book/changelog.md              |  4 ++
 src/api/client_server/state.rs | 67 ++++++++++++++++++++++++++++------
 2 files changed, 59 insertions(+), 12 deletions(-)

diff --git a/book/changelog.md b/book/changelog.md
index 0ee07aa1..5850e9ed 100644
--- a/book/changelog.md
+++ b/book/changelog.md
@@ -228,6 +228,10 @@ This will be the first release of Grapevine since it was forked from Conduit
 26. Validate schema of new `m.room.canonical_alias` event sent by clients,
     rather than silently allowing any contents if the event can't be parsed.
     ([!158](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/158))
+27. Only validate canonical aliases that are new, rather than rather than
+    revalidating every alias. This makes it possible to add/remove aliases when
+    some of the existing aliases cannot be validated.
+    ([!158](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/158))
 
 ### Added
 
diff --git a/src/api/client_server/state.rs b/src/api/client_server/state.rs
index 85cd2a61..cfa48ef0 100644
--- a/src/api/client_server/state.rs
+++ b/src/api/client_server/state.rs
@@ -1,4 +1,4 @@
-use std::sync::Arc;
+use std::{collections::HashSet, sync::Arc};
 
 use ruma::{
     api::client::{
@@ -10,9 +10,10 @@ use ruma::{
         AnyStateEventContent, StateEventType,
     },
     serde::Raw,
-    EventId, RoomId, UserId,
+    EventId, RoomAliasId, RoomId, UserId,
 };
-use tracing::log::warn;
+use serde::Deserialize;
+use tracing::warn;
 
 use crate::{service::pdu::PduBuilder, services, Ar, Error, Ra, Result};
 
@@ -256,18 +257,60 @@ fn validate_canonical_alias_event(
     room_id: &RoomId,
     json: &Raw<RoomCanonicalAliasEventContent>,
 ) -> Result<()> {
-    // TODO: allow alias if it previously existed
-    if let Ok(canonical_alias) = serde_json::from_str::<
-        RoomCanonicalAliasEventContent,
-    >(json.json().get())
+    // Use a custom struct instead of RoomCanonicalAliasEventContent because we
+    // only want to validate the syntax of new aliases, so can't deserialize
+    // everything to OwnedRoomAliasId.
+    #[derive(Deserialize)]
+    struct Extract {
+        alias: Option<String>,
+        #[serde(default)]
+        alt_aliases: Vec<String>,
+    }
+
+    // If the existing canonical alias event is invalid, treat it as if there
+    // are no existing aliases instead of erroring out. This allows users to
+    // fix a bad canonical alias event by sending a new one, but means that
+    // every alias in the new event will be revalidated.
+    let old_event = services()
+        .rooms
+        .state_accessor
+        .room_state_get(room_id, &StateEventType::RoomCanonicalAlias, "")?
+        .and_then(|old_event| {
+            serde_json::from_str::<Extract>(old_event.content.get())
+                .inspect_err(|error| {
+                    warn!(
+                        %error,
+                        event_id=%old_event.event_id,
+                       "Invalid canonical alias event in database"
+                    );
+                })
+                .ok()
+        });
+
+    let old_aliases = if let Some(old_event) = &old_event {
+        old_event.alias.iter().chain(old_event.alt_aliases.iter()).collect()
+    } else {
+        HashSet::new()
+    };
+
+    if let Ok(canonical_alias) =
+        serde_json::from_str::<Extract>(json.json().get())
     {
-        let mut aliases = canonical_alias.alt_aliases.clone();
+        let aliases = canonical_alias
+            .alias
+            .iter()
+            .chain(canonical_alias.alt_aliases.iter());
+        let new_aliases = aliases.filter(|alias| !old_aliases.contains(alias));
 
-        if let Some(alias) = canonical_alias.alias {
-            aliases.push(alias);
-        }
+        for alias in new_aliases {
+            let alias = RoomAliasId::parse(alias).map_err(|_| {
+                Error::BadRequest(
+                    ErrorKind::InvalidParam,
+                    "One or more aliases in m.room.canonical_alias event have \
+                     invalid syntax",
+                )
+            })?;
 
-        for alias in aliases {
             if alias.server_name() != services().globals.server_name()
                 || services()
                     .rooms