This is an automated email from the ASF dual-hosted git repository. dashorst pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/wicket.git
commit a15361f9d7d8e722f281b7c76ea83e7f7ba87c97 Author: reiern70 <[email protected]> AuthorDate: Mon Sep 14 09:41:08 2026 -0500 Take upload details of FileUploadToResourceField from the stored files FileUploadToResourceField builds the list of UploadInfo it hands to onUploadSuccess from the JSON the browser sends back once the upload to the resource has finished. Until now that list was used as received: UploadInfo#getSize() returned the size named in the JSON, UploadInfo#get() sized its buffer from that value before opening the file, and every listed entry was passed on, whether or not a file had been stored for it. The field now takes these details from the files the IUploadsFileManager has stored: - UploadInfo no longer keeps a size. getSize() returns the length of the stored file, and 0 when none is attached. - UploadInfo#get() returns null unless a stored file is attached, and otherwise reads that file as it is on disk. - The callback passes on each listed file once, and only those that have been stored. A list naming more files than getFileCountMax() is reported to onUploadFailure with the uploadTooManyFiles message, and a list with no stored file left with the message for no selected files. Both failure paths of the callback go through one private method, which also forgets the files of an earlier upload. The constructor UploadInfo(fileName, clientFileName, size, contentType), new in 11.0.0 and not yet released, becomes UploadInfo(fileName, clientFileName, contentType). The deprecated UploadInfo(clientFileName, size, contentType) stays and ignores the size. An application sees, in onUploadSuccess, only the files that were actually stored, each once, and getSize() and get() describe those files. migration-to-11.md says so. --- migration-to-11.md | 3 +- .../FileUploadToResourceFieldSecurityTest.java | 163 +++++++++++++++++++++ .../resource/FileUploadToResourceFieldTest.java | 6 +- .../upload/resource/FileUploadToResourceField.java | 98 ++++++++----- 4 files changed, 232 insertions(+), 38 deletions(-) diff --git a/migration-to-11.md b/migration-to-11.md index 78b4bf9e31..cbd56258af 100644 --- a/migration-to-11.md +++ b/migration-to-11.md @@ -169,7 +169,8 @@ yourself. | | Before | After | |---|---|---| -| Constructor | `UploadInfo(String clientFileName, long size, String contentType)` | `UploadInfo(String fileName, String clientFileName, long size, String contentType)`. The old one is deprecated and uses the client file name as the stored name | +| Constructor | `UploadInfo(String clientFileName, long size, String contentType)` | `UploadInfo(String fileName, String clientFileName, String contentType)`. The old one is deprecated, uses the client file name as the stored name and ignores the size | +| `getSize()` and `get()` | The size the browser reported | The length and the bytes of the stored file; 0 and `null` when no stored file is attached | | Stored name | None | `getFileName()` | | `equals()` and `hashCode()` | By client file name | By stored name: two files of the same client name are no longer equal | | `fromJson()` | Reads `clientFileName`, `size`, `contentType` | Also reads `fileName`, and falls back to `clientFileName` when it is missing, as in the answer of a resource written for Wicket 10 | diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldSecurityTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldSecurityTest.java index cc1dc7b01c..81a12517ef 100644 --- a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldSecurityTest.java +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldSecurityTest.java @@ -17,15 +17,21 @@ package org.apache.wicket.markup.html.form.upload.resource; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; import java.util.Comparator; import java.util.List; +import org.apache.wicket.ajax.AbstractDefaultAjaxBehavior; import org.apache.wicket.ajax.AjaxRequestTarget; +import org.apache.wicket.markup.html.form.upload.resource.FileUploadToResourceField.UploadInfo; import org.apache.wicket.markup.html.WebPage; import org.apache.wicket.request.mapper.parameter.PageParameters; import org.apache.wicket.util.lang.Bytes; @@ -34,6 +40,7 @@ import org.apache.wicket.util.tester.WicketTestCase; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; import com.github.openjson.JSONArray; +import com.github.openjson.JSONObject; class FileUploadToResourceFieldSecurityTest extends WicketTestCase { @@ -100,6 +107,151 @@ class FileUploadToResourceFieldSecurityTest extends WicketTestCase assertTrue(tester.getLastResponseAsString().contains("uploadFailed")); } + @Test + void callbackWithoutStoredFileReportsFailure() throws Exception + { + UploadPage page = new UploadPage(new TestFileUploadResourceReference(newFileManager())); + tester.startPage(page); + + executeCallback(page, fileInfo("a.txt", 2_000_000_000L), fileInfo("b.txt", 2_000_000_000L)); + + assertNull(page.field.received); + assertNotNull(page.field.failure); + } + + @Test + void callbackReportsSizeAndBytesOfStoredFile() throws Exception + { + FolderUploadsFileManager fileManager = newFileManager(); + UploadPage page = new UploadPage(new TestFileUploadResourceReference(fileManager)); + tester.startPage(page); + File upload = createUploadFile("stored.txt", 256); + submitUpload(page.field.buildUploadUrl(), upload); + String storedName = storedFileName(); + + executeCallback(page, fileInfo(storedName, upload.getName(), 2_000_000_000L)); + + assertNull(page.field.failure); + assertEquals(1, page.field.received.size()); + UploadInfo info = page.field.received.get(0); + assertEquals(256L, info.getSize()); + assertEquals(256, info.get().length); + } + + @Test + void callbackIgnoresEntriesWithoutStoredFile() throws Exception + { + FolderUploadsFileManager fileManager = newFileManager(); + UploadPage page = new UploadPage(new TestFileUploadResourceReference(fileManager)); + tester.startPage(page); + File upload = createUploadFile("stored.txt", 16); + submitUpload(page.field.buildUploadUrl(), upload); + String storedName = storedFileName(); + + executeCallback(page, fileInfo("missing.txt", 1L), + fileInfo(storedName, upload.getName(), 16L), fileInfo(storedName, upload.getName(), 16L)); + + assertNull(page.field.failure); + assertEquals(1, page.field.received.size()); + assertEquals(upload.getName(), page.field.received.get(0).getClientFileName()); + } + + @Test + void callbackListingMoreFilesThanAllowedReportsFailure() throws Exception + { + FolderUploadsFileManager fileManager = newFileManager(); + UploadPage page = new UploadPage(new TestFileUploadResourceReference(fileManager)); + page.field.setFileCountMax(1); + tester.startPage(page); + File upload = createUploadFile("stored.txt", 16); + submitUpload(page.field.buildUploadUrl(), upload); + String storedName = storedFileName(); + + executeCallback(page, fileInfo(storedName, upload.getName(), 16L), + fileInfo(storedName, upload.getName(), 16L)); + + assertNull(page.field.received); + assertNotNull(page.field.failure); + } + + @Test + void failedCallbackClearsPreviousUploads() throws Exception + { + FolderUploadsFileManager fileManager = newFileManager(); + UploadPage page = new UploadPage(new TestFileUploadResourceReference(fileManager)); + page.field.setFileCountMax(1); + tester.startPage(page); + File upload = createUploadFile("stored.txt", 16); + submitUpload(page.field.buildUploadUrl(), upload); + String storedName = storedFileName(); + executeCallback(page, fileInfo(storedName, upload.getName(), 16L)); + assertEquals(1, page.field.received.size()); + + executeCallback(page, fileInfo(storedName, upload.getName(), 16L), + fileInfo("other.txt", 16L)); + + assertNotNull(page.field.failure); + assertTrue(((List<?>)page.field.getDefaultModelObject()).isEmpty()); + } + + @Test + void getReadsStoredFile() throws Exception + { + newFileManager(); + File stored = createUploadFile("stored.txt", 32); + + UploadInfo info = new UploadInfo(stored.getName(), stored.getName(), "text/plain"); + info.setFile(stored); + + assertEquals(32L, info.getSize()); + assertArrayEquals(new byte[32], info.get()); + } + + @Test + void getReturnsNullWithoutStoredFile() throws Exception + { + newFileManager(); + UploadInfo info = new UploadInfo("missing.txt", "missing.txt", "text/plain"); + info.setFile(new File(tempDir.resolve("missing.txt").toFile())); + + assertNull(info.get()); + assertEquals(0L, info.getSize()); + } + + private String storedFileName() + { + return new JSONArray(tester.getLastResponseAsString()).getJSONObject(0) + .getString("fileName"); + } + + private static JSONObject fileInfo(String clientFileName, long size) + { + JSONObject json = new JSONObject(); + json.put("clientFileName", clientFileName); + json.put("size", size); + json.put("contentType", "text/plain"); + return json; + } + + private static JSONObject fileInfo(String fileName, String clientFileName, long size) + { + JSONObject json = fileInfo(clientFileName, size); + json.put("fileName", fileName); + return json; + } + + private void executeCallback(UploadPage page, JSONObject... fileInfos) + { + JSONArray filesInfo = new JSONArray(); + for (JSONObject fileInfo : fileInfos) + { + filesInfo.put(fileInfo); + } + tester.getRequest().setParameter("error", "false"); + tester.getRequest().setParameter("filesInfo", filesInfo.toString()); + tester.executeBehavior(page.field.getBehaviors(AbstractDefaultAjaxBehavior.class).get(0)); + } + private FolderUploadsFileManager newFileManager() throws IOException { tempDir = Files.createTempDirectory("wicket-upload-security"); @@ -140,6 +292,10 @@ class FileUploadToResourceFieldSecurityTest extends WicketTestCase private final TestFileUploadResourceReference resourceReference; + List<UploadInfo> received; + + String failure; + TestField(String id, TestFileUploadResourceReference resourceReference) { super(id); @@ -155,6 +311,13 @@ class FileUploadToResourceFieldSecurityTest extends WicketTestCase @Override protected void onUploadSuccess(AjaxRequestTarget target, List<UploadInfo> fileInfos) { + received = new ArrayList<>(fileInfos); + } + + @Override + protected void onUploadFailure(AjaxRequestTarget target, String errorInfo) + { + failure = errorInfo; } String buildUploadUrl() diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldTest.java index da0f6739ce..72e43dcee1 100644 --- a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldTest.java +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldTest.java @@ -222,11 +222,11 @@ class FileUploadToResourceFieldTest extends WicketTestCase void uploadInfosAreEqualByTheirStoredName() { FileUploadToResourceField.UploadInfo info = new FileUploadToResourceField.UploadInfo( - "stored-1", "same.txt", 3, "text/plain"); + "stored-1", "same.txt", "text/plain"); FileUploadToResourceField.UploadInfo sameStoredName = - new FileUploadToResourceField.UploadInfo("stored-1", "other.txt", 5, "text/html"); + new FileUploadToResourceField.UploadInfo("stored-1", "other.txt", "text/html"); FileUploadToResourceField.UploadInfo otherStoredName = - new FileUploadToResourceField.UploadInfo("stored-2", "same.txt", 3, "text/plain"); + new FileUploadToResourceField.UploadInfo("stored-2", "same.txt", "text/plain"); assertEquals(info, sameStoredName); assertEquals(info.hashCode(), sameStoredName.hashCode()); diff --git a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceField.java b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceField.java index ea002d8231..d633e72ac0 100644 --- a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceField.java +++ b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceField.java @@ -17,17 +17,16 @@ package org.apache.wicket.markup.html.form.upload.resource; import java.io.File; -import java.io.FileInputStream; import java.io.IOException; -import java.io.InputStream; +import java.nio.file.Files; import java.util.ArrayList; import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.UUID; -import org.apache.commons.io.IOUtils; import org.apache.wicket.ajax.AbstractDefaultAjaxBehavior; import org.apache.wicket.ajax.AjaxRequestTarget; import org.apache.wicket.ajax.AjaxUtils; @@ -65,6 +64,9 @@ public abstract class FileUploadToResourceField extends FileUploadField /** * Info regarding an upload. + * <p> + * The stored name, the client file name and the content type are reported by the browser and + * must not be trusted. The size and the bytes are taken from the stored file. */ public static final class UploadInfo { @@ -72,37 +74,38 @@ public abstract class FileUploadToResourceField extends FileUploadField private final String fileName; private final String clientFileName; - private final long size; - private final String contentType; /** * @param fileName * the name the {@link IUploadsFileManager} stored the file under * @param clientFileName - * the name of the file on the client - * @param size - * the size of the file in bytes + * the file name reported by the browser * @param contentType - * the content type of the file + * the content type reported by the browser * @since 11.0.0 */ - public UploadInfo(String fileName, String clientFileName, long size, String contentType) + public UploadInfo(String fileName, String clientFileName, String contentType) { this.fileName = fileName; this.clientFileName = clientFileName; - this.size = size; this.contentType = contentType; } /** + * @param clientFileName + * the file name reported by the browser + * @param size + * ignored, the size is taken from the stored file + * @param contentType + * the content type reported by the browser * @deprecated the file is then looked up by its client file name, which several files can - * share. Use {@link #UploadInfo(String, String, long, String)} instead. + * share. Use {@link #UploadInfo(String, String, String)} instead. */ @Deprecated(since = "11.0.0", forRemoval = true) public UploadInfo(String clientFileName, long size, String contentType) { - this(clientFileName, clientFileName, size, contentType); + this(clientFileName, clientFileName, contentType); } public File getFile() @@ -129,9 +132,21 @@ public abstract class FileUploadToResourceField extends FileUploadField return clientFileName; } + /** + * @return the length of the stored file, or 0 if no stored file is present + */ public long getSize() { - return size; + if (isStored()) + { + return file.length(); + } + return 0; + } + + private boolean isStored() + { + return file != null && file.isFile(); } public String getContentType() @@ -149,35 +164,27 @@ public abstract class FileUploadToResourceField extends FileUploadField } /** - * @return the bytes associated with the upload. Return null in case no file is present + * Reads the stored file into memory. + * + * @return the bytes of the stored file. Return null in case no file is present * or some other error condition happens. */ public byte[] get() { - if (file == null) + if (!isStored()) { return null; } - byte[] fileData = new byte[(int) getSize()]; - InputStream fis = null; - try { - fis = new FileInputStream(file); - IOUtils.readFully(fis, fileData); + return Files.readAllBytes(file.toPath()); } catch (IOException e) { LOGGER.debug("IOException at get", e); - fileData = null; - } - finally - { - IOUtils.closeQuietly(fis); + return null; } - - return fileData; } @Override @@ -196,7 +203,7 @@ public abstract class FileUploadToResourceField extends FileUploadField String clientFileName = jsonObject.getString("clientFileName"); // a resource written before files had names of their own stores them by client name infos.add(new UploadInfo(jsonObject.optString("fileName", clientFileName), - clientFileName, jsonObject.getLong("size"), jsonObject.getString("contentType"))); + clientFileName, jsonObject.getString("contentType"))); } return infos; } @@ -241,6 +248,7 @@ public abstract class FileUploadToResourceField extends FileUploadField // so that uploaded files are isolated. uploadInfo.setFile(fileManager().getFile(getUploadFieldId(), uploadInfo.getFileName())); } + fileInfos.removeIf(uploadInfo -> !uploadInfo.isStored()); return fileInfos; } @@ -314,8 +322,20 @@ public abstract class FileUploadToResourceField extends FileUploadField boolean error = parameters.getParameterValue("error").toBoolean(true); if (!error) { String filesIfo = parameters.getParameterValue("filesInfo").toString(); - fileUploadInfos = UploadInfo.fromJson(filesIfo); - onUploadSuccess(target, getFileUploadInfos()); + List<UploadInfo> reportedInfos = UploadInfo.fromJson(filesIfo); + if (getFileCountMax() > -1 && reportedInfos.size() > getFileCountMax()) + { + reportFailure(target, Form.UPLOAD_TOO_MANY_FILES_RESOURCE_KEY); + return; + } + fileUploadInfos = new ArrayList<>(new LinkedHashSet<>(reportedInfos)); + List<UploadInfo> storedInfos = getFileUploadInfos(); + if (storedInfos.isEmpty()) + { + reportFailure(target, AbstractFileUploadResource.NO_FILE_SELECTED); + return; + } + onUploadSuccess(target, storedInfos); } else { @@ -326,9 +346,7 @@ public abstract class FileUploadToResourceField extends FileUploadField } else { - String errorMessage = getUploadErrorMessage(errorKey); - error(errorMessage); - onUploadFailure(target, errorMessage); + reportFailure(target, errorKey); } } } @@ -336,6 +354,14 @@ public abstract class FileUploadToResourceField extends FileUploadField add(ajaxBehavior); } + private void reportFailure(AjaxRequestTarget target, String errorKey) + { + fileUploadInfos = new ArrayList<>(); + String errorMessage = getUploadErrorMessage(errorKey); + error(errorMessage); + onUploadFailure(target, errorMessage); + } + /** * Words the error of a failed upload, which {@link #onUploadFailure(AjaxRequestTarget, String)} * then receives. The key comes from the browser, which can send any, so it is never looked up @@ -389,6 +415,10 @@ public abstract class FileUploadToResourceField extends FileUploadField /** * Override to do something on a successful upload. + * <p> + * Only uploads whose file is present in the {@link IUploadsFileManager} are passed, each once. + * An upload result listing more files than {@link #getFileCountMax()}, or none that are + * present, is reported to {@link #onUploadFailure(AjaxRequestTarget, String)} instead. * * @param target The {@link AjaxRequestTarget} * @param fileInfos The List<FileInfo
