From 7eb7b207f583fb231708c26fbe2df9f912eff74e Mon Sep 17 00:00:00 2001 From: hayageek Date: Mon, 23 Mar 2026 23:17:46 +0530 Subject: [PATCH 1/2] Security Vulnerabilty - Path Traversal Fix --- .../resource/FolderUploadsFileManager.java | 56 +++++++++++++++++-- 1 file changed, 52 insertions(+), 4 deletions(-) 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..4f2b481f002 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 @@ -19,6 +19,7 @@ import java.io.FileOutputStream; import java.io.IOException; import java.nio.file.Files; +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 +61,42 @@ public File getFolder() { return folder; } + /** + * Returns the path for a directory for use in path traversal checks. Uses {@code toRealPath()} + * when the directory exists; otherwise falls back to {@code toAbsolutePath().normalize()} to + * support validation when the directory has not yet been created. + */ + private static Path getPathForComparison(java.io.File dir) throws IOException + { + try + { + return dir.toPath().toRealPath(); + } + catch (IOException e) + { + return dir.toPath().toAbsolutePath().normalize(); + } + } + @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()))); + 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"); + } + uploadFieldFolder.mkdirs(); + java.io.File target = new File(uploadFieldFolder, fileItem.getClientFileName()).getCanonicalFile(); + Path uploadFieldFolderPath = getPathForComparison(uploadFieldFolder); + if (!target.toPath().startsWith(uploadFieldFolderPath)) + { + throw new SecurityException("Path traversal detected in client filename"); + } + IOUtils.copy(fileItem.getInputStream(), new FileOutputStream(target)); } catch (IOException e) { @@ -78,6 +107,25 @@ public void save(FileUpload fileItem, String uploadFieldId) @Override public File getFile(String uploadFieldId, String clientFileName) { - return new File(new File(getFolder(), uploadFieldId), clientFileName); + try + { + 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 new File(target); + } + catch (IOException e) + { + throw new WicketRuntimeException(e); + } } } From e6632e97499c583d5843909f57d839870ed217aa Mon Sep 17 00:00:00 2001 From: hayageek Date: Wed, 22 Apr 2026 16:22:48 +0530 Subject: [PATCH 2/2] Code review comments implemented --- .../FolderUploadsFileManagerTest.java | 96 +++++++++++++++++++ .../resource/FolderUploadsFileManager.java | 68 +++++++------ 2 files changed, 135 insertions(+), 29 deletions(-) create mode 100644 wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManagerTest.java 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 4f2b481f002..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,9 @@ 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; @@ -62,9 +64,12 @@ public File getFolder() { } /** - * Returns the path for a directory for use in path traversal checks. Uses {@code toRealPath()} - * when the directory exists; otherwise falls back to {@code toAbsolutePath().normalize()} to - * support validation when the directory has not yet been created. + * 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 { @@ -72,31 +77,48 @@ private static Path getPathForComparison(java.io.File dir) throws IOException { return dir.toPath().toRealPath(); } - catch (IOException e) + 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) { try { - 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"); - } - uploadFieldFolder.mkdirs(); - java.io.File target = new File(uploadFieldFolder, fileItem.getClientFileName()).getCanonicalFile(); - Path uploadFieldFolderPath = getPathForComparison(uploadFieldFolder); - if (!target.toPath().startsWith(uploadFieldFolderPath)) + java.io.File target = resolveTargetFile(uploadFieldId, fileItem.getClientFileName()); + Files.createDirectories(target.toPath().getParent()); + try (InputStream in = fileItem.getInputStream(); + FileOutputStream out = new FileOutputStream(target)) { - throw new SecurityException("Path traversal detected in client filename"); + IOUtils.copy(in, out); } - IOUtils.copy(fileItem.getInputStream(), new FileOutputStream(target)); } catch (IOException e) { @@ -109,19 +131,7 @@ public File getFile(String uploadFieldId, String clientFileName) { try { - 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 new File(target); + return new File(resolveTargetFile(uploadFieldId, clientFileName)); } catch (IOException e) {