From f12ce5aaf8a8f33d0de467d0fe8b43bb67f62248 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 09:38:02 -0400 Subject: [PATCH] fix: correct two false claims in the terminal-notice copy (review round 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both are accuracy defects in the one message whose entire job is to be actionable, so both matter more than their size. - "before any credentials were sent" is FALSE on the mid-session refresh and re-authentication paths this same notice serves, where credentials were legitimately sent earlier in the session. A user connected for an hour would read it as describing some other failure. Now scoped to the request actually refused: "without sending this request". - The recovery guidance named "Server Settings → Authorization". The UI renders that accordion as "OAuth Settings" and the field as "Token URL override" (ServerSettingsForm.tsx), so the message directed people to a section that does not exist, precisely when it was asking them to go reconfigure something. Both pinned by tests, including negative assertions on the old wording so it cannot drift back. The round's third finding was against test-server-http.ts's strictPort precondition, which no longer exists — that option was dropped in 6c1b532, after the review ran. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- clients/web/src/test/core/auth/oauthUx.test.ts | 15 ++++++++++++++- core/auth/oauthUx.ts | 18 +++++++++++++++--- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/clients/web/src/test/core/auth/oauthUx.test.ts b/clients/web/src/test/core/auth/oauthUx.test.ts index 1ef53e11..439fdeb0 100644 --- a/clients/web/src/test/core/auth/oauthUx.test.ts +++ b/clients/web/src/test/core/auth/oauthUx.test.ts @@ -467,7 +467,20 @@ describe("insecureTokenEndpoint copy", () => { expect(message).toContain(ENDPOINT); expect(message).toContain("HTTPS"); expect(message).toContain("127.0.0.1"); - expect(message).toContain("Token URL"); + expect(message).toContain("Token URL override"); + // The section name the UI actually renders. Sending someone to a settings + // section that does not exist is the worst error this message could make. + expect(message).toContain("OAuth Settings"); + expect(message).not.toContain("Server Settings → Authorization"); + }); + + it("does not claim no credentials were sent, which is false on a refresh", () => { + // The same notice serves mid-session refresh and re-auth, where credentials + // were legitimately sent earlier in the session. Scope the claim to the + // request actually refused. + const message = insecureTokenEndpointMessage({ tokenEndpoint: ENDPOINT }); + expect(message).toContain("without sending this request"); + expect(message).not.toContain("before any credentials were sent"); }); it("says a retry cannot help, which is the whole point of the message", () => { diff --git a/core/auth/oauthUx.ts b/core/auth/oauthUx.ts index 1a1dfc4d..7578187d 100644 --- a/core/auth/oauthUx.ts +++ b/core/auth/oauthUx.ts @@ -383,6 +383,17 @@ export function insecureTokenEndpointTitle(): string { * Does not echo the SDK's own message, which reads as a flat refusal and tells * the user nothing about which lever to reach for. * + * The opening says "without sending **this** request" rather than "before any + * credentials were sent". The absolute form was wrong: this same notice serves + * the mid-session refresh and re-authentication paths, where credentials were + * legitimately sent earlier in the session, and a user who had been connected + * for an hour would rightly read it as describing a different failure. + * + * The section name is the one the UI actually renders — **OAuth Settings**, + * with a **Token URL override** field — not "Authorization". Sending someone to + * a settings section that does not exist is the worst possible error in the one + * message whose entire job is telling them where to go. + * * The scheme half says "not HTTPS" rather than "plain HTTP": the SDK's check is * `protocol !== "https:"`, so anything else an authorization server advertises * — including a mistyped `ftp:` or `ws:` endpoint — lands here too, and naming @@ -403,12 +414,13 @@ export function insecureTokenEndpointMessage(options: { }): string { const target = options.serverName ? `"${options.serverName}"` : "this server"; return ( - `Authorization for ${target} was stopped before any credentials were sent: ` + + `Authorization for ${target} was stopped without sending this request: ` + `its token endpoint ${truncateUrlForDisplay(options.tokenEndpoint)} is ` + "not HTTPS, and its host is outside the MCP SDK's loopback exemption, " + "which covers only localhost, 127.0.0.1 and ::1. Re-authenticating cannot " + "change this. Serve the token endpoint over HTTPS, or move it to one of " + - "those three spellings — Server Settings → Authorization has a Token URL " + - "override if the authorization server advertises a different one." + "those three spellings — Server Settings → OAuth Settings has a " + + '"Token URL override" if the authorization server advertises a different ' + + "one." ); }