diff --git a/routes_passthrough.go b/routes_passthrough.go index 2739052..a9482d4 100644 --- a/routes_passthrough.go +++ b/routes_passthrough.go @@ -245,26 +245,64 @@ func passthroughRoute(path string) (providerName, rest string, ok bool) { return providerName, rest, true } -// hasTraversalSegment reports whether rest, once percent-decoded, contains -// a path segment exactly equal to "..". 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 client sent it. Checking the -// decoded form, not the escaped one, is what catches an encoded traversal -// attempt like "..%2f..%2fsecret": as a single escaped segment it has no -// literal "/" for a naive split to catch, but a permissive upstream -// server decoding that same "%2f" itself would read it as "../../secret" -// — this check rejects it here instead. A rest that fails to -// url.PathUnescape at all is rejected too: a malformed percent-encoding -// is not a path this gateway can reason about safely. +// 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 +// 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 +// client sent it. Checking the decoded form, not the escaped one, is what +// catches an encoded traversal attempt like "..%2f..%2fsecret": as a +// single escaped segment it has no literal "/" for a naive split to +// catch, but a permissive upstream server decoding that same "%2f" +// itself would read it as "../../secret" — this check rejects it here +// instead. A rest that fails to url.PathUnescape at all is rejected too: +// a malformed percent-encoding is not a path this gateway can reason +// about safely. +// +// Round 3 (security review, 2026-08-22) closes three further bypasses the +// single-pass decode-and-split above missed, each measured against a +// real upstream convention: +// +// - Path parameters ("..;/x", the Tomcat/Spring RFC 3986 §3.3 +// 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. +// - 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 +// performs — attempting one only invites indefinite re-encoding) +// would reveal a hidden "..". Rather than decode again, a rest that +// 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. +// +// Literal ".." and single-encoded "..%2f" keep their existing, already +// correct behavior — both still resolve to a segment of exactly "..". func hasTraversalSegment(rest string) bool { decoded, err := url.PathUnescape(rest) if err != nil { return true } - for _, seg := range strings.Split(decoded, "/") { - if seg == ".." { + if strings.Contains(decoded, "%") { + return true + } + normalized := strings.ReplaceAll(decoded, `\`, "/") + for _, seg := range strings.Split(normalized, "/") { + if i := strings.IndexByte(seg, ';'); i >= 0 { + seg = seg[:i] + } + if seg == ".." || seg == "." { return true } } diff --git a/routes_passthrough_test.go b/routes_passthrough_test.go index 4e70781..7515a92 100644 --- a/routes_passthrough_test.go +++ b/routes_passthrough_test.go @@ -206,6 +206,30 @@ func TestHasTraversalSegment(t *testing.T) { {"..%2f..%2fsecret", true}, {"..%2F..%2Fsecret", true}, {"%zz", true}, // invalid percent-encoding + + // Round 3 (security review, 2026-08-22): path-parameter form + // ("..;/x", Tomcat/Spring's RFC 3986 §3.3 convention) — the + // segment's identity is everything before its first ";". + {"..;/x", true}, + {"v1/foo;bar/x", false}, // a genuine path parameter on a non-".." segment is not itself traversal + {"a;../x", false}, // ";../" strips to "a", not "..", on the FIRST segment — the traversal is in a later, untouched segment here, so this specific rest has no ".." segment at all + + // Round 3: backslash normalization ("..%5c..%5cx" decodes to + // "..\..\x" — a Windows/.NET-style separator). + {"..%5c..%5cx", true}, + {"..%5C..%5Cx", true}, + {"v1%5cfiles", false}, // a single encoded backslash segment with no ".." component + + // Round 3: double-encoding ("%252e%252e/x" decodes ONCE to + // "%2e%2e/x" — still containing "%", refused rather than decoded + // 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 + + // Existing correct behavior must be unchanged: a legitimate + // single-encoded "/" within one segment, and literal "..". + {"a%2Fb%2Fc", false}, } for _, tt := range tests { t.Run(tt.rest, func(t *testing.T) { @@ -835,6 +859,13 @@ func TestHandlePassthrough_TraversalPath_Returns400(t *testing.T) { for _, path := range []string{ "/openai/../secret", "/openai/..%2f..%2fsecret", + // Round 3 (security review, 2026-08-22): path-parameter form, + // backslash normalization, and double-encoding, driven through + // the full ServeHTTP dispatch rather than hasTraversalSegment + // directly. + "/openai/..;/secret", + "/openai/..%5c..%5csecret", + "/openai/%252e%252e/secret", } { t.Run(path, func(t *testing.T) { req := httptest.NewRequest(http.MethodGet, path, nil)