feat(mcp): step-up auth gate for destructive ops — Phase 1 (KLA-408)#23
Merged
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Reviewed by Cursor Bugbot for commit 66d37fb. Configure here.
4 tasks
jklaassenjc
pushed a commit
that referenced
this pull request
Apr 28, 2026
Address two Cursor Bugbot findings on PR #23: 1. (low) StepUpAPIKey was wired unconditionally — read config.APIKey() only when --require-step-up is on, matching the --require-auth pattern in the same file. 2. (medium) Missing startup validation when --require-step-up is set but no credential exists. The TTY authenticator would otherwise reject every destructive call at runtime with "no challenge channel available." Now fails fast at startup with a clear message and a pointer to fix it. Both fixes are one block, preceding the mcp.NewServer call. No behavioral change when --require-step-up is off (which is the default). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jrennichjc
previously approved these changes
Apr 28, 2026
Adds an opt-in chokepoint that gates every destructive MCP tool invocation (any tool argument carrying Execute: true) behind a fresh proof of operator presence. Phase 1 ships the framework and a TTY-based authenticator; Touch ID and out-of-band approval are slated for follow- up slices. What's wired: - internal/mcp/stepup.go — stepUpAuthenticator interface, noopStepUp (zero-cost default), ttyStepUp (prompts for last-6 of API key, mutex-serialized so concurrent destructive calls don't interleave), newStepUp factory. - internal/mcp/tools.go — single chokepoint inside addTypedTool's wrappedHandler. Reflection-based check for Execute: true on the generic In type covers all 30+ destructive tool inputs (destructive Input, userUpdateInput, membershipInput, commandRunInput, etc.) without per-handler hooks. On denial, returns an error result and records a step-up entry in the audit log. - internal/mcp/server.go — Options.RequireStepUp + StepUpAPIKey wire config through to the authenticator. Unexported Options.stepUp lets tests inject a recording fake. - internal/cmd/mcp.go — --require-step-up flag (defaults to mcp.require_step_up_for_destructive in config). When set with transport=stdio, prints a startup warning that TTY prompts can't reach the operator and destructive calls will fail closed. - internal/config/config.go — mcp.require_step_up_for_destructive config key (default false), MCPRequireStepUp() getter, ValidConfig Keys + coerceValue handling. Trade-offs: - Last-6-of-API-key is a weak challenge — anyone holding the key answers it. Real value is in (a) catching agent-flipped Execute and (b) forcing in-the-loop pause on destructive calls. Stronger authenticators (Touch ID, push approval) plug into the same interface as Phase 2. - Stdio transport can't host a TTY prompt; --require-step-up is only effective when stdin is a real terminal (e.g. operator-launched --transport http session). Documented via startup warning. Tests: - 9 stepup unit tests (noop, factory, ttyStepUp accept/deny/missing- key/short-key paths) - 2 reflection helper tests (table-driven, multiple tool input types) - 4 chokepoint integration tests (destructive triggers step-up, denial blocks API call, plan mode bypasses, non-destructive tools bypass) - CLI flag-defaults test - Full suite green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address two Cursor Bugbot findings on PR #23: 1. (low) StepUpAPIKey was wired unconditionally — read config.APIKey() only when --require-step-up is on, matching the --require-auth pattern in the same file. 2. (medium) Missing startup validation when --require-step-up is set but no credential exists. The TTY authenticator would otherwise reject every destructive call at runtime with "no challenge channel available." Now fails fast at startup with a clear message and a pointer to fix it. Both fixes are one block, preceding the mcp.NewServer call. No behavioral change when --require-step-up is off (which is the default). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
0668567 to
3037565
Compare
jrennichjc
approved these changes
Apr 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
Phase 1 of KLA-408. Adds an opt-in chokepoint that gates every destructive MCP tool invocation (any tool argument carrying
Execute: true) behind a fresh proof of operator presence. Ships the framework and a TTY-based authenticator; Touch ID and out-of-band approval are slated for follow-up slices.Why
Today the
execute: truebit on the 30+ destructive MCP tools is agent-controlled: any connected MCP client can flip it without operator awareness. The audit log records "the credential called users_delete," not "the operator at the keyboard called it." This adds a real human-in-the-loop pause point for the operators who want one.What's wired
internal/mcp/stepup.go—stepUpAuthenticatorinterface,noopStepUp(zero-cost default),ttyStepUp(mutex-serialized prompt for last-6 of API key),newStepUpfactory. Reflection helpersisExecutingDestructiveanddestructiveTargetso a single chokepoint covers all 30+ destructive tool input types without per-handler hooks.internal/mcp/tools.go— chokepoint inaddTypedTool'swrappedHandler. On denial, returns an error result and records a step-up entry in the audit log.internal/mcp/server.go—Options.RequireStepUp+StepUpAPIKeywire config through. UnexportedOptions.stepUplets tests inject a recording fake.internal/cmd/mcp.go—--require-step-upflag (defaults tomcp.require_step_up_for_destructive). When set withtransport=stdio, prints a startup warning that TTY prompts can't reach the operator.internal/config/config.go—mcp.require_step_up_for_destructiveconfig key (defaultfalse),MCPRequireStepUp()getter,ValidConfigKeys+coerceValuehandling.Trade-offs (called out so reviewers can push back)
Executeand (b) forcing in-the-loop pause on destructive calls. Stronger authenticators (Touch ID, push approval) plug into the same interface as Phase 2.--require-step-upis only effective when stdin is a real terminal (e.g. operator-launched--transport httpsession). Documented via startup warning. For the Claude Desktop / stdio flow, an out-of-band authenticator (Slice 3) is the right answer.Behavior matrix
Execute: true?RequireStepUp?Executefield)Test plan
go build ./...cleango test ./...— full suite passingisExecutingDestructiveanddestructiveTargettable-driven across multiple tool input types incl. pointers and nilExecute: truetriggers step-up; denial blocks the underlying API call; plan mode (Execute: false) bypasses; non-destructive tools (noExecutefield) bypass--require-step-updefaults to false)Follow-ups
LocalAuthentication.framework)docs/AUTH.mdsection once PR docs(auth): add docs/AUTH.md — single source of truth (KLA-410) #21 lands🤖 Generated with Claude Code
Note
Medium Risk
Adds a new authorization chokepoint that can block destructive MCP tool executions based on an interactive prompt, affecting runtime behavior when enabled and introducing new failure modes (no TTY/no API key). Default is off, limiting impact to opt-in deployments.
Overview
Adds an opt-in step-up authentication gate for destructive MCP tool calls (any tool input with
Execute: true), prompting the operator on a controlling TTY and failing closed when denied/unavailable.Wires the feature end-to-end via new config
mcp.require_step_up_for_destructive(defaultfalse), server options (RequireStepUp,StepUpAPIKey), and a newjc mcp serve --require-step-upflag (including startup warnings forstdiotransport and fail-fast if no API key is available).Implements the gate in
addTypedToolso all destructive tools share a single chokepoint, and adds comprehensive unit/integration tests for the authenticator, reflection-based destructiveness detection, and enforcement behavior.Reviewed by Cursor Bugbot for commit 3037565. Bugbot is set up for automated code reviews on this repo. Configure here.