mraible commented on code in PR #173: URL: https://github.com/apache/roller/pull/173#discussion_r3891448583
########## 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. + * + * <p>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. + * + * <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 Review Comment: Not quite true yet: `Trackback.parseTrackbackResponse` (`Trackback.java:177`) still uses a bare `new SAXBuilder()` on a remote server's response. #163 deletes that file, so either merge this after #163 or switch that call site here too. ########## app/src/main/java/org/apache/roller/weblogger/business/jpa/JPABookmarkManagerImpl.java: ########## @@ -142,7 +142,7 @@ public void importBookmarks( try { // Build JDOC document OPML string - SAXBuilder builder = new SAXBuilder(); + SafeSAXBuilder builder = new SafeSAXBuilder(); Review 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. ########## app/src/main/java/org/apache/roller/weblogger/business/themes/ThemeMetadataParser.java: ########## @@ -52,7 +52,7 @@ public ThemeMetadata unmarshall(InputStream instream) ThemeMetadata theme = new ThemeMetadata(); - SAXBuilder builder = new SAXBuilder(); + SafeSAXBuilder builder = new SafeSAXBuilder(); Review 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. ########## app/src/test/java/org/apache/roller/weblogger/business/BookmarkImportParsingTest.java: ########## @@ -0,0 +1,122 @@ +/* + * 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.nio.charset.StandardCharsets; +import java.nio.file.Files; + +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.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * 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 { + + 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<WeblogBookmark> 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); + } + } + + /** 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()); Review Comment: cwd-relative; `BookmarkTest` and `FileContentManagerTest` load the same fixture from the classpath. ########## app/src/test/java/org/apache/roller/weblogger/business/BookmarkImportParsingTest.java: ########## @@ -0,0 +1,122 @@ +/* + * 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.nio.charset.StandardCharsets; +import java.nio.file.Files; + +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.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * 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 { + + 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<WeblogBookmark> 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) { Review 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`. ########## 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. + * + * <p>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. + * + * <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 + * anything. Roller's own descriptors carry no document type declaration, so the + * strict setting costs them nothing. + * + * <p>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. + * + * <p>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 { Review 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. ########## 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. + * + * <p>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. + * + * <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 + * anything. Roller's own descriptors carry no document type declaration, so the + * strict setting costs them nothing. + * + * <p>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); + Review 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
