This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/trunk by this push:
new 950be5b0aa Improved: Allow configuration of file name validation
pattern (OFBIZ-12851)
950be5b0aa is described below
commit 950be5b0aa2283147be6fc2ebdded06d09831627
Author: Jacques Le Roux <jacques.le.r...@les7arts.com>
AuthorDate: Tue Aug 29 17:04:47 2023 +0200
Improved: Allow configuration of file name validation pattern (OFBIZ-12851)
Read file name validation pattern from security.properties to allow
customization
Explanation:
Hard coding the pattern made it difficult to adjust file name validation.
jleroux: Rather than pushing the PR, which is OK with me, I apply as a patch
locally and make some modifications before pushing:
indentation in SecuredUpload, and warning about file names safeness in
security.properties
Thanks: originalnichtskoenner for this PR on GH:
https://github.com/apache/ofbiz-framework/pull/668.
---
framework/security/config/security.properties | 7 +++++
.../org/apache/ofbiz/security/SecuredUpload.java | 30 +++++++++++++++++++---
2 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index 54e91c0ee7..7beb94fca9 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -247,6 +247,13 @@
deniedFileExtensions=html,htm,php,php1,php2,hph3,php4,php5,php6,php7,phps,asp,as
#-- As it name says, allowAllUploads opens all possibilities
allowAllUploads=
+#--
+#-- Default characters that are allowed in file names and file extensions to
guarantee safeness
+#-- Uncomment to change. Note that allowing all characters is at risk.
+#-- You may refer to
https://cheatsheetseries.owasp.org/cheatsheets/File_Upload_Cheat_Sheet.html#filename-sanitization
+#fileNameValidCharacters=[a-zA-Z0-9-_ ]
+#fileNameValidCharactersDuplicates=[a-zA-Z0-9-_ ()]
+
#--
#-- CSV format used to upload CSV files, cf.
https://commons.apache.org/proper/commons-csv/apidocs/org/apache/commons/csv/CSVFormat.html
csvformat=CSVFormat.DEFAULT
diff --git
a/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
b/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
index d4c1b68420..b9206b194b 100644
---
a/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
+++
b/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
@@ -108,6 +108,12 @@ public class SecuredUpload {
private static final Boolean ALLOWSTRINGCONCATENATIONINUPLOADEDFILES =
UtilProperties.getPropertyAsBoolean("security",
"allowStringConcatenationInUploadedFiles", false);
+ // "(" and ")" for duplicates files
+ private static final String FILENAMEVALIDCHARACTERS_DUPLICATES =
+ UtilProperties.getPropertyValue("security",
"fileNameValidCharactersDuplicates", "[a-zA-Z0-9-_ ()]");
+ private static final String FILENAMEVALIDCHARACTERS =
+ UtilProperties.getPropertyValue("security",
"fileNameValidCharacters", "[a-zA-Z0-9-_ ]");
+
public static boolean isValidText(String content, List<String> allowed)
throws IOException {
String contentWithoutSpaces = content.replace(" ", "");
if ((contentWithoutSpaces.contains("\"+\"") ||
contentWithoutSpaces.contains("'+'"))
@@ -157,10 +163,18 @@ public class SecuredUpload {
Debug.logError("Uploaded file name too long", MODULE);
} else if (p.toString().contains(imageServerUrl.replace("/",
"\\"))) {
// TODO check this is still useful in at least 1 case
- if (fileName.matches("[a-zA-Z0-9-_ ()]{1,249}.[a-zA-Z0-9-_
]{1,10}")) { // "(" and ")" for duplicates files
+ if (fileName.matches(
+ FILENAMEVALIDCHARACTERS_DUPLICATES
+ .concat("{1,249}.")
+ .concat(FILENAMEVALIDCHARACTERS)
+ .concat("{1,10}"))) {
wrongFile = false;
}
- } else if (fileName.matches("[a-zA-Z0-9-_
]{1,249}.[a-zA-Z0-9-_ ]{1,10}")) {
+ } else if (fileName.matches(
+ FILENAMEVALIDCHARACTERS
+ .concat("{1,249}.")
+ .concat(FILENAMEVALIDCHARACTERS)
+ .concat("{1,10}"))) {
wrongFile = false;
}
} else { // Suppose a *nix system
@@ -168,10 +182,18 @@ public class SecuredUpload {
Debug.logError("Uploaded file name too long", MODULE);
} else if (p.toString().contains(imageServerUrl)) {
// TODO check this is still useful in at least 1 case
- if (fileName.matches("[a-zA-Z0-9-_
()]{1,4086}.[a-zA-Z0-9-_ ]{1,10}")) { // "(" and ")" for duplicates files
+ if (fileName.matches(
+ FILENAMEVALIDCHARACTERS_DUPLICATES
+ .concat("{1,4086}.")
+ .concat(FILENAMEVALIDCHARACTERS)
+ .concat("{1,10}"))) {
wrongFile = false;
}
- } else if (fileName.matches("[a-zA-Z0-9-_
]{1,4086}.[a-zA-Z0-9-_ ]{1,10}")) {
+ } else if (fileName.matches(
+ FILENAMEVALIDCHARACTERS
+ .concat("{1,4086}.")
+ .concat(FILENAMEVALIDCHARACTERS)
+ .concat("{1,10}"))) {
wrongFile = false;
}
}
