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.
This commit is contained in:
2026-08-22 14:50:00 +01:00
parent c69210fd91
commit 64126db7d4
2 changed files with 37 additions and 10 deletions
+25 -8
View File
@@ -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
}
}
+12 -2
View File
@@ -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) {