Skip to content
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

[DOXIA-716] Update and unify XMLReader creation and configuration #187

Merged
merged 1 commit into from
Dec 30, 2023

Conversation

michael-o
Copy link
Member

This closes #187

Copy link
Member

@kwin kwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@slachiewicz slachiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me.
While we are here maybe something for parsing and later for validation can be hardened:
https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html

@michael-o
Copy link
Member Author

looks good to me. While we are here maybe something for parsing and later for validation can be hardened: https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html

This should be separate, please file a JIRA issue.

@michael-o michael-o requested a review from kwin December 30, 2023 13:05
*/
private XMLReader getXmlReader(boolean hasDtdAndXsd) throws SAXException {
public XMLReader getXmlReader() throws SAXException, ParserConfigurationException {
if (xmlReader == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if caching is wise here, as SAXParser/SAXParserFactory is not not thread-safe AFAIK (https://docs.oracle.com/cd/E17802_01/webservices/webservices/docs/1.5/api/javax/xml/parsers/SAXParserFactory.html). At least this should be documented in the javadoc that the returned XMLReader needs to be synchronized somehow to be used among multiple threads.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections, but that would be a behavioral change and should be a separate issue.

@asfgit asfgit merged commit e43b200 into master Dec 30, 2023
10 of 15 checks passed
@michael-o michael-o deleted the DOXIA-716 branch December 30, 2023 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants