Resolve weblog template resources within the active theme - #172
Resolve weblog template resources within the active theme#172snoopdave wants to merge 4 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
mraible
left a comment
mraible
left a comment
There was a problem hiding this comment.
Dropping the classpath loader and confining #include / #parse is the right fix, but the handler as written refuses Roller's own template ids, so every weblog on a shared (bundled) theme renders without its entries, sidebar, header and footer. That's a blocker; details inline.
Shared-theme template ids are <themeId>:<templateName> (SharedThemeFromDir lines 270 and 368), and the includeTemplate macro in WEB-INF/velocity/weblog.vm calls #parse($pageArg.id + '|' + $model.deviceType), i.e. basic:_day|standard. isOutsideNamespace sees a colon with no slash and returns null, which Velocity treats as "render nothing", with a WARN per include per page view. Only custom-theme weblogs (UUID template ids) keep working, which is also why the new test suite stays green: its allowed list never includes the theme:name|device shape.
Suggested fix: treat <something>:<name> as the theme namespace when the part before the colon has no /, \ or . in it (a scheme like file: / http: / jar: is followed by // or a path, and a Windows drive is a single letter), and drop the extension check entirely, since template names are free-form (basic-custom.css is a template in the basic theme). Then add basic:_day|standard and basic:basic-custom.css|standard to legitimateIncludesStillPass.
| int colon = normalized.indexOf(':'); | ||
| if (colon > -1) { | ||
| int slash = normalized.indexOf('/'); | ||
| return slash == -1 || colon < slash; |
There was a problem hiding this comment.
This refuses every shared-theme template id. SharedThemeFromDir builds ids as themeId + ":" + templateName (lines 270, 368) and weblog.vm's includeTemplate macro does #parse($pageArg.id + '|' + $model.deviceType), so the basic theme's weblog page asks for basic:_day|standard, gets null back, and renders nothing for that fragment. Every bundled theme is affected; only custom (UUID-id) templates pass. A scheme is followed by / (file:///, http://, jar:file:) or is a single drive letter, so something like colon > 1 && (slash == -1 || colon < slash) && normalized.charAt(colon + 1) != '/' is closer, and ThemeResourceLoader is the real authority on what a theme id looks like.
| // 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.
|
|
||
| # 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. |
There was a problem hiding this comment.
The commit message says nothing reads this copy. Rather than keep two hand-maintained copies of security-relevant config plus a test that scans both, I'd delete this one.
|
|
||
| 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.