This is an automated email from the ASF dual-hosted git repository.
mthl pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit f93b9ea29a0cc86ebde57f941c6cbdd307a1417e
Author: Samuel Trégouët <samuel.trego...@nereide.fr>
AuthorDate: Mon Nov 4 22:37:39 2019 +0100
Improved: Lint ‘ServiceEventHandler’ class
(OFBIZ-11260)
---
build.gradle | 2 +-
.../ofbiz/webapp/event/ServiceEventHandler.java | 124 +++++++++++++++------
2 files changed, 88 insertions(+), 38 deletions(-)
diff --git a/build.gradle b/build.gradle
index dbdd492..cfcceb1 100644
--- a/build.gradle
+++ b/build.gradle
@@ -307,7 +307,7 @@ checkstyle {
// the sum of errors that were present before introducing the
// ‘checkstyle’ tool present in the framework and in the official
// plugins.
- tasks.checkstyleMain.maxErrors = 37824
+ tasks.checkstyleMain.maxErrors = 37781
// Currently there are a lot of errors so we need to temporarily
// hide them to avoid polluting the terminal output.
showViolations = false
diff --git
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceEventHandler.java
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceEventHandler.java
index af80792..0e28340 100644
---
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceEventHandler.java
+++
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceEventHandler.java
@@ -65,7 +65,8 @@ public class ServiceEventHandler implements EventHandler {
}
@Override
- public String invoke(Event event, RequestMap requestMap,
HttpServletRequest request, HttpServletResponse response) throws
EventHandlerException {
+ public String invoke(Event event, RequestMap requestMap,
HttpServletRequest request, HttpServletResponse response)
+ throws EventHandlerException {
// make sure we have a valid reference to the Service Engine
LocalDispatcher dispatcher = (LocalDispatcher)
request.getAttribute("dispatcher");
if (dispatcher == null) {
@@ -91,7 +92,9 @@ public class ServiceEventHandler implements EventHandler {
if (serviceName == null) {
throw new EventHandlerException("Service name (eventMethod) cannot
be null");
}
- if (Debug.verboseOn()) Debug.logVerbose("[Set mode/service]: " + mode
+ "/" + serviceName, module);
+ if (Debug.verboseOn()) {
+ Debug.logVerbose("[Set mode/service]: " + mode + "/" +
serviceName, module);
+ }
// some needed info for when running the service
Locale locale = UtilHttp.getLocale(request);
@@ -129,29 +132,43 @@ public class ServiceEventHandler implements EventHandler {
String name = modelParam.name;
// don't include userLogin, that's taken care of below
- if ("userLogin".equals(name)) continue;
+ if ("userLogin".equals(name)) {
+ continue;
+ }
// don't include locale, that is also taken care of below
- if ("locale".equals(name)) continue;
+ if ("locale".equals(name)) {
+ continue;
+ }
// don't include timeZone, that is also taken care of below
- if ("timeZone".equals(name)) continue;
+ if ("timeZone".equals(name)) {
+ continue;
+ }
// don't include theme, that is also taken care of below
- if ("visualTheme".equals(name)) continue;
+ if ("visualTheme".equals(name)) {
+ continue;
+ }
Object value = null;
if (UtilValidate.isNotEmpty(modelParam.stringMapPrefix)) {
- Map<String, Object> paramMap =
UtilHttp.makeParamMapWithPrefix(request, multiPartMap,
modelParam.stringMapPrefix, null);
+ Map<String, Object> paramMap =
UtilHttp.makeParamMapWithPrefix(request, multiPartMap,
+ modelParam.stringMapPrefix, null);
value = paramMap;
- if (Debug.verboseOn()) Debug.logVerbose("Set [" +
modelParam.name + "]: " + paramMap, module);
+ if (Debug.verboseOn()) {
+ Debug.logVerbose("Set [" + modelParam.name + "]: " +
paramMap, module);
+ }
} else if (UtilValidate.isNotEmpty(modelParam.stringListSuffix)) {
- List<Object> paramList =
UtilHttp.makeParamListWithSuffix(request, multiPartMap,
modelParam.stringListSuffix, null);
+ List<Object> paramList =
UtilHttp.makeParamListWithSuffix(request, multiPartMap,
+ modelParam.stringListSuffix, null);
value = paramList;
} else {
// first check the multi-part map
value = multiPartMap.get(name);
- // next check attributes; do this before parameters so that
attribute which can be changed by code can override parameters which can't
+ // next check attributes; do this before parameters so that
attribute which can
+ // be changed by code can override parameters which can't
if (UtilValidate.isEmpty(value)) {
- Object tempVal =
request.getAttribute(UtilValidate.isEmpty(modelParam.requestAttributeName) ?
name : modelParam.requestAttributeName);
+ Object tempVal =
request.getAttribute(UtilValidate.isEmpty(modelParam.requestAttributeName) ?
name
+ : modelParam.requestAttributeName);
if (tempVal != null) {
value = tempVal;
}
@@ -159,17 +176,22 @@ public class ServiceEventHandler implements EventHandler {
// check the request parameters
if (UtilValidate.isEmpty(value)) {
- ServiceEventHandler.checkSecureParameter(requestMap,
urlOnlyParameterNames, name, session, serviceName, dctx.getDelegator());
+ ServiceEventHandler.checkSecureParameter(requestMap,
urlOnlyParameterNames, name, session,
+ serviceName, dctx.getDelegator());
- // if the service modelParam has allow-html="any" then get
this direct from the request instead of in the parameters Map so there will be
no canonicalization possibly messing things up
+ // if the service modelParam has allow-html="any" then get
this direct from the
+ // request instead of in the parameters Map so there will
be no canonicalization
+ // possibly messing things up
if ("any".equals(modelParam.allowHtml)) {
value = request.getParameter(name);
} else {
- // use the rawParametersMap from UtilHttp in order to
also get pathInfo parameters, do canonicalization, etc
+ // use the rawParametersMap from UtilHttp in order to
also get pathInfo
+ // parameters, do canonicalization, etc
value = rawParametersMap.get(name);
}
- // make any composite parameter data (e.g., from a set of
parameters {name_c_date, name_c_hour, name_c_minutes})
+ // make any composite parameter data (e.g., from a set of
parameters
+ // {name_c_date, name_c_hour, name_c_minutes})
if (value == null) {
value = UtilHttp.makeParamValueFromComposite(request,
name);
}
@@ -177,7 +199,9 @@ public class ServiceEventHandler implements EventHandler {
// then session
if (UtilValidate.isEmpty(value)) {
- Object tempVal =
request.getSession().getAttribute(UtilValidate.isEmpty(modelParam.sessionAttributeName)
? name : modelParam.sessionAttributeName);
+ Object tempVal = request.getSession()
+
.getAttribute(UtilValidate.isEmpty(modelParam.sessionAttributeName) ? name
+ : modelParam.sessionAttributeName);
if (tempVal != null) {
value = tempVal;
}
@@ -199,7 +223,8 @@ public class ServiceEventHandler implements EventHandler {
}
// get only the parameters for this service - converted to proper type
- // TODO: pass in a list for error messages, like could not convert
type or not a proper X, return immediately with messages if there are any
+ // TODO: pass in a list for error messages, like could not convert
type or not a
+ // proper X, return immediately with messages if there are any
List<Object> errorMessages = new LinkedList<>();
serviceContext = model.makeValid(serviceContext,
ModelService.IN_PARAM, true, errorMessages, timeZone, locale);
if (errorMessages.size() > 0) {
@@ -279,44 +304,69 @@ public class ServiceEventHandler implements EventHandler {
String resultKey = rme.getKey();
Object resultValue = rme.getValue();
- if (resultKey != null &&
!ModelService.RESPONSE_MESSAGE.equals(resultKey) &&
!ModelService.ERROR_MESSAGE.equals(resultKey) &&
- !ModelService.ERROR_MESSAGE_LIST.equals(resultKey) &&
!ModelService.ERROR_MESSAGE_MAP.equals(resultKey) &&
- !ModelService.SUCCESS_MESSAGE.equals(resultKey) &&
!ModelService.SUCCESS_MESSAGE_LIST.equals(resultKey)) {
+ if (resultKey != null &&
!ModelService.RESPONSE_MESSAGE.equals(resultKey)
+ && !ModelService.ERROR_MESSAGE.equals(resultKey)
+ && !ModelService.ERROR_MESSAGE_LIST.equals(resultKey)
+ && !ModelService.ERROR_MESSAGE_MAP.equals(resultKey)
+ && !ModelService.SUCCESS_MESSAGE.equals(resultKey)
+ &&
!ModelService.SUCCESS_MESSAGE_LIST.equals(resultKey)) {
request.setAttribute(resultKey, resultValue);
}
}
}
- if (Debug.verboseOn()) Debug.logVerbose("[Event Return]: " +
responseString, module);
+ if (Debug.verboseOn()) {
+ Debug.logVerbose("[Event Return]: " + responseString, module);
+ }
return responseString;
}
- public static void checkSecureParameter(RequestMap requestMap, Set<String>
urlOnlyParameterNames, String name, HttpSession session, String serviceName,
Delegator delegator) throws EventHandlerException {
- // special case for security: if this is a request-map defined as
secure in controller.xml then only accept body parameters coming in, ie don't
allow the insecure URL parameters
- // NOTE: the RequestHandler will check the HttpSerletRequest security
to make sure it is secure if the request-map -> security -> https=true,
- // but we can't just look at the request.isSecure() method here
because it is allowed to send secure requests for request-map with https=false
+ public static void checkSecureParameter(RequestMap requestMap, Set<String>
urlOnlyParameterNames, String name,
+ HttpSession session, String serviceName, Delegator delegator)
throws EventHandlerException {
+ // special case for security: if this is a request-map defined as
secure in
+ // controller.xml then only accept body parameters coming in, ie don't
allow the
+ // insecure URL parameters
+ // NOTE: the RequestHandler will check the HttpSerletRequest security
to make
+ // sure it is secure if the request-map -> security -> https=true,
+ // but we can't just look at the request.isSecure() method here
because it is
+ // allowed to send secure requests for request-map with https=false
if (requestMap != null && requestMap.securityHttps) {
if (urlOnlyParameterNames.contains(name)) {
String errMsg = "Found URL parameter [" + name + "] passed to
secure (https) request-map with uri ["
- + requestMap.uri + "] with an event that calls service ["
- + serviceName + "]; this is not allowed for security
reasons! The data should be encrypted by making it part of the request body "
- + "(a form field) instead of the request URL."
- + " Moreover it would be kind if you could create a Jira
sub-task of https://issues.apache.org/jira/browse/OFBIZ-2330 "
- + "(check before if a sub-task for this error does not
exist)."
- + " If you are not sure how to create a Jira issue please
have a look before at
https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices";
- + " Thank you in advance for your help.";
- Debug.logError("=============== " + errMsg + "; In session ["
+ ControlActivationEventListener.showSessionId(session) + "]; Note that this
can be changed using the service.http.parameters.require.encrypted property in
the url.properties file", module);
+ + requestMap.uri + "] with an event that calls service
[" + serviceName
+ + "]; this is not allowed for security reasons! The
data should be encrypted by making it "
+ + "part of the request body " + "(a form field)
instead of the request URL."
+ + " Moreover it would be kind if you could create a
Jira sub-task of "
+ + "https://issues.apache.org/jira/browse/OFBIZ-2330 "
+ + "(check before if a sub-task for this error does not
exist)."
+ + " If you are not sure how to create a Jira issue
please have a look before at "
+ +
"https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices";
+ + " Thank you in advance for your help.";
+ Debug.logError("=============== " + errMsg + "; In session ["
+ + ControlActivationEventListener.showSessionId(session)
+ + "]; Note that this can be changed using the
service.http.parameters.require.encrypted "
+ + "property in the url.properties file",
+ module);
// the default here is true, so anything but N/n is true
- boolean requireEncryptedServiceWebParameters =
!EntityUtilProperties.propertyValueEqualsIgnoreCase("url",
"service.http.parameters.require.encrypted", "N", delegator);
+ boolean requireEncryptedServiceWebParameters =
!EntityUtilProperties.propertyValueEqualsIgnoreCase(
+ "url", "service.http.parameters.require.encrypted",
"N", delegator);
- // NOTE: this forces service call event parameters to be in
the body and not in the URL! can be issues with existing links, like Delete
links or whatever, and those need to be changed to forms!
+ // NOTE: this forces service call event parameters to be in
the body and not in
+ // the URL! can be issues with existing links, like Delete
links or whatever,
+ // and those need to be changed to forms!
if (requireEncryptedServiceWebParameters) {
throw new EventHandlerException(errMsg);
}
}
- // NOTTODO: may want to allow parameters that map to entity PK
fields to be in the URL, but that might be a big security hole since there are
certain security sensitive entities that are made of only PK fields, or that
only need PK fields to function (like UserLoginSecurityGroup)
- // NOTTODO: we could allow URL parameters when it is not a POST
(ie when !request.getMethod().equalsIgnoreCase("POST")), but that would open a
security hole where sensitive parameters can be passed on the URL in a GET/etc
and bypass this security constraint
+ // NOTTODO: may want to allow parameters that map to entity PK
fields to be in
+ // the URL, but that might be a big security hole since there are
certain
+ // security sensitive entities that are made of only PK fields, or
that only
+ // need PK fields to function (like UserLoginSecurityGroup)
+ // NOTTODO: we could allow URL parameters when it is not a POST
(ie when
+ // !request.getMethod().equalsIgnoreCase("POST")), but that would
open a
+ // security hole where sensitive parameters can be passed on the
URL in a
+ // GET/etc and bypass this security constraint
}
}
}
