feat(mcp): add allowedTools filtering support to OneMcpServer#10811
Conversation
2935ad3 to
a4d74e7
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces tool filtering support to OneMcpServer via a new allowedTools configuration option, filtering the tools returned by listTools() and checking permissions in callTool(). The review feedback points out that callTool() currently only logs a warning when a disallowed tool is invoked instead of blocking execution. To properly guard the execution and adhere to the repository style guide, the reviewer recommends throwing a FirebaseError when a disallowed tool is called, and updating the corresponding unit tests to assert this error behavior.
I am having trouble creating individual review comments. Click here to see my feedback.
src/mcp/onemcp/onemcp_server.ts (120-128)
The implementation currently only logs a warning when a disallowed tool is called, and then proceeds to execute the tool anyway. According to the PR description, callTool() is supposed to guard against invoking tools not present in allowedTools. To properly guard and prevent execution, we should throw a FirebaseError as specified in the repository style guide for expected, user-facing errors.
if (
this.options.allowedTools?.length &&
!this.options.allowedTools.includes(toolName) &&
!this.options.allowedTools.includes(this.feature + "_" + toolName)
) {
throw new FirebaseError(
"Tool '" + toolName + "' is not allowed on remote server for feature '" + this.feature + "'."
);
}
References
- Throw FirebaseError (src/error.ts) for expected, user-facing errors. (link)
src/mcp/onemcp/onemcp_server.spec.ts (234-252)
Since callTool should throw a FirebaseError when a disallowed tool is called, this test should be updated to assert that the promise is rejected with a FirebaseError instead of checking for a logged warning.
it("should throw FirebaseError when calling a tool not in allowedTools", async () => {
const serverWithFilter = new OneMcpServer(
feature,
serverUrl,
{ requiresAuth: false, requiresProject: true },
{ allowedTools: ["allowed_tool"] },
);
const fn = (serverWithFilter as any).callTool.bind(serverWithFilter);
await expect(fn("disallowed_tool", {}, mockContext)).to.be.rejectedWith(
FirebaseError,
/is not allowed on remote server/
);
});| @@ -1 +1,2 @@ | |||
| - Add `MCP-Protocol-Version`, `Mcp-Method`, and `Mcp-Name` HTTP headers to `OneMcpServer` requests per the MCP 0728 standard release candidate (https://blog.modelcontextprotocol.io/posts/2026-07-28-release-candidate/ and https://modelcontextprotocol.io/seps/2243-http-standardization). | |||
| - Add tool filtering support to `OneMcpServer` via `allowedTools` configuration option. | |||
There was a problem hiding this comment.
Omit the CHANGELOG for this one - its not a user facing change, but rather an implementation detail
joehan
left a comment
There was a problem hiding this comment.
LGTM, just remove the changelog
### Description Adds support for tool filtering in `OneMcpServer` via an optional `allowedTools?: string[]` parameter in `OneMcpServerOptions` (addressing b/535744232). - `listTools()` filters remote tool list responses against `allowedTools`. - `callTool()` guards against invoking tools not present in `allowedTools`. ### Scenarios Tested - Unit tests in src/mcp/onemcp/onemcp_server.spec.ts verifying `listTools` filters tools when `allowedTools` is configured. - Unit tests in src/mcp/onemcp/onemcp_server.spec.ts verifying `callTool` throws a `FirebaseError` for unallowed tools. - Unit tests verifying backward compatibility when `allowedTools` is omitted or empty. ### Sample Commands npx mocha --require ts-node/register src/mcp/onemcp/onemcp_server.spec.ts
a4d74e7 to
f40bcb6
Compare
Description
Adds support for tool filtering in
OneMcpServervia an optionalallowedTools?: string[]parameter inOneMcpServerOptions(addressing b/535744232).listTools()filters remote tool list responses againstallowedTools.callTool()guards against invoking tools not present inallowedTools.Scenarios Tested
listToolsfilters tools whenallowedToolsis configured.callToolthrows aFirebaseErrorfor unallowed tools.allowedToolsis omitted or empty.Sample Commands
npx mocha --require ts-node/register src/mcp/onemcp/onemcp_server.spec.ts