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.");
+ }
}
