You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by ra...@apache.org on 2018/06/26 16:04:43 UTC
[sling-org-apache-sling-xss] 01/01: SLING-7741 -
org.apache.sling.xss.impl.XSSAPIImpl#getValidHref doesn't correctly handle
the ":" character in URL fragments
This is an automated email from the ASF dual-hosted git repository.
radu pushed a commit to branch issue/SLING-7741
in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-xss.git
commit 634fdd2f75c7c331eab3d8143607f80dbc2db28b
Author: Radu Cotescu <ra...@apache.org>
AuthorDate: Tue Jun 26 18:04:30 2018 +0200
SLING-7741 - org.apache.sling.xss.impl.XSSAPIImpl#getValidHref doesn't correctly handle the ":" character in URL fragments
* modified mangleNamespaces function to only perform namespace mangling for paths
* extended onsiteURL regex to accept a colon character in fragments
* removed redundancy from offsiteURL regex
---
.../java/org/apache/sling/xss/impl/XSSAPIImpl.java | 70 ++++++++++++----------
.../org/apache/sling/xss/impl/XSSFilterImpl.java | 6 +-
src/main/resources/SLING-INF/content/config.xml | 4 +-
.../org/apache/sling/xss/impl/XSSAPIImplTest.java | 28 ++++++++-
4 files changed, 69 insertions(+), 39 deletions(-)
diff --git a/src/main/java/org/apache/sling/xss/impl/XSSAPIImpl.java b/src/main/java/org/apache/sling/xss/impl/XSSAPIImpl.java
index fe6c299..746de22 100644
--- a/src/main/java/org/apache/sling/xss/impl/XSSAPIImpl.java
+++ b/src/main/java/org/apache/sling/xss/impl/XSSAPIImpl.java
@@ -18,6 +18,8 @@ package org.apache.sling.xss.impl;
import java.io.StringReader;
import java.io.StringWriter;
+import java.net.URI;
+import java.net.URISyntaxException;
import java.util.HashMap;
import java.util.Map;
import java.util.regex.Matcher;
@@ -181,29 +183,11 @@ public class XSSAPIImpl implements XSSAPI {
private static final String MANGLE_NAMESPACE_IN_PREFIX = "/_";
- private static final String SCHEME_PATTERN = "://";
-
- private String mangleNamespaces(String absPath) {
- if (absPath != null) {
- // check for absolute urls
- final int schemeIndex = absPath.indexOf(SCHEME_PATTERN);
- final String manglePath;
- final String prefix;
- if (schemeIndex != -1) {
- final int pathIndex = absPath.indexOf("/", schemeIndex + 3);
- if (pathIndex != -1) {
- prefix = absPath.substring(0, pathIndex);
- manglePath = absPath.substring(pathIndex);
- } else {
- prefix = absPath;
- manglePath = "";
- }
- } else {
- prefix = "";
- manglePath = absPath;
- }
- if (manglePath.contains(MANGLE_NAMESPACE_OUT_SUFFIX)) {
- final Matcher m = MANGLE_NAMESPACE_PATTERN.matcher(manglePath);
+ private URI mangleNamespaces(URI uri) {
+ String mangledPath = null;
+ if (uri.getPath() != null) {
+ if (uri.getRawPath().contains(MANGLE_NAMESPACE_OUT_SUFFIX)) {
+ final Matcher m = MANGLE_NAMESPACE_PATTERN.matcher(uri.getRawPath());
final StringBuffer buf = new StringBuffer();
while (m.find()) {
@@ -212,13 +196,17 @@ public class XSSAPIImpl implements XSSAPI {
}
m.appendTail(buf);
-
- absPath = prefix + buf.toString();
-
+ mangledPath = buf.toString();
}
}
-
- return absPath;
+ if (mangledPath != null) {
+ try {
+ return new URI(uri.getScheme(), uri.getUserInfo(), uri.getHost(), uri.getPort(), mangledPath, uri.getRawQuery(), uri.getRawFragment());
+ } catch (URISyntaxException e) {
+ LOGGER.warn("Invalid URI.", e);
+ }
+ }
+ return uri;
}
/**
@@ -237,14 +225,32 @@ public class XSSAPIImpl implements XSSAPI {
.replaceAll("<", "%3C")
.replaceAll("`", "%60")
.replaceAll(" ", "%20");
- int qMarkIx = encodedUrl.indexOf('?');
- if (qMarkIx > 0) {
- encodedUrl = encodedUrl.substring(0, qMarkIx) + encodedUrl.substring(qMarkIx).replaceAll(":", "%3A");
+ URI mangledURI = null;
+ try {
+ mangledURI = mangleNamespaces(new URI(encodedUrl));
+ } catch (URISyntaxException e) {
+ LOGGER.warn("Invalid URI.", e);
+ }
+ if (mangledURI != null) {
+ StringBuilder uriBuilder = new StringBuilder();
+ if (StringUtils.isNotEmpty(mangledURI.getScheme()) && StringUtils.isNotEmpty(mangledURI.getAuthority())) {
+ uriBuilder.append(mangledURI.getScheme()).append("://").append(mangledURI.getRawAuthority());
+ }
+ if (StringUtils.isNotEmpty(mangledURI.getPath())) {
+ uriBuilder.append(mangledURI.getRawPath());
+ }
+ if (StringUtils.isNotEmpty(mangledURI.getQuery())) {
+ uriBuilder.append("?").append(mangledURI.getRawQuery().replaceAll(":", "%3A"));
+ }
+ if (StringUtils.isNotEmpty(mangledURI.getFragment())) {
+ uriBuilder.append("#").append(mangledURI.getRawFragment());
+ }
+ encodedUrl = uriBuilder.toString();
}
- encodedUrl = mangleNamespaces(encodedUrl);
if (xssFilter.isValidHref(encodedUrl)) {
return encodedUrl;
}
+
}
// fall through to empty string
return "";
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 b155d49..2df2ec4 100644
--- a/src/main/java/org/apache/sling/xss/impl/XSSFilterImpl.java
+++ b/src/main/java/org/apache/sling/xss/impl/XSSFilterImpl.java
@@ -72,8 +72,8 @@ public class XSSFilterImpl implements XSSFilter, ResourceChangeListener, Externa
static final Attribute DEFAULT_HREF_ATTRIBUTE = new Attribute(
"href",
Arrays.asList(
- Pattern.compile("([\\p{L}\\p{M}*+\\p{N}\\\\\\.\\#@\\$%\\+&;\\-_~,\\?=/!\\*\\(\\)]*|\\#(\\w)+)"),
- Pattern.compile("(\\s)*((ht|f)tp(s?)://|mailto:)[\\p{L}\\p{M}*+\\p{N}]+[\\p{L}\\p{M}*+\\p{N}\\p{Zs}\\.\\#@\\$%\\+&;:\\-_~,\\?=/!\\*\\(\\)]*(\\s)*")
+ Pattern.compile("([\\p{L}\\p{M}\\p{N}#@$%+&;\\-_~,?=/!*().\\\\]*(#(\\w|:)+)?)"),
+ Pattern.compile("(\\s)*((ht|f)tp(s?)://|mailto:)[\\p{L}\\p{M}\\p{N}]+[\\p{L}\\p{M}\\p{N}\\p{Zs}.#@$%+&;:\\-_~,?=/!*()]*(\\s)*")
),
Collections.<String>emptyList(),
"removeAttribute", ""
@@ -150,7 +150,7 @@ public class XSSFilterImpl implements XSSFilter, ResourceChangeListener, Externa
// Same logic as in org.owasp.validator.html.scan.MagicSAXFilter.startElement()
boolean isValid = hrefAttribute.containsAllowedValue(url.toLowerCase());
if (!isValid) {
- isValid = hrefAttribute.matchesAllowedExpression(url);
+ isValid = hrefAttribute.matchesAllowedExpression(url.toLowerCase());
}
return isValid;
}
diff --git a/src/main/resources/SLING-INF/content/config.xml b/src/main/resources/SLING-INF/content/config.xml
index f71b704..2dbfabd 100644
--- a/src/main/resources/SLING-INF/content/config.xml
+++ b/src/main/resources/SLING-INF/content/config.xml
@@ -67,8 +67,8 @@ http://www.w3.org/TR/html401/struct/global.html
<regexp name="htmlClass" value="[a-zA-Z0-9\s,\-_]+"/>
<!-- Allow empty URL attributes with a '*'-quantifier instead of '+' for the first part of the regexp -->
- <regexp name="onsiteURL" value="([\p{L}\p{M}*+\p{N}\\\.\#@\$%\+&;\-_~,\?=/!\*\(\)]*|\#(\w)+)"/>
- <regexp name="offsiteURL" value="(\s)*((ht|f)tp(s?)://|mailto:)[\p{L}\p{M}*+\p{N}]+[\p{L}\p{M}*+\p{N}\p{Zs}\.\#@\$%\+&;:\-_~,\?=/!\*\(\)]*(\s)*"/>
+ <regexp name="onsiteURL" value="([\p{L}\p{M}\p{N}#@$%+&;\-_~,?=/!*().\\]*(#(\w|:)+)?)"/>
+ <regexp name="offsiteURL" value="(\s)*((ht|f)tp(s?)://|mailto:)[\p{L}\p{M}\p{N}]+[\p{L}\p{M}\p{N}\p{Zs}.#@$%+&;:\-_~,?=/!*()]*(\s)*"/>
<regexp name="boolean" value="(true|false)"/>
<regexp name="singlePrintable" value="[a-zA-Z0-9]{1}"/>
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 889fc1c..ee7ae2f 100644
--- a/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java
+++ b/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java
@@ -19,10 +19,14 @@ package org.apache.sling.xss.impl;
import java.io.FileInputStream;
import java.io.InputStream;
import java.lang.reflect.Field;
+import java.util.regex.Pattern;
+import org.apache.commons.lang.StringEscapeUtils;
+import org.apache.commons.lang3.StringUtils;
import org.apache.sling.api.SlingHttpServletRequest;
import org.apache.sling.api.resource.ResourceResolver;
import org.apache.sling.xss.XSSAPI;
+import org.apache.sling.xss.XSSFilter;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
@@ -35,6 +39,7 @@ import org.powermock.reflect.Whitebox;
import junit.framework.TestCase;
+import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.mockito.Matchers.anyString;
import static org.mockito.Mockito.mock;
@@ -286,14 +291,33 @@ public class XSSAPIImplTest {
{ // ? in query string
"/test/search.html?0_tag:id=test?ing&1_tag:id=abc",
"/test/search.html?0_tag%3Aid=test?ing&1_tag%3Aid=abc",
+ },
+ {
+ "/test/search.html?0_tag:id=test?ing&1_tag:id=abc#fragment:test",
+ "/test/search.html?0_tag%3Aid=test?ing&1_tag%3Aid=abc#fragment:test",
+ },
+ {
+ "https://sling.apache.org/?a=1#fragment:test",
+ "https://sling.apache.org/?a=1#fragment:test"
+ },
+ {
+ "https://sling.apache.org/#fragment:test",
+ "https://sling.apache.org/#fragment:test"
}
};
+ StringBuilder errors = new StringBuilder();
for (String[] aTestData : testData) {
String href = aTestData[0];
String expected = aTestData[1];
-
- TestCase.assertEquals("Requested '" + href + "'", expected, xssAPI.getValidHref(href));
+ String result = xssAPI.getValidHref(href);
+ if (!expected.equals(result)) {
+ errors.append("Requested '").append(href).append("'\nGot '").append(result).append("'\nExpected '").append(expected).append("'\n\n");
+ }
+ }
+ if (errors.length() > 0) {
+ errors.insert(0, "\n");
+ TestCase.fail(errors.toString());
}
}