MultipartEntityBuilder to use a random boundary value by default

Author: benweissmann

httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java

@@ -47,18 +47,14 @@
 /**
  * Builder for multipart {@link HttpEntity}s.
  * <p>
- * This class constructs multipart entities with a boundary determined by either a fixed
- * value ("httpclient_boundary_7k9p2m4x8n5j3q6t1r0vwyzabcdefghi") or a random UUID. If no
- * boundary is explicitly set via {@link #setBoundary(String)}, it defaults to the fixed
- * value unless {@link #withRandomBoundary()} is called to request a random UUID at build
- * time. Users can provide a custom boundary with {@link #setBoundary(String)}. A warning
- * is logged when no explicit boundary is set via {@link #setBoundary(String)}, encouraging
- * deliberate choice.
+ * This class constructs multipart entities with a boundary determined by either a random UUID
+ * or an explicit boundary set via {@link #setBoundary(String)}.
  * </p>
  * <p>
- * IMPORTANT: it is responsibility of the caller to validate / sanitize content of body
- * parts, for instance, to ensure they do not contain the boundary value that can prevent
- * the consumer of the entity from correctly parsing / processing the body parts.
+ *  IMPORTANT: it is responsibility of the caller to validate / sanitize content of body
+ *  parts. For instance, when using an explicit boundary, it's the caller's responsibility to
+ *  ensure the body parts do not contain the boundary value, which can prevent the consumer of
+ *  the entity from correctly parsing / processing the body parts.
  * </p>
  *
  * @since 5.0
@@ -74,7 +70,6 @@ public class MultipartEntityBuilder {
 
     private static final String BOUNDARY_PREFIX = "httpclient_boundary_";
 
-    private boolean isRandomBoundaryRequested = false;
     /**
      * The logger for this class.
      */
@@ -125,12 +120,14 @@ public MultipartEntityBuilder setStrictMode() {
     /**
      * Sets a custom boundary string for the multipart entity.
      * <p>
-     * If {@code null} is provided, the builder reverts to its default boundary logic:
-     * either using a boundary from the {@code contentType} if present, or falling back
-     * to a fixed or random boundary (depending on {@link #withRandomBoundary()}).
+     * If {@code null} is provided, the builder reverts to its default logic of using a random UUID.
+     * </p>
+     * <p>
+     * IMPORTANT: when setting an explicit boundary, it is responsibility of the caller to validate / sanitize content
+     * of body parts to ensure they do not contain the boundary value.
      * </p>
      *
-     * @param boundary the boundary string, or {@code null} to use the default boundary logic
+     * @param boundary the boundary string, or {@code null} to use a random UUID.
      * @return this builder instance
      */
     public MultipartEntityBuilder setBoundary(final String boundary) {
@@ -234,14 +231,12 @@ public MultipartEntityBuilder addBinaryBody(final String name, final InputStream
     }
 
     /**
-     * Returns the fixed default boundary value.
-     */
-    private String getFixedBoundary() {
-        return BOUNDARY_PREFIX + "7k9p2m4x8n5j3q6t1r0vwyzabcdefghi";
-    }
-
-    /**
-     * Generates a random boundary using UUID.
+     * Generates a random boundary using UUID. The UUID is a v4 random UUID generated from a cryptographically-secure
+     * random source.
+     * <p>
+     * A cryptographically-secure random number source is used to generate the UUID, to avoid a malicious actor crafting
+     * a body part that contains the boundary value to tamper with the entity structure.
+     * </p>
      */
     private String getRandomBoundary() {
         return BOUNDARY_PREFIX + UUID.randomUUID();
@@ -274,29 +269,13 @@ public MultipartEntityBuilder addEpilogue(final String epilogue) {
         return this;
     }
 
-    /**
-     * Configures the builder to request a random boundary generated by UUID.randomUUID()
-     * at build time if no explicit boundary is set via {@link #setBoundary(String)}.
-     *
-     * @return this builder instance
-     * @since 5.5
-     */
-    public MultipartEntityBuilder withRandomBoundary() {
-        this.isRandomBoundaryRequested = true;
-        this.boundary = null;
-        return this;
-    }
-
     MultipartFormEntity buildEntity() {
         String boundaryCopy = boundary;
         if (boundaryCopy == null && contentType != null) {
             boundaryCopy = contentType.getParameter("boundary");
         }
         if (boundaryCopy == null) {
-            boundaryCopy = isRandomBoundaryRequested ? getRandomBoundary() : getFixedBoundary();
-            if (LOG.isWarnEnabled()) {
-                LOG.warn("No boundary explicitly set; using generated default: {}", boundaryCopy);
-            }
+            boundaryCopy = getRandomBoundary();
         }
         Charset charsetCopy = charset;
         if (charsetCopy == null && contentType != null) {

httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartEntityBuilder.java

@@ -313,9 +313,8 @@ void testMultipartWriteToRFC7578ModeWithFilenameStar() throws Exception {
     @Test
     void testRandomBoundary() {
         final MultipartFormEntity entity = MultipartEntityBuilder.create()
-                .withRandomBoundary()
                 .buildEntity();
-        final NameValuePair boundaryParam = extractBoundary(entity.getContentType());
+        final NameValuePair boundaryParam = extractBoundary(entity.getContentType(), "multipart/mixed");
         final String boundary = boundaryParam.getValue();
         Assertions.assertNotNull(boundary);
         Assertions.assertEquals(56, boundary.length());
@@ -324,21 +323,43 @@ void testRandomBoundary() {
     }
 
     @Test
-    void testExplicitBoundaryOverridesRandom() {
-        final String customBoundary = "my_custom_boundary";
+    void testRandomBoundaryWriteTo() throws Exception {
+        final String helloWorld = "hello world";
+        final List<NameValuePair> parameters = new ArrayList<>();
+        parameters.add(new BasicNameValuePair(MimeConsts.FIELD_PARAM_NAME, "test"));
+        parameters.add(new BasicNameValuePair(MimeConsts.FIELD_PARAM_FILENAME, helloWorld));
         final MultipartFormEntity entity = MultipartEntityBuilder.create()
-                .withRandomBoundary()
-                .setBoundary(customBoundary)
+                .setStrictMode()
+                .addPart(new FormBodyPartBuilder()
+                        .setName("test")
+                        .setBody(new StringBody("hello world", ContentType.TEXT_PLAIN))
+                        .addField("Content-Disposition", "multipart/form-data", parameters)
+                        .build())
                 .buildEntity();
-        final NameValuePair boundaryParam = extractBoundary(entity.getContentType());
-        Assertions.assertEquals(customBoundary, boundaryParam.getValue());
+
+        final NameValuePair boundaryParam = extractBoundary(entity.getContentType(), "multipart/form-data");
+        final String boundary = boundaryParam.getValue();
+        Assertions.assertNotNull(boundary);
+        Assertions.assertEquals(56, boundary.length());
+        Assertions.assertTrue(boundary.startsWith("httpclient_boundary_"));
+        Assertions.assertTrue(boundary.substring(20).matches("[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}"));
+
+        final ByteArrayOutputStream out = new ByteArrayOutputStream();
+        entity.writeTo(out);
+        out.close();
+        Assertions.assertEquals("--" + boundary + "\r\n" +
+                "Content-Disposition: multipart/form-data; name=\"test\"; filename=\"hello world\"\r\n" +
+                "Content-Type: text/plain; charset=UTF-8\r\n" +
+                "\r\n" +
+                helloWorld + "\r\n" +
+                "--" + boundary + "--\r\n", out.toString(StandardCharsets.US_ASCII.name()));
     }
 
-    private NameValuePair extractBoundary(final String contentType) {
+    private NameValuePair extractBoundary(final String contentType, final String expectedName) {
         final BasicHeaderValueParser parser = BasicHeaderValueParser.INSTANCE;
         final ParserCursor cursor = new ParserCursor(0, contentType.length());
         final HeaderElement elem = parser.parseHeaderElement(contentType, cursor);
-        Assertions.assertEquals("multipart/mixed", elem.getName());
+        Assertions.assertEquals(expectedName, elem.getName());
         return elem.getParameterByName("boundary");
     }
 

httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartFormHttpEntity.java

@@ -78,7 +78,8 @@ void testImplicitContractorParams() {
         final String boundary = p1.getValue();
         Assertions.assertNotNull(boundary);
 
-        Assertions.assertEquals(52, boundary.length());
+        // "httpclient_boundary_" + UUID (32 characters + 4 dashes) + 56 characters
+        Assertions.assertEquals(56, boundary.length());
         final NameValuePair p2 = elem.getParameterByName("charset");
         Assertions.assertNull(p2);
     }