Proposal: optional endpoint-bound preflight hook before remote MCP calls #449

Activity

1. Hotragn commented on Sep 14, 2026

   

   Hotragn

   on Sep 14, 2026

   Contributor

   More actions

   This has sat a week with no reply, so here is a read from someone who has been working in this exact function. I am not a maintainer and cannot tell you whether it is welcome — but four of the details are checkable against the source, and I checked them, because they change what the PR would have to be.

   The slot you want exists, and it is not where you cite it

   store.ts#L2799 is where row and entry are in hand, but the order you describe lands further down. On main today, callTool runs:

   L2891	decide("mcp", ...) — the grant
   L2967	evaluateActionPolicy — the policy
   L3038	inspectToolArguments — the content check
   L3078	connectionTokenFor — credentials resolved
   L3079-L3082	transportFor(entry).callTool, effectiveUrl(row, entry)

   So "after the content check, before credentials" is a single seam between L3061 and L3077, and it is clean: nothing between them touches the vault or the network.

   Your concurrency worry is already answered by that shape, which is worth knowing because it removes work from your PR. row and entry are locals captured before any of these awaits, and effectiveUrl and transportFor are both pure functions of them. So a hook handed those same two objects cannot be given evidence for endpoint A and then dial endpoint B — provided it does not re-read the server row itself. "Capture the selection once" is therefore a constraint on the hook's signature (pass row/entry, never serverId), not something needing a new capture step.

   The audit contract is not quite what you describe, and this is the real design question

   You write that the current contract is "a refused invocation records one refusal". It is narrower than that: every refusal on this path records one row of the same type, mcp.call_rejected. The content check at L3040-L3054 is already a second, distinct reason wearing that label, told apart only by a refusal: "sensitive_tool_arguments" field inside the payload.

   That matters because the Audit screen's Blocked tab filters on REFUSED_EVENT_TYPES (app/src/routes/_authed/admin/audit.tsx:53) — by event type, not by payload. A preflight refusal added as a third mcp.call_rejected would therefore appear on that page as this deployment's boundary refusing the call, when what actually happened is that a third-party service declined to vouch for an endpoint. Those need different answers from an operator: one is "fix your rule", the other is "your evidence provider is down, or wrong, or being gamed". Conflating them is the kind of thing this trail is specifically built not to do.

   I would put a distinct event type in the proposal — the taxonomy is a plain union in server/src/audit.ts and adding one is cheap — rather than a fourth payload discriminator.

   It needs a dry-run mode on day one, and the reason is written above your seam

   Read the comment at L2997-L3020. evaluateActionPolicy returns allowed: false, forward: true in dry-run so an operator can measure a rule against live traffic before it refuses anybody, and that surface had a bug where dry-run rejections were not recorded at all — so the Blocked page told operators "this rule would refuse none of your tool calls" about calls it would refuse, the rule looked inert, and enforcing it then broke Bots with no warning in the trail.

   A deny-only hook with no equivalent repeats that exactly. The first thing an operator would learn about their provider's false-positive rate is Bots being refused in production, on evidence from a service the operator does not run. Whatever forward means for the hook, it should mean it before the first PR, not after.

   Fail-open and fail-closed are both bad here, and the default is the whole proposal

   "Unavailable or unknown evidence follows explicit operator policy" is the right mechanism, but I would want the consequences said out loud rather than deferred to config, because they are severe in both directions:

   • Fail-closed puts a third party's uptime and latency on the critical path of every MCP tool call in the deployment. An outage at the provider is an outage of every Bot's tools.
   • Fail-open makes the check inert exactly when it matters most — an endpoint under attack, or a provider being DoSed to get past it.

   This repo's default elsewhere is fail-closed (see #115 and the neutral-binding block at L2943-L2965, where the whole design exists so an unbound identifier cannot throw and refuse everything). A fail-closed default here would be consistent and would also mean enabling the hook makes an outage at any one provider a deployment-wide outage. I think that is defensible, but it should be the stated trade, not a config key.

   This overlaps #350, and they should be decided together

   Disclosure: #350 is mine, so weigh this accordingly. It asks for the evaluator to be pluggable and async at the browser gateway's equivalent boundary. Your hook is a strict subset — an async, deny-only, post-decision plug on nearly the same seam.

   The argument runs both ways and I do not think it favours mine. A general PDP seam invites arbitrary operator code into the deny path with no bound on what it can see; yours is narrower, cannot override an existing refusal, and gets nothing but a public URL, which is a much easier thing to reason about. But the store should not grow two extension points with different lifecycles and different fail-open semantics because the issues were read in different weeks. Worth a maintainer looking at both in one sitting.

   One thing I could not check

   "Initial scope would be explicitly configured public MCP endpoints; private or credential-bearing URLs would not be disclosed" is the load-bearing privacy claim, and the hook would be reading effectiveUrl(row, entry) (L349-L355), which returns resolveServerUrl(row.id)?.url ?? row.url — a value an administrator typed. Nothing in the type marks one of those as public. So "public" has to be an explicit per-server flag the operator sets, and the hook has to refuse to run rather than fall back when the flag is absent; it cannot be inferred from the URL. Worth saying in the proposal, because the obvious implementation is a hostname heuristic and that is how a private endpoint gets disclosed once.
2. AgentTanuki commented on Sep 16, 2026

   

   AgentTanuki

   on Sep 16, 2026

   Author

   More actions

   Thanks for the detailed review. I checked current main at 93bb0d122bb43ca5a83d50797ee45f2e4cbae483 and would tighten the proposal as follows:

   • Keep the seam after inspectToolArguments(vendorArgs) and before connectionTokenFor. The current dispatch uses transportFor(access.transport), so the optional check must apply only to the selected MCP transport. Resolve its URL once and reuse that exact value for the check and execution; the provider receives a minimal immutable descriptor, not the database row, credentials or arguments.
   • Require explicit per-server permission to disclose that exact endpoint to the configured provider. A hostname heuristic is insufficient. Without that permission, no provider request is made. An operator who requires evidence must configure disclosure before enabling the protected route.
   • Give evidence decisions a distinct audit event with separate observation, enforcement mode and actual forwarding fields. An unavailable observation must remain distinguishable from an adverse observation and from an existing policy refusal. In dry-run, record what would have been refused and preserve the real vendor outcome; dry-run cannot bypass the existing grant, policy or content checks.
   • For explicitly protected calls in enforcement mode, missing/unknown evidence would refuse within the configured timeout. That puts provider availability on those calls' critical path. Dry-run measures the impact before enforcement; an unchecked call must never be labelled evidence-approved.

   I agree that this and #350 need a joint architecture decision before adding an extension point. This remains a proposal pending maintainer direction; I have not implemented or tested this revised design. Agent Guild preflight would supply unsigned endpoint observations, not ownership or safety certification.
3. Hotragn commented on Sep 27, 2026

   

   Hotragn

   on Sep 27, 2026

   Contributor

   More actions

   Sorry for the slow reply. The revised shape answers everything I raised, and the two changes I care most about — a distinct event type rather than a fourth payload discriminator, and dry-run present from the first PR rather than added after — are the ones that make this reviewable at all. Nothing further from me on the design.

   Three practical notes, since store.ts has moved a long way since I gave you those line numbers and I would rather you did not chase stale ones.

   The seam is still there and still clean, at different coordinates. callTool has grown by roughly four thousand lines of file above it. On main at ca8d51c:

   inspectToolArguments(vendorArgs)	:7085
   end of the content-refusal block	:7108
   try {	:7117
   connectionTokenFor(row, entry, input.actorId, access)	:7125
   transportFor(access.transport).callTool	:7132
   effectiveUrl(row, entry)	:7135

   Your slot is the gap between :7108 and :7117, and it is still true that nothing in it touches the vault or the network.

   access.transport is the discriminator you were reaching for. When I first read this the selection was derived at the call site; it is now resolved once by requireServer into an access object and read as transportFor(access.transport). So "apply only to the selected MCP transport" is a property of a value that already exists and is already the thing dispatch uses, rather than something the hook has to re-derive — which also removes the last of the A-authorises-B concern, since the hook and the dispatch would be reading the same field of the same object.

   A new event type now has a test that will hold you to it. I added a guard in #540 — server/tests/audit.test.ts:53, "declares nothing this deployment cannot write" — which reads every entry of auditEventTypes off the sources in server/src and fails if one has no literal writer. It exists because four declared types had outlived their writers and an operator filtering for one got an empty page that reads as this did not happen rather than this cannot happen.

   For you that means the type and the code that writes it have to land in the same commit; you cannot declare mcp.preflight_refused ahead of the implementation and fill it in later. It also means the writer must pass the type as a literal string — a computed event type fails the guard. Worth knowing before you stage the PR, because the natural way to write this is a small map from outcome to event type, and that is fine as long as the values are literals.

   One thing I would add to the enforcement-mode paragraph. You have accepted that provider availability lands on the critical path, which I think is the right call to make explicitly rather than bury. The part worth pinning is that the timeout has to be enforced on the store's side of the call, not delegated to the provider adapter. A provider that refuses inside the deadline is the case you have described; a provider that accepts the connection and never answers is a different one, and if the deadline lives in the adapter then a hung socket stalls somebody's turn instead of refusing it. Same distinction bit the escalation route in #541 — a route that throws was handled and a route that hangs still is not — so it is worth being the one seam where it is handled from the start.