LLM Provider API Key Delete Warning and MCP Server Edit UI fixes - #2908
Conversation
|
Naduni Pamudika seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds an MCP proxy Backend Connection tab with editable upstream settings, masked credentials, refetch validation, secret-safe persistence, and Cypress coverage. It also resolves stored secrets during validation, adjusts URL reachability handling, and warns before API key deletion. ChangesMCP backend connection
API key deletion warnings
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant BackendConnectionTab
participant MCPServerValidationAPI
participant SecretAPI
participant MCPProxyUpdateAPI
User->>BackendConnectionTab: Edit connection or click Refetch
BackendConnectionTab->>MCPServerValidationAPI: Validate with proxyId or edited auth
MCPServerValidationAPI-->>BackendConnectionTab: Return validation result
User->>BackendConnectionTab: Save edited credential
BackendConnectionTab->>SecretAPI: Create replacement secret
SecretAPI-->>BackendConnectionTab: Return secret placeholder
BackendConnectionTab->>MCPProxyUpdateAPI: Update upstream configuration
MCPProxyUpdateAPI-->>BackendConnectionTab: Return saved proxy
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ 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 |
npamudika
changed the title
![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
🧹 Nitpick comments (2)
portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.js (1)
164-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
afterEachdoesn't clean up secrets created during setup/rotation.Each test creates at least one secret (setup) and TC-100 creates a second one via rotation, but only
createdServerId/createdProjectIdare deleted here. Unless deleting the MCP proxy cascades to its secrets, these accumulate in the test environment across runs.🤖 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 `@portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.js` around lines 164 - 183, Extend the afterEach cleanup alongside createdServerId and createdProjectId to track and delete every secret created during setup and TC-100 rotation. Reuse the existing secret identifier and authenticated DELETE API pattern, clear each identifier after cleanup, and ensure cleanup runs for all created secrets even when multiple secrets exist.portals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsx (1)
1119-1167: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
onFocusclears the masked sentinel even without any edit.Merely tabbing/clicking into the Value field unmasks and clears it (Lines 1128-1132), leaving a blank field until the user explicitly cancels — even if they back out without typing anything. Consider only clearing on first keystroke, or restoring the sentinel on blur if
hasCredentialChangedis still false.🤖 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 `@portals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsx` around lines 1119 - 1167, The auth value field’s onFocus handler in the ExternalServersOverview component clears the masked credential without an edit. Move sentinel clearing to the first user change, or restore the masked sentinel on blur when hasCredentialChanged remains false, while preserving normal editing behavior and avoiding blank values when the user only focuses and leaves the field.
🤖 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
`@portals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsx`:
- Around line 546-610: Track the newly created secret handle from the rotation
flow around createSecret and, when updateMCPServer fails, attempt to delete that
secret before showing the save-failure snackbar. Keep cleanup limited to the
newly created handle, tolerate deletion errors, and preserve the existing
successful-update cleanup of the old secret.
- Around line 538-583: Preserve the existing upstream authentication
configuration in the hasBackendConnectionChanges update path when
isRotatingCredential is false, instead of relying on the unavailable
server.upstream.main.auth.value or sending auth: undefined. Only replace or
clear auth when the credential is actively rotated, and retain the existing auth
object for URL-only or header-only edits. Add coverage for saving without
changing the credential if the surrounding test suite supports it.
In
`@portals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderDeploymentsCard.tsx`:
- Around line 825-828: The deletion warning in
ServiceProviderDeploymentsCard.tsx at lines 825-828 must use the existing
FormattedMessage localization pattern instead of raw text. Reuse the same
localized message ID and text in ServiceProviderOverviewTab.tsx at lines
1141-1144 so both warnings share one translation entry.
---
Nitpick comments:
In
`@portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.js`:
- Around line 164-183: Extend the afterEach cleanup alongside createdServerId
and createdProjectId to track and delete every secret created during setup and
TC-100 rotation. Reuse the existing secret identifier and authenticated DELETE
API pattern, clear each identifier after cleanup, and ensure cleanup runs for
all created secrets even when multiple secrets exist.
In
`@portals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsx`:
- Around line 1119-1167: The auth value field’s onFocus handler in the
ExternalServersOverview component clears the masked credential without an edit.
Move sentinel clearing to the first user change, or restore the masked sentinel
on blur when hasCredentialChanged remains false, while preserving normal editing
behavior and avoiding blank values when the user only focuses and leaves the
field.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 48f7f8b6-bed3-4dfb-8fb8-7df39ad380f6
📒 Files selected for processing (4)
portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.jsportals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsxportals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderDeploymentsCard.tsxportals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderOverviewTab.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.js (1)
308-342: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify persisted credential usability, not only the PUT shape.
This test confirms that the client sends
authwithoutvalue, but a backend regression that drops the stored secret would still pass. After saving, assert the persisted proxy or perform an un-stubbedproxyIdrefetch and verify the connection still succeeds.🤖 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 `@portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.js` around lines 308 - 342, Extend TC-101 beyond validating the PUT payload by verifying the persisted credential remains usable after the URL-only save. After the update completes, refetch the proxy through the real proxyId endpoint or assert the persisted proxy state, then verify the connection succeeds using the retained auth credential; keep the existing assertions that no new secret is created and the auth metadata is preserved.
🤖 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.
Nitpick comments:
In
`@portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.js`:
- Around line 308-342: Extend TC-101 beyond validating the PUT payload by
verifying the persisted credential remains usable after the URL-only save. After
the update completes, refetch the proxy through the real proxyId endpoint or
assert the persisted proxy state, then verify the connection succeeds using the
retained auth credential; keep the existing assertions that no new secret is
created and the auth metadata is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 860576f0-6db8-4b79-8f66-5c4db2bd7e0f
📒 Files selected for processing (5)
portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.jsportals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsxportals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderDeploymentsCard.tsxportals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderOverviewTab.tsxportals/ai-workspace/src/utils/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- portals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderOverviewTab.tsx
- portals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderDeploymentsCard.tsx
- portals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsx
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)
platform-api/internal/utils/common.go (1)
441-456: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftValidate every redirect destination before dialing it.
This follows redirects solely by count. A public, user-configured upstream can redirect the probe to a private or cloud-metadata address, and this transport will dial it. Use the shared SSRF-safe transport with dial-time resolved-IP checks, and explicitly follow only redirects whose targets pass the same validation.
As per coding guidelines, redirects must be revalidated and destination checks must occur at dial time.
🤖 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 `@platform-api/internal/utils/common.go` around lines 441 - 456, Update the HTTP client configuration around CheckRedirect to use the shared SSRF-safe transport with dial-time resolved-IP validation instead of a standalone http.Transport. Ensure CheckRedirect validates each redirect target using the same SSRF rules before allowing it, while preserving the existing redirect-count limit and disabled keep-alive behavior.Source: Coding guidelines
🤖 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 `@platform-api/internal/service/mcp.go`:
- Around line 535-539: Update the Decrypt error handling in the
secret-resolution flow around extractSecretHandle so the returned client error
is generic and contains no secret handle or underlying error details. Preserve
restricted audit logging if available, but ensure any reference is redacted or
hash-based and never expose the handle in client responses or standard
internal-error logs.
---
Outside diff comments:
In `@platform-api/internal/utils/common.go`:
- Around line 441-456: Update the HTTP client configuration around CheckRedirect
to use the shared SSRF-safe transport with dial-time resolved-IP validation
instead of a standalone http.Transport. Ensure CheckRedirect validates each
redirect target using the same SSRF rules before allowing it, while preserving
the existing redirect-count limit and disabled keep-alive behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1020fd45-38bf-4dcc-8060-26ea30c082b4
📒 Files selected for processing (3)
platform-api/internal/service/mcp.goplatform-api/internal/utils/common.goportals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- portals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsx
4 participants
Purpose
Fixes #2806 and #2810