This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 22d114371e8c313c3280822f53c3ee9b7d0185d4 Author: opencode <[email protected]> AuthorDate: Sat Oct 17 14:24:56 2026 +0200 Reject WebSocket upgrade requests with an invalid percent-escape sequence in the query string with a 400 response Invalid percent-escape sequences in a query string are not rejected by the HTTP parser since the individual characters are legal. Escape pairs are only checked when the query string is decoded. Building the absolute request URI for the handshake in WsHandshakeRequest uses java.net.URI with the raw query string, so a sequence such as "?x=%ZZ" resulted in a URISyntaxException that was wrapped in an IllegalArgumentException which nothing caught. The uncaught runtime exception propagated out of the filter chain and produced an HTTP 500 response and a SEVERE container log entry with a stack trace for every such request from an untrusted client, instead of the 400 Bad Request response that every other handshake validation failure produces. UpgradeUtil.doUpgrade() now catches the failure to construct the WsHandshakeRequest and rejects the handshake with a 400 response. The response is reset before the error is sent so the rejection is not accompanied by the Upgrade, Connection and Sec-WebSocket-Accept headers that indicate a successful handshake. Adds TestHandshakeRequest covering invalid escape pairs, incomplete escapes and trailing percent characters in the query string (400), as well as valid escapes and an empty query string (101). Note that a query string that is valid URI syntax but undecodable as UTF-8, such as "?x=%C0%80", is already rejected with a 400 through the separate InvalidParameterException handling in the valve layer; that behavior is pinned by a test as well. --- .../tomcat/websocket/server/UpgradeUtil.java | 13 ++- .../websocket/server/WsHandshakeRequest.java | 4 +- .../websocket/server/TestHandshakeRequest.java | 109 +++++++++++++++++++++ 3 files changed, 124 insertions(+), 2 deletions(-) diff --git a/java/org/apache/tomcat/websocket/server/UpgradeUtil.java b/java/org/apache/tomcat/websocket/server/UpgradeUtil.java index 2363a7b7f1..9b6e4013ee 100644 --- a/java/org/apache/tomcat/websocket/server/UpgradeUtil.java +++ b/java/org/apache/tomcat/websocket/server/UpgradeUtil.java @@ -226,7 +226,18 @@ public class UpgradeUtil { WsPerSessionServerEndpointConfig perSessionServerEndpointConfig = new WsPerSessionServerEndpointConfig(sec); - WsHandshakeRequest wsRequest = new WsHandshakeRequest(req, pathParams); + WsHandshakeRequest wsRequest; + try { + wsRequest = new WsHandshakeRequest(req, pathParams); + } catch (IllegalArgumentException iae) { + // The request target could not be represented as a URI, for example because the query + // string contains an invalid percent-escape sequence. Reject the handshake. The + // response is reset first so the rejection is not sent with headers that would + // indicate a successful handshake. + resp.reset(); + resp.sendError(HttpServletResponse.SC_BAD_REQUEST); + return; + } WsHandshakeResponse wsResponse = new WsHandshakeResponse(); sec.getConfigurator().modifyHandshake(perSessionServerEndpointConfig, wsRequest, wsResponse); wsRequest.finished(); diff --git a/java/org/apache/tomcat/websocket/server/WsHandshakeRequest.java b/java/org/apache/tomcat/websocket/server/WsHandshakeRequest.java index 9a3c6656c7..ed249beb03 100644 --- a/java/org/apache/tomcat/websocket/server/WsHandshakeRequest.java +++ b/java/org/apache/tomcat/websocket/server/WsHandshakeRequest.java @@ -181,7 +181,9 @@ public class WsHandshakeRequest implements HandshakeRequest { try { return new URI(uri.toString()); } catch (URISyntaxException e) { - // Should never happen + // This can happen with a client-supplied query string that contains an invalid + // percent-escape sequence. Upgrade rejects the handshake with a 400 response in + // this case. throw new IllegalArgumentException(sm.getString("wsHandshakeRequest.invalidUri", uri.toString()), e); } } diff --git a/test/org/apache/tomcat/websocket/server/TestHandshakeRequest.java b/test/org/apache/tomcat/websocket/server/TestHandshakeRequest.java new file mode 100644 index 0000000000..b435f38137 --- /dev/null +++ b/test/org/apache/tomcat/websocket/server/TestHandshakeRequest.java @@ -0,0 +1,109 @@ +/* + * 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.tomcat.websocket.server; + +import java.nio.charset.StandardCharsets; + +import jakarta.servlet.http.HttpServletResponse; +import jakarta.websocket.CloseReason.CloseCodes; + +import org.junit.Assert; +import org.junit.Test; + +import org.apache.tomcat.websocket.TesterEchoServer; +import org.apache.tomcat.websocket.WebSocketBaseTest; + +/* + * Invalid percent-escape sequences in the query string are not rejected by the HTTP parser (they are only checked + * when the query string is decoded). The upgrade processing must reject such requests with a 400 response rather than + * failing with an uncaught IllegalArgumentException. + */ +public class TestHandshakeRequest extends WebSocketBaseTest { + + @Test + public void testInvalidEscapePair() throws Exception { + doTest("?x=%ZZ", HttpServletResponse.SC_BAD_REQUEST); + } + + + @Test + public void testIncompleteEscape() throws Exception { + doTest("?x=%2", HttpServletResponse.SC_BAD_REQUEST); + } + + + @Test + public void testTrailingPercent() throws Exception { + doTest("?x=%", HttpServletResponse.SC_BAD_REQUEST); + } + + + @Test + public void testLowerCaseInvalidEscapePair() throws Exception { + doTest("?x=%zz", HttpServletResponse.SC_BAD_REQUEST); + } + + + @Test + public void testInvalidEscapeInOneOfMultipleParameters() throws Exception { + doTest("?x=1&y=%GG", HttpServletResponse.SC_BAD_REQUEST); + } + + + /* + * URI syntax is valid here so the request reaches query string decoding. That mechanism already rejects the + * invalid UTF-8 sequence with a 400 response. Pinned here since it is a different code path. + */ + @Test + public void testInvalidUtf8Escape() throws Exception { + doTest("?x=%C0%80", HttpServletResponse.SC_BAD_REQUEST); + } + + + @Test + public void testEmptyQueryString() throws Exception { + doTest("?", HttpServletResponse.SC_SWITCHING_PROTOCOLS); + } + + + @Test + public void testValidEscape() throws Exception { + doTest("?x=%20", HttpServletResponse.SC_SWITCHING_PROTOCOLS); + } + + + private void doTest(String queryString, int expectedStatusCode) throws Exception { + startServer(TesterEchoServer.Config.class); + + TesterWsClient client = new TesterWsClient("localhost", getPort()); + String req = client.createUpgradeRequest(TesterEchoServer.Config.PATH_BASIC + queryString); + client.write(req.getBytes(StandardCharsets.UTF_8)); + int rc = client.readUpgradeResponse(); + + Assert.assertEquals(expectedStatusCode, rc); + + if (expectedStatusCode == HttpServletResponse.SC_SWITCHING_PROTOCOLS) { + client.sendCloseFrame(CloseCodes.NORMAL_CLOSURE); + // Read (and ignore) the response + byte[] buf = new byte[256]; + while (client.read(buf) > 0) { + // Ignore + } + } + client.closeSocket(); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
