Repository navigation
Hard-migrate secrets from app scope to package scope - #694
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe PR replaces app-scoped secrets with package-scoped secrets across storage, routing, account management, runtime context, package workflows, authorization, migrations, tests, and documentation. User-secret approvals remain package-specific, while package-owned secrets use their package bucket binding. ChangesPackage-scoped secret contracts and storage
Account and runtime integration
Validation and documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PackageRuntime
participant SecretResolver
participant PackageAccess
participant AccountSecrets
PackageRuntime->>SecretResolver: Resolve secret with packageId
SecretResolver->>PackageAccess: Validate resolved secret access
PackageAccess->>AccountSecrets: Build approval URL when user approval is missing
PackageAccess-->>PackageRuntime: Allow package-owned or approved user secret
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
kody-bot
marked this pull request as ready for review
July 9, 2026 21:55
Contributor
|
🔎 Preview deployed: https://kody-pr-694.kody-a99.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: 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/src/mcp/capabilities/secrets/secret-set.ts (1)
49-86: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftMake package-authorized user-secret updates atomic.
resolveSecretvalidates existence and approval, butsaveSecretlater performs an upsert. A concurrent account deletion can therefore let the package recreate a user secret despite the explicit no-create rule.Use a shared conditional-update service that verifies
allowed_packagesand updates an existing row atomically; reuse it for the OpenAPI refresh path as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/mcp/capabilities/secrets/secret-set.ts` around lines 49 - 86, Package-authorized user-secret updates are currently non-atomic because secret-set resolves and authorizes the secret first, then saveSecret can still upsert a deleted row. Update the secret-set flow to use a shared conditional update path that checks allowed_packages and only updates an existing user-scoped secret atomically, reusing the same service in both the package secret update path and the OpenAPI refresh path. Keep resolveSecret and assertPackageCanAccessResolvedSecret for validation, but replace the final saveSecret upsert with the atomic conditional-update helper.
🧹 Nitpick comments (1)
packages/worker/client/routes/account-secrets.tsx (1)
284-319: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the legacy secret-id fallback The
parseAccountSecretIdbranch ingetSelectionStateis unreachable now that the account secrets route table only registers/new,/approve,/user/:secretName,/package/:packageId/:secretName, and/session/:sessionId/:secretName.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/client/routes/account-secrets.tsx` around lines 284 - 319, The legacy secret-id fallback in getSelectionState is now dead code because the route table only matches the explicit secrets paths. Remove the url.pathname.startsWith(`${secretsBasePath}/`) branch and the associated parseAccountSecretId fallback logic, leaving getSelectionState to handle only /new, /approve, and the parsed AccountSecretPath cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/worker/src/mcp/capabilities/openapi-provider/operation-request.ts`:
- Around line 451-477: The package approval URL is being built from the OAuth
token URL instead of the app origin, which can send denial links to the external
provider and leak metadata. Update `executeOpenApiOperationRequest` and
`tryRefreshIntegrationAccessToken` to thread `ctx.callerContext.baseUrl` through
the approval flow, and use that value in the `assertPackageCanUpdateUserSecret`
calls instead of `input.integration.tokenUrl`.
In `@packages/worker/src/mcp/secrets/service.node.test.ts`:
- Around line 8-10: The mock in service.node.test.ts only handles the user-scope
secret_entries query, so package-scope listing is never exercised. Update the
`all()` handler in the test fixture to add a branch for
`listPackageScopeSecretMetadata`/the package-scope `secret_entries` query and
return the expected package-scoped rows, while keeping the existing user-scope
branch intact so `listPackageSecretsByPackageIds` hits the intended path.
In `@packages/worker/src/package-registry/service.ts`:
- Around line 381-390: The secret cleanup is currently gated by the
saved-package check, which leaves orphaned package secrets and stale approvals
when the saved-package row is missing. Update the package deletion flow in the
service handling the savedPackage branch so deleteAllPackageScopedSecrets and
removeAllSecretApprovalsForPackage always run for the given
env/userId/packageId, even when savedPackage is absent, while keeping any
saved-package-specific logic separate.
---
Outside diff comments:
In `@packages/worker/src/mcp/capabilities/secrets/secret-set.ts`:
- Around line 49-86: Package-authorized user-secret updates are currently
non-atomic because secret-set resolves and authorizes the secret first, then
saveSecret can still upsert a deleted row. Update the secret-set flow to use a
shared conditional update path that checks allowed_packages and only updates an
existing user-scoped secret atomically, reusing the same service in both the
package secret update path and the OpenAPI refresh path. Keep resolveSecret and
assertPackageCanAccessResolvedSecret for validation, but replace the final
saveSecret upsert with the atomic conditional-update helper.
---
Nitpick comments:
In `@packages/worker/client/routes/account-secrets.tsx`:
- Around line 284-319: The legacy secret-id fallback in getSelectionState is now
dead code because the route table only matches the explicit secrets paths.
Remove the url.pathname.startsWith(`${secretsBasePath}/`) branch and the
associated parseAccountSecretId fallback logic, leaving getSelectionState to
handle only /new, /approve, and the parsed AccountSecretPath cases.
🪄 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: ab5072cd-b06e-442d-97b4-be8e4ca304fd
📒 Files selected for processing (61)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/primitives.yamldocs/contributing/skill-patterns/cloudflare-api-v4.mddocs/guides/account-secret-setup.mddocs/use/secrets-and-values.mdpackages/shared/src/account-secret-route.node.test.tspackages/shared/src/account-secret-route.tspackages/shared/src/chat.tspackages/worker/client/routes/account-approval-shared.tspackages/worker/client/routes/account-secrets.tsxpackages/worker/client/routes/index.tsxpackages/worker/migrations/0057-package-scoped-secrets.sqlpackages/worker/src/app/account-secrets-data.tspackages/worker/src/app/handlers/account-secrets.node.test.tspackages/worker/src/app/handlers/account-secrets.tspackages/worker/src/app/loader-data.tspackages/worker/src/app/router.tspackages/worker/src/app/routes.tspackages/worker/src/jobs/service.node.test.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp/capabilities/meta/execute.tspackages/worker/src/mcp/capabilities/meta/search-and-execute.node.test.tspackages/worker/src/mcp/capabilities/openapi-provider/index.tspackages/worker/src/mcp/capabilities/openapi-provider/operation-request.node.test.tspackages/worker/src/mcp/capabilities/openapi-provider/operation-request.tspackages/worker/src/mcp/capabilities/secrets/jwt-sign.tspackages/worker/src/mcp/capabilities/secrets/secret-delete.tspackages/worker/src/mcp/capabilities/secrets/secret-list.tspackages/worker/src/mcp/capabilities/secrets/secret-set.tspackages/worker/src/mcp/capabilities/secrets/shared.tspackages/worker/src/mcp/execute-modules/kody-runtime-utils.tspackages/worker/src/mcp/fetch-gateway.node.test.tspackages/worker/src/mcp/fetch-gateway.tspackages/worker/src/mcp/run-kody-registry.tspackages/worker/src/mcp/secrets/capability-approval-url.tspackages/worker/src/mcp/secrets/host-approval.tspackages/worker/src/mcp/secrets/package-access.node.test.tspackages/worker/src/mcp/secrets/package-access.tspackages/worker/src/mcp/secrets/package-approval-url.tspackages/worker/src/mcp/secrets/package-scope-migration.node.test.tspackages/worker/src/mcp/secrets/placeholders.tspackages/worker/src/mcp/secrets/repo.tspackages/worker/src/mcp/secrets/secret-bindings.tspackages/worker/src/mcp/secrets/service.node.test.tspackages/worker/src/mcp/secrets/service.tspackages/worker/src/mcp/secrets/types.tspackages/worker/src/mcp/storage.tspackages/worker/src/mcp/tools/execute.node.test.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/package-invocations/service.node.test.tspackages/worker/src/package-invocations/service.tspackages/worker/src/package-registry/service.node.test.tspackages/worker/src/package-registry/service.tspackages/worker/src/package-registry/types.tspackages/worker/src/package-retrievers/service.tspackages/worker/src/package-runtime/package-app.tspackages/worker/src/package-runtime/package-service.tspackages/worker/src/package-runtime/package-workflows.node.test.tspackages/worker/src/package-runtime/package-workflows.tspackages/worker/src/package-runtime/realtime-session.tspackages/worker/src/repo/checks.ts
💤 Files with no reviewable changes (2)
- packages/worker/src/app/router.ts
- packages/worker/src/app/routes.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3130513. Configure here.
| SELECT json_group_array(value) | ||
| FROM json_each(e.allowed_packages) | ||
| WHERE value <> ? | ||
| ), |
Contributor
There was a problem hiding this comment.
Approval list becomes null JSON
Low Severity
When the last package id is removed from a user secret’s allowed_packages, removePackageFromSecretApprovals sets the column via json_group_array with no rows, which SQLite stores as SQL NULL instead of []. Downstream updates that require json_valid(allowed_packages) can then skip those rows.
Reviewed by Cursor Bugbot for commit 3130513. Configure here.
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
3 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.
Hard-migrates secret ownership from app scope to package scope with no compatibility alias. Existing app buckets and legacy package caller contexts are rewritten by migration; package identity is propagated through apps, jobs, services, invocations, inline workflows, and OpenAPI operations. User secrets require package approval across mounts, fetch placeholders, secret-aware capabilities, and atomic secret mutations. Package deletion removes owned secrets and stale grants even when the package projection is already missing.
Walkthrough
Package scope works for headless packages; only user secrets expose package grants.
System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@6e5e315· Head:c88f148Classification: extends — changes the secret ownership and authorization contract without adding a new system primitive.
Primitives touched
secretssaved-packagespackage-runtimecapabilities-executeopenapi-bindingsworkflowsjobsapp-uid1-app-dbSystem map
Package identity flows from saved packages through every runtime into secret resolution and D1 ownership; account UI manages package buckets and user-secret grants.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Before / after
session | app | usersecret scopessession | package | useronlyInvariants
per-user-isolationremains mandatory on every bucket, package lookup, migration, and cleanup query.no-secrets-in-chatremains unchanged; plaintext stays inside server-side resolution.Summary by CodeRabbit