Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cleanup Multipart handling #9344

Merged
merged 4 commits into from
Feb 14, 2023
Merged

Cleanup Multipart handling #9344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a5344d7
bring back some multipart improvements from #9287
lachlan-roberts Feb 9, 2023
e623511
#9344 changes from review
lachlan-roberts Feb 13, 2023
1bb928b
#9344 changes from review
lachlan-roberts Feb 13, 2023
f5a5154
fix test expectation
lachlan-roberts Feb 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 19 additions & 0 deletions jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartFormInputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import javax.servlet.http.Part;

import org.eclipse.jetty.server.MultiParts.NonCompliance;
import org.eclipse.jetty.server.handler.ContextHandler;
import org.eclipse.jetty.util.BufferUtil;
import org.eclipse.jetty.util.ByteArrayOutputStream2;
import org.eclipse.jetty.util.MultiException;
Expand Down Expand Up @@ -98,6 +99,8 @@ private enum State
private final MultipartConfigElement _config;
private final File _contextTmpDir;
private final String _contentType;
private final int _maxParts;
private int _numParts;
private volatile Throwable _err;
private volatile Path _tmpDir;
private volatile boolean _deleteOnExit;
Expand Down Expand Up @@ -367,6 +370,18 @@ public String getContentDispositionFilename()
* @param contextTmpDir javax.servlet.context.tempdir
*/
public MultiPartFormInputStream(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir)
{
this(in, contentType, config, contextTmpDir, ContextHandler.DEFAULT_MAX_FORM_KEYS);
}

/**
* @param in Request input stream
* @param contentType Content-Type header
* @param config MultipartConfigElement
* @param contextTmpDir javax.servlet.context.tempdir
* @param maxParts the maximum number of parts that can be parsed from the multipart content (0 for no parts allowed, -1 for unlimited parts).
*/
public MultiPartFormInputStream(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir, int maxParts)
{
// Must be a multipart request.
_contentType = contentType;
Expand All @@ -375,6 +390,7 @@ public MultiPartFormInputStream(InputStream in, String contentType, MultipartCon

_contextTmpDir = (contextTmpDir != null) ? contextTmpDir : new File(System.getProperty("java.io.tmpdir"));
_config = (config != null) ? config : new MultipartConfigElement(_contextTmpDir.getAbsolutePath());
_maxParts = maxParts;

if (in instanceof ServletInputStream)
{
Expand Down Expand Up @@ -797,6 +813,9 @@ public boolean content(ByteBuffer buffer, boolean last)
public void startPart()
{
reset();
_numParts++;
if (_maxParts >= 0 && _numParts > _maxParts)
throw new IllegalStateException(String.format("Form with too many parts [%d > %d]", _numParts, _maxParts));
}

@Override
Expand Down
23 changes: 22 additions & 1 deletion jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartInputStreamParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import javax.servlet.http.Part;

import org.eclipse.jetty.server.MultiParts.NonCompliance;
import org.eclipse.jetty.server.handler.ContextHandler;
import org.eclipse.jetty.util.ByteArrayOutputStream2;
import org.eclipse.jetty.util.LazyList;
import org.eclipse.jetty.util.MultiException;
Expand All @@ -67,7 +68,9 @@ public class MultiPartInputStreamParser
{
private static final Logger LOG = LoggerFactory.getLogger(MultiPartInputStreamParser.class);
public static final MultipartConfigElement __DEFAULT_MULTIPART_CONFIG = new MultipartConfigElement(System.getProperty("java.io.tmpdir"));
public static final MultiMap<Part> EMPTY_MAP = new MultiMap(Collections.emptyMap());
public static final MultiMap<Part> EMPTY_MAP = new MultiMap<>(Collections.emptyMap());
private final int _maxParts;
private int _numParts;
protected InputStream _in;
protected MultipartConfigElement _config;
protected String _contentType;
Expand Down Expand Up @@ -375,10 +378,23 @@ public String getContentDispositionFilename()
* @param contextTmpDir javax.servlet.context.tempdir
*/
public MultiPartInputStreamParser(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir)
{
this (in, contentType, config, contextTmpDir, ContextHandler.DEFAULT_MAX_FORM_KEYS);
}

/**
* @param in Request input stream
* @param contentType Content-Type header
* @param config MultipartConfigElement
* @param contextTmpDir javax.servlet.context.tempdir
* @param maxParts the maximum number of parts that can be parsed from the multipart content (0 for no parts allowed, -1 for unlimited parts).
*/
public MultiPartInputStreamParser(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir, int maxParts)
{
_contentType = contentType;
_config = config;
_contextTmpDir = contextTmpDir;
_maxParts = maxParts;
if (_contextTmpDir == null)
_contextTmpDir = new File(System.getProperty("java.io.tmpdir"));

Expand Down Expand Up @@ -674,6 +690,11 @@ else if (tl.startsWith("filename="))
continue;
}

// 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));

//Have a new Part
MultiPart part = new MultiPart(name, filename);
part.setHeaders(headers);
Expand Down
14 changes: 12 additions & 2 deletions jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,12 @@ class MultiPartsHttpParser implements MultiParts

public MultiPartsHttpParser(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir, Request request) throws IOException
{
_httpParser = new MultiPartFormInputStream(in, contentType, config, contextTmpDir);
this(in, contentType, config, contextTmpDir, request, ContextHandler.DEFAULT_MAX_FORM_KEYS);
}

public MultiPartsHttpParser(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir, Request request, int maxParts) throws IOException
{
_httpParser = new MultiPartFormInputStream(in, contentType, config, contextTmpDir, maxParts);
_context = request.getContext();
_request = request;
}
Expand Down Expand Up @@ -145,7 +150,12 @@ class MultiPartsUtilParser implements MultiParts

public MultiPartsUtilParser(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir, Request request) throws IOException
{
_utilParser = new MultiPartInputStreamParser(in, contentType, config, contextTmpDir);
this(in, contentType, config, contextTmpDir, request, ContextHandler.DEFAULT_MAX_FORM_KEYS);
}

public MultiPartsUtilParser(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir, Request request, int maxParts) throws IOException
{
_utilParser = new MultiPartInputStreamParser(in, contentType, config, contextTmpDir, maxParts);
_context = request.getContext();
_request = request;
}
Expand Down
31 changes: 25 additions & 6 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2305,7 +2305,21 @@ private Collection<Part> getParts(MultiMap<String> params) throws IOException
if (config == null)
throw new IllegalStateException("No multipart config for servlet");

_multiParts = newMultiParts(config);
int maxFormContentSize = ContextHandler.DEFAULT_MAX_FORM_CONTENT_SIZE;
int maxFormKeys = ContextHandler.DEFAULT_MAX_FORM_KEYS;
if (_context != null)
{
ContextHandler contextHandler = _context.getContextHandler();
maxFormContentSize = contextHandler.getMaxFormContentSize();
maxFormKeys = contextHandler.getMaxFormKeys();
}
else
{
maxFormContentSize = lookupServerAttribute(ContextHandler.MAX_FORM_CONTENT_SIZE_KEY, maxFormContentSize);
maxFormKeys = lookupServerAttribute(ContextHandler.MAX_FORM_KEYS_KEY, maxFormKeys);
}

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

String formCharset = null;
Expand All @@ -2316,7 +2330,7 @@ private Collection<Part> getParts(MultiMap<String> params) throws IOException
{
ByteArrayOutputStream os = new ByteArrayOutputStream();
IO.copy(is, os);
formCharset = new String(os.toByteArray(), StandardCharsets.UTF_8);
formCharset = os.toString(StandardCharsets.UTF_8);
}
}

Expand All @@ -2338,11 +2352,16 @@ else if (getCharacterEncoding() != null)
else
defaultCharset = StandardCharsets.UTF_8;

long formContentSize = 0;
ByteArrayOutputStream os = null;
for (Part p : parts)
{
if (p.getSubmittedFileName() == null)
{
formContentSize = Math.addExact(formContentSize, p.getSize());
if (maxFormContentSize >= 0 && formContentSize > maxFormContentSize)
throw new IllegalStateException("Form is larger than max length " + maxFormContentSize);

// Servlet Spec 3.0 pg 23, parts without filename must be put into params.
String charset = null;
if (p.getContentType() != null)
Expand All @@ -2354,7 +2373,7 @@ else if (getCharacterEncoding() != null)
os = new ByteArrayOutputStream();
IO.copy(is, os);

String content = new String(os.toByteArray(), charset == null ? defaultCharset : Charset.forName(charset));
String content = os.toString(charset == null ? defaultCharset : Charset.forName(charset));
if (_contentParameters == null)
_contentParameters = params == null ? new MultiMap<>() : params;
_contentParameters.add(p.getName(), content);
Expand All @@ -2367,7 +2386,7 @@ else if (getCharacterEncoding() != null)
return _multiParts.getParts();
}

private MultiParts newMultiParts(MultipartConfigElement config) throws IOException
private MultiParts newMultiParts(MultipartConfigElement config, int maxParts) throws IOException
{
MultiPartFormDataCompliance compliance = getHttpChannel().getHttpConfiguration().getMultipartFormDataCompliance();
if (LOG.isDebugEnabled())
Expand All @@ -2377,12 +2396,12 @@ private MultiParts newMultiParts(MultipartConfigElement config) throws IOExcepti
{
case RFC7578:
return new MultiParts.MultiPartsHttpParser(getInputStream(), getContentType(), config,
(_context != null ? (File)_context.getAttribute("javax.servlet.context.tempdir") : null), this);
(_context != null ? (File)_context.getAttribute("javax.servlet.context.tempdir") : null), this, maxParts);

case LEGACY:
default:
return new MultiParts.MultiPartsUtilParser(getInputStream(), getContentType(), config,
(_context != null ? (File)_context.getAttribute("javax.servlet.context.tempdir") : null), this);
(_context != null ? (File)_context.getAttribute("javax.servlet.context.tempdir") : null), this, maxParts);
}
}

Expand Down
128 changes: 128 additions & 0 deletions jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,15 @@
import org.eclipse.jetty.client.util.BytesRequestContent;
import org.eclipse.jetty.client.util.InputStreamResponseListener;
import org.eclipse.jetty.client.util.MultiPartRequestContent;
import org.eclipse.jetty.client.util.OutputStreamRequestContent;
import org.eclipse.jetty.client.util.StringRequestContent;
import org.eclipse.jetty.http.HttpFields;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpMethod;
import org.eclipse.jetty.http.HttpScheme;
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.http.MimeTypes;
import org.eclipse.jetty.io.EofException;
import org.eclipse.jetty.logging.StacklessLogging;
import org.eclipse.jetty.server.HttpChannel;
import org.eclipse.jetty.server.MultiPartFormInputStream;
Expand All @@ -54,6 +57,8 @@

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.startsWith;
import static org.junit.jupiter.api.Assertions.assertEquals;
Expand All @@ -67,8 +72,22 @@ public class MultiPartServletTest
private Path tmpDir;

private static final int MAX_FILE_SIZE = 512 * 1024;
private static final int MAX_REQUEST_SIZE = 1024 * 1024 * 8;
private static final int LARGE_MESSAGE_SIZE = 1024 * 1024;

public static class RequestParameterServlet extends HttpServlet
{
@Override
protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
{
req.getParameterMap();
req.getParts();
resp.setStatus(200);
resp.getWriter().print("success");
resp.getWriter().close();
}
}

public static class MultiPartServlet extends HttpServlet
{
@Override
Expand Down Expand Up @@ -118,11 +137,19 @@ public void start() throws Exception

MultipartConfigElement config = new MultipartConfigElement(tmpDir.toAbsolutePath().toString(),
MAX_FILE_SIZE, -1, 1);
MultipartConfigElement requestSizedConfig = new MultipartConfigElement(tmpDir.toAbsolutePath().toString(),
-1, MAX_REQUEST_SIZE, 1);
MultipartConfigElement defaultConfig = new MultipartConfigElement(tmpDir.toAbsolutePath().toString(),
-1, -1, 1);

ServletContextHandler contextHandler = new ServletContextHandler(ServletContextHandler.SESSIONS);
contextHandler.setContextPath("/");
ServletHolder servletHolder = contextHandler.addServlet(MultiPartServlet.class, "/");
servletHolder.getRegistration().setMultipartConfig(config);
servletHolder = contextHandler.addServlet(RequestParameterServlet.class, "/defaultConfig");
servletHolder.getRegistration().setMultipartConfig(defaultConfig);
servletHolder = contextHandler.addServlet(RequestParameterServlet.class, "/requestSizeLimit");
servletHolder.getRegistration().setMultipartConfig(requestSizedConfig);
servletHolder = contextHandler.addServlet(MultiPartEchoServlet.class, "/echo");
servletHolder.getRegistration().setMultipartConfig(config);

Expand All @@ -148,6 +175,107 @@ public void stop() throws Exception
IO.delete(tmpDir.toFile());
}

@Test
public void testLargePart() throws Exception
{
OutputStreamRequestContent content = new OutputStreamRequestContent();
MultiPartRequestContent multiPart = new MultiPartRequestContent();
multiPart.addFieldPart("param", content, null);
multiPart.close();

InputStreamResponseListener listener = new InputStreamResponseListener();
client.newRequest("localhost", connector.getLocalPort())
.path("/defaultConfig")
.scheme(HttpScheme.HTTP.asString())
.method(HttpMethod.POST)
.body(multiPart)
.send(listener);

// Write large amount of content to the part.
byte[] byteArray = new byte[1024 * 1024];
Arrays.fill(byteArray, (byte)1);
for (int i = 0; i < 1024 * 2; i++)
{
content.getOutputStream().write(byteArray);
}
content.close();

Response response = listener.get(30, TimeUnit.SECONDS);
assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400));
String responseContent = IO.toString(listener.getInputStream());
assertThat(responseContent, containsString("Unable to parse form content"));
assertThat(responseContent, containsString("Form is larger than max length"));
}

@Test
public void testManyParts() throws Exception
{
byte[] byteArray = new byte[1024];
Arrays.fill(byteArray, (byte)1);

MultiPartRequestContent multiPart = new MultiPartRequestContent();
for (int i = 0; i < 1024 * 1024; i++)
{
BytesRequestContent content = new BytesRequestContent(byteArray);
multiPart.addFieldPart("part" + i, content, null);
}
multiPart.close();

InputStreamResponseListener listener = new InputStreamResponseListener();
client.newRequest("localhost", connector.getLocalPort())
.path("/defaultConfig")
.scheme(HttpScheme.HTTP.asString())
.method(HttpMethod.POST)
.body(multiPart)
.send(listener);

Response response = listener.get(30, TimeUnit.SECONDS);
assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400));
String responseContent = IO.toString(listener.getInputStream());
assertThat(responseContent, containsString("Unable to parse form content"));
assertThat(responseContent, containsString("Form with too many parts"));
}

@Test
public void testMaxRequestSize() throws Exception
{
OutputStreamRequestContent content = new OutputStreamRequestContent();
MultiPartRequestContent multiPart = new MultiPartRequestContent();
multiPart.addFieldPart("param", content, null);
multiPart.close();

InputStreamResponseListener listener = new InputStreamResponseListener();
client.newRequest("localhost", connector.getLocalPort())
.path("/requestSizeLimit")
.scheme(HttpScheme.HTTP.asString())
.method(HttpMethod.POST)
.body(multiPart)
.send(listener);

Throwable writeError = null;
try
{
// Write large amount of content to the part.
byte[] byteArray = new byte[1024 * 1024];
Arrays.fill(byteArray, (byte)1);
for (int i = 0; i < 512; i++)
{
content.getOutputStream().write(byteArray);
}
}
catch (Exception e)
{
writeError = e;
}

if (writeError != null)
assertThat(writeError, instanceOf(EofException.class));

// We should get 400 response.
Response response = listener.get(30, TimeUnit.SECONDS);
assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400));
}

@Test
public void testTempFilesDeletedOnError() throws Exception
{
Expand Down