From 285af0d5225e9ce36316eb771fa1afa107aef1c2 Mon Sep 17 00:00:00 2001 From: "David M. Johnson" Date: Sat, 29 Aug 2026 11:30:15 -0400 Subject: [PATCH 1/2] Use a shared JDOM builder for bookmark and configuration parsing 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 --- .../business/jpa/JPABookmarkManagerImpl.java | 4 +- .../business/themes/ThemeMetadataParser.java | 4 +- .../runtime/RuntimeConfigDefsParser.java | 4 +- .../ui/core/util/menu/MenuHelper.java | 4 +- .../roller/weblogger/util/SafeSAXBuilder.java | 127 +++++++++++ .../business/BookmarkImportParsingTest.java | 203 ++++++++++++++++++ .../weblogger/util/SafeSAXBuilderTest.java | 147 +++++++++++++ 7 files changed, 485 insertions(+), 8 deletions(-) create mode 100644 app/src/main/java/org/apache/roller/weblogger/util/SafeSAXBuilder.java create mode 100644 app/src/test/java/org/apache/roller/weblogger/business/BookmarkImportParsingTest.java create mode 100644 app/src/test/java/org/apache/roller/weblogger/util/SafeSAXBuilderTest.java diff --git a/app/src/main/java/org/apache/roller/weblogger/business/jpa/JPABookmarkManagerImpl.java b/app/src/main/java/org/apache/roller/weblogger/business/jpa/JPABookmarkManagerImpl.java index 5b4224e09c..9c4b856068 100644 --- a/app/src/main/java/org/apache/roller/weblogger/business/jpa/JPABookmarkManagerImpl.java +++ b/app/src/main/java/org/apache/roller/weblogger/business/jpa/JPABookmarkManagerImpl.java @@ -34,7 +34,7 @@ import org.apache.roller.weblogger.pojos.Weblog; import org.jdom2.Document; import org.jdom2.Element; -import org.jdom2.input.SAXBuilder; +import org.apache.roller.weblogger.util.SafeSAXBuilder; /* * JPABookmarkManagerImpl.java @@ -142,7 +142,7 @@ public void importBookmarks( try { // Build JDOC document OPML string - SAXBuilder builder = new SAXBuilder(); + SafeSAXBuilder builder = new SafeSAXBuilder(); StringReader reader = new StringReader( opml ); Document doc = builder.build( reader ); diff --git a/app/src/main/java/org/apache/roller/weblogger/business/themes/ThemeMetadataParser.java b/app/src/main/java/org/apache/roller/weblogger/business/themes/ThemeMetadataParser.java index bef2ca50a0..0fb84f6a62 100644 --- a/app/src/main/java/org/apache/roller/weblogger/business/themes/ThemeMetadataParser.java +++ b/app/src/main/java/org/apache/roller/weblogger/business/themes/ThemeMetadataParser.java @@ -25,7 +25,7 @@ import org.jdom2.Document; import org.jdom2.Element; import org.jdom2.JDOMException; -import org.jdom2.input.SAXBuilder; +import org.apache.roller.weblogger.util.SafeSAXBuilder; import java.io.IOException; import java.io.InputStream; @@ -52,7 +52,7 @@ public ThemeMetadata unmarshall(InputStream instream) ThemeMetadata theme = new ThemeMetadata(); - SAXBuilder builder = new SAXBuilder(); + SafeSAXBuilder builder = new SafeSAXBuilder(); Document doc = builder.build(instream); // start at root and get theme id, name, description and author diff --git a/app/src/main/java/org/apache/roller/weblogger/config/runtime/RuntimeConfigDefsParser.java b/app/src/main/java/org/apache/roller/weblogger/config/runtime/RuntimeConfigDefsParser.java index ad2b95e486..02b2119489 100644 --- a/app/src/main/java/org/apache/roller/weblogger/config/runtime/RuntimeConfigDefsParser.java +++ b/app/src/main/java/org/apache/roller/weblogger/config/runtime/RuntimeConfigDefsParser.java @@ -29,7 +29,7 @@ import org.jdom2.Document; import org.jdom2.Element; import org.jdom2.JDOMException; -import org.jdom2.input.SAXBuilder; +import org.apache.roller.weblogger.util.SafeSAXBuilder; /** @@ -57,7 +57,7 @@ public RuntimeConfigDefs unmarshall(InputStream instream) RuntimeConfigDefs configs = new RuntimeConfigDefs(); - SAXBuilder builder = new SAXBuilder(); + SafeSAXBuilder builder = new SafeSAXBuilder(); Document doc = builder.build(instream); Element root = doc.getRootElement(); diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/core/util/menu/MenuHelper.java b/app/src/main/java/org/apache/roller/weblogger/ui/core/util/menu/MenuHelper.java index cafe178c58..381e53cd2b 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/core/util/menu/MenuHelper.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/core/util/menu/MenuHelper.java @@ -42,7 +42,7 @@ import org.jdom2.Document; import org.jdom2.Element; import org.jdom2.JDOMException; -import org.jdom2.input.SAXBuilder; +import org.apache.roller.weblogger.util.SafeSAXBuilder; /** * A helper class for dealing with UI menus. @@ -332,7 +332,7 @@ private static ParsedMenu unmarshall(String menuId, InputStream instream) ParsedMenu config = new ParsedMenu(); - SAXBuilder builder = new SAXBuilder(); + SafeSAXBuilder builder = new SafeSAXBuilder(); Document doc = builder.build(instream); Element root = doc.getRootElement(); diff --git a/app/src/main/java/org/apache/roller/weblogger/util/SafeSAXBuilder.java b/app/src/main/java/org/apache/roller/weblogger/util/SafeSAXBuilder.java new file mode 100644 index 0000000000..f8b1ca4187 --- /dev/null +++ b/app/src/main/java/org/apache/roller/weblogger/util/SafeSAXBuilder.java @@ -0,0 +1,127 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. The ASF licenses this file to You + * under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or + * implied. See the License for the specific language governing + * permissions and limitations under the License. For additional + * information regarding copyright in this work, please see the NOTICE + * file in the top level directory of this distribution. + */ + +package org.apache.roller.weblogger.util; + +import javax.xml.XMLConstants; +import javax.xml.parsers.SAXParserFactory; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.jdom2.JDOMException; +import org.jdom2.input.SAXBuilder; +import org.jdom2.input.sax.XMLReaderJDOMFactory; +import org.xml.sax.XMLReader; + +/** + * A {@link SAXBuilder} that treats a document strictly as data. + * + *

An XML document can name resources for the parser to go and read: 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 + * on behalf of whoever wrote the document, which is only appropriate when the + * document is Roller's own. + * + *

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 + * anything. Roller's own descriptors carry no document type declaration, so the + * strict setting costs them nothing. + * + *

The settings overlap deliberately. Refusing the declaration outright is + * what does the work; the remaining ones close the same door at the layers + * beneath, so a parser configured elsewhere, or a JAXP implementation with + * different defaults, does not quietly reopen it. + */ +public class SafeSAXBuilder extends SAXBuilder { + + /** Xerces feature names, honoured by the JDK's own parser. */ + private static final String DISALLOW_DOCTYPE = + "http://apache.org/xml/features/disallow-doctype-decl"; + private static final String EXTERNAL_GENERAL_ENTITIES = + "http://xml.org/sax/features/external-general-entities"; + private static final String EXTERNAL_PARAMETER_ENTITIES = + "http://xml.org/sax/features/external-parameter-entities"; + private static final String LOAD_EXTERNAL_DTD = + "http://apache.org/xml/features/nonvalidating/load-external-dtd"; + + private static final Log LOG = LogFactory.getLog(SafeSAXBuilder.class); + + public SafeSAXBuilder() { + super(new HardenedReaders()); + + // Secure processing is set explicitly rather than relied on. It is on + // by default in current JDKs, but that default limits resource + // consumption; it does not by itself stop external resolution. + setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + + // A document that declares a doctype is refused. Everything an entity + // could name has to be declared first, so this is the setting the rest + // stand behind. + setFeature(DISALLOW_DOCTYPE, true); + + setFeature(EXTERNAL_GENERAL_ENTITIES, false); + setFeature(EXTERNAL_PARAMETER_ENTITIES, false); + setFeature(LOAD_EXTERNAL_DTD, false); + + setExpandEntities(false); + } + + /** + * Supplies the reader, so that the two access properties can be applied + * where a parser that does not recognise them can be tolerated. + * + *

They are JAXP properties rather than SAX ones, and Roller ships its + * own Xerces, which rejects them outright at the SAX layer. Setting them + * through the builder would therefore fail every parse. They are still + * worth setting where they are understood, because they deny the protocols + * 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 { + + @Override + public XMLReader createXMLReader() throws JDOMException { + try { + SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setNamespaceAware(true); + factory.setValidating(false); + XMLReader reader = factory.newSAXParser().getXMLReader(); + denyProtocol(reader, XMLConstants.ACCESS_EXTERNAL_DTD); + denyProtocol(reader, XMLConstants.ACCESS_EXTERNAL_SCHEMA); + return reader; + } catch (Exception ex) { + throw new JDOMException("Unable to create an XML reader", ex); + } + } + + private void denyProtocol(XMLReader reader, String property) { + try { + reader.setProperty(property, ""); + } catch (Exception unsupported) { + LOG.debug("XML reader does not recognise " + property + + "; the parser features are what constrain resolution", unsupported); + } + } + + @Override + public boolean isValidating() { + return false; + } + } +} diff --git a/app/src/test/java/org/apache/roller/weblogger/business/BookmarkImportParsingTest.java b/app/src/test/java/org/apache/roller/weblogger/business/BookmarkImportParsingTest.java new file mode 100644 index 0000000000..f7c570fe6f --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/business/BookmarkImportParsingTest.java @@ -0,0 +1,203 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. The ASF licenses this file to You + * under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or + * implied. See the License for the specific language governing + * permissions and limitations under the License. For additional + * information regarding copyright in this work, please see the NOTICE + * file in the top level directory of this distribution. + */ +package org.apache.roller.weblogger.business; + +import java.io.File; +import java.net.ServerSocket; +import java.net.Socket; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.roller.weblogger.TestUtils; +import org.apache.roller.weblogger.pojos.User; +import org.apache.roller.weblogger.pojos.Weblog; +import org.apache.roller.weblogger.pojos.WeblogBookmark; +import org.apache.roller.weblogger.pojos.WeblogBookmarkFolder; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Covers what the OPML bookmark import will resolve while parsing. + * + *

The OPML document is supplied by a weblog administrator, a role Roller + * treats as untrusted. The parser must therefore take the document as data: + * the declarations in it name resources, and naming a resource must not cause + * the server to go and read it. + */ +public class BookmarkImportParsingTest { + + private static final Log log = LogFactory.getLog(BookmarkImportParsingTest.class); + + private User testUser = null; + private Weblog testWeblog = null; + private final String folderName = "ZZZ_import_parsing_ZZZ"; + + @BeforeEach + public void setUp() throws Exception { + TestUtils.setupWeblogger(); + testUser = TestUtils.setupUser("importParsingTestUser"); + testWeblog = TestUtils.setupWeblog("importParsingTestWeblog", testUser); + TestUtils.endSession(true); + } + + @AfterEach + public void tearDown() throws Exception { + try { + TestUtils.teardownWeblog(testWeblog.getId()); + TestUtils.teardownUser(testUser.getUserName()); + TestUtils.endSession(true); + } catch (Exception ex) { + log.error("ERROR in tearDown", ex); + } + } + + private BookmarkManager bookmarkManager() { + return WebloggerFactory.getWeblogger().getBookmarkManager(); + } + + /** @return the bookmarks imported into the test folder, empty if none */ + private java.util.List importedBookmarks() throws Exception { + testWeblog = TestUtils.getManagedWebsite(testWeblog); + WeblogBookmarkFolder folder = bookmarkManager().getFolder(testWeblog, folderName); + if (folder == null) { + return java.util.Collections.emptyList(); + } + return folder.retrieveBookmarks(); + } + + private void tryImport(String opml) { + try { + bookmarkManager().importBookmarks( + TestUtils.getManagedWebsite(testWeblog), folderName, opml); + TestUtils.endSession(true); + } catch (Exception expected) { + // A refusal to parse is one acceptable outcome; the assertions in + // each test say what must be true either way. + log.debug("import raised: " + expected); + } + } + + /** + * A declaration naming a local file must not put that file's contents into + * the imported data. + * + *

The reference has to sit in element content rather than an attribute + * value, which XML does not allow it in, and the file has to hold markup + * the importer will walk. That is the shape that stores what it read. + */ + @Test + public void aFileNamedByTheDocumentIsNotReadIntoBookmarks() throws Exception { + Path secret = Files.createTempFile("roller-import-probe", ".xml"); + Files.write(secret, ("").getBytes(StandardCharsets.UTF_8)); + + String opml = "" + + "]>" + + "t" + + "&probe;" + + "" + + ""; + + tryImport(opml); + + for (WeblogBookmark bookmark : importedBookmarks()) { + String name = String.valueOf(bookmark.getName()); + String desc = String.valueOf(bookmark.getDescription()); + assertFalse(name.contains("PROBE-CONTENT") || desc.contains("PROBE-CONTENT"), + "file contents named by the document reached a bookmark: " + + name + " / " + desc); + } + + Files.deleteIfExists(secret); + } + + /** + * A declaration naming an http resource must not cause the server to + * request it. Asserted against a listener that counts connections. + */ + @Test + public void anHttpResourceNamedByTheDocumentIsNotRequested() throws Exception { + AtomicInteger connections = new AtomicInteger(); + try (ServerSocket listener = new ServerSocket(0)) { + listener.setSoTimeout(2000); + Thread accepting = new Thread(() -> { + while (!Thread.currentThread().isInterrupted()) { + try (Socket s = listener.accept()) { + connections.incrementAndGet(); + } catch (Exception stop) { + return; + } + } + }); + accepting.setDaemon(true); + accepting.start(); + + String url = "http://127.0.0.1:" + listener.getLocalPort() + "/probe.dtd"; + String opml = "" + + "" + + "t" + + "" + + ""; + + tryImport(opml); + Thread.sleep(300); + accepting.interrupt(); + + assertEquals(0, connections.get(), + "the import requested a resource named by the document"); + } + } + + /** Ordinary OPML, with no declarations in it, must still import. */ + @Test + public void ordinaryOpmlStillImports() throws Exception { + byte[] opml = Files.readAllBytes( + new File("src/test/resources/bookmarks.opml").toPath()); + bookmarkManager().importBookmarks(TestUtils.getManagedWebsite(testWeblog), + folderName, new String(opml, StandardCharsets.UTF_8)); + TestUtils.endSession(true); + + assertFalse(importedBookmarks().isEmpty(), + "ordinary OPML no longer imports any bookmarks"); + } + + /** The rejection must not depend on where the DOCTYPE points. */ + @Test + public void aDoctypeAloneIsEnoughToBeRefused() throws Exception { + String opml = "" + + "]>" + + "t" + + "" + + ""; + + tryImport(opml); + + assertTrue(importedBookmarks().isEmpty(), + "a document carrying a DOCTYPE was still imported"); + } +} diff --git a/app/src/test/java/org/apache/roller/weblogger/util/SafeSAXBuilderTest.java b/app/src/test/java/org/apache/roller/weblogger/util/SafeSAXBuilderTest.java new file mode 100644 index 0000000000..f69448d37d --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/util/SafeSAXBuilderTest.java @@ -0,0 +1,147 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. The ASF licenses this file to You + * under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or + * implied. See the License for the specific language governing + * permissions and limitations under the License. For additional + * information regarding copyright in this work, please see the NOTICE + * file in the top level directory of this distribution. + */ +package org.apache.roller.weblogger.util; + +import java.io.StringReader; +import java.net.ServerSocket; +import java.net.Socket; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.concurrent.atomic.AtomicInteger; + +import org.jdom2.Document; +import org.jdom2.input.SAXBuilder; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * The parser contract, checked directly rather than through a caller. + * + *

Each case that asserts a resource is not resolved also renders the same + * document through a plain {@link SAXBuilder} first, and asserts that one does + * resolve it. Without that reference the assertions would still pass if the + * document were simply malformed, or if the parser were refusing it for some + * unrelated reason. + */ +public class SafeSAXBuilderTest { + + private static final String ORDINARY = + "t" + + ""; + + /** Ordinary XML, carrying no declarations, still parses. */ + @Test + public void ordinaryDocumentsStillParse() throws Exception { + Document doc = new SafeSAXBuilder().build(new StringReader(ORDINARY)); + assertNotNull(doc.getRootElement()); + assertEquals("opml", doc.getRootElement().getName()); + } + + /** Any document type declaration is refused, whatever it points at. */ + @Test + public void anyDoctypeIsRefused() { + String withInternalSubset = "" + + "]>" + + ""; + assertThrows(Exception.class, + () -> new SafeSAXBuilder().build(new StringReader(withInternalSubset)), + "a document type declaration was accepted"); + } + + /** A declared file is not read. */ + @Test + public void aDeclaredFileIsNotRead() throws Exception { + Path secret = Files.createTempFile("roller-saxbuilder-probe", ".txt"); + Files.write(secret, "PROBE-CONTENT-4d21".getBytes(StandardCharsets.UTF_8)); + + String doc = "" + + "]>" + + "&probe;"; + + // Reference: the unhardened parser does read it. + String reference = renderWith(new SAXBuilder(), doc); + assertTrue(reference.contains("PROBE-CONTENT-4d21"), + "control failed: the plain parser did not read the declared file, so " + + "the assertion below shows nothing:\n" + reference); + + String hardened = renderWith(new SafeSAXBuilder(), doc); + assertFalse(hardened.contains("PROBE-CONTENT-4d21"), + "a declared file was read:\n" + hardened); + + Files.deleteIfExists(secret); + } + + /** A declared URL is not requested. */ + @Test + public void aDeclaredUrlIsNotRequested() throws Exception { + try (ServerSocket listener = new ServerSocket(0)) { + listener.setSoTimeout(1500); + AtomicInteger connections = new AtomicInteger(); + Thread accepting = new Thread(() -> { + while (!Thread.currentThread().isInterrupted()) { + try (Socket s = listener.accept()) { + connections.incrementAndGet(); + } catch (Exception stop) { + return; + } + } + }); + accepting.setDaemon(true); + accepting.start(); + + String url = "http://127.0.0.1:" + listener.getLocalPort() + "/probe.dtd"; + String doc = "" + + "x"; + + // Reference: the unhardened parser does request it. + renderWith(new SAXBuilder(), doc); + Thread.sleep(300); + int afterPlain = connections.get(); + assertTrue(afterPlain > 0, + "control failed: the plain parser made no request, so the " + + "assertion below shows nothing"); + + renderWith(new SafeSAXBuilder(), doc); + Thread.sleep(300); + accepting.interrupt(); + + assertEquals(afterPlain, connections.get(), + "the hardened parser requested a declared URL"); + } + } + + /** + * @return the document's text content, or a marker naming the failure, so a + * parser that refuses the document and one that reads nothing from + * it are not confused with each other + */ + private String renderWith(SAXBuilder builder, String xml) { + try { + Document doc = builder.build(new StringReader(xml)); + return String.valueOf(doc.getRootElement().getValue()); + } catch (Exception refused) { + return "refused: " + refused.getClass().getSimpleName(); + } + } +} From 2185c829167145718e1513648965c553b06b0865 Mon Sep 17 00:00:00 2001 From: "David M. Johnson" Date: Sat, 29 Aug 2026 16:42:23 -0400 Subject: [PATCH 2/2] Restrict the retained parser tests to the hardened builder contract 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 --- .../business/BookmarkImportParsingTest.java | 107 ++--------------- .../weblogger/util/SafeSAXBuilderTest.java | 113 ++---------------- 2 files changed, 24 insertions(+), 196 deletions(-) diff --git a/app/src/test/java/org/apache/roller/weblogger/business/BookmarkImportParsingTest.java b/app/src/test/java/org/apache/roller/weblogger/business/BookmarkImportParsingTest.java index f7c570fe6f..4b43d2db81 100644 --- a/app/src/test/java/org/apache/roller/weblogger/business/BookmarkImportParsingTest.java +++ b/app/src/test/java/org/apache/roller/weblogger/business/BookmarkImportParsingTest.java @@ -1,29 +1,25 @@ /* * Licensed to the Apache Software Foundation (ASF) under one or more * contributor license agreements. The ASF licenses this file to You - * under the Apache License, Version 2.0 (the "License"); you may not - * use this file except in compliance with the License. - * You may obtain a copy of the License at + * under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. + * You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or - * implied. See the License for the specific language governing - * permissions and limitations under the License. For additional - * information regarding copyright in this work, please see the NOTICE - * file in the top level directory of this distribution. + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. For additional information regarding + * copyright in this work, please see the NOTICE file in the top level + * directory of this distribution. */ package org.apache.roller.weblogger.business; import java.io.File; -import java.net.ServerSocket; -import java.net.Socket; import java.nio.charset.StandardCharsets; import java.nio.file.Files; -import java.nio.file.Path; -import java.util.concurrent.atomic.AtomicInteger; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -36,17 +32,12 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; /** - * Covers what the OPML bookmark import will resolve while parsing. - * - *

The OPML document is supplied by a weblog administrator, a role Roller - * treats as untrusted. The parser must therefore take the document as data: - * the declarations in it name resources, and naming a resource must not cause - * the server to go and read it. + * Covers the OPML bookmark import's handling of document type declarations: + * documents that carry one are refused, while ordinary OPML still imports. */ public class BookmarkImportParsingTest { @@ -101,78 +92,6 @@ private void tryImport(String opml) { } } - /** - * A declaration naming a local file must not put that file's contents into - * the imported data. - * - *

The reference has to sit in element content rather than an attribute - * value, which XML does not allow it in, and the file has to hold markup - * the importer will walk. That is the shape that stores what it read. - */ - @Test - public void aFileNamedByTheDocumentIsNotReadIntoBookmarks() throws Exception { - Path secret = Files.createTempFile("roller-import-probe", ".xml"); - Files.write(secret, ("").getBytes(StandardCharsets.UTF_8)); - - String opml = "" - + "]>" - + "t" - + "&probe;" - + "" - + ""; - - tryImport(opml); - - for (WeblogBookmark bookmark : importedBookmarks()) { - String name = String.valueOf(bookmark.getName()); - String desc = String.valueOf(bookmark.getDescription()); - assertFalse(name.contains("PROBE-CONTENT") || desc.contains("PROBE-CONTENT"), - "file contents named by the document reached a bookmark: " - + name + " / " + desc); - } - - Files.deleteIfExists(secret); - } - - /** - * A declaration naming an http resource must not cause the server to - * request it. Asserted against a listener that counts connections. - */ - @Test - public void anHttpResourceNamedByTheDocumentIsNotRequested() throws Exception { - AtomicInteger connections = new AtomicInteger(); - try (ServerSocket listener = new ServerSocket(0)) { - listener.setSoTimeout(2000); - Thread accepting = new Thread(() -> { - while (!Thread.currentThread().isInterrupted()) { - try (Socket s = listener.accept()) { - connections.incrementAndGet(); - } catch (Exception stop) { - return; - } - } - }); - accepting.setDaemon(true); - accepting.start(); - - String url = "http://127.0.0.1:" + listener.getLocalPort() + "/probe.dtd"; - String opml = "" - + "" - + "t" - + "" - + ""; - - tryImport(opml); - Thread.sleep(300); - accepting.interrupt(); - - assertEquals(0, connections.get(), - "the import requested a resource named by the document"); - } - } - /** Ordinary OPML, with no declarations in it, must still import. */ @Test public void ordinaryOpmlStillImports() throws Exception { @@ -200,4 +119,4 @@ public void aDoctypeAloneIsEnoughToBeRefused() throws Exception { assertTrue(importedBookmarks().isEmpty(), "a document carrying a DOCTYPE was still imported"); } -} +} \ No newline at end of file diff --git a/app/src/test/java/org/apache/roller/weblogger/util/SafeSAXBuilderTest.java b/app/src/test/java/org/apache/roller/weblogger/util/SafeSAXBuilderTest.java index f69448d37d..4794b3bed8 100644 --- a/app/src/test/java/org/apache/roller/weblogger/util/SafeSAXBuilderTest.java +++ b/app/src/test/java/org/apache/roller/weblogger/util/SafeSAXBuilderTest.java @@ -1,48 +1,33 @@ /* * Licensed to the Apache Software Foundation (ASF) under one or more * contributor license agreements. The ASF licenses this file to You - * under the Apache License, Version 2.0 (the "License"); you may not - * use this file except in compliance with the License. - * You may obtain a copy of the License at + * under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. + * You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or - * implied. See the License for the specific language governing - * permissions and limitations under the License. For additional - * information regarding copyright in this work, please see the NOTICE - * file in the top level directory of this distribution. + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. For additional information regarding + * copyright in this work, please see the NOTICE file in the top level + * directory of this distribution. */ package org.apache.roller.weblogger.util; import java.io.StringReader; -import java.net.ServerSocket; -import java.net.Socket; -import java.nio.charset.StandardCharsets; -import java.nio.file.Files; -import java.nio.file.Path; -import java.util.concurrent.atomic.AtomicInteger; import org.jdom2.Document; -import org.jdom2.input.SAXBuilder; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; /** * The parser contract, checked directly rather than through a caller. - * - *

Each case that asserts a resource is not resolved also renders the same - * document through a plain {@link SAXBuilder} first, and asserts that one does - * resolve it. Without that reference the assertions would still pass if the - * document were simply malformed, or if the parser were refusing it for some - * unrelated reason. */ public class SafeSAXBuilderTest { @@ -68,80 +53,4 @@ public void anyDoctypeIsRefused() { () -> new SafeSAXBuilder().build(new StringReader(withInternalSubset)), "a document type declaration was accepted"); } - - /** A declared file is not read. */ - @Test - public void aDeclaredFileIsNotRead() throws Exception { - Path secret = Files.createTempFile("roller-saxbuilder-probe", ".txt"); - Files.write(secret, "PROBE-CONTENT-4d21".getBytes(StandardCharsets.UTF_8)); - - String doc = "" - + "]>" - + "&probe;"; - - // Reference: the unhardened parser does read it. - String reference = renderWith(new SAXBuilder(), doc); - assertTrue(reference.contains("PROBE-CONTENT-4d21"), - "control failed: the plain parser did not read the declared file, so " - + "the assertion below shows nothing:\n" + reference); - - String hardened = renderWith(new SafeSAXBuilder(), doc); - assertFalse(hardened.contains("PROBE-CONTENT-4d21"), - "a declared file was read:\n" + hardened); - - Files.deleteIfExists(secret); - } - - /** A declared URL is not requested. */ - @Test - public void aDeclaredUrlIsNotRequested() throws Exception { - try (ServerSocket listener = new ServerSocket(0)) { - listener.setSoTimeout(1500); - AtomicInteger connections = new AtomicInteger(); - Thread accepting = new Thread(() -> { - while (!Thread.currentThread().isInterrupted()) { - try (Socket s = listener.accept()) { - connections.incrementAndGet(); - } catch (Exception stop) { - return; - } - } - }); - accepting.setDaemon(true); - accepting.start(); - - String url = "http://127.0.0.1:" + listener.getLocalPort() + "/probe.dtd"; - String doc = "" - + "x"; - - // Reference: the unhardened parser does request it. - renderWith(new SAXBuilder(), doc); - Thread.sleep(300); - int afterPlain = connections.get(); - assertTrue(afterPlain > 0, - "control failed: the plain parser made no request, so the " - + "assertion below shows nothing"); - - renderWith(new SafeSAXBuilder(), doc); - Thread.sleep(300); - accepting.interrupt(); - - assertEquals(afterPlain, connections.get(), - "the hardened parser requested a declared URL"); - } - } - - /** - * @return the document's text content, or a marker naming the failure, so a - * parser that refuses the document and one that reads nothing from - * it are not confused with each other - */ - private String renderWith(SAXBuilder builder, String xml) { - try { - Document doc = builder.build(new StringReader(xml)); - return String.valueOf(doc.getRootElement().getValue()); - } catch (Exception refused) { - return "refused: " + refused.getClass().getSimpleName(); - } - } -} +} \ No newline at end of file