Repository navigation
Remove secret approval request tokens - #278
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThis PR removes encrypted token-based host and package approval flows, deleting token generation/verification and replacing them with deterministic approval derivation from URL query parameters (e.g., Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser as Client Browser
participant Handler as Account Handler
participant Storage as Secret Storage
User->>Browser: Click approval link (e.g. ?allowed-host=example.com & selected=<id>)
Browser->>Handler: GET /account?allowed-host=example.com&selected=<id>
Handler->>Storage: Read secret + query params -> build ApprovalView
Handler-->>Browser: Render approval view (no token)
User->>Browser: Click "Approve"
Browser->>Handler: POST /account/approve?action=approve (URL carries query params)
Handler->>Storage: Derive approval from query params + selected secret
Handler->>Storage: Persist allowed host / package id
Handler-->>Browser: Success response
Browser->>Browser: Update history / clear query params
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
kentcdodds
marked this pull request as ready for review
April 28, 2026 15:30
Contributor
|
🔎 Preview deployed: https://kody-pr-278.kentcdodds.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/routes/account-secrets.tsx (1)
630-638:⚠️ Potential issue | 🔴 CriticalApproval POST drops required URL context (
allowed-host/package_id).
submitApprovalbuildsrequestUrlfromaccountSecretsApiPathbut does not carry over the current search params. After token removal, the server resolves approval context from URL query params, so this causes runtime failures (e.g., “Approval request is missing a host or package.”).🐛 Proposed fix
try { - const selection = getSelectionState(getCurrentHref()) - const requestUrl = new URL(accountSecretsApiPath, getCurrentHref()) + const currentHref = getCurrentHref() + const selection = getSelectionState(currentHref) + const requestUrl = new URL(accountSecretsApiPath, currentHref) + requestUrl.search = new URL(currentHref).search if (selection.selectedSecretId) { requestUrl.searchParams.set('selected', selection.selectedSecretId) } const payload = await submitApprovalRequest< AccountSecretsPayload & { error?: string; ok?: boolean } >(action, requestUrl.toString())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/client/routes/account-secrets.tsx` around lines 630 - 638, The approval POST loses existing query params because requestUrl is constructed from accountSecretsApiPath without copying the current page's search params; update the code around getSelectionState/getCurrentHref/submitApprovalRequest so you first parse the current href (const current = new URL(getCurrentHref())), then merge or assign its search params into requestUrl (the URL created with accountSecretsApiPath) before setting selection.selectedSecretId and calling submitApprovalRequest, ensuring existing keys like allowed-host or package_id are preserved in the request URL.
🤖 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 77-80: The approval POST drops current query params so
host/package context is lost; update the request URL you pass into
submitApprovalRequest (the variable/action used with AccountSecretsPayload and
payload handling) to include the current location's query string (e.g., merge
window.location.search or router location.search into the action URL) before
calling submitApprovalRequest so query-derived approval context is preserved.
In `@packages/worker/src/app/handlers/account-secrets.ts`:
- Around line 736-740: The approval path currently accepts secret IDs with scope
'session'; after calling parseAccountSecretId(input.secretId) (and before
calling getSecretContextForAccountSecret), add the same guard used in
handleDeleteAction to reject session-scoped secrets—i.e., if parsed.scope ===
'session' then throw an error (e.g., 'Invalid approval request.'); ensure you
reference parseAccountSecretId, parsed.scope, and
getSecretContextForAccountSecret when making this check so session secrets are
blocked here too.
- Around line 741-763: The approval handler currently prefers package when both
input.requestedPackageId and input.requestedHost are present, risking wrong
approvals; update the logic in the handler handling input.requestedPackageId /
input.requestedHost to explicitly detect if both are provided and throw an error
(e.g., "Approval request contains both host and package") instead of proceeding,
and keep the existing branches that return the package object (with packageId
and storageContext) or the host object (after normalizeAllowedHosts and
requestedHost validation) untouched otherwise; reference the symbols
input.requestedPackageId, input.requestedHost, normalizeAllowedHosts, and the
returned objects with kind:'package' and kind:'host' to locate and change the
code.
---
Outside diff comments:
In `@packages/worker/client/routes/account-secrets.tsx`:
- Around line 630-638: The approval POST loses existing query params because
requestUrl is constructed from accountSecretsApiPath without copying the current
page's search params; update the code around
getSelectionState/getCurrentHref/submitApprovalRequest so you first parse the
current href (const current = new URL(getCurrentHref())), then merge or assign
its search params into requestUrl (the URL created with accountSecretsApiPath)
before setting selection.selectedSecretId and calling submitApprovalRequest,
ensuring existing keys like allowed-host or package_id are preserved in the
request URL.
🪄 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 Plus
Run ID: 935bc554-03a1-4659-9ea4-ff78943f924f
📒 Files selected for processing (14)
e2e/account-secrets.spec.tspackages/worker/client/routes/account-approval-shared.tspackages/worker/client/routes/account-secrets.tsxpackages/worker/client/routes/account.tsxpackages/worker/src/app/handlers/account-secrets.node.test.tspackages/worker/src/app/handlers/account-secrets.tspackages/worker/src/mcp/executor.node.test.tspackages/worker/src/mcp/fetch-gateway.node.test.tspackages/worker/src/mcp/fetch-gateway.tspackages/worker/src/mcp/secrets/errors.node.test.tspackages/worker/src/mcp/secrets/host-approval.node.test.tspackages/worker/src/mcp/secrets/host-approval.tspackages/worker/src/mcp/secrets/package-approval-url.tspackages/worker/src/mcp/secrets/package-approval.ts
💤 Files with no reviewable changes (4)
- packages/worker/src/mcp/secrets/package-approval-url.ts
- packages/worker/src/mcp/secrets/package-approval.ts
- packages/worker/src/mcp/secrets/host-approval.ts
- packages/worker/src/mcp/secrets/host-approval.node.test.ts
Comment on lines
+736
to
+740
| const parsed = input.secretId ? parseAccountSecretId(input.secretId) : null | ||
| if (!parsed) { | ||
| throw new Error('Invalid approval request.') | ||
| } | ||
| if (token.startsWith(secretPackageApprovalTokenPrefix)) { | ||
| return await verifySecretPackageApprovalToken(env, token) | ||
| const storageContext = getSecretContextForAccountSecret(parsed) |
There was a problem hiding this comment.
Block session secret ids here as well.
parseAccountSecretId can return scope === 'session'—you already guard that in handleDeleteAction—but this path currently accepts it and feeds that scope into the approval mutators. That lets the account approval endpoint operate on a secret class the account page is supposed to reject.
🛡️ Suggested fix
const parsed = input.secretId ? parseAccountSecretId(input.secretId) : null
- if (!parsed) {
+ if (!parsed || parsed.scope === 'session') {
throw new Error('Invalid approval request.')
}📝 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 parsed = input.secretId ? parseAccountSecretId(input.secretId) : null | |
| if (!parsed) { | |
| throw new Error('Invalid approval request.') | |
| } | |
| if (token.startsWith(secretPackageApprovalTokenPrefix)) { | |
| return await verifySecretPackageApprovalToken(env, token) | |
| const storageContext = getSecretContextForAccountSecret(parsed) | |
| const parsed = input.secretId ? parseAccountSecretId(input.secretId) : null | |
| if (!parsed || parsed.scope === 'session') { | |
| throw new Error('Invalid approval request.') | |
| } | |
| const storageContext = getSecretContextForAccountSecret(parsed) |
🤖 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 736 - 740,
The approval path currently accepts secret IDs with scope 'session'; after
calling parseAccountSecretId(input.secretId) (and before calling
getSecretContextForAccountSecret), add the same guard used in handleDeleteAction
to reject session-scoped secrets—i.e., if parsed.scope === 'session' then throw
an error (e.g., 'Invalid approval request.'); ensure you reference
parseAccountSecretId, parsed.scope, and getSecretContextForAccountSecret when
making this check so session secrets are blocked here too.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7cda239. Configure here.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/worker/src/app/handlers/account-secrets.ts (1)
736-743:⚠️ Potential issue | 🟠 MajorReject
sessionsecret ids in approval requests too.This path still accepts
parseAccountSecretId(...).scope === 'session'and then feeds it intogetSecretContextForAccountSecretand the approval mutators. That re-enables account approvals for a secret class this file otherwise blocks from account-page operations.Suggested fix
const parsed = input.secretId ? parseAccountSecretId(input.secretId) : null - if (!parsed) { + if (!parsed || parsed.scope === 'session') { throw new Error('Invalid approval request.') }🤖 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 736 - 743, The handler currently allows parsed secret IDs with scope 'session' to proceed; update the validation after parseAccountSecretId to reject session-scoped secrets by checking parsed.scope === 'session' and throwing an error (same as other invalid requests) before calling getSecretContextForAccountSecret and performing approval mutators; ensure this check is added alongside the existing parsed null check and the requestedHost/requestedPackageId mutual exclusion check so session secrets cannot reach getSecretContextForAccountSecret or the approval logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/worker/src/app/handlers/account-secrets.ts`:
- Around line 736-743: The handler currently allows parsed secret IDs with scope
'session' to proceed; update the validation after parseAccountSecretId to reject
session-scoped secrets by checking parsed.scope === 'session' and throwing an
error (same as other invalid requests) before calling
getSecretContextForAccountSecret and performing approval mutators; ensure this
check is added alongside the existing parsed null check and the
requestedHost/requestedPackageId mutual exclusion check so session secrets
cannot reach getSecretContextForAccountSecret or the approval logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ab8218a-c00b-46c9-aacb-e60e772c33f6
📒 Files selected for processing (4)
packages/worker/client/routes/account-secrets.tsxpackages/worker/client/routes/account.tsxpackages/worker/src/app/handlers/account-secrets.node.test.tspackages/worker/src/app/handlers/account-secrets.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/worker/client/routes/account.tsx
- packages/worker/client/routes/account-secrets.tsx
- packages/worker/src/app/handlers/account-secrets.node.test.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This was referenced Apr 30, 2026
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-hostorpackage_idonly./accountapproval card now that token-free approvals require a selected secret route.Validation
npx vitest run "packages/worker/src/app/handlers/account-secrets.node.test.ts" "packages/worker/src/mcp/fetch-gateway.node.test.ts" "packages/worker/src/mcp/secrets/errors.node.test.ts" "packages/worker/src/mcp/executor.node.test.ts"npm run typecheckSummary by CodeRabbit
Refactor
Tests