This is an automated email from the ASF dual-hosted git repository. jsedding pushed a commit to branch jsedding/SLING-12366-read-inputstream-from-closed-session in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-xss.git
commit 500ac9a7df72c7a25c69942b64642aea79349e23 Author: Julian Sedding <jsedd...@apache.org> AuthorDate: Tue Jul 2 12:42:44 2024 +0200 SLING-12366 - Failure to read from InputStream backed by closed session - add failing test case - introduce sling-mock-oak, required for test case - update sling-mocks and sling.api and sling.serviceusermapper in order to get a working test setup - restructure unit tests while working around "new" default mocks for XSSAPI and XSSFilter provided by sling-mocks --- pom.xml | 25 +++++-- .../org/apache/sling/xss/impl/XSSAPIImplTest.java | 82 +++++++++------------- .../apache/sling/xss/impl/XSSFilterImplTest.java | 63 +++++++++++------ 3 files changed, 94 insertions(+), 76 deletions(-) diff --git a/pom.xml b/pom.xml index 859bf1b..8a77747 100644 --- a/pom.xml +++ b/pom.xml @@ -77,9 +77,9 @@ <ignore>org.apache.xml.*</ignore> <ignore>org.apache.commons.*</ignore> <ignore>javax.servlet.jsp.*</ignore> - <ignore>org.apache.avalon.*</ignore> - <ignore>org.apache.log.*</ignore> - <ignore>org.owasp.validator.html.*</ignore> + <ignore>org.apache.avalon.*</ignore> + <ignore>org.apache.log.*</ignore> + <ignore>org.owasp.validator.html.*</ignore> <ignore>org.w3c.dom.svg.*</ignore> <!-- Classes with newer method signatures dynamically loaded by the java-html-sanitizer java10 shim --> <ignore>java.util.Set</ignore> @@ -292,13 +292,13 @@ <dependency> <groupId>org.apache.sling</groupId> <artifactId>org.apache.sling.api</artifactId> - <version>2.11.0</version> + <version>2.16.4</version> <scope>provided</scope> </dependency> <dependency> <groupId>org.apache.sling</groupId> <artifactId>org.apache.sling.serviceusermapper</artifactId> - <version>1.2.0</version> + <version>1.4.6</version> <scope>provided</scope> </dependency> <dependency> @@ -351,7 +351,20 @@ <dependency> <groupId>org.apache.sling</groupId> <artifactId>org.apache.sling.testing.sling-mock.junit5</artifactId> - <version>2.3.4</version> + <version>3.0.0</version> + <scope>test</scope> + <exclusions> + <!-- prevent loading older version of the xss api, as their OSGi metadata can interfere with tests --> + <exclusion> + <groupId>org.apache.sling</groupId> + <artifactId>org.apache.sling.xss</artifactId> + </exclusion> + </exclusions> + </dependency> + <dependency> + <groupId>org.apache.sling</groupId> + <artifactId>org.apache.sling.testing.sling-mock-oak</artifactId> + <version>3.0.0-1.16.0</version> <scope>test</scope> </dependency> <dependency> diff --git a/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java b/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java index 8fbfb96..355c920 100644 --- a/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java +++ b/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java @@ -17,13 +17,19 @@ package org.apache.sling.xss.impl; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import java.util.Collection; +import java.util.Collections; import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; import java.util.regex.Pattern; import org.apache.commons.lang.reflect.FieldUtils; @@ -36,12 +42,14 @@ import org.apache.sling.testing.mock.sling.junit5.SlingContextExtension; import org.apache.sling.xss.XSSAPI; import org.apache.sling.xss.impl.status.XSSStatusService; import org.apache.sling.xss.impl.xml.Attribute; -import org.junit.jupiter.api.AfterEach; +import org.jetbrains.annotations.NotNull; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; +import org.osgi.framework.Constants; +import org.osgi.framework.InvalidSyntaxException; import org.osgi.framework.ServiceReference; @ExtendWith(SlingContextExtension.class) @@ -51,6 +59,8 @@ public class XSSAPIImplTest { private static final String RUBBISH_JSON = "[\"rubbish\"]"; private static final String RUBBISH_XML = "<rubbish/>"; + private final AtomicInteger serviceRanking = new AtomicInteger(100); + public SlingContext context = new SlingContext(); private XSSAPI xssAPI; @@ -58,61 +68,59 @@ public class XSSAPIImplTest { /** * Due to how OSGi mocks are currently designed, it's impossible to unregister services. Therefore this method has to be explicitly * called by each method that needs the default setup. - * + * <p> * The only exception currently is {@link #testGetValidHrefWithoutHrefConfig()}. + * + * @return */ - private void setUp() { - context.registerInjectActivateService(new XSSFilterImpl()); - context.registerInjectActivateService(new XSSAPIImpl()); - xssAPI = context.getService(XSSAPI.class); + private @NotNull <T> T activateComponent(T component, Map<String, Object> properties) { + // A higher service ranking helps make sure that none of the mock implementations + // registered by default take precedence over the services registered here. Such + // mock services exist at least for both XSSAPI and XSSFilter. + Map<String, Object> props = new HashMap<String, Object>(properties) {{ + put(Constants.SERVICE_RANKING, serviceRanking.getAndAdd(100)); + }}; + return context.registerInjectActivateService(component, props); } @BeforeEach - public void before() { + public void before() throws Exception{ MetricsService metricsService = mock(MetricsService.class); when(metricsService.counter(anyString())).thenReturn(mock(Counter.class)); context.registerService(MetricsService.class, metricsService); context.registerService(ServiceUserMapped.class, mock(ServiceUserMapped.class)); context.registerService(new XSSStatusService()); - } - - @AfterEach - public void tearDown() { - xssAPI = null; + activateComponent(new XSSFilterImpl(), Collections.emptyMap()); + xssAPI = activateComponent(new XSSAPIImpl(), Collections.emptyMap()); } @ParameterizedTest @MethodSource("dataForEncodeToHtml") public void testEncodeForHTML(String source, String expected) { - setUp(); assertEquals(expected, xssAPI.encodeForHTML(source), "HTML Encoding '" + source + "'"); } @ParameterizedTest @MethodSource("dataForEncodeToHtmlAttr") public void testEncodeForHTMLAttr(String source, String expected) { - setUp(); assertEquals(expected, xssAPI.encodeForHTMLAttr(source), "HTML Encoding '" + source + "'"); } @ParameterizedTest @MethodSource("dataForEncodeToXml") public void testEncodeForXML(String source, String expected) { - setUp(); assertEquals(expected, xssAPI.encodeForXML(source), "XML Encoding '" + source + "'"); } @ParameterizedTest @MethodSource("dataForEncodeToXmlAttr") public void testEncodeForXMLAttr(String source, String expected) { - setUp(); assertEquals(expected, xssAPI.encodeForXMLAttr(source), "XML Encoding '" + source + "'"); } @ParameterizedTest @MethodSource("dataForFilterHtml") public void testFilterHTML(String source, String expected) { - setUp(); assertEquals(expected, xssAPI.filterHTML(source), "Filtering '" + source + "'"); } @@ -124,24 +132,26 @@ public class XSSAPIImplTest { @ParameterizedTest @MethodSource("dataForValidHref") public void testGetValidHref(String input, String expected) { - setUp(); testHref(input, expected); } @ParameterizedTest @MethodSource("dataForValidHref") - public void testGetValidHrefWithoutHrefConfig(String input, String expected) throws ReflectiveOperationException { + public void testGetValidHrefWithoutHrefConfig(String input, String expected) throws ReflectiveOperationException, InvalidSyntaxException { context.load().binaryFile("/configWithoutHref.xml", "/apps/sling/xss/configWithoutHref.xml"); - context.registerInjectActivateService(new XSSFilterImpl(), new HashMap<String, Object>(){{ + XSSFilterImpl xssFilterImpl = activateComponent(new XSSFilterImpl(), new HashMap<String, Object>(){{ put("policyPath", "/apps/sling/xss/configWithoutHref.xml"); }}); - context.registerInjectActivateService(new XSSAPIImpl()); - xssAPI = context.getService(XSSAPI.class); - ServiceReference<ResourceChangeListener> xssFilterRCL = context.bundleContext().getServiceReference(ResourceChangeListener.class); + XSSAPIImpl xssAPI = activateComponent(new XSSAPIImpl(), Collections.emptyMap()); + Collection<ServiceReference<ResourceChangeListener>> xssFilterRCLs = context.bundleContext().getServiceReferences( + ResourceChangeListener.class, + String.format("(%s=%s)",ResourceChangeListener.PATHS, "/apps/sling/xss/configWithoutHref.xml")); + assertFalse(xssFilterRCLs.isEmpty(), "Expected at least one ResourceChangeListener with the correct path to be registered"); + ServiceReference<ResourceChangeListener> xssFilterRCL = xssFilterRCLs.iterator().next(); assertEquals("/apps/sling/xss/configWithoutHref.xml", xssFilterRCL.getProperty(ResourceChangeListener.PATHS)); // Load AntiSamy configuration without href filter XSSFilterImpl xssFilter = (XSSFilterImpl) FieldUtils.getField(XSSAPIImpl.class, "xssFilter", true).get(xssAPI); - + assertSame(xssFilterImpl, xssFilter); Attribute hrefAttribute = (Attribute) FieldUtils.getField(XSSFilterImpl.class, "hrefAttribute", true).get(xssFilter); assertEquals(hrefAttribute, XSSFilterImpl.DEFAULT_HREF_ATTRIBUTE); @@ -152,8 +162,6 @@ public class XSSAPIImplTest { @ParameterizedTest @MethodSource("dataForValidInteger") public void testGetValidInteger(String source, String expectedAsString) { - setUp(); - Integer expected = (expectedAsString != null) ? Integer.parseInt(expectedAsString) : null; assertEquals(expected, xssAPI.getValidInteger(source, 123), "Validating integer '" + source + "'"); } @@ -161,8 +169,6 @@ public class XSSAPIImplTest { @ParameterizedTest @MethodSource("dataForValidLong") public void testGetValidLong(String source, String expectedAsString) { - setUp(); - Long expected = (expectedAsString != null) ? Long.parseLong(expectedAsString) : null; assertEquals(expected, xssAPI.getValidLong(source, 123), "Validating long '" + source + "'"); } @@ -170,8 +176,6 @@ public class XSSAPIImplTest { @ParameterizedTest @MethodSource("dataForValidDouble") public void testGetValidDouble(String source, String expectedAsString) { - setUp(); - Double expected = (expectedAsString != null) ? Double.parseDouble(expectedAsString) : null; assertEquals(expected, xssAPI.getValidDouble(source, 123), "Validating double '" + source + "'"); } @@ -179,32 +183,24 @@ public class XSSAPIImplTest { @ParameterizedTest @MethodSource("dataForValidDimension") public void testGetValidDimension(String source, String expected) { - setUp(); - assertEquals(expected, xssAPI.getValidDimension(source, "123"), "Validating dimension '" + source + "'"); } @ParameterizedTest @MethodSource("dataForValidJSString") public void testEncodeForJSString(String source, String expected) { - setUp(); - assertEquals(expected, xssAPI.encodeForJSString(source), "Encoding '" + source + "'"); } @ParameterizedTest @MethodSource("dataForValidJSToken") public void testGetValidJSToken(String source, String expected) { - setUp(); - assertEquals(expected, xssAPI.getValidJSToken(source, RUBBISH), "Validating Javascript token '" + source + "'"); } @ParameterizedTest @MethodSource("dataForCSSString") public void testEncodeForCSSString(String source, String expected) { - setUp(); - String result = xssAPI.encodeForCSSString(source); assertEquals(expected, result, "Encoding '" + source + "'"); } @@ -212,8 +208,6 @@ public class XSSAPIImplTest { @ParameterizedTest @MethodSource("dataForValidStyleToken") public void testGetValidStyleToken(String source, String expected) { - setUp(); - String result = xssAPI.getValidStyleToken(source, RUBBISH); if (result == null || !result.equals(expected)) { fail("Validating style token '" + source + "', expecting '" + expected + "', but got '" + result + "'"); @@ -223,8 +217,6 @@ public class XSSAPIImplTest { @ParameterizedTest @MethodSource("dataForValidCSSColor") public void testGetValidCSSColor(String source, String expected) { - setUp(); - String result = xssAPI.getValidCSSColor(source, RUBBISH); if (result == null || !result.equals(expected)) { fail("Validating CSS Color '" + source + "', expecting '" + expected + "', but got '" + result + "'"); @@ -234,8 +226,6 @@ public class XSSAPIImplTest { @ParameterizedTest @MethodSource("dataForValidMultilineComment") public void testGetValidMultiLineComment(String source, String expected) { - setUp(); - String result = xssAPI.getValidMultiLineComment(source, RUBBISH); if (!result.equals(expected)) { fail("Validating multiline comment '" + source + "', expecting '" + expected + "', but got '" + result + "'"); @@ -245,8 +235,6 @@ public class XSSAPIImplTest { @ParameterizedTest @MethodSource("dataForValidMultilineJSON") public void testGetValidJSON(String source, String expected) { - setUp(); - String result = xssAPI.getValidJSON(source, RUBBISH_JSON); if (!result.equals(expected)) { fail("Validating JSON '" + source + "', expecting '" + expected + "', but got '" + result + "'"); @@ -256,8 +244,6 @@ public class XSSAPIImplTest { @ParameterizedTest @MethodSource("dataForValidXML") public void testGetValidXML(String source, String expected) { - setUp(); - String result = xssAPI.getValidXML(source, RUBBISH_XML); if (!result.equals(expected)) { fail("Validating XML '" + source + "', expecting '" + expected + "', but got '" + result + "'"); diff --git a/src/test/java/org/apache/sling/xss/impl/XSSFilterImplTest.java b/src/test/java/org/apache/sling/xss/impl/XSSFilterImplTest.java index 5cd7f44..8c9671e 100644 --- a/src/test/java/org/apache/sling/xss/impl/XSSFilterImplTest.java +++ b/src/test/java/org/apache/sling/xss/impl/XSSFilterImplTest.java @@ -20,28 +20,36 @@ package org.apache.sling.xss.impl; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import java.io.InputStream; +import java.net.URI; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Objects; +import org.apache.sling.api.resource.Resource; +import org.apache.sling.api.resource.external.URIProvider; import org.apache.sling.commons.metrics.Counter; import org.apache.sling.commons.metrics.MetricsService; import org.apache.sling.serviceusermapping.ServiceUserMapped; +import org.apache.sling.testing.mock.sling.ResourceResolverType; import org.apache.sling.testing.mock.sling.junit5.SlingContext; import org.apache.sling.testing.mock.sling.junit5.SlingContextExtension; import org.apache.sling.xss.XSSFilter; import org.apache.sling.xss.impl.status.XSSStatusService; +import org.jetbrains.annotations.NotNull; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; +import org.osgi.framework.Constants; @ExtendWith(SlingContextExtension.class) public class XSSFilterImplTest { @@ -76,9 +84,9 @@ public class XSSFilterImplTest { return testData; } - public SlingContext context = new SlingContext(); + public SlingContext context = new SlingContext(ResourceResolverType.JCR_OAK); - private XSSFilter xssFilter; + private XSSFilterImpl xssFilter; @AfterEach public void tearDown() { @@ -92,26 +100,39 @@ public class XSSFilterImplTest { context.registerService(MetricsService.class, metricsService); context.registerService(ServiceUserMapped.class, mock(ServiceUserMapped.class)); context.registerService(new XSSStatusService()); + xssFilter = context.registerInjectActivateService(new XSSFilterImpl()); } @Test public void testResourceBasedPolicy() { - context.load().binaryFile(this.getClass().getClassLoader().getResourceAsStream(XSSFilterImpl.EMBEDDED_POLICY_PATH), - "/libs/" + XSSFilterImpl.DEFAULT_POLICY_PATH); - context.registerInjectActivateService(new XSSFilterImpl()); - xssFilter = context.getService(XSSFilter.class); - XSSFilterImpl xssFilterImpl = (XSSFilterImpl) xssFilter; - XSSFilterImpl.AntiSamyPolicy antiSamyPolicy = xssFilterImpl.getActivePolicy(); + String policyPath = "/libs/" + XSSFilterImpl.DEFAULT_POLICY_PATH; + context.load().binaryFile(getPolicyFileAsStream(), policyPath); + // re-register in order to pick up the newly uploaded policy + xssFilter = context.registerInjectActivateService(new XSSFilterImpl()); + XSSFilterImpl.AntiSamyPolicy antiSamyPolicy = xssFilter.getActivePolicy(); assertFalse(antiSamyPolicy.isEmbedded(), "Expected a Resource based policy."); - assertEquals("/libs/" + XSSFilterImpl.DEFAULT_POLICY_PATH, antiSamyPolicy.getPath(), "This is not the policy we're looking for."); + assertEquals(policyPath, antiSamyPolicy.getPath(), "This is not the policy we're looking for."); + } + + @Test + // see SLING-12366 for why this test exists + public void testResourceBasedPolicyWithExternalizableBlob() { + // note: having a URI provided for the policy-resource causes a JcrExternalizableInputStream + // to be returned from resource.adaptTo(InputStream.class) + URIProvider uriProvider = new URIProvider() { + @Override + public @NotNull URI toURI(@NotNull Resource resource, @NotNull URIProvider.Scope scope, @NotNull URIProvider.Operation operation) { + return URI.create("https://example.com/blob" + resource.getPath()); + } + }; + context.registerService(URIProvider.class, uriProvider); + + testResourceBasedPolicy(); } @Test public void testDefaultEmbeddedPolicy() { - context.registerInjectActivateService(new XSSFilterImpl()); - xssFilter = context.getService(XSSFilter.class); - XSSFilterImpl xssFilterImpl = (XSSFilterImpl) xssFilter; - XSSFilterImpl.AntiSamyPolicy antiSamyPolicy = xssFilterImpl.getActivePolicy(); + XSSFilterImpl.AntiSamyPolicy antiSamyPolicy = xssFilter.getActivePolicy(); assertTrue(antiSamyPolicy.isEmbedded(), "Expected the default embedded policy."); assertEquals(XSSFilterImpl.EMBEDDED_POLICY_PATH, antiSamyPolicy.getPath(), "This is not the policy we're looking for."); } @@ -119,9 +140,6 @@ public class XSSFilterImplTest { @ParameterizedTest @MethodSource("dataForCheckMethod") public void testCheckMethod(String input, boolean isValid) { - context.registerInjectActivateService(new XSSFilterImpl()); - xssFilter = context.getService(XSSFilter.class); - System.out.println(input); if (isValid) { assertTrue(xssFilter.check(XSSFilter.DEFAULT_CONTEXT, input), "Expected valid input value for: " + input); } else { @@ -132,8 +150,6 @@ public class XSSFilterImplTest { @ParameterizedTest @MethodSource("dataForValidHref") public void isValidHref(String input, boolean isValid) { - context.registerInjectActivateService(new XSSFilterImpl()); - xssFilter = context.getService(XSSFilter.class); if (isValid) { assertTrue(xssFilter.isValidHref(input), "Expected valid href value for: " + input); } else { @@ -145,9 +161,12 @@ public class XSSFilterImplTest { public void testFallbackFiltering() { final String longURLContext = "<a href=\"https://sling.apache.org" + "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa [...] - context.registerInjectActivateService(new XSSFilterImpl()); - xssFilter = context.getService(XSSFilter.class); - assertNotNull(xssFilter); assertEquals(longURLContext, xssFilter.filter(longURLContext)); } + + private static @NotNull InputStream getPolicyFileAsStream() { + return Objects.requireNonNull( + XSSFilterImplTest.class.getClassLoader().getResourceAsStream(XSSFilterImpl.EMBEDDED_POLICY_PATH), + "The resource " + XSSFilterImpl.EMBEDDED_POLICY_PATH + " is required to exist for testing."); + } }