fix: disallow updating DB-stored secret when readonly vault is enabled

[brojd]

No description provided.

[claude]

Claude finished @brojd's task in 2m 17s —— View job

---

PR Review: fix: disallow updating DB-stored secret when readonly vault is enabled

• Read repository guidelines (CLAUDE.md)
• Gather context and read changed files
• Analyze changes and provide review feedback

---

Summary

This PR addresses the scenario where a readonly vault (BYOS/READONLY_VAULT) is enabled but existing catalog items still have DB-stored secrets (non-vault-reference secrets). The changes:

1. Tighten the GET /api/secrets/:id endpoint — now checks isByosVault on the secret itself rather than the global isByosEnabled() flag, preventing accidental leaking of DB-stored secret values.
2. Add a new DELETE /api/internal_mcp_catalog/:id/local-config-secret endpoint — allows deleting stale DB-stored secrets so users can recreate them as vault references.
3. Frontend: detect and surface stale DB secrets — shows a destructive alert banner when editing a catalog item that has DB-stored secrets while readonly vault is active, with a confirmation dialog to delete them.
4. Disable env var inputs while stale secrets exist — prevents editing environment variables until the stale DB secret is cleaned up.

Overall this is a well-structured fix. The approach is sound: detect the conflict on the frontend, surface it clearly to the user, provide a one-click remediation, and block edits that would be invalid.

---

Feedback

1. Backend: updateSecret still allows writing to DB-stored secrets in READONLY_VAULT mode

The PR title says "disallow updating DB-stored secret when readonly vault is enabled," and the frontend correctly blocks this by disabling the env var form. However, the backend PUT /api/internal_mcp_catalog/:id handler still calls secretManager().updateSecret() on DB-stored secrets at platform/backend/src/routes/internal-mcp-catalog.ts:494. The ReadonlyVaultSecretManager.updateSecret() (at readonly-vault.ee.ts:206) writes directly to the DB without checking whether the secret has isByosVault=false.

This means a direct API call (bypassing the frontend) could still update a DB-stored secret when readonly vault is enabled. Consider adding a backend guard — either in the route handler or in ReadonlyVaultSecretManager.updateSecret() — to reject updates to non-BYOS secrets. This would provide defense-in-depth beyond the frontend-only block.

2. hasStaleDbSecret false-positive risk during loading

At mcp-catalog-form.tsx:107-118, the hasStaleDbSecret detection logic is:

const hasStaleDbSecret =
  showByosOption &&
  mode === "edit" &&
  !!initialValues?.localConfigSecretId &&
  !localConfigSecret &&
  !isLoadingSecret;

The useGetSecret hook has enabled: showByosOption. If showByosOption is true, the query runs, and this is fine. But the detection depends on the query completing with a null result. If the API call fails for a transient reason (network error, server down), localConfigSecret would also be null, and isLoadingSecret would be false, causing a false positive. The user would see "Database-stored secrets detected" even if the secret might actually be a valid BYOS secret.

This is a minor edge case, but worth noting. You could check isError from the query to distinguish "secret is non-BYOS (403)" from "request failed (network error)."

3. useGetSecret signature is a breaking change

The useGetSecret hook signature changed from positional args to an object (platform/frontend/src/lib/secrets.query.ts:24). The grep confirms it's only called in one place, so this is safe. Just flagging for awareness.

4. Good: proper RBAC and route registration

The new DeleteInternalMcpCatalogItemLocalConfigSecret route is correctly registered in:

• platform/shared/routes.ts (RouteId)
• platform/shared/access-control.ts (internalMcpCatalog: ["update"] — appropriate permission level)
• platform/backend/src/routes/internal-mcp-catalog.ts (handler with built-in catalog check)

5. Good: AlertDialog for destructive action

The "Delete secrets" action uses a confirmation dialog (AlertDialog), which is the right UX pattern for an irreversible destructive operation.

6. Good: fieldset for disabling inputs

Using <fieldset disabled={disabled}> in environment-variables-form-field.tsx:127 is a clean approach that natively disables all form controls within the fieldset without needing to pass disabled to each individual input.

7. Minor: the new endpoint returns { success: true } even when there's no secret to delete

At internal-mcp-catalog.ts:692, if catalogItem.localConfigSecretId is null, the endpoint returns { success: true } without doing anything. This is a reasonable idempotent behavior, but worth noting that it could mask cases where the client calls this on an item that never had a local config secret.

---

Verdict

This is a solid fix with good frontend UX. The main actionable suggestion is to add a backend-side guard against updating non-BYOS secrets when READONLY_VAULT is enabled (point #1 above), to ensure the protection isn't frontend-only.

[London-Cat]

📊 Reputation Summary

User	Rep	Pull Requests	Activity	Assigned	Core Reactions
brojd	⚡ 1856	89✅ 2🔄 9❌	32 issues, 50 comments	3	—

---

How is the score calculated? Read about it in the Reputation Bot repository 🤖

[github-actions]

Playwright test results

 1 failed
 578 passed
 2 flaky
 1 skipped

Details

 582 tests across 57 suites
 4 minutes, 15 seconds
 04a6332

Failed tests

api › api/chat-settings.spec.ts › Chat API Keys Access Control › member should be able to read chat API keys

Flaky tests

api › api/mcp-gateway.spec.ts › MCP Gateway - External MCP Server Tests › should list internal-dev-test-server tool
chromium › ui/chat-settings.spec.ts › Provider Settings - API Keys › Admin can create, update, and delete an API key

Skipped tests

api › api/chat-settings.spec.ts › Chat API Keys Access Control › member should not be able to create chat API keys