You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by fm...@apache.org on 2012/01/17 12:34:51 UTC
svn commit: r1232389 - in /sling/trunk/bundles/auth/core/src:
main/java/org/apache/sling/auth/core/AuthUtil.java
test/java/org/apache/sling/auth/core/AuthUtilTest.java
Author: fmeschbe
Date: Tue Jan 17 11:34:51 2012
New Revision: 1232389
URL: http://svn.apache.org/viewvc?rev=1232389&view=rev
Log:
SLING-2360 Improve redirect path validity test
- target must start with servlet context path
- target minus servlet context path must be absolute
- accept target resolving to an existing resource
- check target for illegal characters if no resource resolver is available
or if it does not resolve to an existing resource
- add more unit tests
Modified:
sling/trunk/bundles/auth/core/src/main/java/org/apache/sling/auth/core/AuthUtil.java
sling/trunk/bundles/auth/core/src/test/java/org/apache/sling/auth/core/AuthUtilTest.java
Modified: sling/trunk/bundles/auth/core/src/main/java/org/apache/sling/auth/core/AuthUtil.java
URL: http://svn.apache.org/viewvc/sling/trunk/bundles/auth/core/src/main/java/org/apache/sling/auth/core/AuthUtil.java?rev=1232389&r1=1232388&r2=1232389&view=diff
==============================================================================
--- sling/trunk/bundles/auth/core/src/main/java/org/apache/sling/auth/core/AuthUtil.java (original)
+++ sling/trunk/bundles/auth/core/src/main/java/org/apache/sling/auth/core/AuthUtil.java Tue Jan 17 11:34:51 2012
@@ -27,6 +27,7 @@ import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;
+import java.util.regex.Pattern;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
@@ -429,13 +430,18 @@ public final class AuthUtil {
* string</li>
* <li>The <code>target</code> is not an URL which is identified by the
* character sequence <code>://</code> separating the scheme from the host</li>
- * <li>The <code>target</code> is not normalized; that is it either contains
- * single or double dots in segments or consecutive slashes</li>
+ * <li>The <code>target</code> is normalized such that it contains no
+ * consecutive slashes and no path segment contains a single or double dot</li>
+ * <li>The <code>target</code> must be prefixed with the servlet context
+ * path</li>
* <li>If a <code>ResourceResolver</code> is available as a request
- * attribute the <code>target</code> must resolve to an existing resource</li>
+ * attribute the <code>target</code> (without the servlet context path
+ * prefix) must resolve to an existing resource</li>
* <li>If a <code>ResourceResolver</code> is <i>not</i> available as a
* request attribute the <code>target</code> must be an absolute path
- * starting with a slash character</li>
+ * starting with a slash character does not contain any of the characters
+ * <code><</code>, <code>></code>, <code>'</code>, or <code>"</code>
+ * in plain or URL encoding</li>
* </ul>
* <p>
* If any of the conditions does not hold, the method returns
@@ -468,26 +474,65 @@ public final class AuthUtil {
return false;
}
- final int query = target.indexOf('?');
- final String path = (query > 0) ? target.substring(0, query) : target;
+ final String ctxPath = getContextPath(request);
+ if (ctxPath.length() > 0 && !target.startsWith(ctxPath)) {
+ getLog().warn("isRedirectValid: Redirect target '{}' does not start with servlet context path '{}'",
+ target, ctxPath);
+ return false;
+ }
- if (request != null) {
- ResourceResolver resolver = (ResourceResolver) request.getAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_RESOLVER);
- if (resolver != null) {
- final boolean isValid = !ResourceUtil.isNonExistingResource(resolver.resolve(request, path));
- if (!isValid) {
- getLog().warn("isRedirectValid: Redirect target '{}' does not resolve to an existing resource",
- target);
- }
- return isValid;
+ final String localTarget = target.substring(ctxPath.length());
+ if (!localTarget.startsWith("/")) {
+ getLog().warn(
+ "isRedirectValid: Redirect target '{}' without servlet context path '{}' must be an absolute path",
+ target, ctxPath);
+ return false;
+ }
+
+ final int query = localTarget.indexOf('?');
+ final String path = (query > 0) ? localTarget.substring(0, query) : localTarget;
+
+ ResourceResolver resolver = getResourceResolver(request);
+ if (resolver != null) {
+ // assume all is fine if the path resolves to a resource
+ if (!ResourceUtil.isNonExistingResource(resolver.resolve(request, path))) {
+ return true;
}
+
+ // not resolving to a resource, check for illegal characters
}
- final boolean isValid = target.startsWith("/");
- if (!isValid) {
- getLog().warn("isRedirectValid: Redirect target '{}' must be an absolute path", target);
+ final Pattern illegal = Pattern.compile("[<>'\"]");
+ if (illegal.matcher(path).find()) {
+ getLog().warn("isRedirectValid: Redirect target '{}' must not contain any of <>'\"", target);
+ return false;
}
- return isValid;
+
+ return true;
+ }
+
+ /**
+ * Returns the context path from the request or an empty string if the
+ * request is <code>null</code>.
+ */
+ private static String getContextPath(final HttpServletRequest request) {
+ if (request != null) {
+ return request.getContextPath();
+ }
+ return "";
+ }
+
+ /**
+ * Returns the resource resolver set as the
+ * {@link AuthenticationSupport#REQUEST_ATTRIBUTE_RESOLVER} request
+ * attribute or <code>null</code> if the request object is <code>null</code>
+ * or the resource resolver is not present.
+ */
+ private static ResourceResolver getResourceResolver(final HttpServletRequest request) {
+ if (request != null) {
+ return (ResourceResolver) request.getAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_RESOLVER);
+ }
+ return null;
}
/**
Modified: sling/trunk/bundles/auth/core/src/test/java/org/apache/sling/auth/core/AuthUtilTest.java
URL: http://svn.apache.org/viewvc/sling/trunk/bundles/auth/core/src/test/java/org/apache/sling/auth/core/AuthUtilTest.java?rev=1232389&r1=1232388&r2=1232389&view=diff
==============================================================================
--- sling/trunk/bundles/auth/core/src/test/java/org/apache/sling/auth/core/AuthUtilTest.java (original)
+++ sling/trunk/bundles/auth/core/src/test/java/org/apache/sling/auth/core/AuthUtilTest.java Tue Jan 17 11:34:51 2012
@@ -69,9 +69,28 @@ public class AuthUtilTest {
}
@Test
- public void test_isRedirectValid_no_resource_resolver() {
+ public void test_isRedirectValid_invalid_characters() {
context.checking(new Expectations() {
{
+ allowing(request).getContextPath();
+ will(returnValue(""));
+ allowing(request).getAttribute(with(any(String.class)));
+ will(returnValue(null));
+ }
+ });
+
+ TestCase.assertFalse(AuthUtil.isRedirectValid(request, "/illegal/</x"));
+ TestCase.assertFalse(AuthUtil.isRedirectValid(request, "/illegal/>/x"));
+ TestCase.assertFalse(AuthUtil.isRedirectValid(request, "/illegal/'/x"));
+ TestCase.assertFalse(AuthUtil.isRedirectValid(request, "/illegal/\"/x"));
+ }
+
+ @Test
+ public void test_isRedirectValid_no_resource_resolver_root_context() {
+ context.checking(new Expectations() {
+ {
+ allowing(request).getContextPath();
+ will(returnValue(""));
allowing(request).getAttribute(with(any(String.class)));
will(returnValue(null));
}
@@ -82,7 +101,25 @@ public class AuthUtilTest {
}
@Test
- public void test_isRedirectValid_resource_resolver() {
+ public void test_isRedirectValid_no_resource_resolver_non_root_context() {
+ context.checking(new Expectations() {
+ {
+ allowing(request).getContextPath();
+ will(returnValue("/ctx"));
+ allowing(request).getAttribute(with(any(String.class)));
+ will(returnValue(null));
+ }
+ });
+
+ TestCase.assertFalse(AuthUtil.isRedirectValid(request, "relative/path"));
+ TestCase.assertFalse(AuthUtil.isRedirectValid(request, "/absolute/path"));
+
+ TestCase.assertFalse(AuthUtil.isRedirectValid(request, "ctx/relative/path"));
+ TestCase.assertTrue(AuthUtil.isRedirectValid(request, "/ctx/absolute/path"));
+ }
+
+ @Test
+ public void test_isRedirectValid_resource_resolver_root_context() {
context.checking(new Expectations() {
{
allowing(resolver).resolve(with(any(HttpServletRequest.class)), with(equal("/absolute/path")));
@@ -91,16 +128,59 @@ public class AuthUtilTest {
allowing(resolver).resolve(with(any(HttpServletRequest.class)), with(equal("relative/path")));
will(returnValue(new NonExistingResource(resolver, "relative/path")));
+ allowing(resolver).resolve(with(any(HttpServletRequest.class)), with(any(String.class)));
+ will(returnValue(new NonExistingResource(resolver, "/absolute/missing")));
+
allowing(request).getAttribute(with(AuthenticationSupport.REQUEST_ATTRIBUTE_RESOLVER));
will(returnValue(resolver));
allowing(request).getAttribute(with(any(String.class)));
will(returnValue(null));
+
+ allowing(request).getContextPath();
+ will(returnValue(""));
}
});
TestCase.assertFalse(AuthUtil.isRedirectValid(request, "relative/path"));
TestCase.assertTrue(AuthUtil.isRedirectValid(request, "/absolute/path"));
+
+ TestCase.assertTrue(AuthUtil.isRedirectValid(request, "/absolute/missing"));
+ TestCase.assertTrue(AuthUtil.isRedirectValid(request, "/absolute/missing/valid"));
+ TestCase.assertFalse(AuthUtil.isRedirectValid(request, "/absolute/missing/invalid/<"));
+ TestCase.assertFalse(AuthUtil.isRedirectValid(request, "/absolute/missing/invalid/>"));
+ TestCase.assertFalse(AuthUtil.isRedirectValid(request, "/absolute/missing/invalid/'"));
+ TestCase.assertFalse(AuthUtil.isRedirectValid(request, "/absolute/missing/invalid/\""));
+ }
+
+ @Test
+ public void test_isRedirectValid_resource_resolver_non_root_context() {
+ context.checking(new Expectations() {
+ {
+ allowing(resolver).resolve(with(any(HttpServletRequest.class)), with(equal("/absolute/path")));
+ will(returnValue(new SyntheticResource(resolver, "/absolute/path", "test")));
+
+ allowing(resolver).resolve(with(any(HttpServletRequest.class)), with(equal("relative/path")));
+ will(returnValue(new NonExistingResource(resolver, "relative/path")));
+
+ allowing(request).getAttribute(with(AuthenticationSupport.REQUEST_ATTRIBUTE_RESOLVER));
+ will(returnValue(resolver));
+
+ allowing(request).getAttribute(with(any(String.class)));
+ will(returnValue(null));
+
+ allowing(request).getContextPath();
+ will(returnValue("/ctx"));
+ }
+ });
+
+ TestCase.assertFalse(AuthUtil.isRedirectValid(request, "relative/path"));
+ TestCase.assertFalse(AuthUtil.isRedirectValid(request, "/absolute/path"));
+
+ TestCase.assertFalse(AuthUtil.isRedirectValid(request, "ctx/relative/path"));
+ TestCase.assertTrue(AuthUtil.isRedirectValid(request, "/ctx/absolute/path"));
+
+ TestCase.assertFalse(AuthUtil.isRedirectValid(request, "/ctxrelative/path"));
}
@Test