You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by gg...@apache.org on 2023/06/10 13:22:23 UTC
[commons-fileupload] branch master updated: Reuse Java's InvalidPathException instead of the custom InvalidFileNameException
This is an automated email from the ASF dual-hosted git repository.
ggregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-fileupload.git
The following commit(s) were added to refs/heads/master by this push:
new 63e243f Reuse Java's InvalidPathException instead of the custom InvalidFileNameException
63e243f is described below
commit 63e243fe8ded60a5d40659db8226a9f5ea43fd88
Author: Gary Gregory <ga...@gmail.com>
AuthorDate: Sat Jun 10 09:22:19 2023 -0400
Reuse Java's InvalidPathException instead of the custom
InvalidFileNameException
---
.../org/apache/commons/fileupload2/FileItem.java | 3 +-
.../commons/fileupload2/FileItemStreamImpl.java | 11 +++--
.../fileupload2/InvalidFileNameException.java | 56 ---------------------
.../commons/fileupload2/disk/DiskFileItem.java | 57 ++++++++++++----------
.../commons/fileupload2/AbstractStreamingTest.java | 17 ++++---
.../disk/DiskFileItemSerializeTest.java | 4 +-
...treamingTest.java => JakartaStreamingTest.java} | 2 +-
src/changes/changes.xml | 1 +
8 files changed, 53 insertions(+), 98 deletions(-)
diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/FileItem.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/FileItem.java
index a264dcb..2fb7f26 100644
--- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/FileItem.java
+++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/FileItem.java
@@ -21,6 +21,7 @@ import java.io.InputStream;
import java.io.OutputStream;
import java.io.UncheckedIOException;
import java.io.UnsupportedEncodingException;
+import java.nio.file.InvalidPathException;
import java.nio.file.Path;
/**
@@ -85,7 +86,7 @@ public interface FileItem extends FileItemHeadersSupport {
* name, without path information. However, some clients, such as the Opera browser, do include path information.
*
* @return The original file name in the client's file system.
- * @throws InvalidFileNameException The file name contains a NUL character, which might be an indicator of a security attack. If you intend to use the file
+ * @throws InvalidPathException The file name contains a NUL character, which might be an indicator of a security attack. If you intend to use the file
* name anyways, catch the exception and use InvalidFileNameException#getName().
*/
String getName();
diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/FileItemStreamImpl.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/FileItemStreamImpl.java
index 4ef7e47..4b7096c 100644
--- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/FileItemStreamImpl.java
+++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/FileItemStreamImpl.java
@@ -18,6 +18,7 @@ package org.apache.commons.fileupload2;
import java.io.IOException;
import java.io.InputStream;
+import java.nio.file.InvalidPathException;
import org.apache.commons.fileupload2.MultipartStream.ItemInputStream;
import org.apache.commons.fileupload2.disk.DiskFileItem;
@@ -122,7 +123,7 @@ class FileItemStreamImpl implements FileItemStream {
}
/**
- * Gets the items content type, or null.
+ * Gets the content type, or null.
*
* @return Content type, if known, or null.
*/
@@ -142,7 +143,7 @@ class FileItemStreamImpl implements FileItemStream {
}
/**
- * Gets the file item headers.
+ * Gets the headers.
*
* @return The items header object
*/
@@ -152,11 +153,11 @@ class FileItemStreamImpl implements FileItemStream {
}
/**
- * Gets the items file name.
+ * Gets the file name.
*
* @return File name, if known, or null.
- * @throws InvalidFileNameException The file name contains a NUL character, which might be an indicator of a security attack. If you intend to use the file
- * name anyways, catch the exception and use InvalidFileNameException#getName().
+ * @throws InvalidPathException The file name is invalid, for example it contains a NUL character, which might be an indicator of a security attack. If you
+ * intend to use the file name anyways, catch the exception and use InvalidPathException#getInput().
*/
@Override
public String getName() {
diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/InvalidFileNameException.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/InvalidFileNameException.java
deleted file mode 100644
index 064238c..0000000
--- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/InvalidFileNameException.java
+++ /dev/null
@@ -1,56 +0,0 @@
-/*
- * 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.commons.fileupload2;
-
-/**
- * Signals an invalid file name. A file name is invalid, if it contains a NUL character. Attackers might use this to circumvent security checks: For example, a
- * malicious user might upload a file with the name "foo.exe\0.png". This file name might pass security checks (i.e. checks for the extension ".png"), while,
- * depending on the underlying C library, it might create a file named "foo.exe", as the NUL character is the string terminator in C.
- */
-public class InvalidFileNameException extends RuntimeException {
-
- /**
- * Serial version UID, being used, if the exception is serialized.
- */
- private static final long serialVersionUID = 2;
-
- /**
- * The file name causing the exception.
- */
- private final String name;
-
- /**
- * Constructs a new instance.
- *
- * @param name The file name causing the exception.
- * @param message A human readable error message.
- */
- public InvalidFileNameException(final String name, final String message) {
- super(message);
- this.name = name;
- }
-
- /**
- * Gets the invalid file name.
- *
- * @return the invalid file name.
- */
- public String getName() {
- return name;
- }
-
-}
diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/disk/DiskFileItem.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/disk/DiskFileItem.java
index 0d08c59..6ed2800 100644
--- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/disk/DiskFileItem.java
+++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/disk/DiskFileItem.java
@@ -27,7 +27,9 @@ import java.nio.charset.StandardCharsets;
import java.nio.charset.UnsupportedCharsetException;
import java.nio.file.CopyOption;
import java.nio.file.Files;
+import java.nio.file.InvalidPathException;
import java.nio.file.Path;
+import java.nio.file.Paths;
import java.nio.file.StandardCopyOption;
import java.util.Map;
import java.util.UUID;
@@ -36,7 +38,6 @@ import java.util.concurrent.atomic.AtomicInteger;
import org.apache.commons.fileupload2.FileItem;
import org.apache.commons.fileupload2.FileItemHeaders;
import org.apache.commons.fileupload2.FileUploadException;
-import org.apache.commons.fileupload2.InvalidFileNameException;
import org.apache.commons.fileupload2.ParameterParser;
import org.apache.commons.io.Charsets;
import org.apache.commons.io.file.PathUtils;
@@ -80,29 +81,34 @@ public class DiskFileItem implements FileItem {
private static final AtomicInteger COUNTER = new AtomicInteger(0);
/**
- * Checks, whether the given file name is valid in the sense, that it doesn't contain any NUL characters. If the file name is valid, it will be returned
- * without any modifications. Otherwise, an {@link InvalidFileNameException} is raised.
+ * Tests if the file name is valid. For example, if it contains a NUL characters, it's invalid. If the file name is valid, it will be returned without any
+ * modifications. Otherwise, throw an {@link InvalidPathException}.
*
* @param fileName The file name to check
* @return Unmodified file name, if valid.
- * @throws InvalidFileNameException The file name was found to be invalid.
+ * @throws InvalidPathException The file name is invalid.
*/
public static String checkFileName(final String fileName) {
- if (fileName != null && fileName.indexOf('\u0000') != -1) {
- // fileName.replace("\u0000", "\\0")
- final StringBuilder sb = new StringBuilder();
- for (int i = 0; i < fileName.length(); i++) {
- final char c = fileName.charAt(i);
- switch (c) {
- case 0:
- sb.append("\\0");
- break;
- default:
- sb.append(c);
- break;
+ if (fileName != null) {
+ // Specific NUL check to build a better exception message.
+ final int indexOf0 = fileName.indexOf(0);
+ if (indexOf0 != -1) {
+ final StringBuilder sb = new StringBuilder();
+ for (int i = 0; i < fileName.length(); i++) {
+ final char c = fileName.charAt(i);
+ switch (c) {
+ case 0:
+ sb.append("\\0");
+ break;
+ default:
+ sb.append(c);
+ break;
+ }
}
+ throw new InvalidPathException(fileName, sb.toString(), indexOf0);
}
- throw new InvalidFileNameException(fileName, "Invalid file name: " + sb);
+ // Throws InvalidPathException on invalid file names
+ Paths.get(fileName);
}
return fileName;
}
@@ -188,12 +194,12 @@ public class DiskFileItem implements FileItem {
/**
* Constructs a new {@code DiskFileItem} instance.
*
- * @param fieldName The name of the form field.
- * @param contentType The content type passed by the browser or {@code null} if not specified.
- * @param isFormField Whether or not this item is a plain form field, as opposed to a file upload.
- * @param fileName The original file name in the user's file system, or {@code null} if not specified.
- * @param threshold The threshold, in bytes, below which items will be retained in memory and above which they will be stored as a file.
- * @param repository The data repository, which is the directory in which files will be created, should the item size exceed the threshold.
+ * @param fieldName The name of the form field.
+ * @param contentType The content type passed by the browser or {@code null} if not specified.
+ * @param isFormField Whether or not this item is a plain form field, as opposed to a file upload.
+ * @param fileName The original file name in the user's file system, or {@code null} if not specified.
+ * @param threshold The threshold, in bytes, below which items will be retained in memory and above which they will be stored as a file.
+ * @param repository The data repository, which is the directory in which files will be created, should the item size exceed the threshold.
*/
public DiskFileItem(final String fieldName, final String contentType, final boolean isFormField, final String fileName, final int threshold,
final Path repository) {
@@ -317,9 +323,8 @@ public class DiskFileItem implements FileItem {
* Gets the original file name in the client's file system.
*
* @return The original file name in the client's file system.
- * @throws org.apache.commons.fileupload2.InvalidFileNameException The file name contains a NUL character, which might be an indicator of a security attack.
- * If you intend to use the file name anyways, catch the exception and use
- * {@link InvalidFileNameException#getName()}.
+ * @throws InvalidPathException The file name contains a NUL character, which might be an indicator of a security attack. If you intend to use the file name
+ * anyways, catch the exception and use {@link InvalidPathException#getInput()}.
*/
@Override
public String getName() {
diff --git a/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/AbstractStreamingTest.java b/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/AbstractStreamingTest.java
index 9ba0ee1..1b39cb4 100644
--- a/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/AbstractStreamingTest.java
+++ b/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/AbstractStreamingTest.java
@@ -27,6 +27,7 @@ import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStreamWriter;
import java.nio.charset.StandardCharsets;
+import java.nio.file.InvalidPathException;
import java.util.Iterator;
import java.util.List;
@@ -185,7 +186,7 @@ public abstract class AbstractStreamingTest<F extends FileUpload<R>, R, C extend
}
/**
- * Tests, whether an {@link InvalidFileNameException} is thrown.
+ * Tests, whether an {@link InvalidPathException} is thrown.
*/
@Test
public void testInvalidFileNameException() throws Exception {
@@ -219,18 +220,20 @@ public abstract class AbstractStreamingTest<F extends FileUpload<R>, R, C extend
try {
fileItemStream.getName();
fail("Expected exception");
- } catch (final InvalidFileNameException e) {
- assertEquals(fileName, e.getName());
- assertEquals(-1, e.getMessage().indexOf(fileName));
+ } catch (final InvalidPathException e) {
+ assertEquals(fileName, e.getInput());
+ assertEquals(26, e.getMessage().indexOf(fileName));
+ assertEquals(7, e.getIndex());
assertTrue(e.getMessage().contains("foo.exe\\0.png"));
}
try {
parseUpload(reqBytes);
fail("Expected exception");
- } catch (final InvalidFileNameException e) {
- assertEquals(fileName, e.getName());
- assertEquals(-1, e.getMessage().indexOf(fileName));
+ } catch (final InvalidPathException e) {
+ assertEquals(fileName, e.getInput());
+ assertEquals(26, e.getMessage().indexOf(fileName));
+ assertEquals(7, e.getIndex());
assertTrue(e.getMessage().contains("foo.exe\\0.png"));
}
}
diff --git a/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/disk/DiskFileItemSerializeTest.java b/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/disk/DiskFileItemSerializeTest.java
index 2899d46..9b86b61 100644
--- a/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/disk/DiskFileItemSerializeTest.java
+++ b/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/disk/DiskFileItemSerializeTest.java
@@ -28,12 +28,12 @@ import java.io.ObjectOutputStream;
import java.io.OutputStream;
import java.nio.file.FileVisitResult;
import java.nio.file.Files;
+import java.nio.file.InvalidPathException;
import java.nio.file.Path;
import java.nio.file.attribute.BasicFileAttributes;
import org.apache.commons.fileupload2.FileItem;
import org.apache.commons.fileupload2.FileItemFactory;
-import org.apache.commons.fileupload2.InvalidFileNameException;
import org.apache.commons.io.file.PathUtils;
import org.apache.commons.io.file.SimplePathVisitor;
import org.apache.commons.lang3.SerializationUtils;
@@ -183,7 +183,7 @@ public class DiskFileItemSerializeTest {
@Test
public void testCheckFileName() {
- assertThrows(InvalidFileNameException.class, () -> DiskFileItem.checkFileName("\0"));
+ assertThrows(InvalidPathException.class, () -> DiskFileItem.checkFileName("\0"));
}
/**
diff --git a/commons-fileupload2-jakarta/src/test/java/org/apache/commons/fileupload2/jakarta/JavaxStreamingTest.java b/commons-fileupload2-jakarta/src/test/java/org/apache/commons/fileupload2/jakarta/JakartaStreamingTest.java
similarity index 92%
rename from commons-fileupload2-jakarta/src/test/java/org/apache/commons/fileupload2/jakarta/JavaxStreamingTest.java
rename to commons-fileupload2-jakarta/src/test/java/org/apache/commons/fileupload2/jakarta/JakartaStreamingTest.java
index 51b0932..8d8f053 100644
--- a/commons-fileupload2-jakarta/src/test/java/org/apache/commons/fileupload2/jakarta/JavaxStreamingTest.java
+++ b/commons-fileupload2-jakarta/src/test/java/org/apache/commons/fileupload2/jakarta/JakartaStreamingTest.java
@@ -25,7 +25,7 @@ import jakarta.servlet.http.HttpServletRequest;
/**
* Unit test for items with varying sizes.
*/
-public class JavaxStreamingTest extends AbstractStreamingTest<JakartaServletFileUpload, HttpServletRequest, JakartaServletRequestContext> {
+public class JakartaStreamingTest extends AbstractStreamingTest<JakartaServletFileUpload, HttpServletRequest, JakartaServletRequestContext> {
@Override
protected JakartaServletFileUpload newFileUpload() {
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 49c2876..e9f7866 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -75,6 +75,7 @@ The <action> type attribute can be add,update,fix,remove.
<action dev="ggregory" type="remove" due-to="Gary Gregory">Replace org.apache.commons.fileupload2.util.mime.Base64Decoder with java.util.Base64.</action>
<action dev="ggregory" type="remove" due-to="Gary Gregory">Replace LimitedInputStream with BoundedInputStream.</action>
<action dev="ggregory" type="remove" due-to="Gary Gregory">FileItemHeadersImpl is no longer Serializable.</action>
+ <action dev="ggregory" type="remove" due-to="Gary Gregory">Reuse Java's InvalidPathException instead of the custom InvalidFileNameException.</action>
<!-- UPDATE -->
<action dev="ggregory" type="update" due-to="Dependabot, Gary Gregory">Bump actions/cache from 2.1.6 to 3.0.8 #128, #140.</action>
<action dev="ggregory" type="update" due-to="Dependabot, Gary Gregory">Bump actions/checkout from 2.3.4 to 3.0.2 #125.</action>