Fix MCP +Add path bypassing secret redaction check#11297
Conversation
The new-server save branch was calling `from_user_json` directly, skipping the `detect_secrets_in_templatable_mcp_server` check that the edit-existing branch runs via `parse_templatable_json`. Result: secrets in a freshly added MCP server config were never blocked locally, and the user saw a generic "Failed to create mcp server" toast instead of the actionable redaction toast. Reuse the existing per-server helper after parsing so both save paths get the same local guard. Fixes warpdotdev#11265.
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR updates the new MCP server save path to run the existing templatable MCP server secret detection before creating templates, matching the edit-existing path.
Concerns
- The change is user-facing because it alters the MCP Servers + Add save flow and expected error toast, but the PR does not include screenshots or a screen recording demonstrating the actionable secret-redaction toast end to end. For this user-facing change, please include screenshots or a screen recording demonstrating it working end to end.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
/oz-review I added video of the fix. |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR adds the missing local secret-redaction guard to the new MCP server +Add save path before templatable MCP servers are created or installed.
Concerns
- No blocking correctness, security, or spec-alignment concerns found in the changed hunk.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
@peicodes can you take a look at this? A very small but important change in the MCP addition. |
|
Hey @SagarSDagdu, I'm taking over here. Will take a look later today! |
vkodithala
left a comment
There was a problem hiding this comment.
This is a great, well-scoped PR. Great job!
Added a small comment that's non-blocking. I like that you didn't make an extra test here. Approved, will merge once CI passes.
Address review nit: replace the manual `for ... if .is_err() return` with `try_for_each` over the parsed servers.
Description
The "+ Add" (new MCP server) save path in
app/src/settings_view/mcp_servers/edit_page.rswas callingParsedTemplatableMCPServerResult::from_user_jsondirectly and skipping thedetect_secrets_in_templatable_mcp_servercheck that the edit-existing path runs viaparse_templatable_json.Net effect today: secrets pasted into a freshly added MCP server config are not blocked locally — the cloud-side
sync_queuereject is the only guard, and the user sees a genericFailed to create mcp servertoast instead of the actionableThis MCP server contains secrets. Visit Settings > Privacy ...toast.This PR reuses the existing per-server helper after parsing so both save paths get the same local guard.
Linked Issue
Fixes #11265.
ready-to-implement.Screenshots / Videos
Video showing error toast after the fix:
Screen.Recording.2026-05-19.at.9.21.42.PM.mov
Testing
Manual repro per #11265:
{ "demo-new-server-bypass": { "command": "/usr/bin/true", "args": ["--api-key=sk-FAKE0000000000FAKEdemoWARP8761"] } }No automated test added; the secret-detection helper is already exercised via the edit-existing path and the change is a 12-line reuse of that helper. Happy to add an integration test on request.
Agent Mode