Use a shared JDOM builder for bookmark and configuration parsing #173
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
Contributor
|
||
| * 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); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Nit: in jdom2 |
||
| 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 { | ||
|
Contributor
There was a problem hiding this comment. On this classpath |
||
|
|
||
| @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; | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) { | ||
|
Contributor
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 |
||
| // 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()); | ||
|
Contributor
There was a problem hiding this comment. cwd-relative; |
||
| 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 = "<?xml version=\"1.0\"?>" | ||
| + "<!DOCTYPE opml [<!ELEMENT opml ANY>]>" | ||
| + "<opml version=\"1.1\"><head><title>t</title></head><body>" | ||
| + "<outline text=\"harmless\" type=\"link\" url=\"http://example.test/\"/>" | ||
| + "</body></opml>"; | ||
|
|
||
| tryImport(opml); | ||
|
|
||
| assertTrue(importedBookmarks().isEmpty(), | ||
| "a document carrying a DOCTYPE was still imported"); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| /* | ||
| * 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 org.jdom2.Document; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
|
|
||
| /** | ||
| * The parser contract, checked directly rather than through a caller. | ||
| */ | ||
| public class SafeSAXBuilderTest { | ||
|
|
||
| private static final String ORDINARY = | ||
| "<?xml version=\"1.0\"?><opml version=\"1.1\"><head><title>t</title>" | ||
| + "</head><body><outline text=\"a\"/></body></opml>"; | ||
|
|
||
| /** 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 = "<?xml version=\"1.0\"?>" | ||
| + "<!DOCTYPE opml [<!ELEMENT opml ANY>]>" | ||
| + "<opml version=\"1.1\"><body/></opml>"; | ||
| assertThrows(Exception.class, | ||
| () -> new SafeSAXBuilder().build(new StringReader(withInternalSubset)), | ||
| "a document type declaration was accepted"); | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BookmarksImportshowsex.toString(), so a user whose OPML export has<!DOCTYPE opml>now seesorg.apache.roller.weblogger.WebloggerException: org.jdom2.input.JDOMParseException: ... DOCTYPE is disallowed when the feature .... CatchingJDOMParseExceptionhere and wrapping it with a message like "OPML files with a DOCTYPE are not accepted" would tell them what to do.