Merge 2e0cc81bd4 into 7e60406373

CHANGELOG.md

@@ -61,6 +61,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])
+* Rojo now returns a structured error response instead of panicking when `serve` API request bodies fail to read. ([#1276])
 * 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])
@@ -79,6 +80,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
+[#1276]: https://github.com/rojo-rbx/rojo/pull/1276
 [#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

@@ -4,7 +4,10 @@
 use std::{collections::HashMap, fs, net::SocketAddr, path::PathBuf, str::FromStr, sync::Arc};
 
 use futures::{sink::SinkExt, stream::StreamExt};
-use hyper::{body, Body, Method, Request, Response, StatusCode};
+use hyper::{
+    body::{self, Bytes},
+    Body, Method, Request, Response, StatusCode,
+};
 use hyper_tungstenite::{is_upgrade_request, tungstenite::Message, upgrade, HyperWebsocket};
 use opener::OpenError;
 use rbx_dom_weak::{
@@ -141,7 +144,10 @@ impl ApiService {
         let session_id = self.serve_session.session_id();
         let tree_mutation_sender = self.serve_session.tree_mutation_sender();
 
-        let body = body::to_bytes(request.into_body()).await.unwrap();
+        let body = match read_request_body(request.into_body()).await {
+            Ok(body) => body,
+            Err(response) => return response,
+        };
 
         let request: WriteRequest = match deserialize_msgpack(&body) {
             Ok(request) => request,
@@ -230,7 +236,10 @@ impl ApiService {
     /// `Value` property set to point to the requested Instance.
     async fn handle_api_serialize(&self, request: Request<Body>) -> Response<Body> {
         let session_id = self.serve_session.session_id();
-        let body = body::to_bytes(request.into_body()).await.unwrap();
+        let body = match read_request_body(request.into_body()).await {
+            Ok(body) => body,
+            Err(response) => return response,
+        };
 
         let request: SerializeRequest = match deserialize_msgpack(&body) {
             Ok(request) => request,
@@ -299,7 +308,10 @@ impl ApiService {
     /// endpoint is used.
     async fn handle_api_ref_patch(self, request: Request<Body>) -> Response<Body> {
         let session_id = self.serve_session.session_id();
-        let body = body::to_bytes(request.into_body()).await.unwrap();
+        let body = match read_request_body(request.into_body()).await {
+            Ok(body) => body,
+            Err(response) => return response,
+        };
 
         let request: RefPatchRequest = match deserialize_msgpack(&body) {
             Ok(request) => request,
@@ -456,6 +468,15 @@ impl ApiService {
     }
 }
 
+async fn read_request_body(body: Body) -> Result<Bytes, Response<Body>> {
+    body::to_bytes(body).await.map_err(|err| {
+        msgpack(
+            ErrorResponse::bad_request(format!("Failed to read request body: {}", err)),
+            StatusCode::BAD_REQUEST,
+        )
+    })
+}
+
 /// If this instance is represented by a script, try to find the correct .lua or .luau
 /// file to open to edit it.
 fn pick_script_path(instance: InstanceWithMeta<'_>) -> Option<PathBuf> {
@@ -605,3 +626,45 @@ fn parent_requirements(class: &str) -> Option<&str> {
         _ => return None,
     })
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    use serde::Deserialize;
+
+    #[derive(Debug, Deserialize, PartialEq)]
+    enum ApiErrorKind {
+        BadRequest,
+    }
+
+    #[derive(Debug, Deserialize)]
+    #[serde(rename_all = "camelCase")]
+    struct ApiError {
+        kind: ApiErrorKind,
+        details: String,
+    }
+
+    #[tokio::test]
+    async fn read_request_body_returns_body_bytes() {
+        let body = read_request_body(Body::from("hello")).await.unwrap();
+
+        assert_eq!(body, Bytes::from_static(b"hello"));
+    }
+
+    #[tokio::test]
+    async fn read_request_body_returns_structured_bad_request_for_body_errors() {
+        let (sender, body) = Body::channel();
+        sender.abort();
+
+        let response = read_request_body(body).await.unwrap_err();
+
+        assert_eq!(response.status(), StatusCode::BAD_REQUEST);
+
+        let body = body::to_bytes(response.into_body()).await.unwrap();
+        let error: ApiError = deserialize_msgpack(&body).unwrap();
+
+        assert_eq!(error.kind, ApiErrorKind::BadRequest);
+        assert!(error.details.contains("Failed to read request body"));
+    }
+}