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 new file mode 100644 index 00000000000..4930aed2d71 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManagerTest.java @@ -0,0 +1,96 @@ +/* + * 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.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; + +import org.apache.wicket.markup.html.form.upload.FileUpload; +import org.apache.wicket.util.file.File; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +class FolderUploadsFileManagerTest +{ + @TempDir + Path tempDir; + + @Test + void getFileRejectsTraversalInUploadFieldId() + { + FolderUploadsFileManager manager = new FolderUploadsFileManager(new File(tempDir.toFile())); + + assertThrows(SecurityException.class, () -> manager.getFile("../escaped", "safe.txt")); + } + + @Test + void getFileRejectsTraversalInClientFileName() + { + FolderUploadsFileManager manager = new FolderUploadsFileManager(new File(tempDir.toFile())); + + assertThrows(SecurityException.class, () -> manager.getFile("uploadField", "../../etc/passwd")); + } + + @Test + void saveRejectsTraversalInUploadFieldId() throws IOException + { + FolderUploadsFileManager manager = new FolderUploadsFileManager(new File(tempDir.toFile())); + FileUpload fileUpload = mock(FileUpload.class); + when(fileUpload.getClientFileName()).thenReturn("safe.txt"); + when(fileUpload.getInputStream()) + .thenReturn(new ByteArrayInputStream("content".getBytes(StandardCharsets.UTF_8))); + + assertThrows(SecurityException.class, () -> manager.save(fileUpload, "../escaped")); + } + + @Test + void saveRejectsTraversalInClientFileName() throws IOException + { + FolderUploadsFileManager manager = new FolderUploadsFileManager(new File(tempDir.toFile())); + FileUpload fileUpload = mock(FileUpload.class); + when(fileUpload.getClientFileName()).thenReturn("../../etc/passwd"); + when(fileUpload.getInputStream()) + .thenReturn(new ByteArrayInputStream("content".getBytes(StandardCharsets.UTF_8))); + + assertThrows(SecurityException.class, () -> manager.save(fileUpload, "uploadField")); + } + + @Test + void saveSucceedsWithValidUploadFieldIdAndClientFileName() throws IOException + { + FolderUploadsFileManager manager = new FolderUploadsFileManager(new File(tempDir.toFile())); + FileUpload fileUpload = mock(FileUpload.class); + when(fileUpload.getClientFileName()).thenReturn("safe.txt"); + when(fileUpload.getInputStream()) + .thenReturn(new ByteArrayInputStream("content".getBytes(StandardCharsets.UTF_8))); + + manager.save(fileUpload, "uploadField"); + + Path savedFile = tempDir.resolve("uploadField").resolve("safe.txt"); + assertTrue(Files.exists(savedFile)); + assertEquals("content", Files.readString(savedFile, StandardCharsets.UTF_8)); + } +} 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 0146b820996..f877acc9a62 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 @@ -18,7 +18,10 @@ import java.io.FileOutputStream; import java.io.IOException; +import java.io.InputStream; import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.nio.file.Path; import org.apache.wicket.WicketRuntimeException; import org.apache.wicket.markup.html.form.upload.FileUpload; import org.apache.wicket.util.file.File; @@ -60,14 +63,62 @@ public File getFolder() { return folder; } + /** + * Returns the canonical path for a directory for use in path traversal checks. + * Uses {@code toRealPath()} when the directory exists; falls back to + * {@code toAbsolutePath().normalize()} only when the directory does not yet exist + * (e.g. an upload sub-folder that will be created during {@link #save}). + * Other {@link IOException} subtypes (e.g. permission errors) are intentionally + * re-thrown so they are not silently swallowed. + */ + private static Path getPathForComparison(java.io.File dir) throws IOException + { + try + { + return dir.toPath().toRealPath(); + } + catch (NoSuchFileException e) + { + return dir.toPath().toAbsolutePath().normalize(); + } + } + + /** + * Validates {@code uploadFieldId} and {@code clientFileName} 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) + throws IOException + { + Path baseFolderPath = getFolder().toPath().toRealPath(); + java.io.File uploadFieldFolder = new File(getFolder(), uploadFieldId).getCanonicalFile(); + if (!uploadFieldFolder.toPath().startsWith(baseFolderPath)) + { + throw new SecurityException("Path traversal detected in uploadFieldId"); + } + java.io.File target = new File(uploadFieldFolder, clientFileName).getCanonicalFile(); + Path uploadFieldFolderPath = getPathForComparison(uploadFieldFolder); + if (!target.toPath().startsWith(uploadFieldFolderPath)) + { + throw new SecurityException("Path traversal detected in client filename"); + } + return target; + } + @Override public void save(FileUpload fileItem, String uploadFieldId) { - File uploadFieldFolder = new File(getFolder(), uploadFieldId); - uploadFieldFolder.mkdirs(); try { - IOUtils.copy(fileItem.getInputStream(), new FileOutputStream(new File(uploadFieldFolder, fileItem.getClientFileName()))); + java.io.File target = resolveTargetFile(uploadFieldId, fileItem.getClientFileName()); + Files.createDirectories(target.toPath().getParent()); + try (InputStream in = fileItem.getInputStream(); + FileOutputStream out = new FileOutputStream(target)) + { + IOUtils.copy(in, out); + } } catch (IOException e) { @@ -78,6 +129,13 @@ public void save(FileUpload fileItem, String uploadFieldId) @Override public File getFile(String uploadFieldId, String clientFileName) { - return new File(new File(getFolder(), uploadFieldId), clientFileName); + try + { + return new File(resolveTargetFile(uploadFieldId, clientFileName)); + } + catch (IOException e) + { + throw new WicketRuntimeException(e); + } } }