fix(id): refuse duplicate JSON keys in world_session first/joined frames (#6501)

pkgs/id/src/world_session/mod.rs

@@ -873,6 +873,131 @@ pub(crate) fn frame_admin_token(frame: &ClientFrame) -> Option<&str> {
     }
 }
 
+/// Parse one JSON value while refusing repeated object keys at every depth.
+/// `serde_json::Value` normally keeps only the last value for a repeated key,
+/// which can make routing and later checks observe different bytes than the
+/// client sent.
+fn parse_json_without_duplicate_keys(text: &str) -> Result<serde_json::Value, serde_json::Error> {
+    use serde::Deserialize;
+    use serde::de::{self, MapAccess, SeqAccess, Visitor};
+    use std::collections::HashSet;
+    use std::fmt;
+
+    struct UniqueValue(serde_json::Value);
+
+    impl<'de> Deserialize<'de> for UniqueValue {
+        fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+        where
+            D: serde::Deserializer<'de>,
+        {
+            struct UniqueValueVisitor;
+
+            impl<'de> Visitor<'de> for UniqueValueVisitor {
+                type Value = UniqueValue;
+
+                fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
+                    formatter.write_str("a JSON value without duplicate object keys")
+                }
+
+                fn visit_bool<E>(self, value: bool) -> Result<Self::Value, E>
+                where
+                    E: de::Error,
+                {
+                    Ok(UniqueValue(serde_json::Value::Bool(value)))
+                }
+
+                fn visit_i64<E>(self, value: i64) -> Result<Self::Value, E>
+                where
+                    E: de::Error,
+                {
+                    Ok(UniqueValue(serde_json::Value::Number(value.into())))
+                }
+
+                fn visit_u64<E>(self, value: u64) -> Result<Self::Value, E>
+                where
+                    E: de::Error,
+                {
+                    Ok(UniqueValue(serde_json::Value::Number(value.into())))
+                }
+
+                fn visit_f64<E>(self, value: f64) -> Result<Self::Value, E>
+                where
+                    E: de::Error,
+                {
+                    serde_json::Number::from_f64(value)
+                        .map(serde_json::Value::Number)
+                        .map(UniqueValue)
+                        .ok_or_else(|| E::custom("JSON number is not finite"))
+                }
+
+                fn visit_str<E>(self, value: &str) -> Result<Self::Value, E>
+                where
+                    E: de::Error,
+                {
+                    Ok(UniqueValue(serde_json::Value::String(value.to_owned())))
+                }
+
+                fn visit_string<E>(self, value: String) -> Result<Self::Value, E>
+                where
+                    E: de::Error,
+                {
+                    Ok(UniqueValue(serde_json::Value::String(value)))
+                }
+
+                fn visit_none<E>(self) -> Result<Self::Value, E>
+                where
+                    E: de::Error,
+                {
+                    Ok(UniqueValue(serde_json::Value::Null))
+                }
+
+                fn visit_unit<E>(self) -> Result<Self::Value, E>
+                where
+                    E: de::Error,
+                {
+                    Ok(UniqueValue(serde_json::Value::Null))
+                }
+
+                fn visit_seq<A>(self, mut sequence: A) -> Result<Self::Value, A::Error>
+                where
+                    A: SeqAccess<'de>,
+                {
+                    let mut values = Vec::new();
+                    while let Some(value) = sequence.next_element::<UniqueValue>()? {
+                        values.push(value.0);
+                    }
+                    Ok(UniqueValue(serde_json::Value::Array(values)))
+                }
+
+                fn visit_map<A>(self, mut map: A) -> Result<Self::Value, A::Error>
+                where
+                    A: MapAccess<'de>,
+                {
+                    let mut seen = HashSet::new();
+                    let mut values = serde_json::Map::new();
+                    while let Some(key) = map.next_key::<String>()? {
+                        if !seen.insert(key.clone()) {
+                            return Err(de::Error::custom(format!(
+                                "duplicate JSON object key {key:?}"
+                            )));
+                        }
+                        let value = map.next_value::<UniqueValue>()?;
+                        values.insert(key, value.0);
+                    }
+                    Ok(UniqueValue(serde_json::Value::Object(values)))
+                }
+            }
+
+            deserializer.deserialize_any(UniqueValueVisitor)
+        }
+    }
+
+    let mut deserializer = serde_json::Deserializer::from_str(text);
+    let value = UniqueValue::deserialize(&mut deserializer)?.0;
+    deserializer.end()?;
+    Ok(value)
+}
+
 /// Run one session to completion.
 ///
 /// The first frame names the world (`"world": "<name>"`, default world if
@@ -898,7 +1023,7 @@ pub async fn run_session_with<I: SessionIo>(hub: &WorldHub, io: &mut I, config:
             return;
         }
     };
-    let Ok(mut value) = serde_json::from_str::<serde_json::Value>(&first) else {
+    let Ok(mut value) = parse_json_without_duplicate_keys(&first) else {
         let _ = send_error(
             &unattributed,
             io,
@@ -1593,7 +1718,9 @@ async fn query_session<I: SessionIo>(service: &WorldService, io: &mut I, first:
             Some(frame) => frame,
             None => match io.recv().await {
                 Inbound::Text(text) => {
-                    if let Ok(frame) = serde_json::from_str::<ClientFrame>(&text) {
+                    if let Ok(frame) = parse_json_without_duplicate_keys(&text)
+                        .and_then(|value| serde_json::from_value::<ClientFrame>(value))
+                    {
                         frame
                     } else {
                         let _ = send_error(service.frame_meter(), io, "invalid world frame").await;
@@ -2533,7 +2660,7 @@ async fn handle_client_frame<I: SessionIo>(
     pending_view_deadline: &mut Option<tokio::time::Instant>,
 ) -> bool {
     let world = service.world();
-    let Ok(value) = serde_json::from_str::<serde_json::Value>(text) else {
+    let Ok(value) = parse_json_without_duplicate_keys(text) else {
         return send_error(service.frame_meter(), io, "invalid world frame").await;
     };
     if delegation_names_uses(&value) {

pkgs/id/src/world_session/tests.rs

@@ -1,5 +1,6 @@
 #[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)]
 mod tests {
+    use proptest::prelude::*;
     use tokio::sync::mpsc;
 
     use ed25519_dalek::{Signer, SigningKey};
@@ -1147,6 +1148,169 @@ mod tests {
         assert_eq!(bin.next().await.unwrap()["type"], "error");
     }
 
+    #[tokio::test]
+    async fn first_frame_rejects_duplicate_json_keys_at_every_depth() {
+        const FRAMES: [(&str, &str); 7] = [
+            (
+                "identical type",
+                r#"{"type":"join","type":"join","capability":"x"}"#,
+            ),
+            (
+                "conflicting type",
+                r#"{"type":"join","type":"chat","capability":"x"}"#,
+            ),
+            (
+                "world duplicate with last value",
+                r#"{"type":"join","capability":"x","world":"lobby","world":"other"}"#,
+            ),
+            (
+                "world duplicate with first value",
+                r#"{"world":"other","world":"lobby","type":"join","capability":"x"}"#,
+            ),
+            (
+                "capability",
+                r#"{"type":"join","capability":"x","capability":"y"}"#,
+            ),
+            (
+                "admin token",
+                r#"{"type":"invite","admin_token":"x","admin_token":"y","display_name":"ann"}"#,
+            ),
+            (
+                "nested signed nonce",
+                r#"{"type":"join","capability":"x","signed":{"key":"k","at":1,"nonce":"one","nonce":"two","signature":"s"}}"#,
+            ),
+        ];
+
+        for (case, text) in FRAMES {
+            let mut peer = Peer::connect(service(None), SessionConfig::default());
+            peer.say_text(text).await;
+            let reply = peer.next().await.unwrap();
+            assert_eq!(reply["type"], "error", "{case}: {reply}");
+            assert_eq!(
+                reply["message"], "first frame must be a valid join or invite request",
+                "{case}: {reply}"
+            );
+            assert!(peer.next().await.is_none(), "{case}");
+            peer.task.await.unwrap();
+        }
+    }
+
+    #[tokio::test]
+    async fn joined_frame_rejects_duplicate_json_keys_at_every_depth() {
+        const FRAMES: [(&str, &str); 7] = [
+            (
+                "identical type",
+                r#"{"type":"chat","type":"chat","text":"hi"}"#,
+            ),
+            (
+                "conflicting type",
+                r#"{"type":"chat","type":"input","text":"hi","data_hex":""}"#,
+            ),
+            (
+                "world duplicate with last value",
+                r#"{"type":"chat","text":"hi","world":"lobby","world":"other"}"#,
+            ),
+            (
+                "world duplicate with first value",
+                r#"{"world":"other","world":"lobby","type":"chat","text":"hi"}"#,
+            ),
+            (
+                "capability",
+                r#"{"type":"chat","text":"hi","capability":"x","capability":"y"}"#,
+            ),
+            (
+                "admin token",
+                r#"{"type":"set_isolation","isolation":"isolated","admin_token":"x","admin_token":"y"}"#,
+            ),
+            (
+                "nested signed nonce",
+                r#"{"type":"set_isolation","isolation":"isolated","signed":{"key":"k","at":1,"nonce":"one","nonce":"two","signature":"s"}}"#,
+            ),
+        ];
+
+        let svc = service(None);
+        let (_, capability) = svc
+            .world()
+            .issue("ann".to_owned(), WorldScopes::GUEST)
+            .await
+            .unwrap();
+        let mut peer = Peer::connect(svc, SessionConfig::default());
+        peer.say(serde_json::json!({"type": "join", "capability": capability.expose()}))
+            .await;
+        assert_eq!(peer.next().await.unwrap()["type"], "snapshot");
+
+        for (case, text) in FRAMES {
+            peer.say_text(text).await;
+            let reply = peer.next().await.unwrap();
+            assert_eq!(reply["type"], "error", "{case}: {reply}");
+            assert_eq!(reply["message"], "invalid world frame", "{case}: {reply}");
+        }
+
+        peer.to_server.send(Inbound::Closed).await.unwrap();
+        peer.task.await.unwrap();
+    }
+
+    #[tokio::test]
+    async fn query_frame_rejects_duplicate_json_keys_before_join() {
+        // Regression test for the PR #6508 review finding: frames received
+        // after a valid query/info first frame were parsed straight into
+        // ClientFrame, so duplicate keys in fields the frame type ignores
+        // collapsed silently and a join carrying them still entered the
+        // joined phase. Oracle: the strict duplicate-key parser — any
+        // repeated key at any depth refuses the frame before conversion.
+        let svc = service(None);
+        let (_, capability) = svc
+            .world()
+            .issue("ann".to_owned(), WorldScopes::GUEST)
+            .await
+            .unwrap();
+        let cap = capability.expose().to_owned();
+
+        let frames: [(&str, String); 3] = [
+            (
+                "duplicate ignored world field",
+                format!(
+                    r#"{{"type":"join","capability":"{cap}","world":"lobby","world":"other"}}"#
+                ),
+            ),
+            (
+                "duplicate unknown field",
+                format!(r#"{{"type":"join","capability":"{cap}","bogus":1,"bogus":2}}"#),
+            ),
+            (
+                "duplicate inside ignored nested object",
+                format!(r#"{{"type":"join","capability":"{cap}","extra":{{"a":1,"a":2}}}}"#),
+            ),
+        ];
+
+        for (case, text) in frames {
+            let mut peer = Peer::connect(svc.clone(), SessionConfig::default());
+            peer.say(serde_json::json!({"type": "info", "capability": cap}))
+                .await;
+            assert_eq!(peer.next().await.unwrap()["type"], "module_info", "{case}");
+            peer.say_text(&text).await;
+            let reply = peer.next().await.unwrap();
+            assert_eq!(reply["type"], "error", "{case}: {reply}");
+            assert_eq!(reply["message"], "invalid world frame", "{case}: {reply}");
+            assert!(peer.next().await.is_none(), "{case}");
+            peer.task.await.unwrap();
+        }
+    }
+
+    proptest! {
+        #[test]
+        fn duplicate_json_keys_are_rejected_at_any_nesting_depth(
+            key in "[a-zA-Z_][a-zA-Z0-9_]{0,15}",
+            depth in 0usize..8,
+        ) {
+            let duplicate = format!(r#"{{"{key}":0,"{key}":1}}"#);
+            let nested = (0..depth).fold(duplicate, |value, _| {
+                format!(r#"{{"nested":{value}}}"#)
+            });
+            prop_assert!(parse_json_without_duplicate_keys(&nested).is_err());
+        }
+    }
+
     #[test]
     fn invite_lifetimes_default_to_a_week_and_stop_at_thirty_days() {
         let now = 1_000_000;