On the lenient-audience path (validateStandardTokensRS), jwt.Verify
short-circuits at the aud check before reaching exp/iat/nbf validation,
leaving every time claim unvalidated. accessTokenUnexpired previously
re-patched only 'exp' (the R66 fix), so a token with a not-yet-reached
'nbf' but a valid exp was trusted for authorization before it was valid.
accessTokenUnexpired now re-applies the full main-path time validation:
verifyExpiration, verifyIssuedAt (iat required), and verifyNotBefore when
present — all clock-skew-aware. Regression TestR92LenientPathRejectsFutureNbf
(plus valid control) fails on old code (future-nbf token accepted), passes on
new.
R92 clean/declined: bearer auth (aud enforced, full claims — clean);
refresh rotation (correctly bound to session; no local reuse detection =
design); logout cookie completeness (all chunk names cleared, no revive;
two P3 non-security notes declined).
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.
lukaszraczylo