RemoveKey shifts later keys into the removed index, so a forward loop skipped
the key after each removal. Two adjacent alg=none keys left the second one in
the set handed to signature verification.
Client assertion signing is algorithm-agnostic, but the fixtures only
produced RS256 keys, so nothing proved it. Covers RSA, ECDSA and Ed25519,
including both RFC 9864 identifiers.
Also drops a kty assignment that jwx ignores and that would have mislabelled
non-RSA keys had it taken effect.
Fail at startup rather than on the first token request when the identity
provider does not accept the client assertion algorithm. The check is
skipped when the provider omits the field, which is optional in Discovery.
Both the assertion signer and the provider validation derived the algorithm
from the key and had to handle a missing "alg" that NewClientConfig already
rejects. Validate it once at construction and keep the result.
The flag never described id_token verification. It assigns an algorithm to
provider JWKS keys that omit "alg", which is a compatibility shim for
providers such as Azure and ID-porten.
BREAKING CHANGE: openid.id-token-signing-alg is now openid.jwks-fallback-alg.
A config file with the old key fails to start. The old environment variable
is ignored and the value falls back to RS256.
Both were reported by deadcode: retry.WithBase was never used, and
PrometheusMiddleware.Initialize is a leftover from the chi-prometheus
code this middleware was adapted from.
When an equal collector was already registered, the caller kept using
its own instance, whose observations are never scraped. RegisterCollector
now returns the registered collector so callers can adopt it.
Each finding was reviewed individually; the annotations record why the
flagged code is safe rather than suppressing the rules globally.
- G101: viper configuration keys, not credentials
- G117: the marshalled value is the plaintext input to the encryption
that immediately follows, or a token endpoint response that is
required to carry tokens
- G118: the background deletion must outlive the request, so the
request context deliberately is not used
- G124: cookie attributes are validated in config.Cookie.Validate
- G710: the redirect targets are relative by construction, validated by
the url validators, or read from the provider's metadata document
The retry callback assigned to the enclosing if-scoped err instead of
declaring its own. Nothing read that variable after the handler
returned, so behaviour is unchanged.
The response was built by concatenating the request path into a JSON
string. The path is percent-decoded, so it can contain quotes and
backslashes, which produced malformed JSON and let a caller inject
arbitrary keys into the object.
Requests to the token and pushed authorization endpoints carry the
client secret or a signed client assertion. Following a redirect would
forward those credentials to a host the provider never advertised.
The metadata document was fetched with http.Get, which has no deadline,
so a hung connection blocked startup indefinitely. The response status
was never checked either, leaving an error page to surface as a JSON
decoding error.
The fetch now runs with a 10s deadline derived from the context passed
in from main, goes through the shared transport so it is traced and
pooled, and rejects any non-200 response.
Registration errors were discarded, leaving the collector silently
absent from the metrics endpoint while the vectors still recorded.
Duplicate registrations remain ignored; they are expected when several
instances are constructed in the same process.
Satisfies errcheck. Errors that carry no actionable information are
ignored explicitly: writes to an already-committed response, and closing
a fully read response body or file.
Test fixture setup asserts with require.NoError instead, since a failure
there means the fixture itself is broken.
Introduce SecondsNotApplicable for *_in_seconds fields whose feature is
disabled (no inactivity timeout, auto-refresh off), replacing scattered
magic -1 values and the post-construction mutation in Verbose(). JSON
output is unchanged.
Consolidate the refresh schedule into a single NextRefresh function (leeway +
inactivity half-life, floored at the cooldown). ShouldRefresh now defers to it,
so the explicit cooldown gate is redundant and removed. Add doc comments naming
the mechanisms. No behavior change.
Refreshes are activity-based and synchronous-on-expiry, so the proactive
leeway only hides refresh latency and provides retry headroom; it is not
what prevents serving expired tokens. Shrink it from 5m to 30s to align
with identity provider guidance against refreshing long before expiry,
while keeping decent end-user UX.
The previous if/else only checked for WarnLevel, causing any other
level (e.g. InfoLevel in 24fb9b1) to fall through to Errorf. Use Logf to
dispatch on the actual level passed by callers.
Trong Huu Nguyen
Kyrre HavikandGitHub