Skip to content

Commit 3623ee1

Browse files
committed
fix(snapstart): decode hook path a single pass to match the app router
Bot review of 5ee28f2 found a false-positive 403 in the hook guard: canonicalize_hook_path decoded percent-escapes repeatedly (up to 8 passes), but the downstream app router decodes exactly once. A validly single-encoded literal percent — e.g. /reports/100%25 meaning /reports/100% — decoded to /reports/100% on pass 1 and to None (bare %) on pass 2, so canonicalize returned None and decodable_prefix_matches 403'd it whenever a hook shared its first segment. The repeated decode also bought no real protection: a router seeing /snapstart/%2561fter decodes it once to the literal /snapstart/%61fter, which is NOT the hook route, so it should not have been blocked either. Decode a single pass, matching the router. /snapstart/%61fter still canonicalizes to /snapstart/after and is guarded; /snapstart/%2561fter and /reports/100%25 are now correctly treated as distinct literal routes and pass through. Updated the equivalence-class tests accordingly and added regressions for /reports/100%25 (not blocked) and the double-encoded literal. 47 lib tests pass, clippy clean, fmt clean.
1 parent 5ee28f2 commit 3623ee1

1 file changed

Lines changed: 35 additions & 13 deletions

File tree

‎src/lib.rs‎

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -556,7 +556,7 @@ fn percent_decode_once(input: &str) -> Option<String> {
556556
///
557557
/// The guard must block *every* spelling that the downstream app router would
558558
/// resolve to the configured hook route, so this over-approximates: it
559-
/// percent-decodes (repeatedly, to collapse double-encoding), splits on `/`,
559+
/// percent-decodes (a single pass, matching the router), splits on `/`,
560560
/// drops empty segments (collapsing `//`, leading/trailing slashes), resolves
561561
/// `.`/`..`, and lowercases each segment.
562562
///
@@ -570,16 +570,16 @@ fn percent_decode_once(input: &str) -> Option<String> {
570570
/// non-UTF-8 after decoding, or a control/null byte). The caller treats `None`
571571
/// as "reject" (fail closed).
572572
fn canonicalize_hook_path(path: &str) -> Option<Vec<String>> {
573-
// Repeatedly percent-decode until stable, so `%2561` -> `%61` -> `a` and
574-
// `%2f` -> `/`. A malformed escape or non-UTF-8 result yields None (reject).
575-
let mut current = path.to_string();
576-
for _ in 0..8 {
577-
match percent_decode_once(&current) {
578-
Some(decoded) if decoded == current => break,
579-
Some(decoded) => current = decoded,
580-
None => return None, // malformed escape -> ambiguous -> reject
581-
}
582-
}
573+
// Percent-decode a SINGLE pass, mirroring what the downstream app router
574+
// does. A router decodes exactly once, so `/snapstart/%61fter` reaches the
575+
// app as `/snapstart/after` (and must be guarded), while `/snapstart/%2561fter`
576+
// reaches it as the literal `/snapstart/%61fter` (a different route the app
577+
// does NOT resolve to the hook). Decoding more than once would over-decode
578+
// relative to the router — buying no extra protection while making a validly
579+
// single-encoded path like `/reports/100%25` (i.e. `/reports/100%`) look
580+
// undecidable and get a false 403. A malformed escape or non-UTF-8 result
581+
// still yields None (reject).
582+
let current = percent_decode_once(path)?;
583583
// Reject any control/null byte on the hook-adjacent path.
584584
if current.bytes().any(|b| b < 0x20 || b == 0x7F) {
585585
return None;
@@ -1924,7 +1924,6 @@ mod tests {
19241924
"/snapstart/./after", // dot segment
19251925
"/foo/../snapstart/after", // parent segment resolves onto the hook
19261926
"/snapstart/%61fter", // percent-encoded 'a'
1927-
"/snapstart/%2561fter", // double-encoded 'a'
19281927
"/SnapStart/After", // case variance
19291928
"/snapstart/%2fafter", // encoded slash: ambiguous -> reject (fail closed)
19301929
];
@@ -2068,7 +2067,6 @@ mod tests {
20682067
"/snapstart/./after",
20692068
"/foo/../snapstart/after",
20702069
"/snapstart/%61fter",
2071-
"/snapstart/%2561fter",
20722070
"/SnapStart/After",
20732071
"/snapstart/%2fafter", // encoded slash decodes to a real slash
20742072
] {
@@ -2090,14 +2088,38 @@ mod tests {
20902088
"/snapstart/afterx",
20912089
"/hello",
20922090
"/foo/%2fbar",
2091+
// Single-pass decode: `%2561` decodes ONCE to the literal `%61fter`,
2092+
// which the app router does NOT resolve to `/snapstart/after`.
2093+
"/snapstart/%2561fter",
20932094
] {
20942095
assert_ne!(canonicalize_hook_path(distinct).as_ref(), Some(&hook), "{distinct:?}");
20952096
}
2097+
// A validly single-encoded literal percent (`%25` -> `%`) is DECIDABLE and
2098+
// must not fall into the fail-closed branch (regression: decode-until-stable
2099+
// used to 403 `/reports/100%25`). It canonicalizes to a concrete route.
2100+
assert_eq!(
2101+
canonicalize_hook_path("/reports/100%25"),
2102+
Some(vec!["reports".to_string(), "100%".to_string()]),
2103+
);
20962104
// Undecidable inputs -> None (caller fails closed).
20972105
assert_eq!(canonicalize_hook_path("/snapstart/%2"), None);
20982106
assert_eq!(canonicalize_hook_path("/snapstart/af\u{0}ter"), None);
20992107
}
21002108

2109+
/// Regression for the single-pass fix: with a hook that shares a first
2110+
/// segment, a validly single-encoded request under that segment must NOT be
2111+
/// blocked (bot finding: `/reports/100%25` vs hook `/reports/snapshot`).
2112+
#[test]
2113+
fn test_matches_hook_path_valid_encoded_percent_not_blocked() {
2114+
let cfg = Some("/reports/snapshot".to_string());
2115+
assert!(
2116+
!matches_hook_path(&cfg, "/reports/100%25"),
2117+
"/reports/100%25 must not 403"
2118+
);
2119+
// The genuine single-encoded hook spelling is still blocked.
2120+
assert!(matches_hook_path(&cfg, "/reports/%73napshot"));
2121+
}
2122+
21012123
#[test]
21022124
fn test_matches_hook_path_fail_closed() {
21032125
let cfg = Some("/snapstart/after".to_string());

0 commit comments

Comments
 (0)