This is an automated email from the ASF dual-hosted git repository. reiern70 pushed a commit to branch upload-to-resource in repository https://gitbox.apache.org/repos/asf/wicket.git
commit a75841a9de31f8f4e50737d6b6bd4732ebcc848c Author: reiern70 <[email protected]> AuthorDate: Thu Oct 15 10:50:51 2026 -0500 Make uploads to a resource fail cleanly and keep files of one name apart GitHub issue #1670 FileUploadToResourceField and the resource it uploads to had several problems: - A failed upload ended on the error page instead of in onUploadFailure(). For a failure other than a limit the resource answers the key uploadFailed, whose text expects an exception in ${exception.localizedMessage}; the field put the key, a String, in its place, and the interpolation threw. Such failures now have a text of their own, uploadToResourceFailed ("The upload failed."). - The field took whatever the browser reported as a resource key. The status text of an HTTP error, or the exception message the resource sent with debug logging on, threw MissingResourceException, and an empty one (HTTP/2, a network failure) read "File must be less than ...". The field now accepts the keys of the limits and of no files selected, and reads anything else as a failed upload. The script reports HTTP errors, network failures and answers that are not JSON as uploadFailed. The resource no longer sends exception messages to the browser; it logs the exception as an error, which it did not do at all before. - The script checks the limits of the field before sending: files beyond them are reported with the key of the limit and not sent. Before, the server refused them, often by resetting the connection, which the browser saw as a network failure. - A second upload started while one ran sent a second request, which shared the progress information of the first in the session. Whichever ended first removed it, and the other failed with an IllegalStateException. The script now refuses to start an upload while one of the field runs. - The script reported the received files to the field by GET, with their JSON in the URL. Beyond some 65 files that exceeded the 8 KB request line of Jetty and Tomcat: the server answered 414 and onUploadSuccess() was never called, although the files were stored. They are reported by POST now. - FolderUploadsFileManager stored each file under its client file name, so two files of one name, in one upload or in two, overwrote each other while the field was told of both. Files are now stored under a random name, which the resource sends to the browser with the client file name and UploadInfo carries back to the field. - cancel() after an upload had ended still claimed to cancel it, the return value of the before-send callback was ignored, and an answer that was not JSON left the field waiting. Now cancel() does nothing once the upload has ended, false from the before-send callback keeps the upload from being sent and calls onUploadCanceled(), and an answer that is not JSON fails the upload. The Javadoc of AbstractFileUploadResource said the field uploads a single file with jQuery; it uploads all the files of its input with a plain XMLHttpRequest. Keeping files of one name apart needs a name for each stored file that travels from the file manager through the browser back to the field, so the API changes: - IUploadsFileManager gets store(FileUpload, String), which returns the name the file was stored under, and getFile(String, String) takes that name. save() is deprecated. Its default calls store(), and the default of store() calls save() and returns the client file name, so an implementation written for save() keeps working, with the old behaviour for files of one name. - AbstractFileUploadResource.saveFiles() returns the stored names, and prepareResponse() and generateJsonResponse() receive them. These are hooks for subclasses: an old signature kept as deprecated would no longer be called, so the signatures change and an override fails to compile rather than being skipped silently. - FileUploadResourceReference.generateFileInfo() receives the stored name and puts it into the JSON as fileName. - UploadInfo carries the stored name (getFileName() and a new constructor), and equals() and hashCode() use it instead of the client file name. The old constructor is deprecated. A JSON answer without fileName, from a custom resource, falls back to the client file name. What an application sees after the change: files stored by FolderUploadsFileManager no longer have their client file name on disk, so code looking them up with getFile(fieldId, clientFileName) has to use UploadInfo#getFile() or #getFileName() instead. A failed upload calls onUploadFailure() with "The upload failed."; an application that had changed the text of uploadFailed to work around the error page changes uploadToResourceFailed instead. The client-side error callback receives only resource keys as errorMessage. A before-send callback returning false now keeps the upload from being sent. Draft for the migration guide: Uploads to a resource (FileUploadToResourceField): implement IUploadsFileManager#store(FileUpload, String) instead of the deprecated save(). It stores the file under a name of its own and returns that name, which getFile(uploadFieldId, fileName) then receives; the name goes to the browser and back, so getFile() must not return a file of another upload field. FolderUploadsFileManager stores files under random names: find them with UploadInfo#getFile() or UploadInfo#getFileName(), not by client file name. Subclasses of AbstractFileUploadResource receive the stored names in saveFiles(), prepareResponse() and generateJsonResponse(), subclasses of FileUploadResourceReference in generateFileInfo(). A failed upload reads "The upload failed." (resource key uploadToResourceFailed) instead of ending on the error page. --- testing/wicket-js-tests/Gruntfile.js | 3 +- .../FileUploadToResourceFieldSecurityTest.java | 8 +- .../FileUploadToResourceFieldTest$UploadPage.html | 5 + .../resource/FileUploadToResourceFieldTest.java | 293 +++++++++++++++++++++ .../resource/FolderUploadsFileManagerTest.java | 38 +++ .../resource/AbstractFileUploadResource.java | 97 +++---- .../resource/FileUploadResourceReference.java | 22 +- .../upload/resource/FileUploadToResourceField.java | 129 ++++++--- .../upload/resource/FileUploadToResourceField.js | 137 +++++++--- .../resource/FileUploadToResourceField.properties | 3 +- .../upload/resource/FolderUploadsFileManager.java | 37 ++- .../form/upload/resource/IUploadsFileManager.java | 50 +++- wicket-core/src/test/js/all.html | 7 + wicket-core/src/test/js/upload-to-resource.js | 221 ++++++++++++++++ .../src/main/resources/META-INF/rewrite/wicket.yml | 10 + 15 files changed, 903 insertions(+), 157 deletions(-) diff --git a/testing/wicket-js-tests/Gruntfile.js b/testing/wicket-js-tests/Gruntfile.js index 848722e083..e9cfb0d78e 100644 --- a/testing/wicket-js-tests/Gruntfile.js +++ b/testing/wicket-js-tests/Gruntfile.js @@ -46,7 +46,8 @@ module.exports = function(grunt) { "../../wicket-core/src/test/js/dom.js", "../../wicket-core/src/test/js/channels.js", "../../wicket-core/src/test/js/event.js", - "../../wicket-core/src/test/js/timer.js" + "../../wicket-core/src/test/js/timer.js", + "../../wicket-core/src/test/js/upload-to-resource.js" ], extensionsTestsJs = [ "../../wicket-extensions/src/test/js/palette-test.js", 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..cc1dc7b01c 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 @@ -33,6 +33,7 @@ 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; class FileUploadToResourceFieldSecurityTest extends WicketTestCase { @@ -70,7 +71,9 @@ class FileUploadToResourceFieldSecurityTest extends WicketTestCase File upload = createUploadFile("allowed.txt", 256); submitUpload(page.field.buildUploadUrl(), upload); - File storedFile = fileManager.getFile(page.field.getMarkupId(), upload.getName()); + String fileName = new JSONArray(tester.getLastResponseAsString()).getJSONObject(0) + .getString("fileName"); + File storedFile = fileManager.getFile(page.field.getMarkupId(), fileName); assertTrue(storedFile.exists()); assertEquals(upload.length(), storedFile.length()); } @@ -93,8 +96,7 @@ class FileUploadToResourceFieldSecurityTest extends WicketTestCase submitUpload(tamperedUrl, upload); - File storedFile = fileManager.getFile(page.field.getMarkupId(), upload.getName()); - assertFalse(storedFile.exists()); + assertFalse(Files.exists(tempDir.resolve(page.field.getMarkupId()))); assertTrue(tester.getLastResponseAsString().contains("uploadFailed")); } diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldTest$UploadPage.html b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldTest$UploadPage.html new file mode 100644 index 0000000000..2658b563d3 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldTest$UploadPage.html @@ -0,0 +1,5 @@ +<html xmlns:wicket="http://wicket.apache.org";> +<body> + <input type="file" wicket:id="upload"/> +</body> +</html> 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 new file mode 100644 index 0000000000..faec0fdbbd --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldTest.java @@ -0,0 +1,293 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.wicket.markup.html.form.upload.resource; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import org.apache.wicket.ajax.AbstractDefaultAjaxBehavior; +import org.apache.wicket.ajax.AjaxRequestTarget; +import org.apache.wicket.markup.html.WebPage; +import org.apache.wicket.markup.html.form.upload.FileUpload; +import org.apache.wicket.request.mapper.parameter.PageParameters; +import org.apache.wicket.util.file.File; +import org.apache.wicket.util.lang.Bytes; +import org.apache.wicket.util.tester.WicketTestCase; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import com.github.openjson.JSONArray; +import com.github.openjson.JSONObject; + +/** + * Tests the upload of files to a resource by {@link FileUploadToResourceField}, and how the field + * takes what the browser reports of it. + */ +class FileUploadToResourceFieldTest extends WicketTestCase +{ + @TempDir + Path tempDir; + + private UploadPage page; + + @Test + void storesFilesOfTheSameClientFileNameApart() throws IOException + { + FolderUploadsFileManager fileManager = startPage(); + + submitUpload(createUploadFile("first", "same.txt", "first content"), + createUploadFile("second", "same.txt", "second content")); + + JSONArray filesInfo = new JSONArray(tester.getLastResponseAsString()); + assertEquals(2, filesInfo.length()); + JSONObject first = filesInfo.getJSONObject(0); + JSONObject second = filesInfo.getJSONObject(1); + assertEquals("same.txt", first.getString("clientFileName")); + assertEquals("same.txt", second.getString("clientFileName")); + assertNotEquals(first.getString("fileName"), second.getString("fileName")); + assertEquals("first content", readStoredFile(fileManager, first.getString("fileName"))); + assertEquals("second content", readStoredFile(fileManager, second.getString("fileName"))); + } + + @Test + void givesTheStoredFilesToOnUploadSuccess() throws IOException + { + startPage(); + submitUpload(createUploadFile("first", "same.txt", "first content"), + createUploadFile("second", "same.txt", "second content")); + + report("error", "false", "filesInfo", tester.getLastResponseAsString()); + + List<FileUploadToResourceField.UploadInfo> uploadInfos = page.field.uploadInfos; + assertEquals(2, uploadInfos.size()); + assertNotEquals(uploadInfos.get(0), uploadInfos.get(1)); + assertEquals("first content", read(uploadInfos.get(0).getFile())); + assertEquals("second content", read(uploadInfos.get(1).getFile())); + } + + @Test + void answersAFailureOfTheFileManagerWithTheKeyOfAFailedUpload() throws IOException + { + startPage(new FolderUploadsFileManager(new File(tempDir.resolve("uploads").toFile())) + { + @Override + public String store(FileUpload fileItem, String uploadFieldId) + { + throw new IllegalStateException("disk full"); + } + }); + + submitUpload(createUploadFile("first", "file.txt", "content")); + + JSONObject answer = new JSONObject(tester.getLastResponseAsString()); + assertTrue(answer.getBoolean("error")); + assertEquals("uploadFailed", answer.getString("errorMessage")); + } + + @Test + void callsOnUploadFailureForAFailedUpload() + { + startPage(); + + report("error", "true", "errorMessage", "uploadFailed"); + + assertEquals(List.of("The upload failed."), page.field.failures); + } + + @Test + void callsOnUploadFailureForAnErrorItHasNoTextFor() + { + startPage(); + + report("error", "true", "errorMessage", "Internal Server Error"); + + assertEquals(List.of("The upload failed."), page.field.failures); + } + + @Test + void callsOnUploadFailureForAnErrorWithoutKey() + { + startPage(); + + report("error", "true"); + + assertEquals(List.of("The upload failed."), page.field.failures); + } + + @Test + void callsOnUploadFailureWithTheTextOfABrokenLimit() + { + startPage(); + + report("error", "true", "errorMessage", "uploadTooManyFiles"); + + assertEquals(List.of("There should be no more than 3 files."), page.field.failures); + } + + @Test + void callsOnUploadCanceledForACanceledUpload() + { + startPage(); + + report("error", "true", "errorMessage", FileUploadToResourceField.UPLOAD_CANCELED); + + assertTrue(page.field.canceled); + assertTrue(page.field.failures.isEmpty()); + } + + private FolderUploadsFileManager startPage() + { + FolderUploadsFileManager fileManager = new FolderUploadsFileManager( + new File(tempDir.resolve("uploads").toFile())); + startPage(fileManager); + return fileManager; + } + + private void startPage(IUploadsFileManager fileManager) + { + page = new UploadPage(new TestFileUploadResourceReference(fileManager)); + tester.startPage(page); + } + + private File createUploadFile(String folder, String fileName, String content) + throws IOException + { + Path filePath = Files.createDirectories(tempDir.resolve(folder)).resolve(fileName); + Files.writeString(filePath, content, StandardCharsets.UTF_8); + return new File(filePath.toFile()); + } + + private void submitUpload(File... uploads) + { + tester.getRequest().setMethod("POST"); + for (File upload : uploads) + { + tester.getRequest().addFile(AbstractFileUploadResource.PARAM_NAME, upload, "text/plain"); + } + tester.executeUrl(page.field.buildUploadUrl()); + } + + /** + * Reports to the field how the upload ended, as its script does. + */ + private void report(String... parameters) + { + tester.getRequest().setMethod("POST"); + for (int i = 0; i < parameters.length; i += 2) + { + tester.getRequest().getPostParameters().setParameterValue(parameters[i], + parameters[i + 1]); + } + tester.executeBehavior(page.field.getBehaviors(AbstractDefaultAjaxBehavior.class).get(0)); + } + + private String readStoredFile(FolderUploadsFileManager fileManager, String fileName) + throws IOException + { + return read(fileManager.getFile(page.field.getMarkupId(), fileName)); + } + + private static String read(java.io.File file) throws IOException + { + return Files.readString(file.toPath(), StandardCharsets.UTF_8); + } + + static class UploadPage extends WebPage + { + private static final long serialVersionUID = 1L; + + final TestField field; + + UploadPage(TestFileUploadResourceReference resourceReference) + { + field = new TestField("upload", resourceReference); + field.setMaxSize(Bytes.kilobytes(1)); + field.setFileCountMax(3); + add(field); + } + } + + static class TestField extends FileUploadToResourceField + { + private static final long serialVersionUID = 1L; + + private final TestFileUploadResourceReference resourceReference; + + List<UploadInfo> uploadInfos; + + final List<String> failures = new ArrayList<>(); + + boolean canceled; + + TestField(String id, TestFileUploadResourceReference resourceReference) + { + super(id); + this.resourceReference = resourceReference; + } + + @Override + protected FileUploadResourceReference getFileUploadResourceReference() + { + return resourceReference; + } + + @Override + protected void onUploadSuccess(AjaxRequestTarget target, List<UploadInfo> fileInfos) + { + uploadInfos = fileInfos; + } + + @Override + protected void onUploadFailure(AjaxRequestTarget target, String errorInfo) + { + failures.add(errorInfo); + } + + @Override + protected void onUploadCanceled(AjaxRequestTarget target) + { + canceled = true; + } + + String buildUploadUrl() + { + return urlFor(resourceReference, new PageParameters()).toString() + '?' + + AbstractFileUploadResource.UPLOAD_ID + '=' + getMarkupId() + '&' + + AbstractFileUploadResource.MAX_SIZE + '=' + getMaxSize().bytes() + '&' + + AbstractFileUploadResource.FILE_COUNT_MAX + '=' + getFileCountMax() + '&' + + AbstractFileUploadResource.UPLOAD_TOKEN + '=' + + AbstractFileUploadResource.createUploadToken(getMarkupId(), getMaxSize(), + getFileMaxSize(), getFileCountMax()); + } + } + + static class TestFileUploadResourceReference extends FileUploadResourceReference + { + private static final long serialVersionUID = 1L; + + TestFileUploadResourceReference(IUploadsFileManager uploadFileManager) + { + super(uploadFileManager); + } + } +} diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManagerTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManagerTest.java index 4930aed2d7..92b4e4d526 100644 --- a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManagerTest.java +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManagerTest.java @@ -17,6 +17,7 @@ package org.apache.wicket.markup.html.form.upload.resource; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; @@ -54,6 +55,31 @@ class FolderUploadsFileManagerTest assertThrows(SecurityException.class, () -> manager.getFile("uploadField", "../../etc/passwd")); } + @Test + void storeRejectsTraversalInUploadFieldId() throws IOException + { + FolderUploadsFileManager manager = new FolderUploadsFileManager(new File(tempDir.toFile())); + FileUpload fileUpload = newFileUpload("safe.txt", "content"); + + assertThrows(SecurityException.class, () -> manager.store(fileUpload, "../escaped")); + } + + @Test + void storeKeepsFilesOfTheSameClientFileNameApart() throws IOException + { + FolderUploadsFileManager manager = new FolderUploadsFileManager(new File(tempDir.toFile())); + + String first = manager.store(newFileUpload("same.txt", "first"), "uploadField"); + String second = manager.store(newFileUpload("same.txt", "second"), "uploadField"); + + assertNotEquals(first, second); + assertEquals("first", + Files.readString(manager.getFile("uploadField", first).toPath(), StandardCharsets.UTF_8)); + assertEquals("second", + Files.readString(manager.getFile("uploadField", second).toPath(), StandardCharsets.UTF_8)); + } + + @SuppressWarnings("removal") @Test void saveRejectsTraversalInUploadFieldId() throws IOException { @@ -66,6 +92,7 @@ class FolderUploadsFileManagerTest assertThrows(SecurityException.class, () -> manager.save(fileUpload, "../escaped")); } + @SuppressWarnings("removal") @Test void saveRejectsTraversalInClientFileName() throws IOException { @@ -78,6 +105,7 @@ class FolderUploadsFileManagerTest assertThrows(SecurityException.class, () -> manager.save(fileUpload, "uploadField")); } + @SuppressWarnings("removal") @Test void saveSucceedsWithValidUploadFieldIdAndClientFileName() throws IOException { @@ -93,4 +121,14 @@ class FolderUploadsFileManagerTest assertTrue(Files.exists(savedFile)); assertEquals("content", Files.readString(savedFile, StandardCharsets.UTF_8)); } + + private static FileUpload newFileUpload(String clientFileName, String content) + throws IOException + { + FileUpload fileUpload = mock(FileUpload.class); + when(fileUpload.getClientFileName()).thenReturn(clientFileName); + when(fileUpload.getInputStream()) + .thenReturn(new ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8))); + return fileUpload; + } } diff --git a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/AbstractFileUploadResource.java b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/AbstractFileUploadResource.java index c20e7a5ce8..e71f2d5cfa 100644 --- a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/AbstractFileUploadResource.java +++ b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/AbstractFileUploadResource.java @@ -55,7 +55,8 @@ import jakarta.servlet.http.HttpServletResponse; * <p> * The main difference is that there some JQuery plugin is used at client side (and it supports multiple uploads + * some UI allowing to delete/preview files and so on). - * Here we are just using plain jQuery code at client side to upload a single file. + * Here {@link FileUploadToResourceField} sends all the files of its input in one request, with a + * plain {@code XMLHttpRequest}. */ public abstract class AbstractFileUploadResource extends AbstractResource { @@ -134,76 +135,28 @@ public abstract class AbstractFileUploadResource extends AbstractResource Map<String, List<FileItem>> files = multiPartRequest.getFiles(); List<FileItem> fileItems = files.get(PARAM_NAME); - if (fileItems != null) + if (fileItems == null) { - List<FileUpload> fileUploads = new ArrayList<>(); - for (FileItem fileItem : fileItems) - { - fileUploads.add(new FileUpload(fileItem)); - } - saveFiles(fileUploads, uploadId); - prepareResponse(resourceResponse, webRequest, fileUploads); + return createErrorResponse(resourceResponse, NO_FILE_SELECTED); } - else + List<FileUpload> fileUploads = new ArrayList<>(); + for (FileItem fileItem : fileItems) { - resourceResponse.setContentType("application/json"); - resourceResponse.setWriteCallback(new WriteCallback() - { - @Override - public void writeData(Attributes attributes) throws IOException - { - JSONObject json = new JSONObject(); - json.put("error", true); - json.put("errorMessage", NO_FILE_SELECTED); - String error = json.toString(); - attributes.getResponse().write(error); - } - }); + fileUploads.add(new FileUpload(fileItem)); } + List<String> fileNames = saveFiles(fileUploads, uploadId); + prepareResponse(resourceResponse, webRequest, fileUploads, fileNames); return resourceResponse; - } catch (FileUploadException fux) { - resourceResponse.setContentType("application/json"); - // even when this is an exceptional situation we want the request to be successful - // as we are just sending back to the client some JSON with error messages, - // which will in turn be sent to wicket component (who will handle the errors) - resourceResponse.setStatusCode(HttpServletResponse.SC_OK); - JSONObject json = new JSONObject(); - json.put("error", true); - String errorMessage = getFileUploadExceptionKey(fux); - json.put("errorMessage", errorMessage); - String error = json.toString(); - resourceResponse.setWriteCallback(new WriteCallback() - { - @Override - public void writeData(Attributes attributes) throws IOException - { - attributes.getResponse().write(error); - } - }); - return resourceResponse; + return createErrorResponse(resourceResponse, getFileUploadExceptionKey(fux)); } - catch (Exception fux) + catch (Exception e) { - resourceResponse.setContentType("application/json"); - JSONObject json = new JSONObject(); - json.put("error", true); - String errorMessage = LOG.isDebugEnabled() ? fux.getMessage() : Form.UPLOAD_FAILED_RESOURCE_KEY; - json.put("errorMessage", errorMessage); - String error = json.toString(); - resourceResponse.setWriteCallback(new WriteCallback() - { - @Override - public void writeData(Attributes attributes) throws IOException - { - attributes.getResponse().write(error); - } - }); + LOG.error("Upload to '{}' failed", uploadId, e); + return createErrorResponse(resourceResponse, Form.UPLOAD_FAILED_RESOURCE_KEY); } - - return resourceResponse; } private static String serializeUploadSettings(String uploadId, Bytes maxSize, Bytes fileMaxSize, @@ -247,6 +200,7 @@ public abstract class AbstractFileUploadResource extends AbstractResource private ResourceResponse createErrorResponse(ResourceResponse resourceResponse, String errorMessage) { resourceResponse.setContentType("application/json"); + // the error goes to the field in the JSON, which it reads from successful answers only resourceResponse.setStatusCode(HttpServletResponse.SC_OK); JSONObject json = new JSONObject(); json.put("error", true); @@ -293,11 +247,15 @@ public abstract class AbstractFileUploadResource extends AbstractResource * ServletWebRequest * @param fileItems * List<FileUpload> + * @param fileNames + * the names the files were stored under, in the order of {@code fileItems} */ - protected void prepareResponse(ResourceResponse resourceResponse, ServletWebRequest webRequest, List<FileUpload> fileItems) + protected void prepareResponse(ResourceResponse resourceResponse, ServletWebRequest webRequest, + List<FileUpload> fileItems, List<String> fileNames) { resourceResponse.setContentType("application/json"); - final String responseContent = generateJsonResponse(resourceResponse, webRequest, fileItems); + final String responseContent = generateJsonResponse(resourceResponse, webRequest, fileItems, + fileNames); resourceResponse.setWriteCallback(new WriteCallback() { @@ -314,13 +272,19 @@ public abstract class AbstractFileUploadResource extends AbstractResource * * @param fileItems * List<FileUpload> + * @param uploadId + * the unique ID of the upload field + * @return the names the files were stored under, in the order of {@code fileItems} + * @see IUploadsFileManager#store(FileUpload, String) */ - protected void saveFiles(List<FileUpload> fileItems, String uploadId) + protected List<String> saveFiles(List<FileUpload> fileItems, String uploadId) { + List<String> fileNames = new ArrayList<>(); for (FileUpload fileItem : fileItems) { - fileManager.save(fileItem, uploadId); + fileNames.add(fileManager.store(fileItem, uploadId)); } + return fileNames; } // Use getQueryParameters() instead of getRequestParameters() to avoid reading POST parameters @@ -389,9 +353,12 @@ public abstract class AbstractFileUploadResource extends AbstractResource * ServletWebRequest * @param files * List<FileUpload> + * @param fileNames + * the names the files were stored under, in the order of {@code files}; the browser + * sends them back for {@link FileUploadToResourceField} to find the files * @return The generated JSON */ protected abstract String generateJsonResponse(ResourceResponse resourceResponse, - ServletWebRequest webRequest, List<FileUpload> files); + ServletWebRequest webRequest, List<FileUpload> files, List<String> fileNames); } diff --git a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadResourceReference.java b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadResourceReference.java index b57644503c..1a4db65e90 100644 --- a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadResourceReference.java +++ b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadResourceReference.java @@ -83,17 +83,18 @@ public class FileUploadResourceReference extends ResourceReference { @Override - protected String generateJsonResponse(ResourceResponse resourceResponse, ServletWebRequest webRequest, List<FileUpload> files) + protected String generateJsonResponse(ResourceResponse resourceResponse, + ServletWebRequest webRequest, List<FileUpload> files, List<String> fileNames) { JSONArray json = new JSONArray(); - for (FileUpload fileItem : files) + for (int i = 0; i < files.size(); i++) { JSONObject fileJson = new JSONObject(); try { - generateFileInfo(fileJson, fileItem); + generateFileInfo(fileJson, files.get(i), fileNames.get(i)); json.put(fileJson); } catch (JSONException e) @@ -111,8 +112,21 @@ public class FileUploadResourceReference extends ResourceReference return uploadFileManager; } - protected void generateFileInfo(JSONObject fileJson, FileUpload fileItem) + /** + * Puts the information about an uploaded file the field receives into the JSON answering the + * upload. + * + * @param fileJson + * the JSON of the file + * @param fileItem + * the uploaded file + * @param fileName + * the name the file was stored under, see + * {@link IUploadsFileManager#store(FileUpload, String)} + */ + protected void generateFileInfo(JSONObject fileJson, FileUpload fileItem, String fileName) { + fileJson.put("fileName", fileName); fileJson.put("clientFileName", fileItem.getClientFileName()); fileJson.put("size", fileItem.getSize()); fileJson.put("contentType", fileItem.getContentType()); 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..97d5822508 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 @@ -25,6 +25,7 @@ import java.util.HashMap; 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; @@ -34,17 +35,17 @@ 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; import org.apache.wicket.model.Model; -import org.apache.wicket.request.Request; +import org.apache.wicket.request.IRequestParameters; import org.apache.wicket.request.cycle.RequestCycle; import org.apache.wicket.request.mapper.parameter.PageParameters; import org.apache.wicket.request.resource.JavaScriptResourceReference; import org.apache.wicket.resource.CoreLibrariesContributor; import org.apache.wicket.util.lang.Bytes; -import org.apache.wicket.util.string.Strings; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import com.github.openjson.JSONArray; @@ -65,19 +66,42 @@ public abstract class FileUploadToResourceField extends FileUploadField public static final class UploadInfo { private File file; + private final String fileName; private final String clientFileName; private final long size; private final String contentType; - public UploadInfo(String clientFileName, long size, 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 + * @param contentType + * the content type of the file + * @since 11.0.0 + */ + public UploadInfo(String fileName, String clientFileName, long size, String contentType) { + this.fileName = fileName; this.clientFileName = clientFileName; this.size = size; this.contentType = contentType; } + /** + * @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. + */ + @Deprecated(since = "11.0.0", forRemoval = true) + public UploadInfo(String clientFileName, long size, String contentType) + { + this(clientFileName, clientFileName, size, contentType); + } + public File getFile() { return file; @@ -88,6 +112,15 @@ public abstract class FileUploadToResourceField extends FileUploadField this.file = file; } + /** + * @return the name the {@link IUploadsFileManager} stored the file under + * @since 11.0.0 + */ + public String getFileName() + { + return fileName; + } + public String getClientFileName() { return clientFileName; @@ -109,7 +142,7 @@ public abstract class FileUploadToResourceField extends FileUploadField if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; UploadInfo fileInfo = (UploadInfo) o; - return Objects.equals(clientFileName, fileInfo.clientFileName); + return Objects.equals(fileName, fileInfo.fileName); } /** @@ -147,7 +180,7 @@ public abstract class FileUploadToResourceField extends FileUploadField @Override public int hashCode() { - return Objects.hash(clientFileName); + return Objects.hash(fileName); } public static List<UploadInfo> fromJson(String json) @@ -157,7 +190,10 @@ 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"))); + 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"))); } return infos; } @@ -167,6 +203,17 @@ public abstract class FileUploadToResourceField extends FileUploadField public static String UPLOAD_CANCELED = "upload.canceled"; + /** + * The resource key of the text of an upload that failed for a reason other than a limit. + * + * @since 11.0.0 + */ + public static final String UPLOAD_FAILED_RESOURCE_KEY = "uploadToResourceFailed"; + + private static final Set<String> LIMIT_RESOURCE_KEYS = Set.of( + Form.UPLOAD_TOO_LARGE_RESOURCE_KEY, Form.UPLOAD_SINGLE_FILE_TOO_LARGE_RESOURCE_KEY, + Form.UPLOAD_TOO_MANY_FILES_RESOURCE_KEY); + private static abstract class FileModel implements IModel<List<UploadInfo>> { @@ -180,7 +227,7 @@ public abstract class FileUploadToResourceField extends FileUploadField // at this point files were stored at the server side by resource // UploadFieldId acts as a discriminator at application level // so that uploaded files are isolated. - uploadInfo.setFile(fileManager().getFile(getUploadFieldId(), uploadInfo.clientFileName)); + uploadInfo.setFile(fileManager().getFile(getUploadFieldId(), uploadInfo.getFileName())); } return fileInfos; } @@ -251,38 +298,23 @@ public abstract class FileUploadToResourceField extends FileUploadField @Override protected void respond(AjaxRequestTarget target) { - Request request = RequestCycle.get().getRequest(); - boolean error = request.getRequestParameters().getParameterValue("error").toBoolean(true); + IRequestParameters parameters = RequestCycle.get().getRequest().getRequestParameters(); + boolean error = parameters.getParameterValue("error").toBoolean(true); if (!error) { - String filesIfo = request.getRequestParameters().getParameterValue("filesInfo").toString(); + String filesIfo = parameters.getParameterValue("filesInfo").toString(); fileUploadInfos = UploadInfo.fromJson(filesIfo); onUploadSuccess(target, getFileUploadInfos()); } else { - String errorMessage = request.getRequestParameters().getParameterValue("errorMessage").toString(null); - if (UPLOAD_CANCELED.equals(errorMessage)) + String errorKey = parameters.getParameterValue("errorMessage").toString(null); + if (UPLOAD_CANCELED.equals(errorKey)) { onUploadCanceled(target); } 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)); - } + String errorMessage = getUploadErrorMessage(errorKey); error(errorMessage); onUploadFailure(target, errorMessage); } @@ -292,6 +324,27 @@ public abstract class FileUploadToResourceField extends FileUploadField add(ajaxBehavior); } + /** + * @param errorKey + * the key of the error the browser reported, which the browser can make up + * @return the text of a limit or of no files selected, else that of + * {@link #UPLOAD_FAILED_RESOURCE_KEY} + */ + private String getUploadErrorMessage(String errorKey) + { + if (AbstractFileUploadResource.NO_FILE_SELECTED.equals(errorKey)) + { + return getString(AbstractFileUploadResource.NO_FILE_SELECTED); + } + final Map<String, Object> model = new HashMap<>(); + model.put("maxSize", getMaxSize()); + model.put("fileMaxSize", getFileMaxSize()); + model.put("fileCountMax", getFileCountMax()); + String key = errorKey != null && LIMIT_RESOURCE_KEYS.contains(errorKey) ? errorKey + : UPLOAD_FAILED_RESOURCE_KEY; + return getString(key, Model.ofMap(model)); + } + private List<UploadInfo> getFileUploadInfos() { // mind that this makes files to be added. @@ -469,7 +522,10 @@ public abstract class FileUploadToResourceField extends FileUploadField } /** - * Starts the upload via an AJAX request. + * Starts the upload via an AJAX request. Only one upload of the field runs at a time: while one + * runs, starting another one does nothing. Files beyond the limits of the field are not sent: + * {@link #onUploadFailure(AjaxRequestTarget, String)} is called with the text of the limit + * instead. * * @param target The {@link AjaxRequestTarget} */ @@ -496,13 +552,13 @@ public abstract class FileUploadToResourceField extends FileUploadField } /** - * See jQuery.ajax documentation. - * - * @return A JavaScript function to be executed on beforeSend the upload request. + * @return A JavaScript function called with the {@code XMLHttpRequest} of the upload before it + * is sent. If it returns {@code false}, the upload is not sent, and + * {@link #onUploadCanceled(AjaxRequestTarget)} is called. */ protected CharSequence getClientBeforeSendCallBack() { - return "function (xhr, settings) { return true; }"; + return "function (xhr) { return true; }"; } /** @@ -516,7 +572,7 @@ public abstract class FileUploadToResourceField extends FileUploadField /** * @return A JavaScript function to be executed on upload canceled. This is, besides the normal wicket - * AJAX request (see {@link #onUploadFailure(AjaxRequestTarget, String)}). + * AJAX request (see {@link #onUploadCanceled(AjaxRequestTarget)}). */ protected CharSequence getClientSideCancelCallBack() { @@ -524,9 +580,10 @@ public abstract class FileUploadToResourceField extends FileUploadField } /** - * @return A JavaScript function to be executed on upload canceled. This is, besides the normal wicket + * @return A JavaScript function to be executed on upload failed. This is, besides the normal wicket * AJAX request (see {@link #onUploadFailure(AjaxRequestTarget, String)}). It receives as parameter a JSON object - * like <code>{'error': true, errorMessage: 'xxx'}</code>. + * like <code>{'error': true, errorMessage: 'uploadTooLarge'}</code>, where {@code errorMessage} is the + * resource key of the error. */ protected CharSequence getClientSideUploadErrorCallBack() { diff --git a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceField.js b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceField.js index 5d79c59f0e..31a7580d61 100644 --- a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceField.js +++ b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceField.js @@ -43,65 +43,136 @@ this.uploadErrorCallBack = uploadErrorCallBack; }; + // the keys of the errors, as the field knows them + const UPLOAD_TOO_LARGE = 'uploadTooLarge'; + const UPLOAD_SINGLE_FILE_TOO_LARGE = 'uploadSingleFileTooLarge'; + const UPLOAD_TOO_MANY_FILES = 'uploadTooManyFiles'; + const UPLOAD_FAILED = 'uploadFailed'; + const UPLOAD_CANCELED = 'upload.canceled'; + + /** + * Uploads the files of the input, unless an upload of the field is running already, or the + * files break a limit of the field: the server is told of those as of an upload it refused, + * without the files being sent. + * + * @returns {boolean} whether the upload was sent + */ Wicket.FileUploadToResourceField.prototype.upload = function() { + if (this.xhr) { + Wicket.Log.log("An upload of field '" + this.inputName + "' is running: it has to end before another one starts."); + return false; + } // get a fresh reference to input this.input = document.getElementById(this.inputName); + const files = this.input.files; + const exceededLimit = this.exceededLimit(files); + if (exceededLimit) { + this.failed(exceededLimit); + return false; + } // we add the files to a FormData object. const formData = new FormData(); - const totalfiles = this.input.files.length; - for (let index = 0; index < totalfiles; index++) { - formData.append("WICKET-FILE-UPLOAD",this.input.files[index]); + for (let index = 0; index < files.length; index++) { + formData.append("WICKET-FILE-UPLOAD", files[index]); } const self = this; // we post the files to the resource (this.resourceUrl) with a plain XMLHttpRequest // and keep a reference to it in order to be able to cancel the upload const xhr = new XMLHttpRequest(); - this.xhr = xhr; xhr.open("POST", this.resourceUrl); xhr.onload = function () { - let res, ep; - if (xhr.status >= 200 && xhr.status < 300) { - try { - res = xhr.responseText ? JSON.parse(xhr.responseText) : {}; - } catch (parseError) { - // this error will only happen is generated JSON at server side is faulty - Wicket.Log.log(xhr.responseText); - return; - } - // do clean up on success - if (res.error) { - self.uploadErrorCallBack(res); - Wicket.Ajax.get({"u": self.ajaxCallBackUrl, "ep": res}); - } else { - self.clientSideSuccessCallBack(); - ep = {'error': false, 'filesInfo': JSON.stringify(res)}; - Wicket.Ajax.get({"u": self.ajaxCallBackUrl, "ep": ep}); - } + if (xhr.status < 200 || xhr.status >= 300) { + self.failed(UPLOAD_FAILED); + return; + } + let res; + try { + res = xhr.responseText ? JSON.parse(xhr.responseText) : {}; + } catch (parseError) { + Wicket.Log.error("The answer to the upload of field '" + self.inputName + "' is not JSON: " + xhr.responseText); + self.failed(UPLOAD_FAILED); + return; + } + if (res.error) { + self.failed(res.errorMessage); } else { - ep = {'error': true, "errorMessage": xhr.statusText}; - self.uploadErrorCallBack(ep); - Wicket.Ajax.get({"u": self.ajaxCallBackUrl, "ep": ep}); + delete self.xhr; + self.clientSideSuccessCallBack(); + self.report({'error': false, 'filesInfo': JSON.stringify(res)}); } }; xhr.onerror = function () { - const ep = {'error': true, "errorMessage": xhr.statusText}; - self.uploadErrorCallBack(ep); - Wicket.Ajax.get({"u": self.ajaxCallBackUrl, "ep": ep}); + self.failed(UPLOAD_FAILED); }; xhr.onabort = function () { // user aborted the upload. - const ep = {'error': true, 'errorMessage': 'upload.canceled'}; - Wicket.Ajax.get({"u": self.ajaxCallBackUrl, "ep": ep}); + delete self.xhr; + self.report({'error': true, 'errorMessage': UPLOAD_CANCELED}); }; - self.clientBeforeSendCallBack(xhr); + if (this.clientBeforeSendCallBack(xhr) === false) { + Wicket.Log.log("The upload of field '" + this.inputName + "' was not sent: the before-send callback returned false."); + this.report({'error': true, 'errorMessage': UPLOAD_CANCELED}); + return false; + } + this.xhr = xhr; xhr.send(formData); + return true; + }; + + /** + * @param files the files to upload + * @returns {string|null} the key of the first limit of the field the files break, if any + */ + Wicket.FileUploadToResourceField.prototype.exceededLimit = function (files) + { + const settings = this.settings; + let totalSize = 0; + let largestSize = 0; + for (let index = 0; index < files.length; index++) { + totalSize += files[index].size; + largestSize = Math.max(largestSize, files[index].size); + } + if (totalSize > settings.maxSize) { + return UPLOAD_TOO_LARGE; + } + if (settings.fileCountMax >= 0 && files.length > settings.fileCountMax) { + return UPLOAD_TOO_MANY_FILES; + } + if (settings.fileMaxSize != null && largestSize > settings.fileMaxSize) { + return UPLOAD_SINGLE_FILE_TOO_LARGE; + } + return null; + }; + + /** + * Ends an upload that failed, and tells the page and the server why. + * + * @param errorMessage the key of the error + */ + Wicket.FileUploadToResourceField.prototype.failed = function (errorMessage) + { + delete this.xhr; + const ep = {'error': true, 'errorMessage': errorMessage}; + this.uploadErrorCallBack(ep); + this.report(ep); + }; + + /** + * Tells the server how an upload ended. A POST carries the information about the files in its + * body, which would make the URL of a GET too long for the server with many files. + * + * @param ep the parameters of the request + */ + Wicket.FileUploadToResourceField.prototype.report = function (ep) + { + Wicket.Ajax.post({"u": this.ajaxCallBackUrl, "ep": ep}); }; // cancel the upload @@ -109,11 +180,11 @@ // we have a reference to the request we can cancel it. if (this.xhr) { this.xhr.abort(); + delete (this.xhr); this.clientSideCancelCallBack(); Wicket.Log.log("The upload associated with field '" + this.inputName + "' has been canceled!"); - delete (this.xhr); } else { - Wicket.Log.log("Too late to cancel upload for field '" + this.inputName + "': the upload has already finished."); + Wicket.Log.log("No upload of field '" + this.inputName + "' to cancel: it has already ended, or never started."); } }; })(); diff --git a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceField.properties b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceField.properties index 5c115a887c..2073fe09c5 100644 --- a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceField.properties +++ b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceField.properties @@ -12,4 +12,5 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -wicket.no.files.selected = Nothing to upload: no files were selected. \ No newline at end of file +wicket.no.files.selected = Nothing to upload: no files were selected. +uploadToResourceFailed = The upload failed. diff --git a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManager.java b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManager.java index f877acc9a6..90414da153 100644 --- a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManager.java +++ b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManager.java @@ -22,6 +22,7 @@ import java.io.InputStream; import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.Path; +import java.util.UUID; import org.apache.wicket.WicketRuntimeException; import org.apache.wicket.markup.html.form.upload.FileUpload; import org.apache.wicket.util.file.File; @@ -31,7 +32,8 @@ import org.apache.wicket.util.lang.Args; /** * Implementation of {@link IUploadsFileManager} that stores files in sub-folders of a given folder. * Sub-folders are named using the input field ID (and those are unique per application). Thus, there - * is no possibility that uploads get mixed up. + * is no possibility that uploads get mixed up. In its sub-folder, each file gets a random name of its + * own, so files of the same client file name do not replace each other. */ public class FolderUploadsFileManager implements IUploadsFileManager { @@ -84,12 +86,12 @@ public class FolderUploadsFileManager implements IUploadsFileManager } /** - * Validates {@code uploadFieldId} and {@code clientFileName} against the base folder to + * Validates {@code uploadFieldId} and {@code fileName} against the base folder to * prevent path traversal, and returns the fully resolved, canonical target file. * * @throws SecurityException if either component would escape the base folder */ - private java.io.File resolveTargetFile(String uploadFieldId, String clientFileName) + private java.io.File resolveTargetFile(String uploadFieldId, String fileName) throws IOException { Path baseFolderPath = getFolder().toPath().toRealPath(); @@ -98,21 +100,40 @@ public class FolderUploadsFileManager implements IUploadsFileManager { throw new SecurityException("Path traversal detected in uploadFieldId"); } - java.io.File target = new File(uploadFieldFolder, clientFileName).getCanonicalFile(); + java.io.File target = new File(uploadFieldFolder, fileName).getCanonicalFile(); Path uploadFieldFolderPath = getPathForComparison(uploadFieldFolder); if (!target.toPath().startsWith(uploadFieldFolderPath)) { - throw new SecurityException("Path traversal detected in client filename"); + throw new SecurityException("Path traversal detected in file name"); } return target; } + @Override + public String store(FileUpload fileItem, String uploadFieldId) + { + String fileName = UUID.randomUUID().toString(); + write(fileItem, uploadFieldId, fileName); + return fileName; + } + + /** + * @deprecated files saved under their client file name replace each other when they share it. + * Use {@link #store(FileUpload, String)} instead. + */ + @Deprecated(since = "11.0.0", forRemoval = true) + @SuppressWarnings("removal") @Override public void save(FileUpload fileItem, String uploadFieldId) + { + write(fileItem, uploadFieldId, fileItem.getClientFileName()); + } + + private void write(FileUpload fileItem, String uploadFieldId, String fileName) { try { - java.io.File target = resolveTargetFile(uploadFieldId, fileItem.getClientFileName()); + java.io.File target = resolveTargetFile(uploadFieldId, fileName); Files.createDirectories(target.toPath().getParent()); try (InputStream in = fileItem.getInputStream(); FileOutputStream out = new FileOutputStream(target)) @@ -127,11 +148,11 @@ public class FolderUploadsFileManager implements IUploadsFileManager } @Override - public File getFile(String uploadFieldId, String clientFileName) + public File getFile(String uploadFieldId, String fileName) { try { - return new File(resolveTargetFile(uploadFieldId, clientFileName)); + return new File(resolveTargetFile(uploadFieldId, fileName)); } catch (IOException e) { diff --git a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/IUploadsFileManager.java b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/IUploadsFileManager.java index 78a1208503..aeca43c8b6 100644 --- a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/IUploadsFileManager.java +++ b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/IUploadsFileManager.java @@ -25,28 +25,66 @@ import org.apache.wicket.util.file.File; * how to store file. Same identifier is used to retrieve the file, once uploaded, from component in page. * Mind that uploader resource is a singleton => identifier needs to be unique among different sessions * (and pages in a session). + * <p> + * An implementation implements {@link #store(FileUpload, String)} and + * {@link #getFile(String, String)}. */ public interface IUploadsFileManager { /** - * Saves an uploaded files into some persistent storage (e,g, disk). + * Saves an uploaded file into some persistent storage (e.g. disk), under a name of its own: + * files of the same client file name, sent in one upload or in several, must not replace each + * other. + * <p> + * The name goes to the browser, which sends it back for {@link #getFile(String, String)} to + * find the file. It must not reveal anything the user is not supposed to know. + * <p> + * The default implementation saves the file with {@link #save(FileUpload, String)} and returns + * its client file name, for implementations written before this method existed. * * @param fileItem * The {@link FileUpload} * @param uploadFieldId * The unique ID of the upload field. + * @return the name the file was saved under, unique among the files of the upload field + * @since 11.0.0 */ - void save(FileUpload fileItem, String uploadFieldId); + default String store(FileUpload fileItem, String uploadFieldId) + { + save(fileItem, uploadFieldId); + return fileItem.getClientFileName(); + } /** - * Retrieves the file based on an uploadFieldId and clientFileName. + * Saves an uploaded file into some persistent storage (e.g. disk), under its client file name. + * <p> + * The default implementation delegates to {@link #store(FileUpload, String)}. + * + * @param fileItem + * The {@link FileUpload} + * @param uploadFieldId + * The unique ID of the upload field. + * @deprecated files saved under their client file name replace each other when they share it. + * Implement {@link #store(FileUpload, String)} instead. + */ + @Deprecated(since = "11.0.0", forRemoval = true) + default void save(FileUpload fileItem, String uploadFieldId) + { + store(fileItem, uploadFieldId); + } + + /** + * Retrieves the file saved under a name by {@link #store(FileUpload, String)}. + * <p> + * The name comes back from the browser, which can change it: an implementation must never + * return a file that does not belong to the upload field. * * @param uploadFieldId * The unique ID of the upload field. - * @param clientFileName - * The client file name + * @param fileName + * The name {@link #store(FileUpload, String)} returned for the file * @return File the file */ - File getFile(String uploadFieldId, String clientFileName); + File getFile(String uploadFieldId, String fileName); } diff --git a/wicket-core/src/test/js/all.html b/wicket-core/src/test/js/all.html index ae6c778aad..83f4085202 100644 --- a/wicket-core/src/test/js/all.html +++ b/wicket-core/src/test/js/all.html @@ -156,6 +156,11 @@ <div id="usedAsContextWicket5025"></div> + <!-- + FileUploadToResourceField + --> + <input type="file" id="uploadToResourceInput" multiple="multiple"/> + <div id="ajaxIndicator"></div> </div> @@ -179,6 +184,7 @@ document.write("<scr"+"ipt src='../../main/java/org/apache/wicket/ajax/res/js/wicket-ajax.js'></scr"+"ipt>"); } </script> + <script type="text/javascript" src="../../main/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceField.js"></script> <script type="text/javascript" src="qunit/qunit.js"></script> <script type="text/javascript" src="qunit/blanket.min.js"></script> @@ -194,5 +200,6 @@ <script type="text/javascript" src="head.js"></script> <script type="text/javascript" src="ajax.js"></script> <script type="text/javascript" src="timer.js"></script> + <script type="text/javascript" src="upload-to-resource.js"></script> </body> </html> diff --git a/wicket-core/src/test/js/upload-to-resource.js b/wicket-core/src/test/js/upload-to-resource.js new file mode 100644 index 0000000000..d43314ea20 --- /dev/null +++ b/wicket-core/src/test/js/upload-to-resource.js @@ -0,0 +1,221 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/*global QUnit: true, DataTransfer: true */ + +Wicket.Event.add(window, 'domready', function() { + "use strict"; + + const { module, test } = QUnit; + + const INPUT_ID = 'uploadToResourceInput'; + + // stands in for the XMLHttpRequest the field uploads with + function FakeXhr() { + FakeXhr.instances.push(this); + this.status = 0; + this.responseText = ''; + } + FakeXhr.prototype.open = function (method, url) { + this.method = method; + this.url = url; + }; + FakeXhr.prototype.send = function (body) { + this.body = body; + }; + FakeXhr.prototype.abort = function () { + this.aborted = true; + this.onabort(); + }; + FakeXhr.prototype.respond = function (status, responseText) { + this.status = status; + this.responseText = responseText; + this.onload(); + }; + + let originalXhr, originalPost, reports, calls; + + function chooseFiles(files) { + const dataTransfer = new DataTransfer(); + files.forEach(file => dataTransfer.items.add(file)); + document.getElementById(INPUT_ID).files = dataTransfer.files; + } + + function newFile(name, size) { + return new File([new Uint8Array(size)], name, {type: 'text/plain'}); + } + + function newField(settings, beforeSend) { + return new Wicket.FileUploadToResourceField(Object.assign({ + inputName: INPUT_ID, + resourceUrl: 'resource', + ajaxCallBackUrl: 'callback', + maxSize: 100, + fileCountMax: -1, + uploadToken: 'token' + }, settings), + beforeSend || function () { return true; }, + function () { calls.push('success'); }, + function () { calls.push('cancel'); }, + function (res) { calls.push('error:' + res.errorMessage); }); + } + + module('Wicket.FileUploadToResourceField', { + beforeEach: function () { + originalXhr = window.XMLHttpRequest; + originalPost = Wicket.Ajax.post; + FakeXhr.instances = []; + window.XMLHttpRequest = FakeXhr; + reports = []; + calls = []; + Wicket.Ajax.post = function (attrs) { + reports.push(attrs); + }; + }, + afterEach: function () { + window.XMLHttpRequest = originalXhr; + Wicket.Ajax.post = originalPost; + } + }); + + test('sends all the files of the input in one request', assert => { + chooseFiles([newFile('a.txt', 3), newFile('b.txt', 4)]); + + assert.true(newField().upload()); + + assert.equal(FakeXhr.instances.length, 1); + const xhr = FakeXhr.instances[0]; + assert.equal(xhr.method, 'POST'); + assert.equal(xhr.body.getAll('WICKET-FILE-UPLOAD').length, 2); + }); + + test('reports the files received by POST', assert => { + chooseFiles([newFile('a.txt', 3)]); + newField().upload(); + + const filesInfo = '[{"fileName":"f1","clientFileName":"a.txt","size":3,"contentType":"text/plain"}]'; + FakeXhr.instances[0].respond(200, filesInfo); + + assert.deepEqual(calls, ['success']); + assert.equal(reports.length, 1); + assert.equal(reports[0].u, 'callback'); + assert.deepEqual(reports[0].ep, {'error': false, 'filesInfo': JSON.stringify(JSON.parse(filesInfo))}); + }); + + test('reports the error the resource answered', assert => { + chooseFiles([newFile('a.txt', 3)]); + newField().upload(); + + FakeXhr.instances[0].respond(200, '{"error":true,"errorMessage":"uploadTooManyFiles"}'); + + assert.deepEqual(calls, ['error:uploadTooManyFiles']); + assert.deepEqual(reports[0].ep, {'error': true, 'errorMessage': 'uploadTooManyFiles'}); + }); + + test('reports an HTTP error as a failed upload', assert => { + chooseFiles([newFile('a.txt', 3)]); + newField().upload(); + + FakeXhr.instances[0].respond(500, 'Internal Server Error'); + + assert.deepEqual(calls, ['error:uploadFailed']); + assert.deepEqual(reports[0].ep, {'error': true, 'errorMessage': 'uploadFailed'}); + }); + + test('reports a network failure as a failed upload', assert => { + chooseFiles([newFile('a.txt', 3)]); + newField().upload(); + + FakeXhr.instances[0].onerror(); + + assert.deepEqual(reports[0].ep, {'error': true, 'errorMessage': 'uploadFailed'}); + }); + + test('reports an answer that is not JSON as a failed upload', assert => { + chooseFiles([newFile('a.txt', 3)]); + newField().upload(); + + FakeXhr.instances[0].respond(200, '<html>not JSON</html>'); + + assert.deepEqual(calls, ['error:uploadFailed']); + assert.deepEqual(reports[0].ep, {'error': true, 'errorMessage': 'uploadFailed'}); + }); + + test('does not send files beyond the limits of the field', assert => { + const field = newField({maxSize: 10, fileMaxSize: 4, fileCountMax: 2}); + + chooseFiles([newFile('a.txt', 6), newFile('b.txt', 6)]); + assert.false(field.upload(), 'beyond maxSize'); + chooseFiles([newFile('a.txt', 1), newFile('b.txt', 1), newFile('c.txt', 1)]); + assert.false(field.upload(), 'beyond fileCountMax'); + chooseFiles([newFile('a.txt', 5)]); + assert.false(field.upload(), 'beyond fileMaxSize'); + + assert.equal(FakeXhr.instances.length, 0); + assert.deepEqual(reports.map(report => report.ep.errorMessage), + ['uploadTooLarge', 'uploadTooManyFiles', 'uploadSingleFileTooLarge']); + }); + + test('refuses a second upload while one is running', assert => { + chooseFiles([newFile('a.txt', 3)]); + const field = newField(); + + assert.true(field.upload()); + assert.false(field.upload(), 'refused while the first one runs'); + assert.equal(FakeXhr.instances.length, 1); + + FakeXhr.instances[0].respond(200, '[]'); + assert.true(field.upload(), 'sent once the first one ended'); + assert.equal(FakeXhr.instances.length, 2); + }); + + test('cancels the running upload', assert => { + chooseFiles([newFile('a.txt', 3)]); + const field = newField(); + field.upload(); + + field.cancel(); + + assert.true(FakeXhr.instances[0].aborted); + assert.deepEqual(calls, ['cancel']); + assert.deepEqual(reports[0].ep, {'error': true, 'errorMessage': 'upload.canceled'}); + }); + + test('does not cancel an upload that has ended', assert => { + chooseFiles([newFile('a.txt', 3)]); + const field = newField(); + field.upload(); + FakeXhr.instances[0].respond(200, '[]'); + + field.cancel(); + + assert.notOk(FakeXhr.instances[0].aborted); + assert.deepEqual(calls, ['success']); + }); + + test('does not send the upload if the before-send callback returns false', assert => { + chooseFiles([newFile('a.txt', 3)]); + const field = newField({}, function () { return false; }); + + assert.false(field.upload()); + + assert.notOk(FakeXhr.instances[0].body, 'not sent'); + assert.deepEqual(reports[0].ep, {'error': true, 'errorMessage': 'upload.canceled'}); + field.upload(); + assert.equal(FakeXhr.instances.length, 2, 'the field is not left busy'); + }); +}); diff --git a/wicket-migration/src/main/resources/META-INF/rewrite/wicket.yml b/wicket-migration/src/main/resources/META-INF/rewrite/wicket.yml index 7bd0e14305..e81195b61a 100644 --- a/wicket-migration/src/main/resources/META-INF/rewrite/wicket.yml +++ b/wicket-migration/src/main/resources/META-INF/rewrite/wicket.yml @@ -89,6 +89,16 @@ recipeList: # handed the object the model was holding before that object is discarded; the default onDetach() # delegates to it, so an override of onDetach() that does not call super suppresses it. These # require manual migration. +# +# org.apache.wicket.markup.html.form.upload.resource.IUploadsFileManager#save() is deprecated in +# favour of store(), which stores the file under a name of its own and returns it; getFile() then +# receives that name. The name travels through the browser, so getFile() must not resolve it to a +# file of another upload field. FolderUploadsFileManager stores files under random names, so code +# finding them by client file name uses UploadInfo#getFile() or #getFileName() instead. Subclasses +# of AbstractFileUploadResource receive the stored names in saveFiles(), prepareResponse() and +# generateJsonResponse(), subclasses of FileUploadResourceReference in generateFileInfo(). A +# failed upload reads "The upload failed." (resource key uploadToResourceFailed) instead of +# ending on the error page. These require manual migration. type: specs.openrewrite.org/v1beta/recipe name: org.apache.wicket.MigrateToWicket11 displayName: Migrate to Wicket 11.x
