You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by ta...@apache.org on 2018/08/09 08:27:58 UTC
svn commit: r1837697 -
/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/XmlRpcEventHandler.java
Author: taher
Date: Thu Aug 9 08:27:57 2018
New Revision: 1837697
URL: http://svn.apache.org/viewvc?rev=1837697&view=rev
Log:
Improved: disable DTDs for XML-RPC requests
(OFBIZ-10509)
Disable any doctype declarations in XML-RPC requests to OFBiz to enhance
performance and security.
Modified:
ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/XmlRpcEventHandler.java
Modified: ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/XmlRpcEventHandler.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/XmlRpcEventHandler.java?rev=1837697&r1=1837696&r2=1837697&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/XmlRpcEventHandler.java (original)
+++ ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/XmlRpcEventHandler.java Thu Aug 9 08:27:57 2018
@@ -29,6 +29,7 @@ import java.io.InputStreamReader;
import java.io.OutputStream;
import java.io.Writer;
import java.util.HashMap;
+import java.util.List;
import java.util.Locale;
import java.util.Map;
@@ -52,15 +53,21 @@ import org.apache.ofbiz.webapp.control.C
import org.apache.xmlrpc.XmlRpcException;
import org.apache.xmlrpc.XmlRpcHandler;
import org.apache.xmlrpc.XmlRpcRequest;
+import org.apache.xmlrpc.XmlRpcRequestConfig;
import org.apache.xmlrpc.common.ServerStreamConnection;
import org.apache.xmlrpc.common.XmlRpcHttpRequestConfig;
import org.apache.xmlrpc.common.XmlRpcHttpRequestConfigImpl;
import org.apache.xmlrpc.common.XmlRpcStreamRequestConfig;
+import org.apache.xmlrpc.parser.XmlRpcRequestParser;
import org.apache.xmlrpc.server.AbstractReflectiveHandlerMapping;
import org.apache.xmlrpc.server.XmlRpcHttpServer;
import org.apache.xmlrpc.server.XmlRpcHttpServerConfig;
import org.apache.xmlrpc.server.XmlRpcNoSuchHandlerException;
import org.apache.xmlrpc.util.HttpUtil;
+import org.apache.xmlrpc.util.SAXParsers;
+import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
+import org.xml.sax.XMLReader;
/**
* XmlRpcEventHandler
@@ -265,6 +272,37 @@ public class XmlRpcEventHandler extends
}
}
+ protected XmlRpcRequest getRequest(final XmlRpcStreamRequestConfig pConfig, InputStream pStream)
+ throws XmlRpcException {
+ final XmlRpcRequestParser parser = new XmlRpcRequestParser(pConfig, getTypeFactory());
+ final XMLReader xr = SAXParsers.newXMLReader();
+ xr.setContentHandler(parser);
+ try {
+ xr.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+ xr.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+ xr.setFeature("http://xml.org/sax/features/external-general-entities", false);
+ xr.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+ xr.parse(new InputSource(pStream));
+ } catch (SAXException | IOException e) {
+ throw new XmlRpcException("Failed to parse / read XML-RPC request: " + e.getMessage(), e);
+ }
+ final List<?> params = parser.getParams();
+ return new XmlRpcRequest() {
+ public XmlRpcRequestConfig getConfig() {
+ return pConfig;
+ }
+ public String getMethodName() {
+ return parser.getMethodName();
+ }
+ public int getParameterCount() {
+ return params == null ? 0 : params.size();
+ }
+ public Object getParameter(int pIndex) {
+ return params.get(pIndex);
+ }
+ };
+ }
+
class ServiceRpcHandler extends AbstractReflectiveHandlerMapping implements XmlRpcHandler {
public ServiceRpcHandler() {
Re: svn commit: r1837697 - /ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/XmlRpcEventHandler.java
Posted by Taher Alkhateeb <sl...@gmail.com>.
Hi Mathieu,
To make it short, I did not look deeply into this code nor did I care to.
This was essentially a copy-paste of the parent getRequest method which
belongs to XML-RPC library. My opinion is that the whole thing is ugly but
I'm not necessairly interested in refactoring. The reason why I copied it
into the child class is to get my hands on the parser so I can enforce
certain attributes to disable DTDs. I would rather not touch the code
beyond that because it is not maintained and might cause other issues.
However if you feel comfortable and have the time to refactor properly,
then please issue a JIRA and I would gladly assist in pushing your code.
On Fri, Aug 10, 2018, 11:54 AM Mathieu Lirzin <ma...@nereide.fr>
wrote:
> Hello Taher,
>
> Just few nitpicks.
>
> taher@apache.org writes:
>
> > + protected XmlRpcRequest getRequest(final XmlRpcStreamRequestConfig
> pConfig, InputStream pStream)
> > + throws XmlRpcException {
> > + final XmlRpcRequestParser parser = new
> XmlRpcRequestParser(pConfig, getTypeFactory());
> > + final XMLReader xr = SAXParsers.newXMLReader();
> > + xr.setContentHandler(parser);
> > + try {
> > + xr.setFeature("
> http://apache.org/xml/features/disallow-doctype-decl", true);
> > + xr.setFeature("
> http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
> > + xr.setFeature("
> http://xml.org/sax/features/external-general-entities", false);
> > + xr.setFeature("
> http://xml.org/sax/features/external-parameter-entities", false);
> > + xr.parse(new InputSource(pStream));
> > + } catch (SAXException | IOException e) {
> > + throw new XmlRpcException("Failed to parse / read XML-RPC
> request: " + e.getMessage(), e);
> > + }
> > + final List<?> params = parser.getParams();
> > + return new XmlRpcRequest() {
> > + public XmlRpcRequestConfig getConfig() {
> > + return pConfig;
> > + }
> > + public String getMethodName() {
> > + return parser.getMethodName();
> > + }
> > + public int getParameterCount() {
> > + return params == null ? 0 : params.size();
> > + }
> > + public Object getParameter(int pIndex) {
> > + return params.get(pIndex);
> > + }
> > + };
> > + }
> > +
>
> I would recommend against using explicit ‘final’ outside of class
> fields where it conveys the semantic of immutable objects which is
> meaningful since object are inherently stateful. In the case of method
> parameter and local variables I think being explicit about the
> immutability of the reference is a bit noisy IMHO. I like the citation
> of Remi Forax [1] where in one of his french version of “Design Pattern
> Reloaded” presentation [2] said “I want to read code, not to read
> ‘final’”. :-)
>
> besides that, I have found ‘XmlRpcClientRequestImpl’ which is a default
> implementation of ‘XmlRpcRequest’ which can be used to avoid the
> anonymous class.
>
> Here is a cosmetic *untested* patch applying those two recommendations.
>
>
> Feel free to do whatever you want with it. I am not sure to understand
> what is the purpose of the ‘getRequest’ method so it would be nice if
> you could provide a docstring for it.
>
> Thanks.
>
> [1] https://forax.github.io/
> [2] https://www.youtube.com/watch?v=aC1wGHDOQic
>
> --
> Mathieu Lirzin
> GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37
>
Re: svn commit: r1837697 - /ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/XmlRpcEventHandler.java
Posted by Mathieu Lirzin <ma...@nereide.fr>.
Hello Taher,
Just few nitpicks.
taher@apache.org writes:
> + protected XmlRpcRequest getRequest(final XmlRpcStreamRequestConfig pConfig, InputStream pStream)
> + throws XmlRpcException {
> + final XmlRpcRequestParser parser = new XmlRpcRequestParser(pConfig, getTypeFactory());
> + final XMLReader xr = SAXParsers.newXMLReader();
> + xr.setContentHandler(parser);
> + try {
> + xr.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
> + xr.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
> + xr.setFeature("http://xml.org/sax/features/external-general-entities", false);
> + xr.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
> + xr.parse(new InputSource(pStream));
> + } catch (SAXException | IOException e) {
> + throw new XmlRpcException("Failed to parse / read XML-RPC request: " + e.getMessage(), e);
> + }
> + final List<?> params = parser.getParams();
> + return new XmlRpcRequest() {
> + public XmlRpcRequestConfig getConfig() {
> + return pConfig;
> + }
> + public String getMethodName() {
> + return parser.getMethodName();
> + }
> + public int getParameterCount() {
> + return params == null ? 0 : params.size();
> + }
> + public Object getParameter(int pIndex) {
> + return params.get(pIndex);
> + }
> + };
> + }
> +
I would recommend against using explicit ‘final’ outside of class
fields where it conveys the semantic of immutable objects which is
meaningful since object are inherently stateful. In the case of method
parameter and local variables I think being explicit about the
immutability of the reference is a bit noisy IMHO. I like the citation
of Remi Forax [1] where in one of his french version of “Design Pattern
Reloaded” presentation [2] said “I want to read code, not to read
‘final’”. :-)
besides that, I have found ‘XmlRpcClientRequestImpl’ which is a default
implementation of ‘XmlRpcRequest’ which can be used to avoid the
anonymous class.
Here is a cosmetic *untested* patch applying those two recommendations.