Skip to content

Commit

Permalink
Fix Regression on ee9 / ee8 MultiPart parsing (#12031)
Browse files Browse the repository at this point in the history 
* 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).
  • Loading branch information
@joakime
joakime authored Jul 16, 2024
1 parent 913e82d commit 7c42a5b
Show file tree
Hide file tree
Showing 4 changed files with 204 additions and 24 deletions.
2 changes: 1 addition & 1 deletion ...jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartFormInputStream.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down
6 changes: 3 additions & 3 deletions ...9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartInputStreamLegacyParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
{
Expand Down
13 changes: 11 additions & 2 deletions jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -2044,7 +2044,16 @@ private Collection<Part> getParts(Fields params) throws IOException
MultiPartCompliance multiPartCompliance = getHttpChannel().getHttpConfiguration().getMultiPartCompliance();

_multiParts = newMultiParts(multiPartCompliance, config, maxFormKeys);
Collection<Part> parts = _multiParts.getParts();

Collection<Part> parts;
try
{
parts = _multiParts.getParts();
}
catch (IOException e)
{
throw new BadMessageException("Unable to parse form content", e);
}
reportComplianceViolations();

String formCharset = null;
Expand Down
207 changes: 189 additions & 18 deletions ...9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/MultiPartServletTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -180,9 +187,50 @@ public void stop() throws Exception
IO.delete(tmpDir.toFile());
}

@Test
public void testLargePart() throws Exception
public static Stream<Arguments> 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));
Expand Down Expand Up @@ -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=&apos;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);

Expand All @@ -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));
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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<Part> 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<Part> parts = new ArrayList<>(mpis.getParts());
assertThat(parts.size(), is(1));
assertThat(IO.toString(parts.get(0).getInputStream()), is(contentString));
}
}
}
}

0 comments on commit 7c42a5b

Please sign in to comment.