Skip to content

Commit

Permalink
Improved: Lint ‘ServiceEventHandler’ class
Browse files Browse the repository at this point in the history 
(OFBIZ-11260)
  • Loading branch information
@mthl
Samuel Trégouët authored and mthl committed Nov 4, 2019
1 parent 6f5924c commit f93b9ea
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 38 deletions.
2 changes: 1 addition & 1 deletion build.gradle
View file
Expand Up @@ -307,7 +307,7 @@ checkstyle {
// the sum of errors that were present before introducing the // the sum of errors that were present before introducing the
// ‘checkstyle’ tool present in the framework and in the official // ‘checkstyle’ tool present in the framework and in the official
// plugins. // plugins.
tasks.checkstyleMain.maxErrors = 37824 tasks.checkstyleMain.maxErrors = 37781
// Currently there are a lot of errors so we need to temporarily // Currently there are a lot of errors so we need to temporarily
// hide them to avoid polluting the terminal output. // hide them to avoid polluting the terminal output.
showViolations = false showViolations = false
Expand Down
124 changes: 87 additions & 37 deletions framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceEventHandler.java
View file
Expand Up @@ -65,7 +65,8 @@ public void init(ServletContext context) throws EventHandlerException {
} }


@Override @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 // make sure we have a valid reference to the Service Engine
LocalDispatcher dispatcher = (LocalDispatcher) request.getAttribute("dispatcher"); LocalDispatcher dispatcher = (LocalDispatcher) request.getAttribute("dispatcher");
if (dispatcher == null) { if (dispatcher == null) {
Expand All @@ -91,7 +92,9 @@ public String invoke(Event event, RequestMap requestMap, HttpServletRequest requ
if (serviceName == null) { if (serviceName == null) {
throw new EventHandlerException("Service name (eventMethod) cannot be 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 // some needed info for when running the service
Locale locale = UtilHttp.getLocale(request); Locale locale = UtilHttp.getLocale(request);
Expand Down Expand Up @@ -129,55 +132,76 @@ public String invoke(Event event, RequestMap requestMap, HttpServletRequest requ
String name = modelParam.name; String name = modelParam.name;


// don't include userLogin, that's taken care of below // 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 // 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 // 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 // don't include theme, that is also taken care of below
if ("visualTheme".equals(name)) continue; if ("visualTheme".equals(name)) {
continue;
}


Object value = null; Object value = null;
if (UtilValidate.isNotEmpty(modelParam.stringMapPrefix)) { 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; 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)) { } 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; value = paramList;
} else { } else {
// first check the multi-part map // first check the multi-part map
value = multiPartMap.get(name); 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)) { 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) { if (tempVal != null) {
value = tempVal; value = tempVal;
} }
} }


// check the request parameters // check the request parameters
if (UtilValidate.isEmpty(value)) { 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)) { if ("any".equals(modelParam.allowHtml)) {
value = request.getParameter(name); value = request.getParameter(name);
} else { } 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); 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) { if (value == null) {
value = UtilHttp.makeParamValueFromComposite(request, name); value = UtilHttp.makeParamValueFromComposite(request, name);
} }
} }


// then session // then session
if (UtilValidate.isEmpty(value)) { 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) { if (tempVal != null) {
value = tempVal; value = tempVal;
} }
Expand All @@ -199,7 +223,8 @@ public String invoke(Event event, RequestMap requestMap, HttpServletRequest requ
} }


// get only the parameters for this service - converted to proper type // 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<>(); List<Object> errorMessages = new LinkedList<>();
serviceContext = model.makeValid(serviceContext, ModelService.IN_PARAM, true, errorMessages, timeZone, locale); serviceContext = model.makeValid(serviceContext, ModelService.IN_PARAM, true, errorMessages, timeZone, locale);
if (errorMessages.size() > 0) { if (errorMessages.size() > 0) {
Expand Down Expand Up @@ -279,44 +304,69 @@ public String invoke(Event event, RequestMap requestMap, HttpServletRequest requ
String resultKey = rme.getKey(); String resultKey = rme.getKey();
Object resultValue = rme.getValue(); Object resultValue = rme.getValue();


if (resultKey != null && !ModelService.RESPONSE_MESSAGE.equals(resultKey) && !ModelService.ERROR_MESSAGE.equals(resultKey) && if (resultKey != null && !ModelService.RESPONSE_MESSAGE.equals(resultKey)
!ModelService.ERROR_MESSAGE_LIST.equals(resultKey) && !ModelService.ERROR_MESSAGE_MAP.equals(resultKey) && && !ModelService.ERROR_MESSAGE.equals(resultKey)
!ModelService.SUCCESS_MESSAGE.equals(resultKey) && !ModelService.SUCCESS_MESSAGE_LIST.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); request.setAttribute(resultKey, resultValue);
} }
} }
} }


if (Debug.verboseOn()) Debug.logVerbose("[Event Return]: " + responseString, module); if (Debug.verboseOn()) {
Debug.logVerbose("[Event Return]: " + responseString, module);
}
return responseString; return responseString;
} }


public static void checkSecureParameter(RequestMap requestMap, Set<String> urlOnlyParameterNames, String name, HttpSession session, String serviceName, Delegator delegator) throws EventHandlerException { public static void checkSecureParameter(RequestMap requestMap, Set<String> urlOnlyParameterNames, String name,
// 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 HttpSession session, String serviceName, Delegator delegator) throws EventHandlerException {
// NOTE: the RequestHandler will check the HttpSerletRequest security to make sure it is secure if the request-map -> security -> https=true, // special case for security: if this is a request-map defined as secure in
// 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 // 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 (requestMap != null && requestMap.securityHttps) {
if (urlOnlyParameterNames.contains(name)) { if (urlOnlyParameterNames.contains(name)) {
String errMsg = "Found URL parameter [" + name + "] passed to secure (https) request-map with uri [" String errMsg = "Found URL parameter [" + name + "] passed to secure (https) request-map with uri ["
+ requestMap.uri + "] with an event that calls service [" + requestMap.uri + "] with an event that calls service [" + serviceName
+ serviceName + "]; this is not allowed for security reasons! The data should be encrypted by making it part of the request body " + "]; this is not allowed for security reasons! The data should be encrypted by making it "
+ "(a form field) instead of the request URL." + "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 " + " Moreover it would be kind if you could create a Jira sub-task of "
+ "(check before if a sub-task for this error does not exist)." + "https://issues.apache.org/jira/browse/OFBIZ-2330 "
+ " 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" + "(check before if a sub-task for this error does not exist)."
+ " Thank you in advance for your help."; + " If you are not sure how to create a Jira issue please have a look before at "
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); + "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 // 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) { if (requireEncryptedServiceWebParameters) {
throw new EventHandlerException(errMsg); 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: may want to allow parameters that map to entity PK fields to be in
// 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 // 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
} }
} }
} }

0 comments on commit f93b9ea

Please sign in to comment.