diff --git a/http_client_factory.go b/http_client_factory.go index ea67792..55e6b5d 100644 --- a/http_client_factory.go +++ b/http_client_factory.go @@ -138,10 +138,13 @@ func (f *HTTPClientFactory) ValidateHTTPClientConfig(config *HTTPClientConfig) e return nil } -// CreateHTTPClient creates an HTTP client with the given configuration -// Validates configuration parameters before creating the client -func (f *HTTPClientFactory) CreateHTTPClient(config HTTPClientConfig) *http.Client { - // Set defaults for zero values before validation +// applyHTTPClientDefaults fills zero-valued fields in cfg with the standard +// conservative defaults so that a partially-populated config never yields an +// unbounded HTTP client (zero overall Timeout or ResponseHeaderTimeout would make +// a stalled upstream hang forever). All construction paths MUST converge on this +// helper so behavior is identical: CreateHTTPClient and the shared transport +// pool (CreatePooledHTTPClient / GetOrCreateTransport). +func applyHTTPClientDefaults(config *HTTPClientConfig) { if config.Timeout == 0 { config.Timeout = 30 * time.Second } @@ -178,6 +181,13 @@ func (f *HTTPClientFactory) CreateHTTPClient(config HTTPClientConfig) *http.Clie if config.ReadBufferSize == 0 { config.ReadBufferSize = 4096 } +} + +// CreateHTTPClient creates an HTTP client with the given configuration +// Validates configuration parameters before creating the client +func (f *HTTPClientFactory) CreateHTTPClient(config HTTPClientConfig) *http.Client { + // Set defaults for zero values before validation + applyHTTPClientDefaults(&config) // Validate configuration - only fail on critical errors if err := f.ValidateHTTPClientConfig(&config); err != nil { diff --git a/http_client_pool.go b/http_client_pool.go index 80d1eb0..972c4da 100644 --- a/http_client_pool.go +++ b/http_client_pool.go @@ -58,6 +58,12 @@ func GetGlobalTransportPool() *SharedTransportPool { // GetOrCreateTransport gets or creates a shared transport with the given config func (p *SharedTransportPool) GetOrCreateTransport(config HTTPClientConfig) *http.Transport { + // Apply the same zero-value defaults as CreateHTTPClient so a partially + // populated config never yields an unbounded client/transport, and so the + // pool cache key reflects normalized values (configKey). Bounded here, + // before the client-count gate, so both the gate and the key see + // normalized timeouts. + applyHTTPClientDefaults(&config) // SECURITY FIX: Check client limit before creating new transport. if atomic.LoadInt32(&p.clientCount) >= p.maxClients { // At the client limit: only reuse a transport that was built for the @@ -282,6 +288,9 @@ func (p *SharedTransportPool) Cleanup() { // CreatePooledHTTPClient creates an HTTP client using the shared transport pool func CreatePooledHTTPClient(config HTTPClientConfig) *http.Client { + // Apply the same zero-value defaults as CreateHTTPClient so a partially + // populated config still gets a bounded overall request Timeout. + applyHTTPClientDefaults(&config) pool := GetGlobalTransportPool() transport := pool.GetOrCreateTransport(config) diff --git a/review_r89_test.go b/review_r89_test.go new file mode 100644 index 0000000..28cfaa8 --- /dev/null +++ b/review_r89_test.go @@ -0,0 +1,64 @@ +package traefikoidc + +import ( + "net/http" + "testing" +) + +// R89 regression: pathExcluded matched the RAW request path as a string +// prefix. Go's server leaves ".." in req.URL.Path (url.ParseRequestURI does +// not normalize dot segments), so an exclusion for "/public" also matched +// "/public/../admin", letting a dot-segment traversal reach a sibling +// resource (e.g. /admin) unauthenticated. The matcher must normalize the +// request path (path.Clean) before the boundary comparison so traversal +// cannot widen an exclusion into an authentication bypass. +func TestR89ExcludedPathDotTraversal(t *testing.T) { + // Exact-matching and legitimate-subtree exclusions must be preserved. + if !pathExcluded("/public", "/public") { + t.Error("exact match should be excluded") + } + if !pathExcluded("/public/page", "/public") { + t.Error("sub-path should still be excluded") + } + if !pathExcluded("/public/./page", "/public") { + t.Error("un-escaped ./ inside the excluded subtree should still be excluded") + } + + // Dot-segment traversal must NOT escape INTO the exclusion namespace and + // thereby reach a sibling resource unauthenticated. + if pathExcluded("/public/../admin", "/public") { + t.Error("excluded /public must not also cover /admin reached via ../ (auth bypass)") + } + if pathExcluded("/public/..", "/public") { + t.Error("excluded /public must not cover root reached via ..") + } + if pathExcluded("/public/../", "/public") { + t.Error("excluded /public must not cover the bare root reached via ../") + } + if pathExcluded("/public/x/../../admin", "/public") { + t.Error("nested traversal must not escape the subtree into /admin") + } +} + +// R89 regression: CreatePooledHTTPClient / GetOrCreateTransport must apply the +// same zero-value defaults as CreateHTTPClient. Before, a partially populated +// config (e.g. only {Timeout: 10s}, or zero) produced an http.Client with +// Timeout == 0 and a transport with ResponseHeaderTimeout == 0 — an unbounded +// stall on a hanging upstream. Both must now be bounded by the shared +// defaults. +func TestR89PooledClientAppliesDefaults(t *testing.T) { + client := CreatePooledHTTPClient(HTTPClientConfig{}) + if client.Timeout == 0 { + t.Error("pooled HTTP client must carry a default overall Timeout; zero = unbounded stall") + } + tr, ok := client.Transport.(*http.Transport) + if !ok { + t.Fatalf("pooled client transport is %T, want *http.Transport", client.Transport) + } + if tr.ResponseHeaderTimeout == 0 { + t.Error("pooled transport must carry a default ResponseHeaderTimeout; zero = unbounded stall") + } + if tr.IdleConnTimeout == 0 { + t.Error("pooled transport must carry a default IdleConnTimeout") + } +} diff --git a/url_helpers.go b/url_helpers.go index 341ec28..dd6ee3c 100644 --- a/url_helpers.go +++ b/url_helpers.go @@ -7,6 +7,7 @@ import ( "fmt" "net" "net/url" + "path" "strings" ) @@ -33,6 +34,16 @@ func (t *TraefikOidc) determineExcludedURL(currentRequest string) bool { // an unrelated sibling such as "/publicsecret", so a configured exclusion can no // longer be widened into an authentication bypass on a different resource. func pathExcluded(requestPath, excluded string) bool { + // Normalize dot segments and redundant slashes so an exclusion cannot be + // escaped by traversal. Go's server leaves ".." in req.URL.Path (verified: + // url.ParseRequestURI keeps dot segments), so "/public/../admin" would match + // an "/public" exclusion even though the real resource is "/admin". path.Clean + // resolves this to "/admin" before the boundary check. "." (empty or bare) + // maps back to "" preserving the original root-match semantics. + requestPath = path.Clean(requestPath) + if requestPath == "." { + requestPath = "" + } excluded = strings.TrimRight(excluded, "/") if excluded == "" { // A "/" (root) exclusion only matches the root path, not everything.