lizhimins commented on PR #4560:
URL:
https://github.com/apache/rocketmq-dashboard/pull/4560#issuecomment-5812251395
Closing this one: the premise it guards against no longer exists on the
current baseline, and the guard itself contradicts a design decision that is
documented in the code.
**1. The premise is false on trunk.** The PR adds a bypass in
`ToolCapabilityFilter.filter` for the case where `context.instanceId() == null`
and the tool is instance-id-exempt, on the basis that both entry points pass
`null` for exempt tools. They do not.
`ToolExecutionService.resolveTargetInstance`
(`server/src/main/java/org/apache/rocketmq/studio/ops/ai/tool/service/ToolExecutionService.java:139-152`)
computes `boolean exempt = ToolCatalog.isInstanceIdExempt(definition.name())`
at `:144` and, when the tool takes no `instanceId` argument, returns the
transport-bound instance at `:148`:
```java
if (exempt) {
return targetInstanceId == null || targetInstanceId.isBlank() ? null :
targetInstanceId;
}
```
Its own Javadoc says why: exempt tools' "Instance comes from the transport
binding (the signed MCP header, or the target the console operator selected) so
that the capability gate still has something to resolve against". So an exempt
tool normally arrives at the gate *with* an instance, and the `null` case is
the exceptional one — not the routine one this PR assumes.
**2. The guard weakens a gate that is deliberately closed.**
`ToolCapabilityFilter`'s class Javadoc (`:33-38`) states the intent explicitly:
> Platform-level tools (`ToolCatalog.isInstanceIdExempt`) take no
`instanceId` argument, but the gate is deliberately *not* skipped for them … An
exempt tool therefore still runs only against an Instance that supports it, and
an exempt tool with no binding at all fails loudly with
`CAPABILITY_INSTANCE_REQUIRED` instead of running unscoped.
That error is real and reachable (`CapabilityResolver.java:41` throws
`ToolError.CAPABILITY_INSTANCE_REQUIRED`, defined at `ToolError.java:63`).
Skipping the capability check whenever `instanceId` is null turns "fail loudly"
into "run unscoped", i.e. it removes the only signal that an exempt tool was
invoked without a resolvable instance.
The same Javadoc also records why the binding cannot leak data: "Exempt
handlers never read `context.instanceId()` — only this gate and
`ToolAuditFilter` do — so binding one cannot widen the data a platform-level
tool returns."
If you are hitting a concrete path where an exempt tool reaches the gate
with no binding and you need it to succeed, please open an issue with the
reproduction (which transport, which tool, which auth state) — the fix would be
to bind the instance on that path, as `resolveTargetInstance` already does for
the MCP and console paths, rather than to exempt the path from the gate. No
rework is expected on this PR as it stands.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
