From 164a9d754306ad5150dd4315c41c334abbd1162b Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Sat, 29 Aug 2026 10:09:08 +0000 Subject: [PATCH] fix(adapters): safe outbound URL construction (FWD-01, FWD-02) - percent-encode path-parameter values with a WHATWG path-segment superset (/, %, ?, #, \\, controls): traversal values, query/fragment structure, and later-placeholder strings can no longer alter the request line (FWD-01) - single-pass template rendering; rendered values are never re-substituted; unbound or unterminated placeholders error loudly - append the request path to the base URL directory (https://host/v1 + /chat/completions keeps /v1) instead of Url::join semantics, with a post-assembly origin-equality check (FWD-02) - base_url validation: https/http-only scheme allowlist, explicit host required, userinfo rejected (credentials flow via Capabilities only); request_path is never empty Verification: cargo test (238 lib tests incl. 8 new FWD-01/02 tests), cargo clippy --all-targets -- -D warnings, cargo fmt --check --- Cargo.lock | 1 + Cargo.toml | 1 + src/adapters/forward.rs | 399 ++++++++++++++++++++++++++++++++++++++-- 3 files changed, 386 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 202fa41..e8e38e9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -64,6 +64,7 @@ dependencies = [ "jsonschema", "openapiv3", "parking_lot", + "percent-encoding", "reqwest", "reqwest-middleware", "reqwest-retry", diff --git a/Cargo.toml b/Cargo.toml index 682adc0..f275baf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -45,6 +45,7 @@ openapiv3 = "2" http = "1" http-body-util = "0.1" url = "2" +percent-encoding = "2" bytes = "1" jsonschema = { version = "0.46", default-features = false } parking_lot = "0.12" diff --git a/src/adapters/forward.rs b/src/adapters/forward.rs index db5e098..77afe76 100644 --- a/src/adapters/forward.rs +++ b/src/adapters/forward.rs @@ -19,6 +19,7 @@ use alkcall::registry::registration::ResponseStream; use alkcall::registry::spec::OperationType; use futures::stream; use futures::StreamExt; +use percent_encoding::{utf8_percent_encode, AsciiSet, CONTROLS}; use reqwest::header::{HeaderMap, HeaderName, HeaderValue, ACCEPT, AUTHORIZATION, CONTENT_TYPE}; use reqwest::Method; use serde_json::Value; @@ -51,19 +52,16 @@ pub(crate) fn build_request( input: &Value, context: &OperationContext, ) -> Result<(Method, Url, Option, HeaderMap), CallError> { - let input_obj = input.as_object(); + let inputs = input.as_object(); - let mut url_path = path_template.to_string(); let mut query_params: Vec<(String, String)> = Vec::new(); let mut body: Option = None; - - if let Some(obj) = input_obj { + if let Some(obj) = inputs { for (key, value) in obj { - let placeholder = format!("{{{key}}}"); - if url_path.contains(&placeholder) { - let rendered = value_to_path_segment(value); - url_path = url_path.replace(&placeholder, &rendered); - } else if key == "body" { + if is_path_placeholder(key, path_template) { + continue; + } + if key == "body" { body = Some(value.clone()); } else { query_params.push((key.clone(), value_to_query(value))); @@ -71,11 +69,8 @@ pub(crate) fn build_request( } } - let base = Url::parse(base_url) - .map_err(|e| CallError::internal(format!("invalid base_url `{base_url}`: {e}")))?; - let mut url = base - .join(url_path.trim_start_matches('/')) - .map_err(|e| CallError::internal(format!("invalid path `{url_path}`: {e}")))?; + let rendered_path = render_path_template(path_template, inputs)?; + let mut url = assemble_request_url(base_url, &rendered_path)?; if !query_params.is_empty() { let mut pairs = url.query_pairs_mut(); for (k, v) in &query_params { @@ -130,14 +125,190 @@ pub(crate) fn build_request( Ok((http_method, url, body, headers)) } +/// Percent-encode set for a spliced path-parameter value: the WHATWG +/// path set (controls, space, `"`, `<`, `>`, `` ` ``, `#`, `?`, `{`, `}`) +/// plus `/` so a value stays one literal segment, plus `%` `?` `#` +/// belt-and-braces, plus `\` so a Windows-style separator cannot smuggle +/// a backslash segment on special-scheme URLs. +/// +/// Because `%` is in this set, percent-encoding with it is idempotent: +/// every `%` in a value becomes `%25`, so the result contains `%` only +/// as the lead byte of an escape this crate itself produced. +const PATH_VALUE_ENCODE_SET: &AsciiSet = &CONTROLS + .add(b' ') + .add(b'"') + .add(b'<') + .add(b'>') + .add(b'`') + .add(b'#') + .add(b'?') + .add(b'{') + .add(b'}') + .add(b'/') + .add(b'%') + .add(b'\\'); + +/// Encode set for the second pass over a rendered segment in +/// [`request_path`]: applied to the text *between* `%` characters, so +/// anything that still looks like a separator is encoded, while the `%` +/// itself is left untouched to preserve this crate's own `%2F`-style +/// escapes. +const PATH_AFTER_PERCENT_ENCODE_SET: &AsciiSet = &CONTROLS + .add(b' ') + .add(b'"') + .add(b'<') + .add(b'>') + .add(b'`') + .add(b'#') + .add(b'?') + .add(b'{') + .add(b'}') + .add(b'/') + .add(b'\\'); + +/// Percent-encoding scheme used by [`render_path_template`]: a value +/// containing `/`, `?`, `#`, or `\` — and any raw `%` it carries — is +/// encoded so that a rendered value stays one literal path segment +/// (FWD-01 traversal/ smuggled-segments gate). Path parameters are never +/// rejected: they are rendered safely instead. pub(crate) fn value_to_path_segment(value: &Value) -> String { - match value { + let raw = match value { Value::String(s) => s.clone(), Value::Number(n) => n.to_string(), Value::Bool(b) => b.to_string(), Value::Null => String::new(), other => other.to_string(), + }; + utf8_percent_encode(&raw, PATH_VALUE_ENCODE_SET).to_string() +} + +/// Single-pass template renderer. Two invariants instead of the old +/// iterative `replace` (FWD-01): +/// +/// 1. A rendered value is never re-scanned — each template placeholder +/// is replaced exactly once, so a `{repo}` value that itself contains +/// a later placeholder string cannot trigger a second substitution. +/// 2. The whole template and every input key are walked in one pass, so +/// `?` and `#` in a value split into percent-encoded octets (`%3F`, +/// `%23`) rather than injecting URL structure. +pub(crate) fn render_path_template( + template: &str, + inputs: Option<&serde_json::Map>, +) -> Result { + let mut out = String::with_capacity(template.len()); + let mut rest = template; + let mut unresolved: Vec = Vec::new(); + while let Some(start) = rest.find('{') { + let (head, tail) = rest.split_at(start); + out.push_str(head); + let Some(end) = tail.find('}') else { + return Err(CallError::internal(format!( + "invalid path template `{template}`: unterminated placeholder" + ))); + }; + let name = &tail[1..end]; + let raw_value = match inputs.and_then(|map| map.get(name)) { + Some(v) => v, + None => { + unresolved.push(format!("{{{name}}}")); + rest = &tail[end + 1..]; + continue; + } + }; + out.push_str(&value_to_path_segment(raw_value)); + rest = &tail[end + 1..]; } + out.push_str(rest); + if !unresolved.is_empty() { + return Err(CallError::internal(format!( + "path template `{template}` references unbound placeholder(s): {}", + unresolved.join(", ") + ))); + } + Ok(out) +} + +/// True when `input_name` is consumed by a `{input_name}` placeholder in +/// the path template. Drives input routing in [`build_request`]; the +/// value itself is rendered by [`render_path_template`]. +pub(crate) fn is_path_placeholder(input_name: &str, template: &str) -> bool { + let placeholder = format!("{{{input_name}}}"); + template.contains(&placeholder) +} + +fn parse_base_url(base_url: &str) -> Result { + let parsed = Url::parse(base_url) + .map_err(|e| CallError::internal(format!("invalid base_url `{base_url}`: {e}")))?; + let scheme = parsed.scheme(); + if scheme != "https" && scheme != "http" { + return Err(CallError::internal(format!( + "base_url `{base_url}` must use https (or http for plain non-TLS origins); `{scheme}` is not an HTTP scheme" + ))); + } + let host = parsed.host_str().unwrap_or_default(); + if host.is_empty() { + return Err(CallError::internal(format!( + "base_url `{base_url}` must include an explicit host" + ))); + } + if !parsed.username().is_empty() || parsed.password().is_some() { + return Err(CallError::internal(format!( + "base_url `{base_url}` must not embed userinfo; credentials are injected per-operation from Capabilities" + ))); + } + Ok(parsed) +} + +fn request_path(rendered_path: &str) -> Result { + let trimmed = rendered_path.trim_matches('/'); + if trimmed.is_empty() { + return Err(CallError::internal( + "path template resolves to an empty request path; at least one segment is required", + )); + } + let mut path = String::new(); + for segment in trimmed.split('/') { + path.push('/'); + let mut pieces = segment.split('%'); + utf8_percent_encode_into(&mut path, pieces.next().unwrap_or_default()); + for piece in pieces { + path.push('%'); + utf8_percent_encode_into(&mut path, piece); + } + } + Ok(path) +} + +fn utf8_percent_encode_into(out: &mut String, text: &str) { + for piece in utf8_percent_encode(text, PATH_AFTER_PERCENT_ENCODE_SET) { + out.push_str(piece); + } +} + +fn assemble_request_url(base_url: &str, rendered_path: &str) -> Result { + let base = parse_base_url(base_url)?; + let request_path = request_path(rendered_path)?; + let base_path = base.path(); + let base_dir = match base_path.strip_suffix('/') { + Some(stripped) => stripped, + None => base_path, + }; + let mut full_path = String::with_capacity(base_dir.len() + request_path.len() + 1); + full_path.push_str(base_dir); + full_path.push_str(&request_path); + let mut url = base.clone(); + url.set_path(&full_path); + let same_origin = url.scheme() == base.scheme() + && url.host() == base.host() + && url.port_or_known_default() == base.port_or_known_default(); + if !same_origin { + return Err(CallError::internal(format!( + "request path `{rendered_path}` resolved against `{base_url}` changed the target origin: {} != {}", + url.origin().ascii_serialization(), + base.origin().ascii_serialization() + ))); + } + Ok(url) } pub(crate) fn value_to_query(value: &Value) -> String { @@ -552,3 +723,201 @@ impl SseParser { }) } } + +#[cfg(test)] +mod tests { + use super::*; + use alkcall::core::types::Capabilities; + use alkcall::registry::context::{AbortPolicy, ScopedPeerEnv}; + use serde_json::json; + use std::collections::HashMap as TestHashMap; + use std::sync::Arc as TestArc; + use std::time::Duration; + + fn noop_context() -> OperationContext { + struct NoopEnv; + #[async_trait::async_trait] + impl alkcall::registry::env::OperationEnv for NoopEnv { + async fn invoke_with_policy( + &self, + _ns: &str, + _op: &str, + _input: Value, + parent: &OperationContext, + _policy: AbortPolicy, + ) -> ResponseEnvelope { + ResponseEnvelope::ok(parent.request_id.clone(), Value::Null) + } + fn contains(&self, _name: &str) -> bool { + false + } + } + OperationContext { + request_id: "req-fwd".to_string(), + parent_request_id: None, + identity: None, + handler_identity: None, + forwarded_for: None, + capabilities: Capabilities::new(), + metadata: TestHashMap::new(), + scoped_env: ScopedPeerEnv::empty(), + env: TestArc::new(NoopEnv), + abort_policy: AbortPolicy::default(), + deadline: Some(std::time::Instant::now() + Duration::from_secs(30)), + internal: true, + ownership: None, + } + } + + fn request_url(base_url: &str, template: &str, input: Value) -> Result { + let ctx = noop_context(); + let (_, url, _, _) = build_request( + base_url, + template, + "GET", + &None, + &TestHashMap::new(), + "svc", + &input, + &ctx, + )?; + Ok(url) + } + + #[test] + fn traversal_value_cannot_escape_template_path() { + let url = request_url( + "https://api.example.com", + "/repos/{owner}/{repo}/issues", + json!({"owner": "../../admin", "repo": "x"}), + ) + .expect("request builds"); + assert_eq!(url.host_str(), Some("api.example.com")); + assert_eq!(url.path(), "/repos/..%2F..%2Fadmin/x/issues"); + assert!(!url.path().contains("/admin")); + } + + #[test] + fn structural_characters_cannot_split_or_inject_url_parts() { + let url = request_url( + "https://api.example.com", + "/files/{name}", + json!({"name": "a?via=query#frag"}), + ) + .expect("request builds"); + assert_eq!(url.query(), None, "`?` in a value must be encoded"); + assert_eq!(url.fragment(), None, "`#` in a value must be encoded"); + assert_eq!(url.path(), "/files/a%3Fvia=query%23frag"); + + let url = request_url( + "https://api.example.com", + "/files/{name}", + json!({"name": "a b/c\\d"}), + ) + .expect("request builds"); + assert_eq!(url.path_segments().map(|s| s.count()), Some(2)); + assert_eq!(url.path(), "/files/a%20b%2Fc%5Cd"); + + let url = request_url( + "https://api.example.com", + "/files/{name}", + json!({"name": "héllo→世界"}), + ) + .expect("request builds"); + assert_eq!(url.path(), "/files/h%C3%A9llo%E2%86%92%E4%B8%96%E7%95%8C"); + } + + #[test] + fn rendering_is_single_pass_and_never_re_substitutes() { + let url = request_url( + "https://api.example.com", + "/x/{a}/{b}", + json!({"a": "{b}", "b": "second"}), + ) + .expect("request builds"); + assert_eq!(url.path(), "/x/%7Bb%7D/second"); + + let rendered = render_path_template( + "/x/{a}", + Some(&json!({"a": "../../{b}"}).as_object().unwrap().clone()), + ) + .expect("renders"); + assert_eq!(rendered, "/x/..%2F..%2F%7Bb%7D"); + } + + #[test] + fn base_path_prefix_is_preserved() { + let url = request_url("https://api.openai.com/v1", "/chat/completions", json!({})) + .expect("request builds"); + assert_eq!(url.path(), "/v1/chat/completions"); + + let url = + request_url("https://api.example.com", "/data", json!({})).expect("request builds"); + assert_eq!(url.path(), "/data"); + } + + #[test] + fn absolute_url_in_input_cannot_change_origin_or_hop_paths() { + let url = request_url( + "https://api.example.com", + "/fetch/{url}", + json!({"url": "http://169.254.169.254/latest/meta-data"}), + ) + .expect("absolute URL in a path value stays an encoded segment"); + assert_eq!(url.host_str(), Some("api.example.com")); + assert_eq!( + url.path(), + "/fetch/http:%2F%2F169.254.169.254%2Flatest%2Fmeta-data" + ); + + let url = request_url( + "https://api.example.com", + "/fetch/{target}", + json!({"target": "https://evil.example.com/x"}), + ) + .expect("https absolute URL also stays an encoded segment"); + assert_eq!(url.host_str(), Some("api.example.com")); + assert_eq!(url.path(), "/fetch/https:%2F%2Fevil.example.com%2Fx"); + } + + #[test] + fn unbound_and_malformed_templates_error_loudly() { + let err = request_url("https://api.example.com", "/x/{missing}", json!({})) + .expect_err("unbound placeholder must error"); + assert!(err.message.contains("unbound placeholder")); + + let err = request_url("https://api.example.com", "/x/{open", json!({})) + .expect_err("unterminated placeholder must error"); + assert!(err.message.contains("unterminated")); + + let err = request_url("https://api.example.com", "/x{missing}/a/b", json!({})) + .expect_err("partial render without the placeholder is still loud"); + assert!(err.message.contains("unbound placeholder")); + } + + #[test] + fn base_url_validation_rejects_bad_inputs() { + let err = request_url("ftp://api.example.com", "/x", json!({})) + .expect_err("non-http scheme must be rejected"); + assert!(err.message.contains("not an HTTP scheme")); + + let err = request_url("https://u:p@api.example.com", "/x", json!({})) + .expect_err("userinfo must be rejected"); + assert!(err.message.contains("userinfo")); + + let err = request_url("not a url at all", "/x", json!({})) + .expect_err("unparseable base must be rejected"); + assert!(err.message.contains("invalid base_url")); + } + + #[test] + fn query_values_remain_encoded_via_query_pairs_mut() { + let url = request_url( + "https://api.example.com", + "/search", + json!({"q": "a&b=c d", "lang": "en"}), + ) + .expect("request builds"); + assert_eq!(url.query(), Some("lang=en&q=a%26b%3Dc+d")); + } +}