fix(oauth): the consent screen names Pad, the instance, the user's workspaces and the return host (TASK-1071) (#2017)

* fix(oauth): the consent screen names Pad, the instance, the user's workspaces and the return host (TASK-1071) The consent screen is the trust anchor the RFC 8707 relaxation leans on. It had lost its 'your Pad workspaces' identification, the default 'All my workspaces' choice showed no workspace names, and the app's self-chosen name was unbounded and unlabelled. - 'Pad · <host>' above the heading; the card reads '<app> is requesting access to your Pad workspaces on <host>'. - The app's name is labelled unverified, with the redirect host the browser returns to. - The first five workspace names (and N more) under 'All my workspaces'. - Client names capped at 80 runes, whitespace collapsed, long tokens wrap. - Tests pin the identification, the membership-only list, and the four name/logo edge cases. * fix(oauth): name a custom-scheme return with its scheme (TASK-1071, codex)
2026-10-26 06:19:49 +00:00 · 2026-10-10 09:15:51 -04:00
parent 5960cf1978
commit 6f6af0cf2c
2 changed files with 375 additions and 9 deletions
@@ -1602,7 +1602,17 @@ body {
    background: #2a2a2a; color: #e8e8e8; border-color: #555;
  }
 }
-h1 { font-size: 1.5em; margin: 0 0 .25em; }
+h1 { font-size: 1.5em; margin: 0 0 .25em; overflow-wrap: anywhere; }
+.resource-brand {
+  margin: 0 0 .75em; font-size: 1.05em; color: #444;
+  overflow-wrap: anywhere;
+}
+.resource-brand strong { font-size: 1.15em; color: #1a1a1a; }
+@media (prefers-color-scheme: dark) {
+  .resource-brand { color: #bbb; }
+  .resource-brand strong { color: #fff; }
+}
+.client-card .meta { min-width: 0; overflow-wrap: anywhere; }
 .client-card {
  display: flex; align-items: center; gap: 1em; margin: 1.5em 0;
  padding: 1em; border: 1px solid #ddd; border-radius: 10px; background: #fafafa;
@@ -1656,6 +1666,11 @@ input[type=text], input[type=search] {
  .picker { background: #1c1c1c; }
 }
 .hint { color: #555; font-size: .9em; margin: .75em 0; }
+.ws-summary {
+  margin: .1em 0 .5em 1.9em; color: #555; font-size: .9em;
+  overflow-wrap: anywhere;
+}
+@media (prefers-color-scheme: dark) { .ws-summary { color: #aaa; } }
 .footer-disclosure {
  margin: 1.5em 0; padding: .85em 1em;
  background: #f5f5f5; border-radius: 8px; color: #555; font-size: .9em;
@@ -1690,6 +1705,7 @@ button.primary:hover:not(:disabled) { background: #1e54d4; }
 </style>
 </head>
 <body>
+<p class="resource-brand"><strong>Pad</strong> · {{.ResourceHost}}</p>
 <h1>Authorize {{.ClientName}}</h1>
 {{if .Surface}}<p class="surface">Connecting Pad's <strong>{{.Surface}}</strong> tools.</p>{{end}}
 <p>Signed in as <strong>{{.Username}}</strong>{{if .UserEmail}} ({{.UserEmail}}){{end}}.</p>
@@ -1698,7 +1714,8 @@ button.primary:hover:not(:disabled) { background: #1e54d4; }
  {{if .ClientLogoURL}}<img class="logo" src="{{.ClientLogoURL}}" alt="">{{end}}
  <div class="meta">
    <strong>{{.ClientName}}</strong>
-    <small>wants to access your Pad on your behalf.</small>
+    <small>is requesting access to your Pad workspaces on {{.ResourceHost}}.</small>
+    <small class="unverified">The app chose this name; Pad has not verified it.{{if .RedirectHost}} After you decide, you return to <strong>{{.RedirectHost}}</strong>.{{end}}</small>
  </div>
 </div>

@@ -1748,6 +1765,9 @@ button.primary:hover:not(:disabled) { background: #1e54d4; }
      <input type="radio" name="workspace_access" value="all" id="access-all"{{if or .DefaultWildcard (not .Workspaces)}} checked{{end}}>
      <span><strong>All my workspaces</strong><span class="desc">— includes workspaces you join or create later.</span></span>
    </label>
+    {{if .WorkspaceSummary}}
+    <p class="ws-summary">Your Pad workspaces now: {{range $i, $n := .WorkspaceSummary}}{{if $i}}, {{end}}<strong>{{$n}}</strong>{{end}}{{if .WorkspaceMore}} and {{.WorkspaceMore}} more{{end}}.</p>
+    {{end}}
    {{if .Workspaces}}
      <label class="access-row">
        <input type="radio" name="workspace_access" value="specific" id="access-specific"{{if not .DefaultWildcard}} checked{{end}}>
@@ -1878,18 +1898,38 @@ button.primary:hover:not(:disabled) { background: #1e54d4; }
 // consentData is the template's data shape. Field names match the
 // template's references; adding a new field requires updating both.
 type consentData struct {
+	// ClientName is the name the app registered for itself (or its
+	// client_id), capped by consentClientName. It is the app's CLAIM, so
+	// the page labels it unverified and names Pad and the redirect host,
+	// which the app cannot choose (TASK-1071).
 	ClientName string
+	// ResourceHost names the Pad instance being authorized: the host of
+	// the configured public URL, else the request's Host. This is the
+	// "your Pad workspaces on <host>" identification the RFC 8707
+	// relaxation leans on (see audienceForAuthorize).
+	ResourceHost string
+	// RedirectHost names the redirect_uri fosite validated against the
+	// client's registration: where the user's browser goes after deciding
+	// (see consentRedirectTarget).
+	RedirectHost string
 	// Surface names the Pad surface being connected when it is not /mcp
 	// (TASK-3321 U2b): "ChatGPT" when the request names the ChatGPT
 	// catalog's resource. Set by the server from the requested audience,
 	// never from the client's own name, so a user is not approving an
 	// unlabelled second door.
-	Surface          string
-	ClientLogoURL    string
-	Username         string
-	UserEmail        string
-	Workspaces       []consentWorkspaceRow // user's workspace memberships
-	CanRead          bool                  // tier radios — true iff client requested the corresponding pad:* scope
+	Surface       string
+	ClientLogoURL string
+	Username      string
+	UserEmail     string
+	Workspaces    []consentWorkspaceRow // user's workspace memberships
+	// WorkspaceSummary is the first consentWorkspaceSummaryMax workspace
+	// names, shown under "All my workspaces" so the default choice still
+	// names the user's actual Pad workspaces (the picker that lists them
+	// is hidden until "Only specific" is picked); WorkspaceMore counts the
+	// rest (TASK-1071).
+	WorkspaceSummary []string
+	WorkspaceMore    int
+	CanRead          bool // tier radios — true iff client requested the corresponding pad:* scope
 	CanWrite         bool
 	CanAdmin         bool
 	DefaultTier      string // "read" / "write" / "admin" — initially-checked radio
@@ -2045,8 +2085,22 @@ func (s *Server) renderConsent(w http.ResponseWriter, r *http.Request, ar fosite
 		suggested = truncateBindableText(suggested, 120)
 	}

+	summary := make([]string, 0, consentWorkspaceSummaryMax)
+	for i, row := range rows {
+		if i == consentWorkspaceSummaryMax {
+			break
+		}
+		summary = append(summary, row.Name)
+	}
+
+	redirectHost := consentRedirectTarget(ar.GetRedirectURI())
+
 	data := consentData{
-		ClientName:       clientName,
+		ClientName:       consentClientName(clientName),
+		ResourceHost:     s.consentResourceHost(r),
+		RedirectHost:     redirectHost,
+		WorkspaceSummary: summary,
+		WorkspaceMore:    len(rows) - len(summary),
 		Surface:          s.consentSurface(ar),
 		ClientLogoURL:    logo,
 		Username:         user.Name,
@@ -2207,6 +2261,66 @@ func (s *Server) consentSurface(ar fosite.AuthorizeRequester) string {
 	return ""
 }

+// consentClientNameMax caps the client's self-chosen name on the consent
+// page, in runes. Registration does not limit it (RFC 7591 sets no bound),
+// and an unbounded name could push the Pad identification and the
+// workspace list below the fold (TASK-1071).
+const consentClientNameMax = 80
+
+// consentWorkspaceSummaryMax is how many workspace names the "All my
+// workspaces" line names before "and N more".
+const consentWorkspaceSummaryMax = 5
+
+// consentClientName returns name cut to consentClientNameMax runes, with an
+// ellipsis when it was cut, and whitespace runs collapsed so a name cannot
+// pad itself out with blank lines.
+func consentClientName(name string) string {
+	name = strings.Join(strings.Fields(name), " ")
+	r := []rune(name)
+	if len(r) <= consentClientNameMax {
+		return name
+	}
+	return string(r[:consentClientNameMax-1]) + "…"
+}
+
+// consentRedirectTarget names where the browser goes after the decision.
+// An https or http redirect is named by its host. Any other scheme hands the
+// browser to an app, so it is named with the scheme: "claude://oauth", not
+// "oauth", which reads as a website. A URI with no host (registration refuses
+// one today, but the page must not go quiet if that changes) is named by its
+// scheme and path.
+func consentRedirectTarget(u *url.URL) string {
+	if u == nil {
+		return ""
+	}
+	switch {
+	case u.Scheme == "http" || u.Scheme == "https":
+		return u.Host
+	case u.Host != "":
+		return u.Scheme + "://" + u.Host
+	case u.Scheme != "":
+		return consentClientName(u.Scheme + ":" + u.Opaque + u.Path)
+	}
+	return ""
+}
+
+// consentResourceHost names the Pad instance on the consent page: the host
+// of the configured public URL, else the host the request came in on (also
+// when the configured URL names an unspecified address like 0.0.0.0, which
+// is a bind address and not a name the user would recognize).
+func (s *Server) consentResourceHost(r *http.Request) string {
+	if s.baseURL != "" {
+		if u, err := url.Parse(s.baseURL); err == nil {
+			switch u.Hostname() {
+			case "", "0.0.0.0", "::":
+			default:
+				return u.Host
+			}
+		}
+	}
+	return r.Host
+}
+
 // dedupeStrings returns values with each string once, in first-seen order.
 func dedupeStrings(values []string) []string {
 	seen := make(map[string]bool, len(values))
@@ -0,0 +1,252 @@
+package server
+
+import (
+	"html"
+	"net/http"
+	"net/url"
+	"strings"
+	"testing"
+
+	"github.com/PerpetualSoftware/pad/internal/models"
+)
+
+// TASK-1071: the consent screen is the trust anchor the RFC 8707 relaxation
+// leans on (audienceForAuthorize), so it must say plainly that the resource
+// is Pad, name which Pad, list the user's actual workspaces, and not let the
+// app's self-chosen name crowd that out or pass as verified.
+
+func registerNamedTestClient(t *testing.T, srv *Server, name, redirectURI string) string {
+	t.Helper()
+	rr := doRequest(srv, "POST", "/oauth/register", map[string]any{
+		"client_name":   name,
+		"redirect_uris": []string{redirectURI},
+	})
+	if rr.Code != http.StatusCreated {
+		t.Fatalf("register %q: %d (body: %s)", name, rr.Code, rr.Body.String())
+	}
+	var resp map[string]any
+	parseJSON(t, rr, &resp)
+	id, _ := resp["client_id"].(string)
+	if id == "" {
+		t.Fatal("register: empty client_id")
+	}
+	return id
+}
+
+func renderConsentFor(t *testing.T, srv *Server, clientID, redirectURI, sessionToken string) string {
+	t.Helper()
+	q := url.Values{
+		"client_id":             {clientID},
+		"response_type":         {"code"},
+		"redirect_uri":          {redirectURI},
+		"scope":                 {"pad:read"},
+		"resource":              {testCanonicalAudience},
+		"audience":              {testCanonicalAudience},
+		"code_challenge":        {s256Challenge("code-verifier-abc123-must-be-43-to-128-chars-long")},
+		"code_challenge_method": {"S256"},
+		"state":                 {"task-1071-state"},
+	}
+	rr := doAuthedRequest(srv, "GET", "/oauth/authorize?"+q.Encode(), nil, sessionToken)
+	if rr.Code != http.StatusOK {
+		t.Fatalf("consent: %d (location: %s)", rr.Code, rr.Header().Get("Location"))
+	}
+	return rr.Body.String()
+}
+
+// bodyOf is the page after <body>, so CSS and the <title> never satisfy an
+// assertion about what the user reads.
+func bodyOf(t *testing.T, page string) string {
+	t.Helper()
+	i := strings.Index(page, "<body>")
+	if i < 0 {
+		t.Fatal("no <body> in consent page")
+	}
+	return page[i:]
+}
+
+func TestConsentIdentifiesPadAndTheUsersWorkspaces(t *testing.T) {
+	t.Parallel()
+	srv, _ := oauthEnabledTestServer(t)
+	srv.SetBaseURL("https://pad.example.org")
+	user, session := loginTestUser(t, srv)
+	mustSeedWorkspaceWithRole(t, srv, user.ID, "Alpha Project", "alpha-project", "owner")
+	mustSeedWorkspaceWithRole(t, srv, user.ID, "Beta Notes", "beta-notes", "viewer")
+	other, err := srv.store.CreateUser(models.UserCreate{Email: "other-1071@example.com", Name: "Other", Password: "pw-test-12345"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	mustSeedWorkspaceWithRole(t, srv, other.ID, "Not Yours", "not-yours", "owner")
+
+	id := registerNamedTestClient(t, srv, "Cursor", "https://app.test/cb")
+	page := bodyOf(t, renderConsentFor(t, srv, id, "https://app.test/cb", session))
+
+	for _, want := range []string{
+		`<strong>Pad</strong> · pad.example.org`,
+		"is requesting access to your Pad workspaces on pad.example.org.",
+		"The app chose this name; Pad has not verified it.",
+		"you return to <strong>app.test</strong>",
+		`value="alpha-project"`,
+		"Alpha Project",
+		`value="beta-notes"`,
+		"Beta Notes",
+		// Named under the default "All my workspaces" choice too, where the
+		// per-workspace picker is hidden.
+		"Your Pad workspaces now: <strong>Alpha Project</strong>, <strong>Beta Notes</strong>.",
+	} {
+		if !strings.Contains(page, want) {
+			t.Errorf("consent page must contain %q", want)
+		}
+	}
+	if strings.Contains(page, "not-yours") || strings.Contains(page, "Not Yours") {
+		t.Error("consent page lists a workspace the user is not a member of")
+	}
+	// Pad is named before the app is.
+	if p, c := strings.Index(page, "<strong>Pad</strong>"), strings.Index(page, "Authorize Cursor"); p < 0 || c < 0 || p > c {
+		t.Errorf("Pad must be identified above the app's name (pad at %d, app at %d)", p, c)
+	}
+}
+
+func TestConsentWorkspaceSummaryNamesFiveThenCounts(t *testing.T) {
+	t.Parallel()
+	srv, _ := oauthEnabledTestServer(t)
+	user, session := loginTestUser(t, srv)
+	for _, n := range []string{"A1", "A2", "A3", "A4", "A5", "A6", "A7"} {
+		mustSeedWorkspaceWithRole(t, srv, user.ID, "WS "+n, "ws-"+strings.ToLower(n), "editor")
+	}
+	id := registerNamedTestClient(t, srv, "Cursor", "https://app.test/cb")
+	page := bodyOf(t, renderConsentFor(t, srv, id, "https://app.test/cb", session))
+	want := "Your Pad workspaces now: <strong>WS A1</strong>, <strong>WS A2</strong>, <strong>WS A3</strong>, <strong>WS A4</strong>, <strong>WS A5</strong> and 2 more."
+	if !strings.Contains(page, want) {
+		t.Errorf("summary must name five and count the rest; want %q", want)
+	}
+}
+
+func TestConsentResourceHostFallsBackToTheRequestHost(t *testing.T) {
+	t.Parallel()
+	for _, base := range []string{"", "http://0.0.0.0:7777"} {
+		srv, _ := oauthEnabledTestServer(t)
+		srv.SetBaseURL(base)
+		_, session := loginTestUser(t, srv)
+		id := registerNamedTestClient(t, srv, "Cursor", "https://app.test/cb")
+		page := bodyOf(t, renderConsentFor(t, srv, id, "https://app.test/cb", session))
+		// httptest requests arrive with Host "example.com".
+		if !strings.Contains(page, "your Pad workspaces on example.com.") {
+			t.Errorf("base %q: consent page must name the request host", base)
+		}
+	}
+}
+
+// The four client-name/logo edge cases the task names.
+func TestConsentClientNameEdgeCases(t *testing.T) {
+	t.Parallel()
+
+	t.Run("a long name is cut, and Pad's identification still renders", func(t *testing.T) {
+		t.Parallel()
+		srv, _ := oauthEnabledTestServer(t)
+		_, session := loginTestUser(t, srv)
+		long := strings.Repeat("Very Long Client Name ", 50)
+		id := registerNamedTestClient(t, srv, long, "https://app.test/cb")
+		page := bodyOf(t, renderConsentFor(t, srv, id, "https://app.test/cb", session))
+		if strings.Contains(page, strings.TrimSpace(long)) {
+			t.Error("the full 1100-character name must not render")
+		}
+		if !strings.Contains(page, "…") {
+			t.Error("a cut name must show it was cut")
+		}
+		if !strings.Contains(page, "your Pad workspaces on") {
+			t.Error("Pad identification must survive a long client name")
+		}
+	})
+
+	t.Run("a name claiming to be Pad is still labelled the app's own claim", func(t *testing.T) {
+		t.Parallel()
+		srv, _ := oauthEnabledTestServer(t)
+		_, session := loginTestUser(t, srv)
+		id := registerNamedTestClient(t, srv, "Pad (getpad.dev) official", "https://evil.test/cb")
+		page := bodyOf(t, renderConsentFor(t, srv, id, "https://evil.test/cb", session))
+		for _, want := range []string{
+			"The app chose this name; Pad has not verified it.",
+			"you return to <strong>evil.test</strong>",
+		} {
+			if !strings.Contains(page, want) {
+				t.Errorf("must contain %q", want)
+			}
+		}
+	})
+
+	t.Run("markup in the name is escaped, never rendered", func(t *testing.T) {
+		t.Parallel()
+		srv, _ := oauthEnabledTestServer(t)
+		_, session := loginTestUser(t, srv)
+		evil := `<script>alert(1)</script><img src=x onerror=alert(2)>`
+		id := registerNamedTestClient(t, srv, evil, "https://app.test/cb")
+		page := bodyOf(t, renderConsentFor(t, srv, id, "https://app.test/cb", session))
+		if strings.Contains(page, "<script>alert(1)") || strings.Contains(page, "<img src=x") {
+			t.Error("client name markup must not reach the page unescaped")
+		}
+		if !strings.Contains(page, html.EscapeString("<script>alert(1)</script>")) {
+			t.Error("the escaped name must render")
+		}
+	})
+
+	t.Run("no logo renders no img, and the card still identifies Pad", func(t *testing.T) {
+		t.Parallel()
+		srv, _ := oauthEnabledTestServer(t)
+		_, session := loginTestUser(t, srv)
+		id := registerNamedTestClient(t, srv, "Cursor", "https://app.test/cb")
+		page := bodyOf(t, renderConsentFor(t, srv, id, "https://app.test/cb", session))
+		if strings.Contains(page, `<img class="logo"`) {
+			t.Error("no logo registered, so no logo img")
+		}
+		if !strings.Contains(page, "your Pad workspaces") {
+			t.Error("card must identify Pad without a logo")
+		}
+	})
+}
+
+func TestConsentClientNameCap(t *testing.T) {
+	t.Parallel()
+	if got := consentClientName("  Claude\n\n\n  Desktop  "); got != "Claude Desktop" {
+		t.Errorf("whitespace runs collapse: got %q", got)
+	}
+	exact := strings.Repeat("é", consentClientNameMax)
+	if got := consentClientName(exact); got != exact {
+		t.Error("a name at the cap is unchanged")
+	}
+	got := consentClientName(exact + "x")
+	if r := []rune(got); len(r) != consentClientNameMax || r[len(r)-1] != '…' {
+		t.Errorf("one rune over the cap is cut to the cap with an ellipsis: %q", got)
+	}
+}
+
+func TestConsentRedirectTarget(t *testing.T) {
+	t.Parallel()
+	for raw, want := range map[string]string{
+		"https://app.test/cb":             "app.test",
+		"http://127.0.0.1:33418/callback": "127.0.0.1:33418",
+		"claude://oauth/callback":         "claude://oauth",
+		"com.example.app:/oauth/callback": "com.example.app:/oauth/callback",
+	} {
+		u, err := url.Parse(raw)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if got := consentRedirectTarget(u); got != want {
+			t.Errorf("%s: got %q, want %q", raw, got, want)
+		}
+	}
+	if consentRedirectTarget(nil) != "" {
+		t.Error("nil URL names nothing")
+	}
+}
+
+func TestConsentNamesACustomSchemeReturnWithItsScheme(t *testing.T) {
+	t.Parallel()
+	srv, _ := oauthEnabledTestServer(t)
+	_, session := loginTestUser(t, srv)
+	id := registerNamedTestClient(t, srv, "Claude", "claude://oauth/callback")
+	page := bodyOf(t, renderConsentFor(t, srv, id, "claude://oauth/callback", session))
+	if !strings.Contains(page, "you return to <strong>claude://oauth</strong>") {
+		t.Error("a custom-scheme return must be named with its scheme")
+	}
+}