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

Add TRANSFER_ENCODING violation for MultiPart RFC7578 parser. (#7976)

jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartFormInputStream.java

@@ -30,6 +30,7 @@
 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 @@ private enum State
 
     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 @@ private enum State
     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 @@ else if (key.equalsIgnoreCase("content-type"))
 
             // 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

jetty-server/src/main/java/org/eclipse/jetty/server/Request.java

@@ -66,6 +66,7 @@
 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 @@ private Collection<Part> getParts(MultiMap<String> params) throws IOException
 
             _multiParts = newMultiParts(config);
             Collection<Part> parts = _multiParts.getParts();
+            setNonComplianceViolationsOnRequest();
 
             String formCharset = null;
             Part charsetPart = _multiParts.getPart("_charset_");
@@ -2355,6 +2357,22 @@ else if (getCharacterEncoding() != null)
         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,

jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartFormInputStreamTest.java

@@ -18,10 +18,12 @@
 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.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 void testBase64EncodedContent() throws Exception
         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 void testQuotedPrintableEncoding() throws Exception
         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

jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java

@@ -501,6 +501,7 @@ public void testMultiPart() throws Exception
             "--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 void testMultiPart() throws Exception
 
         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" +