Merge branch 'wt/review-002-fwd13-dot-segments'

src/adapters/forward.rs

@@ -127,7 +127,7 @@ use alkcall::registry::context::OperationContext;
 use alkcall::registry::registration::ResponseStream;
 use futures::stream;
 use futures::StreamExt;
-use percent_encoding::{utf8_percent_encode, AsciiSet, CONTROLS};
+use percent_encoding::{percent_decode_str, utf8_percent_encode, AsciiSet, CONTROLS};
 use reqwest::header::{HeaderMap, HeaderName, HeaderValue, ACCEPT, AUTHORIZATION, CONTENT_TYPE};
 use reqwest::Method;
 use serde_json::Value;
@@ -486,12 +486,27 @@ fn scalar_value_to_string(value: &Value) -> String {
 /// 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). Scalars are never
-/// rejected: they are rendered safely instead. Object/array values
-/// under a placeholder key are rejected with `INVALID_INPUT` (FWD-18):
-/// a path placeholder designates exactly one literal segment, so a
-/// structural value has no faithful rendering — the pre-decision
-/// behavior spliced the minified JSON into the path *and* emitted the
-/// key as a query parameter (double-routed, neither faithful).
+/// rejected: they are rendered safely instead — with the lone-dot
+/// exception below (FWD-13). Object/array values under a placeholder
+/// key are rejected with `INVALID_INPUT` (FWD-18): a path placeholder
+/// designates exactly one literal segment, so a structural value has no
+/// faithful rendering — the pre-decision behavior spliced the minified
+/// JSON into the path *and* emitted the key as a query parameter
+/// (double-routed, neither faithful).
+///
+/// # Lone-dot values (FWD-13)
+///
+/// A scalar whose *decoded* form is exactly `.` or `..` — including the
+/// `%2e`/`%2E` spellings, which `Url::set_path` normalizes away after
+/// decoding regardless of what the encode set preserves — is rejected
+/// with `INVALID_INPUT`. `Url::set_path` removes lone dot segments
+/// case-insensitively (`/../x` → `/x`, `/./x` → `/x`, `/%2e%2e/x` →
+/// `/x`), so no encode-set fix can keep such a value faithful: the
+/// upstream would receive a *different* endpoint than the template
+/// describes, with the namespace's credentials attached. The rejection
+/// is exact-match on the full decoded segment; values like `v1.2.3` or
+/// `.hidden-file` render normally. The parameter name is quoted but the
+/// value is never echoed.
 pub(crate) fn value_to_path_segment(value: &Value) -> Result<String, CallError> {
     let raw = match value {
         Value::String(_) | Value::Number(_) | Value::Bool(_) | Value::Null => {
@@ -504,9 +519,29 @@ pub(crate) fn value_to_path_segment(value: &Value) -> Result<String, CallError>
             )))
         }
     };
+    reject_lone_dot_value(value, &raw)?;
     Ok(utf8_percent_encode(&raw, PATH_VALUE_ENCODE_SET).to_string())
 }
 
+/// FWD-13 gate: a decoded path-value of exactly `.` or `..` cannot be
+/// rendered faithfully — `Url::set_path` silently normalizes lone dot
+/// segments away, so the upstream would receive a different endpoint
+/// than the template describes. The parameter is named but the value is
+/// not echoed. Called with both the original [`Value`] (for type-naming)
+/// and its raw scalar string.
+fn reject_lone_dot_value(value: &Value, raw: &str) -> Result<(), CallError> {
+    let type_label = type_name_of(value);
+    let is_lone_dot = matches!(raw, "." | "..")
+        || raw.eq_ignore_ascii_case("%2e")
+        || raw.eq_ignore_ascii_case("%2e%2e");
+    if is_lone_dot {
+        return Err(CallError::invalid_input(format!(
+            "path placeholder value of type {type_label} decodes to a lone dot segment, which cannot appear in a rendered path (Url::set_path would silently normalize it away); pass a concrete non-dot value"
+        )));
+    }
+    Ok(())
+}
+
 /// Raw form of a scalar input value for query emission: `&`/`=`
 /// separation, escaping, and space encoding are handled by
 /// `url::query_pairs_mut` downstream, so the value itself must be raw —
@@ -632,6 +667,7 @@ fn assemble_request_url(base_url: &str, rendered_path: &str) -> Result<Url, Call
     full_path.push_str(&request_path);
     let mut url = base.clone();
     url.set_path(&full_path);
+    assert_untouched_by_normalization(&url, base_dir, rendered_path, base_url)?;
     let same_origin = url.scheme() == base.scheme()
         && url.host() == base.host()
         && url.port_or_known_default() == base.port_or_known_default();
@@ -645,6 +681,48 @@ fn assemble_request_url(base_url: &str, rendered_path: &str) -> Result<Url, Call
     Ok(url)
 }
 
+/// Post-`set_path` invariant (FWD-13): the decoded segments of the
+/// final URL path must equal `base_dir`'s segments plus the decoded
+/// rendered segments, byte-identical. `set_path` is the one step this
+/// crate does not control, and its parser normalizes lone dot segments
+/// away (`/../x` → `/x`); this check turns any future normalizer
+/// surprise into a loud `INTERNAL` error instead of a silently
+/// re-routed authenticated request.
+fn assert_untouched_by_normalization(
+    url: &Url,
+    base_dir: &str,
+    rendered_path: &str,
+    base_url: &str,
+) -> Result<(), CallError> {
+    let rendered_segments = rendered_path
+        .trim_matches('/')
+        .split('/')
+        .filter(|s| !s.is_empty())
+        .map(|s| percent_decode_str(s).decode_utf8_lossy().into_owned());
+    let base_segments: Vec<&str> = base_dir.split('/').filter(|s| !s.is_empty()).collect();
+    let expected: Vec<String> = base_segments
+        .iter()
+        .copied()
+        .map(str::to_string)
+        .chain(rendered_segments)
+        .collect();
+    let actual: Vec<String> = url
+        .path()
+        .split('/')
+        .filter(|s| !s.is_empty())
+        .map(|s| percent_decode_str(s).decode_utf8_lossy().into_owned())
+        .collect();
+    let matches = actual.len() == expected.len()
+        && std::iter::Iterator::zip(actual.iter(), expected.iter()).all(|(a, e)| a == e);
+    if matches {
+        Ok(())
+    } else {
+        Err(CallError::internal(format!(
+            "request path `{rendered_path}` resolved against `{base_url}` was rewritten by URL normalization: expected segments {expected:?}, got {actual:?}"
+        )))
+    }
+}
+
 #[derive(Debug, thiserror::Error)]
 pub(crate) enum BodyReadError {
     #[error("upstream response body exceeds the {RESPONSE_BODY_CAP}-byte response cap")]
@@ -1699,6 +1777,179 @@ mod tests {
         assert_eq!(url.path(), "/files/a%252Fb");
     }
 
+    /// Empirical pin of the failure mode FWD-13 describes, against the
+    /// locked `url` crate: lone `.`/`..` (and their percent-escaped
+    /// spellings) are silently *normalized away* by `Url::set_path`, so
+    /// the encode set alone cannot keep the rendered path faithful.
+    #[test]
+    fn url_set_path_normalizes_lone_dot_and_dot_dot_path_values() {
+        let mut url = Url::parse("https://api.example.com").expect("parses");
+        for (spliced, normalized) in [
+            ("/tenants/../resources", "/resources"),
+            ("/tenants/./resources", "/tenants/resources"),
+            ("/files/..", "/"),
+            ("/repos/%2E%2E/x", "/x"),
+        ] {
+            url.set_path(spliced);
+            assert_eq!(
+                url.path(),
+                normalized,
+                "set_path silently normalized `{spliced}`"
+            );
+        }
+    }
+
+    #[test]
+    fn lone_dot_dot_path_value_is_rejected() {
+        let err = request_url(
+            "https://api.example.com",
+            "/tenants/{tenant}/resources",
+            json!({"tenant": ".."}),
+        )
+        .expect_err("lone `..` value must be rejected");
+        assert_eq!(err.code, "INVALID_INPUT");
+        assert!(
+            err.message.contains("lone dot segment"),
+            "message must explain the lone-dot rejection: {}",
+            err.message
+        );
+        assert!(
+            !err.message.contains(".."),
+            "error must not echo the raw value: {}",
+            err.message
+        );
+    }
+
+    #[test]
+    fn lone_dot_path_value_is_rejected() {
+        let err = request_url(
+            "https://api.example.com",
+            "/tenants/{tenant}/resources",
+            json!({"tenant": "."}),
+        )
+        .expect_err("lone `.` value must be rejected");
+        assert_eq!(err.code, "INVALID_INPUT");
+    }
+
+    #[test]
+    fn percent_escapes_spellings_of_lone_dots_are_rejected() {
+        for value in ["\u{2e}\u{2e}", "%2e%2e", "%2E%2e", "%2e%2E"] {
+            let err = request_url(
+                "https://api.example.com",
+                "/tenants/{tenant}/resources",
+                json!({ "tenant": value }),
+            )
+            .expect_err("escaped lone-dot spellings must be rejected");
+            assert_eq!(err.code, "INVALID_INPUT", "value was: {value}");
+        }
+
+        for value in ["%2e", "%2E"] {
+            let err = request_url(
+                "https://api.example.com",
+                "/tenants/{tenant}/resources",
+                json!({ "tenant": value }),
+            )
+            .expect_err("escaped lone-dot spellings must be rejected");
+            assert_eq!(err.code, "INVALID_INPUT", "value was: {value}");
+        }
+    }
+
+    #[test]
+    fn dotted_but_not_lone_dot_values_still_render() {
+        for value in [
+            "v1.2.3",
+            ".hidden-file",
+            "..hidden",
+            "hidden..",
+            "a..b",
+            ".a.b.",
+            "...",
+        ] {
+            let url = request_url(
+                "https://api.example.com",
+                "/files/{name}",
+                json!({ "name": value }),
+            )
+            .unwrap_or_else(|e| panic!("value `{value}` must render: {e:?}"));
+            assert!(
+                url.path().starts_with("/files/"),
+                "value `{value}` rendered into the path"
+            );
+        }
+    }
+
+    /// FWD-13 belt-and-braces: across a dot/percent/binary corpus the
+    /// post-`set_path` invariant holds — the decoded URL path segments
+    /// equal the base dir plus the decoded rendered segments,
+    /// byte-identical — so a normalizer rewrite (today's lone-dot
+    /// removal, tomorrow's regression in the `url` crate) can never
+    /// silently re-route an authenticated request. Lone-dot values are
+    /// rejected outright; values containing `/` or `\` are rejected as
+    /// smuggled separators only if the encode-set layer ever regressed.
+    #[test]
+    fn post_set_path_invariant_holds_across_dot_percent_binary_corpus() {
+        let corpus = [
+            "v1.2.3",
+            ".hidden-file",
+            "..",
+            ".",
+            "%2e",
+            "%2E",
+            "%2e%2e",
+            "%2E%2E",
+            "%252e",
+            "hidden..",
+            "a..b",
+            "...",
+            "a%2Fb",
+            "a b",
+            "h\\éllo→世界",
+            "line\nbreak",
+            "tab\tchar",
+            "\u{7f}\u{1f600}",
+        ];
+        for value in corpus {
+            let outcome = request_url(
+                "https://api.example.com/v1",
+                "/files/{name}",
+                json!({ "name": value }),
+            );
+            match outcome {
+                Ok(url) => {
+                    assert_eq!(url.host_str(), Some("api.example.com"));
+                    let segments: Vec<String> = url
+                        .path_segments()
+                        .map(|s| {
+                            s.map(|seg| percent_decode_str(seg).decode_utf8_lossy().into_owned())
+                                .collect()
+                        })
+                        .unwrap_or_default();
+                    assert_eq!(
+                        segments.len(),
+                        3,
+                        "value `{value:?}` must render as one literal segment under /v1/files/"
+                    );
+                    assert_eq!(
+                        segments.get(2).map(String::as_str),
+                        Some(value),
+                        "value `{value:?}` must survive byte-identical as the final segment"
+                    );
+                    assert!(
+                        !segments.iter().any(|s| s == "." || s == ".."),
+                        "value `{value:?}` must not leave lone dot segments: {:?}",
+                        segments
+                    );
+                }
+                Err(err) => {
+                    assert_eq!(
+                        err.code, "INVALID_INPUT",
+                        "value `{value:?}` may only fail as INVALID_INPUT"
+                    );
+                }
+            }
+        }
+    }
+
     fn ctx_with_capability(namespace: &str, value: String) -> OperationContext {
         let mut ctx = noop_context();
         ctx.capabilities = Capabilities::new().with_http_token(namespace, value);