From 7c42a5b3ae51201e87b196781410c13e03fe6b2c Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 16 Jul 2024 07:52:00 -0500 Subject: [PATCH] Fix Regression on ee9 / ee8 MultiPart parsing (#12031) * Regressions on ee9 / ee8 MultiPart parsing error behavior * More testing of error regressions * Multipart parsing errors return 400 (Bad Request) now, not 500 (Server Error). --- .../ee9/nested/MultiPartFormInputStream.java | 2 +- .../MultiPartInputStreamLegacyParser.java | 6 +- .../org/eclipse/jetty/ee9/nested/Request.java | 13 +- .../ee9/servlet/MultiPartServletTest.java | 207 ++++++++++++++++-- 4 files changed, 204 insertions(+), 24 deletions(-) diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartFormInputStream.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartFormInputStream.java index df32f184dd40..10bc336b0e4e 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartFormInputStream.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartFormInputStream.java @@ -632,7 +632,7 @@ else if (len == -1) if (parser.getState() != MultiPartParser.State.END) { if (parser.getState() == MultiPartParser.State.PREAMBLE) - _err = new IOException("Missing initial multi part boundary"); + _err = new IOException("Missing content for multipart request"); else _err = new IOException("Incomplete Multipart"); } diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartInputStreamLegacyParser.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartInputStreamLegacyParser.java index 3cbe41c39d8f..6bce1bc4af36 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartInputStreamLegacyParser.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartInputStreamLegacyParser.java @@ -599,7 +599,7 @@ else if ("".equals(_config.getLocation())) } if (line == null || line.length() == 0) - throw new IOException("Missing initial multi part boundary"); + throw new IOException("Missing content for multipart request"); // Empty multipart. if (line.equals(lastBoundary)) @@ -700,7 +700,7 @@ else if (tl.startsWith("filename=")) // Check if we can create a new part. _numParts++; if (_maxParts >= 0 && _numParts > _maxParts) - throw new IllegalStateException(String.format("Form with too many parts [%d > %d]", _numParts, _maxParts)); + throw new IllegalStateException(String.format("Form with too many keys [%d > %d]", _numParts, _maxParts)); //Have a new Part MultiPart part = new MultiPart(name, filename); @@ -864,7 +864,7 @@ public int read() throws IOException MultiPartCompliance.Violation.LF_LINE_TERMINATION, "0x10")); } else - throw new IOException("Incomplete parts"); + throw new IOException("Incomplete Multipart"); } catch (Exception e) { diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java index 650857f0d215..6a865ffc5f1c 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java @@ -515,7 +515,7 @@ else if (MimeTypes.Type.MULTIPART_FORM_DATA.is(baseType) && String msg = "Unable to extract content parameters"; if (LOG.isDebugEnabled()) LOG.debug(msg, e); - throw new RuntimeIOException(msg, e); + throw new BadMessageException(msg, e); } } } @@ -2044,7 +2044,16 @@ private Collection getParts(Fields params) throws IOException MultiPartCompliance multiPartCompliance = getHttpChannel().getHttpConfiguration().getMultiPartCompliance(); _multiParts = newMultiParts(multiPartCompliance, config, maxFormKeys); - Collection parts = _multiParts.getParts(); + + Collection parts; + try + { + parts = _multiParts.getParts(); + } + catch (IOException e) + { + throw new BadMessageException("Unable to parse form content", e); + } reportComplianceViolations(); String formCharset = null; diff --git a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/MultiPartServletTest.java b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/MultiPartServletTest.java index b351e8bc3e22..fbe745cee8bc 100644 --- a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/MultiPartServletTest.java +++ b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/MultiPartServletTest.java @@ -24,6 +24,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.function.Consumer; +import java.util.stream.Stream; import java.util.zip.GZIPInputStream; import jakarta.servlet.MultipartConfigElement; @@ -49,15 +50,20 @@ import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.http.MultiPart; +import org.eclipse.jetty.http.MultiPartCompliance; import org.eclipse.jetty.io.EofException; import org.eclipse.jetty.logging.StacklessLogging; +import org.eclipse.jetty.server.HttpConfiguration; +import org.eclipse.jetty.server.HttpConnectionFactory; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.server.handler.gzip.GzipHandler; import org.eclipse.jetty.util.IO; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +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; @@ -130,14 +136,15 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws S } } - @BeforeEach - public void start() throws Exception + private void startServer(MultiPartCompliance multiPartCompliance) throws Exception { tmpDir = Files.createTempDirectory(MultiPartServletTest.class.getSimpleName()); assertNotNull(tmpDir); server = new Server(); - connector = new ServerConnector(server); + HttpConfiguration httpConfiguration = new HttpConfiguration(); + httpConfiguration.setMultiPartCompliance(multiPartCompliance); + connector = new ServerConnector(server, new HttpConnectionFactory(httpConfiguration)); server.addConnector(connector); MultipartConfigElement config = new MultipartConfigElement(tmpDir.toAbsolutePath().toString(), @@ -180,9 +187,50 @@ public void stop() throws Exception IO.delete(tmpDir.toFile()); } - @Test - public void testLargePart() throws Exception + public static Stream multipartModes() + { + return Stream.of( + Arguments.of(MultiPartCompliance.RFC7578), + Arguments.of(MultiPartCompliance.LEGACY) + ); + } + + /** + * The request indicates that it is a multipart/form-data, but no body is sent. + */ + @ParameterizedTest + @MethodSource("multipartModes") + public void testEmptyBodyMultipartForm(MultiPartCompliance multiPartCompliance) throws Exception + { + startServer(multiPartCompliance); + + String contentType = "multipart/form-data; boundary=---------------boundaryXYZ123"; + StringRequestContent emptyContent = new StringRequestContent(contentType, ""); + + InputStreamResponseListener listener = new InputStreamResponseListener(); + client.newRequest("localhost", connector.getLocalPort()) + .path("/defaultConfig") + .scheme(HttpScheme.HTTP.asString()) + .method(HttpMethod.POST) + .body(emptyContent) + .send(listener); + + Response response = listener.get(60, TimeUnit.SECONDS); + assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400)); + + assert400orEof(listener, responseContent -> + { + assertThat(responseContent, containsString("Unable to parse form content")); + assertThat(responseContent, containsString("Missing content for multipart request")); + }); + } + + @ParameterizedTest + @MethodSource("multipartModes") + public void testLargePart(MultiPartCompliance multiPartCompliance) throws Exception { + startServer(multiPartCompliance); + OutputStreamRequestContent content = new OutputStreamRequestContent(); MultiPartRequestContent multiPart = new MultiPartRequestContent(); multiPart.addPart(new MultiPart.ContentSourcePart("param", null, null, content)); @@ -212,9 +260,122 @@ public void testLargePart() throws Exception }); } - @Test - public void testManyParts() throws Exception + @ParameterizedTest + @MethodSource("multipartModes") + public void testIncompleteMultipart(MultiPartCompliance multiPartCompliance) throws Exception + { + startServer(multiPartCompliance); + + String contentType = "multipart/form-data; boundary=-------------------------7e21c038151054"; + String incompleteForm = """ + ---------------------------7e21c038151054 + Content-Disposition: form-data; name="description" + + Some data, but incomplete + ---------------------------7e21c038151054 + Content-Disposition: form-d"""; // intentionally incomplete + + StringRequestContent incomplete = new StringRequestContent( + contentType, + incompleteForm + ); + + InputStreamResponseListener listener = new InputStreamResponseListener(); + client.newRequest("localhost", connector.getLocalPort()) + .path("/defaultConfig") + .scheme(HttpScheme.HTTP.asString()) + .method(HttpMethod.POST) + .body(incomplete) + .send(listener); + + assert400orEof(listener, responseContent -> + { + assertThat(responseContent, containsString("Unable to parse form content")); + assertThat(responseContent, containsString("Incomplete Multipart")); + }); + } + + @ParameterizedTest + @MethodSource("multipartModes") + public void testLineFeedCarriageReturnEOL(MultiPartCompliance multiPartCompliance) throws Exception + { + startServer(multiPartCompliance); + + String contentType = "multipart/form-data; boundary=---------------------------7e25e1e151054"; + String rawForm = """ + -----------------------------7e25e1e151054\r + Content-Disposition: form-data; name="user"\r + \r + anotheruser\r + -----------------------------7e25e1e151054\r + Content-Disposition: form-data; name="comment"\r + \r + with something to say\r + -----------------------------7e25e1e151054--\r + """; + + StringRequestContent form = new StringRequestContent( + contentType, + rawForm + ); + + InputStreamResponseListener listener = new InputStreamResponseListener(); + client.newRequest("localhost", connector.getLocalPort()) + .path("/defaultConfig") + .scheme(HttpScheme.HTTP.asString()) + .method(HttpMethod.POST) + .body(form) + .send(listener); + + assert400orEof(listener, responseContent -> + { + assertThat(responseContent, containsString("Unable to parse form content")); + if (multiPartCompliance == MultiPartCompliance.RFC7578) + { + assertThat(responseContent, containsString("Illegal character ALPHA='s&apos")); + } + else if (multiPartCompliance == MultiPartCompliance.LEGACY) + { + assertThat(responseContent, containsString("Incomplete Multipart")); + } + }); + } + + @ParameterizedTest + @MethodSource("multipartModes") + public void testAllWhitespaceForm(MultiPartCompliance multiPartCompliance) throws Exception { + startServer(multiPartCompliance); + + String contentType = "multipart/form-data; boundary=----WebKitFormBoundaryjwqONTsAFgubfMZc"; + String rawForm = " \n \n \n \n \n \n \n \n \n "; + + StringRequestContent form = new StringRequestContent( + contentType, + rawForm + ); + + InputStreamResponseListener listener = new InputStreamResponseListener(); + client.newRequest("localhost", connector.getLocalPort()) + .path("/defaultConfig") + .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("Missing content for multipart request")); + }); + } + + @ParameterizedTest + @MethodSource("multipartModes") + public void testManyParts(MultiPartCompliance multiPartCompliance) throws Exception + { + startServer(multiPartCompliance); + byte[] byteArray = new byte[1024]; Arrays.fill(byteArray, (byte)1); @@ -241,9 +402,12 @@ public void testManyParts() throws Exception }); } - @Test - public void testMaxRequestSize() throws Exception + @ParameterizedTest + @MethodSource("multipartModes") + public void testMaxRequestSize(MultiPartCompliance multiPartCompliance) throws Exception { + startServer(multiPartCompliance); + OutputStreamRequestContent content = new OutputStreamRequestContent(); MultiPartRequestContent multiPart = new MultiPartRequestContent(); multiPart.addPart(new MultiPart.ContentSourcePart("param", null, null, content)); @@ -301,9 +465,12 @@ private static void assert400orEof(InputStreamResponseListener listener, Consume checkbody.accept(responseContent); } - @Test - public void testTempFilesDeletedOnError() throws Exception + @ParameterizedTest + @MethodSource("multipartModes") + public void testTempFilesDeletedOnError(MultiPartCompliance multiPartCompliance) throws Exception { + startServer(multiPartCompliance); + byte[] byteArray = new byte[LARGE_MESSAGE_SIZE]; Arrays.fill(byteArray, (byte)1); BytesRequestContent content = new BytesRequestContent(byteArray); @@ -333,6 +500,8 @@ public void testTempFilesDeletedOnError() throws Exception @Test public void testMultiPartGzip() throws Exception { + startServer(MultiPartCompliance.RFC7578); + String contentString = "the quick brown fox jumps over the lazy dog, " + "the quick brown fox jumps over the lazy dog"; StringRequestContent content = new StringRequestContent(contentString); @@ -357,12 +526,14 @@ public void testMultiPartGzip() throws Exception assertThat(headers.get(HttpHeader.CONTENT_TYPE), startsWith("multipart/form-data")); assertThat(headers.get(HttpHeader.CONTENT_ENCODING), is("gzip")); - InputStream inputStream = new GZIPInputStream(responseStream.getInputStream()); - String contentType = headers.get(HttpHeader.CONTENT_TYPE); - MultiPartFormInputStream mpis = new MultiPartFormInputStream(inputStream, contentType, null, null); - List parts = new ArrayList<>(mpis.getParts()); - assertThat(parts.size(), is(1)); - assertThat(IO.toString(parts.get(0).getInputStream()), is(contentString)); + try (InputStream inputStream = new GZIPInputStream(responseStream.getInputStream())) + { + String contentType = headers.get(HttpHeader.CONTENT_TYPE); + MultiPartFormInputStream mpis = new MultiPartFormInputStream(inputStream, contentType, null, null); + List parts = new ArrayList<>(mpis.getParts()); + assertThat(parts.size(), is(1)); + assertThat(IO.toString(parts.get(0).getInputStream()), is(contentString)); + } } } }