RockteMQ-AI commented on PR #2370: URL: https://github.com/apache/rocketmq-dashboard/pull/2370#issuecomment-5331199678
### 🔍 Automated Code Review The PR correctly shifts reader AI-tool authorization from a blanket POST whitelist to per-tool catalog metadata and hardens the AI page for non-admins. A few issues need attention before merging. ### Issues **`server/src/main/java/org/apache/rocketmq/studio/ops/ai/tool/ToolAccessPolicy.java:58`** — **Critical** `URLDecoder.decode(encodedName, StandardCharsets.UTF_8)` can throw `IllegalArgumentException` for malformed percent-encoding (e.g. `%ZZ`). This would surface as a 500 instead of a controlled 403/404. Wrap the decode in a try/catch and return `Optional.empty()` on failure. **`server/src/test/java/org/apache/rocketmq/studio/auth/AuthInterceptorTest.java`** — **Warning** The diff excerpt shows file paths such as `@repos/apache_rocketmq/auth/src/test/java/org/apache/rocketmq/auth/authentication/AuthenticationEvaluatorTest.java` where `@Test` annotations should appear. Verify this is only a rendering artifact and the actual source file does not contain stray paths. **`server/src/main/java/org/apache/rocketmq/studio/ops/ai/tool/ToolGatewayService.java:54`** — **Warning** The constructor signature changed by inserting `ToolAccessPolicy` before `List<ToolHandler>`. Check for any other manual instantiations in integration tests, configuration classes, or other modules that now need to be updated. **`server/src/main/java/org/apache/rocketmq/studio/ops/ai/tool/ToolGatewayService.java:118`** — **Suggestion** `toolAccessPolicy.authorizeCurrentUser(definition)` throws `BusinessException(403, ...)`. Confirm the global exception handler maps a `BusinessException` with code 403 to HTTP 403; otherwise readers may receive 500 on unauthorized execution. **`server/src/main/java/org/apache/rocketmq/studio/ops/ai/tool/ToolAccessPolicy.java:74-77`** — **Suggestion** The blank-name check is performed on the encoded substring. Validate the decoded `toolName` as well, so a decoded empty/blank name also returns `Optional.empty()`. **`server/src/main/java/org/apache/rocketmq/studio/auth/AuthInterceptor.java:113-116`** — **Suggestion** `requiresAdmin` now resolves the tool from the catalog on every non-GET request under `/api/ai/tools/`. If `catalog.find` is not an in-memory lookup, consider caching reader-accessible tool paths to avoid a per-request catalog search. **Tests** — **Suggestion** Add coverage for: - Malformed percent-encoded tool names in the interceptor. - Unknown/missing tools in the catalog (interceptor should require admin). - Reader attempts to execute `rmq.message.query` and `rmq.message.trace` returning 403. - Reader `discover` only returning L1 read-only tools. ### Summary The server-side authorization logic is sound and safely falls back to admin-only when `ToolAccessPolicy` is unavailable. Fix the `URLDecoder` exception handling and verify the test-file rendering artifact, then this should be good to merge. --- <sub>🤖 Generated by github-manager | Review dimensions: correctness, performance, tests, compatibility</sub> -- 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]
