feat(byok): support multiple keys per provider with round-robin rotation#4963
feat(byok): support multiple keys per provider with round-robin rotation#4963waleedlatif1 wants to merge 91 commits into
Conversation
…ership workflow edits via sockets, ui improvements
…ng improvements, posthog, secrets mutations
…ration, signup method feature flags, SSO improvements
…nts, secrets performance, polling refactors, drag resources in mothership
…y invalidation, HITL docs
…endar triggers, docs updates, integrations/models pages improvements
…ions, jira forms endpoints
…mat, logs performance improvements fix(csp): add missing analytics domains, remove unsafe-eval, fix workspace CSP gap (#4179) fix(landing): return 404 for invalid dynamic route slugs (#4182) improvement(seo): optimize sitemaps, robots.txt, and core web vitals across sim and docs (#4170) fix(gemini): support structured output with tools on Gemini 3 models (#4184) feat(brightdata): add Bright Data integration with 8 tools (#4183) fix(mothership): fix superagent credentials (#4185) fix(logs): close sidebar when selected log disappears from filtered list; cleanup (#4186)
v0.6.46: mothership streaming fixes, brightdata integration
…m integration, atlassian triggers
…d new relic integrations
…oogle slides endpoints, DB access pattern improvements
…er, search & replace UX, kb connectors multi-select, mcp negative cache
…fal-ai hosted key
… support, executor code cleanup
…ter, hosted-key queueing v0.6.92: enrichment table column type, table run fixes, scheduled jitter, hosted-key queueing
… messages table, user stats hot path improvements, RB2B
v0.6.95: data enrichment block, nullable workflow description fix
…ables UI improvements, new slack scopes, model-level denylists, object storage tracespans
…ble, integration icons audit
…ion, together AI, Baseten, Ollama Cloud, Linq integrations, kb doc ownership bindings
…dgebase connector, SSO provider ID allowlist, singleton memory leak fix
…rs, clickhouse integration
…ration, smooth streaming, security hardening, db fixes
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 29606901 | Triggered | Generic High Entropy Secret | a54dcbe | apps/sim/providers/utils.test.ts | View secret |
| 32763747 | Triggered | Generic Password | 3e9849b | helm/sim/tests/validators_test.yaml | View secret |
| 33881454 | Triggered | Generic Password | 7ffc495 | apps/sim/app/api/chat/utils.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
PR SummaryHigh Risk Overview Runtime: API: GET returns every key with UI: Workspace BYOK settings use multi-key mode (key count, Manage modal for per-key add/update/delete). Contracts, React Query hooks, and route/rotation unit tests are updated accordingly. Reviewed by Cursor Bugbot for commit 30bb92a. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 30bb92a. Configure here.
| workspaceId, | ||
| providerId, | ||
| encryptedApiKey: encrypted, | ||
| name: name || null, |
There was a problem hiding this comment.
Concurrent adds exceed key cap
Medium Severity
New-key creation reads the per-provider count and inserts in separate steps with no transaction or lock. Concurrent POSTs without keyId can each see a count below MAX_BYOK_KEYS_PER_PROVIDER and all succeed, storing more than the advertised ten keys per provider.
Reviewed by Cursor Bugbot for commit 30bb92a. Configure here.
Greptile SummaryThis PR upgrades BYOK from one key per provider to a pool of up to 10 keys per workspace+provider pair, with round-robin rotation handled entirely in-process via a module-level cursor map. The unique
Confidence Score: 3/5The feature is well-structured and thoroughly tested, but the key-cap check in the route handler is not atomic, allowing concurrent requests to store more than 10 keys per provider. The count-then-insert sequence for adding new keys has no transaction boundary and no database-level enforcement. Two simultaneous POST requests for the same workspace+provider can both read a count below 10 and both successfully insert, silently bypassing the cap. The old unique index provided DB-level safety; nothing equivalent was added in its place. apps/sim/app/api/workspaces/[id]/byok-keys/route.ts — the count-check → insert path needs a transaction or a DB-level constraint to be safe under concurrent load. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant RouteHandler as POST /byok-keys
participant DB as Database
participant Encryption
Client->>RouteHandler: "POST {providerId, apiKey, keyId?, name?}"
RouteHandler->>Encryption: encryptSecret(apiKey)
Encryption-->>RouteHandler: encrypted
alt keyId provided (update existing key)
RouteHandler->>DB: "SELECT id, name WHERE id=keyId AND workspaceId AND providerId"
DB-->>RouteHandler: existingKey or empty
alt key not found
RouteHandler-->>Client: 404 Not Found
else key found
RouteHandler->>DB: UPDATE SET encryptedApiKey, name, updatedAt
RouteHandler-->>Client: 200 success
end
else no keyId - add new key
RouteHandler->>DB: SELECT count() WHERE workspaceId AND providerId
DB-->>RouteHandler: keyCount
alt "keyCount >= 10 race window"
RouteHandler-->>Client: 400 cap exceeded
else "keyCount < 10"
RouteHandler->>DB: INSERT new key
DB-->>RouteHandler: newKey
RouteHandler-->>Client: 200 success
end
end
Note over RouteHandler,DB: count-check and insert are not atomic
Reviews (1): Last reviewed commit: "feat(byok): support multiple keys per pr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| success: true, | ||
| key: { | ||
| id: existingKey[0].id, | ||
| id: existingKey.id, | ||
| providerId, | ||
| name: updatedName, | ||
| maskedKey: maskApiKey(apiKey), | ||
| updatedAt: new Date(), | ||
| updatedAt, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| const [{ keyCount }] = await db | ||
| .select({ keyCount: count() }) | ||
| .from(workspaceBYOKKeys) | ||
| .where( | ||
| and( | ||
| eq(workspaceBYOKKeys.workspaceId, workspaceId), | ||
| eq(workspaceBYOKKeys.providerId, providerId) | ||
| ) | ||
| ) | ||
|
|
||
| if (keyCount >= MAX_BYOK_KEYS_PER_PROVIDER) { | ||
| return NextResponse.json( | ||
| { | ||
| error: `A workspace can store at most ${MAX_BYOK_KEYS_PER_PROVIDER} keys per provider`, | ||
| }, | ||
| { status: 400 } |
There was a problem hiding this comment.
The SELECT count() → INSERT sequence is not wrapped in a transaction and there is no database-level constraint that enforces the 10-key cap. Two simultaneous POST requests for the same workspace + provider can both read keyCount = 9, both pass the >= 10 guard, and both successfully insert, leaving the workspace with 11 or more keys for that provider. The previous unique index prevented duplicate rows; the new plain lookup index provides no such protection for the cap.
Wrapping the count-check and insert in a serializable transaction (or using a row-level lock with SELECT ... FOR UPDATE) would close the window.
| const { encrypted } = await encryptSecret(apiKey) | ||
|
|
||
| const existingKey = await db | ||
| .select() | ||
| .from(workspaceBYOKKeys) | ||
| .where( | ||
| and( | ||
| eq(workspaceBYOKKeys.workspaceId, workspaceId), | ||
| eq(workspaceBYOKKeys.providerId, providerId) | ||
| if (keyId) { |
There was a problem hiding this comment.
encryptSecret is called before any request validation
If the keyId doesn't exist in the workspace (→ 404) or the cap is already reached (→ 400), the encryption work has already been done and is simply discarded. Moving the encryptSecret call inside each branch avoids wasting compute on requests that will fail validation.
| const { encrypted } = await encryptSecret(apiKey) | |
| const existingKey = await db | |
| .select() | |
| .from(workspaceBYOKKeys) | |
| .where( | |
| and( | |
| eq(workspaceBYOKKeys.workspaceId, workspaceId), | |
| eq(workspaceBYOKKeys.providerId, providerId) | |
| if (keyId) { | |
| if (keyId) { | |
| const { encrypted } = await encryptSecret(apiKey) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| } | ||
|
|
||
| const handleDelete = async () => { | ||
| if (!deleteConfirmProvider) return | ||
| if (!deleteConfirm) return | ||
|
|
||
| try { | ||
| await onDelete(deleteConfirmProvider) | ||
| setDeleteConfirmProvider(null) | ||
| if (props.multiKey) { | ||
| if (deleteConfirm.keyId) { | ||
| await props.onDeleteKey(deleteConfirm.providerId, deleteConfirm.keyId) | ||
| } | ||
| } else { | ||
| await props.onDelete(deleteConfirm.providerId) | ||
| } | ||
| setDeleteConfirm(null) | ||
| } catch (err) { | ||
| logger.error('Failed to delete BYOK key', { error: err }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
keyId is absent in multi-key mode
In the multi-key branch of handleDelete, if deleteConfirm.keyId is undefined the inner if body is skipped, but execution still falls through to setDeleteConfirm(null) — the confirmation dialog closes without calling props.onDeleteKey and without surfacing any error or feedback to the user. While today's UI always populates keyId before opening the confirm modal in multi-key mode, the missing else branch (or an early return) means a future caller could silently no-op a delete.
4 participants
Summary
(workspace_id, provider_id)unique index, added a plain composite lookup index and a nullablenamelabel column — existing rows need no backfillgetBYOKKey(in-process cursor, decrypt-failure skips to the next key), so all call sites are untouched/api/workspaces/[id]/byok-keystake an optionalkeyId: with it they update/delete that key, without it they keep the old add / delete-all-for-provider semanticsType of Change
Testing
Added unit tests for rotation (cycle order, per-provider cursors, decrypt fallback) and route semantics (add vs update-by-keyId, cap, delete one/all, authz) — 24 tests passing. Typecheck and
check:api-validationpass.Checklist