fix(supervisor-network): canonicalize dot-segments before policy evaluation (#2699)

* fix(supervisor-network): strip path parameters before dot-segment resolution

Signed-off-by: Adrien Langou <alangou@nvidia.com>

* fix(supervisor-network): scope allow_encoded_slash to the matched L7 endpoint

Signed-off-by: Adrien Langou <alangou@nvidia.com>

* fix(supervisor-network): check the canonical target for encoded slashes

Signed-off-by: Adrien Langou <alangou@nvidia.com>

---------

Signed-off-by: Adrien Langou <alangou@nvidia.com>

Author: alangou

crates/openshell-supervisor-network/src/l7/path.rs

@@ -22,7 +22,11 @@
 //!   request-target, raw non-ASCII bytes, and paths that cannot be parsed
 //!   as origin-form.
 //! - Strip trailing `;params` from each segment by default (Tomcat-class
-//!   `;jsessionid` ACL-bypass mitigation).
+//!   `;jsessionid` ACL-bypass mitigation). Stripping happens *before*
+//!   dot-segment resolution so that `..;` cannot evade the traversal guard
+//!   and then revert to `..` on the way out.
+//! - Reject any `.`/`..` that survives to the end of canonicalization. The
+//!   policy engine trusts this function to have removed them.
 //! - Reject `%2F` (encoded slash) inside a segment by default. Operators
 //!   can opt in per-endpoint for APIs that rely on encoded slashes in
 //!   slugs.
@@ -44,6 +48,8 @@ pub enum CanonicalizeError {
     NonAscii,
     #[error("request-target's `..` segment would escape the path root")]
     TraversalAboveRoot,
+    #[error("request-target still contains a `.`/`..` segment after canonicalization")]
+    ResidualDotSegment,
     #[error("request-target exceeds the configured maximum length")]
     PathTooLong,
     #[error("request-target is not a valid origin-form path")]
@@ -172,10 +178,32 @@ pub fn canonicalize_request_target(
     //    with a real path separator.
     let decoded = percent_decode_with_sentinel(raw_path.as_bytes(), opts.allow_encoded_slash)?;
 
-    // 9. Split on literal `/` and resolve dot-segments.
+    // 9. Split on literal `/`, then strip `;params` *before* resolving
+    //    dot-segments. Stripping afterwards would let a `..;` segment slip
+    //    past the traversal guard (it is not byte-equal to `..`) and then
+    //    revert to a bare `..` during reconstruction.
     let segments = split_path_segments(&decoded);
+    let segments: Vec<&[u8]> = if opts.strip_path_parameters {
+        segments.into_iter().map(strip_path_parameters).collect()
+    } else {
+        segments
+    };
     let resolved = resolve_dot_segments(segments)?;
 
+    // 9b. Defense in depth: no `.`/`..` may survive canonicalization. The
+    //     policy engine trusts this function to have removed them, so a
+    //     residual dot-segment is always a bug or an attack. This also
+    //     catches a `..` hidden behind a `%2F` sentinel on endpoints that
+    //     opted into `allow_encoded_slash`, where the sentinel keeps the
+    //     dot-segment inside its segment and out of `resolve_dot_segments`.
+    for seg in &resolved {
+        for part in seg.split(|&b| b == ENCODED_SLASH_SENTINEL) {
+            if part == b".." || part == b"." {
+                return Err(CanonicalizeError::ResidualDotSegment);
+            }
+        }
+    }
+
     // 10. Reconstruct. Strip `;params` per segment if requested; re-encode
     //     any byte that must be percent-encoded in the pchar set.
     let canonical = build_canonical_path(&resolved, decoded.last().copied() == Some(b'/'), *opts);
@@ -190,6 +218,20 @@ pub fn canonicalize_request_target(
     ))
 }
 
+/// Report whether a canonical path carries a `%2F` that survived
+/// canonicalization.
+///
+/// Callers that canonicalize before they know which endpoint config applies
+/// use this to re-check the result against the config that actually matched.
+///
+/// The test is exact: [`build_canonical_path`] emits the literal `%2F` only
+/// for the encoded-slash sentinel, and percent-encodes any other `%` byte as
+/// `%25`, so no `%2F` substring can reach the output by another route.
+#[must_use]
+pub fn canonical_path_has_encoded_slash(canonical_path: &str) -> bool {
+    canonical_path.contains("%2F")
+}
+
 // ---------------------------------------------------------------------------
 // Internals
 // ---------------------------------------------------------------------------
@@ -238,6 +280,13 @@ fn percent_decode_with_sentinel(
     Ok(out)
 }
 
+/// Drop a trailing `;params` suffix from a single path segment.
+fn strip_path_parameters(seg: &[u8]) -> &[u8] {
+    seg.iter()
+        .position(|&b| b == b';')
+        .map_or(seg, |pos| &seg[..pos])
+}
+
 fn split_path_segments(decoded: &[u8]) -> Vec<&[u8]> {
     // decoded is guaranteed to start with `/`. Skip the leading `/` and
     // split on subsequent `/` bytes. The sentinel byte for encoded slashes
@@ -286,10 +335,7 @@ fn build_canonical_path(
             out.push('/');
         }
         let trimmed: &[u8] = if opts.strip_path_parameters {
-            match seg.iter().position(|&b| b == b';') {
-                Some(pos) => &seg[..pos],
-                None => seg,
-            }
+            strip_path_parameters(seg)
         } else {
             seg
         };
@@ -582,4 +628,124 @@ mod tests {
     fn regression_dot_slash_dotdot() {
         assert_eq!(canon("/public/./../secret").unwrap(), "/secret");
     }
+
+    // ---------------------------------------------------------------------
+    // A `;params` suffix on the dot segment itself must not let the segment
+    // slip past the traversal guard. Stripping used to run after dot-segment
+    // resolution, so `..;` was not byte-equal to `..` when the guard looked
+    // at it, and reverted to a bare `..` during reconstruction — handing the
+    // policy engine and the upstream a path that still escaped its prefix.
+    // ---------------------------------------------------------------------
+
+    #[test]
+    fn regression_dotdot_with_path_parameter_is_resolved_not_smuggled() {
+        assert_eq!(canon("/public/..;/secret").unwrap(), "/secret");
+        assert_eq!(canon("/public/..;x/secret").unwrap(), "/secret");
+        assert_eq!(
+            canon("/public/..;jsessionid=xyz/secret").unwrap(),
+            "/secret"
+        );
+        assert_eq!(canon("/public/.;/secret").unwrap(), "/public/secret");
+        assert_eq!(canon("/public/.;x/secret").unwrap(), "/public/secret");
+    }
+
+    #[test]
+    fn regression_percent_encoded_dotdot_with_path_parameter() {
+        // `%2e%2e;` and `..%3B` both decode to `..;` before segmentation.
+        assert_eq!(canon("/public/%2e%2e;/secret").unwrap(), "/secret");
+        assert_eq!(canon("/public/..%3B/secret").unwrap(), "/secret");
+        assert_eq!(canon("/public/..%3b/secret").unwrap(), "/secret");
+    }
+
+    #[test]
+    fn regression_chained_dotdot_with_path_parameters_hits_root_guard() {
+        assert_eq!(
+            canon("/public/..;/..;/secret"),
+            Err(CanonicalizeError::TraversalAboveRoot)
+        );
+        assert_eq!(
+            canon("/api/v1/..;/..;/..;/admin/keys"),
+            Err(CanonicalizeError::TraversalAboveRoot)
+        );
+        assert_eq!(canon("/..;"), Err(CanonicalizeError::TraversalAboveRoot));
+    }
+
+    #[test]
+    fn regression_dotdot_behind_encoded_slash_is_rejected_when_opted_in() {
+        // With `allow_encoded_slash`, the `%2F` sentinel keeps the dot-segment
+        // inside its segment, so `resolve_dot_segments` never sees it. The
+        // residual guard is what closes this.
+        let opts = CanonicalizeOptions {
+            allow_encoded_slash: true,
+            ..CanonicalizeOptions::default()
+        };
+        assert_eq!(
+            canon_with("/public/..%2fsecret", opts),
+            Err(CanonicalizeError::ResidualDotSegment)
+        );
+        assert_eq!(
+            canon_with("/public/..%2f..%2fsecret", opts),
+            Err(CanonicalizeError::ResidualDotSegment)
+        );
+        assert_eq!(
+            canon_with("/public/.%2fsecret", opts),
+            Err(CanonicalizeError::ResidualDotSegment)
+        );
+        // A legitimate encoded-slash slug still round-trips.
+        assert_eq!(
+            canon_with("/repos/group%2fproject/issues", opts).unwrap(),
+            "/repos/group%2Fproject/issues"
+        );
+    }
+
+    #[test]
+    fn encoded_slash_detection_on_canonical_paths_is_exact() {
+        let opts = CanonicalizeOptions {
+            allow_encoded_slash: true,
+            ..CanonicalizeOptions::default()
+        };
+
+        // A surviving sentinel is detected.
+        let slug = canon_with("/repos/group%2fproject/issues", opts).unwrap();
+        assert_eq!(slug, "/repos/group%2Fproject/issues");
+        assert!(canonical_path_has_encoded_slash(&slug));
+
+        // Ordinary paths are not.
+        assert!(!canonical_path_has_encoded_slash(
+            &canon("/public/secret").unwrap()
+        ));
+
+        // A literal `%` in the input is re-emitted as `%25`, so it cannot
+        // fabricate a `%2F` substring — including when the input spells out
+        // `%252F`, which decodes to the three characters `%`, `2`, `F`.
+        let escaped = canon("/a/%252F/b").unwrap();
+        assert_eq!(escaped, "/a/%252F/b");
+        assert!(!canonical_path_has_encoded_slash(&escaped));
+
+        let percent = canon("/a/100%25/b").unwrap();
+        assert_eq!(percent, "/a/100%25/b");
+        assert!(!canonical_path_has_encoded_slash(&percent));
+    }
+
+    #[test]
+    fn canonical_output_never_contains_dot_segments() {
+        // The contract the policy engine relies on: whatever comes back is
+        // free of `.`/`..`, so rego never has to defend against them.
+        for target in [
+            "/public/..;/secret",
+            "/public/.;/secret",
+            "/public/%2e%2e;/secret",
+            "/a;jsessionid=xyz/b",
+            "/public//../secret",
+            "/a/b/..",
+            "/a/b/.",
+        ] {
+            if let Ok(path) = canon(target) {
+                assert!(
+                    !path.split('/').any(|seg| seg == ".." || seg == "."),
+                    "{target} canonicalized to {path}, which still has a dot-segment"
+                );
+            }
+        }
+    }
 }