This is an automated email from the ASF dual-hosted git repository. robertlazarski pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/axis-axis2-java-core.git
commit 2c0577f36f32e3ca7902b45fc705b759452e2c22 Author: Robert Lazarski <[email protected]> AuthorDate: Mon Apr 6 13:21:00 2026 -1000 mcp: apply Gemini 2.5 Pro code review fixes Four issues found and fixed: CRITICAL — McpStdioServer: tools/call errors now always produce a JSON-RPC response. Previously, IllegalArgumentException (unknown tool, missing param) propagated to run() where it was only logged to stderr, leaving the client waiting forever. Added try-catch inside processLine() with correct error codes: -32602 for invalid params, -32000 for I/O/interrupt, -32603 for internal errors. HIGH — OpenApiSpecGenerator.generateMcpCatalogJson(): replaced StringBuilder + custom escapeJson() with Jackson ObjectMapper. The old escapeJson() only handled backslash and double-quote, silently corrupting any operation name containing \n, \t, \r, or other control characters. Jackson guarantees RFC 8259-correct escaping. MEDIUM — X509AuthenticationFilter.extractCN(): replaced comma-split with LdapName + Rdn iteration. RFC 2253 allows escaped commas inside RDN values (e.g. O="Example, Inc."); a plain split(",") would mis-parse such DNs. LdapName handles escaping correctly, iterates in reverse since CN is typically the most-specific (last) RDN. LOW — McpStdioServerTest: renamed testToolsCallUnknownToolProducesNo- StdoutResponse and updated assertion. The old test validated the buggy behavior (empty stdout). The new test asserts a -32602 error response with the echoed id and the unknown tool name in the message. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> --- .../apache/axis2/mcp/bridge/McpStdioServer.java | 38 +++++++++----- .../axis2/mcp/bridge/McpStdioServerTest.java | 28 +++++++---- .../apache/axis2/openapi/OpenApiSpecGenerator.java | 58 +++++++++++----------- .../axis2/openapi/McpCatalogGeneratorTest.java | 18 +++++++ .../webservices/X509AuthenticationFilter.java | 27 +++++++--- 5 files changed, 109 insertions(+), 60 deletions(-) diff --git a/modules/mcp-bridge/src/main/java/org/apache/axis2/mcp/bridge/McpStdioServer.java b/modules/mcp-bridge/src/main/java/org/apache/axis2/mcp/bridge/McpStdioServer.java index c378661900..715f0f469d 100644 --- a/modules/mcp-bridge/src/main/java/org/apache/axis2/mcp/bridge/McpStdioServer.java +++ b/modules/mcp-bridge/src/main/java/org/apache/axis2/mcp/bridge/McpStdioServer.java @@ -119,18 +119,32 @@ public class McpStdioServer { JsonNode params = request.path("params"); System.err.println("[axis2-mcp-bridge] Request id=" + id + " method=" + method); - switch (method) { - case "initialize": - writeSuccess(id, buildInitializeResult(params)); - break; - case "tools/list": - writeSuccess(id, buildToolsListResult()); - break; - case "tools/call": - writeSuccess(id, buildToolsCallResult(params)); - break; - default: - writeError(id, -32601, "Method not found: " + method); + try { + switch (method) { + case "initialize": + writeSuccess(id, buildInitializeResult(params)); + break; + case "tools/list": + writeSuccess(id, buildToolsListResult()); + break; + case "tools/call": + writeSuccess(id, buildToolsCallResult(params)); + break; + default: + writeError(id, -32601, "Method not found: " + method); + } + } catch (IllegalArgumentException e) { + // Invalid params: unknown tool name, missing required param, etc. + writeError(id, -32602, "Invalid params: " + e.getMessage()); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + writeError(id, -32000, "Server error during tool call: " + e.getMessage()); + } catch (IOException e) { + writeError(id, -32000, "Server error during tool call: " + e.getMessage()); + } catch (Exception e) { + System.err.println("[axis2-mcp-bridge] Internal error id=" + id + ": " + e.getMessage()); + e.printStackTrace(System.err); + writeError(id, -32603, "Internal error: " + e.getMessage()); } } diff --git a/modules/mcp-bridge/src/test/java/org/apache/axis2/mcp/bridge/McpStdioServerTest.java b/modules/mcp-bridge/src/test/java/org/apache/axis2/mcp/bridge/McpStdioServerTest.java index f5866b9c28..2499a60a5a 100644 --- a/modules/mcp-bridge/src/test/java/org/apache/axis2/mcp/bridge/McpStdioServerTest.java +++ b/modules/mcp-bridge/src/test/java/org/apache/axis2/mcp/bridge/McpStdioServerTest.java @@ -202,27 +202,35 @@ public class McpStdioServerTest extends TestCase { // ── tools/call ────────────────────────────────────────────────────────── /** - * When tools/call names a tool that is not in the registry, the server - * throws IllegalArgumentException, which is caught by the run() loop and - * logged to stderr. No JSON-RPC response is written to stdout. + * When tools/call names a tool not in the registry, the server must return + * a JSON-RPC error with code -32602 (Invalid Params) per the spec. The id + * from the request must be echoed in the error response so the client can + * correlate it. */ - public void testToolsCallUnknownToolProducesNoStdoutResponse() throws Exception { + public void testToolsCallUnknownToolReturnsInvalidParamsError() throws Exception { String output = runServer(new StubToolRegistry(), "{\"jsonrpc\":\"2.0\",\"id\":3,\"method\":\"tools/call\"," + "\"params\":{\"name\":\"nonexistentTool\",\"arguments\":{}}}"); - // Nothing should be written to stdout for the unknown-tool error path - // (exception is caught in run() and logged to stderr only) - assertTrue("No stdout response expected for unknown tool", output.isEmpty()); + JsonNode response = parseSingleResponse(output); + assertEquals("id must be echoed", 3, response.path("id").asInt()); + assertTrue("Response must contain 'error'", response.has("error")); + assertFalse("Response must not contain 'result'", response.has("result")); + assertEquals(-32602, response.path("error").path("code").asInt()); + assertTrue("Error message must mention the tool name", + response.path("error").path("message").asText().contains("nonexistentTool")); } - public void testToolsCallMissingNameProducesNoStdoutResponse() throws Exception { + public void testToolsCallMissingNameReturnsInvalidParamsError() throws Exception { // params.name is absent — throws IllegalArgumentException in buildToolsCallResult String output = runServer(new StubToolRegistry(), - "{\"jsonrpc\":\"2.0\",\"id\":3,\"method\":\"tools/call\"," + + "{\"jsonrpc\":\"2.0\",\"id\":7,\"method\":\"tools/call\"," + "\"params\":{\"arguments\":{}}}"); - assertTrue("No stdout response expected for missing name", output.isEmpty()); + JsonNode response = parseSingleResponse(output); + assertEquals(7, response.path("id").asInt()); + assertTrue("Response must contain 'error'", response.has("error")); + assertEquals(-32602, response.path("error").path("code").asInt()); } // ── error cases ───────────────────────────────────────────────────────── diff --git a/modules/openapi/src/main/java/org/apache/axis2/openapi/OpenApiSpecGenerator.java b/modules/openapi/src/main/java/org/apache/axis2/openapi/OpenApiSpecGenerator.java index 27c825b3a7..dce127e498 100644 --- a/modules/openapi/src/main/java/org/apache/axis2/openapi/OpenApiSpecGenerator.java +++ b/modules/openapi/src/main/java/org/apache/axis2/openapi/OpenApiSpecGenerator.java @@ -633,12 +633,23 @@ public class OpenApiSpecGenerator { * Uses the same service filtering as {@link #generatePaths()} so the tool * catalog is consistent with the OpenAPI spec. */ + /** + * Generates the MCP tool catalog JSON for the {@code /openapi-mcp.json} endpoint. + * + * <p>Uses Jackson to build the JSON object graph, which guarantees correct + * escaping of all string values including control characters — something a + * hand-rolled {@code StringBuilder} approach cannot safely guarantee. + * + * <p>Uses the same service/operation filtering as {@link #generatePaths()}. + */ public String generateMcpCatalogJson(HttpServletRequest request) { try { AxisConfiguration axisConfig = configurationContext.getAxisConfiguration(); - StringBuilder json = new StringBuilder(); - json.append("{\n \"tools\": [\n"); - boolean firstTool = true; + + com.fasterxml.jackson.databind.ObjectMapper jackson = + new com.fasterxml.jackson.databind.ObjectMapper(); + com.fasterxml.jackson.databind.node.ObjectNode root = jackson.createObjectNode(); + com.fasterxml.jackson.databind.node.ArrayNode toolsArray = root.putArray("tools"); Iterator<AxisService> services = axisConfig.getServices().values().iterator(); while (services.hasNext()) { @@ -654,26 +665,24 @@ public class OpenApiSpecGenerator { String opName = operation.getName().getLocalPart(); String path = "/services/" + service.getName() + "/" + opName; - if (!firstTool) json.append(",\n"); - firstTool = false; - - json.append(" {\n"); - json.append(" \"name\": \"").append(escapeJson(opName)).append("\",\n"); - json.append(" \"description\": \"").append(escapeJson(service.getName())) - .append(": ").append(escapeJson(opName)).append("\",\n"); - json.append(" \"inputSchema\": {\n"); - json.append(" \"type\": \"object\",\n"); - json.append(" \"properties\": {},\n"); - json.append(" \"required\": []\n"); - json.append(" },\n"); - json.append(" \"endpoint\": \"POST ").append(escapeJson(path)).append("\"\n"); - json.append(" }"); + com.fasterxml.jackson.databind.node.ObjectNode toolNode = toolsArray.addObject(); + toolNode.put("name", opName); + toolNode.put("description", service.getName() + ": " + opName); + + // inputSchema: minimal MCP-compliant structure. Richer schemas are + // produced when services carry @McpTool annotations (future work). + com.fasterxml.jackson.databind.node.ObjectNode schema = + toolNode.putObject("inputSchema"); + schema.put("type", "object"); + schema.putObject("properties"); + schema.putArray("required"); + + toolNode.put("endpoint", "POST " + path); } } - json.append("\n ]\n}"); log.debug("Generated MCP catalog JSON"); - return json.toString(); + return jackson.writeValueAsString(root); } catch (Exception e) { log.error("Failed to generate MCP catalog JSON", e); @@ -681,17 +690,6 @@ public class OpenApiSpecGenerator { } } - /** - * Escape a string for safe inclusion in a JSON string value. - * Axis2 service/operation names follow XML NCName rules and will not - * contain control characters, but backslash and double-quote are escaped - * defensively. - */ - private String escapeJson(String s) { - if (s == null) return ""; - return s.replace("\\", "\\\\").replace("\"", "\\\""); - } - /** * Get OpenAPI JSON processing performance statistics using moshih2 metrics. */ diff --git a/modules/openapi/src/test/java/org/apache/axis2/openapi/McpCatalogGeneratorTest.java b/modules/openapi/src/test/java/org/apache/axis2/openapi/McpCatalogGeneratorTest.java index 3f0c01db50..242468d35e 100644 --- a/modules/openapi/src/test/java/org/apache/axis2/openapi/McpCatalogGeneratorTest.java +++ b/modules/openapi/src/test/java/org/apache/axis2/openapi/McpCatalogGeneratorTest.java @@ -266,6 +266,24 @@ public class McpCatalogGeneratorTest extends TestCase { assertNotNull(root); } + /** + * Jackson correctly escapes all JSON control characters including tab, newline, + * and carriage return — not just backslash and double-quote. + */ + public void testControlCharactersInOperationNameAreEscaped() throws Exception { + AxisService svc = new AxisService("TestService"); + AxisOperation op = new InOutAxisOperation(); + op.setName(QName.valueOf("op\twith\ttabs")); // tab chars + svc.addOperation(op); + axisConfig.addService(svc); + + String json = generator.generateMcpCatalogJson(mockRequest); + assertFalse("Tab characters must not appear literally in JSON output", + json.contains("\t")); + JsonNode root = MAPPER.readTree(json); // still parseable + assertNotNull(root); + } + // ── catalog request is null-safe ────────────────────────────────────────── public void testGenerateMcpCatalogWithNullRequestDoesNotThrow() throws Exception { diff --git a/modules/samples/userguide/src/userguide/springbootdemo-tomcat11/src/main/java/userguide/springboot/security/webservices/X509AuthenticationFilter.java b/modules/samples/userguide/src/userguide/springbootdemo-tomcat11/src/main/java/userguide/springboot/security/webservices/X509AuthenticationFilter.java index a1c96e1af1..358e52e7a1 100644 --- a/modules/samples/userguide/src/userguide/springbootdemo-tomcat11/src/main/java/userguide/springboot/security/webservices/X509AuthenticationFilter.java +++ b/modules/samples/userguide/src/userguide/springbootdemo-tomcat11/src/main/java/userguide/springboot/security/webservices/X509AuthenticationFilter.java @@ -33,6 +33,9 @@ import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.web.filter.GenericFilterBean; +import javax.naming.InvalidNameException; +import javax.naming.ldap.LdapName; +import javax.naming.ldap.Rdn; import javax.security.auth.x500.X500Principal; import java.io.IOException; import java.security.cert.X509Certificate; @@ -95,17 +98,25 @@ public class X509AuthenticationFilter extends GenericFilterBean { /** * Extracts the CN value from an {@link X500Principal}. * - * <p>Uses RFC 2253 format ({@code CN=value,O=...}) for reliable parsing. - * Returns the full DN string if no CN attribute is present. + * <p>Uses {@link LdapName} to parse the RFC 2253 DN, which correctly + * handles escaped commas in RDN values (e.g. {@code O="Example, Inc."}). + * Falls back to the full DN string if parsing fails or no CN attribute + * is present. */ private String extractCN(X500Principal principal) { - String dn = principal.getName(X500Principal.RFC2253); - for (String part : dn.split(",")) { - part = part.trim(); - if (part.startsWith("CN=")) { - return part.substring(3); + try { + LdapName ldapDN = new LdapName(principal.getName()); + // Iterate in reverse — CN is typically the most-specific (last) RDN + for (int i = ldapDN.size() - 1; i >= 0; i--) { + Rdn rdn = ldapDN.getRdn(i); + if ("CN".equalsIgnoreCase(rdn.getType())) { + return rdn.getValue().toString(); + } } + } catch (InvalidNameException e) { + logger.warn("X509AuthenticationFilter: could not parse DN, using full DN: " + + principal.getName()); } - return dn; + return principal.getName(); } }
