Skip to content

Commit

Permalink
Improved: Prevent URL parameters manipulation (OFBIZ-13147)
Browse files Browse the repository at this point in the history
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.

Conflict handled by hand
  • Loading branch information
JacquesLeRoux committed Oct 24, 2024
1 parent 05d0de9 commit 5f4f316
Showing 1 changed file with 7 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,10 @@ private static Set<String> 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.
*/
Expand Down Expand Up @@ -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;
}
}
Expand All @@ -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.isUrl(queryString)
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");
}
Expand Down

0 comments on commit 5f4f316

Please sign in to comment.