Croway commented on code in PR #29592:
URL: https://github.com/apache/camel/pull/29592#discussion_r4537428008


##########
components/camel-ai/camel-mcp-server/src/main/java/org/apache/camel/component/mcp/server/vertx/VertxMcpStreamableServerTransportProvider.java:
##########
@@ -227,6 +280,121 @@ private void checkOrigin(RoutingContext ctx) {
         ctx.next();
     }
 
+    /**
+     * Validates a Bearer token before the MCP body handler or MCP dispatch 
runs. Validation executes on a worker thread
+     * because JWKS and introspection may require outbound HTTP calls.
+     */
+    private void checkOAuth(RoutingContext ctx) {
+        List<String> authorizations = 
List.copyOf(ctx.request().headers().getAll("Authorization"));
+        ctx.request().pause();

Review Comment:
   **`checkOAuth` leaves the request paused when it rejects it**
   
   `checkOAuth` pauses the request (line 289) and resumes it only on the 
success path (line 343). The 503 (line 295), 400/401 (line 302) and 403 (line 
323) branches end the response without calling `resume()`. When the unread part 
of the body is larger than the Vert.x inbound buffer, the server stops reading 
from the socket, so the request never completes and the next request on that 
keep-alive connection is never served. 
`VertxPlatformHttpConsumer.handleSecurity` resumes on every branch.
   
   This can happen during an ordinary token refresh. A client's access token 
expires partway through a session, and a tool call with a large argument 
(document text, file contents, a base64 image) gets a 401. The client refreshes 
the token and retries on the same pooled connection, and the retry hangs until 
the client times out.
   
   I reproduced it with the MCP Java SDK 2.1.0 client 
(`HttpClientStreamableHttpTransport`, whose `authorizationErrorHandler` 
refreshes the token and retries after a 401). The server was Camel Main with an 
MCP OAuth profile and a stub token validator, on f0ceb80. This file has not 
changed since 94ec47e, which gives the same result:
   
   | tool argument | f0ceb80 | with `resume()` in the 3 rejection branches | 
valid token, no expiry |
   |---|---|---|---|
   | 1 KB | completes (8 ms) | completes (9 ms) | – |
   | 300 KB | **times out after 20 s** | completes (8 ms) | completes (57 ms) |
   | 2 MB | **times out after 20 s** | completes (27 ms) | completes (22 ms) |
   
   In every row with an expired token, the client received the 401 and 
refreshed the token once; the retried call is what hangs. A raw-socket test 
(POST `/mcp` with a 2 MB body, then a GET on the same connection) shows the 
same for the 401, 403 and 503 branches: the second request gets no response, 
while bodies up to 100 KB are fine.
   
   Suggested fix: resume before ending the response in each rejection branch:
   
   ```java
   if (ar.failed()) {
       LOG.warn("Unable to execute MCP OAuth validation", ar.cause());
       ctx.request().resume();
       ctx.response().setStatusCode(503).end("Service Unavailable");
       return;
   }
   ...
   if (!validation.isAuthenticated()) {
       ctx.request().resume();
       ...
   }
   ...
   if (!missingScopes.isEmpty()) {
       ctx.request().resume();
       ...
   }
   ```
   
   A regression test could send a POST with an invalid token and a body of a 
few hundred KB, then a second request on the same connection, and assert that 
the second request is answered. I'm happy to share the reproducer.
   
   _Claude Code on behalf of Croway_
   



-- 
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]

Reply via email to