From 64126db7d47d1e34483126f21dfecae4ea5063d8 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sat, 22 Aug 2026 14:33:56 +0100 Subject: [PATCH] docs(passthrough): drop out-of-scope "." rejection, correct %25 comment Two doc/scope corrections from the round-3, 2026-08-22 review (FOLDED + SHOULD-5), no behavior change to the traversal bypasses finding 4 itself fixed: - hasTraversalSegment rejected an exact "." segment alongside "..", outside finding 4's actual scope: "v1/x/./y" is a legal, non-traversal path and was silently getting 400'd. Dropped the "." check; only ".." is rejected again, matching every other traversal rule this function enforces. - The double-encoding rejection's own comment claimed it "rejects an intentionally double-encoded %25" -- overstated. "%25" is the correct, single, standards-conforming encoding of a literal "%" (RFC 3986); after exactly one unescape pass, a legitimate single-encoded literal "%" is byte-for-byte indistinguishable from a genuine double-encoding. The behavior (fail closed) is still correct and unchanged -- refusing an occasional literal "%" is a far smaller cost than admitting a real double-encoded traversal -- but the comment now says so honestly, including the concrete false-positive example ("v1/100%25done" decodes to "v1/100%done" and is rejected even though it names a real, non-malicious resource). Tests: added "v1/x/./y" (must not 400) and "v1/100%25done" (the documented false positive, still 403s) to hasTraversalSegment's table. --- routes_passthrough.go | 33 +++++++++++++++++++++++++-------- routes_passthrough_test.go | 14 ++++++++++++-- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/routes_passthrough.go b/routes_passthrough.go index 6c45a09..00b9edc 100644 --- a/routes_passthrough.go +++ b/routes_passthrough.go @@ -295,8 +295,8 @@ func passthroughRoute(path string) (providerName, rest string, ok bool) { } // hasTraversalSegment reports whether rest, once percent-decoded and -// normalized, contains a path segment that resolves to ".." or ".". rest -// is still in its escaped form here (see passthroughRoute) — decoding it +// normalized, contains a path segment that resolves to exactly "..". +// rest is still in its escaped form here (see passthroughRoute) — decoding it // is a validation-only step, never used to build the outgoing upstream // URL, so a legitimate percent-encoded segment (e.g. "a%2Fb" naming a // literal "a/b" resource) still reaches the upstream exactly as the @@ -317,12 +317,14 @@ func passthroughRoute(path string) (providerName, rest string, ok bool) { // convention): a segment's own identity is everything BEFORE its // first ";" — an upstream honoring that convention resolves // "..;foo=bar" identically to "..". Every segment has its -// ";"-suffix stripped before the ".."/"." comparison below. +// ";"-suffix stripped before the ".." comparison below. +// // - Backslash normalization ("..%5c..%5cx", decoding to "..\..\x"): // Windows/.NET and other backslash-normalizing upstreams treat "\" // as an equivalent path separator. The fully decoded string has // every "\" replaced with "/" before splitting, so a segment // hidden behind a backslash is caught the same as one behind "/". +// // - Double-encoding ("%252e%252e/x"): one url.PathUnescape pass // decodes this to "%2e%2e/x" — still containing "%", meaning a // SECOND decode pass (which this function deliberately never @@ -331,10 +333,25 @@ func passthroughRoute(path string) (providerName, rest string, ok bool) { // still contains "%" after the one legitimate unescape pass is // rejected outright — fail closed on ambiguity, matching this // function's existing rule for a rest that fails to unescape at -// all. This does reject an operator's literal, intentionally -// double-encoded "%25" in a resource name; accepted, since this -// package has no legitimate use for one and the alternative is an -// unbounded decode loop. +// all. +// +// CORRECTED (round 3, 2026-08-22 review): this rule's own comment +// used to say it "rejects an intentionally double-encoded %25" — +// that is not quite right and overstated the certainty. "%25" is +// the CORRECT, single, standards-conforming encoding of a literal +// "%" character (RFC 3986). After exactly one unescape pass, a +// legitimately single-encoded literal "%" is byte-for-byte +// indistinguishable from a genuinely double-encoded sequence (e.g. +// "%252e") that would reveal a hidden "..%2e" on a second pass — +// there is no way to tell them apart from the decoded bytes alone. +// The false-positive cost is real and known: a rest like +// "v1/100%25done" (a literal "100%done" resource path) decodes once +// to "v1/100%done", still contains "%", and is rejected here even +// though the ORIGINAL request carried no traversal attempt at all. +// Fail-closed on this ambiguity is still the correct call — refusing +// an occasional legitimate literal "%" is a far smaller cost than +// admitting a real double-encoded traversal — but the comment should +// not have implied the rejected input was provably malicious. // // Literal ".." and single-encoded "..%2f" keep their existing, already // correct behavior — both still resolve to a segment of exactly "..". @@ -351,7 +368,7 @@ func hasTraversalSegment(rest string) bool { if i := strings.IndexByte(seg, ';'); i >= 0 { seg = seg[:i] } - if seg == ".." || seg == "." { + if seg == ".." { return true } } diff --git a/routes_passthrough_test.go b/routes_passthrough_test.go index d6a0e13..4095afe 100644 --- a/routes_passthrough_test.go +++ b/routes_passthrough_test.go @@ -225,11 +225,21 @@ func TestHasTraversalSegment(t *testing.T) { // a second time). {"%252e%252e/x", true}, {"%252E%252E/x", true}, - {"a%25b", true}, // a literal, intentionally double-encoded "%25" (a percent sign) is refused too — accepted trade-off, no legitimate use in this package + // A false positive, KNOWN and accepted (SHOULD-5, round 3 + // review, corrected doc comment): "%25" is the correct, single + // encoding of a literal "%", indistinguishable after one decode + // pass from a genuine double-encoding — this rejects a + // completely benign request naming a literal "%" in its path. + {"a%25b", true}, + {"v1/100%25done", true}, // decodes once to "v1/100%done" — a real resource path, still rejected // Existing correct behavior must be unchanged: a legitimate - // single-encoded "/" within one segment, and literal "..". + // single-encoded "/" within one segment, literal "..", and a + // literal "." segment (dropped from finding 4's scope, round 3 + // review — "v1/x/./y" is a legal, non-traversal path and must + // not 400). {"a%2Fb%2Fc", false}, + {"v1/x/./y", false}, } for _, tt := range tests { t.Run(tt.rest, func(t *testing.T) {