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

ICU-22888 Enhance XML Processor Configuration for Tools Directory in ICU4J Package #3243

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vignesh28s1995
Copy link

@vignesh28s1995 vignesh28s1995 commented Oct 17, 2024

Enhance XML Processor Configuration for Tools Directory in ICU4J Package

Checklist

  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22888
  • Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@CLAassistant
Copy link

CLAassistant commented Oct 17, 2024

CLA assistant check
All committers have signed the CLA.

@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

e.printStackTrace();
System.exit(-1);
}
// Set secure processing features to avoid XXE attacks
Copy link
Contributor

@mihnita mihnita Nov 7, 2024

Choose a reason for hiding this comment

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

Can you please wrap only this new configuration section in a try - catch block?

It is more consistent with what the rest of the method does (there are try blocks starting at lines 433 and 470).
And we don't end up with nested try blocks, and everything indented.

Something like this:

Before:

        DocumentBuilderFactory dfactory = DocumentBuilderFactory.newInstance();
        dfactory.setNamespaceAware(true);
        Document doc = null;

After:

        DocumentBuilderFactory dfactory = DocumentBuilderFactory.newInstance();

        try {
            // Set secure processing features to avoid XXE attacks
            dfactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
            dfactory.setNamespaceAware(true);

            // Disable access to external DTDs and entities to mitigate XXE attacks
            dfactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
            dfactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
            dfactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
            dfactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
        } catch (ParserConfigurationException e) {
            System.err.println("ERROR: Parser configuration error: " + e.getMessage());
            System.exit(-1);
        }        

        Document doc = null;

Thank you,
Mihai

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.

3 participants