Use a shared JDOM builder for bookmark and configuration parsing - #173
Use a shared JDOM builder for bookmark and configuration parsing#173snoopdave wants to merge 2 commits into
Conversation
An XML document can name resources for the parser to fetch: a document type declaration can point at an external subset, and entity declarations can point at files or URLs. Resolving those makes the parser act for whoever wrote the document, which suits Roller's own descriptors and not documents it parses from user input. SafeSAXBuilder settles that once for every retained JDOM parser rather than per call site: the document type declaration is refused, external entity and DTD resolution is switched off, and entity expansion is disabled. The OPML bookmark import, the menu parser, the runtime config parser and the theme metadata parser all build through it. Roller's own descriptors carry no document type declaration, so nothing about how they parse changes. The two JAXP access properties are applied through the reader factory and tolerated when unrecognised, because the Xerces Roller ships rejects them at the SAX layer; the parser features are what carry the behaviour. Trackback.java is deliberately left alone. Claude-Session: https://claude.ai/code/session_01A1fhY1E2PCFU6UAPXu2WtV
Move the resource-resolution demonstrations out of the committed suite. The retained tests verify that ordinary documents still parse and that any document type declaration is refused. Claude-Session: https://claude.ai/code/session_01A1fhY1E2PCFU6UAPXu2WtV
mraible
left a comment
mraible
left a comment
There was a problem hiding this comment.
SafeSAXBuilder is the right shape (secure processing, disallow-doctype-decl, external entities and DTD loading off) and the four call sites are converted correctly. Approving with one sequencing condition and some notes:
Trackback.java:177still builds a barenew SAXBuilder()and parses a response body controlled by a remote server, then reflects<message>into the author's UI. The description says it's removed separately; that's #163, so this must merge after #163 (or convert that one call site too). Until then the XXE this PR closes is still open on that path.- The DOCTYPE refusal also applies to
theme.xml,runtimeConfigDefs.xmland the menu XML, not just OPML. A custom theme with a DOCTYPE now silently disappears at startup (ThemeManagerImplonly logs it). Worth a line in the release note alongside the OPML one. - The PR description's test paragraph still describes external-entity assertions that the second commit removed.
| * | ||
| * <p>Roller parses documents from user input and from its own menu, theme and | ||
| * configuration descriptors alike. Rather than track which parser is on which | ||
| * side, every retained JDOM parser is built here, and none of them resolve |
| ThemeMetadata theme = new ThemeMetadata(); | ||
|
|
||
| SAXBuilder builder = new SAXBuilder(); | ||
| SafeSAXBuilder builder = new SafeSAXBuilder(); |
There was a problem hiding this comment.
A theme.xml with a DOCTYPE is now refused, and ThemeManagerImpl.loadAllThemesFromDisk just logs "Problem processing theme", so the theme vanishes and its weblogs throw ThemeNotFoundException. Shipped themes have no DOCTYPE, so this only hits hand-written ones, but please mention it with the OPML note.
| try { | ||
| // Build JDOC document OPML string | ||
| SAXBuilder builder = new SAXBuilder(); | ||
| SafeSAXBuilder builder = new SafeSAXBuilder(); |
There was a problem hiding this comment.
BookmarksImport shows ex.toString(), so a user whose OPML export has <!DOCTYPE opml> now sees org.apache.roller.weblogger.WebloggerException: org.jdom2.input.JDOMParseException: ... DOCTYPE is disallowed when the feature .... Catching JDOMParseException here and wrapping it with a message like "OPML files with a DOCTYPE are not accepted" would tell them what to do.
| bookmarkManager().importBookmarks( | ||
| TestUtils.getManagedWebsite(testWeblog), folderName, opml); | ||
| TestUtils.endSession(true); | ||
| } catch (Exception expected) { |
There was a problem hiding this comment.
This catches everything, so the DOCTYPE test passes whenever the import fails for any reason (DB state, a getFolder regression), and the session is left un-ended when importBookmarks throws. Assert on WebloggerException with a JDOMParseException cause, and end the session in a finally.
| @Test | ||
| public void ordinaryOpmlStillImports() throws Exception { | ||
| byte[] opml = Files.readAllBytes( | ||
| new File("src/test/resources/bookmarks.opml").toPath()); |
There was a problem hiding this comment.
cwd-relative; BookmarkTest and FileContentManagerTest load the same fixture from the classpath.
| * outright, so they are applied here and a rejection is logged and passed | ||
| * over — the features above are what carry the guarantee. | ||
| */ | ||
| private static final class HardenedReaders implements XMLReaderJDOMFactory { |
There was a problem hiding this comment.
On this classpath SAXParserFactory.newInstance() resolves to Xerces 2.11 (via nekohtml), which throws SAXNotRecognizedException for both ACCESS_EXTERNAL_DTD and ACCESS_EXTERNAL_SCHEMA, so denyProtocol() always takes the swallowed-exception branch. With DOCTYPE refused there's nothing for those properties to restrict anyway; XMLReaders.NONVALIDATING plus the feature calls above is enough and this inner class can go.
| setFeature(EXTERNAL_GENERAL_ENTITIES, false); | ||
| setFeature(EXTERNAL_PARAMETER_ENTITIES, false); | ||
| setFeature(LOAD_EXTERNAL_DTD, false); | ||
|
|
There was a problem hiding this comment.
Nit: in jdom2 setExpandEntities(false) writes the same feature key as setFeature(EXTERNAL_GENERAL_ENTITIES, false) four lines up; one of the two is a no-op.
2 participants
Several call sites each construct their own JDOM
SAXBuilder, with inconsistentparser settings. This change consolidates them behind one shared,
consistently-configured builder.
What changed
SafeSAXBuilder(extendsSAXBuilder) configured with secure processingenabled, document type declarations disallowed, external entity and external
DTD resolution disabled, and entity expansion disabled.
(
MenuHelper,RuntimeConfigDefsParser,ThemeMetadataParser).Trackback.javauntouched; that file is removed separately.Note for the release notes: OPML documents carrying a DOCTYPE are now rejected
and will no longer import. OPML does not require a DOCTYPE.
Tests
SafeSAXBuilderTestasserts the parser contract directly, andBookmarkImportParsingTestexercises it through the import path, including thatordinary OPML still imports and that documents with external entities store no
entity content.