From c77a8f72fb6db0310b8bed8d6ec25565ce3b08c7 Mon Sep 17 00:00:00 2001 From: Roberto Iskandarani Date: Fri, 7 Aug 2026 13:53:10 -0300 Subject: [PATCH 1/5] ci: pin the conformance catalog by SHA instead of cloning its default branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI cloned github.com/AuthPlane/conformance at its default branch, so any change to the catalog reached this repo immediately — a case added there could turn an unrelated PR red here, with nothing in this repo having changed. Pinning decouples them: a catalog change arrives only when this repo deliberately bumps the pin together with the coverage for it. The revision is single-sourced in a tracked .conformance-catalog-ref, guarded by a ^[0-9a-f]{40}$ shape check so a branch or tag name cannot silently un-pin CI, and read by both ci.yml and release.yml through one script rather than a copy-pasted block — the pin was single-sourced before, the logic reading it was not. A weekly conformance-catalog-drift workflow clones the unpinned tip and runs the alignment assertion, so new cases surface as an early warning instead of a surprise. Its report step distinguishes real drift from a harness failure by matching the two messages that mean drift rather than the marker prefix, which is also emitted when the catalog cannot be read at all. The alignment step declares shell: bash so the pipeline gets pipefail — without it the step takes tee's exit status and reports success unconditionally. The catalog now lands in $RUNNER_TEMP rather than the workspace, so it stays out of `go list ./...`, coverage globs, and `git add -A` in the release commit. No SDK source changes. Verified locally: the fetch script runs end-to-end against the pinned revision, and the conformance suite passes against the catalog it checks out — the repo already aligns with this revision, so the pin adopts no new cases. --- .conformance-catalog-ref | 1 + .github/scripts/fetch-conformance-catalog.sh | 53 +++++++++ .github/workflows/ci.yml | 6 +- .../workflows/conformance-catalog-drift.yml | 103 ++++++++++++++++++ .github/workflows/release.yml | 9 +- 5 files changed, 162 insertions(+), 10 deletions(-) create mode 100644 .conformance-catalog-ref create mode 100755 .github/scripts/fetch-conformance-catalog.sh 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..efa9db0 --- /dev/null +++ b/.conformance-catalog-ref @@ -0,0 +1 @@ +b4c758a7dac698d7fcacd32dafcd4bb2f5dbddaf diff --git a/.github/scripts/fetch-conformance-catalog.sh b/.github/scripts/fetch-conformance-catalog.sh new file mode 100755 index 0000000..5652a5b --- /dev/null +++ b/.github/scripts/fetch-conformance-catalog.sh @@ -0,0 +1,53 @@ +#!/usr/bin/env bash +# +# Fetch the conformance catalog at the revision this repo pins. +# +# The catalog lives in github.com/AuthPlane/conformance and is updated +# independently of this repo, so cloning its default branch would let a catalog +# change turn an unrelated PR red here. The ref is pinned instead, single-sourced +# from the tracked .conformance-catalog-ref at the repo root — bump it there when +# adopting new catalog cases, together with the coverage for them, so a catalog +# change can never break CI on its own. +# +# This script exists because the read/guard/fetch sequence is needed by more than +# one workflow (ci.yml and release.yml). Keeping it inline in both meant the +# guard could be tightened in one and not the other; the pin was single-sourced +# but the logic reading it was not. +# +# Clones into $RUNNER_TEMP — outside $GITHUB_WORKSPACE — so the catalog stays out +# of the working tree: it must never trip `go list ./...` or a coverage glob, and +# `git add -A` in the release commit must never stage it as a gitlink. +# +# Requires: GITHUB_WORKSPACE, RUNNER_TEMP. + +set -euo pipefail + +: "${GITHUB_WORKSPACE:?GITHUB_WORKSPACE must be set}" +: "${RUNNER_TEMP:?RUNNER_TEMP must be set}" + +REF_FILE="$GITHUB_WORKSPACE/.conformance-catalog-ref" +DEST="$RUNNER_TEMP/conformance" +CATALOG_REPO="https://github.com/AuthPlane/conformance.git" + +if [[ ! -f "$REF_FILE" ]]; then + echo "::error::$REF_FILE is missing; the conformance catalog revision is unpinned" + exit 1 +fi + +CONFORMANCE_CATALOG_REF="$(tr -d '[:space:]' < "$REF_FILE")" + +# Guard against un-pinning: the ref must be a full commit SHA, not a branch or +# tag name, either of which would silently track a moving target. +if ! grep -Eq '^[0-9a-f]{40}$' <<< "$CONFORMANCE_CATALOG_REF"; then + echo "::error::.conformance-catalog-ref must be a 40-hex commit SHA, got '$CONFORMANCE_CATALOG_REF'" + exit 1 +fi + +git init -q "$DEST" +if ! git -C "$DEST" fetch --depth=1 "$CATALOG_REPO" "$CONFORMANCE_CATALOG_REF"; then + echo "::error::Pinned conformance catalog ref $CONFORMANCE_CATALOG_REF is unreachable" + exit 1 +fi +git -C "$DEST" checkout -q FETCH_HEAD + +echo "Conformance catalog checked out at $CONFORMANCE_CATALOG_REF" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e5d1adf..42dd3dd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -38,10 +38,8 @@ jobs: # conformance suite). - 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" + shell: bash + run: .github/scripts/fetch-conformance-catalog.sh - 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..700135a --- /dev/null +++ b/.github/workflows/conformance-catalog-drift.yml @@ -0,0 +1,103 @@ +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 + # `shell: bash` is load-bearing, not decoration. The default shell for a + # `run:` step on Linux is `bash -e {0}`, which does NOT set pipefail, so + # the pipeline below would exit with tee's status — always 0 — and this + # step would report success no matter what `go test` did. `shell: bash` + # is what adds `-o pipefail`. + shell: bash + run: go test ./conformancetests/ -v 2>&1 | tee "$RUNNER_TEMP/align.log" + + - 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" + elif ! grep -qE "has no conformance test|registers unknown case" "$RUNNER_TEMP/align.log" 2>/dev/null; then + # Match the two messages that actually mean drift, not the bare + # "CATALOG ALIGNMENT:" prefix. verifyCatalogAlignment emits that + # prefix for three distinct outcomes (catalog_alignment_test.go): + # + # catalog case %q has no conformance test -> drift + # conformance test registers unknown case %q -> drift + # load catalog: read catalog: ... -> harness problem + # + # The third fires when the catalog clone is missing or the path is + # wrong. Grepping the prefix would classify that as drift, which is + # exactly the case this branch exists to separate out. + echo "::warning::The catalog-alignment step failed without a drift message. This is a build or harness problem — a compile error, a failed module download, an unreadable catalog clone, or an unrelated conformance assertion — not catalog drift. Read the step log before touching .conformance-catalog-ref." + { + echo "## Conformance alignment check failed for another reason" + echo "" + echo "The step failed, but its output carries neither drift message" + echo "(\`has no conformance test\` / \`registers unknown case\`)." + echo "That points at a build or harness problem rather than a catalog change." + } >> "$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..482af25 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -102,11 +102,8 @@ 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 + shell: bash + run: .github/scripts/fetch-conformance-catalog.sh - name: Set up Go 1.25 uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 @@ -226,7 +223,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 ./...) From 8aef58b4df31922e39661bcc3c1fb0a45b728d73 Mon Sep 17 00:00:00 2001 From: Roberto Iskandarani Date: Fri, 7 Aug 2026 14:09:35 -0300 Subject: [PATCH 2/5] docs: document the pinned catalog for contributors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pin is only useful if a contributor can reproduce CI. CONTRIBUTING and the conformance-tests README now say where the ref lives, how to check the catalog out at it, and that adopting new cases means bumping the ref in the same change. Also drops a clause the pin change made false: the release bot token is no longer used to check out the conformance catalog — the fetch script clones it directly, since the repo is public and needs no credential. --- CONTRIBUTING.md | 13 +++++++++++++ RELEASE_GUIDE.md | 2 +- core/conformancetests/README.md | 5 +++++ 3 files changed, 19 insertions(+), 1 deletion(-) 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..5fed83f 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 four 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/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 From 42f38fe26f655e2bd7ec8a438946c698fcc2ab70 Mon Sep 17 00:00:00 2001 From: Roberto Iskandarani Date: Fri, 7 Aug 2026 16:27:17 -0300 Subject: [PATCH 3/5] fix(resource,metadata,verifier,http): 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 Identifiers are identity, not something to normalise. The SDK stripped and reconciled them in several places, and every one of those was a silent rewrite of a value the operator configured. Identity is now preserved verbatim (RFC 8414/9728 §3.3) and slash removal happens only at derivation (§3.1) — the two concerns the old code conflated: - The token verifier stores the configured issuer byte-for-byte and compares a token's `iss` exactly (§4 spells the comparison out as code-point-for-code-point with no normalisation), so an AS whose issuer legitimately ends in `/` stops having every token rejected. - The RFC 8414 §3.3 metadata check compares both sides raw. Derivation is many-to-one, and the strict comparison is what turns an unavoidable collision into a clean discovery failure instead of a silent bind to another issuer's metadata. - PRM derivation reads the escaped path, so a percent-encoded octet survives instead of decoding to a delimiter and changing which resource is named. One rule, one place. `verifier.ValidateIssuer` is exported and every construction boundary calls it — `NewTokenVerifier`, `resource.New`, and `authplane.NewClient`, which needs its own call because a client used only for token, introspection and revocation never builds a verifier and would otherwise have no gate at all. The rejected identifier is redacted to scheme and host: that branch fires precisely for a credential-shaped query, and a construction error lands in a startup log. The `net/http` adapter's PRM discovery bypass compares escaped paths on both sides. Comparing the decoded path let `%2F` collapse to `/`, the two sides disagreed, and the discovery endpoint returned 401 — which RFC 9728 §3.2 requires to be publicly reachable. Breaking changes are enumerated in the changelog with migration notes. All are construction-time rejections of configurations that used to start, or output changes on two exported methods. Conformance rows added for the trailing-slash and escaped-path cases the catalog already carries. --- CHANGELOG.md | 19 ++++ core/authplane/client.go | 26 +++++ core/authplane/client_test.go | 82 ++++++++++++++ core/authplane/errors.go | 12 ++ core/conformancetests/rfc8414_test.go | 26 +++++ core/conformancetests/rfc9728_test.go | 11 ++ core/docs/user-guide.md | 4 + core/internal/metadata/metadata.go | 51 +++++++-- core/internal/metadata/metadata_test.go | 85 ++++++++++++++ core/resource/resource.go | 105 +++++++++++++++--- core/resource/resource_test.go | 91 ++++++++++++++- core/resource/verifier/errors.go | 10 ++ core/resource/verifier/types_test.go | 3 +- core/resource/verifier/verifier.go | 67 ++++++++++- .../resource/verifier/verifier_issuer_test.go | 59 ++++++++++ core/resource/verifier/verifier_test.go | 91 ++++++++------- http/docs/user-guide.md | 2 +- http/pkg/authplanehttp/adapter.go | 24 +++- http/pkg/authplanehttp/adapter_test.go | 45 ++++++++ llm-full.txt | 3 +- mark3labs/docs/user-guide.md | 2 +- mcp/docs/user-guide.md | 2 + 22 files changed, 738 insertions(+), 82 deletions(-) create mode 100644 core/authplane/errors.go create mode 100644 core/resource/verifier/verifier_issuer_test.go 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`. From 1a795190f2a4e696f69e7ffe65dda8fd1288a955 Mon Sep 17 00:00:00 2001 From: Roberto Iskandarani Date: Tue, 18 Aug 2026 11:56:57 -0300 Subject: [PATCH 4/5] ci,scripts: accept a tag as --from in backport-fixes.sh, and lint the release scripts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit release.yml deletes release/vX.Y.Z once the tag is pushed, so the tag is the only ref that names those commits afterwards — which is what the release summary tells the operator to pass. --from only resolved branches, so the documented invocation failed at the point it was needed. The resolver now asks the remote what --from and --to name before fetching just those two refs, and validates both as ref names first: they are interpolated into fetch refspecs, and `git ls-remote` matches its arguments as globs, so an unvalidated `--from 'release/*'` fetched a wildcard expansion and left a ref name that is not a commit. `--flag=value` spellings and empty values are now usage errors instead of an exit 1 with no message or a silent fallback to a derived branch name. workflows-lint.yml gains scripts/*.sh: shellcheck plus the new backport-fixes.test.sh suite. A break in these scripts otherwise surfaces only when someone reaches for them right after a release, which is the worst moment to discover it. --- .github/workflows/workflows-lint.yml | 26 +- scripts/backport-fixes.sh | 220 ++++++++++-- scripts/backport-fixes.test.sh | 513 +++++++++++++++++++++++++++ 3 files changed, 730 insertions(+), 29 deletions(-) create mode 100755 scripts/backport-fixes.test.sh diff --git a/.github/workflows/workflows-lint.yml b/.github/workflows/workflows-lint.yml index dfadf36..48cfbda 100644 --- a/.github/workflows/workflows-lint.yml +++ b/.github/workflows/workflows-lint.yml @@ -1,19 +1,29 @@ -name: Lint workflows +name: Release tooling # Catches workflow YAML / shell-in-`run:` regressions at PR time so a # typo can't reach a release tag and surface only when a publish run -# fails. Scoped to changes under `.github/workflows/**` to keep CI -# overhead off unrelated PRs. +# fails. The shell scripts at the top of scripts/ are in the same category +# — a break in them surfaces only when someone reaches for them after a +# release, which is the worst moment to discover it — so they are linted +# and tested here too. +# +# Scoped to `.github/workflows/**` and `scripts/*.sh` to keep CI overhead +# off unrelated PRs. `.github/scripts/*.sh` is deliberately not in scope: +# it is driven by conformance-catalog-drift.yml, not by the release flow +# this job guards, and pulling it in would widen the trigger to every PR +# touching `.github/**`. on: pull_request: paths: - ".github/workflows/**" + - "scripts/*.sh" push: branches: - main paths: - ".github/workflows/**" + - "scripts/*.sh" permissions: contents: read @@ -62,3 +72,13 @@ jobs: # job fails loudly instead of silently degrading. - name: Run actionlint run: actionlint -color -shellcheck=shellcheck + + - name: Shellcheck the release scripts + run: shellcheck scripts/*.sh + + # backport-fixes.sh accepts a branch or a tag as --from, and only the + # branch form has a remote-tracking ref. The tag form is what the release + # flow tells you to use once release.yml has deleted the branch, so it is + # the form least likely to be exercised before it is needed. + - name: Test backport-fixes.sh + run: scripts/backport-fixes.test.sh diff --git a/scripts/backport-fixes.sh b/scripts/backport-fixes.sh index ac18df7..518063e 100755 --- a/scripts/backport-fixes.sh +++ b/scripts/backport-fixes.sh @@ -1,9 +1,10 @@ #!/usr/bin/env bash set -euo pipefail -# Cherry-pick commits from a release/hotfix branch to a local backport -# branch off main (or another target). Does NOT push, create PRs, or -# touch remotes beyond `git fetch`. +# Cherry-pick commits from a release/hotfix branch — or from the tag that +# names them once the branch is gone — to a local backport branch off main +# (or another target). Does NOT push, create PRs, or touch remotes beyond +# `git fetch`. # # Conflicts use git's native cherry-pick state machine — resolve, then # `git cherry-pick --continue` (or --skip / --abort). Re-running this @@ -16,22 +17,34 @@ set -euo pipefail usage() { cat <<'EOF' Usage: - backport-fixes.sh --from [--to ] [--branch ] + backport-fixes.sh --from [--to ] [--branch ] Options: - --from Source branch on origin (e.g. release/v0.6.0, - hotfix/v0.5.1). Do not include 'origin/'. Required. - --to Target branch on origin (default: main). + --from + Source branch OR tag on origin (e.g. release/v0.6.0, + hotfix/v0.5.1, v0.6.0). Do not include 'origin/'. + Required. After a release, release.yml has deleted + release/vX.Y.Z, so the tag is the only ref naming + those commits — which is what release.yml's summary + and backport-fixes.yml both tell you to pass. A + branch wins if a branch and a tag share the name. + --to Target branch on origin (default: main). Branch only: + backport-fixes.yml opens a PR with --base, which + needs a branch that exists on the remote. --branch Name for the local backport branch (default: `backport/vX.Y.Z` derived from --from when it - matches release/vX.Y.Z or hotfix/vX.Y.Z; otherwise - `backport/`). + matches release/vX.Y.Z or hotfix/vX.Y.Z. Anything + else — a tag included — is flattened into + `backport/`, so --from v0.6.0 gives + `backport/v0.6.0`). -h, --help Show this help. Behavior: - 1. Fetches origin. - 2. Lists commits on origin/ that aren't already on origin/, - and commits that are already there (skipped). + 1. Asks origin what and name (git ls-remote), then fetches + those two refs — not the whole remote. + 2. Lists commits on the resolved ref — origin/ for a + branch, refs/tags/ for a tag — that aren't already on + origin/, and commits that are already there (skipped). 3. Creates the backport branch off origin/. 4. Runs `git cherry-pick -x` with the candidates, oldest-first. 5. On conflict: stops. Resolve, then `git cherry-pick --continue`. @@ -49,12 +62,28 @@ EOF FROM="" TO="main" BRANCH_OVERRIDE="" +BRANCH_SET=0 while [[ $# -gt 0 ]]; do case "$1" in - --from) FROM="${2-}"; shift 2 ;; - --to) TO="${2-}"; shift 2 ;; - --branch) BRANCH_OVERRIDE="${2-}"; shift 2 ;; + # Both spellings. `--from=v1.0.0` used to take the `*)` arm and exit 2 as an + # unknown argument, and a trailing `--from` with no value exited 1 with no + # message at all — `shift 2` fails under `set -e`. Neither is a defensible + # answer for the flag --help now leads with. + --from=*) FROM="${1#*=}"; shift ;; + --to=*) TO="${1#*=}"; shift ;; + --branch=*) BRANCH_OVERRIDE="${1#*=}"; BRANCH_SET=1; shift ;; + --from|--to|--branch) + if [[ $# -lt 2 ]]; then + echo "error: $1 requires a value" >&2 + exit 2 + fi + case "$1" in + --from) FROM="$2" ;; + --to) TO="$2" ;; + --branch) BRANCH_OVERRIDE="$2"; BRANCH_SET=1 ;; + esac + shift 2 ;; -h|--help) usage; exit 0 ;; *) echo "error: unknown argument: $1" >&2; usage >&2; exit 2 ;; esac @@ -69,6 +98,15 @@ if [[ -z "$TO" ]]; then echo "error: --to cannot be empty" >&2 exit 2 fi +# An empty --branch used to fall through to the derived name and exit 0, because +# "not passed" and "passed empty" are the same empty string in BRANCH_OVERRIDE — +# whereas --to, which defaults to a non-empty value, caught its own empty form +# above. BRANCH_SET is what separates the two, so `--branch=` is now the usage +# error it obviously is rather than a silent fallback to a name nobody asked for. +if [[ "$BRANCH_SET" -eq 1 && -z "$BRANCH_OVERRIDE" ]]; then + echo "error: --branch cannot be empty" >&2 + exit 2 +fi if [[ "$FROM" == origin/* || "$TO" == origin/* ]]; then echo "error: branch names must not include 'origin/'" >&2 exit 2 @@ -78,12 +116,45 @@ if [[ "$FROM" == "$TO" ]]; then exit 2 fi -# Must be in a git repo +# Both values are interpolated into fetch refspecs below, so they have to be +# valid ref names before they get anywhere near one. `git ls-remote` matches its +# arguments as globs and `*` is legal in a refspec too, so an unvalidated +# `--from 'release/*'` passed the resolver, fetched wildcard-expanded, and left +# a ref name that is not a commit. +# +# `git check-ref-format` is git's own definition of the grammar, so this rejects +# `*`, `?`, `[`, `~`, `^`, `:`, `..`, control characters, trailing `.lock` and +# the rest without keeping a hand-written metacharacter list here that would +# drift from git's. Checking before the network also means a typo costs no round +# trip and touches no refs. +require_ref_name() { + if ! git check-ref-format "refs/heads/$2"; then + echo "error: $1 '$2' is not a valid git ref name" >&2 + exit 2 + fi +} + +# Must be in a git repo. Ahead of the ref-name checks on purpose: those need no +# repo, so running them first meant someone outside a repo was told their ref +# name was wrong, fixed it, and only then learned the real blocker. The one +# precondition nothing else can proceed without is reported first. if ! git rev-parse --git-dir >/dev/null 2>&1; then echo "error: not inside a git repository" >&2 exit 1 fi +require_ref_name --from "$FROM" +require_ref_name --to "$TO" +# --branch was the one ref-shaped flag that skipped this, so a bad value survived +# all the way to `git checkout -b` and failed with git's exit 128 — after the +# ls-remote, the fetch and the printed candidate list. Nothing unsafe reached +# git: `-b` consumes the next word, so no value could turn into an option. It is +# the exit code and the timing that were wrong, and both are the same principle +# the --from guard above is built on: a usage error costs no round trip. +if [[ -n "$BRANCH_OVERRIDE" ]]; then + require_ref_name --branch "$BRANCH_OVERRIDE" +fi + # Detect in-progress cherry-pick first — gives a more actionable error # than the generic dirty-tree check, which also trips during a conflict. if [[ -f "$(git rev-parse --git-dir)/CHERRY_PICK_HEAD" ]]; then @@ -99,21 +170,118 @@ if ! git diff --quiet || ! git diff --cached --quiet; then fi echo "Fetching origin..." -git fetch origin "$FROM" "$TO" --no-tags -if ! git rev-parse --verify "origin/$FROM" >/dev/null 2>&1; then - echo "error: origin/$FROM not found on remote" >&2 +# Ask the remote what every name is in one query, decide what to fetch, then +# fetch once. Asking per name and fetching between the asks cost four round +# trips for a branch --from and five for a tag; over SSH with a hardware-token +# key that is one touch each. It also fetched the source before establishing +# that --to exists, so a bad --to left a new tag or remote-tracking ref behind +# in the user's repo on the way to an error. Nothing is written now until both +# names have resolved. +# +# No --exit-code: without it, "the remote answered and has no such ref" is exit +# 0 with the name simply absent from the output, and any non-zero is +# unambiguously "could not ask" — unreachable, or refused. Reporting that as a +# missing ref sends the operator after a ref that is fine, so that arm exits +# without adding a claim about the ref; git has already described the failure on +# stderr and a guess on top of it would only mislead. +ls_out="" +if ! ls_out="$(git ls-remote origin \ + "refs/heads/$FROM" "refs/tags/$FROM" "refs/heads/$TO")"; then exit 1 fi -if ! git rev-parse --verify "origin/$TO" >/dev/null 2>&1; then - echo "error: origin/$TO not found on remote" >&2 + +# Exact match on the ref name, not a substring: an annotated tag also emits a +# `refs/tags/^{}` peel line, and `refs/heads/v1.0` must not answer for +# `refs/heads/v1.0.1`. +has_remote_ref() { + awk -v want="$1" '$2 == want { hit = 1 } END { exit hit ? 0 : 1 }' <<<"$ls_out" +} + +FROM_REF="" +TO_REF="" +refspecs=() + +# --from accepts a branch or a tag. After a release, release.yml has deleted +# release/vX.Y.Z, so the tag is the only ref naming those commits. A branch wins +# on a name collision, which is what --help states. +# +# A bare-name refspec — `git fetch origin v1.0.0` — writes FETCH_HEAD and +# nothing else: no refs/tags entry, no remote-tracking ref. That is why looking +# the name up as `origin/` afterwards could never see a tag. The explicit +# destinations below materialise it. +if has_remote_ref "refs/heads/$FROM"; then + FROM_REF="origin/$FROM" + refspecs+=("+refs/heads/$FROM:refs/remotes/origin/$FROM") +elif has_remote_ref "refs/tags/$FROM"; then + # A re-cut tag (deleted on origin and re-pushed at a new commit) lands here. + # `+` overwrites the stale local tag, which otherwise keeps pointing at the + # superseded release and would backport the wrong commits. + FROM_REF="refs/tags/$FROM" + refspecs+=("+refs/tags/$FROM:refs/tags/$FROM") +else + echo "error: $FROM not found on origin as a branch or a tag" >&2 + exit 1 +fi + +# --to is branch-only, deliberately. A tag would resolve and `git checkout -b` +# would even work, but backport-fixes.yml opens a PR with `--base "$TO"`, which +# needs a branch that exists on the remote. +if has_remote_ref "refs/heads/$TO"; then + TO_REF="origin/$TO" + refspecs+=("+refs/heads/$TO:refs/remotes/origin/$TO") +else + echo "error: $TO not found on origin as a branch (--to must be a branch)" >&2 + exit 1 +fi + +# Every refspec carries a leading `+`: the bare-name form they replaced still +# got its remote-tracking update through `remote.origin.fetch`, whose refspec is +# forced. Writing the destination out without the `+` silently drops that force, +# and a source branch that was force-pushed — routine during release prep, e.g. +# an amended release commit — stops fast-forwarding and fails a backport that +# used to work. +# +# Without -q on purpose: -q suppresses the per-ref status table, which is where +# `! [rejected]` is written, so a fetch that fails after ls-remote said the ref +# was there would exit with no explanation at all. +# +# --no-tags disables git's automatic tag following only; an explicit +# `refs/tags/` refspec is still fetched, which is what the tag --from path +# depends on. Without it a branch --from downloaded every tag reachable from the +# fetched history as a side effect, so `--from release/v1.0.0` left refs/tags/* +# in the user's repo — refs nobody asked for, and more than the "those two refs +# — not the whole remote" that --help promises. +git fetch --no-tags origin "${refspecs[@]}" || exit 1 + +# Resolution established that the ref exists. This establishes that it names a +# commit, which is not the same claim: a tag may point at a tree or a blob, and +# such a tag has a perfectly valid ref name, so check-ref-format cannot see it +# and ls-remote reports it like any other ref. `git cherry` fatals on it. +# +# --quiet covers "no such ref"; it does not suppress the type-mismatch error, +# which is left alone on purpose — it names the type git actually found, which +# this script cannot. The line below adds only the mapping from what the +# operator typed to the ref it landed on. +if ! git rev-parse --verify --quiet "${FROM_REF}^{commit}" >/dev/null; then + echo "error: $FROM resolved to $FROM_REF, which does not name a commit" >&2 exit 1 fi # `git cherry -v ` prints one line per commit: # + -> not on upstream (candidate for backport) # - -> already on upstream via patch-ID match -cherry_out="$(git cherry -v "origin/$TO" "origin/$FROM" || true)" +# +# Not `|| true`. git-cherry documents no meaningful non-zero exit, so a non-zero +# means it could not answer the question — and swallowing that turns a fatal +# into an empty candidate list, which this script then reports as "Nothing to +# backport" and exit 0: a tooling failure told to the operator as a fact about +# the refs, and a green run in backport-fixes.yml. +cherry_out="" +if ! cherry_out="$(git cherry -v "$TO_REF" "$FROM_REF")"; then + echo "error: git cherry could not compare $FROM_REF against $TO_REF" >&2 + exit 1 +fi candidates_pretty="$(echo "$cherry_out" | awk '$1 == "+" { sub(/^\+ /, ""); print }')" already_pretty="$(echo "$cherry_out" | awk '$1 == "-" { sub(/^- /, ""); print }')" @@ -125,7 +293,7 @@ n_already=0 [[ -n "$already_pretty" ]] && n_already=$(echo "$already_pretty" | wc -l | tr -d ' ') echo -echo "=== Commits on origin/$FROM not yet on origin/$TO ($n_candidates) ===" +echo "=== Commits on $FROM_REF not yet on $TO_REF ($n_candidates) ===" if [[ "$n_candidates" -gt 0 ]]; then echo "$candidates_pretty" else @@ -134,7 +302,7 @@ fi if [[ "$n_already" -gt 0 ]]; then echo - echo "=== Already on origin/$TO, excluded ($n_already) ===" + echo "=== Already on $TO_REF, excluded ($n_already) ===" echo "$already_pretty" fi @@ -160,8 +328,8 @@ if git show-ref --verify --quiet "refs/heads/$branch"; then fi echo -echo "Creating branch $branch off origin/$TO..." -git checkout -b "$branch" "origin/$TO" +echo "Creating branch $branch off $TO_REF..." +git checkout -b "$branch" "$TO_REF" echo echo "Cherry-picking $n_candidates commit(s) with -x, oldest first..." diff --git a/scripts/backport-fixes.test.sh b/scripts/backport-fixes.test.sh new file mode 100755 index 0000000..7e1478a --- /dev/null +++ b/scripts/backport-fixes.test.sh @@ -0,0 +1,513 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Tests for backport-fixes.sh --from ref resolution. +# +# The script has no other test, and the case it regressed on is not one a reader +# would guess: --from accepts a branch OR a tag, and only the branch form has a +# remote-tracking ref. After a release, release.yml deletes the release branch, +# so the tag is the only ref naming those commits — the tag form is the one +# backport-fixes.yml's input description and release.yml's job summary both tell +# you to use. +# +# Each case builds a throwaway origin + clone in a temp dir, so nothing here +# touches the real repository or the network. +# +# Run: scripts/backport-fixes.test.sh + +# Pin out the ambient git config, so "nothing here touches the real repository" +# is enforced rather than asserted. make_fixture sets user.* per-repo but +# nothing else, and a global `commit.gpgsign = true` — common on developer +# machines — made the fixture die with `error: cannot run gpg` mid-suite: no +# test name, no failure count, and a leaked temp dir. This also pins out +# core.hooksPath, init.templateDir and fetch.prune. +# +# GIT_CONFIG_NOSYSTEM, not GIT_CONFIG_SYSTEM=/dev/null: the latter only redirects +# the one path git calls "the" system file, and Apple Git reads a second one that +# it does not cover, so the isolation was silently partial on the platform half +# this suite runs on: +# +# $ GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null \ +# /usr/bin/git config --show-origin --list +# file:/Applications/Xcode.app/.../git-core/gitconfig credential.helper=osxkeychain +# file:/Applications/Xcode.app/.../git-core/gitconfig init.defaultbranch=main +# +# $ GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_NOSYSTEM=1 /usr/bin/git config --list +# (nothing) +# +# GIT_CONFIG_GLOBAL has no such spelling and is simply ignored before git 2.32, +# which would put the global file back in scope with no sign that it had — the +# failure mode being a suite that passes for the wrong reason on the one machine +# whose config breaks it. Assert the version instead of documenting it. +export GIT_CONFIG_GLOBAL=/dev/null +export GIT_CONFIG_NOSYSTEM=1 + +git_version="$(git --version | awk '{print $3}')" +git_major="${git_version%%.*}" +git_rest="${git_version#*.}" +git_minor="${git_rest%%.*}" +if [[ "$git_major" -lt 2 || ( "$git_major" -eq 2 && "$git_minor" -lt 32 ) ]]; then + echo "error: these tests need git >= 2.32 for GIT_CONFIG_GLOBAL; found $git_version" >&2 + echo " older git ignores it and the suite would read your real ~/.gitconfig" >&2 + exit 1 +fi + +SCRIPT="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/backport-fixes.sh" +failures=0 + +# Every fixture is created under one root that an EXIT trap removes. The +# per-case `trap ... RETURN` below still cleans up as it goes, but it does not +# fire when `set -e` kills the shell from inside make_fixture — which is exactly +# when a leak is least welcome. +TESTROOT="$(mktemp -d)" +trap 'rm -rf "$TESTROOT"' EXIT + +pass() { printf ' ok %s\n' "$1"; } +fail() { printf ' FAIL %s\n %s\n' "$1" "$2"; failures=$((failures + 1)); } + +# Builds: origin with `main`, a v1.0.0 tag, and one commit after the tag that is +# only reachable from the tag's branch — the shape of a fix landed on a release +# branch at step 3 of the release flow. +make_fixture() { + local root="$1" + # -b main explicitly: the default branch name comes from init.defaultBranch, + # which differs between a developer machine and a CI runner. Without it the + # fixture builds `master` somewhere and every checkout of `main` fails. + git init -q -b main "$root/origin" + git -C "$root/origin" config user.email t@example.com + git -C "$root/origin" config user.name "Test" + echo base > "$root/origin/f.txt" + git -C "$root/origin" add -A + git -C "$root/origin" commit -qm "base" + + # Clone before the tag exists. A clone made afterwards fetches every tag, which + # leaves refs/tags/v1.0.0 populated locally and hides whether the script's own + # fetch materialises it — the exact blind spot that let a broken resolver pass. + # The real scenario is a maintainer who last fetched before the release. + git clone -q "$root/origin" "$root/clone" + + git -C "$root/origin" checkout -q -b release/v1.0.0 + echo fix > "$root/origin/f.txt" + git -C "$root/origin" commit -qam "fix: something landed on the release branch" + # Annotated, matching release.yml's `git tag -a`. A lightweight tag resolves + # the same way here, but the fixture should produce what the flow it models + # produces. + git -C "$root/origin" tag -a v1.0.0 -m "v1.0.0" + git -C "$root/origin" checkout -q main + git -C "$root/clone" config user.email t@example.com + git -C "$root/clone" config user.name "Test" +} + +# --- a branch as --from keeps working ----------------------------------------- +t_branch() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from release/v1.0.0 --to main 2>&1)"; then + if git -C "$root/clone" log --oneline main..HEAD | grep -q "landed on the release branch"; then + pass "a branch as --from cherry-picks its commits" + else + fail "a branch as --from cherry-picks its commits" "branch created but the commit is missing" + fi + else + fail "a branch as --from cherry-picks its commits" "script exited non-zero: ${out##*$'\n'}" + fi +} + +# --- a tag as --from: the regression ------------------------------------------ +# Before the fix this exited 1 with "origin/v1.0.0 not found on remote", because +# origin/ resolves only against refs/remotes and a tag has none. +t_tag() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from v1.0.0 --to main 2>&1)"; then + if git -C "$root/clone" log --oneline main..HEAD | grep -q "landed on the release branch"; then + pass "a tag as --from cherry-picks its commits" + else + fail "a tag as --from cherry-picks its commits" "branch created but the commit is missing" + fi + else + fail "a tag as --from cherry-picks its commits" "script exited non-zero: ${out##*$'\n'}" + fi +} + +# --- an unknown ref fails, and leaves nothing behind --------------------------- +# It fails at the resolver, which is what the assertion below pins: the name is +# simply absent from the single `git ls-remote` answer, neither the branch nor +# the tag arm matches, and the script prints its own message before any fetch. +# What matters is the contract: non-zero, and no branch created. +t_unknown() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from does-not-exist --to main 2>&1)"; then + fail "an unknown --from fails" "script exited zero" + elif [[ -n "$(git -C "$root/clone" branch --list 'backport/*')" ]]; then + fail "an unknown --from fails" "it created a backport branch anyway" + elif ! grep -q "as a branch or a tag" <<<"$out"; then + fail "an unknown --from fails" "reached the fetch, not the resolver: ${out##*$'\n'}" + else + pass "an unknown --from fails at the resolver, creating no branch" + fi +} + +# --- --to is branch-only ------------------------------------------------------ +# A tag resolves and `git checkout -b` would even work, but the workflow opens a +# PR with `--base "$TO"`, which needs a branch on the remote. Rejecting it here +# beats failing after the cherry-picks have run. +t_to_rejects_a_tag() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from main --to v1.0.0 2>&1)"; then + fail "--to rejects a tag" "script exited zero" + elif grep -q "must be a branch" <<<"$out"; then + pass "--to rejects a tag, naming the reason" + else + fail "--to rejects a tag" "unexpected message: ${out##*$'\n'}" + fi +} + +# --- a force-pushed source branch still backports ------------------------------ +# What the `+` on the refspecs is for. Without it the fetch is a non-fast-forward +# rejection, and the resolver would report that as "not found on origin as a +# branch or a tag" — sending the maintainer after a ref that is present and +# current. Amending a release commit during release prep is routine, and the +# bare-name form this replaces handled it (its remote-tracking update came +# through remote.origin.fetch, which is forced), so losing it would be a +# regression against main rather than a pre-existing bug. +t_force_pushed_source() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + + # Seed the remote-tracking ref at the pre-amend commit: the state of a + # maintainer who last fetched before the force-push. Without this the clone + # has no origin/release/v1.0.0 at all and any fetch is trivially a + # fast-forward, which is how a missing `+` would go unnoticed. + git -C "$root/clone" fetch -q origin \ + '+refs/heads/release/v1.0.0:refs/remotes/origin/release/v1.0.0' + + git -C "$root/origin" checkout -q release/v1.0.0 + echo amended > "$root/origin/f.txt" + git -C "$root/origin" commit -q --amend -am "fix: something landed on the release branch (amended)" + git -C "$root/origin" checkout -q main + + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from release/v1.0.0 --to main 2>&1)"; then + if git -C "$root/clone" log --oneline main..HEAD | grep -q "(amended)"; then + pass "a force-pushed source branch backports the rewritten commit" + else + fail "a force-pushed source branch backports the rewritten commit" \ + "it backported the pre-amend commit" + fi + else + fail "a force-pushed source branch backports the rewritten commit" \ + "script exited non-zero: ${out##*$'\n'}" + fi +} + +# --- a branch wins when a branch and a tag share the name ---------------------- +# The arm order in fetch_source_ref decides this and --help now states it, so it +# needs a case: a repo that tags v1.0.0 and later cuts a branch of the same name +# would otherwise silently change which commits get backported. +t_branch_beats_tag() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + + # A branch literally named v1.0.0, carrying a commit the tag does not. + git -C "$root/origin" checkout -q -b v1.0.0 main + echo from-branch > "$root/origin/f.txt" + git -C "$root/origin" commit -qam "fix: reached through the branch" + git -C "$root/origin" checkout -q main + + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from v1.0.0 --to main 2>&1)"; then + if git -C "$root/clone" log --oneline main..HEAD | grep -q "reached through the branch"; then + pass "a branch wins over a tag of the same name" + else + fail "a branch wins over a tag of the same name" "it resolved the tag instead" + fi + else + fail "a branch wins over a tag of the same name" "script exited non-zero: ${out##*$'\n'}" + fi +} + +# --- an unreachable remote is not a missing ref -------------------------------- +# `git ls-remote --exit-code` answers 2 for "asked, and the remote has no such +# ref" and 128 for "could not ask" — unreachable, or refused. The 128 arm is the +# one that separates them, and without a case a regression in it is invisible: +# the suite passes while a network failure is reported as a missing ref, sending +# the operator after a ref that is fine. +# +# It asserts the absence of the resolver's message rather than the presence of +# git's, because the wording of `fatal: Could not read from remote repository.` +# is git's to change. What must hold is that the script does not add a claim +# about the ref on top of it. +t_unreachable_remote() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + git -C "$root/clone" remote set-url origin /nonexistent + + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from release/v1.0.0 --to main 2>&1)"; then + fail "an unreachable remote is not reported as a missing ref" "script exited zero" + elif grep -q "not found on origin" <<<"$out"; then + fail "an unreachable remote is not reported as a missing ref" \ + "the network failure was reported as a missing ref" + elif [[ -n "$(git -C "$root/clone" branch --list 'backport/*')" ]]; then + fail "an unreachable remote is not reported as a missing ref" "it created a backport branch anyway" + else + pass "an unreachable remote is not reported as a missing ref" + fi +} + +# --- a glob as --from is rejected, not resolved -------------------------------- +# `git ls-remote` matches its argument as a glob, and `*` is legal in a fetch +# refspec, so `release/*` passed the resolver AND the fetch: origin/release/* +# became FROM_REF, `git cherry` fatalled on a ref that is not a commit, and the +# `|| true` on that line turned the fatal into an empty candidate list. The +# script printed "Nothing to backport" and exited 0 — and backport-fixes.yml +# renders that as a green run with a ::notice::. fromBranch is a free-text +# workflow_dispatch input, so this is typeable. +# +# The exit-0 arm is the assertion that matters: a non-zero with a confusing +# message would be a bad error, but exit 0 is a tooling failure reported to the +# operator as a fact about the refs. +t_glob_from() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from 'release/*' --to main 2>&1)"; then + fail "a glob as --from is rejected" \ + "script exited zero: ${out##*$'\n'}" + elif grep -q "Nothing to backport" <<<"$out"; then + fail "a glob as --from is rejected" "it reported the failure as an empty backport" + elif [[ -n "$(git -C "$root/clone" branch --list 'backport/*')" ]]; then + fail "a glob as --from is rejected" "it created a backport branch anyway" + elif [[ -n "$(git -C "$root/clone" for-each-ref --format='%(refname)' 'refs/tags/*')" ]]; then + fail "a glob as --from is rejected" "it fetched refs before rejecting the input" + elif ! grep -q "not a valid git ref name" <<<"$out"; then + fail "a glob as --from is rejected" "unexpected message: ${out##*$'\n'}" + else + pass "a glob as --from is rejected before any network round trip" + fi +} + +# --- --from resolving to a non-commit is caught -------------------------------- +# The companion to the glob case, and the reason validating the input is not on +# its own enough: a tag may point at a tree or a blob, `treetag` is a perfectly +# valid ref name, and ls-remote reports it like any other ref. Only a +# `rev-parse --verify ^{commit}` after resolution sees it. Same failure +# shape as the glob without the guard — `git cherry` fatals, and pre-fix the +# `|| true` reported it as "Nothing to backport", exit 0. +t_from_is_not_a_commit() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + local tree + tree="$(git -C "$root/origin" rev-parse 'main^{tree}')" + git -C "$root/origin" tag treetag "$tree" + + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from treetag --to main 2>&1)"; then + fail "--from that is not a commit is caught" "script exited zero: ${out##*$'\n'}" + elif grep -q "Nothing to backport" <<<"$out"; then + fail "--from that is not a commit is caught" "it reported the failure as an empty backport" + elif ! grep -q "does not name a commit" <<<"$out"; then + fail "--from that is not a commit is caught" "unexpected message: ${out##*$'\n'}" + else + pass "--from that resolves to a non-commit is caught, not counted as zero commits" + fi +} + +# --- a bad --to writes nothing to the user's repo ------------------------------ +# Resolution used to fetch the source ref and only then ask about --to, so a +# typo'd --to still left refs/tags/v1.0.0 (or a remote-tracking ref) behind on +# the way to an error. Both names resolve from one ls-remote answer now, and the +# single fetch runs only once both have. +t_bad_to_writes_nothing() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from v1.0.0 --to nope 2>&1)"; then + fail "a bad --to writes nothing" "script exited zero" + elif [[ -n "$(git -C "$root/clone" for-each-ref --format='%(refname)' 'refs/tags/*')" ]]; then + fail "a bad --to writes nothing" "it fetched the tag before rejecting --to" + elif [[ -n "$(git -C "$root/clone" for-each-ref --format='%(refname)' 'refs/remotes/origin/release/*')" ]]; then + fail "a bad --to writes nothing" "it fetched the source branch before rejecting --to" + else + pass "a bad --to leaves no fetched refs behind" + fi +} + +# --- the branch name --help promises ------------------------------------------- +# --help claims `--from v0.6.0` yields `backport/v0.6.0`, which is true only +# because the flattening arm happens to leave a bare tag alone. Neither that nor +# --branch had a case; t_tag asserts the commit landed, not the branch name. +t_branch_naming() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + + if ! (cd "$root/clone" && "$SCRIPT" --from v1.0.0 --to main >/dev/null 2>&1); then + fail "a tag --from names the branch backport/" "script exited non-zero" + elif [[ "$(git -C "$root/clone" rev-parse --abbrev-ref HEAD)" != "backport/v1.0.0" ]]; then + fail "a tag --from names the branch backport/" \ + "got $(git -C "$root/clone" rev-parse --abbrev-ref HEAD)" + else + pass "a tag --from names the branch backport/" + fi + + local root2; root2="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root" "$root2"' RETURN + make_fixture "$root2" + if ! (cd "$root2/clone" && "$SCRIPT" --from v1.0.0 --to main --branch mine >/dev/null 2>&1); then + fail "--branch overrides the derived name" "script exited non-zero" + elif [[ "$(git -C "$root2/clone" rev-parse --abbrev-ref HEAD)" != "mine" ]]; then + fail "--branch overrides the derived name" \ + "got $(git -C "$root2/clone" rev-parse --abbrev-ref HEAD)" + else + pass "--branch overrides the derived name" + fi +} + +# --- both spellings of a flag, and a flag with no value ------------------------ +# `--from=v1.0.0` took the unknown-argument arm and exited 2, and a trailing +# `--from` exited 1 with no message at all, because `shift 2` fails under +# `set -e`. --help leads with --from, so neither answer was defensible. +t_arg_forms() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + + local out + if ! out="$(cd "$root/clone" && "$SCRIPT" --from=v1.0.0 --to=main 2>&1)"; then + fail "--flag=value is accepted" "script exited non-zero: ${out##*$'\n'}" + elif ! git -C "$root/clone" log --oneline main..HEAD | grep -q "landed on the release branch"; then + fail "--flag=value is accepted" "branch created but the commit is missing" + else + pass "--flag=value is accepted" + fi + + local rc=0 + out="$(cd "$root/clone" && "$SCRIPT" --to main --from 2>&1)" || rc=$? + if [[ "$rc" -ne 2 ]]; then + fail "a flag with no value is a usage error" "exit $rc, want 2" + elif ! grep -q -- "--from requires a value" <<<"$out"; then + fail "a flag with no value is a usage error" "no message: ${out:-(empty)}" + else + pass "a flag with no value is a usage error, not a silent exit 1" + fi +} + +# --- a bad --branch is a usage error, before the network ------------------------ +# --branch was the one ref-shaped flag with no ref-name check, so it failed at +# `git checkout -b` with git's exit 128 — after the ls-remote, the fetch and the +# printed candidate list. The exit code is half the point; the refs assertion +# below is the other half, and it is the one that pins "before any round trip" +# rather than merely "with a nicer message". +t_bad_branch_override() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + local out rc=0 + out="$(cd "$root/clone" && "$SCRIPT" --from release/v1.0.0 --to main --branch 'bad/*name' 2>&1)" || rc=$? + if [[ "$rc" -ne 2 ]]; then + fail "a bad --branch is a usage error" "exit $rc, want 2" + elif ! grep -q "not a valid git ref name" <<<"$out"; then + fail "a bad --branch is a usage error" "unexpected message: ${out##*$'\n'}" + elif [[ -n "$(git -C "$root/clone" for-each-ref --format='%(refname)' 'refs/remotes/origin/release/*' 'refs/tags/*')" ]]; then + fail "a bad --branch is a usage error" "it fetched refs before rejecting the input" + else + pass "a bad --branch is rejected at exit 2 before any network round trip" + fi +} + +# --- an empty --branch is a usage error, not the derived name ------------------- +# `--branch=` used to reach the derivation and produce backport/v1.0.0 at exit 0, +# because BRANCH_OVERRIDE cannot tell "not passed" from "passed empty". --to, +# which defaults to a non-empty value, always caught its own empty form. Both +# spellings are covered: the `=` form and a separate empty word. +t_empty_branch_override() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + + local out rc=0 + out="$(cd "$root/clone" && "$SCRIPT" --from release/v1.0.0 --to main --branch= 2>&1)" || rc=$? + if [[ "$rc" -ne 2 ]]; then + fail "--branch= is a usage error" "exit $rc, want 2" + elif ! grep -q -- "--branch cannot be empty" <<<"$out"; then + fail "--branch= is a usage error" "unexpected message: ${out##*$'\n'}" + elif [[ -n "$(git -C "$root/clone" branch --list 'backport/*')" ]]; then + fail "--branch= is a usage error" "it fell back to the derived name" + else + pass "--branch= is a usage error, not a silent fallback to the derived name" + fi + + rc=0 + out="$(cd "$root/clone" && "$SCRIPT" --from release/v1.0.0 --to main --branch "" 2>&1)" || rc=$? + if [[ "$rc" -ne 2 ]]; then + fail "--branch '' is a usage error" "exit $rc, want 2" + else + pass "--branch with an empty value is a usage error in both spellings" + fi +} + +# --- a branch --from leaves no tags behind ------------------------------------- +# --no-tags was dropped when the bare-name refspecs were replaced with explicit +# ones, and git's automatic tag following filled the gap: a branch --from +# downloaded every tag reachable from the fetched history, so a maintainer who +# had never fetched v1.0.0 got it anyway. --help says the script fetches "those +# two refs — not the whole remote", and t_tag only proves the tag path still +# works, not that the branch path stays out of refs/tags. +t_branch_from_leaves_no_tags() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + local out + if ! out="$(cd "$root/clone" && "$SCRIPT" --from release/v1.0.0 --to main 2>&1)"; then + fail "a branch --from fetches no tags" "script exited non-zero: ${out##*$'\n'}" + elif [[ -n "$(git -C "$root/clone" for-each-ref --format='%(refname)' 'refs/tags/*')" ]]; then + fail "a branch --from fetches no tags" \ + "it left $(git -C "$root/clone" for-each-ref --format='%(refname)' 'refs/tags/*' | tr '\n' ' ')behind" + else + pass "a branch --from fetches no tags into the user's repo" + fi +} + +# --- outside a repo, the missing repo is the error ----------------------------- +# check-ref-format needs no repository, so the ref-name guards ran first and +# someone outside a repo was told their ref name was wrong — fixing which only +# earned them the real blocker on the next run. Ordering only; both errors are +# still reported, and the guards are still ahead of the network. +t_outside_a_repo() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + local out rc=0 + out="$(cd "$root" && "$SCRIPT" --from 'release/*' --to main 2>&1)" || rc=$? + if [[ "$rc" -ne 1 ]]; then + fail "outside a repo, the missing repo is the error" "exit $rc, want 1" + elif ! grep -q "not inside a git repository" <<<"$out"; then + fail "outside a repo, the missing repo is the error" "unexpected message: ${out##*$'\n'}" + else + pass "outside a repo, the missing repo is reported before the ref name" + fi +} + +echo "backport-fixes.sh — --from ref resolution" +t_branch +t_tag +t_unknown +t_to_rejects_a_tag +t_force_pushed_source +t_branch_beats_tag +t_unreachable_remote +t_glob_from +t_from_is_not_a_commit +t_bad_to_writes_nothing +t_branch_naming +t_arg_forms +t_bad_branch_override +t_empty_branch_override +t_branch_from_leaves_no_tags +t_outside_a_repo + +if [[ "$failures" -gt 0 ]]; then + echo "$failures failing" + exit 1 +fi +echo "all passing" From 8ce6551d476de021c9f12cc9a7e0b5e4a5ae22fe Mon Sep 17 00:00:00 2001 From: Roberto Iskandarani Date: Tue, 18 Aug 2026 11:58:07 -0300 Subject: [PATCH 5/5] docs(release): the release pushes five annotated tags, not four --- RELEASE_GUIDE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RELEASE_GUIDE.md b/RELEASE_GUIDE.md index 5fed83f..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. (`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**.