React UI

[jbouder]

Reference Issues or PRs

Fixes #92

What does this implement/fix?

Put a x in the boxes that apply

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds a feature)
• Breaking change (fix or feature that would cause existing features not to work as expected)
• Documentation Update
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Build related changes
• Other (please describe):

Testing

• Did you test the pull request locally?
• Did you add new tests?

Documentation

Access-centered content checklist

Text styling

• The content is written with plain language (where relevant).
• If there are headers, they use the proper header tags (with only one level-one header: H1 or # in markdown).
• All links describe where they link to (for example, check the Nebari website).
• This content adheres to the Nebari style guides.

Non-text content

• All content is represented as text (for example, images need alt text, and videos need captions or descriptive transcripts).
• If there are emojis, there are not more than three in a row.
• Don't use flashing GIFs or videos.
• If the content were to be read as plain text, it still makes sense, and no information is missing.

Any other comments?

Screenshots (new)

[github-actions]

📄 Docs preview for react-ui:
https://react-ui.llm-serving-pack.pages.dev

[dcmcand]

Review scope

This pass covers the Go (key-manager), Helm chart, docs, and CI changes. I skipped the React/TS app this round.

Overall this is careful, well-structured work. The docs were updated alongside the code, the token validator has good security instincts (RSA-only signing method, fail-closed middleware), and the removal of the embedded internal/ui package is clean with no dangling references. Everything builds and passes locally: go build ./..., go vet ./..., key-manager go test ./..., helm lint, and helm template are all green.

I'm requesting changes for three defects plus one design question that I think need a decision before this is exposed publicly. Specifics are in the inline comments; summary below.

Blockers

1. JWKS unknown-kid refetch is a DoS amplification vector (jwt_validator.go, inline). An unauthenticated caller can force an outbound Keycloak call per request with a forged token.
2. Data race on lastFetch (jwt_validator.go:206 vs :308, inline). go test -race will flag it.
3. NebariApp can target a Service that isn't rendered (key-manager-nebariapp.yaml:16, inline). frontend.enabled: false is a documented config and breaks the gateway target.

Question (needs a maintainer decision)

• aud/azp are intentionally unvalidated (jwt_validator.go, inline). Fine if the realm is single-purpose; a real gap if it hosts other OIDC clients. That call depends on the realm's client topology.

Non-blocking cleanups

• docs/src/content/docs/ui-development.md:99-102 tells readers to rollout restart deployment/llm-frontend, but the dev Makefile builds no frontend image and creates no such deployment. Drop the paragraph or add a real dev target.
• Both nginx configs (the baked frontend/nginx.default.conf and the Helm-rendered frontend-configmap.yaml) lack Cache-Control: no-store on /config.json and lack X-Frame-Options/frame-ancestors plus X-Content-Type-Options. The framing header matters here because the UI exposes one-click "revoke key." Fix both, since they are hand-duplicated.
• frontend/public/config.json is committed and not gitignored, contradicting frontend-rewrite-plan.md:155. Harmless while the ConfigMap overrides it, a footgun if the raw image runs without the mount.
• provider: keycloak is hardcoded in the NebariApp while architecture.md:371 advertises keycloak or generic-oidc. Consistent with the Keycloak-only limitation in #61; worth a one-line comment or a values passthrough.
• AuthConfig.Validator is a concrete *JWTValidator rather than an interface, which is why middleware_test.go builds it from unexported fields. A small TokenValidator interface would let tests use a fake.
• No jwt_validator_test.go: the retry/backoff state machine, fetchPublicKeys error handling, key parsing, and the unknown-kid path have no direct coverage, even though the knobs were made overridable specifically for testing.
• The retry/poll knobs are package-level mutable vars and would race under t.Parallel(); instance fields with constructor defaults would be safer.
• frontend-rewrite-plan.md (234 lines) is a scratch planning doc at the repo root that already contradicts the shipped design. Consider dropping it now that architecture.md covers the as-built design.

Nice work

• Explicit *jwt.SigningMethodRSA assertion blocks alg:none and HMAC-confusion attacks.
• TestAuthMiddlewareMisconfigured verifies the middleware fails closed rather than admitting requests when misconfigured.
• The ErrNotReady to 503 + Retry-After distinction from a bad-token 401 is a thoughtful operational touch.
• go mod tidy is a zero-diff, so the removed indirect deps are genuinely unused.
• CI actions are SHA-pinned and the new frontend/key-manager build contexts are correct.

[dcmcand]

Re-review: Go, backend, and infra

Scope matches the last round: key-manager Go, Helm chart, docs, CI, and dev tooling. The React app is still out of scope for me.

I verified each claimed fix from 23d541b by reading the code and running the commands, not by trusting the thread. Results:

Prior finding	Status
JWKS unknown-kid DoS amplification	Fixed for the stated attack. The cooldown gate introduces a new availability bug, see the inline blocker.
Data race on lastFetch	Fixed. Read at jwt_validator.go:242-244 now snapshots under keysMu.RLock(). go test -race -count=1 ./... is clean on my machine.
NebariApp targeting an unrendered Service	Fixed. I ran helm template on all three enable combinations: the bad combo fails with a clear message, the happy path renders, disabled renders clean. The required guards on hostname and frontend.keycloak.url are a nice touch.
aud/azp unvalidated	Fixed. The pin is ordered after iss, a missing or non-string azp fails closed, and TestAzpValidation covers all six set/unset/mismatch cases. azp is the right claim for a public PKCE client.

Kudos

• The azp fix is exactly what I hoped for: correct claim choice, fail-closed on absent claims, and a table test that covers the matrix.
• TestUnknownKIDRefreshIsRateLimited proving one fetch for a 50-goroutine burst is a good test. It has one structural blind spot, covered inline.
• The chart derivation for the pinned client ID reuses frontend.keycloak.clientId with the same default convention the ConfigMap uses, so the pin and the SPA always agree out of the box.
• Handler authorization held up under a fresh look: deleteKey verifies ownership before revoking, createKey checks model access, getKeys is scoped to the caller.

Blockers

1 new, inline on jwt_validator.go. Short version: the cooldown that fixes the DoS makes concurrent requests with a legitimately rotated key fail with 401.

Questions

Three, inline: whether the hourly JWKS refresh belongs on the request path, the pinClientAudience + frontend.enabled=false combination, and two config values that must be overridden in pairs.

Non-blocking, carried over from last round

23d541b addressed the Go items and the Helm blocker but none of the cleanups, so re-listing the ones I still care about:

• Both nginx configs (frontend/nginx.default.conf and frontend-configmap.yaml) still lack X-Frame-Options/frame-ancestors, X-Content-Type-Options, and Cache-Control: no-store on /config.json. There is no dedicated location = /config.json block at all. The framing headers matter because the UI has one-click revoke.
• frontend/public/config.json is still tracked, and frontend-rewrite-plan.md:155 still claims it is gitignored. The Dockerfile bakes the localhost placeholder into the image.
• docs/src/content/docs/ui-development.md:100-101 still tells readers to run make build-images (builds no frontend image) and restart deployment/llm-frontend (does not exist).
• frontend-rewrite-plan.md is still at the repo root and now contradicts both the shipped design and itself.

Non-blocking, new this round

• No workload in the chart has liveness or readiness probes, and the frontend Deployment has no ConfigMap checksum annotation, so a config-only helm upgrade never rolls the pod (which is why the docs reach for a manual rollout restart). Note the key-manager is now API-only with no health endpoint, so it needs a /healthz before it can get probes.
• frontend.image.pullPolicy: Always against a pinned tag; IfNotPresent is the usual choice.
• provider: keycloak in the NebariApp is fine given the documented Keycloak-only limitation, but a comment pointing at #61 would stop the next reviewer from flagging it.
• Go minors: ValidateToken takes no context.Context so a cancelled request can't cancel the JWKS fetch it may trigger; fetchPublicKeys decodes the response body with no size cap; main.go never calls validator.Stop() in shutdown; SetExpectedClientID lacks the empty-string guard SetIssuerURL has; AuthConfig.Validator is still the concrete type; the retry/poll knobs are still package-level vars.
• 401 bodies echo err.Error(), which includes the expected issuer and azp values. Everything in there is already public via config.json, so this is a nit.
• test-auth.sh:115 has an em dash in a FAIL message.

Everything builds and passes here: go build, go vet, go test -race, helm lint, helm template.

[jbouder]

Round-2 review addressed — be84cf9

Thanks for the thorough re-review. The new blocker and all four questions are handled in the inline threads. Rolling up the non-blocking items:

Fixed this commit:

• nginx security headers, both configs — X-Frame-Options: DENY, CSP frame-ancestors 'none', X-Content-Type-Options: nosniff, plus a dedicated location = /config.json with Cache-Control: no-store. Applied to frontend/nginx.default.conf and the Helm-rendered frontend-configmap.yaml (comment notes the add_header non-inheritance gotcha, which is why the config.json block repeats them).
• fetchPublicKeys body cap — io.LimitReader at 1 MiB.
• validator.Stop() now called in the shutdown goroutine (and Stop() is now meaningful, since the refresh goroutine stays alive until it).
• ui-development.md — rewrote the shipping paragraph; the make build-images / rollout restart deployment/llm-frontend commands were bogus (the dev stack builds no frontend image and has no such deployment). Dev is the Vite server; shipping is commit → CI.
• frontend-rewrite-plan.md — deleted (stale, contradicted the shipped design and itself; also clears the config.json-gitignore contradiction it claimed).
• provider: keycloak comment pointing at Internal SecurityPolicy points at non-existent Keycloak JWKS path /.well-known/jwks.json #61.
• test-auth.sh em dash → hyphen.

Follow-up commit (in progress on this branch):

• Derive spaClientId from frontend.keycloak.clientId (SPA-client-ID thread).
• TokenValidator interface for AuthConfig.Validator + context.Context on ValidateToken so a cancelled request can cancel the JWKS fetch.
• Liveness/readiness probes + a /healthz on the (now API-only) key-manager + a ConfigMap checksum annotation on the frontend Deployment so a config-only helm upgrade rolls the pod.
• Retry/poll knobs → instance fields (safe under t.Parallel()).

Deliberately not changed: frontend.image.pullPolicy: Always — kept as-is for now since during alpha the tag may be re-pushed; happy to flip to IfNotPresent if tags are immutable. And SetExpectedClientID has no empty-string guard because empty is the documented "disabled" state for the azp pin (unlike SetIssuerURL, where empty would clobber a real default).

Verified: go build, go vet, go test -race ./..., helm lint, helm template (all three enable combinations) green.

[jbouder]

Follow-up suggestions addressed — 2c63aac

Picked up the remaining non-blocking suggestions in a second commit:

• TokenValidator interface — AuthConfig.Validator is now an interface, so the middleware can be tested with a fake (added TestAuthMiddlewareWithFakeValidator) instead of building a *JWTValidator from unexported fields.
• context.Context on ValidateToken — the request context now cancels the caller's wait on a JWKS fetch (singleflight.DoChan + select), while the fetch itself runs on a validator-lifetime context. This deliberately avoids the trap where a single cancelled request would abort a fetch that other in-flight requests are sharing. Stop() cancels that lifetime context, so shutdown aborts an in-flight fetch too.
• JWKS fetch context wired to the validator lifetime (bounded by the existing 10s timeout).
• Retry/poll/refresh knobs → instance fields with constructor defaults (safe under t.Parallel()), plus direct coverage of the retry state machine that previously had none: TestInitialFetchRetriesThenSucceeds and TestInitialFetchFallsBackToSlowPoll.
• /healthz + probes + checksum — added an unauthenticated GET /healthz to the now-API-only key-manager (outside /api/, so the auth middleware doesn't gate it), liveness/readiness probes on both Deployments, and a checksum/config annotation on the frontend pod template so a config-only helm upgrade rolls the pod. The probes intentionally don't gate on JWKS readiness (that would CrashLoop the pod while Keycloak is briefly unreachable); per-request not-ready is still surfaced as 503. Verified the checksum changes when config.json content changes.

Still deliberately unchanged: frontend.image.pullPolicy: Always (kept for the alpha re-push workflow — say the word to flip it to IfNotPresent).

Verified: gofmt, go build, go vet, go test -race ./..., helm lint, helm template (all enable combinations) green.

[dcmcand]

Re-review (round 3)

Reviewed at HEAD 2c63aac4 in an isolated worktree. This pass verified the round-2 fixes across the Go key-manager, Helm chart, and docs, and it is the first review of the React/TS frontend (the prior two rounds skipped it).

Round-2 fixes: all resolved

Prior finding	Status
Blocker: cooldown gate 401s legit tokens during key rotation	Fixed. The cooldown check moved inside the singleflight callback and keyFunc re-checks the cache regardless of refresh outcome. TestRotatedKIDResolvesForConcurrentBurst starts with only the old key cached, signs with an uncached rotated kid, fires 50 concurrent validations, and asserts all 50 validate with exactly one fetch. That test fails under the old code, so it is a real regression guard. go test -race is clean.
Q: hourly JWKS refresh on the request path	Fixed. Moved to a background refreshLoop ticker; ValidateToken does no synchronous refresh; fail-open on stale keys; dead lastFetch field removed.
Q: pinClientAudience + frontend.enabled=false locks out callers	Documented in values.yaml (see Q5 below on guarding it).
Q: groups-claim name could drift	Fixed. The mapper claim.name is templated from .Values.auth.oidc.groupsClaim; helm template --set auth.oidc.groupsClaim=custom_groups renders both the mapper and LLM_OIDC_GROUPS_CLAIM as custom_groups.
Q: SPA client ID in two values needing joint override	Fixed. spaClientId defaults to frontend.keycloak.clientId; helm template --set frontend.keycloak.clientId=my-spa renders config.json, the NebariApp spaClient.clientId, and the azp env identically, and the both-empty path falls back to the operator convention consistently.
New non-blockers: context / size-cap / Stop / interface / knobs	All addressed. ValidateToken takes context.Context; fetchPublicKeys caps the body at 1 MiB; main.go calls validator.Stop() on shutdown; AuthConfig.Validator is now the TokenValidator interface; the retry/poll knobs are instance fields.
Carried-over cleanups	All addressed. Security headers plus a location = /config.json no-store block are in both nginx configs; ui-development.md is corrected; frontend-rewrite-plan.md is deleted; the test-auth.sh em dash is gone.

Verification run: go build, go vet, go test -race -count=1 (all green), go mod tidy zero-diff, helm lint clean, helm template across all enable/override combinations, and confirmed the removed embedded UI leaves no dangling references.

Questions (need a maintainer answer)

1. KeyCreatedDialog.tsx:42 offers a "Download .txt" of the plaintext key, which can sync to cloud backups. This is common for API-key UIs (AWS, Stripe), so likely intentional, but worth a conscious call.
2. keycloak.ts:69 sets checkLoginIframe:false, so the SPA will not detect a server-side single-logout until the next token refresh. The documented compensating control is a short realm access-token lifespan. Is the realm tuned for that? (That setting lives outside this diff.)

Suggestions (non-blocking, worth addressing)

• The OIDC issuer is expressed twice with nothing cross-validating them. The operator/model endpoint reads full-form auth.oidc.issuerURL (.../realms/nebari); the SPA and key-manager read split-form frontend.keycloak.url + frontend.keycloak.realm and reconstruct iss as <url>/realms/<realm>. After round 2 single-sourced the groups claim and the client ID, this is the last split source: a typo or an http/https drift silently makes the two auth paths trust different issuers. Deriving one from the other in _helpers.tpl, or a fail/NOTES assertion that they describe the same realm, would close it.
• api.ts:32-36: the 401-retry does not force a refresh. It calls getToken() again, which calls updateToken(30), a no-op when the token still has more than 30s left, so it re-sends the identical bearer and reproduces the same 401. The comment says "force a refresh." Either updateToken(-1) before the retry, or escalate a second consecutive 401 to login(). The error does surface to the UI, so this is not a hidden failure.
• main.tsx:16: the bootstrap await initKeycloak() has no try/catch, so a malformed /config.json (a realistic operator misconfig) yields a blank white page. Render a fallback message instead.
• The nginx server{} block is duplicated byte-for-byte between nginx.default.conf and frontend-configmap.yaml. They are in sync today, but they are two sources of truth to keep in lockstep; consider single-sourcing or a parity test.
• frontend.title is dead plumbing: wired through values, the ConfigMap, and the TS type, but nothing reads it or sets document.title. Wire it up in main.tsx or remove it.
• The pinClientAudience footgun is documented but not guarded. A {{- fail }} on pinClientAudience && not frontend.enabled would mirror the round-1 NebariApp guard. Fine as-is since it is off by default.
• useUser() uses useMemo(..., []), a one-time snapshot rather than reactive state, so name/email can go stale after a token refresh. Low impact.
• Dead leftovers from the embedded-Go UI: the vite.config.ts /logout proxy and the exported-but-unused getAppConfig().

Nits (cosmetic)

• architecture.md:371 still annotates provider: keycloak # or generic-oidc while everything shipped is Keycloak-only (generic-oidc is #61). Drop the annotation.
• config.json's placeholder clientId: "nebari-frontend-spa" is a fourth distinct client-ID string that matches none of the three runtime sources. Align it or use an obvious sentinel.
• ui-development.md says Node 20+ but the Dockerfile and .node-version pin 22.
• keycloak.yaml:12 comment says login testuser/testuser but the realm defines dev/password.
• The dev Keycloak user dev is in /llm-users while dev models are scoped to group llm, so the real-auth path shows an empty model list. Align the realm group or the model scope.
• imagePullPolicy: Always on a pinned tag (prefer IfNotPresent).
• A few em dashes crept back into comments and markdown after round 2 cleaned them out (values.yaml:134,184, key-manager-nebariapp.yaml:63, key-manager-deployment.yaml:82, frontend-configmap.yaml:69,80, README.md:291).

Note (accepted limitation, no change requested)

• Go: the concurrent-burst rotation case is fully fixed. One narrow case remains: a real rotation landing within 30s after a prior unknown-kid fetch can 401 the first requests until the cooldown clears (self-healing, at most 30s). An attacker could keep the cooldown warm to extend that first-request window to ~30s after each rotation. Bounded and low severity; flagging for awareness, and a one-line doc note would be enough if you want to record it.

Test coverage gaps (frontend)

• auth/keycloak.ts and auth/user.ts have no direct tests, and they are the most security-relevant files. getToken()'s refresh-failure, SessionExpiredError, and missing-token branches are never exercised; existing tests always go through the fakeSession() shim.
• KeyCreatedDialog (secret display, copy, download) and RevokeKeyDialog (the one-click destructive action the clickjacking headers exist to protect) have no test files.
• api.ts's two-consecutive-401 path is untested, as are Topbar sign-out and the config.ts error path.

What is well done

• Go: the rotation fix uses the right claim, fails closed, and ships a genuine concurrent regression test; the refresh is off the request path; and /healthz deliberately does not couple readiness to Keycloak, so a slow IdP will not CrashLoop the pod.
• Frontend: tokens are never persisted (in-memory only, confirmed against the Keycloak adapter docs; localStorage holds only the theme string), PKCE S256, bearer auth with no CSRF surface, the minted secret lives only in an in-memory atom, and there is no XSS surface (dangerouslySetInnerHTML, innerHTML, and eval are all absent). The E2E auth-bypass branches are gated behind import.meta.env.MODE !== "production", so they are dead-code-eliminated from the shipped bundle.
• Helm: guards hold across every render combination, the pods are well hardened (non-root UID 101, readOnlyRootFilesystem, drop: [ALL], seccomp RuntimeDefault), and CI now builds, lints, and tests the frontend image with every third-party action pinned to a SHA.

No blockers. All round-2 blockers and questions are resolved, and the first frontend review surfaced only questions, cleanups, and test-coverage gaps.

[jbouder]

In reference to the 2 questions...

Re: KeyCreatedDialog.tsx:42 — "Download .txt" of the plaintext key

Intentional, and a conscious call. This mirrors the display-once secret pattern used by AWS (access key .csv), Stripe, GitHub PATs, etc.: the key is shown exactly once and cannot be retrieved later (KeyCreatedDialog.tsx:87,90-93), so we give the user a reliable way to capture it.

On the cloud-backup concern specifically: the exposure is inherent to any "capture this secret now" flow rather than unique to the download button. The Copy path has the same property — clipboards sync too (macOS Universal Clipboard, cloud clipboard managers), and the download lands in whatever the user's browser download dir is, which they control. We're not writing to a predictable synced location ourselves.

We've kept the guardrails that matter: destructive-styled warning that the key won't be shown again, the display-once server contract, and no persistence of the plaintext on our side. Net: we accept the tradeoff in favor of not stranding users who can't paste into a manager immediately. If we later want to be stricter we could drop the download and lean on Copy only, but that's a UX regression I don't think is worth it here.

Re: keycloak.ts:69 — checkLoginIframe:false and single-logout detection

Good catch, and the compensating control is exactly the access-token lifespan. Worth spelling out the effective detection window:

getToken() runs on every API request (lib/api.ts:18) and calls updateToken(30), whloak whenever the access token has <30s of validity left. If the SSO session wasterminated server-side, that refresh fails and we hit login() + SessionExpiredError (keycloak.ts:105-108). So for an active user, single-logout propagates within roughly one access-token lifespan of their next request — the iframe would only shorten the window for an i
On whether the realm is tuned for it: that config lives outside this diff — the reri, not this repo (grep for lifespan/ssoSession/realm settings returns nothinghere). We're relying on Keycloak's default accessTokenLifespan of 5 minutes, which is the documented compensating control and keeps the SLO window at ~5 min by default. This also matches the established nebari-landing auth pattern (Model B) we're deliberately mirroring.

Action item, but out of scope for this PR: confirm/document the Nebari realm's accnot silently depending on the default. If ops has bumped it well above 5 min, we'dwant to reconsider re-enabling checkLoginIframe for idle-tab logout detection.