From 2a0608bc8b1667efc88b9a427bd26f8dd7de0ce5 Mon Sep 17 00:00:00 2001 From: "David M. Johnson" Date: Sat, 29 Aug 2026 10:43:58 -0400 Subject: [PATCH 1/4] Resolve weblog template resources within the active theme Weblog templates are authored by weblog administrators, whom Roller already treats as untrusted: the rendering engine runs them under SecureUberspector. That sandbox governs method access rather than resource resolution, so the loader set and the include directives are constrained to match it. The loader set for weblog rendering is now the webapp templates, the active theme, and the weblog's own stored templates. A ThemeIncludeEventHandler keeps #include and #parse within the namespace they are written in, refusing names that are absolute, walk upward, or carry a scheme. Macro libraries and feed templates resolve through the webapp loader and are unaffected. --- .../velocity/ThemeIncludeEventHandler.java | 90 ++++++++ .../main/webapp/WEB-INF/velocity.properties | 16 +- .../velocity/ThemeIncludeConfinementTest.java | 192 ++++++++++++++++++ 3 files changed, 291 insertions(+), 7 deletions(-) create mode 100644 app/src/main/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeEventHandler.java create mode 100644 app/src/test/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeConfinementTest.java diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeEventHandler.java b/app/src/main/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeEventHandler.java new file mode 100644 index 000000000..931fa3c6d --- /dev/null +++ b/app/src/main/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeEventHandler.java @@ -0,0 +1,90 @@ +/* + * 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 #include and #parse inside the template + * namespace they are rendered from. + * + *

Weblog templates are authored by weblog administrators, whom Roller treats + * as untrusted: the rendering engine runs them under + * SecureUberspector so they cannot reach arbitrary objects. That + * sandbox governs method calls, not resource resolution, so the include + * directives are confined here instead. + * + *

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. 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.warn("Refusing #" + directiveName + " of '" + path + + "' from '" + currentResourcePath + "': outside the template namespace"); + return null; + } + + return path; + } + + /** + * @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; + } + return false; + } +} diff --git a/app/src/main/webapp/WEB-INF/velocity.properties b/app/src/main/webapp/WEB-INF/velocity.properties index 6d043a815..190a7bbbc 100644 --- a/app/src/main/webapp/WEB-INF/velocity.properties +++ b/app/src/main/webapp/WEB-INF/velocity.properties @@ -15,7 +15,11 @@ # directory of this distribution. # specify resource loaders to use -resource.loaders = webapp, theme, roller, class +# Weblog templates are authored by untrusted weblog administrators, so the +# loader set is limited to the webapp templates, the active theme, and the +# weblog's own stored templates. The classpath is deliberately not a +# resolvable namespace for them. +resource.loaders = webapp, theme, roller # theme resource loader resource.loader.theme.public.name=theme @@ -31,12 +35,6 @@ resource.loader.roller.class=org.apache.roller.weblogger.ui.rendering.velocity.R resource.loader.roller.cache=false resource.loader.roller.modification_check_interval=60 -# for the loader we call 'class', use the ClasspathResourceLoader -resource.loader.class.description = Velocity Classpath Resource Loader -resource.loader.class.class = org.apache.velocity.runtime.resource.loader.ClasspathResourceLoader -resource.loader.class.cache=true -resource.loader.class.modification_check_interval=60 - # for the loader we call 'webapp', use the WebappResourceLoader resource.loader.webapp.description=Webapp Resource Loader resource.loader.webapp.class=org.apache.roller.weblogger.ui.rendering.velocity.WebappResourceLoader @@ -73,3 +71,7 @@ default.contentType=text/html; charset=utf-8 introspector.uberspect.class=org.apache.velocity.util.introspection.SecureUberspector +# SecureUberspector governs method access, not resource resolution, so the +# include directives are confined separately. +event_handler.include.class=org.apache.roller.weblogger.ui.rendering.velocity.ThemeIncludeEventHandler + diff --git a/app/src/test/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeConfinementTest.java b/app/src/test/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeConfinementTest.java new file mode 100644 index 000000000..4d273ba81 --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeConfinementTest.java @@ -0,0 +1,192 @@ +/* + * 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. + * + *

Weblog templates are authored by weblog administrators, a role Roller + * treats as untrusted and renders under SecureUberspector. 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 { + + private static final Path VELOCITY_PROPERTIES = + Paths.get("src", "main", "webapp", "WEB-INF", "velocity.properties"); + + private String velocityProperties() throws Exception { + assertTrue(Files.isReadable(VELOCITY_PROPERTIES), + "cannot read " + VELOCITY_PROPERTIES.toAbsolutePath() + + " (run from the app module)"); + return new String(Files.readAllBytes(VELOCITY_PROPERTIES), 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 { + String props = velocityProperties(); + for (String line : props.split("\n")) { + String trimmed = line.trim(); + if (trimmed.startsWith("resource.loaders")) { + assertFalse(trimmed.matches(".*\\bclass\\b.*"), + "the classpath loader must not be in the weblog loader set: " + trimmed); + } + } + assertFalse(props.contains("ClasspathResourceLoader"), + "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 { + String props = velocityProperties(); + assertTrue(props.contains( + "event_handler.include.class=org.apache.roller.weblogger.ui." + + "rendering.velocity.ThemeIncludeEventHandler"), + "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 { + assertTrue(velocityProperties().contains("SecureUberspector"), + "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/roller-custom.properties", + "../roller-custom.properties", + "../../WEB-INF/classes/roller-custom.properties", + "themes/../../roller-custom.properties", + "..", + "file:/etc/passwd", + "http://example.test/evil.vm", + "\\WEB-INF\\classes\\roller-custom.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 = { + "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. + * + *

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. This renders the + * same template with that loader and without it, which anchors the + * assertion to a demonstrated difference rather than to an include that + * might not have resolved under either configuration. + */ + @Test + public void classpathResourcesAreUnreachableOnceTheLoaderIsRemoved() throws Exception { + Path dir = Files.createTempDirectory("roller-include-confinement"); + Files.write(dir.resolve("include-by-name.vm"), + "BEFORE[#include(\"roller-custom.properties\")]AFTER" + .getBytes(StandardCharsets.UTF_8)); + + String withClasspath = render(dir, true); + assertTrue(withClasspath.contains("database.jdbc"), + "control failed: the classpath loader did not resolve the resource, so " + + "this test cannot show that the loader set matters:\n" + withClasspath); + + String withoutClasspath = render(dir, false); + assertFalse(withoutClasspath.contains("database.jdbc"), + "a weblog template resolved a classpath resource:\n" + withoutClasspath); + } + + /** + * Renders include-by-name.vm with and without the classpath in the loader set, + * mirroring the shipped configuration in each case. + */ + private String render(Path dir, boolean includeClasspathLoader) { + Properties props = new Properties(); + props.setProperty("resource.loaders", includeClasspathLoader ? "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"); + 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 we want in the without-classpath case. + return "unresolved: " + ex.getClass().getSimpleName(); + } + return out.toString(); + } +} From ce12618b162afa35b871524f29f13b15df5c7316 Mon Sep 17 00:00:00 2001 From: "David M. Johnson" Date: Sat, 29 Aug 2026 10:46:39 -0400 Subject: [PATCH 2/4] Align the test Velocity configuration with the shipped one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The copy under src/test/resources had drifted: it carried a loader set the webapp no longer uses and lacked the introspection sandbox entirely. Nothing reads it today — the servlet context resolves /WEB-INF/velocity.properties from src/main/webapp — but a second configuration that disagrees with the shipped one is a configuration that can quietly become live. The configuration assertions now run over both files so they cannot drift apart again. --- .../velocity/ThemeIncludeConfinementTest.java | 60 +++++++++++-------- .../resources/WEB-INF/velocity.properties | 18 +++--- 2 files changed, 47 insertions(+), 31 deletions(-) diff --git a/app/src/test/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeConfinementTest.java b/app/src/test/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeConfinementTest.java index 4d273ba81..18100ab1a 100644 --- a/app/src/test/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeConfinementTest.java +++ b/app/src/test/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeConfinementTest.java @@ -45,14 +45,19 @@ */ public class ThemeIncludeConfinementTest { - private static final Path VELOCITY_PROPERTIES = - Paths.get("src", "main", "webapp", "WEB-INF", "velocity.properties"); - - private String velocityProperties() throws Exception { - assertTrue(Files.isReadable(VELOCITY_PROPERTIES), - "cannot read " + VELOCITY_PROPERTIES.toAbsolutePath() - + " (run from the app module)"); - return new String(Files.readAllBytes(VELOCITY_PROPERTIES), StandardCharsets.UTF_8); + /** + * 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)"); + return new String(Files.readAllBytes(path), StandardCharsets.UTF_8); } /** @@ -61,34 +66,41 @@ private String velocityProperties() throws Exception { */ @Test public void classpathIsNotAResolvableNamespace() throws Exception { - String props = velocityProperties(); - for (String line : props.split("\n")) { - String trimmed = line.trim(); - if (trimmed.startsWith("resource.loaders")) { - assertFalse(trimmed.matches(".*\\bclass\\b.*"), - "the classpath loader must not be in the weblog loader set: " + trimmed); + 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"); } - assertFalse(props.contains("ClasspathResourceLoader"), - "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 { - String props = velocityProperties(); - assertTrue(props.contains( - "event_handler.include.class=org.apache.roller.weblogger.ui." - + "rendering.velocity.ThemeIncludeEventHandler"), - "the include event handler must be registered under Velocity 2's " - + "event_handler.include.class key"); + 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 { - assertTrue(velocityProperties().contains("SecureUberspector"), - "the introspection sandbox must be retained"); + 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. */ diff --git a/app/src/test/resources/WEB-INF/velocity.properties b/app/src/test/resources/WEB-INF/velocity.properties index 3af60e6e7..4c218366f 100644 --- a/app/src/test/resources/WEB-INF/velocity.properties +++ b/app/src/test/resources/WEB-INF/velocity.properties @@ -15,7 +15,11 @@ # directory of this distribution. # specify resource loaders to use -resource.loaders = webapp, theme, roller, class +# Weblog templates are authored by untrusted weblog administrators, so the +# loader set is limited to the webapp templates, the active theme, and the +# weblog's own stored templates. The classpath is deliberately not a +# resolvable namespace for them. +resource.loaders = webapp, theme, roller # theme resource loader resource.loader.theme.public.name=theme @@ -31,12 +35,6 @@ resource.loader.roller.class=org.apache.roller.weblogger.ui.rendering.velocity.R resource.loader.roller.cache=false resource.loader.roller.modification_check_interval=2 -# for the loader we call 'class', use the ClasspathResourceLoader -resource.loader.class.description = Velocity Classpath Resource Loader -resource.loader.class.class = org.apache.velocity.runtime.resource.loader.ClasspathResourceLoader -resource.loader.class.cache=true -resource.loader.class.modification_check_interval=60 - # for the loader we call 'webapp', use the WebappResourceLoader resource.loader.webapp.description = Roller Webapp Resource Loader resource.loader.webapp.class = org.apache.roller.weblogger.ui.rendering.velocity.WebappResourceLoader @@ -70,3 +68,9 @@ velocimacro.inline.local_scope=false # set encoding/charset to UTF-8 resource.default_encoding=UTF-8 default.contentType=text/html; charset=utf-8 + +# Weblog templates render under SecureUberspector, which governs method access +# rather than resource resolution, so the include directives are confined +# separately. Keep this aligned with /WEB-INF/velocity.properties. +introspector.uberspect.class=org.apache.velocity.util.introspection.SecureUberspector +event_handler.include.class=org.apache.roller.weblogger.ui.rendering.velocity.ThemeIncludeEventHandler From 8a03f17b4b0445ad48c8f582fc4ac682089b1e66 Mon Sep 17 00:00:00 2001 From: "David M. Johnson" Date: Sat, 29 Aug 2026 11:17:38 -0400 Subject: [PATCH 3/4] Hold template include names to the shapes a template can take The include directives resolve names through whichever loaders are configured, so a name needs no traversal to reach whatever those loaders can see. Constrain the names themselves as well as the loader set: a stored template id carries no extension, and a theme resource is a Velocity template, so a name bearing some other extension is not a template reference and is refused. The engine test now measures the loader set and the include handler separately, against a reference rendering, so neither is resting on the other. --- .../velocity/ThemeIncludeEventHandler.java | 37 +++++++++++++-- .../velocity/ThemeIncludeConfinementTest.java | 47 ++++++++++++------- 2 files changed, 63 insertions(+), 21 deletions(-) diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeEventHandler.java b/app/src/main/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeEventHandler.java index 931fa3c6d..ce1561ffa 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeEventHandler.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeEventHandler.java @@ -36,8 +36,15 @@ *

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. Returning null tells Velocity not to resolve the - * resource at all. + * carries a scheme is refused. + * + *

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. + * + *

Returning null tells Velocity not to resolve the resource at all. */ public class ThemeIncludeEventHandler implements IncludeEventHandler { @@ -53,7 +60,7 @@ public String includeEvent(Context context, String includeResourcePath, String path = includeResourcePath.trim(); - if (isOutsideNamespace(path)) { + 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. @@ -65,6 +72,30 @@ public String includeEvent(Context context, String includeResourcePath, 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