BE: MCP: Enforce cluster readOnly restriction for write operations#1764
Closed
paikend wants to merge 4 commits intokafbat:mainfrom
Closed
BE: MCP: Enforce cluster readOnly restriction for write operations#1764paikend wants to merge 4 commits intokafbat:mainfrom
paikend wants to merge 4 commits intokafbat:mainfrom
Conversation
MCP tool calls bypass the ReadOnlyModeFilter because they arrive via
POST /mcp/message instead of /api/clusters/{clusterName}/... paths.
This adds a readOnly check in McpSpecificationGenerator.methodCall()
that inspects the @RequestMapping annotation on the generated API
interface to determine whether an operation is a write (POST/PUT/PATCH/
DELETE). Write operations targeting a readOnly cluster now return a
ReadOnlyModeException error result.
Safe operations (analyzeTopic, cancelTopicAnalysis, registerFilter)
are exempted, consistent with ReadOnlyModeFilter.SAFE_ENDPOINTS.
Closes kafbat#1751
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kapybro
Bot
added
status/triage
📝 WalkthroughWalkthroughThe MCP dispatch classifies operations as write vs read-only and enforces cluster read-only status before invoking write operations, returning a ReadOnlyModeException-derived error when the target cluster is read-only. Changes
Sequence Diagram(s)sequenceDiagram
actor Client as MCP Client
participant Gen as McpSpecificationGenerator
participant Store as ClustersStorage
participant Controller as Controller Method
Client->>Gen: invoke tool (operationId, args)
Gen->>Gen: isWriteOperation(operationId)?
alt Write operation
Gen->>Gen: extract `clusterName` from args
Gen->>Store: getClusterByName(clusterName)
Store-->>Gen: Optional<KafkaCluster>
alt Cluster found and isReadOnly == true
Gen-->>Client: return ReadOnlyModeException error
else Cluster absent or not read-only
Gen->>Controller: invoke controller method with args
Controller-->>Gen: result
Gen-->>Client: result
end
else Read operation
Gen->>Controller: invoke controller method with args
Controller-->>Gen: result
Gen-->>Client: result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi paikend! 👋
Welcome, and thank you for opening your first PR in the repo!
Please wait for triaging by our maintainers.
Please take a look at our contributing guide.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
api/src/test/java/io/kafbat/ui/service/mcp/McpSpecificationGeneratorTest.java (2)
41-43: Static mock unused in new tests; consider removing or utilizing.The static
CLUSTERS_STORAGEmock at line 41 is injected into the staticMCP_SPECIFICATION_GENERATORbut is never stubbed. The new tests create their own localClustersStoragemocks with proper stubbing. This is fine for test isolation, but the static mock remains unused and could be confusing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/test/java/io/kafbat/ui/service/mcp/McpSpecificationGeneratorTest.java` around lines 41 - 43, The static CLUSTERS_STORAGE mock is declared but never stubbed or used and it is injected into the static MCP_SPECIFICATION_GENERATOR; remove the unused static CLUSTERS_STORAGE and stop using a static MCP_SPECIFICATION_GENERATOR, or instead make MCP_SPECIFICATION_GENERATOR non-static and initialize it per-test with the locally created/stubbed ClustersStorage mocks used by the tests (reference symbols: CLUSTERS_STORAGE, MCP_SPECIFICATION_GENERATOR, McpSpecificationGenerator) so each test uses its own properly stubbed ClustersStorage instance.
116-210: Consider adding tests for additional edge cases.The current tests cover the primary scenarios well. For more comprehensive coverage, consider adding tests for:
- Read operation (e.g.,
getTopics) on a read-only cluster — should succeed- Safe write operations (
analyzeTopic,cancelTopicAnalysis) on a read-only cluster — should succeed perREAD_ONLY_SAFE_OPERATIONS- Write operation with unknown cluster name — should proceed (current behavior defaults to
orElse(false))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/test/java/io/kafbat/ui/service/mcp/McpSpecificationGeneratorTest.java` around lines 116 - 210, Add unit tests to cover the edge cases described: create a test that invokes the getTopics AsyncToolSpecification (tool().name() == "getTopics") against a KafkaCluster with readOnly=true and assert the call proceeds without a "read-only" error; create tests that invoke the analyzeTopic and cancelTopic AsyncToolSpecifications against a readOnly=true cluster and assert they succeed (these correspond to READ_ONLY_SAFE_OPERATIONS in McpSpecificationGenerator); and add a test that calls a write operation (e.g., createTopic) with args.clusterName set to an unknown cluster (ClustersStorage.getClusterByName returns Optional.empty()) and assert the behavior matches current default (i.e., the readOnly check does not block or it proceeds with orElse(false) semantics). Ensure each test locates the AsyncToolSpecification from generator.convertTool(topicsController) by tool().name() and exercises call().apply(...) with a mocked McpAsyncServerExchange and ServerWebExchange, verifying CallToolResult as in the existing tests.api/src/main/java/io/kafbat/ui/service/mcp/McpSpecificationGenerator.java (2)
89-99: Consider handling missing or invalidclusterNamefor write operations.The current logic only enforces the read-only check when
clusterNameis present and is aString. If a write operation is called without aclusterNameargument (or with a non-String value), it bypasses this check entirely.While most write operations likely require
clusterName, consider whether failing fast for write operations with missing cluster context would be safer:💡 Optional: Fail fast for write operations without valid clusterName
if (writeOperation) { Object clusterName = args.get(CLUSTER_NAME_PARAM); if (clusterName instanceof String cn) { boolean readOnly = clustersStorage.getClusterByName(cn) .map(cluster -> cluster.isReadOnly()) .orElse(false); if (readOnly) { return Mono.just(toErrorResult(new ReadOnlyModeException())); } + } else if (clusterName != null) { + log.warn("Write operation called with non-String clusterName: {}", clusterName.getClass()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/main/java/io/kafbat/ui/service/mcp/McpSpecificationGenerator.java` around lines 89 - 99, The writeOperation branch currently skips the read-only check if args.get(CLUSTER_NAME_PARAM) is missing or not a String; update the logic in McpSpecificationGenerator so that when writeOperation is true and the clusterName is missing or not a valid String you fail fast (return Mono.just(toErrorResult(...))) with an appropriate error (e.g., IllegalArgumentException or a specific validation exception) instead of proceeding, and otherwise continue to call clustersStorage.getClusterByName(cn).map(cluster -> cluster.isReadOnly()).orElse(false) and return toErrorResult(new ReadOnlyModeException()) when readOnly is true.
237-255: Consider using unnamed pattern for unused exception variable.The exception
eat line 251 is caught for logging context but the variable itself isn't used in the message. Per static analysis hints, consider using an unnamed pattern (Java 22+) or simply removing the variable name if only logging context matters.✨ Use unnamed pattern (if Java 22+)
- } catch (RuntimeException e) { + } catch (RuntimeException _) { log.warn("Could not determine HTTP method for operation {}, treating as read-only", operationId); }If not on Java 22+, this is acceptable as-is since the catch provides a safe fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/main/java/io/kafbat/ui/service/mcp/McpSpecificationGenerator.java` around lines 237 - 255, In isWriteOperation (McpSpecificationGenerator) the caught RuntimeException variable e is unused; replace "catch (RuntimeException e)" with an unused-name catch like "catch (RuntimeException ignored)" (or the Java 22 unnamed pattern if your project targets Java 22+) so the intent is clear while preserving the existing log.warn call; locate the catch in isWriteOperation (which calls findAnnotatedMethod and checks RequestMapping) and change only the exception variable name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/src/main/java/io/kafbat/ui/service/mcp/McpSpecificationGenerator.java`:
- Around line 89-99: The writeOperation branch currently skips the read-only
check if args.get(CLUSTER_NAME_PARAM) is missing or not a String; update the
logic in McpSpecificationGenerator so that when writeOperation is true and the
clusterName is missing or not a valid String you fail fast (return
Mono.just(toErrorResult(...))) with an appropriate error (e.g.,
IllegalArgumentException or a specific validation exception) instead of
proceeding, and otherwise continue to call
clustersStorage.getClusterByName(cn).map(cluster ->
cluster.isReadOnly()).orElse(false) and return toErrorResult(new
ReadOnlyModeException()) when readOnly is true.
- Around line 237-255: In isWriteOperation (McpSpecificationGenerator) the
caught RuntimeException variable e is unused; replace "catch (RuntimeException
e)" with an unused-name catch like "catch (RuntimeException ignored)" (or the
Java 22 unnamed pattern if your project targets Java 22+) so the intent is clear
while preserving the existing log.warn call; locate the catch in
isWriteOperation (which calls findAnnotatedMethod and checks RequestMapping) and
change only the exception variable name.
In
`@api/src/test/java/io/kafbat/ui/service/mcp/McpSpecificationGeneratorTest.java`:
- Around line 41-43: The static CLUSTERS_STORAGE mock is declared but never
stubbed or used and it is injected into the static MCP_SPECIFICATION_GENERATOR;
remove the unused static CLUSTERS_STORAGE and stop using a static
MCP_SPECIFICATION_GENERATOR, or instead make MCP_SPECIFICATION_GENERATOR
non-static and initialize it per-test with the locally created/stubbed
ClustersStorage mocks used by the tests (reference symbols: CLUSTERS_STORAGE,
MCP_SPECIFICATION_GENERATOR, McpSpecificationGenerator) so each test uses its
own properly stubbed ClustersStorage instance.
- Around line 116-210: Add unit tests to cover the edge cases described: create
a test that invokes the getTopics AsyncToolSpecification (tool().name() ==
"getTopics") against a KafkaCluster with readOnly=true and assert the call
proceeds without a "read-only" error; create tests that invoke the analyzeTopic
and cancelTopic AsyncToolSpecifications against a readOnly=true cluster and
assert they succeed (these correspond to READ_ONLY_SAFE_OPERATIONS in
McpSpecificationGenerator); and add a test that calls a write operation (e.g.,
createTopic) with args.clusterName set to an unknown cluster
(ClustersStorage.getClusterByName returns Optional.empty()) and assert the
behavior matches current default (i.e., the readOnly check does not block or it
proceeds with orElse(false) semantics). Ensure each test locates the
AsyncToolSpecification from generator.convertTool(topicsController) by
tool().name() and exercises call().apply(...) with a mocked
McpAsyncServerExchange and ServerWebExchange, verifying CallToolResult as in the
existing tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3936a379-7d93-45de-bfa5-674942cca80b
📒 Files selected for processing (2)
api/src/main/java/io/kafbat/ui/service/mcp/McpSpecificationGenerator.javaapi/src/test/java/io/kafbat/ui/service/mcp/McpSpecificationGeneratorTest.java
- Use unnamed pattern for unused exception variable (Java 22+) - Add warning log for non-String clusterName in write operations - Remove unused static ClustersStorage mock, extract test helpers - Add edge-case tests: read op on readOnly cluster, safe write op (analyzeTopic) on readOnly cluster, unknown cluster name Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/src/main/java/io/kafbat/ui/service/mcp/McpSpecificationGenerator.java (1)
85-115: Consider extracting read-only check to reduce cognitive complexity.SonarCloud reports cognitive complexity of 16 (limit: 15). Extracting the read-only validation into a helper method would reduce nesting and improve readability.
Also, a minor style suggestion: use method reference on line 93.
♻️ Proposed refactor to extract helper method
+ private Optional<CallToolResult> checkReadOnlyViolation(Map<String, Object> args) { + Object clusterName = args.get(CLUSTER_NAME_PARAM); + if (clusterName instanceof String cn) { + boolean readOnly = clustersStorage.getClusterByName(cn) + .map(KafkaCluster::isReadOnly) + .orElse(false); + if (readOnly) { + return Optional.of(toErrorResult(new ReadOnlyModeException())); + } + } else if (clusterName != null) { + log.warn("Write operation called with non-String clusterName: {}", clusterName.getClass()); + } + return Optional.empty(); + } + `@SuppressWarnings`("unchecked") private BiFunction<McpAsyncServerExchange, Map<String, Object>, Mono<CallToolResult>> methodCall(Method method, Object instance, boolean writeOperation) { return (ex, args) -> Mono.deferContextual(ctx -> { try { if (writeOperation) { - Object clusterName = args.get(CLUSTER_NAME_PARAM); - if (clusterName instanceof String cn) { - boolean readOnly = clustersStorage.getClusterByName(cn) - .map(cluster -> cluster.isReadOnly()) - .orElse(false); - if (readOnly) { - return Mono.just(toErrorResult(new ReadOnlyModeException())); - } - } else if (clusterName != null) { - log.warn("Write operation called with non-String clusterName: {}", clusterName.getClass()); + Optional<CallToolResult> readOnlyError = checkReadOnlyViolation(args); + if (readOnlyError.isPresent()) { + return Mono.just(readOnlyError.get()); } }Add the import:
import java.util.Optional;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/main/java/io/kafbat/ui/service/mcp/McpSpecificationGenerator.java` around lines 85 - 115, Extract the read-only validation from methodCall into a new helper (e.g., isClusterReadOnly or validateWriteAllowed) that accepts the clusterName/Object and returns a boolean or an Optional/Mono indicating read-only state; replace the inline block (the if (writeOperation) { ... } logic that uses clustersStorage.getClusterByName(...).map(cluster -> cluster.isReadOnly()).orElse(false) and the non-String warning) with a single call to that helper to reduce nesting and cognitive complexity in methodCall, and while refactoring replace the lambda map(cluster -> cluster.isReadOnly()) with a method reference map(Cluster::isReadOnly) to follow the style suggestion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/main/java/io/kafbat/ui/service/mcp/McpSpecificationGenerator.java`:
- Around line 89-101: In McpSpecificationGenerator within the writeOperation
branch, add a runtime check for a missing clusterName
(args.get(CLUSTER_NAME_PARAM) == null) and return an error Mono instead of
silently proceeding; specifically, in the writeOperation conditional around
CLUSTER_NAME_PARAM, if clusterName is null call return
Mono.just(toErrorResult(new IllegalArgumentException("clusterName is required
for write operations"))) (or a project-standard validation exception) so
requests lacking clusterName are rejected before any cluster lookup or
invocation; keep the existing branch that logs non-String values and preserve
the ReadOnlyModeException handling when clusterName is a String.
---
Nitpick comments:
In `@api/src/main/java/io/kafbat/ui/service/mcp/McpSpecificationGenerator.java`:
- Around line 85-115: Extract the read-only validation from methodCall into a
new helper (e.g., isClusterReadOnly or validateWriteAllowed) that accepts the
clusterName/Object and returns a boolean or an Optional/Mono indicating
read-only state; replace the inline block (the if (writeOperation) { ... } logic
that uses clustersStorage.getClusterByName(...).map(cluster ->
cluster.isReadOnly()).orElse(false) and the non-String warning) with a single
call to that helper to reduce nesting and cognitive complexity in methodCall,
and while refactoring replace the lambda map(cluster -> cluster.isReadOnly())
with a method reference map(Cluster::isReadOnly) to follow the style suggestion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d5be563-7f88-4c2d-a613-9fbe4ba122c9
📒 Files selected for processing (2)
api/src/main/java/io/kafbat/ui/service/mcp/McpSpecificationGenerator.javaapi/src/test/java/io/kafbat/ui/service/mcp/McpSpecificationGeneratorTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/test/java/io/kafbat/ui/service/mcp/McpSpecificationGeneratorTest.java
Adds explicit null check for clusterName in write operations instead of silently proceeding to controller invocation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/src/main/java/io/kafbat/ui/service/mcp/McpSpecificationGenerator.java (2)
83-117: Consider extracting read-only check to reduce cognitive complexity.SonarCloud flags cognitive complexity of 17 (allowed: 15). Extracting the read-only validation logic into a helper method would improve readability and satisfy the complexity threshold.
♻️ Suggested refactor to reduce complexity
+ private Optional<CallToolResult> checkWriteOperationAllowed( + boolean writeOperation, Map<String, Object> args) { + if (!writeOperation) { + return Optional.empty(); + } + Object clusterName = args.get(CLUSTER_NAME_PARAM); + if (clusterName instanceof String cn) { + boolean readOnly = clustersStorage.getClusterByName(cn) + .map(KafkaCluster::isReadOnly) + .orElse(false); + if (readOnly) { + return Optional.of(toErrorResult(new ReadOnlyModeException())); + } + } else if (clusterName == null) { + return Optional.of(toErrorResult("clusterName is required for write operations")); + } else { + log.warn("Write operation called with non-String clusterName: {}", clusterName.getClass()); + } + return Optional.empty(); + } + `@SuppressWarnings`("unchecked") private BiFunction<McpAsyncServerExchange, Map<String, Object>, Mono<CallToolResult>> methodCall(Method method, Object instance, boolean writeOperation) { return (ex, args) -> Mono.deferContextual(ctx -> { try { - if (writeOperation) { - Object clusterName = args.get(CLUSTER_NAME_PARAM); - if (clusterName instanceof String cn) { - boolean readOnly = clustersStorage.getClusterByName(cn) - .map(cluster -> cluster.isReadOnly()) - .orElse(false); - if (readOnly) { - return Mono.just(toErrorResult(new ReadOnlyModeException())); - } - } else if (clusterName == null) { - return Mono.just(toErrorResult("clusterName is required for write operations")); - } else { - log.warn("Write operation called with non-String clusterName: {}", clusterName.getClass()); - } + Optional<CallToolResult> blocked = checkWriteOperationAllowed(writeOperation, args); + if (blocked.isPresent()) { + return Mono.just(blocked.get()); } ServerWebExchange serverWebExchange = ctx.get(ServerWebExchange.class);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/main/java/io/kafbat/ui/service/mcp/McpSpecificationGenerator.java` around lines 83 - 117, Extract the write-operation read-only validation from methodCall into a private helper (e.g., checkWriteAllowed or validateClusterWrite) that takes the args Map and returns either Optional<CallToolResult> or a Mono<CallToolResult> (so existing callers can short-circuit when a validation error is present); move the logic that reads CLUSTER_NAME_PARAM, checks instance-of String, looks up clustersStorage.getClusterByName(...).map(cluster -> cluster.isReadOnly()).orElse(false), returns Mono.just(toErrorResult(new ReadOnlyModeException())) when read-only, returns Mono.just(toErrorResult("clusterName is required for write operations")) when clusterName is null, and logs non-String clusterName via log.warn(...) otherwise; then replace the inlined block in methodCall with a single call to that helper and short-circuit if it returns an error result, preserving the existing calls to toErrorResult, ReadOnlyModeException, CLUSTER_NAME_PARAM, clustersStorage, and log.
91-94: Minor: Use method reference for clarity.✨ Proposed improvement
- boolean readOnly = clustersStorage.getClusterByName(cn) - .map(cluster -> cluster.isReadOnly()) - .orElse(false); + boolean readOnly = clustersStorage.getClusterByName(cn) + .map(KafkaCluster::isReadOnly) + .orElse(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/main/java/io/kafbat/ui/service/mcp/McpSpecificationGenerator.java` around lines 91 - 94, The lambda passed to clustersStorage.getClusterByName(...).map(...) uses cluster -> cluster.isReadOnly(); replace it with a method reference to improve clarity: use Cluster::isReadOnly (keeping the surrounding logic intact where clusterName is pattern-matched to cn and assigned to boolean readOnly). Update the map call in McpSpecificationGenerator so it reads .map(Cluster::isReadOnly).orElse(false) while leaving the clusterName instanceof String cn check unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/src/main/java/io/kafbat/ui/service/mcp/McpSpecificationGenerator.java`:
- Around line 83-117: Extract the write-operation read-only validation from
methodCall into a private helper (e.g., checkWriteAllowed or
validateClusterWrite) that takes the args Map and returns either
Optional<CallToolResult> or a Mono<CallToolResult> (so existing callers can
short-circuit when a validation error is present); move the logic that reads
CLUSTER_NAME_PARAM, checks instance-of String, looks up
clustersStorage.getClusterByName(...).map(cluster ->
cluster.isReadOnly()).orElse(false), returns Mono.just(toErrorResult(new
ReadOnlyModeException())) when read-only, returns
Mono.just(toErrorResult("clusterName is required for write operations")) when
clusterName is null, and logs non-String clusterName via log.warn(...)
otherwise; then replace the inlined block in methodCall with a single call to
that helper and short-circuit if it returns an error result, preserving the
existing calls to toErrorResult, ReadOnlyModeException, CLUSTER_NAME_PARAM,
clustersStorage, and log.
- Around line 91-94: The lambda passed to
clustersStorage.getClusterByName(...).map(...) uses cluster ->
cluster.isReadOnly(); replace it with a method reference to improve clarity: use
Cluster::isReadOnly (keeping the surrounding logic intact where clusterName is
pattern-matched to cn and assigned to boolean readOnly). Update the map call in
McpSpecificationGenerator so it reads .map(Cluster::isReadOnly).orElse(false)
while leaving the clusterName instanceof String cn check unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53bafcc9-6400-4f8c-8e62-bcd54794940f
📒 Files selected for processing (2)
api/src/main/java/io/kafbat/ui/service/mcp/McpSpecificationGenerator.javaapi/src/test/java/io/kafbat/ui/service/mcp/McpSpecificationGeneratorTest.java
Author
|
Closing in favor of #1766. Good luck @MukulGhare! 🙌 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1751
MCP tool calls bypass the
ReadOnlyModeFilterbecause they arrive viaPOST /mcp/messageinstead of/api/clusters/{clusterName}/...paths. This means write operations (create topic, delete schema, etc.) can be executed through MCP even when the cluster is configured as read-only.Root Cause
ReadOnlyModeFilteris aWebFilterthat checks the HTTP request path against/api/clusters/{clusterName}pattern. MCP requests use a different path (/mcp/message,/mcp/sse), so they bypass this filter entirely.Solution
Added a readOnly check in
McpSpecificationGenerator.methodCall():@RequestMappingannotation on the generated API interface method to determine if the operation is a write (POST/PUT/PATCH/DELETE) vs read (GET/OPTIONS/HEAD)clusterNamefrom MCP arguments and checksClustersStorage.getClusterByName(clusterName).isReadOnly(). If the cluster is read-only, returns aReadOnlyModeExceptionerror result instead of invoking the controller method.analyzeTopic,cancelTopicAnalysis, andregisterFilterare exempted from the readOnly check, consistent withReadOnlyModeFilter.SAFE_ENDPOINTS.Changes
McpSpecificationGenerator.java: InjectClustersStorage, addisWriteOperation()detection via@RequestMapping, add readOnly guard inmethodCall()McpSpecificationGeneratorTest.java: Add tests verifying write operations are blocked on readOnly clusters and allowed on non-readOnly clustersTest plan
testConvertControllertest passes (tool generation unchanged)writeOperationBlockedOnReadOnlyClustertest: verifiescreateTopic(POST) returns "read-only" error on readOnly clusterwriteOperationNotBlockedOnNonReadOnlyClustertest: verifiescreateTopicproceeds past readOnly check on normal cluster🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests