feat(core): metadata refresh on the verification path, and identifier gating at construction - #28
Merged
Merged
Conversation
RobertoIskandarani
force-pushed
the
docs/record-0.1.0-on-main
branch
from
September 8, 2026 18:02
505a29f to
f4bfe7b
Compare
RobertoIskandarani
force-pushed
the
export/labs-to-public
branch
from
September 8, 2026 18:03
9831bf3 to
f5fa4cf
Compare
… gating at construction
Brings the accumulated core work onto the release line. No API is
removed; two behaviours become stricter at construction time and are
called out with migration notes in the changelog.
The largest item is that authorization server metadata was never re-read
on a resource server that only verifies tokens. Such a server never
calls the token, introspection or revocation endpoints, so nothing on
its request path touched the metadata document after start-up: the copy
fetched at build time was kept indefinitely and a rotated `jwks_uri` was
never followed. `metadataRefreshSeconds` was configuration that did
nothing. Verification now reads through the metadata cache before each
key lookup.
Moving metadata onto the verification path is what makes the rest of the
cache work load-bearing rather than tidy-up, and each piece is a defect
that only bites once metadata is read per request:
- a refresh returning an invalid document used to displace the good
one and repoint key retrieval; documents are now validated at fetch,
and the last valid one keeps being served
- a `jwks_uri` rebind that failed once stranded key retrieval on the
withdrawn URI forever, because the rebind fired on the document
*changing* and the document had already been published; the binding
is now reconciled on each lookup, so a failed rebind retries
- a failed refresh left the document permanently expired, so every
read took the synchronous re-fetch branch — a full HTTP timeout per
verification. Failed refreshes now back off up to 30s, and a read
that finds a refresh in flight serves what it holds
The identifier work closes the sinks that splice a resource identifier
into a response. A missing scheme was refused at derivation in 0.1.0,
which covered the PRM URL but not the DPoP `htu` target, where it read
`null://…` and failed every bound request; scheme, userinfo and fragment
are now all gated at construction, with derivation keeping its checks as
a backstop. The derived PRM URL preserves the identifier's query
component per RFC 9728 §3, and the query is validated against the RFC
3986 §3.4 grammar because it is spliced verbatim into a header field
RFC 9110 §5.5 confines to US-ASCII — an unescaped bracket used to
construct cleanly and then throw a 500 out of the 401 path.
`JwksCache` and `MetadataCache` take an optional `Clock` so the refresh
intervals can be driven from an injected time source; that is what makes
the backoff and interval behaviour testable without sleeping.
Adds `spring-test` as a test-scoped dependency: the new PRM
RouterFunction test drives a real request through
MockHttpServletRequest/Response.
Bumps .conformance-catalog-ref to the revision the new RFC 8707 and
RFC 9728 conformance cases come from.
985 tests, 0 failures (from 917).
…se the review findings
Review found four things that needed action before this ships. Three are
here; the fourth is the release number, which is not set in this diff.
The one that mattered: the backoff did not cover the path a rotation
actually puts every token on. `forceRefresh()` never consulted
`retryNotBeforeEpochSeconds`, and `JwksCache.getKeyByKid(kid, true)`
routes straight into it — which `JwtValidator` calls on every kid the
cached document does not hold. That is exactly the state a rotation to
an unreachable `jwks_uri` leaves behind: tokens arrive signed with keys
the old document does not carry, and each verification paid a full HTTP
timeout on the caller's thread, serialized behind the cache's fetch
lock. Verbatim the failure this change claimed to have fixed. It is also
an amplification surface — an unauthenticated caller sending unknown
kids set the fetch rate.
The existing rotation test could not see it: it reuses one signing key,
so `find(kid, true)` is never reached and its assertion held for the
wrong reason. The new test drives the miss directly, and the negative
control confirms it: with the guard disabled it fails while the old test
still passes.
The bypass is a separate named method rather than a boolean, so no
request path reaches it by flipping an argument. Its only caller is the
test-only metadata refresh, which does want the attempt made.
Second: the RFC 9728 absoluteness case was registered `NONE`/`@Disabled`
with gaps saying scheme-relative identifiers are accepted — which this
same change stopped being true. Enabled and asserted against both
stimuli plus the http-localhost case. PARTIAL rather than FULL: the
requirement is scheme *and* host and only the scheme half is gated at
construction — `https:example.com/mcp` still constructs and fails at
derivation. Understating coverage defeats the point of the catalog as
much as overstating it.
Third, from the Minors:
- `clock` is volatile. It is written after construction, so unlike
every other infra field on the class it carries no JMM final-field
guarantee, and a racily-published client could hand a request thread
a null.
- the cause walk in `isInterrupt` is bounded. `initCause` refuses a
self-reference but not A causes B causes A, which is reachable and
would not terminate. Both that and the wrapped/unwrapped asymmetry
the two call sites depend on are now pinned by tests.
- `wellKnownUrl` backstops all four gates. It ran `requireNoFragment`
and `requireValidQuery` only, so it still spliced userinfo into a
401 challenge. The reason the backstops exist — the method is public
and reachable with a string no constructor saw — does not
distinguish between the four.
- the `normalizeRequestUrl` javadoc argued against its own change:
`%2D` is `-`, an unreserved octet, which RFC 3986 §6.2.2.2 has a
conformant client decode before comparing, so the raw form would
fail to match. Replaced with a reserved octet, and the raw-authority
test now uses a reg-name escape since userinfo can no longer reach
that derivation.
- the `authorityBounds` javadoc sat above `bestEffortAuthorityBounds`,
which has its own; moved.
- the jetty comment claimed the dependency was example-only, which a
new test made false. A second test-scope declaration is not the fix
— Maven keeps the last of two identical coordinates and would strip
`jakarta.servlet` from the compile classpath. The comment was the
thing that was wrong.
- `spring.version` property, so spring-test cannot drift from
spring-webmvc.
- the metadata reconcile ran twice per verification on the kid-miss
path; hoisted to once via a `beforeLookup` hook on `KeyLookup`.
Changelog: the two construction-time rejections move from Fixed to
Changed with migration notes — they narrow behaviour, which is not a fix
from the caller's side — and the backoff entry now says it covers the
kid-miss refresh rather than implying more than it did.
985 -> 988 tests, 0 failures, 1 skipped (was 2).
Both entries said "tracked separately" and pointed at nothing, which is the shape a deferral takes right before it disappears. A reader of a public changelog cannot follow an internal tracker, so the follow-ups are now issues on this repository and the sentences name them. - #29 — the authority and path components of the resource identifier are not gated at construction, while scheme, userinfo, fragment and the query grammar are. Both are spliced into the same 401 challenge and the same served document, and escapeQuotedString strips only control characters, so the argument that justified the query gate covers them unchanged. - #30 — the resource form parameter that the two grants send is not validated. It carries a caller-supplied string rather than the configured identifier, so the fix is not simply the same gates: the issue records the question of whether an SDK should refuse a value it forwards rather than owns. No behaviour change.
…ew nits
Four of these are mine from the previous round, including repeating a
defect I had just fixed one file over.
The changelog was silent on the two things `forceRefresh` did. It is a
public method, inherited by the public `JwksCache` and `MetadataCache`,
and it stopped fetching unconditionally: an embedder calling it inside a
backoff window now gets the held document back, and the return type
cannot say which happened. That is a narrowing and belongs under
`Changed` with the others, naming the method — the previous entry
covered it obliquely under `Fixed`, framed as an internal path.
`forceRefreshIgnoringFailureBackoff()` is likewise new public API and
was in no section at all; it is now under `Added`, saying what it is for
and that no request path should call it.
`doForceRefresh` also took the blocking lock while `get()` deliberately
uses `tryLock` — and `forceRefresh`'s own new javadoc makes the same
request-path argument `get()` makes. The backoff stops a *failing*
endpoint costing a fetch per request but does nothing for the burst
arriving before the first failure records `retryNotBefore`, which would
all queue for one HTTP timeout. It now serves the held document when a
fetch is already in flight, so the two methods agree that no
request-path caller waits on another thread's fetch. Re-ran the negative
control after the change: with the backoff guard disabled the kid-miss
test fails and passes again restored, so the backoff is still what that
test pins rather than the lock change.
The changelog also asserted a contested question as settled: "no valid
identifier is turned away — `urn:example:api` still constructs". The
gate is scheme-only and that value does construct here, but whether
construction is the right place to refuse an identifier with no host is
open — cs-sdk refuses it, and the catalog's own wording asks for a
scheme *and* a host. Stated as scoped and tracked instead of decided.
For the same reason the "every implementation of this derivation settled
on the query-less URL" comment now says which sub-case it means. It was
about the empty query, where the family does agree; for a non-empty
query the implementations still differ, and the sentence could be read
as claiming otherwise.
The rest are nits, all mine:
- `@FunctionalInterface` is back on `KeyLookup`. Dropping it when
`beforeLookup()` was added was unnecessary — a functional interface
may declare default methods — and the annotation is what stops a
second abstract one being added by accident.
- the `buildClientWithClock` javadoc was orphaned by inserting the two
`isInterrupt` tests between it and its method, leaving two blocks
stacked. Exactly the defect the previous commit fixed in
`ProtectedResourceMetadata`.
- the "interrupt reachable inside a cycle" case had no cycle in it:
`d -> c -> InterruptedException` terminates. The loop now closes
through the interrupt itself, so the comment describes the chain.
988 tests, 0 failures.
…document permanently `effectiveTtlSeconds()` subtracted the cache timestamp from the server expiry, so an expiry that was not in the future produced a negative TTL. `CacheHeaderParser.parseExpiresAt` returns `0` for `Cache-Control: no-store` and `no-cache`, the current second for `max-age=0`, and a past epoch for a stale `Expires:` — for `no-store` the result is about -1.7e9. A negative TTL is expired on every read, so `get()` took the synchronous re-fetch branch on the caller's thread every time, forever. The failure backoff could not cover it: that only arms when a fetch *throws*, and an endpoint answering `no-store` successfully clears the backoff and re-arms the expiry on the same call. This was latent while nothing on a verification path read the metadata cache. This change series is what puts it there — verification reads through the cache before every key lookup, and `beforeLookup()` runs before signature verification, so an unauthenticated caller would have set the fetch rate against the authorization server. It is the failure this series exists to remove, reached through a different door. An expiry at or before the cache timestamp is now read as "no preference" and the configured interval governs. go-sdk clamps the equivalent case the same way rather than taking a zero expiry literally. Two tests, and the negative control holds: with the clamp removed both fail and the pre-existing `get_serverExpiresTtl_usesMinOfConfigured...` still passes, which is why it never covered this — it only exercises a *future* server expiry. Also corrects two claims about gate ordering that were not true. The `requireNoUserinfo` javadoc said it runs "last of the four", and the changelog said `wellKnownUrl` "enforces the same four gates as the constructors". The set is the same; the order is not — the three construction sites run fragment, query, scheme, userinfo and `wellKnownUrl` runs fragment, scheme, userinfo, query. An identifier violating two gates can therefore be reported for a different component depending on the entrypoint. Both reject, so only the message differs; unifying the four behind one private gate touches four call sites with different shapes and is left to its own change rather than bolted onto this one. 990 tests, 0 failures.
muralx
previously approved these changes
Sep 8, 2026
RobertoIskandarani
changed the base branch from
docs/record-0.1.0-on-main
to
main
September 8, 2026 20:59
RobertoIskandarani
dismissed
muralx’s stale review
September 8, 2026 20:59
The base branch was changed.
RobertoIskandarani
force-pushed
the
export/labs-to-public
branch
from
September 8, 2026 21:00
6e0e851 to
41604f3
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…fig that reaches the same permanent expiry Three things downstream of the server-expiry clamp, all of which the clamp itself created. `CacheHeaderParser.parseExpiresAt` now returns `null` for `no-store` and `no-cache` rather than `0`. Those directives say the response should not be reused, which for a document this SDK has to keep serving is not an expiry it can honour — the honest answer is "no usable preference". The `0` was read as an absolute expiry at the epoch, which is exactly what made the document permanently stale; once the cache started discarding a non-future expiry the sentinel became indistinguishable from `null`, while the class javadoc still claimed `effectiveTTL = min(configuredTTL, serverExpiry)` and the parameter doc still said "treat as immediately expired". Both were false as of the previous commit. This is a public method, so the changelog says what changed for a caller reading the value directly; go and ts already model the case as absent rather than as zero. `DocumentCache`'s constructor now refuses a non-positive refresh interval and a null clock. `AuthplaneClientBuilder` already rejected the interval, but `JwksCache` and `MetadataCache` expose these constructors publicly and this change series argues they are contract — and a zero interval reaches the same failure through the other parameter: `effectiveTtlSeconds()` returns it verbatim, `age >= 0` on the first read, and every `get()` pays a synchronous fetch. The null clock was surfacing as an NPE from the first read instead of at construction, which is the reasoning `MetadataCache` already applies to `expectedIssuer`. The `Clock` constructors now state the constraint the clamp introduced: a server `max-age` is turned into an absolute expiry against the system clock, in the header parser, before it reaches this cache — and the clamp compares it against a timestamp from the injected clock. A clock offset far from wall time therefore makes every real server expiry read as past, so directives are discarded wholesale and the configured interval governs alone. Safe, but silent, and worth saying on a constructor documented as supported API for simulation clocks. Threading the clock into the parser would remove the constraint, and means changing two public `DocumentFetcher` factory signatures, so it is tracked rather than folded in here. Also: `**no preference**` rendered as literal asterisks in javadoc — the house style is `<em>`; and the scheme migration note said "0.1.0 refused such an identifier at derivation", which reads as a pointer to that version's notes now that they say only "Initial release". Reworded to describe the behaviour, which is what it always meant. 809 + 63 + 120 tests, 0 failures.
Four claims in this branch said something was tracked and pointed at nothing, next to #29 and #30 which do. A deferral with no reference is the shape one takes right before it disappears, and a reader of a public repository cannot follow an internal tracker — so these are issues on this repository: - #31 — whether a resource identifier must carry a host and not only a scheme. `urn:example:api` constructs here and is refused at construction by another implementation in the family, so the same configured value starts one server and stops another. Both readings are defensible; the issue records why, and what moves the RFC 9728 conformance case off PARTIAL. - #32 — query preservation in the derived PRM URL is not yet uniform. The interop failure is concrete: a client deriving the query-less URL receives a document whose `resource` member does not match what it asked for, which RFC 9728 §3.3 requires it to discard. - #33 — the four identifier gates run in two different orders, so the component named in the error depends on the entrypoint. Worth doing before #29 adds two more axes. - #34 — the clock the caches compare against is not the clock the header parser reads, so an injected clock far from wall time silently discards server expiries. Every remaining "tracked" in the changelog and in source now names a number.
9.40 sits inside the vulnerable range (>= 9.38-rc1, < 10.0.2): a denial of service on deeply nested JSON. 0.1.0 shipped with it and is on Maven Central. The reachability is the part that matters. This library parses the access token, on the verification path, before any signature is checked — so the input is attacker-supplied and the caller does not need to be authenticated. That is the same amplification shape as the two cache findings in this branch, on a dependency rather than on our own code. Taken to the current 10.9.1 rather than the minimum fixed 10.0.2, because both are 10.x and impose the identical major on consumers, so the minimum buys nothing while leaving us nine minors behind on the library that does the parsing. The pin carries a comment saying so, and why the version matters, so the next person does not have to re-derive it from an advisory id. Consumer impact is real and is in the changelog: nimbus types are part of this SDK's public API — `DPoPKeyMaterial.fromJwk` takes a `com.nimbusds.jose.jwk.JWK` and `publicJwk()` returns one — so anyone constructing DPoP key material themselves, or pinning nimbus in their own build, needs 10.x. Callers using only the SDK's entrypoints get it transitively. Full suite green on both candidate versions before choosing: 992 tests, 0 failures, on 10.0.2 and on 10.9.1. Found by checking the repository's code-scanning alerts, which is not something the PR checks surface — the five green checks on this branch say nothing about an open advisory on a dependency.
muralx
approved these changes
Sep 9, 2026
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
3 participants
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.
Brings the accumulated core work onto the release line so it can ship as 0.1.1. 31 files, +2890 / −170. 985 tests, 0 failures (from 917). No API is removed; two behaviours become stricter at construction time and carry migration notes in the changelog.
The one that matters most
Authorization server metadata was never re-read on a resource server that only verifies tokens. Such a server never calls the token, introspection or revocation endpoints, so nothing on its request path touched the metadata document after start-up. The copy fetched at build time was kept indefinitely and a rotated
jwks_uriwas never followed —metadataRefreshSecondswas configuration that did nothing. Verified against the released tag:AuthplaneResourceinv0.1.0has no refresh call at all.Verification now reads through the metadata cache before each key lookup.
Why the rest of the cache work is in the same change
Moving metadata onto the verification path is what makes these load-bearing rather than tidy-up. Each is a defect that only bites once the document is read per request, so they cannot land separately without shipping a window where the first change makes the others reachable:
jwks_uri) used to displace the good one and repoint key retrieval. Documents are validated at fetch now, and the last valid one keeps being served.jwks_urirebind that failed once stranded key retrieval on the withdrawn URI permanently: the rebind fired on the document changing, and the document had already been published to the cache, so every later refresh returned the same document and the change never fired again. One 503 at the wrong moment meant every token failed to verify until restart. The binding is reconciled on each lookup now, so a failed rebind retries.Identifier gating
The sinks that splice a resource identifier into a response are now closed at construction rather than one at a time at each sink.
A missing scheme was refused at derivation in 0.1.0. That covered the PRM URL but not the other splice: the DPoP
htutarget readnull://api.example.com/mcp, so every bound request against a scheme-relative identifier failed. Scheme, userinfo and fragment are all gated at construction now, with the derivation checks kept as a backstop.The derived PRM URL also preserves the identifier's query component (RFC 9728 §3 inserts the well-known string "between the host component and the path and/or query components"). The query is validated against the RFC 3986 §3.4 grammar because it is spliced verbatim into a header field RFC 9110 §5.5 confines to US-ASCII — an unescaped bracket (
?filter[a]=b, whichjava.net.URI, browsers and servlet containers all accept) used to construct cleanly and then throw a 500 out of the 401 path.Both migrations are in the changelog. The realistic one is the bracket case.
Also here
JwksCacheandMetadataCachetake an optionalClock. This is what makes the backoff and interval behaviour testable without sleeping — the reason it is additive rather than a test-only seam.spring-testas a test-scoped dependency: the new PRM RouterFunction test drives a real request throughMockHttpServletRequest/Response..conformance-catalog-refmoves to the revision the new RFC 8707 and RFC 9728 conformance cases come from.What is deliberately not here
scripts/backport-fixes.shand its test suite are untouched here. They are release tooling rather than product, and reworking them alongside a behaviour change would bury the parts above. No follow-up is promised for them in the changelog — this is a scoping note, not a deferral.Verification
mvn verifygreen across all four modules against the pinned catalog: 802 + 63 + 120 tests, 0 failures, 2 skipped.