Two verified defects from R89 scout round:
1. Excluded-URL dot-segment traversal widening (P1, auth bypass).
shouldBypassAuth passes req.URL.Path raw (middleware.go:54); Go's server
leaves '..' in URL.Path (url.ParseRequestURI does not normalize), so an
exclusion for /public also matched /public/../admin, letting a real
sibling (/admin) reach the backend unauthenticated. pathExcluded now
path.Clean()s the request path ('.' -> '' to preserve root semantics)
before boundary matching. Regression TestR89ExcludedPathDotTraversal
fails on old (all 4 traversal cases wrongly excluded), passes on new.
2. Pooled HTTP client had no zero-value defaults (P2, unbounded stall).
CreateHTTPClient applies defaults; CreatePooledHTTPClient and
GetOrCreateTransport did not, so a partial config yielded Timeout=0 and
ResponseHeaderTimeout=0 (a hanging upstream stalls forever). Extracted
applyHTTPClientDefaults (single source of truth) and applied it in all
three construction paths; pool cache key now reflects normalized values.
Regression TestR89PooledClientAppliesDefaults fails on old, passes on new.
R89 clean/declined: session max-age IS absolute-and-enforced-every-request
(verified, review_r68 already locks it); AllowedUserDomains fail-closed (P2
'*' not a wildcard, P3 TrimSpace — declined as config-scoped).
TaskRegistry.CreateSingletonTask delegates to ResourceManager.RegisterBackgroundTask
and stores the task in the ResourceManager's registry, not TaskRegistry.tasks.
TaskRegistry.StopAllTasks previously iterated its own (always-empty) registry,
silently leaking every process-global singleton task's ticker goroutine past the
last Close (memory-monitor, replay-cache-cleanup) — tasks kept running real
periodic work with no instance to serve.
Add ResourceManager.StopAllTasks (stops all rm-owned tasks) and have
TaskRegistry.StopAllTasks stop both its own RegisterTask-direct tasks and the
ResourceManager-owned singletons.
Regression TestR88StopAllTasksStopsResourceManagerOwnedTasks (review_r88_test.go):
fails on old code (task still running after StopAllTasks), passes on new.
http_client_pool.go SharedTransportPool.GetOrCreateTransport built the shared
http.Transport with hardcoded MaxConnsPerHost=5, MaxIdleConnsPerHost=2,
MaxIdleConns=10, IdleConnTimeout=30s while HTTPClientConfig's versions of
those fields drove configKey (cache identity) and were validated and tuned
per preset (Default: 20/10/50/30s; OIDCProvider: 50/25/100/90s). The
config was thus valid, keyed, and validated but silently ignored in the
transport used by main.go (CreatePooledHTTPClient) - dead config that left
OIDC-provider connection tuning without effect. The sibling factory path
(http_client_factory.go) already applied these values.
Now the pooled transport derives MaxConnsPerHost, MaxIdleConnsPerHost,
MaxIdleConns, and IdleConnTimeout from the config, consistent with the
factory path and with configKey.
Test: added TestSharedTransportPoolAppliesConfigLimits, which fails on the
old hardcoded code (fixed values) and passes now.
Verified: full source-order suite, full -race -count=1 -shuffle=on ./...
(all packages), golangci-lint run ./... (0 issues).
error_recovery.go: ResetTimeout was copied into the CircuitBreaker but never
consulted by the state machine (used only for a metric), so the documented
"how long to wait before fully closing the circuit" (reset_timeout, default
30s) had no effect: Open->HalfOpen used Timeout (60s), and a single
half-open success closed the circuit immediately, losing the intended
open -> half-open -> (reset cooldown) -> closed hysteresis and letting a
stray success immediately reopen (bounce).
Now the circuit remains half-open after entering it until resetTimeout has
elapsed; only then can a successful request fully close it. Half-open
requests are still allowed as probes. Default behavior is more
conservative (up to Timeout+ResetTimeout before full close).
Tests: updated TestCircuitBreakerHalfOpenTransition to the corrected
semantics; added TestCircuitBreakerResetTimeoutEnforced (fails on the old
dead-field code, where resetTimeout was ignored).
Verified: full source-order suite, full -race -count=1 -shuffle=on ./...
(all packages), golangci-lint run ./... (0 issues).
Four verified fixes from the round-36 agent team (each confirmed against source
before acting):
- settings.go: Config.Validate now skips the client-credential block when
Dynamic Client Registration is enabled and no clientID is pre-provisioned
(RFC 7591 provides credentials at registration). Previously the
unconditional 'clientID is required' made DCR unreachable via New.
- dynamic_client_registration.go: default registered token_endpoint_auth_method
to client_secret_post to match the plugin's runtime default; an enforcing
IdP would otherwise reject the plugin's token exchange.
- utilities.go: gate the shared per-provider singleton-metadata-refresh task
stop on lastInstance (matching singleton-token-cleanup) so one instance's
tear down no longer kills 2h metadata refresh for a surviving sibling.
- token_manager.go: stop self-recording the JTI into the per-instance
tokenBlacklist on every verify. On the cookie path the same valid token is
re-presented per request; an LRU eviction of the raw-token cache (MaxSize
1000 / 5 MiB) then turned a legitimate cache miss into a false 'token
replay detected'. Replay tracking stays in the shared shardedReplayCache.
Regression tests: new review_r36_regression_test.go (4 tests); updated
dynamic_client_registration_test.go and security_edge_cases_test.go which had
encoded the old defaults.
Verified: full source-order suite, full -race -count=1 -shuffle=on ./...
(all packages), and golangci-lint run ./... (0 issues).
Concurrency/lifecycle (P1, highest-value):
- singleton_resources.go: RegisterBackgroundTask now REPLACES a stopped
task instead of reusing it. BackgroundTask.Start is sync.Once-guarded,
so a task stopped on instance close and re-registered (Traefik
close+recreate for the same provider) was permanently dead: IsTaskRunning
returned false (started && !stopped) so callers retried a start that
was a no-op. Metadata refresh, token cleanup, and health checks now
resume across a recreate cycle.
- error_recovery.go: the shared graceful-degradation health task was
bound to the FIRST instance's performHealthChecks (later instances'
checks never ran) and any instance's Close stopped it for all. Added a
global live-instance registry; the shared task runs every instance's
checks and is stopped only when the last instance closes (mirrors the
singleton-token-cleanup gate).
Storage/cache:
- hybrid.go recordL2Error: Store(now) then immediately compared
time.Since(t)<1s against its own write => ALWAYS fired, so a single
transient L2 error flipped the whole cache into fallback (full L2
bypass -> upstream stampede). Now enters fallback only on a PRIOR
error within the window (true "multiple errors"). Updated two tests
that had encoded the old single-error-fallback bug.
- redis_pool.go: ConnectionPool.Put checked closed but sent on
p.connections without holding p.mu; Close closed it under p.mu.
Window => 'send on closed channel' panic at shutdown. Put now holds
p.mu (the lock only Close used).
HTTP/transport:
- jwk.go fetchJWKS: added an explicit >1 MiB guard (was silent partial
read, could yield a truncated keyset and spurious missing-kid).
- http_client_pool.go configKey: now includes every transport-behavior
field it actually consumes (compression, HTTP2, keep-alives, timeouts,
buffer sizes, dial) so behaviorally different configs no longer share
one transport built with the first config's settings.
Each fix has a regression test. Verified: full source-order suite green,
golangci-lint 0 issues, 3x full -race -count=1 -shuffle=on ./... all exit 0.
GetResourceManager returned the process-global via an unprotected read
(singleton_resources.go:50), while resetResourceManagerForTesting and
ResetResourceManager write it under resourceManagerMutex. A live background
task (graceful-degradation health check started by one test) reading the
singleton while a shuffled test resets it was a data race (found via
-race -shuffle: Write at singleton_resources_test.go:941/942 vs Read at
autocleanup.go:554 / error_recovery.go:910).
Fix: hold resourceManagerMutex around the read, matching the sibling
GetGlobalTaskRegistry (autocleanup.go:414). Added a deterministic
concurrent regression test (GetResourceManager vs reset under -race).
Verified: 4x full -race -count=1 -shuffle=on ./... all exit 0, 0 races,
0 fails; source-order suite green; lint clean.
- TransportPool configKey: write conn-count ints verbatim (strconv) instead of
byte() truncation — a config value >=256 previously truncated and made two
distinct configs collide on one key, silently sharing a transport with the
wrong settings (real fix, gosec G115).
- Document design-intentional gosec findings with //nolint:gosec + rationale:
G117 (clients_secret intentionally marshaled into stored creds JSON),
G704 (tokenURL is the trusted discovery endpoint), G124 (delete/test cookie,
Secure set conditionally), G118 (long-lived background refresh) and pool
utils short-ID rune, testutil mock server redirects.
Result: full-tree golangci-lint now reports 0 issues (was ~12). Build, vet,
and the full test suite remain green.
Issue #152 asks for a manual introspection endpoint because some IdPs
omit introspection_endpoint from discovery. Analysis found the two
documented overrides revocationURL and oidcEndSessionURL were dead
code: the 1b49e13 rebuild dropped their constructor wiring.
Add the IntrospectionURL config field and wire all three overrides.
The overrides apply inside updateMetadataEndpoints, so they survive
every metadata refresh. A manual value always wins over discovery.
The constructor also seeds the live fields, so a failed discovery
does not lose operator configuration.
Manual values skip the poisoned-discovery defences on purpose. They
sit at the same trust tier as providerURL and isValidSecureURL gates
them at config time.
Add issue152_regression_test.go: 8 tests pin precedence, refresh
survival, cold-start seeding, end-to-end introspection over HTTP,
and the validation contract.
Issue #154: a providerURL that points at a localhost IdP caused an
infinite redirect loop. validateHost rejected loopback hosts
unconditionally, so buildAuthURL returned an empty string.
Derive allowLoopbackHosts from a loopback providerURL at construction
time. Discovery validation already used this derivation, so the two
validation layers now agree. This adds no new config surface.
Cloud metadata hosts (169.254.169.254, metadata.google.internal),
link-local, and unspecified addresses stay blocked unconditionally.
Non-loopback providers keep identical behaviour.
Add issue154_regression_test.go: 8 tests pin the new allowance, the
default block, and the security envelope.
The install snippet in the README still pinned v0.7.10 and the example
configs v0.8.0, so anyone following the docs installed a release far
behind the current v1.0.29.
Co-authored-by: Markus Renschler <[email protected]>
The plugin catalog analyzer failed v1.0.28 with "invalid handler
type: <nil>" (#151). Two defects combined.
The analyzer feeds .traefik.yml testData to New() verbatim. The header
values used quadruple braces on a false premise: no configuration path
collapses them. Template validation rejected them, so New() failed. The
testData now carries plain, valid templates.
New() also returned failures through a multi-value tail call
(return NewWithContext(...)). Yaegi v0.16.1 zeroes both results of that
statement on the error path. The analyzer therefore saw (nil, nil) and
reported the opaque handler-type message instead of the real validation
error. New() now splits the return explicitly. Construction failures
surface as real error messages under yaegi and in Traefik logs.
The regression tests pin both fixes. README and CONFIGURATION.md now
give correct escaping guidance: plain double braces everywhere; file
provider users escape with a raw string. Quadruple braces do not work
anywhere.
Fixes#151.
lukaszraczylo
