This is an automated email from the ASF dual-hosted git repository. dashorst pushed a commit to branch wicket-10.x in repository https://gitbox.apache.org/repos/asf/wicket.git
commit e254a030bfe0ec2668cf4c2bf18e3fd9ffcae234 Author: Emond Papegaaij <[email protected]> AuthorDate: Wed Sep 16 14:50:19 2026 +0200 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#get() returns null unless a stored file is attached, and otherwise reads that file as it is on disk. - UploadInfo#getSize() returns the length of the stored file when one is attached, and the size it was constructed with otherwise. - 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. The failure handling of the callback moves into a private method shared by both paths; its messages are unchanged. No signature changes. An application sees, in onUploadSuccess, only the files that were actually stored, each once, and getSize() and get() describe those files. An application that constructs UploadInfo itself without attaching a file keeps getting the size it passed in, and null from get(). --- .../FileUploadToResourceFieldSecurityTest.java | 145 +++++++++++++++++++++ .../upload/resource/FileUploadToResourceField.java | 135 +++++++++++++------ 2 files changed, 237 insertions(+), 43 deletions(-) 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 0251ba071a..bb11f70067 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; @@ -33,6 +39,8 @@ import org.apache.wicket.util.file.File; 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 { @@ -98,6 +106,132 @@ 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); + + executeCallback(page, fileInfo(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); + + executeCallback(page, fileInfo("missing.txt", 1L), fileInfo(upload.getName(), 16L), + fileInfo(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); + + executeCallback(page, fileInfo(upload.getName(), 16L), fileInfo(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); + executeCallback(page, fileInfo(upload.getName(), 16L)); + assertEquals(1, page.field.received.size()); + + executeCallback(page, fileInfo(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(), "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", "text/plain"); + info.setFile(new File(tempDir.resolve("missing.txt").toFile())); + + assertNull(info.get()); + assertEquals(0L, info.getSize()); + } + + 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 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"); @@ -138,6 +272,10 @@ class FileUploadToResourceFieldSecurityTest extends WicketTestCase private final TestFileUploadResourceReference resourceReference; + List<UploadInfo> received; + + String failure; + TestField(String id, TestFileUploadResourceReference resourceReference) { super(id); @@ -153,6 +291,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/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 47d10fae0e..a886739e36 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,16 +17,15 @@ 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.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; @@ -34,6 +33,7 @@ import org.apache.wicket.core.request.handler.IPartialPageRequestHandler; import org.apache.wicket.markup.head.IHeaderResponse; import org.apache.wicket.markup.head.JavaScriptHeaderItem; import org.apache.wicket.markup.head.OnDomReadyHeaderItem; +import org.apache.wicket.markup.html.form.Form; import org.apache.wicket.markup.html.form.upload.FileUpload; import org.apache.wicket.markup.html.form.upload.FileUploadField; import org.apache.wicket.model.IModel; @@ -61,23 +61,45 @@ public abstract class FileUploadToResourceField extends FileUploadField /** * Info regarding an upload. + * <p> + * 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 { private File file; private final String clientFileName; - private final long size; - private final String contentType; - public UploadInfo(String clientFileName, long size, String contentType) + /** + * @param clientFileName + * the file name reported by the browser + * @param contentType + * the content type reported by the browser + * @since 10.12.0 + */ + public UploadInfo(String clientFileName, String contentType) { 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 use {@link #UploadInfo(String, String)} instead + */ + @Deprecated(since = "10.12.0", forRemoval = true) + public UploadInfo(String clientFileName, long size, String contentType) + { + this(clientFileName, contentType); + } + public File getFile() { return file; @@ -93,9 +115,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() @@ -113,35 +147,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 @@ -157,7 +183,7 @@ public abstract class FileUploadToResourceField extends FileUploadField for (int i = 0; i < jsonArray.length(); i++) { JSONObject jsonObject = jsonArray.getJSONObject(i); - infos.add(new UploadInfo(jsonObject.getString("clientFileName"), jsonObject.getLong("size"), jsonObject.getString("contentType"))); + infos.add(new UploadInfo(jsonObject.getString("clientFileName"), jsonObject.getString("contentType"))); } return infos; } @@ -182,6 +208,7 @@ public abstract class FileUploadToResourceField extends FileUploadField // so that uploaded files are isolated. uploadInfo.setFile(fileManager().getFile(getUploadFieldId(), uploadInfo.clientFileName)); } + fileInfos.removeIf(uploadInfo -> !uploadInfo.isStored()); return fileInfos; } @@ -255,8 +282,20 @@ public abstract class FileUploadToResourceField extends FileUploadField boolean error = request.getRequestParameters().getParameterValue("error").toBoolean(true); if (!error) { String filesIfo = request.getRequestParameters().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 { @@ -267,24 +306,7 @@ public abstract class FileUploadToResourceField extends FileUploadField } else { - if (AbstractFileUploadResource.NO_FILE_SELECTED.equals(errorMessage)) - { - errorMessage = getString(AbstractFileUploadResource.NO_FILE_SELECTED); - } - else - { - final Map<String, Object> model = new HashMap<>(); - if (Strings.isEmpty(errorMessage)) { - errorMessage = "uploadTooLarge"; - } - model.put("exception", errorMessage); - model.put("maxSize", getMaxSize()); - model.put("fileMaxSize", getFileMaxSize()); - model.put("fileCountMax", getFileCountMax()); - errorMessage = getString(errorMessage, Model.ofMap(model)); - } - error(errorMessage); - onUploadFailure(target, errorMessage); + reportFailure(target, errorMessage); } } } @@ -292,6 +314,29 @@ public abstract class FileUploadToResourceField extends FileUploadField add(ajaxBehavior); } + private void reportFailure(AjaxRequestTarget target, String errorMessage) + { + fileUploadInfos = new ArrayList<>(); + if (AbstractFileUploadResource.NO_FILE_SELECTED.equals(errorMessage)) + { + errorMessage = getString(AbstractFileUploadResource.NO_FILE_SELECTED); + } + else + { + final Map<String, Object> model = new HashMap<>(); + if (Strings.isEmpty(errorMessage)) { + errorMessage = "uploadTooLarge"; + } + model.put("exception", errorMessage); + model.put("maxSize", getMaxSize()); + model.put("fileMaxSize", getFileMaxSize()); + model.put("fileCountMax", getFileCountMax()); + errorMessage = getString(errorMessage, Model.ofMap(model)); + } + error(errorMessage); + onUploadFailure(target, errorMessage); + } + private List<UploadInfo> getFileUploadInfos() { // mind that this makes files to be added. @@ -321,6 +366,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
