Encode workflow ids as one path segment in describe and update

CHANGELOG.md

@@ -2,6 +2,14 @@
 
 ## Unreleased
 
+- `describe_workflow` and `update_workflow` (and `update_workflow_avro_value`)
+  percent-encode the workflow id, and the update name, as one path segment.
+  Before, they were put into the path raw, so an id containing `/`, `?` or `#`
+  addressed a different route. An empty, `.` or `..` id or update name now
+  returns `Error::InvalidPathSegment` before any request; `..` would otherwise
+  have described or updated another route. Found in the security review of
+  sdk-rust#243 (durable-workflow/.github#212).
+
 - A `request_id_conflict` refusal from native Server 3.9.0 (server#934,
   server#1191) now names the key that was sent. Native doesn't echo the key, so
   `Error::request_id()` returned `None` and `RequestIdConflict::request_id` was

src/lib.rs

@@ -186,6 +186,19 @@ fn validate_request_id(request_id: &str) -> Result<()> {
     Ok(())
 }
 
+/// Encode one identifier as exactly one control-plane path segment.
+///
+/// The value is percent-encoded like [`RawRequest`] segments, so `/`, `?` or
+/// `#` in an id stays inside its segment. An empty, `.` or `..` value is
+/// refused before any request: it would address another route, such as
+/// `/workflows` instead of one workflow.
+fn path_segment(field: &'static str, value: &str) -> Result<String> {
+    if value.is_empty() || value == "." || value == ".." {
+        return Err(Error::InvalidPathSegment { field });
+    }
+    Ok(percent_encode_path_segment(value))
+}
+
 fn generated_request_id(command: &str) -> String {
     format!("sdk-rust-{command}-{}", Uuid::new_v4().simple())
 }
@@ -293,6 +306,10 @@ pub enum Error {
     InvalidRootCertificates(String),
     #[error("invalid raw request: {0}")]
     InvalidRawRequest(String),
+    /// An identifier placed in a request path was empty, `.` or `..`, so it
+    /// cannot stay one path segment. Nothing was sent.
+    #[error("invalid path segment for {field}: must not be empty, \".\" or \"..\"")]
+    InvalidPathSegment { field: &'static str },
     #[error(transparent)]
     FailureReporting(#[from] FailureReportError),
     #[error(transparent)]
@@ -3738,6 +3755,11 @@ impl Client {
         input: T,
         request_id: Option<&str>,
     ) -> Result<Value> {
+        let path = format!(
+            "/workflows/{}/update/{}",
+            path_segment("workflow_id", workflow_id)?,
+            path_segment("update_name", update_name)?
+        );
         let request_id = match request_id {
             Some(request_id) => {
                 validate_request_id(request_id)?;
@@ -3754,7 +3776,7 @@ impl Client {
         let receipt = self
             .request_json(
                 reqwest::Method::POST,
-                &format!("/workflows/{workflow_id}/update/{update_name}"),
+                &path,
                 RequestProtocol::ControlPlane,
                 Some(&body),
             )
@@ -3764,7 +3786,7 @@ impl Client {
     }
 
     pub async fn describe_workflow(&self, workflow_id: &str) -> Result<WorkflowDescription> {
-        let path = format!("/workflows/{workflow_id}");
+        let path = format!("/workflows/{}", path_segment("workflow_id", workflow_id)?);
         let mut data: WorkflowDescription = self
             .request_json(
                 reqwest::Method::GET,
@@ -29784,6 +29806,109 @@ mod tests {
         assert_key_sent_only_in_body(&server, KEYED_UPDATE_PATH);
     }
 
+    fn segment_server(path: &str, _: &str, _: usize) -> Option<(&'static str, String)> {
+        if path.contains("/update/") {
+            return Some((
+                "200 OK",
+                json!({"update_id": "upd-1", "update_status": "completed",
+                    "result_envelope": encode_typed_envelope(&AvroValue::Long(1), DEFAULT_CODEC).ok()})
+                .to_string(),
+            ));
+        }
+        path.starts_with("/api/workflows/").then(|| {
+            (
+                "200 OK",
+                json!({"workflow_id": "x", "run_id": "run-1", "status": "running"}).to_string(),
+            )
+        })
+    }
+
+    #[tokio::test]
+    async fn describe_and_update_keep_each_identifier_in_one_encoded_segment() {
+        let server = MockWorkerServer::start_with_behavior(MockWorkerBehavior {
+            request_override: Some(segment_server),
+            ..MockWorkerBehavior::default()
+        });
+        let client = keyed_client(&server);
+
+        client
+            .describe_workflow("orders/2026?x#y")
+            .await
+            .expect("describe");
+        assert_eq!(
+            server.method_for("/api/workflows/orders%2F2026%3Fx%23y"),
+            Some("GET".to_string()),
+            "the workflow id stays one segment: {:?}",
+            server.captured_paths()
+        );
+        client
+            .update_workflow("order 1/a", "re/name", json!([1]), Some("key-1"))
+            .await
+            .expect("update");
+        assert_eq!(
+            server.request_body("/api/workflows/order%201%2Fa/update/re%2Fname")["request_id"],
+            "key-1"
+        );
+    }
+
+    #[tokio::test]
+    async fn dot_segment_and_empty_identifiers_are_refused_before_any_request() {
+        let server = MockWorkerServer::start_with_behavior(MockWorkerBehavior {
+            request_override: Some(segment_server),
+            ..MockWorkerBehavior::default()
+        });
+        let client = keyed_client(&server);
+
+        for workflow_id in ["", ".", ".."] {
+            let error = client
+                .describe_workflow(workflow_id)
+                .await
+                .expect_err("a dot segment would describe another route");
+            assert!(
+                matches!(
+                    error,
+                    Error::InvalidPathSegment {
+                        field: "workflow_id"
+                    }
+                ),
+                "{workflow_id:?}: {error:?}"
+            );
+            let error = client
+                .update_workflow(workflow_id, "adjust", json!([1]), None)
+                .await
+                .expect_err("a dot segment would update another route");
+            assert!(
+                matches!(
+                    error,
+                    Error::InvalidPathSegment {
+                        field: "workflow_id"
+                    }
+                ),
+                "{workflow_id:?}: {error:?}"
+            );
+        }
+        for update_name in ["", ".", ".."] {
+            let error = client
+                .update_workflow("order-1", update_name, json!([1]), None)
+                .await
+                .expect_err("a dot segment update name is refused");
+            assert!(
+                matches!(
+                    error,
+                    Error::InvalidPathSegment {
+                        field: "update_name"
+                    }
+                ),
+                "{update_name:?}: {error:?}"
+            );
+        }
+        assert!(
+            server.captured_paths().is_empty(),
+            "nothing was sent: {:?}",
+            server.captured_paths()
+        );
+    }
+
     #[tokio::test]
     async fn explicit_command_keys_are_sent_verbatim_as_the_request_id_body_field() {
         let server = MockWorkerServer::start_with_behavior(MockWorkerBehavior {