diff --git a/CHANGELOG.md b/CHANGELOG.md index 87bd872..77c2eb6 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/lib.rs b/src/lib.rs index e2592e2..5d222c1 100644 --- a/src/lib.rs +++ b/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 { + 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 { + 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 { - 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 {