Repository navigation
CAMEL-25917: camel-undertow - a WebSocket connection accepted while only a producer served the path is not treated as having passed a later consumer's custom handlers - #28237
Conversation
…nly a producer served the path is not treated as having passed a later consumer's custom handlers
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔁 1 test passed only after a retry on JDK 21 (1 retried attempt) Recovered flaky tests on JDK 21 (1)
🔬 Scalpel shadow comparison — Scalpel: 46 of 707 tested, 26 compile-only — current: 46 all testedMaveniverse Scalpel detected 46 affected modules (current approach: 46). Skip-tests mode would test 46 modules (2 direct + 45 downstream), skip tests for 26 (generated code, meta-modules) Modules Scalpel would test (46)
Modules with tests skipped (26)
Build reactor — dependencies compiled but only changed modules were tested (2 modules, 49.4s total)Total reactor time: 49.4s
Top 20 slowest modules:
|
luigidemasi
left a comment
luigidemasi
left a comment
There was a problem hiding this comment.
Reviewed commit 8ffcb1587d4a35d09bf299aabe8de1371813f578.
Request changes for one P2 lifecycle issue: a connection opened before the consumer starts can remain open while all subsequent broadcasts are silently filtered out. Receive-only clients receive no close notification to trigger reconnection. The inline comment describes the reproduction and the required peer invalidation and regression coverage.
Hosted CI rechecked for this head: Java 21 and Java 25 builds passed, as did dependency review, documentation validation, license checking and the PR ID uploader. Sonar analysis was skipped.
AI-generated review by Codex on behalf of @luigidemasi.
…s when the consumer starts Close the already-open connections of a path right away when a consumer is set, rather than only when they next send a message, so a receive-only client is told to reconnect instead of silently receiving nothing after the consumer with custom handlers starts. Add a regression test that awaits the close without the client sending, and adjust the existing window tests for the now proactive close.
luigidemasi
left a comment
luigidemasi
left a comment
There was a problem hiding this comment.
Re-reviewed commit e26aaed4b1228147e0f43fda256e879bf564370a.
The previous idle-connection finding is addressed: registering the consumer now closes stale peers, and the original receive-only probe passes. The added regression test detects an inverted peer-close condition. Authentication and lifecycle probes, including an authenticated connection surviving consumer restart, also passed.
One P2 test regression remains: UndertowWsSecurityWithoutProviderTest still requires a successful send after the consumer has proactively closed the socket. The inline comment includes the concrete adjustment for both affected methods.
Hosted CI rechecked for this exact head: Java 21 and Java 25 builds, dependency review, documentation validation, license checking and PR ID upload passed. Sonar analysis was skipped. The test failure described below depends on close/send timing and was reproduced despite those passing hosted checks.
AI-generated review by Codex on behalf of @luigidemasi.
…ctive close The consumer now closes a connection opened before it started as soon as it starts, so sending on that connection afterwards is best effort and bounded.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Correct fix for an authentication-bypass window where a WebSocket connection accepted while only a producer served a path was not re-evaluated against a later consumer's custom handlers.
The HandshakeResult.consumer flag, the updated isAuthenticated logic, and the proactive closePeersThatNoLongerPassChecks() on consumer start are all sound. Concurrency is safe (consumerLock is a ReentrantLock, so the re-entry from closePeersThatNoLongerPassChecks() → pathSecurity() is valid). There is no delivery gap: send() independently filters peers with isAuthenticated(peer, path), so stale connections cannot receive messages between the setConsumer() call and the close.
Tests cover the three key scenarios: a connection accepted in the window (not served + closed), a receive-only connection accepted in the window (closed proactively), and a legitimate post-consumer connection (served correctly). The existing tests in UndertowWsSecurityWithoutProviderTest and UndertowWsOAuthProfileConsumerWindowTest are correctly updated to treat the stale-connection send as best-effort.
Pre-fix validation: dynamic check skipped (no Maven build available). Static trace confirms the test exercises the corrected code path: startRoute("guarded") → setConsumer() → closePeersThatNoLongerPassChecks() (new call, sends close frame) → listener.closeCode completes. Without the fix, closePeersThatNoLongerPassChecks() is not called, listener.closeCode never completes, and the test hangs at orTimeout(10s).
Minor: LOG.warn fires per closed stale peer in closePeersThatNoLongerPassChecks()
The log call is inside .forEach(peer -> { ... }), so a deployment with many stale connections at consumer-start time emits one WARN line per peer — all identical. In high-connection scenarios (e.g., a long producer window before a slow consumer start), this floods the log and buries the real signal that security enforcement kicked in.
Consider logging once at WARN with the count, then DEBUG per peer:
private void closePeersThatNoLongerPassChecks() {
PathSecurity path = pathSecurity();
List<WebSocketChannel> stale = delegate.getPeerConnections().stream()
.filter(peer -> !isAuthenticated(peer, path))
.toList();
if (!stale.isEmpty()) {
LOG.warn("{} WebSocket channel(s) closed: handshake did not pass the security checks of the consumer",
stale.size());
for (WebSocketChannel peer : stale) {
LOG.debug("Closing stale WebSocket channel {}", peer);
WebSockets.sendClose(CloseMessage.MSG_VIOLATES_POLICY, "Authentication required", peer, null);
}
}
}Backport: This is an authentication-bypass fix. Consider whether a port/camel-4.22.x label is appropriate for users on the stable branch.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Hi Federico,
The fix looks correct to me. Approving, to merge once CI is green (the build runs on this head are still queued).
What I checked:
HandshakeResult.consumertogether with theCONSUMER_HANDLERS_ATTACHMENTcheck means a recorded URI only counts for a consumer withhandlerswhen that chain actually ran. It fails closed.- The race where a handshake starts in producer mode and completes after
setConsumer:upgrade()then sees consumer mode without the attachment, oronConnectre-checks. Either way the channel is rejected. A channel that passedonConnectjust before the consumer was set is already ingetPeerConnections(), soclosePeersThatNoLongerPassChecks()catches it. - Consumer restart keeps
consumerEndpointand the consumer handlers, so a connection that ran the handlers survives a restart. A new consumer with a different endpoint URI on the path closes the old peers, because the result is keyed by URI. - Inbound messages and events from a stale peer are still rejected by
UndertowConsumer.rejectUnauthenticatedWebSocketChannel, so nothing leaks between the close frame and the peer going away.
Points:
-
Not blocking - backports and upgrade guides. CAMEL-25375 was backported to both
camel-4.18.xandcamel-4.22.x, and both branches have the samerequiresHandshakeResult/CONSUMER_HANDLERS_ATTACHMENTcode. So this fix should go to 4.18.x as well, not only 4.22.x (the JIRA fix versions list 4.22.3 and 4.24.0). The JIRA also asks for a 4.22 upgrade-guide note. Per the backport policy, the 4.22.3 (and 4.18.x) notes go intocamel-4x-upgrade-guide-4_22.adoc/4_18.adoconmain, in this PR or in a doc-sync PR after the backports. -
Not blocking - upgrade-guide scope (inline). The proactive close in
setConsumerapplies to every open peer that does not pass the consumer's checks, not only thehandlerscase. For example, a consumer that addsallowedRoles, a security provider oroauthProfileon a path whose producer had none. Before, those peers were closed on their first message; now they are closed when the consumer starts. One sentence in the note would cover it. -
Optional - test (inline). In
connectionAcceptedBeforeAConsumerWithHandlersStartedIsNotServed, thehellosend is fire-and-forget androuteInvocationsis checked right after the close arrives. The proactive close almost always wins, so the inbound rejection is not really exercised there, and the0would also pass if the message were still in flight. It is fine as a lifecycle test, but the comment overstates it a little. A small extra case would guard the new close step against closing legitimate clients: a connection that ran the consumer handlers stays open and is served afterstopRoute("guarded")+startRoute("guarded"). -
Optional - follow-up.
addProduceron a producer-only path can make the path stricter (a second producer withallowedRoles).send()then filters the existing peers out, but they are not closed. It is the same receive-only symptom as the one fixed here, without a security impact.closePeersThatNoLongerPassChecks()could be called there too. Fine as a separate JIRA.
Claude Code on behalf of @davsclaus.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| the consumer was started, the connection was still treated as having passed the consumer checks, so its messages | ||
| reached the route and it kept receiving what the producer sent. | ||
|
|
||
| Once a consumer that configures `handlers` is started on a path, a connection opened on that path before the consumer |
There was a problem hiding this comment.
Not blocking: the close at consumer start applies to every open peer that does not pass the consumer's checks (for example a consumer that adds allowedRoles, a security provider or oauthProfile on a path whose producer had none). Before, those were only closed on their first message. Worth one sentence, so users with receive-only clients are not surprised.
|
|
||
| assertEquals(CloseMessage.MSG_VIOLATES_POLICY, listener.closeCode.orTimeout(10, TimeUnit.SECONDS).join()); | ||
| assertTrue(listener.received.isEmpty()); | ||
| assertEquals(0, routeInvocations.get()); |
There was a problem hiding this comment.
Optional: the hello above is fire-and-forget, and the close usually reaches the client before it is sent, so this 0 does not really prove the inbound rejection. It would also pass while the message is still in flight. Fine as a lifecycle check. Maybe add a case where a connection that ran the consumer handlers stays open and is served across stopRoute/startRoute, so the new close step cannot start closing legitimate clients unnoticed.
luigidemasi
left a comment
luigidemasi
left a comment
There was a problem hiding this comment.
Re-reviewed commit f041456ba5b03aa2d61be235b21b219061c0c138. Both of my previous change requests are addressed, with no remaining blocking findings.
Consumer startup now proactively closes stale peers, including receive-only clients. The latest commit fixes both remaining tests to tolerate a closed-socket send while retaining the close-code 1008, no-delivery and route-invocation assertions.
Local validation on this exact head: the full camel-undertow suite passed with retries disabled — 238 tests, zero failures/errors, one skipped.
Approving the code; merge once pending CI passes. At publication, the Java 21 build and documentation validation remain queued. Dependency review, license checking and PR ID upload passed; Java 25 and Sonar were skipped.
AI-generated review by Codex on behalf of @luigidemasi.
…ccepted while only a producer served the path is not treated as having passed a later consumer's custom handlers (#28505) CAMEL-25917: camel-undertow - a WebSocket connection accepted while only a producer served the path is not treated as having passed a later consumer's custom handlers (#28237) * CAMEL-25917: camel-undertow - a WebSocket connection accepted while only a producer served the path is not treated as having passed a later consumer's custom handlers * CAMEL-25917: close WebSocket peers that no longer pass the path checks when the consumer starts Close the already-open connections of a path right away when a consumer is set, rather than only when they next send a message, so a receive-only client is told to reconnect instead of silently receiving nothing after the consumer with custom handlers starts. Add a regression test that awaits the close without the client sending, and adjust the existing window tests for the now proactive close. * CAMEL-25917: camel-undertow - make the window tests tolerate the proactive close The consumer now closes a connection opened before it started as soon as it starts, so sending on that connection afterwards is best effort and bounded. (cherry picked from commit 56aa5f4)
…ccepted while only a producer served the path is not treated as having passed a later consumer's custom handlers (#28506) CAMEL-25917: camel-undertow - a WebSocket connection accepted while only a producer served the path is not treated as having passed a later consumer's custom handlers (#28237) * CAMEL-25917: camel-undertow - a WebSocket connection accepted while only a producer served the path is not treated as having passed a later consumer's custom handlers * CAMEL-25917: close WebSocket peers that no longer pass the path checks when the consumer starts Close the already-open connections of a path right away when a consumer is set, rather than only when they next send a message, so a receive-only client is told to reconnect instead of silently receiving nothing after the consumer with custom handlers starts. Add a regression test that awaits the close without the client sending, and adjust the existing window tests for the now proactive close. * CAMEL-25917: camel-undertow - make the window tests tolerate the proactive close The consumer now closes a connection opened before it started as soon as it starts, so sending on that connection afterwards is best effort and bounded. (cherry picked from commit 56aa5f4)
CAMEL-25917: camel-undertow WebSocket consumer handlers and a connection opened before the consumer started
Problem
In
camel-undertow, the consumer and the producers of a WebSocket path share oneCamelWebSocketHandler. When ahandshake passes the security checks of the path, the handler records, per channel, the set of endpoint URIs whose
checks passed. The checks depend on whether a consumer is registered: the custom
handlersan endpoint configures arerun only for consumers, so while only a producer serves the path they are skipped.
The recorded result did not note which mode the handshake ran in. On a path whose producer applies a security provider
or allowed roles, a connection could be accepted while only the producer served the path, before its consumer was
started for the first time. That handshake ran the producer checks and recorded the endpoint URI, but not the custom
handlers. Once a consumer configuringhandlerson the same endpoint URI was started, the recorded URI was treatedas proof that the consumer checks had passed, so messages from that connection reached the consumer route and the
connection kept receiving what the producer sent, without the consumer
handlersever being applied to it.This follows up on CAMEL-25375, which made the
handlers, security provider and allowed roles apply to WebSocketconsumers.
Change
HandshakeResultnow records whether the handshake ran in consumer mode.isAuthenticated(...)requires, for anendpoint whose consumer configures
handlers, that the recorded result came from a consumer-mode handshake; a resultrecorded while only producers served the path is no longer sufficient. Producer-only behaviour is unchanged (producers
still do not run
handlers), and connections that go through the consumer handlers are served as before.A connection accepted before a consumer with
handlerswas first started is now not served once that consumer is up:its messages no longer reach the route and it no longer receives producer output, and it is closed, exactly as the
existing fail-closed handling already does for the security-provider and OAuth cases. Clients opening a connection in
that window must reconnect after the consumer has started. An upgrade-guide note for 4.24 describes this.
Tests
New
SecurityProviderWebSocketConsumerHandlersWindowTestincomponents/camel-undertow:connectionAcceptedBeforeAConsumerWithHandlersStartedIsNotServed- a path whose producer appliesallowedRolesandwhose consumer adds
handlerson the same endpoint URI; a connection opened while only the producer serves the pathis closed once the consumer starts, its messages do not reach the route, and it receives no producer broadcast. This
test fails before the change (the connection is served and never closed) and passes after it.
connectionThatRunsTheConsumerHandlersIsServed- a regression guard: a connection that goes through the consumerhandlers after the consumer started is served.
The full
camel-undertowmodule test suite passes.