From b54ad323e202f1eaa95ffc2eac1ad27b0f68f434 Mon Sep 17 00:00:00 2001 From: Roberto Iskandarani Date: Wed, 5 Aug 2026 08:45:59 -0300 Subject: [PATCH] fix(resource,metadata,verifier): preserve issuer identity, derive-only slash handling, escaped-path PRM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - core/authplane: NewClient rejects an issuer carrying a query or fragment component (RFC 8414 §2) instead of letting the two discovery-URL builders diverge on it. - core/internal/metadata: the RFC 8414 §3.3 issuer check compares configured and document issuer byte-for-byte (§4, no normalization) instead of trailing-slash-insensitively. - core/resource/verifier: the token verifier stores the configured issuer verbatim and matches iss byte-for-byte; a trailing-slash mismatch is now ErrIssuerMismatch. - core/resource: resource.New rejects fragment-bearing resource URIs (RFC 8707 §2); the RFC 9728 §3.1 PRM well-known URL strips a terminating slash at derivation only, and WellKnownPRMPath()/PRMURL() derive from the escaped path so percent-encoded octets survive verbatim (RFC 3986 §3.3). - http: the PRM discovery bypass compares EscapedPath() against the escaped well-known path, so percent-encoded resource identifiers keep the discovery endpoint publicly reachable (RFC 9728 §3.2). - ci: single-source the conformance catalog pin in .conformance-catalog-ref (40-hex guarded) with a scheduled drift check against the catalog tip. --- .conformance-catalog-ref | 1 + .github/workflows/ci.yml | 20 +++- .../workflows/conformance-catalog-drift.yml | 77 ++++++++++++++++ .github/workflows/release.yml | 27 ++++-- CHANGELOG.md | 11 +++ CONTRIBUTING.md | 13 +++ RELEASE_GUIDE.md | 2 +- core/authplane/client.go | 20 ++++ core/authplane/client_test.go | 33 +++++++ core/authplane/types.go | 7 ++ core/conformancetests/README.md | 5 + core/internal/metadata/metadata.go | 51 ++++++++--- core/internal/metadata/metadata_test.go | 85 +++++++++++++++++ core/resource/resource.go | 65 +++++++++++-- core/resource/resource_test.go | 91 ++++++++++++++++++- core/resource/verifier/types_test.go | 3 +- core/resource/verifier/verifier.go | 7 +- core/resource/verifier/verifier_test.go | 91 +++++++++++-------- http/pkg/authplanehttp/adapter.go | 24 ++++- http/pkg/authplanehttp/adapter_test.go | 45 +++++++++ 20 files changed, 602 insertions(+), 76 deletions(-) create mode 100644 .conformance-catalog-ref create mode 100644 .github/workflows/conformance-catalog-drift.yml diff --git a/.conformance-catalog-ref b/.conformance-catalog-ref new file mode 100644 index 0000000..e650be5 --- /dev/null +++ b/.conformance-catalog-ref @@ -0,0 +1 @@ +d8e8bc3c18c7a8c9f10c6acdc9960214259fdb41 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e5d1adf..da0ac2d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,9 +39,23 @@ jobs: - name: Clone shared conformance catalog (out of tree) if: matrix.module == 'core' run: | - git -c advice.detachedHead=false clone --depth=1 \ - https://github.com/AuthPlane/conformance.git \ - "$RUNNER_TEMP/conformance" + # Conformance catalog pinned by SHA (was: clone of the latest default + # branch). The ref is single-sourced from the tracked + # .conformance-catalog-ref file at the repo root — bump it there when + # adopting new catalog cases, together with the SDK-side conformance + # coverage, so a catalog change can never break CI on its own. The + # checkout step above populates $GITHUB_WORKSPACE before this read. + # Source: github.com/AuthPlane/conformance. + CONFORMANCE_CATALOG_REF="$(cat "$GITHUB_WORKSPACE/.conformance-catalog-ref")" + # Guard against un-pinning: the ref must be a full commit SHA, not a + # branch/tag name (which would silently track a moving target). + grep -Eq '^[0-9a-f]{40}$' <<< "$CONFORMANCE_CATALOG_REF" \ + || { echo "::error::.conformance-catalog-ref must be a 40-hex commit SHA"; exit 1; } + git init -q "$RUNNER_TEMP/conformance" + git -C "$RUNNER_TEMP/conformance" \ + fetch --depth=1 https://github.com/AuthPlane/conformance.git "$CONFORMANCE_CATALOG_REF" \ + || { echo "::error::Pinned conformance catalog ref $CONFORMANCE_CATALOG_REF is unreachable"; exit 1; } + git -C "$RUNNER_TEMP/conformance" checkout -q FETCH_HEAD - name: Setup Go uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 diff --git a/.github/workflows/conformance-catalog-drift.yml b/.github/workflows/conformance-catalog-drift.yml new file mode 100644 index 0000000..6fe10cd --- /dev/null +++ b/.github/workflows/conformance-catalog-drift.yml @@ -0,0 +1,77 @@ +name: Conformance Catalog Drift + +# The main CI (ci.yml) and release (release.yml) workflows pin the shared +# conformance catalog to a fixed SHA (.conformance-catalog-ref) so a catalog +# change can never break PR CI on its own. The trade-off is that new catalog +# cases stay invisible until someone bumps the pin. This job closes that gap: +# on a weekly schedule it runs the SDK's catalog-alignment check against the +# LATEST (unpinned) default branch of the catalog and FAILS the job on any +# drift, so the scheduled run goes red and GitHub notifies maintainers (the +# same convention as security.yml). This workflow has no pull_request trigger, +# so a failure here can never block a PR. +# +# When this job fails on drift, adopt the new cases in core/conformancetests/ +# and bump .conformance-catalog-ref to the new catalog SHA in the same change. + +on: + schedule: + # Mondays at 06:00 UTC. + - cron: "0 6 * * 1" + workflow_dispatch: + +# Least-privilege default: this workflow only reads the repo. +permissions: + contents: read + +jobs: + drift: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + # Clone the catalog's DEFAULT branch (latest, unpinned) — deliberately + # NOT the pinned .conformance-catalog-ref — so newly added cases show up. + # Cloned to $RUNNER_TEMP, outside $GITHUB_WORKSPACE, so it stays out of + # the working tree. Source: github.com/AuthPlane/conformance. + - name: Clone latest conformance catalog (out of tree) + run: | + git clone --depth=1 https://github.com/AuthPlane/conformance.git \ + "$RUNNER_TEMP/conformance" + + - name: Setup Go + uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 + with: + go-version-file: "core/go.mod" + check-latest: true + cache-dependency-path: "core/go.sum" + + # The alignment check runs in TestMain, AFTER m.Run(), so the full suite + # must execute for every Case() registration to fire — the same command + # ci.yml runs, just pointed at the latest catalog. TestMain then fails the + # suite if the latest catalog holds a case ID with no matching Case() + # registration, which fails this step and the job — a red scheduled run is + # the signal GitHub notifies on. This workflow has no pull_request + # trigger, so the failure never blocks a PR. + - name: Run catalog-alignment check against latest catalog + id: align + working-directory: core + env: + CONFORMANCE_CATALOG_PATH: ${{ runner.temp }}/conformance/oauth-sdk-conformance-catalog.yaml + run: go test ./conformancetests/ -v + + - name: Report drift + if: always() + run: | + if [ "${{ steps.align.outcome }}" = "success" ]; then + echo "Conformance catalog alignment: no drift against the latest catalog." >> "$GITHUB_STEP_SUMMARY" + else + echo "::warning::Conformance catalog drift detected — the latest catalog has cases not yet covered by the SDK. Adopt them in core/conformancetests/ and bump .conformance-catalog-ref." + { + echo "## Conformance catalog drift detected" + echo "" + echo "The latest (unpinned) conformance catalog contains cases the SDK does not yet cover, or the alignment check otherwise failed." + echo "" + echo "**Next steps:** adopt the new cases in \`core/conformancetests/\` and bump \`.conformance-catalog-ref\` to the new catalog SHA in the same change." + } >> "$GITHUB_STEP_SUMMARY" + fi diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c9721bf..2c7b601 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -102,11 +102,26 @@ jobs: token: ${{ steps.app_token.outputs.token }} - name: Check out shared conformance catalog - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - with: - repository: AuthPlane/conformance - path: conformance - fetch-depth: 1 + run: | + # Conformance catalog pinned by SHA (was: clone of the latest default + # branch). The ref is single-sourced from the tracked + # .conformance-catalog-ref file at the repo root — bump it there when + # adopting new catalog cases, together with the SDK-side conformance + # coverage, so a catalog change can never break CI on its own. The + # repo checkout step above populates $GITHUB_WORKSPACE before this read. + # Fetched into $RUNNER_TEMP — outside $GITHUB_WORKSPACE — so `git add + # -A` in the release commit below never stages it as a gitlink. + # Source: github.com/AuthPlane/conformance. + CONFORMANCE_CATALOG_REF="$(cat "$GITHUB_WORKSPACE/.conformance-catalog-ref")" + # Guard against un-pinning: the ref must be a full commit SHA, not a + # branch/tag name (which would silently track a moving target). + grep -Eq '^[0-9a-f]{40}$' <<< "$CONFORMANCE_CATALOG_REF" \ + || { echo "::error::.conformance-catalog-ref must be a 40-hex commit SHA"; exit 1; } + git init -q "$RUNNER_TEMP/conformance" + git -C "$RUNNER_TEMP/conformance" \ + fetch --depth=1 https://github.com/AuthPlane/conformance.git "$CONFORMANCE_CATALOG_REF" \ + || { echo "::error::Pinned conformance catalog ref $CONFORMANCE_CATALOG_REF is unreachable"; exit 1; } + git -C "$RUNNER_TEMP/conformance" checkout -q FETCH_HEAD - name: Set up Go 1.25 uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 @@ -226,7 +241,7 @@ jobs: - name: Run tests in all four modules env: - CONFORMANCE_CATALOG_PATH: ${{ github.workspace }}/conformance/oauth-sdk-conformance-catalog.yaml + CONFORMANCE_CATALOG_PATH: ${{ runner.temp }}/conformance/oauth-sdk-conformance-catalog.yaml run: | (cd core && go test ./...) (cd mcp && go test ./...) diff --git a/CHANGELOG.md b/CHANGELOG.md index a848256..4a063ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- `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/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/CONTRIBUTING.md b/CONTRIBUTING.md index 348c85a..70095c1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -76,6 +76,19 @@ go install golang.org/x/vuln/cmd/govulncheck@latest (cd mcp && govulncheck ./...) ``` +**Conformance catalog:** + +The `core` conformance suite maps to the shared [conformance catalog](https://github.com/AuthPlane/conformance). CI pins the catalog to the SHA tracked in [`.conformance-catalog-ref`](.conformance-catalog-ref) at the repo root, so a catalog change can never break CI on its own. To reproduce CI locally, check out that same ref: + +```bash +git clone https://github.com/AuthPlane/conformance.git /path/to/catalog +git -C /path/to/catalog checkout "$(cat .conformance-catalog-ref)" +export CONFORMANCE_CATALOG_PATH=/path/to/catalog/oauth-sdk-conformance-catalog.yaml +(cd core && go test ./conformancetests/ -v) +``` + +A weekly `conformance-catalog-drift` workflow runs the alignment check against the latest catalog and fails when new cases need adopting. When adopting them, update `core/conformancetests/` and bump `.conformance-catalog-ref` in the same change. See [`core/conformancetests/README.md`](core/conformancetests/README.md) for details. + ## Pull Request Guidelines - Branch off `main`. Release branches (`release/v*`, `hotfix/v*`) are managed by the release flow — see [RELEASE_POLICY.md](RELEASE_POLICY.md). diff --git a/RELEASE_GUIDE.md b/RELEASE_GUIDE.md index 58aabfa..bed6d5f 100644 --- a/RELEASE_GUIDE.md +++ b/RELEASE_GUIDE.md @@ -5,7 +5,7 @@ How to ship a new version of the Go SDK (`core`, `http`, `mcp`). All three modul ## Prerequisites - You are a maintainer on `AuthPlane/go-sdk`. -- **`RELEASE_BOT_APP_ID`** and **`RELEASE_BOT_PRIVATE_KEY`** are set as organization secrets scoped to this repo. The Release Bot GitHub App mints a short-lived token used to push the four annotated tags and check out the conformance catalog. (`ci.yml` does not need these — the conformance repo is public.) `release.yml` fails fast with a clear error if either secret is missing — the workflow will not silently proceed. +- **`RELEASE_BOT_APP_ID`** and **`RELEASE_BOT_PRIVATE_KEY`** are set as organization secrets scoped to this repo. The Release Bot GitHub App mints a short-lived token used to push the five annotated tags. (`ci.yml` does not need these — the conformance repo is public.) `release.yml` fails fast with a clear error if either secret is missing — the workflow will not silently proceed. - `CHANGELOG.md` on `main` has a populated `## [Unreleased]` section. There is no registry to configure. `proxy.golang.org` polls public tags and begins serving the new module versions within seconds of the atomic push — **the tag push is the publish**. diff --git a/core/authplane/client.go b/core/authplane/client.go index e2e1f21..983c161 100644 --- a/core/authplane/client.go +++ b/core/authplane/client.go @@ -59,6 +59,26 @@ 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 is deliberately scoped to the query/fragment gate. Full issuer + // URL-shape validation (requiring a scheme and host, matching the checks + // resource.New applies to the resource URI) is a tracked follow-up. + if strings.ContainsAny(issuer, "?#") { + return nil, fmt.Errorf("%w: must not contain a query or fragment (RFC 8414 §2), got %q", ErrInvalidIssuer, issuer) + } + // 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..0112e91 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,38 @@ 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 + }{ + {"query", "https://as.example.com/tenant?x=1"}, + {"fragment", "https://as.example.com/tenant#frag"}, + {"both", "https://as.example.com/tenant?x=1#frag"}, + } + 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(), "query or fragment") { + t.Fatalf("expected query/fragment rejection message, got %v", err) + } + }) + } +} + func TestNewClient_NoCredentials(t *testing.T) { server, serverURL := mockAS(t) defer server.Close() diff --git a/core/authplane/types.go b/core/authplane/types.go index 8f85702..4868113 100644 --- a/core/authplane/types.go +++ b/core/authplane/types.go @@ -1,10 +1,17 @@ package authplane import ( + "errors" + "github.com/authplane/go-sdk/core/internal/oauth" "github.com/authplane/go-sdk/core/internal/ssrf" ) +// ErrInvalidIssuer is returned by NewClient when the issuer identifier is not +// acceptable — currently, when it carries a query or fragment component, which +// RFC 8414 §2 forbids. Callers can match it with errors.Is. +var ErrInvalidIssuer = errors.New("authplane: invalid issuer") + // TokenResponse is an OAuth 2.0 token endpoint response. type TokenResponse = oauth.TokenResponse diff --git a/core/conformancetests/README.md b/core/conformancetests/README.md index 61fc42e..fbef6b9 100644 --- a/core/conformancetests/README.md +++ b/core/conformancetests/README.md @@ -85,12 +85,17 @@ Set `CONFORMANCE_CATALOG_PATH` (or `AUTHPLANE_CONFORMANCE_CATALOG`) to the absol # Clone the catalog repo anywhere git clone git@github.com:AuthPlane/conformance.git /path/to/catalog +# Check out the same pinned ref CI uses (single-sourced at the repo root) +git -C /path/to/catalog checkout "$(cat /path/to/go-sdk/.conformance-catalog-ref)" + # Point the harness at it export CONFORMANCE_CATALOG_PATH=/path/to/catalog/oauth-sdk-conformance-catalog.yaml ``` This is useful in CI or when the catalog is not a sibling directory. +CI pins the catalog to the SHA tracked in [`.conformance-catalog-ref`](../../.conformance-catalog-ref) at the repo root, so a catalog change can never break CI on its own. Check out that same ref locally (as above) to match CI exactly. A weekly `conformance-catalog-drift` workflow runs the alignment check against the latest catalog and fails when new cases need adopting; adopt them here and bump `.conformance-catalog-ref` in the same change. + ## Running ```bash 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..126ef9b 100644 --- a/core/resource/resource.go +++ b/core/resource/resource.go @@ -6,6 +6,7 @@ import ( "fmt" "maps" "net/url" + "strings" "github.com/authplane/go-sdk/core/resource/verifier" ) @@ -102,21 +103,55 @@ 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 any 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. 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%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) } func wellKnownPRMPath(resourceURI string) string { u, err := url.Parse(resourceURI) - if err != nil || u.Path == "" || u.Path == "/" { + if err != nil { return "/.well-known/oauth-protected-resource" } - return "/.well-known/oauth-protected-resource" + u.Path + // 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" + } + // 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: RFC 9728 §3.1 defines the derivation over the resource + // identifier's "path and/or query components"; only the path half is + // handled here. Carrying a query component into the well-known URL is a + // deferred follow-up. + return "/.well-known/oauth-protected-resource" + strings.TrimRight(escPath, "/") } // New creates a new Resource. @@ -135,6 +170,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 { @@ -247,6 +289,17 @@ func (r *Resource) buildPRM() { // 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(). + // + // 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. u, _ := url.Parse(r.uri) - r.prmURL = u.ResolveReference(&url.URL{Path: wellKnownPRMPath(r.uri)}).String() + // ResolveReference dereferences ref immediately, so a nil ref would panic. + // That is unreachable for the same reason u is safe: wellKnownPRMPath derives + // from the already-parsed r.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. + ref, _ := url.Parse(wellKnownPRMPath(r.uri)) + 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/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..aa71833 100644 --- a/core/resource/verifier/verifier.go +++ b/core/resource/verifier/verifier.go @@ -5,7 +5,6 @@ import ( "fmt" "net/url" "slices" - "strings" "time" "github.com/go-jose/go-jose/v4" @@ -38,7 +37,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, 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/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) {