feat(agents-api, manage-ui): App Credentials & End-User Authentication#2501
feat(agents-api, manage-ui): App Credentials & End-User Authentication#2501amikofalvy merged 70 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
🦋 Changeset detectedLatest commit: 9b9b923 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 |
|
Introduces App Credentials — a polymorphic replacement for flat API keys that encodes channel type, domain restrictions, agent access control, and end-user identity (Phase 1). Backend (
Dashboard UI (
Docs & OpenAPI:
Tests:
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid foundation for app credentials and end-user auth. The auth exclusivity model (no fallback when X-Inkeep-App-Id is present), scrypt-based secret hashing, and test coverage are all well done.
There are a few issues worth addressing before merging — the highest-priority ones are the hs256Secret leaking in API responses, the updateAppLastUsed tenant-scoping gap, and the wildcard domain (*) silent no-op. See inline comments for details.
| if (domain === '*') { | ||
| continue; |
There was a problem hiding this comment.
Bug: wildcard * silently blocks all origins. When allowedDomains is ['*'], the continue causes the loop to exhaust without returning true, so validateOrigin returns false. If * should mean "allow all origins", this should return true. If * is intentionally unsupported, reject it at config validation time with a clear error rather than silently skipping.
agents-api/src/middleware/runAuth.ts
Outdated
| if (!jwtVerified && config.webClient.hs256Enabled && config.webClient.hs256Secret) { | ||
| try { | ||
| const customerSecret = new TextEncoder().encode(config.webClient.hs256Secret); | ||
| const { payload } = await jwtVerify(bearerToken, customerSecret); |
There was a problem hiding this comment.
Missing app-binding check on customer HS256 JWTs. Unlike the anonymous JWT path (which checks payload.app !== appIdHeader), customer-issued HS256 JWTs are verified without any claim tying them to the current app. If a customer reuses the same HS256 secret across multiple apps, a token from one app can be replayed against another. Consider requiring an aud or app claim check, or documenting that each app MUST use a unique HS256 secret.
agents-api/src/middleware/runAuth.ts
Outdated
| return { authResult: null, failureMessage: 'Invalid app secret' }; | ||
| } | ||
| } else { | ||
| return { authResult: null, failureMessage: `Unsupported app type: ${app.type}` }; |
There was a problem hiding this comment.
Error message leaks app type. Unsupported app type: ${app.type} discloses the internal type value to unauthenticated callers. Use a generic message like 'Unsupported app configuration'.
agents-api/src/middleware/runAuth.ts
Outdated
| return { | ||
| authResult: null, | ||
| failureMessage: requestedAgentId | ||
| ? `Agent '${requestedAgentId}' is not allowed for this app` |
There was a problem hiding this comment.
Error message echoes user input. Agent '${requestedAgentId}' is not allowed for this app confirms which agent IDs are not in the allowlist. Consider a generic message like 'Requested agent is not available for this app'.
|
|
||
| const logger = getLogger('run-auth'); | ||
|
|
||
| const DEV_ANON_SECRET = 'insecure-anon-dev-secret-change-in-production-32c'; |
There was a problem hiding this comment.
Dev secret fallback is too broad. DEV_ANON_SECRET is used whenever ENVIRONMENT !== 'production', which includes staging or any misconfigured value. Consider restricting to 'development' and 'test' explicitly, and throwing for other environments when INKEEP_ANON_JWT_SECRET is missing.
| export const AppUpdateSchema = AppInsertSchema.partial().omit({ | ||
| tenantId: true, | ||
| projectId: true, | ||
| id: true, | ||
| publicId: true, | ||
| keyHash: true, | ||
| keyPrefix: true, | ||
| createdAt: true, | ||
| }); |
There was a problem hiding this comment.
AppUpdateSchema allows changing type. The update schema is AppInsertSchema.partial().omit(...) but does not omit type. Changing from web_client to api won't generate credentials; changing from api to web_client leaves orphaned hash/prefix. Add type: true to the .omit() call.
agents-manage-ui/src/lib/api/apps.ts
Outdated
| const rawResponse = response.data as any; | ||
| if (rawResponse?.data?.app) { | ||
| const { app, appSecret } = rawResponse.data; | ||
| return { | ||
| app: { | ||
| ...app, | ||
| lastUsedAt: app.lastUsedAt ?? undefined, | ||
| }, | ||
| appSecret, | ||
| }; | ||
| } | ||
|
|
||
| if (rawResponse?.app) { | ||
| return { | ||
| app: { | ||
| ...rawResponse.app, | ||
| lastUsedAt: rawResponse.app.lastUsedAt ?? undefined, | ||
| }, | ||
| appSecret: rawResponse.appSecret, | ||
| }; | ||
| } | ||
|
|
||
| return rawResponse; |
There was a problem hiding this comment.
Fragile response parsing with cascading any casts. The code guesses at multiple response shapes (rawResponse?.data?.app and rawResponse?.app) via any. Since AppApiCreationResponseSchema defines the shape as { data: { app, appSecret } }, type the response properly rather than casting through any — this will catch regressions if the API shape changes.
| agentAccessMode: z.enum(AGENT_ACCESS_MODE_ENUM), | ||
| allowedAgentIds: z.array(z.string()), | ||
| defaultAgentId: z.string().optional(), | ||
| allowedDomains: z.string().optional(), |
There was a problem hiding this comment.
Client-side validation diverges from backend. The backend requires allowedDomains: z.array(z.string().min(1)).min(1) for web_client apps, but the client schema declares allowedDomains: z.string().optional(). Users creating a web_client app with empty domains will pass client validation but get a raw server error. Add a client-side constraint to match.
| <AlertDialog | ||
| open={open} | ||
| onOpenChange={(isOpen) => { | ||
| if (!isOpen) onClose(); | ||
| }} |
There was a problem hiding this comment.
Secret dialog can be accidentally dismissed. The AlertDialog allows closing via Escape/backdrop click, which permanently loses the one-time API secret. Consider preventing accidental dismissal with onEscapeKeyDown / onPointerDownOutside on AlertDialogContent.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(11) Total Issues | Risk: High
🔴❗ Critical (2) ❗🔴
🔴 1) agents-api/src/domains/manage/routes/apps.ts:70-119 Customer HS256 JWT secret exposed in API responses
Issue: The sanitizedData mapping removes keyHash, tenantId, and projectId but does NOT sanitize the config field. For web_client apps with hs256Enabled: true, the plaintext config.webClient.hs256Secret is returned to any user with view permission on the project.
Why: An attacker with view-only project access can extract the HS256 secret and forge valid end-user JWTs to impersonate any user for that app. This secret grants the ability to mint arbitrary user identities. The issue affects all three response paths: list apps, get single app, and create app responses.
Fix: Sanitize the config field before returning in all response paths:
const sanitizeAppConfig = (config: AppConfig): AppConfig => {
if (config.type === 'web_client' && config.webClient?.hs256Secret) {
return {
...config,
webClient: { ...config.webClient, hs256Secret: undefined }
};
}
return config;
};Apply this sanitization in all three routes (list, get, create).
Refs:
Inline Comments:
- 🔴 Critical:
auth.ts:17Hardcoded development secret is predictable and publicly visible - 🔴 Critical:
apps.ts:70-119Customer HS256 JWT secret exposed in API responses
🟠⚠️ Major (5) 🟠⚠️
🟠 1) system Missing changeset for publishable packages
Issue: This PR modifies multiple publishable packages (agents-api, agents-core) with significant user-facing changes including new API endpoints, new data model, and new authentication flow. The changeset-bot has flagged this PR as missing a changeset.
Why: Per AGENTS.md guidelines, changesets are MANDATORY for "any bug fix, feature, or behavior change to a published package." This PR adds a new database table, new API routes, and new authentication mechanisms—all runtime behavior changes that should be versioned and documented in release notes.
Fix: Create changesets for affected packages:
pnpm bump minor --pkg agents-api --pkg agents-core "Add App Credentials and end-user authentication support"Use minor because this adds new capabilities requiring schema migrations and new API endpoints.
Refs:
🟠 2) system Missing conceptual documentation for App Credentials
Issue: This PR introduces a significant new authentication model (App Credentials with web_client and api types, anonymous sessions, end-user conversation history) but no conceptual documentation explains when to use Apps vs API keys, the two app types, or the end-user authentication flow.
Why: Users will see 'Apps' in the UI and API reference but won't understand: (1) what an App Credential is vs an API key, (2) when to use web_client vs api type, (3) how anonymous session JWTs work, (4) how to implement the full web client auth flow. The existing RunApiAuthModes.mdx snippet only documents API Keys and dev mode headers—not the new X-Inkeep-App-Id authentication path.
Fix:
- Create conceptual documentation at
agents-docs/content/talk-to-your-agents/authentication.mdxcovering authentication options, app types, and the anonymous session flow with code examples - Update
agents-docs/_snippets/auth/RunApiAuthModes.mdxto add a third tab documenting App Credential authentication - Update
agents-docs/content/concepts.mdx(lines 86-94) to mention App Credentials alongside API Keys
Refs:
🟠 3) agents-api/src/middleware/runAuth.ts:546-548 JWT verification error handling hides failure classification
Issue: JWT verification failures in both the anonymous JWT path and customer HS256 path use empty catch { } blocks that discard errors entirely. This makes it impossible to distinguish between expired tokens, invalid signatures, malformed tokens, or clock skew issues.
Why: During incidents where users report auth failures, there's no way to determine the root cause from logs or traces. Operators cannot differentiate between expected failures (expired tokens) and potential attacks (invalid signatures).
Fix: Log JWT verification failures at debug level with error classification using jose's error types (JWTExpired, JWSSignatureVerificationFailed, etc.).
Refs:
🟠 4) packages/agents-core/src/db/runtime/runtime-schema.ts:179 Customer HS256 secret stored in plaintext in database
Issue: The config JSONB column stores the customer-provided hs256Secret in plaintext. Unlike the app API secret (which is properly hashed via scrypt), the JWT signing secret is stored without encryption.
Why: Database administrators or anyone with DB access can read all customer JWT secrets. Database backups contain plaintext secrets. If the database is compromised, all customer JWT signing keys are exposed. This is inconsistent with the proper handling of keyHash for API-type apps.
Fix: Either encrypt the secret using envelope encryption with a key stored in a secrets manager, or store HS256 secrets in a dedicated encrypted column. At minimum, document the security tradeoff if plaintext storage is intentional for Phase 1.
Refs:
Inline Comments:
- 🟠 Major:
domain-validation.ts:17-20Bare wildcard*is silently skipped, causing unexpected validation failures - 🟠 Major:
schemas.ts:1853Customer HS256 secret has no minimum length validation - 🟠 Major:
runAuth.ts:597-599Fire-and-forget DB update without backpressure control
🟡 Minor (4) 🟡
🟡 1) agents-manage-ui/src/components/apps/form/ AgentMultiSelect component duplicated between forms
Issue: The AgentMultiSelect component is copy-pasted identically between app-create-form.tsx (lines 167-259) and app-update-form.tsx (lines 203-295)—92 lines of duplicated code.
Why: Maintenance burden—any bug fix or accessibility improvement needs to be made in two places. Risk of implementations diverging over time.
Fix: Extract AgentMultiSelect to a shared component file (e.g., /components/apps/form/agent-multi-select.tsx) and import in both forms.
Refs:
🟡 2) create-agents-template/.env.example Self-hosting template missing INKEEP_ANON_JWT_SECRET
Issue: The root .env.example was updated with INKEEP_ANON_JWT_SECRET, but create-agents-template/.env.example was not. Self-hosted users bootstrapping from the template won't see this new configuration.
Why: Self-hosted deployments use the template as their starting point. Missing the env var documentation means users may not configure anonymous session support correctly.
Fix: Add the same entry to create-agents-template/.env.example:
# Anonymous session JWT secret (for end-user anonymous sessions via app credentials)
# Generate with: openssl rand -base64 32
# INKEEP_ANON_JWT_SECRET=Refs:
🟡 3) agents-api/src/__tests__/ Missing test for JWT expiration validation
Issue: While unit tests mock jwtVerify, there's no integration test verifying that an actually expired JWT is rejected. The jose library handles expiration, but this is security-critical and should have explicit coverage.
Why: If JWT expiration checking is accidentally disabled or the library has a bug, expired tokens could grant access indefinitely. Token expiration is fundamental to session security.
Fix: Add integration test: create anonymous session with very short lifetime (1 second), wait, then verify the token is rejected with appropriate error.
Refs:
🟡 4) agents-api/src/middleware/runAuth.ts:462-481 Missing tests for resolveAgentId edge cases
Issue: The resolveAgentId function handles multiple scenarios but some edge cases lack test coverage: (1) agentAccessMode='selected' with defaultAgentId not in allowedAgentIds, (2) agentAccessMode='selected' with empty allowedAgentIds array.
Why: Agent access control is a security boundary. Edge cases like empty allowedAgentIds in 'selected' mode could behave unexpectedly.
Fix: Add tests for: empty allowedAgentIds returning null/401, and defaultAgentId not in allowedAgentIds falling back to first allowed agent.
Refs:
Inline Comments:
- 🟡 Minor:
app-create-form.tsx:220-226Icon-only button missing accessible label - 🟡 Minor:
app-update-form.tsx:256-262Icon-only button missing accessible label - 🟡 Minor:
runAuth.ts:546-548Silent catch on JWT verification hides failure modes
💭 Consider (2) 💭
💭 1) agents-api/src/middleware/runAuth.ts:512-519 Type assertion duplicates existing types
Issue: The app.config is cast to an inline type literal instead of using the existing WebClientConfig type from @inkeep/agents-core.
Why: Inline type definitions that duplicate existing types can drift out of sync when the source type changes.
Fix: Import and use the existing type: import type { WebClientConfig } from '@inkeep/agents-core';
💭 2) agents-api/src/domains/run/routes/conversations.ts:98-114 Response envelope differs from peer endpoints
Issue: The run conversations endpoint nests conversations inside data.conversations while peer manage endpoints use a flat data array structure.
Why: Creates a split-world where consumers need to handle different envelope shapes.
Fix: Consider aligning with the flat envelope pattern used by ApiKeyListResponse and AppListResponse.
🚫 REQUEST CHANGES
Summary: This is a well-architected Phase 1 implementation of App Credentials with good test coverage and proper separation of concerns. However, there are two critical security issues that must be addressed before merging:
- HS256 secret exposure in API responses — Customer JWT signing secrets are returned in plaintext to users with view permission, allowing JWT forgery
- Hardcoded dev secret — The predictable fallback secret could be exploited if non-production environments are exposed
Additionally, the PR is missing a changeset (flagged by changeset-bot) and conceptual documentation for the new authentication model.
The architecture decisions are sound: exclusive app credential auth path, stateless anonymous JWTs for Phase 1, proper secret hashing for API-type apps. The consistency with existing patterns (data access layer, route structure, ID formats) is excellent.
Discarded (12)
| Location | Issue | Reason Discarded |
|---|---|---|
runAuth.ts:476-480 |
resolveAgentId falls back to first allowed agent when defaultAgentId not in list | Intentional fallback behavior with appropriate logging; edge case unlikely in practice |
runAuth.ts:550-565 |
Customer HS256 JWT doesn't validate issuer/audience | Valid concern but explicitly Phase 1 scoped; issuer validation would require config changes |
auth.ts:41-68 |
Anonymous session endpoint lacks rate limiting | Valid concern but infra-level rate limiting is typically handled at load balancer/WAF layer |
apps.ts:131 |
Apps route uses 'edit' permission vs API keys 'use' | Intentional—Apps grant broader access than single-agent API keys |
runtime-schema.ts:179 |
JSONB config column lacks DB-level validation | Known tradeoff for JSONB columns; Zod validation at app layer is sufficient for Phase 1 |
domain-validation.ts:5 |
Returns false when allowedDomains is empty | Correct fail-safe behavior; empty domains should reject all |
auth.ts:119 |
Default anonymousSessionLifetimeSeconds untested | Low priority; 86400 is sensible default and behavior is documented |
runAuth.ts:583-584 |
Unsupported app type handling untested | Schema validation prevents invalid types at creation; defensive code unlikely to be hit |
conversations.ts:30-42 |
Schema naming inconsistency | Stylistic preference; both names are valid and follow internal conventions |
apps.ts:597-599 |
updateAppLastUsed failure doesn't affect auth | Intentional fire-and-forget pattern with error logging; correct behavior |
apiKeys.ts:168-181 |
ID prefix conventions | Positive finding—follows established patterns correctly |
| Various | OpenAPI tags properly registered | Positive finding—no action needed |
Reviewers (13)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
7 | 2 | 0 | 0 | 1 | 0 | 4 |
pr-review-standards |
4 | 0 | 0 | 0 | 1 | 0 | 3 |
pr-review-product |
6 | 1 | 0 | 0 | 0 | 0 | 5 |
pr-review-docs |
4 | 1 | 0 | 0 | 0 | 0 | 3 |
pr-review-devops |
6 | 1 | 0 | 0 | 0 | 0 | 5 |
pr-review-sre |
8 | 1 | 0 | 0 | 2 | 0 | 5 |
pr-review-tests |
10 | 2 | 0 | 0 | 0 | 0 | 8 |
pr-review-frontend |
12 | 1 | 0 | 0 | 2 | 0 | 9 |
pr-review-types |
4 | 0 | 1 | 0 | 0 | 0 | 3 |
pr-review-consistency |
10 | 0 | 1 | 0 | 0 | 0 | 9 |
pr-review-architecture |
7 | 1 | 0 | 0 | 0 | 0 | 6 |
pr-review-precision |
4 | 0 | 0 | 0 | 0 | 0 | 4 |
pr-review-breaking-changes |
5 | 0 | 0 | 0 | 0 | 0 | 5 |
| Total | 87 | 10 | 2 | 0 | 6 | 0 | 69 |
|
|
||
| const logger = getLogger('run-auth'); | ||
|
|
||
| const DEV_ANON_SECRET = 'insecure-anon-dev-secret-change-in-production-32c'; |
There was a problem hiding this comment.
🔴 CRITICAL: Hardcoded development secret is predictable and publicly visible
Issue: The DEV_ANON_SECRET constant is a static string visible in source code. If a development/staging environment is accidentally exposed without INKEEP_ANON_JWT_SECRET set, attackers can forge anonymous session JWTs.
Why: While production correctly requires the env var, non-production environments (dev, staging, pentest) silently use this predictable value. A misconfigured deployment or leaked staging credentials gives attackers the ability to mint valid anonymous sessions.
Fix: Consider one of:
- Generate the dev secret randomly at startup:
const DEV_ANON_SECRET = crypto.randomUUID() + crypto.randomUUID(); - Log a warning when using the dev fallback to surface misconfigurations earlier
- Require the env var in all non-test environments (
ENVIRONMENT !== 'test')
Refs:
| authMode: z.enum(['anonymous_only', 'anonymous_and_authenticated', 'authenticated_only']), | ||
| anonymousSessionLifetimeSeconds: z.number().int().min(60).max(604800).default(86400), | ||
| hs256Enabled: z.boolean().default(false), | ||
| hs256Secret: z.string().optional(), |
There was a problem hiding this comment.
🟠 MAJOR: Customer HS256 secret has no minimum length validation
Issue: The hs256Secret field accepts any string without minimum length. HS256 security relies on secret entropy - a weak secret like 'password' can be brute-forced, allowing attackers to forge valid end-user JWTs.
Why: The INKEEP_ANON_JWT_SECRET correctly enforces 32-char minimum, but customer secrets have no protection. Customers may unknowingly configure weak secrets.
Fix:
| hs256Secret: z.string().optional(), | |
| hs256Secret: z.string().min(32, 'HS256 secret must be at least 32 characters for security').optional(), |
Refs:
- agents-api/src/env.ts:90 — INKEEP_ANON_JWT_SECRET correctly enforces min(32)
| <button | ||
| type="button" | ||
| onClick={() => removeAgent(id)} | ||
| className="hover:text-destructive" | ||
| > | ||
| <X className="size-3" /> | ||
| </button> |
There was a problem hiding this comment.
🟡 Minor: Icon-only button missing accessible label
Issue: The X button for removing agents lacks an aria-label. Screen reader users will hear "button" with no indication of what it does.
Why: Fails WCAG 2.1 SC 4.1.2 (Name, Role, Value). Per web-design-guidelines: "Icon-only buttons need aria-label".
Fix:
| <button | |
| type="button" | |
| onClick={() => removeAgent(id)} | |
| className="hover:text-destructive" | |
| > | |
| <X className="size-3" /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => removeAgent(id)} | |
| className="hover:text-destructive" | |
| aria-label={`Remove ${opt?.label ?? id}`} | |
| > | |
| <X className="size-3" aria-hidden="true" /> | |
| </button> |
Refs:
| <button | ||
| type="button" | ||
| onClick={() => removeAgent(id)} | ||
| className="hover:text-destructive" | ||
| > | ||
| <X className="size-3" /> | ||
| </button> |
There was a problem hiding this comment.
🟡 Minor: Icon-only button missing accessible label (same as app-create-form.tsx)
Issue: The X button for removing agents lacks an aria-label. Screen reader users will hear "button" with no indication of what it does.
Fix:
| <button | |
| type="button" | |
| onClick={() => removeAgent(id)} | |
| className="hover:text-destructive" | |
| > | |
| <X className="size-3" /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => removeAgent(id)} | |
| className="hover:text-destructive" | |
| aria-label={`Remove ${opt?.label ?? id}`} | |
| > | |
| <X className="size-3" aria-hidden="true" /> | |
| </button> |
| const sanitizedData = result.data.map(({ keyHash, tenantId, projectId, ...app }) => app); | ||
|
|
||
| return c.json({ | ||
| data: sanitizedData, | ||
| pagination: result.pagination, | ||
| }); | ||
| } | ||
| ); | ||
|
|
||
| app.openapi( | ||
| createProtectedRoute({ | ||
| method: 'get', | ||
| path: '/{id}', | ||
| summary: 'Get App', | ||
| description: 'Get a specific app credential by ID', | ||
| operationId: 'get-app-by-id', | ||
| tags: ['Apps'], | ||
| permission: requireProjectPermission('view'), | ||
| request: { | ||
| params: TenantProjectIdParamsSchema, | ||
| }, | ||
| responses: { | ||
| 200: { | ||
| description: 'App found', | ||
| content: { | ||
| 'application/json': { | ||
| schema: AppResponse, | ||
| }, | ||
| }, | ||
| }, | ||
| ...commonGetErrorResponses, | ||
| }, | ||
| }), | ||
| async (c) => { | ||
| const { tenantId, projectId, id } = c.req.valid('param'); | ||
| const appRecord = await getAppById(runDbClient)({ | ||
| scopes: { tenantId, projectId }, | ||
| id, | ||
| }); | ||
|
|
||
| if (!appRecord) { | ||
| throw createApiError({ | ||
| code: 'not_found', | ||
| message: 'App not found', | ||
| }); | ||
| } | ||
|
|
||
| const { keyHash: _, tenantId: __, projectId: ___, ...sanitized } = appRecord; | ||
|
|
||
| return c.json({ data: sanitized }); |
There was a problem hiding this comment.
🔴 CRITICAL: Customer HS256 JWT secret exposed in API responses
Issue: The sanitizedData removes keyHash, tenantId, and projectId, but does NOT sanitize the config field. For web_client apps with hs256Enabled: true, the plaintext config.webClient.hs256Secret is returned to any user with view permission.
Why: An attacker with view-only project access can extract the HS256 secret and forge valid end-user JWTs to impersonate any user for that app. This secret should never be returned in API responses.
Fix: Sanitize the config field before returning:
const sanitizeAppConfig = (config: AppConfig): AppConfig => {
if (config.type === 'web_client' && config.webClient.hs256Secret) {
return {
...config,
webClient: { ...config.webClient, hs256Secret: undefined }
};
}
return config;
};
const sanitizedData = result.data.map(({ keyHash, tenantId, projectId, config, ...app }) => ({
...app,
config: sanitizeAppConfig(config)
}));Refs:
agents-api/src/middleware/runAuth.ts
Outdated
| updateAppLastUsed(runDbClient)(app.id).catch((err) => { | ||
| logger.error({ error: err, appId: app.id }, 'Failed to update app lastUsedAt'); | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR: Fire-and-forget DB update without backpressure control
Issue: Under high load, these fire-and-forget updateAppLastUsed calls can accumulate unbounded pending promises, overwhelming the database with UPDATE queries and causing memory pressure.
Why: Since this runs on every authenticated request, a traffic spike could generate thousands of concurrent UPDATE statements, impacting database performance and potentially causing cascading failures.
Fix: Consider batching or sampling these updates:
// Option 1: Sample updates (e.g., 1 in 10 requests)
if (Math.random() < 0.1) {
updateAppLastUsed(runDbClient)(app.id).catch((err) => {
logger.error({ error: err, appId: app.id }, 'Failed to update app lastUsedAt');
});
}
// Option 2: Only update if stale (requires caching last update time)Refs:
agents-api/src/middleware/runAuth.ts
Outdated
| } catch { | ||
| // Anonymous JWT failed — try customer HS256 | ||
| } |
There was a problem hiding this comment.
🟡 Minor: Silent catch on JWT verification hides failure modes
Issue: JWT verification failures are caught with empty catch blocks, discarding the error entirely. This makes it impossible to distinguish between expired tokens (expected), invalid signatures (potential attack), malformed tokens (client bug), or clock skew issues (infrastructure problem).
Why: During incidents where users report auth failures, there's no way to determine the root cause from logs or traces.
Fix: Log JWT verification failures at debug level with error classification:
} catch (err) {
const errorType = err instanceof errors.JWTExpired ? 'expired' :
err instanceof errors.JWTClaimValidationFailed ? 'claim_invalid' :
err instanceof errors.JWSSignatureVerificationFailed ? 'signature_invalid' : 'unknown';
logger.debug({ errorType, appId: appIdHeader, error: err.message }, 'Anonymous JWT verification failed');
// Continue to next auth method
}Refs:
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
Delta Review: This review covers only changes since the last automated review (commit
308d69b19458). The delta includes 1 commit updating Ship modal code snippets to use anonymous session auth flow.
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
js-chat-button.ts:12Code snippets lack error handling for session token fetch (affects all 5 snippet files)
💭 Consider (1) 💭
💭 1) chat-ui-code.tsx:79 Placeholder naming convention inconsistency
Issue: The new snippets use YOUR_APP_ID as the placeholder, while other guides (MCP, API, SDK, Node.js) use INKEEP_APP_SECRET without a YOUR_ prefix.
Why: Creates inconsistent developer experience. The YOUR_ prefix is arguably clearer that replacement is needed.
Fix: Either standardize on YOUR_* prefix across all guides (better UX) or use INKEEP_APP_ID to match existing patterns.
Refs:
- api-guide.tsx:23 — uses
INKEEP_APP_SECRET
🕐 Pending Recommendations (11)
Previous review issues that remain unresolved. These should be addressed before merging:
- 🔴
auth.ts:18Hardcoded development secret is predictable and publicly visible - 🔴
apps.ts:70-119Customer HS256 JWT secret exposed in API responses - 🟠
systemMissing changeset for publishable packages - 🟠
systemMissing conceptual documentation for App Credentials - 🟠
runAuth.ts:546-548JWT verification error handling hides failure classification - 🟠
runtime-schema.ts:179Customer HS256 secret stored in plaintext in database - 🟠
domain-validation.ts:17-20Bare wildcard*silently skipped - 🟠
schemas.ts:1853Customer HS256 secret has no minimum length validation - 🟠
runAuth.ts:597-599Fire-and-forget DB update without backpressure control - 🟡
app-create-form.tsx:226Icon-only button missing accessible label - 🟡 Various Multiple other minor issues from prior review
🚫 REQUEST CHANGES
Summary: The delta changes are clean — the Ship modal code snippets correctly implement the anonymous session auth flow. The string concatenation technique for SESSION_URL is intentional and produces valid JavaScript.
However, prior critical issues from the last review remain unresolved (HS256 secret exposure, hardcoded dev secret, missing changeset). The delta itself is approvable, but the overall PR still has blocking issues that need attention before merging.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
chat-ui-code.tsx:72 |
Session URL construction "malformed" | Intentional design — uses quote characters to produce JS concatenation; valid code generation |
utils.ts:36-38 |
Missing trailing comma in serializeExtraSettings | Not a bug — template structure handles comma placement correctly |
react-component.ts:21 |
Returns null during loading | Common pattern in example code for brevity |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
4 | 0 | 0 | 0 | 1 | 0 | 3 |
pr-review-docs |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
pr-review-consistency |
4 | 0 | 1 | 0 | 0 | 0 | 3 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-product |
4 | 0 | 0 | 0 | 0 | 0 | 4 |
| Total | 15 | 0 | 1 | 0 | 1 | 0 | 13 |
Note: Most findings were duplicates or misunderstandings of the intentional string concatenation technique used for SESSION_URL generation.
| const response = await fetch(SESSION_URL, { method: "POST" }); | ||
| const data = await response.json(); | ||
| return data.token; | ||
| } |
There was a problem hiding this comment.
🟡 Minor: Code snippet lacks error handling for session token fetch
Issue: The getSessionToken() function silently fails if the fetch fails. Developers copying this code will get cryptic errors like "Cannot read property 'token' of undefined" when the session endpoint returns an error.
Why: This is a customer-facing code example — the first code developers copy to integrate the chat widget. Clear error handling helps developers self-diagnose setup issues (invalid App ID, misconfigured domain allowlist, network problems).
Fix: Consider adding response status check:
async function getSessionToken() {
const response = await fetch(SESSION_URL, { method: "POST" });
if (!response.ok) {
throw new Error(`Session token request failed: ${response.status}`);
}
const data = await response.json();
return data.token;
}Refs:
- js-embedded-chat.ts:8-12 — same pattern
- js-sidebar-chat.ts:8-12 — same pattern
- react-component.ts:8-12 — same pattern
- react-sidebar-component.ts:8-12 — same pattern
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low (delta only)
Delta Review: This review covers only changes since the last automated review (commit
26d4295c2cd6). The delta adds comprehensive spec documentation and a cleanup commit.
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
specs/UntitledAccidental stray file should be removed
💭 Consider (1) 💭
💭 1) .gitignore Asymmetric spec tracking
Issue: The gitignore change creates an asymmetry: .claude/specs remains gitignored (line 102) while specs/ is now tracked.
Why: This appears intentional (draft specs in .claude/specs, finalized specs promoted to specs/). If this is the intended workflow, no action needed. If you want to track all specs uniformly, also remove .claude/specs from gitignore.
Refs:
✅ Delta Approvals
The following delta changes are clean:
| Change | Status | Notes |
|---|---|---|
specs/2026-03-02-app-credentials-and-end-user-auth/SPEC.md |
✅ Good | Comprehensive 811-line spec with clear decisions, evidence, and phase planning |
specs/.../evidence/*.md |
✅ Good | 8 well-researched evidence files backing major decisions |
specs/.../meta/_changelog.md |
✅ Good | Proper change tracking for the spec |
agents-manage-ui/src/lib/api/apps.ts |
✅ Good | Correct removal of unused fetchApp export (knip cleanup, no callers) |
.gitignore |
✅ Good | Intentional change to track spec files |
🕐 Pending Recommendations (11)
Prior critical issues from previous reviews remain unresolved. The delta does not address these:
- 🔴
auth.ts:18Hardcoded development secret is predictable and publicly visible - 🔴
apps.ts:70-119Customer HS256 JWT secret exposed in API responses - 🟠
systemMissing changeset for publishable packages - 🟠
systemMissing conceptual documentation for App Credentials - 🟠
runAuth.ts:546-548JWT verification error handling hides failure classification - 🟠
runtime-schema.ts:179Customer HS256 secret stored in plaintext in database - 🟠
domain-validation.ts:17-20Bare wildcard*silently skipped - 🟠
schemas.ts:1853Customer HS256 secret has no minimum length validation - 🟠
runAuth.ts:597-599Fire-and-forget DB update without backpressure control - 🟡
app-create-form.tsx:226Icon-only button missing accessible label - 🟡
js-chat-button.ts:12Code snippets lack error handling for session token fetch
🚫 REQUEST CHANGES
Summary: The delta changes are clean — the spec documentation is comprehensive and well-structured, and the knip cleanup is correct. However, prior critical security issues from previous reviews remain unresolved:
- HS256 secret exposure in API responses — allows JWT forgery by users with view permission
- Hardcoded dev secret — predictable fallback in non-production environments
Additionally, a minor cleanup is needed: remove the accidental specs/Untitled stray file.
The spec documentation itself is excellent — 811 lines with clear decisions, evidence backing, and phase planning. It accurately reflects the implementation.
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
specs/ |
Internal spec out of scope for customer docs | Correct — internal engineering docs, not customer-facing |
.gitignore:102 |
Asymmetric spec tracking | Intentional design — draft vs finalized specs |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-docs |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-devops |
3 | 0 | 1 | 0 | 1 | 0 | 1 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 4 | 0 | 1 | 0 | 1 | 0 | 2 |
Note: Delta scope limited reviewers. Most prior issues were on code outside the delta.
specs/Untitled
Outdated
There was a problem hiding this comment.
🟡 Minor: Accidental stray file committed
Issue: This file appears to be an accidental artifact. It contains only a reference to specs/slack-manifest-single-source/SPEC.md - a path that doesn't exist in the repository.
Why: The filename Untitled and the content (a path to a non-existent spec) suggest this was not intentionally created. It was added in the same commit (30cfa622c) that removed specs/ from .gitignore.
Fix: Remove the stray file before merging:
git rm specs/Untitled
git commit -m "Remove accidental stray file"There was a problem hiding this comment.
Fixed — specs/Untitled has been removed in commit 5aeac12.
There was a problem hiding this comment.
PR Review Summary
(5) Total Issues | Risk: Medium
Delta Review: This review covers only changes since the last automated review (commit
30cfa622c4bb). The delta includes 1 commit (5aeac12e5) titled "Address PR review feedback: security hardening and UX improvements" that addresses most critical/major issues from prior reviews.
✅ Security Fixes Verified
All security fixes from the prior review have been correctly implemented:
| Fix | Status | Location |
|---|---|---|
| JWT error classification | ✅ Implemented | runAuth.ts:546-553, 574-581 |
| JWT audience validation | ✅ Implemented | runAuth.ts:568-570 |
| Error message sanitization | ✅ Implemented | runAuth.ts:600, 608 |
| HS256 secret sanitization | ✅ Implemented | apiKeys.ts:sanitizeAppConfig, applied in all apps.ts routes |
| Dev secret hardening | ✅ Implemented | auth.ts:18 - now uses crypto.randomUUID() + crypto.randomUUID() |
| HS256 min length validation | ✅ Implemented | schemas.ts - .min(32) |
| updateAppLastUsed tenant scoping | ✅ Implemented | apps.ts data-access - uses full composite key with 10% sampling |
| Domain validation wildcard | ✅ Fixed | domain-validation.ts - * now returns true |
| Accessibility labels | ✅ Implemented | Form components have aria-label and aria-hidden |
| Client-side validation | ✅ Aligned | validation.ts refine matches backend |
| Code snippet error handling | ✅ Implemented | All 5 snippets check !response.ok |
🟠⚠️ Major (2) 🟠⚠️
🟠 1) system Missing test coverage for security-critical sanitizeAppConfig function
Issue: The new sanitizeAppConfig function (apiKeys.ts:194-204) is called in 4 places to strip hs256Secret from API responses, but has zero unit test coverage.
Why: This function protects customer JWT signing secrets from exposure. If it were removed, modified, or its call sites regressed, plaintext secrets would leak in API responses. Security-critical sanitization should have explicit test coverage.
Fix: Add unit tests to appCredentials.test.ts:
describe('sanitizeAppConfig', () => {
it('should remove hs256Secret from web_client config', () => {
const app = { config: { type: 'web_client', webClient: { hs256Secret: 'secret', hs256Enabled: true } } };
expect(sanitizeAppConfig(app).config.webClient).not.toHaveProperty('hs256Secret');
});
it('should pass through api config unchanged', () => { /* ... */ });
});Refs:
🟠 2) system Missing test coverage for JWT audience validation
Issue: The new JWT audience validation (runAuth.ts:568-570) that prevents cross-app token reuse is not tested.
Why: This validation prevents JWTs issued for one app from authenticating to a different app. Without test coverage, the security boundary could regress undetected.
Fix: Add test case in app-credential-auth.test.ts:
it('should reject customer JWT when aud claim does not match app ID', async () => {
// Setup app with HS256 enabled
jwtVerifyMock.mockResolvedValueOnce({ payload: { sub: 'user', aud: 'app_different_app_id' } });
// Assert 401 with 'JWT audience does not match app'
});Refs:
🟡 Minor (1) 🟡
🟡 1) agents-api/src/__tests__/run/middleware/app-credential-auth.test.ts:599 Missing test for sampling exclusion path
Issue: The sampling test only verifies Math.random() < 0.1 triggers the update. There's no test confirming updateAppLastUsed is NOT called when sampling excludes the request.
Why: Without this test, the sampling condition could be changed to always fire (e.g., < 1.0) without detection.
Fix: Add complementary test with Math.random() returning 0.5 and asserting updateAppLastUsed is not called.
Refs:
💭 Consider (1) 💭
💭 1) agents-manage-ui/src/components/apps/form/app-update-form.tsx:101 Update flow sends undefined hs256Secret back to server
Issue: Line 101 sends hs256Secret: webConfig?.hs256Secret in the update payload. Since API responses no longer include hs256Secret (thanks to sanitization), this will always be undefined after fetching the app.
Why: If the backend merges this undefined value into the config, it could clear the stored secret on any app update. This is a functional regression (broken HS256 auth after editing), not a security issue (secret is never exposed).
Fix: Backend should preserve existing hs256Secret when not provided in update payload:
hs256Secret: body.config?.webClient?.hs256Secret ?? existingApp.config.webClient?.hs256SecretRefs:
🕐 Pending Recommendations (2)
Prior review issues that remain unresolved:
- 🟠
systemMissing changeset for publishable packages — changeset-bot flagged this PR as needing a changeset - 🟠
systemMissing conceptual documentation for App Credentials — docs explaining the new auth model (when to use Apps vs API keys, web_client vs api types, anonymous session flow)
💡 APPROVE WITH SUGGESTIONS
Summary: The delta successfully addresses all critical and major security issues from the prior review. The security fixes are correctly implemented:
- JWT secrets are properly sanitized from all API responses
- Error messages no longer leak internal details
- Dev secret is now random per startup
- Audience validation prevents cross-app token reuse
- Tenant scoping is enforced with sampling to reduce DB load
The remaining concerns are test coverage gaps for the new security functions and the still-pending changeset/docs. The functional regression around update flow clearing hs256Secret should be addressed but is not blocking since it's a UX issue rather than a security issue.
Recommended next steps:
- Add unit tests for
sanitizeAppConfig(2 test cases) - Add integration test for JWT audience validation
- Create changeset:
pnpm bump minor --pkg agents-api --pkg agents-core "Add App Credentials and end-user authentication support" - Consider adding conceptual docs (can be follow-up)
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
validation.ts |
Frontend domain validation empty string handling | Reviewer self-corrected: validation is actually correct (empty string filters to [], which fails >= 1 check) |
app-credential-display.tsx:34 |
Escape prevention trapping users | Acceptable UX tradeoff: Done button is accessible via keyboard, and preventing accidental dismissal of one-time secret is important |
| Various snippet files | Error handling added | Positive change, correctly implemented |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
1 | 0 | 1 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
4 | 2 | 0 | 0 | 0 | 0 | 1 |
pr-review-frontend |
6 | 0 | 0 | 0 | 0 | 0 | 5 |
| Total | 11 | 2 | 1 | 0 | 0 | 0 | 6 |
Note: Most frontend findings were confirmations that the accessibility/validation fixes are correctly implemented.
|
@claude for names like
|
|
Claude finished @robert-inkeep's task in 24s —— View job The codebase convention is Looking at the patterns across the codebase:
The general pattern is:
Since |
|
@claude is this backward compatible? what happens if i'm already using api key and i have it connected to a widget |
|
Claude finished @robert-inkeep's task in 31s —— View job Backward Compatibility Analysis
Yes, this is fully backward compatible. Existing API key auth is completely untouched. Here's how the auth flow works (see The new app credential path is only triggered when the
So if you're using an API key connected to a widget today:
The only behavioral difference is a new exclusivity rule: if someone does send TL;DR: No changes needed. Your existing API key + widget setup will continue to work exactly as before. |
amikofalvy
force-pushed
the
implement/app-credentials-end-user-auth
branch
from
5aeac12 to
8f7502b
Compare
- Add INKEEP_ANON_JWT_SECRET and INKEEP_POW_HMAC_SECRET to all generate-secrets test cases so they don't fail on the "not found" append path. - Add a simple friendly-agent to the activities-planner project that works without MCP tools (for CI E2E chat tests). - Point Cypress E2E agent ID to friendly-agent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review (scope:
delta): This review covers only changes since the last automated review (commitf65904f193a4). The delta is 1 commit with 4 files focused on CI/E2E test infrastructure improvements.
🔴❗ Critical (0) ❗🔴
No critical issues in the delta. ✅
🟠⚠️ Major (0) 🟠⚠️
No major issues in the delta. ✅
🟡 Minor (0) 🟡
No minor issues in the delta. ✅
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
generate-secrets.test.ts:276Add assertions for new secret preservation
✅ Delta Assessment
The delta correctly sets up CI infrastructure for E2E testing of the App Credentials feature:
| File | Change | Assessment |
|---|---|---|
.github/composite-actions/cypress-e2e/action.yml |
CYPRESS_E2E_AGENT_ID: friendly-agent |
✅ CORRECT — Points to new simple agent without external tool dependencies |
agents/friendly-agent.ts |
New 16-line agent definition | ✅ CORRECT — Minimal agent for reliable CI testing (no MCP tools needed) |
activities-planner/index.ts |
Register friendlyAgent in project |
✅ CORRECT — Standard SDK pattern |
generate-secrets.test.ts |
Test 7 secrets (was 5) | ✅ CORRECT — Covers INKEEP_ANON_JWT_SECRET and INKEEP_POW_HMAC_SECRET |
Technical verification:
-
Agent design — The
friendly-agentis intentionally simple: no external tools, no MCP dependencies, just a system prompt. This makes it ideal for CI where external service availability shouldn't cause test flakiness. -
Test coverage — All 6 test cases now include the two new secrets in their
.envmock content and assertions. The implementation correctly generates 32-byte hex strings for both secrets. -
CI workflow — Changing from
activities-plannertofriendly-agentreduces CI surface area since the latter doesn't require weather/Exa MCP tools to be available.
📊 Prior Review Summary
This PR has been through 20+ review iterations with all issues addressed:
- ✅ IDOR vulnerabilities — FIXED (tenant-scoped data access functions)
- ✅ Missing changeset — FIXED (
objective-moccasin-kangaroo.md) - ✅ Self-hosted template env vars — FIXED (all 5 new vars documented)
- ✅ Security hardening — IMPLEMENTED (JWT sanitization, error handling)
- ✅ Accessibility — IMPLEMENTED (aria labels, keyboard handling)
- ✅ PoW error standardization — IMPLEMENTED (400 status, human-readable messages)
- ✅ Setup script secrets — IMPLEMENTED (auto-generates new secrets)
✅ APPROVE
Summary: The delta adds reliable CI test infrastructure by introducing a simple test agent without external dependencies and updating test coverage for the new App Credentials secrets. All changes are clean and correct. This PR is ready to merge! 🎉
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
generate-secrets.test.ts:223-236 |
"append secrets" test missing assertions for new secrets | LOW confidence, INFO severity. The loop-based implementation means if appending works for one secret it works for all — existing test coverage is sufficient. |
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
2 | 0 | 0 | 0 | 1 | 0 | 1 |
| Total | 2 | 0 | 0 | 0 | 1 | 0 | 1 |
| 'INKEEP_AGENTS_TEMP_JWT_PRIVATE_KEY=my-real-private-key', | ||
| 'INKEEP_AGENTS_TEMP_JWT_PUBLIC_KEY=my-real-public-key', | ||
| 'INKEEP_AGENTS_JWT_SIGNING_SECRET=my-custom-signing-secret', | ||
| 'BETTER_AUTH_SECRET=my-real-secret', |
There was a problem hiding this comment.
💭 Consider — Add assertions for new secret preservation
Issue: The test sets INKEEP_ANON_JWT_SECRET and INKEEP_POW_HMAC_SECRET in process.env (lines 266-267) and adds them to .env content (lines 275-276), but doesn't explicitly assert these values are preserved after runSetup.
Why: While the implementation doesn't modify process.env after reading it (so this is low-risk), explicit assertions would catch any future regression where user-provided values are accidentally overwritten in memory.
Fix: Consider adding assertions after line 287:
expect(process.env.INKEEP_ANON_JWT_SECRET).toBe('my-custom-anon-secret');
expect(process.env.INKEEP_POW_HMAC_SECRET).toBe('my-custom-pow-secret');Refs:
- Write ANTHROPIC_API_KEY from workflow env into .env before starting the API server, so the agent can make LLM calls during E2E tests. - Update generate-secrets test to include new PoW/anon secrets in all test cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review Scope
This review covers the delta since the last automated review (commit d658e1239dcc). The delta consists of 1 commit (05d50323f) modifying 1 file:
| File | Change |
|---|---|
.github/composite-actions/cypress-e2e/action.yml |
CI infrastructure: Write ANTHROPIC_API_KEY to .env for E2E tests |
Delta Analysis
The change adds a step to the Cypress E2E composite action that writes ANTHROPIC_API_KEY to the .env file before starting the API server. This is necessary because:
- The API server reads environment from
.envvialoadEnvironmentFiles() - Exporting to
$GITHUB_ENVwould leak into the Next.js build step (as documented in the code comment) - The
sedreplacement correctly handles the placeholder pattern in.env
Security: The secret is passed via GitHub Actions secrets and written directly to .env without logging the value.
Full PR Summary
This PR implements a comprehensive App Credentials & End-User Authentication system:
- ✅ App Credentials - New resource type (
web_client,api) for public client authentication - ✅ Anonymous JWT Sessions - HS256-signed tokens for end-user identification
- ✅ ALTCHA Proof-of-Work - Optional bot protection for session endpoint
- ✅ Origin/Domain Validation - Wildcard-aware CORS-like validation
- ✅ End-User Conversation Scoping - IDOR prevention via userId filtering
- ✅ Full CRUD UI - Manage UI with two-step creation wizard
The PR has undergone 20+ review iterations with all critical and major issues addressed.
✅ APPROVE
Summary: The delta contains only a CI infrastructure fix for E2E testing. All prior review feedback has been addressed. The app credentials feature is well-architected with proper security controls, tenant isolation, and comprehensive test coverage. Ship it! 🚀
Reviewers (12)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-architecture |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-frontend |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-breaking-changes |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-types |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-errors |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-docs |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-devops |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-sre |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta review scope - only 1 CI infrastructure commit since last automated review. All 12 reviewers correctly identified no issues in the delta.
Ito Test Report ✅19 test cases ran. 19 passed. ✅ Verified app management, session/auth, conversation scoping, and mobile flows against the implementation, with passed outcomes showing expected behavior across routing, CRUD, boundary checks, and responsive UI interactions. ✅ Passed (19)
📋 View Recording |
Ito Test Report ❌17 test cases ran. 16 passed, 1 failed. ✅ The run validated most Apps UI, API contract, auth edge-case, and isolation behaviors successfully. 🔍 Verification found one confirmed product bug in the Ship flow where opening the Chat UI guide crashes due to invalid form component composition. ✅ Passed (16)
❌ Failed (1)
API Keys page and Ship modal point users to Apps – Failed
📋 View Recording |
Replace FormLabel with Label for standalone form items (color picker, switch toggle) that aren't wrapped in a FormField component. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Inkeep widget library now handles PoW and session token management internally. Remove all authentication boilerplate from React and JS code snippets, replacing with simple appId + baseUrl top-level props. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move conditional expression out of try/catch to avoid React Compiler's unsupported value block limitation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review Scope
This review covers the delta since the last automated review (commit 05d50323f5d6). The delta consists of 24 commits but only 4 PR-specific commits with meaningful changes:
| Commit | Change |
|---|---|
59f772764 |
Simplify Ship modal code samples to only require appId and baseUrl |
b0e069b02 |
Fix runtime error: FormLabel used outside FormField |
2f927ca09 |
Add defaultProjectId auto-set logic when agent is selected |
c8b876593 |
Add agents-api changeset |
The remaining commits are lockfile updates and main branch merges (shadcn skills, context compression, UI improvements).
🔴❗ Critical (0) ❗🔴
No critical issues in the delta. ✅
🟠⚠️ Major (0) 🟠⚠️
No major issues in the delta. ✅
🟡 Minor (0) 🟡
No minor issues in the delta. ✅
💭 Consider (0) 💭
No actionable nitpicks in the delta. ✅
✅ Delta Verification
| Change | Status | Assessment |
|---|---|---|
| Ship modal simplification | ✅ VALID | Snippets reduced from ~70 to ~15 lines each. Now use {appId, baseUrl} interface, assuming @inkeep/agents-ui SDK handles session/PoW internally. Pattern is consistent across all 5 templates. |
| FormLabel → Label fix | ✅ CORRECT | FormLabel requires useFormField() context which throws when missing. Using base Label for standalone form items follows established codebase patterns (see edit-credential-form.tsx, simulation-agent-form.tsx). |
| defaultProjectId auto-set | ✅ WELL-TESTED | Logic ensures defaultProjectId is set when defaultAgentId is provided, cleared when agent is removed. 4 new test cases cover all branches (create with/without agent, update with/without agent). |
| Changeset added | ✅ CORRECT | parliamentary-tan-lion.md properly declares @inkeep/agents-api: patch for the conversation history endpoint. |
📋 Documentation Note
The Ship modal now generates simplified SDK snippets (appId + baseUrl only), while app-credentials.mdx still documents the manual PoW/session flow. This appears intentional — the docs explain the underlying mechanism while the SDK abstracts it. No action needed unless SDK compatibility is a concern.
✅ APPROVE
Summary: The delta contains clean, well-tested changes. The Ship modal simplification improves developer UX, the FormLabel fix resolves a runtime error following established patterns, and the defaultProjectId logic has comprehensive test coverage. All prior review feedback has been addressed. Ship it! 🚢
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
| Total | 3 | 0 | 0 | 0 | 0 | 0 | 3 |
Note: All 3 consistency findings were INFO-level observations (correct patterns, well-tested code, SDK assumptions) — no actionable issues.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
chat-ui-preview-form.tsx:35 |
Label usage pattern | INFO: Validated as correct — follows established codebase conventions |
apps.ts:166 |
defaultProjectId pattern | INFO: Novel but well-tested with 4 new test cases |
snippets/*.ts |
SDK interface assumption | INFO: External SDK — cannot verify compatibility in this repo |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review Scope
This review covers the delta since the last automated review (commit 3872db2a55a1). The delta consists of 3 commits but contains no PR-specific code changes:
| Commit | Change | Assessment |
|---|---|---|
61fb1fa95 |
Merge branch 'main' into implement/app-credentials-end-user-auth | ✅ Merge commit only |
7c764aaf5 |
Version Packages (#2614) | ✅ Automated version bumps (0.58.6 → 0.58.7) |
92a676b1b |
Fix React Compiler error in mcp-node-editor try/catch block | ✅ Valid fix (see below) |
Delta Files Changed (12)
All but one file are automated version bumps from the Version Packages PR:
.changeset/young-pans-see.md(deleted)agents-api/package.json,agents-cli/package.json,agents-manage-ui/package.jsonpackages/agents-core/package.json,packages/agents-email/package.json,packages/agents-mcp/package.jsonpackages/agents-sdk/package.json,packages/agents-work-apps/package.jsonpackages/ai-sdk-provider/package.json,packages/create-agents/package.json
One functional change:
agents-manage-ui/src/components/agent/sidepane/nodes/mcp-node-editor.tsx— React Compiler fix
🔴❗ Critical (0) ❗🔴
No critical issues in the delta. ✅
🟠⚠️ Major (0) 🟠⚠️
No major issues in the delta. ✅
🟡 Minor (0) 🟡
No minor issues in the delta. ✅
💭 Consider (0) 💭
No actionable nitpicks in the delta. ✅
✅ Delta Verification
| Change | Status | Assessment |
|---|---|---|
| React Compiler fix | ✅ CORRECT | The change extracts value.trim() to a const trimmed variable before the try/catch block. This satisfies React Compiler's requirement that expressions used inside try/catch blocks must not be evaluated as part of the try statement itself. The behavior is unchanged — trimmed is still evaluated from value.trim() and used in the subsequent conditional. |
| Version bumps | ✅ EXPECTED | Standard automated version package release from main branch merge. |
📋 Full PR Status
This PR has been through 25+ review iterations with all critical and major issues addressed:
- ✅ IDOR vulnerabilities — FIXED (tenant-scoped data access functions)
- ✅ Missing changeset — FIXED (
objective-moccasin-kangaroo.md,parliamentary-tan-lion.md) - ✅ Self-hosted template env vars — FIXED (all 5 new vars documented)
- ✅ Security hardening — IMPLEMENTED (JWT sanitization, error handling)
- ✅ Accessibility improvements — IMPLEMENTED (aria labels, keyboard handling)
- ✅ PoW error standardization — IMPLEMENTED (400 status codes, human-readable messages)
- ✅ FormLabel runtime error — FIXED (uses base
Labelcomponent) - ✅ Ship modal simplification — COMPLETED (reduced snippet complexity)
✅ APPROVE
Summary: The delta contains only automated version bumps from a main branch merge and one valid React Compiler fix. All prior review feedback from 25+ iterations has been addressed. The App Credentials & End-User Authentication feature is well-architected with proper security controls, tenant isolation, and comprehensive test coverage. This PR is ready to merge! 🚀
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 subagent reviewers were dispatched for this delta review as changes were limited to automated version bumps and a trivial React Compiler fix.
Move conditional expression out of try/catch in handleHeadersChange to avoid React Compiler's unsupported value block limitation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review Scope
This review covers the delta since the last automated review (commit 61fb1fa95). The delta consists of 1 commit modifying 1 file:
| Commit | Change |
|---|---|
96412cea0 |
Fix React Compiler error in mcp-node-editor handleHeadersChange |
Delta Analysis
The change refactors handleHeadersChange to satisfy React Compiler requirements:
Before:
try {
const parsedHeaders = trimmed === '' ? {} : JSON.parse(value);
// ...
}After:
if (trimmed === '') {
updateNodeData(selectedNode.id, {
...selectedNode.data,
tempHeaders: {},
});
markUnsaved();
return;
}
try {
const parsedHeaders = JSON.parse(value);
// ...
}Assessment: ✅ CORRECT
The fix extracts the empty string check (trimmed === '') outside the try/catch block with an early return. This satisfies React Compiler's requirement that expressions evaluated inside try/catch blocks must not contain conditional logic that could be hoisted. The behavior is functionally identical — empty input still results in tempHeaders: {} being saved.
🔴❗ Critical (0) ❗🔴
No critical issues in the delta. ✅
🟠⚠️ Major (0) 🟠⚠️
No major issues in the delta. ✅
🟡 Minor (0) 🟡
No minor issues in the delta. ✅
💭 Consider (0) 💭
No actionable suggestions in the delta. ✅
📊 Full PR Summary
This PR implements a comprehensive App Credentials & End-User Authentication system:
- ✅ App Credentials — New resource type (
web_client,api) for public client authentication - ✅ Anonymous JWT Sessions — HS256-signed tokens for end-user identification
- ✅ ALTCHA Proof-of-Work — Optional bot protection for session endpoint
- ✅ Origin/Domain Validation — Wildcard-aware CORS-like validation
- ✅ End-User Conversation Scoping — IDOR prevention via userId filtering
- ✅ Full CRUD UI — Manage UI with app management
The PR has undergone 26+ review iterations with all critical and major issues addressed.
✅ APPROVE
Summary: The delta contains only a React Compiler compliance fix — extracting an empty-string check outside a try/catch block with early return. Behavior is unchanged; the refactor satisfies compiler constraints. All prior review feedback has been addressed. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| (none dispatched — trivial delta) | — | — | — | — | — | — | — |
Note: No sub-reviewers were dispatched for this delta review as the change is a single-file React Compiler compliance fix with no security, architecture, or product implications.
React Compiler doesn't support any value blocks (conditionals, logical expressions) inside try/catch. Keep only JSON.parse in the try block. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review Scope
This review covers the delta since the last automated review (commit 96412cea0). The delta consists of 1 commit modifying 1 file:
| Commit | Change |
|---|---|
9b9b9236e |
Move all conditional logic out of try/catch in handleHeadersChange |
Delta Analysis
The change completes the React Compiler refactor for handleHeadersChange:
Before:
try {
const parsedHeaders = JSON.parse(value);
if (typeof parsedHeaders === 'object' && ...) {
updateNodeData(...);
markUnsaved();
}
} catch {
// Invalid JSON - don't save
}After:
let parsedHeaders: unknown;
try {
parsedHeaders = JSON.parse(value);
} catch {
return;
}
if (typeof parsedHeaders === 'object' && ...) {
updateNodeData(...);
markUnsaved();
}Assessment: ✅ CORRECT
The fix extracts the parsedHeaders variable declaration outside the try/catch block and moves all conditional logic (the if block with updateNodeData) after the try/catch with an early return on parse failure. This satisfies React Compiler's requirement that expressions inside try/catch blocks must not contain conditional logic that could be hoisted.
Behavior is functionally identical:
- Empty input (
trimmed === '') → savestempHeaders: {} - Valid JSON object → saves the parsed object
- Invalid JSON → early return, no save (allows user to continue typing)
- Valid JSON but non-object (array, primitive) → no save
🔴❗ Critical (0) ❗🔴
No critical issues in the delta. ✅
🟠⚠️ Major (0) 🟠⚠️
No major issues in the delta. ✅
🟡 Minor (0) 🟡
No minor issues in the delta. ✅
💭 Consider (0) 💭
No actionable suggestions in the delta. ✅
📊 Full PR Summary
This PR implements a comprehensive App Credentials & End-User Authentication system:
- ✅ App Credentials — New resource type (
web_client,api) for public client authentication - ✅ Anonymous JWT Sessions — HS256-signed tokens for end-user identification
- ✅ ALTCHA Proof-of-Work — Optional bot protection for session endpoint
- ✅ Origin/Domain Validation — Wildcard-aware CORS-like validation
- ✅ End-User Conversation Scoping — IDOR prevention via userId filtering
- ✅ Full CRUD UI — Manage UI with app management, credential display, and Ship modal integration
- ✅ Documentation —
app-credentials.mdxwith usage examples and security model - ✅ Comprehensive Test Coverage — 45+ new tests across data access, middleware, routes, and E2E
The PR has undergone 27+ review iterations with all critical and major issues addressed.
✅ APPROVE
Summary: The delta contains a clean mechanical fix to satisfy React Compiler requirements. All conditional logic has been correctly moved outside the try/catch block while preserving identical behavior. The full PR is well-architected with proper security controls, tenant isolation, and comprehensive test coverage. Ship it! 🚀
Discarded (0)
No discarded findings — delta was clean.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| N/A | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: No subagent reviewers dispatched — delta was a single-file mechanical refactor (React Compiler fix) with no functional changes.
|
🔎💬 Inkeep AI search and chat service is syncing content for source 'Inkeep Agent Framework Docs' |
Ito Test Report ❌14 test cases ran. 13 passed, 1 failed. ✅ Core Apps UI, auth hardening, and navigation resilience behaviors passed verification. 🔍 One confirmed defect remains in the run chat compatibility path where a handled route-level failure is returned as an internal server error. ✅ Passed (13)
❌ Failed (1)
Data stream chat route remains compatible with app auth – Failed
📋 View Recording |
2 participants
Summary
Phase 1: App Credentials & End-User Authentication
tenantId/projectId/defaultAgentId, removedpublicId/agentAccessMode/allowedAgentIdskeyHash/keyPrefix(no app secrets), removedauthMode/hs256Enabled/hs256Secretfrom WebClientConfig — apps only support anonymous JWT sessions for nowanonymousSessionLifetimeSecondswithINKEEP_ANON_SESSION_LIFETIME_SECONDSenv var (default 86400s)tryAppCredentialAuthto anonymous JWT verification only; removed API-type app secret auth path and HS256 customer JWT fallbackPOST /run/auth/apps/{appId}/anonymous-sessionissues HS256 JWTs with origin validationGET /run/v1/conversationsscoped by anonymous user identityPOST/GET/PUT/DELETE) under/manage/tenants/{tid}/projects/{pid}/apps*.example.com), port-aware matchingappstable (properly generated viadrizzle-kit)Phase 2: ALTCHA Proof-of-Work for Run API
Spec:
specs/2026-03-06-altcha-pow/SPEC.mdGates all run API requests from
web_clientapps with PoW challenges, enabled globally byINKEEP_POW_HMAC_SECRETenv var.INKEEP_POW_HMAC_SECRET(enables PoW),INKEEP_POW_DIFFICULTY(default 50K),INKEEP_POW_CHALLENGE_TTL_SECONDS(default 300s)captchaEnabledfromWebClientConfig; derivedWebClientConfig,ApiConfig,AppConfigtypes viaz.infer<>instead of manual duplicationpackages/agents-core/src/utils/pow.ts—isPoWEnabled()andverifyPoW(request, hmacSecret)usingaltcha-lib(>= 1.4.1, CVE-2025-68113 fix)GET /run/auth/pow/challenge— public (noAuth()), returns{ algorithm, challenge, maxnumber, salt, signature }when enabled, 404 when disabledverifyPoW()intryAppCredentialAuth()— runs before JWT verification (fail-fast), only forweb_clienttype apps;apitype unaffectedverifyPoW()after origin validation, before JWT issuance; returns 400 with{ error: 'pow_required' | 'pow_invalid' }captchaEnabledtoggle from app create/update formsgetPowHeaders()helper showing challenge fetch -> solve -> attach flow withX-Inkeep-Challenge-SolutionheaderX-Inkeep-AltchatoX-Inkeep-Challenge-Solutionfor implementation-agnostic namingNot in scope (deferred): replay protection (challenge expiry is sole temporal bound), widget/client changes (separate repo), database migrations (captchaEnabled is in JSONB)
Test plan
Phase 1
Phase 2 (PoW)
verifyPoW()unit tests (8 tests) — disabled path, missing header, invalid solution, valid solutionpnpm lintpasses (14/14)Generated with Claude Code