Repository navigation
feat: add secret-aware fetch gateway - #61
Merged
kentcdodds merged 23 commits intoMar 27, 2026
Merged
Conversation
Co-authored-by: me <me@kentcdodds.com>
|
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:
📝 WalkthroughWalkthroughAdds a secret-host approval flow: new account UI/API for approving outbound-host use of saved secrets, DB/schema and service changes to persist allowed hosts, a fetch gateway that expands {{secret:...}} placeholders and enforces approvals, executor/registry changes to route fetches through the gateway, and test/docs updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant AccountAPI as Account API
participant Auth as Auth Session
participant DB as Database
User->>Browser: Open /account/secrets/approve?request=TOKEN
Browser->>AccountAPI: GET /account/secrets.json?request=TOKEN
AccountAPI->>Auth: validate session
Auth-->>AccountAPI: userId
AccountAPI->>DB: verify token & load secret metadata
DB-->>AccountAPI: secret metadata (incl. allowed_hosts)
AccountAPI-->>Browser: { approval, secrets }
Browser->>User: render approval UI
User->>Browser: Click "Approve"
Browser->>AccountAPI: POST /account/secrets.json { action: "approve", requestToken: TOKEN }
AccountAPI->>DB: append requestedHost to allowed_hosts
DB-->>AccountAPI: updated metadata
AccountAPI-->>Browser: { secrets, approval: null }
Browser->>Browser: navigate to /account
sequenceDiagram
participant Exec as Execute-time Code
participant Gateway as CodemodeFetchGateway
participant SecretSvc as Secret Service
participant Host as External Host
Exec->>Gateway: fetch(url, { headers/body with {{secret:...}} })
Gateway->>Gateway: extract placeholders
Gateway->>SecretSvc: resolveSecretForHost(name, scope, targetHost)
SecretSvc-->>Gateway: { value?, allowedForHost: true/false }
alt allowed
Gateway->>Gateway: replace placeholders with values
Gateway->>Host: fetch(transformed request)
Host-->>Gateway: response
Gateway-->>Exec: response
else not allowed
Gateway->>Gateway: create approval token & URL
Gateway-->>Exec: throw Error(with approval URL)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kentcdodds
marked this pull request as ready for review
March 27, 2026 00:54
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/worker/src/mcp/executor.ts (1)
31-37:⚠️ Potential issue | 🔴 CriticalFilter value-returning secret capabilities from execute-time registry.
The wrapper correctly restricts
secrets.getandsecrets.requireaccess, butbuildCodemodeFns()exposes the fullcapabilityHandlersregistry without filtering. Code running in the execute sandbox can still callcodemode.secret_get(...)directly, bypassing the intentional approval-flow restriction. Thesecret_getcapability handler returns plaintext secret values, creating the vulnerability.
getCapabilityRegistryForContext()must filter outsecret_getandsecret_requirewhen called from execute context, or these capabilities must be excluded at thebuildCodemodeFns()orbuildCodemodeProvider()level when building the execute-time registry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/executor.ts` around lines 31 - 37, buildCodemodeFns()/buildCodemodeProvider() currently exposes the full capabilityHandlers registry so code in the execute sandbox can call codemode.secret_get/secret_require and obtain plaintext secrets; update getCapabilityRegistryForContext (or filter at buildCodemodeFns()/buildCodemodeProvider time) to explicitly remove or block the 'secret_get' and 'secret_require' handlers when the context is an execute-time sandbox, ensuring the execute registry returned by getCapabilityRegistryForContext only includes allowed handlers and that buildCodemodeFns() uses that filtered registry rather than capabilityHandlers directly.packages/worker/src/mcp/mcp-server-e2e.test.ts (1)
1218-1256:⚠️ Potential issue | 🔴 CriticalTest fails because only one of three secrets is approved for the host.
The test approves
cloudflareTokenforapi.example.com, but the fetch request also usesglobalApiKey(user scope) andephemeralCode(session scope) as secret placeholders. According tofetch-gateway.ts, each secret placeholder is resolved and checked against itsallowedHosts. SinceglobalApiKeyandephemeralCodewere never approved forapi.example.com, the gateway will reject the request with a 400 error for the first unapproved secret it encounters.To fix this, either:
- Approve all three secrets for the host before the final fetch, or
- Simplify the test to use only one secret placeholder in the "approved fetch" scenario.
🐛 Option 1: Approve all secrets (add approval loops for globalApiKey and ephemeralCode)
Alternatively, simplify the approved fetch to only use the approved secret:
const approvedFetchExecuteResponse = await fetch(executeUrl!, { method: 'POST', headers: { Authorization: `Bearer ${token}`, 'Content-Type': 'application/json', Accept: 'application/json', }, body: JSON.stringify({ code: `async () => { const response = await fetch('https://api.example.com/deploy', { method: 'POST', headers: { Authorization: 'Bearer {{secret:cloudflareToken|scope=app}}', - 'X-Global-Key': '{{secret:globalApiKey|scope=user}}', - 'X-Session-Code': '{{secret:ephemeralCode|scope=session}}', }, body: JSON.stringify({ note: 'deploy' }), }) return { ok: response.ok, status: response.status, } }`, }), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/mcp-server-e2e.test.ts` around lines 1218 - 1256, The approved-fetch test is failing because only cloudflareToken was approved for api.example.com while the executed code also uses globalApiKey and ephemeralCode placeholders; update the test to approve those secrets as well before calling the execute endpoint (mirror how cloudflareToken was approved) so all three secrets (cloudflareToken, globalApiKey, ephemeralCode) have allowedHosts including api.example.com, or alternatively change the executed code payload in the POST body to use only the already-approved cloudflareToken placeholder; adjust the setup that creates approvals (the same helper/loop used for cloudflareToken) or the POST body string used in approvedFetchExecuteResponse to ensure all placeholders are valid for api.example.com.
🧹 Nitpick comments (7)
packages/worker/client/routes/index.tsx (1)
16-16: Add a client route entry for/account/secretsto keep path parity.Since approval path is explicitly mapped, mapping the base secrets path too helps avoid client-side route misses when navigating directly to secrets management.
Proposed route-map addition
export const clientRoutes = { '/': <HomeRoute />, '/chat': <ChatRoute />, '/chat/:threadId': <ChatRoute />, '/ui/:id': <SavedUiRoute />, '/account': <AccountRoute />, + '/account/secrets': <AccountRoute />, '/account/secrets/approve': <AccountRoute />, '/login': <LoginRoute />,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/client/routes/index.tsx` at line 16, Add a client route entry for the base secrets path so client-side navigation to /account/secrets works; update the route map that currently contains '/account/secrets/approve': <AccountRoute /> by adding an entry for '/account/secrets': <AccountRoute /> (or the appropriate component) next to the approve route in the routes object so both paths are explicitly mapped.packages/worker/src/app/router.ts (1)
50-51: Optional: reuse onecreateAccountSecretsHandler(...)instance.You can reduce tiny duplication by creating one handler variable and mapping both page routes to it.
♻️ Small refactor
+ const accountSecretsHandler = createAccountSecretsHandler(appEnv as Env) - router.map(routes.accountSecrets, createAccountSecretsHandler(appEnv as Env)) - router.map(routes.accountSecretsApprove, createAccountSecretsHandler(appEnv as Env)) + router.map(routes.accountSecrets, accountSecretsHandler) + router.map(routes.accountSecretsApprove, accountSecretsHandler)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/app/router.ts` around lines 50 - 51, Create a single handler instance by calling createAccountSecretsHandler(appEnv as Env) once and reuse it for both router.map calls (routes.accountSecrets and routes.accountSecretsApprove) instead of calling createAccountSecretsHandler twice; locate the two router.map lines that reference createAccountSecretsHandler, extract the result into a const (e.g., accountSecretsHandler) and pass that variable to both router.map invocations.packages/worker/src/mcp/index.ts (1)
2-2: Convert to a type-only import forworkerExports.This symbol is only used in a type assertion on line 115 (
as typeof workerExports), so importing it at runtime is unnecessary and triggers theconsistent-type-importslint rule.🧹 Minimal fix
-import { exports as workerExports } from 'cloudflare:workers' +import { type exports as WorkerExports } from 'cloudflare:workers' ... getLoopbackExports() { - return this.ctx.exports as typeof workerExports + return this.ctx.exports as WorkerExports }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/index.ts` at line 2, Change the runtime import of exports to a type-only import: replace the current import of "exports as workerExports" from 'cloudflare:workers' with a type-only import so the value is not emitted at runtime (used only for the type assertion `as typeof workerExports` on line 115). Update the import statement to use the TypeScript `import type` form for the symbol workerExports and leave the type assertion unchanged.packages/worker/src/app/handlers/account-secrets.ts (1)
114-120: Fix indentation issue in approval block.The code inside the
if (action === 'approve')block has inconsistent indentation - line 115 appears to use spaces instead of tabs or has extra indentation.♻️ Proposed fix for consistent indentation
if (action === 'approve') { - const current = await listSecrets({ - env, - userId: user.mcpUser.userId, - scope: approval.scope, - secretContext: approval.secretContext, - }) + const current = await listSecrets({ + env, + userId: user.mcpUser.userId, + scope: approval.scope, + secretContext: approval.secretContext, + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/app/handlers/account-secrets.ts` around lines 114 - 120, The code inside the if (action === 'approve') block is mis-indented; align the subsequent await listSecrets call and its object properties (env, user.mcpUser.userId, approval.scope, approval.secretContext) consistently with the project's indentation style so the entire block is visually nested under the if statement (i.e., same indentation level for the await listSecrets line and its object keys), ensuring no mixed tabs/spaces remain.packages/worker/src/mcp/fetch-gateway.ts (2)
37-97: Consider handling multiple unapproved secrets more gracefully.Currently, when multiple secrets are used in a request and several are unapproved for the target host, the function throws on the first unapproved secret encountered. This could lead to a frustrating UX where users must approve hosts one secret at a time.
Consider collecting all unapproved secrets and including all approval URLs in a single error message, or at minimum documenting this behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/fetch-gateway.ts` around lines 37 - 97, The loop in expandSecretPlaceholders stops at the first secret with allowedForHost !== true and throws, forcing one-at-a-time approvals; change it to collect all unapproved secrets by name (and their scope) as you iterate (using the existing resolveSecretForHost results), generate approval tokens via createSecretHostApprovalToken and URLs via buildSecretHostApprovalUrl for each unapproved secret, and after resolving all references throw a single Error that lists every unapproved secret along with its approval URL(s); keep the existing checks for not found secrets and the replacement logic (buildSecretPlaceholder, replacements.set) intact so valid secrets still get processed.
141-150: Potential ReDoS or performance issue with large replacement maps.The
replaceSecretPlaceholdersfunction callsreplaceAllin a loop for each placeholder. For requests with many secret references or long bodies, this could be inefficient. Consider using a single regex replacement pass if performance becomes a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/fetch-gateway.ts` around lines 141 - 150, The function replaceSecretPlaceholders currently loops and calls String.replaceAll for each entry which can be O(n*m) and cause ReDoS/perf issues for large replacement maps; refactor replaceSecretPlaceholders to perform a single pass by building one combined regex from the replacement map keys (escape regex metacharacters when constructing the pattern), then call value.replace(combinedRegex, match => replacements.get(match) || match) so all placeholders are replaced in one traversal; ensure you still accept a ReadonlyMap<string,string> and handle potential overlapping keys by ordering or using a non-capturing group in the combined pattern.packages/worker/src/mcp/secrets/host-approval.ts (1)
56-63: Consider validatingscopeagainst allowed values.The verification only checks that
scopeis a string before casting toSecretScope. While tokens are system-generated with valid values, adding explicit validation would provide defense-in-depth against token tampering or future bugs.♻️ Proposed validation
+const validScopes = ['session', 'app', 'user'] as const + if ( typeof parsed.userId !== 'string' || typeof parsed.name !== 'string' || typeof parsed.scope !== 'string' || - typeof parsed.requestedHost !== 'string' + typeof parsed.requestedHost !== 'string' || + !validScopes.includes(parsed.scope as typeof validScopes[number]) ) { throw new Error('Invalid secret host approval request.') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/secrets/host-approval.ts` around lines 56 - 63, The current check only ensures parsed.scope is a string but doesn't verify it's one of the allowed SecretScope values; update validation in host-approval.ts to assert parsed.scope is a valid SecretScope (e.g., compare against Object.values(SecretScope) or use an isValidSecretScope helper) before casting, and throw a clear error (instead of silently accepting) if the value is not permitted; reference parsed.scope and SecretScope so reviewers can locate and change the validation logic where the secret host approval request is parsed.
🤖 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/routes/account.tsx`:
- Around line 216-230: The Approve and Reject buttons are missing onClick
handlers so submitApproval is never invoked; wire the buttons to call
submitApproval with the appropriate action strings and disable behavior: for the
"Approve host" button call submitApproval('approve') and for the "Reject" button
call submitApproval('reject'), preserving the existing disabled prop that checks
submittingApprovalAction and keeping the css props (primaryButtonCss,
secondaryButtonCss); ensure any event default is prevented if needed and that
submitApproval is in-scope where the buttons are rendered.
In `@packages/worker/src/app/authenticated-user.ts`:
- Line 42: The change setting mcpUser.userId to emailBasedUserId breaks
verification of existing approval tokens because token validation does strict
equality against mcpUser.userId; revert to maintaining the previous stable ID or
support both formats during verification. Update authenticated-user.ts to (1)
preserve the original ID value used for past tokens (e.g., keep legacyUserId or
retain numeric/previous format) when assigning mcpUser.userId and (2) modify the
approval-token verification logic (the function that compares token.subject or
token.userId to mcpUser.userId) to accept either the legacy ID or
emailBasedUserId (i.e., check equality against both mcpUser.userId and the
legacy form) so in-flight approval links remain valid. Ensure references to
mcpUser.userId and the token verification routine are updated together to avoid
mismatches.
In `@packages/worker/src/app/handlers/account-secrets.ts`:
- Line 11: Remove the unused import getUiArtifactByOwnerIds from the import
statement in account-secrets.ts (where getUiArtifactByOwnerIds and
listUiArtifactsByUserId are imported from '#mcp/ui-artifacts-repo.ts'); keep the
used symbol listUiArtifactsByUserId and update the import to only include what
is referenced to resolve the TS6133 unused import/typecheck error.
In `@packages/worker/src/mcp/executor.ts`:
- Around line 14-24: In createExecuteExecutor ensure we fail-closed when the
CodemodeFetchGateway export is absent: before constructing the
DynamicWorkerExecutor, resolve the gateway export from (input.exports ??
workerExports).CodemodeFetchGateway, and if it is undefined throw a clear error;
otherwise call that export with gatewayProps and pass the resulting function
into new DynamicWorkerExecutor as globalOutbound. This enforces that missing
CodemodeFetchGateway (used for secret placeholder resolution and fetch
permission validation) causes an explicit failure instead of silently setting
globalOutbound to undefined.
---
Outside diff comments:
In `@packages/worker/src/mcp/executor.ts`:
- Around line 31-37: buildCodemodeFns()/buildCodemodeProvider() currently
exposes the full capabilityHandlers registry so code in the execute sandbox can
call codemode.secret_get/secret_require and obtain plaintext secrets; update
getCapabilityRegistryForContext (or filter at
buildCodemodeFns()/buildCodemodeProvider time) to explicitly remove or block the
'secret_get' and 'secret_require' handlers when the context is an execute-time
sandbox, ensuring the execute registry returned by
getCapabilityRegistryForContext only includes allowed handlers and that
buildCodemodeFns() uses that filtered registry rather than capabilityHandlers
directly.
In `@packages/worker/src/mcp/mcp-server-e2e.test.ts`:
- Around line 1218-1256: The approved-fetch test is failing because only
cloudflareToken was approved for api.example.com while the executed code also
uses globalApiKey and ephemeralCode placeholders; update the test to approve
those secrets as well before calling the execute endpoint (mirror how
cloudflareToken was approved) so all three secrets (cloudflareToken,
globalApiKey, ephemeralCode) have allowedHosts including api.example.com, or
alternatively change the executed code payload in the POST body to use only the
already-approved cloudflareToken placeholder; adjust the setup that creates
approvals (the same helper/loop used for cloudflareToken) or the POST body
string used in approvedFetchExecuteResponse to ensure all placeholders are valid
for api.example.com.
---
Nitpick comments:
In `@packages/worker/client/routes/index.tsx`:
- Line 16: Add a client route entry for the base secrets path so client-side
navigation to /account/secrets works; update the route map that currently
contains '/account/secrets/approve': <AccountRoute /> by adding an entry for
'/account/secrets': <AccountRoute /> (or the appropriate component) next to the
approve route in the routes object so both paths are explicitly mapped.
In `@packages/worker/src/app/handlers/account-secrets.ts`:
- Around line 114-120: The code inside the if (action === 'approve') block is
mis-indented; align the subsequent await listSecrets call and its object
properties (env, user.mcpUser.userId, approval.scope, approval.secretContext)
consistently with the project's indentation style so the entire block is
visually nested under the if statement (i.e., same indentation level for the
await listSecrets line and its object keys), ensuring no mixed tabs/spaces
remain.
In `@packages/worker/src/app/router.ts`:
- Around line 50-51: Create a single handler instance by calling
createAccountSecretsHandler(appEnv as Env) once and reuse it for both router.map
calls (routes.accountSecrets and routes.accountSecretsApprove) instead of
calling createAccountSecretsHandler twice; locate the two router.map lines that
reference createAccountSecretsHandler, extract the result into a const (e.g.,
accountSecretsHandler) and pass that variable to both router.map invocations.
In `@packages/worker/src/mcp/fetch-gateway.ts`:
- Around line 37-97: The loop in expandSecretPlaceholders stops at the first
secret with allowedForHost !== true and throws, forcing one-at-a-time approvals;
change it to collect all unapproved secrets by name (and their scope) as you
iterate (using the existing resolveSecretForHost results), generate approval
tokens via createSecretHostApprovalToken and URLs via buildSecretHostApprovalUrl
for each unapproved secret, and after resolving all references throw a single
Error that lists every unapproved secret along with its approval URL(s); keep
the existing checks for not found secrets and the replacement logic
(buildSecretPlaceholder, replacements.set) intact so valid secrets still get
processed.
- Around line 141-150: The function replaceSecretPlaceholders currently loops
and calls String.replaceAll for each entry which can be O(n*m) and cause
ReDoS/perf issues for large replacement maps; refactor replaceSecretPlaceholders
to perform a single pass by building one combined regex from the replacement map
keys (escape regex metacharacters when constructing the pattern), then call
value.replace(combinedRegex, match => replacements.get(match) || match) so all
placeholders are replaced in one traversal; ensure you still accept a
ReadonlyMap<string,string> and handle potential overlapping keys by ordering or
using a non-capturing group in the combined pattern.
In `@packages/worker/src/mcp/index.ts`:
- Line 2: Change the runtime import of exports to a type-only import: replace
the current import of "exports as workerExports" from 'cloudflare:workers' with
a type-only import so the value is not emitted at runtime (used only for the
type assertion `as typeof workerExports` on line 115). Update the import
statement to use the TypeScript `import type` form for the symbol workerExports
and leave the type assertion unchanged.
In `@packages/worker/src/mcp/secrets/host-approval.ts`:
- Around line 56-63: The current check only ensures parsed.scope is a string but
doesn't verify it's one of the allowed SecretScope values; update validation in
host-approval.ts to assert parsed.scope is a valid SecretScope (e.g., compare
against Object.values(SecretScope) or use an isValidSecretScope helper) before
casting, and throw a clear error (instead of silently accepting) if the value is
not permitted; reference parsed.scope and SecretScope so reviewers can locate
and change the validation logic where the secret host approval request is
parsed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8eadf83-9f04-4dc1-ba92-c3230467f58e
📒 Files selected for processing (32)
packages/worker/client/routes/account.tsxpackages/worker/client/routes/index.tsxpackages/worker/migrations/0009-secret-allowed-hosts.sqlpackages/worker/src/app/authenticated-user.tspackages/worker/src/app/handlers/account-secrets.tspackages/worker/src/app/handlers/account.tspackages/worker/src/app/router.tspackages/worker/src/app/routes.tspackages/worker/src/index.tspackages/worker/src/mcp/capabilities/meta/meta-run-skill.tspackages/worker/src/mcp/capabilities/secrets/domain.tspackages/worker/src/mcp/capabilities/secrets/secret-get.tspackages/worker/src/mcp/capabilities/secrets/secret-list.tspackages/worker/src/mcp/capabilities/secrets/secret-update.tspackages/worker/src/mcp/capabilities/secrets/shared.tspackages/worker/src/mcp/capabilities/unified-search.test.tspackages/worker/src/mcp/capabilities/unified-search.tspackages/worker/src/mcp/executor.tspackages/worker/src/mcp/fetch-gateway.tspackages/worker/src/mcp/generated-ui-api.tspackages/worker/src/mcp/index.tspackages/worker/src/mcp/mcp-registration-agent.tspackages/worker/src/mcp/mcp-server-e2e.test.tspackages/worker/src/mcp/run-codemode-registry.tspackages/worker/src/mcp/secrets/allowed-hosts.tspackages/worker/src/mcp/secrets/host-approval.tspackages/worker/src/mcp/secrets/repo.tspackages/worker/src/mcp/secrets/service.tspackages/worker/src/mcp/secrets/types.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/mcp/tools/open-generated-ui.tspackages/worker/src/test-support/cloudflare-workers-stub.ts
| ), | ||
| mcpUser: { | ||
| userId: session.id, | ||
| userId: emailBasedUserId, |
There was a problem hiding this comment.
User ID format switch can invalidate in-flight approval links.
Changing mcpUser.userId here breaks compatibility with approval tokens minted with the previous ID representation, because verification later does strict equality against the current mcpUser.userId and returns 403 on mismatch.
Suggested compatibility patch (account-secrets verification)
- if (approval.userId !== user.mcpUser.userId) {
+ const validUserIds = new Set([
+ user.mcpUser.userId,
+ user.sessionUserId,
+ ...user.artifactOwnerIds,
+ ])
+ if (!validUserIds.has(approval.userId)) {
return jsonResponse({ ok: false, error: 'Approval request mismatch.' }, 403)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/app/authenticated-user.ts` at line 42, The change setting
mcpUser.userId to emailBasedUserId breaks verification of existing approval
tokens because token validation does strict equality against mcpUser.userId;
revert to maintaining the previous stable ID or support both formats during
verification. Update authenticated-user.ts to (1) preserve the original ID value
used for past tokens (e.g., keep legacyUserId or retain numeric/previous format)
when assigning mcpUser.userId and (2) modify the approval-token verification
logic (the function that compares token.subject or token.userId to
mcpUser.userId) to accept either the legacy ID or emailBasedUserId (i.e., check
equality against both mcpUser.userId and the legacy form) so in-flight approval
links remain valid. Ensure references to mcpUser.userId and the token
verification routine are updated together to avoid mismatches.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/worker/src/app/handlers/account-secrets.ts (1)
127-134: Consider deduplicating allowed hosts before persisting.If the user somehow triggers an approval for a host that's already in
allowedHosts, line 132 will add a duplicate entry. While unlikely in normal flow, a defensive check prevents data inconsistency.♻️ Proposed fix to prevent duplicates
const secret = current.find( (item) => item.name === approval.name && item.scope === approval.scope, ) if (!secret) { return jsonResponse({ ok: false, error: 'Secret not found.' }, 404) } + const newAllowedHosts = secret.allowedHosts.includes(approval.requestedHost) + ? secret.allowedHosts + : [...secret.allowedHosts, approval.requestedHost] await setSecretAllowedHosts({ env, userId: user.mcpUser.userId, name: approval.name, scope: approval.scope, - allowedHosts: [...secret.allowedHosts, approval.requestedHost], + allowedHosts: newAllowedHosts, secretContext: approval.secretContext, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/app/handlers/account-secrets.ts` around lines 127 - 134, The call to setSecretAllowedHosts can append approval.requestedHost to secret.allowedHosts and create duplicates; before calling setSecretAllowedHosts (in the block using setSecretAllowedHosts, secret.allowedHosts, and approval.requestedHost) deduplicate the resulting allowedHosts array (e.g., construct the new list by combining secret.allowedHosts and approval.requestedHost then removing duplicates via a Set or Array.filter) and pass that deduplicated array as allowedHosts to setSecretAllowedHosts.
🤖 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/routes/account.tsx`:
- Around line 216-231: The onClick handlers call the async function
submitApproval('approve'/'reject') directly, returning a Promise<void> and
triggering a TS type error; change the handlers to explicitly discard the
promise so they return void (e.g., call submitApproval inside a void-expression
or an inline block that calls submitApproval and returns void) for both the
Approve host and Reject buttons (referencing the submitApproval function and the
two button onClick props).
- Around line 133-147: The success message is being cleared on the next render
because navigate('/account') changes currentHref and loadAccountSecrets resets
message; instead update the UI in place: remove the navigate('/account') call in
the try block where message is set (the block that sets email, secrets,
approval, submittingApprovalAction and message), call handle.update() to render
the success message immediately, and then remove the request query param from
the URL without causing a full navigation (e.g., replace the history entry) so
loadAccountSecrets is not retriggered; references: the try/catch block that sets
message and calls navigate('/account'), loadAccountSecrets which sets message =
null, and handle.update().
In `@packages/worker/src/app/handlers/account-secrets.ts`:
- Around line 107-111: The approval check fails when the token's userId (set in
fetch-gateway.ts as input.props.userId!) doesn’t match the authenticated
session's email-based id (user.mcpUser.userId) in verifySecretHostApprovalToken;
fix by making the token creation use the authenticated user's emailBasedUserId
instead of caller-supplied input.props.userId, or alternatively update the
handler to verify against the original session user id stored on the request;
locate the token creation in fetch-gateway.ts (where input.props.userId! is set)
and change it to the authenticated user's userId (emailBasedUserId), or change
the compare in account-secrets handler to use the session's original userId
consistently with verifySecretHostApprovalToken.
---
Nitpick comments:
In `@packages/worker/src/app/handlers/account-secrets.ts`:
- Around line 127-134: The call to setSecretAllowedHosts can append
approval.requestedHost to secret.allowedHosts and create duplicates; before
calling setSecretAllowedHosts (in the block using setSecretAllowedHosts,
secret.allowedHosts, and approval.requestedHost) deduplicate the resulting
allowedHosts array (e.g., construct the new list by combining
secret.allowedHosts and approval.requestedHost then removing duplicates via a
Set or Array.filter) and pass that deduplicated array as allowedHosts to
setSecretAllowedHosts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eec19851-02f8-4d06-ac08-df890886f95e
📒 Files selected for processing (2)
packages/worker/client/routes/account.tsxpackages/worker/src/app/handlers/account-secrets.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/worker/src/mcp/fetch-gateway.ts (1)
159-166: Consider preserving binary bodies that don't contain placeholders.Reading the body as
text()and writing it back could corrupt binary data (images, protobuf, etc.) even if no placeholders are present. For text-based APIs this is fine, but if codemodes ever need to proxy binary content, the encoding could be corrupted.💡 Potential improvement to preserve binary bodies
async function readRequestBody(request: Request) { if (!shouldSendBody(request.method)) return null - return request.text() + const contentType = request.headers.get('content-type') ?? '' + const isLikelyText = + contentType.includes('text/') || + contentType.includes('application/json') || + contentType.includes('application/x-www-form-urlencoded') + if (!isLikelyText) { + // Binary content - return as ArrayBuffer to preserve encoding + return request.arrayBuffer() + } + return request.text() }This would require adjusting
replaceSecretPlaceholdersandnextBodyhandling to differentiate between text and binary bodies. If this isn't a current use case, deferring is reasonable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/fetch-gateway.ts` around lines 159 - 166, The current implementation always calls request.text(), which can corrupt binary payloads; change readRequestBody to read the raw body as an ArrayBuffer (or Uint8Array) when shouldSendBody(method) is true, then detect whether the body is text (e.g., check request.headers.get('content-type') for text/*, application/json, or use TextDecoder to attempt decoding) and only convert to string when you need to run replaceSecretPlaceholders; if no placeholders are found, return the original binary buffer so nextBody and downstream logic preserve binary data; update replaceSecretPlaceholders (and any callers of nextBody) to accept and handle both string and binary (ArrayBuffer/Uint8Array) inputs 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/fetch-gateway.ts`:
- Around line 98-103: The new Request created in the fetch gateway drops
important original request properties (signal, credentials, mode, cache,
integrity, keepalive) which breaks abort and other behaviors; update the Request
constructor (the new Request(nextUrl, { ... }) call) to copy these properties
from the original request (e.g., signal: input.request.signal, credentials:
input.request.credentials, mode: input.request.mode, cache: input.request.cache,
integrity: input.request.integrity, keepalive: input.request.keepalive) so the
replacement request preserves cancellation and other semantics.
---
Nitpick comments:
In `@packages/worker/src/mcp/fetch-gateway.ts`:
- Around line 159-166: The current implementation always calls request.text(),
which can corrupt binary payloads; change readRequestBody to read the raw body
as an ArrayBuffer (or Uint8Array) when shouldSendBody(method) is true, then
detect whether the body is text (e.g., check request.headers.get('content-type')
for text/*, application/json, or use TextDecoder to attempt decoding) and only
convert to string when you need to run replaceSecretPlaceholders; if no
placeholders are found, return the original binary buffer so nextBody and
downstream logic preserve binary data; update replaceSecretPlaceholders (and any
callers of nextBody) to accept and handle both string and binary
(ArrayBuffer/Uint8Array) inputs accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5ae88a0-1e70-4ace-a065-25a951ee1b82
📒 Files selected for processing (2)
packages/worker/src/mcp/fetch-gateway.tspackages/worker/src/mcp/generated-ui-api.ts
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Test expects fetch success with two unapproved secrets
- Updated the approved fetch test to only include the host-approved secret placeholder so it no longer expects success with unapproved secrets.
Preview (98b268387a)
diff --git a/packages/worker/client/routes/account.tsx b/packages/worker/client/routes/account.tsx
--- a/packages/worker/client/routes/account.tsx
+++ b/packages/worker/client/routes/account.tsx
@@ -1,56 +1,166 @@
import { type Handle } from 'remix/component'
-import { colors, spacing, typography } from '#client/styles/tokens.ts'
+import { navigate } from '#client/client-router.tsx'
+import { colors, mq, spacing, typography } from '#client/styles/tokens.ts'
-type AccountStatus = 'idle' | 'loading' | 'ready' | 'error'
+type AccountStatus = 'loading' | 'ready' | 'error'
+type ApprovalAction = 'approve' | 'reject'
+type SecretScope = 'session' | 'app' | 'user'
+type SecretListItem = {
+ name: string
+ scope: SecretScope
+ description: string
+ appId: string | null
+ appTitle: string | null
+ allowedHosts: Array<string>
+ createdAt: string
+ updatedAt: string
+ ttlMs: number | null
+}
+
+type ApprovalView = {
+ token: string
+ name: string
+ scope: SecretScope
+ requestedHost: string
+ currentAllowedHosts: Array<string>
+}
+
+type AccountSecretsPayload = {
+ ok: true
+ email: string
+ secrets: Array<SecretListItem>
+ approval: ApprovalView | null
+}
+
+const accountSecretsApiPath = '/account/secrets.json'
+
+function getScopeLabel(scope: SecretScope) {
+ if (scope === 'app') return 'App'
+ if (scope === 'session') return 'Session'
+ return 'User'
+}
+
+function formatRelativeTtl(ttlMs: number | null) {
+ if (ttlMs == null) return 'No expiry'
+ const totalMinutes = Math.max(1, Math.round(ttlMs / 60_000))
+ if (totalMinutes < 60) return `Expires in ${totalMinutes} min`
+ const totalHours = Math.round(totalMinutes / 60)
+ if (totalHours < 48) return `Expires in ${totalHours} hr`
+ const totalDays = Math.round(totalHours / 24)
+ return `Expires in ${totalDays} day${totalDays === 1 ? '' : 's'}`
+}
+
+async function readJson<T>(response: Response) {
+ return (await response.json().catch(() => null)) as T | null
+}
+
export function AccountRoute(handle: Handle) {
let status: AccountStatus = 'loading'
let email = ''
+ let secrets: Array<SecretListItem> = []
+ let approval: ApprovalView | null = null
let message: string | null = null
+ let submittingApprovalAction: ApprovalAction | null = null
+ let lastLoadedHref = ''
- async function loadAccount(signal: AbortSignal) {
+ async function loadAccountSecrets(signal: AbortSignal) {
try {
- const response = await fetch('/session', {
- headers: { Accept: 'application/json' },
- credentials: 'include',
- signal,
- })
+ const href =
+ typeof window === 'undefined' ? '/account' : window.location.href
+ lastLoadedHref = href
+ const response = await fetch(
+ `${accountSecretsApiPath}${new URL(href).search}`,
+ {
+ headers: { Accept: 'application/json' },
+ credentials: 'include',
+ signal,
+ },
+ )
if (signal.aborted) return
- const payload = await response.json().catch(() => null)
- const sessionEmail =
- response.ok &&
- payload?.ok &&
- typeof payload?.session?.email === 'string'
- ? payload.session.email.trim()
- : ''
- if (!sessionEmail) {
+ if (response.status === 401) {
window.location.assign('/login')
return
}
- email = sessionEmail
+ const payload = await readJson<AccountSecretsPayload>(response)
+ if (!response.ok || !payload?.ok) {
+ throw new Error('Unable to load your account secrets.')
+ }
+ email = payload.email
+ secrets = payload.secrets
+ approval = payload.approval
status = 'ready'
message = null
+ submittingApprovalAction = null
handle.update()
- } catch {
+ } catch (error) {
if (signal.aborted) return
status = 'error'
- message = 'Unable to load your account.'
+ message =
+ error instanceof Error ? error.message : 'Unable to load your account.'
handle.update()
}
}
+ async function submitApproval(action: ApprovalAction) {
+ if (!approval || submittingApprovalAction != null) return
+ submittingApprovalAction = action
+ message = null
+ handle.update()
+ try {
+ const response = await fetch(accountSecretsApiPath, {
+ method: 'POST',
+ headers: {
+ Accept: 'application/json',
+ 'Content-Type': 'application/json',
+ },
+ credentials: 'include',
+ body: JSON.stringify({
+ action,
+ requestToken: approval.token,
+ }),
+ })
+ if (response.status === 401) {
+ window.location.assign('/login')
+ return
+ }
+ const payload = await readJson<
+ AccountSecretsPayload & { error?: string; ok?: boolean }
+ >(response)
+ if (!response.ok || !payload?.ok) {
+ throw new Error(payload?.error || 'Unable to process approval.')
+ }
+ email = payload.email
+ secrets = payload.secrets
+ approval = payload.approval
+ submittingApprovalAction = null
+ message =
+ action === 'approve'
+ ? 'Approved requested host.'
+ : 'Rejected host approval request.'
+ navigate('/account')
+ } catch (error) {
+ submittingApprovalAction = null
+ message =
+ error instanceof Error ? error.message : 'Unable to process approval.'
+ handle.update()
+ }
+ }
+
return () => {
- if (status === 'loading') {
- handle.queueTask(loadAccount)
+ const currentHref =
+ typeof window === 'undefined' ? '/account' : window.location.href
+ if (status === 'loading' || currentHref !== lastLoadedHref) {
+ handle.queueTask(loadAccountSecrets)
}
return (
<section
css={{
- maxWidth: '28rem',
+ maxWidth: '64rem',
margin: '0 auto',
display: 'grid',
- gap: spacing.lg,
+ gap: spacing.xl,
}}
>
<header css={{ display: 'grid', gap: spacing.xs }}>
@@ -62,19 +172,201 @@
margin: 0,
}}
>
- {email ? `Welcome, ${email}` : 'Welcome'}
+ {email ? `${email} secret approvals` : 'Secret approvals'}
</h1>
- <p css={{ color: colors.textMuted }}>You are signed in to kody.</p>
+ <p css={{ color: colors.textMuted, margin: 0 }}>
+ Manage which hosts may receive stored secrets.
+ </p>
</header>
+
+ {approval ? (
+ <section
+ css={{
+ display: 'grid',
+ gap: spacing.md,
+ padding: spacing.lg,
+ borderRadius: '1rem',
+ border: `1px solid ${colors.primary}`,
+ backgroundColor: colors.primarySoftest,
+ }}
+ >
+ <div css={{ display: 'grid', gap: spacing.xs }}>
+ <h2
+ css={{
+ margin: 0,
+ fontSize: typography.fontSize.lg,
+ fontWeight: typography.fontWeight.semibold,
+ color: colors.text,
+ }}
+ >
+ Approve host access
+ </h2>
+ <p css={{ margin: 0, color: colors.textMuted }}>
+ Allow <code>{approval.requestedHost}</code> to receive secret{' '}
+ <code>{approval.name}</code> from the {approval.scope} scope.
+ </p>
+ <p css={{ margin: 0, color: colors.textMuted }}>
+ Current allowed hosts:{' '}
+ {approval.currentAllowedHosts.length > 0
+ ? approval.currentAllowedHosts.join(', ')
+ : 'none'}
+ </p>
+ </div>
+ <div css={{ display: 'flex', gap: spacing.sm, flexWrap: 'wrap' }}>
+ <button
+ type="button"
+ disabled={submittingApprovalAction != null}
+ onClick={() => submitApproval('approve')}
+ css={primaryButtonCss}
+ >
+ Approve host
+ </button>
+ <button
+ type="button"
+ disabled={submittingApprovalAction != null}
+ onClick={() => submitApproval('reject')}
+ css={secondaryButtonCss}
+ >
+ Reject
+ </button>
+ </div>
+ </section>
+ ) : null}
+
{status === 'loading' ? (
- <p css={{ color: colors.textMuted }}>Loading your account…</p>
+ <p css={{ color: colors.textMuted, margin: 0 }}>
+ Loading secret approvals…
+ </p>
) : null}
{message ? (
- <p css={{ color: colors.error }} role="alert">
+ <p
+ css={{ color: status === 'error' ? colors.error : colors.text }}
+ role="alert"
+ >
{message}
</p>
) : null}
+
+ {status === 'ready' ? (
+ <section css={{ display: 'grid', gap: spacing.md }}>
+ <h2
+ css={{
+ margin: 0,
+ fontSize: typography.fontSize.lg,
+ fontWeight: typography.fontWeight.semibold,
+ color: colors.text,
+ }}
+ >
+ Saved secrets
+ </h2>
+ {secrets.length === 0 ? (
+ <p css={{ margin: 0, color: colors.textMuted }}>
+ No user or app secrets are currently stored.
+ </p>
+ ) : (
+ <ul
+ css={{
+ listStyle: 'none',
+ padding: 0,
+ margin: 0,
+ display: 'grid',
+ gap: spacing.md,
+ }}
+ >
+ {secrets.map((secret) => (
+ <li
+ key={`${secret.scope}:${secret.appId ?? 'global'}:${secret.name}`}
+ css={{
+ display: 'grid',
+ gap: spacing.sm,
+ padding: spacing.lg,
+ border: `1px solid ${colors.border}`,
+ borderRadius: '1rem',
+ backgroundColor: colors.surface,
+ }}
+ >
+ <div
+ css={{
+ display: 'flex',
+ alignItems: 'baseline',
+ justifyContent: 'space-between',
+ gap: spacing.sm,
+ flexWrap: 'wrap',
+ }}
+ >
+ <div css={{ display: 'grid', gap: spacing.xs }}>
+ <strong css={{ color: colors.text }}>{secret.name}</strong>
+ <span css={{ color: colors.textMuted }}>
+ {getScopeLabel(secret.scope)}
+ {secret.appTitle ? ` - ${secret.appTitle}` : ''}
+ </span>
+ </div>
+ <span css={{ color: colors.textMuted }}>
+ {formatRelativeTtl(secret.ttlMs)}
+ </span>
+ </div>
+ {secret.description ? (
+ <p css={{ margin: 0, color: colors.textMuted }}>
+ {secret.description}
+ </p>
+ ) : null}
+ <div css={{ display: 'grid', gap: spacing.xs }}>
+ <span css={{ color: colors.textMuted }}>Allowed hosts</span>
+ {secret.allowedHosts.length > 0 ? (
+ <div
+ css={{
+ display: 'flex',
+ flexWrap: 'wrap',
+ gap: spacing.xs,
+ }}
+ >
+ {secret.allowedHosts.map((host) => (
+ <code
+ key={host}
+ css={{
+ padding: `${spacing.xs} ${spacing.sm}`,
+ borderRadius: '999px',
+ backgroundColor: colors.primarySoftSubtle,
+ color: colors.text,
+ }}
+ >
+ {host}
+ </code>
+ ))}
+ </div>
+ ) : (
+ <p css={{ margin: 0, color: colors.textMuted }}>
+ No hosts approved yet.
+ </p>
+ )}
+ </div>
+ </li>
+ ))}
+ </ul>
+ )}
+ </section>
+ ) : null}
</section>
)
}
}
+
+const primaryButtonCss = {
+ padding: `${spacing.sm} ${spacing.md}`,
+ borderRadius: '999px',
+ border: 'none',
+ backgroundColor: colors.primary,
+ color: 'white',
+ fontWeight: typography.fontWeight.medium,
+ cursor: 'pointer',
+ [mq.mobile]: {
+ width: '100%',
+ },
+}
+
+const secondaryButtonCss = {
+ ...primaryButtonCss,
+ backgroundColor: 'transparent',
+ color: colors.text,
+ border: `1px solid ${colors.border}`,
+}
diff --git a/packages/worker/client/routes/index.tsx b/packages/worker/client/routes/index.tsx
--- a/packages/worker/client/routes/index.tsx
+++ b/packages/worker/client/routes/index.tsx
@@ -13,6 +13,7 @@
'/chat/:threadId': <ChatRoute />,
'/ui/:id': <SavedUiRoute />,
'/account': <AccountRoute />,
+ '/account/secrets/approve': <AccountRoute />,
'/login': <LoginRoute />,
'/signup': <LoginRoute />,
'/reset-password': <ResetPasswordRoute />,
diff --git a/packages/worker/migrations/0009-secret-allowed-hosts.sql b/packages/worker/migrations/0009-secret-allowed-hosts.sql
new file mode 100644
--- /dev/null
+++ b/packages/worker/migrations/0009-secret-allowed-hosts.sql
@@ -1,0 +1,2 @@
+ALTER TABLE secret_entries
+ADD COLUMN allowed_hosts TEXT NOT NULL DEFAULT '[]';
diff --git a/packages/worker/src/app/authenticated-user.ts b/packages/worker/src/app/authenticated-user.ts
--- a/packages/worker/src/app/authenticated-user.ts
+++ b/packages/worker/src/app/authenticated-user.ts
@@ -39,7 +39,7 @@
new Set([session.id, emailBasedUserId].filter(Boolean)),
),
mcpUser: {
- userId: session.id,
+ userId: emailBasedUserId,
email: session.email,
displayName: buildDisplayName(session.email),
},
diff --git a/packages/worker/src/app/handlers/account-secrets.ts b/packages/worker/src/app/handlers/account-secrets.ts
new file mode 100644
--- /dev/null
+++ b/packages/worker/src/app/handlers/account-secrets.ts
@@ -1,0 +1,304 @@
+import { type BuildAction } from 'remix/fetch-router'
+import { readAuthSessionResult } from '#app/auth-session.ts'
+import { readAuthenticatedAppUser } from '#app/authenticated-user.ts'
+import { redirectToLogin } from '#app/auth-redirect.ts'
+import { Layout } from '#app/layout.ts'
+import { render } from '#app/render.ts'
+import { verifySecretHostApprovalToken } from '#mcp/secrets/host-approval.ts'
+import {
+ listAppSecretsByAppIds,
+ listSecrets,
+ setSecretAllowedHosts,
+} from '#mcp/secrets/service.ts'
+import { type SecretScope } from '#mcp/secrets/types.ts'
+import { listUiArtifactsByUserId } from '#mcp/ui-artifacts-repo.ts'
+import { type routes } from '#app/routes.ts'
+
+type AccountSecretListItem = {
+ name: string
+ scope: SecretScope
+ description: string
+ appId: string | null
+ appTitle: string | null
+ allowedHosts: Array<string>
+ createdAt: string
+ updatedAt: string
+ ttlMs: number | null
+}
+
+type SecretApprovalView = {
+ token: string
+ name: string
+ scope: SecretScope
+ requestedHost: string
+ currentAllowedHosts: Array<string>
+}
+
+type AccountSecretsPayload = {
+ ok: true
+ email: string
+ secrets: Array<AccountSecretListItem>
+ approval: SecretApprovalView | null
+}
+
+type SecretApprovalAction = 'approve' | 'reject'
+
+export function createAccountSecretsHandler(_env: Env) {
+ return {
+ middleware: [],
+ async action({ request }) {
+ const { session, setCookie } = await readAuthSessionResult(request)
+ if (!session) {
+ return redirectToLogin(request)
+ }
+
+ const response = render(Layout({ title: 'Account' }))
+ if (setCookie) {
+ response.headers.set('Set-Cookie', setCookie)
+ }
+ return response
+ },
+ } satisfies BuildAction<
+ typeof routes.accountSecrets.method,
+ typeof routes.accountSecrets.pattern
+ >
+}
+
+export function createAccountSecretsApiHandler(env: Env) {
+ return {
+ middleware: [],
+ async action({ request }) {
+ const user = await readAuthenticatedAppUser(request, env)
+ if (!user) {
+ return jsonResponse({ ok: false, error: 'Unauthorized.' }, 401)
+ }
+
+ if (request.method === 'GET') {
+ const payload = await buildAccountSecretsPayload({
+ request,
+ env,
+ user,
+ })
+ return jsonResponse(payload)
+ }
+
+ if (request.method !== 'POST') {
+ return jsonResponse({ ok: false, error: 'Method not allowed.' }, 405)
+ }
+
+ const body = await request.json().catch(() => null)
+ if (!body || typeof body !== 'object') {
+ return jsonResponse({ ok: false, error: 'Invalid request body.' }, 400)
+ }
+
+ const action = readApprovalAction(body)
+ if (!action) {
+ return jsonResponse({ ok: false, error: 'Invalid approval action.' }, 400)
+ }
+
+ const token = readString(body, 'requestToken')
+ if (!token) {
+ return jsonResponse(
+ { ok: false, error: 'Approval request token is required.' },
+ 400,
+ )
+ }
+
+ try {
+ const approval = await verifySecretHostApprovalToken(env, token)
+ if (approval.userId !== user.mcpUser.userId) {
+ return jsonResponse({ ok: false, error: 'Approval request mismatch.' }, 403)
+ }
+
+ if (action === 'approve') {
+ const current = await listSecrets({
+ env,
+ userId: user.mcpUser.userId,
+ scope: approval.scope,
+ secretContext: approval.secretContext,
+ })
+ const secret = current.find(
+ (item) =>
+ item.name === approval.name && item.scope === approval.scope,
+ )
+ if (!secret) {
+ return jsonResponse({ ok: false, error: 'Secret not found.' }, 404)
+ }
+ await setSecretAllowedHosts({
+ env,
+ userId: user.mcpUser.userId,
+ name: approval.name,
+ scope: approval.scope,
+ allowedHosts: [...secret.allowedHosts, approval.requestedHost],
+ secretContext: approval.secretContext,
+ })
+ }
+
+ const payload = await buildAccountSecretsPayload({
+ request,
+ env,
+ user,
+ })
+ return jsonResponse(payload)
+ } catch (error) {
+ return jsonResponse(
+ {
+ ok: false,
+ error:
+ error instanceof Error
+ ? error.message
+ : 'Unable to process approval request.',
+ },
+ 400,
+ )
+ }
+ },
+ } satisfies BuildAction<
+ typeof routes.accountSecretsApi.method,
+ typeof routes.accountSecretsApi.pattern
+ >
+}
+
+async function buildAccountSecretsPayload(input: {
+ request: Request
+ env: Env
+ user: NonNullable<Awaited<ReturnType<typeof readAuthenticatedAppUser>>>
+}): Promise<AccountSecretsPayload> {
+ const url = new URL(input.request.url)
+ const approvalToken = url.searchParams.get('request')
+
+ const savedApps = await listSavedAppsForUser({
+ env: input.env,
+ user: input.user,
+ })
+ const appTitles = new Map(savedApps.map((app) => [app.id, app.title]))
+ const [userSecrets, appSecrets] = await Promise.all([
+ listSecrets({
+ env: input.env,
+ userId: input.user.mcpUser.userId,
+ scope: 'user',
+ }),
+ listAppSecretsByAppIds({
+ env: input.env,
+ userId: input.user.mcpUser.userId,
+ appIds: savedApps.map((app) => app.id),
+ }),
+ ])
+
+ const secrets = [
+ ...userSecrets.map((secret) => toAccountSecretListItem(secret, appTitles)),
+ ...Array.from(appSecrets.values())
+ .flat()
+ .map((secret) => toAccountSecretListItem(secret, appTitles)),
+ ].sort((left, right) => {
+ return left.name.localeCompare(right.name) || left.scope.localeCompare(right.scope)
+ })
+
+ const approval = approvalToken
+ ? await resolveSecretApprovalView({
+ env: input.env,
+ userId: input.user.mcpUser.userId,
+ token: approvalToken,
+ }).catch(() => null)
+ : null
+
+ return {
+ ok: true,
+ email: input.user.email,
+ secrets,
+ approval,
+ }
+}
+
+async function listSavedAppsForUser(input: {
+ env: Env
+ user: NonNullable<Awaited<ReturnType<typeof readAuthenticatedAppUser>>>
+}) {
+ const apps = await Promise.all(
+ input.user.artifactOwnerIds.map((ownerId) =>
+ listUiArtifactsByUserId(input.env.APP_DB, ownerId),
+ ),
+ )
+ const dedupedIds = new Set<string>()
+ return apps.flat().filter((app) => {
+ if (dedupedIds.has(app.id)) return false
+ dedupedIds.add(app.id)
+ return true
+ })
+}
+
+async function resolveSecretApprovalView(input: {
+ env: Env
+ userId: string
+ token: string
+}) {
+ const approval = await verifySecretHostApprovalToken(input.env, input.token)
+ if (approval.userId !== input.userId) {
+ throw new Error('Approval request mismatch.')
+ }
+ const secrets = await listSecrets({
+ env: input.env,
+ userId: input.userId,
+ scope: approval.scope,
+ secretContext: approval.secretContext,
+ })
+ const secret = secrets.find(
+ (item) => item.name === approval.name && item.scope === approval.scope,
+ )
+ if (!secret) {
+ throw new Error('Secret not found.')
+ }
+ return {
+ token: input.token,
+ name: approval.name,
+ scope: approval.scope,
+ requestedHost: approval.requestedHost,
+ currentAllowedHosts: secret.allowedHosts,
+ } satisfies SecretApprovalView
+}
+
+function toAccountSecretListItem(
+ secret: {
+ name: string
+ scope: SecretScope
+ description: string
+ appId: string | null
+ allowedHosts: Array<string>
+ createdAt: string
+ updatedAt: string
+ ttlMs: number | null
+ },
+ appTitles: Map<string, string>,
+) {
+ return {
+ name: secret.name,
+ scope: secret.scope,
+ description: secret.description,
+ appId: secret.appId,
+ appTitle: secret.appId ? (appTitles.get(secret.appId) ?? null) : null,
+ allowedHosts: secret.allowedHosts,
+ createdAt: secret.createdAt,
+ updatedAt: secret.updatedAt,
+ ttlMs: secret.ttlMs,
+ } satisfies AccountSecretListItem
+}
+
+function readString(body: object, key: string) {
+ const value = (body as Record<string, unknown>)[key]
+ return typeof value === 'string' && value.trim() ? value.trim() : null
+}
+
+function readApprovalAction(body: object): SecretApprovalAction | null {
+ const raw = readString(body, 'action')
+ return raw === 'approve' || raw === 'reject' ? raw : null
+}
+
+function jsonResponse(body: Record<string, unknown>, status = 200) {
+ return new Response(JSON.stringify(body), {
+ status,
+ headers: {
+ 'Cache-Control': 'no-store',
+ 'Content-Type': 'application/json; charset=utf-8',
+ },
+ })
+}
diff --git a/packages/worker/src/app/handlers/account.ts b/packages/worker/src/app/handlers/account.ts
--- a/packages/worker/src/app/handlers/account.ts
+++ b/packages/worker/src/app/handlers/account.ts
@@ -14,11 +14,7 @@
return redirectToLogin(request)
}
- const response = render(
- Layout({
- title: 'Account',
- }),
- )
+ const response = render(Layout({ title: 'Account' }))
if (setCookie) {
response.headers.set('Set-Cookie', setCookie)
}
diff --git a/packages/worker/src/app/router.ts b/packages/worker/src/app/router.ts
--- a/packages/worker/src/app/router.ts
+++ b/packages/worker/src/app/router.ts
@@ -1,5 +1,9 @@
import { createRouter } from 'remix/fetch-router'
import { account } from '#app/handlers/account.ts'
+import {
+ createAccountSecretsApiHandler,
+ createAccountSecretsHandler,
+} from '#app/handlers/account-secrets.ts'
import { createAuthHandler } from '#app/handlers/auth.ts'
import { chat } from '#app/handlers/chat.ts'
import {
@@ -43,6 +47,9 @@
router.map(routes.login, login)
router.map(routes.signup, signup)
router.map(routes.account, account)
+ router.map(routes.accountSecrets, createAccountSecretsHandler(appEnv as Env))
+ router.map(routes.accountSecretsApprove, createAccountSecretsHandler(appEnv as Env))
+ router.map(routes.accountSecretsApi, createAccountSecretsApiHandler(appEnv as Env))
router.map(routes.savedUi, createSavedUiPageHandler(appEnv as Env))
router.map(routes.auth, createAuthHandler(appEnv))
router.map(routes.session, session)
diff --git a/packages/worker/src/app/routes.ts b/packages/worker/src/app/routes.ts
--- a/packages/worker/src/app/routes.ts
+++ b/packages/worker/src/app/routes.ts
@@ -5,6 +5,9 @@
chat: '/chat',
chatThread: '/chat/:threadId',
savedUi: '/ui/:id',
+ accountSecrets: '/account/secrets',
+ accountSecretsApprove: '/account/secrets/approve',
+ accountSecretsApi: '/account/secrets.json',
chatThreads: '/chat-threads',
chatThreadsCreate: post('/chat-threads'),
chatThreadsUpdate: post('/chat-threads/update'),
diff --git a/packages/worker/src/index.ts b/packages/worker/src/index.ts
--- a/packages/worker/src/index.ts
... diff truncated: showing 800 of 2200 linesCo-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Owner
Author
|
@cursoragent please address all valid feedback on this pull request. |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 1
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/mcp-server-e2e.test.ts (1)
1216-1254:⚠️ Potential issue | 🔴 CriticalPipeline failure: The approved fetch uses unapproved secrets.
The test expects
approvedFetchExecuteResponse.okto betrue, but the request at lines 1225-1232 includes three secrets:
{{secret:cloudflareToken|scope=app}}— approved forapi.example.com✓{{secret:globalApiKey|scope=user}}— not approved ✗{{secret:ephemeralCode|scope=session}}— not approved ✗The fetch gateway will block the request because
globalApiKeyandephemeralCodehaven't been approved forapi.example.com.🐛 Proposed fix: Only use the approved secret in the verification fetch
Either approve all three secrets before the verification fetch, or simplify the test to only use
cloudflareToken:body: JSON.stringify({ code: `async () => { const response = await fetch('https://api.example.com/deploy', { method: 'POST', headers: { Authorization: 'Bearer {{secret:cloudflareToken|scope=app}}', - 'X-Global-Key': '{{secret:globalApiKey|scope=user}}', - 'X-Session-Code': '{{secret:ephemeralCode|scope=session}}', }, body: JSON.stringify({ note: 'deploy' }), }) return { ok: response.ok, status: response.status, } }`, }),Alternatively, if you want to test multiple secret scopes in a single fetch, add approval flows for
globalApiKeyandephemeralCodebefore the approved fetch execution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/mcp-server-e2e.test.ts` around lines 1216 - 1254, The test's approved fetch payload uses unapproved secrets (globalApiKey and ephemeralCode) which causes the gateway to block the request; update the test around the POST to executeUrl (the approvedFetchExecuteResponse/approvedFetchExecutePayload block) so it only uses the approved secret {{secret:cloudflareToken|scope=app}} or, alternatively, add the prior approval flows for globalApiKey (scope=user) and ephemeralCode (scope=session) before sending the approved fetch; locate the fetch call that builds the body (the code string passed in the fetch to executeUrl) and either remove the two unapproved secret headers or insert calls to the approval endpoints/fixtures that mark those secrets approved for api.example.com so the assertion expecting approvedFetchExecuteResponse.ok === true remains valid.
♻️ Duplicate comments (1)
packages/worker/client/routes/account.tsx (1)
219-230:⚠️ Potential issue | 🔴 CriticalThis button handler change still doesn’t typecheck.
The build is still red at Lines 222 and 230:
void submitApproval(...)fixes the Promise return, but these<button>props are still not assignable to this runtime’sButtonHTMLProps<HTMLButtonElement>. Please switch to the click-handler API/casing that this JSX runtime actually accepts before merging.Verify against the runtime’s supported DOM event props. Expected result: either an existing repo pattern or the framework docs should show the accepted prop name/signature for button click handlers.
In the JSX/runtime used by `remix/component`, what prop name and TypeScript signature should a `<button>` use for click handlers? Does it support `onClick`, or should this repo use a different event prop/casing?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/client/routes/account.tsx` around lines 219 - 230, The JSX runtime used here expects DOM event props in the lowercase HTML form and a handler with an explicit event parameter, so update the two button handlers to use the runtime-supported prop name (replace onClick with onclick) and pass a function that accepts the event and returns void; e.g. change onClick={() => void submitApproval('approve')} to onclick={(e: MouseEvent) => { e.preventDefault?.(); void submitApproval('approve'); }} (same for the 'reject' button), keeping existing symbols submitApproval and primaryButtonCss.
🤖 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/routes/account.tsx`:
- Around line 154-157: The approval card/buttons can remain actionable when a
URL-driven reload is queued; modify the account component to hide or clear stale
approvals whenever the location has changed or a reload is pending by checking
currentHref !== lastLoadedHref or status === 'loading' before rendering approval
UI. Concretely, in the logic around currentHref, lastLoadedHref, status and the
call to handle.queueTask(loadAccountSecrets) ensure you either set the approval
state to null/undefined (clearApproval) before queuing the reload or add a
render-gate (e.g., only render approval card when currentHref === lastLoadedHref
&& status !== 'loading' && approval != null). Apply the same guard to the
approval-related rendering and action handlers referenced later in the file (the
approval card/buttons region around the 185-230 block) so stale approve/reject
buttons are never shown or clickable for a different URL.
---
Outside diff comments:
In `@packages/worker/src/mcp/mcp-server-e2e.test.ts`:
- Around line 1216-1254: The test's approved fetch payload uses unapproved
secrets (globalApiKey and ephemeralCode) which causes the gateway to block the
request; update the test around the POST to executeUrl (the
approvedFetchExecuteResponse/approvedFetchExecutePayload block) so it only uses
the approved secret {{secret:cloudflareToken|scope=app}} or, alternatively, add
the prior approval flows for globalApiKey (scope=user) and ephemeralCode
(scope=session) before sending the approved fetch; locate the fetch call that
builds the body (the code string passed in the fetch to executeUrl) and either
remove the two unapproved secret headers or insert calls to the approval
endpoints/fixtures that mark those secrets approved for api.example.com so the
assertion expecting approvedFetchExecuteResponse.ok === true remains valid.
---
Duplicate comments:
In `@packages/worker/client/routes/account.tsx`:
- Around line 219-230: The JSX runtime used here expects DOM event props in the
lowercase HTML form and a handler with an explicit event parameter, so update
the two button handlers to use the runtime-supported prop name (replace onClick
with onclick) and pass a function that accepts the event and returns void; e.g.
change onClick={() => void submitApproval('approve')} to onclick={(e:
MouseEvent) => { e.preventDefault?.(); void submitApproval('approve'); }} (same
for the 'reject' button), keeping existing symbols submitApproval and
primaryButtonCss.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79726c95-de2d-405a-a455-dbce8189bbb4
📒 Files selected for processing (4)
packages/worker/client/routes/account.tsxpackages/worker/src/mcp/executor.tspackages/worker/src/mcp/fetch-gateway.tspackages/worker/src/mcp/mcp-server-e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/mcp/executor.ts
Owner
Author
|
@cursoragent please address any remaining valid feedback on this PR. Also please check CI on this PR and get it fixed |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
| 'Content-Type': 'application/json; charset=utf-8', | ||
| }, | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Duplicated jsonResponse helper across two handler files
Low Severity
The jsonResponse helper function in account-secrets.ts is an identical copy of the one already in generated-ui-api.ts. Both construct a Response with JSON.stringify, no-store cache control, and application/json content type. Extracting this into a shared utility would avoid maintaining duplicated logic.
Additional Locations (1)
Owner
Author
|
@cursoragent please address any remaining valid feedback on this PR. Also please check CI on this PR and get it fixed. Again |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
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/mcp-server-e2e.test.ts (1)
1216-1252:⚠️ Potential issue | 🟠 MajorApproval context mismatch: verify that secretContext.appId is validated during approval.
The test correctly approves the host via
/account/secrets.jsonand verifies the response containsallowedHosts: ['api.example.com']. However, the subsequent fetch execution may fail if the secretContext used during approval doesn't match the secretContext used during execution.The code properly threads secretContext (containing appId and sessionId) through the approval and execution paths. However, there's a potential gap: in account-secrets.ts (lines 113-135), when approving a host,
listSecretsis called withapproval.secretContext, and the secret is located by name and scope only—without validating that the secret's appId (stored asbinding_keyin the database) matches the approval'ssecretContext.appId. This could allow an approval from one app session to modify allowedHosts for a secret belonging to a different app.Add validation in account-secrets.ts that the approval context's appId matches the secret's appId before calling setSecretAllowedHosts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/mcp-server-e2e.test.ts` around lines 1216 - 1252, When approving a host in account-secrets.ts (within the approval handling that calls listSecrets and then setSecretAllowedHosts), validate that the secret being modified actually belongs to the approving app by comparing the secret's stored app id (binding_key on the secret returned by listSecrets) against approval.secretContext.appId; if they differ, abort the approval (return an error/throw) instead of calling setSecretAllowedHosts. Update the approval path that currently locates the secret by name/scope only to perform this binding_key === approval.secretContext.appId check before proceeding.
🧹 Nitpick comments (1)
packages/worker/client/routes/account.tsx (1)
365-383: Consider: Button styles could benefit fromas constfor type safety.The CSS objects are well-structured and reusable. For stricter type checking with CSS-in-JS, consider adding
as const.♻️ Optional: Add const assertion
-const primaryButtonCss = { +const primaryButtonCss = { padding: `${spacing.sm} ${spacing.md}`, borderRadius: '999px', border: 'none', backgroundColor: colors.primary, color: 'white', fontWeight: typography.fontWeight.medium, cursor: 'pointer', [mq.mobile]: { width: '100%', }, -} +} as const🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/client/routes/account.tsx` around lines 365 - 383, The primaryButtonCss and secondaryButtonCss objects should use TypeScript const assertions for stricter typing: update the declarations of primaryButtonCss and secondaryButtonCss to include an "as const" const assertion (ensuring primaryButtonCss is asserted before it's spread into secondaryButtonCss so the spread preserves literal types), so both objects are typed with readonly literal values for safer CSS-in-JS usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/worker/src/mcp/mcp-server-e2e.test.ts`:
- Around line 1216-1252: When approving a host in account-secrets.ts (within the
approval handling that calls listSecrets and then setSecretAllowedHosts),
validate that the secret being modified actually belongs to the approving app by
comparing the secret's stored app id (binding_key on the secret returned by
listSecrets) against approval.secretContext.appId; if they differ, abort the
approval (return an error/throw) instead of calling setSecretAllowedHosts.
Update the approval path that currently locates the secret by name/scope only to
perform this binding_key === approval.secretContext.appId check before
proceeding.
---
Nitpick comments:
In `@packages/worker/client/routes/account.tsx`:
- Around line 365-383: The primaryButtonCss and secondaryButtonCss objects
should use TypeScript const assertions for stricter typing: update the
declarations of primaryButtonCss and secondaryButtonCss to include an "as const"
const assertion (ensuring primaryButtonCss is asserted before it's spread into
secondaryButtonCss so the spread preserves literal types), so both objects are
typed with readonly literal values for safer CSS-in-JS usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fceaff5a-8722-4abb-917a-cf9b6483ce37
📒 Files selected for processing (3)
e2e/smoke.spec.tspackages/worker/client/routes/account.tsxpackages/worker/src/mcp/mcp-server-e2e.test.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
|
🔎 Preview deployed: https://kody-pr-61.kentcdodds.workers.dev Worker: Mocks:
|
Owner
Author
|
@cursoragent please address any remaining valid feedback on this PR. Again |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
- Added guidance on handling secret-bearing outbound requests and host approval policies in `adding-capabilities.md`. - Clarified that only the account admin UI can modify allowed outbound hosts and emphasized the separation of secret saving and host approval in `mcp-apps-starter-guide.md`. - Updated descriptions to ensure clear understanding of the approval model for secret metadata and network behavior.
This was referenced Mar 28, 2026
This was referenced Apr 12, 2026
This was referenced Apr 28, 2026
This was referenced May 12, 2026
This was referenced Jul 1, 2026
security: harden web/auth surface (headers, dev-route gate, rate limits, PKCE, password policy)
#582
Merged
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
allowed_hostsmetadata and approval-token plumbing for secret egress policyfetch()through a host-side gateway that expands{{secret:name|scope=...}}placeholders and blocks unapproved destinations with actionable approval linksTesting
npm run typechecknpm run test -- packages/worker/src/app/handlers/session-handler.test.ts packages/worker/src/app/handlers/health-handler.test.ts packages/worker/src/mcp/capabilities/unified-search.test.tsnpm run test:mcp -- --testNamePattern "generated ui sessions support secret storage, execute-time resolution, and scoped search visibility"(still has one approval-path assertion gap in the MCP/app identity bridge)/accountsecrets page in local devWalkthrough
Account secrets page
App homepage
Summary by CodeRabbit
New Features
Database
Documentation
Tests