From 1dc0c744cf2dc8028bbb3e1abecd423a5c5701be Mon Sep 17 00:00:00 2001 From: Cliff Hall Date: Sat, 27 Jun 2026 16:08:22 -0400 Subject: [PATCH] fix(web): validate IdP issuer URL inline in Client Settings (#1533) (#1534) The Enterprise-Managed Authorization "Issuer" field accepted arbitrary non-URL text and only failed at save time, surfacing a raw Zod issue array in "Failed to save client settings" toasts. - Inline field-level validation: the Issuer input shows a friendly error ("Must be an http(s) URL, like https://idp.example.com"). - canPersistClientSettingsDraft gates on issuer validity (deferring to validateClientSettings) so an invalid value is never sent to the backend. - Extract isAbsoluteHttpUrl in core/client/config-parse so the Zod schema and the form share one http(s)-only URL rule. - Harden the fallback save-error toast via formatClientConfigLoadError. Closes #1533 Co-Authored-By: Claude Opus 4.8 (1M context) --- clients/web/src/App.tsx | 2 +- .../ClientSettingsForm.test.tsx | 41 ++++++++++++- .../ClientSettingsForm/ClientSettingsForm.tsx | 8 ++- .../clientSettingsValues.test.ts | 57 +++++++++++++++++++ .../clientSettingsValues.ts | 36 +++++++++++- .../web/src/test/core/client/config.test.ts | 30 +++++++++- core/client/config-parse.ts | 16 +++++- 7 files changed, 183 insertions(+), 7 deletions(-) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index a6fb788b..f7636b8c 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -2358,7 +2358,7 @@ function App() { onError: (err) => { notifications.show({ title: "Failed to save client settings", - message: err instanceof Error ? err.message : String(err), + message: formatClientConfigLoadError(err), color: "red", }); }, diff --git a/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.test.tsx b/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.test.tsx index 29180ab6..4a680447 100644 --- a/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.test.tsx +++ b/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.test.tsx @@ -2,7 +2,10 @@ import { describe, it, expect, vi } from "vitest"; import userEvent from "@testing-library/user-event"; import { renderWithMantine, screen } from "../../../test/renderWithMantine"; import { ClientSettingsForm } from "./ClientSettingsForm"; -import { EMPTY_CLIENT_SETTINGS } from "./clientSettingsValues"; +import { + EMPTY_CLIENT_SETTINGS, + ISSUER_URL_ERROR, +} from "./clientSettingsValues"; describe("ClientSettingsForm EMA IdP session", () => { it("shows signed-in state and sign out", async () => { @@ -50,6 +53,42 @@ describe("ClientSettingsForm EMA IdP session", () => { ).not.toBeInTheDocument(); }); + it("shows an inline error for an invalid issuer URL", () => { + renderWithMantine( + , + ); + + expect(screen.getByText(ISSUER_URL_ERROR)).toBeInTheDocument(); + }); + + it("shows no issuer error for a valid URL", () => { + renderWithMantine( + , + ); + + expect(screen.queryByText(ISSUER_URL_ERROR)).not.toBeInTheDocument(); + }); + it("shows expired state with sign out", () => { renderWithMantine( patch({ issuer: e.currentTarget.value })} + error={errors.issuer} rightSectionPointerEvents="auto" rightSection={ settings.issuer ? ( diff --git a/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.test.ts b/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.test.ts index 7cb6cbba..67fb48ed 100644 --- a/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.test.ts +++ b/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.test.ts @@ -4,6 +4,8 @@ import { clientConfigToFormValues, EMPTY_CLIENT_SETTINGS, formValuesToClientConfig, + ISSUER_URL_ERROR, + validateClientSettings, } from "./clientSettingsValues"; describe("clientSettingsValues", () => { @@ -128,4 +130,59 @@ describe("clientSettingsValues", () => { }), ).toBe(false); }); + + it("canPersistClientSettingsDraft blocks an invalid issuer URL", () => { + expect( + canPersistClientSettingsDraft({ + emaEnabled: true, + issuer: "not-a-url", + clientId: "cid", + clientSecret: "", + }), + ).toBe(false); + }); + + it("validateClientSettings flags an invalid issuer URL", () => { + expect( + validateClientSettings({ + emaEnabled: true, + issuer: "not-a-url", + clientId: "cid", + clientSecret: "", + }), + ).toEqual({ issuer: ISSUER_URL_ERROR }); + }); + + it("validateClientSettings passes a valid issuer URL", () => { + expect( + validateClientSettings({ + emaEnabled: true, + issuer: "https://idp.test", + clientId: "cid", + clientSecret: "", + }), + ).toEqual({}); + }); + + it("validateClientSettings ignores an empty issuer", () => { + expect( + validateClientSettings({ + emaEnabled: true, + issuer: "", + clientId: "cid", + clientSecret: "", + }), + ).toEqual({}); + }); + + it("validateClientSettings ignores issuer when EMA disabled", () => { + expect( + validateClientSettings({ + emaEnabled: false, + issuer: "not-a-url", + clientId: "cid", + clientSecret: "", + }), + ).toEqual({}); + }); }); diff --git a/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.ts b/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.ts index a72975d9..811f5c52 100644 --- a/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.ts +++ b/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.ts @@ -1,4 +1,33 @@ import type { ClientConfig } from "@inspector/core/client/types.js"; +import { isAbsoluteHttpUrl } from "@inspector/core/client/config-parse.js"; + +/** Field-level error message for an issuer that is not an http(s) URL. */ +export const ISSUER_URL_ERROR = + "Must be an http(s) URL, like https://idp.example.com"; + +/** Field-level validation errors for the client settings form. */ +export interface ClientSettingsErrors { + issuer?: string; +} + +/** + * Inline validation for the client settings form. Only flags fields that the + * user has actually filled in — empty required fields are gated by + * {@link canPersistClientSettingsDraft} rather than surfaced as errors. + */ +export function validateClientSettings( + values: ClientSettingsFormValues, +): ClientSettingsErrors { + const errors: ClientSettingsErrors = {}; + if ( + values.emaEnabled && + values.issuer.trim() !== "" && + !isAbsoluteHttpUrl(values.issuer) + ) { + errors.issuer = ISSUER_URL_ERROR; + } + return errors; +} /** Form shape for install-level client settings (client.json). */ export interface ClientSettingsFormValues { @@ -65,5 +94,10 @@ export function canPersistClientSettingsDraft( values: ClientSettingsFormValues, ): boolean { if (!values.emaEnabled) return true; - return values.issuer.trim() !== "" && values.clientId.trim() !== ""; + if (values.issuer.trim() === "" || values.clientId.trim() === "") + return false; + // Defer to validateClientSettings so the persist gate and the inline field + // errors can never drift: an invalid issuer is never sent to the backend, and + // the field error guides the user instead of a raw validation failure toast. + return Object.keys(validateClientSettings(values)).length === 0; } diff --git a/clients/web/src/test/core/client/config.test.ts b/clients/web/src/test/core/client/config.test.ts index 5e5da2fd..60aae6c6 100644 --- a/clients/web/src/test/core/client/config.test.ts +++ b/clients/web/src/test/core/client/config.test.ts @@ -17,7 +17,10 @@ import { parseClientConfig, saveClientConfig, } from "@inspector/core/client/config.js"; -import { formatClientConfigLoadError } from "@inspector/core/client/config-parse.js"; +import { + formatClientConfigLoadError, + isAbsoluteHttpUrl, +} from "@inspector/core/client/config-parse.js"; import { getActiveEnterpriseManagedAuthIdp, isEnterpriseManagedAuthEnabled, @@ -87,6 +90,31 @@ describe("client config", () => { ).toThrow(); }); + it("isAbsoluteHttpUrl accepts http(s) URLs and trims, rejects others", () => { + expect(isAbsoluteHttpUrl("https://idp.example.com")).toBe(true); + expect(isAbsoluteHttpUrl("http://localhost:6274")).toBe(true); + expect(isAbsoluteHttpUrl(" https://idp.example.com ")).toBe(true); + expect(isAbsoluteHttpUrl("not-a-url")).toBe(false); + expect(isAbsoluteHttpUrl("")).toBe(false); + }); + + it("isAbsoluteHttpUrl rejects non-http(s) schemes", () => { + expect(isAbsoluteHttpUrl("foo:bar")).toBe(false); + expect(isAbsoluteHttpUrl("mailto:a@b.com")).toBe(false); + expect(isAbsoluteHttpUrl("ftp://idp.example.com")).toBe(false); + expect(isAbsoluteHttpUrl("javascript:void(0)")).toBe(false); + }); + + it("parseClientConfig rejects a non-http(s) issuer scheme", () => { + expect(() => + parseClientConfig({ + enterpriseManagedAuth: { + idp: { issuer: "mailto:idp@example.com", clientId: "c" }, + }, + }), + ).toThrow(/Invalid URL/); + }); + it("loadClientConfig returns {} when file is absent", async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "client-config-")); const filePath = path.join(tmpDir, "client.json"); diff --git a/core/client/config-parse.ts b/core/client/config-parse.ts index 8ccfae11..dfbf2e13 100644 --- a/core/client/config-parse.ts +++ b/core/client/config-parse.ts @@ -5,12 +5,24 @@ import { z } from "zod"; import type { ClientConfig } from "./types.js"; +/** + * True when `value` (trimmed) is an absolute `http:`/`https:` URL. An OAuth IdP + * issuer is always http(s), so other parseable schemes (`mailto:`, `foo:bar`, + * `javascript:`) are rejected rather than deferred to a later connect failure. + */ +export function isAbsoluteHttpUrl(value: string): boolean { + const trimmed = value.trim(); + if (!URL.canParse(trimmed)) return false; + const { protocol } = new URL(trimmed); + return protocol === "https:" || protocol === "http:"; +} + const HttpUrlStringSchema = z.string().min(1).superRefine((val, ctx) => { const trimmed = val.trim(); - if (!URL.canParse(trimmed)) { + if (!isAbsoluteHttpUrl(trimmed)) { ctx.addIssue({ code: z.ZodIssueCode.custom, - message: `Invalid URL: "${trimmed}" — must be an absolute URL (e.g. https://idp.example.com)`, + message: `Invalid URL: "${trimmed}" — must be an http(s) URL (e.g. https://idp.example.com)`, }); } });