feat: expose a hook to set readonly at execution time#1275
Conversation
There was a problem hiding this comment.
Pull request overview
Adds execution-time authorization and read-only enforcement so tool availability can be decided per-request/per-target (even when the server is initialized as writable), and introduces a shared MaybePromise<T> type to simplify sync/async callback signatures across the API surface.
Changes:
- Add a hosting-injected
authorizeToolExecutionhook, invoked at the top ofToolBase.invoke(), to allow/deny each tool call with a reason. - Add execution-time write guarding via
isEffectivelyReadOnly()/assertWriteOperationAllowed(), with MongoDB tools additionally honoring a connection-scopedreadOnlyflag. - Introduce
MaybePromise<T>in@mongodb-js/mcp-typesand update multiple callback signatures + public API extractor reports; add unit tests for the new behaviors.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/tools/mongodb/mongodbTool.test.ts | New unit tests covering connection-scoped effective read-only and $out pipeline rejection. |
| tests/unit/toolBase.test.ts | Adds unit tests for the authorizeToolExecution hook and execution-time read-only write guard. |
| tests/unit/toolAuthorization.test.ts | Verifies Server threads the authorizer into tool instances and blocks denied calls. |
| src/web.ts | Re-exports ToolExecutionAuthorizer from the web entrypoint. |
| src/ui/registry/registry.ts | Switches customUIs callback type to MaybePromise<string | null>. |
| src/transports/base.ts | Threads authorizeToolExecution through transport server options; updates CreateSessionConfigFn to MaybePromise. |
| src/tools/tool.ts | Implements ToolExecutionAuthorizer, execution-time write guard, and MaybePromise error handler typing. |
| src/tools/mongodb/mongodbTool.ts | Overrides effective read-only to include connection-state readOnly; uses it for $out/$merge stage guarding. |
| src/tools/mongodb/metadata/collectionStorageSize.ts | Updates handleError return typing to MaybePromise. |
| src/tools/mongodb/metadata/collectionIndexes.ts | Updates handleError return typing to MaybePromise. |
| src/tools/index.ts | Re-exports ToolExecutionAuthorizer. |
| src/tools/atlasLocal/atlasLocalTool.ts | Updates handleError return typing to MaybePromise. |
| src/tools/atlas/streams/streamsToolBase.ts | Updates handleError return typing to MaybePromise. |
| src/tools/atlas/atlasTool.ts | Updates handleError return typing to MaybePromise. |
| src/server.ts | Adds authorizeToolExecution to ServerOptions and forwards it into tool constructors. |
| src/resources/resource.ts | Updates ReactiveResource.toOutput() return typing to MaybePromise<string>. |
| src/lib.ts | Re-exports ToolExecutionAuthorizer from the library entrypoint. |
| src/common/managedTimeout.ts | Uses MaybePromise<void> for managed timeout callbacks. |
| src/common/connectionManager.ts | Adds readOnly?: boolean to ConnectionSettings and persists it in connection state. |
| src/common/connectionErrorHandler.ts | Updates handler return type to MaybePromise. |
| packages/types/src/ui.ts | Updates types package UI registry option signature to MaybePromise. |
| packages/types/src/helpers.ts | Introduces MaybePromise<T> helper type. |
| api-extractor/reports/web.public.api.md | Updates public API snapshot for new types/signatures. |
| api-extractor/reports/ui.public.api.md | Updates public API snapshot for MaybePromise UI registry signature. |
| api-extractor/reports/tools.public.api.md | Updates public API snapshot for tool authorizer and MaybePromise signatures. |
| api-extractor/reports/mongodb-mcp-server.public.api.md | Updates public API snapshot for server options/connection state read-only and MaybePromise. |
| // a separate field for elicitation time in the future. | ||
| startTime = Date.now(); | ||
| } | ||
|
|
||
| this.assertWriteOperationAllowed(); |
| protected override isEffectivelyReadOnly(): boolean { | ||
| const connectionState = this.session.connectionManager.currentConnectionState; | ||
| return ( | ||
| super.isEffectivelyReadOnly() || (connectionState.tag === "connected" && connectionState.readOnly === true) |
There was a problem hiding this comment.
I think this change in the interface doesn't really address what we need for remote MCP: we need a way to make sure that we are providing a tool with the readOnly state during the tool call itself, not when connection is being established. This is also similar to the same behavior we need for compass where the readOnly flag can change at any arbitrary point during the session, so if we apply the value during connect, it might be already out of sync by the time the tool call using said connection happen.
Maybe we can do our own override of the isEffectiveReadOnly as a way to achieve this, but then the method is missing some crucial context like invoke args for us to be able to derive the state, sync method is also a no-go if we need to run async checks
There was a problem hiding this comment.
This is for data-plane tools though where I think we want to set it at connection time, don't we - at least that's when we know what the project and cluster are, so we can make a decision whether the connection should be readonly or not.
There was a problem hiding this comment.
I think just "connection time" is a wrong way of thinking about this, it does kinda cover the remote mcp case, but almost by accident, just by the nature of very specific connection flow we have at the moment. As I mentioned, if you look at a slightly more generalised case of mcp server being used in Compass desktop, this falls apart right away.
I'm also concerned that there are no guarantees that another tool won't be introduced in the future that announces itself as read-only, but does writes if the config is not set, and is not db-related, so has no access to this connection state.
| let startTime: number = Date.now(); | ||
|
|
||
| try { | ||
| const toolExecutionAuthorized = await this.authorizeToolExecution?.({ |
There was a problem hiding this comment.
This interface does partially address our needs, but not fully (see my other comment): in addition to just straight up denying the invocation we need to be able to set some sort of invoke scoped config that should take precedence over the other ones
There was a problem hiding this comment.
What do you envision going into the invoke-scoped config? That was originally my approach but then didn't like the design as it was too intrusive (configs go into many places, so making sure that they're all updated per each invocation was quite a bit more involved). I guess in a future where we allow control of more parameters than read-only, we could want some form of config merging, but I didn't want to overengineer for a future that may not come.
There was a problem hiding this comment.
I think if there is a case that invoke relies on configs for critical behavioral differences and we know that config can change dynamically at very different points in the server lifetime, then anything critical from config that can be used during invoke should go into this invoke-scoped config probably. Maybe right now it's only readOnly, I haven't looked through the actual invokes / execute code to know of any other instance. Also maybe config (as in the same config interface we pass to the constructor) shouldn't be the same config completely but a different thing altogether.
Understandable that the way config is being accessed in the mcp server right now makes it a messy endeavour to introduce another "scoping level" here, but this doesn't seem like a great reason to reject this approach.
If that's the only way of control we're willing to introduce though I think we should seriously consider if that's a good idea in the first place to have seemingly "read" tools be allowed write operations. Maybe it's a matter of moving those agg stages to be only allowed in some special, explicitly write tool calls that don't depend on the config.
Proposed changes
Adds execution-time, per-target read-only enforcement so a single server (initialized readOnly: false) can still reject writes against read-only targets. Two cooperating mechanisms: a generic, hosting-injected ToolExecutionAuthorizer hook (run at the top of ToolBase.invoke()) that lets the host allow/deny each call — e.g. mapping an Atlas tool's orgId/projectId arg to that org's policy — and a built-in connection-scoped read-only flag stashed on ConnectionStateConnected that data-plane tools honor via isEffectivelyReadOnly(), gating write operations and the $out/$merge aggregation guard.
To integrate into hosted MCP services:
Pass
authorizeToolExecutiontocreateServer:authorizeToolExecutionis invoked on every tool call and allows integrators to reject the execution based on the arguments, tool name, session state, etc.In the ConnectionManager implementation, pass
readOnly: true | falseto theconnectmethod:The connection readonly flag is stored on the state and is checked in all
mongodbtools, where we compute an "effective" readonly configuration based on the connection state and the config.readOnly flag.