diff --git a/CHANGELOG.md b/CHANGELOG.md index a848256..8ad28e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added +- `core/resource/verifier`: `ValidateIssuer(issuer string) error` — the RFC 8414 §2 issuer-shape rule, exported so every construction boundary applies one implementation rather than a copy. Rejects a query or fragment component, and requires an absolute URL with a scheme and host. `NewTokenVerifier`, `resource.New` and `authplane.NewClient` all route through it. +- `core/resource/verifier`: `ErrInvalidIssuer` sentinel, returned by everything that validates an issuer identifier. Match it with `errors.Is`. + +### Fixed +- `core/resource/verifier`, `core/authplane`: an issuer rejected at construction is no longer echoed verbatim into the error. The query/fragment branch fires for exactly the shape that can carry a credential (`https://as.example.com?access_token=…`), and `net/url.Error` prints its URL field without redacting, so the raw identifier — query, fragment and any userinfo — reached whatever log the construction error landed in. Messages now carry scheme and host only. Parse failures are still wrapped with `%w`, so `errors.As(err, new(*url.Error))` keeps working; only the URL the error prints is substituted. +- `http`: the RFC 9728 PRM discovery bypass in the `net/http` adapter now compares `r.URL.EscapedPath()` against the escaped well-known path instead of the decoded `r.URL.Path`. A resource identifier carrying a percent-encoded octet (e.g. `%2F`) yields an escaped well-known path; comparing the decoded path let `%2F` collapse to `/`, the two sides disagreed, and the discovery endpoint stopped being bypassed and returned 401 even though RFC 9728 §3.2 requires it publicly reachable. The check is deliberately stricter than RFC 3986 §6.2.2.1 (a percent-encoded *unreserved* octet won't match its decoded form), an accepted trade-off since a conformant client signs the same octets the operator configured. + +### Changed +- **BREAKING** `core/resource/verifier`, `core/resource`: `NewTokenVerifier` and `resource.New` now reject an issuer carrying a query or fragment component, and require the identifier to be an absolute URL with a scheme and host (RFC 8414 §2). Construction that succeeded in 0.2.0 — a relative reference such as `/tenant`, or an issuer with `?x=1` — now fails. `url.ParseRequestURI` alone accepted both: it takes a path-only reference, and it folds a fragment into `Path` rather than splitting it. **Migration:** pass the authorization server's issuer identifier exactly as published — absolute, `https`, no query, no fragment. +- **BREAKING** `core/authplane`: `NewClient` additionally requires the issuer to be absolute with a scheme and host, beyond the query/fragment rule below. This gate is not redundant with the verifier's: a `*Client` used only for token, introspection and revocation calls never constructs a `TokenVerifier`, so it is the only thing keeping a relative reference out of eager discovery. **Migration:** as above. +- **BREAKING** `core/authplane`: `ErrInvalidIssuer` is now an alias of `verifier.ErrInvalidIssuer` rather than its own sentinel. Two consequences for code that inspects it: the message changes from `authplane: invalid issuer` to `verifier: invalid issuer`, and `errors.Is(err, authplane.ErrInvalidIssuer)` now returns true for a rejection raised by the verifier, where it previously returned false. **Migration:** if you relied on the two sentinels being distinct to tell which layer rejected an identifier, that distinction is gone — both boundaries now apply the same rule, so match on the single sentinel and read the message for the specific violation. Code that only did `errors.Is(err, authplane.ErrInvalidIssuer)` on a `NewClient` error is unaffected. +- **BREAKING** `core/authplane`: `NewClient` now rejects an issuer containing a query or fragment component (RFC 8414 §2 forbids both) instead of passing it straight into metadata discovery. Previously the resource side rejected a fragment but the issuer had no such check, and the two discovery-URL builders diverged when either was present — the RFC 8414 builder silently dropped the issuer's query/fragment while the OIDC builder carried them along, so the two discovery attempts targeted different identities. Construction now fails immediately with a clear error. **Migration:** strip any query or fragment from the issuer you pass to `NewClient`; an issuer identifier never carries one. +- **BREAKING** `core/resource`: `resource.New` now rejects a resource URI containing a `#` (RFC 8707 §2 forbids a fragment in a resource indicator). `url.ParseRequestURI` does not split the fragment, so `https://api.example.com/mcp#frag` previously passed the scheme/host check and leaked the fragment into the derived PRM URL. This is a construction-time change on the exported constructor. **Migration:** remove any fragment from the resource URI you pass to `resource.New`. +- **BREAKING** `core/resource`: the RFC 9728 §3.1 PRM well-known URL now strips any terminating slash following the host component before inserting the well-known path suffix, so a resource identifier ending in `/mcp/` is served at (and derived by a conformant client as) `/.well-known/oauth-protected-resource/mcp` rather than `.../mcp/`. The resource identifier itself is unchanged — only the derived publication URL loses the slash. **Migration:** if you currently serve your PRM document at a trailing-slash well-known path, move it to the slash-stripped path (or route both) so RFC 9728 clients stop 404ing. +- **BREAKING** `core/resource`: `WellKnownPRMPath()` and `PRMURL()` now derive from the resource identifier's escaped path, so a percent-encoded octet (RFC 3986 §3.3 path data, e.g. `%2F`) is carried through verbatim instead of being decoded to `/`. A resource identifier such as `https://api.example.com/mcp%2Fx` therefore yields `.../oauth-protected-resource/mcp%2Fx` where 0.2.0 returned `.../mcp/x` — a visible output change on both exported methods. **Migration:** if you consume these values (routing the PRM handler, advertising `resource_metadata`), ensure your router matches the escaped path. +- **BREAKING** `core/internal/metadata`: the RFC 8414 §3.3 issuer check now compares the configured issuer and the metadata document's `issuer` byte-for-byte (§4: code-point-for-code-point, no normalization) instead of trailing-slash-insensitively. A document whose issuer differs from the configured issuer only by a trailing slash is now rejected as a mismatch. Because discovery is eager, this surfaces at `NewClient` as `metadata: issuer mismatch` — construction fails immediately, not at the first token verification. **Migration:** If your configured issuer differs from your authorization server's actual identifier by a trailing slash, correct the config — the SDK no longer silently reconciles them. +- **BREAKING** `core/resource/verifier`: the token verifier stores the issuer passed to `NewTokenVerifier` verbatim and matches a token's `iss` claim byte-for-byte (RFC 8414 §4: code-point-for-code-point, no normalization) instead of trailing-slash-insensitively. A token whose `iss` differs from the configured issuer only by a trailing slash is now an `ErrIssuerMismatch`. **Migration:** If the issuer you pass to `NewTokenVerifier` differs from your authorization server's actual identifier by a trailing slash, correct it — the SDK no longer silently reconciles them. + ## [0.2.0] - 2026-07-21 ### Added diff --git a/core/authplane/client.go b/core/authplane/client.go index e2e1f21..8885d24 100644 --- a/core/authplane/client.go +++ b/core/authplane/client.go @@ -59,6 +59,32 @@ func NewClient(ctx context.Context, issuer string, opts ...Option) (*Client, err opt(cfg) } + // RFC 8414 §2 forbids both a query and a fragment component in an issuer + // identifier. The resource side already rejects a fragment (resource.New), + // but the issuer flowed straight into metadata.Config with no such check — + // and the two discovery-URL builders disagree when either is present. + // buildOAuthMetadataURL now resolves a well-known reference against the + // issuer, silently dropping the issuer's query and fragment, while + // buildOIDCDiscoveryURL still trims only a trailing slash and concatenates + // the well-known suffix onto the whole string, carrying the query/fragment + // along. For "https://as.example.com/tenant?x=1" the RFC 8414 and OIDC + // discovery attempts would therefore target two different identities. + // Rejecting a query- or fragment-bearing issuer here makes the two helpers + // agree by construction. + // + // This gate is load-bearing on its own, not merely an earlier copy of the + // verifier's. A *Client used only for token, introspection and revocation + // calls never constructs a TokenVerifier, so verifier.NewTokenVerifier is + // never reached and this is the only thing keeping a relative or + // query-bearing issuer out of eager discovery. + // + // It calls the same exported rule rather than restating it, so the two + // boundaries cannot drift — and both reject with the same redacted message + // and the same ErrInvalidIssuer sentinel. + if err := verifier.ValidateIssuer(issuer); err != nil { + return nil, err + } + // Fetch settings precedence: explicit WithFetchSettings > AUTHPLANE_DEV_MODE env > defaults. var fetchSettings ssrf.FetchSettings switch { diff --git a/core/authplane/client_test.go b/core/authplane/client_test.go index 8661cf2..f08877f 100644 --- a/core/authplane/client_test.go +++ b/core/authplane/client_test.go @@ -6,6 +6,7 @@ import ( "errors" "net/http" "net/http/httptest" + "strings" "sync/atomic" "testing" "time" @@ -85,6 +86,87 @@ func TestNewClient_Success(t *testing.T) { defer client.Close() } +func TestNewClient_RejectsIssuerWithQueryOrFragment(t *testing.T) { + // RFC 8414 §2 forbids both a query and a fragment in an issuer identifier. + // NewClient must reject them at construction — before discovery — so the two + // discovery-URL builders cannot diverge on a query/fragment-bearing issuer. + cases := []struct { + name string + issuer string + // wantMsg is the substring the rejection message must carry. The two + // rules produce different wording, so asserting the shared sentinel + // alone would not tell them apart. + wantMsg string + }{ + {"query", "https://as.example.com/tenant?x=1", "query or fragment"}, + {"fragment", "https://as.example.com/tenant#frag", "query or fragment"}, + {"both", "https://as.example.com/tenant?x=1#frag", "query or fragment"}, + // The scheme/host rule is not redundant with the verifier's gate: a + // *Client used only for token, introspection and revocation calls never + // constructs a TokenVerifier, so NewClient is the only boundary that + // keeps a relative reference out of eager discovery. Without these rows + // the branch could be deleted and no test would go red. + {"no scheme or host", "/tenant", "scheme and host"}, + {"scheme only", "https://", "scheme and host"}, + // A bare authority fails earlier, in url.ParseRequestURI, so it takes + // the wrapped-parse-error branch rather than the scheme/host one. It is + // still rejected, and its message is still redacted. + {"host only", "as.example.com/tenant", "unparseable issuer"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + client, err := authplane.NewClient(context.Background(), tc.issuer, + authplane.WithFetchSettings(authplane.DevModeFetchSettings())) + if err == nil { + if client != nil { + client.Close() + } + t.Fatalf("expected error for issuer %q, got nil", tc.issuer) + } + if !errors.Is(err, authplane.ErrInvalidIssuer) { + t.Fatalf("expected error to wrap ErrInvalidIssuer, got %v", err) + } + if !strings.Contains(err.Error(), tc.wantMsg) { + t.Fatalf("expected %q in rejection message, got %v", tc.wantMsg, err) + } + }) + } +} + +func TestNewClient_RejectionDoesNotEchoIssuerSecrets(t *testing.T) { + // The query/fragment branch fires for exactly the shape that carries a + // secret. Construction errors land in startup logs, so the message must not + // reproduce the query, the fragment or any userinfo. + // The needles must not be substrings of the rejection wording itself — + // "frag" would match the word "fragment" in the message and report a leak + // that is not one. + const ( + secret = "s3cr3t-token-value" + password = "hunter2-not-a-word" + fragNeed = "zz-fragment-needle" + ) + issuer := "https://admin:" + password + "@as.example.com/tenant?access_token=" + secret + "#" + fragNeed + + client, err := authplane.NewClient(context.Background(), issuer, + authplane.WithFetchSettings(authplane.DevModeFetchSettings())) + if err == nil { + if client != nil { + client.Close() + } + t.Fatal("expected error for issuer carrying a query and fragment, got nil") + } + msg := err.Error() + for _, leaked := range []string{secret, password, "access_token", fragNeed} { + if strings.Contains(msg, leaked) { + t.Fatalf("rejection message leaked %q: %s", leaked, msg) + } + } + // The host is deliberately kept — without it the error is unactionable. + if !strings.Contains(msg, "as.example.com") { + t.Fatalf("expected the host to survive redaction, got %s", msg) + } +} + func TestNewClient_NoCredentials(t *testing.T) { server, serverURL := mockAS(t) defer server.Close() diff --git a/core/authplane/errors.go b/core/authplane/errors.go new file mode 100644 index 0000000..4dfe1b3 --- /dev/null +++ b/core/authplane/errors.go @@ -0,0 +1,12 @@ +package authplane + +import "github.com/authplane/go-sdk/core/resource/verifier" + +// ErrInvalidIssuer is returned when the issuer identifier is not the shape RFC +// 8414 requires: §2 forbids a query and a fragment component, and the +// identifier must be an absolute URL with a scheme and host. +// +// It is the same sentinel value verifier.ErrInvalidIssuer names, so errors.Is +// matches whether the rejection came from NewClient or from the authoritative +// gate in verifier.NewTokenVerifier. +var ErrInvalidIssuer = verifier.ErrInvalidIssuer diff --git a/core/conformancetests/rfc8414_test.go b/core/conformancetests/rfc8414_test.go index 67efa5d..1b92882 100644 --- a/core/conformancetests/rfc8414_test.go +++ b/core/conformancetests/rfc8414_test.go @@ -59,6 +59,32 @@ func TestRFC8414MetadataIssuerMustMatchConfiguredIssuer(t *testing.T) { if !strings.Contains(err.Error(), "issuer mismatch") { t.Errorf("expected issuer mismatch error, got: %v", err) } + + // Catalog variant: §3.3 requires the advertised issuer to be *identical*, + // and §4 spells the comparison out as code-point-for-code-point. A metadata + // issuer differing from the configured one only by a terminating slash is + // therefore also a mismatch — this is the case a normalizing comparison + // would silently accept, binding the client to a different identity. + slashTS := metadataServerDynamic(t, func(issuer string) map[string]any { + return map[string]any{ + "issuer": issuer + "/", + "jwks_uri": issuer + "/jwks", + } + }) + + slashMC := metadata.New(metadata.Config{ + IssuerURL: slashTS.URL, + FetchSettings: ssrf.DevModeFetchSettings(), + }) + defer slashMC.Close() + + _, err = slashMC.Get(ctx) + if err == nil { + t.Fatal("expected error when the metadata issuer differs only by a terminating slash") + } + if !strings.Contains(err.Error(), "issuer mismatch") { + t.Errorf("expected issuer mismatch error, got: %v", err) + } } func TestRFC8414JWKSURIRequiredForJWTValidation(t *testing.T) { diff --git a/core/conformancetests/rfc9728_test.go b/core/conformancetests/rfc9728_test.go index 6216a35..de536e0 100644 --- a/core/conformancetests/rfc9728_test.go +++ b/core/conformancetests/rfc9728_test.go @@ -120,6 +120,17 @@ func TestRFC9728WellKnownPathMustDeriveFromResourceURI(t *testing.T) { {"https://api.example.com", "/.well-known/oauth-protected-resource"}, {"https://api.example.com/mcp", "/.well-known/oauth-protected-resource/mcp"}, {"https://api.example.com/v2/mcp", "/.well-known/oauth-protected-resource/v2/mcp"}, + // Catalog row: a resource published with a terminating slash serves its + // metadata at the slash-less well-known path, so identifiers differing + // only by that slash resolve to the same document (RFC 9728 §3.1). + {"https://api.example.com/mcp/", "/.well-known/oauth-protected-resource/mcp"}, + // Every terminating slash is stripped, not one — pinned so the choice + // cannot silently drift back to a single-character trim. + {"https://api.example.com/mcp//", "/.well-known/oauth-protected-resource/mcp"}, + // A percent-encoded octet is path data (RFC 3986 §3.3), not the "/" + // delimiter, so it survives the derivation verbatim rather than + // decoding into a separator and naming a different resource. + {"https://api.example.com/mcp%2Fx", "/.well-known/oauth-protected-resource/mcp%2Fx"}, } for _, tc := range cases { diff --git a/core/docs/user-guide.md b/core/docs/user-guide.md index 72355ef..f93a811 100644 --- a/core/docs/user-guide.md +++ b/core/docs/user-guide.md @@ -67,6 +67,10 @@ func main() { `authplane.NewClient` is the top-level entry point. It owns AS metadata discovery, JWKS caching, token caching, DPoP configuration, and the circuit breaker. +The issuer must be the authorization server's identifier exactly as published: an absolute `https` URL with a host, carrying **no query and no fragment** (RFC 8414 §2 forbids both). Anything else is rejected at construction with an error wrapping `verifier.ErrInvalidIssuer` — match it with `errors.Is`. The same rule is applied by `verifier.NewTokenVerifier` and `resource.New`, all three through the exported `verifier.ValidateIssuer`. + +The identifier is stored verbatim: a trailing slash is significant. If your AS publishes `https://auth.example.com/`, configure that, including the slash — the SDK compares the token's `iss` byte-for-byte (RFC 8414 §4) and no longer reconciles the two forms. Deriving the `.well-known` discovery URL still drops the terminating slash, but that is derivation, not identity. + ```go import "github.com/authplane/go-sdk/core/authplane" diff --git a/core/internal/metadata/metadata.go b/core/internal/metadata/metadata.go index 7e9dff9..d4117f9 100644 --- a/core/internal/metadata/metadata.go +++ b/core/internal/metadata/metadata.go @@ -201,6 +201,21 @@ func (mc *MetadataCache) fetchMetadata(ctx context.Context) (data []byte, header return nil, nil, fmt.Errorf("metadata: discovery failed (tried RFC 8414 and OIDC): %w", lastErr) } +// buildOAuthMetadataURL derives the RFC 8414 authorization-server metadata URL +// from the issuer. Per RFC 8414 §3.1 the well-known path component is inserted +// between the host and the issuer's path component (not appended to the end), +// and any terminating slash on the issuer's path is removed first, so an issuer +// of "https://as.example.com/tenant/" derives +// ".../oauth-authorization-server/tenant". The escaped path is used so a +// percent-encoded octet (RFC 3986 §3.3 path data) survives into the derived URL +// rather than being decoded and mistaken for a delimiter. +// +// The well-known suffix is parsed into a reference and resolved against the +// issuer (rather than assigned to u.Path with u.RawPath cleared): the escaped +// path already carries the encoding, and assigning it to u.Path would make +// String() re-escape a literal "%2F" into "%252F" (a 404). Parsing the suffix +// populates its RawPath so the escaping round-trips unchanged. This mirrors the +// PRM URL derivation in core/resource.buildPRM. func buildOAuthMetadataURL(issuer string) string { u, err := url.Parse(issuer) if err != nil { @@ -208,15 +223,22 @@ func buildOAuthMetadataURL(issuer string) string { } path := strings.TrimRight(u.EscapedPath(), "/") - if path == "" { - u.Path = "/.well-known/oauth-authorization-server" - } else { - u.Path = "/.well-known/oauth-authorization-server" + path - } - u.RawPath = "" - return u.String() + // ResolveReference dereferences ref immediately, so a nil ref would panic. + // That is unreachable here: path is u.EscapedPath() (an already-valid + // escaped path from a successfully parsed URL) prefixed with a literal + // well-known segment, so url.Parse cannot fail and ref is never nil. The + // discarded error is therefore safe to ignore. + ref, _ := url.Parse("/.well-known/oauth-authorization-server" + path) + return u.ResolveReference(ref).String() } +// buildOIDCDiscoveryURL derives the OIDC discovery URL from the issuer. OIDC +// Discovery §4 appends "/.well-known/openid-configuration" to the end of the +// issuer, whereas RFC 8414 (see buildOAuthMetadataURL) inserts the well-known +// path between host and path; both nonetheless require removing the issuer's +// terminating slash first, for different reasons — appending to a trailing +// slash would double it, and inserting past one would leave it stranded before +// the path component. func buildOIDCDiscoveryURL(issuer string) string { return strings.TrimRight(issuer, "/") + "/.well-known/openid-configuration" } @@ -256,10 +278,17 @@ func (mc *MetadataCache) parse(data []byte) (*ASMetadata, error) { if meta.Issuer == "" { return nil, fmt.Errorf("metadata: missing required field \"issuer\"") } - configuredIssuer := strings.TrimRight(mc.issuerURL, "/") - metaIssuer := strings.TrimRight(meta.Issuer, "/") - if metaIssuer != configuredIssuer { - return nil, fmt.Errorf("metadata: issuer mismatch: expected %q, got %q", configuredIssuer, metaIssuer) + // RFC 8414 §3.3 requires the metadata "issuer" to be identical to the + // configured issuer, and §4 specifies a code-point-for-code-point comparison + // with no normalization applied. Compare both sides verbatim: a document + // whose issuer differs only by a trailing slash is a different identifier and + // is rejected. Derivation is many-to-one (an issuer and its trailing-slash + // variant share one well-known URL), so the strict comparison turns that + // unavoidable collision into a clean discovery failure rather than a silent + // bind to a different issuer's metadata (the attack RFC 8414 §3.3 and + // RFC 9728 §7.3 exist to defeat). + if meta.Issuer != mc.issuerURL { + return nil, fmt.Errorf("metadata: issuer mismatch: expected %q, got %q", mc.issuerURL, meta.Issuer) } if meta.JWKSURI == "" { return nil, fmt.Errorf("metadata: missing required field \"jwks_uri\"") diff --git a/core/internal/metadata/metadata_test.go b/core/internal/metadata/metadata_test.go index 3a803aa..e8ad3f5 100644 --- a/core/internal/metadata/metadata_test.go +++ b/core/internal/metadata/metadata_test.go @@ -373,6 +373,47 @@ func TestMetadataCache_JWKSURIChange(t *testing.T) { } } +// TestMetadataCache_IssuerTrailingSlashMismatch is the regression for the +// RFC 8414 §3.3 comparison: the configured issuer and the document "issuer" +// are compared byte-for-byte (§4, code-point-for-code-point, no normalization). +// A metadata document whose issuer differs from the configured issuer only by a +// trailing slash is a different identifier and is rejected — a clean discovery +// failure rather than a silent bind to a different issuer's metadata. +func TestMetadataCache_IssuerTrailingSlashMismatch(t *testing.T) { + var serverURL string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/.well-known/oauth-authorization-server" { + w.Header().Set("Content-Type", "application/json") + // Document issuer carries a trailing slash the configured issuer lacks. + meta := ASMetadata{ + Issuer: serverURL + "/", + JWKSURI: serverURL + "/jwks", + } + data, _ := json.Marshal(meta) + w.Write(data) + } else { + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + serverURL = server.URL + + mc := New(Config{ + IssuerURL: server.URL, // configured without a trailing slash + FetchSettings: testSettings(), + RefreshInterval: time.Hour, + }) + defer mc.Close() + + _, err := mc.Get(context.Background()) + if err == nil { + t.Fatal("expected issuer-mismatch error for a trailing-slash difference, got nil") + } + if !strings.Contains(err.Error(), "issuer mismatch") { + t.Errorf("expected issuer mismatch error, got: %v", err) + } +} + // TestMetadataCache_Close_Idempotent verifies that calling Close multiple times // does not panic. func TestMetadataCache_Close_Idempotent(t *testing.T) { @@ -433,3 +474,47 @@ func TestMetadataCache_BothDiscoveryFail(t *testing.T) { t.Fatal("expected error when both discovery paths fail, got nil") } } + +// TestBuildOAuthMetadataURL_TrailingSlash asserts the RFC 8414 §3.1 derivation +// removes a terminating slash on the issuer path before inserting the +// well-known component, so an issuer ending in "/tenant/" derives +// ".../oauth-authorization-server/tenant" (not ".../tenant/"). This keeps the +// derived metadata URL aligned with the byte-for-byte issuer identity. +func TestBuildOAuthMetadataURL_TrailingSlash(t *testing.T) { + tests := []struct { + name string + issuer string + want string + }{ + { + name: "path with trailing slash", + issuer: "https://as.example.com/tenant/", + want: "https://as.example.com/.well-known/oauth-authorization-server/tenant", + }, + { + name: "path without trailing slash", + issuer: "https://as.example.com/tenant", + want: "https://as.example.com/.well-known/oauth-authorization-server/tenant", + }, + { + name: "bare origin", + issuer: "https://as.example.com", + want: "https://as.example.com/.well-known/oauth-authorization-server", + }, + { + // A percent-encoded octet is path data (RFC 3986 §3.3), not a + // delimiter: it must survive verbatim into the derived URL, never + // re-escaped into "%252F" (a 404). + name: "path with encoded octet", + issuer: "https://as.example.com/tenant%2Fx", + want: "https://as.example.com/.well-known/oauth-authorization-server/tenant%2Fx", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := buildOAuthMetadataURL(tt.issuer); got != tt.want { + t.Errorf("buildOAuthMetadataURL(%q) = %q, want %q", tt.issuer, got, tt.want) + } + }) + } +} diff --git a/core/resource/resource.go b/core/resource/resource.go index e0a465e..0c042c3 100644 --- a/core/resource/resource.go +++ b/core/resource/resource.go @@ -6,13 +6,21 @@ import ( "fmt" "maps" "net/url" + "strings" "github.com/authplane/go-sdk/core/resource/verifier" ) // Resource represents a protected resource with PRM generation and token verification. type Resource struct { - uri string + uri string + // parsedURI is uri after New's validation. Keeping it removes three + // re-parses of the same already-validated string (WellKnownPRMPath and two + // in buildPRM) and, more importantly, lets wellKnownPRMPath take a + // *url.URL: as a string-taking function it had to decide what to return on + // a parse failure, and returning the origin-level well-known path handed + // back a plausible-looking wrong answer instead of failing. + parsedURI *url.URL scopes []string issuer string verifier *verifier.TokenVerifier @@ -102,21 +110,63 @@ func (r *Resource) PRMURL() string { // The path is formed by inserting "/.well-known/oauth-protected-resource" // between the host and the path component of the resource URI. // +// Per RFC 9728 §3.1 the terminating slash following the host component is +// removed before insertion, so a resource identifier and its trailing-slash +// variant resolve to the same well-known path. The section says "any +// terminating '/'", which is read here as every one of them: "/mcp//" derives +// the same path as "/mcp/". A single-character strip would leave "/mcp/" for +// the former and "/mcp" for the latter, publishing two documents for what §3.1 +// treats as one identifier. This is derivation, not identity: the resource +// identifier itself is preserved verbatim everywhere it is stored, advertised +// or compared. +// +// The path is derived from the escaped path, so a percent-encoded octet such +// as "%2F" (path data per RFC 3986 §3.3, not a delimiter) is carried through +// unchanged rather than being decoded into a "/" and stripped. +// // Examples: // -// resource URI "https://api.example.com" → "/.well-known/oauth-protected-resource" -// resource URI "https://api.example.com/mcp" → "/.well-known/oauth-protected-resource/mcp" -// resource URI "https://api.example.com/v2/mcp" → "/.well-known/oauth-protected-resource/v2/mcp" +// resource URI "https://api.example.com" → "/.well-known/oauth-protected-resource" +// resource URI "https://api.example.com/mcp" → "/.well-known/oauth-protected-resource/mcp" +// resource URI "https://api.example.com/mcp/" → "/.well-known/oauth-protected-resource/mcp" +// resource URI "https://api.example.com/mcp//" → "/.well-known/oauth-protected-resource/mcp" +// resource URI "https://api.example.com/mcp%2F" → "/.well-known/oauth-protected-resource/mcp%2F" +// resource URI "https://api.example.com/v2/mcp" → "/.well-known/oauth-protected-resource/v2/mcp" func (r *Resource) WellKnownPRMPath() string { - return wellKnownPRMPath(r.uri) + return wellKnownPRMPath(r.parsedURI) } -func wellKnownPRMPath(resourceURI string) string { - u, err := url.Parse(resourceURI) - if err != nil || u.Path == "" || u.Path == "/" { +// wellKnownPRMPath takes an already-parsed URI rather than a string: every +// caller holds one (New parsed it, and the Resource keeps it), and a +// string-taking version had to invent an answer for a parse failure it could +// not actually encounter — returning the origin-level well-known path, which is +// a wrong answer that looks right. +func wellKnownPRMPath(u *url.URL) string { + // Operate on the escaped path, not the decoded u.Path: per RFC 3986 §3.3 a + // percent-encoded octet such as "%2F" is data within a path segment, not the + // "/" delimiter, so it must survive into the derived well-known URL verbatim. + // Using u.Path would decode "%2F" to "/" and then TrimRight would strip it, + // changing the resource's identity. This mirrors buildOAuthMetadataURL, which + // derives the RFC 8414 metadata URL from EscapedPath() for the same reason. + escPath := u.EscapedPath() + if escPath == "" || escPath == "/" { return "/.well-known/oauth-protected-resource" } - return "/.well-known/oauth-protected-resource" + u.Path + // RFC 9728 §3.1: any terminating slash following the host component MUST be + // removed before inserting the well-known path suffix between the host and + // the path component, so "/mcp/" is served at + // ".../oauth-protected-resource/mcp" — the same URL a conformant client + // derives. This strips only a genuine delimiter slash (a "%2F" is left + // intact) and only from the derived URL; the resource identifier is + // unchanged. + // + // TODO(AuthPlane/go-sdk#24): RFC 9728 §3.1 defines the derivation over the + // resource identifier's "path and/or query components"; only the path half + // is handled here, so two identifiers differing only by query collapse onto + // one document. Whether to preserve the query or reject a query-bearing + // identifier at New is an open cross-implementation decision — see the + // issue. + return "/.well-known/oauth-protected-resource" + strings.TrimRight(escPath, "/") } // New creates a new Resource. @@ -135,6 +185,13 @@ func New(uri, issuer string, jwksCache *verifier.JWKSCache, opts ...Option) (*Re if parsed.Scheme == "" || parsed.Host == "" { return nil, fmt.Errorf("resource: resource URI must be absolute with scheme and host, got %q", uri) } + // RFC 8707 §2 forbids a fragment in a resource indicator. url.ParseRequestURI + // does not split a fragment, so "https://api.example.com/mcp#frag" parses with + // the "#frag" folded into Path and would otherwise pass the scheme/host check + // and leak into the derived PRM URL. Reject it explicitly. + if strings.Contains(uri, "#") { + return nil, fmt.Errorf("resource: resource URI must not contain a fragment (RFC 8707 §2), got %q", uri) + } cfg := &resourceConfig{} for _, opt := range opts { @@ -147,10 +204,11 @@ func New(uri, issuer string, jwksCache *verifier.JWKSCache, opts ...Option) (*Re } r := &Resource{ - uri: uri, - scopes: cfg.scopes, - issuer: issuer, - verifier: tv, + uri: uri, + parsedURI: parsed, + scopes: cfg.scopes, + issuer: issuer, + verifier: tv, } r.buildPRM() @@ -244,9 +302,20 @@ func (r *Resource) buildPRM() { r.prmMap = prm r.prmJSON, _ = json.Marshal(prm) - // r.uri was validated by New (url.ParseRequestURI), so url.Parse cannot - // fail here — this is the single, infallible source of truth that - // adapters consume via PRMURL(). - u, _ := url.Parse(r.uri) - r.prmURL = u.ResolveReference(&url.URL{Path: wellKnownPRMPath(r.uri)}).String() + // r.parsedURI is New's own parse of the validated URI — the single, + // infallible source of truth adapters consume via PRMURL(). Reusing it here + // replaces two re-parses of a string that was already parsed once. + // + // Parse the well-known path (rather than assigning it to url.URL.Path + // directly) so its RawPath is populated: wellKnownPRMPath already returns an + // escaped path, and String() would otherwise re-escape a literal "%2F" into + // "%252F". Parsing round-trips the escaping so an encoded "%2F" is preserved. + // + // ResolveReference dereferences ref immediately, so a nil ref would panic. + // That is unreachable: wellKnownPRMPath derives from the already-parsed + // URI's escaped path, so url.Parse of the resulting well-known path cannot + // fail and ref is never nil. The discarded error is therefore safe to ignore. + u := r.parsedURI + ref, _ := url.Parse(wellKnownPRMPath(r.parsedURI)) + r.prmURL = u.ResolveReference(ref).String() } diff --git a/core/resource/resource_test.go b/core/resource/resource_test.go index 8ef3854..a47cfdb 100644 --- a/core/resource/resource_test.go +++ b/core/resource/resource_test.go @@ -179,11 +179,13 @@ func TestPRMURL(t *testing.T) { want: "https://api.example.com/.well-known/oauth-protected-resource/v2/mcp", }, { - // url.ResolveReference preserves trailing slashes in the resource path; - // pin that here so the contract doesn't drift. - name: "trailing slash preserved", + // RFC 9728 §3.1: a terminating slash following the host component is + // removed before insertion, so "/mcp/" derives the same well-known URL + // as "/mcp". This is derivation, not identity — the resource identifier + // itself is preserved verbatim. + name: "trailing slash stripped from derived URL", resourceURI: "https://api.example.com/mcp/", - want: "https://api.example.com/.well-known/oauth-protected-resource/mcp/", + want: "https://api.example.com/.well-known/oauth-protected-resource/mcp", }, } for _, tc := range tests { @@ -215,6 +217,83 @@ func TestPRMURL(t *testing.T) { } } +// TestWellKnownPRMPath_TrailingSlashStripped is the regression for the RFC 9728 +// §3.1 derivation: a resource identifier ending in "/mcp/" derives the PRM +// well-known path with the terminating slash removed, yielding +// "/.well-known/oauth-protected-resource/mcp" — not the trailing-slash form a +// conformant client would 404 on. The identifier is preserved verbatim; only the +// derived URL loses the slash. +func TestWellKnownPRMPath_TrailingSlashStripped(t *testing.T) { + key, err := testutil.GenerateES256Key() + if err != nil { + t.Fatalf("generate key: %v", err) + } + jwksData, err := testutil.BuildJWKSWithKID(&key.PublicKey, testKID) + if err != nil { + t.Fatalf("build jwks: %v", err) + } + jc := verifier.NewJWKSCache(verifier.JWKSCacheConfig{ + FetchFn: func(ctx context.Context) ([]byte, map[string][]string, error) { + return jwksData, nil, nil + }, + DefaultTTL: time.Hour, + }) + t.Cleanup(jc.Close) + + res, err := resource.New("https://api.example.com/mcp/", testIssuer, jc) + if err != nil { + t.Fatalf("resource.New: %v", err) + } + + if got, want := res.WellKnownPRMPath(), "/.well-known/oauth-protected-resource/mcp"; got != want { + t.Errorf("WellKnownPRMPath() = %q, want %q", got, want) + } + if got, want := res.PRMURL(), "https://api.example.com/.well-known/oauth-protected-resource/mcp"; got != want { + t.Errorf("PRMURL() = %q, want %q", got, want) + } + // The resource identifier itself is untouched: RFC 9728 §3.3 uses the + // resource identifier as-is; only the derived well-known URL drops the slash. + if got, want := res.URI(), "https://api.example.com/mcp/"; got != want { + t.Errorf("URI() = %q, want %q (identifier must be preserved verbatim)", got, want) + } +} + +// TestWellKnownPRMPath_EncodedSlashPreserved locks in the distinction between a +// terminating delimiter slash (stripped) and a percent-encoded "%2F", which is +// path data per RFC 3986 §3.3 and must survive into the derived PRM URL. A +// naive strip on the decoded path would turn "/mcp%2F" into ".../mcp", changing +// the resource's identity; a naive URL rebuild would re-escape it into +// "%252F". Both are guarded here. +func TestWellKnownPRMPath_EncodedSlashPreserved(t *testing.T) { + key, err := testutil.GenerateES256Key() + if err != nil { + t.Fatalf("generate key: %v", err) + } + jwksData, err := testutil.BuildJWKSWithKID(&key.PublicKey, testKID) + if err != nil { + t.Fatalf("build jwks: %v", err) + } + jc := verifier.NewJWKSCache(verifier.JWKSCacheConfig{ + FetchFn: func(ctx context.Context) ([]byte, map[string][]string, error) { + return jwksData, nil, nil + }, + DefaultTTL: time.Hour, + }) + t.Cleanup(jc.Close) + + res, err := resource.New("https://api.example.com/mcp%2F", testIssuer, jc) + if err != nil { + t.Fatalf("resource.New: %v", err) + } + + if got, want := res.WellKnownPRMPath(), "/.well-known/oauth-protected-resource/mcp%2F"; got != want { + t.Errorf("WellKnownPRMPath() = %q, want %q", got, want) + } + if got, want := res.PRMURL(), "https://api.example.com/.well-known/oauth-protected-resource/mcp%2F"; got != want { + t.Errorf("PRMURL() = %q, want %q (encoded %%2F must not become %%252F or /)", got, want) + } +} + func TestPRMResponse_DPoPNotConfigured_OmitsDPoPFields(t *testing.T) { res, _ := makeResource(t) prm := res.PRMResponse() @@ -830,6 +909,10 @@ func TestNew_RejectsInvalidResourceURI(t *testing.T) { {"authority-less scheme", "file:///tmp/mcp"}, {"empty", ""}, {"malformed", "://no-scheme"}, + // RFC 8707 §2 forbids a fragment in a resource indicator. url.ParseRequestURI + // folds "#frag" into the path instead of splitting it, so this must be + // rejected explicitly rather than silently leaking into the derived PRM URL. + {"fragment", "https://api.example.com/mcp#frag"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/core/resource/verifier/errors.go b/core/resource/verifier/errors.go index c04573a..733d496 100644 --- a/core/resource/verifier/errors.go +++ b/core/resource/verifier/errors.go @@ -4,6 +4,16 @@ import "errors" // Sentinel errors returned by TokenVerifier. var ( + // ErrInvalidIssuer is returned when an issuer identifier is not the shape + // RFC 8414 requires. Every construction boundary that accepts an issuer + // routes through ValidateIssuer, so this is the single sentinel for all of + // them: NewTokenVerifier, resource.New (which calls it) and + // authplane.NewClient, which needs its own call because a client used only + // for token, introspection and revocation never builds a TokenVerifier. + // + // authplane.ErrInvalidIssuer is an alias of this value, so errors.Is + // matches regardless of which boundary rejected the identifier. + ErrInvalidIssuer = errors.New("verifier: invalid issuer") ErrTokenMissing = errors.New("verifier: token missing") ErrTokenExpired = errors.New("verifier: token expired") ErrInvalidSignature = errors.New("verifier: invalid signature") diff --git a/core/resource/verifier/types_test.go b/core/resource/verifier/types_test.go index 4c811ea..3fb94cf 100644 --- a/core/resource/verifier/types_test.go +++ b/core/resource/verifier/types_test.go @@ -29,8 +29,7 @@ func TestNewDPoPContext_SingleProof(t *testing.T) { } // TestNewDPoPContext_FiltersBlanks ensures whitespace-only entries are -// dropped before the §4.3 cardinality check fires, matching the Java/TS -// reference implementations. +// dropped before the §4.3 cardinality check fires. func TestNewDPoPContext_FiltersBlanks(t *testing.T) { ctx, err := NewDPoPContext("POST", "https://api.example.com/mcp", []string{"", " ", " proof "}) if err != nil { diff --git a/core/resource/verifier/verifier.go b/core/resource/verifier/verifier.go index 10f0b48..6cdfd45 100644 --- a/core/resource/verifier/verifier.go +++ b/core/resource/verifier/verifier.go @@ -2,6 +2,7 @@ package verifier import ( "context" + "errors" "fmt" "net/url" "slices" @@ -38,7 +39,11 @@ type resolvedInboundDPoP struct { // The JWKSCache is injected from outside (the facade manages its lifecycle). func NewTokenVerifier(issuer, audience string, jwksCache *JWKSCache, opts ...Option) (*TokenVerifier, error) { v := &TokenVerifier{ - issuer: strings.TrimRight(issuer, "/"), + // RFC 8414 §4: the issuer is an identifier, stored and compared + // code-point-for-code-point. Keep it verbatim (including any trailing + // slash) so token "iss" is matched byte-for-byte and a trailing-slash + // difference is a mismatch, not something the SDK silently reconciles. + issuer: issuer, audience: audience, jwks: jwksCache, clockSkew: DefaultClockSkew, @@ -54,8 +59,8 @@ func NewTokenVerifier(issuer, audience string, jwksCache *JWKSCache, opts ...Opt v.algorithms = defaultAlgorithms } - if _, err := url.ParseRequestURI(v.issuer); err != nil { - return nil, fmt.Errorf("verifier: invalid issuer URI: %w", err) + if err := ValidateIssuer(v.issuer); err != nil { + return nil, err } if _, err := url.ParseRequestURI(v.audience); err != nil { return nil, fmt.Errorf("verifier: invalid audience URI: %w", err) @@ -242,3 +247,59 @@ func (v *TokenVerifier) VerifyToken(ctx context.Context, rawToken string, dpop * return claims, nil } + +// ValidateIssuer enforces the shape RFC 8414 requires of an issuer identifier. +// +// §2 forbids both a query and a fragment component, and the identifier must be +// an absolute URL with a scheme and host: it anchors the byte-for-byte `iss` +// comparison above and the well-known derivation in internal/metadata, neither +// of which is meaningful for a relative reference. url.ParseRequestURI alone is +// not enough on either count — it accepts "/tenant" (no scheme, no host), and +// it does not split a fragment, so "https://as.example.com/t#frag" parses +// cleanly with the fragment folded into Path. +// +// It is exported so every construction boundary applies one rule rather than a +// copy of it: NewTokenVerifier below, resource.New (which calls it), and +// authplane.NewClient, whose discovery path needs its own gate because a client +// used only for token, introspection and revocation calls never constructs a +// TokenVerifier. +// +// Every error it returns wraps ErrInvalidIssuer and carries a redacted form of +// the identifier — see redactIssuer. +func ValidateIssuer(issuer string) error { + if strings.ContainsAny(issuer, "?#") { + return fmt.Errorf("%w: must not contain a query or fragment (RFC 8414 §2), got %s", ErrInvalidIssuer, redactIssuer(issuer)) + } + parsed, err := url.ParseRequestURI(issuer) + if err != nil { + // Wrap with %w so errors.As(err, new(*url.Error)) keeps working, but + // substitute the URL: url.Error.Error() prints its URL field verbatim + // and does not redact. + var uerr *url.Error + if errors.As(err, &uerr) { + err = &url.Error{Op: uerr.Op, URL: redactIssuer(issuer), Err: uerr.Err} + } + return fmt.Errorf("%w: %w", ErrInvalidIssuer, err) + } + if parsed.Scheme == "" || parsed.Host == "" { + return fmt.Errorf("%w: must be absolute with a scheme and host, got %s", ErrInvalidIssuer, redactIssuer(issuer)) + } + return nil +} + +// redactIssuer renders an issuer identifier for an error message without +// echoing anything credential-shaped. +// +// The branches above fire precisely for malformed identifiers, and the +// query/fragment branch fires for exactly the shape that carries a secret — +// "https://as.example.com?token=…". Echoing the raw value there would put it in +// whatever log the construction error lands in. Only the scheme and host +// survive: url.URL keeps userinfo in User, the query in RawQuery and the +// fragment in Fragment, so Host alone is safe to print. +func redactIssuer(issuer string) string { + parsed, err := url.Parse(issuer) + if err != nil || parsed.Scheme == "" || parsed.Host == "" { + return "(unparseable issuer)" + } + return parsed.Scheme + "://" + parsed.Host + " (path, query and fragment redacted)" +} diff --git a/core/resource/verifier/verifier_issuer_test.go b/core/resource/verifier/verifier_issuer_test.go new file mode 100644 index 0000000..cc64807 --- /dev/null +++ b/core/resource/verifier/verifier_issuer_test.go @@ -0,0 +1,59 @@ +package verifier_test + +import ( + "errors" + "testing" + + "github.com/authplane/go-sdk/core/resource" + "github.com/authplane/go-sdk/core/resource/verifier" +) + +// The issuer gate lives in NewTokenVerifier because that is where all three +// exported construction paths converge: NewTokenVerifier itself, resource.New +// (which calls it), and authplane.NewClient's Resource method. Before this, +// only NewClient checked, so resource.New accepted an issuer it then wrote into +// the PRM document's authorization_servers and compared token `iss` against. +func TestNewTokenVerifierRejectsMalformedIssuer(t *testing.T) { + cases := []struct { + name string + issuer string + }{ + {"fragment", "https://as.example.com/tenant#frag"}, + {"query", "https://as.example.com/tenant?x=1"}, + {"no scheme or host", "/tenant"}, + {"scheme only", "https://"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := verifier.NewTokenVerifier(tc.issuer, "https://api.example.com", nil) + if err == nil { + t.Fatalf("expected %q to be rejected", tc.issuer) + } + if !errors.Is(err, verifier.ErrInvalidIssuer) { + t.Fatalf("expected error to wrap ErrInvalidIssuer, got %v", err) + } + }) + } +} + +func TestNewTokenVerifierAcceptsIssuerWithTerminatingSlash(t *testing.T) { + // The slash is part of the identifier, not a defect: RFC 8414 §4 compares + // code-point-for-code-point, so an AS whose identifier ends in "/" must be + // storable verbatim. + if _, err := verifier.NewTokenVerifier("https://as.example.com/", "https://api.example.com", nil); err != nil { + t.Fatalf("trailing-slash issuer must be accepted, got %v", err) + } +} + +// The reviewer's exact repro: resource.New is exported and callable without a +// Client, so before the gate moved it accepted this issuer outright. +func TestResourceNewRejectsMalformedIssuer(t *testing.T) { + _, err := resource.New("https://api.example.com/mcp", "https://as.example.com/t#frag", nil) + if err == nil { + t.Fatal("resource.New must reject a fragment-bearing issuer") + } + if !errors.Is(err, verifier.ErrInvalidIssuer) { + t.Fatalf("expected error to wrap ErrInvalidIssuer, got %v", err) + } +} diff --git a/core/resource/verifier/verifier_test.go b/core/resource/verifier/verifier_test.go index a97ceac..693fce4 100644 --- a/core/resource/verifier/verifier_test.go +++ b/core/resource/verifier/verifier_test.go @@ -172,6 +172,59 @@ func TestVerifyToken_WrongIssuer(t *testing.T) { } } +// TestVerifyToken_IssuerTrailingSlashPreserved is the regression for the verify +// path: the configured issuer is stored and compared byte-for-byte +// (RFC 8414 §4, code-point-for-code-point, no normalization). A token whose +// "iss" carries the same trailing slash as the configured issuer verifies, and +// one that differs only by the slash is a mismatch — the SDK no longer silently +// reconciles a trailing-slash difference. +func TestVerifyToken_IssuerTrailingSlashPreserved(t *testing.T) { + key, err := testutil.GenerateES256Key() + if err != nil { + t.Fatalf("generate key: %v", err) + } + jwksData, err := testutil.BuildJWKSWithKID(&key.PublicKey, testKID) + if err != nil { + t.Fatalf("build jwks: %v", err) + } + jc := verifier.NewJWKSCache(verifier.JWKSCacheConfig{ + FetchFn: func(ctx context.Context) ([]byte, map[string][]string, error) { + return jwksData, nil, nil + }, + DefaultTTL: time.Hour, + }) + t.Cleanup(jc.Close) + + issuerWithSlash := testIssuer + "/" + v, err := verifier.NewTokenVerifier(issuerWithSlash, testAudience, jc) + if err != nil { + t.Fatalf("create verifier: %v", err) + } + + // (a) Token iss carries the configured trailing slash → verifies. + matching, err := testutil.SignTokenWithClaims(key, jose.ES256, testKID, issuerWithSlash, testAudience, testSubject, testClientID, nil) + if err != nil { + t.Fatalf("sign token: %v", err) + } + claims, err := v.VerifyToken(context.Background(), matching, nil) + if err != nil { + t.Fatalf("token with matching trailing-slash issuer should verify, got: %v", err) + } + if claims.Issuer() != issuerWithSlash { + t.Errorf("iss = %q, want %q", claims.Issuer(), issuerWithSlash) + } + + // A token whose iss drops the slash is a distinct identifier → rejected. + // This guards against re-introducing a trailing-slash normalization. + slashless, err := testutil.SignTokenWithClaims(key, jose.ES256, testKID, testIssuer, testAudience, testSubject, testClientID, nil) + if err != nil { + t.Fatalf("sign token: %v", err) + } + if _, err := v.VerifyToken(context.Background(), slashless, nil); !errors.Is(err, verifier.ErrIssuerMismatch) { + t.Errorf("token whose iss lacks the configured trailing slash: err = %v, want ErrIssuerMismatch", err) + } +} + func TestVerifyToken_WrongAudience(t *testing.T) { v, key := setupES256Verifier(t) @@ -388,44 +441,6 @@ func TestVerifyToken_Scopes(t *testing.T) { } } -func TestVerifyToken_IssuerTrailingSlash(t *testing.T) { - // Verifier configured with trailing slash should still match issuer without. - key, err := testutil.GenerateES256Key() - if err != nil { - t.Fatalf("generate key: %v", err) - } - jwksData, err := testutil.BuildJWKSWithKID(&key.PublicKey, testKID) - if err != nil { - t.Fatalf("build jwks: %v", err) - } - - jc := verifier.NewJWKSCache(verifier.JWKSCacheConfig{ - FetchFn: func(ctx context.Context) ([]byte, map[string][]string, error) { - return jwksData, nil, nil - }, - DefaultTTL: time.Hour, - }) - t.Cleanup(jc.Close) - - v, err := verifier.NewTokenVerifier(testIssuer+"/", testAudience, jc) - if err != nil { - t.Fatalf("create verifier: %v", err) - } - - token, err := testutil.SignTokenWithClaims(key, jose.ES256, testKID, testIssuer, testAudience, testSubject, testClientID, nil) - if err != nil { - t.Fatalf("sign token: %v", err) - } - - claims, err := v.VerifyToken(context.Background(), token, nil) - if err != nil { - t.Fatalf("trailing slash should be trimmed: %v", err) - } - if claims.Sub() != testSubject { - t.Errorf("sub = %q, want %q", claims.Sub(), testSubject) - } -} - func TestVerifyToken_GarbageToken(t *testing.T) { v, _ := setupES256Verifier(t) diff --git a/http/docs/user-guide.md b/http/docs/user-guide.md index a7d37b4..50943d6 100644 --- a/http/docs/user-guide.md +++ b/http/docs/user-guide.md @@ -129,7 +129,7 @@ Standard `net/http` middleware. - Calls `resource.VerifyToken(ctx, token, opts...)`. - On success, injects `*verifier.VerifiedClaims` and the raw token into the request context. - On failure, writes an RFC 6750 response via `resource.AuthErrorResponse`. -- Requests whose path equals `WellKnownPRMPath()` are passed through unauthenticated. +- Requests whose **escaped** path (`r.URL.EscapedPath()`) equals `WellKnownPRMPath()` are passed through unauthenticated. The comparison is on the escaped form on both sides: a resource identifier carrying a percent-encoded octet (e.g. `%2F`) derives a well-known path that keeps it, and comparing the decoded `r.URL.Path` would let `%2F` collapse to `/`, disagree, and return 401 for the discovery endpoint RFC 9728 §3.2 requires to be publicly reachable. ### `(a *Adapter) RequireScopes(scopes ...string) func(http.Handler) http.Handler` diff --git a/http/pkg/authplanehttp/adapter.go b/http/pkg/authplanehttp/adapter.go index 4902228..32347f6 100644 --- a/http/pkg/authplanehttp/adapter.go +++ b/http/pkg/authplanehttp/adapter.go @@ -111,8 +111,8 @@ func (a *Adapter) writeAuthError(w http.ResponseWriter, err error) { // request, in raw form (`EscapedPath`) so reserved percent-encoding // (e.g. `%2F` vs `/`) is preserved per RFC 3986 §6.2.2.2. Query and fragment // are dropped — RFC 9449 §4.3 #5 defines `htu` as the target URI without -// query or fragment; outbound `normalizeHTU` (`core/authplane/dpop.go`) and -// every sibling SDK (rust/cs/java/python) drop them too. +// query or fragment; outbound `normalizeHTU` (`core/authplane/dpop.go`) +// drops them too, so the inbound and outbound sides of the binding agree. // // Operators must mount this middleware **before** any prefix-stripping // router (`http.StripPrefix`) so `r.URL.EscapedPath()` still reflects the @@ -154,7 +154,25 @@ func (a *Adapter) Middleware() func(http.Handler) http.Handler { prmPath := a.resource.WellKnownPRMPath() return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == prmPath { + // Compare the raw request path (EscapedPath), not the decoded + // r.URL.Path: WellKnownPRMPath returns an escaped path, so a + // resource identifier carrying a percent-encoded octet (e.g. + // "%2F") yields a prmPath with that octet intact. Comparing the + // decoded path here would let "%2F" collapse to "/", the two + // sides would disagree, and the PRM discovery endpoint would stop + // being bypassed and return 401 — RFC 9728 §3.2 requires it + // publicly reachable. This mirrors validateHTU, which likewise + // compares EscapedPath for the DPoP htu binding. + // + // This is deliberately stricter than RFC 3986 §6.2.2.1: a + // percent-encoded *unreserved* octet (e.g. "m%63p" for "mcp") + // compares unequal here even though §6.2.2.1 would treat it as + // equivalent to the decoded form. We accept that asymmetry — a + // conformant client derives the well-known path from the resource + // identifier it was given, so it signs the same octets the + // operator configured; the exact-match check keeps the bypass + // surface minimal rather than admitting encoding variants. + if r.URL.EscapedPath() == prmPath { next.ServeHTTP(w, r) return } diff --git a/http/pkg/authplanehttp/adapter_test.go b/http/pkg/authplanehttp/adapter_test.go index 191f69d..836f471 100644 --- a/http/pkg/authplanehttp/adapter_test.go +++ b/http/pkg/authplanehttp/adapter_test.go @@ -132,6 +132,51 @@ func TestMiddlewareSkipsPRMPathWithQueryString(t *testing.T) { } } +// TestMiddlewareSkipsPRMPathWithEncodedOctet locks in the fix for a resource +// identifier carrying a percent-encoded octet (e.g. "%2F"). WellKnownPRMPath +// keeps the octet escaped, so the middleware must compare the raw request path +// (EscapedPath), not the decoded r.URL.Path. Comparing the decoded path would +// let "%2F" collapse to "/", the two sides would disagree, and the PRM +// discovery endpoint would return 401 instead of being bypassed — violating +// RFC 9728 §3.2, which requires it publicly reachable without a token. +func TestMiddlewareSkipsPRMPathWithEncodedOctet(t *testing.T) { + e := newTestEnvForResource(t, "https://api.example.com/mcp%2Fdata") + prmPath := e.adapter.WellKnownPRMPath() + if !strings.Contains(prmPath, "%2F") { + t.Fatalf("WellKnownPRMPath() = %q, want it to preserve the encoded %%2F", prmPath) + } + // The bypass hands off to the PRM handler, which must serve the metadata + // unauthenticated even though the path contains an encoded octet. Wrapping + // the PRM handler directly (rather than a ServeMux) isolates the bypass + // decision from any router-specific handling of "%2F". + handler := e.adapter.Middleware()(e.adapter.PRMHandler()) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, httptest.NewRequestWithContext(t.Context(), http.MethodGet, prmPath, nil)) + if rec.Code != http.StatusOK { + t.Errorf("PRM with encoded octet: status = %d, want 200 (endpoint must be bypassed)", rec.Code) + } + if ct := rec.Header().Get("Content-Type"); ct != "application/json" { + t.Errorf("PRM Content-Type = %q, want application/json", ct) + } + + // Second case: exercise the documented wiring operators actually deploy — + // register the PRM handler on a ServeMux at WellKnownPRMPath() and wrap the + // mux with Middleware(). The bypass must still keep the encoded-octet PRM + // path publicly reachable after the router resolves it, since that is the + // registration/bypass agreement operators depend on. + mux := http.NewServeMux() + mux.Handle(prmPath, e.adapter.PRMHandler()) + muxHandler := e.adapter.Middleware()(mux) + muxRec := httptest.NewRecorder() + muxHandler.ServeHTTP(muxRec, httptest.NewRequestWithContext(t.Context(), http.MethodGet, prmPath, nil)) + if muxRec.Code != http.StatusOK { + t.Errorf("PRM with encoded octet via mux: status = %d, want 200 (endpoint must stay publicly reachable)", muxRec.Code) + } + if ct := muxRec.Header().Get("Content-Type"); ct != "application/json" { + t.Errorf("PRM Content-Type via mux = %q, want application/json", ct) + } +} + // Middleware tests func TestMiddlewareNoToken(t *testing.T) { diff --git a/llm-full.txt b/llm-full.txt index 240d0ea..4b9ead4 100644 --- a/llm-full.txt +++ b/llm-full.txt @@ -207,7 +207,8 @@ Top-level public types under `github.com/authplane/go-sdk/core/...`: | `DPoPContext`, `DPoPReplayStore`, `InMemoryDPoPReplayStore`, `NewInMemoryDPoPReplayStore` | `verifier` | Per-request DPoP context + replay-store interface | | `InboundDPoPOptions`, `InboundDPoPView`, `WithInboundDPoP` | `verifier` | Resource-level inbound DPoP policy bundle (replay store, MaxProofAge, ClockSkew, AllowedProofAlgorithms, Required); configures the verifier and drives PRM advertisement | | `RevocationChecker`, `NullRevocationChecker`, `WithRevocationChecker`, `WithFailClosed`, `WithAlgorithms`, `WithClockSkew` | `verifier` | Revocation + verifier configuration | -| Verifier error sentinels (`ErrTokenMissing`, `ErrTokenExpired`, `ErrInvalidSignature`, `ErrInvalidClaims`, `ErrTokenRevoked`, `ErrInsufficientScope`, `ErrDPoPRequired`, `ErrDPoPNotSupported`, `ErrDPoPInvalid`, `ErrDPoPKeyMismatch`, `ErrDPoPReplayDetected`, `ErrJWKSUnavailable`, ...) | `verifier` | Test with `errors.Is`; map to HTTP via `resource.HTTPStatus` | +| `ValidateIssuer` | `verifier` | RFC 8414 §2 issuer-shape rule (no query, no fragment, absolute with scheme and host). The single gate every construction boundary calls | +| Verifier error sentinels (`ErrInvalidIssuer`, `ErrTokenMissing`, `ErrTokenExpired`, `ErrInvalidSignature`, `ErrInvalidClaims`, `ErrTokenRevoked`, `ErrInsufficientScope`, `ErrDPoPRequired`, `ErrDPoPNotSupported`, `ErrDPoPInvalid`, `ErrDPoPKeyMismatch`, `ErrDPoPReplayDetected`, `ErrJWKSUnavailable`, ...) | `verifier` | Test with `errors.Is`; map to HTTP via `resource.HTTPStatus`. `authplane.ErrInvalidIssuer` is an alias of `verifier.ErrInvalidIssuer` | ## References diff --git a/mark3labs/docs/user-guide.md b/mark3labs/docs/user-guide.md index 2337884..0017700 100644 --- a/mark3labs/docs/user-guide.md +++ b/mark3labs/docs/user-guide.md @@ -181,7 +181,7 @@ Because `*Adapter` embeds `*authplanehttp.Adapter`, the following methods from t - `Middleware() func(http.Handler) http.Handler` — the underlying middleware (`AuthMiddleware` is a one-line wrapper). - `PRMHandler() http.Handler` — the plain-HTTP PRM handler (`max-age=3600`, no CORS). Prefer `ProtectedResourceMetadataHandler()` below for MCP clients. -- `WellKnownPRMPath() string` — the RFC 9728 well-known path. +- `WellKnownPRMPath() string` — the RFC 9728 well-known path. Derivation strips any terminating slash after the host component (§3.1) and preserves percent-encoded octets in the path; the resource identifier itself is unchanged. - `RequireScopes(scopes ...string) func(http.Handler) http.Handler` — RFC 6750 `insufficient_scope` middleware (useful for non-MCP HTTP routes mounted alongside `/mcp`). ### `(a *Adapter) HTTPContextFunc(opts ...HTTPContextOption) server.HTTPContextFunc` diff --git a/mcp/docs/user-guide.md b/mcp/docs/user-guide.md index 8d6b39a..07399bc 100644 --- a/mcp/docs/user-guide.md +++ b/mcp/docs/user-guide.md @@ -155,6 +155,8 @@ Serves the RFC 9728 PRM JSON. `GET` only; other methods return 405. Sets `Conten Returns the well-known PRM path, e.g. `/.well-known/oauth-protected-resource/mcp`. +Derivation follows RFC 9728 §3.1: any terminating slash after the host component is removed, so `https://api.example.com/mcp/` and `https://api.example.com/mcp` both derive `/.well-known/oauth-protected-resource/mcp`. A percent-encoded octet in the identifier's path survives verbatim — `/mcp%2Fx` derives `/.well-known/oauth-protected-resource/mcp%2Fx` — because RFC 3986 §3.3 makes it path data, not a delimiter. The resource identifier itself is never rewritten; only the derived publication URL loses the slash. + ### `(a *Adapter) TokenExchange(ctx context.Context, input authplane.TokenExchangeInput) (*authplane.TokenResponse, error)` Performs RFC 8693 token exchange via the underlying client. Automatically maps `*authplane.ConsentRequiredError` with a non-empty `ConsentURL` to `mcp.URLElicitationRequiredError` (see §7). Requires credentials (`WithClientCredentials` or `WithClientAuthentication`) in `ClientOptions`.