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
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,103 @@ | ||
| /* | ||
| * 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>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)) { | ||
| // 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.debug("Refusing #" + directiveName + " of '" + path | ||
| + "' from '" + currentResourcePath + "': outside the template namespace"); | ||
| return null; | ||
| } | ||
|
|
||
| // Keep the caller's spelling (including any intentional surrounding | ||
| // whitespace) once validation has accepted the identifier. | ||
| return includeResourcePath; | ||
| } | ||
|
|
||
| /** | ||
| * @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; | ||
| } | ||
| // URI schemes and Windows drive prefixes are not theme identifiers. | ||
| int colon = normalized.indexOf(':'); | ||
| if (colon > -1) { | ||
| if (normalized.indexOf(':', colon + 1) > -1) { | ||
| return true; | ||
| } | ||
| if (colon == 1 && Character.isLetter(normalized.charAt(0))) { | ||
| return true; | ||
| } | ||
| if (colon + 1 == normalized.length() | ||
| || normalized.charAt(colon + 1) == '/') { | ||
| return true; | ||
| } | ||
| String namespace = normalized.substring(0, colon); | ||
| return namespace.indexOf('.') > -1 || namespace.indexOf('/') > -1; | ||
| } | ||
| 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,212 @@ | ||
| /* | ||
| * 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(System.getProperty("project.basedir", System.getProperty("user.dir")), | ||
| "src", "main", "webapp", "WEB-INF", "velocity.properties"), | ||
| Paths.get(System.getProperty("project.basedir", System.getProperty("user.dir")), | ||
| "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", | ||
| "", | ||
| " ", | ||
| }; | ||
| 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 | ||
| "basic:_day|standard", // shared theme template | ||
| "basic:basic-custom.css|standard", // CSS rendition | ||
| "_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"); | ||
|
|
||
| assertTrue(render(dir, true, true).contains("REACHED"), | ||
| "free-form template names should be admitted when a loader is explicitly present"); | ||
| } | ||
|
|
||
| /** | ||
| * 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(); | ||
| } | ||
| } | ||
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
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.