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
This commit is contained in:
Generated
+1
@@ -64,6 +64,7 @@ dependencies = [
|
||||
"jsonschema",
|
||||
"openapiv3",
|
||||
"parking_lot",
|
||||
"percent-encoding",
|
||||
"reqwest",
|
||||
"reqwest-middleware",
|
||||
"reqwest-retry",
|
||||
|
||||
@@ -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"
|
||||
|
||||
+384
-15
@@ -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<Value>, 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<Value> = 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<String, Value>>,
|
||||
) -> Result<String, CallError> {
|
||||
let mut out = String::with_capacity(template.len());
|
||||
let mut rest = template;
|
||||
let mut unresolved: Vec<String> = 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<Url, CallError> {
|
||||
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<String, CallError> {
|
||||
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<Url, CallError> {
|
||||
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<url::Url, CallError> {
|
||||
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"));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user