ramu11 commented on PR #29722: URL: https://github.com/apache/camel/pull/29722#issuecomment-6844639646
> Thanks @ramu11. In addition to the earlier reviews, I ran reproducers against the PR head ([9a5ae37](https://github.com/apache/camel/commit/9a5ae376014bcbd8bcdd89f8cdd3e7398590350a)). They use the Vert.x engine with the MCP Java SDK 2.1.0 client, and I ran a control on the merge base ([fe2f3ce](https://github.com/apache/camel/commit/fe2f3ce5dfbe9cee5379f5fc9626f10353472a94)). The tests are at the end of this comment; each one passes when the problem it describes holds. > > **1. The client's URI reaches variables and the server log unescaped.** Both `AiResourceUriTemplate` and the SDK match a variable with `[^/]+`. As a result: > > * `crm://customers/42?admin=true#frag` gives `id=42?admin=true#frag`. > * `crm://customers/..` gives `id=..`. > * A newline is accepted. > > Because the spec is built with `asResourceSpec(resourceUri)`, a failing read logs the client's URI: > > ``` > ERROR AiResourceExecutor Error reading resource 'camel:///failing/1<LF>2026-11-12 12:00:00,000 ERROR [main] o.a.c.Security - FORGED admin login ok': lookup failed > WARN McpServerBridge MCP resource 'camel:///failing/1<LF>2026-11-12 ... FORGED admin login ok' read failed: ... > ``` > > On the merge base, no server-side logger writes the client URI. Limiting variables to RFC 3986 `pchar` characters, as davsclaus suggested, fixes all three cases. Logging the template rather than the concrete URI would also help. > > **2. Which of two overlapping templates gets served is not stable.** `doStart` loops over `getResourceTemplates()`, which returns a `Map.copyOf(...)` whose iteration order changes from one JVM run to the next. I used `crm://customers/{id}` (tag `crm`) and `crm://customers/{number}` (tag `ops`), with the bridge started after the routes. Over 10 fresh JVMs, each template won 5 times. The winner in `McpServerBridgeResourceTemplateTest`'s setup also changed between two runs. Stopping and starting the exposed route hands the URIs to the other route for good: > > ``` > exposed before restart of by-id: crm://customers/{id}, after: crm://customers/{number} > ``` > > Could the snapshot keep a stable order, and could overlaps be rejected at route startup the way a duplicate `resourceUri` is? > > **3. `{id}` and `{ID}` collapse into one header.** The duplicate-variable check is case-sensitive, but Camel header names are not. For `camel:///orders/{id}/items/{ID}`, reading `camel:///orders/1/items/2` gives `id=2,ID=2`. Comparing names case-insensitively in the duplicate check would fix it. > > **4. A `resourceUri` containing `{...}` silently hides a template.** I set `resourceUri=camel:///accounts/{id}` (meaning to write `uriTemplate`) next to `uriTemplate=camel:///accounts/{accountId}`. Reading `camel:///accounts/7` reaches the fixed route without a `CamelAiResourceUri` header, and the template route is never called. The SDK treats a fixed URI with braces as a pattern, and that already happens on main. Now that `uriTemplate` exists, rejecting `{` and `}` in `resourceUri` at startup would turn this into a clear error. > > ### Reproducer tests > Run with: > > ``` > mvn verify -pl components/camel-ai/camel-ai-resource,components/camel-ai/camel-mcp-server-api,components/camel-ai/camel-mcp-server -Dtest='Pr29722*' -Dsurefire.failIfNoSpecifiedTests=false > ``` > > Pr29722TemplateReproducerTest.java — findings 1, 3, 4 (PR head, `camel-mcp-server`, package `org.apache.camel.component.mcp.server.vertx`) > ```java > package org.apache.camel.component.mcp.server.vertx; > > import java.time.Duration; > import java.util.List; > import java.util.concurrent.CopyOnWriteArrayList; > import java.util.concurrent.atomic.AtomicInteger; > > import io.modelcontextprotocol.client.McpClient; > import io.modelcontextprotocol.client.McpSyncClient; > import io.modelcontextprotocol.client.transport.HttpClientStreamableHttpTransport; > import io.modelcontextprotocol.spec.McpSchema; > import org.apache.camel.CamelContext; > import org.apache.camel.builder.RouteBuilder; > import org.apache.camel.component.mcp.server.McpServerBridge; > import org.apache.camel.component.mcp.server.McpServerConfiguration; > import org.apache.camel.component.platform.http.vertx.VertxPlatformHttpServer; > import org.apache.camel.component.platform.http.vertx.VertxPlatformHttpServerConfiguration; > import org.apache.camel.test.AvailablePortFinder; > import org.apache.camel.test.junit6.CamelTestSupport; > import org.apache.logging.log4j.LogManager; > import org.apache.logging.log4j.core.LogEvent; > import org.apache.logging.log4j.core.LoggerContext; > import org.apache.logging.log4j.core.appender.AbstractAppender; > import org.apache.logging.log4j.core.config.Property; > import org.junit.jupiter.api.AfterEach; > import org.junit.jupiter.api.Test; > > import static org.assertj.core.api.Assertions.assertThat; > import static org.assertj.core.api.Assertions.assertThatThrownBy; > > /** > * Reproducers for the PR #29722 review, end to end: real Vert.x engine, official MCP Java SDK client. Each test passes > * when the reported problem holds. > */ > class Pr29722TemplateReproducerTest extends CamelTestSupport { > > private static final String TAG = "repro"; > private static final String FORGED = "2026-11-12 12:00:00,000 ERROR [main] o.a.c.Security - FORGED admin login ok"; > > private final int port = AvailablePortFinder.getNextAvailable(); > private final AtomicInteger accountTemplateInvocations = new AtomicInteger(); > private final CapturingAppender appender = new CapturingAppender(); > private McpSyncClient client; > > @Override > protected CamelContext createCamelContext() throws Exception { > CamelContext camelContext = super.createCamelContext(); > VertxPlatformHttpServerConfiguration http = new VertxPlatformHttpServerConfiguration(); > http.setBindPort(port); > camelContext.addService(new VertxPlatformHttpServer(http)); > McpServerConfiguration configuration = new McpServerConfiguration(); > configuration.setTags(TAG); > camelContext.addService(new McpServerBridge(configuration)); > return camelContext; > } > > @Override > protected RouteBuilder createRouteBuilder() { > return new RouteBuilder() { > @Override > public void configure() { > // finding 1: variable value is raw client input > from("ai-resource:customer?uriTemplate=camel:///customers/{id}&tags=" + TAG) > .setBody(simple("id=${header.CamelAiResourceTemplateVariable.id}")); > > // finding 1: a failing template route logs the client URI > from("ai-resource:failing?uriTemplate=camel:///failing/{id}&tags=" + TAG) > .process(e -> { > throw new IllegalStateException("lookup failed"); > }); > > // finding 3: {id} and {ID} are two variables for the parser, one header for Camel > from("ai-resource:order_item?uriTemplate=camel:///orders/{id}/items/{ID}&tags=" + TAG) > .setBody(simple("id=${header.CamelAiResourceTemplateVariable.id}," > + "ID=${header.CamelAiResourceTemplateVariable.ID}")); > > // finding 4: resourceUri where uriTemplate was meant, next to the real template route > from("ai-resource:account_typo?resourceUri=camel:///accounts/{id}&tags=" + TAG) > .setBody(simple("fixed-route uriHeader=${header.CamelAiResourceUri}")); > from("ai-resource:account?uriTemplate=camel:///accounts/{accountId}&tags=" + TAG) > .process(e -> accountTemplateInvocations.incrementAndGet()) > .setBody(simple("template-route id=${header.CamelAiResourceTemplateVariable.accountId}")); > } > }; > } > > private McpSyncClient client() { > if (client == null) { > client = McpClient.sync(HttpClientStreamableHttpTransport.builder("http://localhost:"; + port).build()) > .requestTimeout(Duration.ofSeconds(10)) > .initializationTimeout(Duration.ofSeconds(10)) > .build(); > client.initialize(); > } > return client; > } > > private String readText(String uri) { > McpSchema.ReadResourceResult result = client().readResource(new McpSchema.ReadResourceRequest(uri)); > return ((McpSchema.TextResourceContents) result.contents().get(0)).text(); > } > > @AfterEach > void cleanup() { > if (client != null) { > client.closeGracefully(); > client = null; > } > appender.detach(); > } > > // ---- finding 1: query, fragment, dot-segments and CR/LF reach the route as the variable value > > @Test > void finding1QueryAndFragmentEndUpInsideTheVariable() { > String text = readText("camel:///customers/42?admin=true#frag"); > System.out.println("[finding1] query+fragment -> " + text); > assertThat(text).isEqualTo("id=42?admin=true#frag"); > } > > @Test > void finding1DotSegmentIsAcceptedAsVariable() { > String text = readText("camel:///customers/.."); > System.out.println("[finding1] dot-segment -> " + text); > assertThat(text).isEqualTo("id=.."); > } > > @Test > void finding1NewlineIsAcceptedAsVariable() { > String text = readText("camel:///customers/42\nX-Injected: yes"); > System.out.println("[finding1] newline -> " + text.replace("\n", "\\n")); > assertThat(text).isEqualTo("id=42\nX-Injected: yes"); > } > > @Test > void finding1ClientUriIsWrittenUnescapedToTheServerLog() { > appender.attach(); > String uri = "camel:///failing/1\n" + FORGED; > > assertThatThrownBy(() -> client().readResource(new McpSchema.ReadResourceRequest(uri))) > .hasMessageContaining("Resource read failed"); > > List<String> forged = appender.lines.stream().filter(l -> l.contains("\n" + FORGED)) > // the SDK client runs in the same JVM; only server-side loggers count > .filter(l -> !l.contains("McpClientSession")).toList(); > forged.forEach(l -> System.out.println("[finding1] logged -> " + l.replace("\n", "<LF>"))); > assertThat(forged) > .as("a WARN/ERROR line whose message contains a raw newline followed by the forged log line") > .isNotEmpty(); > } > > // ---- finding 3: case-insensitive headers collapse {id} and {ID} > > @Test > void finding3CaseVariantVariablesCollapseIntoOneHeader() { > String text = readText("camel:///orders/1/items/2"); > System.out.println("[finding3] camel:///orders/1/items/2 -> " + text); > // independent variables would give id=1,ID=2 > assertThat(text).isEqualTo("id=2,ID=2"); > } > > // ---- finding 4: a resourceUri containing {..} is a pattern for the SDK and shadows the real template > > @Test > void finding4BracedResourceUriShadowsTemplate() { > accountTemplateInvocations.set(0); > String text = readText("camel:///accounts/7"); > System.out.println("[finding4] camel:///accounts/7 -> " + text + " (template invocations=" > + accountTemplateInvocations.get() + ")"); > assertThat(text).isEqualTo("fixed-route uriHeader="); > assertThat(accountTemplateInvocations).hasValue(0); > } > > static final class CapturingAppender extends AbstractAppender { > final List<String> lines = new CopyOnWriteArrayList<>(); > > CapturingAppender() { > super("pr29722-capture", null, null, true, Property.EMPTY_ARRAY); > } > > @Override > public void append(LogEvent event) { > lines.add(event.getLevel() + " " + event.getLoggerName() + " " + event.getMessage().getFormattedMessage()); > } > > void attach() { > start(); > LoggerContext ctx = (LoggerContext) LogManager.getContext(false); > ctx.getConfiguration().getRootLogger().addAppender(this, null, null); > ctx.updateLoggers(); > } > > void detach() { > LoggerContext ctx = (LoggerContext) LogManager.getContext(false); > ctx.getConfiguration().getRootLogger().removeAppender(getName()); > ctx.updateLoggers(); > } > } > } > ``` > > Pr29722OverlapReproducerTest.java — finding 2 (PR head, `camel-mcp-server-api`, package `org.apache.camel.component.mcp.server`) > ```java > package org.apache.camel.component.mcp.server; > > import java.io.BufferedReader; > import java.io.InputStreamReader; > import java.nio.charset.StandardCharsets; > import java.util.ArrayList; > import java.util.List; > import java.util.Map; > import java.util.TreeMap; > import java.util.concurrent.TimeUnit; > > import org.apache.camel.CamelContext; > import org.apache.camel.builder.RouteBuilder; > import org.apache.camel.impl.DefaultCamelContext; > import org.apache.camel.test.junit6.CamelTestSupport; > import org.junit.jupiter.api.Test; > > import static org.assertj.core.api.Assertions.assertThat; > > /** > * Reproducers for finding 2 of the PR #29722 review: which of two overlapping templates is exposed is not stable. > */ > class Pr29722OverlapReproducerTest extends CamelTestSupport { > > static final String ID_TEMPLATE = "crm://customers/{id}"; > static final String NUMBER_TEMPLATE = "crm://customers/{number}"; > > private final RecordingMcpServerEngine engine = new RecordingMcpServerEngine(); > > @Override > protected CamelContext createCamelContext() throws Exception { > CamelContext camelContext = super.createCamelContext(); > camelContext.getRegistry().bind("mcpServerEngine", engine); > McpServerConfiguration configuration = new McpServerConfiguration(); > configuration.setTags("crm,ops"); > camelContext.addService(new McpServerBridge(configuration)); > return camelContext; > } > > @Override > protected RouteBuilder createRouteBuilder() { > return routes(); > } > > static RouteBuilder routes() { > return new RouteBuilder() { > @Override > public void configure() { > from("ai-resource:customer_by_id?uriTemplate=" + ID_TEMPLATE + "&tags=crm") > .routeId("by-id").setBody(constant("by-id")); > from("ai-resource:customer_by_number?uriTemplate=" + NUMBER_TEMPLATE + "&tags=ops") > .routeId("by-number").setBody(constant("by-number")); > } > }; > } > > /** > * Stopping and starting the exposed route hands crm://customers/* to the other route for good. > */ > @Test > void routeRestartPermanentlyMovesTheUrisToTheOtherRoute() throws Exception { > String before = engine.resourceTemplates().keySet().iterator().next(); > String acceptedRoute = ID_TEMPLATE.equals(before) ? "by-id" : "by-number"; > String otherTemplate = ID_TEMPLATE.equals(before) ? NUMBER_TEMPLATE : ID_TEMPLATE; > > context.getRouteController().stopRoute(acceptedRoute); > context.getRouteController().startRoute(acceptedRoute); > > String after = engine.resourceTemplates().keySet().iterator().next(); > System.out.println("[finding2] exposed before restart of " + acceptedRoute + ": " + before + ", after: " + after); > assertThat(context.getRouteController().getRouteStatus(acceptedRoute).isStarted()).isTrue(); > assertThat(engine.resourceTemplates()).containsOnlyKeys(otherTemplate); > } > > /** > * When the bridge starts after the routes (bridge restart, or a runtime that starts it late), the winner comes from > * the iteration order of Map.copyOf, which is randomized per JVM. Ten fresh JVMs disagree on the winner. > */ > @Test > void winnerDependsOnJvmWhenBridgeStartsAfterRoutes() throws Exception { > Map<String, Integer> winners = new TreeMap<>(); > for (int i = 0; i < 10; i++) { > String winner = runChildJvm(); > winners.merge(winner, 1, Integer::sum); > } > System.out.println("[finding2] winners over 10 JVMs: " + winners); > assertThat(winners).containsOnlyKeys(ID_TEMPLATE, NUMBER_TEMPLATE); > } > > private static String runChildJvm() throws Exception { > List<String> command = new ArrayList<>(); > command.add(System.getProperty("java.home") + "/bin/java"); > command.add("-cp"); > command.add(System.getProperty("java.class.path")); > command.add(LateBridgeMain.class.getName()); > Process process = new ProcessBuilder(command).redirectErrorStream(true).start(); > String winner = null; > try (BufferedReader reader = new BufferedReader( > new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8))) { > String line; > while ((line = reader.readLine()) != null) { > if (line.startsWith("WINNER=")) { > winner = line.substring("WINNER=".length()); > } > } > } > assertThat(process.waitFor(60, TimeUnit.SECONDS)).isTrue(); > assertThat(winner).isNotNull(); > return winner; > } > > public static final class LateBridgeMain { > public static void main(String[] args) throws Exception { > RecordingMcpServerEngine engine = new RecordingMcpServerEngine(); > try (DefaultCamelContext context = new DefaultCamelContext()) { > context.getRegistry().bind("mcpServerEngine", engine); > context.addRoutes(routes()); > context.start(); > // bridge starts after the routes registered their templates > McpServerConfiguration configuration = new McpServerConfiguration(); > configuration.setTags("crm,ops"); > context.addService(new McpServerBridge(configuration), true, true); > System.out.println("WINNER=" + String.join(",", engine.resourceTemplates().keySet())); > } > System.exit(0); > } > } > } > ``` > > Pr29722MainControlTest.java — control on the merge base [fe2f3ce](https://github.com/apache/camel/commit/fe2f3ce5dfbe9cee5379f5fc9626f10353472a94) (`camel-mcp-server`) > ```java > package org.apache.camel.component.mcp.server.vertx; > > import java.time.Duration; > import java.util.List; > import java.util.concurrent.CopyOnWriteArrayList; > > import io.modelcontextprotocol.client.McpClient; > import io.modelcontextprotocol.client.McpSyncClient; > import io.modelcontextprotocol.client.transport.HttpClientStreamableHttpTransport; > import io.modelcontextprotocol.spec.McpSchema; > import org.apache.camel.CamelContext; > import org.apache.camel.builder.RouteBuilder; > import org.apache.camel.component.mcp.server.McpServerBridge; > import org.apache.camel.component.mcp.server.McpServerConfiguration; > import org.apache.camel.component.platform.http.vertx.VertxPlatformHttpServer; > import org.apache.camel.component.platform.http.vertx.VertxPlatformHttpServerConfiguration; > import org.apache.camel.test.AvailablePortFinder; > import org.apache.camel.test.junit6.CamelTestSupport; > import org.apache.logging.log4j.LogManager; > import org.apache.logging.log4j.core.LogEvent; > import org.apache.logging.log4j.core.LoggerContext; > import org.apache.logging.log4j.core.appender.AbstractAppender; > import org.apache.logging.log4j.core.config.Property; > import org.junit.jupiter.api.AfterEach; > import org.junit.jupiter.api.Test; > > import static org.assertj.core.api.Assertions.assertThat; > import static org.assertj.core.api.Assertions.assertThatThrownBy; > > /** > * Main/merge-base control for the PR #29722 reproducers (no uriTemplate support here). > */ > class Pr29722MainControlTest extends CamelTestSupport { > > private static final String TAG = "repro"; > private static final String FORGED = "2026-11-12 12:00:00,000 ERROR [main] o.a.c.Security - FORGED admin login ok"; > > private final int port = AvailablePortFinder.getNextAvailable(); > private final CapturingAppender appender = new CapturingAppender(); > private McpSyncClient client; > > @Override > protected CamelContext createCamelContext() throws Exception { > CamelContext camelContext = super.createCamelContext(); > VertxPlatformHttpServerConfiguration http = new VertxPlatformHttpServerConfiguration(); > http.setBindPort(port); > camelContext.addService(new VertxPlatformHttpServer(http)); > McpServerConfiguration configuration = new McpServerConfiguration(); > configuration.setTags(TAG); > camelContext.addService(new McpServerBridge(configuration)); > return camelContext; > } > > @Override > protected RouteBuilder createRouteBuilder() { > return new RouteBuilder() { > @Override > public void configure() { > from("ai-resource:failing?resourceUri=camel:///failing/1&tags=" + TAG) > .process(e -> { > throw new IllegalStateException("lookup failed"); > }); > from("ai-resource:account_typo?resourceUri=camel:///accounts/{id}&tags=" + TAG) > .setBody(constant("fixed-route")); > } > }; > } > > private McpSyncClient client() { > if (client == null) { > client = McpClient.sync(HttpClientStreamableHttpTransport.builder("http://localhost:"; + port).build()) > .requestTimeout(Duration.ofSeconds(10)) > .initializationTimeout(Duration.ofSeconds(10)) > .build(); > client.initialize(); > } > return client; > } > > @AfterEach > void cleanup() { > if (client != null) { > client.closeGracefully(); > client = null; > } > appender.detach(); > } > > /** Finding 1 control: on main the client URI never reaches the read logs. */ > @Test > void finding1ControlClientUriIsNotLogged() { > appender.attach(); > String uri = "camel:///failing/1\n" + FORGED; > > Throwable t = null; > try { > client().readResource(new McpSchema.ReadResourceRequest(uri)); > } catch (Exception e) { > t = e; > } > System.out.println("[control1] read of forged uri -> " + (t == null ? "no error" : t.getMessage())); > List<String> forged = appender.lines.stream().filter(l -> l.contains("\n" + FORGED)) > // the SDK client runs in the same JVM; only server-side loggers count > .filter(l -> !l.contains("McpClientSession")).toList(); > System.out.println("[control1] forged log lines: " + forged.size()); > forged.forEach(l -> System.out.println("[control1] logged -> " + l.replace("\n", "<LF>"))); > assertThat(t).isNotNull(); > assertThat(forged).isEmpty(); > } > > /** Finding 4 control: is a braced resourceUri already a pattern on main? */ > @Test > void finding4ControlBracedResourceUriOnMain() { > McpSchema.ReadResourceResult result > = client().readResource(new McpSchema.ReadResourceRequest("camel:///accounts/7")); > String text = ((McpSchema.TextResourceContents) result.contents().get(0)).text(); > System.out.println("[control4] camel:///accounts/7 -> " + text); > assertThat(text).isEqualTo("fixed-route"); > } > > /** Findings 2/3 control: uriTemplate does not exist on main. */ > @Test > void finding23ControlUriTemplateUnknownOnMain() { > assertThatThrownBy(() -> context.getEndpoint("ai-resource:x?uriTemplate=camel:///orders/{id}")) > .hasMessageContaining("uriTemplate"); > } > > static final class CapturingAppender extends AbstractAppender { > final List<String> lines = new CopyOnWriteArrayList<>(); > > CapturingAppender() { > super("pr29722-capture", null, null, true, Property.EMPTY_ARRAY); > } > > @Override > public void append(LogEvent event) { > lines.add(event.getLevel() + " " + event.getLoggerName() + " " + event.getMessage().getFormattedMessage()); > } > > void attach() { > start(); > LoggerContext ctx = (LoggerContext) LogManager.getContext(false); > ctx.getConfiguration().getRootLogger().addAppender(this, null, null); > ctx.updateLoggers(); > } > > void detach() { > LoggerContext ctx = (LoggerContext) LogManager.getContext(false); > ctx.getConfiguration().getRootLogger().removeAppender(getName()); > ctx.updateLoggers(); > } > } > } > ``` > > _Claude Code on behalf of Croway_ Thanks for the detailed review and for providing the end-to-end reproducers. I've pushed the fixes in commit `e56bb697b79a`: 1. **URI validation and log injection:** Restricted template variables to RFC 3986 `pchar` segments (including valid `%HH` escapes) and explicitly reject `.` and `..` segments. Query strings, fragments, control characters and malformed percent escapes are rejected. Server-side logging in `AiResourceExecutor` and `McpServerBridge` now uses the stable resource name rather than the concrete client-supplied URI. The bridge also no longer includes the execution error message in its read-failure warning. 2. **Deterministic template selection:** The registry now returns a deterministically ordered snapshot, with tags sorted lexicographically. Overlapping templates across tagged pools are rejected during route registration, causing the conflicting route to fail startup rather than allowing an order-dependent winner. The default untagged pool remains separate from the tagged templates exposed by the MCP bridge. 3. **Case-insensitive variable names:** Duplicate variable names are now checked case-insensitively, preventing `{id}` and `{ID}` from overwriting each other in Camel headers. 4. **Braces in fixed resource URIs:** Route startup now rejects `{` and `}` in `resourceUri` with a message directing users to configure `uriTemplate` instead. I've added regression coverage for these cases, including route-startup validation and template lifecycle behaviour. I also reran the Maven verification across the three affected modules. The build completed successfully: 44 tests run, zero failures, zero errors, and one skipped test. Thanks again for identifying these issues. -- 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]
