From 42b9ad8dbd416bf7ed73ad95e94681329cc83ac7 Mon Sep 17 00:00:00 2001 From: Jacques Le Roux Date: Thu, 24 Oct 2024 20:35:39 +0200 Subject: [PATCH] Improved: Prevent URL parameters manipulation (OFBIZ-13147) The "JavaScriptEnabled=Y" and "&wt=javabin" references are weaknesses. I temporarily put them in ControlFilter::doFilter to allow things (demo and integration tests) to work for my test (only possible on a site w. domain IP), ie not locally. I think we can remove "JavaScriptEnabled=Y". I put it there because we use it in links at https://ofbiz.apache.org/ofbiz-demos.html. Maybe other places where it's easy to remove w/o side effects. It's anyway an user preference, not mandatory in query string. I needed "&wt=javabin" for the Solr tests to pass. Sometimes ago I already faced a such issue. And then put in place what's needed. ControlFilter::isSolrTest is the solution by generalising this usage. --- .../apache/ofbiz/webapp/control/ControlFilter.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 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 7a7511271f..a4c0e59400 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 @@ -131,6 +131,10 @@ private static Set readAllowedPaths(String paths) { : Arrays.stream(paths.split(":")).collect(Collectors.toSet()); } + private static boolean isSolrTest() { + return !GenericValue.getStackTraceAsString().contains("ControlFilterTests") + && null == System.getProperty("SolrDispatchFilter"); + } /** * Makes allowed paths pass through while redirecting the others to a fix location. */ @@ -159,9 +163,7 @@ public void doFilter(HttpServletRequest req, HttpServletResponse resp, FilterCha GenericValue userLogin = (GenericValue) session.getAttribute("userLogin"); if (!LoginWorker.hasBasePermission(userLogin, req)) { // Allows UEL and FlexibleString (OFBIZ-12602) - if (!GenericValue.getStackTraceAsString().contains("ControlFilterTests") - && null == System.getProperty("SolrDispatchFilter") // Allows Solr tests - && SecurityUtil.containsFreemarkerInterpolation(req, resp, uri)) { + if (isSolrTest() && SecurityUtil.containsFreemarkerInterpolation(req, resp, uri)) { return; } } @@ -170,11 +172,9 @@ public void doFilter(HttpServletRequest req, HttpServletResponse resp, FilterCha String queryString = req.getQueryString(); if (queryString != null) { queryString = URLDecoder.decode(queryString, "UTF-8"); - // wt=javabin allows Solr tests, see https://cwiki.apache.org/confluence/display/solr/javabin if (UtilValidate.isUrlInString(queryString) || !SecuredUpload.isValidText(queryString, Collections.emptyList()) - && !(queryString.contains("JavaScriptEnabled=Y") - || queryString.contains("wt=javabin"))) { + && isSolrTest()) { Debug.logError("For security reason this URL is not accepted", MODULE); throw new RuntimeException("For security reason this URL is not accepted"); }