You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by mr...@apache.org on 2026/10/18 17:50:04 UTC
(ofbiz-framework) branch trunk updated (d1665ff2f0 -> 46d7803791)
This is an automated email from the ASF dual-hosted git repository.
mridulpathak pushed a change to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
from d1665ff2f0 Fixed: CodeQL workflow fails while printing debug logs
new ddccb6d363 Fixed: Build the sort links of list forms with an encoded query string
new 5343967656 Improved: Remove the parsing of query strings encoded in the request path
new 46d7803791 Fixed: Check every URL parameter value for markup in ControlFilter
The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../java/org/apache/ofbiz/base/util/UtilCodec.java | 13 -------
.../java/org/apache/ofbiz/base/util/UtilHttp.java | 20 -----------
framework/security/config/security.properties | 5 ---
.../org/apache/ofbiz/security/SecuredUpload.java | 41 +++-------------------
.../apache/ofbiz/webapp/control/ControlFilter.java | 36 +++----------------
.../ofbiz/webtools/entity/FindGeneric.groovy | 4 ---
.../widget/renderer/macro/MacroFormRenderer.java | 17 +--------
.../renderer/macro/MacroFormRendererTest.java | 3 +-
8 files changed, 13 insertions(+), 126 deletions(-)
(ofbiz-framework) 01/03: Fixed: Build the sort links of list forms with an encoded query string
Posted by mr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
mridulpathak pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit ddccb6d363ec70259b3e584690a4722054d20bd7
Author: Mridul Pathak <mr...@gmail.com>
AuthorDate: Sun Oct 18 22:39:10 2026 +0530
Fixed: Build the sort links of list forms with an encoded query string
The sort links of list forms URL-encoded their whole query string and appended it to the request path, and FindGeneric turned that encoding off with the escapeUrlEncode context flag because it broke its sort links, so its sort links carried the request parameters unencoded. The sort link is now built like the pagination links, with a regular query string whose parameter names and values are URL-encoded. The escapeUrlEncode flag and UtilCodec.encodeUrl(String, Map) are removed.
---
.../main/java/org/apache/ofbiz/base/util/UtilCodec.java | 13 -------------
.../org/apache/ofbiz/webtools/entity/FindGeneric.groovy | 4 ----
.../ofbiz/widget/renderer/macro/MacroFormRenderer.java | 17 +----------------
.../widget/renderer/macro/MacroFormRendererTest.java | 3 ++-
4 files changed, 3 insertions(+), 34 deletions(-)
diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilCodec.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilCodec.java
index 2f37c0ddeb..6475906032 100644
--- a/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilCodec.java
+++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilCodec.java
@@ -402,19 +402,6 @@ public class UtilCodec {
return getEncoder("url").encode(queryString);
}
- /**
- * Check if an escapeUrlEncode is present in the context, to escape url encoding in a specific case
- * This is necessary if the url is sent to another encoding tool.
- * @param queryString
- * @param context
- * @return encoding url with OFBiz rule
- */
- public static String encodeUrl(String queryString, Map<String, Object> context) {
- return "true".equalsIgnoreCase((String) context.get("escapeUrlEncode"))
- ? queryString
- : encodeUrl(queryString);
- }
-
/**
* Uses a black-list approach for necessary characters for HTML.
* Does not allow various characters (after canonicalization), including
diff --git a/framework/webtools/src/main/groovy/org/apache/ofbiz/webtools/entity/FindGeneric.groovy b/framework/webtools/src/main/groovy/org/apache/ofbiz/webtools/entity/FindGeneric.groovy
index f96e4f3a91..5103a0b204 100644
--- a/framework/webtools/src/main/groovy/org/apache/ofbiz/webtools/entity/FindGeneric.groovy
+++ b/framework/webtools/src/main/groovy/org/apache/ofbiz/webtools/entity/FindGeneric.groovy
@@ -33,10 +33,6 @@ import org.w3c.dom.Document
ModelEntity modelEntity = null
-// escape the security url encoding that break the sortField with the ftl rendering
-// no security issue here, nothing come from the request
-context.escapeUrlEncode = 'true'
-
try {
modelEntity = delegator.getModelEntity(parameters.entityName)
} catch (GenericEntityException e) {
diff --git a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRenderer.java b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRenderer.java
index b79ab21049..fafbd32e8e 100644
--- a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRenderer.java
+++ b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRenderer.java
@@ -733,23 +733,8 @@ public final class MacroFormRenderer implements FormStringRenderer {
if (ajaxEnabled) {
linkUrl = MacroCommonRenderer.createAjaxParamsFromUpdateAreas(updateAreas, paramMap, modelForm, null, context);
} else {
- StringBuilder sb = new StringBuilder("?");
- Iterator<Map.Entry<String, Object>> iter = paramMap.entrySet().iterator();
- while (iter.hasNext()) {
- Map.Entry<String, Object> entry = iter.next();
- sb.append(entry.getKey()).append("=").append(entry.getValue());
- if (iter.hasNext()) {
- sb.append("&");
- }
- }
- String newQueryString = sb.toString();
String urlPath = UtilHttp.removeQueryStringFromTarget(paginateTarget);
- if (newQueryString.contains("?null=")) { // FIXME, not sure how to handle URL encoding in tests
- newQueryString = newQueryString.replace("?null=LinkFromQBEString", "?sortField=LinkFromQBEString");
- linkUrl = rh.makeLink(this.request, this.response, urlPath.concat(newQueryString));
- } else {
- linkUrl = rh.makeLink(this.request, this.response, urlPath.concat(UtilCodec.encodeUrl(newQueryString, context)));
- }
+ linkUrl = rh.makeLink(this.request, this.response, urlPath + "?" + UtilHttp.urlEncodeArgs(paramMap));
}
String tooltip = modelFormField.getSortFieldHelpText(context);
writeFtlElement(writer, renderableFtlFormElementsBuilder.sortField(sortFieldStyle, titleText, linkUrl, ajaxEnabled, tooltip));
diff --git a/framework/widget/src/test/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRendererTest.java b/framework/widget/src/test/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRendererTest.java
index 911992ff25..12608166e1 100644
--- a/framework/widget/src/test/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRendererTest.java
+++ b/framework/widget/src/test/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRendererTest.java
@@ -646,11 +646,12 @@ public final class MacroFormRendererTest {
final ModelForm modelForm = mock(ModelForm.class);
final String paginateTarget = "TARGET";
final String qbeString = "field2=value2 with spaces";
+ final String encodedQbeString = "field2=value2+with+spaces";
final String linkFromQbeString = "LinkFromQBEString";
when(modelFormField.getModelForm()).thenReturn(modelForm);
when(modelForm.getPaginateTarget(notNull())).thenReturn(paginateTarget);
- when(requestHandler.makeLink(notNull(), notNull(), contains(qbeString))).thenReturn(linkFromQbeString);
+ when(requestHandler.makeLink(notNull(), notNull(), contains(encodedQbeString))).thenReturn(linkFromQbeString);
when(modelFormField.getSortFieldHelpText(notNull())).thenReturn("HELPTEXT");
when(renderableFtlFormElementsBuilder.sortField(nullable(String.class), eq(""), eq(linkFromQbeString), eq(false),
eq("HELPTEXT"))).thenReturn(genericMacroCall);
(ofbiz-framework) 02/03: Improved: Remove the parsing of query strings encoded in the request path
Posted by mr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
mridulpathak pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 5343967656d71f767142fd69e7e978bfacf22708
Author: Mridul Pathak <mr...@gmail.com>
AuthorDate: Sun Oct 18 22:39:10 2026 +0530
Improved: Remove the parsing of query strings encoded in the request path
Now that sort links no longer put their query string in the request path, UtilHttp.getParameterMap no longer parses a query string out of an encoded request path, and ControlFilter no longer checks such a query string against deniedWebShellTokens. The allowedTokens property and the query string mode of SecuredUpload.isValidText, which were only used by that check, are removed.
---
.../java/org/apache/ofbiz/base/util/UtilHttp.java | 20 -----------
framework/security/config/security.properties | 5 ---
.../org/apache/ofbiz/security/SecuredUpload.java | 41 +++-------------------
.../apache/ofbiz/webapp/control/ControlFilter.java | 28 ---------------
4 files changed, 5 insertions(+), 89 deletions(-)
diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilHttp.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilHttp.java
index 9c05f3248a..565122cb20 100644
--- a/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilHttp.java
+++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilHttp.java
@@ -33,10 +33,7 @@ import java.io.InputStream;
import java.io.OutputStream;
import java.io.UnsupportedEncodingException;
import java.net.FileNameMap;
-import java.net.URI;
-import java.net.URISyntaxException;
import java.net.URLConnection;
-import java.net.URLDecoder;
import java.net.URLEncoder;
import java.nio.ByteBuffer;
import java.nio.charset.Charset;
@@ -69,8 +66,6 @@ import org.apache.commons.fileupload2.core.DiskFileItemFactory;
import org.apache.commons.fileupload2.core.FileItem;
import org.apache.commons.fileupload2.core.FileUploadException;
import org.apache.commons.fileupload2.jakarta.JakartaServletFileUpload;
-import org.apache.http.NameValuePair;
-import org.apache.http.client.utils.URLEncodedUtils;
import org.apache.ofbiz.entity.Delegator;
import org.apache.ofbiz.entity.util.EntityUtilProperties;
import org.apache.ofbiz.webapp.control.ConfigXMLReader;
@@ -171,21 +166,6 @@ public final class UtilHttp {
if (Debug.verboseOn()) {
Debug.logVerbose("Made Request Parameter Map with [" + params.size() + "] Entries", MODULE);
}
-
- // Handles encoded queryStrings
- String requestURI = req.getRequestURI();
- if (params.isEmpty() && null != requestURI) {
- try {
- List<NameValuePair> nameValuePairs = URLEncodedUtils.parse(
- new URI(UtilHttp.encodeBlanks(URLDecoder.decode(requestURI, "UTF-8"))),
- Charset.forName("UTF-8"));
- for (NameValuePair element : nameValuePairs) {
- params.put(element.getName(), element.getValue());
- }
- } catch (UnsupportedEncodingException | URISyntaxException e) {
- Debug.logError("Can't handle encoded queryString " + requestURI, MODULE);
- }
- }
return canonicalizeParameterMap(params);
}
diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties
index 65b1e4ff4a..675667b9c6 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -372,11 +372,6 @@ deniedWebShellTokens=java.,beans,freemarker,<script,javascript,<body,body ,<form
execute,println,calc,touch,curl,base64, tcp ,4444,base32, tr , xxd ,bash, od ,|od ,printf,echo
-#-- SHA-1 versions of tokens containing (as String) at least one deniedWebShellTokens
-#-- This is notably used to allow special values in query parameters.
-#-- If you add a token beware that it does not content ",". It's the separator.
-allowedTokens=$SHA$OFBiz$2naHrANKTniFcgLJk4oXr3IRQ48
-
#-- RegExp to secure groovy script execution. If the regExp match a script, it would be disabled and OFBiz run nothing.
#-- In this case, you will have on log the original script with it hash. The hash can be added on allowedScriptletHashes
#-- properties to accept it on the next execution.
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 91112ad74c..9d9009cc22 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
@@ -78,7 +78,6 @@ import org.apache.commons.csv.CSVParser;
import org.apache.commons.io.FileUtils;
import org.apache.commons.io.FilenameUtils;
import org.apache.commons.lang.StringUtils;
-import org.apache.ofbiz.base.crypto.HashCrypt;
import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.FileUtil;
import org.apache.ofbiz.base.util.StringUtil;
@@ -286,28 +285,17 @@ public class SecuredUpload {
return true;
}
- // Cover method of the same name below. Historically used with 84 references when below was created
// check there is no web shell in the uploaded file
// A file containing a reverse shell will be rejected.
public static boolean isValidText(String content, List<String> allowed) throws IOException {
- return isValidText(content, allowed, false);
- }
-
- public static boolean isValidText(String content, List<String> allowed, boolean isQuery) throws IOException {
if (content == null) {
return false;
}
- if (!isQuery) {
- String contentWithoutSpaces = content.replaceAll(" ", "");
- if ((contentWithoutSpaces.contains("\"+\"") || contentWithoutSpaces.contains("'+'"))
- && !ALLOWSTRINGCONCATENATIONINUPLOADEDFILES) {
- Debug.logInfo("The uploaded file contains a string concatenation. It can't be uploaded for security reason", MODULE);
- return false;
- }
- } else {
- // Check the query string is safe, notably no reverse shell
- List<String> queryParameters = StringUtil.split(content, "&");
- return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(queryParameters, token.toLowerCase(), allowed));
+ String contentWithoutSpaces = content.replaceAll(" ", "");
+ if ((contentWithoutSpaces.contains("\"+\"") || contentWithoutSpaces.contains("'+'"))
+ && !ALLOWSTRINGCONCATENATIONINUPLOADEDFILES) {
+ Debug.logInfo("The uploaded file contains a string concatenation. It can't be uploaded for security reason", MODULE);
+ return false;
}
// Check there is no web shell in an uploaded file
@@ -1594,25 +1582,6 @@ public class SecuredUpload {
return isOK;
}
- // Check there is no reverse shell in query string
- private static boolean isValid(List<String> queryParameters, String string, List<String> allowed) {
- boolean isOK = true;
-
- for (String parameter : queryParameters) {
- if (!parameter.contains(string)
- || allowed.contains(HashCrypt.cryptBytes("SHA", "OFBiz", parameter.toLowerCase().getBytes(StandardCharsets.UTF_8)))) {
- continue;
- } else {
- isOK = false;
- break;
- }
- }
- if (!isOK) {
- Debug.logInfo("The HTTP query string contains the string '" + string + "'. It can't be uploaded for security reason", MODULE);
- }
- return isOK;
- }
-
private static void deleteBadFile(String fileToCheck) {
Debug.logError("File : " + fileToCheck + ", can't be uploaded for security reason", MODULE);
File badFile = new File(fileToCheck);
diff --git a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
index acc7d3a704..3694e7f701 100644
--- a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
+++ b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
@@ -22,7 +22,6 @@ import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URLDecoder;
-import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
@@ -36,15 +35,11 @@ import org.apache.commons.text.StringEscapeUtils;
import org.apache.commons.validator.routines.UrlValidator;
import org.apache.logging.log4j.ThreadContext;
import org.apache.ofbiz.base.util.Debug;
-import org.apache.ofbiz.base.util.StringUtil;
import org.apache.ofbiz.base.util.UtilGenerics;
import org.apache.ofbiz.base.util.UtilHttp;
-import org.apache.ofbiz.base.util.UtilProperties;
-import org.apache.ofbiz.base.util.UtilValidate;
import org.apache.ofbiz.common.UrlServletHelper;
import org.apache.ofbiz.entity.GenericValue;
import org.apache.ofbiz.security.SecuredFreemarker;
-import org.apache.ofbiz.security.SecuredUpload;
import jakarta.servlet.FilterChain;
import jakarta.servlet.FilterConfig;
@@ -95,7 +90,6 @@ public class ControlFilter extends HttpFilter {
private int errorCode;
/** The list of all path prefixes that are allowed. */
private Set<String> allowedPaths;
- private static final List<String> ALLOWEDTOKENS = getAllowedTokens();
@Override
@@ -151,11 +145,6 @@ public class ControlFilter extends HttpFilter {
resp.sendRedirect(redirectPathIsUrl ? redirectPath : (contextPath + redirectPath));
}
- private static List<String> getAllowedTokens() {
- String allowedTokens = UtilProperties.getPropertyValue("security", "allowedTokens");
- return UtilValidate.isNotEmpty(allowedTokens) ? StringUtil.split(allowedTokens, ",") : new ArrayList<>();
- }
-
private static boolean isControlFilterTests() {
return null != System.getProperty("ControlFilterTests");
}
@@ -230,23 +219,6 @@ public class ControlFilter extends HttpFilter {
}
}
- // Reject insecure URLs
- String queryString = null;
- try {
- queryString = new URI(uriWithContext).getQuery();
-
- } catch (URISyntaxException e) {
- Debug.logError("Weird URI: " + e, MODULE);
- throw new RuntimeException(e);
- }
- if (queryString != null) {
- queryString = URLDecoder.decode(queryString, "UTF-8");
- if (UtilValidate.isUrlInString(queryString)
- || !SecuredUpload.isValidText(queryString.toLowerCase(), ALLOWEDTOKENS, true)) {
- Debug.logError("For security reason this URL is not accepted", MODULE);
- throw new RuntimeException("For security reason this URL is not accepted");
- }
- }
if (uriWithContext != null) { // "null" allows tests with Mockito because ControlFilterTests sends null.
try {
String uRIFiltered = new URI(uriWithContext)
(ofbiz-framework) 03/03: Fixed: Check every URL parameter value for markup in ControlFilter
Posted by mr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
mridulpathak pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 46d7803791a1036457fe6a08a09ce4e36ccd9180
Author: Mridul Pathak <mr...@gmail.com>
AuthorDate: Sun Oct 18 22:39:10 2026 +0530
Fixed: Check every URL parameter value for markup in ControlFilter
ControlFilter rejected a request whose parameter value contained "<", but the result for each parameter replaced the previous one, so only the last parameter checked counted. Any such parameter now rejects the request. Only the parameters passed in the URL (query string and path) are checked, so form bodies that legitimately contain markup, such as HTML content, are accepted.
---
.../main/java/org/apache/ofbiz/webapp/control/ControlFilter.java | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
index 3694e7f701..aaf7139a8d 100644
--- a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
+++ b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
@@ -164,7 +164,8 @@ public class ControlFilter extends HttpFilter {
if (!(isControlFilterTests() || isEntityImport || isProgramExport)) {
// Prevents stream exploitation
UrlServletHelper.setRequestAttributes(req, null, req.getServletContext());
- Map<String, Object> parameters = UtilHttp.getParameterMap(req);
+ // Only the parameters passed in the URL are checked, a form body can legitimately contain markup
+ Map<String, Object> parameters = UtilHttp.getUrlOnlyParameterMap(req);
boolean reject = false;
if (!parameters.isEmpty()) {
for (String key : parameters.keySet()) {
@@ -174,8 +175,9 @@ public class ControlFilter extends HttpFilter {
List<String> toCheck = object.getClass().equals(String.class)
? List.of((String) object)
: UtilGenerics.checkCollection(object, String.class);
- reject = toCheck.stream()
- .anyMatch(val -> val.contains("<"));
+ if (toCheck.stream().anyMatch(val -> val.contains("<"))) {
+ reject = true;
+ }
} catch (IllegalArgumentException e) {
Debug.logWarning(e, MODULE);
reject = true;