feat(client): the base layer of Abblix.Oidc.Client - #291
Draft
kirill-abblix wants to merge 91 commits into
Draft
Conversation
Add the framework-agnostic client core project under src/ and its unit test project under tests/, wired into the solution and the PR test matrix. The core exposes AddOidcClientCore, which binds OidcClientOptions; feature services and the request and validation pipelines land in the following commits. Mirrors the Abblix.Jwt project conventions: multi-target, package metadata, InternalsVisibleTo.
Align the newly scaffolded Abblix.Oidc.Client.UnitTests with the test-stack upgrade merged from develop: it referenced the xunit.v3 metapackage, which no longer carries a central version after the switch to the mtp-v2 variant.
Adds the first feature of the client core: where the provider's endpoints come from. Every later feature reads them through IProviderMetadataProvider, so a provider that moves an endpoint is followed automatically. Two sources answer that contract. DiscoveredMetadataProvider reads the document the provider publishes, verifies the issuer it declares against the authority it was fetched from (OpenID Connect Discovery 1.0 section 4.3) and caches it. ConfiguredMetadataProvider serves endpoints the host wrote by hand, for the many OAuth 2.0 providers that publish no document at all. The host names its source: AddDiscovery or AddConfiguredMetadata. Nothing is inferred from the shape of the configuration, and there is no default - reading endpoints from a signed-off document and taking them on the host's word are different trust models. Omitting the choice leaves a guard registered that fails with a message naming both calls, mirroring the external-key tier guard. Discovery settings live in their own DiscoveryOptions rather than on the client options, so a client that is told its endpoints never sees settings it cannot use. Unmodelled members of the document are kept rather than discarded, so a paid layer or a host can read a provider capability the base client has no opinion about. 15 unit tests, solution builds clean.
Adds the source of the keys that verify the provider's signatures, read from the address its metadata names and parsed with Abblix.Jwt. Rotation is handled here rather than by callers. A provider replaces its keys on its own schedule, so a token naming a key the client has not seen is a normal event: it triggers a re-read of the key set instead of a rejection. That path is driven by whoever presents the token, so it has a floor - without one, a stream of tokens naming random keys turns this client into a load generator against its own provider. A key still unknown after the re-read leaves every held key on the table, because deciding "no such key" here would turn a labelling difference into a silent authentication failure. Keys marked for encryption are not offered for signature verification, which is what the `use` member exists for. The floor and the cache are per client instance, so N replicas allow N reads per window rather than one. Said plainly in the options documentation, and the contract is registered with TryAdd so an application that needs a bound across replicas supplies its own implementation. Making a free base client depend on shared storage to get that bound would be the wrong trade. Also extracts the fetch-and-hold behaviour both this and the discovery document wanted into one internal primitive, and fixes a real defect found while testing it: concurrent forced refreshes were deduplicated by comparing timestamps, which a clock too coarse to separate two adjacent operations resolves in favour of the entry the refresh was asked to replace. Now compared by entry identity. Cache lifetimes now carry downward jitter, so replicas started by one rollout stop expiring in lockstep and going to the provider in a single wave. 24 unit tests, solution builds clean.
The cache serialised callers with a semaphore, which gives mutual exclusion but not a shared result. Two consequences, both wrong in the direction that matters: callers woke one at a time only to find the value already there, and during an outage each waiter went on to make its own failing attempt, so a burst of requests became a queue of round trips against a provider already in trouble. Callers now share the attempt itself. One publishes it, everyone who arrives while it is in flight awaits the same one, and a failure is shared rather than repeated. The attempt runs without any single caller's cancellation token, so one caller giving up no longer cancels the read everyone else is waiting on; each abandons only its own wait. Also adds a fixed set of verification keys as an alternative to reading the provider's key set: for a provider that publishes none, a deployment that cannot reach the one it publishes, or an operator pinning keys deliberately. The trade is that a rotation then needs a reconfiguration, so it is a choice rather than a default, registered the same way the metadata source is. 29 unit tests, five of them on the cache itself, including concurrent sharing of both a successful and a failing read.
Adds the outbound half of signing in: the address the user is sent to, and everything the client must put aside in order to judge their return. The three arrive together because they are one concern. An authorization response comes back on a separate request, from the browser, carrying only what the provider chose to echo, so every value used to judge it has to be stored beforehand: the state that says which request it answers, the nonce that ties the id_token to it, the code verifier that proves the exchange, the issuer that was actually asked, and where the user was heading. PKCE is SHA-256 and nothing else. RFC 7636 also defines `plain`, where the challenge is the verifier itself, and falling back to it when a provider looks unable to do better is the downgrade: anyone who reads the request then holds the verifier while the request still looks protected. A provider advertising only `plain` therefore gets a refusal rather than a weaker request. One that advertises nothing is given the benefit of the doubt, since the member is optional and a request the provider cannot honour fails at the provider. Stored state is single-use: taking it removes it in the same step. Left in place, a captured authorization response could be replayed and every replay would find its state waiting and look entirely legitimate. Entries also expire and are swept on write, because they carry a code verifier and a store that only grows is a slow leak driven by anyone able to start a sign-in. The in-memory store is correct for one instance only, and says so: a callback may land on any replica. The cookie-backed store of the ASP.NET adapter is the answer there, and it travels with the user rather than living on one node. Resource indicators are sent one parameter per resource rather than collapsed, since RFC 8707 lets it repeat and keeping only the last would silently widen what the token is good for. That needed an append to the shared parameters builder, which until now could only replace. Names follow the server side of the family: the feature is Pkce, as in PkceValidator, and the challenge methods carry the same constant names. The earlier ProofKeys naming was wrong twice over, since it also reached into the vocabulary of DPoP, which is a different feature. 44 unit tests, solution builds clean.
Adds the exchange of an authorization code for tokens and the refresh of an expired set, with the client authentication both need. The client authentication method is configured, not inferred from what the provider advertises. Picking the strongest method a provider claims to support sounds accommodating, but it hands the provider's own document the decision of how this client proves who it is, and a downgrade arrived at that way is silent. A public client is the default, since a client with no secret cannot leak one; a confidential client says so and supplies its secret. Basic credentials form-encode both halves before joining them, as RFC 6749 section 2.3.1 requires. Without it a secret containing a colon or a space reaches the provider as a different secret, and the failure reads as a wrong password rather than a wrong encoding. The error code a refusal carries is kept on the exception rather than folded into its message, because callers act on it: a refresh answered with invalid_grant means the presented token has been rotated away, which is recoverable, and every other code is not. A refusal whose body does not follow the documented shape is still a refusal, so an unreadable body cannot mask the status. The token endpoint gets its own named HttpClient. That is the seam a paid layer needs in order to hang certificate-bound mutual TLS or the DPoP nonce retry on this exact traffic, without either reaching into request building. Unmodelled members of the response are kept, as elsewhere. Single-flight refresh per session is deliberately NOT here. It sits above this service, needs the session store to exist first, and is recorded in the design with the two tests it must pass. 55 unit tests, solution builds clean.
Measured line coverage of Abblix.Oidc.Client at 87.0% and found one real hole rather than a diffuse shortfall: ConfiguredSigningKeysProvider had no test at all. A source of verification keys with zero tests is the kind of thing that looks present and is not, so it is covered first: key selection, the fallback when the named key was never configured, and the refusal of an empty set. Also covers what the measurement showed dark in the token service - the transport failure, a success whose body cannot be read, a success carrying a literal null, and an authentication method this client cannot present - plus a first test of the registration extensions themselves. The features were tested by constructing services directly, which says nothing about whether the extensions name the right lifetimes and dependencies; a missing registration surfaces only when something asks the container, so now something does, with ValidateOnBuild and ValidateScopes on. 87.0% to 98.6% of lines, 88.6% of branches, 65 tests.
The client's files were started from a server file, so all 59 of them opened with "Abblix OIDC Server Library". Corrected to "Abblix OIDC Client Library". The line names the product a file belongs to, and the client is its own product: it lives in this repository only so it can reference Abblix.Jwt by project reference, not because it is part of the server. The other 772 files in the repository keep the server line correctly, including Abblix.Jwt and Abblix.Utils, which are the server's foundations, and the Vault and Azure packages, which are its backends.
Adds AccessTokenHash (at_hash) and CodeHash (c_hash) to JsonWebTokenPayload. Both claim names were already in IanaClaimTypes; only the accessors were missing, so reading them meant a string literal at the call site. That matters more here than for most claims. These two are how a relying party detects a substitution - at_hash catches an access token swapped for another, c_hash catches an authorization code swapped in the front channel - and a misspelled claim name does not fail, it reads as absent. An absent hash is indistinguishable from a provider that chose not to send one, so the check quietly becomes a no-op and the substitution it was meant to catch goes through. The client's id_token validation needs both; the names are spelled out rather than mirroring the wire, matching AuthorizedParty and SessionId alongside them. 450 tests in the Jwt suite, five of them new.
# Conflicts: # Abblix.Oidc.slnx
The client has to verify exactly what the server issues, and it may not reference the server package, so the computation moves down into Abblix.Jwt where both can reach it. Nothing about it changes on the way: same ASCII octets, same left-most half, same digest chosen by the id_token's own alg. The name is BindingHash rather than TokenHash because an authorization code is not a token, and the library already keeps the two apart as CodeHash and AccessTokenHash. What the pair have in common is their purpose: each binds a value delivered alongside the id_token to the signature over it, so an attacker who swaps the code or the access token is caught. A null result now says "this algorithm has no digest paired with it" instead of being handled inside the issuing path. That distinction has to survive the move, because the two sides answer it differently: an issuer omits the claim, while a client must refuse to call the binding satisfied - "no hash was computable" and "the hash matched" must never look alike to a verifier. The tests take their expected values from the worked examples printed in OpenID Connect Core section 3.3.2.11, not from running this code. A vector recorded off the implementation agrees with the implementation by construction; these agree with what everyone else computes.
An authorization code is not a token, so the earlier name spoke for only half of what the type computes - the library itself keeps the two apart, as CodeHash and AccessTokenHash. HashCalculator says what it is without claiming which of them you are asking for.
The thirteen steps of OpenID Connect Core section 3.1.3.7, split along the line that already exists: everything decidable from the token alone stays with the JOSE layer - signature, algorithm, expiry - and everything that has to know what this client asked for lives here. A nonce, a hash binding and a max_age are all comparisons against a question the JWT layer never saw, which is why a validator handed only the token would call a perfectly replayed one valid. Three places where the specification's modality is not what it first looks like, each recorded at the callsite: REQUIRED in a claim table binds the issuer, not the recipient. azp is OPTIONAL and only checked when present; at_hash and c_hash are checked whenever both halves are in hand, but their absence is not a rejection - demanding them outside the authorization endpoint would refuse conformant providers. Where the text says SHOULD and this client refuses anyway, the comment says so in our own voice rather than dressing the choice up as a mandate. Performing the hash check is a SHOULD in the front channel and a MAY in the code flow; skipping it for want of an obligation would leave a swapped code undetected, so it always runs. The accepted algorithm set comes from what this client registered for, never from the provider's advertised id_token_signing_alg_values_supported. Deriving acceptance from what a provider is willing to sign with is the shape of an algorithm-substitution attack. An unverifiable binding is a rejection, not a pass: when the signing algorithm has no digest paired with it, "the hash could not be computed" must not look like "the hash matched". The client's dependency graph is unchanged - Abblix.Jwt and Abblix.Utils only, which is why the hash computation moved down there first. Two of the tests initially had identical bodies and the analyser caught it. They were rewritten to exercise their own claims rather than silenced: azp matching this client, and a nonce arriving that was never asked for.
Five local Fetch functions took a CancellationToken and never touched it, which reads as an oversight and is the opposite: the cache never passes a caller's token into the fetch, because the attempt is shared and honouring one caller's cancellation would abort the read the others are waiting on. That is what OneCallerCancellingDoesNotCancelTheOthers pins. Naming the parameter '_' says ignored-on-purpose, and one comment on the class says why, rather than five copies of the same sentence.
The mix-up defence. A client that talks to more than one provider, tricked into sending its user to an attacker-controlled one, gets back what looks like an ordinary success and hands over the authorization code together with the PKCE verifier. Nothing inside the code says who issued it, so without this the mistake surfaces only when the token endpoint refuses it - one round trip after the code was given away. Three details that the specification states the opposite way round from the obvious reading, each recorded at the callsite: Comparing an issuer that IS present is unconditional. Refusing one that is ABSENT is the half gated on authorization_response_iss_parameter_supported. Getting these backwards ships a client that accepts an iss-stripped response from a server it knows sends one - which is the exact attack. Section 4 lets an ID Token returned from the authorization endpoint stand in for the parameter: with the issuer already named and checked, "the use and verification of the iss parameter is not necessary and MAY be omitted". A blanket reject-if-absent would refuse every JARM response, where the issuer travels inside the response JWT and no top-level parameter exists at all. The same section adds a duty that comparing against an expectation cannot express: "if a client receives an authorization response that contains multiple issuer identifiers, the client MUST reject the response if these issuer identifiers do not match". Two identifiers are compared to each other first, because a response whose parameter is correct and whose ID Token names somebody else would otherwise pass. Section 2.4's SHOULD to discard an issuer from a provider that never advertised sending one is offered as a setting rather than taken as the default, on the authority of the sentence immediately after it: legitimate providers do exactly that, and the value is compared either way, so refusing them by default costs interoperability and buys nothing. Comparison is RFC 3986 section 6.2.1 simple string comparison - ordinal, after form-urldecoding - so a trailing slash or a differently cased host is a different issuer rather than a forgiving one. The tests caught a real defect in the registration: IOptions had no registration unless a configure delegate was passed, so the one host unable to construct the validator was the host that accepted every default.
The value stored as ReturnUri is where the user goes once the login finishes, and in practice it arrives from the request that triggered the login - a "?returnUrl=" the user agent supplied. It was stored verbatim, which makes the client an open redirector: one ordinary-looking link lands the victim on an attacker's page immediately after a genuine interaction with their real identity provider. That is the most trusted moment in the whole flow and therefore the best possible place to be asked to sign in again. No forged response, no stolen state, no misbehaviour by the provider. The check belongs where the value enters the store rather than wherever it is later read. A rule applied at the point of use is one every future caller has to remember, and the point of use does not exist yet - it arrives with the response handler, which will hand this address back on the error path. What this package can enforce is the host-agnostic half: the address must be relative, so it carries no authority of its own. Whether a relative path is one the application serves is a question only the host can answer. A same-origin absolute address would be harmless, but nothing here knows the origin to compare against, so "relative" is the strongest rule that can be enforced rather than merely recommended. The protocol-relative and backslash forms are refused with it: browsers resolve //evil.example against the current scheme, and normalise a backslash to a slash before doing so. The redirection endpoint gets the opposite guard in the same change, because the two are easy to confuse and the requirements point opposite ways. That one is resolved by the provider, not the application, so RFC 6749 section 3.1.2 requires it to be absolute - and Uri holds a relative address just as happily, so the type could not say it. Configured relative, it would have gone out as a malformed parameter and come back as whatever error that provider raises, a long way from the mistake that caused it. Found by an adversarial review of a design that proposed handing ReturnUri back on the error path. The design was refused; the sink it would have opened turned out to be already present.
The guard was right and its explanation was not. It said the provider resolves this address rather than the application, which is true and stops short of the consequence: resolved against the provider's own address, so the user finishes authenticating and lands somewhere on the provider's site, never reaching this application at all. The earlier wording guessed at a malformed parameter and some error coming back, which is not what happens. Stated as the outcome rather than the rule, because that is what a reader hitting the exception needs, and because it is what makes the pairing with ReturnUri obvious: the two point opposite ways precisely because they are resolved in opposite places.
Still not the provider. It hands the address to the browser, and the browser resolves it from where it is standing at that moment, which is the provider's own page. That is where the base comes from, and saying 'the provider resolves it' skipped the step that explains why. It also sharpens the pairing with the return address: the two requirements are opposite because the browser resolves them standing in opposite places - the return address once it is already here, the redirect address while it is still there.
The in-memory store keys entries on the state value alone, so a login is bound to the process and not to the browser that started it. Anyone holding a genuine, unconsumed state can have any browser present it: the entry is found, the nonce matches, the verifier is right, and the client signs that browser into the account the login was started for. That is login CSRF. Neither escape in RFC 9700 section 2.1 is open. Its fallback wants state tokens 'securely bound to the user agent', and section 2.1.1 shuts the other route with 'In any case, the PKCE challenge or OpenID Connect nonce MUST be transaction-specific and securely bound to the client and the user agent in which the transaction was started'. PKCE does not exempt a client from the binding; the binding is what PKCE's CSRF property rests on. No code changes, because there is no fix to make here: this package has no notion of a user agent. What was missing is the sentence saying so. The obligation now sits on the interface, where an implementer will meet it, and the default store's limits are written on the default store - which also promotes the adapter's cookie-backed store from 'the multi-replica answer' to the thing that makes the flow correct. The state value's own doc gets the narrower correction: a value that was issued says which login the response belongs to, not whose browser it arrived in. It read as if the converse held.
Parsing and judging are separate steps because something has to hold the parameters while the checks run, and it must not look like a verdict while it does. Nothing here is verified: until the RFC 9207 issuer check passes, these values are not even known to have come from the provider this client asked - section 2.4 says for error responses "clients MUST NOT assume that the error originates from the intended authorization server". Parameters arrive as a name-to-values map rather than name-to-value on purpose. Which value a collection API keeps for a duplicate - first, last, joined - is that API's business, and letting it decide would mean the token endpoint sees one code while the checks ran against another. RFC 6749 section 3.1 forbids the repetition outright, in a sentence that covers this direction too: "Request and response parameters MUST NOT be included more than once." So it is refused rather than resolved, since resolving it is precisely the attacker's move. The map is also what keeps the package free of ASP.NET Core: an adapter builds it from a query string, a posted form, or parameters a script lifted out of a fragment, and the parser neither knows nor cares which. Four shapes, not two. A response carrying neither a code nor an error is not an error response - it is a request that reached the callback address without being an authorization response, which is a different thing to answer for. A response carrying both is named rather than resolved: reading it as an error discards a real code, reading it as a success acts on a code the provider paired with a refusal, and no specification says which it is. Unknown parameters are left alone, as section 4.1.2 asks in as many words - "The client MUST ignore unrecognized response parameters" - and an unfamiliar error code survives verbatim, because the registry is open and the code nobody recognises is the one an operator most needs to read. error_description and error_uri carry warnings rather than conveniences. The first is text chosen by whoever sent the response; RFC 6749 section 4.1.2.1 bounds its character set and says nothing about meaning, so a conforming value can still read as an instruction to the user. The second is typed as a string rather than a Uri so that nothing about it suggests navigation. The error codes live in their own class next to the token endpoint's, and not because the two sets are disjoint - three values appear in both. It is that a caller reading an authorization response should never be offered invalid_grant, nor one reading a token response login_required.
This is the CSRF check RFC 6749 section 10.12 asks of the redirection endpoint, in the form a client that always sends state can make it: a response is acted on only if it names a login this client is holding. Consuming is take-and- remove, so the same response replayed a second time finds its state already gone - RFC 9700 section 4.7 counts authorization-response replay as a threat to close, not tidiness. A miss has two shapes, kept apart because they mean different things to whoever decides what the user sees. Missing state is a response that never belonged to us: this client sends one on every request, so its absence is malformed or forged, never an expiry to restart. Unknown state merges three situations on purpose - expired, already handled, never issued - because telling them apart needs the entry, or a marker of its key, to outlive the moment it should have been discarded, which lengthens the very window the lifetime exists to bound and turns the store into an oracle answering "was this value ever issued". For a replayed response that answer would confirm to whoever captured it that the victim finished signing in. The consumer is not the whole CSRF story and its doc says so. Whether the matched login belongs to the browser now presenting it is a question this seam cannot ask - it belongs to the store, and the default in-memory one does not answer it. A browser presenting a genuine state without the cookie that carries the ticket lands in the same Unknown, which is how a user-agent-bound store folds login CSRF into an indistinguishable miss. That binding is the ASP.NET adapter's, task #173. Registration is self-sufficient: the consumer feature TryAdds the same store default the request side uses, so a host that only handles the callback still resolves, and a host's own store registered first wins over both.
The order is the whole of this piece, not an implementation detail. A response arriving at the callback is untrusted, and each step is a gate the next depends on: parse, refuse the shapes no specification defines, locate the held login, confirm the issuer, and only then act on what the provider said. Confirming the issuer before reading the error code is the load-bearing part. RFC 9207 section 2.4: "For error responses, clients MUST NOT assume that the error originates from the intended authorization server." An error code logged or returned before that check is an attacker's claim recorded as the provider's. The test that pins this is an error response naming the wrong issuer: it must fail on the issuer, carrying no error code, not come back as though the real provider had refused. Locating the login is split from spending it, and the split is a security boundary rather than tidiness. Reading the stored state yields the issuer the next check needs; removing it is the single-use spend, and that must not happen until the response has earned it. Spending first let a forged wrong-issuer response burn a victim's pending sign-in - the state is not a secret, it rides the request URL to the provider, so anyone who saw it could deny the victim their login. Found in review. The store now offers FindAsync (read) and RemoveAsync (atomic spend) instead of a combined take, the handler reads before the issuer check and spends after, and the regression test proves a forged response leaves the victim's genuine callback able to complete. Verified red against the old order first: only that test fails, the other nine hold. The malformed shapes are refused before the login is even located, for the same reason spelled out one layer down: a response that is neither a code nor an error, or both at once, is not one to spend a stored state on.
AuthorizationState the type collided in the reader's head with the state parameter it carries: 'state.State' is a variable named for the wrong one of its own fields. The record is the whole context of a pending authorization - the state and nonce sent, the verifier and return address kept, the issuer addressed - so AuthorizationContext says what it is without borrowing the name of one field. The namespace and folder stay AuthorizationState, and that is deliberate rather than half-done: the feature manages the transient STATE of an authorization across the redirect, and the record is the context it holds. Renaming the namespace to match the type would produce AuthorizationContext.Authorization Context, the exact stutter the repo's own naming rule forbids, and would put the state-lifecycle machinery - the store, the consumer, the single-use gate - under a name that is about the payload rather than the lifecycle. The State properties on AuthorizationRequest and AuthorizationCodeResult become Context for the same reason, and the using-aliases that reintroduced the word 'State' as a second name for the type are gone.
…mespace The three sibling features - the request builder, the response handler, and the state kept between them - are one story told in three parts, so they now sit under Features/Authorization as Requests, Responses and Context rather than as three top-level AuthorizationRequests / AuthorizationResponses / AuthorizationState folders. The shorter leaf names read the same in context and the shared parent is what a reader scans for. Mechanical: files moved, namespaces and imports updated to match, no behaviour changed. The state feature keeps its own leaf name Context for the record it holds while the machinery around it stays State-named, since that machinery is about the lifecycle and the record is the payload.
The in-memory default keys a login by its state value alone, so any browser that presents a genuine state completes the login - login CSRF, and the gap RFC 9700 section 2.1.1 exists to close: 'the PKCE challenge or OpenID Connect nonce MUST be transaction-specific and securely bound to the client and the user agent in which the transaction was started'. This store is that binding, and it is not a check bolted on - it is where the login lives. The whole context, code verifier and all, rides in a cookie on the browser that started the flow; the state value in the callback URL only names which cookie to read. An attacker who learns that value - it travels in the request URL to the provider, so it is not secret - has nothing, because the cookie is HttpOnly, Secure, same-origin, and never left the victim's browser. The payload is encrypted with Data Protection, since it carries the verifier, and a distinct purpose string means a cookie protected for anything else does not decrypt here. There is no server-side entry, so the store is also correct across replicas: the context travels with the user, not with a node. FindAsync reads without removing and RemoveAsync deletes, matching the split the store contract now requires so a response that fails a later check does not spend a login it was not entitled to. AddCookieAuthorizationStateStore replaces the in-memory registration outright rather than TryAdd, because leaving the default in place would defeat the reason to call it. This puts the ASP.NET integration in an AspNetCore folder inside the client rather than a separate package, and the client now carries a framework reference to Microsoft.AspNetCore.App. The reference is shared, not a download, so a web host already has it; a non-web consumer references the assembly but never touches these types. Tested through the real cookie machinery: a login stored on one response is carried as a cookie into the next request and read back, and the tampered, wrong-purpose, and no-cookie cases all come back as a miss rather than a throw.
RFC 6749 section 4.4: the client's own credentials are the authorization, so there is no user, no code and no redirect address - only the grant and, if the caller names any, the scopes it wants. The scopes are a per-call argument rather than configuration, because a client calling one API is not asking for what its users' sessions ask for, and nothing makes those two lists coincide. Asking for nothing omits the parameter instead of sending it empty. Section 4.4.2 marks scope OPTIONAL, and absent leaves the provider to decide what this client's credentials are worth, while present-and-empty asks for no scope at all. The response is documented for what section 4.4.3 says it is: an access token, no ID Token, and no refresh token to keep - re-authenticating costs a client nothing when its credentials are the grant. Both cases are proved against the real provider, and the second is what makes the first mean anything: the client the rest of the suite uses authenticates with the same secret and is registered for the code flow, so the refusal it receives turns on the grant alone.
The type moved in the working tree while this branch was being built, with its namespace changed and its three consumers left pointing at the old one, so the project did not compile. Finished rather than reverted: the destination reads better, since a cache the discovery and key-set providers both hold is a shared piece of this library and not a private corner of one feature. The cache's own test keeps its folder for now; where it lives follows the move once the reorganisation it belongs to has settled.
RFC 8693: the client offers a token, says what kind it is, and receives one in its place. Six of the eight request parameters are optional, so they arrive as a per-call object rather than as arguments, and only the two the specification marks REQUIRED are required on it. Two things the specification states are enforced here rather than left to the provider. An actor token and its type go together, both ways round: section 2.1 requires the type alongside the token and forbids it otherwise, and the missing half is worth catching because a caller that sets only the type believes it is delegating while the request impersonates instead. And resource and audience may each be given more than once, so they are sent repeated rather than joined - the form now carries a list of pairs alongside the dictionary, since a dictionary cannot express what the specification allows. What came back is read rather than assumed: issued_token_type says what was issued, which need not be what was asked for, and token_type reads N_A when the kind is not one that gets presented to a resource. The member is nullable because that marker binds the provider on one response, not this client on all of them. The end-to-end cases start by logging in, so the token presented is one this provider actually issued - an invented string would be refused on the token rather than on the exchange, and would pass for the wrong reason. The second case presents exactly such a string and requires the refusal, so the first is not passing against a provider that would give a token to anyone.
RFC 8628: the device asks for a pair of codes, shows one to its user, and polls with the other until that user has authorized it somewhere else. The polling is the substance rather than a detail, so it lives in the library instead of in every host that would otherwise write it again: wait the interval before each attempt, take five as the interval when the provider names none, add five seconds on every slow_down and keep them, and stop on anything else. The increase being permanent is the half that gets written wrong. A client that waited longer once and then went back would look right on the next poll and be asking too often again by the one after, so the test pins four attempts at 5, 15, 25 and 35 seconds rather than merely counting them. Polling also stops when the stated lifetime runs out, without waiting to be told. The provider is expected to answer expired_token and this does not depend on it: a device left running against one that keeps saying authorization_pending would otherwise poll for as long as it stays switched on. The waiting is proved on a stopped clock, which is the only place a provider can be made to answer slow_down on cue, and the suite does not sit through the intervals it asserts about. What the end-to-end tests settle instead is what a stub cannot: that this provider accepts the request as formed here, and that the codes polled on are the strings it really sends. One of them came back with something worth keeping. Asking for a token the instant the codes were handed over is refused with slow_down rather than authorization_pending, because the poll arrived before the interval had passed. That is section 3.5 working as written, and it settles by demonstration that waiting before the FIRST attempt is a rule with a counterparty behind it.
…claration Carried in from the working tree, where it arrived alongside the move out of Internals, and committed at green rather than left sitting: an uncommitted half-refactor is invisible to everything except whoever happens to run a build. No behaviour change - the clock is the same dependency, named the same, reached the same way.
Two specifications describe this waiting in the same words, because the second copied the first: RFC 8628 for a device, CIBA for a request made on a user's behalf from elsewhere. The interval, authorization_pending, the five seconds added by slow_down and the instruction to stop on anything else are identical; only the parameter carrying the identifier differs. Written out twice, the two would drift without anyone noticing, because a flow that drifts here still works - it merely asks a stranger's server too often, or gives up on a user who was still deciding. So the loop moves out of the device service before the second caller arrives rather than after. Behaviour is unchanged, and the proof is that the five device polling tests pass untouched: they pin the attempts at exact seconds, so any change to when the waiting happens or how long it lasts would show.
… else CIBA in poll delivery mode: the client asks about a person, the provider reaches them on their own device, and the client waits at the token endpoint. Ping and push are left out on purpose - they have the provider call the application back, which is an unauthenticated endpoint of its own, the same shape as back-channel logout, and a separate piece of work. Two rules the specification states are enforced before anything is sent. A request names the person exactly one way, section 7.1 requiring one and only one of the three hints; naming them twice is a request nobody can resolve and naming them none asks the provider to authenticate anybody. And the scopes must carry openid, which that section requires of every CIBA request. The waiting reuses the poller rather than restating it, which is what that extraction was for. Wire parameter names now come from named constants grouped per feature, matching what the authorization request already did. They had been written as literals here, which is a typo waiting to be diagnosed as anything but a typo. Scope values move to a shared class for the same reason, and the default scopes of an authorization request now read through it too. The end-to-end test covers the request and the acknowledgement against the real provider, and stops there for a reason recorded in the file: returning a session from the provider's device-authentication handler does not mark the request answered, so a client polling afterwards is told it is pending until it expires. What does complete one on the provider side is its own piece of work, and a test written before that is understood would either sit for five minutes or assert something untrue.
RFC 6749 section 4.3, which RFC 9700 section 2.4 forbids outright: "the resource owner password credentials grant MUST NOT be used". Five reasons, and they are worth having in the file rather than in a changelog: the user's credentials reach the client at all; they can now leak from more places than the authorization server alone; users get taught to type their password into things that are not their provider; there is nowhere to carry a second factor or any multi-step authentication; and credentials bound to a web origin, which is what a passkey is, cannot be expressed this way. It ships regardless, because a client library meets providers as they are and some offer nothing else. What the prohibition buys is the shape rather than the absence: the grant has its own service behind its own registration, so nothing AddTokenRequests hands out can perform it, a host reaches it only by writing AddResourceOwnerPasswordCredentials, and one search finds every application that did. That is asserted rather than described - a test resolves the service from a container without the call and requires it to be absent, and the mutation that registers it alongside the ordinary token requests turns the test red. Posting to the token endpoint moved into a shared internal client on the way, so the second caller did not arrive with a second copy of finding the endpoint, presenting credentials and telling a refusal from an unreadable answer.
Added an hour ago behind an explicit registration, and removed now that the prohibition was read properly rather than summarised. RFC 9700 section 2.4 is the only unscoped MUST NOT in the whole of that document's section 2: everywhere else it names the party it binds, and here it forbids the grant's USE. A grant is used by a client. The five reasons it gives are all about what the client does with the credentials, which makes a client library the exact artefact the sentence is written about. For calibration, the implicit flow in the same document earns only SHOULD NOT, and this library's server counterpart is certified across profiles that include it. This grant earns the harder ban. It is also absent from OpenID Connect Core altogether, removed from the OAuth 2.1 core, required to be rejected by FAPI 2.0 servers, and exercised by no OpenID Foundation certification profile on either side - so it was permanently uncertifiable surface inside a package whose whole claim is conformance. The server keeps its own password grant, and that is not an inconsistency: a server accepts its own users' credentials into its own store, while a client collects someone else's users' credentials and posts them elsewhere. The prohibition reaches the second, not the first. The shared token-endpoint client stays. It was extracted so the grant would not duplicate the posting logic, and the ordinary token requests now use it, which is worth having on its own. Deleting before the first release is reversible in a patch, because the registration was purely additive. Keeping it past the first release would not have been.
…must judge it RequireIssuer is documented as requiring the iss claim to be present. The validator read it, and RequireAudience beside it, as an instruction to run the caller's delegate, then dereferenced that delegate unconditionally. So the documented use of the flag on its own threw InvalidOperationException from inside validation - on a request path that is a 500 chosen by whoever sent the token, where the caller had asked for a refusal. Presence and validity are now separate questions asked by separate flags, the split ValidateLifetime and RequireExpirationTime already use. A caller that asks for a claim to be judged and supplies nothing to judge it with gets a typed validation failure rather than an exception: the misconfiguration is real, but it is the token that cannot survive it, not the process. The audience half carried the identical defect one method down and is corrected in the same change. Two callers were relying on the conflation, and this is where the change earns its care. LicenseLoader set RequireIssuer alone and supplied a delegate that decides whether a licence is ours - it now says RequireValidIssuer, so the delegate goes on running; without that line the check would have gone quiet and nothing would have failed. RegistrationAccessTokenValidator derives its options as Default without ValidateAudience and had its delegate run anyway; clearing a flag now means what it says, and the audience check it actually needs is the stricter one already on the line below, requiring aud to contain that exact client identifier. Six tests pin the split, and restoring the old condition turns two of them red.
The seam was found rather than guessed. Nothing on the provider's request path marks a backchannel request answered: the status lives in the request storage, and the only production code that writes Authenticated runs from IAuthenticationCompletionHandler.CompleteAsync, which a host calls when its own out-of-band flow returns a yes. The device handler's return value supplies the identity for the eventual grant, never the answer - which is why an earlier attempt at this test polled for five minutes and expired, and why the file used to say the redemption was out of reach. So the test plays the host for one line, reaching the provider's own container for the storage and the completion handler, and everything on either side of that line is the real thing: the request the endpoint accepts, the acknowledgement it returns, the identifier redeeming into an access token and an ID Token, and the second redemption being refused because the identifier is single-use. No waiting is needed after the completion. The grant handler evaluates its Authenticated arm before the one that would answer slow_down, so a request that has been answered is redeemable at once - which is also why this test costs milliseconds rather than the interval.
RFC 6750 section 5.3 says implementations must not store bearer tokens within cookies that can be sent in the clear, and a cookie-backed session holding an access token is exactly that. The obligation was written down, carefully, on SessionAccessTokenSource - which is internal, so its documentation reaches nobody outside this package. An obligation recorded where only its author reads it is a note to self. It now sits on AddSessionAccessTokenSource, the line a host actually writes, next to the SaveTokens note that was already there for the same reason: both name a setting that installing the package does not switch on. The two are CookieSecurePolicy.Always on the sign-in scheme, and forwarded headers where TLS is terminated ahead of the application. The reasoning for not refusing a plain-HTTP request moves with it, because a host reading the obligation is exactly who would otherwise ask why the library does not simply enforce it: refusing then would not un-send a cookie that has already travelled in the clear, and it would break every deployment behind a proxy whose forwarded headers are not configured yet.
The class comment named RFC 6749 section 5.2 for the lot, and only invalid_grant comes from there. The four beside it are defined by RFC 8628 section 3.5 for the device grant, and CIBA section 11 adopts them in the same words. A single class-level citation read as though one document defined them all, which is the failure mode the house rule about citations exists to prevent: it looks checkable, so nobody checks it. The comment also records why these codes stay separate from the authorization and resource sets, since that question has now been asked twice: the three overlap in two values out of twenty-four and are otherwise specific to their own endpoint, so a reader here sees what a token endpoint can answer rather than a catalogue to filter.
…codes A reader of this class sees one class, not a debate about three, so the argument for keeping the sets separate answers a question they never asked - it belongs with the decision record, not in the code. The citation correction stays, because that one is about these codes: only invalid_grant is RFC 6749 section 5.2; the rest are RFC 8628 section 3.5, adopted by CIBA section 11.
Brings in the server nullability audit (#287-289) and the signed-metadata algorithm fix (#290), the last of which was cherry-picked from this branch. That cherry-pick resolved a conflict differently from the original here, so the same test landed twice under different bodies: git saw no textual conflict because the two copies sat at different offsets, and the merge compiled to a duplicate method (CS0111). Kept the develop copy, which uses the MinimalResponse helper the nullability audit introduced, and dropped this branch's older copy, whose inline initialiser no longer compiles now that the response members are required.
… S5332 SonarCloud's new-code security rating on the PR went to B on two S5332 findings - "using http protocol is insecure" - both on the string http://schemas.openid.net/event/backchannel-logout. That string is not a URL this code fetches: OpenID Connect Back-Channel Logout 1.0 section 2.4 defines it as the events member name, and a conformant provider's token carries it literally, so changing the scheme to https would stop the token matching. Suppressed with a justification at both sites, the same way the server marks its RFC 8176 AMR constants safe for S2068. One is the constant itself; the other is a test that stands a sibling event identifier in to prove a non-logout event is rejected.
SonarCloud's new-code security rating held at B on three S6444 findings, one per Regex.Match in the session-check frame tests, each without a timeout. Unlike the http scheme in the logout event name, this one the analyser has right on the merits: an unbounded match can run away on input it did not expect. The input here is the frame this suite fetched rather than an attacker's, so nothing hangs today, but a two-second ceiling costs nothing and is the habit that keeps a later copy of one of these patterns from being the one that does. All three now pass the shared RegexTimeout as the match's own limit.
kirill-abblix
marked this pull request as draft
July 25, 2026 07:25
Brings in the foundation work that landed as #292 - the shared token-binding hash computation, the typed accessors, and the presence/validity flag split - so this branch carries only its own additions on top of it. # Conflicts: # src/Abblix.Jwt/HashCalculator.cs # src/Abblix.Oidc.Server/Features/Tokens/IdentityTokenService.cs
Every null-forgiving operator in the client and its suites is gone. Each was true, which is what made them worth removing rather than leaving: an operator that is right today reads the same as one that is wrong, and neither says who guaranteed it. Three shapes replaced it. Where the fact is provable, the compiler now proves it - the token-exchange actor token and its type are matched in one pattern, so the check that established they arrive together is the one the compiler reads. Where a test needs a value to be there, it says so with an assertion, which names the failed expectation instead of throwing a null reference from whatever used it next. Where neither is possible - a request that reached a handler without an address, a host asked for before it started - a named exception says what was broken. The sweep surfaced duplication rather than causing it: the same reading of a form body sat in three suites and the same reading of a query in two, so both are now Wire; and the same sign-in-through-the-provider sequence sat in three E2E suites, so it is now the fixture's. Its callers dropped the operators as a side effect of no longer holding their own copy.
The return address a login carries appeared six times across three suites and this application's own host four times, each as a literal. A host name that is written out in several places agrees with itself only by luck, and the suites that have to agree on it are exactly the ones asserting that a redirect landed where it should. Both are constants on the fixtures that own them now: the return path on the one that registers the client, the base address on the one that runs the application.
Each was checked against the code rather than obeyed, and each turns out to describe something the rule cannot see. The cast in the front-channel logout endpoint is what gives the dictionary its nullable value type; removing it produces CS8619, so the build refuses the suggested edit. The ignored parameter on the cache's fetch delegates is the signature RefreshingCache.GetAsync accepts, so it cannot be removed, and these cases drive completion themselves rather than through the token. The addresses in the E2E fixtures are the subject of those suites, not configuration - the redirect address is registered under that exact string and compared against it on return. The eleven constructor parameters of the facade are its feature list, and hiding them behind an options type would shorten one line at the cost of saying what the client is made of. The reasoning is in the suppressions rather than in a review comment, because the next person to meet these findings will be reading the code.
…es each The message every flow switch throws on an unrecognised value was typed out in all five of them, and is now one constant built from nameof, so renaming the enumeration breaks the build rather than leaving five messages naming something gone. The recorder the frame's script posts into was likewise spelled out in each of the five cases that read it. Also drops the last null-forgiving operator, on a cookie read the store test performs: the collection's indexer answers with null for a name it does not hold, and the test now says it expects one rather than insisting.
Two conflicts, neither of them in code. The pull-request test workflow takes develop's shape, where the project list is derived from the projects themselves rather than maintained by hand - so this branch's two client suites are picked up without being listed. The solution file takes both sides: each had appended its own projects at the same insertion point.
|
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.
Builds out the base layer of
Abblix.Oidc.Clientfrom the authorization-code core to the flows that need no user at the browser, each one landed against the realAbblix.Oidc.Serverrather than a stub of our own.The package has never shipped, so this branch is where its surface is still free to move; every decision that shaped it is recorded in commit messages and in the internal queue.
What a host can now do
id_token_encrypted_response_algis supported; the client supplies its own decryption keys and a token encrypted to it opens, while one that asked for no encryption still refuses one.max_age,acr_values,login_hint,display,promptandclaims, withmax_ageandacr_valuescarried through to the ID Token validation that was previously unreachable. An incoherentprompt=nonecombination is refused by the client rather than after a redirect.resource/audiencetargets.slow_down, expiry - implemented in the library rather than left to each host.The resource-owner password grant was added and then removed: RFC 9700 section 2.4 forbids its use, the ban reaches the client specifically, and the grant is absent from OpenID Connect Core, removed from OAuth 2.1, and exercised by no certification profile. The reasoning is in the revert commit and the decision record.
Fixes that came out of the work
fix(discovery): signed discovery metadata is signed with an algorithm the key supports rather than read straight off a key that may declare none - a 500 on the discovery endpoint for any certificate-based deployment, live since 2.3. Found by a client end-to-end test against the real server.fix(jwt):RequireIssuer(andRequireAudience) asked for a claim to be present, but the validator read the flag as an instruction to run the caller's delegate and dereferenced it unconditionally, turning the documented use of the presence flag into anInvalidOperationException- a 500 chosen by whoever sent the token. Presence and validity are now separate questions gated by separate flags; the two callers relying on the conflation are carried in the same change.Boundary held throughout
Abblix.Oidc.Clientreferences onlyAbblix.Jwt; it never reaches intoAbblix.Oidc.Server. Where a client feature needed something the server held, the shared piece stayed put and the client got its own.