Make MSC4388 (sign-in with QR code) PUT requests idempotent (#19808)

changelog.d/19808.feature

@@ -0,0 +1 @@
+Updated experimental support for [MSC4388: Secure out-of-band channel for sign in with QR](https://github.com/matrix-org/matrix-spec-proposals/pull/4388).

rust/src/msc4388_rendezvous/mod.rs

@@ -331,6 +331,14 @@ impl MSC4388RendezvousHandler {
             .ok_or_else(NotFoundError::new)?;
 
         if !session.sequence_token().eq(&sequence_token) {
+            // Allow clients to safely retry a PUT (e.g. after a network error)
+            // by accepting the previous sequence_token as long as the data
+            // being submitted matches what is currently stored. This makes
+            // PUTs idempotent without weakening the concurrent-write check.
+            if session.is_idempotent_retry(&sequence_token, &data) {
+                return Ok((200, session.put_response()));
+            }
+
             return Err(SynapseError::new(
                 StatusCode::CONFLICT,
                 "sequence_token does not match".to_owned(),

rust/src/msc4388_rendezvous/session.rs

@@ -25,6 +25,12 @@ use ulid::Ulid;
 pub struct Session {
     id: Ulid,
     hash: [u8; 32],
+    /// The hash from before the last `update`, if any. Used so that clients can
+    /// safely retry a PUT request (e.g. after a network error) without getting
+    /// a spurious 409 conflict: a PUT whose `sequence_token` matches this
+    /// previous hash and whose `data` matches the currently-stored data is
+    /// treated as an idempotent no-op.
+    previous_hash: Option<[u8; 32]>,
     data: String,
     last_modified: SystemTime,
     expires: SystemTime,
@@ -86,6 +92,7 @@ impl Session {
         Self {
             id,
             hash,
+            previous_hash: None,
             data,
             expires: now + ttl,
             last_modified: now,
@@ -99,11 +106,26 @@ impl Session {
 
     /// Update the session with new data and last modified time.
     pub fn update(&mut self, data: String, now: SystemTime) {
+        self.previous_hash = Some(self.hash);
         self.hash = Self::compute_hash(&data, now);
         self.data = data;
         self.last_modified = now;
     }
 
+    /// Returns true if a PUT with the given `sequence_token` and `data` should
+    /// be treated as an idempotent retry of the most recent update (i.e. the
+    /// token matches the hash from before the last update, and the data
+    /// already matches the currently-stored data).
+    pub fn is_idempotent_retry(&self, sequence_token: &str, data: &str) -> bool {
+        let Some(previous_hash) = self.previous_hash else {
+            return false;
+        };
+        if data != self.data {
+            return false;
+        }
+        URL_SAFE_NO_PAD.encode(previous_hash) == sequence_token
+    }
+
     /// Compute the hash of the data and timestamp.
     fn compute_hash(data: &str, now: SystemTime) -> [u8; 32] {
         let mut hasher = Sha256::new();

tests/rest/client/test_msc4388_rendezvous.py

@@ -252,6 +252,77 @@ class RendezvousServletTestCase(unittest.HomeserverTestCase):
         self.assertEqual(channel.code, 404)
         self.assertEqual(channel.json_body["errcode"], "M_NOT_FOUND")
 
+    @override_config(
+        {
+            "disable_registration": True,
+            "matrix_authentication_service": {
+                "enabled": True,
+                "secret": "secret_value",
+                "endpoint": "https://issuer",
+            },
+            "experimental_features": {
+                "msc4388_mode": "open",
+            },
+        }
+    )
+    def test_rendezvous_put_is_idempotent(self) -> None:
+        """
+        A PUT using the previous sequence_token but with data that already
+        matches what is currently stored should be treated as an idempotent
+        retry and succeed (rather than returning 409). This lets clients
+        safely retry a PUT after a network error without losing the session.
+        """
+        channel = self.make_request(
+            "POST",
+            rz_endpoint,
+            {"data": "foo=bar"},
+            access_token=None,
+        )
+        self.assertEqual(channel.code, 200)
+        rendezvous_id = channel.json_body["id"]
+        initial_sequence_token = channel.json_body["sequence_token"]
+        session_endpoint = rz_endpoint + f"/{rendezvous_id}"
+
+        # Perform an update.
+        channel = self.make_request(
+            "PUT",
+            session_endpoint,
+            {"sequence_token": initial_sequence_token, "data": "foo=baz"},
+            access_token=None,
+        )
+        self.assertEqual(channel.code, 200)
+        updated_sequence_token = channel.json_body["sequence_token"]
+
+        # Replaying the same PUT with the previous (now-stale) sequence_token
+        # and matching data should succeed and return the current token.
+        channel = self.make_request(
+            "PUT",
+            session_endpoint,
+            {"sequence_token": initial_sequence_token, "data": "foo=baz"},
+            access_token=None,
+        )
+        self.assertEqual(channel.code, 200)
+        self.assertEqual(channel.json_body["sequence_token"], updated_sequence_token)
+
+        # But replaying with the previous token and *different* data must
+        # still be rejected as a concurrent write.
+        channel = self.make_request(
+            "PUT",
+            session_endpoint,
+            {"sequence_token": initial_sequence_token, "data": "something=else"},
+            access_token=None,
+        )
+        self.assertEqual(channel.code, 409)
+        self.assertEqual(
+            channel.json_body["errcode"], "IO_ELEMENT_MSC4388_CONCURRENT_WRITE"
+        )
+
+        # The stored data should be unchanged.
+        channel = self.make_request("GET", session_endpoint, access_token=None)
+        self.assertEqual(channel.code, 200)
+        self.assertEqual(channel.json_body["data"], "foo=baz")
+        self.assertEqual(channel.json_body["sequence_token"], updated_sequence_token)
+
     @override_config(
         {
             "disable_registration": True,