fix(mcp): run secret-redaction predicate on the new-server '+ Add' save path (#11265) by david-engelmann · Pull Request #11360 · warpdotdev/warp

david-engelmann · 2026-05-19T22:38:53Z

Description

The new-MCP-server save branch in MCPServersEditPageView::handle_action calls ParsedTemplatableMCPServerResult::from_user_json(&json) directly and skips parse_templatable_json — which is where #10839's predicate (detect_secrets_in_templatable_mcp_server → should_block_save_for_secrets) is invoked. As a result, a config pasted into the + Add dialog and saved bypasses the redaction check entirely, even with Settings → Privacy → Secret redaction enabled or enterprise enforcement active. The same JSON saved via the edit-existing flow is correctly blocked.

Fix

Mirror the existing per-parsed-server detection loop from parse_templatable_json into the new-server branch in app/src/settings_view/mcp_servers/edit_page.rs:

// Run the same secret-redaction predicate that gates the
// edit-existing save path. Without this loop the new-server
// (`+ Add`) save bypasses `parse_templatable_json` (where
// the predicate is wired) and ignores Settings → Privacy
// → Secret redaction entirely (#11265).
for parsed_server in &parsed_servers {
    if self
        .detect_secrets_in_templatable_mcp_server(
            ctx,
            &parsed_server.templatable_mcp_server,
        )
        .is_err()
    {
        return;
    }
}

The predicate itself (added in #10839 with the truth-table tests) is unchanged — this PR closes the gap by routing the + Add path through it. Net diff: +17 / -0 lines.

Stacked on #10839

This PR depends on should_block_save_for_secrets and detect_secrets_in_templatable_mcp_server from #10839. The branch was cut from david/8761-mcp-secret-redaction-config-save (the #10839 branch); once #10839 merges, the GitHub diff here will narrow to just the new-server-branch loop.

Demo — fix in action

End-to-end Settings UI recording demonstrating the fix. Build state for this recording: PR #10839's predicate + this PR's new-server detection loop + DEMO-PATCH-B (so SECRETS_REGEX recompiles when the toggle is flipped — needed to demonstrate this PR's fix in isolation; the underlying recompile gap is tracked at #11262). The token sits in the top-level url to keep the demo isolated from #11263's env/headers templatization gap.

A Settings → Privacy → Secret redaction toggle normalized to OFF.
B Click toggle ON → "Secret visual redaction mode" dropdown appears (visual proof safe_mode_enabled = true at runtime).
C Settings → MCP Servers → + Add → paste a config with sk-FAKE0000000000FAKEdemoWARP11360fix in the top-level url.
D Click Save → error toast appears: "This MCP server contains secrets. Visit Settings > Privacy to modify your secret redaction settings." The modal stays open with the secret-bearing JSON; no entry lands in MY MCPS.

Recipe: scripts/recipes/11360-mcp-add-server-fix.json, driven by scripts/warp-driver.swift (OCR-gated, no manual driving).

Demo — pre-fix bypass (for contrast)

The earlier reproduction recording shows the same save proceeding silently against the unpatched build:

Build state: DEMO-PATCH-B + DEMO-PATCH-C applied locally (the proposed fixes for #11262 / #11263), this PR's fix not applied — so the bug is fully isolated to the + Add save branch. Phases:

A Toggle starts OFF.
B Click toggle ON → "Secret visual redaction mode" dropdown appears.
C + Add → paste a baseline (no-secret) config demo-baseline-11265 → Save → established in MY MCPS. Establishes a server for the control comparison.
D + Add again → paste a config with sk-FAKE0000000000FAKEdemoWARP11265 in the top-level url → Save → server lands in MY MCPS as demo-add-bypass-11265. BUG: the new-server save bypassed the predicate entirely.

Recipe: scripts/recipes/11265-mcp-add-server-bypass.json.

Testing

$ cargo nextest run -p warp -E 'test(should_block_save_for_secrets) or test(does_not_block_when) or test(blocks_when_)'
5 tests run: 5 passed

$ cargo clippy -p warp --tests -- -D warnings
clean

$ cargo fmt -p warp -- --check
clean

The predicate's existing 5-case truth-table tests (from #10839) continue to cover the logic — this PR only adds a new call site, not new logic. The local-repro recording above exercises the new call site end-to-end.

Server API dependencies

None — this is client-only.

Agent Mode

Unchecked (external contribution).

Changelog

CHANGELOG-BUG-FIX: Defining a new MCP server via Settings → MCP Servers → + Add no longer bypasses the secret-redaction check when Settings → Privacy → Secret redaction (or enterprise enforcement) is active.

Fixes #11265

oz-for-oss · 2026-05-19T22:39:11Z

@david-engelmann

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 /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

oz-for-oss

Overview

This PR routes the new MCP server + Add save path through the same secret-redaction predicate used by the edit-existing flow and keeps the predicate gated on user or enterprise redaction settings.

Concerns

⚠️ [IMPORTANT] For this user-facing change, please include screenshots or a screen recording demonstrating the fixed + Add save path working end to end. The attached recording reproduces the pre-fix bypass with this PR's fix not applied, so it does not verify the user-visible behavior after the change.

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

david-engelmann · 2026-05-20T04:49:21Z

Re-recorded the demo against a build with this PR's fix applied (PR #10839 predicate + this PR's new-server detection loop + DEMO-PATCH-B). Toast This MCP server contains secrets… fires; modal stays open; no MY MCPS entry created. The previous bug-reproduction recording is preserved below the new one for contrast.

/oz-review

oz-for-oss · 2026-05-20T04:49:38Z

@david-engelmann

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: @lucieleblanc.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Oz no longer requests changes for this pull request after the latest automated review.

oz-for-oss

Overview

This PR adds the missing secret-redaction check to the new MCP server + Add save path and updates the shared predicate so saves are blocked only when user or enterprise secret redaction is active. The attached spec context contains no approved spec commitments, and the PR description includes end-to-end visual evidence for the user-facing behavior.

Concerns

No blocking correctness, spec-alignment, or security concerns found in the annotated diff.

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

…ig (warpdotdev#8761) When a user defines an MCP server config (Settings → MCP Servers → Add) that contains a value matching the secret-detection regexes — for example, `"Authorization": "Bearer <real-token>"` — the save was rejected with the error "This MCP server contains secrets. Visit Settings > Privacy to modify your secret redaction settings." *even when the user had explicitly disabled secret redaction in Settings → Privacy*. Root cause: `detect_secrets_in_templatable_mcp_server` in `settings_view::mcp_servers::edit_page` called `find_secrets_in_text` unconditionally and rejected the save if any match was found, without consulting the user's redaction-enabled toggle or the workspace-level enterprise-enforcement flag. The error message itself told the user to visit the Privacy settings — but those settings had no effect on this code path. Fix: gate the save-blocking decision on whether redaction is actually in force. Extracted a pure `should_block_save_for_secrets(safe_mode_enabled, enterprise_enforced, contains_secrets)` predicate so the policy is one expression in one place: (safe_mode_enabled || enterprise_enforced) && contains_secrets When both the user-level toggle (`SafeModeSettings::safe_mode_enabled`) AND the enterprise enforcement flag (`UserWorkspaces::is_enterprise_secret_redaction_enabled`) are off, the user has explicitly opted to embed secrets in the config and we save it as written. Enterprise enforcement still wins over a personal toggle — orgs that mandate redaction cannot be bypassed at the MCP-config layer. Tests: 5 unit tests covering the predicate's truth table: - does_not_block_when_redaction_off_even_if_secrets_present (the warpdotdev#8761 case) - blocks_when_user_redaction_on_and_secrets_present - blocks_when_enterprise_enforced_and_secrets_present - does_not_block_when_no_secrets_regardless_of_toggle (2x2 sweep) - blocks_when_both_redactions_on_and_secrets_present Verified locally: cargo nextest run -p warp -E 'test(should_block_save_for_secrets) or test(does_not_block_when) or test(blocks_when_)' 5 tests run: 5 passed cargo clippy -p warp --tests -- -D warnings Finished (no warnings) cargo fmt -p warp -- --check clean Fixes warpdotdev#8761 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ve path (warpdotdev#11265) The new-MCP-server save branch in `MCPServersEditPageView::handle_action` calls `ParsedTemplatableMCPServerResult::from_user_json` directly and skips `parse_templatable_json` — which is where warpdotdev#10839's predicate (`detect_secrets_in_templatable_mcp_server` → `should_block_save_for_secrets`) is invoked. As a result, a config pasted into `+ Add` and saved bypasses the redaction check entirely even with Settings → Privacy → Secret redaction enabled or enterprise enforcement active. The same JSON saved via the edit-existing flow is correctly blocked. Mirror the existing per-parsed-server detection loop from `parse_templatable_json` into the new-server branch. The predicate itself (added in warpdotdev#10839 with five truth-table tests) is unchanged — this PR closes the gap by routing the `+ Add` path through it. ## Reproduction Recipe + recording: https://github.com/david-engelmann/warp-taper/blob/main/scripts/recipes/11265-mcp-add-server-bypass.json Evidence GIF: https://raw.githubusercontent.com/david-engelmann/warp-taper/main/docs/evidence/11265-mcp-add-server-bypass/master.gif (also posted in the warpdotdev#11265 issue thread) ## Testing ``` $ cargo nextest run -p warp -E 'test(should_block_save_for_secrets) or test(does_not_block_when) or test(blocks_when_)' 5 tests run: 5 passed $ cargo clippy -p warp --tests -- -D warnings clean $ cargo fmt -p warp -- --check clean ``` Fixes warpdotdev#11265 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

david-engelmann · 2026-05-27T20:49:18Z

Rebased to clear the merge conflict from recent master movement. Rebuilt on top of #10839's current head (d29ca6e0) plus this PR's single new-server-path commit, so the net diff over #10839 is just the +17-line + Add routing fix. git merge-tree against master is clean; the 5 redaction truth-table tests pass; clippy + fmt clean.

Dependency unchanged: still stacked on #10839 (the should_block_save_for_secrets predicate lives there). #10839 is not yet merged — once it lands this rebases to just the new-server delta.

vkodithala · 2026-05-29T14:25:43Z

Hey @david-engelmann, I'm taking over here on review. Will take a look later today!

vkodithala · 2026-06-03T17:56:45Z

Hey @david-engelmann, thanks for the PR and for filing this issue! I took a peek, for now I'm working on merging in this PR, which predates yours by a few hours.

It's not super relevant now anyways, but for reference I found a few issues with your PR on first glance. Namely, I think you re-iterate logic that already appears in detect_secrets_in_templatable_mcp_server (which the other PR reuses), and think if we reuse this logic, we can omit tests altogether. Thanks again, and hopefully this was helpful!

vkodithala · 2026-06-03T17:56:54Z

Closing per the comment above.

cla-bot Bot added the cla-signed label May 19, 2026

github-actions Bot added the external-contributor Indicates that a PR has been opened by someone outside the Warp team. label May 19, 2026

david-engelmann mentioned this pull request May 19, 2026

fix(mcp): respect secret-redaction toggle when saving MCP server config (#8761) #10839

Open

oz-for-oss Bot previously requested changes May 19, 2026

View reviewed changes

oz-for-oss Bot reviewed May 20, 2026

View reviewed changes

oz-for-oss Bot requested a review from lucieleblanc May 20, 2026 04:52

lucieleblanc requested review from peicodes and removed request for lucieleblanc May 20, 2026 16:02

david-engelmann and others added 2 commits May 27, 2026 15:31

david-engelmann force-pushed the david/11265-mcp-add-server-bypass branch from c330c49 to edad1bc Compare May 27, 2026 19:54

vkodithala requested review from vkodithala and removed request for peicodes May 29, 2026 14:24

vkodithala closed this Jun 3, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mcp): run secret-redaction predicate on the new-server '+ Add' save path (#11265)#11360

fix(mcp): run secret-redaction predicate on the new-server '+ Add' save path (#11265)#11360
david-engelmann wants to merge 2 commits into
warpdotdev:masterfrom
david-engelmann:david/11265-mcp-add-server-bypass

david-engelmann commented May 19, 2026 •

edited

Loading

Uh oh!

oz-for-oss Bot commented May 19, 2026 •

edited

Loading

Uh oh!

oz-for-oss Bot left a comment

Uh oh!

david-engelmann commented May 20, 2026

Uh oh!

oz-for-oss Bot commented May 20, 2026 •

edited

Loading

Uh oh!

oz-for-oss Bot left a comment

Uh oh!

david-engelmann commented May 27, 2026

Uh oh!

vkodithala commented May 29, 2026

Uh oh!

vkodithala commented Jun 3, 2026

Uh oh!

vkodithala commented Jun 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

david-engelmann commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fix

Stacked on #10839

Demo — fix in action

Demo — pre-fix bypass (for contrast)

Testing

Server API dependencies

Agent Mode

Changelog

Uh oh!

oz-for-oss Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oz-for-oss Bot left a comment

Choose a reason for hiding this comment

Overview

Concerns

Verdict

Uh oh!

david-engelmann commented May 20, 2026

Uh oh!

oz-for-oss Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oz-for-oss Bot left a comment

Choose a reason for hiding this comment

Overview

Concerns

Verdict

Uh oh!

david-engelmann commented May 27, 2026

Uh oh!

vkodithala commented May 29, 2026

Uh oh!

vkodithala commented Jun 3, 2026

Uh oh!

vkodithala commented Jun 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

david-engelmann commented May 19, 2026 •

edited

Loading

oz-for-oss Bot commented May 19, 2026 •

edited

Loading

oz-for-oss Bot commented May 20, 2026 •

edited

Loading