From df5c0ad5121d25786efcf37043475b523238e848 Mon Sep 17 00:00:00 2001 From: Lin Gao Date: Wed, 20 Dec 2023 12:31:00 +0800 Subject: [PATCH 1/2] [UNDERTOW-2337] Multipart form-data larger than 16KiB is not available through Servlet getParameter API --- .../handlers/RequestDumpingHandler.java | 2 +- .../server/handlers/form/FormData.java | 37 ++++++++++++-- .../form/MultiPartParserDefinition.java | 27 ++++++----- .../main/java/io/undertow/util/FileUtils.java | 20 +++++++- .../form/MultipartFormDataParserTestCase.java | 3 +- .../servlet/spec/HttpServletRequestImpl.java | 10 ++-- .../multipart/AddMultipartServetListener.java | 4 ++ .../test/multipart/MultiPartServlet.java | 19 ++++++++ .../test/multipart/MultiPartTestCase.java | 48 +++++++++++++++++++ 9 files changed, 145 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/io/undertow/server/handlers/RequestDumpingHandler.java b/core/src/main/java/io/undertow/server/handlers/RequestDumpingHandler.java index 6b97ab8db4..48282ae5b6 100644 --- a/core/src/main/java/io/undertow/server/handlers/RequestDumpingHandler.java +++ b/core/src/main/java/io/undertow/server/handlers/RequestDumpingHandler.java @@ -164,7 +164,7 @@ private void dumpRequestBody(HttpServerExchange exchange, StringBuilder sb) { sb.append(formField) .append("="); for (FormData.FormValue formValue : formValues) { - sb.append(formValue.isFileItem() ? "[file-content]" : formValue.getValue()); + sb.append(formValue.isFileItem() && !formValue.isBigField() ? "[file-content]" : formValue.getValue()); sb.append("\n"); if (formValue.getHeaders() != null) { diff --git a/core/src/main/java/io/undertow/server/handlers/form/FormData.java b/core/src/main/java/io/undertow/server/handlers/form/FormData.java index d7b5689b66..b4d9eef053 100644 --- a/core/src/main/java/io/undertow/server/handlers/form/FormData.java +++ b/core/src/main/java/io/undertow/server/handlers/form/FormData.java @@ -34,6 +34,7 @@ import java.util.Map; import io.undertow.UndertowMessages; +import io.undertow.util.FileUtils; import io.undertow.util.HeaderMap; /** @@ -102,11 +103,15 @@ public void add(String name, String value, String charset, final HeaderMap heade } public void add(String name, Path value, String fileName, final HeaderMap headers) { + add(name, value, fileName, headers, false, null); + } + + public void add(String name, Path value, String fileName, final HeaderMap headers, boolean bigField, String charset) { Deque values = this.values.get(name); if (values == null) { this.values.put(name, values = new ArrayDeque<>(1)); } - values.add(new FormValueImpl(value, fileName, headers)); + values.add(new FormValueImpl(value, fileName, headers, bigField, charset)); if (values.size() > maxValues) { throw new RuntimeException(UndertowMessages.MESSAGES.tooManyParameters(maxValues)); } @@ -211,6 +216,16 @@ public interface FormValue { * @return The headers that were present in the multipart request, or null if this was not a multipart request */ HeaderMap getHeaders(); + + /** + * @return true if size of the FormValue comes from a multipart request exceeds the fieldSizeThreshold of + * {@link MultiPartParserDefinition} without filename specified. + * + * A big field is stored in disk, it is a file based FileItem. + * + * getValue() returns getCharset() decoded string from the file if it is true. + */ + boolean isBigField(); } public static class FileItem { @@ -284,6 +299,7 @@ static class FormValueImpl implements FormValue { private final HeaderMap headers; private final FileItem fileItem; private final String charset; + private final boolean bigField; FormValueImpl(String value, HeaderMap headers) { this.value = value; @@ -291,6 +307,7 @@ static class FormValueImpl implements FormValue { this.fileName = null; this.fileItem = null; this.charset = null; + this.bigField = false; } FormValueImpl(String value, String charset, HeaderMap headers) { @@ -299,14 +316,16 @@ static class FormValueImpl implements FormValue { this.headers = headers; this.fileName = null; this.fileItem = null; + this.bigField = false; } - FormValueImpl(Path file, final String fileName, HeaderMap headers) { + FormValueImpl(Path file, final String fileName, HeaderMap headers, boolean bigField, String charset) { this.fileItem = new FileItem(file); this.headers = headers; this.fileName = fileName; this.value = null; - this.charset = null; + this.charset = charset; + this.bigField = bigField; } FormValueImpl(byte[] data, String fileName, HeaderMap headers) { @@ -315,12 +334,20 @@ static class FormValueImpl implements FormValue { this.headers = headers; this.value = null; this.charset = null; + this.bigField = false; } @Override public String getValue() { if (value == null) { + if (bigField) { + try (InputStream inputStream = getFileItem().getInputStream()) { + return FileUtils.readFile(inputStream, this.charset); + } catch (IOException e) { + // ignore + } + } throw UndertowMessages.MESSAGES.formValueIsAFile(); } return value; @@ -360,6 +387,10 @@ public FileItem getFileItem() { return fileItem; } + public boolean isBigField() { + return bigField; + } + @Override public boolean isFileItem() { return fileItem != null; diff --git a/core/src/main/java/io/undertow/server/handlers/form/MultiPartParserDefinition.java b/core/src/main/java/io/undertow/server/handlers/form/MultiPartParserDefinition.java index 0655f3758c..2a78e203b6 100644 --- a/core/src/main/java/io/undertow/server/handlers/form/MultiPartParserDefinition.java +++ b/core/src/main/java/io/undertow/server/handlers/form/MultiPartParserDefinition.java @@ -354,8 +354,7 @@ public void endPart() { if (fileName != null) { data.add(currentName, file, fileName, headers); } else { - final Path fileNamePath = file.getFileName(); - data.add(currentName, file, fileNamePath != null ? fileNamePath.toString() : "", headers); + data.add(currentName, file, null, headers, true, getCharset()); } file = null; contentBytes.reset(); @@ -369,18 +368,8 @@ public void endPart() { data.add(currentName, Arrays.copyOf(contentBytes.toByteArray(), contentBytes.size()), fileName, headers); contentBytes.reset(); } else { - - try { - String charset = defaultEncoding; - String contentType = headers.getFirst(Headers.CONTENT_TYPE); - if (contentType != null) { - String cs = Headers.extractQuotedValueFromHeader(contentType, "charset"); - if (cs != null) { - charset = cs; - } - } - + String charset = getCharset(); data.add(currentName, new String(contentBytes.toByteArray(), charset), charset, headers); } catch (UnsupportedEncodingException e) { throw new RuntimeException(e); @@ -389,6 +378,18 @@ public void endPart() { } } + private String getCharset() { + String charset = defaultEncoding; + String contentType = headers.getFirst(Headers.CONTENT_TYPE); + if (contentType != null) { + String cs = Headers.extractQuotedValueFromHeader(contentType, "charset"); + if (cs != null) { + charset = cs; + } + } + return charset; + } + public List getCreatedFiles() { return createdFiles; } diff --git a/core/src/main/java/io/undertow/util/FileUtils.java b/core/src/main/java/io/undertow/util/FileUtils.java index ad34a45b35..f47d3448c3 100644 --- a/core/src/main/java/io/undertow/util/FileUtils.java +++ b/core/src/main/java/io/undertow/util/FileUtils.java @@ -22,7 +22,9 @@ import java.io.IOException; import java.io.InputStream; import java.net.URL; +import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; +import java.nio.charset.UnsupportedCharsetException; import java.nio.file.FileVisitResult; import java.nio.file.Files; import java.nio.file.Path; @@ -51,16 +53,32 @@ public static String readFile(URL url) { } } + public static String readFile(InputStream file, String charset) { + try { + Charset charSet = charset != null ? Charset.forName(charset) : StandardCharsets.UTF_8; + return readFile(file, charSet); + } catch (UnsupportedCharsetException e) { + throw new RuntimeException(e); + } + } + /** * Reads the {@link InputStream file} and converting it to {@link String} using UTF-8 encoding. */ public static String readFile(InputStream file) { + return readFile(file, StandardCharsets.UTF_8); + } + + /** + * Reads the {@link InputStream file} and converting it to {@link String} using charSet encoding. + */ + public static String readFile(InputStream file, Charset charSet) { try (BufferedInputStream stream = new BufferedInputStream(file)) { byte[] buff = new byte[1024]; StringBuilder builder = new StringBuilder(); int read; while ((read = stream.read(buff)) != -1) { - builder.append(new String(buff, 0, read, StandardCharsets.UTF_8)); + builder.append(new String(buff, 0, read, charSet)); } return builder.toString(); } catch (IOException e) { diff --git a/core/src/test/java/io/undertow/server/handlers/form/MultipartFormDataParserTestCase.java b/core/src/test/java/io/undertow/server/handlers/form/MultipartFormDataParserTestCase.java index ecfe9f7420..c6ab4c792b 100644 --- a/core/src/test/java/io/undertow/server/handlers/form/MultipartFormDataParserTestCase.java +++ b/core/src/test/java/io/undertow/server/handlers/form/MultipartFormDataParserTestCase.java @@ -365,8 +365,7 @@ public void testLargeContentWithoutFileNameWithSmallFileSizeThreshold() throws E Assert.assertEquals("false", parsedResponse.get("in_memory")); Assert.assertEquals(DigestUtils.md5Hex(Files.newInputStream(file.toPath())), parsedResponse.get("hash")); - Assert.assertTrue(parsedResponse.get("file_name").startsWith("undertow")); - Assert.assertTrue(parsedResponse.get("file_name").endsWith("upload")); + Assert.assertEquals(parsedResponse.get("file_name"), "null"); } finally { if (!file.delete()) { file.deleteOnExit(); diff --git a/servlet/src/main/java/io/undertow/servlet/spec/HttpServletRequestImpl.java b/servlet/src/main/java/io/undertow/servlet/spec/HttpServletRequestImpl.java index e925aae843..57172ff10b 100644 --- a/servlet/src/main/java/io/undertow/servlet/spec/HttpServletRequestImpl.java +++ b/servlet/src/main/java/io/undertow/servlet/spec/HttpServletRequestImpl.java @@ -758,7 +758,7 @@ public String getParameter(final String name) { final FormData parsedFormData = parseFormData(); if (parsedFormData != null) { FormData.FormValue res = parsedFormData.getFirst(name); - if (res == null || res.isFileItem()) { + if (res == null || res.isFileItem() && !res.isBigField()) { return null; } else { return res.getValue(); @@ -781,7 +781,7 @@ public Enumeration getParameterNames() { while (it.hasNext()) { String name = it.next(); for(FormData.FormValue param : parsedFormData.get(name)) { - if(!param.isFileItem()) { + if(!param.isFileItem() || param.isBigField()) { parameterNames.add(name); break; } @@ -808,7 +808,7 @@ public String[] getParameterValues(final String name) { Deque res = parsedFormData.get(name); if (res != null) { for (FormData.FormValue value : res) { - if(!value.isFileItem()) { + if(!value.isFileItem() || value.isBigField()) { ret.add(value.getValue()); } } @@ -839,14 +839,14 @@ public Map getParameterMap() { if (arrayMap.containsKey(name)) { ArrayList existing = arrayMap.get(name); for (final FormData.FormValue v : val) { - if(!v.isFileItem()) { + if(!v.isFileItem() || v.isBigField()) { existing.add(v.getValue()); } } } else { final ArrayList values = new ArrayList<>(); for (final FormData.FormValue v : val) { - if(!v.isFileItem()) { + if(!v.isFileItem() || v.isBigField()) { values.add(v.getValue()); } } diff --git a/servlet/src/test/java/io/undertow/servlet/test/multipart/AddMultipartServetListener.java b/servlet/src/test/java/io/undertow/servlet/test/multipart/AddMultipartServetListener.java index 6c36bc7927..8fac05bd72 100644 --- a/servlet/src/test/java/io/undertow/servlet/test/multipart/AddMultipartServetListener.java +++ b/servlet/src/test/java/io/undertow/servlet/test/multipart/AddMultipartServetListener.java @@ -14,6 +14,10 @@ public void contextInitialized(ServletContextEvent sce) { ServletRegistration.Dynamic reg = sce.getServletContext().addServlet("added", new MultiPartServlet()); reg.addMapping("/added"); reg.setMultipartConfig(new MultipartConfigElement(System.getProperty("java.io.tmpdir"))); + + reg = sce.getServletContext().addServlet("getParam", new MultiPartServlet(true)); + reg.addMapping("/getParam"); + reg.setMultipartConfig(new MultipartConfigElement(System.getProperty("java.io.tmpdir"))); } @Override diff --git a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java index 77300eaecc..5759992438 100644 --- a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java +++ b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java @@ -37,6 +37,17 @@ */ public class MultiPartServlet extends HttpServlet { + private final boolean getParam; + public MultiPartServlet() { + super(); + this.getParam = false; + } + + public MultiPartServlet(boolean getParam) { + super(); + this.getParam = getParam; + } + @Override protected void doPost(final HttpServletRequest req, final HttpServletResponse resp) throws ServletException, IOException { try { @@ -58,6 +69,14 @@ protected void doPost(final HttpServletRequest req, final HttpServletResponse re writer.println("size: " + part.getSize()); writer.println("content: " + FileUtils.readFile(part.getInputStream())); } + if (getParam) { + Enumeration paramNames = req.getParameterNames(); + while (paramNames.hasMoreElements()) { + String name = paramNames.nextElement(); + writer.println("param name: " + name); + writer.println("param value: " + req.getParameter(name)); + } + } } catch (Exception e) { resp.getWriter().write("EXCEPTION: " + e.getClass()); } diff --git a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java index 6f3e34941c..5a4380c7e4 100644 --- a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java +++ b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java @@ -251,4 +251,52 @@ public void testMultiPartRequestUtf8CharsetInPart() throws IOException { } } + @Test + public void testMultiPartRequestBigPostForm() throws IOException { + TestHttpClient client = new TestHttpClient(); + try { + String uri = DefaultServer.getDefaultServerURL() + "/servletContext/getParam"; + HttpPost post = new HttpPost(uri); + + MultipartEntity entity = new MultipartEntity(HttpMultipartMode.BROWSER_COMPATIBLE, null, StandardCharsets.UTF_8); + + String myValue = generateContent("myValue", 0x4000 * 2); + entity.addPart("formValue", new StringBody(myValue, "text/plain", StandardCharsets.UTF_8)); + entity.addPart("file", new FileBody(new File(MultiPartTestCase.class.getResource("uploadfile.txt").getFile()))); + + post.setEntity(entity); + HttpResponse result = client.execute(post); + Assert.assertEquals(StatusCodes.OK, result.getStatusLine().getStatusCode()); + final String response = HttpClientUtils.readResponse(result); + Assert.assertEquals("PARAMS:\r\n" + + "parameter count: 1\r\n" + + "parameter name count: 1\r\n" + + "name: formValue\r\n" + + "filename: null\r\n" + + "content-type: null\r\n" + + "Content-Disposition: form-data; name=\"formValue\"\r\n" + + "size: " + myValue.getBytes(StandardCharsets.UTF_8).length + "\r\n" + + "content: " + myValue + "\r\n" + + "name: file\r\n" + + "filename: uploadfile.txt\r\n" + + "content-type: application/octet-stream\r\n" + + "Content-Disposition: form-data; name=\"file\"; filename=\"uploadfile.txt\"\r\n" + + "Content-Type: application/octet-stream\r\n" + + "size: 13\r\n" + + "content: file contents\r\n" + + "param name: formValue\r\n" + + "param value: " + myValue + "\r\n", response); + } finally { + client.getConnectionManager().shutdown(); + } + } + + private String generateContent(String chunk, int size) { + int checkLength = chunk.getBytes().length; + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < size / checkLength; i++) { + sb.append(chunk); + } + return sb.toString(); + } } From 765f69b52e8f6344a762a388ff09e1f61cb39138 Mon Sep 17 00:00:00 2001 From: Flavia Rainone Date: Mon, 12 Feb 2024 16:50:06 -0300 Subject: [PATCH 2/2] [UNDERTOW-2337] Also verify if the value of the part can be retrieved as a parameter Signed-off-by: Flavia Rainone --- .../servlet/test/multipart/MultiPartServlet.java | 12 ++++++++++++ .../servlet/test/multipart/MultiPartTestCase.java | 4 ++++ 2 files changed, 16 insertions(+) diff --git a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java index 5759992438..12cce345f4 100644 --- a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java +++ b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java @@ -24,6 +24,7 @@ import java.util.Enumeration; import java.util.TreeSet; +import io.undertow.util.Headers; import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServlet; import jakarta.servlet.http.HttpServletRequest; @@ -65,6 +66,17 @@ protected void doPost(final HttpServletRequest req, final HttpServletResponse re Collection headerNames = new TreeSet<>(part.getHeaderNames()); for (String header : headerNames) { writer.println(header + ": " + part.getHeader(header)); + if (header.equals(Headers.CONTENT_DISPOSITION_STRING)) { + final String parameterValue = part.getHeader(header); + String parameterName = parameterValue.substring(parameterValue.indexOf("name=") + "name=\"".length()); + parameterName = parameterName.substring(0, parameterName.indexOf('\"')); + final String[] values = req.getParameterValues(parameterName); + if (values != null) { + for (String value: values) { + writer.println("value: " + value); + } + } + } } writer.println("size: " + part.getSize()); writer.println("content: " + FileUtils.readFile(part.getInputStream())); diff --git a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java index 5a4380c7e4..7c63de3e22 100644 --- a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java +++ b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java @@ -126,6 +126,7 @@ public void testMultiPartRequest() throws IOException { "filename: null\r\n" + "content-type: null\r\n" + "Content-Disposition: form-data; name=\"formValue\"\r\n" + + "value: myValue\r\n" + "size: 7\r\n" + "content: myValue\r\n" + "name: file\r\n" + @@ -163,6 +164,7 @@ public void testMultiPartRequestWithAddedServlet() throws IOException { "filename: null\r\n" + "content-type: null\r\n" + "Content-Disposition: form-data; name=\"formValue\"\r\n" + + "value: myValue\r\n" + "size: 7\r\n" + "content: myValue\r\n" + "name: file\r\n" + @@ -242,6 +244,7 @@ public void testMultiPartRequestUtf8CharsetInPart() throws IOException { "filename: null\r\n" + "content-type: text/plain; charset=UTF-8\r\n" + "Content-Disposition: form-data; name=\"formValue\"\r\n" + + "value: " + "myValue" + '\u00E5' + "\r\n" + "Content-Transfer-Encoding: 8bit\r\n" + "Content-Type: text/plain; charset=UTF-8\r\n" + "size: 9\r\n" + @@ -275,6 +278,7 @@ public void testMultiPartRequestBigPostForm() throws IOException { "filename: null\r\n" + "content-type: null\r\n" + "Content-Disposition: form-data; name=\"formValue\"\r\n" + + "value: " + myValue + "\r\n" + "size: " + myValue.getBytes(StandardCharsets.UTF_8).length + "\r\n" + "content: " + myValue + "\r\n" + "name: file\r\n" +