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) <noreply@anthropic.com>
This commit is contained in:
parent
c96db0402e
commit
1dc0c744cf
7 changed files with 183 additions and 7 deletions
|
|
@ -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",
|
||||
});
|
||||
},
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
<ClientSettingsForm
|
||||
settings={{
|
||||
...EMPTY_CLIENT_SETTINGS,
|
||||
emaEnabled: true,
|
||||
issuer: "not-a-url",
|
||||
}}
|
||||
expandedSections={["ema"]}
|
||||
onExpandedSectionsChange={vi.fn()}
|
||||
onSettingsChange={vi.fn()}
|
||||
emaIdpLoginState="none"
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(screen.getByText(ISSUER_URL_ERROR)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("shows no issuer error for a valid URL", () => {
|
||||
renderWithMantine(
|
||||
<ClientSettingsForm
|
||||
settings={{
|
||||
...EMPTY_CLIENT_SETTINGS,
|
||||
emaEnabled: true,
|
||||
issuer: "https://idp.test",
|
||||
}}
|
||||
expandedSections={["ema"]}
|
||||
onExpandedSectionsChange={vi.fn()}
|
||||
onSettingsChange={vi.fn()}
|
||||
emaIdpLoginState="none"
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(screen.queryByText(ISSUER_URL_ERROR)).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("shows expired state with sign out", () => {
|
||||
renderWithMantine(
|
||||
<ClientSettingsForm
|
||||
|
|
|
|||
|
|
@ -10,7 +10,10 @@ import {
|
|||
} from "@mantine/core";
|
||||
import { ClearButton } from "../../elements/ClearButton/ClearButton";
|
||||
import type { EmaIdpLoginState } from "@inspector/core/auth/ema/idpSession.js";
|
||||
import type { ClientSettingsFormValues } from "./clientSettingsValues.js";
|
||||
import {
|
||||
validateClientSettings,
|
||||
type ClientSettingsFormValues,
|
||||
} from "./clientSettingsValues.js";
|
||||
|
||||
export type ClientSettingsSection = "ema";
|
||||
|
||||
|
|
@ -40,6 +43,8 @@ export function ClientSettingsForm({
|
|||
onSettingsChange({ ...settings, ...partial });
|
||||
}
|
||||
|
||||
const errors = validateClientSettings(settings);
|
||||
|
||||
const showIdpSession =
|
||||
settings.emaEnabled &&
|
||||
settings.issuer.trim() !== "" &&
|
||||
|
|
@ -77,6 +82,7 @@ export function ClientSettingsForm({
|
|||
description="Your enterprise IdP issuer URL."
|
||||
value={settings.issuer}
|
||||
onChange={(e) => patch({ issuer: e.currentTarget.value })}
|
||||
error={errors.issuer}
|
||||
rightSectionPointerEvents="auto"
|
||||
rightSection={
|
||||
settings.issuer ? (
|
||||
|
|
|
|||
|
|
@ -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({});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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");
|
||||
|
|
|
|||
|
|
@ -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)`,
|
||||
});
|
||||
}
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue