[UNDERTOW-2312] Fix ALLOW_UNESCAPED_CHARACTERS_IN_URL with query stri…

…ngs in Http2 upgrade Also: add a test to LotsOfQueryParametersTestCase to test what happens when that test runs without that option enabled The new code caused ALLOW_UNESCAPED_CHARACTERS_IN_URL to not work correctly in HTTP2 upgrade scenarios when the characters are in the query. The reason for that is that Http2UpgradeHandler passes decode as false when asking Connectors to parse the request path, assuming that otherwise the decode would have been done twice. While that may the true for the parsing of the path, it is not the case when Connectors is parsing query params to set them one-by-one in the HttpServerExchange.This caused LotsOfQueryParametersTestCase to fail when run with HTTP2 Upgrade config. Signed-off-by: Flavia Rainone <[email protected]>
undertow-io · Aug 21, 2024 · 189e340 · 189e340
1 parent d66f660
commit 189e340
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 15 deletions.
diff --git a/core/src/main/java/io/undertow/server/Connectors.java b/core/src/main/java/io/undertow/server/Connectors.java
@@ -477,12 +477,34 @@ public static void setExchangeRequestPath(final HttpServerExchange exchange, fin
     /**
      * Sets the request path and query parameters, decoding to the requested charset.
      *
-     * @param exchange    The exchange
-     * @param encodedPath The encoded path
-     * @param charset     The charset
-     * @throws BadRequestException
+     * @param exchange             the exchange
+     * @param encodedPath          the encoded path
+     * @param decode               indicates if the request path should be decoded
+     * @param decodeSlashFlag      indicates if slash characters contained in the encoded path should be decoded
+     * @param decodeBuffer         the buffer used for decoding
+     * @param maxParameters        maximum number of parameters allowed in the path
+     * @param charset                  the charset
+     * @throws BadRequestException if there is something wrong with the request, such as non-allowed characters
      */
     public static void setExchangeRequestPath(final HttpServerExchange exchange, final String encodedPath, final String charset, boolean decode, final boolean decodeSlashFlag, StringBuilder decodeBuffer, int maxParameters) throws ParameterLimitException, BadRequestException {
+        setExchangeRequestPath(exchange, encodedPath, charset, decode, decode, decodeSlashFlag, decodeBuffer, maxParameters);
+    }
+
+    /**
+     * Sets the request path and query parameters, decoding to the requested charset.
+     *
+     * @param exchange             the exchange
+     * @param encodedPath          the encoded path
+     * @param decode               indicates if the request path should be decoded, apart from the query string part of the
+     *                             request (see next parameter)
+     * @param decodeQueryString    indicates if the query string of the path, when present, should be decoded
+     * @param decodeSlashFlag      indicates if slash characters contained in the request path should be decoded
+     * @param decodeBuffer         the buffer used for decoding
+     * @param maxParameters        maximum number of parameters allowed in the path
+     * @param charset                  the charset
+     * @throws BadRequestException if there is something wrong with the request, such as non-allowed characters
+     */
+    public static void setExchangeRequestPath(final HttpServerExchange exchange, final String encodedPath, final String charset, boolean decode, boolean decodeQueryString, final boolean decodeSlashFlag, StringBuilder decodeBuffer, int maxParameters) throws ParameterLimitException, BadRequestException {
         final OptionMap options = exchange.getConnection().getUndertowOptions();
         final boolean allowUnescapedCharactersInUrl = options.get(UndertowOptions.ALLOW_UNESCAPED_CHARACTERS_IN_URL, false);
         boolean requiresDecode = false;
@@ -520,7 +542,7 @@ public static void setExchangeRequestPath(final HttpServerExchange exchange, fin
                     exchange.setQueryString(qs);
                 }
 
-                URLUtils.parseQueryString(qs, exchange, charset, decode, maxParameters);
+                URLUtils.parseQueryString(qs, exchange, charset, decodeQueryString, maxParameters);
                 return;
             } else if(c == ';') {
                 String part;

diff --git a/core/src/main/java/io/undertow/server/protocol/http2/Http2ReceiveListener.java b/core/src/main/java/io/undertow/server/protocol/http2/Http2ReceiveListener.java
@@ -218,15 +218,19 @@ public void handleEvent(Http2StreamSourceChannel channel) {
      * @param initial The initial upgrade request that started the HTTP2 connection
      */
     void handleInitialRequest(HttpServerExchange initial, Http2Channel channel, byte[] data) {
-        handleInitialRequest(initial, channel, data, this.decode);
+        handleInitialRequest(initial, channel, data, this.decode, this.decode);
     }
 
     /**
-      * Handles the initial request when the exchange was started by a HTTP upgrade.
-      *
-      * @param initial The initial upgrade request that started the HTTP2 connection
-      */
-    void handleInitialRequest(HttpServerExchange initial, Http2Channel channel, byte[] data, boolean decode) {
+     * Handles the initial request when the exchange was started by a HTTP upgrade.
+     *
+     * @param initial               the initial upgrade request that started the HTTP2 connection
+     * @param channel               the channel that received the request
+     * @param data                  any extra data read by the channel that has not been parsed yet
+     * @param decode                indicates if the request path should be decoded, apart from the query string
+     * @param decodeQueryString     indicates if the query string of the path, when present, should be decoded
+     */
+    void handleInitialRequest(HttpServerExchange initial, Http2Channel channel, byte[] data, boolean decode, boolean decodeQueryString) {
         //we have a request
         Http2HeadersStreamSinkChannel sink = channel.createInitialUpgradeResponseStream();
         final Http2ServerConnection connection = new Http2ServerConnection(channel, sink, undertowOptions, bufferSize, rootHandler);
@@ -253,7 +257,7 @@ void handleInitialRequest(HttpServerExchange initial, Http2Channel channel, byte
         Connectors.terminateRequest(exchange);
         String uri = exchange.getQueryString().isEmpty() ? initial.getRequestURI() : initial.getRequestURI() + '?' + exchange.getQueryString();
         try {
-            Connectors.setExchangeRequestPath(exchange, uri, encoding, decode, slashDecodingFlag, decodeBuffer, maxParameters);
+            Connectors.setExchangeRequestPath(exchange, uri, encoding, decode, decodeQueryString, slashDecodingFlag, decodeBuffer, maxParameters);
         } catch (ParameterLimitException | BadRequestException e) {
             exchange.setStatusCode(StatusCodes.BAD_REQUEST);
             exchange.endExchange();

diff --git a/core/src/main/java/io/undertow/server/protocol/http2/Http2UpgradeHandler.java b/core/src/main/java/io/undertow/server/protocol/http2/Http2UpgradeHandler.java
@@ -176,7 +176,9 @@ public void handleRequest(HttpServerExchange exchange) throws Exception {
                 }, undertowOptions, exchange.getConnection().getBufferSize(), null);
                 channel.getReceiveSetter().set(receiveListener);
                 // don't decode requests from upgrade, they are already decoded by the parser for protocol HTTP 1.1 (HttpRequestParser)
-                receiveListener.handleInitialRequest(exchange, channel, data, false);
+                // however, the queries have to be decoded, since this is decoded only once, when the connector parses the query string
+                // to fill in the query param collection of the HttpServerExchange (see Connectors invoking URLUtils.QUERY_STRING_PARSER)
+                receiveListener.handleInitialRequest(exchange, channel, data, false, undertowOptions.get(UndertowOptions.DECODE_URL, true));
                 channel.resumeReceives();
             }
         });

diff --git a/core/src/test/java/io/undertow/server/handlers/LotsOfQueryParametersTestCase.java b/core/src/test/java/io/undertow/server/handlers/LotsOfQueryParametersTestCase.java
@@ -51,6 +51,7 @@ public class LotsOfQueryParametersTestCase {
 
     private static final String QUERY = "QUERY";
     private static final String MESSAGE = "Hello Query";
+    private static final String NON_DECODED_MESSAGE = "Hello+Query";
 
     private static final int DEFAULT_MAX_PARAMETERS = 1000;
     private static final int TEST_MAX_PARAMETERS = 10;
@@ -142,7 +143,7 @@ public void testLotsOfQueryParameters_MaxParameters_Ok() throws IOException {
             }
             qs.deleteCharAt(qs.length()-1); // delete last useless '&'
             HttpGet get = new HttpGet(DefaultServer.getDefaultServerURL() + "/path?" + qs.toString());
-            DefaultServer.setUndertowOptions(OptionMap.create(UndertowOptions.MAX_PARAMETERS, TEST_MAX_PARAMETERS));
+            DefaultServer.setUndertowOptions(OptionMap.create(UndertowOptions.MAX_PARAMETERS, TEST_MAX_PARAMETERS, UndertowOptions.ALLOW_UNESCAPED_CHARACTERS_IN_URL, true));
             HttpResponse result = client.execute(get);
             Assert.assertEquals(StatusCodes.OK, result.getStatusLine().getStatusCode());
             for (int i = 0; i < TEST_MAX_PARAMETERS; ++i) {
@@ -170,7 +171,7 @@ public void testLotsOfQueryParameters_MaxParameters_BadRequest() throws IOExcept
             }
             qs.deleteCharAt(qs.length()-1); // delete last useless '&'
             HttpGet get = new HttpGet(DefaultServer.getDefaultServerURL() + "/path?" + qs.toString());
-            DefaultServer.setUndertowOptions(OptionMap.create(UndertowOptions.MAX_PARAMETERS, TEST_MAX_PARAMETERS));
+            DefaultServer.setUndertowOptions(OptionMap.create(UndertowOptions.MAX_PARAMETERS, TEST_MAX_PARAMETERS, UndertowOptions.ALLOW_UNESCAPED_CHARACTERS_IN_URL, true));
             HttpResponse result = client.execute(get);
             Assert.assertEquals(StatusCodes.BAD_REQUEST, result.getStatusLine().getStatusCode());
         } finally {
@@ -179,4 +180,30 @@ public void testLotsOfQueryParameters_MaxParameters_BadRequest() throws IOExcept
         }
     }
 
+    @Test @AjpIgnore
+    public void testLotsOfQueryParameters_MaxParameters_Ok_UnescapedCharactersNotAllowed() throws IOException {
+        OptionMap existing = DefaultServer.getUndertowOptions();
+        TestHttpClient client = new TestHttpClient();
+        try {
+            StringBuilder qs = new StringBuilder();
+            for (int i = 0; i < TEST_MAX_PARAMETERS; ++i) {
+                qs.append(QUERY + i);
+                qs.append("=");
+                qs.append(URLEncoder.encode(MESSAGE + i, "UTF-8"));
+                qs.append("&");
+            }
+            qs.deleteCharAt(qs.length()-1); // delete last useless '&'
+            HttpGet get = new HttpGet(DefaultServer.getDefaultServerURL() + "/path?" + qs.toString());
+            DefaultServer.setUndertowOptions(OptionMap.create(UndertowOptions.MAX_PARAMETERS, TEST_MAX_PARAMETERS));
+            HttpResponse result = client.execute(get);
+            Assert.assertEquals(StatusCodes.OK, result.getStatusLine().getStatusCode());
+            for (int i = 0; i < TEST_MAX_PARAMETERS; ++i) {
+                Header[] header = result.getHeaders(QUERY + i);
+                Assert.assertEquals(NON_DECODED_MESSAGE + i, header[0].getValue());
+            }
+        } finally {
+            DefaultServer.setUndertowOptions(existing);
+            client.getConnectionManager().shutdown();
+        }
+    }
 }