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 <noreply@anthropic.com>

Author: robertlazarski

modules/mcp-bridge/src/main/java/org/apache/axis2/mcp/bridge/McpStdioServer.java

@@ -119,18 +119,32 @@ private void processLine(String line) throws Exception {
         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());
         }
     }
 

modules/mcp-bridge/src/test/java/org/apache/axis2/mcp/bridge/McpStdioServerTest.java

@@ -202,27 +202,35 @@ public void testToolsListReturnsMultipleTools() throws Exception {
     // ── 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 ─────────────────────────────────────────────────────────

modules/openapi/src/main/java/org/apache/axis2/openapi/OpenApiSpecGenerator.java

@@ -633,12 +633,23 @@ public ConfigurationContext getConfigurationContext() {
      * 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,44 +665,31 @@ public String generateMcpCatalogJson(HttpServletRequest request) {
                     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);
             return "{\"tools\":[]}";
         }
     }
 
-    /**
-     * 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.
      */

modules/openapi/src/test/java/org/apache/axis2/openapi/McpCatalogGeneratorTest.java

@@ -266,6 +266,24 @@ public void testServiceNameWithBackslashIsEscaped() throws Exception {
         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 {

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.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 void doFilter(ServletRequest servletRequest, ServletResponse servletRespo
     /**
      * 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();
     }
 }