[tgrunnagle]

Summary

The embedded auth server's consent page says "You will sign in with {{.Upstream}} next.", where the value is the upstream's Name. Name is a routing and storage key restricted to DNS-label characters, so an embedder that keys upstreams by an opaque id (for example a UUID provider id) shows that id to every end user, and even hand-written configs cannot say something like "Acme Okta".

• Add an optional, display-only DisplayName (display_name) to authserver.UpstreamRunConfig, authserver.UpstreamConfig and handlers.NamedUpstream, copied through buildUpstreamConfig (both oidc and oauth2) and buildUpstreams. It is never compared, persisted, or used as a key, and validation never rejects it.
• The consent page's "You will sign in with … next." line now shows the upstream's DisplayName, resolved from the handler's current upstream list at render time. It falls back to Name when the label is unset, empty after sanitizing, or the pending authorization's upstream is no longer configured. Technical details keep showing Name as the internal name.
• The label gets the same treatment as the client name: control and format runes are dropped via displayClientName, whitespace is trimmed, it is capped at maxConsentTextRunes (renamed from maxConsentClientNameRunes, since it now covers both) with a trailing ellipsis via a new shared truncateConsentText helper, and the template HTML-escapes it. The helper now also trims whitespace left before the ellipsis, which applies to client names too (see reviewer notes).
• Document the field, its fallback, that it is not a key, and the embedder's trust obligation in docs/arch/11-auth-server-storage.md; regenerate swagger and the Go SDK OpenAPI client.

Closes #6922

Type of change

• New feature

Test plan

• Unit tests (task test)

New tests, mapped to the issue's acceptance criteria:

• TestBuildUpstreamConfigs_CopiesDisplayName and TestBuildUpstreams_CopiesDisplayName (AC1.1, AC1.2): the field is copied unchanged for oidc and oauth2 through both conversion paths. The runner test goes through buildUpstreamConfigs, so validateUpstreamRunConfig runs on each malformed label. A temporary display-name rejection in the validator makes it fail.
• TestUpstreamConfig_DisplayNameNeverFailsValidation (AC1.2): Config.Validate accepts any display name, including empty or malformed, and still rejects an invalid Name.
• TestRunConfig_DisplayNameRoundTrip: display_name decodes from YAML and JSON, survives re-encoding, and is omitted when empty.
• TestDisplayNameChangeKeepsStoredRecordsValid (AC1.3): after a display name change, the pending authorization, upstream tokens and remembered consent still load and match by Name, for both memory and Redis backends.
• TestConsentPage_UpstreamDisplayName (AC2.1): page reads "You will sign in with Acme Okta next." for an upstream named ffffffff-ffff-ffff-ffff-ffffffffffff, with the UUID still in Technical details.
• TestConsentPage_UpstreamDisplayNameViaAuthorize (AC2.1): the label reaches the page through a real GET /oauth/authorize. With two upstreams, only the first one's label appears.
• TestConsentPage_UpstreamDisplayNameSanitized (AC2.2, AC2.3, AC2.5): bidi overrides, zero-width characters and newlines are stripped; HTML, & and quotes are escaped; a label exactly at the cap is kept whole; longer labels are cut with … and no space before it; empty and whitespace-only labels fall back to Name.
• TestConsentPage_MissingUpstreamFallsBackToName (AC2.4): falls back to the pending UpstreamProviderName when the upstream is gone.
• The existing TestConsentPage_LayoutAndNameTruncation passes unchanged, so the page is unchanged when DisplayName is empty.

Changes

File	Change
pkg/authserver/config.go	Add DisplayName to UpstreamRunConfig and UpstreamConfig
pkg/authserver/runner/embeddedauthserver.go	Copy DisplayName in buildUpstreamConfig for both provider types
pkg/authserver/server_impl.go	Copy DisplayName onto handlers.NamedUpstream
pkg/authserver/server/handlers/handler.go	Add DisplayName to NamedUpstream
pkg/authserver/server/handlers/consent.go	Add UpstreamDisplay to page data; resolve and sanitize the label at render; extract truncateConsentText; rename the cap to maxConsentTextRunes
docs/arch/11-auth-server-storage.md	Document the display name, its fallback, sanitizing and the embedder obligation
docs/server/*, sdk/go/*	Regenerated for display_name

Does this introduce a user-facing change?

Yes. Before: the consent page always said "You will sign in with <upstream name> next." After: when an upstream sets display_name, that line shows the sanitized display name instead (for example "You will sign in with Acme Okta next."), and Technical details still show the internal name. Configs without display_name render the same page as before.

Special notes for reviewers

• Trust model / spoofing (CWE-451): sanitizing removes control and format runes but does not judge meaning. Printable misleading text such as "Acme Okta (verified)" is shown in full on the anti-phishing gate. Toolhive cannot control who writes the field, so the architecture doc requires embedders to populate DisplayName only from operator-controlled values.
• Cleaned at render, not rejected at validation: this is a deliberate choice so that a cosmetic field cannot fail server start or an embedder's runtime rebuild of its upstreams.
• Resolved at render time: the label comes from the handler's current upstream list instead of being captured at authorize time, so a handler rebuilt with a different upstream set shows the current value while pending authorizations in shared storage stay valid. Approval still fails closed on a changed upstream (TestConsentHandler_ChangedUpstreamFailsClosed).
• Client-name truncation changes slightly: truncateConsentText is shared with the client name, so a client name cut at 80 runes whose last kept rune is whitespace now shows name… instead of name …. This is cosmetic, and the change was intended so both values follow one rule.
• Invisible fillers are documented, not handled: characters such as U+3164 are neither control nor format runes, so a label made only of them renders blank instead of falling back to Name. The arch doc states this. The cleaner is shared with client names, and the field is operator-controlled.
• Out of scope (per the issue): a displayName field on the operator's MCPExternalAuthConfig CRD, defaulting the label to the issuer host, and showing the label on the device verification page or in logs.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.01%. Comparing base (f4f0d83) to head (c72b811).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6929      +/-   ##
==========================================
+ Coverage   80.00%   80.01%   +0.01%     
==========================================
  Files         837      837              
  Lines       85205    85229      +24     
==========================================
+ Hits        68172    68200      +28     
+ Misses      17028    17024       -4     
  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.

[tgrunnagle]

Multi-Agent Consensus Review

Agents consulted: security, test-coverage, architecture/API/docs, general-quality

Consensus Summary

#	Finding	Consensus	Severity	Action
1	Pin empty DisplayName on a configured upstream	10/10	LOW	Fix
2	No YAML/JSON round-trip test for display_name	10/10	MEDIUM	Fix
3	No authorize→consent or multi-upstream display-name test	10/10	LOW	Fix
4	AC1.3 storage test is largely tautological	10/10	LOW	Discuss
5	RunConfig half of the validation test is vacuous	10/10	LOW	Fix
6	Invisible filler runes survive sanitising	10/10	LOW	Discuss
7	Truncation edge cases and comment accuracy	10/10	LOW	Fix
8	Sanitisation and truncation test gaps	10/10	LOW	Fix
9	"Swapped upstream set" wording overstates	10/10	LOW	Fix
10	maxConsentClientNameRunes name is client-only	10/10	LOW	Fix
11	upstreamDisplayName loop uses continue/break	10/10	LOW	Fix
12	British "sanitised" in public API and SDK docs	10/10	LOW	Discuss
13	Dense arch-doc paragraph; hard-coded 80	10/10	LOW	Fix
14	Test structure and a weaker fail-closed assertion	10/10	LOW	Fix

Overall

This adds an optional, display-only DisplayName for upstreams and shows it on the consent page's "You will sign in with … next." line. The approach is sound and matches #6922. Name stays the key everywhere, the label only reaches an escaped text node, approval still fails closed, and the field is additive, so older binaries and existing configs are unaffected. Each acceptance criterion has a named test, the generated swagger, OpenAPI and SDK artifacts agree with each other, and out-of-scope items are left alone.

Nothing here blocks the PR. The one MEDIUM item is that no test round-trips the display_name tag through YAML or JSON, so a tag typo would pass every new test. The rest is polish: a few sanitiser and truncation edge cases, wording that implies the handler hot-swaps upstreams, and test structure.

Documentation

• docs/arch/11-auth-server-storage.md:240: "a swapped upstream set shows the current value" implies in-place mutation, but h.upstreams is set once in NewHandler. The paragraph is also one dense block with a hard-coded "80 runes", and could note the field isn't yet settable from the MCPExternalAuthConfig CRD.
• pkg/authserver/config.go:766: "sanitised" is copied into swagger, the SDK OpenAPI files and the ogen docs. The repo overwhelmingly uses "sanitize".

---

Generated with Claude Code

[tgrunnagle]

Rechecked the earlier review against bbf779a6b. 13 of the 14 threads are resolved. One is still open, and the PR description needs a refresh.

Open: the runner-side AC1.2 test doesn't exercise validation (LOW, pkg/authserver/runner/embeddedauthserver_test.go, TestBuildUpstreamConfig_CopiesDisplayName)

The test comment and the thread reply say this goes through validateUpstreamRunConfig and shows the runner's validation never rejects a display name. But the test calls buildUpstreamConfig, which doesn't validate. validateUpstreamRunConfig is called from buildOneUpstreamConfig (embeddedauthserver.go:902). A display-name check added to that validator would not fail this test, so it isn't yet the guard AC1.2 asks for on the runner side.

Either drive the same cases through buildUpstreamConfigs(ctx, []authserver.UpstreamRunConfig{rc}, issuer, store, false), as the existing tests around embeddedauthserver_test.go:2934 do, or drop the validation claim from the comment so it only says "copies through unchanged".

PR description is stale after the renames (LOW)

• It still names maxConsentClientNameRunes (now maxConsentTextRunes) and TestConsentPage_UpstreamDisplayNameSanitised (now ...Sanitized).
• It says the AC1.2 test covers RunConfig.Validate, but those subtests were removed.
• It doesn't list the new TestRunConfig_DisplayNameRoundTrip or TestConsentPage_UpstreamDisplayNameViaAuthorize.
• truncateConsentText now trims whitespace before the ellipsis for client names too, not only the new label. It's cosmetic, but the repo asks for deviations from sibling paths to be stated in the PR body.

Everything else checks out against the code: the display_name round-trip test, the real authorize→consent test including the multi-upstream case, the empty and boundary rows, the "" fallback, the maxConsentTextRunes rename, the return name simplification, the reworded and split arch-doc paragraph, and the regenerated "sanitized" swagger and SDK files. The Hangul-filler gap is documented rather than coded around, which is reasonable for an operator-controlled field.

---

Generated with Claude Code

[tgrunnagle]

Thanks for the recheck.

• Runner-side AC1.2 test: addressed in c72b811. TestBuildUpstreamConfigs_CopiesDisplayName now calls buildUpstreamConfigs, so validateUpstreamRunConfig runs on each label. I checked that a display-name rejection added to the validator makes it fail. I also corrected my earlier reply on that thread.
• PR description: refreshed. It now uses the renamed constant and test, drops the RunConfig.Validate claim, and lists TestRunConfig_DisplayNameRoundTrip and TestConsentPage_UpstreamDisplayNameViaAuthorize. Reviewer notes now state the change to client-name truncation and the documented invisible-filler gap.