Resolve weblog template resources within the active theme - #172
Resolve weblog template resources within the active theme#172snoopdave wants to merge 5 commits into
Conversation
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.
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.
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.
The engine test now demonstrates the classpath loader was reachable and is now closed against a generic test-fixture property rather than a named packaged resource, and the refused-name list uses generic paths. Claude-Session: https://claude.ai/code/session_01A1fhY1E2PCFU6UAPXu2WtV
| // No extension: a stored template id. | ||
| return false; | ||
| } | ||
| return !name.regionMatches(true, dot, ".vm", 0, 3) || dot != name.length() - 3; |
There was a problem hiding this comment.
Shared-theme template names are free-form: the basic theme's stylesheet template is literally named basic-custom.css, and the template guide documents inlining it through #includeTemplate. This check strips |standard, sees .css, and refuses it. Since the resource loaders are already confined to the theme and webapp namespaces, I'd drop the extension check rather than try to enumerate template-name shapes.
| @Test | ||
| public void legitimateIncludesStillPass() { | ||
| ThemeIncludeEventHandler handler = new ThemeIncludeEventHandler(); | ||
| String[] allowed = { |
There was a problem hiding this comment.
None of these are the shape Roller's own macros pass to #parse. Add basic:_day|standard and basic:basic-custom.css|standard here; both fail against the current handler and would have caught the regression.
| // 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 |
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.
| # 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 |
There was a problem hiding this comment.
With the class loader gone, RollerVelocity (lines 66-67) still sets resource.loader.class.cache and resource.loader.class.modification_check_interval under themes.reload.mode; those two lines should go in the same change.
|
|
||
| private String read(Path path) throws Exception { | ||
| assertTrue(Files.isReadable(path), | ||
| "cannot read " + path.toAbsolutePath() + " (run from the app module)"); |
There was a problem hiding this comment.
cwd-relative, so this only runs from app/; surefire sets project.build.directory for this module (see ApplicationResourcesTest), which would let it run from an IDE rooted at the repo.
| */ | ||
| @Test | ||
| public void aPlainNameDoesNotReachAPackagedFile() throws Exception { | ||
| Path dir = Files.createTempDirectory("roller-include-confinement"); |
There was a problem hiding this comment.
Nit: the temp directory is never deleted; a @TempDir parameter does the cleanup.
| return null; | ||
| } | ||
|
|
||
| String path = includeResourcePath.trim(); |
There was a problem hiding this comment.
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.
2 participants
Weblog template rendering currently loads resources from the application
classpath in addition to the theme namespace. This change confines template
resource resolution to the active theme and restricts include targets to the
shapes a template actually takes.
What changed
rendering, and align the stale test copy of
velocity.properties.#include/#parseto thetheme namespace and refuses absolute paths, upward traversal, schemes, and
names that are neither a stored template id nor a
.vmfile.SecureUberspectorunchanged.Tests
ThemeIncludeConfinementTestcovers loader set, handler registration, anduberspector configuration; handler unit cases for refused and allowed names; and
an end-to-end engine test that measures each control independently.