Repository navigation
mcp_server: bind approval retries to the original action - #848
Conversation
The safe and delete_safe approval gate read inputResponses.stackql_approval before checking elicitation support and never tied the answer to a prompt, so a first call carrying an unsolicited accept executed a gated mutation. The approval prompt now carries a RequestState holding the tool name, a SHA-256 digest of the canonical arguments and SQL, the session id, a nonce and a 10 minute expiry, authenticated with HMAC-SHA256 under a server key. On the retry the gate checks elicitation support first, then rejects an answer with missing, altered, expired or replayed state, or state that no longer matches the call, before the answer is read. The backend is not called on rejection. The SDK echoes the state on its server-side round trip for pre-2026-07-28 clients, so both lifecycles take the same path. The key is random per process. server.approval_key_env_var names an env var holding a shared key for sessionless Streamable HTTP behind several instances, read at startup like auth_token_env_var and kept out of logs and the audit sink. A per-process used-nonce set blocks replay. Across instances replay is bounded by the expiry, which the docs state next to the note that approval trusts a cooperating client.
jeffreyaven
left a comment
There was a problem hiding this comment.
Looks good, this is the design from the issue and the file footprint is what it needs to be. Threading the binder through the register helpers the same way as auditSink is fine.
One change before merge: the README additions duplicate docs/mcp.md almost verbatim, and the Server Modes paragraph is a wall of text. Cut it to a sentence or two that says the prompt carries a signed requestState the client echoes, mismatches are refused before the backend runs, and point at docs/mcp.md and approval_key_env_var for the rest.
The choices you flagged are all fine as they stand: needs_approval_declined for a rejected state, the fixed 10 minute expiry, and single-use state for every answer. Leave them.
Minor, optional: the approvalOutcome doc comment could lose a few lines now that the behaviour is documented in docs/mcp.md.
Wait until the CI checks pass from the current run, once green push the updates
The 2026-07-28 robot helpers answered the approval prompt with inputResponses only. Since stackql#827 the server refuses an answer that does not carry the signed requestState from the prompt, so the six current revision gated write tests got "no approval state was echoed with the answer" instead of the mutation result or the declined error. run_stdio_stateless_roundtrip and run_http_stateless_gated_write now copy result.requestState into the retry params, as a conforming client does. The legacy elicitation helpers need no change because the SDK echoes the state on its server side round trip. Signed-off-by: Tymofii Pidlisnyi <tima@aeoess.com>
The README repeated docs/mcp.md almost verbatim. The Server Modes paragraph now says in two sentences that the prompt carries a signed requestState the client echoes and that mismatches are refused before the backend runs, and points at docs/mcp.md and approval_key_env_var for the rest. The approval_key_env_var block under HTTP client authentication is removed for the same reason. The approvalOutcome doc comment is shortened to match. Signed-off-by: Tymofii Pidlisnyi <tima@aeoess.com>
|
@jeffreyaven the CI failures were mine. Two test clients retried without sending back Fixed in All six previously failing robot tests now pass locally, along with |
|
All green. Merging, thanks for the quick turnaround on the robot helpers. |
|
@jeffreyaven thanks for the review, and for the deployment details on #827. They shaped the design. |
What problem this solves
Fixes #827. In
safeanddelete_safemode the approval gate readinputResponses.stackql_approvalbefore it checked elicitation support, and nothing tied the answer to an earlier prompt. A client could send an accept on the first call, with no prompt and no elicitation capability, and the gated mutation ran. The same gap let a client reuse an old accept, or change the SQL or tool after the user approved something else.Why this change
I followed the design agreed in the issue. The approval prompt now carries a
requestStatewith the tool name, a SHA-256 digest of the canonical arguments and SQL, the session id where there is one, a nonce and a 10 minute expiry, authenticated with HMAC-SHA256 under a server key. On the retry the gate checks elicitation support first. Then it rejects an answer with missing, altered, expired or replayed state, or with state that no longer matches the call, before it reads the answer. The backend is not called on rejection. The Go SDK echoes the state itself on its server-side round trip for clients older than 2026-07-28, so both lifecycles go through the same check.The key is random per process by default, which fits stdio, the primary transport, with one process per client. For sessionless Streamable HTTP behind several instances,
server.approval_key_env_varnames an env var holding a shared key of at least 32 bytes. It is read at startup likeauth_token_env_varand kept out of logs and the audit sink. stdio reports an empty session id, so there the per-process key carries the binding. A per-process set of used nonces blocks replay. Across instances replay is bounded only by the expiry, and I documented that next to the note that approval trusts a cooperating client. This hardens the workflow against stale, altered or unsolicited answers. It does not prove a person saw the prompt.User impact
Elicitation-capable clients see no change, because the SDK echoes
requestStateon both lifecycles. A client that drives the 2026-07-28 retry by hand must echorequestState, as the spec already requires. An accept from a client that never advertised elicitation is now refused. Single-instance deployments need no configuration. Multi-instance sessionless HTTP deployments should setapproval_key_env_varto the same value on every instance, otherwise a retry that lands on another instance is refused.Evidence
go build ./...,go vet ./pkg/mcp_server/...andgolangci-lintv2.5.0 on the package are clean.go test ./pkg/mcp_server/... -racepasses, andgo test ./...passes.The new tests cover the acceptance list in the issue:
Each rejection asserts the backend did not run. The three unsolicited-accept tests fail on main, where the mutation executes. Disabling the state check makes seven of the new tests fail, and moving the elicitation check back after the answer makes the two elicitation tests fail. I did not run the robot functional suite.
Choices for your review
A rejected state is recorded as
needs_approval_declinedwith the reason in the error text, since the OTel mapping enumerates decision values. The expiry is fixed at 10 minutes, decline and cancel answers also need valid state, and the state is single use for any answer. The binder is passed as a parameter, which touches the register helpers inserver.goandquery_library.go. Happy to change any of these.