davsclaus commented on code in PR #28237: URL: https://github.com/apache/camel/pull/28237#discussion_r4348909787
########## docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc: ########## @@ -1376,6 +1376,19 @@ framing, are still allowed. Routes that put a percent-encoded control character `CamelHttpPath` or `CamelHttpQuery` header, which the producer decodes before building the request, are the ones most likely to be affected. +=== camel-undertow - a WebSocket connection opened before a consumer with custom handlers started is no longer served + +On a WebSocket 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. Such a connection ran the +producer checks but not the custom `handlers` the consumer configures, as those handlers run only for consumers. Once +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 Review 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. ########## components/camel-undertow/src/test/java/org/apache/camel/component/undertow/spi/SecurityProviderWebSocketConsumerHandlersWindowTest.java: ########## @@ -0,0 +1,170 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.component.undertow.spi; + +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.WebSocket; +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionStage; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import io.undertow.websockets.core.CloseMessage; +import org.apache.camel.CamelContext; +import org.apache.camel.builder.RouteBuilder; +import org.apache.camel.component.mock.MockEndpoint; +import org.apache.camel.component.undertow.UndertowBasicAuthHandler; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * A WebSocket path whose producer applies the allowed roles while its consumer, started later, adds its own handlers on + * the same endpoint URI: a connection accepted while only the producer served the path runs the allowed-roles check but + * not the consumer handlers, so once that consumer is up the connection is neither served by the route nor sent + * messages, while a connection that goes through the consumer handlers is served. + */ +class SecurityProviderWebSocketConsumerHandlersWindowTest extends AbstractSecurityProviderTest { + + private final AtomicInteger routeInvocations = new AtomicInteger(); + + @Override + protected CamelContext createCamelContext() throws Exception { + CamelContext context = super.createCamelContext(); + context.getRegistry().bind("wsHandlers", new UndertowBasicAuthHandler()); + return context; + } + + @Override + protected RouteBuilder createRouteBuilder() { + return new RouteBuilder() { + @Override + public void configure() { + // producer and consumer share the same endpoint URI: the producer keeps the shared handler + // registered and applies allowedRoles, the consumer adds the handlers on top + from("direct:feed") + .to("undertow:ws://localhost:{{port}}/guarded?allowedRoles=user&handlers=#wsHandlers&sendToAll=true"); + from("undertow:ws://localhost:{{port}}/guarded?allowedRoles=user&handlers=#wsHandlers&sendToAll=true") + .routeId("guarded").autoStartup(false) + .process(exchange -> routeInvocations.incrementAndGet()) + .to("mock:guarded"); + } + }; + } + + @Test + void connectionAcceptedBeforeAConsumerWithHandlersStartedIsNotServed() throws Exception { + securityConfiguration.setRoleToAssign("user"); + + // only the producer serves the path, so the handshake runs the allowed-roles check but not the consumer handlers + RecordingListener listener = new RecordingListener(); + WebSocket webSocket = connect("/guarded", null, listener); + + context.getRouteController().startRoute("guarded"); + + // the connection does not get what the producer sends + template.sendBody("direct:feed", "broadcast"); + // a message from the stale connection never reaches the route; the send may already fail because the + // connection was closed when the consumer started, so it is best effort here + webSocket.sendText("hello", true).whenComplete((result, error) -> { + }); + + assertEquals(CloseMessage.MSG_VIOLATES_POLICY, listener.closeCode.orTimeout(10, TimeUnit.SECONDS).join()); + assertTrue(listener.received.isEmpty()); + assertEquals(0, routeInvocations.get()); Review 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. -- 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]
