diff --git a/auth.go b/auth.go index abd5f3d..96359b7 100644 --- a/auth.go +++ b/auth.go @@ -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 diff --git a/auth_test.go b/auth_test.go index 723fade..40cca70 100644 --- a/auth_test.go +++ b/auth_test.go @@ -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 { diff --git a/routes_passthrough_test.go b/routes_passthrough_test.go index 1081dc0..4e70781 100644 --- a/routes_passthrough_test.go +++ b/routes_passthrough_test.go @@ -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