Skip to content

Let embedders skip consent for preauthorized clients - #6882

Merged
jhrozek merged 8 commits into
mainfrom
skip-consent
Oct 20, 2026
Merged

jhrozek merged 8 commits into
mainfrom
skip-consent

Conversation

@jhrozek

@jhrozek jhrozek commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Since v0.52.0, /oauth/authorize always shows ToolHive's consent page. A program that embeds the authorization server in-process and is the only client of that server has no way to skip it. The user started the flow seconds earlier from that same program, so the page adds nothing. Automated callers are worse off: a GET of the authorize URL returns HTML instead of a redirect, and the flow silently stalls. Remembered consent doesn't help, because it needs a prior human click and a persistent cookie jar.

This adds an opt-in, Go-only allowlist of client IDs that skip the consent page:

  • authserver.Config.ConsentExemptClientIDs, runner.WithConsentExemptClientIDs (a trailing variadic option on NewEmbeddedAuthServer and NewEmbeddedAuthServerWithStorage), and handlers.WithConsentExemptClientIDs. It is not exposed through RunConfig, YAML, the CLI, CRDs or Swagger.

  • A shared internal consentpolicy package validates the list. Config.Validate, NewHandler, and the runner (before upstream DCR) all use it. An empty list changes nothing. A non-empty list rejects:

    • empty or duplicate IDs
    • URL-shaped IDs, so CIMD clients can't be listed
    • a server with no upstreams
    • a non-HTTPS authorization base
  • authserver.New resolves every listed ID through the decorated storage. Construction fails unless each client is confidential, interactive, has the authorization_code grant and code response type, and has a registered redirect.

  • At /oauth/authorize, after all existing validation, the page is skipped only if all of the following hold:

    • the validated client passes the same eligibility checks
    • the request asks for exactly code
    • it carries exactly one explicit, non-empty redirect_uri
    • that URI is registered and equals fosite's final validated redirect, with no loopback rewrite
    • no prompt token is consent

    The request is then marked approved and goes through the existing beginUpstream, so it gets fresh upstream state, PKCE, nonce and browser binding. Remembered consent is never read or written on this path.

  • prompt=consent is now checked once, ahead of both the exemption and remembered consent.

  • docs/arch/11-auth-server-storage.md documents the trust model, what the embedder must still do, and the deliberate deviation from the MCP consent requirement.

Fixes #6881

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

go build ./pkg/authserver/... and go vet ./pkg/authserver/server/handlers/ pass on the final commit. task test and task lint-fix have not yet been run on this revision. As #6881 notes, an in-process embedder was tested against an earlier revision of the branch. That test should be repeated against this one.

Changes

File Change
pkg/authserver/internal/consentpolicy/ Shared allowlist syntax and deployment checks. Client eligibility predicate. IsBackChannelOnlyClient moved here so startup, runtime and /authorize rejection share one definition.
pkg/authserver/config.go ConsentExemptClientIDs field and its validation in Config.Validate
pkg/authserver/server_impl.go Resolves listed clients through decorated storage before upstreams are built. Wires the handler option.
pkg/authserver/server/handlers/handler.go WithConsentExemptClientIDs option. Validation in NewHandler after options are applied.
pkg/authserver/server/handlers/authorize.go promptRequiresConsent, the consentExempt predicate, and the exemption branch
pkg/authserver/runner/embeddedauthserver.go Typed variadic EmbeddedAuthServerOption. Early validation inside the storage-cleanup path, before upstream DCR.
docs/arch/11-auth-server-storage.md Trust model, downstream transaction binding, standards trade-off, prompt behavior

Does this introduce a user-facing change?

Yes, for Go embedders only. Before: every /oauth/authorize request without a covering remembered approval returned the consent page (HTTP 200). After: an embedder can list client IDs. For an eligible request from a listed client, /oauth/authorize returns 303 straight to the upstream IdP. With no list configured, behavior is unchanged.

The runner constructors are now variadic. Existing calls compile unchanged, but code that assigns them to a variable of the old non-variadic function type will no longer compile.

Implementation plan

Approved implementation plan

Administrator consent preauthorization

Status: replacement implementation plan, incorporating the review corrections; not implemented. RFC decision: the trade-off section in the PR body carries the design rationale; write an RFC in toolhive-rfcs if maintainers request one.

This supersedes the earlier combined registration-API and consent plan. It is a local planning document, not a published ToolHive RFC. Formal proposals belong in toolhive-rfcs, as described in docs/proposals/README.md.

Scope and deliverable

One ToolHive PR adds a Go-only administrator policy for skipping ToolHive's consent page for explicitly listed clients. An empty list preserves existing behavior.

No registration API removals, persisted-format changes, registry renames, or changes to configured-client fingerprints. mecatl adoption is a separate PR.

Trust model

Use this wording in GoDoc and the architecture documentation:

Listing a client ID preauthorizes whatever registration currently holds that ID, for whichever user completes upstream authentication. It is not per-user consent, and runtime eligibility checks do not prove where the registration came from.

State these consequences plainly:

  • An actual HTTP-DCR client qualifies if an embedder explicitly lists its ID and it meets the eligibility requirements.
  • Replacing the registration's credentials or redirects transfers the exemption to the replacement.
  • Expanding its permitted scopes, audiences or grants can expand what is preauthorized.
  • The embedder must control registration writes for exempt IDs. HTTP DCR cannot choose or overwrite an exempt ID: it generates IDs server-side and registers create-only. CIMD replacement targets URL-shaped IDs, which the allowlist excludes. These statements concern registration writes, not access to the authorization endpoint.
  • A privileged provisioner can replace an exempt registration. This policy does not protect against that.
  • DCRIssued retains its existing roles in retention and replacement rules. It plays no part in exemption eligibility.
  • Removing an ID restores ordinary consent decision-making for new requests. It does not revoke tokens or automatically cancel already-approved flows.

Required downstream client transaction binding

Anyone who knows an exempt client ID can send a victim an authorization link. If the victim already has an upstream SSO session, authentication can complete silently and a code can arrive at the legitimate client's callback without a new user action.

Therefore the embedder must:

  1. Generate fresh, unpredictable state and an S256 PKCE verifier/challenge for each authorization transaction it initiates.
  2. Bind that transaction to the initiating client user/session and browser or equivalent user-agent context, as appropriate to the deployment. Merely finding a live state created for a different user's session is not sufficient.
  3. Reject unsolicited, mismatched, expired, cancelled or already-consumed callback state before exchanging a code or attaching credentials to a session.
  4. Select the PKCE verifier from its own stored transaction, not callback or other attacker-controlled input, and use it when redeeming the code.
  5. Preserve issuer and other existing authorization-response checks.

State is checked at the downstream callback. The PKCE verifier is used at the token endpoint; it is not normally carried in the callback. PKCE and confidential-client authentication do not, by themselves, prove which browser/session initiated the callback or that the user intended the initial authorization navigation.

ToolHive's upstream browser-binding cookie remains necessary, but it protects a different hop. It does not replace the embedder's downstream transaction binding. The exemption does not establish user intent.

Step 1 — Add the Go-only policy and shared validation

Changes

Add to authserver.Config:

ConsentExemptClientIDs []string

Add to handlers:

func WithConsentExemptClientIDs(ids []string) Option

The handler option copies its input, replaces rather than accumulates prior entries when repeated, and builds a handler-local lookup set. It performs no storage lookups.

Create a small internal package at pkg/authserver/internal/consentpolicy for the shared syntax and deployment-prerequisite validation. Both Config.Validate and NewHandler use it, and the runner uses it before upstream resolution. Keep it independent of the parent authserver and handlers packages to avoid import cycles. Pass only the values it needs; do not introduce a parallel configuration struct or new public validation API.

When the list is nonempty, reject:

  • Empty or duplicate IDs.
  • URL-shaped IDs.
  • No configured upstreams.
  • A non-HTTPS effective authorization base: AuthorizationEndpointBaseURL, otherwise issuer.

For this feature, URL-shaped means an absolute URI with a parsed scheme, or a network-path URL with an authority. This includes HTTP(S)/CIMD-shaped IDs. Use the same definition everywhere; do not normalize allowed opaque IDs or rely on a narrower CIMD-only detector.

InsecureAllowHTTP does not relax the exemption's HTTPS requirement. An empty list changes none of the existing token-only or HTTP-development validation behavior.

Run shared validation through Config.Validate for early core-server failure, and in NewHandler after options are applied so direct callers receive the same checks.

Deliverables

  • Config field, handler option and internal validation package.
  • Tests for empty/duplicate/URL-shaped IDs, effective-base selection, missing upstreams and HTTPS requirements.
  • Direct-handler and core-config tests.
  • Tests for caller-input copying and repeated-option replacement.
  • Regression tests for unchanged empty-list behavior.

Step 2 — Validate exempt registrations during server construction

Changes

In authserver.New, resolve every listed ID using the supplied context and effective decorated storage. Do not use storage.Unwrap to bypass encryption or policy decorators.

Each listed client must:

  • Exist.
  • Not be public.
  • Not be back-channel-only, including the existing metadata-shape exclusions represented by isBackChannelOnlyClient.
  • Have the authorization_code grant.
  • Have the code response type.
  • Have at least one registered redirect.

A missing/ineligible registration or a storage error fails construction.

Syntax validation precedes client resolution. Insert registration checks before avoidable server-side upstream construction without reorganizing the existing bootstrap. Do not promise network-free validation: storage resolution may itself require network access.

The low-level handler constructor does no storage lookup. Runtime decisions always use the client returned by normal request validation, not a startup-cached registration.

Keep the capability predicate consistent between startup and runtime without expanding the public API merely to expose an existing private helper. Its placement must respect package dependencies.

Deliverables

  • Contextual registration validation.
  • Rejection tests for missing, public, back-channel, grant-deficient, response-type-deficient and redirect-less clients.
  • Storage-error tests.
  • Decorator tests proving lookups use effective storage.
  • Constructor-failure tests preserving resource ownership and cleanup.

Step 3 — Add runner options and validate before upstream resolution

Changes

Add:

type EmbeddedAuthServerOption func(*embeddedAuthServerOptions)

func WithConsentExemptClientIDs(ids []string) EmbeddedAuthServerOption

Accept typed variadic options on both:

func NewEmbeddedAuthServer(
    ctx context.Context,
    cfg *authserver.RunConfig,
    opts ...EmbeddedAuthServerOption,
) (*EmbeddedAuthServer, error)

func NewEmbeddedAuthServerWithStorage(
    ctx context.Context,
    cfg *authserver.RunConfig,
    stor storage.Storage,
    opts ...EmbeddedAuthServerOption,
) (*EmbeddedAuthServer, error)

Use private concrete option state, not an arbitrary mutation callback over resolved authserver.Config, and not a capability assertion that can silently do nothing. Copy IDs at option creation; repeated options replace the list.

Apply and validate the option values early. Call the shared non-storage validation before buildUpstreamConfigs, currently runner bootstrap step 5. That function can perform upstream DCR network registration before authserver.New is called. A malformed allowlist must therefore be rejected in the runner before this side effect, not only in the later core constructor.

This is an additional early check, not a bootstrap reorder. Semantic registration lookups still happen in authserver.New after effective decoration. Missing or ineligible registrations are not guaranteed to fail before runner upstream resolution.

Preserve current ownership rules for caller-supplied storage, including cleanup on constructor failure. Do not let early validation introduce a leak or an inconsistent ownership transfer between constructor paths.

Thread the validated policy into the resolved authserver.Config. Do not add it to RunConfig, YAML, CLI, CRDs, Swagger, DCR or CIMD metadata.

The early runner check reads RunConfig values (issuer, AuthorizationEndpointBaseURL, upstream count), while authserver.New checks the resolved Config after applyDefaults. These are passed through unchanged today. Add one test asserting both paths reject the same invalid inputs, so future divergence is caught.

Deliverables

  • Typed runner option and both constructor changes.
  • Integration tests proving the option reaches the handler.
  • Tests proving invalid option syntax fails before any upstream DCR/discovery call.
  • Option copying/replacement and default-behavior tests.
  • Ownership/decorated-storage/failure-cleanup regression tests.
  • Compatibility note: the variadic addition preserves existing call syntax, but assignments to old non-variadic function types break.

Step 4 — Add the authorization branch

Changes

Extract promptRequiresConsent(form) from beginIfRemembered, retaining repeated and space-separated prompt handling. Remove the old prompt scan from beginIfRemembered once every caller receives the common prompt gate.

After existing request validation, token-only handling and resource checks:

promptRequiresConsent
    -> store pending request, set binding cookie, render consent

consentExempt
    -> pending.ConsentStage = Approved; beginUpstream

beginIfRemembered
    -> existing remembered-consent behavior

otherwise
    -> store pending request, set binding cookie, render consent

Share the existing ordinary-consent tail rather than duplicating storage/cookie/rendering code for forced consent.

The forced-consent path no longer reaches coveringConsent. This matches current behavior, because beginIfRemembered already returns before the remembered lookup when prompt=consent is present. State this in the PR body so reviewers do not read it as a remembered-consent change.

The exemption predicate returns true only when:

  1. The validated client ID is in the allowlist.
  2. The request response type is exactly code.
  3. The validated client is confidential, not back-channel-only, and supports the authorization-code grant and code response type.
  4. The parsed original request form contains exactly one explicit, nonempty redirect_uri.
  5. That value is exactly equal to a registered redirect string.
  6. rewrittenFrom == "", proving no loopback rewrite occurred.

Pass rewrite information explicitly to the predicate. Do not infer an explicit redirect from Fosite's possibly inferred redirect, or attempt to detect rewriting from the mutated request alone.

Reuse beginUpstream unchanged. Exempt requests do not read/write remembered consent, issue its cookie, or populate Expected*, ConsentSessionDigest or remember-approval state. They still receive the upstream browser-binding cookie.

Preserve state, PKCE, nonce, pending storage before redirect, callback browser binding, expiration, single-use handling, upstream authentication/consent, resource validation and downstream token checks. Do not claim that every PKCE check already occurs before this branch if existing enforcement is deferred.

If an allowlisted client becomes ineligible at runtime, fall back to remembered consent and then ordinary consent. A covering remembered approval may independently skip the page unless prompt=consent is present.

Deliverables

  • Shared prompt detection, exemption predicate and small handler branch.
  • Synchronized comments.
  • No new upstream-login or callback implementation.
  • Focused tests for decision precedence and eligibility.

Step 5 — Verify invariants and regressions

Configuration and startup

  • Empty list preserves existing token-only and HTTP deployment behavior.
  • Nonempty list with HTTP or no upstreams fails through core construction and direct NewHandler.
  • Effective HTTPS base overrides/fallbacks are tested.
  • Invalid/duplicate/URL-shaped IDs fail consistently.
  • Registration eligibility and storage failures fail core startup.
  • Runner syntax validation occurs before upstream DCR/discovery side effects.

DCR ID ownership

Add a named test such as TestDCRIgnoresCallerClientIDAndPreservesExistingRegistration:

  1. Submit DCR with a caller-supplied client_id.
  2. Assert the response client_id differs from the supplied value.
  3. Repeat registration while supplying the first issued ID and different metadata.
  4. Assert a different ID is issued and the first registration remains unchanged.

Use assertion messages naming these invariants. Retain/test create-only collision behavior for the storage operation so the trust model does not rely solely on UUID generation.

Authorization

  • Explicit exact redirect is eligible.
  • Omitted redirect, even when inferred by Fosite, is not exempt.
  • Repeated or empty redirects accepted by existing validation are not exempt.
  • Invalid/mismatched redirects rejected by existing validation retain their OAuth errors; do not turn them into consent-page requests.
  • Valid loopback rewrites are not exempt.
  • Non-code response types are not exempt; preserve existing OAuth errors where applicable.
  • Single, repeated and space-separated prompt=consent wins over exemption and remembered approval.
  • Public, back-channel and capability-deficient clients do not qualify.
  • An explicitly allowlisted eligible client from registration.New can qualify regardless of its lifecycle marker.
  • Runtime loss of eligibility falls back safely.

Consent-page assertions use a browser without a covering remembered approval unless remembered consent is the subject of the test.

Flow security and integration

  • Exempt authorization returns 303 upstream with approved pending state and a browser-binding cookie.
  • A remembered-consent mock that fails on every call proves the exempt path does not access it; no remembered cookie is issued.
  • Two flows have independent upstream state, PKCE, nonce and binding.
  • Callback without the correct binding is rejected; retain replay/expiry coverage relevant to the reused path.
  • Normal OAuth validation/error behavior is unchanged.
  • Both runner constructors reach the handler with copied options.
  • Decorated storage, lifecycle ownership and constructor-failure cleanup remain unchanged.

Deliverables

  • Table-driven unit coverage and focused core/runner integration tests.
  • Explicit DCR ownership regression test.
  • No tests that assert a new consent fallback in place of an existing OAuth rejection.

Step 6 — Logging and documentation

Logging

  • One DEBUG startup line describing the policy after successful construction/validation.
  • One DEBUG line per exemption decision, keyed on client_id.
  • No INFO success logs.
  • No credentials, state, tokens, cookies or raw request URLs.

Avoid duplicate startup logs from runner/core/handler plumbing.

Documentation

Update:

  • authserver.Config.ConsentExemptClientIDs GoDoc.
  • Handler and runner option GoDoc.
  • docs/arch/11-auth-server-storage.md, especially interactive client approval.
  • Release-note/PR text describing user-visible behavior and the variadic-signature caveat.

Include the trust model and required downstream state/PKCE transaction binding above, registration write ownership, replacement/permission-expansion consequences, unchanged upstream authentication, and non-revocation on allowlist removal.

State the standards trade-off accurately: applicable stable MCP requirements call for user consent for dynamically registered downstream clients. Allowlisting an actual HTTP-DCR client is a deliberate deviation, not a standards exception. The broader administrator-preauthorization trade-off gets the same treatment.

Keep existing client TTL/eviction behavior unchanged. A dynamic-lifecycle exempt registration can expire or be evicted, requiring embedder reprovisioning. Do not silently change registration retention to make exemption durable.

Prompt behavior out of scope

prompt=none and prompt=login are not specially implemented today. With exemption, an otherwise eligible prompt=none request may redirect upstream instead of displaying ToolHive's consent page. Neither existing behavior nor this proposal establishes OIDC prompt=none compliance, and upstream interaction is not guaranteed to be silent. Do not claim prompt=login forces upstream reauthentication. Implementing either prompt's full semantics is separate work.

Deliverables

  • Accurate GoDoc, architecture documentation and PR/release-note text.
  • Explicit security requirements, standards trade-offs, prompt limitations and retention limitations.

Step 7 — Verification and PR handoff

Run repository Taskfile verification, including:

task test
task lint-fix
task license-check

Review the diff for unintended changes to defaults, persistence, registration, callback security or ownership. Do not change CLI/API handlers or generate unrelated documentation artifacts.

Deliverables

  • One scoped ToolHive PR, subject to repository size limits. Expected non-test files: config.go, server_impl.go, server/handlers/handler.go, server/handlers/authorize.go, internal/consentpolicy, runner/embeddedauthserver.go and docs/arch/11-auth-server-storage.md, which fits within the 400-line, 10-file limit.
  • Completed test plan listing only verification actually performed.
  • This implementation plan and deliberate trade-offs in the PR body.
  • Before/after user-facing description.
  • Consumer migration/handoff notes.

Separate mecatl adoption PR

The later mecatl change should:

  1. Upgrade ToolHive and supply its own client ID through the runner option.
  2. Mount /oauth/consent for forced-consent and fallback requests.
  3. Confirm with tests that its downstream callback rejects unsolicited, mismatched, expired and replayed state before token exchange or credential attachment.
  4. Confirm state is bound to the correct initiating user/logical session and browser/user-agent context; check cross-session and cross-user completion, not only random unknown state.
  5. Confirm token exchange uses the pending transaction's own PKCE verifier and preserves issuer/client authentication checks.
  6. Exercise enrollment and protected-tool flows through vMCP, retaining incoming authentication and upstream credential injection.
  7. Document existing registration TTL/eviction and reprovisioning behavior.

The callback requirements are acceptance criteria to verify, not a claim that all required browser/session binding has already been demonstrated in mecatl.

Out of scope

  • Registration API cleanup or removals.
  • Provisioning provenance/classification changes.
  • Persisted-format changes.
  • Registry renames.
  • Adding redirects to configured-client fingerprints.
  • New authentication methods or grant combinations.
  • Changes to remembered consent or device flow.
  • Full prompt=none / prompt=login semantics.
  • Per-user, per-scope or per-redirect exemption policies.
  • Automatic registration cleanup, retention changes or token revocation.
  • mecatl or enterprise implementation changes.

Special notes for reviewers

  • Trust model: listing an ID preauthorizes whatever registration currently holds that ID, for whichever user completes upstream authentication. Eligibility is not checked against the DCRIssued marker, which governs retention only. The safety argument is that no remote path can occupy a listed ID: HTTP DCR mints its own UUIDs and is create-only, and CIMD IDs are URL-shaped and rejected by the allowlist. TestDCRIgnoresCallerClientIDAndPreservesExistingRegistration pins the DCR half of that.
  • Deliberate deviation from the stable MCP spec: "MCP proxy servers using static client IDs MUST obtain user consent for each dynamically registered client before forwarding to third-party authorization servers." Allowlisting a DCR-issued client skips exactly that. The arch doc says so explicitly rather than calling it an exception.
  • Without the consent page, an attacker who knows a listed ID can send a victim an authorize link, and a silent upstream SSO session can complete it. What prevents abuse is the embedder's callback rejecting unsolicited state, which the docs make an explicit requirement. Checking for a code_challenge or state at the exemption point would not help, because an attacker who builds the URL chooses both values. PKCE itself is still enforced when the code is issued.
  • A signed OIDC request object can rewrite redirect_uri in the form before fosite validates the redirect (fosite v0.49.0). The predicate therefore also compares ar.GetRedirectURI() with the explicit value.
  • prompt=none, prompt=login and prompt=select_account are still not specially handled. For a listed client they now redirect upstream instead of rendering the consent page. Full prompt semantics are out of scope.
  • Listed clients keep their existing TTL and eviction behavior. A dynamic-lifecycle client can expire and then needs to be reprovisioned.
  • The forced-consent path no longer reaches coveringConsent. That matches current behavior, because beginIfRemembered already returned before its lookup when prompt=consent was present.

🤖 Generated with Claude Code

jhrozek and others added 8 commits October 20, 2026 13:36
Embedders of the authorization server need a way to let trusted
confidential clients skip ToolHive's consent page. A list of
preauthorized client IDs is only safe if bad entries and unsafe
deployments are rejected before any request is served, so this commit
adds the configuration and its validation. Nothing reads the list at
request time yet.

Config.ConsentExemptClientIDs and handlers.WithConsentExemptClientIDs
carry the list. The shared checks live in the new internal package
pkg/authserver/internal/consentpolicy. Config.Validate and NewHandler
both call it, so direct handler callers get the same checks as the
core server. When the list is not empty, consentpolicy.Validate
rejects:

- empty or duplicate IDs
- URL-shaped IDs (any parsed scheme, or a "//host" authority), which
  covers CIMD IDs that a remote party can change
- a server with no upstreams
- an effective authorization base (AuthorizationEndpointBaseURL,
  otherwise the issuer) that is not HTTPS

InsecureAllowHTTP does not relax the HTTPS rule. An empty list leaves
all existing validation as it was. The option copies the caller's
slice and replaces earlier values instead of adding to them.
NewHandler turns the list into a lookup set and does no storage
lookups.

Co-authored-by: Mecatl <noreply@mecatl.dev>
The syntax checks alone do not stop an operator from listing an ID
that is missing, public, or cannot do the interactive flow. Such an
entry would only show up later, at request time. This commit makes
authserver.New resolve each listed ID and fail construction if the
registration is not usable.

validateConsentExemptClients in server_impl.go looks up every ID
through the storage passed to newServer. That is the decorated
storage, so encryption and policy decorators still apply and
storage.Unwrap is not used. A storage error or a missing or ineligible
client is a construction error. The check runs after Config.Validate,
so syntax errors are reported before any lookup, and before
registerDelegateClients and upstream construction.

consentpolicy.IsEligibleClient defines eligibility. The client must be
confidential, not back-channel-only, and must have the
authorization_code grant, the code response type and at least one
redirect URI. The private isBackChannelOnlyClient helper moves from
handlers/authorize.go into consentpolicy as IsBackChannelOnlyClient,
without changes. The /authorize rejection path and the startup check
now use the same predicate, and no new public API is added. The
handler constructor still does no storage lookups. The
AuthorizationServerParams diff is only gofmt realignment.

Co-authored-by: Mecatl <noreply@mecatl.dev>
Embedders need a way to preauthorize specific confidential clients so
they skip ToolHive's consent page. The allowlist is a Go-only setting
for administrators. It is deliberately left out of RunConfig, so it
cannot come from CLI flags, config files or CRDs.

Add the EmbeddedAuthServerOption functional option type and
WithConsentExemptClientIDs. The option copies its input when it is
created and again when it is applied, so a caller that later changes
its slice has no effect on the server. Repeated options replace the
list; they do not merge. The resolved IDs are copied into
authserver.Config.ConsentExemptClientIDs. The GoDoc states the trust
model: listing an ID preauthorizes whichever registration currently
holds it, for any user who completes upstream login, and the caller
must control registration writes for those IDs.

Both runner constructors validate the list through
consentpolicy.Validate. NewEmbeddedAuthServer checks before it creates
storage. The shared core checks again inside the cleanup gate, before
upstream resolution, so a bad policy fails before any discovery or DCR
network I/O and caller-supplied storage is still closed.

NewEmbeddedAuthServer and NewEmbeddedAuthServerWithStorage are now
variadic. Existing calls compile unchanged, but code that assigns
these functions to a variable or field of the old non-variadic
function type will no longer compile.

Co-authored-by: Mecatl <noreply@mecatl.dev>
Clients that an administrator has allowlisted should go straight to
upstream login and not stop at ToolHive's consent page. This commit
adds that branch to AuthorizeHandler. Fosite validation still runs
first, so invalid requests keep their existing OAuth errors.

The new consentExempt check is narrow. A request qualifies only when
all of these are true:
- the client ID is in the allowlist and consentpolicy.IsEligibleClient
  accepts the client (confidential, interactive authorization code)
- the request's response_type is exactly "code"
- the request sent exactly one non-empty redirect_uri that is
  registered for the client, and the loopback rewrite did not change it

To check the redirect, the handler copies the parsed form before
rewriteLoopbackRedirectURI runs. A redirect that fosite infers, or one
that the loopback rewrite produces, never qualifies.

The prompt=consent check moves out of beginIfRemembered into
promptRequiresConsent. It now gates both the exemption and the
remembered-approval lookup, so a client that asks for consent always
sees the page. An exempt request marks the pending authorization
ConsentStageApproved and calls beginUpstream, which stores its own
copy of the pending request. It never reads remembered approvals.
Upstream authentication is unchanged.

Co-authored-by: Mecatl <noreply@mecatl.dev>
The consent exemption lets allowlisted clients skip ToolHive's consent
page, so a small regression could quietly preauthorize requests that
should still need a user's approval. These tests fix in place the
guarantees that the trust model depends on.

- Exemption needs one explicit redirect_uri that exactly matches a
  registered one. Inferred, repeated, empty, mismatched or malformed
  redirects, and non-code response types, fall back to consent or keep
  their usual OAuth errors.
- A valid loopback port rewrite never counts as exempt, so a dynamic
  redirect cannot be preauthorized.
- prompt=consent, whether single, repeated or space-separated, still
  forces the page, even when a remembered approval would cover it.
- The exempt path never reads remembered consent state and never sets a
  remembered-consent cookie. A client that has stopped being eligible
  (for example, became public) gets ordinary consent or its remembered
  approval with that approval's user binding.
- Each exempt flow gets its own upstream state, PKCE verifier, nonce
  and browser-binding cookie. A callback that does not carry the
  initiating browser's cookie is rejected without storing tokens or
  consuming the pending flow.
- DCR ignores a client_id supplied by the caller and never overwrites
  an existing registration. The trust model relies on this to claim
  that remote registration cannot take over an exempt ID.

Co-authored-by: Mecatl <noreply@mecatl.dev>
Allowlisting a client ID is an administrator trust decision, not user
consent. Embedders need to know what they are agreeing to before they
use it. This adds an architecture section and expands the Go doc
comments on Config.ConsentExemptClientIDs and both
WithConsentExemptClientIDs options.

The docs state that an allowlist entry preauthorizes whatever
registration currently holds that ID, for any user who completes
upstream login. If the registration is replaced or its permissions
grow, the exemption moves or grows with it, so the embedder must
control registration writes. DCR cannot choose or overwrite an exempt
ID, but a privileged provisioner can. Removing an ID only affects new
requests and does not revoke tokens.

Because an exempt link can complete silently for a user who is already
signed in upstream, the downstream client must bind each transaction
itself. That means fresh state and an S256 PKCE pair per transaction,
tied to the initiating session and browser and checked at the callback,
with the verifier taken from stored state. The doc also records that
exempting an HTTP-DCR client deliberately departs from the MCP
authorization spec's user-consent requirement, and that prompt=none and
prompt=login are not specially handled.

Debug logging changes to match. The authorize log no longer records
client_id and redirect_uri. A per-request exemption decision is logged
only when an allowlist is configured. The startup log reports whether
exemption is on and how many IDs are listed, instead of the issuer.

Co-authored-by: Mecatl <noreply@mecatl.dev>
Config.Validate, newServer and AuthorizeHandler needed changes to pass
lint after the consent exemption work. This moves self-contained blocks
out of them into named helpers, so each function reads as a short
sequence of steps:

- validateIssuerAndConsentConfig runs the issuer, confidential client,
  private_key_jwt, authorization endpoint and consent exemption checks
  from Config.Validate, in the same order.
- validateAuthorizationResource does resource selection and audience
  validation for AuthorizeHandler, writing the same OAuth errors.
- prepareServerStorage does the storage nil check, DCR store assertion,
  SPIFFE bundle source and decoration, consent-exempt client validation
  and delegate client registration for newServer. It closes the bundle
  source itself when a later step fails, so nothing leaks.

Behavior is unchanged. The rest is gofmt alignment, wrapping the long
ConsentExemptClientIDs comment and test table entries, and importing
upstream as authupstream in a handler test.

Co-authored-by: Mecatl <noreply@mecatl.dev>
The consent exemption checked the explicit redirect_uri, copied from
the form before fosite runs, against the registered redirects. That is
not always the redirect fosite ends up using. In fosite v0.49.0,
newAuthorizeRequest sets request.Form = r.Form, applies the claims of a
signed OIDC request object to that form, and only then validates the
redirect. A request object can therefore replace redirect_uri after the
explicit value was checked. consentExempt now also requires
ar.GetRedirectURI() to equal the explicit value. If it does not, the
request gets the consent page.

isURLShaped treated a url.Parse error as an opaque client ID and let it
through. It now fails closed and rejects IDs that do not parse.

Other changes:
- Restore client_id and redirect_uri in the authorize debug log and
  the issuer in the startup log, which earlier commits had dropped.
- Quote the MCP spec MUST on consent for dynamically registered clients
  in the architecture doc, and state that prompt=consent takes
  precedence over the exemption and remembered approval.
- Move eligibility and malformed-URL tests into the consentpolicy
  package, trim the wrapper tests to checking they reach the shared
  validator, and test the request-object override and log redaction.
- Rename prepareServerStorage to prepareServerStorageAndRegisterClients,
  wrap the NewHandler validation error, use oauthproto.ResponseTypeCode.

Co-authored-by: Mecatl <noreply@mecatl.dev>
@github-actions github-actions Bot added the size/XL Extra large PR: 1000+ lines changed label Oct 20, 2026
@codecov

codecov Bot commented Oct 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.42857% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.69%. Comparing base (a8b7a01) to head (a30e9af).

Files with missing lines Patch % Lines
pkg/authserver/server_impl.go 87.87% 4 Missing ⚠️
pkg/authserver/server/handlers/authorize.go 96.55% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6882      +/-   ##
==========================================
+ Coverage   79.65%   79.69%   +0.03%     
==========================================
  Files         821      822       +1     
  Lines       83728    83843     +115     
==========================================
+ Hits        66696    66819     +123     
+ Misses      17027    17019       -8     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rdimitrov

Copy link
Copy Markdown
Member

Reviewed a30e9af; no substantive findings. The exemption stays behind normal authorization validation, requires an eligible allowlisted client and an explicit exact redirect, and preserves prompt=consent precedence. It reuses the existing upstream state, PKCE, nonce and browser-binding path. The administrator-preauthorization trade-off and downstream transaction-binding responsibilities are clearly documented.

GitHub unit-test and lint checks passed. My local task test run timed out without completing.

@jhrozek
jhrozek merged commit 8aaed56 into main Oct 20, 2026
47 checks passed
@jhrozek
jhrozek deleted the skip-consent branch October 20, 2026 12:15
tgrunnagle added a commit that referenced this pull request Oct 20, 2026
Resolve conflicts with the consent-exempt clients change (#6882) by
keeping both sides: Config.Validate runs both the client admission and
the issuer/consent checks, and buildHandlerOptions wires both the
register rate limit and the consent-exempt client IDs.

NewHandler moves the consent-exemption setup into a helper to stay
under the gocyclo limit, and runs it before registering the DCR
metrics so a validation failure cannot leave a gauge callback behind.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot mentioned this pull request Oct 22, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Let an embedder exempt its own pre-registered client from the consent page

2 participants