From b307812b2eb5181866905a2cd80e2fa210427254 Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Fri, 28 Feb 2025 13:20:38 +0100 Subject: [PATCH 1/2] Replace random boundary generation with fixed "boundary-blah" default in MultipartEntityBuilder. Update Javadoc to clarify users must set their own boundary for uniqueness, and adjust test to verify the new fixed value. --- .../entity/mime/MultipartEntityBuilder.java | 42 +++++++++---------- .../mime/TestMultipartFormHttpEntity.java | 3 +- 2 files changed, 22 insertions(+), 23 deletions(-) diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java b/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java index d86ec95ca1..1cffd33de2 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java @@ -29,13 +29,11 @@ import java.io.File; import java.io.InputStream; -import java.nio.CharBuffer; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collections; import java.util.List; -import java.util.concurrent.ThreadLocalRandom; import org.apache.hc.core5.http.ContentType; import org.apache.hc.core5.http.HttpEntity; @@ -45,24 +43,37 @@ /** * Builder for multipart {@link HttpEntity}s. + *

+ * This class constructs multipart entities with a default boundary of "httpclient_boundary_7k9p2m4x8n5j3q6t1r0vwyzabcdefghi" + * if none is explicitly set. The default boundary is a simple convenience and is not + * intended to be unique or secure. If a unique or secure boundary is required (e.g., + * to avoid collisions with content or for security-sensitive applications), users must + * explicitly set their own boundary using {@link #setBoundary(String)}. For randomized + * or cryptographically secure boundaries, users should generate their own value using + * an appropriate random number generator or cryptographic library. + *

* * @since 5.0 */ public class MultipartEntityBuilder { - /** - * The pool of ASCII chars to be used for generating a multipart boundary. - */ - private final static char[] MULTIPART_CHARS = - "-_1234567890abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" - .toCharArray(); - private ContentType contentType; private HttpMultipartMode mode = HttpMultipartMode.STRICT; private String boundary; private Charset charset; private List multipartParts; + /** + * The default boundary value used if none is explicitly set. + *

+ * This is a fixed value ("httpclient_boundary_7k9p2m4x8n5j3q6t1r0vwyzabcdefghi") provided for convenience. It is not + * random or secure. Users requiring uniqueness or security must override this + * by calling {@link #setBoundary(String)} with their own value, potentially + * generated using a random number generator or cryptographic library. + *

+ */ + private static final String DEFAULT_BOUNDARY = "httpclient_boundary_7k9p2m4x8n5j3q6t1r0vwyzabcdefghi"; + /** * The preamble of the multipart message. * This field stores the optional preamble that should be added at the beginning of the multipart message. @@ -231,24 +242,13 @@ public MultipartEntityBuilder addEpilogue(final String epilogue) { return this; } - private String generateBoundary() { - final ThreadLocalRandom rand = ThreadLocalRandom.current(); - final int count = rand.nextInt(30, 41); // a random size from 30 to 40 - final CharBuffer buffer = CharBuffer.allocate(count); - while (buffer.hasRemaining()) { - buffer.put(MULTIPART_CHARS[rand.nextInt(MULTIPART_CHARS.length)]); - } - buffer.flip(); - return buffer.toString(); - } - MultipartFormEntity buildEntity() { String boundaryCopy = boundary; if (boundaryCopy == null && contentType != null) { boundaryCopy = contentType.getParameter("boundary"); } if (boundaryCopy == null) { - boundaryCopy = generateBoundary(); + boundaryCopy = DEFAULT_BOUNDARY; } Charset charsetCopy = charset; if (charsetCopy == null && contentType != null) { diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartFormHttpEntity.java b/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartFormHttpEntity.java index 47170e128b..765d26ecfa 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartFormHttpEntity.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartFormHttpEntity.java @@ -78,8 +78,7 @@ void testImplicitContractorParams() { final String boundary = p1.getValue(); Assertions.assertNotNull(boundary); - Assertions.assertTrue(boundary.length() >= 30); - Assertions.assertTrue(boundary.length() <= 40); + Assertions.assertEquals("httpclient_boundary_7k9p2m4x8n5j3q6t1r0vwyzabcdefghi", boundary); final NameValuePair p2 = elem.getParameterByName("charset"); Assertions.assertNull(p2); From 9f63d9d50202c541dd01541a44a64256999c9aa0 Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Fri, 28 Feb 2025 23:13:12 +0100 Subject: [PATCH 2/2] Simplify boundary generation in MultipartEntityBuilder by removing useRandomBoundary. --- .../entity/mime/MultipartEntityBuilder.java | 75 +++++++++++++++---- .../mime/TestMultipartEntityBuilder.java | 35 +++++++++ .../mime/TestMultipartFormHttpEntity.java | 3 +- 3 files changed, 95 insertions(+), 18 deletions(-) diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java b/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java index 1cffd33de2..805267c8c5 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java @@ -34,23 +34,26 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.UUID; import org.apache.hc.core5.http.ContentType; import org.apache.hc.core5.http.HttpEntity; import org.apache.hc.core5.http.NameValuePair; import org.apache.hc.core5.http.message.BasicNameValuePair; import org.apache.hc.core5.util.Args; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Builder for multipart {@link HttpEntity}s. *

- * This class constructs multipart entities with a default boundary of "httpclient_boundary_7k9p2m4x8n5j3q6t1r0vwyzabcdefghi" - * if none is explicitly set. The default boundary is a simple convenience and is not - * intended to be unique or secure. If a unique or secure boundary is required (e.g., - * to avoid collisions with content or for security-sensitive applications), users must - * explicitly set their own boundary using {@link #setBoundary(String)}. For randomized - * or cryptographically secure boundaries, users should generate their own value using - * an appropriate random number generator or cryptographic library. + * 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. *

* * @since 5.0 @@ -63,16 +66,15 @@ public class MultipartEntityBuilder { private Charset charset; private List multipartParts; + + private static final String BOUNDARY_PREFIX = "httpclient_boundary_"; + + private boolean isRandomBoundaryRequested = false; /** - * The default boundary value used if none is explicitly set. - *

- * This is a fixed value ("httpclient_boundary_7k9p2m4x8n5j3q6t1r0vwyzabcdefghi") provided for convenience. It is not - * random or secure. Users requiring uniqueness or security must override this - * by calling {@link #setBoundary(String)} with their own value, potentially - * generated using a random number generator or cryptographic library. - *

+ * The logger for this class. */ - private static final String DEFAULT_BOUNDARY = "httpclient_boundary_7k9p2m4x8n5j3q6t1r0vwyzabcdefghi"; + private static final Logger LOG = LoggerFactory.getLogger(MultipartEntityBuilder.class); + /** * The preamble of the multipart message. @@ -115,6 +117,17 @@ public MultipartEntityBuilder setStrictMode() { return this; } + /** + * Sets a custom boundary string for the multipart entity. + *

+ * 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()}). + *

+ * + * @param boundary the boundary string, or {@code null} to use the default boundary logic + * @return this builder instance + */ public MultipartEntityBuilder setBoundary(final String boundary) { this.boundary = boundary; return this; @@ -215,6 +228,20 @@ public MultipartEntityBuilder addBinaryBody(final String name, final InputStream return addBinaryBody(name, stream, ContentType.DEFAULT_BINARY, null); } + /** + * Returns the fixed default boundary value. + */ + private String getFixedBoundary() { + return BOUNDARY_PREFIX + "7k9p2m4x8n5j3q6t1r0vwyzabcdefghi"; + } + + /** + * Generates a random boundary using UUID. + */ + private String getRandomBoundary() { + return BOUNDARY_PREFIX + UUID.randomUUID(); + } + /** * Adds a preamble to the multipart entity being constructed. The preamble is the text that appears before the first * boundary delimiter. The preamble is optional and may be null. @@ -242,13 +269,29 @@ 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 = DEFAULT_BOUNDARY; + boundaryCopy = isRandomBoundaryRequested ? getRandomBoundary() : getFixedBoundary(); + if (LOG.isWarnEnabled()) { + LOG.warn("No boundary explicitly set; using generated default: {}", boundaryCopy); + } } Charset charsetCopy = charset; if (charsetCopy == null && contentType != null) { diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartEntityBuilder.java b/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartEntityBuilder.java index b96c59fb81..5cbc878881 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartEntityBuilder.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartEntityBuilder.java @@ -35,8 +35,11 @@ import java.util.List; import org.apache.hc.core5.http.ContentType; +import org.apache.hc.core5.http.HeaderElement; import org.apache.hc.core5.http.NameValuePair; +import org.apache.hc.core5.http.message.BasicHeaderValueParser; import org.apache.hc.core5.http.message.BasicNameValuePair; +import org.apache.hc.core5.http.message.ParserCursor; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -307,4 +310,36 @@ void testMultipartWriteToRFC7578ModeWithFilenameStar() throws Exception { "--xxxxxxxxxxxxxxxxxxxxxxxx--\r\n", out.toString(StandardCharsets.ISO_8859_1.name())); } + @Test + void testRandomBoundary() { + final MultipartFormEntity entity = MultipartEntityBuilder.create() + .withRandomBoundary() + .buildEntity(); + final NameValuePair boundaryParam = extractBoundary(entity.getContentType()); + 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}")); + } + + @Test + void testExplicitBoundaryOverridesRandom() { + final String customBoundary = "my_custom_boundary"; + final MultipartFormEntity entity = MultipartEntityBuilder.create() + .withRandomBoundary() + .setBoundary(customBoundary) + .buildEntity(); + final NameValuePair boundaryParam = extractBoundary(entity.getContentType()); + Assertions.assertEquals(customBoundary, boundaryParam.getValue()); + } + + private NameValuePair extractBoundary(final String contentType) { + 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()); + return elem.getParameterByName("boundary"); + } + } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartFormHttpEntity.java b/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartFormHttpEntity.java index 765d26ecfa..3e4d79b1fb 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartFormHttpEntity.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartFormHttpEntity.java @@ -78,8 +78,7 @@ void testImplicitContractorParams() { final String boundary = p1.getValue(); Assertions.assertNotNull(boundary); - Assertions.assertEquals("httpclient_boundary_7k9p2m4x8n5j3q6t1r0vwyzabcdefghi", boundary); - + Assertions.assertEquals(52, boundary.length()); final NameValuePair p2 = elem.getParameterByName("charset"); Assertions.assertNull(p2); }