diff --git a/.conformance-catalog-ref b/.conformance-catalog-ref index efa9db0..bf2d5bd 100644 --- a/.conformance-catalog-ref +++ b/.conformance-catalog-ref @@ -1 +1 @@ -b4c758a7dac698d7fcacd32dafcd4bb2f5dbddaf +583a6d92412543ea352251c88f15f2c5a39d2593 diff --git a/CHANGELOG.md b/CHANGELOG.md index a8c4520..d484476 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,209 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `JwksCache` and `MetadataCache` gained public constructors taking a `java.time.Clock`, so the + refresh intervals can be driven from an injected time source rather than the wall clock. Additive; + the existing constructors are unchanged and delegate with `Clock.systemUTC()`. +- `DocumentCache.forceRefreshIgnoringFailureBackoff()` — public, and the only way to force a + fetch while a failed refresh is backing off. It exists for a caller that is asking a question + and wants the attempt made rather than the cached answer, and is prepared to wait for a + timeout: an administrative refresh, or a test. It is named rather than a boolean on + `forceRefresh` so no request path reaches it by flipping an argument, and no request path + should. + +### Fixed + +- A server cache directive that is not in the future no longer makes the document permanently + expired. `Cache-Control: no-store` and `no-cache` parse to an expiry of `0`, `max-age=0` to the + current second, and a stale `Expires:` to a past one; the effective TTL was computed by + subtracting the cache timestamp from that, so any of them produced a *negative* TTL — about + -1.7e9 for `no-store`. A negative TTL is expired on every read, so every read took the + synchronous re-fetch branch on the caller's thread, and the failure backoff could not help + because it only arms when a fetch throws: an endpoint answering `no-store` successfully cleared + the backoff and re-armed the expiry on the same call. Such an expiry is now treated as no + preference and the configured interval governs. This was latent while nothing on a verification + path read the metadata cache; the change above puts it there, before signature verification, so + an unauthenticated caller would otherwise have set the fetch rate against the authorization + server. + +- Authorization server metadata is now re-read under ordinary verification traffic, so + `metadataRefreshSeconds` takes effect 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 SDK kept the copy fetched at build time + indefinitely and never followed a rotated `jwks_uri`. Verification now reads through the metadata + cache before each key lookup; once the configured interval has elapsed the document is re-fetched + and a rotated `jwks_uri` rebinds JWKS fetching before the lookup runs. +- A metadata document is validated when it is fetched rather than when it is read, so a refresh + returning an invalid document (wrong issuer, non-HTTPS endpoint, missing `jwks_uri`) no longer + displaces the good one and cannot repoint key retrieval. The last valid document keeps being + served. `jwks_uri` is now part of that validation rather than being checked at read time. RFC 8414 + §2 marks it OPTIONAL — REQUIRED is OpenID Connect Discovery, a different document — but this SDK + requires it regardless: every verification path builds a JWT validator, and introspection is + layered on top of that rather than offered instead of it. It is also the field the refresh + mechanism itself runs on — a document without it left nothing to reconcile the JWKS binding + against, so a later rotation would not be followed even after the authorization server fixed its + document. A document lacking it already failed at `AuthplaneClient` build time; what changes is + that a *refresh* returning one is now rejected and backed off like any other failed refresh, + instead of being published and then raising on every read. +- A failed `jwks_uri` rebind is retried instead of stranding key retrieval on the withdrawn URI. + The rebind used to be driven by the metadata document *changing*, and the document is published to + the cache before anything acts on the change — so one transient failure at the new endpoint left + JWKS fetching pinned to a URI the authorization server had already withdrawn, with nothing left to + re-trigger it: every later refresh returned that same document, so the change never fired again. A + single 503 at the wrong moment meant every token failed to verify until the process restarted. The + binding is now reconciled against the document on each key lookup, so a rebind that fails simply + retries on the next one. +- A failed document refresh no longer costs a network round trip on every subsequent lookup. The + cache timestamp advances only on success, so against an unreachable endpoint the document stayed + permanently expired and each read took the synchronous re-fetch branch — a full HTTP timeout per + verification once metadata moved onto the verification path, serialized behind the cache's fetch + lock. A failed refresh now backs off for up to 30 seconds (never longer than the configured + refresh interval), and a read that finds a refresh already in flight serves the document it holds + rather than waiting for it. The same backoff governs a failed `jwks_uri` rebind: the binding is + reconciled against the metadata document on every key lookup, so a rotated `jwks_uri` that is + down would otherwise cost a JWKS fetch per verification — the rebind builds a fresh cache per + attempt, which has no backoff of its own to inherit. Tokens whose keys are already cached keep + verifying throughout, and the rebind is still retried until it succeeds, on the backoff instead + of on every lookup. The backoff also governs the forced refresh a `kid` miss triggers, which is + the path a rotation puts every token on once the keys in hand are the new ones: without it each + such verification paid a full fetch against a failing endpoint, and an unauthenticated caller + presenting unknown `kid` values set that rate. +- `elideSecrets` no longer ships the userinfo of a scheme-relative identifier whose path or query + contains a later `://`, such as `//svc:pw@api.example.com/mcp?next=https://x`, in a message that + claims to have elided it — the authority is now located by testing the leading `//` first. + +### Changed + +- **BREAKING** `nimbus-jose-jwt` moves from 9.40 to 10.9.1, closing GHSA-xwmg-2g98-w7v9 — a + denial of service on deeply nested JSON. 9.40 sits inside the vulnerable range + (`>= 9.38-rc1, < 10.0.2`), and 0.1.0 shipped with it. This library is what parses an + 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. + + **Migration:** nimbus types are part of this SDK's public API — `DPoPKeyMaterial.fromJwk` + takes a `com.nimbusds.jose.jwk.JWK` and `publicJwk()` returns one. If you construct DPoP key + material yourself, or pin `nimbus-jose-jwt` in your own build, you need nimbus 10.x. If you + only use the SDK's own entrypoints, Maven resolves it transitively and there is nothing to do. + Pinning to the minimum fixed version (10.0.2) rather than the current one would not have + softened this: both are 10.x and impose the same major. + +- `CacheHeaderParser.parseExpiresAt` returns `null` for `Cache-Control: no-store` and `no-cache` + instead of `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 — so the honest answer is "no usable + preference", and the caller falls back to its configured interval. The `0` was read as an + absolute expiry at the epoch, which is what made the document permanently stale; once the cache + started discarding a non-future expiry the sentinel became indistinguishable from `null` while + the javadoc still claimed it meant "immediately expired". Both sibling SDKs already model this as + absent rather than as zero. A caller reading the return value directly should treat `null` as + "use your own interval"; nothing else in this SDK distinguished the two values. + +- `DocumentCache.forceRefresh()` now respects the failure backoff instead of fetching + unconditionally, and returns the currently held document when a refresh is backing off or + already in flight. This is a public method, inherited by the public `JwksCache` and + `MetadataCache`, so an embedder calling it during a backoff window now gets a cached document + back — and the return type cannot say which happened. It changed because the SDK's own + request path reaches it: `JwksCache.getKeyByKid(kid, true)` is called on every `kid` the + cached document does not hold, which is the state a rotation to an unreachable `jwks_uri` + leaves behind, and fetching unconditionally there cost a full HTTP timeout per verification. + Use `forceRefreshIgnoringFailureBackoff()` if you need the old unconditional behaviour. + +- A resource identifier carrying userinfo (`https://svc:pw@api.example.com/mcp`) is now rejected at + construction by the new `ProtectedResourceMetadata.requireNoUserinfo(String)` gate, called from + `AuthplaneClient.resource(...)`, the `AuthplaneResource` constructor and + `ProtectedResourceMetadata.Builder#build()`, because the identifier is published verbatim to + unauthenticated callers (RFC 9110 §4.2.4, RFC 3986 §3.2.1); a host with a port and an opaque + identifier such as `urn:example:api` are unaffected. + + **Migration:** If `authplane.resource` (or the `resourceUri` passed to + `AuthplaneClient.resource(...)`) carries userinfo, remove it — otherwise the resource, and a + Spring context that builds one, now fails at startup. Present the credential in the + `Authorization` header instead. + +- A resource identifier without a scheme is now rejected at construction — + `AuthplaneClient.resource(...)`, the `AuthplaneResource` constructor, and + `ProtectedResourceMetadata.Builder#build()` all call the new + `ProtectedResourceMetadata.requireScheme(String)` gate. As shipped in 0.1.0 the SDK refused such an identifier at + derivation, which covered the PRM URL but not the other sink that splices the scheme: the DPoP + `htu` binding target read `null://api.example.com/mcp`, so every DPoP-bound request against a + scheme-relative identifier failed. Rejecting at construction closes both. The derivation-time gate + stays as a backstop and now names the requirement the identifier actually fails (no scheme, no + authority, or opaque) instead of listing all of them. RFC 8707 §2 requires an absolute URI, which + RFC 3986 §4.3 defines as always carrying a scheme, so an identifier that names a resource by + scheme is not turned away. Note the gate is scheme-only: an opaque identifier such as + `urn:example:api` still constructs here and fails later if a PRM URL is derived from it. + Whether construction is the right place to refuse an identifier with no host is a separate + question, not settled by this change, and tracked in #31. + + **Migration:** A scheme-relative or relative resource identifier now fails at startup instead of + at the first 401. Prefix the intended scheme. `wellKnownUrl` enforces the same four gates (in a + different order, so the component named in the message can differ from a constructor's) as the + constructors, so a caller reaching it directly with a string no constructor saw is refused there + too rather than splicing a malformed identifier into a challenge. + +- The derived Protected Resource Metadata URL now preserves the resource identifier's query + component. RFC 9728 §3 forms the well-known URI by inserting the well-known string "between the + host component and the path and/or query components, if any", and §3.1 removes the terminating + slash following the host when a path or query is present: + `https://api.example.com/mcp?tenant=a` → + `https://api.example.com/.well-known/oauth-protected-resource/mcp?tenant=a`, and both + `https://api.example.com?x=1` and `https://api.example.com/?x=1` → + `https://api.example.com/.well-known/oauth-protected-resource?x=1`. A query component is legal + in a resource indicator — RFC 8707 §2 states the SHOULD NOT and its exception in the same + sentence, and RFC 9728 §1.2 carries it forward. The raw (percent-encoded) query is preserved + verbatim, as the raw authority already was in 0.1.0. An empty query (a bare trailing `?`) is + treated as absent and derives the query-less URL. An identifier without a query derives exactly + the same URL as in 0.1.0. + + A query component is also now validated at construction, against the RFC 3986 §3.4 grammar + (`query = *( pchar / "/" / "?" )`), by the same four boundaries that reject a fragment. The query + is spliced verbatim into the `resource_metadata` parameter of the `WWW-Authenticate` challenge, + and `WwwAuthenticate.escapeQuotedString` strips only control characters, `\` and `"` — so a raw + non-ASCII octet shipped into a header field that RFC 9110 §5.5 confines to US-ASCII, and a + character `java.net.URI` rejects (a space, `"`, `\`, `|`, `^`, `{`, `}`, `<`, `>`, a malformed + percent-escape) passed construction and then threw out of the 401 response path — a 500 in place + of the challenge. Nothing is escaped on the operator's behalf: rewriting the query would change + the resource's identity. Neither the authority nor the path is gated — both were always part of + the derived URL and are unchanged here — and closing them is tracked in #29. + + The DPoP `htu` derived by `AuthplaneResource.normalizeRequestUrl` now reads the raw authority + too. Previously a percent-encoded userinfo was decoded into it, so the server computed + `https://u@b@host` where the client proved `https://u%40b@host` and the proof never matched. The + matching correction to the PRM URL shipped in 0.1.0; this is the same defect on the DPoP path. + + **Migration:** If your resource identifier contains a query component, the PRM document URL + advertised in `WWW-Authenticate: … resource_metadata=` now includes that query. Update any + hard-coded expectation of the old query-less URL. Your existing PRM route continues to serve the + document — routing is unchanged. Serving distinct documents per query value is not supported. + If that query falls outside the RFC 3986 §3.4 grammar, the resource — and a Spring context that + builds one — now fails at startup rather than at the first 401. The realistic case is unescaped + brackets, `?filter[a]=b`, which `java.net.URI`, browsers and servlet containers all accept; + percent-encode the offending octets (`?filter%5Ba%5D=b`) to keep the identifier. + +- A resource identifier carrying a URI fragment is now rejected at construction + (`AuthplaneClient.resource(...)`, the `AuthplaneResource` constructor and + `ProtectedResourceMetadata.builder()`), with `IllegalArgumentException`. RFC 8707 §2 states the + resource URI "MUST NOT include a fragment component", and RFC 9728 §1.2 defines the resource + identifier as a URL with no fragment. Previously the fragment was dropped when deriving the + `.well-known` URL but published verbatim in the document's `resource` field, so the served + document named an identifier its own URL disagreed with — which RFC 9728 §3.3 requires a + conformant client to discard, with no error raised on the server. Deriving a PRM path or URL + from a fragment-bearing identifier is refused as well, and refused before the identifier is + checked for derivability, so an identifier that is both fragment-bearing and non-derivable is + reported for the fragment — which is also what keeps the fragment out of the message. + + The `resource` form parameter that `ClientCredentialsGrant` and `TokenExchange` send to the + authorization server is *not* gated: that parameter carries whatever string the caller passes to + the grant, not the configured resource identifier, and it is the RFC 8707 §2 indicator in its + primary role. Closing it is tracked in #30. + + **Migration:** If `authplane.resource` (or the `resourceUri` passed to + `AuthplaneClient.resource(...)`) contains a `#`, remove the fragment — otherwise the resource, + and a Spring context that builds one, now fails at startup. Only an unescaped `#` is a fragment + delimiter; a percent-encoded `%23` inside the path is unaffected. Resource identifiers without a + fragment are unchanged, as is the handling of opaque identifiers such as `urn:example:api`. + ## [0.1.0] - 2026-09-07 - Initial release. diff --git a/core/docs/user-guide.md b/core/docs/user-guide.md index 9abb97d..4b248cf 100644 --- a/core/docs/user-guide.md +++ b/core/docs/user-guide.md @@ -212,8 +212,8 @@ Every builder method on `AuthplaneClient.builder(...)`: |---|---|---|---| | `devMode(boolean)` | `boolean` | `false` | Relax SSRF — allow HTTP, localhost, private networks. Overrides `fetchSettings` unless the latter is explicitly set | | `fetchSettings(FetchSettings)` | `FetchSettings` | — | Full control over SSRF / fetch behaviour; overrides `devMode` | -| `jwksRefreshSeconds(int)` | `int` | `300` | JWKS background-refresh interval | -| `metadataRefreshSeconds(int)` | `int` | `3600` | AS metadata background-refresh interval | +| `jwksRefreshSeconds(int)` | `int` | `300` | JWKS refresh interval | +| `metadataRefreshSeconds(int)` | `int` | `3600` | AS metadata refresh interval | | `authProvider(AuthProvider)` | `AuthProvider` | `null` | AS authentication for token / introspection / revocation calls. Pass `new ASCredentials(clientId, clientSecret)` for static HTTP Basic, or a custom provider for credential rotation / non-Basic schemes | | `outboundDPoP(OutboundDPoPOptions)` | `OutboundDPoPOptions` | `null` | Enables DPoP proofs on AS POSTs and `dpopHeaders(...)` | | `executor(Executor)` | `Executor` | `ForkJoinPool.commonPool()` | Executor for all async work. **Production deployments should supply a dedicated executor** — the common pool has limited parallelism (CPU cores − 1) and is shared JVM-wide | @@ -224,6 +224,8 @@ Every builder method on `AuthplaneClient.builder(...)`: `AUTHPLANE_DEV_MODE=true` in the environment flips `devMode` on at build time. +Both refresh intervals are driven by traffic, not by a background timer: the first call past the interval pays for the refetch. For the metadata document that call is a `verify()` — a resource server that only verifies tokens therefore still tracks the AS. When `jwks_uri` changes, the metadata read that discovers it rebinds JWKS fetching to the new URI before the token in hand is verified, and if that rebind fails (the new endpoint is briefly down) the next key lookup retries it. A metadata endpoint that is unreachable never fails verification: the last known good document keeps being served, and a failed refresh is not retried on the network for another 30 seconds. + ### `ResourceOptions` Per-resource configuration. Supply to `client.resource(resourceUri, scopes, options)`. @@ -467,7 +469,7 @@ Well-known path derivation: | `https://api.example.com/mcp` | `/.well-known/oauth-protected-resource/mcp` | | `https://api.example.com/v2/mcp` | `/.well-known/oauth-protected-resource/v2/mcp` | -`ProtectedResourceMetadata.wellKnownUrl(String resourceUri)` returns the full URL. The framework adapters (`authplane-mcp`, `authplane-spring`) register the servlet/router automatically — this is only needed when writing your own adapter. +`ProtectedResourceMetadata.wellKnownUrl(String resourceUri)` returns the full URL. If the resource identifier carries a query component, the returned URL carries it verbatim (`https://api.example.com/mcp?tenant=a` → `https://api.example.com/.well-known/oauth-protected-resource/mcp?tenant=a`) while routing stays path-keyed — the derivation table above is unaffected. The framework adapters (`authplane-mcp`, `authplane-spring`) register the servlet/router automatically — this is only needed when writing your own adapter. ### Dev mode diff --git a/core/pom.xml b/core/pom.xml index 40bfeb7..5bc7557 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -24,10 +24,17 @@ + com.nimbusds nimbus-jose-jwt - 9.40 + 10.9.1 diff --git a/core/src/conformance/README.md b/core/src/conformance/README.md index 111f7e3..2c53692 100644 --- a/core/src/conformance/README.md +++ b/core/src/conformance/README.md @@ -140,18 +140,19 @@ class RfcXxxxConformanceTest { ## Known gaps -There are **no `not_run` cases** — every catalog case has a test implementation, and all -implemented cases are `passed` in `conformance-report.md`. - -Two cases carry **partial** coverage rather than full: +There are **no `not_run` cases** — every catalog case has a test implementation. Three cases carry +less than full coverage: | Case ID | RFC | Coverage | Reason | |---------|-----|----------|--------| | `rfc9449-dpop-inbound-nonce-must-be-validated-when-required` | RFC 9449 §8 | partial (`@Disabled`) | Server-issued inbound nonce enforcement is not yet implemented. RFC 9449 §8 allows but does not require resource servers to enforce nonces. Documented via `@ConformanceCoverage` on the test. | | `rfc9449-dpop-proof-jwk-must-not-include-private-key-material` | RFC 9449 §4.2 | partial (passed) | The proof is rejected as `invalid_dpop_proof`, but the Java SDK does not surface a stable private-key-material diagnostic independent of Nimbus's parsing error. Documented via `@ConformanceCoverage` on the test. | +| `rfc9728-resource-identifier-must-be-an-absolute-url-with-scheme-and-host` | RFC 9728 §3, RFC 8707 §2 | partial (passed) | Both values the case exercises — `/mcp` and `//api.example.com/mcp` — are now refused at construction by `requireScheme`, from the resource factory and the PRM builder. Partial rather than full because the requirement is scheme *and* host and only the scheme half is gated there: `https:example.com/mcp` carries a scheme and no authority, constructs, and is refused only at derivation. Documented via `@ConformanceCoverage` on the test. | ## Definition of done for a conformance case - Status is `passed` in `conformance-report.md` - Coverage level is `full`, or `partial` with all gaps documented in `@ConformanceCoverage` +- A case the SDK does not implement is registered and `@Disabled` with a reason, and reports + `skipped` with coverage `none` — never marked covered, and never left to surface as `not_run` - No uncatalogued tests (every test method has a `@ConformanceCase` mapping) diff --git a/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceCatalogTest.java b/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceCatalogTest.java index 761d52a..b70348b 100644 --- a/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceCatalogTest.java +++ b/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceCatalogTest.java @@ -12,6 +12,7 @@ import java.util.Set; import java.util.TreeSet; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; class ConformanceCatalogTest { @@ -99,8 +100,38 @@ void catalogCasesAndConformanceMappingsAgree() throws Exception { .isEmpty(); } + @Test + void noneCoverageStubsMustCarryDisabled() throws Exception { + // A stub annotated @ConformanceCoverage(level = NONE) is honest only while @Disabled + // keeps it from running: drop the annotation without writing a body and the extension + // records status "passed" for a case with zero coverage — a green report entry that + // asserts nothing. This converts that coupling from a convention into a failure, using + // the same scan the alignment check trusts. + SuiteScan scan = scanConformanceSuites(); + List vacuous = new ArrayList<>(); + for (Class suite : scan.suiteClasses()) { + for (Method method : suite.getDeclaredMethods()) { + ConformanceCoverage coverage = method.getAnnotation(ConformanceCoverage.class); + if (coverage != null + && coverage.level() == ConformanceCoverageLevel.NONE + && !method.isAnnotationPresent(Disabled.class)) { + vacuous.add(suite.getSimpleName() + "#" + method.getName()); + } + } + } + assertThat(vacuous) + .withFailMessage( + "%d test(s) declare @ConformanceCoverage(level = NONE) without @Disabled." + + " Running such a stub reports the case as passed with zero" + + " coverage. Either implement the case (and raise the coverage" + + " level) or keep @Disabled attached:%n - %s", + vacuous.size(), String.join(NL + " - ", vacuous)) + .isEmpty(); + } + /** Case ids declared across the suite, plus whatever the scan could not read. */ - private record SuiteScan(TreeSet caseIds, List loadFailures) {} + private record SuiteScan( + TreeSet caseIds, List loadFailures, List> suiteClasses) {} /** * Collects every {@link ConformanceCase} case id declared by a {@link ConformanceSuite} test @@ -114,6 +145,7 @@ private record SuiteScan(TreeSet caseIds, List loadFailures) {} private static SuiteScan scanConformanceSuites() throws Exception { TreeSet ids = new TreeSet<>(); List loadFailures = new ArrayList<>(); + List> suiteClasses = new ArrayList<>(); String packageName = ConformanceCatalogTest.class.getPackageName(); String packagePath = packageName.replace('.', '/'); ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); @@ -145,6 +177,7 @@ private static SuiteScan scanConformanceSuites() throws Exception { if (!clazz.isAnnotationPresent(ConformanceSuite.class)) { continue; } + suiteClasses.add(clazz); for (Method method : clazz.getDeclaredMethods()) { ConformanceCase mapping = method.getAnnotation(ConformanceCase.class); if (mapping != null) { @@ -153,7 +186,7 @@ private static SuiteScan scanConformanceSuites() throws Exception { } } } - return new SuiteScan(ids, loadFailures); + return new SuiteScan(ids, loadFailures, suiteClasses); } /** diff --git a/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceCoverageLevel.java b/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceCoverageLevel.java index 850dd29..820dd5e 100644 --- a/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceCoverageLevel.java +++ b/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceCoverageLevel.java @@ -2,7 +2,13 @@ public enum ConformanceCoverageLevel { FULL("full"), - PARTIAL("partial"); + PARTIAL("partial"), + /** + * No part of the case is exercised. Distinct from {@link #PARTIAL}: the catalog's report + * contract uses the level to tell a consciously deferred case apart from one that is partly + * covered, so a case whose behaviour is absent altogether must not report as partial. + */ + NONE("none"); private final String wireValue; diff --git a/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceRunStateTest.java b/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceRunStateTest.java index c0bb669..1847584 100644 --- a/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceRunStateTest.java +++ b/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceRunStateTest.java @@ -24,6 +24,7 @@ void close_writesJsonAndMarkdownReports() throws Exception { cases: - id: "case-a" - id: "case-b" + - id: "case-c" """); ConformanceRunState state = @@ -48,6 +49,12 @@ void close_writesJsonAndMarkdownReports() throws Exception { ConformanceStatus.PASSED, null, annotatedCoverage()); + state.recordMapped( + "case-c", + "ai.authplane.sdk.core.conformance.ExampleConformanceTest#caseC", + ConformanceStatus.SKIPPED, + null, + noneCoverage()); state.recordUncatalogued( "ai.authplane.sdk.core.conformance.HarnessSmokeTest#helper", ConformanceStatus.PASSED, @@ -64,6 +71,11 @@ void close_writesJsonAndMarkdownReports() throws Exception { assertThat(json).contains("\"case_id\":\"case-b\""); assertThat(json).contains("\"status\":\"passed\""); assertThat(json).contains("\"coverage\":{\"level\":\"partial\""); + // The NONE constant's wire value, asserted end-to-end like partial's: a consciously + // deferred case must reach the report as "none", never as an absent or partial level. + assertThat(json).contains("\"case_id\":\"case-c\""); + assertThat(json).contains("\"status\":\"skipped\""); + assertThat(json).contains("\"coverage\":{\"level\":\"none\""); assertThat(json).contains("\"gaps\":[\"expected.error_hint\"]"); assertThat(json) .contains( @@ -74,6 +86,7 @@ void close_writesJsonAndMarkdownReports() throws Exception { assertThat(markdown).contains("`failed`"); assertThat(markdown).contains("`case-b`"); assertThat(markdown).contains("`partial`"); + assertThat(markdown).contains("`none`"); assertThat(markdown).contains("## Coverage Notes"); assertThat(markdown).contains("## Uncatalogued Test Details"); } @@ -90,4 +103,15 @@ private static ConformanceCoverage annotatedCoverage() throws NoSuchMethodExcept .getDeclaredMethod("coverageFixture") .getAnnotation(ConformanceCoverage.class); } + + @ConformanceCoverage( + level = ConformanceCoverageLevel.NONE, + note = "Deferred: the gate this case requires is not implemented yet.") + private static void noneCoverageFixture() {} + + private static ConformanceCoverage noneCoverage() throws NoSuchMethodException { + return ConformanceRunStateTest.class + .getDeclaredMethod("noneCoverageFixture") + .getAnnotation(ConformanceCoverage.class); + } } diff --git a/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceTestSupport.java b/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceTestSupport.java index 821a1b5..fe4290b 100644 --- a/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceTestSupport.java +++ b/core/src/conformance/java/ai/authplane/sdk/core/conformance/ConformanceTestSupport.java @@ -4,7 +4,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.get; import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; -import java.lang.reflect.Method; +import java.time.Clock; import java.util.List; import java.util.Map; import java.util.concurrent.ExecutionException; @@ -98,9 +98,14 @@ static Throwable unwrapExecutionException(Throwable throwable) { return cursor; } - static void forceMetadataRefresh(AuthplaneClient client) throws Exception { - Method method = AuthplaneClient.class.getDeclaredMethod("forceMetadataRefreshForTest"); - method.setAccessible(true); - method.invoke(client); + /** + * Builds a client whose caches read time from {@code clock}, so a refresh interval can be + * crossed by advancing the clock. The client is otherwise ordinary: the suite reaches + * refresh-driven behaviour through normal verification calls, never through a test-only + * trigger. + */ + static AuthplaneClient buildClient(String issuer, Clock clock, int metadataRefreshSeconds) + throws Exception { + return TestFixtures.clientWithClock(issuer, clock, metadataRefreshSeconds); } } diff --git a/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc8414ConformanceTest.java b/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc8414ConformanceTest.java index 832c200..7b04455 100644 --- a/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc8414ConformanceTest.java +++ b/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc8414ConformanceTest.java @@ -2,6 +2,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; import static com.github.tomakehurst.wiremock.client.WireMock.get; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -28,6 +29,8 @@ @ConformanceSuite class Rfc8414ConformanceTest extends AbstractPlaceholderConformanceTest { + private static final String WELL_KNOWN_PATH = "/.well-known/oauth-authorization-server"; + private static WireMockServer wireMock; private static String baseUrl; private static TestFixtures.RSAKeyPair rsaKeys; @@ -116,9 +119,9 @@ void rfc8414_discovery_url_must_insert_well_known_before_issuer_path() throws Ex baseUrl + "/jwks"))))); ConformanceTestSupport.stubJwks(wireMock, "/jwks", rsaKeys); - AuthplaneClient client = ConformanceTestSupport.buildClient(issuerWithPath); - assertThat(client.issuer()).isEqualTo(issuerWithPath); - client.close(); + try (AuthplaneClient client = ConformanceTestSupport.buildClient(issuerWithPath)) { + assertThat(client.issuer()).isEqualTo(issuerWithPath); + } } @Test @@ -237,42 +240,84 @@ void rfc8414_revocation_endpoint_required_when_revocation_is_used() { .hasMessageContaining("revocation_endpoint"); } + /** + * Rotation must be reached by ordinary verification traffic and nothing else. + * + *

A resource server that only verifies tokens never calls the token, introspection or + * revocation endpoints, so verification is the only thing on its request path that can notice + * the authorization server has moved its key set. This case therefore drives the rotation the + * way a deployment would: configured refresh interval, ordinary {@code verify()} calls, and an + * injected clock advanced past the interval instead of a sleep. No test-only refresh trigger is + * used — if the SDK reads the metadata document once at start-up and never again, this fails. + * + *

Both key pairs publish the same {@code kid}, which is what makes the assertion sharp: a + * cache still bound to the withdrawn {@code jwks_uri} would find that {@code kid} and reject + * the token on the signature, so a successful verification can only mean key retrieval actually + * moved to the new URI. + */ @Test @ConformanceCase("rfc8414-jwks-uri-rotation-must-reconfigure-jwks-cache") - void rfc8414_jwks_uri_rotation_must_reconfigure_jwks_cache() { + void rfc8414_jwks_uri_rotation_must_reconfigure_jwks_cache() throws Exception { + int metadataRefreshSeconds = 60; TestFixtures.RSAKeyPair rotatedKeys = TestFixtures.generateRsaKeyPair(); + ConformanceTestSupport.stubMetadata( wireMock, Map.of("issuer", baseUrl, "jwks_uri", baseUrl + "/jwks-1")); ConformanceTestSupport.stubJwks(wireMock, "/jwks-1", rsaKeys); ConformanceTestSupport.stubJwks(wireMock, "/jwks-2", rotatedKeys); - AuthplaneClient client = - assertDoesNotThrow(() -> ConformanceTestSupport.buildClient(baseUrl)); - AuthplaneResource verifier = - ConformanceTestSupport.buildVerifier( - client, TestFixtures.RESOURCE, List.of("read:data")); - - VerifiedClaims initialClaims = - assertDoesNotThrow( - () -> - verifier.verify(ConformanceTestSupport.validToken(rsaKeys, baseUrl)) - .get() - .claims()); - assertThat(initialClaims.kid()).isEqualTo(TestFixtures.KID); - - ConformanceTestSupport.stubMetadata( - wireMock, Map.of("issuer", baseUrl, "jwks_uri", baseUrl + "/jwks-2")); - - assertDoesNotThrow(() -> ConformanceTestSupport.forceMetadataRefresh(client)); + TestFixtures.AdvanceableClock clock = new TestFixtures.AdvanceableClock(); + try (AuthplaneClient client = + ConformanceTestSupport.buildClient(baseUrl, clock, metadataRefreshSeconds)) { + AuthplaneResource verifier = + ConformanceTestSupport.buildVerifier( + client, TestFixtures.RESOURCE, List.of("read:data")); + + VerifiedClaims initialClaims = + verifier.verify(ConformanceTestSupport.validToken(rsaKeys, baseUrl)) + .get() + .claims(); + assertThat(initialClaims.kid()).isEqualTo(TestFixtures.KID); + assertThat(requestCount("/jwks-1")).as("the original key set was fetched").isPositive(); + + // The AS rotates: the key set moves to /jwks-2 and the old URI is withdrawn. Nothing + // notifies the SDK — the new document is only visible to a client that re-reads + // metadata. + ConformanceTestSupport.stubMetadata( + wireMock, Map.of("issuer", baseUrl, "jwks_uri", baseUrl + "/jwks-2")); + wireMock.stubFor(get(urlEqualTo("/jwks-1")).willReturn(aResponse().withStatus(404))); + + // Still inside the refresh interval: the configured interval is respected, so + // verification keeps using the document it already holds rather than re-reading on + // every request. + int metadataReadsBefore = requestCount(WELL_KNOWN_PATH); + verifier.verify(ConformanceTestSupport.validToken(rsaKeys, baseUrl)).get(); + assertThat(requestCount(WELL_KNOWN_PATH)) + .as("metadata must not be re-read before the interval elapses") + .isEqualTo(metadataReadsBefore); + + clock.advanceSeconds(metadataRefreshSeconds + 1); + + // One ordinary verification past the interval is all it takes: the metadata read that + // verification performs picks up the new jwks_uri and rebinds key retrieval to it, and + // the token signed by the key published only at the new URI verifies. + VerifiedClaims rotatedClaims = + verifier.verify(ConformanceTestSupport.validToken(rotatedKeys, baseUrl)) + .get() + .claims(); + assertThat(rotatedClaims.kid()).isEqualTo(TestFixtures.KID); + assertThat(requestCount("/jwks-2")).as("the rotated key set was fetched").isPositive(); + + // The withdrawn URI is out of the picture: later verifications must not go back to it. + int withdrawnUriReads = requestCount("/jwks-1"); + verifier.verify(ConformanceTestSupport.validToken(rotatedKeys, baseUrl)).get(); + assertThat(requestCount("/jwks-1")) + .as("the withdrawn jwks_uri must not be fetched again after the rebind") + .isEqualTo(withdrawnUriReads); + } + } - VerifiedClaims rotatedClaims = - assertDoesNotThrow( - () -> - verifier.verify( - ConformanceTestSupport.validToken( - rotatedKeys, baseUrl)) - .get() - .claims()); - assertThat(rotatedClaims.kid()).isEqualTo(TestFixtures.KID); + private static int requestCount(String path) { + return wireMock.countRequestsMatching(getRequestedFor(urlEqualTo(path)).build()).getCount(); } } diff --git a/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc8707ConformanceTest.java b/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc8707ConformanceTest.java index 4b09414..1431348 100644 --- a/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc8707ConformanceTest.java +++ b/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc8707ConformanceTest.java @@ -7,6 +7,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import java.util.List; @@ -28,6 +29,7 @@ import ai.authplane.sdk.core.fetching.FetchSettings; import ai.authplane.sdk.core.fetching.HttpTransport; import ai.authplane.sdk.core.oauth.ClientCredentialsGrant; +import ai.authplane.sdk.core.prm.ProtectedResourceMetadata; @ConformanceSuite class Rfc8707ConformanceTest extends AbstractPlaceholderConformanceTest { @@ -139,4 +141,47 @@ void rfc8707_client_credentials_multiple_resource_parameters_must_be_emitted() { .withRequestBody(containing("resource=https%3A%2F%2Fapi-one.example.com")) .withRequestBody(containing("resource=https%3A%2F%2Fapi-two.example.com"))); } + + @Test + @ConformanceCase("rfc8707-resource-indicator-must-not-contain-a-fragment") + void rfc8707_resource_indicator_must_not_contain_a_fragment() { + ConformanceTestSupport.stubMetadata( + wireMock, Map.of("issuer", baseUrl, "jwks_uri", baseUrl + "/jwks")); + AuthplaneClient client = + assertDoesNotThrow(() -> ConformanceTestSupport.buildClient(baseUrl)); + + // The case is satisfied only by a rejection observable from the construction call itself + // (RFC 8707 §2, RFC 9728 §1.2). Asserted on the operator-facing factory, which is the + // `resource.create` stimulus, so a gate that moved to the derivation helpers would fail + // here rather than pass on a fragment silently dropped at prmUrl() time. + assertThatThrownBy( + () -> + ConformanceTestSupport.buildVerifier( + client, + "https://api.example.com/mcp#section", + List.of("read:data"))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a fragment component"); + + // Same gate on the PRM builder: an identifier reaching the document through the builder + // rather than through a resource must not get past construction either. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.builder() + .resource("https://api.example.com/mcp#section") + .authorizationServer("https://auth.example.com") + .build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a fragment component"); + + // A fragment-free identifier is unaffected — the gate rejects the fragment, it does not + // narrow what a resource identifier may otherwise be. + assertThatCode( + () -> + ConformanceTestSupport.buildVerifier( + client, + "https://api.example.com/mcp", + List.of("read:data"))) + .doesNotThrowAnyException(); + } } diff --git a/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc9728ConformanceTest.java b/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc9728ConformanceTest.java index 6ed3626..9a11886 100644 --- a/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc9728ConformanceTest.java +++ b/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc9728ConformanceTest.java @@ -1,6 +1,7 @@ package ai.authplane.sdk.core.conformance; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import java.net.URI; @@ -157,4 +158,73 @@ void rfc9728_well_known_path_must_derive_from_resource_uri() { URI.create("https://api.example.com/mcp/"))) .isEqualTo("/.well-known/oauth-protected-resource/mcp"); } + + @Test + @ConformanceCase("rfc9728-well-known-url-must-preserve-the-resource-query-component") + void rfc9728_well_known_url_must_preserve_the_resource_query_component() { + // RFC 9728 §3 inserts the well-known string "between the host component and the path + // and/or query components", so the query survives the derivation. The stimulus is the + // full URL rather than the path, because a path-only accessor cannot express a query. + assertThat(ProtectedResourceMetadata.wellKnownUrl("https://api.example.com/mcp?tenant=a")) + .isEqualTo( + "https://api.example.com/.well-known/oauth-protected-resource/mcp?tenant=a"); + + assertThat(ProtectedResourceMetadata.wellKnownUrl("https://api.example.com/mcp?tenant=b")) + .isEqualTo( + "https://api.example.com/.well-known/oauth-protected-resource/mcp?tenant=b"); + + // No path and no terminating slash: §3.1 has no slash to remove, so the suffix goes + // directly after the host and the query follows it. + assertThat(ProtectedResourceMetadata.wellKnownUrl("https://api.example.com?x=1")) + .isEqualTo("https://api.example.com/.well-known/oauth-protected-resource?x=1"); + + // The point of the case: two identifiers differing only by query must not collapse onto + // one metadata document URL, which is what makes every tenant on a host distinct. + assertThat(ProtectedResourceMetadata.wellKnownUrl("https://api.example.com/mcp?tenant=a")) + .isNotEqualTo( + ProtectedResourceMetadata.wellKnownUrl( + "https://api.example.com/mcp?tenant=b")); + } + + @Test + @ConformanceCase("rfc9728-resource-identifier-must-be-an-absolute-url-with-scheme-and-host") + @ConformanceCoverage( + level = ConformanceCoverageLevel.PARTIAL, + gaps = { + "the host half is not gated at construction: \"https:example.com/mcp\" carries a" + + " scheme and no authority, and is refused only at derivation" + }, + note = + "Both values the case exercises are now rejected from the resource factory and" + + " from the PRM builder, by requireScheme. PARTIAL rather than FULL" + + " because the requirement is scheme *and* host and only the scheme" + + " half is enforced where the stimulus points: an identifier with a" + + " scheme but no authority still constructs and throws later, on the" + + " 401 challenge path, which is the shape of failure moving these" + + " gates to construction was meant to remove.") + void rfc9728_resource_identifier_must_be_an_absolute_url_with_scheme_and_host() { + // Each value rejects on its own — the case is explicit that rejecting one does not satisfy + // it, because a guard that only asks "opaque or authority-less?" catches "/mcp" while + // letting the scheme-relative form through. + for (String identifier : List.of("/mcp", "//api.example.com/mcp")) { + assertThatThrownBy(() -> ProtectedResourceMetadata.requireScheme(identifier)) + .isInstanceOf(IllegalArgumentException.class); + + assertThatThrownBy( + () -> + ProtectedResourceMetadata.builder() + .resource(identifier) + .authorizationServer(TestFixtures.ISSUER) + .build()) + .isInstanceOf(IllegalArgumentException.class); + } + + // Scheme-and-host, not https-only: local development loops depend on this one constructing. + assertDoesNotThrow( + () -> + ProtectedResourceMetadata.builder() + .resource("http://localhost:8080/mcp") + .authorizationServer(TestFixtures.ISSUER) + .build()); + } } diff --git a/core/src/main/java/ai/authplane/sdk/core/AuthplaneClient.java b/core/src/main/java/ai/authplane/sdk/core/AuthplaneClient.java index 6672425..7fbd8d9 100644 --- a/core/src/main/java/ai/authplane/sdk/core/AuthplaneClient.java +++ b/core/src/main/java/ai/authplane/sdk/core/AuthplaneClient.java @@ -1,5 +1,6 @@ package ai.authplane.sdk.core; +import java.time.Clock; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -9,11 +10,14 @@ import java.util.concurrent.CompletionException; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executor; +import java.util.concurrent.locks.ReentrantLock; +import java.util.logging.Level; import java.util.logging.Logger; import ai.authplane.sdk.core.dpop.DPoPProvider; import ai.authplane.sdk.core.dpop.OutboundDPoPOptions; import ai.authplane.sdk.core.errors.TokenExchangeException; +import ai.authplane.sdk.core.fetching.DocumentCache; import ai.authplane.sdk.core.fetching.DocumentFetcher; import ai.authplane.sdk.core.fetching.HttpTransport; import ai.authplane.sdk.core.fetching.JwksCache; @@ -23,6 +27,7 @@ import ai.authplane.sdk.core.oauth.IntrospectionResponse; import ai.authplane.sdk.core.oauth.Revocation; import ai.authplane.sdk.core.oauth.TokenExchange; +import ai.authplane.sdk.core.prm.ProtectedResourceMetadata; /** * Central owner of Authorization Server connection state and token operations. @@ -52,6 +57,9 @@ @SuppressWarnings("checkstyle:FinalClass") public class AuthplaneClient implements AutoCloseable { + /** Depth bound for the cause walk in {@link #isInterrupt}; see the comment there. */ + private static final int MAX_CAUSE_HOPS = 16; + private static final Logger LOG = Logger.getLogger(AuthplaneClient.class.getName()); /** Algorithms that must never be allowed. */ @@ -65,6 +73,45 @@ public class AuthplaneClient implements AutoCloseable { // Infrastructure volatile JwksCache jwksCache; final MetadataCache metadataCache; // null if metadata not available + + /** Installed by the builder right after construction; null when there is no metadata cache. */ + volatile JwksCacheFactory jwksCacheFactory; + + private final ReentrantLock jwksRebindLock = new ReentrantLock(); + + /** + * Suppresses rebind attempts after one fails, on the same policy the caches use. + * + *

The factory builds a fresh {@link JwksCache} per attempt, so the backoff a cache keeps for + * itself starts from zero every time and cannot govern this. Without a backoff here, a rotated + * {@code jwks_uri} that is down costs a full HTTP timeout on the verification path for as long + * as the outage lasts — reconciling means the mismatch is re-detected on every key lookup, so + * every lookup pays. Tokens whose keys are already cached do not need that fetch to succeed; + * they only need it not to block them. + * + *

Volatile rather than lock-guarded: the fast path reads it before taking {@link + * #jwksRebindLock}, and a read that races a write costs at most one extra attempt. + */ + private volatile long jwksRebindRetryNotBeforeEpochSeconds; + + /** + * Time source for the rebind backoff. Replaced by the builder so tests can advance it. + * + *

{@code volatile} for the same reason {@code jwksCacheFactory} is: it is written after the + * constructor returns, so it carries none of the JMM final-field guarantees the other infra + * fields on this class get. A client published through a data race could otherwise hand a + * request thread {@code clock == null}, which NPEs in {@link #rebindJwksIfMoved}. + */ + private volatile Clock clock = Clock.systemUTC(); + + /** + * Set by {@link AuthplaneClientBuilder} after construction, alongside {@code jwksCacheFactory}, + * rather than as a thirteenth constructor parameter. + */ + void setClock(Clock clock) { + this.clock = clock; + } + final HttpTransport transport; final AuthProvider authProvider; // nullable final DocumentFetcher fetcher; @@ -142,6 +189,18 @@ public AuthplaneResource resource( Objects.requireNonNull(options, "options must not be null"); if (resourceUri.isBlank()) throw new IllegalArgumentException("resourceUri must not be blank"); + // RFC 8707 §2 / RFC 9728 §1.2: no fragment component. Redundant with the gate in the + // AuthplaneResource constructor, kept so the stack trace points at the caller's line. + ProtectedResourceMetadata.requireNoFragment(resourceUri); + // RFC 3986 §3.4: the query is now part of the identifier and is spliced into the + // WWW-Authenticate challenge, so an octet outside the query production must not get + // past construction. Same reason, same boundary. + ProtectedResourceMetadata.requireValidQuery(resourceUri); + // RFC 8707 §2: an absolute URI always carries a scheme. Same reason, same boundary. + ProtectedResourceMetadata.requireScheme(resourceUri); + // RFC 9110 §4.2.4: no userinfo. The identifier is published to unauthenticated callers + // verbatim, so a credential in the authority is disclosed. Same reason, same boundary. + ProtectedResourceMetadata.requireNoUserinfo(resourceUri); // Validate algorithms Set dangerous = new HashSet<>(options.allowedAlgorithms()); @@ -412,12 +471,161 @@ public void close() { // ----------------------------------------------------------------------- /** - * Forces a synchronous metadata refresh, triggering the jwks_uri rotation callback if the - * metadata document has changed. Package-private — for use in tests only. + * Reads through the AS metadata cache and reconciles {@link #jwksCache} against the {@code + * jwks_uri} it advertises. This is what makes {@code metadataRefreshSeconds} effective 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 would otherwise touch the metadata document after start-up: the cache would + * hold the copy fetched at build time forever, and a rotated {@code jwks_uri} would never be + * followed. Verification calls this before every key lookup. The read is cheap while the + * document is fresh; once the interval has elapsed the cache re-fetches, and a rotation takes + * effect on the very lookup that discovered it. + * + *

The comparison is against the URI the cache is currently bound to, not against a change in + * the document, and that difference is the whole point. {@code DocumentCache} publishes a + * refreshed document before it notifies its change listener, so an edge-triggered rebind that + * failed — one 503 at the new URI — would leave key retrieval pinned to the withdrawn one with + * nothing left to re-trigger it: every later refresh returns that same document, so the edge + * never fires again. Comparing desired state to actual state instead means a failed rebind is + * simply retried on the next lookup. + * + *

Failures are swallowed deliberately. A metadata endpoint that is briefly unreachable must + * not fail verification of tokens whose signing keys the JWKS cache already holds; the cache + * falls back to the last good document, so this only logs when there is nothing to fall back + * on. + */ + void refreshMetadataIfDue() { + if (metadataCache == null) { + return; + } + String discoveredJwksUri; + try { + discoveredJwksUri = metadataCache.getJwksUri(); + } catch (InterruptedException e) { + // Shutdown, not a metadata problem. The flag is restored by DocumentCache; re-raising + // it here and returning keeps a stack trace out of the log on the way down. + Thread.currentThread().interrupt(); + return; + } catch (Exception e) { + // The interrupt does not reach the branch above from this call site: MetadataCache + // wraps everything that is not a MetadataFetchException, so it arrives here wrapped. + // The flag is still restored upstream — only the logging would be wrong, and a stack + // trace on the way down is exactly what that branch exists to avoid. The sibling catch + // in rebindJwksIfMoved does see it unwrapped, since DocumentCache.fetch rethrows. + if (isInterrupt(e)) { + Thread.currentThread().interrupt(); + return; + } + LOG.log( + Level.WARNING, + "AS metadata refresh failed; continuing with the current JWKS binding", + e); + return; + } + rebindJwksIfMoved(discoveredJwksUri); + } + + /** + * Whether a failure is an interrupt, however deeply it was wrapped on the way here. + * + *

Checking the thread's own flag would answer a different question: it stays set from an + * interrupt this call had nothing to do with, and would then silence a real metadata failure. + */ + // Package-private rather than private: the wrapped/unwrapped asymmetry the two call sites + // rely on, and the cycle bound below, are both worth pinning directly. + static boolean isInterrupt(Throwable error) { + // Bounded rather than walked to the end. `initCause` refuses a self-reference, so the + // `t.getCause() == t` guard alone looks sufficient — but it does not stop a cycle built + // through the `Throwable(String, Throwable)` constructors, where A causes B causes A. That + // walk never terminates. No real chain approaches this depth. + int hops = 0; + for (Throwable t = error; t != null && hops < MAX_CAUSE_HOPS; t = t.getCause(), hops++) { + if (t instanceof InterruptedException) { + return true; + } + if (t.getCause() == t) { + break; + } + } + return false; + } + + /** + * Rebinds {@link #jwksCache} when the metadata document points key retrieval somewhere else. + * No-op when the two already agree, which is every call but the one that follows a rotation. + */ + private void rebindJwksIfMoved(String discoveredJwksUri) { + if (jwksCacheFactory == null || discoveredJwksUri.equals(jwksCache.getUrl())) { + return; + } + long now = clock.instant().getEpochSecond(); + if (now < jwksRebindRetryNotBeforeEpochSeconds) { + LOG.fine( + () -> + "jwks_uri rebind backing off after a failed attempt (retry in " + + (jwksRebindRetryNotBeforeEpochSeconds - now) + + "s); keeping the current binding"); + return; + } + // One rebind at a time. A caller that loses the race keeps the current binding for this + // lookup rather than queueing behind a JWKS fetch; the winner publishes for everyone, and + // a kid miss forces a refresh anyway. + if (!jwksRebindLock.tryLock()) { + return; + } + try { + String boundUri = jwksCache.getUrl(); + if (discoveredJwksUri.equals(boundUri)) { + return; // another thread got there first + } + LOG.warning( + "jwks_uri changed from '" + + boundUri + + "' to '" + + discoveredJwksUri + + "', restarting JWKS cache"); + jwksCache = jwksCacheFactory.create(discoveredJwksUri); + jwksRebindRetryNotBeforeEpochSeconds = 0; + LOG.info(() -> "JWKS cache restarted with new URI: " + discoveredJwksUri); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } catch (Exception e) { + long backoff = DocumentCache.failureBackoffSeconds(jwksRefreshSeconds); + jwksRebindRetryNotBeforeEpochSeconds = clock.instant().getEpochSecond() + backoff; + LOG.log( + Level.WARNING, + "Failed to initialise new JWKS cache for URI: " + + discoveredJwksUri + + ". Keeping the existing cache; retrying in " + + backoff + + "s.", + e); + } finally { + jwksRebindLock.unlock(); + } + } + + /** + * Builds a JWKS cache bound to a newly discovered {@code jwks_uri}, already populated. Supplied + * by {@link AuthplaneClientBuilder}, which owns the fetcher, the refresh interval and the time + * source a new cache needs. + */ + @FunctionalInterface + interface JwksCacheFactory { + JwksCache create(String jwksUri) throws Exception; + } + + /** + * Forces a synchronous metadata refresh, bypassing the configured interval. The JWKS binding is + * not touched here — it is reconciled by the next {@link #refreshMetadataIfDue()}, which is + * what every key lookup calls. Package-private — for use in tests only. */ void forceMetadataRefreshForTest() throws Exception { if (metadataCache != null) { - metadataCache.forceRefresh(); + // Bypasses the failure backoff: a test asking for a refresh wants the attempt made, not + // the cached copy handed back. The request-path callers deliberately do not. + metadataCache.forceRefreshIgnoringFailureBackoff(); } } diff --git a/core/src/main/java/ai/authplane/sdk/core/AuthplaneClientBuilder.java b/core/src/main/java/ai/authplane/sdk/core/AuthplaneClientBuilder.java index 2bd662c..593edd8 100644 --- a/core/src/main/java/ai/authplane/sdk/core/AuthplaneClientBuilder.java +++ b/core/src/main/java/ai/authplane/sdk/core/AuthplaneClientBuilder.java @@ -1,11 +1,11 @@ package ai.authplane.sdk.core; +import java.time.Clock; import java.util.Objects; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.Executor; import java.util.concurrent.ForkJoinPool; -import java.util.logging.Level; import java.util.logging.Logger; import ai.authplane.sdk.core.dpop.OutboundDPoPOptions; @@ -44,6 +44,7 @@ public final class AuthplaneClientBuilder { private TokenCacheConfig tokenCacheConfig = TokenCacheConfig.defaults(); private OutboundDPoPOptions outboundDPoP = null; private Executor executor = null; + private Clock clock = Clock.systemUTC(); AuthplaneClientBuilder(String issuer) { Objects.requireNonNull(issuer, "issuer must not be null"); @@ -77,6 +78,19 @@ public AuthplaneClientBuilder metadataRefreshSeconds(int seconds) { return this; } + /** + * Sets the time source the metadata and JWKS caches use to evaluate their TTLs. + * + *

Package-private: production callers have no reason to run the caches on anything but the + * system clock. It exists so tests can drive refresh intervals by advancing a clock rather than + * sleeping against wall time, which is the only way to assert refresh behaviour without a + * shortened interval racing the CI runner. + */ + AuthplaneClientBuilder clock(Clock clock) { + this.clock = Objects.requireNonNull(clock, "clock must not be null"); + return this; + } + /** * Sets the {@link AuthProvider} for Authorization Server calls (token, introspection, * revocation). Pass static client credentials as {@code new ASCredentials(clientId, @@ -190,13 +204,15 @@ private AuthplaneClient buildSync( metadataRefreshSeconds, issuer, effectiveFetchSettings.allowHttp(), - null); + null, + clock); metadataCache.fetch(); String resolvedJwksUri = metadataCache.getJwksUri(); LOG.info(() -> "Discovered JWKS URI: " + resolvedJwksUri); - JwksCache jwksCache = new JwksCache(fetcher, resolvedJwksUri, jwksRefreshSeconds, null); + JwksCache jwksCache = + new JwksCache(fetcher, resolvedJwksUri, jwksRefreshSeconds, null, clock); jwksCache.fetch(); CircuitBreaker circuitBreaker = @@ -220,15 +236,25 @@ private AuthplaneClient buildSync( outboundDPoP, effectiveExecutor); - wireMetadataCallback(client, metadataCache, fetcher); + client.setClock(clock); + client.jwksCacheFactory = + jwksUri -> { + JwksCache newCache = + new JwksCache(fetcher, jwksUri, jwksRefreshSeconds, null, clock); + newCache.fetch(); + return newCache; + }; + wireMetadataCallback(metadataCache); return client; } - private void wireMetadataCallback( - AuthplaneClient client, MetadataCache metadataCache, DocumentFetcher fetcher) { - // Safe from concurrent races: the MetadataCache invokes this callback - // inside its fetchLock, so jwks_uri rotation is serialized even if - // multiple background refreshes overlap. + /** + * Logs the AS endpoints a refresh moved. The {@code jwks_uri} is deliberately not handled here: + * a change callback is edge-triggered, and {@code DocumentCache} publishes the new document + * before it fires, so a rebind that failed could never be retried. {@link + * AuthplaneClient#refreshMetadataIfDue()} reconciles that binding against the document instead. + */ + private void wireMetadataCallback(MetadataCache metadataCache) { metadataCache.setOnChangeCallback( (oldDoc, newDoc) -> { Object newEp = newDoc.get("introspection_endpoint"); @@ -242,31 +268,6 @@ private void wireMetadataCallback( if (!Objects.equals(oldTe, newTe)) { LOG.info(() -> "AS token_endpoint changed to: " + newTe); } - - Object newUriObj = newDoc.get("jwks_uri"); - if (!(newUriObj instanceof String newUri)) return; - if (newUri.equals(client.jwksCache.getUrl())) return; - - LOG.warning( - "jwks_uri changed from '" - + client.jwksCache.getUrl() - + "' to '" - + newUri - + "', restarting JWKS cache"); - - JwksCache newCache = new JwksCache(fetcher, newUri, jwksRefreshSeconds, null); - try { - newCache.fetch(); - client.jwksCache = newCache; - LOG.info(() -> "JWKS cache restarted with new URI: " + newUri); - } catch (Exception e) { - LOG.log( - Level.WARNING, - "Failed to initialise new JWKS cache for URI: " - + newUri - + ". Keeping existing cache.", - e); - } }); } } diff --git a/core/src/main/java/ai/authplane/sdk/core/AuthplaneResource.java b/core/src/main/java/ai/authplane/sdk/core/AuthplaneResource.java index 119c507..51ef6ba 100644 --- a/core/src/main/java/ai/authplane/sdk/core/AuthplaneResource.java +++ b/core/src/main/java/ai/authplane/sdk/core/AuthplaneResource.java @@ -5,6 +5,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; @@ -64,6 +65,18 @@ public class AuthplaneResource { String resourceUri, List scopes, ResourceOptions options) { + // Authoritative fragment gate: every AuthplaneResource is built here, so an identifier + // carrying a fragment cannot reach prmResponse(), which publishes it verbatim. + ProtectedResourceMetadata.requireNoFragment(resourceUri); + ProtectedResourceMetadata.requireValidQuery(resourceUri); + // Authoritative scheme gate: the scheme feeds more than the PRM derivation — it is + // spliced into the DPoP htu binding target in normalizeRequestUrl below, where a missing + // one reads as the literal text "null" and fails every DPoP-bound request. + ProtectedResourceMetadata.requireScheme(resourceUri); + // Authoritative userinfo gate: the identifier is published verbatim as the PRM `resource` + // member and in the resource_metadata parameter of the 401 challenge, both of which reach + // unauthenticated callers, so a credential in the authority must not get this far. + ProtectedResourceMetadata.requireNoUserinfo(resourceUri); this.client = client; this.resourceUri = resourceUri; this.scopes = List.copyOf(scopes); @@ -80,14 +93,29 @@ public class AuthplaneResource { this.failClosed = options.failClosed(); this.inboundDPoP = options.inboundDPoP(); - // KeyLookup reads through the client's JWKS cache + // KeyLookup reads through the client's JWKS cache, after giving the metadata cache the + // chance to re-read: verification is the only traffic a verify-only resource server has, + // so this is what keeps metadataRefreshSeconds honoured and follows a rotated jwks_uri. + // The rebind happens before the volatile jwksCache field is read below, so a rotation + // takes effect on the very lookup that discovered it. this.validator = new JwtValidator( client.issuer(), resourceUri, this.allowedAlgorithms, options.clockSkewSeconds(), - (kid, force) -> client.jwksCache.getKeyByKid(kid, force)); + new JwtValidator.KeyLookup() { + @Override + public void beforeLookup() { + client.refreshMetadataIfDue(); + } + + @Override + public Optional> find(String kid, boolean force) + throws Exception { + return client.jwksCache.getKeyByKid(kid, force); + } + }); } // ----------------------------------------------------------------------- @@ -312,6 +340,13 @@ public Map prmResponse() { * Returns the URL path at which this resource's RFC 9728 Protected Resource Metadata document * should be served (e.g. {@code /.well-known/oauth-protected-resource}, or a path-qualified * variant when the resource URI has a path). + * + *

Routing is path-keyed: a query component of the resource URI never appears here — it is + * carried in {@link #prmUrl()}. Identifiers differing only by query therefore share one route + * serving one document. Serving distinct documents per query value is not supported: RFC 9728 + * §3.3 requires a client to discard a response whose {@code resource} member differs from the + * identifier it derived the request from, so any query value the shared document's {@code + * resource} was not built for fails that client-side check. */ public String prmPath() { return ProtectedResourceMetadata.wellKnownPath(URI.create(resourceUri)); @@ -321,6 +356,11 @@ public String prmPath() { * Returns the absolute URL of this resource's RFC 9728 Protected Resource Metadata document, * suitable for the {@code resource_metadata} parameter of a {@code WWW-Authenticate} challenge. * + *

A query component of the resource URI is preserved in the returned URL (RFC 9728 §3 + * inserts the well-known string between the host and "the path and/or query components, if + * any"), so a challenge for {@code https://api.example.com/mcp?tenant=a} advertises {@code + * .../.well-known/oauth-protected-resource/mcp?tenant=a}. + * *

Not header-safe. The value is derived from the operator-configured * resource URI and is returned verbatim — it is NOT escaped for use in an HTTP header. When * interpolating it into a header value, pass it through {@link @@ -362,7 +402,19 @@ public String path() { public String normalizeRequestUrl(String requestUrl) { URI base = URI.create(resourceUri); String path = URI.create(requestUrl).getRawPath(); - return base.getScheme() + "://" + base.getAuthority() + (path == null ? "" : path); + // getRawAuthority(), for the same reason the PRM derivation reads it raw: getAuthority() + // percent-decodes, so an identifier whose authority carries a percent-escape would yield an + // htu naming the decoded form — structurally different from the one the identifier names, + // which the client's proof can never match. + // + // The escape has to be of a *reserved* octet for raw to be the right answer. RFC 3986 + // §6.2.2.2 requires percent-encoded *unreserved* octets to be decoded before comparison, so + // a conformant client normalizes "https://a%2Db.example.com" (%2D is "-") to + // "https://a-b.example.com" and sends that — the raw form would fail to match it. The case + // this preserves is "https://a%3Ab.example.com", where %3A is ":" and decoding it would + // change where the authority ends. Userinfo is rejected at construction now, but the escape + // is not confined to userinfo: a registered name may carry one too (RFC 3986 §3.2.2). + return base.getScheme() + "://" + base.getRawAuthority() + (path == null ? "" : path); } // ----------------------------------------------------------------------- diff --git a/core/src/main/java/ai/authplane/sdk/core/JwtValidator.java b/core/src/main/java/ai/authplane/sdk/core/JwtValidator.java index 4051e37..722636f 100644 --- a/core/src/main/java/ai/authplane/sdk/core/JwtValidator.java +++ b/core/src/main/java/ai/authplane/sdk/core/JwtValidator.java @@ -44,9 +44,20 @@ class JwtValidator { * Abstracts JWK key lookup by kid. Implemented by a lambda in AuthplaneResource that reads the * volatile jwksCache on each invocation. */ + // Still @FunctionalInterface: a functional interface may declare default methods, and the + // annotation is what stops a second *abstract* one being added by accident. JwtValidatorTest + // passes a lambda. @FunctionalInterface interface KeyLookup { Optional> find(String kid, boolean forceRefresh) throws Exception; + + /** + * Called once per verification, before any {@link #find} call. This is where the metadata + * document is re-read and the JWKS binding reconciled against it, so a rotated {@code + * jwks_uri} is in effect for the lookups below. Hoisted out of {@code find} because a kid + * miss calls that twice and the reconcile is not wanted twice. + */ + default void beforeLookup() {} } private final String issuer; @@ -113,7 +124,9 @@ private VerifiedClaims doVerify(String token) throws Exception { String kid = validateHeader(header); String alg = getRequiredStringClaim(header, "alg", true); - // Step 5: JWKS key lookup + // Step 5: JWKS key lookup. Reconcile the binding first, once, so both the cached lookup + // and the forced refresh below run against the currently bound jwks_uri. + keyLookup.beforeLookup(); Optional> keyOpt = keyLookup.find(kid, false); if (keyOpt.isEmpty()) { LOG.info(() -> "kid '" + kid + "' not in JWKS cache, forcing refresh"); diff --git a/core/src/main/java/ai/authplane/sdk/core/fetching/CacheHeaderParser.java b/core/src/main/java/ai/authplane/sdk/core/fetching/CacheHeaderParser.java index e02fee1..e91f093 100644 --- a/core/src/main/java/ai/authplane/sdk/core/fetching/CacheHeaderParser.java +++ b/core/src/main/java/ai/authplane/sdk/core/fetching/CacheHeaderParser.java @@ -9,8 +9,16 @@ /** * Parses RFC 7234 HTTP cache headers to determine a server-suggested expiry time. * - *

The effective cache TTL used by DocumentCache is: effectiveTTL = min(configuredTTL, - * serverExpiry) where serverExpiry is the value returned by this class. + *

A {@code null} return means the server expressed no cacheable preference, and the caller's + * configured interval governs. That is what {@code no-store} and {@code no-cache} return: they say + * the response should not be reused, which for a document this SDK must keep serving is not an + * expiry it can honour — the caller falls back to its own interval rather than treating the + * document as permanently stale. Both siblings model it the same way, as go's zero {@code + * time.Time} and ts's {@code undefined}. + * + *

A non-null return is an absolute expiry, and {@code DocumentCache} shortens its configured TTL + * to it when it is in the future. An expiry already in the past — a stale {@code Expires:}, or + * {@code max-age=0} — is discarded there for the same reason. * *

Thread-safe — all methods are stateless. */ @@ -25,16 +33,24 @@ private CacheHeaderParser() {} * provide cache directives. * * @param headers response headers with lower-cased header names - * @return Unix epoch seconds of expiry, 0 for no-store/no-cache, or null + * @return Unix epoch seconds of expiry, or {@code null} when the server expressed no usable + * preference — no cache headers, an unparseable value, or {@code no-store}/{@code no-cache} */ public static Long parseExpiresAt(Map headers) { String cacheControl = headers.get("cache-control"); if (cacheControl != null) { String cc = cacheControl.toLowerCase(); - // no-store or no-cache → treat as immediately expired + // no-store / no-cache → no usable preference, not "expired now". + // + // These used to return 0L, which DocumentCache read as an absolute expiry at the + // epoch: subtracting the cache timestamp gave a TTL of about -1.7e9, the document was + // expired on every read, and every read paid a synchronous fetch. The cache now + // discards a non-future expiry, which made the 0L sentinel indistinguishable from + // null — it carried no information while the javadoc still claimed it meant + // "immediately expired". Returning null says the thing that is true. if (cc.contains("no-store") || cc.contains("no-cache")) { - return 0L; + return null; } // max-age=N diff --git a/core/src/main/java/ai/authplane/sdk/core/fetching/DocumentCache.java b/core/src/main/java/ai/authplane/sdk/core/fetching/DocumentCache.java index a2f920b..01ced82 100644 --- a/core/src/main/java/ai/authplane/sdk/core/fetching/DocumentCache.java +++ b/core/src/main/java/ai/authplane/sdk/core/fetching/DocumentCache.java @@ -2,6 +2,7 @@ import java.time.Clock; import java.util.Map; +import java.util.Objects; import java.util.concurrent.CompletableFuture; import java.util.concurrent.locks.ReentrantLock; import java.util.function.BiConsumer; @@ -14,9 +15,11 @@ *

Lifecycle: 1. Call fetch() to populate the cache for the first time. 2. Subsequent get() calls * return the cached document. 3. At 80% of effective TTL, a background refresh is fired * asynchronously via the common ForkJoinPool (daemon threads — no explicit shutdown needed). 4. On - * fetch failure, the stale document is returned (if available). + * fetch failure, the stale document is returned (if available) and the next network attempt is + * suppressed for a short backoff. * - *

Thread-safe. The ReentrantLock prevents concurrent fetch storms. + *

Thread-safe. The ReentrantLock prevents concurrent fetch storms, and {@link #get()} never + * waits on it once a document is cached — see that method. */ public class DocumentCache { @@ -25,6 +28,19 @@ public class DocumentCache { /** Background refreshes start at this fraction of the effective TTL. */ private static final double REFRESH_THRESHOLD = 0.80; + /** + * Upper bound on how long a failed refresh suppresses the next network attempt. + * + *

{@link #doFetch} advances {@code cachedAtEpochSeconds} only on success, so against an + * endpoint that is down — or one returning a document {@link #validateFetched} rejects — the + * cached copy stays permanently expired and every {@link #get()} would otherwise take the + * synchronous branch: one full HTTP timeout per call. That is invisible while nothing on a + * request path reads the cache, and is not once something does ({@code + * AuthplaneClient.refreshMetadataIfDue}). Bounding the retry rate keeps a stale-but-serviceable + * document cheap to read. + */ + private static final int FAILURE_BACKOFF_SECONDS = 30; + private final DocumentFetcher fetcher; private final String url; private final int configuredRefreshSeconds; @@ -34,10 +50,14 @@ public class DocumentCache { private final ReentrantLock fetchLock = new ReentrantLock(); + // Written under fetchLock. Volatile so get() can serve it without waiting for a fetch that + // another thread is already performing. + private volatile Map cachedDocument; + // Guarded by fetchLock - private Map cachedDocument; private long cachedAtEpochSeconds; // when the current cache was stored private Long serverExpiresAtSeconds; // from HTTP cache headers, or null + private long retryNotBeforeEpochSeconds; // set after a failed refresh; 0 = no backoff // Written under fetchLock; volatile so the package-private accessor can read it without // taking fetchLock. @@ -87,13 +107,28 @@ public DocumentCache( String documentType, BiConsumer, Map> onChangeCallback, Clock clock) { + // Validated here, not only in AuthplaneClientBuilder. These constructors are public API on + // JwksCache and MetadataCache, so the builder's check does not cover a caller that builds a + // cache directly — and a non-positive interval reaches the same permanent-expiry state the + // server-expiry clamp was added for: effectiveTtlSeconds() returns it verbatim, age >= 0 on + // the first read, and every get() pays a synchronous fetch. + if (configuredRefreshSeconds <= 0) { + throw new IllegalArgumentException( + "refresh interval must be positive, got " + + configuredRefreshSeconds + + ": a non-positive interval leaves the document permanently expired," + + " so every read would pay a synchronous fetch on the caller's" + + " thread."); + } + // Checked at construction rather than surfacing as an NPE from the first get(), the same + // reasoning MetadataCache applies to expectedIssuer. + this.clock = Objects.requireNonNull(clock, "clock must not be null"); this.fetcher = fetcher; this.url = url; this.configuredRefreshSeconds = configuredRefreshSeconds; this.documentType = documentType; this.onChangeCallback = onChangeCallback; - this.clock = clock; } /** Returns the URL this cache fetches from. */ @@ -123,12 +158,25 @@ public void fetch() throws Exception { /** * Returns the cached document, triggering a background refresh if at 80% of TTL. If the * document has fully expired, performs a synchronous refresh. If the refresh fails and a stale - * document exists, returns stale. + * document exists, returns stale and suppresses the next attempt for {@value + * #FAILURE_BACKOFF_SECONDS} seconds (or the configured interval, whichever is shorter). + * + *

Once a document is cached this never blocks on another thread's fetch: if {@code + * fetchLock} is held it returns the copy currently published instead of queueing behind a + * network round trip. Callers on a request path — verification reads through the metadata cache + * before every key lookup — would otherwise serialize on an exclusive lock for the length of an + * HTTP timeout. The uncontended path is unchanged. * * @throws Exception if the document is expired, no stale exists, and fetch fails */ public Map get() throws Exception { - fetchLock.lock(); + Map current = cachedDocument; + if (current == null) { + fetchLock.lock(); + } else if (!fetchLock.tryLock()) { + LOG.fine(() -> documentType + " refresh in flight elsewhere; serving the current copy"); + return current; + } try { if (cachedDocument == null) { // No cache at all — must fetch now @@ -141,7 +189,14 @@ public Map get() throws Exception { long age = now - cachedAtEpochSeconds; double fraction = effectiveTtl > 0 ? (double) age / effectiveTtl : 1.0; - if (age >= effectiveTtl) { + if (now < retryNotBeforeEpochSeconds) { + LOG.fine( + () -> + documentType + + " refresh backing off after a failed attempt (retry in " + + (retryNotBeforeEpochSeconds - now) + + "s); serving the cached copy"); + } else if (age >= effectiveTtl) { // Fully expired — refresh now LOG.fine(() -> documentType + " cache expired, refreshing synchronously"); doFetch(true); // true = allow stale on failure @@ -171,10 +226,65 @@ public Map get() throws Exception { } } - /** Forces a cache refresh regardless of TTL. */ + /** + * Forces a cache refresh regardless of TTL, but not regardless of the failure backoff. + * + *

The backoff applies here for the same reason it applies to {@link #get()}: the caller is + * on a request path. {@code JwksCache.getKeyByKid(kid, true)} is reached on every {@code kid} + * the cached document does not hold, which is exactly the state a rotation to an unreachable + * {@code jwks_uri} leaves the process in — tokens arrive signed with keys the old document does + * not carry. Fetching unconditionally there costs a full HTTP timeout per verification, on the + * caller's thread and serialized behind {@code fetchLock}, which is the failure this cache's + * backoff exists to prevent. It is also an amplification surface: an unauthenticated caller + * presenting tokens with unknown {@code kid} values would drive one fetch per request. + * + *

While backing off, the currently held document is returned. A caller that finds no usable + * key in it fails that verification, which is the correct outcome — the alternative is paying a + * doomed network round trip first. + */ public Map forceRefresh() throws Exception { - fetchLock.lock(); + return doForceRefresh(false); + } + + /** + * Forces a refresh even while backing off from a failed one. Named rather than an overload so + * it cannot be reached by flipping a boolean at a call site: no request path should call this. + * It exists for callers that are asking a question and want the attempt made — a test, or an + * administrative refresh — and are prepared to wait for a timeout. + */ + public Map forceRefreshIgnoringFailureBackoff() throws Exception { + return doForceRefresh(true); + } + + private Map doForceRefresh(boolean ignoreFailureBackoff) throws Exception { + // tryLock, for the same reason {@link #get()} uses it: this is a request-path caller. The + // backoff above keeps a *failing* endpoint from costing a fetch per request, but it does + // nothing for the burst that arrives before the first failure records retryNotBefore — + // those would all queue on an exclusive lock for one HTTP timeout. A caller that finds a + // fetch already in flight is served the document currently held; the two methods now + // agree that no request-path caller blocks on another thread's fetch. + Map inFlight = cachedDocument; + if (inFlight != null && !fetchLock.tryLock()) { + LOG.fine(() -> documentType + " refresh in flight elsewhere; serving the current copy"); + return inFlight; + } + if (inFlight == null) { + fetchLock.lock(); + } try { + long now = nowEpochSeconds(); + if (!ignoreFailureBackoff + && cachedDocument != null + && now < retryNotBeforeEpochSeconds) { + LOG.fine( + () -> + documentType + + " forced refresh backing off after a failed attempt (retry" + + " in " + + (retryNotBeforeEpochSeconds - now) + + "s); serving the cached copy"); + return cachedDocument; + } doFetch(true); return cachedDocument; } finally { @@ -185,15 +295,32 @@ public Map forceRefresh() throws Exception { // ----------------------------------------------------------------------- // Internal + /** + * Validation hook for subclasses, applied to a freshly fetched document before it is published + * to the cache and before the change callback sees it. The default implementation accepts + * everything. + * + *

Rejecting here rather than at read time is what keeps a bad refresh from displacing a good + * document: the previously cached copy stays in place and, where a stale fallback is permitted, + * keeps being served. It also means a listener wired to the change callback — jwks_uri + * rotation, say — is only ever handed a document that passed validation. + * + * @param document the freshly fetched document + * @throws Exception to reject the document + */ + protected void validateFetched(Map document) throws Exception {} + /** Must be called with fetchLock held. */ private void doFetch(boolean allowStaleOnFailure) throws Exception { try { FetchResult result = fetcher.fetch(url).get(); // blocks until done + validateFetched(result.document()); Map oldDoc = cachedDocument; cachedDocument = result.document(); cachedAtEpochSeconds = nowEpochSeconds(); serverExpiresAtSeconds = result.expiresAt(); + retryNotBeforeEpochSeconds = 0; LOG.info( () -> @@ -217,6 +344,7 @@ private void doFetch(boolean allowStaleOnFailure) throws Exception { Thread.currentThread().interrupt(); throw e; } catch (Exception e) { + retryNotBeforeEpochSeconds = nowEpochSeconds() + failureBackoffSeconds(); if (allowStaleOnFailure && cachedDocument != null) { LOG.log( Level.WARNING, @@ -224,7 +352,9 @@ private void doFetch(boolean allowStaleOnFailure) throws Exception { + documentType + " from " + url - + "; using stale cache", + + "; using stale cache for up to " + + failureBackoffSeconds() + + "s before retrying", e); } else { throw e; @@ -232,9 +362,55 @@ private void doFetch(boolean allowStaleOnFailure) throws Exception { } } + /** + * Backoff applied after a failed refresh, never longer than the configured interval — a cache + * asked to refresh every 5 seconds must not be pinned to a 30-second retry floor. + * + *

Exposed as a static so the one caller outside this class that retries a network operation + * on the verification path — the {@code jwks_uri} rebind in {@code AuthplaneClient}, which + * builds a fresh cache per attempt and so has no instance state to carry a backoff on — applies + * the same policy rather than a second copy of it. + * + * @param configuredRefreshSeconds the refresh interval the backoff is being applied to + * @return seconds to wait before the next attempt, at least 1 + */ + public static long failureBackoffSeconds(long configuredRefreshSeconds) { + return Math.max(1, Math.min(FAILURE_BACKOFF_SECONDS, configuredRefreshSeconds)); + } + + private long failureBackoffSeconds() { + return failureBackoffSeconds(configuredRefreshSeconds); + } + + /** + * The TTL actually in force: the configured interval, shortened by a server expiry when the + * server asks for something sooner. + * + *

A server expiry at or before the moment the document was cached is treated as no + * preference rather than as an expiry, and the configured interval governs. It has to be: + * {@code CacheHeaderParser.parseExpiresAt} returns {@code 0L} for {@code Cache-Control: + * no-store} or {@code no-cache}, {@code now} for {@code max-age=0}, and a past epoch for a + * stale {@code Expires:} — and subtracting {@code cachedAtEpochSeconds} from any of those + * yields a negative TTL. For {@code no-store} that is about -1.7e9. + * + *

A negative TTL makes {@code age >= effectiveTtl} true on every read, so {@link #get()} + * takes the synchronous re-fetch branch on the caller's thread every single time, forever. The + * failure backoff does not cover it, because that only arms when a fetch *throws*: an endpoint + * that answers {@code no-store} successfully clears the backoff and re-arms the expiry on the + * same call. + * + *

That was harmless while nothing on a verification path read this cache. It stopped being + * harmless when metadata moved onto that path — verification now reads through here before + * every key lookup, and that runs before signature verification, so an unauthenticated caller + * would set the rate. This is the same failure the backoff was added to remove, reached by a + * different door. + * + *

go-sdk clamps the equivalent case the same way: a zero expiry falls back to the configured + * default rather than being taken literally. + */ private long effectiveTtlSeconds() { - long configuredExpiry = cachedAtEpochSeconds + configuredRefreshSeconds; - if (serverExpiresAtSeconds != null) { + if (serverExpiresAtSeconds != null && serverExpiresAtSeconds > cachedAtEpochSeconds) { + long configuredExpiry = cachedAtEpochSeconds + configuredRefreshSeconds; return Math.min(configuredExpiry, serverExpiresAtSeconds) - cachedAtEpochSeconds; } return configuredRefreshSeconds; diff --git a/core/src/main/java/ai/authplane/sdk/core/fetching/JwksCache.java b/core/src/main/java/ai/authplane/sdk/core/fetching/JwksCache.java index 1c379bb..7d36d98 100644 --- a/core/src/main/java/ai/authplane/sdk/core/fetching/JwksCache.java +++ b/core/src/main/java/ai/authplane/sdk/core/fetching/JwksCache.java @@ -1,5 +1,6 @@ package ai.authplane.sdk.core.fetching; +import java.time.Clock; import java.util.List; import java.util.Map; import java.util.Optional; @@ -21,7 +22,41 @@ public JwksCache( String jwksUrl, int refreshSeconds, BiConsumer, Map> onChangeCallback) { - super(fetcher, jwksUrl, refreshSeconds, "JWKS", onChangeCallback); + this(fetcher, jwksUrl, refreshSeconds, onChangeCallback, Clock.systemUTC()); + } + + /** + * Same as the four-argument constructor, but with the time source used for TTL evaluation + * supplied by the caller. + * + *

Supported public API, not a test seam: this class has no {@code internal} package and no + * binary-compatibility gate, so anything public here is contract. It is the only way an + * embedder can drive refresh intervals deterministically — from a simulation clock, or from a + * test that states elapsed time instead of waiting for it. The superclass keeps the equivalent + * constructor package-private because {@code DocumentCache} is never constructed directly by a + * caller outside this package. + * + *

The clock must not diverge from wall time by more than a cache TTL. A + * server {@code max-age} is turned into an absolute expiry against the system clock, in {@link + * CacheHeaderParser}, before it ever reaches this cache — and this cache compares it against a + * timestamp taken from the clock supplied here. Offset the two far apart and every real server + * expiry reads as already past, so server cache directives are discarded wholesale and the + * configured interval governs alone. That is a safe fallback rather than a failure, but it is + * silent. Advancing the clock forward from the real present, which is what a deterministic TTL + * test does, is fine: the offset only has to stay inside a TTL of wall time at the moment a + * document is fetched. Threading the clock into the header parser would remove the constraint + * and is tracked in #34. + * + * @param clock time source; pass {@link Clock#systemUTC()} unless driving TTL expiry + * deterministically + */ + public JwksCache( + DocumentFetcher fetcher, + String jwksUrl, + int refreshSeconds, + BiConsumer, Map> onChangeCallback, + Clock clock) { + super(fetcher, jwksUrl, refreshSeconds, "JWKS", onChangeCallback, clock); } /** diff --git a/core/src/main/java/ai/authplane/sdk/core/fetching/MetadataCache.java b/core/src/main/java/ai/authplane/sdk/core/fetching/MetadataCache.java index 914088f..9e09727 100644 --- a/core/src/main/java/ai/authplane/sdk/core/fetching/MetadataCache.java +++ b/core/src/main/java/ai/authplane/sdk/core/fetching/MetadataCache.java @@ -1,6 +1,7 @@ package ai.authplane.sdk.core.fetching; import java.net.URI; +import java.time.Clock; import java.util.Map; import java.util.Objects; import java.util.function.BiConsumer; @@ -44,45 +45,86 @@ public MetadataCache( String expectedIssuer, boolean allowHttp, BiConsumer, Map> onChangeCallback) { - super(fetcher, metadataUrl, refreshSeconds, "metadata", onChangeCallback); - // Required: the RFC 8414 §3.3 comparison in getJwksUri() dereferences this. Without the - // check a null surfaces as a bare NPE from the first metadata read rather than as a + this( + fetcher, + metadataUrl, + refreshSeconds, + expectedIssuer, + allowHttp, + onChangeCallback, + Clock.systemUTC()); + } + + /** + * Same as the six-argument constructor, but with the time source used for TTL evaluation + * supplied by the caller. + * + *

Supported public API, not a test seam — see {@link JwksCache#JwksCache(DocumentFetcher, + * String, int, java.util.function.BiConsumer, Clock)}. The parameter count is inherited from + * the six-argument constructor this one extends; a builder would fix it for both, which is a + * change to make on its own rather than folded into a behavioural fix. + * + * @param clock time source; pass {@link Clock#systemUTC()} unless driving TTL expiry + * deterministically + */ + @SuppressWarnings("checkstyle:ParameterNumber") + public MetadataCache( + DocumentFetcher fetcher, + String metadataUrl, + int refreshSeconds, + String expectedIssuer, + boolean allowHttp, + BiConsumer, Map> onChangeCallback, + Clock clock) { + super(fetcher, metadataUrl, refreshSeconds, "metadata", onChangeCallback, clock); + // Required: the RFC 8414 §3.3 comparison in validateMetadata() dereferences this. Without + // the check a null surfaces as a bare NPE from the first metadata read rather than as a // contract violation at construction. this.expectedIssuer = Objects.requireNonNull(expectedIssuer, "expectedIssuer must not be null"); this.allowHttp = allowHttp; } + /** + * Validates every freshly fetched document, so an invalid one is never published to the cache + * and never reaches the change callback. + * + *

Validating at fetch time rather than at read time matters once metadata is re-read under + * ordinary traffic: a refresh that returns a document with the wrong issuer must not displace + * the good one, and — because the change callback rebinds JWKS fetching to the document's + * {@code jwks_uri} — must not be able to point key retrieval somewhere new either. + */ + @Override + protected void validateFetched(Map document) throws MetadataFetchException { + validateMetadata(document); + } + /** * Returns the {@code jwks_uri} from the current (or freshly fetched) metadata. * - * @throws MetadataFetchException if the metadata is unavailable or missing jwks_uri + *

A read, not a check: {@code validateMetadata} rejects a document without a usable {@code + * jwks_uri} before it reaches the cache, so anything served from here has one. The failure this + * used to raise is now raised at fetch time, which is what keeps an invalid refresh from + * displacing the document being served. + * + * @throws MetadataFetchException if the metadata is unavailable */ public String getJwksUri() throws Exception { - Map metadata = getMetadata(); - - Object jwksUri = metadata.get("jwks_uri"); - if (!(jwksUri instanceof String jwksUriStr) || jwksUriStr.isBlank()) { - throw new MetadataFetchException( - "OAuth server metadata is missing or has empty 'jwks_uri' field"); - } - - LOG.fine(() -> "jwks_uri from metadata: " + jwksUriStr); - return jwksUriStr; + String jwksUri = (String) getMetadata().get("jwks_uri"); + LOG.fine(() -> "jwks_uri from metadata: " + jwksUri); + return jwksUri; } private Map getMetadata() throws MetadataFetchException { - Map metadata; try { - metadata = get(); + // Whatever the cache returns has already passed validateFetched(). + return get(); } catch (MetadataFetchException e) { throw e; } catch (Exception e) { throw new MetadataFetchException( "Failed to fetch OAuth server metadata: " + e.getMessage(), e); } - validateMetadata(metadata); - return metadata; } /** @@ -108,6 +150,22 @@ private void validateMetadata(Map metadata) throws MetadataFetch + "'"); } + // RFC 8414 §2 marks jwks_uri OPTIONAL — REQUIRED is OpenID Connect Discovery, a + // different document. This SDK requires it anyway: every verification path builds a + // JwtValidator, and introspection is layered on top of JWT validation rather than + // offered as an alternative to it, so a document without jwks_uri is one this SDK + // cannot use. Checking presence here rather than at read time is the same move as the + // rest of this change: a document the SDK cannot use must not displace the one already + // being served. + // It is also the field the refresh mechanism itself runs on — with jwks_uri gone, + // AuthplaneClient.refreshMetadataIfDue has nothing to reconcile the binding against, so a + // later rotation would not be followed even after the AS fixed its document. + Object jwksUri = metadata.get("jwks_uri"); + if (!(jwksUri instanceof String jwksUriStr) || jwksUriStr.isBlank()) { + throw new MetadataFetchException( + "OAuth server metadata is missing or has empty 'jwks_uri' field"); + } + // Validate endpoint URLs (RFC 8414 §2: endpoints MUST be absolute HTTPS URLs) for (String field : ENDPOINT_FIELDS) { Object value = metadata.get(field); diff --git a/core/src/main/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadata.java b/core/src/main/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadata.java index f899a53..2d8e4a9 100644 --- a/core/src/main/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadata.java +++ b/core/src/main/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadata.java @@ -73,13 +73,38 @@ private ProtectedResourceMetadata( * verbatim rather than decoded into a path separator — decoding it would name a different path * than the resource identifier does. * + *

The returned value is the URL path only — the route at which the document is + * served. A query component of the resource identifier is never part of it: RFC 9728 §3 inserts + * the well-known string "between the host component and the path and/or query components", so + * the query follows the derived path in the full document URL (see {@link + * #wellKnownUrl(String)}) but does not select a different route. Routing stays path-keyed, so + * identifiers differing only by query share one registered route serving one document. Serving + * distinct documents per query value is not supported: RFC 9728 §3.3 requires a client to + * discard a response whose {@code resource} member differs from the identifier it derived the + * request from, so any query value the shared document's {@code resource} was not built for + * fails that client-side check. + * * @param resourceUri the resource server URI; must be hierarchical and carry a scheme and an * authority * @return the URL path (including leading slash) where the PRM should be served - * @throws IllegalArgumentException if {@code resourceUri} is opaque, has no scheme, or has no - * authority + * @throws IllegalArgumentException if {@code resourceUri} is opaque, has no scheme, has no + * authority, or carries a fragment component */ public static String wellKnownPath(URI resourceUri) { + // Backstop only: the fragment is rejected at construction, so a resource built through + // AuthplaneClient.resource() or this class's builder can never reach here carrying one. + // Kept because these helpers are public and are called on the 401 challenge path, where a + // silently dropped fragment would publish a document that names a different identifier. + // + // It runs before requireDerivable because the two answer different questions: a fragment + // is unconditionally illegal (RFC 8707 §2), while non-derivability is a limitation of this + // helper. An identifier that is both — "urn:example:api#secret" — should be reported for + // the fragment, not for the URN, and reporting the fragment is also what keeps it out of + // the message. Called unconditionally: requireNoFragment returns on the no-'#' path, and + // gating on getRawFragment() would be the one place where the parsed-URI view and the raw + // string could disagree about whether there is a fragment at all. + requireNoFragment(resourceUri.toString()); + requireValidQuery(resourceUri.toString()); requireDerivable(resourceUri); // Read the raw path: URI.getPath() percent-decodes, which would turn a resource @@ -110,19 +135,514 @@ public static String wellKnownPath(URI resourceUri) { *

The path component is derived by {@link #wellKnownPath(URI)}, so both helpers agree by * construction: the slash stripping happens in exactly one place. * + *

A query component of the resource identifier is preserved: RFC 9728 §3 forms the + * well-known URI by inserting the well-known string "between the host component and the path + * and/or query components, if any", so the query follows the derived path. A resource + * identifier may legitimately carry one — RFC 8707 §2 states the SHOULD NOT and its exception + * in the same sentence, and RFC 9728 §1.2 carries that forward. Dropping it would advertise a + * document URL for a different identifier than the one this resource publishes in its {@code + * resource} field. + * + * 

+     * "https://api.example.com/mcp?tenant=a" → "https://api.example.com/.well-known/oauth-protected-resource/mcp?tenant=a"
+     * "https://api.example.com?x=1"          → "https://api.example.com/.well-known/oauth-protected-resource?x=1"
+     * "https://api.example.com/?x=1"         → "https://api.example.com/.well-known/oauth-protected-resource?x=1"
+     *
+ * + *

The last two agree because RFC 9728 §3.1 removes the terminating slash following the host + * when a path or query is present. The raw (percent-encoded) authority and query are carried + * through verbatim — decoding and re-encoding could rewrite octets and name a different + * identifier. An empty query (a bare trailing {@code ?}) is treated as absent: the derived URL + * carries no {@code ?}. + * * @param resourceUri the resource server URI string; must be hierarchical and carry a scheme * and an authority * @return the full PRM document URL - * @throws IllegalArgumentException if {@code resourceUri} is opaque, has no scheme, or has no - * authority + * @throws IllegalArgumentException if {@code resourceUri} is opaque, has no scheme, has no + * authority, carries userinfo, carries a fragment component, or carries a query outside the + * RFC 3986 §3.4 grammar */ public static String wellKnownUrl(String resourceUri) { + // Before URI.create: an identifier that is both malformed and fragment-bearing should + // report the fragment, which is the illegal part, rather than a wrapped URISyntaxException. + // requireDerivable is left to wellKnownPath — calling it here as well only duplicated the + // answer. + // + // All four construction gates run here, not two. The reason the backstops exist at all is + // that this method is public and reachable with a string no constructor ever saw, and that + // reason does not distinguish between them: without requireNoUserinfo, + // wellKnownUrl("https://svc:pw@h/mcp") still splices a credential into the URL this SDK + // publishes in a 401 challenge. + requireNoFragment(resourceUri); + requireScheme(resourceUri); + requireNoUserinfo(resourceUri); + requireValidQuery(resourceUri); URI uri = URI.create(resourceUri); - requireDerivable(uri); // getRawAuthority(): the raw-preservation rule applies to every component, the authority // included. getAuthority() percent-decodes, so "u%40b@host" derived "u@b@host" — an // authority structurally different from the one the identifier names. - return uri.getScheme() + "://" + uri.getRawAuthority() + wellKnownPath(uri); + String url = uri.getScheme() + "://" + uri.getRawAuthority() + wellKnownPath(uri); + // Preserve the query component (RFC 9728 §3: the well-known string is inserted between + // the host and "the path and/or query components, if any"). Raw form, so the encoding is + // exactly what the operator configured. An empty query (a bare trailing '?', for which + // getRawQuery() returns "") is treated as absent: RFC 3986 would allow reading it as + // present-but-empty, but on *this* sub-case — an empty query — the family agrees on the + // query-less URL, and parity wins over that reading. It is only the empty-query reading + // that is settled: for a non-empty query the implementations still differ, which is + // tracked in #32 rather than asserted here. + String query = uri.getRawQuery(); + return query == null || query.isEmpty() ? url : url + "?" + query; + } + + /** + * Rejects a resource identifier that carries a URI fragment component. + * + *

RFC 8707 §2: "The URI MUST NOT include a fragment component." RFC 9728 §1.2 restates it + * for the identifier a PRM document names. + * + *

The check runs on the raw string rather than on a parsed {@link URI} because {@link URI} + * is exactly what hides the defect: it splits the fragment off, so {@code + * https://api.example.com/mcp#x} derives the well-known URL of {@code + * https://api.example.com/mcp} while the document publishes the identifier verbatim. The served + * document's {@code resource} then disagrees with the URL it was fetched from, and RFC 9728 + * §3.3 requires a conformant client to discard the response — an interop failure with no + * server-side error to notice. + * + *

Scanning for the character is precise: an unescaped {@code #} always begins a fragment + * (RFC 3986 §3.5), and a literal {@code #} inside a path is spelled {@code %23}, so {@code + * https://api.example.com/a%23b} is fragment-free and passes. + * + *

This is the construction-time gate. It is called from every path that accepts an + * operator-configured identifier: {@link Builder#build()}, the {@code AuthplaneResource} + * constructor (which every resource reaches), and {@code AuthplaneClient.resource(...)} — the + * last one redundant for the guarantee but worth the stack trace, since it fails at the line + * the operator wrote. Gating only in the derivation helpers above would defer the failure to + * {@code prmUrl()}, i.e. into a 401 response path, turning a configuration error into a 500 at + * the worst possible moment. + * + * @param resourceUri the resource identifier, as configured by the operator + * @throws IllegalArgumentException if the identifier carries a fragment component + */ + public static void requireNoFragment(String resourceUri) { + // AuthplaneClient.resource(...) two frames up reports this condition as a + // NullPointerException with a message; this is public API and should not report it as a + // bare NPE from indexOf. + Objects.requireNonNull(resourceUri, "resourceUri must not be null"); + if (resourceUri.indexOf('#') < 0) { + return; + } + throw new IllegalArgumentException( + "Resource identifier \"" + + elideSecrets(resourceUri) + + "\" (fragment and any userinfo elided) must not include a fragment" + + " component (RFC 8707 §2, RFC 9728 §1.2). The fragment is dropped when" + + " the Protected Resource Metadata URL is derived, so the published" + + " document would name an identifier that its own URL disagrees with, and" + + " RFC 9728 §3.3 requires a client to discard such a document. Remove the" + + " fragment from the configured resource identifier."); + } + + /** + * Rejects a resource identifier whose query component is not a valid RFC 3986 §3.4 query. + * + *

{@code query = *( pchar / "/" / "?" )}, with {@code pchar = unreserved / pct-encoded / + * sub-delims / ":" / "@"}. Every octet must therefore be a query character or part of a + * well-formed two-hex-digit percent-escape. This validates only — nothing is escaped on the + * operator's behalf, because rewriting the query would change the resource's identity (RFC 3986 + * §6.2.2.2 permits decoding only unreserved octets when comparing identifiers). + * + *

The query is now a supported part of the identifier and is spliced verbatim into the + * {@code resource_metadata} value of a {@code WWW-Authenticate} challenge, so an octet outside + * the production has two ways to do damage, both of which the gate closes: + * + *

    + *
  • A raw non-ASCII octet ships into the header. {@code WwwAuthenticate.escapeQuotedString} + * strips control characters and escapes {@code \} and {@code "}; a non-ASCII octet is + * none of those. RFC 9110 §5.5 confines field values to US-ASCII (obs-text is deprecated + * and recipients treat it opaquely), so the advertised {@code resource_metadata} is not a + * URI and two clients decoding those octets differently fetch two different URLs. + *
  • Nothing else rejects it at construction. {@link URI} does reject a space, {@code "}, + * {@code \}, {@code |}, {@code ^}, {, }, {@code <} and + * {@code >} — but only when the identifier is finally parsed, which happens in {@code + * prmUrl()}, on the 401 response path. {@code resource("…/mcp?a=b c")} constructed + * cleanly and then threw out of {@code AuthplaneAuthenticationEntryPoint.commence()}: a + * 500 in place of the 401. That is verbatim the failure {@link + * #requireNoFragment(String)} exists to prevent. + *
+ * + *

Called from the same four boundaries as {@link #requireNoFragment(String)}. Neither the + * authority nor the path is gated here, and both carry the same two failure modes listed above: + * a non-ASCII octet in either ships into the quoted-string, and a path {@link URI} rejects + * still throws out of {@code prmUrl()} on the 401 response path. Both are pre-existing — the + * authority and the path were always part of the derived URL, and this change neither widens + * nor narrows them — so tightening this construction boundary is deliberately deferred to a + * single follow-up, so that it happens in one step rather than piecemeal. When that follow-up + * lands, the remedy for the path has to be the same as for the query — reject, do not escape: + * percent-encoding the path on the operator's behalf would make the derived URL name a + * different path than the {@code resource} member does. + * + * @param resourceUri the resource identifier, as configured by the operator + * @throws IllegalArgumentException if the identifier's query is not a valid RFC 3986 §3.4 query + */ + public static void requireValidQuery(String resourceUri) { + Objects.requireNonNull(resourceUri, "resourceUri must not be null"); + // Cut the fragment first: a '?' after a '#' belongs to the fragment, not the query. In + // practice requireNoFragment has already rejected any '#' at every call site, but this + // method is public and answers its own question. + int fragmentStart = resourceUri.indexOf('#'); + String beforeFragment = + fragmentStart < 0 ? resourceUri : resourceUri.substring(0, fragmentStart); + int queryStart = beforeFragment.indexOf('?'); + if (queryStart < 0) { + return; + } + String reason = invalidQueryReason(beforeFragment.substring(queryStart + 1)); + if (reason == null) { + return; + } + throw new IllegalArgumentException( + "Resource identifier \"" + + elideQuery(resourceUri) + + "\" (query and any userinfo elided) has a query component that is not a" + + " valid query per RFC 3986 §3.4: " + + reason + + ". The query is preserved verbatim into the Protected Resource Metadata" + + " URL and from there into the resource_metadata parameter of the" + + " WWW-Authenticate challenge, where an octet outside the query production" + + " yields a value that is not a URI (RFC 9110 §5.5 confines field values to" + + " US-ASCII). Percent-encode the offending octet in the configured resource" + + " identifier."); + } + + /** + * Reports why {@code query} is not a valid RFC 3986 §3.4 query, or {@code null} when it is. The + * offending octet is named by position and, when it is not printable ASCII, by code point — + * printing the character itself would render something that never appeared in the input. + */ + private static String invalidQueryReason(String query) { + for (int i = 0; i < query.length(); i++) { + char c = query.charAt(i); + if (c == '%') { + if (i + 2 >= query.length() + || !isHexDigit(query.charAt(i + 1)) + || !isHexDigit(query.charAt(i + 2))) { + return "malformed percent-escape at index " + i; + } + i += 2; + continue; + } + if (isQueryChar(c)) { + continue; + } + if (c >= 0x20 && c < 0x7F) { + return "invalid character '" + c + "' at index " + i; + } + return String.format( + "invalid non-ASCII or control character U+%04X at index %d", (int) c, i); + } + return null; + } + + /** + * Whether {@code c} may appear literally in a URI query: the pchar set (unreserved, sub-delims, + * {@code ":"}, {@code "@"}) plus {@code "/"} and {@code "?"} (RFC 3986 §§2.2, 2.3, 3.3, 3.4). + * Percent-escapes are handled by the caller. + */ + private static boolean isQueryChar(char c) { + return (c >= 'A' && c <= 'Z') + || (c >= 'a' && c <= 'z') + || (c >= '0' && c <= '9') + || "-._~".indexOf(c) >= 0 // unreserved + || "!$&'()*+,;=".indexOf(c) >= 0 // sub-delims + || c == ':' + || c == '@' // pchar extras + || c == '/' + || c == '?'; // query extras + } + + private static boolean isHexDigit(char c) { + return (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'); + } + + /** + * Requires the identifier to carry a URI scheme — the absolute-URI requirement of RFC 8707 §2, + * gated at construction like {@link #requireNoFragment(String)} and {@link + * #requireValidQuery(String)}. + * + *

A scheme-less identifier is unconditionally illegal, not merely non-derivable: RFC 8707 §2 + * requires the resource indicator to be an absolute URI, and RFC 3986 §4.3 defines one as + * always carrying a scheme. Leaving the check to the derivation gate ({@link + * #wellKnownPath(URI)}) is not enough, because the derivation is not the only sink — {@code + * AuthplaneResource} splices the identifier's scheme into the DPoP {@code htu} it verifies + * against, so {@code //api.example.com/mcp} yields the literal binding target {@code + * null://api.example.com/mcp} and every DPoP-bound request fails with a mismatch that names + * nothing an operator can act on. + * + *

Only the scheme is required here. The identifier may still be any absolute URI RFC 8707 §2 + * permits — {@code urn:example:api} constructs; whether it can derive a PRM URL is the + * derivation gate's question, answered when a derivation is actually asked for. + * + *

Works on the raw string, like the sibling gates: a scheme is present exactly when a {@code + * :} appears before any {@code /}, {@code ?} or {@code #} and the text before it matches the + * RFC 3986 §3.1 scheme production ({@code ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )}). + * + *

Called from the same construction boundaries as the sibling gates: {@link + * Builder#build()}, the {@code AuthplaneResource} constructor, and {@code + * AuthplaneClient.resource(...)}. The derivation-time {@code requireDerivable} stays as the + * backstop for the public derivation helpers. + * + * @param resourceUri the resource identifier, as configured by the operator + * @throws IllegalArgumentException if the identifier does not begin with a URI scheme + */ + public static void requireScheme(String resourceUri) { + Objects.requireNonNull(resourceUri, "resourceUri must not be null"); + if (hasScheme(resourceUri)) { + return; + } + throw new IllegalArgumentException( + "Resource identifier \"" + + elideSecrets(resourceUri) + + "\" (fragment and any userinfo elided) has no scheme: RFC 8707 §2" + + " requires the resource indicator to be an absolute URI, which always" + + " begins with a scheme (RFC 3986 §4.3). A scheme-relative or relative" + + " reference cannot name the resource: the scheme is spliced into the" + + " derived Protected Resource Metadata URL and into the DPoP htu binding" + + " target, both of which would read the missing scheme as the literal" + + " text \"null\". Prefix the intended scheme (e.g." + + " https://api.example.com/mcp)."); + } + + /** + * Rejects a resource identifier whose authority carries a userinfo component. + * + *

RFC 9110 §4.2.4 deprecates userinfo in an {@code http} or {@code https} URI and directs a + * recipient to reject one that carries it; RFC 3986 §3.2.1 warns that it routinely holds a + * credential in clear text. Here the stakes are higher than for a request target that merely + * gets logged: the identifier is stored verbatim, published as the {@code resource} member of + * the Protected Resource Metadata document that RFC 9728 §3 serves to unauthenticated callers, + * and spliced into the {@code resource_metadata} parameter of the 401 {@code WWW-Authenticate} + * challenge. A credential in the userinfo is therefore handed to everyone who asks. + * + *

Rejecting at construction is what makes that guarantee. Eliding the userinfo at each sink + * only covers the sinks that remember to elide, and every sink added later has to remember + * again; the error messages keep eliding regardless, because these gates are public and are + * applied to strings this one never saw. + * + *

Works on the raw string like the sibling gates, reading the authority as everything + * between the {@code //} that opens it and the first {@code /} or {@code ?} that closes it. An + * unescaped {@code @} delimits userinfo there and appears nowhere else in an authority (RFC + * 3986 §3.2), so a host with a port ({@code https://api.example.com:8443/mcp}) and an IPv6 + * literal ({@code https://[::1]:8443/mcp}) both pass, as does an identifier with no authority + * at all ({@code urn:example:api}). An empty userinfo ({@code https://@api.example.com/mcp}) is + * still a userinfo component and is rejected. + * + *

Called from the same construction boundaries as the sibling gates — {@link + * Builder#build()}, the {@code AuthplaneResource} constructor, and {@code + * AuthplaneClient.resource(...)} — after {@link #requireScheme(String)}, so an identifier that + * is also scheme-relative is reported for the missing scheme, the defect an operator fixes + * first. + * + *

The four gates run in the same *set* everywhere but not in the same *order*: the three + * construction sites run fragment, query, scheme, userinfo, while {@link #wellKnownUrl(String)} + * runs fragment, scheme, userinfo, query. So an identifier that violates two of them can be + * reported for a different component depending on the entrypoint. Both reject either way; only + * the message differs. Unifying the four behind one private gate is tracked in #33. + * + * @param resourceUri the resource identifier, as configured by the operator + * @throws IllegalArgumentException if the identifier's authority carries a userinfo component + */ + public static void requireNoUserinfo(String resourceUri) { + Objects.requireNonNull(resourceUri, "resourceUri must not be null"); + // Cut the fragment first: an '@' after a '#' belongs to the fragment, not the authority. + // requireNoFragment has already rejected any '#' at every call site, but this method is + // public and answers its own question. + int fragmentStart = resourceUri.indexOf('#'); + String beforeFragment = + fragmentStart < 0 ? resourceUri : resourceUri.substring(0, fragmentStart); + int[] authority = authorityBounds(beforeFragment); + if (authority == null || beforeFragment.lastIndexOf('@', authority[1] - 1) < authority[0]) { + return; + } + throw new IllegalArgumentException( + "Resource identifier \"" + + elideSecrets(resourceUri) + + "\" (fragment and any userinfo elided) must not include a userinfo" + + " component in its authority: RFC 9110 §4.2.4 deprecates userinfo and" + + " directs a recipient to reject a URI carrying it, and RFC 3986 §3.2.1" + + " notes it routinely holds a credential in clear text. The identifier is" + + " stored verbatim — it is published as the resource member of the" + + " Protected Resource Metadata document, which RFC 9728 §3 serves to" + + " unauthenticated callers, and spliced into the resource_metadata" + + " parameter of the 401 WWW-Authenticate challenge — so the credential" + + " would be disclosed to every client that asks. Remove the userinfo from" + + " the configured resource identifier (e.g." + + " https://api.example.com/mcp) and present the credential in the" + + " Authorization header instead."); + } + + /** Whether {@code resourceUri} begins with an RFC 3986 §3.1 scheme followed by {@code :}. */ + private static boolean hasScheme(String resourceUri) { + return schemeEnd(resourceUri) >= 0; + } + + /** + * Index of the {@code :} terminating the RFC 3986 §3.1 scheme of {@code resourceUri}, or {@code + * -1} when the string does not begin with a scheme. Only a {@code :} reached through scheme + * characters alone delimits a scheme, so a {@code :} inside an authority, path or query is + * never mistaken for one. + */ + private static int schemeEnd(String resourceUri) { + for (int i = 0; i < resourceUri.length(); i++) { + char c = resourceUri.charAt(i); + if (c == ':') { + return i > 0 ? i : -1; // non-empty; earlier iterations validated every character + } + if (c == '/' || c == '?' || c == '#') { + return -1; // path, query or fragment began before any ':' + } + boolean validSchemeChar = + i == 0 + ? isAlpha(c) + : isAlpha(c) || (c >= '0' && c <= '9') || "+-.".indexOf(c) >= 0; + if (!validSchemeChar) { + return -1; // not a scheme character, so any later ':' is not a scheme delimiter + } + } + return -1; // no ':' at all + } + + /** + * Best-effort authority bounds for {@link #elideSecrets(String)} only, used when {@link + * #authorityBounds(String)} declines. + * + *

Anchors on the first {@code "//"} anywhere in the string rather than requiring a valid + * scheme in front of it, so an identifier the strict helper cannot parse still gets redacted. + * Never use this for a gate: it will happily find an "authority" inside a query, which is a + * false rejection there and merely a harmless over-redaction here. + * + * @return {@code {start, end}} of the presumed authority, or {@code null} if there is no {@code + * "//"} at all — a genuinely opaque identifier such as {@code urn:example:api}, which has + * no authority and therefore no userinfo. + */ + private static int[] bestEffortAuthorityBounds(String beforeFragment) { + int slashes = beforeFragment.indexOf("//"); + if (slashes < 0) { + return null; + } + int start = slashes + "//".length(); + int end = beforeFragment.length(); + for (int i = start; i < end; i++) { + char c = beforeFragment.charAt(i); + if (c == '/' || c == '?') { + end = i; + break; + } + } + return new int[] {start, end}; + } + + /** + * Bounds {@code [start, end)} of the authority component of a fragment-free identifier, or + * {@code null} when it has none. Shared by {@link #requireNoUserinfo(String)} and {@link + * #elideSecrets(String)} so the gate and the redactor can never disagree about where the + * authority is. + * + *

A leading {@code //} is tested before the {@code ://} of an absolute URI, never + * the other way round: index 0 cannot be preceded by a scheme, so a scheme-relative reference + * (RFC 3986 §4.2) opens its authority at index 2 and a later {@code ://} in its path or query + * is not an authority delimiter. Testing {@code ://} first anchored the authority inside the + * query of {@code //svc:pw@api.example.com/mcp?next=https://x}, which put the real userinfo + * before the supposed authority and made the redactor return the credential verbatim. + * + *

The {@code ://} of an absolute URI is located through {@link #schemeEnd(String)} rather + * than by searching for the literal, for the same reason in the other direction: an + * authority-less identifier such as {@code https:example.com/mcp?u=http://x} must not have an + * authority conjured out of its query. + */ + private static int[] authorityBounds(String beforeFragment) { + int start; + if (beforeFragment.startsWith("//")) { + start = "//".length(); + } else { + int schemeColon = schemeEnd(beforeFragment); + if (schemeColon < 0 || !beforeFragment.startsWith("//", schemeColon + 1)) { + return null; // opaque or path-relative: no authority + } + start = schemeColon + 1 + "//".length(); + } + + int end = beforeFragment.length(); + for (int i = start; i < end; i++) { + char c = beforeFragment.charAt(i); + if (c == '/' || c == '?') { + end = i; + break; + } + } + return new int[] {start, end}; + } + + private static boolean isAlpha(char c) { + return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z'); + } + + /** + * {@link #elideSecrets(String)} plus the query, for a message about the query itself — the same + * reason the fragment is elided from the message about the fragment. + */ + private static String elideQuery(String resourceUri) { + String elided = elideSecrets(resourceUri); + int queryStart = elided.indexOf('?'); + return queryStart < 0 ? elided : elided.substring(0, queryStart); + } + + /** + * Renders an identifier for an error message with the two components that should not reach a + * log removed: the fragment, and the userinfo of the authority. Both are operator-supplied and + * both routinely carry a credential; the scheme, host and path are what let an operator find + * the offending configuration. + * + *

Works on the raw string rather than a parsed {@link URI}, so it is usable on an identifier + * that does not parse — which is the case these messages most need to describe. + */ + private static String elideSecrets(String resourceUri) { + String elided = resourceUri; + int fragmentStart = elided.indexOf('#'); + if (fragmentStart >= 0) { + elided = elided.substring(0, fragmentStart); + } + + // An authority follows a leading "//" in a scheme-relative reference (RFC 3986 §4.2) — + // exactly the shape the scheme gate rejects — or the "://" of an absolute hierarchical + // URI. This method must elide the userinfo of either, or the message asserting elision + // would carry the credential verbatim. + // + // The two callers pull in opposite directions and cannot share one helper: the gate must + // not over-reject, so authorityBounds requires a valid scheme; the redactor must not + // under-redact, and giving up on a strict miss is precisely under-redacting. An + // identifier whose scheme does not parse — "1https://svc:pw@host/mcp", or the likelier + // " https://svc:pw@host/mcp" out of YAML or env, which nothing on the construction path + // trims — reaches requireScheme and used to ship its credential verbatim in a message + // ending "(fragment and any userinfo elided)". So on a strict miss, fall back to a + // best-effort scan. Over-redacting something that is not an authority costs nothing in a + // message; leaking a password costs everything. + int[] authority = authorityBounds(elided); + if (authority == null) { + authority = bestEffortAuthorityBounds(elided); + } + if (authority == null) { + return elided; // opaque or path-relative: no authority, so no userinfo + } + int authorityStart = authority[0]; + int authorityEnd = authority[1]; + + // RFC 3986 §3.2.1: '@' is not allowed unescaped inside userinfo, so the last '@' within + // the authority is its delimiter. + int userInfoEnd = elided.lastIndexOf('@', authorityEnd - 1); + if (userInfoEnd < authorityStart) { + return elided; + } + return elided.substring(0, authorityStart) + "***@" + elided.substring(userInfoEnd + 1); } /** @@ -142,18 +662,28 @@ public static String wellKnownUrl(String resourceUri) { * §4.3 defines one as always carrying a scheme, so no legitimate identifier is turned away. */ private static void requireDerivable(URI resourceUri) { - if (resourceUri.isOpaque() - || resourceUri.getScheme() == null - || resourceUri.getAuthority() == null) { - throw new IllegalArgumentException( - "Cannot derive a Protected Resource Metadata URL from \"" - + resourceUri - + "\": PRM derivation requires a hierarchical resource identifier with" - + " a scheme and an authority (e.g. https://api.example.com/mcp). The" - + " resource identifier itself may be any absolute URI permitted by RFC" - + " 8707 §2 and is stored verbatim; only the derivation is" - + " restricted."); + // Name the requirement this identifier actually fails — a generic both-requirements + // message reads as though everything is missing, which for "urn:example:api" (a scheme, + // no hierarchy) points the operator at the wrong half. + String defect; + if (resourceUri.isOpaque()) { + defect = "it is opaque (no hierarchical part follows the scheme)"; + } else if (resourceUri.getScheme() == null) { + defect = "it has no scheme"; + } else if (resourceUri.getAuthority() == null) { + defect = "it has no authority"; + } else { + return; } + throw new IllegalArgumentException( + "Cannot derive a Protected Resource Metadata URL from \"" + + elideSecrets(resourceUri.toString()) + + "\" (any userinfo elided): " + + defect + + ". PRM derivation requires a hierarchical resource identifier with a" + + " scheme and an authority (e.g. https://api.example.com/mcp). The" + + " resource identifier itself may be any absolute URI permitted by RFC" + + " 8707 §2 and is stored verbatim; only the derivation is restricted."); } // ----------------------------------------------------------------------- @@ -248,6 +778,10 @@ public ProtectedResourceMetadata build() { throw new IllegalArgumentException("resource must not be blank"); if (authorizationServer.isBlank()) throw new IllegalArgumentException("authorizationServer must not be blank"); + requireNoFragment(resource); + requireValidQuery(resource); + requireScheme(resource); + requireNoUserinfo(resource); return new ProtectedResourceMetadata( resource, List.of(authorizationServer), List.of("header"), scopes); diff --git a/core/src/test/java/ai/authplane/sdk/core/AuthplaneClientTest.java b/core/src/test/java/ai/authplane/sdk/core/AuthplaneClientTest.java index 12f2c16..17c4e79 100644 --- a/core/src/test/java/ai/authplane/sdk/core/AuthplaneClientTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/AuthplaneClientTest.java @@ -4,12 +4,14 @@ import static com.github.tomakehurst.wiremock.client.WireMock.containing; import static com.github.tomakehurst.wiremock.client.WireMock.equalTo; import static com.github.tomakehurst.wiremock.client.WireMock.get; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; import static com.github.tomakehurst.wiremock.client.WireMock.post; import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor; import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import java.time.Clock; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -20,6 +22,7 @@ import java.util.concurrent.atomic.AtomicReference; import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -30,6 +33,7 @@ import ai.authplane.sdk.core.errors.MetadataFetchException; import ai.authplane.sdk.core.errors.TokenExchangeException; +import ai.authplane.sdk.core.fetching.DocumentCache; import ai.authplane.sdk.core.oauth.IntrospectionResponse; /** @@ -41,6 +45,8 @@ */ class AuthplaneClientTest { + private static final String WELL_KNOWN_PATH = "/.well-known/oauth-authorization-server"; + private static WireMockServer wireMock; private static String baseUrl; private static TestFixtures.RSAKeyPair rsaKeys; @@ -58,6 +64,16 @@ static void stopWireMock() { wireMock.stop(); } + /** + * Clients built by this class, closed after every test. + * + *

Closing at the end of each test body leaks the client whenever an assertion above it fails + * — which is exactly when a test is already telling you something — and the failure then + * arrives with an executor and a JWKS refresh task still attached. Registering here keeps the + * cleanup on a path that runs either way, without a try/finally around every test body. + */ + private final List clients = new ArrayList<>(); + @BeforeEach void resetStubs() { wireMock.resetAll(); @@ -65,6 +81,14 @@ void resetStubs() { stubJwks(); } + @AfterEach + void closeClients() { + for (AuthplaneClient client : clients) { + client.close(); + } + clients.clear(); + } + // ----------------------------------------------------------------------- // Stub helpers // ----------------------------------------------------------------------- @@ -99,15 +123,21 @@ private void stubJwks() { } private AuthplaneClient buildClient() throws Exception { - return AuthplaneClient.builder(baseUrl) - .devMode(true) - .authProvider(new ASCredentials("test-client", "test-secret")) - .build() - .get(); + return register( + AuthplaneClient.builder(baseUrl) + .devMode(true) + .authProvider(new ASCredentials("test-client", "test-secret")) + .build() + .get()); + } + + private AuthplaneClient register(AuthplaneClient client) { + clients.add(client); + return client; } private AuthplaneClient buildClientNoCredentials() throws Exception { - return AuthplaneClient.builder(baseUrl).devMode(true).build().get(); + return register(AuthplaneClient.builder(baseUrl).devMode(true).build().get()); } private String validToken() { @@ -123,7 +153,6 @@ void build_discoversMetadata_andFetchesJwks() throws Exception { AuthplaneClient client = buildClient(); assertThat(client.issuer()).isEqualTo(baseUrl); assertThat(client.devMode()).isTrue(); - client.close(); } @Test @@ -228,8 +257,6 @@ void build_issuerWithTrailingSlash_reachesMetadataCacheAndValidatorVerbatim() th String token = TestFixtures.token().rsaKey(rsaKeys).issuer(issuerWithSlash).build(); VerifiedClaims claims = verifier.verify(token).get().claims(); assertThat(claims.issuer()).isEqualTo(issuerWithSlash); - - client.close(); } // ----------------------------------------------------------------------- @@ -244,7 +271,6 @@ void resource_createsWorkingResource() throws Exception { VerifiedClaims claims = verifier.verify(validToken()).get().claims(); assertThat(claims.sub()).isEqualTo(TestFixtures.SUBJECT); assertThat(claims.issuer()).isEqualTo(baseUrl); - client.close(); } @Test @@ -256,7 +282,6 @@ void resource_withOptions_createsWorkingResource() throws Exception { VerifiedClaims claims = verifier.verify(validToken()).get().claims(); assertThat(claims.sub()).isEqualTo(TestFixtures.SUBJECT); - client.close(); } @Test @@ -264,7 +289,6 @@ void resource_nullResource_throwsNPE() throws Exception { AuthplaneClient client = buildClient(); assertThatThrownBy(() -> client.resource(null, TestFixtures.SCOPES)) .isInstanceOf(NullPointerException.class); - client.close(); } @Test @@ -272,6 +296,142 @@ void resource_blankResource_throwsIAE() throws Exception { AuthplaneClient client = buildClient(); assertThatThrownBy(() -> client.resource(" ", TestFixtures.SCOPES)) .isInstanceOf(IllegalArgumentException.class); + } + + @Test + void resource_fragmentInResource_throwsIAE() throws Exception { + // RFC 8707 §2 forbids a fragment in a resource indicator. java.net.URI splits it off when + // the PRM URL is derived while prmResponse() publishes the identifier verbatim, so the + // served document would name an identifier its own URL disagrees with (RFC 9728 §3.3 + // requires a client to discard it). Reject at construction, not on the 401 path. + AuthplaneClient client = buildClient(); + assertThatThrownBy( + () -> + client.resource( + TestFixtures.RESOURCE + "/mcp#section", + TestFixtures.SCOPES)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a fragment component") + // The fragment itself is elided from the message; the prefix identifies the config. + .hasMessageContaining(TestFixtures.RESOURCE + "/mcp") + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("section")); + client.close(); + } + + @Test + void resourceConstructor_fragmentInResource_throwsIAE() throws Exception { + // The constructor gate is what the guarantee rests on — every AuthplaneResource is built + // here, including the ones the client factory never sees. Every other fragment case enters + // through client.resource(...), which throws at its own gate first, so without this the + // authoritative line is the one no test pins: delete it and the suite stays green. + AuthplaneClient client = buildClient(); + assertThatThrownBy( + () -> + new AuthplaneResource( + client, + TestFixtures.RESOURCE + "/mcp#section", + TestFixtures.SCOPES, + ResourceOptions.defaults())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a fragment component"); + client.close(); + } + + @Test + void resource_schemeRelativeResource_throwsIAE() throws Exception { + // RFC 8707 §2 requires an absolute URI, which always begins with a scheme (RFC 3986 + // §4.3). A scheme-relative identifier used to construct cleanly and then fail at every + // sink that splices the scheme — the PRM derivation on the 401 path and the DPoP htu + // binding target, both reading the missing scheme as the literal text "null". Reject at + // construction, where the operator sees the line they wrote. + AuthplaneClient client = buildClient(); + assertThatThrownBy(() -> client.resource("//api.example.com/mcp", TestFixtures.SCOPES)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("has no scheme"); + client.close(); + } + + @Test + void resourceConstructor_schemeRelativeResource_throwsIAE() throws Exception { + // Same authoritative-line reasoning as the fragment case above: every other scheme-less + // identifier enters through client.resource(...), which throws at its own gate first, so + // without this test the constructor's gate is the line no test pins. + AuthplaneClient client = buildClient(); + assertThatThrownBy( + () -> + new AuthplaneResource( + client, + "//api.example.com/mcp", + TestFixtures.SCOPES, + ResourceOptions.defaults())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("has no scheme"); + client.close(); + } + + @Test + void resource_userinfoInResource_throwsIAE() throws Exception { + // RFC 9110 §4.2.4 deprecates userinfo and directs a recipient to reject a URI carrying + // it. Here it is a disclosure, not a style question: the identifier is published verbatim + // as the PRM `resource` member (served to unauthenticated callers) and in the + // resource_metadata parameter of the 401 challenge, so the credential would be handed to + // anyone who asks. Reject at construction rather than redacting it at each sink. + AuthplaneClient client = buildClient(); + assertThatThrownBy( + () -> + client.resource( + "https://svc:s3cr3t@api.example.com/mcp", + TestFixtures.SCOPES)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a userinfo component") + // The credential is elided from the message; the host and path identify the + // configuration that has to change. + .hasMessageContaining("***@api.example.com/mcp") + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("s3cr3t")); + client.close(); + } + + @Test + void resourceConstructor_userinfoInResource_throwsIAE() throws Exception { + // Same authoritative-line reasoning as the fragment and scheme cases above: every other + // userinfo-bearing identifier enters through client.resource(...), which throws at its + // own gate first, so without this test the constructor's gate is the line no test pins. + AuthplaneClient client = buildClient(); + assertThatThrownBy( + () -> + new AuthplaneResource( + client, + "https://svc:s3cr3t@api.example.com/mcp", + TestFixtures.SCOPES, + ResourceOptions.defaults())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a userinfo component") + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("s3cr3t")); + client.close(); + } + + @Test + void resource_hostWithPort_isAccepted() throws Exception { + // A ':' in the authority is a port delimiter far more often than a userinfo one, so the + // userinfo gate must not be a bare scan for ':'. An ordinary host:port identifier still + // constructs and is published verbatim. + AuthplaneClient client = buildClient(); + AuthplaneResource verifier = + client.resource("https://api.example.com:8443/mcp", TestFixtures.SCOPES); + assertThat(verifier.prmResponse()) + .containsEntry("resource", "https://api.example.com:8443/mcp"); + client.close(); + } + + @Test + void resource_percentEncodedHashInPath_isAccepted() throws Exception { + // "%23" is a literal '#' inside the path, not a fragment delimiter (RFC 3986 §3.5), so the + // identifier is fragment-free and must survive the gate. + AuthplaneClient client = buildClient(); + AuthplaneResource verifier = + client.resource(TestFixtures.RESOURCE + "/a%23b", TestFixtures.SCOPES); + assertThat(verifier.prmResponse()) + .containsEntry("resource", TestFixtures.RESOURCE + "/a%23b"); client.close(); } @@ -280,7 +440,6 @@ void resource_nullScopes_throwsNPE() throws Exception { AuthplaneClient client = buildClient(); assertThatThrownBy(() -> client.resource(TestFixtures.RESOURCE, null)) .isInstanceOf(NullPointerException.class); - client.close(); } @Test @@ -291,7 +450,6 @@ void resource_dangerousAlgorithm_throwsIAE() throws Exception { assertThatThrownBy(() -> client.resource(TestFixtures.RESOURCE, TestFixtures.SCOPES, opts)) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("HS256"); - client.close(); } // ----------------------------------------------------------------------- @@ -318,7 +476,6 @@ void exchange_delegatesToTokenEndpoint() throws Exception { assertThat(resp.accessToken()).isEqualTo("exchanged"); assertThat(resp.expiresIn()).isEqualTo(1800); - client.close(); } @Test @@ -343,7 +500,6 @@ void exchange_oauthError_throwsTokenExchangeException() throws Exception { .isInstanceOf(ExecutionException.class) .cause() .isInstanceOf(TokenExchangeException.class); - client.close(); } @Test @@ -388,7 +544,6 @@ void exchange_sameInputs_cachesTokenAndReusesIt() throws Exception { assertThat(resp1.accessToken()).isEqualTo("first-exchange"); assertThat(resp2.accessToken()).isEqualTo("first-exchange"); wireMock.verify(1, postRequestedFor(urlEqualTo("/token"))); - client.close(); } @Test @@ -435,7 +590,6 @@ void exchange_withoutExpiresIn_usesDefaultTtlAndReusesToken() throws Exception { assertThat(resp1.accessToken()).isEqualTo("first-default-ttl"); assertThat(resp2.accessToken()).isEqualTo("first-default-ttl"); wireMock.verify(1, postRequestedFor(urlEqualTo("/token"))); - client.close(); } @Test @@ -485,7 +639,6 @@ void exchange_distinctInputs_doNotReuseCachedToken() throws Exception { assertThat(resp1.accessToken()).isEqualTo("subject-token-1-issued"); assertThat(resp2.accessToken()).isEqualTo("subject-token-2-issued"); wireMock.verify(2, postRequestedFor(urlEqualTo("/token"))); - client.close(); } // ----------------------------------------------------------------------- @@ -513,7 +666,6 @@ void clientCredentials_success() throws Exception { wireMock.verify( postRequestedFor(urlEqualTo("/token")) .withRequestBody(containing("grant_type=client_credentials"))); - client.close(); } @Test @@ -533,7 +685,6 @@ void clientCredentials_withResource_sendsResource() throws Exception { wireMock.verify( postRequestedFor(urlEqualTo("/token")) .withRequestBody(containing("resource=https%3A%2F%2Fapi.example.com"))); - client.close(); } @Test @@ -565,7 +716,6 @@ void authProvider_invokedPerRequest_appliesRotatedCredentials() throws Exception wireMock.verify( postRequestedFor(urlEqualTo("/token")) .withHeader("Authorization", equalTo("Basic second"))); - client.close(); } @Test @@ -576,7 +726,6 @@ void clientCredentials_noCredentials_throwsISE() throws Exception { .cause() .isInstanceOf(IllegalStateException.class) .hasMessageContaining("authProvider"); - client.close(); } // ----------------------------------------------------------------------- @@ -598,7 +747,6 @@ void introspect_activeTrue_returnsResponse() throws Exception { assertThat(resp.active()).isTrue(); assertThat(resp.raw()).containsEntry("sub", "user-123"); - client.close(); } @Test @@ -615,7 +763,6 @@ void introspect_activeFalse_returnsResponse() throws Exception { IntrospectionResponse resp = client.introspect("revoked-token").get(); assertThat(resp.active()).isFalse(); - client.close(); } // ----------------------------------------------------------------------- @@ -632,7 +779,6 @@ void revoke_sendsTokenToEndpoint() throws Exception { wireMock.verify( postRequestedFor(urlEqualTo("/revoke")) .withRequestBody(containing("token=token-to-revoke"))); - client.close(); } // ----------------------------------------------------------------------- @@ -736,9 +882,348 @@ void builder_nullAuthProvider_throwsNPE() { } // ----------------------------------------------------------------------- - // jwks_uri rotation via metadata change callback + // jwks_uri rotation, reconciled on the verification path // ----------------------------------------------------------------------- + /** + * A rebind that fails must be retried, not stranded. + * + *

The metadata cache publishes a refreshed document before anything acts on it, so a rebind + * driven by the document *changing* gets exactly one attempt: every later refresh returns that + * same document, and the change never fires again. One 503 at the new URI while the old one is + * withdrawn would then reject every token for the life of the process. Reconciling the binding + * against the document instead means the next key lookup simply tries again. + */ + @Test + void jwksUriRotation_transientFailureAtTheNewUri_recoversOnALaterLookup() throws Exception { + int refreshSeconds = 60; + TestFixtures.AdvanceableClock clock = new TestFixtures.AdvanceableClock(); + AuthplaneClient client = buildClientWithClock(clock, refreshSeconds); + AuthplaneResource verifier = client.resource(TestFixtures.RESOURCE, TestFixtures.SCOPES); + assertThat(verifier.verify(validToken()).get().claims().sub()) + .isEqualTo(TestFixtures.SUBJECT); + + // The AS moves its key set to /jwks2 and withdraws /jwks. /jwks2 is down. + TestFixtures.RSAKeyPair rotatedKeys = TestFixtures.generateRsaKeyPair(); + wireMock.stubFor(get(urlEqualTo("/jwks2")).willReturn(aResponse().withStatus(503))); + wireMock.stubFor(get(urlEqualTo("/jwks")).willReturn(aResponse().withStatus(404))); + stubMetadataWithJwksUri(baseUrl + "/jwks2"); + + clock.advanceSeconds(refreshSeconds + 1); + + // Both key pairs publish the same kid, so the withdrawn key set still answers the lookup + // and the token fails on the signature — the shape a stranded binding takes in production. + String rotatedToken = TestFixtures.token().rsaKey(rotatedKeys).issuer(baseUrl).build(); + assertThatThrownBy(() -> verifier.verify(rotatedToken).get()) + .isInstanceOf(ExecutionException.class); + assertThat(client.jwksCache.getUrl()).isEqualTo(baseUrl + "/jwks"); + + // The new endpoint comes up. Nothing else changes — in particular the metadata document is + // byte-identical to the one already cached, so there is no edge left for a change-triggered + // rebind to fire on. + wireMock.stubFor( + get(urlEqualTo("/jwks2")) + .willReturn( + aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody( + TestFixtures.serializeMap( + rotatedKeys.jwksDocument())))); + + // The failed attempt above put the rebind in backoff, so recovery is deferred rather than + // immediate — that is the trade for not paying an HTTP timeout on every lookup during the + // outage. The binding is still stale here, and the token still fails, without a fetch. + assertThatThrownBy(() -> verifier.verify(rotatedToken).get()) + .isInstanceOf(ExecutionException.class); + assertThat(client.jwksCache.getUrl()).isEqualTo(baseUrl + "/jwks"); + + clock.advanceSeconds(DocumentCache.failureBackoffSeconds(refreshSeconds) + 1); + + assertThat(verifier.verify(rotatedToken).get().claims().sub()) + .isEqualTo(TestFixtures.SUBJECT); + assertThat(client.jwksCache.getUrl()).isEqualTo(baseUrl + "/jwks2"); + } + + /** + * A rotated {@code jwks_uri} that is down must not cost a JWKS fetch on every verification. + * + *

Reconciling the binding against the document means the mismatch is re-detected on every + * key lookup, so without a backoff every lookup pays a full HTTP timeout for as long as the new + * endpoint stays down — on a low-QPS resource server that is every request, and the caches' own + * backoff cannot help because the factory builds a fresh {@code JwksCache} per attempt. Tokens + * whose keys are already cached never needed that fetch to succeed; they only needed it not to + * block them. + */ + @Test + void jwksUriRotation_newUriDown_retriesOnBackoffRatherThanEveryVerification() throws Exception { + int refreshSeconds = 60; + TestFixtures.AdvanceableClock clock = new TestFixtures.AdvanceableClock(); + AuthplaneClient client = buildClientWithClock(clock, refreshSeconds); + AuthplaneResource verifier = client.resource(TestFixtures.RESOURCE, TestFixtures.SCOPES); + assertThat(verifier.verify(validToken()).get().claims().sub()) + .isEqualTo(TestFixtures.SUBJECT); + + // The AS moves its key set, and the new endpoint is down. The old one keeps serving, so + // every key these tokens need is already cached and verification is never in danger — the + // rebind is the only thing failing. + wireMock.stubFor(get(urlEqualTo("/jwks2")).willReturn(aResponse().withStatus(500))); + stubMetadataWithJwksUri(baseUrl + "/jwks2"); + clock.advanceSeconds(refreshSeconds + 1); + + assertThat(verifier.verify(validToken()).get().claims().sub()) + .isEqualTo(TestFixtures.SUBJECT); + // One rebind attempt. Counted rather than asserted as a literal: the SSRF-safe fetcher + // walks every address `localhost` resolves to, so a single attempt is more than one + // request here. What the finding is about is whether this number grows per verification. + int requestsAfterFirstAttempt = + wireMock.findAll(getRequestedFor(urlEqualTo("/jwks2"))).size(); + assertThat(requestsAfterFirstAttempt).isGreaterThan(0); + + for (int i = 0; i < 5; i++) { + assertThat(verifier.verify(validToken()).get().claims().sub()) + .isEqualTo(TestFixtures.SUBJECT); + } + + // Unchanged: five more verifications cost nothing. Before the backoff each one paid a full + // JWKS fetch at the dead endpoint, on the thread the caller is blocked on. + assertThat(wireMock.findAll(getRequestedFor(urlEqualTo("/jwks2"))).size()) + .isEqualTo(requestsAfterFirstAttempt); + assertThat(client.jwksCache.getUrl()).isEqualTo(baseUrl + "/jwks"); + + // The mismatch persists, so it is still retried — on the backoff rather than on every + // lookup. This is the reconcile property the previous round established, unchanged. + clock.advanceSeconds(DocumentCache.failureBackoffSeconds(refreshSeconds) + 1); + assertThat(verifier.verify(validToken()).get().claims().sub()) + .isEqualTo(TestFixtures.SUBJECT); + assertThat(wireMock.findAll(getRequestedFor(urlEqualTo("/jwks2"))).size()) + .isEqualTo(requestsAfterFirstAttempt * 2); + } + + /** + * The kid-miss path is the one a rotation puts every token on, and it reaches the cache through + * {@code forceRefresh()} rather than {@code get()}. The rotation test above cannot see this: it + * reuses one signing key throughout, so every key its tokens need is already cached and {@code + * find(kid, true)} is never called. This one drives the miss directly. + * + *

Without the backoff inside {@code forceRefresh}, each verification below costs a full + * fetch against a failing endpoint, on the caller's thread and serialized behind the cache's + * fetch lock — and an unauthenticated caller sending unknown kids drives that rate. + */ + @Test + void unknownKid_whileJwksIsFailing_doesNotFetchOnEveryVerification() throws Exception { + int refreshSeconds = 60; + TestFixtures.AdvanceableClock clock = new TestFixtures.AdvanceableClock(); + AuthplaneClient client = buildClientWithClock(clock, refreshSeconds); + AuthplaneResource verifier = client.resource(TestFixtures.RESOURCE, TestFixtures.SCOPES); + assertThat(verifier.verify(validToken()).get().claims().sub()) + .isEqualTo(TestFixtures.SUBJECT); + + // The JWKS endpoint starts failing, and tokens arrive naming a kid the cached document does + // not carry — the state a key rotation leaves behind. + wireMock.stubFor(get(urlEqualTo("/jwks")).willReturn(aResponse().withStatus(500))); + int before = wireMock.findAll(getRequestedFor(urlEqualTo("/jwks"))).size(); + + String unknownKidToken = + TestFixtures.token().rsaKey(rsaKeys).issuer(baseUrl).kid("rotated-key-2").build(); + + assertThatThrownBy(() -> verifier.verify(unknownKidToken).get()).isNotNull(); + int afterFirstMiss = wireMock.findAll(getRequestedFor(urlEqualTo("/jwks"))).size(); + assertThat(afterFirstMiss).isGreaterThan(before); + + // Five more misses cost nothing: the failed forced refresh is backing off like any other. + for (int i = 0; i < 5; i++) { + assertThatThrownBy(() -> verifier.verify(unknownKidToken).get()).isNotNull(); + } + assertThat(wireMock.findAll(getRequestedFor(urlEqualTo("/jwks"))).size()) + .isEqualTo(afterFirstMiss); + + // Still retried once the backoff elapses — throttled, not abandoned. + clock.advanceSeconds(DocumentCache.failureBackoffSeconds(refreshSeconds) + 1); + assertThatThrownBy(() -> verifier.verify(unknownKidToken).get()).isNotNull(); + assertThat(wireMock.findAll(getRequestedFor(urlEqualTo("/jwks"))).size()) + .isGreaterThan(afterFirstMiss); + } + + /** + * A refresh that returns an invalid document must not displace the good one, and — because the + * document is what key retrieval is reconciled against — must not be able to repoint it either. + */ + @Test + void metadataRefresh_invalidDocument_keepsTheGoodDocumentAndTheJwksBinding() throws Exception { + int refreshSeconds = 60; + TestFixtures.AdvanceableClock clock = new TestFixtures.AdvanceableClock(); + AuthplaneClient client = buildClientWithClock(clock, refreshSeconds); + AuthplaneResource verifier = client.resource(TestFixtures.RESOURCE, TestFixtures.SCOPES); + assertThat(verifier.verify(validToken()).get().claims().sub()) + .isEqualTo(TestFixtures.SUBJECT); + + // The endpoint starts answering for a different issuer, pointing jwks_uri at a key set the + // SDK must never fetch (RFC 8414 §3.3). + TestFixtures.RSAKeyPair foreignKeys = TestFixtures.generateRsaKeyPair(); + wireMock.stubFor( + get(urlEqualTo("/jwks-foreign")) + .willReturn( + aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody( + TestFixtures.serializeMap( + foreignKeys.jwksDocument())))); + wireMock.stubFor( + get(urlEqualTo(WELL_KNOWN_PATH)) + .willReturn( + aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody( + TestFixtures.serializeMap( + Map.of( + "issuer", + "https://evil.example.com", + "jwks_uri", + baseUrl + "/jwks-foreign"))))); + + clock.advanceSeconds(refreshSeconds + 1); + + assertThat(verifier.verify(validToken()).get().claims().sub()) + .isEqualTo(TestFixtures.SUBJECT); + assertThat(client.jwksCache.getUrl()).isEqualTo(baseUrl + "/jwks"); + assertThat(requestCount("/jwks-foreign")) + .as("a document that failed validation must not repoint key retrieval") + .isZero(); + } + + /** + * An unreachable metadata endpoint costs neither a failed verification nor a network round trip + * per verification. {@code doFetch} advances the cache timestamp only on success, so without a + * retry backoff the document stays permanently expired and every lookup pays a full HTTP + * timeout — on a request path, behind an exclusive lock. + */ + @Test + void metadataEndpointDown_verificationKeepsWorkingAndTheRefreshBacksOff() throws Exception { + int refreshSeconds = 60; + TestFixtures.AdvanceableClock clock = new TestFixtures.AdvanceableClock(); + AuthplaneClient client = buildClientWithClock(clock, refreshSeconds); + AuthplaneResource verifier = client.resource(TestFixtures.RESOURCE, TestFixtures.SCOPES); + verifier.verify(validToken()).get(); + + wireMock.stubFor(get(urlEqualTo(WELL_KNOWN_PATH)).willReturn(aResponse().withStatus(500))); + clock.advanceSeconds(refreshSeconds + 1); + + int readsBeforeOutage = requestCount(WELL_KNOWN_PATH); + assertThat(verifier.verify(validToken()).get().claims().sub()) + .as("keys the JWKS cache already holds must still verify") + .isEqualTo(TestFixtures.SUBJECT); + int readsAfterOneAttempt = requestCount(WELL_KNOWN_PATH); + assertThat(readsAfterOneAttempt) + .as("the refresh was attempted") + .isGreaterThan(readsBeforeOutage); + + for (int i = 0; i < 4; i++) { + assertThat(verifier.verify(validToken()).get().claims().sub()) + .isEqualTo(TestFixtures.SUBJECT); + } + assertThat(requestCount(WELL_KNOWN_PATH)) + .as("a failed refresh must back off, not retry on every verification") + .isEqualTo(readsAfterOneAttempt); + + clock.advanceSeconds(31); // past the backoff + verifier.verify(validToken()).get(); + assertThat(requestCount(WELL_KNOWN_PATH)) + .as("the retry resumes once the backoff elapses") + .isGreaterThan(readsAfterOneAttempt); + } + + /** + * The two call sites of {@code isInterrupt} see the same interrupt differently — {@code + * refreshMetadataIfDue} gets it wrapped, because MetadataCache wraps anything that is not a + * MetadataFetchException, while {@code rebindJwksIfMoved} gets it bare. Both must restore the + * flag, so both shapes have to be recognised. + */ + @Test + void isInterrupt_recognisesTheInterruptBareAndWrapped() { + assertThat(AuthplaneClient.isInterrupt(new InterruptedException("bare"))).isTrue(); + assertThat(AuthplaneClient.isInterrupt(new RuntimeException(new InterruptedException()))) + .isTrue(); + assertThat( + AuthplaneClient.isInterrupt( + new IllegalStateException( + new RuntimeException(new InterruptedException())))) + .isTrue(); + + assertThat(AuthplaneClient.isInterrupt(new RuntimeException("boom"))).isFalse(); + assertThat(AuthplaneClient.isInterrupt(new RuntimeException(new java.io.IOException()))) + .isFalse(); + } + + /** + * A cause chain can be made cyclic without reflection: {@code initCause} refuses only a + * self-reference and a cause that is already set, so A constructed with cause B, then B given + * cause A, closes the loop. {@code getCause() == t} does not catch that shape — the walk is + * bounded so it terminates anyway — without the bound the walk never reaches a null cause on + * this shape, so it would not fail here, it would not return. + */ + @Test + void isInterrupt_terminatesOnACauseCycle() { + RuntimeException b = new RuntimeException("b"); // cause deliberately left unset + RuntimeException a = new RuntimeException("a", b); + b.initCause(a); + + assertThat(b.getCause()).isSameAs(a); + assertThat(a.getCause()).isSameAs(b); + assertThat(AuthplaneClient.isInterrupt(a)).isFalse(); + + // An interrupt reachable *inside* a cycle is still found, before the bound is hit. The + // loop has to close through the interrupt itself for this to be a cycle at all: walking + // from `outer` reaches the InterruptedException on the third hop, and the fourth would + // return to `outer` if the walk kept going. + InterruptedException interrupt = new InterruptedException(); + RuntimeException inner = new RuntimeException("inner", interrupt); + RuntimeException outer = new RuntimeException("outer", inner); + interrupt.initCause(outer); + + assertThat(interrupt.getCause()).isSameAs(outer); + assertThat(AuthplaneClient.isInterrupt(outer)).isTrue(); + } + + /** + * Sets both refresh intervals to the same value. + * + *

Only the metadata one used to be set, which left the rebind backoff — computed from the + * JWKS interval — reading a knob the test never named. The two agreed at 60 only because both + * exceed the 30 s ceiling, so the tests below would have kept passing while measuring the wrong + * thing, and stopped agreeing at any interval under 30. + */ + private AuthplaneClient buildClientWithClock(Clock clock, int refreshSeconds) throws Exception { + return register( + AuthplaneClient.builder(baseUrl) + .devMode(true) + .metadataRefreshSeconds(refreshSeconds) + .jwksRefreshSeconds(refreshSeconds) + .clock(clock) + .build() + .get()); + } + + private void stubMetadataWithJwksUri(String jwksUri) { + wireMock.stubFor( + get(urlEqualTo(WELL_KNOWN_PATH)) + .willReturn( + aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody( + TestFixtures.serializeMap( + Map.of( + "issuer", baseUrl, + "jwks_uri", jwksUri))))); + } + + private static int requestCount(String path) { + return wireMock.countRequestsMatching(getRequestedFor(urlEqualTo(path)).build()).getCount(); + } + @Test void jwksUriRotation_updatesJwksCache() throws Exception { // Build client with initial metadata @@ -785,8 +1270,6 @@ void jwksUriRotation_updatesJwksCache() throws Exception { AuthplaneResource verifier2 = client.resource(TestFixtures.RESOURCE, TestFixtures.SCOPES); VerifiedClaims rotatedClaims = verifier2.verify(rotatedToken).get().claims(); assertThat(rotatedClaims.sub()).isEqualTo(TestFixtures.SUBJECT); - - client.close(); } // ----------------------------------------------------------------------- @@ -834,8 +1317,6 @@ void circuitBreaker_opensAfterFailures_thenRejectsRequests() throws Exception { .cause() .isInstanceOf(TokenExchangeException.class) .hasMessageContaining("Circuit breaker"); - - client.close(); } @Test @@ -867,8 +1348,6 @@ void circuitBreaker_doesNotOpenOnRepeatedInvalidScope() throws Exception { } assertThat(client.circuitBreaker.state()).isNotEqualTo(CircuitBreaker.State.OPEN); - - client.close(); } // ----------------------------------------------------------------------- @@ -907,8 +1386,6 @@ void clientCredentials_cachesTokenAndReusesIt() throws Exception { // Second call with same scope should return cached token TokenResponse resp2 = client.clientCredentials(List.of("read"), List.of()).get(); assertThat(resp2.accessToken()).isEqualTo("cached"); - - client.close(); } // ----------------------------------------------------------------------- @@ -959,7 +1436,6 @@ void clientCredentials_concurrentColdMisses_deduplicateToSingleCall() throws Exc // Only one POST should have been made wireMock.verify(1, postRequestedFor(urlEqualTo("/token"))); - client.close(); } @Test @@ -1002,7 +1478,6 @@ void clientCredentials_afterInflightCompletes_secondCallUsesCacheNotEndpoint() // Only one POST wireMock.verify(1, postRequestedFor(urlEqualTo("/token"))); - client.close(); } // ----------------------------------------------------------------------- @@ -1024,7 +1499,6 @@ void builder_settersAreChainable() throws Exception { .get(); assertThat(client.issuer()).isEqualTo(baseUrl); - client.close(); } private static Throwable rootCause(Throwable throwable) { diff --git a/core/src/test/java/ai/authplane/sdk/core/AuthplaneResourceTest.java b/core/src/test/java/ai/authplane/sdk/core/AuthplaneResourceTest.java index a3aa3f7..3373a69 100644 --- a/core/src/test/java/ai/authplane/sdk/core/AuthplaneResourceTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/AuthplaneResourceTest.java @@ -313,6 +313,25 @@ void prmUrl_resourceWithPathSuffix_appendsSuffix() throws Exception { .isEqualTo("https://mcp.example.com/.well-known/oauth-protected-resource/mcp"); } + @Test + void prmUrl_resourceWithQuery_preservesQuery() throws Exception { + // The query is part of the resource identifier; RFC 9728 §3 places the well-known + // string between the host and "the path and/or query components, if any", so the + // challenge's resource_metadata URL carries the query through. + resource = createResource("https://api.example.com/mcp?tenant=a"); + assertThat(resource.prmUrl()) + .isEqualTo( + "https://api.example.com/.well-known/oauth-protected-resource/mcp?tenant=a"); + } + + @Test + void prmPath_resourceWithQuery_staysPathKeyed() throws Exception { + // Routing is path-keyed: the query appears only in prmUrl(). The single route serves + // the document for every query value. + resource = createResource("https://api.example.com/mcp?tenant=a"); + assertThat(resource.prmPath()).isEqualTo("/.well-known/oauth-protected-resource/mcp"); + } + @Test void normalizeRequestUrl_substitutesResourceHost_keepsRequestPath() throws Exception { resource = createResource("https://api.example.com/mcp"); @@ -330,6 +349,18 @@ void normalizeRequestUrl_keepsResourceSubPath() throws Exception { .isEqualTo("https://my.mcp.org/mcp1/tool"); } + @Test + void normalizeRequestUrl_preservesTheRawAuthority() throws Exception { + // Same rule the PRM derivation follows: getAuthority() percent-decodes, so an escape in + // the authority used to produce an htu naming the decoded form — an authority + // structurally different from the one the identifier names, which the client's proof + // never matches. The escape sits in the registered name (RFC 3986 §3.2.2), not in + // userinfo: userinfo no longer constructs at all. + resource = createResource("https://a%2Db.example.com/mcp"); + assertThat(resource.normalizeRequestUrl("http://10.0.0.5:8080/mcp")) + .isEqualTo("https://a%2Db.example.com/mcp"); + } + @Test void normalizeRequestUrl_ignoresQuery() throws Exception { resource = createResource("https://api.example.com/mcp"); diff --git a/core/src/test/java/ai/authplane/sdk/core/TestFixtures.java b/core/src/test/java/ai/authplane/sdk/core/TestFixtures.java index fde3dd0..3053906 100644 --- a/core/src/test/java/ai/authplane/sdk/core/TestFixtures.java +++ b/core/src/test/java/ai/authplane/sdk/core/TestFixtures.java @@ -1,12 +1,16 @@ package ai.authplane.sdk.core; +import java.time.Clock; import java.time.Instant; +import java.time.ZoneId; +import java.time.ZoneOffset; import java.util.ArrayList; import java.util.Date; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.concurrent.atomic.AtomicLong; import com.nimbusds.jose.JOSEObjectType; import com.nimbusds.jose.JWSAlgorithm; @@ -321,6 +325,80 @@ public static TokenBuilder token() { return new TokenBuilder(); } + // ----------------------------------------------------------------------- + // Client construction with a driven clock + // ----------------------------------------------------------------------- + + /** + * Builds a dev-mode client whose caches read time from {@code clock}, so cache TTL expiry can + * be driven by advancing the clock instead of sleeping against wall time. + * + *

Lives here because {@link AuthplaneClientBuilder#clock(Clock)} is package-private and this + * class shares its package. Everything else about the returned client is ordinary production + * configuration — tests built this way exercise the real request path, with no test-only + * trigger to reach the behaviour under test. + * + * @param issuer authorization server issuer identifier + * @param clock time source for the metadata and JWKS caches + * @param metadataRefreshSeconds metadata refresh interval to configure + */ + public static AuthplaneClient clientWithClock( + String issuer, Clock clock, int metadataRefreshSeconds) throws Exception { + return AuthplaneClient.builder(issuer) + .devMode(true) + .metadataRefreshSeconds(metadataRefreshSeconds) + .clock(clock) + .build() + .get(); + } + + /** + * Clock advanced by hand, so a test states the elapsed time it wants instead of waiting for it. + */ + public static final class AdvanceableClock extends Clock { + + /** Arbitrary fixed start time; only the deltas from it matter. */ + private static final long START_EPOCH_SECONDS = 1_700_000_000L; + + private final AtomicLong nowSeconds; + private final ZoneId zone; + + public AdvanceableClock() { + this(new AtomicLong(START_EPOCH_SECONDS), ZoneOffset.UTC); + } + + private AdvanceableClock(AtomicLong nowSeconds, ZoneId zone) { + this.nowSeconds = nowSeconds; + this.zone = zone; + } + + @Override + public ZoneId getZone() { + return zone; + } + + /** + * Honours the requested zone, as {@link Clock} requires. Returning {@code this} regardless + * is harmless only until something composes this clock — {@code Clock.fixed} and {@code + * Clock.offset} both go through here — at which point the zone would be silently dropped. + * The returned view shares the same instant, so advancing either advances both. + */ + @Override + public Clock withZone(ZoneId zone) { + return zone.equals(this.zone) ? this : new AdvanceableClock(nowSeconds, zone); + } + + @Override + public Instant instant() { + return Instant.ofEpochSecond(nowSeconds.get()); + } + + /** Moves the clock forward by the given number of seconds. */ + public void advanceSeconds(long seconds) { + nowSeconds.addAndGet(seconds); + } + } + // ----------------------------------------------------------------------- // JWKS JSON serialization (for WireMock stubs) // ----------------------------------------------------------------------- diff --git a/core/src/test/java/ai/authplane/sdk/core/errors/ErrorsTest.java b/core/src/test/java/ai/authplane/sdk/core/errors/ErrorsTest.java index 50ff838..859aea6 100644 --- a/core/src/test/java/ai/authplane/sdk/core/errors/ErrorsTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/errors/ErrorsTest.java @@ -305,6 +305,23 @@ void wwwAuthenticate_challengeOptions_escapesResourceMetadataAndScope() { .contains("resource_metadata=\"https://api.example.com/prmX-Injected: 1\""); } + @Test + void wwwAuthenticate_resourceMetadataWithQuery_isCarriedVerbatim() { + // prmUrl() carries the resource identifier's query into the PRM URL (RFC 9728 §3). The + // sanitiser must pass '?', '&' and '=' through untouched: they are legal inside an HTTP + // quoted-string, and only control characters, '"' and '\' are rewritten. + var ex = new TokenExpiredException("expired"); + String header = + WwwAuthenticate.of( + ex, + WwwAuthenticate.ChallengeOptions.empty() + .withResourceMetadataUrl( + "https://api.example.com/.well-known/oauth-protected-resource/mcp?tenant=a&x=1")); + assertThat(header) + .contains( + "resource_metadata=\"https://api.example.com/.well-known/oauth-protected-resource/mcp?tenant=a&x=1\""); + } + @Test void wwwAuthenticate_challengeOptions_nullArgs_throw() { assertThat(WwwAuthenticate.ChallengeOptions.empty().scope()).isEmpty(); diff --git a/core/src/test/java/ai/authplane/sdk/core/fetching/CacheHeaderParserTest.java b/core/src/test/java/ai/authplane/sdk/core/fetching/CacheHeaderParserTest.java index d5739ef..cb55de2 100644 --- a/core/src/test/java/ai/authplane/sdk/core/fetching/CacheHeaderParserTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/fetching/CacheHeaderParserTest.java @@ -31,30 +31,32 @@ void parseExpiresAt_unrelatedHeaders_returnsNull() { // Cache-Control: no-store / no-cache // ----------------------------------------------------------------------- + // no-store / no-cache mean "no usable preference", reported as null — not an expiry at the + // epoch. The 0L these used to return was read by DocumentCache as an absolute expiry, which + // made the document permanently stale and cost a synchronous fetch on every read. + @Test - void parseExpiresAt_noStore_returnsZero() { - Long result = CacheHeaderParser.parseExpiresAt(Map.of("cache-control", "no-store")); - assertThat(result).isEqualTo(0L); + void parseExpiresAt_noStore_returnsNull() { + assertThat(CacheHeaderParser.parseExpiresAt(Map.of("cache-control", "no-store"))).isNull(); } @Test - void parseExpiresAt_noCache_returnsZero() { - Long result = CacheHeaderParser.parseExpiresAt(Map.of("cache-control", "no-cache")); - assertThat(result).isEqualTo(0L); + void parseExpiresAt_noCache_returnsNull() { + assertThat(CacheHeaderParser.parseExpiresAt(Map.of("cache-control", "no-cache"))).isNull(); } @Test - void parseExpiresAt_noCacheUpperCase_returnsZero() { - Long result = CacheHeaderParser.parseExpiresAt(Map.of("cache-control", "NO-CACHE")); - assertThat(result).isEqualTo(0L); + void parseExpiresAt_noCacheUpperCase_returnsNull() { + assertThat(CacheHeaderParser.parseExpiresAt(Map.of("cache-control", "NO-CACHE"))).isNull(); } + /** no-store wins over a max-age in the same header: it is the stronger directive. */ @Test - void parseExpiresAt_noStoreWithOtherDirectives_returnsZero() { - Long result = - CacheHeaderParser.parseExpiresAt( - Map.of("cache-control", "public, no-store, max-age=300")); - assertThat(result).isEqualTo(0L); + void parseExpiresAt_noStoreWithOtherDirectives_returnsNull() { + assertThat( + CacheHeaderParser.parseExpiresAt( + Map.of("cache-control", "public, no-store, max-age=300"))) + .isNull(); } // ----------------------------------------------------------------------- diff --git a/core/src/test/java/ai/authplane/sdk/core/fetching/DocumentCacheTest.java b/core/src/test/java/ai/authplane/sdk/core/fetching/DocumentCacheTest.java index 5dc970a..1c8a30e 100644 --- a/core/src/test/java/ai/authplane/sdk/core/fetching/DocumentCacheTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/fetching/DocumentCacheTest.java @@ -10,11 +10,14 @@ import java.util.Map; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import java.util.function.BiConsumer; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; class DocumentCacheTest { @@ -133,6 +136,103 @@ void get_triggersBackgroundRefreshAt80PercentTtl() throws Exception { assertThat(fetchCount.get()).isEqualTo(2); } + /** + * A non-positive refresh interval reaches the same permanent-expiry state the server-expiry + * clamp was added for, through the other parameter. `AuthplaneClientBuilder` rejects it, but + * `JwksCache` and `MetadataCache` expose these constructors publicly, so the builder's check + * does not cover a caller that builds a cache directly. + */ + @Test + void constructor_rejectsANonPositiveRefreshInterval() { + TestClock clock = new TestClock(); + DocumentFetcher fetcher = + url -> CompletableFuture.completedFuture(new FetchResult(DOC_V1, null)); + + for (int interval : new int[] {0, -1}) { + assertThatThrownBy(() -> cacheWith(fetcher, interval, clock)) + .as("interval %s must be refused at construction", interval) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must be positive"); + } + } + + @Test + void constructor_rejectsANullClock() { + DocumentFetcher fetcher = + url -> CompletableFuture.completedFuture(new FetchResult(DOC_V1, null)); + assertThatThrownBy(() -> cacheWith(fetcher, 300, null)) + .isInstanceOf(NullPointerException.class) + .hasMessageContaining("clock"); + } + + /** + * A server expiry that is not in the future is no expiry at all. + * + *

`Cache-Control: no-store` and `no-cache` parse to `0L`, `max-age=0` to `now`, and a stale + * `Expires:` to a past epoch. Subtracting the cache timestamp from any of those gives a + * negative TTL, which makes the document permanently expired: every read takes the synchronous + * re-fetch branch, on the caller's thread. The failure backoff cannot help, because a + * `no-store` endpoint that *answers* clears it and re-arms the expiry on the same call. + * + *

This matters now that verification reads through the metadata cache on every key lookup — + * and does so before signature verification, so an unauthenticated caller would set the fetch + * rate against the authorization server. + */ + @Test + void get_serverExpiryNotInTheFuture_fallsBackToTheConfiguredInterval() throws Exception { + // 0L is what no-store and no-cache parse to; -1 stands for a stale Expires: header. + for (long serverExpiry : new long[] {0L, -1L}) { + AtomicInteger fetchCount = new AtomicInteger(); + TestClock clock = new TestClock(); + DocumentFetcher fetcher = + url -> { + fetchCount.incrementAndGet(); + return CompletableFuture.completedFuture( + new FetchResult(DOC_V1, serverExpiry)); + }; + cache = cacheWith(fetcher, 300, clock); + cache.fetch(); + assertThat(fetchCount.get()).isEqualTo(1); + + for (int i = 0; i < 5; i++) { + assertThat(cache.get()).isEqualTo(DOC_V1); + } + assertThat(fetchCount.get()) + .as( + "server expiry %s must not make the document permanently expired", + serverExpiry) + .isEqualTo(1); + + clock.advanceSeconds(301); + cache.get(); + assertThat(fetchCount.get()) + .as("the configured interval still governs for server expiry %s", serverExpiry) + .isEqualTo(2); + } + } + + /** + * A server expiry exactly at the cache timestamp is the max-age=0 case, and behaves the same. + */ + @Test + void get_serverExpiryEqualToCachedAt_fallsBackToTheConfiguredInterval() throws Exception { + AtomicInteger fetchCount = new AtomicInteger(); + TestClock clock = new TestClock(); + DocumentFetcher fetcher = + url -> { + fetchCount.incrementAndGet(); + return CompletableFuture.completedFuture( + new FetchResult(DOC_V1, clock.instant().getEpochSecond())); + }; + cache = cacheWith(fetcher, 300, clock); + cache.fetch(); + + for (int i = 0; i < 5; i++) { + cache.get(); + } + assertThat(fetchCount.get()).as("max-age=0 must not cost a fetch per read").isEqualTo(1); + } + @Test void get_serverExpiresTtl_usesMinOfConfiguredAndServer() throws Exception { // Server says the document expires 10s from now; the configured TTL is 300s. The @@ -196,6 +296,68 @@ void advanceSeconds(long seconds) { } } + // A regression here blocks rather than returning the wrong value, and in CI a hang is not + // the same as a failure: it burns the job's wall clock and surfaces as a build timeout + // instead of a named test. The bound turns it back into a failure that says what broke. + @Test + @Timeout(10) + void get_whileARefreshIsInFlight_servesTheCurrentCopyInsteadOfQueueing() throws Exception { + // The lock-free read is a semantic change to a public method, not a performance tweak: + // once a refresh is in flight, get() returns the published document without honouring + // expiry. That is deliberate — on the verification path the alternative is every caller + // queueing behind one network round trip — but it is only exercised when fetchLock is + // actually contended, which no single-threaded test does. + CountDownLatch fetchStarted = new CountDownLatch(1); + CountDownLatch releaseFetch = new CountDownLatch(1); + AtomicInteger fetchCount = new AtomicInteger(); + TestClock clock = new TestClock(); + DocumentFetcher fetcher = + url -> + CompletableFuture.supplyAsync( + () -> { + if (fetchCount.incrementAndGet() > 1) { + fetchStarted.countDown(); + try { + releaseFetch.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new CompletionException(e); + } + return new FetchResult(DOC_V2, null); + } + return new FetchResult(DOC_V1, null); + }); + cache = cacheWith(fetcher, 100, clock); + cache.fetch(); + + clock.advanceSeconds(101); // expired, so the refresher below takes the synchronous branch + + Thread refresher = + new Thread( + () -> { + try { + cache.get(); + } catch (Exception e) { + throw new IllegalStateException(e); + } + }); + refresher.start(); + assertThat(fetchStarted.await(5, TimeUnit.SECONDS)) + .as("the refresh reached the fetcher and is holding fetchLock") + .isTrue(); + + // Returns while the refresh is still blocked — asserted by ordering rather than by a + // timeout: releaseFetch has not been counted down yet, so a get() that queued behind the + // lock could not have returned at all. + assertThat(cache.get()).isEqualTo(DOC_V1); + assertThat(fetchCount.get()).as("no second fetch was started").isEqualTo(2); + + releaseFetch.countDown(); + refresher.join(5_000); + assertThat(refresher.isAlive()).isFalse(); + assertThat(cache.get()).isEqualTo(DOC_V2); + } + private static DocumentCache cacheWith(DocumentFetcher fetcher, int ttl) { return new DocumentCache(fetcher, "https://example.com/jwks", ttl, "JWKS", null); } diff --git a/core/src/test/java/ai/authplane/sdk/core/fetching/MetadataCacheTest.java b/core/src/test/java/ai/authplane/sdk/core/fetching/MetadataCacheTest.java index 677c581..a4435f9 100644 --- a/core/src/test/java/ai/authplane/sdk/core/fetching/MetadataCacheTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/fetching/MetadataCacheTest.java @@ -5,9 +5,11 @@ import java.util.Map; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.Test; +import ai.authplane.sdk.core.TestFixtures; import ai.authplane.sdk.core.errors.MetadataFetchException; /** @@ -217,4 +219,148 @@ void getJwksUri_fetcherThrowsRuntime_wrappedInMetadataFetchException() { assertThatThrownBy(cache::getJwksUri).isInstanceOf(MetadataFetchException.class); } + + // ----------------------------------------------------------------------- + // Validation at fetch time rather than read time + // + // The cases above all reject on the *first* fetch, which passed whether validation ran before + // or after the document was published. What follows is the part that only fetch-time + // validation gets right: a refresh that returns an invalid document must leave the good one in + // place and must not reach the change callback, which is what rebinds key retrieval. + // ----------------------------------------------------------------------- + + @Test + void refreshWithWrongIssuer_keepsServingTheLastValidDocument() throws Exception { + AtomicInteger fetches = new AtomicInteger(); + AtomicInteger callbackCalls = new AtomicInteger(); + TestFixtures.AdvanceableClock clock = new TestFixtures.AdvanceableClock(); + DocumentFetcher fetcher = + url -> + CompletableFuture.completedFuture( + new FetchResult( + fetches.incrementAndGet() == 1 + ? Map.of( + "issuer", + ISSUER, + "jwks_uri", + ISSUER + "/jwks") + : Map.of( + "issuer", "https://evil.example.com", + "jwks_uri", + "https://evil.example.com/jwks"), + null)); + MetadataCache cache = + new MetadataCache( + fetcher, + ISSUER + "/.well-known/oauth-authorization-server", + 100, + ISSUER, + false, + (old, next) -> callbackCalls.incrementAndGet(), + clock); + cache.fetch(); + + clock.advanceSeconds(101); // past the TTL, so the read below re-fetches + + assertThat(cache.getJwksUri()) + .as("the rejected refresh must not displace the good document") + .isEqualTo(ISSUER + "/jwks"); + assertThat(fetches.get()).as("the refresh was attempted").isEqualTo(2); + assertThat(callbackCalls.get()) + .as("a document that failed validation must not reach the change callback") + .isZero(); + } + + @Test + void refreshWithoutJwksUri_keepsServingTheLastValidDocument() throws Exception { + // Same shape as the wrong-issuer case, on the one field the refresh mechanism itself runs + // on. Presence used to be checked at read time, so a document without jwks_uri passed + // validation, was published, and displaced the good one — after which + // AuthplaneClient.refreshMetadataIfDue had nothing left to reconcile the binding against + // and every verification raised the failure again, on the request path, with nothing to + // rate-limit it: the fetch had succeeded, so no backoff applied. + AtomicInteger fetches = new AtomicInteger(); + AtomicInteger callbackCalls = new AtomicInteger(); + TestFixtures.AdvanceableClock clock = new TestFixtures.AdvanceableClock(); + DocumentFetcher fetcher = + url -> + CompletableFuture.completedFuture( + new FetchResult( + fetches.incrementAndGet() == 1 + ? Map.of( + "issuer", + ISSUER, + "jwks_uri", + ISSUER + "/jwks") + : Map.of("issuer", ISSUER), + null)); + MetadataCache cache = + new MetadataCache( + fetcher, + ISSUER + "/.well-known/oauth-authorization-server", + 100, + ISSUER, + false, + (old, next) -> callbackCalls.incrementAndGet(), + clock); + cache.fetch(); + + clock.advanceSeconds(101); // past the TTL, so the read below re-fetches + + assertThat(cache.getJwksUri()) + .as("the rejected refresh must not displace the good document") + .isEqualTo(ISSUER + "/jwks"); + assertThat(fetches.get()).as("the refresh was attempted").isEqualTo(2); + assertThat(callbackCalls.get()) + .as("a document that failed validation must not reach the change callback") + .isZero(); + + // And the rejection is now a failed refresh, so it backs off like one rather than being + // re-raised on every read. + assertThat(cache.getJwksUri()).isEqualTo(ISSUER + "/jwks"); + assertThat(fetches.get()).as("the rejected refresh backs off").isEqualTo(2); + } + + @Test + void refreshFailure_isNotRetriedOnEveryRead() throws Exception { + AtomicInteger fetches = new AtomicInteger(); + TestFixtures.AdvanceableClock clock = new TestFixtures.AdvanceableClock(); + DocumentFetcher fetcher = + url -> + fetches.incrementAndGet() == 1 + ? CompletableFuture.completedFuture( + new FetchResult( + Map.of( + "issuer", + ISSUER, + "jwks_uri", + ISSUER + "/jwks"), + null)) + : CompletableFuture.failedFuture(new RuntimeException("down")); + MetadataCache cache = + new MetadataCache( + fetcher, + ISSUER + "/.well-known/oauth-authorization-server", + 100, + ISSUER, + false, + null, + clock); + cache.fetch(); + + clock.advanceSeconds(101); + + for (int i = 0; i < 5; i++) { + assertThat(cache.getJwksUri()).isEqualTo(ISSUER + "/jwks"); + } + assertThat(fetches.get()) + .as("a failed refresh backs off instead of retrying on every read") + .isEqualTo(2); + + clock.advanceSeconds(31); // past the backoff + assertThat(cache.getJwksUri()).isEqualTo(ISSUER + "/jwks"); + assertThat(fetches.get()).as("the retry resumes once the backoff elapses").isEqualTo(3); + } + + /** Manually advanced clock, so TTL expiry is driven rather than waited on. */ } diff --git a/core/src/test/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadataTest.java b/core/src/test/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadataTest.java index 486aebc..b06363b 100644 --- a/core/src/test/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadataTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadataTest.java @@ -47,6 +47,239 @@ void wellKnownPath_rootResourceWithTrailingSlash() { .isEqualTo("/.well-known/oauth-protected-resource"); } + @Test + void builder_rejectsFragmentInResource() { + // RFC 8707 §2: "The URI MUST NOT include a fragment component." The builder stores the + // identifier verbatim into the document's "resource" field while the well-known URL is + // derived from a java.net.URI that has already dropped the fragment — the two would name + // different identifiers, and RFC 9728 §3.3 makes a conformant client discard such a + // document. There is no server-side error in that failure, so it must be rejected here. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.builder() + .resource("https://api.example.com/mcp#section") + .authorizationServer("https://auth.example.com") + .build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a fragment component") + .hasMessageContaining("https://api.example.com/mcp"); + } + + @Test + void builder_rejectsEmptyFragmentInResource() { + // A bare trailing "#" is still a fragment component, and it is dropped just as silently. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.builder() + .resource("https://api.example.com/mcp#") + .authorizationServer("https://auth.example.com") + .build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a fragment component"); + } + + @Test + void builder_acceptsPercentEncodedHash() { + // "%23" is a literal '#' in the path, not a fragment delimiter (RFC 3986 §3.5). The gate + // scans for the raw character precisely so an encoded octet is not mistaken for one. + var prm = + ProtectedResourceMetadata.builder() + .resource("https://api.example.com/a%23b") + .authorizationServer("https://auth.example.com") + .build(); + assertThat(prm.getResource()).isEqualTo("https://api.example.com/a%23b"); + assertThat(ProtectedResourceMetadata.wellKnownUrl("https://api.example.com/a%23b")) + .isEqualTo("https://api.example.com/.well-known/oauth-protected-resource/a%23b"); + } + + @Test + void derivationHelpers_rejectFragmentAsBackstop() { + // Both helpers are public and are called on the 401 challenge path (prmUrl() feeds the + // resource_metadata parameter), so they refuse to derive from a fragment-bearing + // identifier rather than silently deriving from its fragment-free prefix. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.wellKnownPath( + URI.create("https://api.example.com/mcp#section"))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a fragment component"); + + assertThatThrownBy( + () -> + ProtectedResourceMetadata.wellKnownUrl( + "https://api.example.com/mcp#section")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a fragment component"); + } + + @Test + void derivationHelpers_reportTheFragmentBeforeNonDerivability() { + // Both checks can fail on the same identifier. A fragment is unconditionally illegal + // (RFC 8707 §2); non-derivability is a limitation of these helpers. Reporting the fragment + // first is both the more accurate diagnosis and what keeps the fragment out of the message + // — requireDerivable interpolates the identifier whole. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.wellKnownUrl( + "urn:example:api#s3cr3t-fragment")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a fragment component") + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("s3cr3t-fragment")); + + assertThatThrownBy( + () -> + ProtectedResourceMetadata.wellKnownPath( + URI.create("/mcp#s3cr3t-fragment"))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a fragment component") + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("s3cr3t-fragment")); + } + + @Test + void errorMessages_elideUserinfoAsWellAsTheFragment() { + // The elision stopped at the fragment, but the userinfo is in the prefix that gets echoed + // — and it is the component most likely to carry a credential. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.requireNoFragment( + "https://user:s3cr3t@api.example.com/mcp#x")) + .isInstanceOf(IllegalArgumentException.class) + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("s3cr3t")) + .hasMessageContaining("https://***@api.example.com/mcp"); + + // Same for the derivation guard, which interpolates an identifier it could not parse into + // something derivable. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.wellKnownUrl( + "https://user:s3cr3t@api.example.com" + + "/mcp?x=1#invalid^authority")) + .isInstanceOf(IllegalArgumentException.class) + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("s3cr3t")); + } + + @Test + void requireValidQuery_rejectsOctetsOutsideTheRfc3986Grammar() { + // A raw non-ASCII octet ships straight through escapeQuotedString (which strips only + // control characters, '\' and '"') into a WWW-Authenticate field value that RFC 9110 §5.5 + // confines to US-ASCII. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.requireValidQuery( + "https://api.example.com/mcp?q=ñ")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("RFC 3986 §3.4") + .hasMessageContaining("U+00F1"); + + // '[' and ']' are the ASCII residue: java.net.URI accepts them, §3.4 does not. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.requireValidQuery( + "https://api.example.com/mcp?a=b[c]")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("invalid character '['"); + + // A malformed escape is not a valid pct-encoded triplet. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.requireValidQuery( + "https://api.example.com/mcp?a=%zz")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("malformed percent-escape"); + + // What java.net.URI rejects, but only when something finally parses the identifier — on + // the 401 response path, as a 500. The gate moves that to construction. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.requireValidQuery( + "https://api.example.com/mcp?a=b c")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("invalid character ' '"); + } + + @Test + void requireValidQuery_acceptsTheFullQueryProduction() { + // query = *( pchar / "/" / "?" ), pchar = unreserved / pct-encoded / sub-delims / ":" / "@" + ProtectedResourceMetadata.requireValidQuery("https://api.example.com/mcp"); + ProtectedResourceMetadata.requireValidQuery("https://api.example.com/mcp?"); + ProtectedResourceMetadata.requireValidQuery("https://api.example.com/mcp?tenant=a"); + ProtectedResourceMetadata.requireValidQuery("https://api.example.com/mcp?a%23b=c"); + ProtectedResourceMetadata.requireValidQuery( + "https://api.example.com/mcp?a=-._~!$&'()*+,;=:@/?"); + // The '?' inside a fragment is not a query; requireNoFragment owns that rejection. + ProtectedResourceMetadata.requireValidQuery("https://api.example.com/mcp#frag?a=b c"); + } + + @Test + void requireValidQuery_errorMessage_elidesTheQueryAndUserinfo() { + assertThatThrownBy( + () -> + ProtectedResourceMetadata.requireValidQuery( + "https://user:s3cr3t@api.example.com/mcp?token=t0ps3cr3t[")) + .isInstanceOf(IllegalArgumentException.class) + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("s3cr3t")) + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("t0ps3cr3t")) + .hasMessageContaining("https://***@api.example.com/mcp"); + } + + @Test + void requireValidQuery_null_throwsNamedNpe() { + assertThatThrownBy(() -> ProtectedResourceMetadata.requireValidQuery(null)) + .isInstanceOf(NullPointerException.class) + .hasMessageContaining("resourceUri must not be null"); + } + + @Test + void derivationHelpers_rejectAnInvalidQueryAsBackstop() { + // Both sit on the 401 challenge path, where an unparseable resource_metadata value would + // surface as a 500 rather than as the challenge the client is waiting for. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.wellKnownUrl( + "https://api.example.com/mcp?a=b[c]")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("RFC 3986 §3.4"); + + // A URI java.net.URI itself accepts, so the backstop is the only thing standing between + // the identifier and the challenge. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.wellKnownPath( + URI.create("https://api.example.com/mcp?q=ñ"))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("RFC 3986 §3.4"); + } + + @Test + void builder_rejectsAnInvalidQuery() { + assertThatThrownBy( + () -> + ProtectedResourceMetadata.builder() + .resource("https://api.example.com/mcp?q=ñ") + .authorizationServer("https://auth.example.com") + .build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("RFC 3986 §3.4"); + } + + @Test + void requireNoFragment_null_throwsNamedNpe() { + // Public API: AuthplaneClient.resource(...) two frames up reports the same condition with + // a message, so this must not surface as a bare NPE out of String.indexOf. + assertThatThrownBy(() -> ProtectedResourceMetadata.requireNoFragment(null)) + .isInstanceOf(NullPointerException.class) + .hasMessageContaining("resourceUri must not be null"); + } + + @Test + void requireNoFragment_acceptsAFragmentFreeIdentifier() { + // Query components, trailing slashes and opaque identifiers are a separate axis: the gate + // is about the fragment and nothing else. + ProtectedResourceMetadata.requireNoFragment("https://api.example.com/mcp"); + ProtectedResourceMetadata.requireNoFragment("https://api.example.com/mcp/?tenant=acme"); + ProtectedResourceMetadata.requireNoFragment("urn:example:api"); + } + @Test void urnStyleResource_isAccepted() { // RFC 8707 §2 permits non-http(s) resource indicators. A urn: identifier must not be @@ -92,9 +325,232 @@ void schemeRelativeResource_cannotDeriveAPrmUrl() { .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("a scheme and an authority"); + // wellKnownUrl now reports the missing scheme by name: requireScheme runs as one of its + // four backstops and answers before requireDerivable is reached from wellKnownPath. Both + // reject; this one says which component is missing. assertThatThrownBy(() -> ProtectedResourceMetadata.wellKnownUrl("//api.example.com/mcp")) .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("a scheme and an authority"); + .hasMessageContaining("has no scheme"); + } + + @Test + void requireScheme_rejectsSchemelessIdentifiers() { + // The construction gate, not the derivation backstop: a scheme-less identifier is + // unconditionally illegal (RFC 8707 §2 requires an absolute URI; RFC 3986 §4.3 says one + // always begins with a scheme), and the derivation is not its only sink — the scheme is + // also spliced into the DPoP htu binding target. + for (String identifier : + new String[] { + "//api.example.com/mcp", // scheme-relative + "/mcp", // path-relative + "api.example.com/mcp", // host without a scheme + "1https://api.example.com/mcp", // scheme must start with ALPHA (§3.1) + "?tenant=acme", // query-only reference + }) { + assertThatThrownBy(() -> ProtectedResourceMetadata.requireScheme(identifier)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("has no scheme"); + } + } + + @Test + void requireScheme_acceptsAnyAbsoluteUri() { + // Scheme only — not scheme+host. An opaque absolute URI constructs (stored verbatim); + // whether it can derive a PRM URL is the derivation gate's question. + ProtectedResourceMetadata.requireScheme("https://api.example.com/mcp"); + ProtectedResourceMetadata.requireScheme("urn:example:api"); + ProtectedResourceMetadata.requireScheme("custom+v1.2-x://host/path"); + } + + @Test + void requireScheme_errorMessage_elidesUserinfoInASchemeRelativeIdentifier() { + // The rejected shape is exactly the one the redactor used to miss: no "://" anchor, so + // the early return shipped the credential verbatim in a message asserting it was elided. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.requireScheme( + "//svc:s3cr3t@api.example.com/mcp")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("***@api.example.com/mcp") + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("s3cr3t")); + } + + @Test + void requireScheme_null_throwsNamedNpe() { + assertThatThrownBy(() -> ProtectedResourceMetadata.requireScheme(null)) + .isInstanceOf(NullPointerException.class) + .hasMessageContaining("resourceUri"); + } + + @Test + void builder_rejectsASchemeRelativeResource() { + assertThatThrownBy( + () -> + ProtectedResourceMetadata.builder() + .resource("//api.example.com/mcp") + .authorizationServer("https://auth.example.com") + .build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("has no scheme"); + } + + @Test + void builder_reportsTheFragmentBeforeTheMissingScheme() { + // The gate order holds for the new axis too: an identifier that is both scheme-less and + // fragment-bearing is reported for the fragment, which also keeps the fragment out of + // the message. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.builder() + .resource("//api.example.com/mcp#secret") + .authorizationServer("https://auth.example.com") + .build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a fragment component") + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("secret")); + } + + @Test + void requireNoUserinfo_rejectsUserinfoBearingIdentifiers() { + // RFC 9110 §4.2.4: userinfo is deprecated and a recipient is to reject a URI carrying + // it. The identifier is published verbatim to unauthenticated callers, so the credential + // must not survive construction — redacting it at the sinks only covers the sinks that + // remember to redact. + for (String identifier : + new String[] { + "https://svc:s3cr3t@api.example.com/mcp", + "https://svc@api.example.com/mcp", // user, no password + "https://@api.example.com/mcp", // empty userinfo is still userinfo + "https://svc:s3cr3t@api.example.com:8443/mcp", // alongside a port + "https://svc:s3cr3t@api.example.com", // authority is the whole remainder + "https://svc:s3cr3t@api.example.com?tenant=acme", // authority ends at '?' + }) { + assertThatThrownBy(() -> ProtectedResourceMetadata.requireNoUserinfo(identifier)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a userinfo component") + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("s3cr3t")); + } + } + + @Test + void requireNoUserinfo_acceptsIdentifiersWithoutUserinfo() { + // A ':' in the authority is a port delimiter far more often than a userinfo one, and an + // IPv6 literal is nothing but colons — none of that is userinfo, and turning any of it + // away would break ordinary configurations. + ProtectedResourceMetadata.requireNoUserinfo("https://api.example.com/mcp"); + ProtectedResourceMetadata.requireNoUserinfo("https://api.example.com:8443/mcp"); + ProtectedResourceMetadata.requireNoUserinfo("http://localhost:8080/mcp"); + ProtectedResourceMetadata.requireNoUserinfo("https://[::1]:8443/mcp"); + ProtectedResourceMetadata.requireNoUserinfo("https://api.example.com/mcp?tenant=acme"); + // '@' is a pchar, so it is legal in a path and in a query and is not a userinfo + // delimiter there (RFC 3986 §§3.3, 3.4). + ProtectedResourceMetadata.requireNoUserinfo("https://api.example.com/mcp/a@b"); + ProtectedResourceMetadata.requireNoUserinfo("https://api.example.com/mcp?to=a@b"); + // No authority at all, so nothing can delimit userinfo — an opaque identifier still + // constructs (RFC 8707 §2 permits any absolute URI). + ProtectedResourceMetadata.requireNoUserinfo("urn:example:api"); + ProtectedResourceMetadata.requireNoUserinfo("mailto:ops@example.com"); + // The '@' lives in the fragment, which is not part of the authority. requireNoFragment + // owns that rejection; this gate must not claim it. + ProtectedResourceMetadata.requireNoUserinfo("https://api.example.com/mcp#a@b"); + } + + @Test + void requireNoUserinfo_isNotFooledByASchemeRelativeIdentifierWithALaterSchemeSeparator() { + // The redactor's anchoring defect in gate form: with "://" tested first the authority + // would be anchored inside the query, leaving the real userinfo before it and the + // identifier looking clean. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.requireNoUserinfo( + "//svc:s3cr3t@api.example.com/mcp?next=https://x")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a userinfo component"); + } + + @Test + void requireNoUserinfo_doesNotConjureAnAuthorityOutOfAQuery() { + // "https:example.com/mcp" has a scheme but no authority; the "://" in its query is not + // an authority delimiter, so the '@' that follows is a query octet, not userinfo. + ProtectedResourceMetadata.requireNoUserinfo("https:example.com/mcp?u=http://a:b@c"); + } + + @Test + void requireNoUserinfo_null_throwsNamedNpe() { + assertThatThrownBy(() -> ProtectedResourceMetadata.requireNoUserinfo(null)) + .isInstanceOf(NullPointerException.class) + .hasMessageContaining("resourceUri"); + } + + @Test + void builder_rejectsUserinfoInResource() { + assertThatThrownBy( + () -> + ProtectedResourceMetadata.builder() + .resource("https://svc:s3cr3t@api.example.com/mcp") + .authorizationServer("https://auth.example.com") + .build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a userinfo component") + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("s3cr3t")); + } + + @Test + void builder_reportsTheMissingSchemeBeforeTheUserinfo() { + // The userinfo gate runs last of the four, so a scheme-relative identifier that also + // carries userinfo is reported for the scheme — the defect an operator fixes first — and + // the credential is elided from that message either way. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.builder() + .resource("//svc:s3cr3t@api.example.com/mcp") + .authorizationServer("https://auth.example.com") + .build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("has no scheme") + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("s3cr3t")); + } + + @Test + void elideSecrets_elidesUserinfoWhenASchemeSeparatorFollowsInTheQuery() { + // The residue this closes: "://" was matched before the leading "//", anchoring the + // authority inside the query, so userInfoEnd < authorityStart returned early and the + // message claiming the userinfo was elided carried it verbatim. Exercised through + // requireScheme, which is the gate that rejects this shape and the one that renders it. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.requireScheme( + "//svc:s3cr3t@api.example.com/mcp?next=https://x")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("***@api.example.com/mcp?next=https://x") + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("s3cr3t")); + } + + @Test + void elideSecrets_elidesUserinfoWhenTheSchemeIsInvalid() { + // The redactor must not fail open. `authorityBounds` is strict on purpose — the gate must + // not over-reject — but the redactor giving up and returning its input means an + // identifier whose scheme does not parse ships its credential verbatim in a message that + // ends "(fragment and any userinfo elided)". Over-redacting costs nothing here. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.requireScheme( + "1https://svc:s3cr3t@api.example.com/mcp")) + .isInstanceOf(IllegalArgumentException.class) + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("s3cr3t")); + } + + @Test + void elideSecrets_elidesUserinfoWhenTheIdentifierHasLeadingWhitespace() { + // The likelier spelling of the same fault: a config value out of YAML or env with a + // leading space. Nothing on the construction path trims it, so it reaches the scheme gate + // untouched and `schemeEnd` refuses it at index 0. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.requireScheme( + " https://svc:s3cr3t@api.example.com/mcp")) + .isInstanceOf(IllegalArgumentException.class) + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("s3cr3t")); } @Test @@ -141,14 +597,108 @@ void wellKnownUrl_pathWithTrailingSlash_stripped() { .isEqualTo("https://api.example.com/.well-known/oauth-protected-resource/mcp"); } + @Test + void wellKnownUrl_preservesQueryComponent() { + // RFC 9728 §3 forms the well-known URI by inserting the well-known string "between the + // host component and the path and/or query components, if any" — the query is part of + // the identifier and follows the derived path. A query is legal in a resource + // indicator: RFC 8707 §2 states the SHOULD NOT and its exception in the same sentence, + // and RFC 9728 §1.2 carries it forward. Dropping it advertised a document URL for a + // different identifier than the one the document's "resource" field publishes. + assertThat(ProtectedResourceMetadata.wellKnownUrl("https://api.example.com/mcp?tenant=a")) + .isEqualTo( + "https://api.example.com/.well-known/oauth-protected-resource/mcp?tenant=a"); + } + + @Test + void wellKnownUrl_queryOnRootResource_insertsSuffixBeforeQuery() { + // With no path there is no terminating slash to remove: the well-known suffix lands + // directly after the host and the query follows it. + assertThat(ProtectedResourceMetadata.wellKnownUrl("https://api.example.com?x=1")) + .isEqualTo("https://api.example.com/.well-known/oauth-protected-resource?x=1"); + } + + @Test + void wellKnownUrl_queryAfterTerminatingSlash_slashStripped() { + // RFC 9728 §3.1 removes the terminating slash following the host when a path or query + // is present, so "/?x=1" derives the same document URL as "?x=1". + assertThat(ProtectedResourceMetadata.wellKnownUrl("https://api.example.com/?x=1")) + .isEqualTo("https://api.example.com/.well-known/oauth-protected-resource?x=1"); + } + + @Test + void wellKnownUrl_queryDifferingIdentifiers_deriveDistinctUrls() { + // Identifiers differing only in their query are distinct resource identities and must + // advertise distinct PRM URLs — collapsing them would make a document fetched for one + // identifier name another, the mismatch RFC 9728 §3.3 makes a client discard. + String a = ProtectedResourceMetadata.wellKnownUrl("https://api.example.com/mcp?tenant=a"); + String b = ProtectedResourceMetadata.wellKnownUrl("https://api.example.com/mcp?tenant=b"); + String bare = ProtectedResourceMetadata.wellKnownUrl("https://api.example.com/mcp"); + assertThat(a).isNotEqualTo(b); + assertThat(a).isNotEqualTo(bare); + assertThat(b).isNotEqualTo(bare); + } + + @Test + void wellKnownUrl_preservesRawQueryEncoding() { + // getRawQuery(): a percent-encoded octet in the query is carried through verbatim. + // Decoding and re-encoding could rewrite it, and the advertised URL would carry a + // different query than the identifier does. + assertThat( + ProtectedResourceMetadata.wellKnownUrl( + "https://api.example.com/mcp?filter=a%2Fb")) + .isEqualTo( + "https://api.example.com/.well-known/oauth-protected-resource/mcp?filter=a%2Fb"); + } + @Test void wellKnownUrl_preservesRawAuthority() { - // getAuthority() percent-decodes, so "u%40b@" derived "u@b@" — an authority structurally - // different from the one the identifier names (two '@' delimiters instead of one). The - // raw-preservation rule applies to the authority exactly as it does to the path. - assertThat(ProtectedResourceMetadata.wellKnownUrl("https://u%40b@api.example.com/mcp")) + // getAuthority() percent-decodes, so an escaped delimiter in the authority derives a + // structurally different authority. The vehicle is a percent-escape in the registered name + // (RFC 3986 §3.2.2 admits pct-encoded there), not userinfo: userinfo is now refused at + // construction and by wellKnownUrl itself, so it can no longer reach this derivation. + // + // %3A is ":" — decoding it would turn the reg-name "a%3Ab.example.com" into "a:b.example + // .com", which reads as host "a" with port "b". Note the escape must be of a *reserved* + // octet for raw to be the right answer at all: RFC 3986 §6.2.2.2 has a conformant client + // decode escaped *unreserved* octets before comparing, so preserving those raw would make + // the derived URL fail to match what the client actually resolves. + assertThat(ProtectedResourceMetadata.wellKnownUrl("https://a%3Ab.example.com/mcp?x=1")) .isEqualTo( - "https://u%40b@api.example.com/.well-known/oauth-protected-resource/mcp"); + "https://a%3Ab.example.com/.well-known/oauth-protected-resource/mcp?x=1"); + } + + @Test + void wellKnownUrl_emptyQuery_derivesQueryLessUrl() { + // getRawQuery() returns "" (not null) for a bare trailing '?'. RFC 3986 would allow + // reading an empty query as present-but-empty, but every implementation of this + // derivation treats it as absent — parity across implementations wins over that reading, + // so the derived URL carries no '?'. + assertThat(ProtectedResourceMetadata.wellKnownUrl("https://api.example.com/mcp?")) + .isEqualTo("https://api.example.com/.well-known/oauth-protected-resource/mcp"); + } + + @Test + void wellKnownPath_ignoresQueryComponent() { + // Routing stays path-keyed: the query belongs to the full document URL (wellKnownUrl), + // never to the route the document is served at. One registered route serves the + // document for every query value. + assertThat( + ProtectedResourceMetadata.wellKnownPath( + URI.create("https://api.example.com/mcp?tenant=a"))) + .isEqualTo("/.well-known/oauth-protected-resource/mcp"); + } + + @Test + void wellKnownUrl_queryDoesNotWeakenFragmentGate() { + // A '#' after the query still begins a fragment (RFC 3986 §3.5); preserving the query + // must not loosen the fragment rejection. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.wellKnownUrl( + "https://api.example.com/mcp?tenant=a#x")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a fragment component"); } @Test diff --git a/mcp/docs/user-guide.md b/mcp/docs/user-guide.md index 0ed4746..d5ba162 100644 --- a/mcp/docs/user-guide.md +++ b/mcp/docs/user-guide.md @@ -94,6 +94,8 @@ Every builder method on `AuthplaneMcpSetup.Builder`: | `outboundDPoP(OutboundDPoPOptions)` | `null` | Enables outbound DPoP proofs on AS calls | | `inboundDPoP(InboundDPoPOptions)` | `null` | Enables inbound DPoP proof validation | +Both refresh intervals are driven by traffic rather than by a background timer: the first token verification past the interval pays for the refetch. That is what keeps an MCP server — which never calls the token, introspection or revocation endpoints — following a rotated `jwks_uri`: the metadata read that discovers the new URI rebinds JWKS fetching before the token is verified. A metadata endpoint that is unreachable does not fail verification; the last known good document keeps being served. + ## 5. Scope enforcement Enforce per-tool scope requirements inside tool handlers: diff --git a/mcp/pom.xml b/mcp/pom.xml index 2fdf3a6..00d77a3 100644 --- a/mcp/pom.xml +++ b/mcp/pom.xml @@ -51,7 +51,19 @@ mcp-json-jackson3 - + org.eclipse.jetty.ee10 jetty-ee10-servlet diff --git a/mcp/src/main/java/ai/authplane/sdk/mcp/PrmServlet.java b/mcp/src/main/java/ai/authplane/sdk/mcp/PrmServlet.java index 9805c27..65dd4cb 100644 --- a/mcp/src/main/java/ai/authplane/sdk/mcp/PrmServlet.java +++ b/mcp/src/main/java/ai/authplane/sdk/mcp/PrmServlet.java @@ -18,6 +18,10 @@ * Servlet that serves an RFC 9728 Protected Resource Metadata document as JSON. * *

Register it at the path returned by {@link ProtectedResourceMetadata#wellKnownPath(URI)}. + * Registration is path-keyed: a query component of the resource identifier appears in the + * advertised PRM URL (RFC 9728 §3) but does not change the registration path — this servlet serves + * the same document whatever query string a request carries. Serving distinct documents per query + * value is not supported. * *

Example: * diff --git a/mcp/src/test/java/ai/authplane/sdk/mcp/PrmServletTest.java b/mcp/src/test/java/ai/authplane/sdk/mcp/PrmServletTest.java index f566a4b..414cc09 100644 --- a/mcp/src/test/java/ai/authplane/sdk/mcp/PrmServletTest.java +++ b/mcp/src/test/java/ai/authplane/sdk/mcp/PrmServletTest.java @@ -7,6 +7,10 @@ import java.io.PrintWriter; import java.io.StringWriter; +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -14,6 +18,10 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import org.eclipse.jetty.ee10.servlet.ServletContextHandler; +import org.eclipse.jetty.ee10.servlet.ServletHolder; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; @@ -72,6 +80,51 @@ void doGet_serializesMapBody_includingDpopFields() throws Exception { assertThat(json).contains("\"dpop_bound_access_tokens_required\":true"); } + @Test + void queryBearingRequest_reachesThePathRegisteredServlet() throws Exception { + // The advertised PRM URL may carry the resource identifier's query component, while the + // servlet is registered path-keyed (wellKnownPath ignores the query). This pins the claim + // that makes that split safe: a query-bearing GET at the advertised path still reaches the + // servlet mapping and is served the document. Registration mirrors the class javadoc + // (ServletHolder at wellKnownPath) against a real container, not a mocked dispatch. + ProtectedResourceMetadata prm = + ProtectedResourceMetadata.builder() + .resource("https://mcp.example.com/mcp?tenant=a") + .authorizationServer("https://auth.example.com") + .scopes(List.of("tools/read")) + .build(); + String path = ProtectedResourceMetadata.wellKnownPath(URI.create(prm.getResource())); + + Server server = new Server(0); + ServletContextHandler context = new ServletContextHandler(); + context.addServlet(new ServletHolder(new PrmServlet(prm)), path); + server.setHandler(context); + server.start(); + try { + int port = ((ServerConnector) server.getConnectors()[0]).getLocalPort(); + HttpResponse res = + HttpClient.newHttpClient() + .send( + HttpRequest.newBuilder( + URI.create( + "http://localhost:" + + port + + path + + "?tenant=a")) + .GET() + .build(), + HttpResponse.BodyHandlers.ofString()); + + assertThat(res.statusCode()).isEqualTo(200); + assertThat(res.headers().firstValue("Content-Type").orElse("")) + .startsWith("application/json"); + assertThat(res.body()) + .contains("\"resource\":\"https://mcp.example.com/mcp?tenant=a\""); + } finally { + server.stop(); + } + } + @Test void constructor_rejectsNullPrm() { assertThatNullPointerException() diff --git a/spring/docs/user-guide.md b/spring/docs/user-guide.md index 8f4e497..a66f8ad 100644 --- a/spring/docs/user-guide.md +++ b/spring/docs/user-guide.md @@ -177,6 +177,8 @@ Both paths use the same `application.properties` keys: | `authplane.circuit-breaker-cooldown-seconds` | `0` (SDK default: 30s) | Cooldown before the circuit breaker transitions to half-open | | `authplane.token-cache-ttl-buffer-seconds` | `0` (SDK default: 30s) | Buffer in seconds before token cache entries expire | +Both cache TTLs are driven by traffic rather than by a background timer: the first request past the interval pays for the refetch. Verification is what re-reads the AS metadata document, so a Spring resource server that only validates bearer tokens still follows a rotated `jwks_uri` — the metadata read that discovers the new URI rebinds JWKS fetching before the token is verified. A metadata endpoint that is unreachable does not fail authentication; the last known good document keeps being served. + ### Optional Beans For advanced features that can't be expressed as properties, expose these beans: diff --git a/spring/pom.xml b/spring/pom.xml index d331e77..6aa2442 100644 --- a/spring/pom.xml +++ b/spring/pom.xml @@ -12,6 +12,16 @@ ../pom.xml + + + 7.0.8 + + authplane-spring jar @@ -65,7 +75,7 @@ org.springframework spring-webmvc - 7.0.8 + ${spring.version} provided @@ -134,6 +144,17 @@ mcp-json-jackson3 test + + + + org.springframework + spring-test + ${spring.version} + test + diff --git a/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneSecurityConfigTest.java b/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneSecurityConfigTest.java index fc6ae8f..fbdafd6 100644 --- a/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneSecurityConfigTest.java +++ b/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneSecurityConfigTest.java @@ -10,6 +10,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.concurrent.Executor; import java.util.concurrent.ForkJoinPool; @@ -23,6 +24,10 @@ import org.springframework.beans.factory.ObjectProvider; import org.springframework.context.ApplicationContext; import org.springframework.context.support.GenericApplicationContext; +import org.springframework.http.converter.HttpMessageConverter; +import org.springframework.http.converter.json.JacksonJsonHttpMessageConverter; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.config.ObjectPostProcessor; import org.springframework.security.config.annotation.authentication.builders.AuthenticationManagerBuilder; import org.springframework.security.config.annotation.web.builders.HttpSecurity; @@ -30,6 +35,10 @@ import org.springframework.security.web.DefaultSecurityFilterChain; import org.springframework.security.web.SecurityFilterChain; import org.springframework.security.web.servlet.util.matcher.PathPatternRequestMatcher; +import org.springframework.web.servlet.function.HandlerFunction; +import org.springframework.web.servlet.function.RouterFunction; +import org.springframework.web.servlet.function.ServerRequest; +import org.springframework.web.servlet.function.ServerResponse; import com.github.tomakehurst.wiremock.WireMockServer; import com.github.tomakehurst.wiremock.core.WireMockConfiguration; @@ -178,6 +187,107 @@ void authplanePrmEndpoint_returnsRouterFunction() throws Exception { assertThat(router).isNotNull(); } + @Test + void authplanePrmEndpoint_queryBearingRequest_resolvesToHandlerAndServesDocument() + throws Exception { + // The advertised PRM URL may carry the resource identifier's query component while the + // route is registered path-keyed. This drives the RouterFunction with a query-bearing + // request rather than asserting registration: the request must resolve to the handler and + // be served the document. + AuthplaneResource v = buildVerifier(buildClient(0), false); + RouterFunction router = config.authplanePrmEndpoint(v); + + MockHttpServletRequest servletRequest = new MockHttpServletRequest("GET", v.prmPath()); + servletRequest.setQueryString("tenant=a"); + servletRequest.addParameter("tenant", "a"); + List> converters = List.of(new JacksonJsonHttpMessageConverter()); + ServerRequest request = ServerRequest.create(servletRequest, converters); + + Optional> handler = router.route(request); + assertThat(handler).isPresent(); + + ServerResponse serverResponse = handler.get().handle(request); + MockHttpServletResponse servletResponse = new MockHttpServletResponse(); + serverResponse.writeTo(servletRequest, servletResponse, () -> converters); + + assertThat(servletResponse.getStatus()).isEqualTo(200); + assertThat(servletResponse.getContentAsString()) + .contains("\"resource\":\"" + baseUrl + "/mcp\""); + } + + @Test + void authenticationEntryPoint_queryBearingResource_advertisesTheQueryInTheChallenge() + throws Exception { + // The one end-to-end link the query axis left unasserted: a resource whose *identifier* + // carries a query, through the real prmUrl(), into the resource_metadata parameter of a + // real 401. The entry-point test mocks prmUrl(); ErrorsTest hand-feeds an already-derived + // URL to the sanitiser; and the routing test above uses a query-less resource with a + // query-bearing request, so the spring side never saw a query-bearing identifier at all. + AuthplaneResource v = + config.authplaneResource( + buildClient(0), + baseUrl + "/mcp?tenant=a", + List.of("tools/add"), + List.of("RS256"), + 30, + false, + revocationCheckerProvider, + inboundDPoPProvider); + + MockHttpServletResponse response = new MockHttpServletResponse(); + new AuthplaneAuthenticationEntryPoint(v) + .commence(new MockHttpServletRequest("GET", "/mcp"), response, null); + + assertThat(response.getStatus()).isEqualTo(401); + assertThat(response.getHeader("WWW-Authenticate")) + .contains( + "resource_metadata=\"" + + baseUrl + + "/.well-known/oauth-protected-resource/mcp?tenant=a\""); + } + + @Test + void authplaneResource_queryOutsideTheRfc3986Grammar_failsAtContextStartup() { + // A query octet the §3.4 grammar does not admit used to construct cleanly and then throw + // out of prmUrl() inside commence() — a 500 in place of the 401. It now fails where the + // operator wrote it, which for a Spring host is context startup. + assertThatThrownBy( + () -> + config.authplaneResource( + buildClient(0), + baseUrl + "/mcp?a=b c", + List.of("tools/add"), + List.of("RS256"), + 30, + false, + revocationCheckerProvider, + inboundDPoPProvider)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("RFC 3986 §3.4"); + } + + @Test + void authplaneResource_schemeRelativeIdentifier_failsAtContextStartup() { + // A scheme-relative identifier used to construct cleanly and fail only at the sinks that + // splice the scheme — the PRM derivation inside commence() and the DPoP htu binding + // target, both reading the missing scheme as the literal text "null". Like the invalid + // query above, it now fails where the operator wrote it: context startup for a Spring + // host. + assertThatThrownBy( + () -> + config.authplaneResource( + buildClient(0), + "//api.example.com/mcp", + List.of("tools/add"), + List.of("RS256"), + 30, + false, + revocationCheckerProvider, + inboundDPoPProvider)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("has no scheme"); + } + @Test void authplaneWebSecurityCustomizer_returnsCustomizer() throws Exception { AuthplaneResource v = buildVerifier(buildClient(0), false); @@ -338,6 +448,29 @@ void authplaneSecurityFilterChain_resourceWithoutPath_throwsIllegalState() throw .hasMessageContaining("must include a path"); } + @Test + void authplaneResource_fragmentInProperty_throwsIllegalArgument() throws Exception { + // authplane.resource is operator-supplied, and this bean is the only place a Spring host + // constructs the resource. A fragment must fail at context startup, not silently publish a + // PRM document whose "resource" disagrees with the URL it is served from (RFC 8707 §2, + // RFC 9728 §1.2/§3.3). + AuthplaneClient client = buildClient(0); + + assertThatThrownBy( + () -> + config.authplaneResource( + client, + baseUrl + "/mcp#section", + List.of("tools/add"), + List.of("RS256"), + 30, + false, + revocationCheckerProvider, + inboundDPoPProvider)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("must not include a fragment component"); + } + // ----------------------------------------------------------------------- // Helpers // -----------------------------------------------------------------------