This is an automated email from the ASF dual-hosted git repository. joerghoh pushed a commit to branch SLING-13382 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-api.git
commit 96fbd6a6453c176a1696ff613f2a8eef01d561c6 Author: Joerg Hoh <[email protected]> AuthorDate: Thu Oct 8 18:36:59 2026 +0200 SLING-13382 Warn on parent traversal in DeepReadValueMapDecorator do not allow to go up the tree when resolving properties for this map. --- pom.xml | 15 + .../api/wrappers/DeepReadValueMapDecorator.java | 64 +++- .../impl/DeepReadValueMapConfiguration.java | 67 +++++ .../apache/sling/api/wrappers/package-info.java | 2 +- .../wrappers/DeepReadValueMapDecoratorTest.java | 330 +++++++++++++++++++++ 5 files changed, 474 insertions(+), 4 deletions(-) diff --git a/pom.xml b/pom.xml index 9244f3c..ae49bde 100644 --- a/pom.xml +++ b/pom.xml @@ -75,6 +75,16 @@ <artifactId>org.osgi.annotation.versioning</artifactId> <scope>provided</scope> </dependency> + <dependency> + <groupId>org.osgi</groupId> + <artifactId>org.osgi.service.component.annotations</artifactId> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.osgi</groupId> + <artifactId>org.osgi.service.metatype.annotations</artifactId> + <scope>provided</scope> + </dependency> <dependency> <groupId>org.slf4j</groupId> @@ -95,6 +105,11 @@ </dependency> <!-- Testing --> + <dependency> + <groupId>org.slf4j</groupId> + <artifactId>slf4j-simple</artifactId> + <scope>test</scope> + </dependency> <dependency> <groupId>junit</groupId> <artifactId>junit</artifactId> diff --git a/src/main/java/org/apache/sling/api/wrappers/DeepReadValueMapDecorator.java b/src/main/java/org/apache/sling/api/wrappers/DeepReadValueMapDecorator.java index 51f73b5..6bfe6a8 100644 --- a/src/main/java/org/apache/sling/api/wrappers/DeepReadValueMapDecorator.java +++ b/src/main/java/org/apache/sling/api/wrappers/DeepReadValueMapDecorator.java @@ -18,27 +18,59 @@ */ package org.apache.sling.api.wrappers; +import java.util.Locale; + import org.apache.sling.api.resource.Resource; import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.api.resource.ResourceUtil; import org.apache.sling.api.resource.ValueMap; +import org.apache.sling.api.wrappers.impl.DeepReadValueMapConfiguration; import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * A value map wrapper which implements deep reading of properties * based on the resource tree. + * <p>Deep reads outside the starting resource's subtree, or whose resource paths + * cannot be normalized, log a WARN and continue the lookup by default. + * The OSGi configuration PID {@code org.apache.sling.api.wrappers.DeepReadValueMapDecorator} + * supports the boolean property {@code enforce} (default {@code false}). + * When enabled, these reads throw {@link IllegalArgumentException} before resource + * resolution instead. Configuration changes also apply to existing decorators. + * Diagnostics include the starting resource and requested resource path. + * This setting only controls this decorator, not other {@link ValueMap} implementations. * @since 2.5 (Sling API Bundle 2.7.0) */ public class DeepReadValueMapDecorator extends ValueMapDecorator { + private static final Logger LOG = LoggerFactory.getLogger(DeepReadValueMapDecorator.class); + + private final String resourcePath; + + private final String normalizedResourcePath; + private final String pathPrefix; private final ResourceResolver resolver; private final ValueMap base; - public DeepReadValueMapDecorator(final Resource resource, final ValueMap base) { + /** + * Creates a deep-reading view of the resource's properties. + * @param resource The starting resource. + * @param base The starting resource's properties. + * @throws IllegalArgumentException If the resource path cannot be normalized + * to an absolute path. + */ + public DeepReadValueMapDecorator(@NotNull final Resource resource, @NotNull final ValueMap base) { super(base); - this.pathPrefix = resource.getPath() + "/"; + this.resourcePath = resource.getPath(); + this.normalizedResourcePath = ResourceUtil.normalize(resourcePath); + if (normalizedResourcePath == null || !normalizedResourcePath.startsWith("/")) { + throw new IllegalArgumentException("Invalid resource path: " + sanitizeForMessage(resourcePath)); + } + this.pathPrefix = resourcePath + "/"; this.resolver = resource.getResourceResolver(); this.base = base; } @@ -48,7 +80,20 @@ public class DeepReadValueMapDecorator extends ValueMapDecorator { if (pos == -1) { return this.base; } - final Resource rsrc = this.resolver.getResource(pathPrefix + name.substring(0, pos)); + final String requestedPath = pathPrefix + name.substring(0, pos); + final String normalizedPath = ResourceUtil.normalize(requestedPath); + if (normalizedPath == null + || !(normalizedResourcePath.equals("/") + || normalizedPath.equals(normalizedResourcePath) + || normalizedPath.startsWith(normalizedResourcePath + "/"))) { + final String message = "Deep read outside resource subtree: resource '" + sanitizeForMessage(resourcePath) + + "', requested path '" + sanitizeForMessage(requestedPath) + "'"; + if (DeepReadValueMapConfiguration.isEnforcementEnabled()) { + throw new IllegalArgumentException(message); + } + LOG.warn(message); + } + final Resource rsrc = this.resolver.getResource(requestedPath); if (rsrc != null) { final ValueMap vm = rsrc.adaptTo(ValueMap.class); if (vm != null) { @@ -58,6 +103,19 @@ public class DeepReadValueMapDecorator extends ValueMapDecorator { return ValueMap.EMPTY; // fall back } + private static String sanitizeForMessage(final String value) { + final StringBuilder result = new StringBuilder(value.length()); + for (int i = 0; i < value.length(); i++) { + final char character = value.charAt(i); + if (Character.isISOControl(character) || character == '\u2028' || character == '\u2029') { + result.append(String.format(Locale.ROOT, "\\u%04x", (int) character)); + } else { + result.append(character); + } + } + return result.toString(); + } + private String getPropertyName(final String name) { final int pos = name.lastIndexOf("/"); if (pos == -1) { diff --git a/src/main/java/org/apache/sling/api/wrappers/impl/DeepReadValueMapConfiguration.java b/src/main/java/org/apache/sling/api/wrappers/impl/DeepReadValueMapConfiguration.java new file mode 100644 index 0000000..2dbc290 --- /dev/null +++ b/src/main/java/org/apache/sling/api/wrappers/impl/DeepReadValueMapConfiguration.java @@ -0,0 +1,67 @@ +/* + * 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.sling.api.wrappers.impl; + +import org.jetbrains.annotations.NotNull; +import org.osgi.service.component.annotations.Activate; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Deactivate; +import org.osgi.service.component.annotations.Modified; +import org.osgi.service.metatype.annotations.AttributeDefinition; +import org.osgi.service.metatype.annotations.Designate; +import org.osgi.service.metatype.annotations.ObjectClassDefinition; + +@Component( + service = {}, + immediate = true, + configurationPid = "org.apache.sling.api.wrappers.DeepReadValueMapDecorator") +@Designate(ocd = DeepReadValueMapConfiguration.Configuration.class) +public class DeepReadValueMapConfiguration { + + @ObjectClassDefinition( + id = "org.apache.sling.api.wrappers.DeepReadValueMapDecorator", + name = "Apache Sling Deep Read Value Map", + description = "Controls resource subtree containment for DeepReadValueMapDecorator.") + public @interface Configuration { + @AttributeDefinition( + name = "Enforce subtree containment", + description = "Throw IllegalArgumentException for out-of-subtree or invalid deep reads. " + + "When disabled, log a WARN and allow the lookup.") + boolean enforce() default false; + } + + private static volatile boolean enforcementEnabled; + + public DeepReadValueMapConfiguration() {} + + @Activate + @Modified + public void configure(@NotNull final Configuration configuration) { + enforcementEnabled = configuration.enforce(); + } + + @Deactivate + public void deactivate() { + enforcementEnabled = false; + } + + public static boolean isEnforcementEnabled() { + return enforcementEnabled; + } +} diff --git a/src/main/java/org/apache/sling/api/wrappers/package-info.java b/src/main/java/org/apache/sling/api/wrappers/package-info.java index 0baf621..2446166 100644 --- a/src/main/java/org/apache/sling/api/wrappers/package-info.java +++ b/src/main/java/org/apache/sling/api/wrappers/package-info.java @@ -21,7 +21,7 @@ * This package provides wrapper classes for the Javax based Servlet API and the Jakarta Servlet API to wrap entities from one API to the other API. * Most wrapper classes implement a method {@code getWrappedObject()} which returns the wrapped object. */ -@Version("2.11.0") +@Version("2.11.1") package org.apache.sling.api.wrappers; import org.osgi.annotation.versioning.Version; diff --git a/src/test/java/org/apache/sling/api/wrappers/DeepReadValueMapDecoratorTest.java b/src/test/java/org/apache/sling/api/wrappers/DeepReadValueMapDecoratorTest.java new file mode 100644 index 0000000..2b45be0 --- /dev/null +++ b/src/test/java/org/apache/sling/api/wrappers/DeepReadValueMapDecoratorTest.java @@ -0,0 +1,330 @@ +/* + * 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.sling.api.wrappers; + +import java.io.ByteArrayOutputStream; +import java.io.PrintStream; +import java.nio.charset.StandardCharsets; +import java.util.Map; + +import org.apache.sling.api.resource.AbstractResource; +import org.apache.sling.api.resource.Resource; +import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.api.resource.ValueMap; +import org.apache.sling.api.wrappers.impl.DeepReadValueMapConfiguration; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.CALLS_REAL_METHODS; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +/** + * Tests deep reads relative to {@code /content/site/page}, using this conceptual + * resource structure: + * <pre> + * /content/site + * page starting resource; base map has title = "base" + * child inside the permitted subtree + * grandchild inside the permitted subtree + * sibling outside the permitted subtree + * page2 outside, despite sharing the "/content/site/page" prefix + * </pre> + */ +public class DeepReadValueMapDecoratorTest { + + private final DeepReadValueMapConfiguration component = new DeepReadValueMapConfiguration(); + private ResourceResolver resolver; + private DeepReadValueMapDecorator decorator; + private ByteArrayOutputStream logOutput; + private PrintStream originalError; + private PrintStream capturedError; + + @Before + public void setUp() { + component.deactivate(); + originalError = System.err; + logOutput = new ByteArrayOutputStream(); + capturedError = new PrintStream(logOutput, true, StandardCharsets.UTF_8); + System.setErr(capturedError); + resolver = mock(ResourceResolver.class); + decorator = createDecorator("/content/site/page"); + logOutput.reset(); + } + + @After + public void tearDown() { + component.deactivate(); + System.setErr(originalError); + capturedError.close(); + } + + /** + * Creates a decorator whose starting resource has {@code title = "base"}. + * All decorators use the same resolver mock, but this does not create + * resources in a repository or stub any deep-read targets. + * @param path The starting resource's path, used as the containment boundary. + */ + private DeepReadValueMapDecorator createDecorator(final String path) { + final Resource resource = mock(Resource.class); + when(resource.getPath()).thenReturn(path); + when(resource.getResourceResolver()).thenReturn(resolver); + return new DeepReadValueMapDecorator(resource, new ValueMapDecorator(Map.of("title", "base"))); + } + + /** + * Calls the configuration component directly, without an OSGi runtime. + * The policy is shared by all decorators, including ones already created. + * @param enforce {@code true} to throw on violations; {@code false} to warn + * and continue the lookup. + */ + private void configure(final boolean enforce) { + final DeepReadValueMapConfiguration.Configuration configuration = + mock(DeepReadValueMapConfiguration.Configuration.class); + when(configuration.enforce()).thenReturn(enforce); + component.configure(configuration); + } + + /** + * Makes one exact resolver lookup return a resource with {@code title = "target"}. + * For example, to read {@code ../sibling/title} from the standard starting + * resource, stub {@code /content/site/page/../sibling}. The resolver mock + * does not normalize dots or slashes; unstubbed lookups return {@code null}. + * @param path The complete resource lookup string, excluding the property name. + */ + private void stubTarget(final String path) { + final Resource target = mock(Resource.class); + when(resolver.getResource(path)).thenReturn(target); + when(target.adaptTo(ValueMap.class)).thenReturn(new ValueMapDecorator(Map.of("title", "target"))); + } + + /** + * Checks all four deep-read methods against a target configured by + * {@link #stubTarget(String)}. Each getter must return {@code "target"}, + * and {@code containsKey} must report that the property exists. + * @param map The decorator being tested. + * @param key The deep-read key, including the final {@code title} property name. + */ + private void assertTargetReads(final DeepReadValueMapDecorator map, final String key) { + assertEquals("target", map.get(key)); + assertEquals("target", map.get(key, String.class)); + assertEquals("target", map.get(key, "default")); + assertTrue(map.containsKey(key)); + } + + /** + * Returns the captured standard-error output since setup, including any + * WARN messages emitted by the test logging backend. Reading does not + * clear the buffer, so later assertions also see earlier messages. + */ + private String logs() { + return logOutput.toString(StandardCharsets.UTF_8); + } + + @Test + public void testDefaultModeWarnsAndAllowsAllReadMethods() { + final String requestedPath = "/content/site/page/../sibling"; + stubTarget(requestedPath); + + assertTargetReads(decorator, "../sibling/title"); + + // all 4 relevant methods have been invoked + assertEquals(4, logs().lines().filter(line -> line.contains("WARN")).count()); + assertTrue(logs().contains("resource '/content/site/page'")); + assertTrue(logs().contains("requested path '" + requestedPath + "'")); + } + + @Test + public void testEnforcementRejectsAllReadMethodsBeforeResolution() { + configure(true); + final String key = "../sibling/title"; + final String expected = "Deep read outside resource subtree: resource '/content/site/page', " + + "requested path '/content/site/page/../sibling'"; + + assertEquals( + expected, + assertThrows(IllegalArgumentException.class, () -> decorator.get(key)) + .getMessage()); + assertEquals( + expected, + assertThrows(IllegalArgumentException.class, () -> decorator.get(key, String.class)) + .getMessage()); + assertEquals( + expected, + assertThrows(IllegalArgumentException.class, () -> decorator.get(key, "default")) + .getMessage()); + assertEquals( + expected, + assertThrows(IllegalArgumentException.class, () -> decorator.containsKey(key)) + .getMessage()); + verifyNoInteractions(resolver); + assertTrue(logs().isEmpty()); + } + + @Test + public void testContainedPathsAreAllowedInBothModes() { + final String[] keys = {"child/title", "child/../title", "./title", "child//grandchild/title"}; + for (final boolean enforce : new boolean[] {false, true}) { + configure(enforce); + for (final String key : keys) { + stubTarget("/content/site/page/" + key.substring(0, key.lastIndexOf('/'))); + assertTargetReads(decorator, key); + } + } + assertTrue(logs().isEmpty()); + } + + @Test + public void testSimilarPrefixDoesNotEstablishContainment() { + configure(true); + + assertThrows(IllegalArgumentException.class, () -> decorator.get("../page2/title")); + verifyNoInteractions(resolver); + } + + @Test + public void testInvalidPathsAreRejectedInEnforcementMode() { + configure(true); + + assertThrows(IllegalArgumentException.class, () -> decorator.get("../../../../secret/title")); + assertThrows(IllegalArgumentException.class, () -> decorator.get(".../title")); + verifyNoInteractions(resolver); + } + + @Test + public void testInvalidPathsWarnAndContinueInLoggingMode() { + assertNull(decorator.get("../../../../secret/title")); + assertNull(decorator.get(".../title")); + + verify(resolver).getResource("/content/site/page/../../../../secret"); + verify(resolver).getResource("/content/site/page/..."); + assertEquals(2, logs().lines().filter(line -> line.contains("WARN")).count()); + } + + @Test + public void testRootAllowsAnyValidAbsoluteTargetButNotAboveRoot() { + configure(true); + final DeepReadValueMapDecorator root = createDecorator("/"); + stubTarget("//content"); + stubTarget("//."); + + assertTargetReads(root, "content/title"); + assertTargetReads(root, "./title"); + assertThrows(IllegalArgumentException.class, () -> root.get("../title")); + assertFalse(logs().contains("WARN")); + } + + @Test + public void testConfigurationChangesAffectExistingDecorators() { + configure(true); + assertThrows(IllegalArgumentException.class, () -> decorator.get("../sibling/title")); + + configure(false); + assertNull(decorator.get("../sibling/title")); + assertTrue(logs().contains("WARN")); + + configure(true); + assertThrows(IllegalArgumentException.class, () -> decorator.get("../sibling/title")); + + component.deactivate(); + assertNull(decorator.get("../sibling/title")); + assertEquals(2, logs().lines().filter(line -> line.contains("WARN")).count()); + } + + @Test + public void testBaseAndMissingPropertiesRetainTheirBehavior() { + configure(true); + + assertEquals("base", decorator.get("title")); + assertEquals("base", decorator.get("title", String.class)); + assertEquals("base", decorator.get("title", "default")); + assertTrue(decorator.containsKey("title")); + assertNull(decorator.get((Object) null)); + assertFalse(decorator.containsKey(null)); + assertNull(decorator.get("missing")); + assertEquals("default", decorator.get("missing", "default")); + verifyNoInteractions(resolver); + + assertNull(decorator.get("child/missing")); + assertFalse(decorator.containsKey("child/missing")); + assertEquals("default", decorator.get("child/missing", "default")); + when(resolver.getResource(anyString())).thenReturn(mock(Resource.class)); + assertNull(decorator.get("child/missing")); + assertTrue(logs().isEmpty()); + } + + @Test + public void testDiagnosticsEscapeControlCharactersInBothModes() { + final DeepReadValueMapDecorator map = createDecorator("/content/site\npage"); + final String key = "../sibling\r\u001b\u2028/title"; + assertNull(map.get(key)); + final String message = "Deep read outside resource subtree: resource '/content/site\\u000apage', " + + "requested path '/content/site\\u000apage/../sibling\\u000d\\u001b\\u2028'"; + assertTrue(logs().contains(message)); + assertEquals(1, logs().lines().filter(line -> line.contains("WARN")).count()); + + configure(true); + assertEquals( + message, + assertThrows(IllegalArgumentException.class, () -> map.get(key)).getMessage()); + } + + @Test + public void testStartingResourcePathIsNormalizedForContainment() { + configure(true); + final DeepReadValueMapDecorator map = createDecorator("/content//site/page/"); + stubTarget("/content//site/page//child"); + + assertTargetReads(map, "child/title"); + assertThrows(IllegalArgumentException.class, () -> map.get("../sibling/title")); + assertTrue(logs().isEmpty()); + } + + @Test + public void testInvalidStartingResourceIsRejected() { + assertThrows(IllegalArgumentException.class, () -> createDecorator("/../content")); + assertThrows(IllegalArgumentException.class, () -> createDecorator("relative")); + } + + @Test + public void testAbstractResourceFallbackUsesConfiguredPolicy() { + final AbstractResource resource = mock(AbstractResource.class, CALLS_REAL_METHODS); + when(resource.getPath()).thenReturn("/content/site/page"); + when(resource.getResourceResolver()).thenReturn(resolver); + doReturn(null).when(resource).adaptTo(ValueMap.class); + doReturn(Map.of("title", "base")).when(resource).adaptTo(Map.class); + final ValueMap map = resource.getValueMap(); + + assertEquals("base", map.get("title")); + assertNull(map.get("../sibling/title")); + assertTrue(logs().contains("WARN")); + configure(true); + assertThrows(IllegalArgumentException.class, () -> map.get("../sibling/title")); + } +}
