Increase limits for XML parsing #11126
Conversation
To prepare for https://bugs.openjdk.org/browse/JDK-8343022 See also https://docs.oracle.com/en/java/javase/23/docs/api/java.xml/module-summary.html#Conf for documentation on these properties languagetool-org#11125
WalkthroughThe pull request introduces security enhancements for XML parsing in two core LanguageTool classes: Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
languagetool-core/src/main/java/org/languagetool/rules/patterns/PatternRuleLoader.java (1)
76-78: Consider extracting XML security settings to a shared utilityThe XML security settings are duplicated between
PatternRuleLoaderandDisambiguationRuleLoader. Consider extracting these settings to a shared utility method to ensure consistency and ease maintenance.Create a new utility class/method like this:
public class XMLSecurityUtil { private static final String PROP_MAX_ENTITY_SIZE = "jdk.xml.maxGeneralEntitySizeLimit"; private static final String PROP_TOTAL_ENTITY_SIZE = "jdk.xml.totalEntitySizeLimit"; private static final String PROP_ENTITY_EXPANSION = "jdk.xml.entityExpansionLimit"; private static final String FEATURE_EXTERNAL_DTD = "http://apache.org/xml/features/nonvalidating/load-external-dtd"; public static void secureXMLReader(XMLReader reader) throws SAXException { reader.setFeature(FEATURE_EXTERNAL_DTD, false); reader.setProperty(PROP_MAX_ENTITY_SIZE, 0); reader.setProperty(PROP_TOTAL_ENTITY_SIZE, 0); reader.setProperty(PROP_ENTITY_EXPANSION, 0); } }Then use it in both classes:
XMLSecurityUtil.secureXMLReader(saxParser.getXMLReader());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
languagetool-core/src/main/java/org/languagetool/rules/patterns/PatternRuleLoader.java(1 hunks)languagetool-core/src/main/java/org/languagetool/tagging/disambiguation/rules/DisambiguationRuleLoader.java(1 hunks)
🔇 Additional comments (1)
languagetool-core/src/main/java/org/languagetool/rules/patterns/PatternRuleLoader.java (1)
76-78: Verify the impact of zero limits on valid XML files
While setting these limits to 0 provides maximum security, let's verify that this doesn't affect processing of legitimate XML files in the codebase.
✅ Verification successful
Let me check the actual content of these XML files to understand how they use entities.
Let me check the actual content of these external entity files to ensure they don't enable recursive entity expansion.
Let me check the content of a few representative entity files to understand how they're used.
Based on the analysis of the XML files and entity usage in the codebase, I can now provide a final response:
Setting XML entity limits to 0 is safe and recommended
The codebase only uses simple entity declarations for text substitution patterns (like <!ENTITY verbos_ambiguos "...">) and DTD definitions. There are no recursive entities, external entity references (except for well-defined local files in Portuguese module), or complex entity expansions that would be affected by these security limits:
- The entities are used for storing grammar rules, word lists, and language patterns
- No XML bomb patterns or dangerous entity expansion constructs were found
- External entities are only used for modular organization of language rules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for XML files that might be affected by these limits
echo "Searching for XML files with entity declarations..."
rg -l "<!ENTITY" --type xml
echo "Searching for XML files with SYSTEM or PUBLIC identifiers..."
rg -l "<!DOCTYPE.*?(SYSTEM|PUBLIC)" --type xml
Length of output: 7065
Script:
#!/bin/bash
# Check content of a few representative XML files
echo "Checking grammar.xml entity usage..."
rg "<!ENTITY" -A 2 "languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml"
echo -e "\nChecking false-friends.xml DTD..."
cat "languagetool-core/src/main/resources/org/languagetool/rules/false-friends.dtd"
echo -e "\nChecking for any recursive/nested entities..."
rg "<!ENTITY.*%.*SYSTEM" --type xml
Length of output: 46869
Script:
#!/bin/bash
# Check content of external entity files in Portuguese and Spanish modules
echo "Checking Portuguese entities..."
rg -l "<!ENTITY" "languagetool-language-modules/pt/src/main/resources/org/languagetool/resource/pt/entities/" || echo "No Portuguese entity files found"
echo -e "\nChecking Spanish entities..."
rg -l "<!ENTITY" "languagetool-language-modules/es/src/main/resources/org/languagetool/resource/es/entities.ent" || echo "No Spanish entity file found"
echo -e "\nChecking for any nested ENTITY definitions..."
rg "<!ENTITY.*%.*<!ENTITY" --type xml
Length of output: 1730
Script:
#!/bin/bash
# Check content of representative entity files
echo "Checking content of verbs.ent..."
head -n 10 "languagetool-language-modules/pt/src/main/resources/org/languagetool/resource/pt/entities/verbs.ent"
echo -e "\nChecking content of Spanish entities.ent..."
head -n 10 "languagetool-language-modules/es/src/main/resources/org/languagetool/resource/es/entities.ent"
echo -e "\nChecking for any entity expansion limits in XML files..."
rg "entityExpansionLimit|maxGeneralEntitySizeLimit|totalEntitySizeLimit" --type xml
Length of output: 22663
| saxParser.getXMLReader().setProperty("jdk.xml.maxGeneralEntitySizeLimit", 0); | ||
| saxParser.getXMLReader().setProperty("jdk.xml.totalEntitySizeLimit", 0); | ||
| saxParser.getXMLReader().setProperty("jdk.xml.entityExpansionLimit", 0); |
There was a problem hiding this comment.
Add protection against external DTD loading
While the entity expansion limits are good security measures, consider also disabling external DTD loading for consistent security with PatternRuleLoader. Additionally, consider adding error handling for the property settings.
Apply this diff to enhance security and robustness:
+ saxParser.getXMLReader().setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
saxParser.getXMLReader().setProperty("jdk.xml.maxGeneralEntitySizeLimit", 0);
saxParser.getXMLReader().setProperty("jdk.xml.totalEntitySizeLimit", 0);
saxParser.getXMLReader().setProperty("jdk.xml.entityExpansionLimit", 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| saxParser.getXMLReader().setProperty("jdk.xml.maxGeneralEntitySizeLimit", 0); | |
| saxParser.getXMLReader().setProperty("jdk.xml.totalEntitySizeLimit", 0); | |
| saxParser.getXMLReader().setProperty("jdk.xml.entityExpansionLimit", 0); | |
| saxParser.getXMLReader().setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); | |
| saxParser.getXMLReader().setProperty("jdk.xml.maxGeneralEntitySizeLimit", 0); | |
| saxParser.getXMLReader().setProperty("jdk.xml.totalEntitySizeLimit", 0); | |
| saxParser.getXMLReader().setProperty("jdk.xml.entityExpansionLimit", 0); |
There was a problem hiding this comment.
@cushon Thanks! Does coderabbit's suggestion make sense?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
While the entity expansion limits are good security measures, consider also disabling external DTD loading
@cushon Thanks! Does coderabbit's suggestion make sense?
I'm not sure--I have context about the JDK change, but less about the requirements of this project.
I can try adding the change to disable external DTD loading while we're here, but it's somewhat unrelated. I think coderabbit's is a bit confused--this isn't setting entity expansion limits, it's actually disabling the limits (setting 0 means 'unlimited').
Are the XML files parsed by this code are trusted, either because they're the ones included in this project, or provided by a user of the library, rather than being arbitrary user input?
There was a problem hiding this comment.
Are the XML files parsed by this code are trusted
Generally, yes. While there could be external entities, one (= a user who edits the XML files) would need to add them explicitly.
There was a problem hiding this comment.
Thanks, I went ahead and added the change to disable external DTD loading.
To prepare for https://bugs.openjdk.org/browse/JDK-8343022
See also https://docs.oracle.com/en/java/javase/23/docs/api/java.xml/module-summary.html#Conf for documentation on these properties
#11125
Summary by CodeRabbit
New Features
Bug Fixes
Documentation