diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index f7636b8c..6f31c8d6 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -2377,6 +2377,9 @@ function App() { ); const onClientSettingsModalClose = useCallback(() => { + // Only fires when ClientSettingsModal allows the close (it blocks on an + // invalid issuer and reveals the error instead). The implicit "save" is the + // flush below; the persist gate drops anything still incomplete. flushClientSettingsDraft(); setClientSettingsOpen(false); }, [flushClientSettingsDraft]); diff --git a/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.test.tsx b/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.test.tsx index 4a680447..8fac7bd3 100644 --- a/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.test.tsx +++ b/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.test.tsx @@ -1,3 +1,4 @@ +import { useState } from "react"; import { describe, it, expect, vi } from "vitest"; import userEvent from "@testing-library/user-event"; import { renderWithMantine, screen } from "../../../test/renderWithMantine"; @@ -5,8 +6,27 @@ import { ClientSettingsForm } from "./ClientSettingsForm"; import { EMPTY_CLIENT_SETTINGS, ISSUER_URL_ERROR, + type ClientSettingsFormValues, } from "./clientSettingsValues"; +/** Stateful host so the controlled issuer input reflects edits, like the app. */ +function ClientSettingsFormHarness({ + initial, +}: { + initial: ClientSettingsFormValues; +}) { + const [settings, setSettings] = useState(initial); + return ( + + ); +} + describe("ClientSettingsForm EMA IdP session", () => { it("shows signed-in state and sign out", async () => { const user = userEvent.setup(); @@ -53,7 +73,8 @@ describe("ClientSettingsForm EMA IdP session", () => { ).not.toBeInTheDocument(); }); - it("shows an inline error for an invalid issuer URL", () => { + it("defers the invalid-issuer error until the field is blurred", async () => { + const user = userEvent.setup(); renderWithMantine( { />, ); + // Invalid value present, but the error stays hidden until the user leaves + // the field — no nagging while it may still be mid-typing. + expect(screen.queryByText(ISSUER_URL_ERROR)).not.toBeInTheDocument(); + + await user.click(screen.getByLabelText("Issuer")); + await user.tab(); // blur the issuer field expect(screen.getByText(ISSUER_URL_ERROR)).toBeInTheDocument(); }); + it("clears the issuer error live once a valid URL is entered after blur", async () => { + const user = userEvent.setup(); + renderWithMantine( + , + ); + + const issuer = screen.getByLabelText("Issuer"); + await user.click(issuer); + await user.tab(); // touched -> error shows for the invalid value + expect(screen.getByText(ISSUER_URL_ERROR)).toBeInTheDocument(); + + // Once touched, the error tracks the value live: fixing it clears the error + // without needing another blur. + await user.clear(issuer); + await user.type(issuer, "https://idp.test"); + expect(screen.queryByText(ISSUER_URL_ERROR)).not.toBeInTheDocument(); + }); + + it("reveals the issuer error without blur when revealIssuerError is set", () => { + renderWithMantine( + , + ); + + // Parent forced the error on (e.g. a close was attempted) — shown even + // though the field was never blurred. + expect(screen.getByText(ISSUER_URL_ERROR)).toBeInTheDocument(); + }); + + it("shows no error for a valid issuer even when revealIssuerError is set", () => { + renderWithMantine( + , + ); + + expect(screen.queryByText(ISSUER_URL_ERROR)).not.toBeInTheDocument(); + }); + it("shows no issuer error for a valid URL", () => { renderWithMantine( void; emaIdpLoginState?: EmaIdpLoginState; onEmaIdpLogout?: () => void; + /** + * Force the issuer error to show even before the field is blurred. The parent + * sets this when a save/close is attempted with an invalid issuer, so the user + * isn't left with a silently-dropped value and no explanation. + */ + revealIssuerError?: boolean; } const HintText = Text.withProps({ @@ -38,12 +45,26 @@ export function ClientSettingsForm({ onSettingsChange, emaIdpLoginState = "none", onEmaIdpLogout, + revealIssuerError = false, }: ClientSettingsFormProps) { function patch(partial: Partial) { onSettingsChange({ ...settings, ...partial }); } + // Defer the issuer error until the field has been blurred so it doesn't nag + // mid-typing (e.g. while "https:/…" is still incomplete). Once touched it + // updates live, so the error clears as soon as a valid URL is entered. The + // parent also forces it on via `revealIssuerError` when a close/save is + // attempted with an invalid value, so the value is never silently dropped + // without explanation. The persist gate (canPersistClientSettingsDraft) + // validates independently — an invalid issuer is never written regardless. + const [issuerTouched, setIssuerTouched] = useState(false); + const errors = validateClientSettings(settings); + const showIssuerError = + (issuerTouched || revealIssuerError) && errors.issuer + ? errors.issuer + : undefined; const showIdpSession = settings.emaEnabled && @@ -82,7 +103,8 @@ export function ClientSettingsForm({ description="Your enterprise IdP issuer URL." value={settings.issuer} onChange={(e) => patch({ issuer: e.currentTarget.value })} - error={errors.issuer} + onBlur={() => setIssuerTouched(true)} + error={showIssuerError} rightSectionPointerEvents="auto" rightSection={ settings.issuer ? ( diff --git a/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx b/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx index eb737f22..2f07c8a6 100644 --- a/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx +++ b/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx @@ -1,8 +1,12 @@ +import { useState } from "react"; import { describe, it, expect, vi } from "vitest"; import userEvent from "@testing-library/user-event"; import { renderWithMantine, screen } from "../../../test/renderWithMantine"; import { ClientSettingsModal } from "./ClientSettingsModal"; -import { EMPTY_CLIENT_SETTINGS } from "../ClientSettingsForm/clientSettingsValues"; +import { + EMPTY_CLIENT_SETTINGS, + ISSUER_URL_ERROR, +} from "../ClientSettingsForm/clientSettingsValues"; describe("ClientSettingsModal", () => { it("renders the title when opened", () => { @@ -120,6 +124,94 @@ describe("ClientSettingsModal", () => { ).toBeInTheDocument(); }); + it("blocks close and reveals the issuer error when the issuer is invalid", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + renderWithMantine( + , + ); + + // Error is hidden initially (form gates it on blur)... + expect(screen.queryByText(ISSUER_URL_ERROR)).not.toBeInTheDocument(); + // ...but a close attempt reveals it and does NOT close (no silent drop). + await user.click( + document.querySelector("button.mantine-CloseButton-root")!, + ); + expect(onClose).not.toHaveBeenCalled(); + expect(screen.getByText(ISSUER_URL_ERROR)).toBeInTheDocument(); + }); + + it("allows close once the invalid issuer is corrected", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + function Host() { + const [settings, setSettings] = useState({ + ...EMPTY_CLIENT_SETTINGS, + emaEnabled: true, + issuer: "not-a-url", + }); + return ( + + ); + } + renderWithMantine(); + + const close = () => + user.click(document.querySelector("button.mantine-CloseButton-root")!); + + await close(); // blocked, error revealed + expect(onClose).not.toHaveBeenCalled(); + + const issuer = screen.getByLabelText("Issuer"); + await user.clear(issuer); + await user.type(issuer, "https://idp.test"); + expect(screen.queryByText(ISSUER_URL_ERROR)).not.toBeInTheDocument(); + + await close(); // now valid -> closes + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it("allows close when an invalid issuer is cleared (empty is valid to leave)", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + function Host() { + const [settings, setSettings] = useState({ + ...EMPTY_CLIENT_SETTINGS, + emaEnabled: true, + issuer: "not-a-url", + }); + return ( + + ); + } + renderWithMantine(); + + await user.clear(screen.getByLabelText("Issuer")); + await user.click( + document.querySelector("button.mantine-CloseButton-root")!, + ); + expect(onClose).toHaveBeenCalledTimes(1); + }); + it("does not render the modal body when closed", () => { renderWithMantine( (["ema"]); + const [revealIssuerError, setRevealIssuerError] = useState(false); const allExpanded = expandedSections.length === ALL_SECTIONS.length; @@ -37,10 +41,24 @@ export function ClientSettingsModal({ setExpandedSections(allExpanded ? [] : ALL_SECTIONS); } + // Closing (X / Esc / overlay) is the implicit "save" for this auto-saving + // modal — the parent flushes the debounced persist on close. If the issuer is + // invalid the persist gate would silently drop it, so instead of closing we + // reveal the issuer error (overriding the form's on-blur gating). The user + // can fix the URL or clear the field — an empty issuer is valid to leave — + // and then close. Resets via the parent's open/close remount key. + function handleClose() { + if (validateClientSettings(settings).issuer) { + setRevealIssuerError(true); + return; + } + onClose(); + } + return ( Client Settings - + diff --git a/clients/web/src/test/core/client/config.test.ts b/clients/web/src/test/core/client/config.test.ts index 60aae6c6..e503734f 100644 --- a/clients/web/src/test/core/client/config.test.ts +++ b/clients/web/src/test/core/client/config.test.ts @@ -98,6 +98,25 @@ describe("client config", () => { expect(isAbsoluteHttpUrl("")).toBe(false); }); + it("isAbsoluteHttpUrl accepts dotted domains, IPv4, IPv6 and localhost", () => { + expect(isAbsoluteHttpUrl("https://idp.example.com/realms/main")).toBe(true); + expect(isAbsoluteHttpUrl("https://127.0.0.1:8443")).toBe(true); + expect(isAbsoluteHttpUrl("http://[::1]:3000")).toBe(true); + expect(isAbsoluteHttpUrl("https://你好.com")).toBe(true); + }); + + it("isAbsoluteHttpUrl rejects bare/degenerate hosts the URL parser allows", () => { + // "Looks like a URL" but is not actually one — these all parse and start + // with https:// yet have no real host. + expect(isAbsoluteHttpUrl("https://")).toBe(false); + expect(isAbsoluteHttpUrl("https://foo")).toBe(false); // single-label + expect(isAbsoluteHttpUrl("https://example")).toBe(false); // no TLD + expect(isAbsoluteHttpUrl("https://.")).toBe(false); + expect(isAbsoluteHttpUrl("https://..")).toBe(false); + expect(isAbsoluteHttpUrl("https://idp..example.com")).toBe(false); // empty label + expect(isAbsoluteHttpUrl("https:///path")).toBe(false); // empty host + }); + it("isAbsoluteHttpUrl rejects non-http(s) schemes", () => { expect(isAbsoluteHttpUrl("foo:bar")).toBe(false); expect(isAbsoluteHttpUrl("mailto:a@b.com")).toBe(false); diff --git a/core/client/config-parse.ts b/core/client/config-parse.ts index dfbf2e13..367f035b 100644 --- a/core/client/config-parse.ts +++ b/core/client/config-parse.ts @@ -6,26 +6,53 @@ 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. + * True when `value` (trimmed) is an absolute `http:`/`https:` URL with a real + * host. 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. Beyond "parses + http(s)", the host must actually look + * like a host — a dotted domain or IP, or `localhost` — so bare or degenerate + * values the URL parser still accepts (`https://foo`, `https://.`, `https://..`, + * `https://example`) are rejected too. */ 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:"; + let url: URL; + try { + url = new URL(value.trim()); + } catch { + return false; + } + if (url.protocol !== "http:" && url.protocol !== "https:") return false; + return isRealHost(url.hostname); } -const HttpUrlStringSchema = z.string().min(1).superRefine((val, ctx) => { - const trimmed = val.trim(); - if (!isAbsoluteHttpUrl(trimmed)) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - message: `Invalid URL: "${trimmed}" — must be an http(s) URL (e.g. https://idp.example.com)`, - }); - } -}); +/** + * A hostname the URL parser produced that we accept as an actual host: + * - `localhost` (common in dev), + * - an IPv6 literal (arrives bracketed, e.g. `[::1]`), + * - otherwise a dotted name / IPv4 with at least two non-empty labels — which + * rejects single-label (`foo`), bare-dot (`.`, `..`) and empty-label + * (`a..b`, `.a`, `a.`) hosts. An empty hostname splits to `[""]` and is + * rejected here too (http(s) URLs can't reach this with an empty host). + */ +function isRealHost(hostname: string): boolean { + if (hostname === "localhost") return true; + if (hostname.startsWith("[")) return hostname.endsWith("]"); + const labels = hostname.split("."); + return labels.length >= 2 && labels.every((label) => label !== ""); +} + +const HttpUrlStringSchema = z + .string() + .min(1) + .superRefine((val, ctx) => { + const trimmed = val.trim(); + if (!isAbsoluteHttpUrl(trimmed)) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `Invalid URL: "${trimmed}" — must be an http(s) URL (e.g. https://idp.example.com)`, + }); + } + }); const EnterpriseManagedAuthIdpConfigSchema = z.object({ issuer: HttpUrlStringSchema,