Improved setup for XML parsing#1284
Conversation
labkey-jeckels
changed the title
labkey-alan
left a comment
labkey-alan
left a comment
There was a problem hiding this comment.
Overall this looks good to me, however, I did have to search the web to find an explanation for why we want to do this. I think it would be a good idea to add a comment explaining what we're doing and why. I also left a more specific comment, but I'm not sure it's important enough to prevent merging.
| SAXParser parser = SAXParserFactory.newDefaultInstance().newSAXParser(); | ||
| SAXParserFactory factory = SAXParserFactory.newDefaultInstance(); | ||
| factory.setFeature(javax.xml.XMLConstants.FEATURE_SECURE_PROCESSING, true); | ||
| factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); |
There was a problem hiding this comment.
According to this blog post I found from 2012 these settings seem to be mutually exclusive. My understanding is that you need to either disable doctype declaration (what this line does) or disable external entities (what the following two lines do). I'm not sure it's problematic to do both though, so maybe we should keep it.
https://blog.compass-security.com/2012/08/secure-xml-parser-configuration/
There was a problem hiding this comment.
Yes, should be safe to do both, just in case.
Added two-way pointers here and in XmlBeansUtil, improved comments, and made them more consistent. |
2 participants
Rationale
We should configure XML parsers consistently. Code in this repo can't leverage XmlBeanUtil so do it manually.
Changes