This is an automated email from the ASF dual-hosted git repository. rombert pushed a commit to annotated tag org.apache.sling.xss-1.0.0 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-xss.git
commit 10c577ca8a1a071252c534039ced6d6a0d7345e1 Author: Radu Cotescu <[email protected]> AuthorDate: Wed Mar 11 16:48:44 2015 +0000 SLING-4492 - Prevent configuring the ESAPI policies through random content files * removed API methods that were allowing to create AntiSammy policies on-demand from random content files * allow loading of custom policies from the implementation, only if the implementation class is used directly (e.g. through bundle embedding) * corrected minor performance issue in LongValidationRule * updated tests git-svn-id: https://svn.apache.org/repos/asf/sling/trunk/contrib/extensions/xss@1665939 13f79535-47bb-0310-9956-ffa450edef68 --- pom.xml | 10 + src/main/java/org/apache/sling/xss/XSSFilter.java | 24 --- .../apache/sling/xss/impl/LongValidationRule.java | 2 +- .../org/apache/sling/xss/impl/PolicyHandler.java | 57 ++---- .../org/apache/sling/xss/impl/XSSFilterImpl.java | 209 ++++++++++----------- .../org/apache/sling/xss/impl/XSSFilterRule.java | 5 - .../org/apache/sling/xss/impl/XSSAPIImplTest.java | 61 +++--- 7 files changed, 159 insertions(+), 209 deletions(-) diff --git a/pom.xml b/pom.xml index 4ecb483..1cb6ad6 100644 --- a/pom.xml +++ b/pom.xml @@ -45,6 +45,10 @@ <url>http://svn.apache.org/viewvc/sling/trunk/contrib/extensions/xss</url> </scm> + <properties> + <sling.java.version>6</sling.java.version> + </properties> + <!-- ======================================================================= --> <!-- B U I L D --> <!-- ======================================================================= --> @@ -281,6 +285,12 @@ <scope>test</scope> </dependency> <dependency> + <groupId>org.powermock</groupId> + <artifactId>powermock-api-mockito</artifactId> + <version>1.5.5</version> + <scope>test</scope> + </dependency> + <dependency> <groupId>org.slf4j</groupId> <artifactId>slf4j-simple</artifactId> </dependency> diff --git a/src/main/java/org/apache/sling/xss/XSSFilter.java b/src/main/java/org/apache/sling/xss/XSSFilter.java index 4f212c0..c907e8e 100644 --- a/src/main/java/org/apache/sling/xss/XSSFilter.java +++ b/src/main/java/org/apache/sling/xss/XSSFilter.java @@ -41,17 +41,6 @@ public interface XSSFilter { boolean check(ProtectionContext context, String src); /** - * Indicates whether or not a given source string contains XSS policy violations. - * - * @param context context to use for checking - * @param src source string - * @param policy the name/path of the policy to use - * @return true if the source is violation-free - * @throws NullPointerException if context is <code>null</code> - */ - boolean check(ProtectionContext context, String src, String policy); - - /** * Prevents the given source string from containing XSS stuff. * <p/> * The default protection context is used for checking. @@ -70,17 +59,4 @@ public interface XSSFilter { * @throws NullPointerException if context is <code>null</code> */ String filter(ProtectionContext context, String src); - - /** - * Protects the given source string from containing XSS stuff. - * <p/> - * If the context is unknown or <code>null</code> the default context is used. - * - * @param context context to use for checking - * @param src source string - * @param policy the name/path of the policy to use - * @return string that does not contain XSS stuff - * @throws NullPointerException if context is <code>null</code> - */ - String filter(ProtectionContext context, String src, String policy); } diff --git a/src/main/java/org/apache/sling/xss/impl/LongValidationRule.java b/src/main/java/org/apache/sling/xss/impl/LongValidationRule.java index 7d258d9..6a5dc6b 100644 --- a/src/main/java/org/apache/sling/xss/impl/LongValidationRule.java +++ b/src/main/java/org/apache/sling/xss/impl/LongValidationRule.java @@ -88,7 +88,7 @@ class LongValidationRule extends BaseValidationRule { // validate min and max try { - long i = Long.valueOf(canonical); + long i = Long.parseLong(canonical); if (i < minValue) { throw new ValidationException( "Invalid number input must be between " + minValue + " and " + maxValue + ": context=" + context, "Invalid number input must be between " + minValue + " and " + maxValue + ": context=" + context + ", input=" + input, context ); } diff --git a/src/main/java/org/apache/sling/xss/impl/PolicyHandler.java b/src/main/java/org/apache/sling/xss/impl/PolicyHandler.java index e8452be..b3cb4ff 100644 --- a/src/main/java/org/apache/sling/xss/impl/PolicyHandler.java +++ b/src/main/java/org/apache/sling/xss/impl/PolicyHandler.java @@ -19,63 +19,38 @@ package org.apache.sling.xss.impl; import java.io.IOException; import java.io.InputStream; -import org.apache.sling.api.resource.Resource; -import org.apache.sling.api.resource.ResourceResolver; -import org.apache.sling.api.resource.ResourceResolverFactory; import org.owasp.validator.html.AntiSamy; import org.owasp.validator.html.Policy; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; /** - * Class that provides the capability of securing input provided as plain text for - * HTML output. + * Class that provides the capability of securing input provided as plain text for HTML output. */ public class PolicyHandler { - /** - * Logger - */ - private static final Logger LOGGER = LoggerFactory.getLogger(PolicyHandler.class); - private Policy policy; - private AntiSamy antiSamy; /** * Try to load a policy from the given relative path. */ - public PolicyHandler(final ResourceResolverFactory factory, final String policyPath) throws Exception { - final ResourceResolver resolver = factory.getAdministrativeResourceResolver(null); + public PolicyHandler(InputStream policyStream) throws Exception { + // fix for classloader issue with IBM JVM: see bug #31946 + // (currently: http://bugs.day.com/bugzilla/show_bug.cgi?id=31946) + Thread currentThread = Thread.currentThread(); + ClassLoader cl = currentThread.getContextClassLoader(); try { - final Resource rsrc = resolver.getResource(policyPath); - if (rsrc == null) { - throw new IllegalArgumentException("Could not resolve '" + policyPath + " to a valid policy resource."); - } - LOGGER.debug("Loading policy from '{}'.", rsrc.getPath()); - - InputStream policyStream = null; - // fix for classloader issue with IBM JVM: see bug #31946 - // (currently: http://bugs.day.com/bugzilla/show_bug.cgi?id=31946) - Thread currentThread = Thread.currentThread(); - ClassLoader cl = currentThread.getContextClassLoader(); - try { - currentThread.setContextClassLoader(this.getClass().getClassLoader()); - policyStream = rsrc.adaptTo(InputStream.class); - this.policy = Policy.getInstance(policyStream); - this.antiSamy = new AntiSamy(this.policy); - } finally { - if (policyStream != null) { - try { - policyStream.close(); - } catch (final IOException ioe) { - // ignored as we can't do anything about this (besides logging) - } + currentThread.setContextClassLoader(this.getClass().getClassLoader()); + this.policy = Policy.getInstance(policyStream); + this.antiSamy = new AntiSamy(this.policy); + } finally { + if (policyStream != null) { + try { + policyStream.close(); + } catch (final IOException ioe) { + // ignored as we can't do anything about this (besides logging) } - currentThread.setContextClassLoader(cl); } - } finally { - resolver.close(); + currentThread.setContextClassLoader(cl); } } diff --git a/src/main/java/org/apache/sling/xss/impl/XSSFilterImpl.java b/src/main/java/org/apache/sling/xss/impl/XSSFilterImpl.java index a6dc82d..d7c6bb0 100644 --- a/src/main/java/org/apache/sling/xss/impl/XSSFilterImpl.java +++ b/src/main/java/org/apache/sling/xss/impl/XSSFilterImpl.java @@ -16,13 +16,10 @@ ******************************************************************************/ package org.apache.sling.xss.impl; -import java.util.HashSet; +import java.io.InputStream; import java.util.Map; -import java.util.Set; import java.util.concurrent.ConcurrentHashMap; -import org.apache.sling.xss.ProtectionContext; -import org.apache.sling.xss.XSSFilter; import org.apache.felix.scr.annotations.Activate; import org.apache.felix.scr.annotations.Component; import org.apache.felix.scr.annotations.Property; @@ -30,92 +27,103 @@ import org.apache.felix.scr.annotations.Reference; import org.apache.felix.scr.annotations.Service; import org.apache.sling.api.SlingConstants; import org.apache.sling.api.resource.LoginException; +import org.apache.sling.api.resource.Resource; import org.apache.sling.api.resource.ResourceResolver; import org.apache.sling.api.resource.ResourceResolverFactory; +import org.apache.sling.xss.ProtectionContext; +import org.apache.sling.xss.XSSFilter; import org.osgi.service.event.Event; import org.osgi.service.event.EventConstants; import org.osgi.service.event.EventHandler; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** - * This class implements the <code>XSSFilter</code> using the Antisamy - * XSS protection library found at + * This class implements the <code>XSSFilter</code> using the Antisamy XSS protection library found at * <a href="http://code.google.com/p/owaspantisamy/";>http://code.google.com/p/owaspantisamy/</a>. */ @Component(immediate = true) @Service(value = {EventHandler.class, XSSFilter.class}) -@Property(name = EventConstants.EVENT_TOPIC, value = {"org/apache/sling/api/resource/Resource/*", - "org/apache/sling/api/resource/ResourceProvider/*"}) +@Property(name = EventConstants.EVENT_TOPIC, value = {"org/apache/sling/api/resource/Resource/*"}) public class XSSFilterImpl implements XSSFilter, EventHandler { - @Reference - private ResourceResolverFactory resourceResolverFactory = null; + private static final Logger LOGGER = LoggerFactory.getLogger(XSSFilterImpl.class); - /** - * available contexts - */ - private final XSSFilterRule htmlHtmlContext = new HtmlToHtmlContentContext(); + private static final String DEFAULT_POLICY_PATH = "sling/xss/config.xml"; + private static final int DEFAULT_POLICY_CACHE_SIZE = 128; + private PolicyHandler defaultHandler; + // available contexts + private final XSSFilterRule htmlHtmlContext = new HtmlToHtmlContentContext(); private final XSSFilterRule plainHtmlContext = new PlainTextToHtmlContentContext(); - /** - * The paths to check for changes. - */ - private Set<String> checkPaths = new HashSet<String>(); - - /** - * A cache for the policies. - */ + // policies cache private Map<String, PolicyHandler> policies = new ConcurrentHashMap<String, PolicyHandler>(); - /** - * Maximum size for policy cache. - */ - private static final int DEFAULT_POLICY_CACHE_SIZE = 128; + @Reference + private ResourceResolverFactory resourceResolverFactory = null; - private void updateCheckPaths(final String policyPath) { - final Set<String> newCheckPaths = new HashSet<String>(checkPaths); - ResourceResolver resolver = null; - try { - resolver = this.resourceResolverFactory.getAdministrativeResourceResolver(null); - for (final String path : resolver.getSearchPath()) { - newCheckPaths.add(path + policyPath); - newCheckPaths.add(path + policyPath + "/jcr:content"); - } - this.checkPaths = newCheckPaths; - } catch (final LoginException le) { - throw new RuntimeException("Unable to get administrative login.", le); - } finally { - if (resolver != null) { - resolver.close(); - } + @Override + public void handleEvent(final Event event) { + final String path = (String) event.getProperty(SlingConstants.PROPERTY_PATH); + if (path.endsWith("/" + DEFAULT_POLICY_PATH)) { + updateDefaultHandler(); } } + @Override + public boolean check(final ProtectionContext context, final String src) { + return this.check(context, src, null); + } + + @Override + public String filter(final String src) { + return this.filter(XSSFilter.DEFAULT_CONTEXT, src); + } + + @Override + public String filter(final ProtectionContext context, final String src) { + return this.filter(context, src, null); + } + @Activate @SuppressWarnings("unused") protected void activate() { // load default handler - this.getPolicyHandler(null); + updateDefaultHandler(); } - - /** - * @see org.osgi.service.event.EventHandler#handleEvent(org.osgi.service.event.Event) - */ - public void handleEvent(final Event event) { - final String path = (String) event.getProperty(SlingConstants.PROPERTY_PATH); - boolean reload = false; - for (final String checkPath : this.checkPaths) { - if (path.equals(checkPath)) { - reload = true; - break; + private void updateDefaultHandler() { + ResourceResolver adminResolver = null; + try { + adminResolver = resourceResolverFactory.getAdministrativeResourceResolver(null); + Resource policyResource = adminResolver.getResource(DEFAULT_POLICY_PATH); + if (policyResource != null) { + InputStream policyStream = policyResource.adaptTo(InputStream.class); + if (policyStream != null) { + try { + if (defaultHandler == null) { + defaultHandler = new PolicyHandler(policyStream); + policyStream.close(); + } + } catch (Exception e) { + LOGGER.error("Unable to load policy from " + policyResource.getPath(), e); + } + } + } + if (defaultHandler == null) { + throw new IllegalStateException("Cannot load a default policy handler."); + } + } catch (LoginException e) { + LOGGER.error("Unable to load the default policy file.", e); + } finally { + if (adminResolver != null) { + adminResolver.close(); } - } - if (reload) { - this.policies.clear(); } } + /** * Get the filter rule context. */ @@ -129,67 +137,56 @@ public class XSSFilterImpl implements XSSFilter, EventHandler { return this.plainHtmlContext; } - /** - * Get the policy handler. - */ - private PolicyHandler getPolicyHandler(final String policyPath) { - final String policyName = (policyPath == null ? XSSFilterRule.DEFAULT_POLICY_PATH : policyPath); - PolicyHandler handler = this.policies.get(policyName); - if (handler == null) { - synchronized (this) { - try { - handler = new PolicyHandler(this.resourceResolverFactory, policyName); - if (this.policies.size() < DEFAULT_POLICY_CACHE_SIZE) { - this.policies.put(policyName, handler); - this.updateCheckPaths(policyName); - } - } catch (final Exception e) { - throw new RuntimeException("Unable to load policy " + policyName, e); - } - } - } - return handler; - } - - /** - * @see org.apache.sling.xss.XSSFilter#check(org.apache.sling.xss.ProtectionContext, String) + /* + The following methods are not part of the API. Client-code dependency to these methods is risky as they can be removed at any + point in time from the implementation. */ - public boolean check(final ProtectionContext context, final String src) { - return this.check(context, src, null); - } - /** - * @see org.apache.sling.xss.XSSFilter#check(org.apache.sling.xss.ProtectionContext, String, String) - */ public boolean check(final ProtectionContext context, final String src, final String policy) { final XSSFilterRule ctx = this.getFilterRule(context); - final PolicyHandler handler = ctx.supportsPolicy() ? this.getPolicyHandler(policy) : null; + PolicyHandler handler = null; + if (ctx.supportsPolicy()) { + if (policy == null || (handler = policies.get(policy)) == null) { + handler = defaultHandler; + } + } return ctx.check(handler, src); } - /** - * @see org.apache.sling.xss.XSSFilter#filter(java.lang.String) - */ - public String filter(final String src) { - return this.filter(XSSFilter.DEFAULT_CONTEXT, src); - } - - /** - * @see org.apache.sling.xss.XSSFilter#filter(ProtectionContext, java.lang.String) - */ - public String filter(final ProtectionContext context, final String src) { - return this.filter(context, src, null); - } - - /** - * @see org.apache.sling.xss.XSSFilter#filter(ProtectionContext, java.lang.String, java.lang.String) - */ public String filter(final ProtectionContext context, final String src, final String policy) { if (src == null) { return ""; } final XSSFilterRule ctx = this.getFilterRule(context); - final PolicyHandler handler = ctx.supportsPolicy() ? this.getPolicyHandler(policy) : null; + PolicyHandler handler = null; + if (ctx.supportsPolicy()) { + if (policy == null || (handler = policies.get(policy)) == null) { + handler = defaultHandler; + } + } return ctx.filter(handler, src); } + + public void setDefaultPolicy(InputStream policyStream) throws Exception { + defaultHandler = new PolicyHandler(policyStream); + } + + public void resetDefaultPolicy() { + updateDefaultHandler(); + } + + public void loadPolicy(String policyName, InputStream policyStream) throws Exception { + if (policies.size() < DEFAULT_POLICY_CACHE_SIZE) { + PolicyHandler policyHandler = new PolicyHandler(policyStream); + policies.put(policyName, policyHandler); + } + } + + public void unloadPolicy(String policyName) { + policies.remove(policyName); + } + + public boolean hasPolicy(String policyName) { + return policies.containsKey(policyName); + } } diff --git a/src/main/java/org/apache/sling/xss/impl/XSSFilterRule.java b/src/main/java/org/apache/sling/xss/impl/XSSFilterRule.java index e5a642b..8e61951 100644 --- a/src/main/java/org/apache/sling/xss/impl/XSSFilterRule.java +++ b/src/main/java/org/apache/sling/xss/impl/XSSFilterRule.java @@ -22,11 +22,6 @@ package org.apache.sling.xss.impl; public interface XSSFilterRule { /** - * Path to default policy - */ - String DEFAULT_POLICY_PATH = "sling/xss/config.xml"; - - /** * Check to see if a given string contains policy violations. * * @param policyHandler the policy handler to use for filtering 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 0bda966..f1347ec 100644 --- a/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java +++ b/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java @@ -16,28 +16,27 @@ ******************************************************************************/ package org.apache.sling.xss.impl; -import static org.junit.Assert.fail; -import static org.mockito.Matchers.anyString; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - import java.io.FileInputStream; import java.io.InputStream; import java.lang.reflect.Field; -import java.util.Map; - -import junit.framework.TestCase; -import org.apache.sling.xss.XSSAPI; import org.apache.sling.api.SlingHttpServletRequest; import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.xss.XSSAPI; import org.junit.Before; import org.junit.Test; -import org.mockito.Mockito; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; import org.owasp.validator.html.AntiSamy; import org.owasp.validator.html.Policy; +import org.powermock.reflect.Whitebox; + +import junit.framework.TestCase; + +import static org.junit.Assert.fail; +import static org.mockito.Matchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; public class XSSAPIImplTest { @@ -46,27 +45,25 @@ public class XSSAPIImplTest { private XSSAPI xssAPI; @Before - public void Setup() { + public void setup() { try { InputStream policyStream = new FileInputStream("./src/main/resources/SLING-INF/content/config.xml"); Policy policy = Policy.getInstance(policyStream); AntiSamy antiSamy = new AntiSamy(policy); - PolicyHandler mockPolicyHandler = mock(PolicyHandler.class, Mockito.RETURNS_DEEP_STUBS); + PolicyHandler mockPolicyHandler = mock(PolicyHandler.class); when(mockPolicyHandler.getPolicy()).thenReturn(policy); when(mockPolicyHandler.getAntiSamy()).thenReturn(antiSamy); XSSFilterImpl xssFilter = new XSSFilterImpl(); - Field policiesField = XSSFilterImpl.class.getDeclaredField("policies"); - policiesField.setAccessible(true); - ((Map<String, PolicyHandler>) policiesField.get(xssFilter)).put(XSSFilterRule.DEFAULT_POLICY_PATH, mockPolicyHandler); + Whitebox.setInternalState(xssFilter, "defaultHandler", mockPolicyHandler); xssAPI = new XSSAPIImpl(); Field filterField = XSSAPIImpl.class.getDeclaredField("xssFilter"); filterField.setAccessible(true); filterField.set(xssAPI, xssFilter); - ResourceResolver mockResolver = mock(ResourceResolver.class, Mockito.RETURNS_DEEP_STUBS); + ResourceResolver mockResolver = mock(ResourceResolver.class); when(mockResolver.map(anyString())).thenAnswer(new Answer() { public Object answer(InvocationOnMock invocation) { Object[] args = invocation.getArguments(); @@ -75,7 +72,7 @@ public class XSSAPIImplTest { } }); - SlingHttpServletRequest mockRequest = mock(SlingHttpServletRequest.class, Mockito.RETURNS_DEEP_STUBS); + SlingHttpServletRequest mockRequest = mock(SlingHttpServletRequest.class); when(mockRequest.getResourceResolver()).thenReturn(mockResolver); xssAPI = xssAPI.getRequestSpecificAPI(mockRequest); @@ -85,7 +82,7 @@ public class XSSAPIImplTest { } @Test - public void TestEncodeForHTML() { + public void testEncodeForHTML() { String[][] testData = { // Source Expected Result // @@ -108,7 +105,7 @@ public class XSSAPIImplTest { } @Test - public void TestEncodeForHTMLAttr() { + public void testEncodeForHTMLAttr() { String[][] testData = { // Source Expected Result // @@ -130,7 +127,7 @@ public class XSSAPIImplTest { } @Test - public void TestEncodeForXML() { + public void testEncodeForXML() { String[][] testData = { // Source Expected Result // @@ -152,7 +149,7 @@ public class XSSAPIImplTest { } @Test - public void TestEncodeForXMLAttr() { + public void testEncodeForXMLAttr() { String[][] testData = { // Source Expected Result // @@ -175,7 +172,7 @@ public class XSSAPIImplTest { } @Test - public void TestFilterHTML() { + public void testFilterHTML() { String[][] testData = { // Source Expected Result {null, ""}, @@ -210,7 +207,7 @@ public class XSSAPIImplTest { } @Test - public void TestGetValidHref() { + public void testGetValidHref() { String[][] testData = { // Href Expected Result // @@ -266,7 +263,7 @@ public class XSSAPIImplTest { } @Test - public void TestGetValidInteger() { + public void testGetValidInteger() { String[][] testData = { // Source Expected Result // @@ -289,7 +286,7 @@ public class XSSAPIImplTest { } @Test - public void TestGetValidLong() { + public void testGetValidLong() { String[][] testData = { // Source Expected Result // @@ -312,7 +309,7 @@ public class XSSAPIImplTest { } @Test - public void TestGetValidDimension() { + public void testGetValidDimension() { String[][] testData = { // Source Expected Result // @@ -342,7 +339,7 @@ public class XSSAPIImplTest { } @Test - public void TestEncodeForJSString() { + public void testEncodeForJSString() { String[][] testData = { // Source Expected Result // @@ -364,7 +361,7 @@ public class XSSAPIImplTest { } @Test - public void TestGetValidJSToken() { + public void testGetValidJSToken() { String[][] testData = { // Source Expected Result // @@ -396,7 +393,7 @@ public class XSSAPIImplTest { } @Test - public void TestEncodeForCSSString() { + public void testEncodeForCSSString() { String[][] testData = { // Source Expected result {null, null}, @@ -416,7 +413,7 @@ public class XSSAPIImplTest { } @Test - public void TestGetValidStyleToken() { + public void testGetValidStyleToken() { String[][] testData = { // Source Expected result {null , RUBBISH}, @@ -488,7 +485,7 @@ public class XSSAPIImplTest { } @Test - public void TestGetValidCSSColor() { + public void testGetValidCSSColor() { String[][] testData = { // Source Expected Result // @@ -524,7 +521,7 @@ public class XSSAPIImplTest { } @Test - public void TestGetValidMultiLineComment() { + public void testGetValidMultiLineComment() { String[][] testData = { //Source Expected Result -- To stop receiving notification emails like this one, please contact "[email protected]" <[email protected]>.
