diff --git a/CHANGES.md b/CHANGES.md index 4871cccbb..67c19db7a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -18,8 +18,14 @@ - Pasted images travel in the form POST as base64. Tomcat's `maxPostSize` defaults to 2 MB, so larger pastes are refused with a "form is too large" message; raise `maxPostSize` to accept them. + ### Behaviour changes worth reading before upgrading +- **Templates can no longer reach the objects behind the template wrappers.** + `$weblog.pojo`, `$entry.pojo` and `getPojo()` no longer resolve in weblog + templates. A custom theme that uses them will print the reference text + as-is, without an error. Use the wrapper's own properties instead, for + example `$weblog.handle` or `$entry.title`. - **Planet is off by default.** `planet.aggregator.enabled` now defaults to `false`. While it is off, the Planet admin pages, `/planetrss` and the Planet background tasks (`RefreshRollerPlanetTask`, `SyncWebsitesTask`) do diff --git a/app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/WeblogEntryWrapper.java b/app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/WeblogEntryWrapper.java index 1dc6a54dc..4ed80b184 100644 --- a/app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/WeblogEntryWrapper.java +++ b/app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/WeblogEntryWrapper.java @@ -289,8 +289,13 @@ public String getSearchDescription() { * we don't really want to do this, but it's necessary * because some parts of the rendering process still need the * orginal pojo object. + * + * Not public on purpose: templates render under an introspection + * sandbox that reaches public methods only, so the wrapped object + * itself must stay out of the template-visible surface. Java code + * that needs it goes through {@link Wrappers#unwrap(WeblogEntryWrapper)}. */ - public WeblogEntry getPojo() { + WeblogEntry getPojo() { return this.pojo; } diff --git a/app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/WeblogWrapper.java b/app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/WeblogWrapper.java index 71ff4d33e..d8341c9ba 100644 --- a/app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/WeblogWrapper.java +++ b/app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/WeblogWrapper.java @@ -337,9 +337,14 @@ public long getEntryCount() { * this is a special method to access the original pojo * we don't really want to do this, but it's necessary * because some parts of the rendering process still need the - * original pojo object + * original pojo object. + * + * Not public on purpose: templates render under an introspection + * sandbox that reaches public methods only, so the wrapped object + * itself must stay out of the template-visible surface. Java code + * that needs it goes through {@link Wrappers#unwrap(WeblogWrapper)}. */ - public Weblog getPojo() { + Weblog getPojo() { return this.pojo; } } diff --git a/app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/Wrappers.java b/app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/Wrappers.java new file mode 100644 index 000000000..6f9e369c1 --- /dev/null +++ b/app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/Wrappers.java @@ -0,0 +1,49 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. 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. + */ + +package org.apache.roller.weblogger.pojos.wrapper; + +import org.apache.roller.weblogger.pojos.Weblog; +import org.apache.roller.weblogger.pojos.WeblogEntry; + +/** + * Java-side access to the objects behind the template wrappers. + * + *

A few parts of the rendering process (pagers, permission checks) + * need the wrapped object itself rather than the wrapper. Those callers + * are Java code in other packages, so the wrappers keep their pojo + * accessor package-private — templates render under an introspection + * sandbox that reaches public methods only, and the wrapped objects + * must stay out of the template-visible surface. This class is the + * one door those callers use instead; it is never put in a template + * context. + */ +public final class Wrappers { + + private Wrappers() { + } + + public static Weblog unwrap(WeblogWrapper wrapper) { + return wrapper.getPojo(); + } + + public static WeblogEntry unwrap(WeblogEntryWrapper wrapper) { + return wrapper.getPojo(); + } +} diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/rendering/model/SiteModel.java b/app/src/main/java/org/apache/roller/weblogger/ui/rendering/model/SiteModel.java index a0c7acfb7..9afb98830 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/rendering/model/SiteModel.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/rendering/model/SiteModel.java @@ -46,6 +46,7 @@ import org.apache.roller.weblogger.pojos.wrapper.WeblogEntryCommentWrapper; import org.apache.roller.weblogger.pojos.wrapper.WeblogEntryWrapper; import org.apache.roller.weblogger.pojos.wrapper.WeblogWrapper; +import org.apache.roller.weblogger.pojos.wrapper.Wrappers; import org.apache.roller.weblogger.ui.rendering.pagers.CommentsPager; import org.apache.roller.weblogger.ui.rendering.pagers.Pager; import org.apache.roller.weblogger.ui.rendering.pagers.UsersPager; @@ -189,7 +190,7 @@ public Pager getWeblogEntriesPager(WeblogWrapper queryWeblog return new WeblogEntriesListPager( urlStrategy, - pagerUrl, queryWeblog.getPojo(), user, cat, + pagerUrl, Wrappers.unwrap(queryWeblog), user, cat, tags, weblogRequest.getLocale(), sinceDays, diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/rendering/model/UtilitiesModel.java b/app/src/main/java/org/apache/roller/weblogger/ui/rendering/model/UtilitiesModel.java index 89f1ddc92..f6a332215 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/rendering/model/UtilitiesModel.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/rendering/model/UtilitiesModel.java @@ -31,6 +31,7 @@ import org.apache.commons.logging.LogFactory; import org.apache.roller.weblogger.WebloggerException; import org.apache.roller.weblogger.pojos.wrapper.WeblogWrapper; +import org.apache.roller.weblogger.pojos.wrapper.Wrappers; import org.apache.roller.weblogger.ui.rendering.util.WeblogRequest; import org.apache.roller.util.DateUtil; import org.apache.roller.util.RegexUtil; @@ -82,7 +83,7 @@ public void init(Map initData) throws WebloggerException { public boolean isUserAuthorizedToAuthor(WeblogWrapper weblog) { try { if (parsedRequest.getAuthenticUser() != null) { - return weblog.getPojo().hasUserPermission( + return Wrappers.unwrap(weblog).hasUserPermission( parsedRequest.getUser(), WeblogPermission.POST); } } catch (Exception e) { @@ -94,7 +95,7 @@ public boolean isUserAuthorizedToAuthor(WeblogWrapper weblog) { public boolean isUserAuthorizedToAdmin(WeblogWrapper weblog) { try { if (parsedRequest.getAuthenticUser() != null) { - return weblog.getPojo().hasUserPermission( + return Wrappers.unwrap(weblog).hasUserPermission( parsedRequest.getUser(), WeblogPermission.ADMIN); } } catch (Exception e) { diff --git a/app/src/test/java/org/apache/roller/weblogger/pojos/wrapper/WrapperPojoConfinementTest.java b/app/src/test/java/org/apache/roller/weblogger/pojos/wrapper/WrapperPojoConfinementTest.java new file mode 100644 index 000000000..7af88ae07 --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/pojos/wrapper/WrapperPojoConfinementTest.java @@ -0,0 +1,168 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. 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.pojos.wrapper; + +import java.io.InputStream; +import java.io.StringWriter; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.lang.reflect.Method; +import java.util.Arrays; +import java.util.Properties; + +import org.apache.roller.weblogger.pojos.User; +import org.apache.roller.weblogger.pojos.Weblog; +import org.apache.roller.weblogger.pojos.WeblogEntry; +import org.apache.velocity.VelocityContext; +import org.apache.velocity.app.VelocityEngine; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +/** + * Covers what a template can reach through the pojo wrappers. + * + *

Weblog templates are authored by weblog administrators, a role Roller + * treats as untrusted and renders under SecureUberspector. The + * wrappers exist precisely so that role only sees a safe surface, so the + * wrapped objects themselves must stay out of the template-visible surface. + * Java rendering code that does need them goes through {@link Wrappers}, + * which is never put in a template context. + */ +public class WrapperPojoConfinementTest { + + private static final Class[] WRAPPED_TYPES = { + Weblog.class, WeblogEntry.class, User.class, + }; + + private static VelocityEngine engine; + + /** The configuration the weblog renderer loads. */ + private static final Path VELOCITY_CONFIG = + Paths.get("src", "main", "webapp", "WEB-INF", "velocity.properties"); + + @BeforeAll + public static void setUpEngine() throws Exception { + // Load the shipped renderer configuration, so the introspection + // settings under test are the real ones. Only the settings that need + // a running webapp (resource loaders, macro libraries, the include + // handler and the logger) are dropped. + Properties shipped = new Properties(); + try (InputStream in = Files.newInputStream(VELOCITY_CONFIG)) { + shipped.load(in); + } + assertNotNull(shipped.getProperty("introspector.uberspect.class"), + VELOCITY_CONFIG + " no longer sets an uberspector"); + + Properties props = new Properties(); + for (String key : shipped.stringPropertyNames()) { + if (!key.startsWith("resource.loader") + && !key.startsWith("velocimacro.") + && !key.startsWith("event_handler.") + && !key.startsWith("runtime.log.logsystem")) { + props.setProperty(key, shipped.getProperty(key)); + } + } + engine = new VelocityEngine(); + engine.init(props); + } + + private static String render(String template, VelocityContext ctx) + throws Exception { + StringWriter out = new StringWriter(); + engine.evaluate(ctx, out, "wrapper-confinement", template); + return out.toString(); + } + + /** + * The sandbox reaches public methods only, so none of the wrapper's + * template-visible methods may hand out the wrapped types — directly or + * through any other public signature the classes carry. + */ + @Test + public void wrapperSurfaceHandsOutNoWrappedObjects() { + for (Class wrapper : Arrays.asList( + WeblogWrapper.class, WeblogEntryWrapper.class)) { + for (Method method : wrapper.getMethods()) { + for (Class wrapped : WRAPPED_TYPES) { + if (wrapped.equals(method.getReturnType())) { + fail(wrapper.getSimpleName() + "." + method.getName() + + " hands out " + wrapped.getSimpleName() + + " through its template-visible surface"); + } + } + } + } + } + + /** + * End to end against the real engine: an unresolved reference is left + * in the output as-is, so each of these renders its own literal text + * when the sandbox finds nothing to call. + */ + @Test + public void templatesDoNotResolveTheWrappedObjects() throws Exception { + Weblog weblog = new Weblog(); + weblog.setName("confinement"); + WeblogEntry entry = new WeblogEntry(); + + VelocityContext ctx = new VelocityContext(); + ctx.put("weblog", WeblogWrapper.wrap(weblog, null)); + ctx.put("entry", WeblogEntryWrapper.wrap(entry, null)); + + // control: the wrappers themselves are resolvable + assertTrue(render("[$weblog.name]", ctx).contains("confinement"), + "control failed: the wrapper is not visible to the engine, so " + + "the assertions below can show nothing"); + + String[] mustStayUnresolved = { + "$weblog.pojo", + "$weblog.getPojo()", + "$weblog.pojo.handle", + "$entry.pojo", + "$entry.getPojo()", + }; + for (String reference : mustStayUnresolved) { + assertEquals(reference, render(reference, ctx), + "the template resolved [" + reference + "] to one of the " + + "wrapped objects"); + } + } + + /** + * The Java door still opens: the pagers and permission checks keep + * working on the same object the wrapper holds. + */ + @Test + public void javaCodeStillReachesTheWrappedObjects() { + Weblog weblog = new Weblog(); + WeblogEntry entry = new WeblogEntry(); + + assertSame(weblog, Wrappers.unwrap(WeblogWrapper.wrap(weblog, null))); + assertSame(entry, Wrappers.unwrap(WeblogEntryWrapper.wrap(entry, null))); + } +}