Resolve weblog template resources within the active theme #172
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| /* | ||
| * 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.ui.rendering.velocity; | ||
|
|
||
| import org.apache.commons.logging.Log; | ||
| import org.apache.commons.logging.LogFactory; | ||
| import org.apache.velocity.app.event.IncludeEventHandler; | ||
| import org.apache.velocity.context.Context; | ||
|
|
||
| /** | ||
| * Keeps <code>#include</code> and <code>#parse</code> inside the template | ||
| * namespace they are rendered from. | ||
| * | ||
| * <p>Weblog templates are authored by weblog administrators, whom Roller treats | ||
| * as untrusted: the rendering engine runs them under | ||
| * <code>SecureUberspector</code> so they cannot reach arbitrary objects. That | ||
| * sandbox governs method calls, not resource resolution, so the include | ||
| * directives are confined here instead. | ||
| * | ||
| * <p>Legitimate includes name a resource within the current theme, or a stored | ||
| * template resolved by id through the weblog's own template collection. Neither | ||
| * needs to leave the namespace, so a name that is absolute, walks upward, or | ||
| * carries a scheme is refused. | ||
| * | ||
| * <p>Names are also held to the shapes a template can actually take: a stored | ||
| * template id, which carries no extension, or a Velocity template file. A name | ||
| * that asks for some other kind of file is not a template reference at all, and | ||
| * refusing it keeps the directives pointed at templates no matter what a loader | ||
| * further down happens to be able to resolve. | ||
| * | ||
| * <p>Returning null tells Velocity not to resolve the resource at all. | ||
| */ | ||
| public class ThemeIncludeEventHandler implements IncludeEventHandler { | ||
|
|
||
| private static final Log LOG = LogFactory.getLog(ThemeIncludeEventHandler.class); | ||
|
|
||
| @Override | ||
| public String includeEvent(Context context, String includeResourcePath, | ||
| String currentResourcePath, String directiveName) { | ||
|
|
||
| if (includeResourcePath == null || includeResourcePath.trim().isEmpty()) { | ||
| return null; | ||
| } | ||
|
|
||
| String path = includeResourcePath.trim(); | ||
|
|
||
| if (isOutsideNamespace(path) || isNotATemplateName(path)) { | ||
| // Logged rather than raised: a template that asks for something it | ||
| // may not have renders without that fragment, which is how Velocity | ||
| // already treats a resource it cannot find. | ||
| LOG.warn("Refusing #" + directiveName + " of '" + path | ||
|
Contributor
There was a problem hiding this comment. Once the id bug is fixed this is fine, but note it's unthrottled: any weblog admin can make the server log a WARN per request by leaving a refused include in a public template. Debug, or a once-per-template warning, would be safer. |
||
| + "' from '" + currentResourcePath + "': outside the template namespace"); | ||
| return null; | ||
| } | ||
|
|
||
| return path; | ||
| } | ||
|
|
||
| /** | ||
| * Stored templates are resolved by id and carry no extension; theme | ||
| * resources are Velocity templates. A name bearing any other extension is | ||
| * asking for something that is not a template. | ||
| * | ||
| * @return true when the name is not one of those two shapes | ||
| */ | ||
| private boolean isNotATemplateName(String path) { | ||
| // Stored template ids arrive as <template>|<deviceType>; the device | ||
| // type is a rendition selector, not part of the resource name. | ||
| String name = path; | ||
| int bar = name.indexOf('|'); | ||
| if (bar > -1) { | ||
| name = name.substring(0, bar); | ||
| } | ||
|
|
||
| int dot = name.lastIndexOf('.'); | ||
| if (dot == -1) { | ||
| // No extension: a stored template id. | ||
| return false; | ||
| } | ||
| return !name.regionMatches(true, dot, ".vm", 0, 3) || dot != name.length() - 3; | ||
|
Contributor
There was a problem hiding this comment. Shared-theme template names are free-form: the basic theme's stylesheet template is literally named |
||
| } | ||
|
|
||
| /** | ||
| * @return true when the name reaches outside the namespace it was written | ||
| * in — an absolute path, an upward traversal, or a scheme such as | ||
| * file: or http: | ||
| */ | ||
| private boolean isOutsideNamespace(String path) { | ||
| String normalized = path.replace('\\', '/'); | ||
|
|
||
| if (normalized.startsWith("/")) { | ||
| return true; | ||
| } | ||
| if (normalized.contains("../") || normalized.endsWith("..")) { | ||
| return true; | ||
| } | ||
| // A colon before any slash indicates a scheme or a Windows drive. | ||
| int colon = normalized.indexOf(':'); | ||
| if (colon > -1) { | ||
| int slash = normalized.indexOf('/'); | ||
| return slash == -1 || colon < slash; | ||
|
Contributor
There was a problem hiding this comment. This refuses every shared-theme template id. |
||
| } | ||
| 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
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,215 @@ | ||
| /* | ||
| * 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.ui.rendering.velocity; | ||
|
|
||
| import java.io.StringWriter; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.nio.file.Paths; | ||
| import java.util.Properties; | ||
|
|
||
| import org.apache.velocity.VelocityContext; | ||
| import org.apache.velocity.app.VelocityEngine; | ||
| 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.assertNull; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| /** | ||
| * Covers where a weblog template may resolve resources from. | ||
| * | ||
| * <p>Weblog templates are authored by weblog administrators, a role Roller | ||
| * treats as untrusted and renders under <code>SecureUberspector</code>. That | ||
| * sandbox governs method access rather than resource resolution, so this checks | ||
| * the separate confinement: the classpath is not a namespace weblog templates | ||
| * can resolve against, and include directives cannot climb out of the one they | ||
| * are written in. | ||
| */ | ||
| public class ThemeIncludeConfinementTest { | ||
|
|
||
| /** | ||
| * Every Velocity configuration in the tree, because a second copy that | ||
| * still admits the classpath is a copy that can quietly become live. | ||
| */ | ||
| private static final Path[] VELOCITY_PROPERTIES = { | ||
| Paths.get("src", "main", "webapp", "WEB-INF", "velocity.properties"), | ||
| Paths.get("src", "test", "resources", "WEB-INF", "velocity.properties"), | ||
| }; | ||
|
|
||
| private String read(Path path) throws Exception { | ||
| assertTrue(Files.isReadable(path), | ||
| "cannot read " + path.toAbsolutePath() + " (run from the app module)"); | ||
|
Contributor
There was a problem hiding this comment. cwd-relative, so this only runs from |
||
| return new String(Files.readAllBytes(path), StandardCharsets.UTF_8); | ||
| } | ||
|
|
||
| /** | ||
| * The classpath must not be in the loader set used for weblog rendering. | ||
| * With it present, any file packaged in the WAR is resolvable by name. | ||
| */ | ||
| @Test | ||
| public void classpathIsNotAResolvableNamespace() throws Exception { | ||
| for (Path path : VELOCITY_PROPERTIES) { | ||
| String props = read(path); | ||
| for (String line : props.split("\n")) { | ||
| String trimmed = line.trim(); | ||
| if (trimmed.startsWith("resource.loaders")) { | ||
| assertFalse(trimmed.matches(".*\\bclass\\b.*"), | ||
| path + ": the classpath loader must not be in the weblog " | ||
| + "loader set: " + trimmed); | ||
| } | ||
| } | ||
| assertFalse(props.contains("ClasspathResourceLoader"), | ||
| path + ": the classpath loader must not be configured for " | ||
| + "weblog rendering"); | ||
| } | ||
| } | ||
|
|
||
| /** The include handler must actually be registered, under Velocity 2's key. */ | ||
| @Test | ||
| public void includeHandlerIsRegistered() throws Exception { | ||
| for (Path path : VELOCITY_PROPERTIES) { | ||
| assertTrue(read(path).contains( | ||
| "event_handler.include.class=org.apache.roller.weblogger.ui." | ||
| + "rendering.velocity.ThemeIncludeEventHandler"), | ||
| path + ": the include event handler must be registered under " | ||
| + "Velocity 2's event_handler.include.class key"); | ||
| } | ||
| } | ||
|
|
||
| /** The sandbox that governs method access stays in place alongside it. */ | ||
| @Test | ||
| public void secureUberspectorIsRetained() throws Exception { | ||
| for (Path path : VELOCITY_PROPERTIES) { | ||
| assertTrue(read(path).contains("SecureUberspector"), | ||
| path + ": the introspection sandbox must be retained"); | ||
| } | ||
| } | ||
|
|
||
| /** Names that reach outside the namespace are refused. */ | ||
| @Test | ||
| public void namesThatLeaveTheNamespaceAreRefused() { | ||
| ThemeIncludeEventHandler handler = new ThemeIncludeEventHandler(); | ||
| String[] refused = { | ||
| "/WEB-INF/classes/secret.properties", | ||
| "../secret.properties", | ||
| "../../WEB-INF/classes/secret.properties", | ||
| "themes/../../secret.properties", | ||
| "..", | ||
| "file:/etc/passwd", | ||
| "http://example.test/evil.vm", | ||
| "\\WEB-INF\\classes\\secret.properties", | ||
| "", | ||
| " ", | ||
| // Not template names: a plain name needs no traversal to reach | ||
| // whatever a loader can resolve, so shape is checked too. | ||
| "secret.properties", | ||
| "web.xml", | ||
| "some/config.properties", | ||
| "weblog.vm.bak", | ||
| "notes.txt", | ||
| }; | ||
| for (String name : refused) { | ||
| assertNull(handler.includeEvent(new VelocityContext(), name, "weblog.vm", "include"), | ||
| "expected [" + name + "] to be refused"); | ||
| } | ||
| assertNull(handler.includeEvent(new VelocityContext(), null, "weblog.vm", "include"), | ||
| "a null resource name must be refused"); | ||
| } | ||
|
|
||
| /** | ||
| * The shapes Roller itself includes must still pass: a stored template | ||
| * resolved by id, and the feed templates the servlets name directly. | ||
| */ | ||
| @Test | ||
| public void legitimateIncludesStillPass() { | ||
| ThemeIncludeEventHandler handler = new ThemeIncludeEventHandler(); | ||
| String[] allowed = { | ||
|
Contributor
There was a problem hiding this comment. None of these are the shape Roller's own macros pass to |
||
| "9cf62fb5-9e6e-11f1-8b02-0e09da24358c|standard", // stored template id | ||
| "_day.vm", // theme resource | ||
| "feeds/weblog-search-atom.vm", // servlet-named feed | ||
| "site-search-atom.vm", | ||
| }; | ||
| for (String name : allowed) { | ||
| assertEquals(name, | ||
| handler.includeEvent(new VelocityContext(), name, "weblog.vm", "parse"), | ||
| "expected [" + name + "] to be allowed through"); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * End to end against the real engine. | ||
| * | ||
| * <p>Velocity's ClasspathResourceLoader resolves a plain resource name | ||
| * against the classpath, with no traversal involved, so a loader set that | ||
| * includes it makes any packaged file resolvable by name. The first case | ||
| * reproduces that resolution, which is what gives the other two something | ||
| * to be measured against: each of the two changes is then shown to stop it | ||
| * on its own, so neither is carrying the other. | ||
| */ | ||
| @Test | ||
| public void aPlainNameDoesNotReachAPackagedFile() throws Exception { | ||
| Path dir = Files.createTempDirectory("roller-include-confinement"); | ||
|
Contributor
There was a problem hiding this comment. Nit: the temp directory is never deleted; a |
||
| Files.write(dir.resolve("include-by-name.vm"), | ||
| "BEFORE[#include(\"confinement-probe.properties\")]AFTER" | ||
| .getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| String reference = render(dir, true, false); | ||
| assertTrue(reference.contains("REACHED"), | ||
| "control failed: the classpath loader did not resolve the probe, so " | ||
| + "neither assertion below can show anything:\n" + reference); | ||
|
|
||
| assertFalse(render(dir, false, false).contains("REACHED"), | ||
| "the shipped loader set still resolved a classpath resource"); | ||
|
|
||
| assertFalse(render(dir, true, true).contains("REACHED"), | ||
| "the include handler still admitted a name that is not a template"); | ||
| } | ||
|
|
||
| /** | ||
| * Renders include-by-name.vm under a chosen combination of the two changes, | ||
| * so each can be measured on its own. | ||
| */ | ||
| private String render(Path dir, boolean classpathLoader, boolean includeHandler) { | ||
| Properties props = new Properties(); | ||
| props.setProperty("resource.loaders", classpathLoader ? "file, class" : "file"); | ||
| props.setProperty("resource.loader.file.class", | ||
| "org.apache.velocity.runtime.resource.loader.FileResourceLoader"); | ||
| props.setProperty("resource.loader.file.path", dir.toString()); | ||
| props.setProperty("resource.loader.class.class", | ||
| "org.apache.velocity.runtime.resource.loader.ClasspathResourceLoader"); | ||
| if (includeHandler) { | ||
| props.setProperty("event_handler.include.class", | ||
| ThemeIncludeEventHandler.class.getName()); | ||
| } | ||
| VelocityEngine engine = new VelocityEngine(); | ||
| engine.init(props); | ||
|
|
||
| StringWriter out = new StringWriter(); | ||
| try { | ||
| engine.mergeTemplate("include-by-name.vm", "UTF-8", new VelocityContext(), out); | ||
| } catch (Exception ex) { | ||
| // Velocity raises when nothing can resolve the name, which is the | ||
| // outcome the assertions below are looking for. | ||
| return "unresolved: " + ex.getClass().getSimpleName(); | ||
| } | ||
| return out.toString(); | ||
| } | ||
| } | ||
Oops, something went wrong.
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.
Nit:
trim()is computed twice, and returning the trimmed value means#parse(" $pageId")now resolves a different name than before. Compute it once and return the original unless trimming is intentional.