XCOMMONS-1503: workaround to use dom4j 2.1.1 #1110
Conversation
When dom4j's SAXReader.read(InputSource) is called (as PdfExportImpl.java does), in the end SAXHelper.createXMLReader(boolean) is called, which in dom4j 2.1.1 sets the "http://apache.org/xml/features/nonvalidating/load-external-dtd" feature to false. This PR sets the feature to true as a workaround. Perhaps this is going to be fixed upstream, in which case this PR could be reverted.
|
Thanks so much @carlosame for looking into this! I'll apply and add a comment to point to the issue at dom4j/dom4j#51 |
| @@ -327,6 +327,7 @@ String applyCSS(String html, String css, XWikiContext context) | |||
| Reader re = new StringReader(html); | |||
| InputSource source = new InputSource(re); | |||
| SAXReader reader = new SAXReader(XHTMLDocumentFactory.getInstance()); | |||
| reader.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", true); | |||
There was a problem hiding this comment.
I would just add a comment explaining why this is needed.
|
However, we're releasing XWiki 11.4RC1 today so I'll wait for master to become 11.5-SNAPSHOT first ;) |
|
Thanks for debugging this @carlosame. Just added a minor comment but better wait for the 11.4RC1 release before applying it. |
|
My PR had a link to the dom4j/dom4j/#51 issue at the end of the commit message, but I no longer see it. |
Seems you did it on https://github.com/carlosame/xwiki-platform/pull/1 (but in the PR comment, not the commit one) but not in this one maybe. |
|
It was my first github PR and I possibly messed things up a bit. The initial PR was probably unnecessary but I pressed the "create PR" button and ended up there. |
|
Merged and added a comment. Thanks for your contribution ! Note that I just noticed the commit was containing the wrong jira issue (it should contains the one for which it's committed which is actually XCOMMONS-1585). |
True, XCOMMONS-1503 was all about XCOMMONS-1585, thank you. Now let's hope that upstream fixes this and the commit can be reverted. |
I doubt this is going to happen since this change seems to be the main reason for 2.1.1 (security bulletproofing). |
|
Yes, advice from security researchers was blindly followed. Quoting Morgan and Al Ibrahim's "XML Schema, DTD, and Entity Attacks" in section "Recommendations For Developers":
The fact that it breaks applications and produces data loss is not a problem, it seems. Still, the dom4j issue is marked as a 'bug', so there is hope... |
When dom4j's SAXReader.read(InputSource) is called (as PdfExportImpl.java does), in the end SAXHelper.createXMLReader(boolean) is called, which in dom4j 2.1.1 sets the "http://apache.org/xml/features/nonvalidating/load-external-dtd" feature to false.
This PR sets the feature to true as a workaround. Perhaps this is going to be fixed upstream, in which case this PR could be reverted.