Use a shared JDOM builder for bookmark and configuration parsing - #173
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
| 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.
mbien
left a comment
mbien
left a comment
There was a problem hiding this comment.
this could also set FEATURE_SECURE_PROCESSING most likely.
tests are green with it
| * beneath, so a parser configured elsewhere, or a JAXP implementation with | ||
| * different defaults, does not quietly reopen it. | ||
| */ | ||
| public class SafeSAXBuilder extends SAXBuilder { |
There was a problem hiding this comment.
interesting that Roller doesn't have DocumentBuilderFactory anywhere. It was brand new in 2002 :)
The factory already refuses a doctype declaration and external DTD loading. Secure processing is the JAXP-standard switch that sits under both, so set it alongside them rather than relying on the parser implementation's default. It is set last so that a parser which does not recognise the feature cannot stop the two settings above it from being applied. Claude-Session: https://claude.ai/code/session_019R1jdtwkaYEeA6L9DXEtEi
3 participants
Several call sites each construct their own JDOM SAXBuilder with inconsistent parser settings. This change consolidates them behind one shared, consistently configured builder.
What changed
Merge order
Merge after #163 so no older parser call site remains.
Release note
OPML and hand-written theme.xml documents carrying a DOCTYPE are now rejected; shipped themes do not use one.
Tests
SafeSAXBuilderTest asserts the shared parser contract. BookmarkImportParsingTest loads its fixture from the classpath, verifies ordinary OPML import, and checks the typed error and retained parse root cause for rejected declarations.
Local Temurin JDK 11 verification: 4 focused tests passed; the full reactor passed 162 tests with 1 skipped.