Repository navigation
feat: add connection-backed skill runtime - #44
Closed
kentcdodds wants to merge 19 commits into
Closed
kentcdodds wants to merge 19 commits into
kentcdodds wants to merge 19 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive provider connections system enabling users to create, OAuth-authenticate, and persist connection credentials for third-party providers. It adds provider request capabilities (HTTP, GraphQL), skill connection bindings, builtin skill templates, generated-UI app sessions, and secure input handling for sensitive credential fields. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant MCP as MCP Server
participant DB as Database
participant Provider as Third-Party Provider
participant Crypto as Crypto Utilities
Client->>MCP: connections_begin_setup(provider, auth_spec, label)
MCP->>DB: insert connection_draft
MCP->>DB: insert placeholder connection_draft_secrets
MCP->>MCP: return setup_id, secret_fields, instructions
Client->>MCP: store_draft_secrets(setup_id, {field_values})
MCP->>Crypto: encryptJson(field_values)
Crypto-->>MCP: encrypted_value
MCP->>DB: upsert connection_draft_secrets
MCP->>MCP: return status update
Client->>MCP: connections_start_oauth(setup_id)
MCP->>DB: load connection_draft
MCP->>Crypto: PKCE code_challenge, state signing
Crypto-->>MCP: code_challenge, signed_state
MCP->>DB: update draft with awaiting_oauth_callback status
MCP->>MCP: return authorize_url
Client->>Provider: redirect to authorize_url
Provider->>Client: redirect /api/connections/oauth/callback?code=...&state=...
Client->>MCP: GET /api/connections/oauth/callback
MCP->>Crypto: verifyToken(signed_state)
MCP->>DB: load connection_draft via state
MCP->>Provider: exchange code + PKCE verifier for tokens
Provider-->>MCP: access_token, refresh_token, scope
MCP->>Crypto: encryptJson(token_fields)
MCP->>DB: upsert connection_draft_secrets with tokens
MCP->>MCP: return HTML confirmation
Client->>MCP: connections_finalize(setup_id, make_default=true)
MCP->>DB: load & validate draft + secrets
MCP->>Provider: verify draft via verification request
Provider-->>MCP: account_id, account_label, scope_set
MCP->>DB: insert provider_connection
MCP->>DB: upsert provider_connection_secrets (encrypted)
MCP->>DB: delete connection_draft_secrets
MCP->>DB: mark draft as completed
MCP->>MCP: return connection metadata
sequenceDiagram
participant Child as Generated UI (iframe)
participant Parent as Kody Widget (parent frame)
participant Session as App Session
participant API as Generated UI API
participant MCP as MCP Server
Child->>Parent: window.parent.postMessage({type: 'invoke-action', code, params})
Parent->>Session: validate appSession.token
Parent->>API: POST /api/generated-ui/actions (token, code, params)
API->>MCP: runCodemodeWithRegistry(code, params, callerContext from token)
MCP-->>API: {ok:true, result, logs}
API-->>Parent: response
Parent->>Child: postMessage({ok:true, result, logs})
Child->>Parent: window.parent.postMessage({type: 'submit-secure-input', setupId, fields})
Parent->>Session: validate appSession.token
Parent->>API: POST /api/generated-ui/secure-input (token, setupId, fields)
API->>MCP: storeDraftSecrets(userId from token, setupId, fields)
MCP-->>API: {ok:true}
API-->>Parent: response
Parent->>Child: postMessage({ok:true})
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 docstrings
🧪 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
force-pushed
the
cursor/testing-infra-adjustments-ac69
branch
from
March 25, 2026 07:43
02cb78a to
b9f083d
Compare
kentcdodds
marked this pull request as ready for review
March 25, 2026 07:55
cursor
Bot
force-pushed
the
cursor/testing-infra-adjustments-ac69
branch
from
March 25, 2026 07:57
b9f083d to
3b90ef9
Compare
Owner
Author
|
@cursoragent please rebase on main, address any relevant changes, run format, run full gate, commit, force push with lease. |
Contributor
Summary
Testing
Final branch head:
  |
cursor
Bot
force-pushed
the
cursor/testing-infra-adjustments-ac69
branch
from
March 25, 2026 15:58
3b90ef9 to
948a9b7
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
|
🔎 Preview deployed: https://kody-pr-44.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/mcp/skills/skill-mutation.ts (1)
61-70:⚠️ Potential issue | 🟡 MinorNormalize blank
template_keyvalues tonullbefore persisting.
args.template_key ?? nullpreserves''as a real key. With the new partial unique index ontemplate_key, that turns “unset” into a uniqueness participant and can make later inserts/updates collide or get ignored unexpectedly.🧩 Suggested normalization
const parameters = normalizeSkillParameters(args.parameters) const connectionBindings = normalizeSkillConnectionBindings( args.connection_bindings, ) + const templateKey = args.template_key?.trim() || null const embedText = buildSkillEmbedText({ title: args.title, description: args.description, @@ - template_key: args.template_key ?? null, + template_key: templateKey,Also applies to: 153-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/skills/skill-mutation.ts` around lines 61 - 70, Normalize blank template_key values to null before persisting: instead of using args.template_key ?? null (which keeps empty string), detect when template_key === '' and set it to null so empty strings do not participate in the partial unique index. Update the code paths that consume SkillPersistenceArgs.template_key (refer to SkillPersistenceArgs and the places around the other occurrence at the 153-166 block) to map '' -> null prior to building the DB insert/update payload so persisted template_key is either a non-empty string or null.
🧹 Nitpick comments (5)
packages/worker/src/mcp/test-process.ts (1)
22-24: Consider cleaning up the spawned process if the guard throws.If this defensive check fails, the spawned
procis orphaned since no cleanup occurs before throwing. While this condition is practically unreachable given the explicit'pipe'stdio configuration, adding aproc.kill()before throwing would ensure no resource leak in unexpected edge cases.🛡️ Optional defensive cleanup
if (!proc.stdout || !proc.stderr) { + proc.kill() throw new Error('spawnProcess requires piped stdout and stderr streams.') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/test-process.ts` around lines 22 - 24, The guard that throws when proc.stdout or proc.stderr is missing can orphan the spawned process; before throwing in that branch inside test-process.ts, call proc.kill() (or proc.kill('SIGTERM') and optionally proc.kill('SIGKILL') fallback) to ensure the child is terminated, and then throw the Error; reference the existing proc variable and the conditional that checks proc.stdout/proc.stderr to locate where to add the cleanup.packages/worker/src/mcp/skills/builtin-skill-templates.ts (1)
189-201: Inconsistent error handling for missing connections across templates.The GitHub REST template (lines 84-149) gracefully handles missing connections by initiating a setup flow with
allow_missing: true. However, the GitHub GraphQL, Cloudflare REST, and Cursor Cloud REST templates will throw an error if no default connection exists since they don't passallow_missing: true.Consider either:
- Adding similar setup flow fallback to these templates, or
- Documenting this as intentional (e.g., users must set up GitHub REST first which creates the connection)
🤖 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/REST templates (the async (params) => handlers that call codemode.connections_resolve) currently assume a default connection exists and will throw if missing; update the codemode.connections_resolve invocation in the GitHub GraphQL, Cloudflare REST and Cursor Cloud REST templates to include allow_missing: true (same pattern used in the GitHub REST template) so the setup flow is returned when no default connection exists, and ensure the subsequent code handles the returned setup/connection object the same way as the GitHub REST template does.packages/worker/src/mcp/connections/provider-connections-repo.ts (1)
230-275: LGTM!Row mappers properly normalize nullable fields and coerce
is_defaultto a strict0 | 1type.Note:
resolveFieldUpdateis duplicated across repo files. Consider extracting to a shared utility if more repositories are added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/connections/provider-connections-repo.ts` around lines 230 - 275, The resolveFieldUpdate function is duplicated across files; extract it into a single shared utility (e.g., create a new exported function resolveFieldUpdate in a common utils module) and replace the local definitions with imports from that module; update all files that currently define resolveFieldUpdate (including provider-connections-repo.ts) to import the shared resolveFieldUpdate and remove the duplicated implementation to avoid drift and ensure a single source of truth.packages/worker/src/mcp/connections/connection-service.ts (2)
847-870: Consider adding an upper bound to the label suffix loop.While practically bounded by existing connections, an explicit limit would prevent theoretical runaway in edge cases.
🔧 Proposed safeguard
async function buildUniqueConnectionLabel(input: { env: Env userId: string providerKey: string preferredLabel: string }) { const base = normalizeConnectionLabel(input.preferredLabel) const existingConnections = await listProviderConnectionsByProvider( input.env.APP_DB, input.userId, input.providerKey, ) const existingLabels = new Set( existingConnections.map((connection) => connection.label), ) if (!existingLabels.has(base)) { return base } let suffix = 2 - while (existingLabels.has(`${base}-${suffix}`)) { + while (existingLabels.has(`${base}-${suffix}`) && suffix < 1000) { suffix += 1 } return `${base}-${suffix}` }🤖 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 847 - 870, The loop in buildUniqueConnectionLabel can theoretically run unbounded when incrementing suffix; add a safeguard by defining a MAX_SUFFIX constant (e.g., 1000 or another project-appropriate limit) and change the while loop to also check suffix <= MAX_SUFFIX, and if exceeded throw a clear error (or return a fallback) indicating no unique label could be generated for the user/provider; update references to suffix and existingLabels accordingly so buildUniqueConnectionLabel throws/returns deterministically instead of looping forever.
1013-1020: Manual HTML escaping is acceptable here but consider the static analysis guidance.The
escapeHtmlfunction correctly escapes the five critical characters in the proper order (ampersand first). For this limited use case—displaying a status message in text content—the implementation is adequate. However, static analysis tools flag manual sanitization as potentially error-prone.If more complex HTML generation is added later, consider a dedicated library. For now, ensure this function is only used for text content, not for attribute values or other contexts.
🤖 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 1013 - 1020, The escapeHtml function manually escapes the five critical characters but flagged by static analysis; update the code by adding a clear JSDoc/comment above the escapeHtml function (or rename it to escapeHtmlText) stating it is only for escaping text node content (not for HTML attributes or rich HTML generation), add a small unit test that verifies its behavior for the five characters, and add a TODO/note recommending using a vetted sanitization library if you later need attribute or complex HTML escaping; reference the function name escapeHtml so reviewers can locate and verify the change.
🤖 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 303-305: The async saved-app render path currently reads shared
latestEnvelope/latestRenderData after awaiting resolveSavedAppCode(), which
allows older renders to resume and overwrite frames with a newer session;
instead thread the current appSession (and associated envelope/render id)
through the helper calls (including resolveSavedAppCode and any completion
handlers) and, after each await, compare the carried session/id against the
current latestEnvelope to drop stale completions before calling setFrameSource;
update functions referenced (resolveSavedAppCode, setFrameSource and any helpers
that use latestEnvelope/latestRenderData) to accept an appSession/envelope
identifier parameter and use that carried value rather than reading
latestEnvelope directly.
- Around line 47-75: coerceAppSession currently accepts any string for endpoints
and can leak the bearer token to arbitrary origins; update coerceAppSession to
parse endpoints.appSource, endpoints.action, and endpoints.secureInput as URLs
(using the URL constructor), verify each URL's origin matches the expected
trusted origin (e.g., the app's canonical origin) and that each pathname begins
with "/api/generated-ui/", and return null if any check fails; apply the same
URL-origin+pathname validation pattern to the other similar validators mentioned
so they also reject untrusted endpoints before the token is attached.
In `@packages/worker/src/mcp-auth.ts`:
- Around line 145-153: The current finally block always calls
recordBuiltinTemplateSeed(props.user.userId, now) even when
ensureBuiltinSkillTemplatesForUser(env, props.user.userId) throws, which starts
the cooldown on failure; remove the recordBuiltinTemplateSeed call from the
finally and instead invoke recordBuiltinTemplateSeed(props.user.userId, now)
only after ensureBuiltinSkillTemplatesForUser completes successfully (e.g.,
place it at the end of the try block or guard it with an explicit success flag),
leaving the catch to log the error and allowing retries when seeding fails.
- Around line 148-150: The warning log currently emits a stable user identifier
(props.user.userId) when seeding builtin MCP skill templates; remove or replace
that direct PII: update the console.warn call that contains the message 'Failed
to ensure builtin MCP skill templates' to omit props.user.userId and instead
include a non-stable correlation/request id (e.g., props.requestId or
correlationId) or a redacted/hashed value derived from props.user.userId (or
omit user info entirely) so logs no longer retain stable user identifiers.
In `@packages/worker/src/mcp/connections/auth-spec.ts`:
- Around line 30-46: The schema currently allows absolute or protocol-relative
URLs; update providerRequestConfigSchema and providerVerificationSchema to
enforce relative-path invariants: require path_prefix and graphql_path (in
providerRequestConfigSchema) and path (in providerVerificationSchema) to be
relative by adding a validation that they start with a single leading slash and
do not start with '//' or contain a scheme (no '://' sequences) — e.g., replace
the permissive .min(1) with a refinement/regex that checks strings start with
'/' but not '//' and reject any value containing '://'; apply this to
providerVerificationSchema.path, providerRequestConfigSchema.path_prefix, and
providerRequestConfigSchema.graphql_path so verification/request config cannot
escape the configured provider origin.
In `@packages/worker/src/mcp/connections/oauth-api.ts`:
- Around line 8-17: The current flow calls handleConnectionOAuthCallback for
both GET and HEAD which can consume one-time OAuth state; change the logic so
HEAD requests return immediately without invoking handleConnectionOAuthCallback:
check if request.method === 'HEAD' first (after allowing only GET/HEAD), and
return a Response with the appropriate status/headers (mirroring what
handleConnectionOAuthCallback would return) before calling
handleConnectionOAuthCallback for GET requests only; update the code around the
handleConnectionOAuthCallback invocation and the HEAD branch so only GET
triggers the OAuth callback handler.
In `@packages/worker/src/mcp/generated-ui-app-session.ts`:
- Around line 7-12: The payload type GeneratedUiAppSessionPayload currently
embeds the full McpUserContext which is exported to the browser via signToken;
remove McpUserContext from the signed, browser-visible payload and replace it
with only a session_id (and any strictly-public minimal fields if truly
necessary), then persist the full McpUserContext server-side keyed by session_id
(or use an encrypted payload instead of signToken) so client-visible tokens
carry no sensitive fields; update any code paths that construct/consume
GeneratedUiAppSessionPayload and signToken to read the full context from the
server session store by session_id (or decrypt the payload) rather than trusting
client-readable McpUserContext.
---
Outside diff comments:
In `@packages/worker/src/mcp/skills/skill-mutation.ts`:
- Around line 61-70: Normalize blank template_key values to null before
persisting: instead of using args.template_key ?? null (which keeps empty
string), detect when template_key === '' and set it to null so empty strings do
not participate in the partial unique index. Update the code paths that consume
SkillPersistenceArgs.template_key (refer to SkillPersistenceArgs and the places
around the other occurrence at the 153-166 block) to map '' -> null prior to
building the DB insert/update payload so persisted template_key is either a
non-empty string or null.
---
Nitpick comments:
In `@packages/worker/src/mcp/connections/connection-service.ts`:
- Around line 847-870: The loop in buildUniqueConnectionLabel can theoretically
run unbounded when incrementing suffix; add a safeguard by defining a MAX_SUFFIX
constant (e.g., 1000 or another project-appropriate limit) and change the while
loop to also check suffix <= MAX_SUFFIX, and if exceeded throw a clear error (or
return a fallback) indicating no unique label could be generated for the
user/provider; update references to suffix and existingLabels accordingly so
buildUniqueConnectionLabel throws/returns deterministically instead of looping
forever.
- Around line 1013-1020: The escapeHtml function manually escapes the five
critical characters but flagged by static analysis; update the code by adding a
clear JSDoc/comment above the escapeHtml function (or rename it to
escapeHtmlText) stating it is only for escaping text node content (not for HTML
attributes or rich HTML generation), add a small unit test that verifies its
behavior for the five characters, and add a TODO/note recommending using a
vetted sanitization library if you later need attribute or complex HTML
escaping; reference the function name escapeHtml so reviewers can locate and
verify the change.
In `@packages/worker/src/mcp/connections/provider-connections-repo.ts`:
- Around line 230-275: The resolveFieldUpdate function is duplicated across
files; extract it into a single shared utility (e.g., create a new exported
function resolveFieldUpdate in a common utils module) and replace the local
definitions with imports from that module; update all files that currently
define resolveFieldUpdate (including provider-connections-repo.ts) to import the
shared resolveFieldUpdate and remove the duplicated implementation to avoid
drift and ensure a single source of truth.
In `@packages/worker/src/mcp/skills/builtin-skill-templates.ts`:
- Around line 189-201: The GraphQL/REST templates (the async (params) =>
handlers that call codemode.connections_resolve) currently assume a default
connection exists and will throw if missing; update the
codemode.connections_resolve invocation in the GitHub GraphQL, Cloudflare REST
and Cursor Cloud REST templates to include allow_missing: true (same pattern
used in the GitHub REST template) so the setup flow is returned when no default
connection exists, and ensure the subsequent code handles the returned
setup/connection object the same way as the GitHub REST template does.
In `@packages/worker/src/mcp/test-process.ts`:
- Around line 22-24: The guard that throws when proc.stdout or proc.stderr is
missing can orphan the spawned process; before throwing in that branch inside
test-process.ts, call proc.kill() (or proc.kill('SIGTERM') and optionally
proc.kill('SIGKILL') fallback) to ensure the child is terminated, and then throw
the Error; reference the existing proc variable and the conditional that checks
proc.stdout/proc.stderr to locate where to add the cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fff0164-a425-4fa7-abf9-d736079fdf04
📒 Files selected for processing (48)
docs/agents/end-to-end-testing.mdpackages/worker/client/app-session-refresh.test.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/test-process.tspackages/worker/src/mcp/tools/open-generated-ui.tspackages/worker/src/mcp/tools/search.tsplaywright.config.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/worker/src/mcp/connections/connection-service.ts (1)
666-676: Consider non-ASCII edge case withbtoa().
btoa()throws on non-ASCII characters. While client credentials are typically ASCII-safe, providers with internationalized client IDs could cause unexpected failures.🔧 Optional: Use base64 encoding that handles UTF-8
if (input.spec.token_auth_method === 'client_secret_basic') { + const credentials = `${input.secretMaterial['client_id'] ?? ''}:${input.secretMaterial['client_secret'] ?? ''}` + const encoded = base64UrlEncode(new TextEncoder().encode(credentials)).replace(/-/g, '+').replace(/_/g, '/') headers.set( 'authorization', - `Basic ${btoa( - `${input.secretMaterial['client_id'] ?? ''}:${input.secretMaterial['client_secret'] ?? ''}`, - )}`, + `Basic ${encoded}`, ) }🤖 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 666 - 676, The use of btoa(...) inside the token auth branch (where input.spec.token_auth_method === 'client_secret_basic') will throw for non-ASCII client IDs/secrets; change the encoding to a UTF-8-safe base64 encoder (e.g., replace btoa(...) with a UTF-8 -> base64 conversion such as Buffer.from(`${input.secretMaterial['client_id'] ?? ''}:${input.secretMaterial['client_secret'] ?? ''}`, 'utf8').toString('base64') or an equivalent helper) so headers.set('authorization', `Basic ${...}`) always works for internationalized credentials; update the code around headers.set and the btoa call accordingly.
🤖 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/src/mcp/connections/connection-service.ts`:
- Around line 809-826: The getMissingOAuthClientSecretNames function currently
only requires client_secret when spec.token_auth_method === 'client_secret_post'
or strategy === 'oauth2_pre_registered_client'; update it to also require
client_secret when spec.token_auth_method === 'client_secret_basic' (regardless
of spec.strategy) so dynamic clients using client_secret_basic without a
returned secret are flagged as missing; modify the conditional in
getMissingOAuthClientSecretNames to include token_auth_method ===
'client_secret_basic' alongside the existing checks.
---
Nitpick comments:
In `@packages/worker/src/mcp/connections/connection-service.ts`:
- Around line 666-676: The use of btoa(...) inside the token auth branch (where
input.spec.token_auth_method === 'client_secret_basic') will throw for non-ASCII
client IDs/secrets; change the encoding to a UTF-8-safe base64 encoder (e.g.,
replace btoa(...) with a UTF-8 -> base64 conversion such as
Buffer.from(`${input.secretMaterial['client_id'] ??
''}:${input.secretMaterial['client_secret'] ?? ''}`, 'utf8').toString('base64')
or an equivalent helper) so headers.set('authorization', `Basic ${...}`) always
works for internationalized credentials; update the code around headers.set and
the btoa call accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bafa615a-13eb-40f1-87bb-6619d3c111e0
📒 Files selected for processing (6)
packages/worker/src/mcp/connections/connection-drafts-repo.tspackages/worker/src/mcp/connections/connection-service.tspackages/worker/src/mcp/connections/crypto.tspackages/worker/src/mcp/connections/provider-connections-repo.tspackages/worker/src/mcp/connections/resolve-field-update.tspackages/worker/src/mcp/generated-ui-api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/mcp/connections/crypto.ts
Owner
Author
|
@cursoragent please address valid feedback on this PR |
Contributor
|
Taking a look! |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/worker/src/mcp/generated-ui-api.ts (2)
31-57: Method validation occurs after authentication.For all routes, authentication happens before checking the HTTP method. This means invalid methods still require a valid token. Consider checking the method first for a slight efficiency gain and clearer error semantics (405 vs 401).
🤖 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 31 - 57, Move the HTTP method check to run before authenticating the session so invalid-method requests return 405 without requiring token validation: in the route handling block where url.pathname === '/api/generated-ui/app-source' (the code using authenticateGeneratedUiSession, request.method, and getUiArtifactById), first verify request.method === 'GET' and return jsonResponse({ error: 'Method not allowed.' }, 405) if not, then call authenticateGeneratedUiSession(request, env) and continue with the existing app_id validation and getUiArtifactById lookup.
126-135: Error messages fromstoreDraftSecretsmay leak internal details.Exceptions from
storeDraftSecrets(e.g., "Connection draft not found for this user", "Connection draft has expired") are returned directly in the response. While these are informational, ensure they don't reveal information useful to attackers probing for valid draft IDs.🛡️ Consider generic error message
} catch (error) { return jsonResponse( { ok: false, - error: - error instanceof Error ? error.message : 'Secure input failed.', + error: 'Secure input failed.', }, 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 126 - 135, The catch block that handles errors from storeDraftSecrets should not echo internal error messages to clients; change the response so jsonResponse returns a generic failure message (e.g., "Secure input failed.") and a 400 status while sending the original error details to internal logs only (use processLogger/errorLogger or whatever logging utility exists) instead of including error.message; preserve the existing use of jsonResponse and the same 400 status but replace the dynamic error string created from error/instanceof Error with a constant, and ensure storeDraftSecrets and its calling handler still record the full error internally for debugging.packages/worker/src/mcp/generated-ui-app-session.ts (1)
20-32: Consider consolidating duplicate type definitions.
GeneratedUiAppSessionandGeneratedUiAppSessionEnvelopehave identical structures. If they serve distinct semantic purposes, add a comment explaining the difference; otherwise, consolidate them into a single exported type to avoid maintenance burden.🤖 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 20 - 32, GeneratedUiAppSession and GeneratedUiAppSessionEnvelope are identical types; either consolidate them into a single exported type (e.g., export type GeneratedUiAppSession = { ... } and export type GeneratedUiAppSessionEnvelope = GeneratedUiAppSession) to remove duplication, or keep both but add a short comment above each type explaining their distinct semantic purpose; update any references to use the consolidated type or preserve names as aliases to avoid future divergence (refer to the type names GeneratedUiAppSession and GeneratedUiAppSessionEnvelope and any usages across the codebase).
🤖 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 1031-1065: The call to
hostBridge.callTool('generated_ui_invoke_action') currently forwards
payload.input directly, but the tool schema requires a token; attach the session
token from latestEnvelope?.appSession?.token to the arguments before calling
hostBridge.callTool so the input becomes the original payload.input merged with
token (use latestEnvelope.appSession.token if present); update the code path
where hostBridge.callTool is invoked (around the generated_ui_invoke_action
handling that reads payload.input and uses isRecord) to merge in the token and
then proceed to handle result/error as before.
- Around line 1067-1101: The host must attach the session token and normalize
field names before calling the generated_ui_submit_secure_input tool: fetch the
current session token (e.g. from the host/session API used elsewhere in this
module), build the arguments object as { token: sessionToken, setup_id:
input.setupId ?? input.setup_id, fields: input.fields }, and pass that object to
hostBridge.callTool('generated_ui_submit_secure_input', { arguments: ... })
instead of passing payload.input directly; update the block that handles
`${childMessagePrefix}submit-secure-input` to perform this transformation and
include the token so the capability receives { token, setup_id, fields } as
expected.
In `@packages/worker/src/mcp/capabilities/apps/ui-generated-ui-invoke-action.ts`:
- Around line 31-47: The handler currently calls verifyGeneratedUiAppSession
without error handling; wrap that call in a try/catch inside the handler (around
verifyGeneratedUiAppSession(ctx.env, args.token)) and on catch return a
structured failure object (e.g., return { ok: false, error: { message:
err.message, name: err.name } }) instead of letting the exception bubble; keep
the rest of the flow (building callerContext and calling
runCodemodeWithRegistry) unchanged and ensure types match the handler's expected
return shape.
---
Nitpick comments:
In `@packages/worker/src/mcp/generated-ui-api.ts`:
- Around line 31-57: Move the HTTP method check to run before authenticating the
session so invalid-method requests return 405 without requiring token
validation: in the route handling block where url.pathname ===
'/api/generated-ui/app-source' (the code using authenticateGeneratedUiSession,
request.method, and getUiArtifactById), first verify request.method === 'GET'
and return jsonResponse({ error: 'Method not allowed.' }, 405) if not, then call
authenticateGeneratedUiSession(request, env) and continue with the existing
app_id validation and getUiArtifactById lookup.
- Around line 126-135: The catch block that handles errors from
storeDraftSecrets should not echo internal error messages to clients; change the
response so jsonResponse returns a generic failure message (e.g., "Secure input
failed.") and a 400 status while sending the original error details to internal
logs only (use processLogger/errorLogger or whatever logging utility exists)
instead of including error.message; preserve the existing use of jsonResponse
and the same 400 status but replace the dynamic error string created from
error/instanceof Error with a constant, and ensure storeDraftSecrets and its
calling handler still record the full error internally for debugging.
In `@packages/worker/src/mcp/generated-ui-app-session.ts`:
- Around line 20-32: GeneratedUiAppSession and GeneratedUiAppSessionEnvelope are
identical types; either consolidate them into a single exported type (e.g.,
export type GeneratedUiAppSession = { ... } and export type
GeneratedUiAppSessionEnvelope = GeneratedUiAppSession) to remove duplication, or
keep both but add a short comment above each type explaining their distinct
semantic purpose; update any references to use the consolidated type or preserve
names as aliases to avoid future divergence (refer to the type names
GeneratedUiAppSession and GeneratedUiAppSessionEnvelope and any usages across
the codebase).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22eacf08-16ba-4716-8d88-33f51283fc92
📒 Files selected for processing (8)
packages/worker/client/mcp-apps/generated-ui-shell.tspackages/worker/src/index.tspackages/worker/src/mcp/capabilities/apps/domain.tspackages/worker/src/mcp/capabilities/apps/ui-generated-ui-invoke-action.tspackages/worker/src/mcp/capabilities/apps/ui-generated-ui-submit-secure-input.tspackages/worker/src/mcp/generated-ui-api.tspackages/worker/src/mcp/generated-ui-app-session.tspackages/worker/src/mcp/tools/open-generated-ui.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/worker/src/mcp/tools/open-generated-ui.ts
- packages/worker/src/index.ts
Owner
Author
|
@cursoragent please check failing CI and fix it. Also please review the feedback on this PR and address valid issues |
Contributor
|
Bugbot Autofix prepared fixes for both issues found in the latest run.
Preview (31218a5974)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/worker/client/app-session-refresh.test.ts b/packages/worker/client/app-session-refresh.test.ts
--- a/packages/worker/client/app-session-refresh.test.ts
+++ b/packages/worker/client/app-session-refresh.test.ts
@@ -44,6 +44,37 @@
await task!(controller.signal)
}
+function collectTextContent(value: unknown): Array<string> {
+ if (typeof value === 'string') {
+ return [value]
+ }
+ if (
+ typeof value === 'number' ||
+ typeof value === 'boolean' ||
+ value == null
+ ) {
+ return []
+ }
+ if (Array.isArray(value)) {
+ return value.flatMap((entry) => collectTextContent(entry))
+ }
+ if (typeof value === 'object') {
+ const props =
+ 'props' in value &&
+ value.props &&
+ typeof value.props === 'object' &&
+ 'children' in value.props
+ ? value.props.children
+ : undefined
+ return collectTextContent(props)
+ }
+ return []
+}
+
+function renderToText(value: unknown) {
+ return collectTextContent(value).join(' ')
+}
+
test('aborted refresh does not erase a ready authenticated session', async () => {
navigationListeners.length = 0
queuedSessionResponses.length = 0
@@ -69,7 +100,7 @@
await runNextTask(queuedTasks, false)
await runNextTask(queuedTasks, false)
- const authenticatedUi = String(render())
+ const authenticatedUi = renderToText(render())
expect(authenticatedUi).toContain('signed-in@example.com')
expect(authenticatedUi).toContain('Log out')
@@ -77,9 +108,9 @@
navigationListeners[0]!()
await runNextTask(queuedTasks, true)
- const uiAfterAbort = String(render())
+ const uiAfterAbort = renderToText(render())
expect(uiAfterAbort).toContain('signed-in@example.com')
expect(uiAfterAbort).toContain('Log out')
- expect(uiAfterAbort).not.toContain('>Login<')
- expect(uiAfterAbort).not.toContain('>Signup<')
+ expect(uiAfterAbort).not.toContain('Login')
+ expect(uiAfterAbort).not.toContain('Signup')
})
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,22 @@
type DisplayMode = 'inline' | 'fullscreen' | 'pip'
type ThemeName = 'light' | 'dark'
+type AppSessionEnvelope = {
+ sessionId: 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 +43,63 @@
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.expiresAt !== 'string'
+ ) {
+ return null
+ }
+ const endpoints = coerceGeneratedUiEndpoints(value.endpoints)
+ if (!endpoints) {
+ return null
+ }
+ return {
+ sessionId: value.sessionId,
+ expiresAt: value.expiresAt,
+ endpoints,
+ }
+}
+
+function coerceGeneratedUiEndpoints(value: unknown) {
+ if (!isRecord(value)) return null
+ if (
+ typeof value.appSource !== 'string' ||
+ typeof value.action !== 'string' ||
+ typeof value.secureInput !== 'string'
+ ) {
+ return null
+ }
+ try {
+ const appSource = new URL(value.appSource)
+ const action = new URL(value.action)
+ const secureInput = new URL(value.secureInput)
+ const origin = appSource.origin
+ const expectedOrigin = new URL('/', import.meta.url).origin
+ if (origin !== expectedOrigin) {
+ return null
+ }
+ if (action.origin !== origin || secureInput.origin !== origin) {
+ return null
+ }
+ if (
+ !appSource.pathname.startsWith('/api/generated-ui/') ||
+ !action.pathname.startsWith('/api/generated-ui/') ||
+ !secureInput.pathname.startsWith('/api/generated-ui/')
+ ) {
+ return null
+ }
+ return {
+ appSource: appSource.toString(),
+ action: action.toString(),
+ secureInput: secureInput.toString(),
+ }
+ } catch {
+ return null
+ }
+}
+
function coerceDisplayMode(value: unknown): DisplayMode | null {
return value === 'inline' || value === 'fullscreen' || value === 'pip'
? value
@@ -42,6 +110,18 @@
return value === 'light' || value === 'dark' ? value : null
}
+function coerceAppSessionContext(value: unknown) {
+ if (!isRecord(value)) return null
+ if (!isRecord(value.endpoints)) return null
+ const { appSource } = value.endpoints
+ if (typeof appSource !== 'string') return null
+ try {
+ return new URL(appSource).origin
+ } catch {
+ return null
+ }
+}
+
function injectThemeAttributeIntoHtmlTag(
htmlTag: string,
theme: ThemeName | null,
@@ -234,7 +314,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) {
@@ -258,6 +339,7 @@
const childMessagePrefix = 'kody-generated-ui:'
let latestRenderData: RenderDataEnvelope | undefined
+ let latestEnvelope: RenderEnvelope | null = null
function getBaseHref() {
try {
@@ -267,7 +349,7 @@
}
}
- function escapeInlineModuleSource(code: string) {
+ function escapeInlineScriptSource(code: string) {
return code.replace(/<\/script/gi, '<\\/script')
}
@@ -493,25 +575,45 @@
`.trim()
}
- function buildHeadInjection(theme: ThemeName | null) {
+ function buildHeadInjection(
+ theme: ThemeName | null,
+ appSession: AppSessionEnvelope | null,
+ ) {
+ const bridgeRuntime = escapeInlineScriptSource(
+ buildChildBridgeRuntimeSource(appSession),
+ )
return `
<style>
${buildShellStyles(theme)}
</style>
<script>
-${buildChildBridgeRuntimeSource()}
+${bridgeRuntime}
</script>
`.trim()
}
- function buildChildBridgeRuntimeSource() {
+ function buildChildBridgeRuntimeSource(
+ appSession: AppSessionEnvelope | null,
+ ) {
+ const hasAppSession = appSession != null
return `
+(() => {
const shellMessagePrefix = '${childMessagePrefix}';
+const hasAppSession = ${hasAppSession ? 'true' : 'false'};
let requestCounter = 0;
function nextRequestId() {
requestCounter += 1;
return 'generated-ui-' + requestCounter;
}
+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);
+}\`;
+}
function waitForShellMessage(type, requestId) {
return new Promise((resolve) => {
function handleMessage(event) {
@@ -535,6 +637,74 @@
}, '*');
return pending;
}
+async function requestAppSessionAction(type, payload, responseType) {
+ if (!hasAppSession || window.parent === window) return null;
+ const messageType = shellMessagePrefix + type;
+ if (messageType === shellMessagePrefix + 'invoke-action') {
+ const input = payload?.input;
+ if (
+ !input ||
+ typeof input !== 'object' ||
+ typeof input.code !== 'string' ||
+ input.code.length === 0
+ ) {
+ return {
+ ok: false,
+ error: 'Action code must be a non-empty string.',
+ };
+ }
+ }
+ const requestId = nextRequestId();
+ const responsePayload = await postRequestAndWaitForShellMessage(
+ messageType,
+ {
+ requestId,
+ ...payload,
+ },
+ shellMessagePrefix + responseType,
+ requestId,
+ );
+ const response =
+ responsePayload && typeof responsePayload === 'object'
+ ? responsePayload.response
+ : null;
+ return response;
+}
+async function invokeActionWithSession(input) {
+ if (!hasAppSession) return null;
+ const payload = await requestAppSessionAction(
+ 'invoke-action',
+ { input },
+ 'invoke-action-result',
+ );
+ if (!payload || typeof payload !== 'object') {
+ return {
+ ok: false,
+ error: 'Generated UI action endpoint returned an invalid response.',
+ };
+ }
+ return payload;
+}
+async function submitSecureInputWithSession(input) {
+ if (!hasAppSession) {
+ return {
+ ok: false,
+ error: 'Secure input is unavailable without a generated UI app session.',
+ };
+ }
+ const payload = await requestAppSessionAction(
+ 'submit-secure-input',
+ { input },
+ 'submit-secure-input-result',
+ );
+ if (!payload || typeof payload !== 'object') {
+ return {
+ ok: false,
+ error: 'Secure input endpoint returned an invalid response.',
+ };
+ }
+ return payload;
+}
window.kodyWidget = {
sendMessage(text) {
if (window.parent === window) return false;
@@ -584,6 +754,64 @@
}
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;
+ const sessionPayload = await invokeActionWithSession(
+ params === undefined ? { code } : { code, params },
+ );
+ if (sessionPayload) {
+ return sessionPayload;
+ }
+ 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(
@@ -597,13 +825,20 @@
event.reason?.message ?? event.reason ?? 'Unknown rejection',
);
});
+})();
`.trim()
}
- function buildInlineModuleSource(code: string) {
- const safeCode = escapeInlineModuleSource(code)
+ function buildInlineModuleSource(
+ code: string,
+ appSession: AppSessionEnvelope | null,
+ ) {
+ const safeCode = escapeInlineScriptSource(code)
+ const bridgeRuntime = escapeInlineScriptSource(
+ buildChildBridgeRuntimeSource(appSession),
+ )
return `
-${buildChildBridgeRuntimeSource()}
+${bridgeRuntime}
${safeCode}
`.trim()
}
@@ -622,7 +857,10 @@
`.trim()
}
- function buildHtmlDocumentFromFragment(code: string) {
+ function buildHtmlDocumentFromFragment(
+ code: string,
+ appSession: AppSessionEnvelope | null,
+ ) {
const theme = coerceTheme(latestRenderData?.theme)
return `
<!doctype html>
@@ -630,7 +868,7 @@
<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}
@@ -639,9 +877,13 @@
`.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)
const theme = coerceTheme(latestRenderData?.theme)
frameElement.srcdoc = `
<!doctype html>
@@ -666,12 +908,19 @@
const theme = coerceTheme(latestRenderData?.theme)
const htmlSource = /<(?:!doctype|html|head|body)\b/i.test(code)
- ? injectIntoHtmlDocument(code, buildHeadInjection(theme), theme)
- : buildHtmlDocumentFromFragment(code)
+ ? injectIntoHtmlDocument(
+ code,
+ buildHeadInjection(theme, appSession),
+ theme,
+ )
+ : buildHtmlDocumentFromFragment(code, appSession)
frameElement.srcdoc = absolutizeHtmlAttributeUrls(htmlSource, getBaseHref())
}
- async function resolveSavedAppCode(appId: string) {
+ async function resolveSavedAppCode(
+ appId: string,
+ appSession: AppSessionEnvelope | null,
+ ) {
const result = (await hostBridge.callTool({
name: 'ui_load_app_source',
arguments: { app_id: appId },
@@ -696,17 +945,20 @@
}
async function renderEnvelope(envelope: RenderEnvelope | null) {
+ latestEnvelope = envelope
if (!envelope) {
frameElement.srcdoc = ''
return
}
+ const appSession = envelope.appSession ?? null
+
if (envelope.mode === 'inline_code') {
if (!envelope.code) {
renderErrorDocument('The tool result did not include inline code.')
return
}
- setFrameSource(envelope.code, envelope.runtime ?? 'html')
+ setFrameSource(envelope.code, envelope.runtime ?? 'html', appSession)
return
}
@@ -716,8 +968,9 @@
}
try {
- const resolved = await resolveSavedAppCode(envelope.appId)
- setFrameSource(resolved.code, resolved.runtime)
+ const resolved = await resolveSavedAppCode(envelope.appId, appSession)
+ if (latestEnvelope !== envelope) return
+ setFrameSource(resolved.code, resolved.runtime, appSession)
} catch (error) {
const message =
error instanceof Error ? error.message : 'Unknown app loading error.'
@@ -829,6 +1082,77 @@
}
return
}
+ if (event.data.type === `${childMessagePrefix}invoke-action`) {
+ const requestId =
+ typeof payload.requestId === 'string' ? payload.requestId : null
+ const input = isRecord(payload.input) ? payload.input : null
+ if (requestId && input && event.source) {
+ void hostBridge
+ .callTool({
+ name: 'generated_ui_invoke_action',
+ arguments: input,
+ })
+ .then((result) => {
+ const errorMessage = getHostToolErrorMessage(result)
+ const structuredContent = isRecord(result?.structuredContent)
+ ? result.structuredContent
+ : null
+ ;(event.source as WindowProxy).postMessage(
+ {
+ type: `${childMessagePrefix}invoke-action-result`,
+ payload: {
+ requestId,
+ response:
+ result?.isError === true
+ ? {
+ ok: false,
+ error:
+ errorMessage ?? 'Generated UI action failed.',
+ }
+ : (structuredContent ?? null),
+ },
+ },
+ '*',
+ )
+ })
+ }
+ return
+ }
+ if (event.data.type === `${childMessagePrefix}submit-secure-input`) {
+ const requestId =
+ typeof payload.requestId === 'string' ? payload.requestId : null
+ const input = isRecord(payload.input) ? payload.input : null
+ if (requestId && input && event.source) {
+ void hostBridge
+ .callTool({
+ name: 'generated_ui_submit_secure_input',
+ arguments: input,
+ })
+ .then((result) => {
+ const errorMessage = getHostToolErrorMessage(result)
+ const structuredContent = isRecord(result?.structuredContent)
+ ? result.structuredContent
+ : null
+ ;(event.source as WindowProxy).postMessage(
+ {
+ type: `${childMessagePrefix}submit-secure-input-result`,
+ payload: {
+ requestId,
+ response:
+ result?.isError === true
+ ? {
+ ok: false,
+ error: errorMessage ?? 'Secure input failed.',
+ }
+ : (structuredContent ?? null),
+ },
+ },
+ '*',
+ )
+ })
+ }
+ return
+ }
}
hostBridge.handleHostMessage(event.data)
})
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,24 +23,59 @@
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'
export { ChatAgent, HomeConnectorSession, HomeMCP, MCP }
+const claudeWidgetDomainSuffix = '.claudemcpcontent.com'
+
+function isAllowedGeneratedUiOrigin(origin: string, requestOrigin: string) {
+ if (origin === requestOrigin) {
+ return true
+ }
+ try {
+ const parsedOrigin = new URL(origin)
+ return parsedOrigin.hostname.endsWith(claudeWidgetDomainSuffix)
+ } catch {
+ return false
+ }
+}
+
const appHandler = withCors({
getCorsHeaders(request) {
+ const url = new URL(request.url)
+ if (isGeneratedUiApiRequest(url.pathname)) {
+ const origin = request.headers.get('Origin')
+ if (!origin || !isAllowedGeneratedUiOrigin(origin, url.origin)) {
+ return null
+ }
+ return new Headers({
+ 'Access-Control-Allow-Origin': origin,
+ 'Access-Control-Allow-Methods': 'GET, POST, OPTIONS',
+ 'Access-Control-Allow-Headers': 'content-type, authorization',
+ Vary: 'Origin',
+ })
+ }
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 +100,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 +123,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
@@ -4,12 +4,28 @@
} from '@cloudflare/workers-oauth-provider'
import { getAppBaseUrl } from '#app/app-base-url.ts'
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) {
+ if (builtinTemplateSeedByUser.has(userId)) {
+ builtinTemplateSeedByUser.delete(userId)
+ }
+ builtinTemplateSeedByUser.set(userId, seededAt)
+ while (builtinTemplateSeedByUser.size > builtinTemplateSeedMaxUsers) {
... diff truncated: showing 800 of 5434 lines |
| if (!oldestKey) break | ||
| builtinTemplateSeedByUser.delete(oldestKey) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
In-memory seed cache unbounded across worker restarts
Medium Severity
builtinTemplateSeedByUser is a module-level Map used as an in-memory LRU-like cache for rate-limiting builtin template seeding. In a Cloudflare Workers environment, isolates can be recycled at any time, making this cache unreliable — it may trigger redundant ensureBuiltinSkillTemplatesForUser DB calls on every request after an isolate restart. More importantly, the recordBuiltinTemplateSeed function deletes and re-inserts to maintain insertion order, but the eviction loop evicts oldest entries globally rather than per-user, so under load this cache may not provide meaningful rate-limiting.
There was a problem hiding this comment.
Actionable comments posted: 6
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)
998-1003:⚠️ Potential issue | 🔴 CriticalTrust only the current iframe when handling
kody-generated-ui:*messages.This branch accepts prefixed messages from any window and then attaches
latestEnvelope?.appSession?.tokento host tool calls. A stale iframe or unrelated window can therefore triggergenerated_ui_invoke_action/generated_ui_submit_secure_inputwith the current session. Gate the child-message branch onevent.source === frameElement.contentWindowbefore processing it.🔒 Suggested fix
globalThis.window.addEventListener('message', (event: MessageEvent) => { + const childWindow = frameElement.contentWindow if ( + event.source === childWindow && isRecord(event.data) && typeof event.data.type === 'string' && event.data.type.startsWith(childMessagePrefix) ) {Also applies to: 1072-1189
🤖 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 998 - 1003, The message handler attached in globalThis.window.addEventListener('message', ...) currently accepts any window for messages beginning with childMessagePrefix and may attach latestEnvelope?.appSession?.token to host calls; restrict processing to only messages from the intended iframe by checking that event.source === frameElement.contentWindow (where frameElement is the iframe element for the child UI) before handling prefixed messages (the branch that uses childMessagePrefix and latestEnvelope and triggers generated_ui_invoke_action / generated_ui_submit_secure_input); apply the same gating to the other handler ranges (around lines 1072-1189) that process child-prefixed messages so stale or unrelated windows cannot reuse the current session token.
♻️ Duplicate comments (1)
packages/worker/client/mcp-apps/generated-ui-shell.ts (1)
957-964:⚠️ Potential issue | 🟠 MajorDrop stale failures in the saved-app path too.
The success path already checks
latestEnvelope !== envelope, but thecatchpath still renders an old failure over a newer frame. Add the same stale guard beforerenderErrorDocument(...).🩹 Suggested fix
try { const resolved = await resolveSavedAppCode(envelope.appId) if (latestEnvelope !== envelope) return setFrameSource(resolved.code, resolved.runtime, appSession) } catch (error) { + if (latestEnvelope !== envelope) return const message = error instanceof Error ? error.message : 'Unknown app loading error.' renderErrorDocument(message) }🤖 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 957 - 964, The catch block for resolveSavedAppCode can render a stale error for an older envelope; add the same stale-check used on success: if (latestEnvelope !== envelope) return before calling renderErrorDocument. In other words, in the catch handling around resolveSavedAppCode, check latestEnvelope against envelope and bail out (return) if they differ, then proceed to build the message (using error instanceof Error ? error.message : ...) and call renderErrorDocument only when the envelope is still current.
🤖 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/src/mcp-auth.ts`:
- Around line 134-147: The current seeding can run concurrently for the same
user because recordBuiltinTemplateSeed is only called after awaiting
ensureBuiltinSkillTemplatesForUser; fix by deduping in-flight seeding per user:
introduce a map (e.g., builtinTemplateSeedInFlight: Map<string, Promise<void>>)
keyed by props.user.userId, check it before calling
ensureBuiltinSkillTemplatesForUser and if present await that Promise; if not
present, create and store the Promise from ensureBuiltinSkillTemplatesForUser,
await it, then call recordBuiltinTemplateSeed(props.user.userId, now) and
finally remove the entry in a finally block so concurrent requests for the same
user reuse the same in-flight Promise and only the first finishes the write.
In `@packages/worker/src/mcp/connections/connection-service.ts`:
- Around line 251-259: The preferredLabel passed into buildUniqueConnectionLabel
may be an empty string because normalizeConnectionLabel can return '' for
blank/punctuation-only names; update the call site in connection-service.ts (the
block that builds label using buildUniqueConnectionLabel and the helper
verificationLabelFromBody) to normalize the candidate label and if the
normalized result is empty, fall back to a non-empty default (e.g.,
`${draft.provider_key}-connection`); similarly apply the same
normalization+fallback logic to the other occurrence referenced (lines ~859-884)
so buildUniqueConnectionLabel never receives an empty string and downstream
schemas (connectionSelectionSchema) remain valid.
- Around line 227-239: finalizeConnectionSetup() does not validate the draft
state so OAuth drafts left in ready_to_authorize by getDraftStatusAfterSecrets()
can be finalized prematurely; update finalizeConnectionSetup() to check
draft.status (use the same enum/state values used by
getDraftStatusAfterSecrets()) and throw or return an error if the draft is not
in the expected finalizable state (e.g., ensure status === 'ready_to_create' or
equivalent) before proceeding with secret checks and DB insert, and apply the
same guard where finalizeConnectionSetup() is called so OAuth flows cannot
bypass the authorization callback.
- Around line 666-671: The Authorization header is being built with a base64url
helper and manual replacements which can omit padding and break strict OAuth
token endpoints; when input.spec.token_auth_method === 'client_secret_basic'
replace the base64UrlEncode/TextEncoder flow with a proper standard Base64
encoding (e.g. use Buffer.from(credentials).toString('base64') in Node or the
platform btoa equivalent) when creating the credentials string from
input.secretMaterial['client_id'] and ['client_secret'], then call
headers.set('authorization', `Basic ${...}`) as before; update the code in the
block handling client_secret_basic (the conditional using
input.spec.token_auth_method, credentials, and headers.set) to use standard
Base64 encoding.
- Around line 101-118: The current loop in upserting draft secrets (using
getAuthSpecSecretFields, allowedFields, encryptJson,
upsertConnectionDraftSecret) validates each field one-by-one and may persist
some secrets before encountering an invalid field; prevalidate the entire
input.fields keys against allowedFields first and throw if any invalid names
exist, then proceed to encrypt and upsert all entries (or perform the upserts
inside a single DB transaction/batch) so the operation is atomic and no partial
writes occur.
In `@packages/worker/src/mcp/generated-ui-api.ts`:
- Around line 165-170: The jsonResponse helper returns JSON without
cache-control which can expose private/authenticated data; update the
jsonResponse function to set an explicit Cache-Control: no-store header (and
keep Content-Type) so responses from endpoints like /api/generated-ui/app-source
and secure action/secure-input routes are not cached by browsers or intermediate
caches.
---
Outside diff comments:
In `@packages/worker/client/mcp-apps/generated-ui-shell.ts`:
- Around line 998-1003: The message handler attached in
globalThis.window.addEventListener('message', ...) currently accepts any window
for messages beginning with childMessagePrefix and may attach
latestEnvelope?.appSession?.token to host calls; restrict processing to only
messages from the intended iframe by checking that event.source ===
frameElement.contentWindow (where frameElement is the iframe element for the
child UI) before handling prefixed messages (the branch that uses
childMessagePrefix and latestEnvelope and triggers generated_ui_invoke_action /
generated_ui_submit_secure_input); apply the same gating to the other handler
ranges (around lines 1072-1189) that process child-prefixed messages so stale or
unrelated windows cannot reuse the current session token.
---
Duplicate comments:
In `@packages/worker/client/mcp-apps/generated-ui-shell.ts`:
- Around line 957-964: The catch block for resolveSavedAppCode can render a
stale error for an older envelope; add the same stale-check used on success: if
(latestEnvelope !== envelope) return before calling renderErrorDocument. In
other words, in the catch handling around resolveSavedAppCode, check
latestEnvelope against envelope and bail out (return) if they differ, then
proceed to build the message (using error instanceof Error ? error.message :
...) and call renderErrorDocument only when the envelope is still current.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69e18e23-3876-46bf-9606-bab3b4b6090d
📒 Files selected for processing (10)
packages/worker/client/mcp-apps/generated-ui-shell.tspackages/worker/src/mcp-auth.tspackages/worker/src/mcp/capabilities/apps/ui-generated-ui-invoke-action.tspackages/worker/src/mcp/capabilities/capability-search.test.tspackages/worker/src/mcp/connections/auth-spec.tspackages/worker/src/mcp/connections/connection-service.tspackages/worker/src/mcp/connections/oauth-api.tspackages/worker/src/mcp/generated-ui-api.tspackages/worker/src/mcp/generated-ui-app-session.tspackages/worker/src/mcp/tools/open-generated-ui.ts
✅ Files skipped from review due to trivial changes (1)
- packages/worker/src/mcp/capabilities/capability-search.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/worker/src/mcp/connections/oauth-api.ts
- packages/worker/src/mcp/capabilities/apps/ui-generated-ui-invoke-action.ts
Comment on lines
+134
to
+147
| 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) | ||
| recordBuiltinTemplateSeed(props.user.userId, now) |
There was a problem hiding this comment.
Prevent per-user seeding stampede on concurrent requests.
recordBuiltinTemplateSeed is written only after await ensureBuiltinSkillTemplatesForUser(...), so concurrent requests for the same user can all run seeding in parallel before the first one finishes. This can multiply writes and increase tail latency.
💡 Proposed fix (dedupe in-flight seeding per user)
const builtinTemplateSeedCooldownMs = 5 * 60 * 1000
const builtinTemplateSeedMaxUsers = 1000
const builtinTemplateSeedByUser = new Map<string, number>()
+const builtinTemplateSeedInFlightByUser = new Map<string, Promise<void>>()
function recordBuiltinTemplateSeed(userId: string, seededAt: number) {
@@
if (props.user?.userId) {
+ const userId = props.user.userId
const now = Date.now()
- const lastSeededAt = builtinTemplateSeedByUser.get(props.user.userId)
+ const lastSeededAt = builtinTemplateSeedByUser.get(userId)
if (lastSeededAt && now - lastSeededAt < builtinTemplateSeedCooldownMs) {
context.props = props
return fetchMcp(
@@
}
try {
- await ensureBuiltinSkillTemplatesForUser(env, props.user.userId)
- recordBuiltinTemplateSeed(props.user.userId, now)
+ let inFlight = builtinTemplateSeedInFlightByUser.get(userId)
+ if (!inFlight) {
+ inFlight = ensureBuiltinSkillTemplatesForUser(env, userId)
+ .then(() => recordBuiltinTemplateSeed(userId, Date.now()))
+ .finally(() => builtinTemplateSeedInFlightByUser.delete(userId))
+ builtinTemplateSeedInFlightByUser.set(userId, inFlight)
+ }
+ await inFlight
} catch (error) {
console.warn('Failed to ensure builtin MCP skill templates', {
error: error instanceof Error ? error.message : String(error),
})
}
}📝 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
| 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) | |
| recordBuiltinTemplateSeed(props.user.userId, now) | |
| if (props.user?.userId) { | |
| const userId = props.user.userId | |
| const now = Date.now() | |
| const lastSeededAt = builtinTemplateSeedByUser.get(userId) | |
| if (lastSeededAt && now - lastSeededAt < builtinTemplateSeedCooldownMs) { | |
| context.props = props | |
| return fetchMcp( | |
| request, | |
| env, | |
| context as ExecutionContext<OAuthContextProps>, | |
| ) | |
| } | |
| try { | |
| let inFlight = builtinTemplateSeedInFlightByUser.get(userId) | |
| if (!inFlight) { | |
| inFlight = ensureBuiltinSkillTemplatesForUser(env, userId) | |
| .then(() => recordBuiltinTemplateSeed(userId, Date.now())) | |
| .finally(() => builtinTemplateSeedInFlightByUser.delete(userId)) | |
| builtinTemplateSeedInFlightByUser.set(userId, inFlight) | |
| } | |
| await inFlight | |
| } catch (error) { | |
| console.warn('Failed to ensure builtin MCP skill templates', { | |
| error: error instanceof Error ? error.message : String(error), | |
| }) | |
| } | |
| } |
Suggested change
| 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) | |
| recordBuiltinTemplateSeed(props.user.userId, now) | |
| const builtinTemplateSeedInFlightByUser = new Map<string, Promise<void>>() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp-auth.ts` around lines 134 - 147, The current seeding
can run concurrently for the same user because recordBuiltinTemplateSeed is only
called after awaiting ensureBuiltinSkillTemplatesForUser; fix by deduping
in-flight seeding per user: introduce a map (e.g., builtinTemplateSeedInFlight:
Map<string, Promise<void>>) keyed by props.user.userId, check it before calling
ensureBuiltinSkillTemplatesForUser and if present await that Promise; if not
present, create and store the Promise from ensureBuiltinSkillTemplatesForUser,
await it, then call recordBuiltinTemplateSeed(props.user.userId, now) and
finally remove the entry in a finally block so concurrent requests for the same
user reuse the same in-flight Promise and only the first finishes the write.
Comment on lines
+101
to
+118
| const allowedFields = new Set( | ||
| getAuthSpecSecretFields(spec).map((field) => field.name), | ||
| ) | ||
| for (const [name, value] of Object.entries(input.fields)) { | ||
| if (!allowedFields.has(name)) { | ||
| throw new Error(`Secret field "${name}" is not allowed for this draft.`) | ||
| } | ||
| const encryptedValue = await encryptJson( | ||
| input.env, | ||
| encryptedDraftSecretPurpose, | ||
| value, | ||
| ) | ||
| await upsertConnectionDraftSecret(input.env.APP_DB, { | ||
| draft_id: draft.id, | ||
| secret_name: name, | ||
| encrypted_value: encryptedValue, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Validate the whole secret payload before the first upsert.
The allowed-field check happens inside the write loop. A payload like { client_id, bad_field } will persist client_id and then throw on bad_field, so the caller gets a failure even though the draft was already mutated. Prevalidate all names first, and batch/transaction the writes if you want this API to stay all-or-nothing.
🧰 Suggested fix
const spec = parseConnectionAuthSpec(draft.auth_spec_json)
const allowedFields = new Set(
getAuthSpecSecretFields(spec).map((field) => field.name),
)
- for (const [name, value] of Object.entries(input.fields)) {
- if (!allowedFields.has(name)) {
- throw new Error(`Secret field "${name}" is not allowed for this draft.`)
- }
+ const entries = Object.entries(input.fields)
+ const invalidName = entries.find(([name]) => !allowedFields.has(name))?.[0]
+ if (invalidName) {
+ throw new Error(
+ `Secret field "${invalidName}" is not allowed for this draft.`,
+ )
+ }
+ for (const [name, value] of entries) {
const encryptedValue = await encryptJson(
input.env,
encryptedDraftSecretPurpose,
value,
)🤖 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 101 -
118, The current loop in upserting draft secrets (using getAuthSpecSecretFields,
allowedFields, encryptJson, upsertConnectionDraftSecret) validates each field
one-by-one and may persist some secrets before encountering an invalid field;
prevalidate the entire input.fields keys against allowedFields first and throw
if any invalid names exist, then proceed to encrypt and upsert all entries (or
perform the upserts inside a single DB transaction/batch) so the operation is
atomic and no partial writes occur.
Comment on lines
+227
to
+239
| 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(', ')}`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Enforce the draft state machine before finalizing OAuth setups.
getDraftStatusAfterSecrets() deliberately leaves OAuth drafts at ready_to_authorize, but finalizeConnectionSetup() never checks draft.status before proceeding. That lets callers bypass the intended flow and rely on downstream verification to fail; with optional verification and oauth2_dynamic_client's default empty secret_fields in packages/worker/src/mcp/connections/auth-spec.ts, this can even reach the insert path without an OAuth callback.
🚦 Suggested fix
const spec = parseConnectionAuthSpec(draft.auth_spec_json)
+ if (spec.strategy === 'manual_token' || spec.strategy === 'api_key') {
+ if (draft.status !== 'ready_to_finalize') {
+ throw new Error('Connection draft is not ready to finalize.')
+ }
+ } else if (draft.status !== 'authorized') {
+ throw new Error('OAuth connection draft is not ready to finalize.')
+ }
const secretMaterial = await loadDraftSecretMaterial(input.env, draft.id)
const missingSecretNames = getMissingSecretNames(spec, secretMaterial)Also applies to: 785-797
🤖 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 227 -
239, finalizeConnectionSetup() does not validate the draft state so OAuth drafts
left in ready_to_authorize by getDraftStatusAfterSecrets() can be finalized
prematurely; update finalizeConnectionSetup() to check draft.status (use the
same enum/state values used by getDraftStatusAfterSecrets()) and throw or return
an error if the draft is not in the expected finalizable state (e.g., ensure
status === 'ready_to_create' or equivalent) before proceeding with secret checks
and DB insert, and apply the same guard where finalizeConnectionSetup() is
called so OAuth flows cannot bypass the authorization callback.
Comment on lines
+251
to
+259
| const label = await buildUniqueConnectionLabel({ | ||
| env: input.env, | ||
| userId: input.userId, | ||
| providerKey: draft.provider_key, | ||
| preferredLabel: | ||
| draft.label ?? | ||
| verificationLabelFromBody(verification?.body) ?? | ||
| `${draft.provider_key}-connection`, | ||
| }) |
There was a problem hiding this comment.
Fallback when label normalization strips everything.
normalizeConnectionLabel() can return '' for blank or punctuation-only provider names, and buildUniqueConnectionLabel() will persist that empty label. That breaks label-based resolution later because connectionSelectionSchema requires label: z.string().min(1) in packages/worker/src/mcp/connections/auth-spec.ts.
🏷️ Suggested fix
async function buildUniqueConnectionLabel(input: {
env: Env
userId: string
providerKey: string
preferredLabel: string
}) {
- const base = normalizeConnectionLabel(input.preferredLabel)
+ const base =
+ normalizeConnectionLabel(input.preferredLabel) ||
+ `${input.providerKey}-connection`
const existingConnections = await listProviderConnectionsByProvider(
input.env.APP_DB,
input.userId,
input.providerKey,Also applies to: 859-884
🤖 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 251 -
259, The preferredLabel passed into buildUniqueConnectionLabel may be an empty
string because normalizeConnectionLabel can return '' for blank/punctuation-only
names; update the call site in connection-service.ts (the block that builds
label using buildUniqueConnectionLabel and the helper verificationLabelFromBody)
to normalize the candidate label and if the normalized result is empty, fall
back to a non-empty default (e.g., `${draft.provider_key}-connection`);
similarly apply the same normalization+fallback logic to the other occurrence
referenced (lines ~859-884) so buildUniqueConnectionLabel never receives an
empty string and downstream schemas (connectionSelectionSchema) remain valid.
Comment on lines
+666
to
+671
| if (input.spec.token_auth_method === 'client_secret_basic') { | ||
| const credentials = `${input.secretMaterial['client_id'] ?? ''}:${input.secretMaterial['client_secret'] ?? ''}` | ||
| const encoded = base64UrlEncode(new TextEncoder().encode(credentials)) | ||
| .replaceAll('-', '+') | ||
| .replaceAll('_', '/') | ||
| headers.set('authorization', `Basic ${encoded}`) |
There was a problem hiding this comment.
Use standard Base64 for client_secret_basic.
base64UrlEncode(...).replaceAll('-', '+').replaceAll('_', '/') still leaves base64url semantics, most notably missing padding. Strict OAuth token endpoints can reject that Authorization: Basic ... header. Build it with standard Base64 instead of adapting the base64url helper.
🤖 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 666 -
671, The Authorization header is being built with a base64url helper and manual
replacements which can omit padding and break strict OAuth token endpoints; when
input.spec.token_auth_method === 'client_secret_basic' replace the
base64UrlEncode/TextEncoder flow with a proper standard Base64 encoding (e.g.
use Buffer.from(credentials).toString('base64') in Node or the platform btoa
equivalent) when creating the credentials string from
input.secretMaterial['client_id'] and ['client_secret'], then call
headers.set('authorization', `Basic ${...}`) as before; update the code in the
block handling client_secret_basic (the conditional using
input.spec.token_auth_method, credentials, and headers.set) to use standard
Base64 encoding.
cursor
Bot
force-pushed
the
cursor/testing-infra-adjustments-ac69
branch
from
March 25, 2026 20:35
15cfe4f to
73e6940
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
packages/worker/src/mcp-auth.ts (1)
145-152:⚠️ Potential issue | 🟠 MajorCooldown recorded before seeding completes, suppressing retries on failure.
recordBuiltinTemplateSeedat line 145 runs beforeensureBuiltinSkillTemplatesForUseris awaited. If seeding throws, the timestamp is already recorded, preventing retries for 5 minutes even though templates weren't seeded.Move the timestamp recording inside the try block after the await succeeds:
💡 Proposed fix
- recordBuiltinTemplateSeed(props.user.userId, now) try { await ensureBuiltinSkillTemplatesForUser(env, props.user.userId) + recordBuiltinTemplateSeed(props.user.userId, now) } catch (error) { console.warn('Failed to ensure builtin MCP skill templates', { error: error instanceof Error ? error.message : String(error), }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp-auth.ts` around lines 145 - 152, The timestamp is recorded before seeding completes which suppresses retries if seeding fails; move the call to recordBuiltinTemplateSeed(props.user.userId, now) so it executes only after ensureBuiltinSkillTemplatesForUser(env, props.user.userId) has successfully awaited (i.e., place the recordBuiltinTemplateSeed call inside the try block immediately after the await), keep the existing catch logging for failures, and ensure you still compute or pass the same now value when recording.packages/worker/src/mcp/connections/connection-service.ts (4)
104-117:⚠️ Potential issue | 🟠 MajorPrevalidate secret names before persisting anything.
This loop can upsert a subset of
input.fieldsand then throw on a later invalid name, so the API reports failure after already mutating the draft. Validate the full key set first, then write.🧰 Suggested fix
const allowedFields = new Set( getAuthSpecSecretFields(spec).map((field) => field.name), ) - for (const [name, value] of Object.entries(input.fields)) { - if (!allowedFields.has(name)) { - throw new Error(`Secret field "${name}" is not allowed for this draft.`) - } + const entries = Object.entries(input.fields) + const invalidName = entries.find(([name]) => !allowedFields.has(name))?.[0] + if (invalidName) { + throw new Error( + `Secret field "${invalidName}" is not allowed for this draft.`, + ) + } + for (const [name, value] of entries) { const encryptedValue = await encryptJson( input.env, encryptedDraftSecretPurpose, value, )🤖 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 104 - 117, The loop over Object.entries(input.fields) currently encrypts and upserts each secret as it iterates, which can leave partial mutations if a later key is invalid; before calling encryptJson or upsertConnectionDraftSecret you should pre-validate the full set of keys in input.fields against allowedFields (e.g., iterate keys and throw if any !allowedFields.has(name)), and only after all keys pass validation proceed to the existing loop that calls encryptJson and upsertConnectionDraftSecret for each field; update the logic around Object.entries(input.fields), allowedFields, encryptJson, and upsertConnectionDraftSecret accordingly so no writes occur until validation completes.
859-875:⚠️ Potential issue | 🟡 MinorFallback when normalization strips the label to empty.
normalizeConnectionLabel()returns''for blank or punctuation-only labels, and this helper will then persist an empty label. That breaks label-based resolution later becauseconnectionSelectionSchemarequires a non-emptylabel.🏷️ Suggested fix
- const base = normalizeConnectionLabel(input.preferredLabel) + const base = + normalizeConnectionLabel(input.preferredLabel) || + `${input.providerKey}-connection`🤖 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 859 - 875, normalizeConnectionLabel may return an empty string for blank/punctuation-only labels, causing an empty label to be persisted and later rejected by connectionSelectionSchema; update the helper so after computing base = normalizeConnectionLabel(input.preferredLabel) you detect an empty base and replace it with a non-empty default (e.g., "connection" or "untitled") before checking existing labels; keep the existing uniqueness loop (using listProviderConnectionsByProvider and existingLabels) but run it against the substituted default so the returned label is never empty and remains unique for the user/provider.
666-671:⚠️ Potential issue | 🟠 MajorUse standard Base64 for
client_secret_basic.This still adapts a base64url string by replacing
-/_, which leaves the padding stripped. Strict OAuth token endpoints can reject thatAuthorization: Basic ...header.packages/worker/src/mcp/connections/provider-request.tsalready usesbtoa(...)for the refresh flow; the initial code exchange should do the same.🔐 Suggested fix
if (input.spec.token_auth_method === 'client_secret_basic') { const credentials = `${input.secretMaterial['client_id'] ?? ''}:${input.secretMaterial['client_secret'] ?? ''}` - const encoded = base64UrlEncode(new TextEncoder().encode(credentials)) - .replaceAll('-', '+') - .replaceAll('_', '/') + const encoded = btoa(credentials) headers.set('authorization', `Basic ${encoded}`) } else {🤖 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 666 - 671, The current branch for handling input.spec.token_auth_method === 'client_secret_basic' builds a base64url string then replaces characters which strips padding and can break strict OAuth servers; replace that logic so the credentials (from input.secretMaterial['client_id'] and ['client_secret']) are encoded using standard Base64 (preserving padding) — e.g., use a standard base64 encoder such as Buffer.from(credentials).toString('base64') (or btoa in browser contexts, consistent with provider-request.ts) and set headers.set('authorization', `Basic ${<standardBase64>}`) in connection-service.ts.
227-239:⚠️ Potential issue | 🟠 MajorRequire the expected draft state before finalizing.
This still finalizes any draft that happens to have its configured secret fields, even if an OAuth flow never reached
authorized. Foroauth2_dynamic_clientwith emptysecret_fieldsand noverification, that creates anactiveconnection with no access token at all.🚦 Suggested fix
const spec = parseConnectionAuthSpec(draft.auth_spec_json) + if (spec.strategy === 'manual_token' || spec.strategy === 'api_key') { + if (draft.status !== 'ready_to_finalize') { + throw new Error('Connection draft is not ready to finalize.') + } + } else if (draft.status !== 'authorized') { + throw new Error('OAuth connection draft is not ready to finalize.') + } const secretMaterial = await loadDraftSecretMaterial(input.env, draft.id) const missingSecretNames = getMissingSecretNames(spec, secretMaterial)🤖 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 227 - 239, After loading the draft (getRequiredConnectionDraft) and parsing its auth spec (parseConnectionAuthSpec), validate the draft.state before finalizing: if the parsed spec indicates an OAuth dynamic client (spec.type === 'oauth2_dynamic_client') or the spec has no verification and relies on authorization, require draft.state === 'authorized' and throw a clear Error if not; otherwise proceed to load secret material (loadDraftSecretMaterial) and check missing secrets (getMissingSecretNames) as before. This ensures drafts that never completed an OAuth authorization cannot be finalized into active connections.
🧹 Nitpick comments (1)
packages/worker/src/mcp-auth.ts (1)
134-153: Concurrent requests for the same user can trigger parallel seeding.Since
recordBuiltinTemplateSeedis only effective after being called, multiple concurrent requests for the same user arriving before the first seeding completes will all pass the cooldown check and runensureBuiltinSkillTemplatesForUserin parallel. This multiplies writes and increases latency.Consider deduplicating in-flight seeding per user with a
Promisemap:♻️ Proposed approach
const builtinTemplateSeedByUser = new Map<string, number>() +const builtinTemplateSeedInFlightByUser = new Map<string, Promise<void>>() // ... in handleMcpRequest after cooldown check passes: - recordBuiltinTemplateSeed(props.user.userId, now) try { - await ensureBuiltinSkillTemplatesForUser(env, props.user.userId) + let inFlight = builtinTemplateSeedInFlightByUser.get(props.user.userId) + if (!inFlight) { + inFlight = ensureBuiltinSkillTemplatesForUser(env, props.user.userId) + .then(() => recordBuiltinTemplateSeed(props.user.userId, Date.now())) + .finally(() => builtinTemplateSeedInFlightByUser.delete(props.user.userId)) + builtinTemplateSeedInFlightByUser.set(props.user.userId, inFlight) + } + await inFlight } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp-auth.ts` around lines 134 - 153, Concurrent requests can bypass the cooldown because recordBuiltinTemplateSeed only sets a timestamp after being called, allowing parallel calls to run ensureBuiltinSkillTemplatesForUser; fix by introducing a per-user in-flight Promise map (e.g., builtinTemplateSeedInFlight: Map<string, Promise<void>>) and, inside the block where props.user.userId exists, check the cooldown as before but then before calling ensureBuiltinSkillTemplatesForUser create and store a Promise in builtinTemplateSeedInFlight keyed by userId (set the timestamp via recordBuiltinTemplateSeed immediately or when the promise is created), have that promise run ensureBuiltinSkillTemplatesForUser, catch errors inside it, and finally remove the entry from builtinTemplateSeedInFlight in a finally handler so concurrent requests await the same promise instead of triggering parallel seeds; reference functions/vars: builtinTemplateSeedByUser, recordBuiltinTemplateSeed, builtinTemplateSeedCooldownMs, ensureBuiltinSkillTemplatesForUser.
🤖 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 1132-1163: The hostBridge.callTool(...) calls in the
invokeAction/submitSecureInput flow (see hostBridge.callTool,
getHostToolErrorMessage, childMessagePrefix and the
`${childMessagePrefix}invoke-action-result`/`${childMessagePrefix}submit-secure-input-result`
response paths) currently only handle .then and never .catch, so rejections
leave the iframe hanging; add a rejection handler (either .catch or use
try/await with try/catch) that captures the thrown error, derives an
errorMessage via getHostToolErrorMessage(err) or falls back to err.message, and
always posts a response back to the child with requestId and ok: false plus the
error text (same shape as the existing error branch) so every call path
(fulfilled, error result, or thrown rejection) sends a
`${childMessagePrefix}*-result` message.
In `@packages/worker/src/mcp/capabilities/apps/ui-generated-ui-invoke-action.ts`:
- Around line 47-52: The code in ui-generated-ui-invoke-action.ts constructs
callerContext and force-sets homeConnectorId to 'default', which breaks
connector selection in registry.ts that reads callerContext.homeConnectorId;
instead, pull the originating connector id from the verified session payload
(e.g., session or session.user fields) and set callerContext.homeConnectorId to
that value (or leave it undefined if absent) rather than hardcoding 'default',
ensuring generated-UI actions use the correct connector; update the
callerContext construction in the callerContext variable accordingly.
In `@packages/worker/src/mcp/connections/provider-request.ts`:
- Around line 520-523: The current check using
trimmed.startsWith(config.path_prefix) allows paths like "/api2" when
config.path_prefix is "/api"; update the validation so the path either equals
the prefix exactly or the prefix is followed by a path separator. In the block
where you inspect config.path_prefix and trimmed, replace the startsWith check
with logic that verifies (trimmed === config.path_prefix) ||
(trimmed.startsWith(config.path_prefix + '/')), and throw the same Error if
neither condition holds; reference variables: config.path_prefix and trimmed in
provider-request.ts.
---
Duplicate comments:
In `@packages/worker/src/mcp-auth.ts`:
- Around line 145-152: The timestamp is recorded before seeding completes which
suppresses retries if seeding fails; move the call to
recordBuiltinTemplateSeed(props.user.userId, now) so it executes only after
ensureBuiltinSkillTemplatesForUser(env, props.user.userId) has successfully
awaited (i.e., place the recordBuiltinTemplateSeed call inside the try block
immediately after the await), keep the existing catch logging for failures, and
ensure you still compute or pass the same now value when recording.
In `@packages/worker/src/mcp/connections/connection-service.ts`:
- Around line 104-117: The loop over Object.entries(input.fields) currently
encrypts and upserts each secret as it iterates, which can leave partial
mutations if a later key is invalid; before calling encryptJson or
upsertConnectionDraftSecret you should pre-validate the full set of keys in
input.fields against allowedFields (e.g., iterate keys and throw if any
!allowedFields.has(name)), and only after all keys pass validation proceed to
the existing loop that calls encryptJson and upsertConnectionDraftSecret for
each field; update the logic around Object.entries(input.fields), allowedFields,
encryptJson, and upsertConnectionDraftSecret accordingly so no writes occur
until validation completes.
- Around line 859-875: normalizeConnectionLabel may return an empty string for
blank/punctuation-only labels, causing an empty label to be persisted and later
rejected by connectionSelectionSchema; update the helper so after computing base
= normalizeConnectionLabel(input.preferredLabel) you detect an empty base and
replace it with a non-empty default (e.g., "connection" or "untitled") before
checking existing labels; keep the existing uniqueness loop (using
listProviderConnectionsByProvider and existingLabels) but run it against the
substituted default so the returned label is never empty and remains unique for
the user/provider.
- Around line 666-671: The current branch for handling
input.spec.token_auth_method === 'client_secret_basic' builds a base64url string
then replaces characters which strips padding and can break strict OAuth
servers; replace that logic so the credentials (from
input.secretMaterial['client_id'] and ['client_secret']) are encoded using
standard Base64 (preserving padding) — e.g., use a standard base64 encoder such
as Buffer.from(credentials).toString('base64') (or btoa in browser contexts,
consistent with provider-request.ts) and set headers.set('authorization', `Basic
${<standardBase64>}`) in connection-service.ts.
- Around line 227-239: After loading the draft (getRequiredConnectionDraft) and
parsing its auth spec (parseConnectionAuthSpec), validate the draft.state before
finalizing: if the parsed spec indicates an OAuth dynamic client (spec.type ===
'oauth2_dynamic_client') or the spec has no verification and relies on
authorization, require draft.state === 'authorized' and throw a clear Error if
not; otherwise proceed to load secret material (loadDraftSecretMaterial) and
check missing secrets (getMissingSecretNames) as before. This ensures drafts
that never completed an OAuth authorization cannot be finalized into active
connections.
---
Nitpick comments:
In `@packages/worker/src/mcp-auth.ts`:
- Around line 134-153: Concurrent requests can bypass the cooldown because
recordBuiltinTemplateSeed only sets a timestamp after being called, allowing
parallel calls to run ensureBuiltinSkillTemplatesForUser; fix by introducing a
per-user in-flight Promise map (e.g., builtinTemplateSeedInFlight: Map<string,
Promise<void>>) and, inside the block where props.user.userId exists, check the
cooldown as before but then before calling ensureBuiltinSkillTemplatesForUser
create and store a Promise in builtinTemplateSeedInFlight keyed by userId (set
the timestamp via recordBuiltinTemplateSeed immediately or when the promise is
created), have that promise run ensureBuiltinSkillTemplatesForUser, catch errors
inside it, and finally remove the entry from builtinTemplateSeedInFlight in a
finally handler so concurrent requests await the same promise instead of
triggering parallel seeds; reference functions/vars: builtinTemplateSeedByUser,
recordBuiltinTemplateSeed, builtinTemplateSeedCooldownMs,
ensureBuiltinSkillTemplatesForUser.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5f27d2c-cd81-41f1-a581-cc3c0ae00598
📒 Files selected for processing (51)
packages/worker/client/app-session-refresh.test.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/apps/domain.tspackages/worker/src/mcp/capabilities/apps/ui-generated-ui-invoke-action.tspackages/worker/src/mcp/capabilities/apps/ui-generated-ui-submit-secure-input.tspackages/worker/src/mcp/capabilities/builtin-domains.tspackages/worker/src/mcp/capabilities/capability-search.test.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/connections/resolve-field-update.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/test-process.tspackages/worker/src/mcp/tools/open-generated-ui.tspackages/worker/src/mcp/tools/search.ts
✅ Files skipped from review due to trivial changes (15)
- packages/worker/src/mcp/index.ts
- packages/worker/src/mcp/capabilities/capability-search.test.ts
- packages/worker/src/mcp/capabilities/domain-metadata.ts
- packages/worker/migrations/0009-skill-connection-bindings.sql
- packages/worker/src/mcp/mcp-server-e2e.test.ts
- packages/worker/src/mcp/capabilities/connections/domain.ts
- packages/worker/src/mcp/capabilities/apps/domain.ts
- packages/worker/src/mcp/connections/oauth-api.ts
- packages/worker/src/mcp/connections/resolve-field-update.ts
- packages/worker/src/mcp/capabilities/connections/provider-refresh-token.ts
- packages/worker/src/mcp/capabilities/connections/provider-http-request.ts
- packages/worker/src/mcp/connections/provider-connections-types.ts
- packages/worker/src/mcp/connections/connection-drafts-types.ts
- packages/worker/src/mcp/skills/skill-connections.ts
- packages/worker/migrations/0008-provider-connections.sql
🚧 Files skipped from review as they are similar to previous changes (22)
- packages/worker/src/mcp/test-process.ts
- packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts
- packages/worker/src/mcp/tools/search.ts
- packages/worker/src/mcp/skills/mcp-skills-types.ts
- packages/worker/src/mcp/capabilities/meta/meta-update-skill.ts
- packages/worker/client/app-session-refresh.test.ts
- packages/worker/src/mcp/capabilities/connections/connections-set-default.ts
- packages/worker/src/mcp/capabilities/builtin-domains.ts
- packages/worker/src/mcp/capabilities/connections/provider-graphql-request.ts
- packages/worker/src/mcp/connections/connection-handles.ts
- packages/worker/src/mcp/capabilities/connections/connections-list.ts
- packages/worker/src/mcp/tools/open-generated-ui.ts
- packages/worker/src/mcp/capabilities/connections/connections-disconnect.ts
- packages/worker/src/mcp/capabilities/connections/connections-finalize.ts
- packages/worker/src/mcp/skills/skill-mutation.ts
- packages/worker/src/mcp/skills/builtin-skill-templates.ts
- packages/worker/src/mcp/capabilities/connections/connections-begin-setup.ts
- packages/worker/src/mcp/capabilities/apps/ui-generated-ui-submit-secure-input.ts
- packages/worker/src/mcp/capabilities/connections/connections-start-oauth.ts
- packages/worker/src/index.ts
- packages/worker/src/mcp/capabilities/connections/connections-resolve.ts
- packages/worker/src/mcp/capabilities/meta/meta-get-skill.ts
Comment on lines
+47
to
+52
| const callerContext = { | ||
| ...ctx.callerContext, | ||
| baseUrl: ctx.callerContext.baseUrl, | ||
| user: session.user, | ||
| homeConnectorId: 'default', | ||
| } |
There was a problem hiding this comment.
Don't force generated-UI actions onto the default home connector.
packages/worker/src/mcp/capabilities/registry.ts:27-42 uses callerContext.homeConnectorId to decide which home domain to load. Overwriting it with 'default' here makes generated-UI actions run against the wrong connector and breaks non-default/connection-backed sessions.
🛠️ Suggested fix
const callerContext = {
...ctx.callerContext,
baseUrl: ctx.callerContext.baseUrl,
user: session.user,
- homeConnectorId: 'default',
+ homeConnectorId: ctx.callerContext.homeConnectorId,
}If the originating connector is supposed to come from the session rather than the ambient caller context, restore it from the verified session payload instead of hardcoding 'default'.
📝 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
| const callerContext = { | |
| ...ctx.callerContext, | |
| baseUrl: ctx.callerContext.baseUrl, | |
| user: session.user, | |
| homeConnectorId: 'default', | |
| } | |
| const callerContext = { | |
| ...ctx.callerContext, | |
| baseUrl: ctx.callerContext.baseUrl, | |
| user: session.user, | |
| homeConnectorId: ctx.callerContext.homeConnectorId, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/apps/ui-generated-ui-invoke-action.ts`
around lines 47 - 52, The code in ui-generated-ui-invoke-action.ts constructs
callerContext and force-sets homeConnectorId to 'default', which breaks
connector selection in registry.ts that reads callerContext.homeConnectorId;
instead, pull the originating connector id from the verified session payload
(e.g., session or session.user fields) and set callerContext.homeConnectorId to
that value (or leave it undefined if absent) rather than hardcoding 'default',
ensuring generated-UI actions use the correct connector; update the
callerContext construction in the callerContext variable accordingly.
Comment on lines
+520
to
+523
| if (config.path_prefix && !trimmed.startsWith(config.path_prefix)) { | ||
| throw new Error( | ||
| `path must start with \`${config.path_prefix}\` for this provider connection.`, | ||
| ) |
There was a problem hiding this comment.
Enforce path_prefix on a path boundary.
startsWith(config.path_prefix) also accepts /api2/... when the configured prefix is /api, so callers can escape the intended subtree on the same host. This needs an exact-prefix-or-prefix/ check.
🛡️ Suggested fix
- if (config.path_prefix && !trimmed.startsWith(config.path_prefix)) {
- throw new Error(
- `path must start with \`${config.path_prefix}\` for this provider connection.`,
- )
- }
+ if (config.path_prefix) {
+ const prefix =
+ config.path_prefix.length > 1 && config.path_prefix.endsWith('/')
+ ? config.path_prefix.slice(0, -1)
+ : config.path_prefix
+ const withinPrefix =
+ prefix === '/' || trimmed === prefix || trimmed.startsWith(`${prefix}/`)
+ if (!withinPrefix) {
+ throw new Error(
+ `path must stay within \`${config.path_prefix}\` for this provider connection.`,
+ )
+ }
+ }🤖 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 520 -
523, The current check using trimmed.startsWith(config.path_prefix) allows paths
like "/api2" when config.path_prefix is "/api"; update the validation so the
path either equals the prefix exactly or the prefix is followed by a path
separator. In the block where you inspect config.path_prefix and trimmed,
replace the startsWith check with logic that verifies (trimmed ===
config.path_prefix) || (trimmed.startsWith(config.path_prefix + '/')), and throw
the same Error if neither condition holds; reference variables:
config.path_prefix and trimmed in provider-request.ts.
| console.warn('Failed to ensure builtin MCP skill templates', { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Builtin template seed recorded before async operation succeeds
Medium Severity
recordBuiltinTemplateSeed is called before ensureBuiltinSkillTemplatesForUser runs. If the async seeding fails (caught by the try/catch), the cooldown timestamp is already recorded, so the user won't get another seeding attempt for 5 minutes. The recordBuiltinTemplateSeed call belongs after the await succeeds, not before.
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
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>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Use one /ui-api/:id/* contract so hosted and MCP-rendered widgets can load source, execute code, and submit secure input through the same shell behavior. Made-with: Cursor
kentcdodds
force-pushed
the
cursor/testing-infra-adjustments-ac69
branch
from
March 25, 2026 21:59
c32e444 to
0dd978d
Compare
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.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
actions/setup-node+npm cican run in both validate and preview workflowsTesting
npm cinpm run format:checknpm run buildnpm run typechecknpm run testnpm run test:mcpSummary by CodeRabbit
Release Notes