Add Require Authentication toggle for web client apps#2913
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 96172b7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
TL;DR — Adds a "Require Authentication" toggle to web client apps, defaulting new apps to require a valid JWT. Auth config fields ( Key changes
Summary ｜ 31 files ｜ 37 commits ｜ base: Flatten auth config and remove
|
| Layer | Change |
|---|---|
Schema (WebClientConfigSchema) |
auth: WebClientAuthConfigSchema.optional() → inline fields |
Schema (WebClientAuthConfigSchema) |
Deleted entirely |
Type (WebClientAuthConfig) |
Deleted from utility.ts |
Runtime (runAuth.ts) |
config.webClient.auth?.publicKeys → config.webClient.publicKeys |
Key handlers (appAuthKeys.ts) |
Remove updatedAuth intermediary; spread directly |
| Playground startup | webClient.auth = { publicKeys: … } → webClient.publicKeys = … |
Auth init (init.ts) |
Unconditionally sets publicKeys and allowAnonymous: false |
schemas.ts · utility.ts · appAuthKeys.ts · runAuth.ts
Remove validateScopeClaims — unconditional scope validation
Before: SpiceDB scope validation was opt-in for global apps via
validateScopeClaims: truein web client config.
After:validateScopeClaimsis deleted from all schemas, runtime conditionals, seed data, and existing apps via migration. The scope validation block inrunAuth.tsis now unconditional (theifguard becomes a bare block).
schemas.ts · runAuth.ts · public-key-schemas.test.ts
allowAnonymous defaults to false — new apps require authentication
Before:
allowAnonymouswasz.boolean().optional(). When omitted, the system implicitly allowed anonymous sessions (200).
After:allowAnonymousisz.boolean().default(false)— Zod always emits a concrete boolean. When omitted, the field resolves tofalse(auth required). Existing apps are unaffected because the backfill migration setsallowAnonymous: truefor all pre-existing web client apps.
schemas.ts · auth.test.ts · init.ts
Zod-validated allowlist config merge on PATCH
Before: No safe way to update just
allowAnonymous— a raw app PATCH could overwritepublicKeysandaudienceif the client didn't echo them back.
After: Incoming config is parsed throughWebClientConfigSchemaviasafeParse, then an allowlist merge applies onlyallowAnonymous,audience, andallowedDomainson top of the existing config.
Why both Zod validation and an allowlist?
The Zod parse strips fields not in the schema and validates types, but the schema includes fields likepublicKeysthat should only be managed by dedicated routes. The allowlist adds a second layer: even if a field passes schema validation, it won't be merged unless explicitly permitted.
Unified app forms with batched auth save
Before:
AuthKeysSectionwas a self-managing component — it loaded keys viauseEffect, fired individual server actions on each add/delete, and managed its own loading state independently of the parent form. Default agent was optional and clearable.
After:AuthKeysSectionis a fully controlled component. Key additions, deletions, the require-auth toggle, and audience are all staged locally via props and committed in a single batch when the parent form submits. Default agent is now required in both forms.
The create form also gains the full auth section so auth can be configured at app creation time. Both forms share a webClientFields validation fragment. Pending (unsaved) keys display a "New" badge. revalidatePath calls are removed from key actions since the parent form now controls data flow.
| Form | Auth workflow |
|---|---|
| Create | Create app → add keys sequentially → navigate |
| Update | PATCH app (config merge preserves existing keys) → delete marked keys → add new keys |
auth-keys-section.tsx · app-create-form.tsx · app-update-form.tsx · validation.ts
Hardened runtime auth enforcement
Before: When
allowAnonymouswas false, only the JWT verification path rejected unauthenticated requests — anonymous session tokens could still be issued, and missing public keys left the app in an ambiguous state.
After: Two new guards: the anonymous session endpoint returns 401 whenallowAnonymous === false, and the middleware rejects requests when auth is required but no public keys are configured. A warning banner in the UI alerts builders when auth is required but no keys exist.
The allowAnonymous computation is hoisted earlier in runAuth.ts so it's available before the hasAuthConfigured check.
auth.ts · runAuth.ts · auth.test.ts · runAuth-appCredentialAuth.test.ts
Backfill migration and schema default alignment
Before: Auth fields were nested under
webClient.auth,allowAnonymouswas implicitly permissive when undefined, andvalidateScopeClaimscontrolled SpiceDB scope checking.
After: Migration0030hoists fields, stripsvalidateScopeClaims, cleans up JSON nulls, backfillsallowAnonymous: truefor existing apps, and sets the playground app tofalse.
| Step | Action |
|---|---|
| 1 | Hoist publicKeys, audience, allowAnonymous from webClient.auth to webClient; remove auth key |
| 2 | Clean up null-valued fields from COALESCE when a sub-field didn't exist |
| 3 | Strip validateScopeClaims from all web_client apps |
| 4 | Backfill allowAnonymous: true for all web_client apps where still unset |
| 5 | Set playground app to allowAnonymous: false |
0030_set-allow-anonymous-for-existing-apps.sql · init.ts · playground-app.ts
Documentation and spec
Before: Docs described "dual-mode support" where apps with auth keys still served anonymous sessions by default.
After: Docs describe "enforcing authentication" — new apps require authenticated sessions by default. Disabling Require Authentication is the opt-out path. A feature spec (specs/enforce-app-auth/SPEC.md) documents the problem statement, goals, technical design, and migration strategy.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
Severity: High — One data-loss bug that will hit users in normal usage, plus an edge case that can lock out anonymous access with no UI to undo it.
The new endpoint, tests, and UI toggle are well-structured. The server-side config merging in PATCH /settings is correct. However, the interaction between the new toggle and the existing parent form creates a real race condition that must be fixed before merge.
Not in diff but related: The parent form's onSubmit at app-update-form.tsx:86-91 spreads webConfig?.auth (captured at render time) into the config payload. When the user toggles "Require Authentication" via the AuthKeysSection (which writes directly to the server), then clicks "Update App", the stale webConfig snapshot overwrites allowAnonymous back to its pre-toggle value. The DAL does a full JSONB column replace, not a deep merge. This is the exact scenario listed as a manual test case ("Parent form submit does not overwrite auth settings") — and it will fail.
Spec drift: specs/enforce-app-auth/SPEC.md says "No API changes needed" in three places and lists "Adding new API endpoints" as a Non-Goal, but the implementation adds PATCH /auth/keys/settings. The dedicated endpoint is the better design — the spec should be updated to match.
Claude Opus ｜ 𝕏
| tenantId={tenantId} | ||
| projectId={projectId} | ||
| appId={app.id} | ||
| allowAnonymous={webConfig?.auth?.allowAnonymous} |
There was a problem hiding this comment.
Bug (data loss): webConfig is a snapshot from render time (line 42–45). When AuthKeysSection toggles allowAnonymous via the dedicated /settings endpoint, this prop is never refreshed. If the user then clicks "Update App", the parent form sends config.webClient.auth spread from the stale snapshot, and since the DAL does a full JSONB column replace, the persisted allowAnonymous: false is silently reverted.
Safest fix: have the parent form not include auth settings it doesn’t own. Strip allowAnonymous (and publicKeys) from the auth object the form submits, since those are managed by the AuthKeysSection endpoints. Alternatively, re-fetch the latest app record in onSubmit before building the payload.
| disabled={isUpdatingAuth} | ||
| /> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Edge case: If a user enables "Require Authentication" (setting allowAnonymous: false), then deletes all public keys, this toggle disappears (keys.length > 0 guard) but allowAnonymous remains false on the server. The runtime will then reject all requests with 401 and there’s no UI affordance to fix it.
Consider auto-resetting allowAnonymous to true in handleDelete when the last key is removed, or keeping the toggle visible (with a warning) when requireAuth is true regardless of key count.
| setRequireAuth(checked); | ||
| setIsUpdatingAuth(true); | ||
| try { | ||
| const result = await updateAppAuthSettingsAction(tenantId, projectId, appId, !checked); |
There was a problem hiding this comment.
The !checked inversion (UI requireAuth → API allowAnonymous) is correct but easy to misread during maintenance. A named variable would make the intent explicit:
const allowAnonymous = !checked;
const result = await updateAppAuthSettingsAction(tenantId, projectId, appId, allowAnonymous);|
|
||
| The existing `PATCH /tenants/{tenantId}/projects/{projectId}/apps/{id}` endpoint already accepts `config` in the body via `AppApiUpdateSchema`. The UI will send the `allowAnonymous` value as part of the `config.webClient.auth` object, merged with existing auth config (preserving `publicKeys`, `audience`, etc.). | ||
|
|
||
| **No API changes needed.** |
There was a problem hiding this comment.
This line says "No API changes needed", but the implementation adds PATCH /auth/keys/settings with new OpenAPI schemas and a changeset for agents-api. The dedicated endpoint is arguably the right call (avoids client-side read-merge-write). Update the spec to reflect reality so it’s not misleading for future readers.
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
No new Major issues identified in this review. See Pending Recommendations below for 2 unresolved Major issues from the prior review.
🟡 Minor (2) 🟡
🟡 1) auth-keys-section.tsx:228 Switch disabled state lacks loading feedback
Issue: When isUpdatingAuth is true, the Switch is disabled but there's no visual indicator (spinner, loading text) that an operation is in progress.
Why: Users may think the toggle is broken when they click and nothing happens immediately. The optimistic UI update helps, but slow API responses leave users confused.
Fix: Add a small Loader2 icon with animate-spin next to the Switch when isUpdatingAuth is true, or overlay a subtle loading state on the toggle row.
Refs: auth-keys-section.tsx:228
🟡 2) appAuthKeys.ts:243-257 Schemas defined locally instead of in agents-core
Issue: UpdateAuthSettingsRequestSchema and AuthSettingsResponseSchema are defined inline in the route file, while related schemas (AddPublicKeyRequestSchema, PublicKeyListResponseSchema, PublicKeyResponseSchema) are defined in agents-core/src/validation/schemas.ts.
Why: This breaks the established pattern for API contract schemas. Local definitions can't be reused across packages and won't be included in SDK generation consistently.
Fix: Move both schemas to packages/agents-core/src/validation/schemas.ts alongside the other PublicKey* schemas, then import them.
Refs: agents-core schemas.ts:1932-1950
Inline Comments:
- 🟡 Minor:
auth-keys-section.tsx:214-231Switch missing aria-describedby for accessibility - 🟡 Minor:
appAuthKeys.ts:243-257OpenAPI schema names missing resource prefix - 🟡 Minor:
appAuthKeys.test.ts:322Missing error body assertion for 400 response
💭 Consider (4) 💭
💭 1) auth-keys-section.tsx:220-222 Toggle description unclear about existing sessions
Issue: "When enabled, all users must present a valid signed JWT" doesn't clarify what happens to existing anonymous sessions.
Why: Users enabling this may not realize existing anonymous sessions will start receiving 401s immediately.
Fix: Update to: "When enabled, new requests must include a valid signed JWT. Existing anonymous sessions will be rejected."
💭 2) auth-keys-section.tsx:174-189 Clipboard button missing focus-visible styling
Issue: The inline button for copying public keys lacks explicit focus indicator for keyboard navigation.
Fix: Add focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 to match design system patterns.
💭 3) appAuthKeys.ts:329 Response returns partial state
Issue: The endpoint returns only { data: { allowAnonymous } } while sibling endpoints return full resource state (e.g., create-app-auth-key returns the full key).
Why: Minor inconsistency with REST conventions for PATCH returning updated state.
Fix: Consider returning full auth settings: { data: { allowAnonymous, audience } }.
💭 4) app-credentials.mdx:112 Docs missing 401 error response format
Issue: Documentation says requests are "rejected with a 401 Unauthorized response" but doesn't show the error body.
Why: Developers need the exact response shape to provide good UX in their apps.
Fix: Add example: { "error": { "code": "unauthorized", "message": "Authentication required" } }
🕐 Pending Recommendations (4)
- 🟠
app-update-form.tsx:177Parent form's stale webConfig snapshot overwrites allowAnonymous on submit - 🟠
auth-keys-section.tsx:231Toggle disappears after deleting all keys but allowAnonymous: false persists on server - 🟡
auth-keys-section.tsx:120!checkedinversion could use a named variable for clarity - 🟡
specs/enforce-app-auth/SPEC.md:47Spec says "No API changes needed" but implementation adds PATCH endpoint — update spec to match
🚫 REQUEST CHANGES
Summary: The implementation is well-structured with proper server-side config merging and good test coverage. However, the data loss bug flagged in the prior review (parent form's stale webConfig overwriting allowAnonymous on submit) and the UI edge case (toggle disappearing after key deletion while setting persists) are both blocking issues that need resolution before merge. The remaining findings are minor improvements for accessibility, consistency, and UX.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
auth-keys-section.tsx:52-57 |
Duplicated state derivation in useState and useEffect | LOW confidence — the useEffect is needed for prop sync on parent refetch; this is an acceptable pattern |
appAuthKeys.test.ts:197 |
Missing idempotency test | LOW confidence — endpoint logic is simple spread+set; unlikely to have double-write bug |
appAuthKeys.test.ts:246 |
Missing validateScopeClaims preservation test | Audience preservation test already covers the spread pattern; additional test adds minimal value |
appAuthKeys.test.ts:197 |
Missing initial state verification | LOW priority — behavioral tests would catch schema drift anyway |
appAuthKeys.ts:262 |
URL path structure (/settings under /keys) | Acknowledged as minor naming concern; current structure is functional and changing it now would break API |
Reviewers (7)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
4 | 1 | 1 | 0 | 1 | 0 | 1 |
pr-review-consistency |
5 | 1 | 1 | 0 | 1 | 0 | 1 |
pr-review-tests |
5 | 0 | 0 | 0 | 1 | 0 | 3 |
pr-review-product |
3 | 0 | 2 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-docs |
1 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 18 | 2 | 4 | 0 | 3 | 0 | 5 |
Note: Prior pullfrog review identified 4 issues that remain unresolved and are listed under Pending Recommendations.
| {keys.length > 0 && ( | ||
| <div className="flex items-center justify-between rounded-md border p-3"> | ||
| <div className="space-y-0.5"> | ||
| <Label htmlFor="require-auth" className="text-sm"> | ||
| Require Authentication | ||
| </Label> | ||
| <p className="text-xs text-muted-foreground"> | ||
| When enabled, all users must present a valid signed JWT. Anonymous access is blocked. | ||
| </p> | ||
| </div> | ||
| <Switch | ||
| id="require-auth" | ||
| checked={requireAuth} | ||
| onCheckedChange={handleToggleRequireAuth} | ||
| disabled={isUpdatingAuth} | ||
| /> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🟡 Minor: Switch missing aria-describedby for screen reader context
Issue: The Switch component has a label via htmlFor, but the description paragraph (lines 220-222) explaining what the toggle does is not programmatically associated with the control.
Why: Screen reader users will hear "Require Authentication, switch" but won't hear the critical context: "When enabled, all users must present a valid signed JWT. Anonymous access is blocked." This reduces accessibility for users who depend on assistive technology.
Fix:
| {keys.length > 0 && ( | |
| <div className="flex items-center justify-between rounded-md border p-3"> | |
| <div className="space-y-0.5"> | |
| <Label htmlFor="require-auth" className="text-sm"> | |
| Require Authentication | |
| </Label> | |
| <p className="text-xs text-muted-foreground"> | |
| When enabled, all users must present a valid signed JWT. Anonymous access is blocked. | |
| </p> | |
| </div> | |
| <Switch | |
| id="require-auth" | |
| checked={requireAuth} | |
| onCheckedChange={handleToggleRequireAuth} | |
| disabled={isUpdatingAuth} | |
| /> | |
| </div> | |
| )} | |
| {keys.length > 0 && ( | |
| <div className="flex items-center justify-between rounded-md border p-3"> | |
| <div className="space-y-0.5"> | |
| <Label htmlFor="require-auth" className="text-sm"> | |
| Require Authentication | |
| </Label> | |
| <p id="require-auth-description" className="text-xs text-muted-foreground"> | |
| When enabled, all users must present a valid signed JWT. Anonymous access is blocked. | |
| </p> | |
| </div> | |
| <Switch | |
| id="require-auth" | |
| checked={requireAuth} | |
| onCheckedChange={handleToggleRequireAuth} | |
| disabled={isUpdatingAuth} | |
| aria-describedby="require-auth-description" | |
| /> | |
| </div> | |
| )} |
Refs:
| const UpdateAuthSettingsRequestSchema = z | ||
| .object({ | ||
| allowAnonymous: z | ||
| .boolean() | ||
| .describe('Whether anonymous access is allowed when JWT verification fails'), | ||
| }) | ||
| .openapi('UpdateAuthSettingsRequest'); | ||
|
|
||
| const AuthSettingsResponseSchema = z | ||
| .object({ | ||
| data: z.object({ | ||
| allowAnonymous: z.boolean(), | ||
| }), | ||
| }) | ||
| .openapi('AuthSettingsResponse'); |
There was a problem hiding this comment.
🟡 Minor: OpenAPI schema names missing resource prefix
Issue: The schema names UpdateAuthSettingsRequest and AuthSettingsResponse don't include the resource prefix, inconsistent with sibling schemas like AddPublicKeyRequest and PublicKeyResponse.
Why: Generated SDK types and OpenAPI documentation will have less clear naming. When users browse the API docs or SDK types, AppAuthSettingsResponse is more discoverable than AuthSettingsResponse.
Fix: Consider renaming for consistency:
const UpdateAuthSettingsRequestSchema = z
.object({
allowAnonymous: z
.boolean()
.describe('Whether anonymous access is allowed when JWT verification fails'),
})
.openapi('UpdateAppAuthSettingsRequest'); // Add 'App' prefix
const AuthSettingsResponseSchema = z
.object({
data: z.object({
allowAnonymous: z.boolean(),
}),
})
.openapi('AppAuthSettingsResponse'); // Add 'App' prefixRefs:
- AddPublicKeyRequestSchema — uses
.openapi("AddPublicKeyRequest")
| body: JSON.stringify({ allowAnonymous: false }), | ||
| }); | ||
|
|
||
| expect(res.status).toBe(400); |
There was a problem hiding this comment.
🟡 Minor: Missing error body assertion for 400 response
Issue: The test asserts res.status is 400 but doesn't verify the error message content.
Why: If the error message regresses to a generic "Bad request" or an incorrect message, this test won't catch it. Meaningful error messages help developers debug integration issues.
Fix: Add assertion for the error message:
it('should return 400 for api app type', async () => {
// ... existing setup code ...
const res = await makeRequest(settingsUrl(tenantId, projectId, apiApp.id), {
method: 'PATCH',
body: JSON.stringify({ allowAnonymous: false }),
});
expect(res.status).toBe(400);
const body = await res.json();
expect(body.detail).toContain('web_client');
});
Ito Test Report ❌17 test cases ran. 2 failed, 15 passed. Across 17 total test cases, 15 passed and 2 failed, confirming that the auth-settings flow largely works end to end: PATCH/UI toggles correctly map to and persist allowAnonymous, schema/auth/not-found/type-confusion defenses return expected 400/401/404 outcomes without unintended mutations or identifier leakage, key and audience data are preserved, rapid toggling and refresh/navigation recover to server truth, deleting the final key correctly removes the Require Authentication toggle, and anonymous run-time gating behaves correctly (401 when require-auth is enabled, non-401 when anonymous is allowed). The two confirmed regressions are a high-severity stale-state bug where submitting the parent Update App form can revert a previously toggled allowAnonymous value and a medium-severity mobile UX bug where toggling Require Authentication closes the Edit App dialog, with code review indicating both are real UI/state issues introduced by this PR. ❌ Failed (2)
🟠 Mobile viewport coverage for auth section
Relevant code:
export async function updateAppAuthSettingsAction(
tenantId: string,
projectId: string,
appId: string,
allowAnonymous: boolean
): Promise<ActionResult<void>> {
try {
await updateAppAuthSettings(tenantId, projectId, appId, { allowAnonymous });
revalidatePath(`/${tenantId}/projects/${projectId}/apps`);
return { success: true };
}
export function AppItemMenu({ app, agentOptions }: AppItemMenuProps) {
const [openDialog, setOpenDialog] = useState<DialogType>(null);
const handleDialogClose = () => {
setOpenDialog(null);
};
<Dialog open={true} onOpenChange={(open) => !open && setIsOpen(false)}>
<DialogContent className="max-w-2xl max-h-[85vh] overflow-y-auto overflow-x-hidden">
<DialogHeader>
<DialogTitle>Edit App</DialogTitle>
</DialogHeader>
<div className="pt-4">
<AppUpdateForm
tenantId={tenantId}
projectId={projectId}
app={app}
agentOptions={agentOptions}
onAppUpdated={() => setIsOpen(false)}
/>
</div>
|
| Category | Summary | Screenshot |
|---|---|---|
| Adversarial | All malformed settings payloads were rejected with HTTP 400 and no allowAnonymous mutation occurred. | |
| Adversarial | Direct unauthenticated PATCH was denied with 401 and authenticated verification confirmed no state change. | |
| Adversarial | API app auth-settings PATCH was rejected with HTTP 400 unsupported-type behavior, and follow-up reads showed no auth/web_client mutation. | |
| Adversarial | PATCH to nonexistent-app and encoded odd app ID returned consistent 404 not_found payloads; responses contained no real app identifier leakage, and follow-up GET on a real app confirmed no collateral mutation. | |
| Adversarial | With allowAnonymous set to false, anonymous-session token issuance succeeded but run chat call with that token returned 401 unauthorized, matching expected auth-gate behavior. | |
| Adversarial | With allowAnonymous explicitly set to true on a keyed web_client app, anonymous-session issued a token and POST /run/api/chat returned 404 (Agent not found), which satisfies the expected non-401 auth-gate behavior. | |
| Edge | Rapid toggling converged to the expected persisted auth mode with no crash or stuck control. | |
| Edge | Refresh and navigation interruptions reloaded the keyed app state from server truth without cross-app bleed. | |
| Edge | Deleted the only key from a web_client app; after refresh/reopen the Authentication Keys section showed empty state and the Require Authentication toggle was fully removed. | |
| Logic | Toggle state stays consistent with server state after close/reopen and full reload checks. | |
| Logic | Updating audience plus auth toggles preserves audience and keys while only allowAnonymous changes. |
|
| Happy-path | Authenticated PATCH returned 200 with allowAnonymous=false and follow-up GET confirmed persisted false on keyed web_client app. | |
| Happy-path | Authenticated PATCH toggled allowAnonymous back to true and auth.publicKeys length remained unchanged. | |
| Ui | Toggle ON maps to allowAnonymous=false, shows auth-required feedback, and persists successfully. |
|
| Ui | Toggle OFF maps to allowAnonymous=true, shows anonymous-access feedback, and persists successfully. |
Commit: 329d200
Tell us how we did: Give Ito Feedback
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Medium
Delta review: This review covers only the changes since the last automated review (commit
3463b9ae1— "Default allowAnonymous to false and enforce across all auth paths").
🟠⚠️ Major (1) 🟠⚠️
🟠 1) 0027_seed-playground-app.sql:9 Playground app seed creates locked-out state
Issue: The seed sets "allowAnonymous": false with "publicKeys": []. The new edge-case check in runAuth.ts:624-633 throws 401 when !hasAuthConfigured && !allowAnonymous. Fresh installs will have a playground app that rejects all anonymous session requests.
Why: New deployments cannot use the playground until an admin manually fixes the config via database or API. The init.ts code (line 213-215) correctly only adds auth config when publicKeys.length > 0, but this seed migration doesn't follow the same pattern — it unconditionally sets allowAnonymous: false even with empty keys.
Fix: Remove allowAnonymous from the seed JSON so undefined means permissive (matching runtime behavior), or only include the auth block when keys are present.
Refs:
Inline Comments:
- 🟠 Major:
0027_seed-playground-app.sql:9Playground seed creates locked-out state
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
appAuthKeys.ts:154Key operations silently migrate undefined → false
💭 Consider (2) 💭
💭 1) schemas.ts:1907 Schema/runtime contract drift
Issue: The change from .optional() to .default(false) affects the Zod schema but runtime code reads raw JSONB with !== false checks. The schema says "default false" but undefined in DB still means permissive at runtime.
Why: This creates misleading SDK types and OpenAPI docs — they imply stricter behavior than runtime delivers.
Fix: Either keep as .optional() to reflect reality, or add a data migration to explicitly set allowAnonymous on existing apps.
💭 2) runAuth.ts:624-633 API can create misconfigured state
Issue: The PATCH /settings endpoint allows setting allowAnonymous: false even when no public keys exist, creating an app that rejects all requests.
Why: The UI guards against this (toggle hidden when no keys), but API callers can reach this state.
Fix: Consider validating in the settings endpoint that publicKeys.length > 0 when setting allowAnonymous: false.
🕐 Pending Recommendations (4)
- 🟠
app-update-form.tsx:177Parent form's stale webConfig snapshot overwrites allowAnonymous on submit - 🟠
auth-keys-section.tsx:231Toggle disappears after deleting all keys but allowAnonymous: false persists on server - 🟡
auth-keys-section.tsx:120!checkedinversion could use a named variable for clarity - 🟡
specs/enforce-app-auth/SPEC.md:47Spec says "No API changes needed" but implementation adds PATCH endpoint — update spec to match
🚫 REQUEST CHANGES
Summary: The auth hardening in this commit is well-intentioned, but the seed migration creates a locked-out state for the playground app on fresh installs (allowAnonymous: false + empty publicKeys = 401 on all requests). This must be fixed before merge. Additionally, the two Major issues from the prior review (stale form state, toggle disappearing) remain unaddressed per the Pending Recommendations.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
auth.ts:170-175 |
Anonymous session endpoint check | Validated as correct — strict equality === false correctly handles undefined as permissive |
runAuth.ts:622 |
allowAnonymous variable moved earlier | Validated as correct refactoring — no behavior change |
init.ts:213-215 |
allowAnonymous: false in programmatic init | Validated as correct — only added when publicKeys.length > 0 |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
4 | 0 | 1 | 0 | 1 | 0 | 2 |
pr-review-breaking-changes |
4 | 1 | 1 | 0 | 1 | 0 | 1 |
pr-review-standards |
1 | 1 | 0 | 0 | 0 | 0 | 0 |
| Total | 9 | 2 | 2 | 0 | 2 | 0 | 3 |
Note: Findings deduplicated across reviewers — seed migration issue surfaced by both standards and breaking-changes reviewers.
| 'web_client', | ||
| true, | ||
| '{"type": "web_client", "webClient": {"allowedDomains": ["*"], "auth": {"publicKeys": [], "validateScopeClaims": true}}}', | ||
| '{"type": "web_client", "webClient": {"allowedDomains": ["*"], "auth": {"publicKeys": [], "validateScopeClaims": true, "allowAnonymous": false}}}', |
There was a problem hiding this comment.
🟠 MAJOR: Playground app seed creates locked-out state
Issue: The seed sets "allowAnonymous": false with "publicKeys": [], but the new edge-case check in runAuth.ts:624-633 throws 'Authentication is required but no public keys are configured' when !hasAuthConfigured && !allowAnonymous. Fresh installs will have a playground app that rejects all requests.
Why: New deployments cannot use the playground until an admin manually fixes the config. The init.ts code (line 213-215) correctly only adds auth config when publicKeys.length > 0, but this migration doesn't follow the same pattern.
Fix: Either remove allowAnonymous from the seed (so undefined means permissive), or set it to true:
| '{"type": "web_client", "webClient": {"allowedDomains": ["*"], "auth": {"publicKeys": [], "validateScopeClaims": true, "allowAnonymous": false}}}', | |
| '{"type": "web_client", "webClient": {"allowedDomains": ["*"], "auth": {"publicKeys": [], "validateScopeClaims": true}}}', |
Refs:
| const updatedKeys = [...existingKeys, newKey]; | ||
| const updatedAuth = { | ||
| ...appRecord.config.webClient.auth, | ||
| allowAnonymous: appRecord.config.webClient.auth?.allowAnonymous ?? false, |
There was a problem hiding this comment.
🟡 Minor: Key operations silently migrate allowAnonymous: undefined → false
Issue: The ?? false fallback means any key add/delete on an app that was implicitly allowing anonymous access (undefined allowAnonymous) will set it to false, blocking anonymous access on that operation.
Why: This is intentional hardening per the PR's goal, but it's a silent one-way migration that happens during unrelated operations. Users might be surprised when adding a new key suddenly blocks anonymous access.
Fix: This is acceptable behavior given the security goals. Consider documenting in release notes: "Apps with undefined allowAnonymous will be set to false on next key operation."
Refs:
Ito Test Report ❌16 test cases ran. 1 failed, 15 passed. Overall, the unified run executed 16 test cases with 15 passes and 1 failure (plus one report with no verifiable functional outcomes), showing that auth settings and authentication-key flows are largely stable: PATCH updates to allowAnonymous persisted and preserved key/audience data, invalid targets were correctly rejected (400 for api apps, 404 for missing apps), UI behavior and persistence held across reopen/reload/mobile, rapid toggles and rollback recovery worked, and strict-mode/origin enforcement on anonymous-session produced expected 401/403/200 results (with PoW bypass only where explicitly noted). ❌ Failed (1)
|
| Category | Summary | Screenshot |
|---|---|---|
| Adversarial | With allowAnonymous enabled, disallowed origin https://evil.example.net returned 403 and allowed origin https://trusted.example.com returned 200 with anonymous token. Note: a test-only bypass was applied to remove PoW gating so origin behavior could be validated. | |
| Adversarial | After issuing an anonymous token in permissive mode and switching to strict mode, anonymous-session with that prior bearer token was rejected with HTTP 401 as expected. | |
| Edge | Rapid toggling remained stable and backend state matched the final UI intent. | |
| Edge | A forced one-time failure rolled back correctly, and the next toggle recovered successfully. | |
| Edge | Submitting Update App after a non-auth edit preserved the toggled auth mode. | |
| Edge | With a web_client app configured for trusted.example.com and allowAnonymous=false, anonymous-session from allowed origin returned 401 Unauthorized exactly as expected. | |
| Edge | Deleting the only key removed the key row, removed the Require Authentication switch, and the edit dialog continued functioning after close/reopen with no crash or loading lock. | |
| Edge | Mobile auth-key controls were reachable and persisted after reopen; backend state matched (allowAnonymous=true). |
|
| Happy-path | PATCH returned 200 and persisted allowAnonymous=false on follow-up GET. |
|
| Happy-path | PATCH returned 200 and persisted allowAnonymous=true on follow-up GET. |
|
| Happy-path | Settings PATCH preserved key and audience data while updating allowAnonymous. |
|
| Happy-path | API-type app correctly rejected settings PATCH with 400 and no mutation. | |
| Happy-path | Missing app settings PATCH returned 404 and app inventory stayed unchanged. | |
| Ui | With zero keys, the dialog showed the empty Authentication Keys state and no switch. After adding the first valid key, the Require Authentication switch appeared immediately. | |
| Ui | After toggling state and receiving successful updates, the switch value persisted across dialog close/reopen and after a page reload. Backend verification confirmed persisted allowAnonymous=false matching UI. |
Commit: 3463b9a
Tell us how we did: Give Ito Feedback
UI-only validation — the API still accepts apps without a default agent, but the form enforces selection to prevent misconfigured apps. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Anonymous session endpoint: - Rejects when allowAnonymous is false (401) - Allows when allowAnonymous is true - Allows when allowAnonymous is not set (default behavior) - Correctly toggles: reject → allow after switching back to true Key operations preserve allowAnonymous: - Adding a key preserves existing allowAnonymous value - Deleting a key preserves existing allowAnonymous value Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Add migration 0030 to backfill allowAnonymous=true for all existing web_client apps (preserves current behavior) and set playground to false - Show warning banner when Require Authentication is enabled but no public keys are configured - Show toggle unconditionally (user requirement) with contextual warning - Remove unused requireAuth field from form validation schema - Always call onAppUpdated() after successful app PATCH even on partial key failure so parent refreshes Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Create form defaults to true (matching schema default of allowAnonymous: false). Edit form uses !== true so undefined/false both show the toggle as ON — only explicitly set allowAnonymous: true turns it off. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Only allow allowAnonymous and audience to be updated through the app PATCH config merge. Previously the merge stripped publicKeys but allowed any other field through, which could let an admin inject validateScopeClaims: false to bypass SpiceDB authorization checks. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Parse incoming config through WebClientConfigSchema before merging to strip any unexpected fields. Combined with the allowlist merge (only allowAnonymous and audience are updatable), this ensures: - Schema validation catches malformed config early - No extra fields can be injected through the merge path - publicKeys and validateScopeClaims remain managed by dedicated routes Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Remove the unnecessary auth nesting — publicKeys, audience, validateScopeClaims, and allowAnonymous now live directly on webClient instead of webClient.auth. Migration 0030 updated to: 1. Hoist fields from webClient.auth to webClient for existing apps 2. Remove the auth key 3. Clean up null-valued hoisted fields 4. Backfill allowAnonymous=true for existing apps 5. Set playground to allowAnonymous=false Also: - Import ALLOWED_DOMAIN_PATTERN from agents-core/client-exports instead of duplicating it in the manage UI validation - Bump both changesets from patch to minor (includes migration) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There's no valid reason to skip SpiceDB scope validation on global apps. If a global app accepts tenant/project claims from JWTs, those claims must always be verified. - Remove validateScopeClaims from WebClientConfigSchema and response schema - Remove from WebClientAuthConfigSchema (kept for backward compat type) - Make scope validation unconditional in runAuth.ts for global apps - Remove from playground seed migration and init code - Migration 0030 strips the field from existing apps - Update test assertion to expect validation call Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…schemas Complete cleanup: remove the old nested auth schema entirely and strip validateScopeClaims from both WebClientConfigSchema and response schema. Remove associated type export and tests. Add test verifying validateScopeClaims is stripped by Zod parsing. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Update Enforcing authentication section to reflect that new apps require auth by default. Remove inline API example. Update security model table and key management cross-reference. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Changed from optional() to default(false) so Zod enforces the field is always present. Tests updated to explicitly set allowAnonymous: true when anonymous access is needed. OpenAPI snapshot updated. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Remove onKeysChange prop and no-op call from AuthKeysSection - Extract allowAnonymous = !requireAuth into named variable in both forms for clarity - Remove onKeysChange from parent form call sites Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The lineage checker requires applied migrations to be unchanged. Migration 0030 already handles stripping validateScopeClaims from existing apps including the playground. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The schema now requires publicKeys and allowAnonymous (non-optional with default). Update init.ts to always provide both fields, and update data-access tests to include them in config objects. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Cypress tests create web_client apps that need anonymous session access. With allowAnonymous now defaulting to false, they need to explicitly opt in. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Update createApp assertion to include publicKeys and allowAnonymous - Update schema test: omitted fields now get defaults ([] and false) instead of being undefined Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The caret range ^1.58.2 allowed version drift between the cached Playwright browsers and the installed package version. CI caches browsers keyed on the lockfile hash, but the fallback restore-key could restore browsers from a different Playwright version, causing "Executable doesn't exist" errors. See: microsoft/playwright#39122 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
✅ Review Analysis
This PR adds a "Require Authentication" toggle for web client apps with comprehensive implementation across the entire stack:
| Component | Assessment |
|---|---|
Schema (WebClientConfigSchema) |
✅ allowAnonymous: z.boolean().default(false) — secure-by-default |
| Migration 0030 | ✅ Correctly flattens auth config, backfills existing apps to allowAnonymous: true, sets playground to false |
Runtime (runAuth.ts:622-635) |
✅ Misconfiguration guard rejects when auth required but no keys configured |
Anonymous session (auth.ts:168-173) |
✅ Rejects with 401 when allowAnonymous === false |
UI (AuthKeysSection) |
✅ Controlled component with warning banner for misconfiguration |
API (apps.ts:221-248) |
✅ Server-side config merge with Zod validation + allowlist |
| Tests (67 tests) | ✅ Comprehensive coverage for all auth enforcement paths |
🔐 Security Analysis
- Secure-by-default: New apps require authentication (
allowAnonymous: false) - Backward compatible: Migration backfills existing apps to
allowAnonymous: true - Allowlist approach: Config merge only permits
allowAnonymous,audience,allowedDomainsupdates - SpiceDB validation: Always-on for global apps (removed opt-in
validateScopeClaims) - Misconfiguration guard: Rejects requests when auth required but no keys configured
🕐 Pending Recommendations (0)
All prior issues have been addressed across 24 review cycles.
📝 Human Reviewer Question
@amikofalvy asked: "Does this format as problem details format?" (on
apps.ts:226)
Status: ✅ Answered in PR discussion — Yes, createApiError returns RFC 9457 Problem Details format.
✅ APPROVE
Summary: Ship it! 🚀 This is a well-implemented, secure-by-default feature with:
- Proper auth config flattening from
webClient.auth.*towebClient.* - Comprehensive test coverage (67 tests) covering all auth enforcement paths
- Clean migration strategy preserving existing app behavior while securing new apps
- Controlled component UI pattern with batched save and clear warning states
The PR has been through 24 review cycles with all issues resolved. The implementation is production-ready.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator (final) |
— | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | — | 0 | 0 | 0 | 0 | 0 | 0 |
Note: This is a delta review building on 24 prior automated reviews. All substantive issues were addressed in earlier iterations.
pnpx resolves to the latest global Playwright version, which downloads browsers for a different build than the project's pinned version. Switch to pnpm exec to use the project-local [email protected]. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta review: This review covers only the changes since the last automated review (commit
9ede1c0eb→9e0a765b0— 1 commit: "Fix CI: use project-local playwright for browser install"). Only 1 file changed.
✅ Delta Analysis
| File | Change | Assessment |
|---|---|---|
.github/workflows/ci.yml:165 |
pnpx playwright install → pnpm exec playwright install |
✅ Correct — uses project-local Playwright |
.github/workflows/ci.yml:340 |
Same change in second workflow job | ✅ Consistent — both jobs updated |
Why this change is correct:
The change from pnpx to pnpm exec ensures the CI workflow uses the project-local Playwright version (pinned at 1.58.2 in package.json) rather than potentially fetching a different version via pnpx. This:
- Ensures version consistency — The lockfile pins
[email protected]explicitly (changed from^1.58.2in a prior commit). Usingpnpm execguarantees this exact version is used for browser installation. - Avoids version mismatch —
pnpx(likenpx) can resolve to a different version than what's in the project, causing browser/library version mismatches that break tests. - Aligns with best practices — When a package is already in
devDependencies,pnpm execis the correct way to run its binaries in CI.
🕐 Pending Recommendations (0)
All prior issues have been addressed across 25 review cycles. ✅
📝 Human Reviewer Question
@amikofalvy asked: "Does this format as problem details format?" (on
apps.ts:226)
Status: ✅ Answered in PR discussion — Yes, createApiError returns RFC 9457 Problem Details format.
✅ APPROVE
Summary: Ship it! 🚀 The delta is a trivial but correct CI fix — ensuring the project-local Playwright version is used for browser installation. All 25 prior review cycles have addressed the substantive issues (schema design, migration safety, UI controlled component refactor, security hardening, etc.). The implementation is solid — well-tested (67 tests), secure-by-default, and properly migrates existing apps.
Reviewers (0)
No domain reviewers dispatched — delta is CI infrastructure only (no application code changes).
pnpm exec at root can't find playwright — it's a devDep of agents-manage-ui, not the root workspace. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta review: This review covers only the changes since the last automated review (commit
9e0a765b0→96172b734— 1 commit: "Fix CI: scope playwright install to agents-manage-ui package"). Only 1 file changed.
✅ Delta Analysis
| File | Change | Assessment |
|---|---|---|
.github/workflows/ci.yml:165 |
pnpm exec playwright install → pnpm --filter @inkeep/agents-manage-ui exec playwright install |
✅ Correct — uses package-local Playwright |
.github/workflows/ci.yml:340 |
Same change in second workflow job | ✅ Consistent — both jobs updated |
Why this change is correct:
The change from pnpm exec to pnpm --filter @inkeep/agents-manage-ui exec ensures the CI workflow uses the package-local Playwright version (pinned at 1.58.2 in agents-manage-ui/package.json:126) rather than resolving from the workspace root.
This:
- Ensures version consistency — The lockfile pins
[email protected]exactly (not^1.58.2). Using--filterguarantees this exact version is used for browser installation. - Avoids version mismatch — Running
pnpm execat the root level could resolve a different Playwright version, causing browser/library version mismatches that break tests. - Aligns with best practices — When a package has its own pinned dependency, scoped execution via
--filteris the correct approach.
🕐 Pending Recommendations (0)
All prior issues have been addressed across 26 review cycles. ✅
✅ APPROVE
Summary: Ship it! 🚀 The delta is a trivial but correct CI fix — scoping the Playwright browser installation to the agents-manage-ui package ensures the pinned version (1.58.2) is used consistently. All substantive review findings from the 26 prior review cycles have been addressed. The implementation is solid — well-tested (67 tests), secure-by-default, and properly migrates existing apps.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: No domain reviewers dispatched — delta is a trivial CI fix with no code logic changes.
|
🔎💬 Inkeep AI search and chat service is syncing content for source 'Inkeep Agent Framework Docs' |
Ito Test Report ✅18 test cases ran. 18 passed. All 18 test cases passed, indicating the web client app create/edit, authentication, authorization, key-management, validation, navigation, responsive, and stress-interaction flows are functioning as expected in local execution with no code-supported product defect identified. Key findings were that Require Authentication defaults to ON and correctly blocks anonymous sessions unless explicitly disabled, attacker-origin and unauthorized/malformed requests are consistently denied (401/403 as designed), malformed or duplicate key operations are safely rejected without state corruption, PATCH toggles preserve existing audience/keys, unsaved edits are discarded unless submitted, and rapid-click plus back/forward/mobile scenarios maintained a single consistent persisted state. ✅ Passed (18)
Commit: Tell us how we did: Give Ito Feedback |
Summary
allowAnonymous: falseschema default)publicKeys,audience,allowAnonymousfrom nestedwebClient.authto directly onwebClientvalidateScopeClaimsoption — SpiceDB scope validation is now always-on for global appsallowAnonymousis falseallowAnonymousin the run auth middleware even when no public keys are configuredallowAnonymous, and stripvalidateScopeClaimsfor existing appsALLOWED_DOMAIN_PATTERNfromagents-core/client-exportsinstead of duplicating in manage UIBreaking Changes
allowAnonymousdefaults tofalse— new apps require authentication unless explicitly set totrue. Existing apps are backfilled totruevia migration 0030.webClient.auth.publicKeys,webClient.auth.audience,webClient.auth.allowAnonymousare nowwebClient.publicKeys,webClient.audience,webClient.allowAnonymous. Migration 0030 handles the data migration.validateScopeClaimsremoved — SpiceDB scope validation is now unconditional for global apps. The field is stripped from existing app configs by migration 0030.Changes
WebClientConfigSchema— auth fields directly onwebClient; removevalidateScopeClaims;allowAnonymousdefaults tofalsewebClient.authtowebClient, stripvalidateScopeClaims, backfillallowAnonymous: truefor existing apps, set playground tofalseallowAnonymousandaudienceupdatable via PATCH)allowAnonymousoutsidehasAuthConfiguredbranch; always validate scope for global appsallowAnonymousis falseALLOWED_DOMAIN_PATTERNfromagents-core/client-exportsTest plan
allowAnonymous: false(401)allowAnonymous: truepublicKeysfrom incoming payload (allowlist enforcement)allowAnonymousvalueGenerated with Claude Code