Repository navigation
feat: add connection-backed skill runtime - #35
Closed
kentcdodds wants to merge 11 commits into
Closed
kentcdodds wants to merge 11 commits into
kentcdodds wants to merge 11 commits into
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces a comprehensive provider connections system enabling secure OAuth-based credential management. It adds database tables for connection drafts and persistent connections, a connection service handling setup/OAuth/finalization workflows, MCP capabilities for connection operations, encrypted secret storage, skill-to-connection bindings, generated UI app sessions, and corresponding API endpoints. Changes
Sequence DiagramsequenceDiagram
participant User
participant Client as Generated UI Client
participant Server as Backend API
participant Provider as OAuth Provider
participant DB as Database
rect rgba(100, 150, 200, 0.5)
Note over User,DB: Connection Setup & OAuth Flow
User->>Client: Request provider connection
Client->>Server: POST /api/connections/begin-setup<br/>(provider, auth spec)
Server->>DB: Insert connection draft<br/>(initial status based on strategy)
Server-->>Client: setup_id, secret fields, instructions
alt OAuth Strategy
Client->>Server: POST /api/connections/start-oauth<br/>(setup_id)
Server->>DB: Fetch draft + stored secrets
Server->>Server: Generate OAuth state token<br/>+ PKCE challenge
Server->>DB: Save OAuth state in draft
Server-->>Client: authorize_url, setup_id
Client->>Provider: Redirect to authorization
User->>Provider: Approve authorization
Provider->>Server: Callback: /api/connections/oauth/callback<br/>(code, state)
Server->>Server: Verify signed state
Server->>Provider: Exchange code for tokens
Provider-->>Server: access_token, refresh_token
Server->>DB: Store encrypted secrets for draft
Server->>DB: Update draft status→authorized
Server-->>Client: HTML success response
else Non-OAuth Strategy
Client->>Server: POST /api/connections/secrets<br/>(setup_id, secret fields)
Server->>DB: Store encrypted secrets
Server->>DB: Check if all required secrets present
Server-->>Client: Stored secrets, missing secrets
end
end
rect rgba(150, 100, 200, 0.5)
Note over User,DB: Connection Finalization
Client->>Server: POST /api/connections/finalize<br/>(setup_id)
Server->>DB: Fetch draft + secrets
Server->>Provider: Verify connection<br/>(via verification request)
Provider-->>Server: account_id, scope_set
Server->>DB: Insert provider_connection<br/>(with verified metadata)
Server->>DB: Store encrypted secrets<br/>in provider_connection_secrets
Server->>DB: Mark draft completed
Server-->>Client: connection_id, account details, scopes
end
rect rgba(200, 150, 100, 0.5)
Note over User,DB: Later: Use Connection for Requests
Client->>Server: invoke_action({ code, params })<br/>with appSession
Server->>Server: Resolve connection by handle<br/>(verify user ownership)
Server->>DB: Fetch connection + encrypted secrets
Server->>Server: Decrypt secrets<br/>Apply auth transport header
Server->>Provider: Execute authenticated request<br/>(HTTP or GraphQL)
Provider-->>Server: Response
Server->>DB: Update connection<br/>last_used_at, token_expires_at
Server-->>Client: result or error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/client/mcp-apps/generated-ui-shell.ts (1)
683-759:⚠️ Potential issue | 🔴 CriticalCapture
appSessionper render instead of readinglatestEnvelopeafterawait.
renderEnvelope()updateslatestEnvelope, awaitsresolveSavedAppCode(), and then the document builders pulllatestEnvelope?.appSessionagain. If another render arrives before the first fetch resolves, the older app can be mounted with the newer app’s session token/endpoints. That is both a stale-render bug and a cross-session leak.Proposed fix
let latestRenderData: RenderDataEnvelope | undefined let latestEnvelope: RenderEnvelope | null = null +let renderVersion = 0 -function buildInlineModuleSource(code: string) { +function buildInlineModuleSource( + code: string, + appSession: AppSessionEnvelope | null, +) { const safeCode = escapeInlineScriptSource(code) - const appSession = latestEnvelope?.appSession ?? null return ` ${buildChildBridgeRuntimeSource(appSession)} ${safeCode} `.trim() } -function buildHtmlDocumentFromFragment(code: string) { +function buildHtmlDocumentFromFragment( + code: string, + appSession: AppSessionEnvelope | null, +) { const theme = coerceTheme(latestRenderData?.theme) - const appSession = latestEnvelope?.appSession ?? null return ` <!doctype html> ... `.trim() } -function setFrameSource(code: string, runtime: AppRuntime) { +function setFrameSource( + code: string, + runtime: AppRuntime, + appSession: AppSessionEnvelope | null, +) { if (runtime === 'javascript') { - const inlineModuleSource = buildInlineModuleSource(code) + const inlineModuleSource = buildInlineModuleSource(code, appSession) ... } ... - : buildHtmlDocumentFromFragment(code) + : buildHtmlDocumentFromFragment(code, appSession) frameElement.srcdoc = htmlSource } -async function resolveSavedAppCode(appId: string) { - const appSession = latestEnvelope?.appSession ?? null +async function resolveSavedAppCode( + appId: string, + appSession: AppSessionEnvelope | null, +) { ... } async function renderEnvelope(envelope: RenderEnvelope | null) { latestEnvelope = envelope + const renderId = ++renderVersion ... try { - const resolved = await resolveSavedAppCode(envelope.appId) - setFrameSource(resolved.code, resolved.runtime) + const appSession = envelope.appSession ?? null + const resolved = await resolveSavedAppCode(envelope.appId, appSession) + if (renderId !== renderVersion) return + setFrameSource(resolved.code, resolved.runtime, appSession) } catch (error) { ... } }Also applies to: 761-788, 812-840
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/client/mcp-apps/generated-ui-shell.ts` around lines 683 - 759, The builders currently read latestEnvelope?.appSession at render time (after awaits), causing cross-session leaks; capture the current appSession before any await in renderEnvelope and thread that captured value into the document builders instead of accessing latestEnvelope inside buildInlineModuleSource, buildHtmlDocumentFromFragment and setFrameSource (and any other builders at lines ~761-788 and ~812-840). Change signatures to accept an appSession parameter (e.g., buildInlineModuleSource(code, appSession), buildHtmlDocumentFromFragment(code, appSession)) and use the passed appSession when calling buildChildBridgeRuntimeSource or buildHeadInjection, and update renderEnvelope to pass the captured appSession into setFrameSource/buildHtmlDocumentFromFragment so each render uses its own session token.
🧹 Nitpick comments (10)
packages/worker/src/mcp/capabilities/unified-search.ts (1)
135-137: Parseconnection_bindingsonly whendetailis requested.Line 135 currently does parsing for every hit, including summary mode where the value is unused. This is a small hot-path optimization.
♻️ Proposed refactor
- const parameters = parseSkillParameters(row.parameters) - const connectionBindings = parseSkillConnectionBindings( - row.connection_bindings, - ) + const parameters = parseSkillParameters(row.parameters) const base: SkillSearchHitSummary = { type: 'skill', skillId: row.id, @@ } if (detail) { + const connectionBindings = parseSkillConnectionBindings( + row.connection_bindings, + ) const hit: SkillSearchHitDetail = { ...base, inferredCapabilities: inferred, usesCapabilities: uses, searchText: row.search_text, parameters, connectionBindings, } return hit }Also applies to: 154-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/unified-search.ts` around lines 135 - 137, Only parse row.connection_bindings when the caller requested detail: replace the unconditional call to parseSkillConnectionBindings(row.connection_bindings) with a conditional/lazy parse that sets connectionBindings = detail ? parseSkillConnectionBindings(row.connection_bindings) : undefined (or only assign when detail is truthy) so summary-mode hits avoid the work; apply the same change to the second occurrence around the other hit-processing block (the code regions currently using connectionBindings and parseSkillConnectionBindings) to ensure both hot-paths skip parsing when detail is false.packages/worker/src/mcp/generated-ui-app-session.ts (1)
40-53: Consider normalizingbaseUrlto prevent double-slash issues.If
baseUrlalready ends with a trailing slash,new URL('/api/...', appBaseUrl)handles it correctly. However, callingnew URL(baseUrl)on line 40 without using it as the base directly in subsequent calls is redundant—you could simplify by usingbaseUrldirectly as the second argument.💡 Optional: Simplify URL construction
- const appBaseUrl = new URL(baseUrl) return { sessionId, token, expiresAt: new Date(expiresAtMs).toISOString(), endpoints: { - appSource: new URL('/api/generated-ui/app-source', appBaseUrl).toString(), - action: new URL('/api/generated-ui/actions', appBaseUrl).toString(), - secureInput: new URL( - '/api/generated-ui/secure-input', - appBaseUrl, - ).toString(), + appSource: new URL('/api/generated-ui/app-source', baseUrl).toString(), + action: new URL('/api/generated-ui/actions', baseUrl).toString(), + secureInput: new URL('/api/generated-ui/secure-input', baseUrl).toString(), }, } satisfies GeneratedUiAppSession🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/generated-ui-app-session.ts` around lines 40 - 53, The code creates appBaseUrl via new URL(baseUrl) then builds endpoints with new URL('/api/…', appBaseUrl), which is fine but redundant and can cause subtle double-slash edge cases if baseUrl includes a trailing slash; update the GeneratedUiAppSession builder to normalize baseUrl first (trim a trailing slash) or construct each endpoint by passing baseUrl string as the base to new URL (use baseUrl normalized) so appBaseUrl is not misused—adjust the logic around appBaseUrl, baseUrl, and the endpoints keys (appSource, action, secureInput) to ensure consistent URL construction and remove the unnecessary new URL(baseUrl) where appropriate.packages/worker/src/mcp/connections/connection-handles.ts (1)
14-31: Consider validatingttlMsto prevent negative or zero TTL values.If a caller passes a negative or zero
ttlMs, the handle would be immediately expired or have unexpected behavior. While unlikely in practice, defensive validation could prevent subtle bugs.💡 Optional: Add TTL validation
export async function createConnectionHandle( env: Env, input: { connectionId: string userId: string providerKey: string ttlMs?: number }, ) { + const ttl = input.ttlMs ?? defaultConnectionHandleTtlMs + if (ttl <= 0) { + throw new Error('ttlMs must be positive') + } const now = Date.now() return signToken(env, connectionHandlePurpose, { connection_id: input.connectionId, user_id: input.userId, provider_key: input.providerKey, iat: now, - exp: now + (input.ttlMs ?? defaultConnectionHandleTtlMs), + exp: now + ttl, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/connections/connection-handles.ts` around lines 14 - 31, The createConnectionHandle function should validate the input.ttlMs to avoid zero/negative TTLs: inside createConnectionHandle (which uses defaultConnectionHandleTtlMs, connectionHandlePurpose, and signToken) check if input.ttlMs is provided and is a positive number; if it's <= 0 or NaN, either replace it with defaultConnectionHandleTtlMs or throw a clear error (choose consistent behavior with the codebase), then compute exp using the validated TTL and call signToken as before. Ensure you reference input.ttlMs when validating and keep the iat/exp calculation using the validated ttl variable.packages/worker/src/mcp/connections/auth-spec.ts (1)
137-144:getPrimarySecretFieldNamereturns hardcoded'access_token'for OAuth strategies.For OAuth flows,
'access_token'isn't insecret_fields(which containsclient_id/client_secretfor pre-registered, or is empty for dynamic). This seems intentional since OAuth access tokens are obtained dynamically, but consider adding a clarifying comment:💡 Suggested documentation
export function getPrimarySecretFieldName(spec: ConnectionAuthSpec) { + // OAuth strategies use dynamically-obtained access tokens, not user-provided secrets if (spec.strategy === 'oauth2_pre_registered_client') { return 'access_token' } if (spec.strategy === 'oauth2_dynamic_client') { return 'access_token' } + // Manual token strategies use the first declared secret field return spec.secret_fields[0]?.name ?? null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/connections/auth-spec.ts` around lines 137 - 144, The getPrimarySecretFieldName function returns a hardcoded 'access_token' for OAuth strategies, which is intentional because OAuth access tokens are obtained dynamically and not listed in spec.secret_fields (which contains client_id/client_secret for pre-registered or is empty for dynamic); update the function by adding a brief clarifying comment above getPrimarySecretFieldName explaining that for 'oauth2_pre_registered_client' and 'oauth2_dynamic_client' the primary secret is the runtime access_token (not present in spec.secret_fields) so returning 'access_token' is expected, and mention that client credentials remain in spec.secret_fields for pre-registered clients to avoid confusion.packages/worker/src/mcp/skills/builtin-skill-templates.ts (1)
347-376: Embedding vector upsert failures are silently ignored.If
upsertSkillVectorfails after a successful insert, the skill exists in D1 but won't be searchable via vector similarity. Consider logging or collecting these failures:💡 Suggested improvement
if (!inserted) { continue } - await upsertSkillVector(env, { - skillId, - userId, - embedText: prep.embedText, - }) + try { + await upsertSkillVector(env, { + skillId, + userId, + embedText: prep.embedText, + }) + } catch (error) { + console.warn(`Failed to upsert vector for builtin skill ${template.templateKey}:`, error) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/skills/builtin-skill-templates.ts` around lines 347 - 376, The loop that inserts new templates calls upsertSkillVector after insertMcpSkillIfMissingTemplate but ignores failures, which can leave skills unsearchable; wrap the upsertSkillVector call in a try/catch, log the error (including skillId and template.templateKey) and either retry or push the failed skillId into a retry/failure list for later processing; ensure the error logging uses existing logger (or processLogger) and preserve the inserted skill when retrying so you don't rollback DB insertion in insertMcpSkillIfMissingTemplate/upsertSkillVector.packages/worker/src/mcp/generated-ui-api.ts (2)
60-65: JSON parse failure produces a generic Zod error rather than a clear message.When
request.json()throws (malformed JSON), the code passesnulltosafeParse, which fails with a Zod type error rather than a clear "Invalid JSON" message. Consider distinguishing parse errors:💡 Suggested improvement
- const body = actionRequestSchema.safeParse( - await request.json().catch(() => null), - ) - if (!body.success) { - return jsonResponse({ error: body.error.message }, 400) - } + let rawBody: unknown + try { + rawBody = await request.json() + } catch { + return jsonResponse({ error: 'Invalid JSON body.' }, 400) + } + const body = actionRequestSchema.safeParse(rawBody) + if (!body.success) { + return jsonResponse({ error: body.error.message }, 400) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/generated-ui-api.ts` around lines 60 - 65, The current flow calls actionRequestSchema.safeParse(await request.json().catch(() => null)), which converts a JSON parse failure into a Zod type error; change it to explicitly catch JSON parse errors before schema validation: await request.json() in a try/catch, if parsing throws return jsonResponse({ error: "Invalid JSON" }, 400), otherwise call actionRequestSchema.safeParse(parsedBody) and continue to return schema errors as before (referencing actionRequestSchema.safeParse, request.json(), body variable and jsonResponse).
97-102: Same JSON parse error handling issue for secure-input endpoint.Apply the same pattern here for clearer error messages on malformed JSON.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/generated-ui-api.ts` around lines 97 - 102, The secure-input handler currently swallows JSON parse errors by using await request.json().catch(() => null) before secureInputRequestSchema.safeParse; change it to explicitly catch and surface the JSON parsing error from request.json() (e.g., await request.json().catch(err => { throw err })) or capture the error into a variable and, if present, return jsonResponse({ error: err.message }, 400) so callers get a clear parse error; keep the secureInputRequestSchema.safeParse usage for schema validation and still return jsonResponse({ error: body.error.message }, 400) on validation failures.packages/worker/src/mcp/connections/crypto.ts (2)
28-50: Inconsistent key derivation between AES and HMAC.
deriveAesKeyuses SHA-256 to hash the purpose+secret material before importing, whilederiveHmacKeyuses the raw bytes directly. This inconsistency could be intentional (HMAC is designed for variable-length keys), but aligning the approaches would improve maintainability:💡 Optional: align HMAC derivation with AES pattern
async function deriveHmacKey(secret: string, purpose: string) { + const material = await crypto.subtle.digest( + 'SHA-256', + textEncoder.encode(`kody:hmac:${purpose}:${secret}`), + ) return crypto.subtle.importKey( 'raw', - textEncoder.encode(`kody:hmac:${purpose}:${secret}`), + material, { name: 'HMAC', hash: 'SHA-256', }, false, ['sign', 'verify'], ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/connections/crypto.ts` around lines 28 - 50, deriveAesKey and deriveHmacKey derive keys differently: AES hashes the formatted material with SHA-256 before import, while HMAC imports raw bytes. Make deriveHmacKey mirror deriveAesKey by first computing crypto.subtle.digest('SHA-256', textEncoder.encode(`kody:hmac:${purpose}:${secret}`)) and then using that digest as the 'raw' key material in crypto.subtle.importKey (preserving name: 'HMAC', hash: 'SHA-256', extractable: false, usages: ['sign','verify']); ensure you handle the digest ArrayBuffer the same way deriveAesKey does.
135-137: Add documentation to clarify theexpfield expects milliseconds, not seconds.All current callers of
signTokencorrectly use milliseconds (verified:generated-ui-app-session.tsusesexpiresAtMs,connection-handles.tsusesttlMs/defaultConnectionHandleTtlMs, andconnection-service.tsuses10 * 60 * 1000). However, theexpfield in the payload types (ConnectionHandlePayload,GeneratedUiAppSessionPayload) is only typed asnumberwithout unit documentation. Consider adding a JSDoc comment to thesignTokenfunction or the payload types to explicitly state thatexpmust be in milliseconds (consistent withDate.now()), preventing future callers from accidentally using seconds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/connections/crypto.ts` around lines 135 - 137, The expiration check uses Date.now() so the payload.exp must be in milliseconds; add a clear JSDoc comment stating that exp is in milliseconds to prevent future confusion: update the JSDoc for the signToken function and the payload type declarations (ConnectionHandlePayload, GeneratedUiAppSessionPayload) to explicitly say "exp is a UNIX timestamp in milliseconds (ms)"; also add an inline JSDoc above the existing expiration check in crypto.ts (the block throwing 'Signed token has expired.') to reinforce the expectation that payload['exp'] is milliseconds.packages/worker/src/mcp/tools/open-generated-ui.ts (1)
127-135: Consider whether session creation failures should fail the tool call.If
createGeneratedUiAppSessionthrows (e.g., due to a transient error), the entire tool call will fail. If app session is a non-essential enhancement (the tool can still render UI without it), consider wrapping in try/catch and falling back tonull:💡 Optional defensive pattern
const callerContext = agent.getCallerContext() - const appSession = - callerContext.user != null - ? await createGeneratedUiAppSession( - agent.getEnv(), - callerContext.baseUrl, - callerContext.user, - ) - : null + let appSession = null + if (callerContext.user != null) { + try { + appSession = await createGeneratedUiAppSession( + agent.getEnv(), + callerContext.baseUrl, + callerContext.user, + ) + } catch (error) { + console.warn('Failed to create generated UI app session:', error) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/open-generated-ui.ts` around lines 127 - 135, The call to createGeneratedUiAppSession inside the block that builds appSession can throw and currently will fail the whole tool call; change it to try/catch around createGeneratedUiAppSession (when callerContext.user != null) so that any thrown error is caught, set appSession = null on failure, and optionally log the error for debugging (use agent.getLogger()/agent.logger or the existing logger). Keep the rest of the flow unchanged so the tool continues to render UI even if session creation fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/client/mcp-apps/generated-ui-shell.ts`:
- Around line 479-523: Both invokeActionWithSession and
submitSecureInputWithSession must treat non-2xx and malformed responses as
failures: after fetch, if response.ok is false, attempt to read a text/json
error message and return { ok: false, error: '...' } instead of trying to use
the body; if response.ok is true parse JSON but validate that payload is an
object and that payload.ok is explicitly present and boolean — if missing or
payload is not an object return { ok: false, error: '...invalid response' }.
Update invokeActionWithSession and submitSecureInputWithSession to perform these
checks and return a consistent {ok:false,...} shape for error cases so callers
(e.g. executeCode) won’t treat error bodies as success.
In `@packages/worker/src/mcp/connections/auth-spec.ts`:
- Around line 55-61: The enum in manualTokenLikeAuthSpecSchema includes
'api_key' but connectionAuthSpecSchema's discriminated union does not include an
api_key branch, causing inconsistent validation; either remove 'api_key' from
manualTokenLikeAuthSpecSchema (and the other identical enum occurrence) if it's
not supported, or add a corresponding discriminated union member to
connectionAuthSpecSchema that accepts the manual token-like shape for 'api_key'
(mirror the existing manual_token member) so the discriminator covers both
'manual_token' and 'api_key'; update both occurrences (the enum at
manualTokenLikeAuthSpecSchema and the duplicate at lines ~98-102) and ensure
secret_fields/request/verification shapes remain the same for the new member.
In `@packages/worker/src/mcp/connections/connection-drafts-repo.ts`:
- Around line 73-135: The current updateConnectionDraft and
updateConnectionDraftUnsafe functions perform a read-then-overwrite using
getConnectionDraftById/getConnectionDraftByIdUnsafe and resolveFieldUpdate,
which risks stomping concurrent updates (e.g., from storeDraftSecrets,
startConnectionOAuth or the OAuth callback); change both functions to build the
SQL UPDATE dynamically using only the keys present in the passed fields object
(only include status, state_json, error_message, expires_at if they exist on
fields), construct the corresponding parameter list and updated_at value, and
execute a single UPDATE WITHOUT reading the existing row (remove reliance on
resolveFieldUpdate and the initial getConnectionDraft* calls) so omitted columns
are left untouched and concurrent writes do not restore stale values.
In `@packages/worker/src/mcp/connections/connection-service.ts`:
- Around line 929-939: The helper getRequiredConnectionDraft should validate
that the returned draft from getConnectionDraftById is not expired and not in a
terminal/completed state before allowing reuse; update
getRequiredConnectionDraft to check draft.expires_at (compare to current time)
and draft.completed (or other terminal flags) and throw an Error like
"Connection draft expired or already finalized" when either condition holds so
downstream callers such as storeDraftSecrets, startConnectionOAuth, and
finalizeConnectionSetup cannot operate on stale or completed drafts.
- Around line 215-238: The finalizeConnectionSetup flow currently proceeds even
for OAuth drafts that have client credentials but no tokens; update
finalizeConnectionSetup to detect OAuth-style specs (use the parsed spec from
parseConnectionAuthSpec) and check secretMaterial for required token fields
(e.g., access_token or refresh_token) after loadDraftSecretMaterial and
getMissingSecretNames; if the spec indicates OAuth and no token is present,
abort finalization by throwing a clear Error or returning a pending state
instead of calling verifyProviderConnectionDraft, so OAuth drafts are only
finalized once authorization has completed.
- Around line 598-628: Wrap the token exchange and secret persistence sequence
(the calls to exchangeOAuthCodeForTokens, encryptJson, and
upsertConnectionDraftSecret) in a try/catch so any thrown error is caught; in
the catch, call updateConnectionDraftUnsafe for the draft (referencing draft.id
and updateConnectionDraftUnsafe) to persist a failure state by setting a
sensible status (e.g., 'error' or leaving 'awaiting_oauth_callback' per desired
semantics) and writing error_message to the error text (include
error.message/stack) and optionally state_json, then rethrow or return the error
page result—this ensures failures during
loadDraftSecretMaterial/exchangeOAuthCodeForTokens/encryptJson/upsertConnectionDraftSecret
are recorded on the draft before the handler exits.
In `@packages/worker/src/mcp/connections/oauth-api.ts`:
- Around line 8-17: The code currently calls handleConnectionOAuthCallback for
HEAD requests which can cause side effects; change the control flow so
handleConnectionOAuthCallback(request, env) is only invoked when request.method
=== 'GET' and not for 'HEAD'. Specifically, check for 'HEAD' immediately after
validating allowed methods and return an empty Response with the appropriate
status/headers without calling handleConnectionOAuthCallback, and move the
handleConnectionOAuthCallback call into the GET branch so side-effecting OAuth
callback logic runs only for GET.
In `@packages/worker/src/mcp/connections/provider-request.ts`:
- Around line 140-155: verifyProviderConnectionDraft currently returns the raw
response from performProviderHttpRequestWithSpec even when the provider returns
non-success statuses (e.g., 401/403/500), allowing downstream
finalizeConnectionSetup to persist invalid/active connections; update
verifyProviderConnectionDraft to inspect the returned response status (from
performProviderHttpRequestWithSpec) and treat any non-2xx status as a failure:
either throw a descriptive error or return a rejected result so callers can
abort setup, include the status and provider response body in the error message
for diagnostics, and keep references to ConnectionAuthSpec,
ProviderSecretMaterial, performProviderHttpRequestWithSpec,
verifyProviderConnectionDraft and finalizeConnectionSetup to locate the change.
In `@packages/worker/src/mcp/generated-ui-app-session.ts`:
- Around line 33-38: The token payload currently includes PII (user.email) when
calling signToken with generatedUiSessionPurpose; remove email from the
McpUserContext passed into signToken so only non-PII fields (e.g., userId and
displayName) are embedded (keep session_id, iat, exp as-is). Locate the call to
signToken and the object referenced by the user variable (McpUserContext) and
change the constructed payload to exclude email (or construct a new minimal user
object) and update any type annotations if necessary; if email is required in
future, switch to an encrypted token (JWE) instead.
In `@packages/worker/src/mcp/skills/builtin-skill-templates.ts`:
- Around line 248-263: The Cloudflare and Cursor template code currently calls
codemode.connections_resolve and codemode.provider_http_request directly and
will throw if no connection exists; update both templates to call
codemode.connections_resolve with allow_missing: true, check the returned
connection (or lack thereof) and return a user-facing setup guidance
object/response when missing, otherwise proceed to call
codemode.provider_http_request using connection.handle (mirror the GitHub REST
template's flow). Ensure you reference the existing calls to
codemode.connections_resolve and codemode.provider_http_request in the template
strings and implement the missing-connection early-return/guide path.
- Around line 189-201: The GraphQL template currently calls
codemode.connections_resolve(...) without allow_missing and will throw if no
GitHub connection exists; change the call in the async template handler to pass
allow_missing: true to match the REST template, then mirror the REST flow: check
the returned connection value and, if it's missing/falsy, return the same
connection-setup initiation path used by the REST template instead of proceeding
to codemode.provider_graphql_request; update the block that calls
provider_graphql_request to only run when a truthy connection.handle is present
and otherwise delegate to the existing connection-setup behavior.
---
Outside diff comments:
In `@packages/worker/client/mcp-apps/generated-ui-shell.ts`:
- Around line 683-759: The builders currently read latestEnvelope?.appSession at
render time (after awaits), causing cross-session leaks; capture the current
appSession before any await in renderEnvelope and thread that captured value
into the document builders instead of accessing latestEnvelope inside
buildInlineModuleSource, buildHtmlDocumentFromFragment and setFrameSource (and
any other builders at lines ~761-788 and ~812-840). Change signatures to accept
an appSession parameter (e.g., buildInlineModuleSource(code, appSession),
buildHtmlDocumentFromFragment(code, appSession)) and use the passed appSession
when calling buildChildBridgeRuntimeSource or buildHeadInjection, and update
renderEnvelope to pass the captured appSession into
setFrameSource/buildHtmlDocumentFromFragment so each render uses its own session
token.
---
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/unified-search.ts`:
- Around line 135-137: Only parse row.connection_bindings when the caller
requested detail: replace the unconditional call to
parseSkillConnectionBindings(row.connection_bindings) with a conditional/lazy
parse that sets connectionBindings = detail ?
parseSkillConnectionBindings(row.connection_bindings) : undefined (or only
assign when detail is truthy) so summary-mode hits avoid the work; apply the
same change to the second occurrence around the other hit-processing block (the
code regions currently using connectionBindings and
parseSkillConnectionBindings) to ensure both hot-paths skip parsing when detail
is false.
In `@packages/worker/src/mcp/connections/auth-spec.ts`:
- Around line 137-144: The getPrimarySecretFieldName function returns a
hardcoded 'access_token' for OAuth strategies, which is intentional because
OAuth access tokens are obtained dynamically and not listed in
spec.secret_fields (which contains client_id/client_secret for pre-registered or
is empty for dynamic); update the function by adding a brief clarifying comment
above getPrimarySecretFieldName explaining that for
'oauth2_pre_registered_client' and 'oauth2_dynamic_client' the primary secret is
the runtime access_token (not present in spec.secret_fields) so returning
'access_token' is expected, and mention that client credentials remain in
spec.secret_fields for pre-registered clients to avoid confusion.
In `@packages/worker/src/mcp/connections/connection-handles.ts`:
- Around line 14-31: The createConnectionHandle function should validate the
input.ttlMs to avoid zero/negative TTLs: inside createConnectionHandle (which
uses defaultConnectionHandleTtlMs, connectionHandlePurpose, and signToken) check
if input.ttlMs is provided and is a positive number; if it's <= 0 or NaN, either
replace it with defaultConnectionHandleTtlMs or throw a clear error (choose
consistent behavior with the codebase), then compute exp using the validated TTL
and call signToken as before. Ensure you reference input.ttlMs when validating
and keep the iat/exp calculation using the validated ttl variable.
In `@packages/worker/src/mcp/connections/crypto.ts`:
- Around line 28-50: deriveAesKey and deriveHmacKey derive keys differently: AES
hashes the formatted material with SHA-256 before import, while HMAC imports raw
bytes. Make deriveHmacKey mirror deriveAesKey by first computing
crypto.subtle.digest('SHA-256',
textEncoder.encode(`kody:hmac:${purpose}:${secret}`)) and then using that digest
as the 'raw' key material in crypto.subtle.importKey (preserving name: 'HMAC',
hash: 'SHA-256', extractable: false, usages: ['sign','verify']); ensure you
handle the digest ArrayBuffer the same way deriveAesKey does.
- Around line 135-137: The expiration check uses Date.now() so the payload.exp
must be in milliseconds; add a clear JSDoc comment stating that exp is in
milliseconds to prevent future confusion: update the JSDoc for the signToken
function and the payload type declarations (ConnectionHandlePayload,
GeneratedUiAppSessionPayload) to explicitly say "exp is a UNIX timestamp in
milliseconds (ms)"; also add an inline JSDoc above the existing expiration check
in crypto.ts (the block throwing 'Signed token has expired.') to reinforce the
expectation that payload['exp'] is milliseconds.
In `@packages/worker/src/mcp/generated-ui-api.ts`:
- Around line 60-65: The current flow calls actionRequestSchema.safeParse(await
request.json().catch(() => null)), which converts a JSON parse failure into a
Zod type error; change it to explicitly catch JSON parse errors before schema
validation: await request.json() in a try/catch, if parsing throws return
jsonResponse({ error: "Invalid JSON" }, 400), otherwise call
actionRequestSchema.safeParse(parsedBody) and continue to return schema errors
as before (referencing actionRequestSchema.safeParse, request.json(), body
variable and jsonResponse).
- Around line 97-102: The secure-input handler currently swallows JSON parse
errors by using await request.json().catch(() => null) before
secureInputRequestSchema.safeParse; change it to explicitly catch and surface
the JSON parsing error from request.json() (e.g., await request.json().catch(err
=> { throw err })) or capture the error into a variable and, if present, return
jsonResponse({ error: err.message }, 400) so callers get a clear parse error;
keep the secureInputRequestSchema.safeParse usage for schema validation and
still return jsonResponse({ error: body.error.message }, 400) on validation
failures.
In `@packages/worker/src/mcp/generated-ui-app-session.ts`:
- Around line 40-53: The code creates appBaseUrl via new URL(baseUrl) then
builds endpoints with new URL('/api/…', appBaseUrl), which is fine but redundant
and can cause subtle double-slash edge cases if baseUrl includes a trailing
slash; update the GeneratedUiAppSession builder to normalize baseUrl first (trim
a trailing slash) or construct each endpoint by passing baseUrl string as the
base to new URL (use baseUrl normalized) so appBaseUrl is not misused—adjust the
logic around appBaseUrl, baseUrl, and the endpoints keys (appSource, action,
secureInput) to ensure consistent URL construction and remove the unnecessary
new URL(baseUrl) where appropriate.
In `@packages/worker/src/mcp/skills/builtin-skill-templates.ts`:
- Around line 347-376: The loop that inserts new templates calls
upsertSkillVector after insertMcpSkillIfMissingTemplate but ignores failures,
which can leave skills unsearchable; wrap the upsertSkillVector call in a
try/catch, log the error (including skillId and template.templateKey) and either
retry or push the failed skillId into a retry/failure list for later processing;
ensure the error logging uses existing logger (or processLogger) and preserve
the inserted skill when retrying so you don't rollback DB insertion in
insertMcpSkillIfMissingTemplate/upsertSkillVector.
In `@packages/worker/src/mcp/tools/open-generated-ui.ts`:
- Around line 127-135: The call to createGeneratedUiAppSession inside the block
that builds appSession can throw and currently will fail the whole tool call;
change it to try/catch around createGeneratedUiAppSession (when
callerContext.user != null) so that any thrown error is caught, set appSession =
null on failure, and optionally log the error for debugging (use
agent.getLogger()/agent.logger or the existing logger). Keep the rest of the
flow unchanged so the tool continues to render UI even if session creation
fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41dba659-81eb-4cc2-a87b-26371ccb5e1b
📒 Files selected for processing (47)
docs/agents/end-to-end-testing.mddocs/agents/mcp-apps-starter-guide.mdpackages/worker/client/mcp-apps/generated-ui-shell.tspackages/worker/migrations/0008-provider-connections.sqlpackages/worker/migrations/0009-skill-connection-bindings.sqlpackages/worker/src/index.tspackages/worker/src/mcp-auth.tspackages/worker/src/mcp/capabilities/builtin-domains.tspackages/worker/src/mcp/capabilities/connections/connections-begin-setup.tspackages/worker/src/mcp/capabilities/connections/connections-disconnect.tspackages/worker/src/mcp/capabilities/connections/connections-finalize.tspackages/worker/src/mcp/capabilities/connections/connections-list.tspackages/worker/src/mcp/capabilities/connections/connections-resolve.tspackages/worker/src/mcp/capabilities/connections/connections-set-default.tspackages/worker/src/mcp/capabilities/connections/connections-start-oauth.tspackages/worker/src/mcp/capabilities/connections/domain.tspackages/worker/src/mcp/capabilities/connections/provider-graphql-request.tspackages/worker/src/mcp/capabilities/connections/provider-http-request.tspackages/worker/src/mcp/capabilities/connections/provider-refresh-token.tspackages/worker/src/mcp/capabilities/domain-metadata.tspackages/worker/src/mcp/capabilities/meta/meta-get-skill.tspackages/worker/src/mcp/capabilities/meta/meta-save-skill.tspackages/worker/src/mcp/capabilities/meta/meta-update-skill.tspackages/worker/src/mcp/capabilities/unified-search.tspackages/worker/src/mcp/connections/auth-spec.tspackages/worker/src/mcp/connections/connection-drafts-repo.tspackages/worker/src/mcp/connections/connection-drafts-types.tspackages/worker/src/mcp/connections/connection-handles.tspackages/worker/src/mcp/connections/connection-service.tspackages/worker/src/mcp/connections/crypto.tspackages/worker/src/mcp/connections/oauth-api.tspackages/worker/src/mcp/connections/provider-connections-repo.tspackages/worker/src/mcp/connections/provider-connections-types.tspackages/worker/src/mcp/connections/provider-request.tspackages/worker/src/mcp/generated-ui-api.tspackages/worker/src/mcp/generated-ui-app-session.tspackages/worker/src/mcp/index.tspackages/worker/src/mcp/mcp-server-e2e.test.tspackages/worker/src/mcp/skills/builtin-skill-templates.tspackages/worker/src/mcp/skills/mcp-skills-repo.tspackages/worker/src/mcp/skills/mcp-skills-types.tspackages/worker/src/mcp/skills/skill-connections.tspackages/worker/src/mcp/skills/skill-embed-and-flags.tspackages/worker/src/mcp/skills/skill-mutation.tspackages/worker/src/mcp/tools/open-generated-ui.tspackages/worker/src/mcp/tools/search.tsplaywright.config.ts
Comment on lines
+479
to
+523
| async function invokeActionWithSession(input) { | ||
| if (!appSession) return null; | ||
| const response = await fetch(appSession.endpoints.action, { | ||
| method: 'POST', | ||
| headers: { | ||
| ...getAppSessionHeaders(), | ||
| 'content-type': 'application/json', | ||
| }, | ||
| body: JSON.stringify(input), | ||
| }); | ||
| const payload = await response.json().catch(() => null); | ||
| if (!payload || typeof payload !== 'object') { | ||
| return { | ||
| ok: false, | ||
| error: 'Generated UI action endpoint returned an invalid response.', | ||
| }; | ||
| } | ||
| return payload; | ||
| } | ||
| async function submitSecureInputWithSession(input) { | ||
| if (!appSession) { | ||
| return { | ||
| ok: false, | ||
| error: 'Secure input is unavailable without a generated UI app session.', | ||
| }; | ||
| } | ||
| const response = await fetch(appSession.endpoints.secureInput, { | ||
| method: 'POST', | ||
| headers: { | ||
| ...getAppSessionHeaders(), | ||
| 'content-type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| setup_id: input.setupId, | ||
| fields: input.fields, | ||
| }), | ||
| }); | ||
| const payload = await response.json().catch(() => null); | ||
| if (!payload || typeof payload !== 'object') { | ||
| return { | ||
| ok: false, | ||
| error: 'Secure input endpoint returned an invalid response.', | ||
| }; | ||
| } | ||
| return payload; |
There was a problem hiding this comment.
Treat non-2xx or malformed session-endpoint responses as failures.
Both helpers currently return any JSON object, even when the endpoint replied with 401/500. executeCode() only throws on payload.ok === false, so a JSON error body without ok is treated as a successful null result.
Proposed fix
async function invokeActionWithSession(input) {
if (!appSession) return null;
const response = await fetch(appSession.endpoints.action, {
method: 'POST',
headers: {
...getAppSessionHeaders(),
'content-type': 'application/json',
},
body: JSON.stringify(input),
});
const payload = await response.json().catch(() => null);
- if (!payload || typeof payload !== 'object') {
+ if (
+ !payload ||
+ typeof payload !== 'object' ||
+ typeof payload.ok !== 'boolean'
+ ) {
return {
ok: false,
error: 'Generated UI action endpoint returned an invalid response.',
};
}
+ if (!response.ok && payload.ok !== false) {
+ return {
+ ok: false,
+ error: 'Generated UI action failed.',
+ };
+ }
return payload;
}
async function submitSecureInputWithSession(input) {
...
const payload = await response.json().catch(() => null);
- if (!payload || typeof payload !== 'object') {
+ if (
+ !payload ||
+ typeof payload !== 'object' ||
+ typeof payload.ok !== 'boolean'
+ ) {
return {
ok: false,
error: 'Secure input endpoint returned an invalid response.',
};
}
+ if (!response.ok && payload.ok !== false) {
+ return {
+ ok: false,
+ error: 'Secure input submission failed.',
+ };
+ }
return payload;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function invokeActionWithSession(input) { | |
| if (!appSession) return null; | |
| const response = await fetch(appSession.endpoints.action, { | |
| method: 'POST', | |
| headers: { | |
| ...getAppSessionHeaders(), | |
| 'content-type': 'application/json', | |
| }, | |
| body: JSON.stringify(input), | |
| }); | |
| const payload = await response.json().catch(() => null); | |
| if (!payload || typeof payload !== 'object') { | |
| return { | |
| ok: false, | |
| error: 'Generated UI action endpoint returned an invalid response.', | |
| }; | |
| } | |
| return payload; | |
| } | |
| async function submitSecureInputWithSession(input) { | |
| if (!appSession) { | |
| return { | |
| ok: false, | |
| error: 'Secure input is unavailable without a generated UI app session.', | |
| }; | |
| } | |
| const response = await fetch(appSession.endpoints.secureInput, { | |
| method: 'POST', | |
| headers: { | |
| ...getAppSessionHeaders(), | |
| 'content-type': 'application/json', | |
| }, | |
| body: JSON.stringify({ | |
| setup_id: input.setupId, | |
| fields: input.fields, | |
| }), | |
| }); | |
| const payload = await response.json().catch(() => null); | |
| if (!payload || typeof payload !== 'object') { | |
| return { | |
| ok: false, | |
| error: 'Secure input endpoint returned an invalid response.', | |
| }; | |
| } | |
| return payload; | |
| async function invokeActionWithSession(input) { | |
| if (!appSession) return null; | |
| const response = await fetch(appSession.endpoints.action, { | |
| method: 'POST', | |
| headers: { | |
| ...getAppSessionHeaders(), | |
| 'content-type': 'application/json', | |
| }, | |
| body: JSON.stringify(input), | |
| }); | |
| const payload = await response.json().catch(() => null); | |
| if ( | |
| !payload || | |
| typeof payload !== 'object' || | |
| typeof payload.ok !== 'boolean' | |
| ) { | |
| return { | |
| ok: false, | |
| error: 'Generated UI action endpoint returned an invalid response.', | |
| }; | |
| } | |
| if (!response.ok && payload.ok !== false) { | |
| return { | |
| ok: false, | |
| error: 'Generated UI action failed.', | |
| }; | |
| } | |
| return payload; | |
| } | |
| async function submitSecureInputWithSession(input) { | |
| if (!appSession) { | |
| return { | |
| ok: false, | |
| error: 'Secure input is unavailable without a generated UI app session.', | |
| }; | |
| } | |
| const response = await fetch(appSession.endpoints.secureInput, { | |
| method: 'POST', | |
| headers: { | |
| ...getAppSessionHeaders(), | |
| 'content-type': 'application/json', | |
| }, | |
| body: JSON.stringify({ | |
| setup_id: input.setupId, | |
| fields: input.fields, | |
| }), | |
| }); | |
| const payload = await response.json().catch(() => null); | |
| if ( | |
| !payload || | |
| typeof payload !== 'object' || | |
| typeof payload.ok !== 'boolean' | |
| ) { | |
| return { | |
| ok: false, | |
| error: 'Secure input endpoint returned an invalid response.', | |
| }; | |
| } | |
| if (!response.ok && payload.ok !== false) { | |
| return { | |
| ok: false, | |
| error: 'Secure input submission failed.', | |
| }; | |
| } | |
| return payload; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/client/mcp-apps/generated-ui-shell.ts` around lines 479 -
523, Both invokeActionWithSession and submitSecureInputWithSession must treat
non-2xx and malformed responses as failures: after fetch, if response.ok is
false, attempt to read a text/json error message and return { ok: false, error:
'...' } instead of trying to use the body; if response.ok is true parse JSON but
validate that payload is an object and that payload.ok is explicitly present and
boolean — if missing or payload is not an object return { ok: false, error:
'...invalid response' }. Update invokeActionWithSession and
submitSecureInputWithSession to perform these checks and return a consistent
{ok:false,...} shape for error cases so callers (e.g. executeCode) won’t treat
error bodies as success.
Comment on lines
+73
to
+135
| export async function updateConnectionDraft( | ||
| db: D1Database, | ||
| draftId: string, | ||
| userId: string, | ||
| fields: { | ||
| status?: string | ||
| state_json?: string | null | ||
| error_message?: string | null | ||
| expires_at?: string | ||
| }, | ||
| ) { | ||
| const now = new Date().toISOString() | ||
| const existing = await getConnectionDraftById(db, userId, draftId) | ||
| if (!existing) return false | ||
| await db | ||
| .prepare( | ||
| `UPDATE connection_drafts | ||
| SET status = ?, state_json = ?, error_message = ?, expires_at = ?, updated_at = ? | ||
| WHERE id = ? AND user_id = ?`, | ||
| ) | ||
| .bind( | ||
| resolveFieldUpdate(fields, 'status', existing.status), | ||
| resolveFieldUpdate(fields, 'state_json', existing.state_json), | ||
| resolveFieldUpdate(fields, 'error_message', existing.error_message), | ||
| resolveFieldUpdate(fields, 'expires_at', existing.expires_at), | ||
| now, | ||
| draftId, | ||
| userId, | ||
| ) | ||
| .run() | ||
| return true | ||
| } | ||
|
|
||
| export async function updateConnectionDraftUnsafe( | ||
| db: D1Database, | ||
| draftId: string, | ||
| fields: { | ||
| status?: string | ||
| state_json?: string | null | ||
| error_message?: string | null | ||
| expires_at?: string | ||
| }, | ||
| ) { | ||
| const now = new Date().toISOString() | ||
| const existing = await getConnectionDraftByIdUnsafe(db, draftId) | ||
| if (!existing) return false | ||
| await db | ||
| .prepare( | ||
| `UPDATE connection_drafts | ||
| SET status = ?, state_json = ?, error_message = ?, expires_at = ?, updated_at = ? | ||
| WHERE id = ?`, | ||
| ) | ||
| .bind( | ||
| resolveFieldUpdate(fields, 'status', existing.status), | ||
| resolveFieldUpdate(fields, 'state_json', existing.state_json), | ||
| resolveFieldUpdate(fields, 'error_message', existing.error_message), | ||
| resolveFieldUpdate(fields, 'expires_at', existing.expires_at), | ||
| now, | ||
| draftId, | ||
| ) | ||
| .run() | ||
| return true | ||
| } |
There was a problem hiding this comment.
Avoid the read-then-overwrite update pattern here.
Both update helpers read a snapshot and then rewrite every mutable column. If storeDraftSecrets(), startConnectionOAuth(), and the OAuth callback touch the same draft concurrently, the later write can restore stale state_json, status, or error_message.
Build the SET clause from only the keys explicitly present in fields instead of hydrating omitted values from a stale row.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/connections/connection-drafts-repo.ts` around lines
73 - 135, The current updateConnectionDraft and updateConnectionDraftUnsafe
functions perform a read-then-overwrite using
getConnectionDraftById/getConnectionDraftByIdUnsafe and resolveFieldUpdate,
which risks stomping concurrent updates (e.g., from storeDraftSecrets,
startConnectionOAuth or the OAuth callback); change both functions to build the
SQL UPDATE dynamically using only the keys present in the passed fields object
(only include status, state_json, error_message, expires_at if they exist on
fields), construct the corresponding parameter list and updated_at value, and
execute a single UPDATE WITHOUT reading the existing row (remove reliance on
resolveFieldUpdate and the initial getConnectionDraft* calls) so omitted columns
are left untouched and concurrent writes do not restore stale values.
Comment on lines
+215
to
+238
| export async function finalizeConnectionSetup(input: { | ||
| env: Env | ||
| userId: string | ||
| draftId: string | ||
| makeDefault?: boolean | ||
| }) { | ||
| const draft = await getRequiredConnectionDraft( | ||
| input.env, | ||
| input.userId, | ||
| input.draftId, | ||
| ) | ||
| const spec = parseConnectionAuthSpec(draft.auth_spec_json) | ||
| const secretMaterial = await loadDraftSecretMaterial(input.env, draft.id) | ||
| const missingSecretNames = getMissingSecretNames(spec, secretMaterial) | ||
| if (missingSecretNames.length > 0) { | ||
| throw new Error( | ||
| `Connection draft is missing required secret fields: ${missingSecretNames.join(', ')}`, | ||
| ) | ||
| } | ||
|
|
||
| const verification = await verifyProviderConnectionDraft({ | ||
| spec, | ||
| secretMaterial, | ||
| }) |
There was a problem hiding this comment.
Block OAuth finalization until authorization has completed.
For OAuth setups, getMissingSecretNames() only validates the stored client credentials. A draft can reach this block with no access token yet, and if the provider has no verification endpoint you will create an active connection that can never authenticate.
Proposed fix
const spec = parseConnectionAuthSpec(draft.auth_spec_json)
const secretMaterial = await loadDraftSecretMaterial(input.env, draft.id)
+ if (
+ (spec.strategy === 'oauth2_pre_registered_client' ||
+ spec.strategy === 'oauth2_dynamic_client') &&
+ (
+ draft.status !== 'authorized' ||
+ typeof secretMaterial['access_token'] !== 'string' ||
+ secretMaterial['access_token'].length === 0
+ )
+ ) {
+ throw new Error(
+ 'OAuth authorization must complete before finalizing this connection.',
+ )
+ }
const missingSecretNames = getMissingSecretNames(spec, secretMaterial)
if (missingSecretNames.length > 0) {
throw new Error(
`Connection draft is missing required secret fields: ${missingSecretNames.join(', ')}`,
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/connections/connection-service.ts` around lines 215 -
238, The finalizeConnectionSetup flow currently proceeds even for OAuth drafts
that have client credentials but no tokens; update finalizeConnectionSetup to
detect OAuth-style specs (use the parsed spec from parseConnectionAuthSpec) and
check secretMaterial for required token fields (e.g., access_token or
refresh_token) after loadDraftSecretMaterial and getMissingSecretNames; if the
spec indicates OAuth and no token is present, abort finalization by throwing a
clear Error or returning a pending state instead of calling
verifyProviderConnectionDraft, so OAuth drafts are only finalized once
authorization has completed.
Comment on lines
+598
to
+628
| const secretMaterial = await loadDraftSecretMaterial(env, draft.id) | ||
| const tokenResponse = await exchangeOAuthCodeForTokens({ | ||
| env, | ||
| spec, | ||
| secretMaterial, | ||
| code, | ||
| redirectUri, | ||
| codeVerifier, | ||
| }) | ||
|
|
||
| for (const [name, value] of Object.entries(tokenResponse.secretMaterial)) { | ||
| if (typeof value !== 'string') continue | ||
| const encryptedValue = await encryptJson( | ||
| env, | ||
| encryptedDraftSecretPurpose, | ||
| value, | ||
| ) | ||
| await upsertConnectionDraftSecret(env.APP_DB, { | ||
| draft_id: draft.id, | ||
| secret_name: name, | ||
| encrypted_value: encryptedValue, | ||
| }) | ||
| } | ||
| await updateConnectionDraftUnsafe(env.APP_DB, draft.id, { | ||
| status: 'authorized', | ||
| error_message: null, | ||
| state_json: JSON.stringify({ | ||
| ...(stateJson ?? {}), | ||
| authorized_at: new Date().toISOString(), | ||
| }), | ||
| }) |
There was a problem hiding this comment.
Persist OAuth callback failures onto the draft.
If exchangeOAuthCodeForTokens() or one of the secret upserts throws, this handler exits with an unhandled error and leaves the draft stuck in awaiting_oauth_callback. Record the failure on the draft before returning the error page.
Proposed fix
const secretMaterial = await loadDraftSecretMaterial(env, draft.id)
- const tokenResponse = await exchangeOAuthCodeForTokens({
- env,
- spec,
- secretMaterial,
- code,
- redirectUri,
- codeVerifier,
- })
-
- for (const [name, value] of Object.entries(tokenResponse.secretMaterial)) {
- if (typeof value !== 'string') continue
- const encryptedValue = await encryptJson(
- env,
- encryptedDraftSecretPurpose,
- value,
- )
- await upsertConnectionDraftSecret(env.APP_DB, {
- draft_id: draft.id,
- secret_name: name,
- encrypted_value: encryptedValue,
- })
- }
- await updateConnectionDraftUnsafe(env.APP_DB, draft.id, {
- status: 'authorized',
- error_message: null,
- state_json: JSON.stringify({
- ...(stateJson ?? {}),
- authorized_at: new Date().toISOString(),
- }),
- })
- return renderOAuthCallbackResponse(
- 200,
- `${draft.display_name} authorization succeeded. Return to Kody and finish setup.`,
- )
+ try {
+ const tokenResponse = await exchangeOAuthCodeForTokens({
+ env,
+ spec,
+ secretMaterial,
+ code,
+ redirectUri,
+ codeVerifier,
+ })
+
+ for (const [name, value] of Object.entries(tokenResponse.secretMaterial)) {
+ if (typeof value !== 'string') continue
+ const encryptedValue = await encryptJson(
+ env,
+ encryptedDraftSecretPurpose,
+ value,
+ )
+ await upsertConnectionDraftSecret(env.APP_DB, {
+ draft_id: draft.id,
+ secret_name: name,
+ encrypted_value: encryptedValue,
+ })
+ }
+ await updateConnectionDraftUnsafe(env.APP_DB, draft.id, {
+ status: 'authorized',
+ error_message: null,
+ state_json: JSON.stringify({
+ ...(stateJson ?? {}),
+ authorized_at: new Date().toISOString(),
+ }),
+ })
+ return renderOAuthCallbackResponse(
+ 200,
+ `${draft.display_name} authorization succeeded. Return to Kody and finish setup.`,
+ )
+ } catch (error) {
+ const message =
+ error instanceof Error ? error.message : 'OAuth token exchange failed.'
+ await updateConnectionDraftUnsafe(env.APP_DB, draft.id, {
+ status: 'error',
+ error_message: message,
+ })
+ return renderOAuthCallbackResponse(400, message)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/connections/connection-service.ts` around lines 598 -
628, Wrap the token exchange and secret persistence sequence (the calls to
exchangeOAuthCodeForTokens, encryptJson, and upsertConnectionDraftSecret) in a
try/catch so any thrown error is caught; in the catch, call
updateConnectionDraftUnsafe for the draft (referencing draft.id and
updateConnectionDraftUnsafe) to persist a failure state by setting a sensible
status (e.g., 'error' or leaving 'awaiting_oauth_callback' per desired
semantics) and writing error_message to the error text (include
error.message/stack) and optionally state_json, then rethrow or return the error
page result—this ensures failures during
loadDraftSecretMaterial/exchangeOAuthCodeForTokens/encryptJson/upsertConnectionDraftSecret
are recorded on the draft before the handler exits.
Comment on lines
+8
to
+17
| if (request.method !== 'GET' && request.method !== 'HEAD') { | ||
| return new Response('Method not allowed.', { status: 405 }) | ||
| } | ||
| const response = await handleConnectionOAuthCallback(request, env) | ||
| if (request.method === 'HEAD') { | ||
| return new Response(null, { | ||
| status: response.status, | ||
| headers: response.headers, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Do not execute OAuth callback flow for HEAD requests.
Line 11 currently runs the full callback for HEAD, which can trigger side effects on a non-interactive probe and jeopardize the subsequent real GET callback.
🛡️ Proposed fix
export async function handleConnectionOAuthRequest(request: Request, env: Env) {
- if (request.method !== 'GET' && request.method !== 'HEAD') {
- return new Response('Method not allowed.', { status: 405 })
+ if (request.method !== 'GET') {
+ return new Response('Method not allowed.', {
+ status: 405,
+ headers: { Allow: 'GET' },
+ })
}
const response = await handleConnectionOAuthCallback(request, env)
- if (request.method === 'HEAD') {
- return new Response(null, {
- status: response.status,
- headers: response.headers,
- })
- }
return response
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/connections/oauth-api.ts` around lines 8 - 17, The
code currently calls handleConnectionOAuthCallback for HEAD requests which can
cause side effects; change the control flow so
handleConnectionOAuthCallback(request, env) is only invoked when request.method
=== 'GET' and not for 'HEAD'. Specifically, check for 'HEAD' immediately after
validating allowed methods and return an empty Response with the appropriate
status/headers without calling handleConnectionOAuthCallback, and move the
handleConnectionOAuthCallback call into the GET branch so side-effecting OAuth
callback logic runs only for GET.
Comment on lines
+140
to
+155
| export async function verifyProviderConnectionDraft(input: { | ||
| spec: ConnectionAuthSpec | ||
| secretMaterial: ProviderSecretMaterial | ||
| }) { | ||
| if (!input.spec.verification) return null | ||
| return performProviderHttpRequestWithSpec({ | ||
| spec: input.spec, | ||
| secretMaterial: input.secretMaterial, | ||
| request: { | ||
| method: input.spec.verification.method, | ||
| path: input.spec.verification.path, | ||
| query: input.spec.verification.query, | ||
| body: input.spec.verification.body, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Fail draft verification on non-success statuses.
This helper currently returns the raw verification response even for 401/403/500. Downstream, finalizeConnectionSetup() can still persist an active connection after the provider rejected the credentials.
Proposed fix
export async function verifyProviderConnectionDraft(input: {
spec: ConnectionAuthSpec
secretMaterial: ProviderSecretMaterial
}) {
if (!input.spec.verification) return null
- return performProviderHttpRequestWithSpec({
+ const response = await performProviderHttpRequestWithSpec({
spec: input.spec,
secretMaterial: input.secretMaterial,
request: {
method: input.spec.verification.method,
path: input.spec.verification.path,
query: input.spec.verification.query,
body: input.spec.verification.body,
},
})
+ if (response.status < 200 || response.status >= 300) {
+ throw new Error(`Provider verification failed (${response.status}).`)
+ }
+ return response
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function verifyProviderConnectionDraft(input: { | |
| spec: ConnectionAuthSpec | |
| secretMaterial: ProviderSecretMaterial | |
| }) { | |
| if (!input.spec.verification) return null | |
| return performProviderHttpRequestWithSpec({ | |
| spec: input.spec, | |
| secretMaterial: input.secretMaterial, | |
| request: { | |
| method: input.spec.verification.method, | |
| path: input.spec.verification.path, | |
| query: input.spec.verification.query, | |
| body: input.spec.verification.body, | |
| }, | |
| }) | |
| } | |
| export async function verifyProviderConnectionDraft(input: { | |
| spec: ConnectionAuthSpec | |
| secretMaterial: ProviderSecretMaterial | |
| }) { | |
| if (!input.spec.verification) return null | |
| const response = await performProviderHttpRequestWithSpec({ | |
| spec: input.spec, | |
| secretMaterial: input.secretMaterial, | |
| request: { | |
| method: input.spec.verification.method, | |
| path: input.spec.verification.path, | |
| query: input.spec.verification.query, | |
| body: input.spec.verification.body, | |
| }, | |
| }) | |
| if (response.status < 200 || response.status >= 300) { | |
| throw new Error(`Provider verification failed (${response.status}).`) | |
| } | |
| return response | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/connections/provider-request.ts` around lines 140 -
155, verifyProviderConnectionDraft currently returns the raw response from
performProviderHttpRequestWithSpec even when the provider returns non-success
statuses (e.g., 401/403/500), allowing downstream finalizeConnectionSetup to
persist invalid/active connections; update verifyProviderConnectionDraft to
inspect the returned response status (from performProviderHttpRequestWithSpec)
and treat any non-2xx status as a failure: either throw a descriptive error or
return a rejected result so callers can abort setup, include the status and
provider response body in the error message for diagnostics, and keep references
to ConnectionAuthSpec, ProviderSecretMaterial,
performProviderHttpRequestWithSpec, verifyProviderConnectionDraft and
finalizeConnectionSetup to locate the change.
Comment on lines
+33
to
+38
| const token = await signToken(env, generatedUiSessionPurpose, { | ||
| session_id: sessionId, | ||
| user, | ||
| iat: now, | ||
| exp: expiresAtMs, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check what fields McpUserContext contains
# Search for McpUserContext type definition
rg -n -A10 'type McpUserContext' packages/shared/
rg -n -A10 'interface McpUserContext' packages/shared/
# Also check the shared chat module
fd -e ts 'chat' packages/shared --exec cat {}Repository: kentcdodds/kody
Length of output: 2515
Remove unnecessary PII from the signed token payload.
The McpUserContext object embedded in the token includes email, which is personally identifiable information. While the token is signed, it is not encrypted, making the payload readable to anyone who possesses the token. The userId and displayName alone should be sufficient for session identification; email should be removed from the token payload or the token should be encrypted (JWE) if email inclusion is necessary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/generated-ui-app-session.ts` around lines 33 - 38,
The token payload currently includes PII (user.email) when calling signToken
with generatedUiSessionPurpose; remove email from the McpUserContext passed into
signToken so only non-PII fields (e.g., userId and displayName) are embedded
(keep session_id, iat, exp as-is). Locate the call to signToken and the object
referenced by the user variable (McpUserContext) and change the constructed
payload to exclude email (or construct a new minimal user object) and update any
type annotations if necessary; if email is required in future, switch to an
encrypted token (JWE) instead.
Comment on lines
+189
to
+201
| code: `async (params) => { | ||
| const connection = await codemode.connections_resolve({ | ||
| provider: 'github', | ||
| selection: { strategy: 'default' }, | ||
| }) | ||
|
|
||
| return await codemode.provider_graphql_request({ | ||
| connection_handle: connection.handle, | ||
| query: params.query, | ||
| ...(params.variables ? { variables: params.variables } : {}), | ||
| ...(params.operationName ? { operationName: params.operationName } : {}), | ||
| }) | ||
| }`, |
There was a problem hiding this comment.
GraphQL template will throw on missing connection, unlike REST template.
The GitHub REST template (lines 84-149) uses allow_missing: true and gracefully handles the missing connection case by initiating setup. The GraphQL template calls connections_resolve without allow_missing, so it will throw an error if no connection exists rather than offering to set one up.
If this is intentional (GraphQL is lower-level and assumes connection exists), consider documenting this in the template description. Otherwise, align the behavior:
💡 Suggested fix to add graceful fallback
code: `async (params) => {
const connection = await codemode.connections_resolve({
provider: 'github',
selection: { strategy: 'default' },
+ allow_missing: true,
})
+
+ if (!connection.found) {
+ return {
+ ok: false,
+ next_action: 'connect_account',
+ provider: 'github',
+ message: 'No GitHub connection found. Please set up a GitHub connection first using the GitHub REST Request skill.',
+ }
+ }
return await codemode.provider_graphql_request({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/skills/builtin-skill-templates.ts` around lines 189 -
201, The GraphQL template currently calls codemode.connections_resolve(...)
without allow_missing and will throw if no GitHub connection exists; change the
call in the async template handler to pass allow_missing: true to match the REST
template, then mirror the REST flow: check the returned connection value and, if
it's missing/falsy, return the same connection-setup initiation path used by the
REST template instead of proceeding to codemode.provider_graphql_request; update
the block that calls provider_graphql_request to only run when a truthy
connection.handle is present and otherwise delegate to the existing
connection-setup behavior.
Comment on lines
+248
to
+263
| code: `async (params) => { | ||
| const connection = await codemode.connections_resolve({ | ||
| provider: 'cloudflare', | ||
| selection: { strategy: 'default' }, | ||
| }) | ||
|
|
||
| return await codemode.provider_http_request({ | ||
| connection_handle: connection.handle, | ||
| request: { | ||
| method: params.method, | ||
| path: params.path, | ||
| ...(params.query ? { query: params.query } : {}), | ||
| ...(params.body !== undefined ? { body: params.body } : {}), | ||
| }, | ||
| }) | ||
| }`, |
There was a problem hiding this comment.
Cloudflare and Cursor templates also lack graceful missing-connection handling.
Same pattern as the GraphQL template - these will throw rather than guide the user through setup. Consider whether these templates should include allow_missing: true with setup flow similar to the GitHub REST template.
Also applies to: 309-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/skills/builtin-skill-templates.ts` around lines 248 -
263, The Cloudflare and Cursor template code currently calls
codemode.connections_resolve and codemode.provider_http_request directly and
will throw if no connection exists; update both templates to call
codemode.connections_resolve with allow_missing: true, check the returned
connection (or lack thereof) and return a user-facing setup guidance
object/response when missing, otherwise proceed to call
codemode.provider_http_request using connection.handle (mirror the GitHub REST
template's flow). Ensure you reference the existing calls to
codemode.connections_resolve and codemode.provider_http_request in the template
strings and implement the missing-connection early-return/guide path.
Move provider auth and capability discovery toward per-user D1-backed connections and skill templates so provider access can change without redeploys. Add generated UI app sessions so frontend actions and secret submission can reach host-owned worker endpoints without exposing sensitive input to codemode. Made-with: Cursor
Normalize the new connection and generated UI runtime changes to the repository formatting rules so the follow-up validation and review diffs stay focused on behavior. Made-with: Cursor
Use a dedicated default Playwright port so validation does not accidentally reuse unrelated local Wrangler workers and fail the app e2e suite with false negatives. Made-with: Cursor
Make builtin skill template seeding idempotent under concurrent first-use requests and keep seeding failures from aborting unrelated MCP calls. Preserve explicit null clears in connection updates and tighten provider refresh/path guards so retries and host-side requests behave predictably. Made-with: Cursor
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Keep the home connector handler aligned with the current lint autofix output so validate finishes with a clean working tree after the rebase. Made-with: Cursor
kentcdodds
force-pushed
the
dynamic-skills-oauth-runtime
branch
from
March 25, 2026 05:03
a113509 to
b646abe
Compare
Contributor
|
🔎 Preview deployed: https://kody-pr-35.kentcdodds.workers.dev Worker: Mocks:
|
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Unhandled exceptions in secure-input API endpoint
- Wrapped secure-input storage in a try/catch to return structured JSON errors instead of uncaught exceptions.
- ✅ Fixed: Content-Type header set unconditionally on GET requests
- Made the JSON content-type header conditional on actually sending a JSON body for HTTP requests.
Preview (b693cc8cc0)
diff --git a/docs/agents/end-to-end-testing.md b/docs/agents/end-to-end-testing.md
--- a/docs/agents/end-to-end-testing.md
+++ b/docs/agents/end-to-end-testing.md
@@ -37,9 +37,9 @@
## Server and routing
- The test server is started via Playwright `webServer` using Wrangler.
-- The base URL defaults to `http://localhost:8788` for Playwright to avoid
- colliding with the dev server. Override with `PLAYWRIGHT_BASE_URL` or
- `PLAYWRIGHT_PORT`.
+- The base URL defaults to `http://localhost:38888` for Playwright to avoid
+ colliding with the common local Wrangler dev ports. Override with
+ `PLAYWRIGHT_BASE_URL` or `PLAYWRIGHT_PORT`.
- Playwright sets `CLOUDFLARE_ENV=test`; Wrangler still loads
`packages/worker/.env` values for local secrets.
- Ensure the `env.test` section in `packages/worker/wrangler.jsonc` includes
diff --git a/packages/home-connector/app/handlers.ts b/packages/home-connector/app/handlers.ts
--- a/packages/home-connector/app/handlers.ts
+++ b/packages/home-connector/app/handlers.ts
@@ -2,7 +2,7 @@
import { html } from 'remix/html-template'
import { render } from './render.ts'
import { RootLayout } from './root.tsx'
-import type { routes } from './routes.ts'
+import { type routes } from './routes.ts'
import { type HomeConnectorState } from '../src/state.ts'
import { type RokuDiscoveryDiagnostics } from '../src/adapters/roku/types.ts'
import { scanRokuDevices } from '../src/adapters/roku/index.ts'
diff --git a/packages/worker/client/mcp-apps/generated-ui-shell.ts b/packages/worker/client/mcp-apps/generated-ui-shell.ts
--- a/packages/worker/client/mcp-apps/generated-ui-shell.ts
+++ b/packages/worker/client/mcp-apps/generated-ui-shell.ts
@@ -5,11 +5,23 @@
type DisplayMode = 'inline' | 'fullscreen' | 'pip'
type ThemeName = 'light' | 'dark'
+type AppSessionEnvelope = {
+ sessionId: string
+ token: string
+ expiresAt: string
+ endpoints: {
+ appSource: string
+ action: string
+ secureInput: string
+ }
+}
+
type RenderEnvelope = {
mode: RenderMode
code?: string
appId?: string
runtime?: AppRuntime
+ appSession?: AppSessionEnvelope | null
}
type RenderDataEnvelope = {
@@ -32,6 +44,36 @@
return value === 'html' || value === 'javascript' ? value : undefined
}
+function coerceAppSession(value: unknown): AppSessionEnvelope | null {
+ if (!isRecord(value)) return null
+ if (
+ typeof value.sessionId !== 'string' ||
+ typeof value.token !== 'string' ||
+ typeof value.expiresAt !== 'string'
+ ) {
+ return null
+ }
+ const endpoints = isRecord(value.endpoints) ? value.endpoints : null
+ if (
+ !endpoints ||
+ typeof endpoints.appSource !== 'string' ||
+ typeof endpoints.action !== 'string' ||
+ typeof endpoints.secureInput !== 'string'
+ ) {
+ return null
+ }
+ return {
+ sessionId: value.sessionId,
+ token: value.token,
+ expiresAt: value.expiresAt,
+ endpoints: {
+ appSource: endpoints.appSource,
+ action: endpoints.action,
+ secureInput: endpoints.secureInput,
+ },
+ }
+}
+
function coerceDisplayMode(value: unknown): DisplayMode | null {
return value === 'inline' || value === 'fullscreen' || value === 'pip'
? value
@@ -65,9 +107,8 @@
) {
if (/<head\b[^>]*>/i.test(code)) {
const withTheme = theme
- ? code.replace(
- /<html\b[^>]*>/i,
- (match) => injectThemeAttributeIntoHtmlTag(match, theme),
+ ? code.replace(/<html\b[^>]*>/i, (match) =>
+ injectThemeAttributeIntoHtmlTag(match, theme),
)
: code
@@ -118,7 +159,8 @@
: undefined
const appId = typeof value.appId === 'string' ? value.appId : undefined
const runtime = coerceRuntime(value.runtime) ?? (code ? 'html' : undefined)
- return { mode: renderSource, code, appId, runtime }
+ const appSession = coerceAppSession(value.appSession)
+ return { mode: renderSource, code, appId, runtime, appSession }
}
function getEnvelopeFromRenderData(renderData: RenderDataEnvelope | undefined) {
@@ -142,6 +184,7 @@
const childMessagePrefix = 'kody-generated-ui:'
let latestRenderData: RenderDataEnvelope | undefined
+ let latestEnvelope: RenderEnvelope | null = null
function getBaseHref() {
try {
@@ -151,7 +194,7 @@
}
}
- function escapeInlineModuleSource(code: string) {
+ function escapeInlineScriptSource(code: string) {
return code.replace(/<\/script/gi, '<\\/script')
}
@@ -385,28 +428,100 @@
`.trim()
}
- function buildHeadInjection(theme: ThemeName | null) {
+ function buildHeadInjection(
+ theme: ThemeName | null,
+ appSession: AppSessionEnvelope | null,
+ ) {
const baseHref = getBaseHref()
const escapedBaseHref = baseHref ? escapeHtmlAttribute(baseHref) : null
+ const bridgeRuntime = escapeInlineScriptSource(
+ buildChildBridgeRuntimeSource(appSession),
+ )
return `
${escapedBaseHref ? `<base href="${escapedBaseHref}" />` : ''}
<style>
${buildShellStyles(theme)}
</style>
<script>
-${buildChildBridgeRuntimeSource()}
+${bridgeRuntime}
</script>
`.trim()
}
- function buildChildBridgeRuntimeSource() {
+ function buildChildBridgeRuntimeSource(
+ appSession: AppSessionEnvelope | null,
+ ) {
+ const serializedAppSession = JSON.stringify(appSession)
return `
const shellMessagePrefix = '${childMessagePrefix}';
+const appSession = ${serializedAppSession};
let requestCounter = 0;
function nextRequestId() {
requestCounter += 1;
return 'generated-ui-' + requestCounter;
}
+function getAppSessionHeaders() {
+ if (!appSession) return null;
+ return {
+ accept: 'application/json',
+ authorization: 'Bearer ' + appSession.token,
+ };
+}
+function wrapActionCode(code, params) {
+ if (params === undefined) return code;
+ const paramsJson = JSON.stringify(params ?? {});
+ return \`async () => {
+ const params = \${paramsJson};
+ const action = (\${code});
+ return await action(params);
+}\`;
+}
+async function invokeActionWithSession(input) {
+ if (!appSession) return null;
+ const response = await fetch(appSession.endpoints.action, {
+ method: 'POST',
+ headers: {
+ ...getAppSessionHeaders(),
+ 'content-type': 'application/json',
+ },
+ body: JSON.stringify(input),
+ });
+ const payload = await response.json().catch(() => null);
+ if (!payload || typeof payload !== 'object') {
+ return {
+ ok: false,
+ error: 'Generated UI action endpoint returned an invalid response.',
+ };
+ }
+ return payload;
+}
+async function submitSecureInputWithSession(input) {
+ if (!appSession) {
+ return {
+ ok: false,
+ error: 'Secure input is unavailable without a generated UI app session.',
+ };
+ }
+ const response = await fetch(appSession.endpoints.secureInput, {
+ method: 'POST',
+ headers: {
+ ...getAppSessionHeaders(),
+ 'content-type': 'application/json',
+ },
+ body: JSON.stringify({
+ setup_id: input.setupId,
+ fields: input.fields,
+ }),
+ });
+ const payload = await response.json().catch(() => null);
+ if (!payload || typeof payload !== 'object') {
+ return {
+ ok: false,
+ error: 'Secure input endpoint returned an invalid response.',
+ };
+ }
+ return payload;
+}
function waitForShellMessage(type, requestId) {
return new Promise((resolve) => {
function handleMessage(event) {
@@ -463,6 +578,19 @@
},
async executeCode(code) {
if (typeof code !== 'string' || code.length === 0) return null;
+ if (appSession) {
+ const payload = await invokeActionWithSession({ code });
+ if (payload && typeof payload === 'object' && payload.ok === false) {
+ throw new Error(
+ typeof payload.error === 'string'
+ ? payload.error
+ : 'Generated UI action failed.',
+ );
+ }
+ return payload && typeof payload === 'object' && 'result' in payload
+ ? payload.result ?? null
+ : null;
+ }
if (window.parent === window) return null;
const requestId = nextRequestId();
const payload = await postRequestAndWaitForShellMessage(
@@ -479,6 +607,63 @@
}
return 'result' in payload ? payload.result ?? null : null;
},
+ async invokeAction(input) {
+ if (!input || typeof input !== 'object') {
+ return {
+ ok: false,
+ error: 'Action input must be an object.',
+ };
+ }
+ const code =
+ typeof input.code === 'string' && input.code.length > 0 ? input.code : null;
+ if (!code) {
+ return {
+ ok: false,
+ error: 'Action code must be a non-empty string.',
+ };
+ }
+ const params =
+ input.params && typeof input.params === 'object' ? input.params : undefined;
+ if (appSession) {
+ return await invokeActionWithSession(
+ params === undefined ? { code } : { code, params },
+ );
+ }
+ try {
+ const result = await this.executeCode(wrapActionCode(code, params));
+ return {
+ ok: true,
+ result,
+ };
+ } catch (error) {
+ return {
+ ok: false,
+ error:
+ error instanceof Error ? error.message : 'Generated UI action failed.',
+ };
+ }
+ },
+ async submitSecureInput(input) {
+ if (!input || typeof input !== 'object') {
+ return {
+ ok: false,
+ error: 'Secure input must be an object.',
+ };
+ }
+ const setupId =
+ typeof input.setupId === 'string' && input.setupId.length > 0
+ ? input.setupId
+ : null;
+ const fields =
+ input.fields && typeof input.fields === 'object' ? input.fields : null;
+ if (!setupId || !fields) {
+ return {
+ ok: false,
+ error: 'Secure input requires setupId and fields.',
+ };
+ }
+ return await submitSecureInputWithSession({ setupId, fields });
+ },
};
window.addEventListener('error', (event) => {
console.error(
@@ -496,9 +681,13 @@
}
function buildInlineModuleSource(code: string) {
- const safeCode = escapeInlineModuleSource(code)
+ const safeCode = escapeInlineScriptSource(code)
+ const appSession = latestEnvelope?.appSession ?? null
+ const bridgeRuntime = escapeInlineScriptSource(
+ buildChildBridgeRuntimeSource(appSession),
+ )
return `
-${buildChildBridgeRuntimeSource()}
+${bridgeRuntime}
${safeCode}
`.trim()
}
@@ -519,13 +708,14 @@
function buildHtmlDocumentFromFragment(code: string) {
const theme = coerceTheme(latestRenderData?.theme)
+ const appSession = latestEnvelope?.appSession ?? null
return `
<!doctype html>
<html lang="en"${theme ? ` data-kody-theme="${theme}"` : ''}>
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1" />
- ${buildHeadInjection(theme)}
+ ${buildHeadInjection(theme, appSession)}
</head>
<body data-kody-runtime="fragment">
${code}
@@ -560,13 +750,45 @@
}
const theme = coerceTheme(latestRenderData?.theme)
+ const appSession = latestEnvelope?.appSession ?? null
const htmlSource = /<(?:!doctype|html|head|body)\b/i.test(code)
- ? injectIntoHtmlDocument(code, buildHeadInjection(theme), theme)
+ ? injectIntoHtmlDocument(
+ code,
+ buildHeadInjection(theme, appSession),
+ theme,
+ )
: buildHtmlDocumentFromFragment(code)
frameElement.srcdoc = htmlSource
}
async function resolveSavedAppCode(appId: string) {
+ const appSession = latestEnvelope?.appSession ?? null
+ if (appSession) {
+ const url = new URL(appSession.endpoints.appSource)
+ url.searchParams.set('app_id', appId)
+ const response = await fetch(url.toString(), {
+ headers: {
+ accept: 'application/json',
+ authorization: `Bearer ${appSession.token}`,
+ },
+ })
+ const payload = (await response.json().catch(() => null)) as Record<
+ string,
+ unknown
+ > | null
+ const app = isRecord(payload?.app) ? payload.app : null
+ if (!response.ok || payload?.ok !== true || !app) {
+ throw new Error('Failed to load saved app source.')
+ }
+ const code = typeof app.code === 'string' ? app.code : null
+ if (!code) {
+ throw new Error('Saved app source is missing code.')
+ }
+ return {
+ code,
+ runtime: coerceRuntime(app.runtime) ?? 'html',
+ }
+ }
const result = (await hostBridge.callTool({
name: 'ui_load_app_source',
arguments: { app_id: appId },
@@ -591,6 +813,7 @@
}
async function renderEnvelope(envelope: RenderEnvelope | null) {
+ latestEnvelope = envelope
if (!envelope) {
frameElement.srcdoc = ''
return
diff --git a/packages/worker/migrations/0008-provider-connections.sql b/packages/worker/migrations/0008-provider-connections.sql
new file mode 100644
--- /dev/null
+++ b/packages/worker/migrations/0008-provider-connections.sql
@@ -1,0 +1,67 @@
+CREATE TABLE IF NOT EXISTS connection_drafts (
+ id TEXT PRIMARY KEY NOT NULL,
+ user_id TEXT NOT NULL,
+ provider_key TEXT NOT NULL,
+ display_name TEXT NOT NULL,
+ label TEXT,
+ auth_spec_json TEXT NOT NULL,
+ status TEXT NOT NULL DEFAULT 'draft',
+ state_json TEXT,
+ error_message TEXT,
+ created_at TEXT NOT NULL DEFAULT (CURRENT_TIMESTAMP),
+ updated_at TEXT NOT NULL DEFAULT (CURRENT_TIMESTAMP),
+ expires_at TEXT NOT NULL
+);
+
+CREATE INDEX IF NOT EXISTS idx_connection_drafts_user_provider
+ ON connection_drafts(user_id, provider_key);
+
+CREATE INDEX IF NOT EXISTS idx_connection_drafts_expires_at
+ ON connection_drafts(expires_at);
+
+CREATE TABLE IF NOT EXISTS connection_draft_secrets (
+ draft_id TEXT NOT NULL,
+ secret_name TEXT NOT NULL,
+ encrypted_value TEXT NOT NULL,
+ created_at TEXT NOT NULL DEFAULT (CURRENT_TIMESTAMP),
+ updated_at TEXT NOT NULL DEFAULT (CURRENT_TIMESTAMP),
+ PRIMARY KEY (draft_id, secret_name),
+ FOREIGN KEY (draft_id) REFERENCES connection_drafts(id) ON DELETE CASCADE
+);
+
+CREATE TABLE IF NOT EXISTS provider_connections (
+ id TEXT PRIMARY KEY NOT NULL,
+ user_id TEXT NOT NULL,
+ provider_key TEXT NOT NULL,
+ display_name TEXT NOT NULL,
+ label TEXT NOT NULL,
+ auth_spec_json TEXT NOT NULL,
+ status TEXT NOT NULL DEFAULT 'active',
+ account_id TEXT,
+ account_label TEXT,
+ scope_set TEXT,
+ metadata_json TEXT,
+ is_default INTEGER NOT NULL DEFAULT 0,
+ token_expires_at TEXT,
+ last_used_at TEXT,
+ created_at TEXT NOT NULL DEFAULT (CURRENT_TIMESTAMP),
+ updated_at TEXT NOT NULL DEFAULT (CURRENT_TIMESTAMP)
+);
+
+CREATE INDEX IF NOT EXISTS idx_provider_connections_user_provider
+ ON provider_connections(user_id, provider_key);
+
+CREATE UNIQUE INDEX IF NOT EXISTS idx_provider_connections_default
+ ON provider_connections(user_id, provider_key)
+ WHERE is_default = 1;
+
+CREATE UNIQUE INDEX IF NOT EXISTS idx_provider_connections_user_label
+ ON provider_connections(user_id, provider_key, label);
+
+CREATE TABLE IF NOT EXISTS provider_connection_secrets (
+ connection_id TEXT PRIMARY KEY NOT NULL,
+ encrypted_secret_json TEXT NOT NULL,
+ created_at TEXT NOT NULL DEFAULT (CURRENT_TIMESTAMP),
+ updated_at TEXT NOT NULL DEFAULT (CURRENT_TIMESTAMP),
+ FOREIGN KEY (connection_id) REFERENCES provider_connections(id) ON DELETE CASCADE
+);
diff --git a/packages/worker/migrations/0009-skill-connection-bindings.sql b/packages/worker/migrations/0009-skill-connection-bindings.sql
new file mode 100644
--- /dev/null
+++ b/packages/worker/migrations/0009-skill-connection-bindings.sql
@@ -1,0 +1,6 @@
+ALTER TABLE mcp_skills ADD COLUMN connection_bindings TEXT;
+ALTER TABLE mcp_skills ADD COLUMN template_key TEXT;
+
+CREATE UNIQUE INDEX IF NOT EXISTS idx_mcp_skills_user_template_key
+ ON mcp_skills(user_id, template_key)
+ WHERE template_key IS NOT NULL;
diff --git a/packages/worker/src/index.ts b/packages/worker/src/index.ts
--- a/packages/worker/src/index.ts
+++ b/packages/worker/src/index.ts
@@ -23,6 +23,14 @@
mcpResourcePath,
protectedResourceMetadataPath,
} from './mcp-auth.ts'
+import {
+ handleGeneratedUiApiRequest,
+ isGeneratedUiApiRequest,
+} from './mcp/generated-ui-api.ts'
+import {
+ handleConnectionOAuthRequest,
+ isConnectionOAuthRequest,
+} from './mcp/connections/oauth-api.ts'
import { withCors } from './utils.ts'
import { handleCapabilityReindexRequest } from './capability-maintenance.ts'
import { handleSkillReindexRequest } from './skill-maintenance.ts'
@@ -31,16 +39,24 @@
const appHandler = withCors({
getCorsHeaders(request) {
+ const url = new URL(request.url)
+ if (isGeneratedUiApiRequest(url.pathname)) {
+ return new Headers({
+ 'Access-Control-Allow-Origin': '*',
+ 'Access-Control-Allow-Methods': 'GET, POST, OPTIONS',
+ 'Access-Control-Allow-Headers': 'content-type, authorization',
+ })
+ }
const origin = request.headers.get('Origin')
if (!origin) return null
- const requestOrigin = new URL(request.url).origin
+ const requestOrigin = url.origin
if (origin !== requestOrigin) return null
- return {
+ return new Headers({
'Access-Control-Allow-Origin': origin,
'Access-Control-Allow-Methods': 'GET, POST, OPTIONS',
'Access-Control-Allow-Headers': 'content-type, authorization',
Vary: 'Origin',
- }
+ })
},
async handler(request, env, ctx) {
const url = new URL(request.url)
@@ -65,6 +81,10 @@
return handleOAuthCallback(request)
}
+ if (isConnectionOAuthRequest(url.pathname)) {
+ return handleConnectionOAuthRequest(request, env)
+ }
+
if (url.pathname === '/.well-known/appspecific/com.chrome.devtools.json') {
return new Response(null, { status: 204 })
}
@@ -84,6 +104,10 @@
})
}
+ if (isGeneratedUiApiRequest(url.pathname)) {
+ return handleGeneratedUiApiRequest(request, env)
+ }
+
if (url.pathname.startsWith('/home/connectors/')) {
const parts = url.pathname.split('/').filter(Boolean)
const connectorId = parts[2]?.trim()
diff --git a/packages/worker/src/mcp-auth.ts b/packages/worker/src/mcp-auth.ts
--- a/packages/worker/src/mcp-auth.ts
+++ b/packages/worker/src/mcp-auth.ts
@@ -3,12 +3,25 @@
type TokenSummary,
} from '@cloudflare/workers-oauth-provider'
import { createMcpCallerContext, type McpServerProps } from './mcp/context.ts'
+import { ensureBuiltinSkillTemplatesForUser } from '#mcp/skills/builtin-skill-templates.ts'
import { oauthScopes } from './oauth-handlers.ts'
export const mcpResourcePath = '/mcp'
export const protectedResourceMetadataPath =
'/.well-known/oauth-protected-resource'
+const builtinTemplateSeedCooldownMs = 5 * 60 * 1000
+const builtinTemplateSeedMaxUsers = 1000
+const builtinTemplateSeedByUser = new Map<string, number>()
+function recordBuiltinTemplateSeed(userId: string, seededAt: number) {
+ builtinTemplateSeedByUser.set(userId, seededAt)
+ while (builtinTemplateSeedByUser.size > builtinTemplateSeedMaxUsers) {
+ const oldestKey = builtinTemplateSeedByUser.keys().next().value
+ if (!oldestKey) break
+ builtinTemplateSeedByUser.delete(oldestKey)
+ }
+}
+
type OAuthEnv = Env & {
OAUTH_PROVIDER?: OAuthHelpers
}
@@ -111,6 +124,28 @@
user: tokenSummary.grant.props ?? null,
homeConnectorId: 'default',
})
+ if (props.user?.userId) {
+ const now = Date.now()
+ const lastSeededAt = builtinTemplateSeedByUser.get(props.user.userId)
+ if (lastSeededAt && now - lastSeededAt < builtinTemplateSeedCooldownMs) {
+ context.props = props
+ return fetchMcp(
+ request,
+ env,
+ context as ExecutionContext<OAuthContextProps>,
+ )
+ }
+ try {
+ await ensureBuiltinSkillTemplatesForUser(env, props.user.userId)
+ } catch (error) {
+ console.warn('Failed to ensure builtin MCP skill templates', {
+ userId: props.user.userId,
+ error: error instanceof Error ? error.message : String(error),
+ })
+ } finally {
+ recordBuiltinTemplateSeed(props.user.userId, now)
+ }
+ }
context.props = props
return fetchMcp(request, env, context as ExecutionContext<OAuthContextProps>)
diff --git a/packages/worker/src/mcp/capabilities/builtin-domains.ts b/packages/worker/src/mcp/capabilities/builtin-domains.ts
--- a/packages/worker/src/mcp/capabilities/builtin-domains.ts
+++ b/packages/worker/src/mcp/capabilities/builtin-domains.ts
@@ -1,5 +1,6 @@
import { appsDomain } from './apps/domain.ts'
import { codingDomain } from './coding/domain.ts'
+import { connectionsDomain } from './connections/domain.ts'
import { metaDomain } from './meta/domain.ts'
/**
@@ -10,4 +11,9 @@
* extra domain is a real `DomainSpec` with bundled `Capability` handlers
* (Workers typically snapshot capabilities at deploy time).
*/
-export const builtinDomains = [appsDomain, codingDomain, metaDomain] as const
+export const builtinDomains = [
+ appsDomain,
+ codingDomain,
+ connectionsDomain,
+ metaDomain,
+] as const
diff --git a/packages/worker/src/mcp/capabilities/connections/connections-begin-setup.ts b/packages/worker/src/mcp/capabilities/connections/connections-begin-setup.ts
new file mode 100644
--- /dev/null
+++ b/packages/worker/src/mcp/capabilities/connections/connections-begin-setup.ts
@@ -1,0 +1,72 @@
+import { z } from 'zod'
+import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
+import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
+import { type CapabilityContext } from '#mcp/capabilities/types.ts'
+import { connectionAuthSpecSchema } from '#mcp/connections/auth-spec.ts'
+import { beginConnectionSetup } from '#mcp/connections/connection-service.ts'
+import { requireMcpUser } from '#mcp/capabilities/meta/require-user.ts'
+
+const outputSchema = z.object({
+ setup_id: z.string(),
+ provider: z.object({
+ key: z.string(),
+ display_name: z.string(),
+ }),
+ label: z.string().nullable(),
+ auth_strategy: z.string(),
+ status: z.string(),
+ secret_fields: z.array(
+ z.object({
+ name: z.string(),
+ label: z.string(),
+ description: z.string().optional(),
+ input_type: z.enum(['text', 'password']),
+ }),
+ ),
+ instructions: z.array(z.string()),
+ expires_at: z.string(),
+})
+
+export const connectionsBeginSetupCapability = defineDomainCapability(
+ capabilityDomainNames.connections,
+ {
+ name: 'connections_begin_setup',
+ description:
+ 'Begin a user-owned provider connection draft. Use this when a skill can do something useful after initiating setup, such as returning a setup id that a generated UI can continue with secure secret collection or OAuth.',
+ keywords: [
+ 'connection',
+ 'provider',
+ 'oauth',
+ 'api key',
+ 'manual token',
+ 'secure input',
+ 'setup',
+ ],
+ readOnly: false,
+ idempotent: false,
+ destructive: false,
+ inputSchema: z.object({
+ provider: z.object({
+ key: z.string().min(1),
+ display_name: z.string().min(1),
+ }),
+ auth: connectionAuthSpecSchema,
+ label: z
+ .string()
+ .min(1)
+ .optional()
+ .describe('Optional preferred label for the resulting connection.'),
+ }),
+ outputSchema,
+ async handler(args, ctx: CapabilityContext) {
+ const user = requireMcpUser(ctx.callerContext)
+ return beginConnectionSetup({
+ env: ctx.env,
+ userId: user.userId,
+ provider: args.provider,
+ auth: args.auth,
+ label: args.label,
+ })
+ },
+ },
+)
diff --git a/packages/worker/src/mcp/capabilities/connections/connections-disconnect.ts b/packages/worker/src/mcp/capabilities/connections/connections-disconnect.ts
new file mode 100644
--- /dev/null
+++ b/packages/worker/src/mcp/capabilities/connections/connections-disconnect.ts
@@ -1,0 +1,34 @@
+import { z } from 'zod'
+import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
+import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
+import { type CapabilityContext } from '#mcp/capabilities/types.ts'
+import { disconnectConnection } from '#mcp/connections/connection-service.ts'
+import { requireMcpUser } from '#mcp/capabilities/meta/require-user.ts'
+
+export const connectionsDisconnectCapability = defineDomainCapability(
+ capabilityDomainNames.connections,
+ {
+ name: 'connections_disconnect',
+ description:
+ 'Disconnect and delete a stored provider connection for the signed-in user.',
+ keywords: ['connection', 'disconnect', 'revoke', 'delete', 'provider'],
+ readOnly: false,
+ idempotent: false,
+ destructive: true,
+ inputSchema: z.object({
+ connection_id: z.string().min(1),
+ }),
+ outputSchema: z.object({
+ connection_id: z.string(),
+ disconnected: z.boolean(),
+ }),
+ async handler(args, ctx: CapabilityContext) {
+ const user = requireMcpUser(ctx.callerContext)
+ return disconnectConnection({
+ env: ctx.env,
+ userId: user.userId,
+ connectionId: args.connection_id,
+ })
+ },
+ },
+)
diff --git a/packages/worker/src/mcp/capabilities/connections/connections-finalize.ts b/packages/worker/src/mcp/capabilities/connections/connections-finalize.ts
new file mode 100644
--- /dev/null
+++ b/packages/worker/src/mcp/capabilities/connections/connections-finalize.ts
@@ -1,0 +1,50 @@
+import { z } from 'zod'
+import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
+import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
+import { type CapabilityContext } from '#mcp/capabilities/types.ts'
+import { finalizeConnectionSetup } from '#mcp/connections/connection-service.ts'
+import { requireMcpUser } from '#mcp/capabilities/meta/require-user.ts'
+
+export const connectionsFinalizeCapability = defineDomainCapability(
+ capabilityDomainNames.connections,
+ {
+ name: 'connections_finalize',
+ description:
+ 'Finalize a connection draft into a durable user-owned provider connection after secure inputs or OAuth credentials have been collected.',
+ keywords: [
+ 'connection',
+ 'finalize',
+ 'provider',
+ 'oauth',
+ 'token',
+ 'api key',
+ ],
+ readOnly: false,
+ idempotent: false,
+ destructive: false,
+ inputSchema: z.object({
+ setup_id: z.string().min(1),
+ make_default: z.boolean().optional(),
+ }),
+ outputSchema: z.object({
+ connection_id: z.string(),
+ provider_key: z.string(),
+ display_name: z.string(),
+ label: z.string(),
... diff truncated: showing 800 of 5036 linesCo-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
| started_at: new Date().toISOString(), | ||
| }), | ||
| error_message: null, | ||
| }) |
Contributor
There was a problem hiding this comment.
PKCE code verifier stored unencrypted in database
Medium Severity
The PKCE code_verifier is stored as plaintext in the state_json column of connection_drafts. PKCE's security relies on the code verifier being kept confidential. Other secret values in the draft flow are encrypted via encryptJson before storage, but the code verifier bypasses this pattern, leaving it exposed to anyone with database read access.
| .replaceAll('+', '-') | ||
| .replaceAll('/', '_') | ||
| .replace(/=+$/g, '') | ||
| } |
Contributor
There was a problem hiding this comment.
Redundant base64UrlString function duplicated across files
Low Severity
base64UrlString in connection-service.ts duplicates the base64UrlEncode function in crypto.ts. Both perform identical base64url encoding of Uint8Array data. The connection-service.ts version uses the spread-in-fromCharCode pattern while crypto.ts uses a loop, but they produce the same output. The service file already imports from crypto.ts, so the shared utility could be reused instead of duplicated.
Additional Locations (1)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/client/mcp-apps/generated-ui-shell.ts`:
- Around line 469-489: The generated bridge currently serializes appSession
including appSession.token into buildChildBridgeRuntimeSource, exposing the
bearer to injected app code; remove token injection and instead proxy
authenticated operations via postMessage: stop embedding appSession.token in
serializedAppSession, change getAppSessionHeaders (in the generated runtime) to
send a postMessage using shellMessagePrefix and nextRequestId and await a
response from the host frame, and update any code paths that used
appSession.token to call this message-based proxy (host will perform fetches or
return headers). Keep buildChildBridgeRuntimeSource, appSession (without token),
getAppSessionHeaders, shellMessagePrefix and nextRequestId as the contract
points so the host can handle requests and respond with headers/results.
In `@packages/worker/src/mcp/generated-ui-api.ts`:
- Around line 57-65: The error responses in the action/secure-input handlers
(around the request.method check, the actionRequestSchema.safeParse validation,
and the verification/storage catch blocks) currently return only { error } or
echo raw exceptions and sometimes map server faults into 4xx; change every
handled error branch to return jsonResponse({ ok: false, error: <message> },
<status>) instead of { error } (e.g., method not allowed -> jsonResponse({ ok:
false, error: 'Method not allowed.' }, 405); validation -> jsonResponse({ ok:
false, error: body.error.message }, 400)), and for unexpected
verification/storage exceptions catch and log the exception internally but
return a generic jsonResponse({ ok: false, error: 'Internal server error' },
500); update all jsonResponse invocations in this file (including the other
ranges noted) accordingly so the client always receives { ok: false, error } and
internal errors aren’t leaked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a8e33dd-666a-4c3e-9381-805c270054b4
📒 Files selected for processing (47)
docs/agents/end-to-end-testing.mdpackages/home-connector/app/handlers.tspackages/worker/client/mcp-apps/generated-ui-shell.tspackages/worker/migrations/0008-provider-connections.sqlpackages/worker/migrations/0009-skill-connection-bindings.sqlpackages/worker/src/index.tspackages/worker/src/mcp-auth.tspackages/worker/src/mcp/capabilities/builtin-domains.tspackages/worker/src/mcp/capabilities/connections/connections-begin-setup.tspackages/worker/src/mcp/capabilities/connections/connections-disconnect.tspackages/worker/src/mcp/capabilities/connections/connections-finalize.tspackages/worker/src/mcp/capabilities/connections/connections-list.tspackages/worker/src/mcp/capabilities/connections/connections-resolve.tspackages/worker/src/mcp/capabilities/connections/connections-set-default.tspackages/worker/src/mcp/capabilities/connections/connections-start-oauth.tspackages/worker/src/mcp/capabilities/connections/domain.tspackages/worker/src/mcp/capabilities/connections/provider-graphql-request.tspackages/worker/src/mcp/capabilities/connections/provider-http-request.tspackages/worker/src/mcp/capabilities/connections/provider-refresh-token.tspackages/worker/src/mcp/capabilities/domain-metadata.tspackages/worker/src/mcp/capabilities/meta/meta-get-skill.tspackages/worker/src/mcp/capabilities/meta/meta-save-skill.tspackages/worker/src/mcp/capabilities/meta/meta-update-skill.tspackages/worker/src/mcp/capabilities/unified-search.tspackages/worker/src/mcp/connections/auth-spec.tspackages/worker/src/mcp/connections/connection-drafts-repo.tspackages/worker/src/mcp/connections/connection-drafts-types.tspackages/worker/src/mcp/connections/connection-handles.tspackages/worker/src/mcp/connections/connection-service.tspackages/worker/src/mcp/connections/crypto.tspackages/worker/src/mcp/connections/oauth-api.tspackages/worker/src/mcp/connections/provider-connections-repo.tspackages/worker/src/mcp/connections/provider-connections-types.tspackages/worker/src/mcp/connections/provider-request.tspackages/worker/src/mcp/generated-ui-api.tspackages/worker/src/mcp/generated-ui-app-session.tspackages/worker/src/mcp/index.tspackages/worker/src/mcp/mcp-server-e2e.test.tspackages/worker/src/mcp/skills/builtin-skill-templates.tspackages/worker/src/mcp/skills/mcp-skills-repo.tspackages/worker/src/mcp/skills/mcp-skills-types.tspackages/worker/src/mcp/skills/skill-connections.tspackages/worker/src/mcp/skills/skill-embed-and-flags.tspackages/worker/src/mcp/skills/skill-mutation.tspackages/worker/src/mcp/tools/open-generated-ui.tspackages/worker/src/mcp/tools/search.tsplaywright.config.ts
✅ Files skipped from review due to trivial changes (13)
- docs/agents/end-to-end-testing.md
- packages/worker/src/mcp/index.ts
- packages/worker/src/mcp/capabilities/domain-metadata.ts
- packages/worker/src/mcp/tools/search.ts
- packages/worker/src/mcp/skills/mcp-skills-types.ts
- packages/worker/src/mcp/mcp-server-e2e.test.ts
- packages/worker/src/mcp/connections/provider-connections-types.ts
- packages/home-connector/app/handlers.ts
- packages/worker/src/mcp/connections/connection-drafts-types.ts
- packages/worker/src/mcp/capabilities/connections/provider-refresh-token.ts
- playwright.config.ts
- packages/worker/migrations/0008-provider-connections.sql
- packages/worker/src/mcp/connections/provider-request.ts
🚧 Files skipped from review as they are similar to previous changes (23)
- packages/worker/migrations/0009-skill-connection-bindings.sql
- packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts
- packages/worker/src/mcp/capabilities/builtin-domains.ts
- packages/worker/src/mcp/capabilities/connections/domain.ts
- packages/worker/src/mcp/capabilities/meta/meta-get-skill.ts
- packages/worker/src/mcp/capabilities/connections/connections-set-default.ts
- packages/worker/src/mcp/capabilities/connections/provider-graphql-request.ts
- packages/worker/src/mcp/connections/oauth-api.ts
- packages/worker/src/mcp/capabilities/connections/provider-http-request.ts
- packages/worker/src/mcp/capabilities/meta/meta-update-skill.ts
- packages/worker/src/mcp/capabilities/connections/connections-begin-setup.ts
- packages/worker/src/mcp/generated-ui-app-session.ts
- packages/worker/src/mcp/capabilities/connections/connections-start-oauth.ts
- packages/worker/src/mcp/skills/skill-embed-and-flags.ts
- packages/worker/src/index.ts
- packages/worker/src/mcp/skills/skill-connections.ts
- packages/worker/src/mcp/capabilities/connections/connections-resolve.ts
- packages/worker/src/mcp/skills/mcp-skills-repo.ts
- packages/worker/src/mcp/capabilities/connections/connections-list.ts
- packages/worker/src/mcp/connections/crypto.ts
- packages/worker/src/mcp/tools/open-generated-ui.ts
- packages/worker/src/mcp/connections/connection-drafts-repo.ts
- packages/worker/src/mcp/connections/provider-connections-repo.ts
Comment on lines
+469
to
+489
| function buildChildBridgeRuntimeSource( | ||
| appSession: AppSessionEnvelope | null, | ||
| ) { | ||
| const serializedAppSession = escapeInlineJsonSource( | ||
| JSON.stringify(appSession), | ||
| ) | ||
| return ` | ||
| const shellMessagePrefix = '${childMessagePrefix}'; | ||
| const appSession = ${serializedAppSession}; | ||
| let requestCounter = 0; | ||
| function nextRequestId() { | ||
| requestCounter += 1; | ||
| return 'generated-ui-' + requestCounter; | ||
| } | ||
| function getAppSessionHeaders() { | ||
| if (!appSession) return null; | ||
| return { | ||
| accept: 'application/json', | ||
| authorization: 'Bearer ' + appSession.token, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Do not inject the app-session bearer into the generated app's JS realm.
This bridge source is injected into HTML documents and prepended to inline module code, so both runtimes expose appSession.token to arbitrary generated app code and any third-party script it loads. That token can be replayed directly against the generated-UI action/secure-input/app-source endpoints, so the trust boundary effectively collapses to the app code itself. Keep the bearer in the host frame and proxy these calls over postMessage instead of embedding the token into the rendered document.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/client/mcp-apps/generated-ui-shell.ts` around lines 469 -
489, The generated bridge currently serializes appSession including
appSession.token into buildChildBridgeRuntimeSource, exposing the bearer to
injected app code; remove token injection and instead proxy authenticated
operations via postMessage: stop embedding appSession.token in
serializedAppSession, change getAppSessionHeaders (in the generated runtime) to
send a postMessage using shellMessagePrefix and nextRequestId and await a
response from the host frame, and update any code paths that used
appSession.token to call this message-based proxy (host will perform fetches or
return headers). Keep buildChildBridgeRuntimeSource, appSession (without token),
getAppSessionHeaders, shellMessagePrefix and nextRequestId as the contract
points so the host can handle requests and respond with headers/results.
Comment on lines
+57
to
+65
| if (request.method !== 'POST') { | ||
| return jsonResponse({ error: 'Method not allowed.' }, 405) | ||
| } | ||
| const body = actionRequestSchema.safeParse( | ||
| await request.json().catch(() => null), | ||
| ) | ||
| if (!body.success) { | ||
| return jsonResponse({ error: body.error.message }, 400) | ||
| } |
There was a problem hiding this comment.
Standardize generated-UI failures as { ok: false }, and don't flatten server faults into 4xx.
packages/worker/client/mcp-apps/generated-ui-shell.ts consumes the action/secure-input routes as { ok, error }, but the method/validation/auth branches here still return { error } only. On top of that, the catch-all blocks turn unexpected verification/storage failures into 400/401 and echo raw exception messages. That combination can make expired/broken sessions look like successful null results on the client and leaks internal failure details. Return { ok: false, error } on every handled error path, and reserve a generic 500 for unexpected exceptions after logging them.
Also applies to: 94-123, 131-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/generated-ui-api.ts` around lines 57 - 65, The error
responses in the action/secure-input handlers (around the request.method check,
the actionRequestSchema.safeParse validation, and the verification/storage catch
blocks) currently return only { error } or echo raw exceptions and sometimes map
server faults into 4xx; change every handled error branch to return
jsonResponse({ ok: false, error: <message> }, <status>) instead of { error }
(e.g., method not allowed -> jsonResponse({ ok: false, error: 'Method not
allowed.' }, 405); validation -> jsonResponse({ ok: false, error:
body.error.message }, 400)), and for unexpected verification/storage exceptions
catch and log the exception internally but return a generic jsonResponse({ ok:
false, error: 'Internal server error' }, 500); update all jsonResponse
invocations in this file (including the other ranges noted) accordingly so the
client always receives { ok: false, error } and internal errors aren’t leaked.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
| user, | ||
| iat: now, | ||
| exp: expiresAtMs, | ||
| }) |
Contributor
There was a problem hiding this comment.
Signed token discarded, app session always null
High Severity
createGeneratedUiAppSession calls await signToken(...) but discards the return value — the signed token string is never stored or included in the returned object. The GeneratedUiAppSession type also lacks a token field. On the client side, AppSessionEnvelope requires token: string, so coerceAppSession always returns null when typeof value.token !== 'string' fails. This makes appSession permanently null in the iframe, completely disabling the direct worker action and secure-input endpoints.
Additional Locations (1)
| 'Access-Control-Allow-Headers': 'content-type, authorization', | ||
| Vary: 'Origin', | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
CORS missing custom session header allowance
Medium Severity
The CORS headers for generated UI API requests only allow content-type, authorization in Access-Control-Allow-Headers, but the client sends the custom X-Generated-Ui-Session header. Cross-origin preflight requests from srcdoc iframes or Claude widget domains would be rejected by the browser because this header isn't listed in the allowed headers.
| if (origin) return origin === url.origin | ||
| if (secFetchSite) return secFetchSite === 'same-origin' | ||
| return true | ||
| } |
Contributor
There was a problem hiding this comment.
Same-origin check rejects legitimate cross-origin iframe requests
Medium Severity
isSameOriginRequest checks origin === url.origin, but generated UI iframes use srcdoc (sending origin "null") or run on Claude widget domains — neither matches the worker's origin. This causes the action and secure-input POST endpoints to always return 403 for legitimate generated UI requests, contradicting the CORS configuration that explicitly allows these origins.
tk-shockwave
pushed a commit
to tk-shockwave/kody
that referenced
this pull request
Oct 17, 2026
* feat(execute): add --credential call map for ADR 0068 Parse repeatable --credential key=name|org:name|personal:name flags into the CapabilityProxy/MCP credentials map. Local execute preflights via credentialClientCheck, attaches the map outside args on every hop, and requires session feature credential-call-map. MCP execute receives the map in tool args; token-auth cloud execute rejects the flag (map stays off kody.execute). Sync host credential()/credentials() with openAll and hint. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * fix(execute): align --credential with final ADR 0068 host contract Reject empty+named picks for the same key, trim pick names, use the exact feature-detection refusal message, and render credentialClient* ok:false errors with code/message plus --credential retry suggestions. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * fix(execute): trim --credential picks before org/personal scope parse A leading space before org: or personal: must not turn a scoped pick into a plain credential name. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
bun run formatbun run validatebun run test:mcpMade with Cursor
Note
High Risk
High risk because it adds new credential storage/encryption, OAuth callback handling, and cross-origin generated UI APIs (CORS + bearer/session headers), plus new D1 schema that affects persisted user data.
Overview
Adds a D1-backed provider connection runtime with draft setup/finalization, encrypted secret storage, OAuth initiation/callback handling, default-connection selection, and opaque short-lived connection handles for provider requests.
Introduces a new
connectionsMCP capability domain (begin/list/resolve/set-default/disconnect/start-oauth/finalize plus authenticatedprovider_http_request/provider_graphql_request/provider_refresh_token), and extends skills/search to persist and surfaceconnection_bindingsand seed per-user builtin skill templates.Upgrades the generated UI shell/tooling to create generated UI app sessions and support direct worker endpoints for
invokeActionandsubmitSecureInput(with stricter origin/CORS checks), and tweaks Playwright docs/config defaults (port/base URL) plus a minor type-only import fix.Written by Cursor Bugbot for commit 95362a9. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes
invokeAction,submitSecureInput).