-
Notifications
You must be signed in to change notification settings - Fork 578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Disable access to external entities in XML parsing #2217
fix: Disable access to external entities in XML parsing #2217
Conversation
try { | ||
reader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); | ||
} catch (Exception e) { | ||
throw new IllegalArgumentException(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that it's not argument issue (this buildSAXReader()
method has no argument).
It is probably better to use another exception type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have modified the exception type and added an error message, please let me know if you had something else in mind or if this requires further adjustments
factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); | ||
factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); | ||
} catch (Exception e) { | ||
throw new IllegalArgumentException(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue here. 🙇♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please add a changelog entry into the CHANGELOG.md
file. We probably need a new ### Security
under the ## Unreleased
.
2ff2de8
to
4c3481c
Compare
Thanks for the review, I have rebased and added a security category in the changelog with a new entry for this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you! 🤝
Fixes security issues detected by sonarcloud such as: https://sonarcloud.io/project/issues?issues=AXDRMcroPCexr-BXKaE_&open=AXDRMcroPCexr-BXKaE_&id=com.github.spotbugs.spotbugs
Added methods in
XMLUtil
to setup XML objects with more secure options