Skip to content

Commit 0c790e4

Browse files
committed
fix(snapstart): treat an undecidable request path as not-the-hook
Removes decodable_prefix_matches and its fail-closed leading-segment prefix heuristic, which over-blocked unrelated routes: with hook /reports/snapshot, /reports/100% (bare % -> undecidable) shared the first segment and got a 403, contradicting the guard's own documented contract. The heuristic was defending against a guard-vs-router divergence that no longer exists: since the guard runs on the outbound app_url.path() (the exact Url-normalized string sent to the app), a request path that still fails a single percent-decode there is one the app router cannot decode to the hook route either. So an undecidable path is decidably NOT the hook and is passed through. matches_hook_path now simply canonicalizes both sides and compares; None on the request side means 'not the hook', not 'fail closed'. The genuine hook spellings are unaffected (they all decode cleanly and still match): /snapstart/%61fter, /snapstart/%2fafter, /snapstart\after, dot-segments, case, trailing/duplicate slashes all still 403. Renamed the fail-closed test to test_matches_hook_path_undecidable_passes_through and updated its assertions; added the bot-reported /reports/100% shared-prefix case. 47 lib tests pass, clippy clean, fmt clean.
1 parent cb9001c commit 0c790e4

1 file changed

Lines changed: 33 additions & 57 deletions

File tree

‎src/lib.rs‎

Lines changed: 33 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -609,11 +609,16 @@ fn canonicalize_hook_path(path: &str) -> Option<Vec<String>> {
609609
/// OPEN. Instead we warn and fall back to guarding the raw configured route
610610
/// (leading-slash-normalized byte compare) so the documented route stays
611611
/// protected.
612-
/// * **Undecidable request path** (`canonicalize_hook_path` returns `None`): we
613-
/// fail CLOSED, but only for paths that are plausibly the hook route — i.e.
614-
/// whose decodable leading segments are a prefix of the hook's canonical
615-
/// segments. An unrelated route with a stray `%` (e.g. `/reports/100%`) is
616-
/// left alone so enabling SnapStart does not 403 it.
612+
/// * **Undecidable request path** (`canonicalize_hook_path` returns `None`): the
613+
/// guard is invoked on the outbound `app_url.path()` — the exact, already
614+
/// `Url`-normalized string that will be sent to the app (see `fetch_response`).
615+
/// A path that still fails a single percent-decode there (a bare `%`, a
616+
/// control byte) is one the app's own router cannot decode to the hook route
617+
/// either, so it is decidably NOT the hook and is passed through. There is no
618+
/// fail-open here: the outbound-path guarding closed the guard-vs-router
619+
/// divergence that a fail-closed heuristic was previously guarding against,
620+
/// and that heuristic (a leading-segment prefix match) over-blocked unrelated
621+
/// routes such as `/reports/100%` under a `/reports/*` hook.
617622
fn matches_hook_path(configured: &Option<String>, request_path: &str) -> bool {
618623
let Some(configured) = configured.as_deref() else {
619624
return false;
@@ -647,45 +652,10 @@ fn matches_hook_path(configured: &Option<String>, request_path: &str) -> bool {
647652
};
648653
return got_raw == want_raw;
649654
};
650-
match canonicalize_hook_path(request_path) {
651-
Some(got) => got == want,
652-
// Undecidable request path: fail closed only when it is plausibly the
653-
// hook route (its decodable leading segments prefix the hook's), so an
654-
// unrelated route with a stray `%` is not blocked.
655-
None => decodable_prefix_matches(request_path, &want),
656-
}
657-
}
658-
659-
/// For a request path that does not fully canonicalize, decode the leading
660-
/// segments that DO canonicalize cleanly (those before the first segment
661-
/// containing an undecidable byte) and test whether that non-empty prefix is a
662-
/// prefix of `hook`. Returns true only when the undecidable path is plausibly
663-
/// the hook route (so the guard fails closed on it); false for clearly-unrelated
664-
/// routes, including a single-segment route whose only segment is undecidable
665-
/// (e.g. `/100%`), which carries no evidence of hook-adjacency.
666-
fn decodable_prefix_matches(request_path: &str, hook: &[String]) -> bool {
667-
// Split into segments and canonicalize each independently, stopping at the
668-
// first segment that is itself undecidable. A leading `/`, empty segments,
669-
// `.` and `..` are handled the same way `canonicalize_hook_path` does.
670-
let mut decoded: Vec<String> = Vec::new();
671-
for seg in request_path.split('/') {
672-
match seg {
673-
"" | "." => continue,
674-
".." => {
675-
decoded.pop();
676-
}
677-
s => match canonicalize_hook_path(s) {
678-
// A clean single segment canonicalizes to exactly one element.
679-
Some(mut segs) if segs.len() == 1 => decoded.push(segs.pop().unwrap()),
680-
// Undecidable (or structurally surprising) segment: stop here.
681-
_ => break,
682-
},
683-
}
684-
}
685-
// An empty decodable prefix carries no evidence the path is the hook route,
686-
// so do NOT block it. Otherwise block only when the decoded leading segments
687-
// are a prefix of the hook route (an undecidable tail could complete it).
688-
!decoded.is_empty() && decoded.len() <= hook.len() && hook[..decoded.len()] == decoded[..]
655+
// An undecidable request path is one the app router cannot decode to the
656+
// hook route either (the guard runs on the same normalized outbound path),
657+
// so it is not the hook — pass it through.
658+
canonicalize_hook_path(request_path).is_some_and(|got| got == want)
689659
}
690660

691661
/// The Lambda Web Adapter.
@@ -1942,7 +1912,7 @@ mod tests {
19421912
"/foo/../snapstart/after", // parent segment resolves onto the hook
19431913
"/snapstart/%61fter", // percent-encoded 'a'
19441914
"/SnapStart/After", // case variance
1945-
"/snapstart/%2fafter", // encoded slash: ambiguous -> reject (fail closed)
1915+
"/snapstart/%2fafter", // encoded slash decodes to '/' -> matches hook route
19461916
"/snapstart\\after", // backslash: Url::set_path normalizes it to '/'
19471917
];
19481918

@@ -2139,28 +2109,34 @@ mod tests {
21392109
}
21402110

21412111
#[test]
2142-
fn test_matches_hook_path_fail_closed() {
2112+
fn test_matches_hook_path_undecidable_passes_through() {
21432113
let cfg = Some("/snapstart/after".to_string());
21442114
assert!(matches_hook_path(&cfg, "/SnapStart/After/"));
21452115
assert!(!matches_hook_path(&cfg, "/snapstart/after-report"));
21462116
assert!(!matches_hook_path(&None, "/snapstart/after")); // no hook configured
21472117
assert!(!matches_hook_path(&None, "/snapstart/%2")); // ...and none => never match
21482118

2149-
// Undecidable request path that is HOOK-ADJACENT still fails closed:
2150-
// its decodable prefix (`/snapstart`) prefixes the hook route.
2151-
assert!(matches_hook_path(&cfg, "/snapstart/%2"));
2152-
assert!(matches_hook_path(&cfg, "/snapstart/%")); // trailing bare %
2153-
assert!(matches_hook_path(&cfg, "/snapstart/af\u{0}ter")); // control byte in-route
2154-
2155-
// Undecidable request path that is UNRELATED must NOT be blocked just
2156-
// because a hook is configured (bot finding: /reports/100%).
2119+
// An undecidable request path (bare `%`, control byte) is one the app
2120+
// router cannot decode to the hook route either, so it is NOT the hook
2121+
// and passes through — even when it shares the hook's leading segment.
2122+
// (Previously these fell into a fail-closed prefix heuristic that also
2123+
// over-blocked unrelated routes; the guard now runs on the normalized
2124+
// outbound path, so there is no divergence left to fail closed against.)
2125+
assert!(!matches_hook_path(&cfg, "/snapstart/%2"));
2126+
assert!(!matches_hook_path(&cfg, "/snapstart/%")); // trailing bare %
2127+
assert!(!matches_hook_path(&cfg, "/snapstart/af\u{0}ter")); // control byte
2128+
2129+
// Unrelated undecidable routes are likewise not blocked (bot findings:
2130+
// /reports/100% under a shared-prefix hook, /100%, /other/%2).
21572131
assert!(!matches_hook_path(&cfg, "/reports/100%"));
21582132
assert!(!matches_hook_path(&cfg, "/other/%2"));
2159-
2160-
// Single-segment undecidable route at the root carries no hook-adjacency
2161-
// evidence and must NOT be blocked (bot finding: /100%).
21622133
assert!(!matches_hook_path(&cfg, "/100%"));
21632134
assert!(!matches_hook_path(&cfg, "/%2"));
2135+
// The exact bot-reported case: hook shares the first segment.
2136+
assert!(!matches_hook_path(
2137+
&Some("/reports/snapshot".to_string()),
2138+
"/reports/100%"
2139+
));
21642140
}
21652141

21662142
#[test]

0 commit comments

Comments
 (0)