Return 400 for missing /api/serialize IDs (#1272)

CHANGELOG.md

@@ -44,6 +44,7 @@ Making a new release? Simply add the new header with the version and date undern
 * Fixed the sync fallback scrambling sibling order; replacements are now re-parented ancestors-first and in their original child order. ([#1265])
 * Instances that share a name and class are now robustly matched on resync by comparing their properties, instead of relying on child order alone. ([#1266])
 * Rojo now reports a clear error instead of panicking in several cases, including when the `serve` port is already in use, when a synced file is read-only or locked, when the filesystem watcher can't be created, and when the working directory is inaccessible. ([#1267])
+* Fixed `/api/serialize` returning success when a requested instance ID is missing from the serve session tree. ([#1272])
 * `rojo serve` now validates the `Host`/`Origin` headers to protect the local/private server against DNS rebinding, gates `/api/open` to local clients, and warns when bound to a network-reachable address. The accepted hosts can be extended with the `--allowed-hosts` option or a project's `serveAllowedHosts` field, for example to reach a network-exposed server by hostname. ([#1270])
 * Fixed syncback not removing stale `$properties` entries when Studio resets a property to its engine default. ([#1244])
 
@@ -60,6 +61,7 @@ Making a new release? Simply add the new header with the version and date undern
 [#1265]: https://github.com/rojo-rbx/rojo/pull/1265
 [#1266]: https://github.com/rojo-rbx/rojo/pull/1266
 [#1267]: https://github.com/rojo-rbx/rojo/pull/1267
+[#1272]: https://github.com/rojo-rbx/rojo/pull/1272
 [#1270]: https://github.com/rojo-rbx/rojo/pull/1270
 [#1244]: https://github.com/rojo-rbx/rojo/pull/1244
 

src/web/api.rs

@@ -276,7 +276,7 @@ impl ApiService {
 
                 response_dom.transfer_within(child_ref, object_value);
             } else {
-                msgpack(
+                return msgpack(
                     ErrorResponse::bad_request(format!("provided id {id} is not in the tree")),
                     StatusCode::BAD_REQUEST,
                 );

tests/rojo_test/serve_util.rs

@@ -228,11 +228,11 @@ impl TestServeSession {
         }
     }
 
-    pub fn get_api_serialize(
+    pub fn post_api_serialize(
         &self,
         ids: &[Ref],
         session_id: SessionId,
-    ) -> Result<SerializeResponse, reqwest::Error> {
+    ) -> Result<reqwest::blocking::Response, reqwest::Error> {
         let client = reqwest::blocking::Client::new();
         let url = format!("http://localhost:{}/api/serialize", self.port);
         let body = serialize_msgpack(&SerializeRequest {
@@ -241,9 +241,7 @@ impl TestServeSession {
         })
         .unwrap();
 
-        let body = client.post(url).body(body).send()?.bytes()?;
-
-        Ok(deserialize_msgpack(&body).expect("Server returned malformed response"))
+        client.post(url).body(body).send()
     }
 
     /// Sends a GET to `/api/rojo` with the given extra request headers and
@@ -291,7 +289,7 @@ fn serialize_msgpack<T: Serialize>(value: T) -> Result<Vec<u8>, rmp_serde::encod
     Ok(serialized)
 }
 
-fn deserialize_msgpack<'a, T: Deserialize<'a>>(
+pub fn deserialize_msgpack<'a, T: Deserialize<'a>>(
     input: &'a [u8],
 ) -> Result<T, rmp_serde::decode::Error> {
     let mut deserializer = rmp_serde::Deserializer::new(input).with_human_readable();

tests/tests/serve.rs

@@ -1,14 +1,16 @@
 use std::fs;
 
 use insta::{assert_snapshot, assert_yaml_snapshot, with_settings};
+use rbx_dom_weak::types::Ref;
+use reqwest::StatusCode;
 use tempfile::tempdir;
 
 use crate::rojo_test::{
     internable::InternAndRedact,
-    serve_util::{run_serve_test, serialize_to_xml_model},
+    serve_util::{deserialize_msgpack, run_serve_test, serialize_to_xml_model},
 };
 
-use librojo::web_api::SocketPacketType;
+use librojo::web_api::{SerializeResponse, SocketPacketType};
 
 #[test]
 fn rejects_dns_rebinding_requests() {
@@ -713,9 +715,13 @@ fn meshpart_with_id() {
             .find(|(_, inst)| inst.class_name == "ObjectValue")
             .unwrap();
 
-        let serialize_response = session
-            .get_api_serialize(&[*meshpart, *objectvalue], info.session_id)
+        let body = session
+            .post_api_serialize(&[*meshpart, *objectvalue], info.session_id)
+            .unwrap()
+            .bytes()
             .unwrap();
+        let serialize_response: SerializeResponse =
+            deserialize_msgpack(&body).expect("Server returned malformed response");
 
         // We don't assert a snapshot on the SerializeResponse because the model includes the
         // Refs from the DOM as names, which means it will obviously be different every time
@@ -727,6 +733,20 @@ fn meshpart_with_id() {
     });
 }
 
+#[test]
+fn serialize_missing_id() {
+    run_serve_test("empty", |session, _| {
+        let info = session.get_api_rojo().unwrap();
+        let missing_id = Ref::new();
+
+        let response = session
+            .post_api_serialize(&[missing_id], info.session_id)
+            .unwrap();
+
+        assert_eq!(response.status(), StatusCode::BAD_REQUEST);
+    });
+}
+
 #[test]
 fn forced_parent() {
     run_serve_test("forced_parent", |session, mut redactions| {
@@ -741,9 +761,13 @@ fn forced_parent() {
             read_response.intern_and_redact(&mut redactions, root_id)
         );
 
-        let serialize_response = session
-            .get_api_serialize(&[root_id], info.session_id)
+        let body = session
+            .post_api_serialize(&[root_id], info.session_id)
+            .unwrap()
+            .bytes()
             .unwrap();
+        let serialize_response: SerializeResponse =
+            deserialize_msgpack(&body).expect("Server returned malformed response");
 
         assert_eq!(serialize_response.session_id, info.session_id);