[cliffhall]

Closes #1544. Follow-up to #1533 (inline issuer validation).

Problem

The Enterprise-Managed Authorization Issuer field validated on every render, so the "Must be an http(s) URL" error nagged while the user was still mid-typing (after h, ht, http:/…). And because Client Settings auto-saves (debounced + flush on close) with no Save button, a user could type an invalid issuer and close the modal without ever blurring — the persist gate silently dropped it with no feedback. Separately, the URL check accepted bare/degenerate values that merely parse as https://….

Changes

1. Defer the issuer error to onBlur (ClientSettingsForm.tsx)
The issuer error is gated on an issuerTouched flag set onBlur; once touched it tracks the value live (clears as soon as it's valid). No more nagging mid-typing.

2. Don't silently drop an invalid issuer on close (ClientSettingsModal.tsx)
The modal intercepts its close (X / Esc / overlay): if the issuer is invalid it reveals the error (overriding the on-blur gating, via the form's new revealIssuerError prop) 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 (unit-tested) modal so App.tsx stays a thin flush+close that only runs when the close is allowed.

3. Require an actually-valid URL, not just an http(s) scheme (core/client/config-parse.ts)
isAbsoluteHttpUrl (shared by the Zod config schema and the form) previously accepted anything URL.canParse + http(s) allowed — including https://foo, https://example, https://., https://.., https://idp..example.com, https:///path. It now also requires the host to look like a host: localhost, an IPv6 literal, or a dotted domain/IPv4 with ≥2 non-empty labels.

The persist gate (canPersistClientSettingsDraft → validateClientSettings) is unchanged and still validates independently, so an invalid issuer is never written to the backend regardless of touched/blur state.

Behavior summary

• Typing an invalid issuer: no error (mid-edit); debounced persist stays silent.
• Blur the field → error shows, then tracks live.
• Attempt to close with an invalid issuer → modal stays open, error revealed (no silent drop).
• Escape hatch: fix the URL or clear the field → close proceeds.

Screencapture

url-validate-on-blur.mov

Tests / verification

• ClientSettingsForm tests: deferral on blur, live-clear, revealIssuerError prop.
• ClientSettingsModal tests: block-close-on-invalid, fix-then-close, clear-then-close.
• core/client tests: accepted (dotted / IPv4 / IPv6 / localhost / unicode) and newly-rejected (bare, single-label, no-TLD, dots, empty-label) hosts.
• Full web unit suite 2281 + Storybook 370 + coverage gate 3114 (changed files at 100%, no ERROR lines); format:check, lint, tsc -b all clean.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 57s —— View job

---

Review: PR #1545 — defer issuer validation, block silent-drop, require real host

• Read the changed files
• Analyze the diff against origin/v2/main
• Provide review feedback

Nice, well-scoped change. The three concerns from #1544 are each handled in the right layer (form gating, modal close-block, core URL check), the comments explain the why clearly, and the test coverage is thorough across all three. The modal remount key in App.tsx:2694 does correctly reset revealIssuerError on each open/close — I verified that. Below are a few things worth a look, ordered by importance.

---

1. (Worth deciding) isAbsoluteHttpUrl now rejects single-label hosts — including internal service names

isRealHost requires ≥2 non-empty labels, with localhost special-cased. That correctly kills https://foo / https://example, but it also rejects legitimate single-label internal hostnames that real IdPs use behind a firewall or in containers:

https://keycloak:8080   → hostname "keycloak" → rejected
https://auth            → rejected

Because isAbsoluteHttpUrl is shared with the Zod HttpUrlStringSchema (config parse) as well as the form, an existing client.json pointing at a single-label Docker/K8s service name (e.g. https://keycloak) would now fail to parse/validate, not just fail to save in the form. localhost is allowed but keycloak is not, which is an asymmetry an enterprise user behind an internal IdP could hit.

Is rejecting these intended? If single-label internal hosts should remain valid, one option is to accept any single-label host that isn't one of the known-degenerate cases, or to treat the degenerate set (., .., empty labels) as the only rejections. Flagging it as a product decision rather than asserting it's wrong. Fix this →

2. (Low) Trailing-dot FQDNs are rejected

A fully-qualified domain with a trailing dot — https://idp.example.com. — is technically valid (it's an absolute DNS name). It parses to hostname idp.example.com., which split(".") turns into ["idp","example","com",""]; the empty final label makes isRealHost return false. Probably an acceptable thing to reject, but if you want to allow it, trimming a single trailing dot before splitting would do it. Fix this →

3. (Observation) Close-block covers only invalid issuer, not incomplete EMA config

handleClose blocks only when validateClientSettings(settings).issuer is set. If EMA is enabled with a valid issuer but an empty clientId, canPersistClientSettingsDraft still returns false, so the draft is silently dropped on close — the exact "silent drop with no feedback" class of bug this PR fixes, just for a different field. This matches the stated scope of #1544 (issuer only), so it's not a blocker, but the asymmetry may be worth a follow-up issue so the other incomplete-EMA cases get the same treatment.

4. (Nit) revealIssuerError persists for the mount, re-enabling mid-typing nagging after a blocked close

Once a close is blocked, revealIssuerError stays true for the rest of the modal's mount. If the user then clears and re-types a new invalid value, the error shows live (mid-typing) rather than deferring to blur — partially defeating the onBlur deferral within that session. It's minor (the user already signaled intent to close, and the remount key resets it on reopen), but if you wanted the deferral to fully recover, you could reset revealIssuerError to false on the next onSettingsChange after a block.

---

Minor positives

• Switching from URL.canParse(...) + new URL(...) to a single try/new URL removes a redundant parse — good.
• error={showIssuerError} returning undefined (not "") when clear is the correct Mantine idiom.
• IPv6 handling is sound: the parser keeps the brackets in hostname ([::1]) and won't produce a bracketed host unless the literal already parsed, so startsWith("[") → endsWith("]") is safe.

No correctness blockers — items 1 and 3 are the ones I'd want a maintainer call on before merge.
• web/1544-issuer-validation-onblur

[cliffhall]

Thanks for the thorough review. Decisions on the four items:

1. Single-label hosts — keeping strict (no change). The reviewer's Docker/K8s reflex is for server-to-server resolution, but the Inspector's OAuth is browser-driven: the user's browser is redirected to the IdP's authorization endpoint, and a browser on the host can't resolve a container-internal name like https://keycloak. So a single-label internal host wouldn't complete the flow here regardless of validation. The genuine local cases all still pass the strict check — https://localhost, an IP (https://127.0.0.1:8443), or the RFC-6761 *.localhost convention (https://keycloak.localhost, dotted → accepted). Rejecting bare single-label hosts is exactly the "actually valid URL, not just https://…" behavior we want, so I'm keeping it. (Re the existing-client.json concern: a config pointing at https://keycloak couldn't drive a browser OAuth redirect anyway, so failing validation surfaces a latent problem rather than breaking a working setup.)

2. Trailing-dot FQDN — intentionally rejected. Consistent with (1)'s strictness; https://idp.example.com. is a rare form and rejecting it is acceptable. Not worth the trailing-dot special-case.

3. Incomplete EMA (valid issuer, empty clientId) silently dropped — filed as #1546. Good catch; it's the same silent-drop class for a different field and out of #1544's issuer-only scope, so I opened a follow-up to extend the close-block/reveal treatment to the other required EMA fields.

4. revealIssuerError persisting after a blocked close — leaving as-is (intentional). After a blocked close the user is in "fix it" mode, and showing the error live while they correct the value that blocked them is the helpful behavior — it clears the instant the URL becomes valid. Resetting on the next edit would trade that live-clear feedback for hiding the error mid-fix. The remount key resets it cleanly on reopen, so it doesn't leak across sessions.

Thanks also for verifying the remount-key reset and the IPv6 bracket handling.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 9s —— View job

---

Re-review: PR #1545 — defer issuer validation, block silent-drop, require real host

• Read the changed files
• Analyze the diff against origin/v2/main
• Provide review feedback

Re-reviewed against origin/v2/main after your responses. All three layers hold together and the prior items are resolved as discussed (1, 2, 4 kept by design with sound rationale; 3 split out to #1546). No blockers — this is good to merge.

Verifying the decisions land in the code

• Close-block escape hatch is correct. handleClose gates on validateClientSettings(settings).issuer (ClientSettingsModal.tsx:51), and validateClientSettings only sets issuer when emaEnabled && issuer.trim() !== "" && !isAbsoluteHttpUrl(...) (clientSettingsValues.ts:21-28). So an empty issuer → no error → close proceeds, exactly matching your "fix the URL or clear the field" escape hatch. Verified.
• isRealHost strictness matches your item-1/item-2 reasoning. localhost passes, 127.0.0.1 / [::1] pass, keycloak.localhost passes (dotted), bare/single-label/trailing-dot all reject. The browser-driven-OAuth argument for rejecting https://keycloak is the right call — a host the browser can't resolve can't complete the redirect anyway.
• App.tsx stays a thin flush+close and only runs when the modal allows the close. The new comment documents that contract clearly.

One small observation (non-blocking)

isRealHost accepts any 2-label dotted value, including all-numeric non-IPv4 forms like https://1.2 or https://999.999 (4 labels but out-of-range octets). The URL parser doesn't range-check IPv4, so these parse to a hostname and slip through as "≥2 non-empty labels." This is harmless — the same browser-redirect failure argument applies, and an issuer field isn't trying to be a full hostname validator — but worth knowing the check is structural (label shape), not semantic (resolvable/valid-IP). No change needed.

Confirmed clean

• Single try { new URL } (no redundant URL.canParse + re-parse).
• error={showIssuerError} yields undefined when clear — correct Mantine idiom.
• IPv6 bracket handling (startsWith("[") → endsWith("]")) is safe given the parser only emits brackets for valid IPv6 literals.
• Test coverage spans all three layers (form deferral/live-clear/revealIssuerError, modal block/fix/clear-then-close, core accept/reject host matrix).

I did not run the suite locally for this pass; relying on the reported validate + coverage results in the PR body. Nice work.
• web/1544-issuer-validation-onblur

[cliffhall]

Thanks for the re-review. On the all-numeric/out-of-range observation (https://1.2, https://999.999): agreed, leaving as-is. The check is deliberately structural (label shape) rather than a full hostname/IP validator — an issuer field shouldn't try to be one, and the same browser-redirect argument applies (a non-resolvable host can't complete the OAuth redirect regardless). Range-checking octets would be gold-plating for no real protection.

@claude review

[claude]

Claude finished @cliffhall's task in 46s —— View job

---

Re-review: PR #1545 — defer issuer validation, block silent-drop, require real host

• Read the changed files
• Analyze the diff against origin/v2/main
• Provide review feedback

Agreed on the structural-vs-semantic call — and there's a concrete reason it's the right boundary, not just an acceptable one: isAbsoluteHttpUrl is shared by the Zod config schema as well as the form. If you range-checked octets or otherwise tried to be a real hostname validator here, you'd risk rejecting a previously-valid client.json on parse, turning a cosmetic field check into a config-loading regression. Keeping it to label shape (isRealHost) means the check stays cheap, deterministic, and identical across both call sites, and the actual reachability verdict is deferred to the connect/redirect where it belongs. https://1.2 slipping through is harmless for exactly the reason you give. No change warranted.

Nothing new on this pass — still good to merge. Quick confirmation the code matches every decision in the thread:

• Structural host check. isRealHost (core/client/config-parse.ts:34-39): localhost ✓, bracketed IPv6 ✓, otherwise ≥2 non-empty labels — rejects bare/single-label/./../empty-label, accepts 1.2/999.999 as you've accepted. Matches items 1, 2, and the all-numeric observation.
• Single try { new URL } replaces the old URL.canParse + re-parse — one parse, no redundancy (config-parse.ts:18-26).
• Close-block escape hatch. handleClose gates on validateClientSettings(settings).issuer (ClientSettingsModal.tsx:51), which is only set for a non-empty, invalid issuer — so "fix the URL or clear the field → close proceeds" holds. Both the Modal.onClose and the CloseButton.onClick route through handleClose, so X / Esc / overlay / button are all covered.
• Form deferral. showIssuerError = (issuerTouched || revealIssuerError) && errors.issuer ? … : undefined (ClientSettingsForm.tsx) — defers to blur, forces on via the prop, yields undefined (not "") when clear. Item 4 left as-is by design.
• App.tsx stays a thin flush+close, only invoked when the modal allows the close; the new comment documents that contract.

The persist gate remains the independent backstop, so an invalid issuer is never written regardless of touched/blur/reveal state. Coverage spans all three layers. I relied on the PR-reported validate + coverage results rather than running the suite this pass. Ship it.
• web/1544-issuer-validation-onblur