Security and conformance review fixes (June 2026)#187
Merged
Conversation
Client authentication via private_key_jwt / client_secret_jwt accepted an assertion without an exp claim and never recorded its jti, leaving it replayable indefinitely (RFC 7523 §3 requires exp). The jti was also marked used in a separate step after the status check, leaving a TOCTOU window between two concurrent presentations of the same assertion. JwtAssertionAuthenticatorBase now rejects an assertion without exp and records the jti through the atomic IJwtReplayCache.TryAddAsync reserve-and-check (the same primitive DPoP and jwt-bearer use), replacing the two-step ITokenRegistry.SetStatusAsync. AddClientAuthentication registers the replay cache defensively.
Registration access tokens were stateless JWTs without exp or jti and were never invalidated: every update issued a new token while all prior tokens stayed valid, so a leaked token granted permanent control of a client's registration (RFC 7592 §5). A new IRegistrationAccessTokenStore (backed by the distributed IEntityStorage, so the binding is shared across replicas) records the jti of each client's current token. The validator accepts only a token whose jti matches the stored value: registration records a fresh jti, update rotates it (invalidating prior tokens), read reuses it (idempotent), delete removes it. When no binding is recorded the check is skipped, so statically configured clients keep working.
The reuse defense detected only sequential reuse: two concurrent redemptions of the same code both read a grant with no issued tokens, both passed the check and both issued tokens (RFC 6749 §4.1.2 single-use). RemoveAuthorizationCodeAsync now performs an atomic get-and-remove and returns the claimed grant as Result<AuthorizedGrant, OidcError>. The reuse-preventing decorator claims the code after the grant validators (so a failed PKCE/client check never burns the code): exactly one concurrent redemption wins, the rest get invalid_grant. Sequential reuse still revokes the previously issued tokens.
A JWE whose Content Encryption Key failed to decrypt skipped the AEAD step and continued, while a successful-but-wrong CEK ran AEAD — an observable difference that turns RSA1_5 (RSAES-PKCS1-v1_5) decryption into a Bleichenbacher/Manger oracle. JsonWebTokenEncryptor.DecryptAsync now resolves the content decryptor by enc up front (an unregistered enc is rejected) and, when CEK decryption fails, substitutes a random CEK and still runs AEAD, so a decryption failure is processed identically regardless of padding validity. RSA1_5 stays supported for compatibility, now without the oracle.
The RFC 8693 audience parameter was written into the issued token's aud claim with no validation, so a client permitted to use token exchange could mint a token for any target service it named — a cross-audience escalation if a resource server authorizes by aud. A new default-deny per-client allowlist ClientInfo.TokenExchangeAllowedAudiences gates it: a request that names an audience is accepted only when the client declares a non-empty allowlist containing every requested value, otherwise the handler returns invalid_target (RFC 8693 §2.2.1). Unlike the unconstrained-by- default subject-token-type allowlist, audience is closed by default because it flows straight into aud. The metadata is wired through the full DCR path (token_exchange_audiences) on both the core and MVC models, processors and responses, mirroring the subject-token-types field.
The token endpoint was annotated [HttpGetOrPost] with [FromQueryOrForm], so a GET request bound its parameters from the query string. RFC 6749 §3.2 requires POST: a code, client_secret or refresh_token in the URL is logged by proxies, servers and browser history. The endpoint is now [HttpPost] + [Consumes(form-urlencoded)] + [FromForm], mirroring the revocation and introspection endpoints; GET is rejected at routing. The now-dead SupportsGet=true is removed from the MVC TokenRequest model so its POST-only intent is explicit. HttpGetOrPost stays on the authorization endpoint, where GET is the RFC 6749 §3.1 mechanism.
RFC 7662 §2.1 requires the introspection endpoint to require some form of authorization to prevent token scanning; the revocation endpoint must likewise authenticate the client (RFC 7009 §2.1). Both accepted the "none" method, so a public client presenting only its client_id could introspect or revoke its own tokens with no credential. Both validators now reject a client whose token_endpoint_auth_method is "none" with invalid_client, before the token is validated. The "none" method stays valid at the token and authorization endpoints, where public PKCE clients need it. A client registered with "none" authenticates only via that method, so the post-authentication check on the registered method is exact.
In the array branch of Read, `case JsonTokenType.EndArray: break;` exited only the switch, so the enclosing `while (reader.Read())` then read the token after the array. When the single-or-array value is a property inside an object — the normal case, e.g. a multi-valued "resource" in a signed request object — that next token is the object's EndObject or the next property name, which the default case rejects with a JsonException, corrupting deserialization. EndArray now returns the accumulated array, matching ArrayConverter. The existing tests missed this because they only parsed top-level arrays, where the stray read past EndArray simply hits end of stream; the new tests parse arrays nested inside an object.
CalculateExpiresAt anchored both the absolute ceiling and the sliding window to the token's original IssuedAt, so the sliding window never moved: with the default 1h sliding / 8h absolute, an actively-refreshed session still expired 1h after the first login and the absolute ceiling was unreachable. That contradicts the documented contract — "each successful refresh extends the token's expiration ... stay alive only while the client keeps using them". The sliding window is now anchored to the refresh moment (now + SlidingExpiresIn) while the absolute ceiling stays anchored to the original IssuedAt; the expiry is the earlier of the two. Idle tokens still expire after the sliding window; active sessions live up to the absolute ceiling. The behaviour fails safe either way (tokens never outlive the absolute ceiling).
… algorithm OpenID Connect Core 1.0 sections 3.1.3.6 and 3.3.2.11 require the at_hash and c_hash values to be computed with the hash algorithm matching the ID token's signature algorithm (SHA-256/384/512 for *256/*384/*512 families). Only the default RSA algorithm was handled, so an ID token signed with any other registered algorithm silently omitted these claims, breaking hybrid and implicit flows where they are REQUIRED. Unsigned tokens and unrecognised algorithms still produce no hash claims.
…ection OpenID Connect Dynamic Client Registration 1.0 section 5 (with Core section 8.1) requires the registered redirect URIs to be a subset of the values listed in the sector identifier document. The check ran in the opposite direction: it rejected legitimate documents shared across several clients of the same sector, and at the same time let a client register a redirect URI absent from the document - claiming a sector it does not belong to and breaking pairwise subject isolation.
RFC 7662 section 2.2 defines the activity member of the introspection response as a JSON boolean. It was serialized as the strings "true"/"false", which breaks strict deserializers and, worse, inverts the semantics for lenient ones: a non-empty string is truthy, so a revoked token could be treated as active by the resource server.
…horization responses RFC 8628 section 3.2 defines an optional verification URI variant that embeds the user code, letting capable devices render a direct link or QR code so the user skips typing the code. The field existed on the wire model but was never populated; it is now derived from the configured verification URI.
…thods Discovery advertised a SHA-512 PKCE transformation that is not registered in the IANA "PKCE Code Challenge Methods" registry (verified 2026-06-11) and is defined by no specification - announcing it in server metadata is a false conformance claim. The transformation itself remains accepted at runtime as an undocumented extension, so existing clients keep working.
OpenID Connect Core 1.0 section 3.3.2.11 makes the nonce REQUIRED for all Hybrid Flow requests, including the combination that returns a code and an access token but no ID token directly: the ID token later minted from that code must still carry the nonce binding. The validator only enforced the section 3.2.2.1 case where an ID token is issued from the authorization endpoint itself.
…I host OpenID Connect Core 1.0 section 8.1: when no sector identifier URI was provided, the sector for pairwise subject calculation is the host component of the registered redirect URI. The previous client-identifier fallback produced subjects that silently change when the same application is re-registered under a new identifier, defeating the stability pairwise identifiers are supposed to give a sector. Affects only statically configured clients; the client identifier remains the last resort for clients with no redirect URIs at all.
…est parameters OpenID Connect Core 1.0 section 6.1: the response type and client identifier passed in the OAuth request syntax MUST match the values inside the request object when the object carries them. The merge gives the object's values precedence, so without this check an attacker-supplied object could silently swap the flow or the client identity relative to what the plain parameters declared.
…on failures Two RFC-mandated challenge behaviours were missing: Client authentication failures at the token, introspection and revocation endpoints now return HTTP 401 with a Basic challenge instead of a plain 400. RFC 6749 section 5.2 requires 401 when the client authenticated via the Authorization header and allows it otherwise, so the uniform 401 satisfies both cases; the error stays in the JSON body because RFC 7617 defines no error attributes for the Basic scheme. Per RFC 6750 section 3.1, a protected-resource request carrying no authentication information at all must receive a bare challenge - scheme and realm only, without error attributes. The userinfo endpoint previously attached an error code to that case; the same rule now applies to the DPoP challenge line.
…ion requests RFC 6749 section 5.2: a request missing a required grant parameter is the caller's protocol error and must produce an invalid_request response. The grant handlers delegated these checks to a parameter validator that throws, surfacing every missing parameter as HTTP 500; they now guard explicitly and return the proper error. For the same reason an authorization code issued to another client now yields invalid_grant. At the authorization endpoint (RFC 6749 sections 4.1.2.1/4.2.2.1): a missing response type is invalid_request, and a response type the server supports but this client is not registered for is unauthorized_client - the unsupported response type error is reserved for methods the server itself cannot produce. Per OAuth 2.0 Multiple Response Types section 5, errors for token-bearing response types now default to fragment encoding: the previous query default delivered the error to a channel the client never reads and exposed it to the server hosting the redirect URI.
…on responses A set of client-metadata and request-object conformance changes that share configuration, client-information and registration wire models: - Registration and management responses now echo the effective grant types, response types and scope (RFC 7591 section 3.2.1, RFC 7592 section 3), so clients see the defaults and adjustments the server applied instead of assuming their request was taken verbatim. - New per-client enforcement flags, registrable via dynamic registration and echoed back: pushed authorization requests only (RFC 9126 section 6), signed request objects only (RFC 9101 section 10.5), and certificate-bound access tokens independent of the authentication method (RFC 8705 section 3.4). FAPI-grade clients set these so a server-wide toggle cannot silently weaken them. - Request objects: an audience addressed to another server is rejected (a request object minted for a different provider must not be replayable here), an unsigned object no longer satisfies a client's signed-object commitment, and an opt-in strict mode implements RFC 9101 section 5 semantics where parameters outside the object are ignored rather than merged. - Symmetric algorithms are excluded from the advertised and accepted response-signing algorithms (OpenID Connect Core 1.0 section 10.1): their key is the client secret, which this server persists only as a hash, so it cannot derive the signing key and previously failed with a server error on the first issued token. An unsigned ID token algorithm is now also rejected at registration when the registered response types deliver ID tokens from the authorization endpoint. - Back-channel logout tokens are signed in the same manner as ID tokens (Back-Channel Logout 1.0 section 2.4), following the client's registered ID token algorithm, with a host-side per-client override. - Authorization requests asking to initiate user registration (Initiating User Registration 1.0) now redirect the user to a configurable registration UI instead of being treated as a plain login; fragment-encoded authorization responses also report the access token lifetime as recommended for implicit and hybrid responses.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| var body = JsonNode.Parse(raw)?.AsObject(); | ||
| Assert.NotNull(body); | ||
| Assert.Equal("true", body["active"]?.GetValue<string>()); | ||
| Assert.True(body[IntrospectionSuccess.Parameters.Active]!.GetValue<bool>()); |
…stop The authorization-time consent path emitted the IUserConsentsProvider result verbatim into the issued grant — scopes, resources and authorization_details were never checked against what the request carried. The consent decision frequently crosses the browser trust boundary (the user ticks checkboxes; the browser POSTs the decision to the host's consent endpoint), so a host that echoes browser-supplied values without intersecting against the request would let an end-user escalate their own grant, with no library-side defense. A new ConsentConstraintEnforcer asserts the invariant before the granted set reaches the issued token: every granted scope, resource (and its nested scopes) and authorization_details type must be present in the request, and the granted authorization_details are re-run through the per-type validators so the per-type policy owns the intra-entry "is B a narrowing of A" decision (RFC 9396 defines no universal comparator). This mirrors the strictly narrowing-only token-endpoint evaluator (RFC 8707 §2.2), giving the authorize-time path the same guarantee. A broader granted set is never a protocol condition — it is a defect in the host's IUserConsentsProvider (or browser tampering its provider failed to defend against), so the enforcer fails loud with an exception rather than masking it as a recoverable error: it surfaces in the host's debugger, fails the host's tests, and is logged as a server error in production while no escalated grant is issued. Ref #185
…the first The audience allowlist check returned on the first audience absent from the client's list, so a client fixing its request had to resubmit and rediscover the rejected values one round-trip at a time. Collect all disallowed audiences and name them in a single invalid_target response. Replacing the early-return loop with a Where projection also resolves SonarCloud S3267 on this method.
The two does-not-throw enforcer tests relied on the implicit "test fails if it throws" behaviour, which SonarCloud flags as S2699 (no assertion). Make the expectation explicit with Record.ExceptionAsync + Assert.Null so the success condition is stated, not inferred. Ref #185
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes from the June 2026 security and conformance review of the library: every finding is addressed with a regression test, grouped one commit per finding (or per theme where changes share files).
Security hardening
Conformance
FAPI-grade per-client enforcement
New client metadata, registrable via dynamic registration: require pushed authorization requests (RFC 9126 §6), require signed request objects (RFC 9101 §10.5), and certificate-bound access tokens independent of the authentication method (RFC 8705 §3.4); plus an opt-in strict request-object mode (RFC 9101 §5). These are the granular building blocks for the per-client FAPI 2.0 profile tracked in #150.
Compatibility notes