Merge pull request #12072 from jetty/fix/12.0.x/ee9-form-regressions

Addressing more regressions on form handling in ee9 / ee8 environments

jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java

@@ -2050,7 +2050,12 @@ private Collection<Part> getParts(Fields params) throws IOException
             {
                 parts = _multiParts.getParts();
             }
-            catch (IOException e)
+            catch (BadMessageException e)
+            {
+                throw e;
+            }
+            // Catch RuntimeException to handle IllegalStateException, IllegalArgumentException, CharacterEncodingException, etc .. (long list)
+            catch (RuntimeException | IOException e)
             {
                 throw new BadMessageException("Unable to parse form content", e);
             }

jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/FormTest.java

@@ -13,9 +13,12 @@
 
 package org.eclipse.jetty.ee9.servlet;
 
+import java.io.IOException;
+import java.io.PrintWriter;
 import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
+import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
 import java.util.function.Function;
@@ -28,6 +31,7 @@
 import org.eclipse.jetty.client.ContentResponse;
 import org.eclipse.jetty.client.FormRequestContent;
 import org.eclipse.jetty.client.HttpClient;
+import org.eclipse.jetty.client.StringRequestContent;
 import org.eclipse.jetty.ee9.nested.ContextHandler;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpMethod;
@@ -42,6 +46,8 @@
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 public class FormTest
@@ -229,4 +235,98 @@ protected void service(HttpServletRequest request, HttpServletResponse response)
 
         assertEquals(HttpStatus.OK_200, response.getStatus());
     }
+
+    public static Stream<Arguments> invalidForm()
+    {
+        return Stream.of(
+            Arguments.of("%A", "java.lang.IllegalArgumentException: Not valid encoding &apos;%A?&apos;"),
+            Arguments.of("name%",  "java.lang.IllegalArgumentException: Not valid encoding &apos;%??&apos;"),
+            Arguments.of("name%A",  "java.lang.IllegalArgumentException: Not valid encoding &apos;%A?&apos;"),
+            Arguments.of("name%A&",  "java.lang.IllegalArgumentException: Not valid encoding &apos;%A&amp;&apos;"),
+            Arguments.of("name=%",  "java.lang.IllegalArgumentException: Not valid encoding &apos;%??&apos;"),
+            Arguments.of("name=A%%A",  "java.lang.IllegalArgumentException: Not valid encoding &apos;%%A&apos;"),
+            Arguments.of("name=A%%3D",  "java.lang.IllegalArgumentException: Not valid encoding &apos;%%3&apos;"),
+            Arguments.of("%=",  "java.lang.IllegalArgumentException: Not valid encoding &apos;%=?&apos;"),
+            Arguments.of("name=%A",  "java.lang.IllegalArgumentException: Not valid encoding &apos;%A?&apos;"),
+            Arguments.of("name=value%A",  "ava.lang.IllegalArgumentException: Not valid encoding &apos;%A?&apos;"),
+            Arguments.of("n%AH=v",  "java.lang.IllegalArgumentException: Not valid encoding &apos;%AH&apos;"),
+            Arguments.of("n=v%AH",  "java.lang.IllegalArgumentException: Not valid encoding &apos;%AH&apos;")
+        );
+    }
+
+    @ParameterizedTest
+    @MethodSource("invalidForm")
+    public void testContentTypeWithoutCharsetDecodeBadUTF8(String rawForm, String expectedCauseMessage) throws Exception
+    {
+        start(handler -> new HttpServlet()
+        {
+            @Override
+            protected void service(HttpServletRequest request, HttpServletResponse response)
+            {
+                // This is expected to throw an exception due to the bad form input
+                request.getParameterMap();
+            }
+        });
+
+        StringRequestContent requestContent = new StringRequestContent("application/x-www-form-urlencoded", rawForm);
+
+        ContentResponse response = client.newRequest("localhost", connector.getLocalPort())
+            .method(HttpMethod.POST)
+            .path(contextPath + servletPath)
+            .body(requestContent)
+            .timeout(5, TimeUnit.SECONDS)
+            .send();
+
+        assertEquals(HttpStatus.BAD_REQUEST_400, response.getStatus(), response::getContentAsString);
+        String responseContent = response.getContentAsString();
+        assertThat(responseContent, containsString("Unable to parse form content"));
+        assertThat(responseContent, containsString(expectedCauseMessage));
+    }
+
+    public static Stream<Arguments> utf8Form()
+    {
+        return Stream.of(
+            Arguments.of("euro=%E2%82%AC", List.of("param[euro] = \"€\"")),
+            Arguments.of("name=%AB%CD", List.of("param[name] = \"��\"")),
+            Arguments.of("name=x%AB%CDz", List.of("param[name] = \"x��z\"")),
+            Arguments.of("name=%FF%FF%FF%FF", List.of("param[name] = \"����\""))
+        );
+    }
+
+    @ParameterizedTest
+    @MethodSource("utf8Form")
+    public void testUtf8Decoding(String rawForm, List<String> expectedParams) throws Exception
+    {
+        start(handler -> new HttpServlet()
+        {
+            @Override
+            protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException
+            {
+                response.setCharacterEncoding("utf-8");
+                response.setContentType("text/plain");
+                PrintWriter out = response.getWriter();
+
+                Map<String, String[]> paramMap = request.getParameterMap();
+                List<String> names = paramMap.keySet().stream().sorted().toList();
+                for (String name: names)
+                {
+                    out.printf("param[%s] = \"%s\"%n", name, String.join(",", paramMap.get(name)));
+                }
+            }
+        });
+
+        StringRequestContent requestContent = new StringRequestContent("application/x-www-form-urlencoded", rawForm);
+
+        ContentResponse response = client.newRequest("localhost", connector.getLocalPort())
+            .method(HttpMethod.POST)
+            .path(contextPath + servletPath)
+            .body(requestContent)
+            .timeout(5, TimeUnit.SECONDS)
+            .send();
+
+        assertEquals(HttpStatus.OK_200, response.getStatus(), response::getContentAsString);
+        String responseContent = response.getContentAsString();
+        for (String expectedParam: expectedParams)
+            assertThat(responseContent, containsString(expectedParam));
+    }
 }

jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/MultiPartServletTest.java

@@ -15,6 +15,7 @@
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.PrintWriter;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
@@ -104,18 +105,27 @@ public static class MultiPartServlet extends HttpServlet
         @Override
         protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
         {
+            resp.setCharacterEncoding("utf-8");
+            resp.setContentType("text/plain");
+
+            PrintWriter out = resp.getWriter();
+
             if (!req.getContentType().contains(MimeTypes.Type.MULTIPART_FORM_DATA.asString()))
             {
-                resp.setContentType("text/plain");
-                resp.getWriter().println("not content type " + MimeTypes.Type.MULTIPART_FORM_DATA);
-                resp.getWriter().println("contentType: " + req.getContentType());
+                out.println("not content type " + MimeTypes.Type.MULTIPART_FORM_DATA);
+                out.println("contentType: " + req.getContentType());
                 return;
             }
 
-            resp.setContentType("text/plain");
             for (Part part : req.getParts())
             {
-                resp.getWriter().println("Part: name=" + part.getName() + ", size=" + part.getSize());
+                out.printf("Part: name=%s, size=%s", part.getName(), part.getSize());
+                if (part.getSize() <= 100)
+                {
+                    String content = IO.toString(part.getInputStream());
+                    out.printf(", content=%s", content);
+                }
+                out.println();
             }
         }
     }
@@ -302,6 +312,7 @@ public void testLineFeedCarriageReturnEOL(MultiPartCompliance multiPartComplianc
         startServer(multiPartCompliance);
 
         String contentType = "multipart/form-data; boundary=---------------------------7e25e1e151054";
+        // NOTE: The extra `\r` here are intentional, do not remove.
         String rawForm = """
             -----------------------------7e25e1e151054\r
             Content-Disposition: form-data; name="user"\r
@@ -370,6 +381,131 @@ public void testAllWhitespaceForm(MultiPartCompliance multiPartCompliance) throw
         });
     }
 
+    /**
+     * A part with Content-Transfer-Encoding: base64, and the content is valid Base64 encoded.
+     *
+     * MultiPartCompliance mode set to allow MultiPartCompliance.Violation.BASE64_TRANSFER_ENCODING
+     */
+    @Test
+    public void testLegacyContentTransferEncodingBase64Allowed() throws Exception
+    {
+        MultiPartCompliance legacyBase64 = MultiPartCompliance.from("LEGACY,BASE64_TRANSFER_ENCODING");
+
+        startServer(legacyBase64);
+
+        String contentType = "multipart/form-data; boundary=8GbcZNTauFWYMt7GeM9BxFMdlNBJ6aLJhGdXp";
+        String rawForm = """
+            --8GbcZNTauFWYMt7GeM9BxFMdlNBJ6aLJhGdXp
+            Content-ID: <[email protected]>
+            Content-Disposition: form-data; name="quote"
+            Content-Transfer-Encoding: base64
+            
+            IkJvb2tzIGFyZSB0aGUgbGliZXJhdGVkIHNwaXJpdHMgb2YgbWVuLiIgLS0gTWFyayBUd2Fpbg==
+            --8GbcZNTauFWYMt7GeM9BxFMdlNBJ6aLJhGdXp--
+            
+            """;
+
+        StringRequestContent form = new StringRequestContent(
+            contentType,
+            rawForm
+        );
+
+        ContentResponse response = client.newRequest("localhost", connector.getLocalPort())
+            .path("/")
+            .scheme(HttpScheme.HTTP.asString())
+            .method(HttpMethod.POST)
+            .body(form)
+            .send();
+
+        assertEquals(200, response.getStatus());
+        assertThat(response.getContentAsString(), containsString("Part: name=quote, size=55, content=\"Books are the liberated spirits of men.\" -- Mark Twain"));
+    }
+
+    /**
+     * A part with Content-Transfer-Encoding: base64, but the content is not actually encoded in Base 64.
+     *
+     * MultiPartCompliance mode set to allow MultiPartCompliance.Violation.BASE64_TRANSFER_ENCODING
+     */
+    @Test
+    public void testLegacyContentTransferEncodingBadBase64Allowed() throws Exception
+    {
+        MultiPartCompliance legacyBase64 = MultiPartCompliance.from("LEGACY,BASE64_TRANSFER_ENCODING");
+
+        startServer(legacyBase64);
+
+        String contentType = "multipart/form-data; boundary=8GbcZNTauFWYMt7GeM9BxFMdlNBJ6aLJhGdXp";
+        String rawForm = """
+            --8GbcZNTauFWYMt7GeM9BxFMdlNBJ6aLJhGdXp
+            Content-ID: <[email protected]>
+            Content-Disposition: form-data; name="quote"
+            Content-Transfer-Encoding: base64
+            
+            "Travel is fatal to prejudice." -- Mark Twain
+            --8GbcZNTauFWYMt7GeM9BxFMdlNBJ6aLJhGdXp--
+            
+            """;
+
+        StringRequestContent form = new StringRequestContent(
+            contentType,
+            rawForm
+        );
+
+        InputStreamResponseListener listener = new InputStreamResponseListener();
+        client.newRequest("localhost", connector.getLocalPort())
+            .path("/")
+            .scheme(HttpScheme.HTTP.asString())
+            .method(HttpMethod.POST)
+            .body(form)
+            .send(listener);
+
+        assert400orEof(listener, responseContent ->
+        {
+            assertThat(responseContent, containsString("Unable to parse form content"));
+            assertThat(responseContent, containsString("java.lang.IllegalArgumentException: Last unit does not have enough valid bits"));
+        });
+    }
+
+    /**
+     * A part with Content-Transfer-Encoding: base64, and the content is valid Base64 encoded.
+     *
+     * MultiPartCompliance mode set to allow MultiPartCompliance.LEGACY, which does not perform
+     * base64 decoding.
+     */
+    @Test
+    public void testLegacyContentTransferEncodingBase64() throws Exception
+    {
+        MultiPartCompliance legacyBase64 = MultiPartCompliance.LEGACY;
+
+        startServer(legacyBase64);
+
+        String contentType = "multipart/form-data; boundary=8GbcZNTauFWYMt7GeM9BxFMdlNBJ6aLJhGdXp";
+        String rawForm = """
+            --8GbcZNTauFWYMt7GeM9BxFMdlNBJ6aLJhGdXp
+            Content-ID: <[email protected]>
+            Content-Disposition: form-data; name="quote"
+            Content-Transfer-Encoding: base64
+            
+            IkJvb2tzIGFyZSB0aGUgbGliZXJhdGVkIHNwaXJpdHMgb2YgbWVuLiIgLS0gTWFyayBUd2Fpbg==
+            --8GbcZNTauFWYMt7GeM9BxFMdlNBJ6aLJhGdXp--
+            
+            """;
+
+        StringRequestContent form = new StringRequestContent(
+            contentType,
+            rawForm
+        );
+
+        ContentResponse response = client.newRequest("localhost", connector.getLocalPort())
+            .path("/")
+            .scheme(HttpScheme.HTTP.asString())
+            .method(HttpMethod.POST)
+            .body(form)
+            .send();
+
+        assertEquals(200, response.getStatus());
+        assertThat(response.getContentAsString(), containsString("Part: name=quote, size=76, content=IkJvb2tzIGFyZSB0aGUgbGliZXJhdGVkIHNwaXJpdHMgb2YgbWVuLiIgLS0gTWFyayBUd2Fpbg=="));
+    }
+
     @ParameterizedTest
     @MethodSource("multipartModes")
     public void testManyParts(MultiPartCompliance multiPartCompliance) throws Exception
@@ -487,7 +623,7 @@ public void testTempFilesDeletedOnError(MultiPartCompliance multiPartCompliance)
                 .body(multiPart)
                 .send();
 
-            assertEquals(500, response.getStatus());
+            assertEquals(400, response.getStatus());
             assertThat(response.getContentAsString(),
                 containsString("Multipart Mime part largePart exceeds max filesize"));
         }