Merge pull request #7981 from eclipse/jetty-10.0.x-multipartViolations

Add TRANSFER_ENCODING violation for MultiPart RFC7578 parser. (#7976)
This commit is contained in:
Lachlan 2022-05-16 12:53:13 +10:00 committed by GitHub
commit 01363fc239
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 155 additions and 2 deletions

View File

@ -30,6 +30,7 @@ import java.nio.file.StandardCopyOption;
import java.nio.file.StandardOpenOption;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.List;
import java.util.stream.Collectors;
import javax.servlet.MultipartConfigElement;
@ -91,6 +92,7 @@ public class MultiPartFormInputStream
private final AutoLock _lock = new AutoLock();
private final MultiMap<Part> _parts = new MultiMap<>();
private final EnumSet<NonCompliance> _nonComplianceWarnings = EnumSet.noneOf(NonCompliance.class);
private final InputStream _in;
private final MultipartConfigElement _config;
private final File _contextTmpDir;
@ -102,6 +104,31 @@ public class MultiPartFormInputStream
private volatile int _bufferSize = 16 * 1024;
private State state = State.UNPARSED;
public enum NonCompliance
{
TRANSFER_ENCODING("https://tools.ietf.org/html/rfc7578#section-4.7");
final String _rfcRef;
NonCompliance(String rfcRef)
{
_rfcRef = rfcRef;
}
public String getURL()
{
return _rfcRef;
}
}
/**
* @return an EnumSet of non compliances with the RFC that were accepted by this parser
*/
public EnumSet<NonCompliance> getNonComplianceWarnings()
{
return _nonComplianceWarnings;
}
public class MultiPart implements Part
{
protected String _name;
@ -671,7 +698,11 @@ public class MultiPartFormInputStream
// Transfer encoding is not longer considers as it is deprecated as per
// https://tools.ietf.org/html/rfc7578#section-4.7
if (key.equalsIgnoreCase("content-transfer-encoding"))
{
if (!"8bit".equalsIgnoreCase(value) && !"binary".equalsIgnoreCase(value))
_nonComplianceWarnings.add(NonCompliance.TRANSFER_ENCODING);
}
}
@Override

View File

@ -66,6 +66,7 @@ import javax.servlet.http.WebConnection;
import org.eclipse.jetty.http.BadMessageException;
import org.eclipse.jetty.http.ComplianceViolation;
import org.eclipse.jetty.http.HostPortHttpField;
import org.eclipse.jetty.http.HttpCompliance;
import org.eclipse.jetty.http.HttpCookie;
import org.eclipse.jetty.http.HttpCookie.SetCookieHttpField;
import org.eclipse.jetty.http.HttpField;
@ -2295,6 +2296,7 @@ public class Request implements HttpServletRequest
_multiParts = newMultiParts(config);
Collection<Part> parts = _multiParts.getParts();
setNonComplianceViolationsOnRequest();
String formCharset = null;
Part charsetPart = _multiParts.getPart("_charset_");
@ -2355,6 +2357,22 @@ public class Request implements HttpServletRequest
return _multiParts.getParts();
}
private void setNonComplianceViolationsOnRequest()
{
@SuppressWarnings("unchecked")
List<String> violations = (List<String>)getAttribute(HttpCompliance.VIOLATIONS_ATTR);
if (violations != null)
return;
EnumSet<MultiPartFormInputStream.NonCompliance> nonComplianceWarnings = _multiParts.getNonComplianceWarnings();
violations = new ArrayList<>();
for (MultiPartFormInputStream.NonCompliance nc : nonComplianceWarnings)
{
violations.add(nc.name() + ": " + nc.getURL());
}
setAttribute(HttpCompliance.VIOLATIONS_ATTR, violations);
}
private MultiPartFormInputStream newMultiParts(MultipartConfigElement config) throws IOException
{
return new MultiPartFormInputStream(getInputStream(), getContentType(), config,

View File

@ -18,10 +18,12 @@ import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
import java.nio.file.Path;
import java.util.Base64;
import java.util.Collection;
import java.util.EnumSet;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
@ -32,13 +34,17 @@ import javax.servlet.ServletInputStream;
import javax.servlet.http.Part;
import org.eclipse.jetty.server.MultiPartFormInputStream.MultiPart;
import org.eclipse.jetty.server.MultiPartFormInputStream.NonCompliance;
import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
import org.eclipse.jetty.util.BlockingArrayQueue;
import org.eclipse.jetty.util.BufferUtil;
import org.eclipse.jetty.util.IO;
import org.junit.jupiter.api.Test;
import static java.nio.charset.StandardCharsets.ISO_8859_1;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.notNullValue;
@ -1024,6 +1030,99 @@ public class MultiPartFormInputStreamTest
baos = new ByteArrayOutputStream();
IO.copy(p3.getInputStream(), baos);
assertEquals("the end", baos.toString(StandardCharsets.US_ASCII));
assertThat(mpis.getNonComplianceWarnings(), equalTo(EnumSet.of(NonCompliance.TRANSFER_ENCODING)));
}
@Test
public void testFragmentation() throws IOException
{
String contentType = "multipart/form-data, boundary=----WebKitFormBoundaryhXfFAMfUnUKhmqT8";
String payload1 =
"------WebKitFormBoundaryhXfFAMfUnUKhmqT8\r\n" +
"Content-Disposition: form-data; name=\"field1\"\r\n\r\n" +
"value1" +
"\r\n--";
String payload2 = "----WebKitFormBoundaryhXfFAMfUnUKhmqT8\r\n" +
"Content-Disposition: form-data; name=\"field2\"\r\n\r\n" +
"value2" +
"\r\n------WebKitFormBoundaryhXfFAMfUnUKhmqT8--\r\n";
// Split the content into separate reads, with the content broken up on the boundary string.
AppendableInputStream stream = new AppendableInputStream();
stream.append(payload1);
stream.append("");
stream.append(payload2);
stream.endOfContent();
MultipartConfigElement config = new MultipartConfigElement(_dirname);
MultiPartFormInputStream mpis = new MultiPartFormInputStream(stream, contentType, config, _tmpDir);
mpis.setDeleteOnExit(true);
// Check size.
Collection<Part> parts = mpis.getParts();
assertThat(parts.size(), is(2));
// Check part content.
assertThat(IO.toString(mpis.getPart("field1").getInputStream()), is("value1"));
assertThat(IO.toString(mpis.getPart("field2").getInputStream()), is("value2"));
}
static class AppendableInputStream extends InputStream
{
private static final ByteBuffer EOF = ByteBuffer.allocate(0);
private final BlockingArrayQueue<ByteBuffer> buffers = new BlockingArrayQueue<>();
private ByteBuffer current;
public void append(String data)
{
append(data.getBytes(StandardCharsets.US_ASCII));
}
public void append(byte[] data)
{
buffers.add(BufferUtil.toBuffer(data));
}
public void endOfContent()
{
buffers.add(EOF);
}
@Override
public int read() throws IOException
{
byte[] buf = new byte[1];
while (true)
{
int len = read(buf, 0, 1);
if (len < 0)
return -1;
if (len > 0)
return buf[0];
}
}
@Override
public int read(byte[] b, int off, int len) throws IOException
{
if (current == null)
current = buffers.poll();
if (current == EOF)
return -1;
if (BufferUtil.isEmpty(current))
{
current = null;
return 0;
}
ByteBuffer buffer = ByteBuffer.wrap(b, off, len);
buffer.flip();
int read = BufferUtil.append(buffer, current);
if (BufferUtil.isEmpty(current))
current = buffers.poll();
return read;
}
}
@Test
@ -1062,6 +1161,8 @@ public class MultiPartFormInputStreamTest
baos = new ByteArrayOutputStream();
IO.copy(p2.getInputStream(), baos);
assertEquals("truth=3Dbeauty", baos.toString(StandardCharsets.US_ASCII));
assertThat(mpis.getNonComplianceWarnings(), equalTo(EnumSet.of(NonCompliance.TRANSFER_ENCODING)));
}
@Test

View File

@ -501,6 +501,7 @@ public class RequestTest
"--AaB03x\r\n" +
"content-disposition: form-data; name=\"stuff\"; filename=\"foo.upload\"\r\n" +
"Content-Type: text/plain;charset=ISO-8859-1\r\n" +
"Content-Transfer-Encoding: something\r\n" +
"\r\n" +
"000000000000000000000000000000000000000000000000000\r\n" +
"--AaB03x--\r\n";
@ -514,7 +515,9 @@ public class RequestTest
LocalEndPoint endPoint = _connector.connect();
endPoint.addInput(request);
assertTrue(endPoint.getResponse().startsWith("HTTP/1.1 200"));
String response = endPoint.getResponse();
assertTrue(response.startsWith("HTTP/1.1 200"));
assertThat(response, containsString("Violation: TRANSFER_ENCODING"));
// We know the previous request has completed if another request can be processed on the same connection.
String cleanupRequest = "GET /foo/cleanup HTTP/1.1\r\n" +