fix(web): defer issuer validation to onBlur, block silent-drop on close, require a real host URL (closes #1544) (#1545)
* fix(web): defer IdP issuer URL error to onBlur in Client Settings (#1544) The Enterprise-Managed Authorization Issuer field validated its URL inline on every render, so the 'Must be an http(s) URL' error nagged while the user was still mid-typing (e.g. before 'https:/…' is complete). Gate the issuer error display on an issuerTouched flag set onBlur; once touched it tracks the value live, so the error clears as soon as a valid URL is entered. The persist gate (canPersistClientSettingsDraft → validateClientSettings) is unchanged and still validates independently, so an invalid issuer is never written to the backend regardless of touched state. Closes #1544 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(web): reveal issuer error and block close on invalid issuer (#1544) Extend the on-blur deferral so an invalid issuer can't be silently dropped on modal close (the auto-save flush would otherwise discard it with no feedback). ClientSettingsModal now intercepts its close (X / Esc / overlay): if the issuer is invalid it reveals the field error (via the form's revealIssuerError prop, overriding on-blur gating) and keeps the modal open instead of closing. The user can fix the URL or clear the field — an empty issuer is valid to leave. The block lives in the modal (unit-tested) rather than App, so App.tsx stays a thin flush+close that only runs when the close is allowed; the persist gate is unchanged. Reveal state resets via the modal's open/close remount key. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(core): require a real host in isAbsoluteHttpUrl, not just an http(s) scheme (#1544) The issuer validation used URL.canParse + an http(s) protocol check, which the WHATWG parser accepts for bare/degenerate values that aren't actually usable URLs — e.g. https://foo, https://example, https://. , https://.. all passed. Tighten the shared rule (used by both the Zod config schema and the form's inline validation): after parsing and the http(s) scheme check, require the host to actually look like a host — localhost, an IPv6 literal, or a dotted domain/IPv4 with at least two non-empty labels. Single-label, bare-dot, and empty-label hosts are now rejected. Add core tests for the accepted (dotted/IPv4/IPv6/localhost) and newly-rejected cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall
GitHub
parent
f43fafc08b
commit
4a927d3978
7 changed files with 295 additions and 22 deletions
|
|
@ -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]);
|
||||
|
|
|
|||
|
|
@ -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 (
|
||||
<ClientSettingsForm
|
||||
settings={settings}
|
||||
expandedSections={["ema"]}
|
||||
onExpandedSectionsChange={vi.fn()}
|
||||
onSettingsChange={setSettings}
|
||||
emaIdpLoginState="none"
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
||||
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(
|
||||
<ClientSettingsForm
|
||||
settings={{
|
||||
|
|
@ -68,9 +89,79 @@ describe("ClientSettingsForm EMA IdP session", () => {
|
|||
/>,
|
||||
);
|
||||
|
||||
// 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(
|
||||
<ClientSettingsFormHarness
|
||||
initial={{
|
||||
...EMPTY_CLIENT_SETTINGS,
|
||||
emaEnabled: true,
|
||||
issuer: "not-a-url",
|
||||
}}
|
||||
/>,
|
||||
);
|
||||
|
||||
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(
|
||||
<ClientSettingsForm
|
||||
settings={{
|
||||
...EMPTY_CLIENT_SETTINGS,
|
||||
emaEnabled: true,
|
||||
issuer: "not-a-url",
|
||||
}}
|
||||
expandedSections={["ema"]}
|
||||
onExpandedSectionsChange={vi.fn()}
|
||||
onSettingsChange={vi.fn()}
|
||||
emaIdpLoginState="none"
|
||||
revealIssuerError
|
||||
/>,
|
||||
);
|
||||
|
||||
// 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(
|
||||
<ClientSettingsForm
|
||||
settings={{
|
||||
...EMPTY_CLIENT_SETTINGS,
|
||||
emaEnabled: true,
|
||||
issuer: "https://idp.test",
|
||||
}}
|
||||
expandedSections={["ema"]}
|
||||
onExpandedSectionsChange={vi.fn()}
|
||||
onSettingsChange={vi.fn()}
|
||||
emaIdpLoginState="none"
|
||||
revealIssuerError
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(screen.queryByText(ISSUER_URL_ERROR)).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("shows no issuer error for a valid URL", () => {
|
||||
renderWithMantine(
|
||||
<ClientSettingsForm
|
||||
|
|
|
|||
|
|
@ -1,3 +1,4 @@
|
|||
import { useState } from "react";
|
||||
import {
|
||||
Accordion,
|
||||
Badge,
|
||||
|
|
@ -24,6 +25,12 @@ export interface ClientSettingsFormProps {
|
|||
onSettingsChange: (settings: ClientSettingsFormValues) => 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<ClientSettingsFormValues>) {
|
||||
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 ? (
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
<ClientSettingsModal
|
||||
opened
|
||||
settings={{
|
||||
...EMPTY_CLIENT_SETTINGS,
|
||||
emaEnabled: true,
|
||||
issuer: "not-a-url",
|
||||
}}
|
||||
onClose={onClose}
|
||||
onSettingsChange={vi.fn()}
|
||||
/>,
|
||||
);
|
||||
|
||||
// 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 (
|
||||
<ClientSettingsModal
|
||||
opened
|
||||
settings={settings}
|
||||
onClose={onClose}
|
||||
onSettingsChange={setSettings}
|
||||
/>
|
||||
);
|
||||
}
|
||||
renderWithMantine(<Host />);
|
||||
|
||||
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 (
|
||||
<ClientSettingsModal
|
||||
opened
|
||||
settings={settings}
|
||||
onClose={onClose}
|
||||
onSettingsChange={setSettings}
|
||||
/>
|
||||
);
|
||||
}
|
||||
renderWithMantine(<Host />);
|
||||
|
||||
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(
|
||||
<ClientSettingsModal
|
||||
|
|
|
|||
|
|
@ -6,7 +6,10 @@ import {
|
|||
type ClientSettingsSection,
|
||||
} from "../ClientSettingsForm/ClientSettingsForm";
|
||||
import type { EmaIdpLoginState } from "@inspector/core/auth/ema/idpSession.js";
|
||||
import type { ClientSettingsFormValues } from "../ClientSettingsForm/clientSettingsValues.js";
|
||||
import {
|
||||
validateClientSettings,
|
||||
type ClientSettingsFormValues,
|
||||
} from "../ClientSettingsForm/clientSettingsValues.js";
|
||||
|
||||
const ALL_SECTIONS: ClientSettingsSection[] = ["ema"];
|
||||
|
||||
|
|
@ -30,6 +33,7 @@ export function ClientSettingsModal({
|
|||
const [expandedSections, setExpandedSections] = useState<
|
||||
ClientSettingsSection[]
|
||||
>(["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 (
|
||||
<Modal
|
||||
opened={opened}
|
||||
onClose={onClose}
|
||||
onClose={handleClose}
|
||||
withCloseButton={false}
|
||||
size="lg"
|
||||
centered
|
||||
|
|
@ -55,7 +73,7 @@ export function ClientSettingsModal({
|
|||
<Title order={4} ta="center" flex={1}>
|
||||
Client Settings
|
||||
</Title>
|
||||
<CloseButton onClick={onClose} />
|
||||
<CloseButton onClick={handleClose} />
|
||||
</Group>
|
||||
<ClientSettingsForm
|
||||
settings={settings}
|
||||
|
|
@ -64,6 +82,7 @@ export function ClientSettingsModal({
|
|||
onSettingsChange={onSettingsChange}
|
||||
emaIdpLoginState={emaIdpLoginState}
|
||||
onEmaIdpLogout={onEmaIdpLogout}
|
||||
revealIssuerError={revealIssuerError}
|
||||
/>
|
||||
</Stack>
|
||||
</Modal>
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Reference in a new issue