mirror of
https://github.com/lukaszraczylo/traefik-llmgateway.git
synced 2026-10-14 05:18:54 +00:00
fix(auth): decode rest before matching the passthrough path allowlist
Close the %2f allowlist bypass (security review finding 3, round 3,
2026-08-22): allowsPassthroughPath and hasTraversalSegment (routes_
passthrough.go) reasoned about rest asymmetrically -- the traversal check
decodes it first, the allowlist matcher ran path.Match against the raw
escaped form. path.Match("v1/*", "v1/chat%2fcompletions") reports true
(one escaped segment, no literal "/" to split it), while the decoded
equivalent, "v1/chat/completions", is two segments and does not match
"v1/*". A group restricted to PassthroughPaths: ["v1/*"] therefore let
"/openai/v1/fine_tuning%2Fjobs" through -- a path an upstream that
itself decodes %2F would read as a different, unauthorized endpoint.
allowsPassthroughPath now url.PathUnescape's rest before matching -- the
same decode hasTraversalSegment already applies -- and denies a rest
that fails to unescape at all, mirroring hasTraversalSegment's own
fail-closed rule for malformed encoding. The empty-pattern fast path
(matchesGlob's own empty-means-all contract) is checked first, before
any decoding: a group with no PassthroughPaths configured -- every live
group today -- keeps allowing every request exactly as before, malformed
encoding included; hasTraversalSegment (unconditional, later in
handlePassthrough) remains the gate for that case.
Tests: a direct unit test proving the decoded three-segment form is
denied (both via the encoded and literal spellings), a single-segment
encoded value still matches, malformed encoding denies for a restricted
group and is unaffected for an unrestricted one, and an end-to-end
ServeHTTP test reproducing the exact finding scenario.
This commit is contained in:
@@ -5,6 +5,7 @@ import (
|
||||
"crypto/subtle"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"net/url"
|
||||
"path"
|
||||
"sort"
|
||||
"strings"
|
||||
@@ -110,8 +111,39 @@ func (grp *group) allowsAgent(name string) bool {
|
||||
// PassthroughPaths' own doc comment (llmgateway.go) for a path.Match
|
||||
// footgun a non-empty list inherits: "*" does not cross "/", so ["*"]
|
||||
// is NOT "allow everything".
|
||||
//
|
||||
// %2f BYPASS (security review finding 3, round 3, 2026-08-22): rest
|
||||
// arrives here still in its ESCAPED form (passthroughRoute's own
|
||||
// contract, routes_passthrough.go — callers must pass r.URL.
|
||||
// EscapedPath()), the same form path.Match evaluates literally, with no
|
||||
// notion of percent-decoding. hasTraversalSegment (routes_passthrough.go)
|
||||
// decodes rest before reasoning about it; this function used to match
|
||||
// the RAW escaped form instead — an asymmetry a client can exploit:
|
||||
// path.Match("v1/*", "v1/chat%2fcompletions") reports true (one escaped
|
||||
// segment, no literal "/" for the glob to see splitting it), while the
|
||||
// DECODED equivalent, "v1/chat/completions", is two segments and does
|
||||
// NOT match "v1/*". A group restricted to ["v1/*"] therefore let
|
||||
// "/openai/v1/fine_tuning%2Fjobs" through — a path an upstream that
|
||||
// itself decodes %2F would read as "v1/fine_tuning/jobs", a different,
|
||||
// unauthorized endpoint. Fixed by decoding rest with the SAME
|
||||
// url.PathUnescape call hasTraversalSegment already uses before matching,
|
||||
// and rejecting (denying) a rest that fails to unescape at all — the
|
||||
// same fail-closed rule hasTraversalSegment already applies. The
|
||||
// zero-pattern (empty-means-all) fast path runs BEFORE decoding: a group
|
||||
// that has not opted into PassthroughPaths at all keeps allowing every
|
||||
// request, malformed encoding included, exactly as before this fix —
|
||||
// hasTraversalSegment (called unconditionally, later, in
|
||||
// handlePassthrough) is still the gate that denies a malformed rest for
|
||||
// that case, unchanged.
|
||||
func (grp *group) allowsPassthroughPath(rest string) bool {
|
||||
return matchesGlob(grp.passthroughPaths, rest)
|
||||
if len(grp.passthroughPaths) == 0 {
|
||||
return true
|
||||
}
|
||||
decoded, err := url.PathUnescape(rest)
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
return matchesGlob(grp.passthroughPaths, decoded)
|
||||
}
|
||||
|
||||
// cloneStringSlice returns an independent copy of s, preserving nil (a nil
|
||||
|
||||
@@ -325,6 +325,61 @@ func TestGroup_AllowsPassthroughPath_Glob(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestGroup_AllowsPassthroughPath_RejectsEncodedSlashBypass is the
|
||||
// security review finding 3 regression test (round 3, 2026-08-22): a
|
||||
// glob of ["v1/*"] (one wildcard segment) must not be defeated by
|
||||
// encoding the "/" that would otherwise split the candidate into a
|
||||
// second segment. Before this fix, matchesGlob ran against the escaped
|
||||
// form directly, so path.Match("v1/*", "v1/fine_tuning%2Fjobs") reported
|
||||
// true — the escaped rest looks like exactly two segments to path.Match,
|
||||
// same as the decoded intent, but MEASURED to actually still match
|
||||
// because "%2Fjobs" contains no literal "/" for the glob to split on
|
||||
// differently — while an upstream that itself decodes %2F would read the
|
||||
// same rest as "v1/fine_tuning/jobs", a THREE-segment path "v1/*" was
|
||||
// never meant to authorize.
|
||||
func TestGroup_AllowsPassthroughPath_RejectsEncodedSlashBypass(t *testing.T) {
|
||||
grp := &group{name: "eng", passthroughPaths: []string{"v1/*"}}
|
||||
if grp.allowsPassthroughPath("v1/fine_tuning%2Fjobs") {
|
||||
t.Error("want no match: the decoded form is three segments (v1/fine_tuning/jobs), which \"v1/*\" does not authorize")
|
||||
}
|
||||
// Sanity: the decoded-equivalent form is independently denied too,
|
||||
// proving the glob genuinely does not authorize the deeper path — not
|
||||
// just that encoding happens to break matching by accident.
|
||||
if grp.allowsPassthroughPath("v1/fine_tuning/jobs") {
|
||||
t.Error("want no match for the literal three-segment form either")
|
||||
}
|
||||
// A genuinely single-segment encoded value must still match: "v1/*"
|
||||
// authorizes any ONE segment after "v1", encoded or not.
|
||||
if !grp.allowsPassthroughPath("v1/chat%2Dcompletions") {
|
||||
t.Error("want match: a single encoded segment (no literal slash once decoded) is still one segment")
|
||||
}
|
||||
}
|
||||
|
||||
// TestGroup_AllowsPassthroughPath_MalformedEncoding_DeniesWhenRestricted
|
||||
// mirrors hasTraversalSegment's own "fails to unescape at all is
|
||||
// rejected too" rule (routes_passthrough.go) — applied here to the
|
||||
// allowlist match itself, for a group that actually restricts paths.
|
||||
func TestGroup_AllowsPassthroughPath_MalformedEncoding_DeniesWhenRestricted(t *testing.T) {
|
||||
grp := &group{name: "eng", passthroughPaths: []string{"v1/*"}}
|
||||
if grp.allowsPassthroughPath("v1/%zz") {
|
||||
t.Error("want no match: malformed percent-encoding must fail closed for a restricted group")
|
||||
}
|
||||
}
|
||||
|
||||
// TestGroup_AllowsPassthroughPath_MalformedEncoding_StillAllowsWhenUnrestricted
|
||||
// pins the zero-config default-preserving requirement precisely at the
|
||||
// boundary this fix touches: an EMPTY PassthroughPaths list (the fast
|
||||
// path, checked before any decoding) must keep allowing a request whose
|
||||
// rest fails to percent-decode — exactly as before this finding, since
|
||||
// hasTraversalSegment (unconditional, in handlePassthrough) is still the
|
||||
// gate for that case, unchanged by this fix.
|
||||
func TestGroup_AllowsPassthroughPath_MalformedEncoding_StillAllowsWhenUnrestricted(t *testing.T) {
|
||||
grp := &group{name: "eng"} // no PassthroughPaths configured
|
||||
if !grp.allowsPassthroughPath("v1/%zz") {
|
||||
t.Error("want match: an unrestricted group's fast path must never decode rest at all")
|
||||
}
|
||||
}
|
||||
|
||||
func TestAuthStore_ReplaceFileUsers_AddsAndSwapsUsers(t *testing.T) {
|
||||
a, err := newAuthStore(testAuthCfg()) // inline user "a" / sk-secret in group eng
|
||||
if err != nil {
|
||||
|
||||
@@ -1610,6 +1610,47 @@ func TestHandlePassthrough_PassthroughPaths_Restrictive(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestHandlePassthrough_PassthroughPaths_EncodedSlashBypass_Returns403 is
|
||||
// the end-to-end regression test for security review finding 3, round 3,
|
||||
// 2026-08-22: a group restricted to PassthroughPaths: ["v1/*"] must deny
|
||||
// "/openai/v1/fine_tuning%2Fjobs" (an encoded "/" hiding a second
|
||||
// segment from the escaped-form matcher) exactly as it would deny the
|
||||
// literal, decoded three-segment path. Before this fix, path.Match
|
||||
// evaluated the escaped rest directly and reported a match, letting the
|
||||
// request reach the upstream — which, if it decodes %2F itself, would
|
||||
// have read the same bytes as "v1/fine_tuning/jobs", a path this group's
|
||||
// glob was never meant to authorize.
|
||||
func TestHandlePassthrough_PassthroughPaths_EncodedSlashBypass_Returns403(t *testing.T) {
|
||||
var upstreamCalled bool
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
upstreamCalled = true
|
||||
w.WriteHeader(http.StatusOK)
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
cfg := CreateConfig()
|
||||
cfg.Providers = map[string]*ProviderConfig{"openai": {Type: "openai", BaseURL: srv.URL, APIKey: "sk-up"}}
|
||||
cfg.Groups = map[string]*GroupConfig{"default": {PassthroughPaths: []string{"v1/*"}}}
|
||||
cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice"}}}
|
||||
next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})
|
||||
h, err := New(context.Background(), next, cfg, "llmgw")
|
||||
if err != nil {
|
||||
t.Fatalf("New: %v", err)
|
||||
}
|
||||
|
||||
req := httptest.NewRequest(http.MethodGet, "/openai/v1/fine_tuning%2Fjobs", nil)
|
||||
req.Header.Set("Authorization", "Bearer sk-alice")
|
||||
rec := httptest.NewRecorder()
|
||||
h.ServeHTTP(rec, req)
|
||||
|
||||
if rec.Code != http.StatusForbidden {
|
||||
t.Fatalf("status = %d, want 403 (the encoded slash must not defeat the \"v1/*\" allowlist), body=%s", rec.Code, rec.Body.String())
|
||||
}
|
||||
if upstreamCalled {
|
||||
t.Error("upstream must never be called: the decoded rest is a three-segment path \"v1/*\" does not authorize")
|
||||
}
|
||||
}
|
||||
|
||||
// TestHandlePassthrough_DangerousHeadersStripped proves every
|
||||
// client-supplied identity/forwarding header, plus the provider-billing-
|
||||
// retargeting headers, are stripped before the request reaches the
|
||||
|
||||
Reference in New Issue
Block a user