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-engine.git
The following commit(s) were added to refs/heads/master by this push:
new 3b39e37 SLING-13371 Fix path resolution divergence for ';' path
parameters (#101)
3b39e37 is described below
commit 3b39e375de13f0ced29bd3a358ab557d3e746294
Author: Jörg Hoh <[email protected]>
AuthorDate: Thu Oct 1 11:05:33 2026 +0200
SLING-13371 Fix path resolution divergence for ';' path parameters (#101)
* SLING-13371 Fix path resolution divergence for ';' path parameters
---
.../sling/engine/impl/request/RequestData.java | 68 +++++-
.../engine/impl/request/InitResourceTest.java | 232 +++++++++++++++++++--
.../impl/request/StripPathParametersTest.java | 79 +++++++
3 files changed, 350 insertions(+), 29 deletions(-)
diff --git
a/src/main/java/org/apache/sling/engine/impl/request/RequestData.java
b/src/main/java/org/apache/sling/engine/impl/request/RequestData.java
index ab64350..114e8f7 100644
--- a/src/main/java/org/apache/sling/engine/impl/request/RequestData.java
+++ b/src/main/java/org/apache/sling/engine/impl/request/RequestData.java
@@ -21,8 +21,8 @@ package org.apache.sling.engine.impl.request;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.UnsupportedEncodingException;
-import java.net.MalformedURLException;
-import java.net.URL;
+import java.net.URI;
+import java.net.URISyntaxException;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Set;
@@ -218,13 +218,35 @@ public class RequestData {
StringBuffer requestURL = servletRequest.getRequestURL();
String path = request.getPathInfo();
- if (requestURL.indexOf(";") > -1 && !path.contains(";")) {
+ if (requestURL.indexOf(";") > -1 && path != null &&
!path.contains(";")) {
+ // The container stripped path parameters (';...') from the path
info. Re-derive
+ // the path from the raw request URL to preserve them, but only
use the re-derived
+ // path if it is equivalent to the container provided path info
modulo the path
+ // parameters. Otherwise every upstream path based decision
(container
+ // normalization, authentication requirements, filter patterns,
access rules)
+ // would have been made on a different path than the one resolved
here.
try {
- final URL rUrl = new URL(requestURL.toString());
+ // java.net.URI#getPath decodes percent escapes, so the
derived path is in
+ // the same canonical (decoded) form as the container provided
path info
+ final String rawPath = new
URI(requestURL.toString()).getPath();
final String prefix =
request.getContextPath().concat(request.getServletPath());
- path = rUrl.getPath().substring(prefix.length());
- } catch (final MalformedURLException e) {
- // ignore
+ if (rawPath.startsWith(prefix)) {
+ final String candidate =
rawPath.substring(prefix.length());
+ if (path.equals(stripPathParameters(candidate))) {
+ path = candidate;
+ } else {
+ log.debug(
+ "initResource: ignoring path with parameters
{} not equivalent to container provided path {}",
+ candidate,
+ path);
+ }
+ }
+ } catch (final URISyntaxException e) {
+ // Unlike java.net.URL (used here previously), java.net.URI
validates the
+ // string and rejects characters that are illegal in a URI,
e.g. a raw,
+ // unencoded space. In that case we fall back to the container
provided path
+ // info, which is always safe, but, as a compatibility
consequence, drops the
+ // path parameters for such (strictly invalid) raw request
URLs.
}
}
@@ -240,6 +262,38 @@ public class RequestData {
return resource;
}
+ /**
+ * Removes URL path parameters ({@code ;name=value}, scoped to a path
segment as per
+ * RFC 3986) from the given path. Used to verify that a path re-derived
from the raw
+ * request URL only differs from the container provided path info by the
path
+ * parameters it preserves.
+ *
+ * @param path the path to strip path parameters from
+ * @return the path without path parameters
+ */
+ static String stripPathParameters(final String path) {
+ final int firstSemicolon = path.indexOf(';');
+ if (firstSemicolon < 0) {
+ return path;
+ }
+ final StringBuilder builder = new StringBuilder(path.length());
+ // everything up to the first ';' is guaranteed to be free of path
parameters, so it
+ // can be copied in one go instead of being re-scanned character by
character
+ builder.append(path, 0, firstSemicolon);
+ for (int i = firstSemicolon; i < path.length(); i++) {
+ final char c = path.charAt(i);
+ if (c == ';') {
+ // skip the path parameter(s) up to the end of the current
segment
+ while (i + 1 < path.length() && path.charAt(i + 1) != '/') {
+ i++;
+ }
+ } else {
+ builder.append(c);
+ }
+ }
+ return builder.toString();
+ }
+
public void initServlet(final Resource resource, final ServletResolver sr)
{
// the resource and the request path info, will never be null
RequestPathInfo requestPathInfo = new SlingRequestPathInfo(resource);
diff --git
a/src/test/java/org/apache/sling/engine/impl/request/InitResourceTest.java
b/src/test/java/org/apache/sling/engine/impl/request/InitResourceTest.java
index 795fa5b..86f0acb 100644
--- a/src/test/java/org/apache/sling/engine/impl/request/InitResourceTest.java
+++ b/src/test/java/org/apache/sling/engine/impl/request/InitResourceTest.java
@@ -36,6 +36,31 @@ import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;
+/**
+ * Verifies {@link RequestData#initResource(ResourceResolver)}, in particular
the fallback
+ * that re-derives the resolution path from the raw request URL when the
container strips
+ * URL path parameters ({@code ;name=value}) from {@link
HttpServletRequest#getPathInfo()}.
+ *
+ * <p>Each row below documents, in this order:
+ * <ul>
+ * <li><b>rawRequestURL</b> - the untouched URL as it appears on the wire
(what
+ * {@code getRequestURL()} returns) - i.e. the
attacker/client-controlled input;</li>
+ * <li><b>containerPathInfo</b> - what the servlet container hands back from
+ * {@code getPathInfo()} after its own decoding/normalization (path
parameters already
+ * stripped);</li>
+ * <li><b>expectedResolvedPath</b> - the path {@code initResource()} must
hand to the
+ * {@code ResourceResolver}, i.e. what the raw URL is effectively
"decoded to" once path
+ * parameters are safely re-attached (or not, if re-attaching them would
be unsafe).</li>
+ * </ul>
+ *
+ * <p>Note: {@code servletPath} is the empty string in every row below except
the
+ * null-path-info row - not because it was not worth testing, but because
+ * {@code SlingJakartaHttpServletRequestImpl.getServletPath()} always returns
{@code ""} (Sling
+ * registers with the HTTP Whiteboard at servlet path {@code "/*"}), so the
container's servlet
+ * path is unreachable at this call site regardless of what the underlying
+ * {@code HttpServletRequest} mock reports; only {@code contextPath}
contributes to the prefix
+ * that gets stripped in practice.
+ */
@RunWith(Parameterized.class)
public class InitResourceTest {
@@ -45,23 +70,78 @@ public class InitResourceTest {
private HttpServletResponse resp;
private ResourceResolver resourceResolver;
- private final String requestURL;
- private final String pathInfo;
- private final String expectedResolvePath;
- private final String servletPath;
+ private final String description;
+ private final String rawRequestURL;
+ private final String containerPathInfo;
+ private final String expectedResolvedPath;
private final String contextPath;
+ private final String servletPath;
- @Parameters(name = "URL={0} path={1}")
+ @Parameters(name = "{index}: {0}")
public static Collection<Object[]> data() {
return Arrays.asList(new Object[][] {
- {"http://localhost/one;v=1.1";, "/one;v=1.1", "/one;v=1.1", "", ""},
- {"http://localhost/two;v=1.1";, "/two", "/two;v=1.1", "", ""},
- {"http://localhost/three";, "/three", "/three", "", ""},
- {"http://localhost/four%3Bv=1.1";, "/four", "/four", "", ""},
- {"http://localhost/five%3Bv=1.1";, "/five;v=1.1", "/five;v=1.1",
"", ""},
- {"http://localhost/six;v=1.1";, "/six;v=1.1", "/six;v=1.1", "", ""},
- {"http://localhost/seven";, "/seven;v=1.1", "/seven;v=1.1", "", ""},
{
+ "container preserves a single path parameter as-is: no
re-derivation needed",
+ "http://localhost/one;v=1.1";,
+ "/one;v=1.1",
+ "/one;v=1.1",
+ "",
+ ""
+ },
+ {
+ "container strips a single path parameter: re-derived and
re-attached",
+ "http://localhost/two;v=1.1";,
+ "/two",
+ "/two;v=1.1",
+ "",
+ ""
+ },
+ {
+ "no path parameter present at all: path passed through
unchanged",
+ "http://localhost/three";,
+ "/three",
+ "/three",
+ "",
+ ""
+ },
+ {
+ "raw URL has a percent-encoded semicolon (not a real path
parameter): container "
+ + "already decoded and dropped it, no re-derivation is
attempted",
+ "http://localhost/four%3Bv=1.1";,
+ "/four",
+ "/four",
+ "",
+ ""
+ },
+ {
+ "raw URL has a percent-encoded semicolon that the container
decoded and kept: "
+ + "path passed through unchanged (already contains
';')",
+ "http://localhost/five%3Bv=1.1";,
+ "/five;v=1.1",
+ "/five;v=1.1",
+ "",
+ ""
+ },
+ {
+ "container already exposes the literal ';' in path info: path
passed through unchanged",
+ "http://localhost/six;v=1.1";,
+ "/six;v=1.1",
+ "/six;v=1.1",
+ "",
+ ""
+ },
+ {
+ "path parameter present only in container-provided path info
(no ';' in raw URL): "
+ + "path passed through unchanged",
+ "http://localhost/seven";,
+ "/seven;v=1.1",
+ "/seven;v=1.1",
+ "",
+ ""
+ },
+ {
+ "multiple path parameters across multiple segments, behind a
context path: all "
+ + "re-attached at their original segment",
"http://localhost/context/path;v=1.1/more/foo;x=y/end";,
"/path/more/foo/end",
"/path;v=1.1/more/foo;x=y/end",
@@ -69,20 +149,123 @@ public class InitResourceTest {
""
},
{
+ "multiple path parameters across multiple segments, no context
path: all "
+ + "re-attached at their original segment",
"http://localhost:4502/content;foo=bar/we-retail;bar=baz/us/en.html";,
"/content/we-retail/us/en.html",
"/content;foo=bar/we-retail;bar=baz/us/en.html",
"",
""
- }
+ },
+ {
+ "raw URL has a percent-encoded space: URI decoding brings the
re-derived path "
+ + "back to the same canonical (decoded) form as the
container path info, "
+ + "'%20' -> ' ', so it can be safely compared and
re-attached",
+ "http://localhost/a%20b;v=1.1";,
+ "/a b",
+ "/a b;v=1.1",
+ "",
+ ""
+ },
+ {
+ "re-derived path (params stripped) does not match container
path info at all: "
+ + "the raw URL is rejected outright, container path
info wins",
+ "http://localhost/other;x=1";,
+ "/two",
+ "/two",
+ "",
+ ""
+ },
+ {
+ "raw URL contains a literal '..;x' traversal segment the
container normalized "
+ + "away: rejected because it is not equivalent to the
container path info "
+ + "modulo path parameters, so it never resurfaces as a
literal segment",
+ "http://localhost/private/..;x/secret";,
+ "/secret",
+ "/secret",
+ "",
+ ""
+ },
+ {
+ "raw URL contains a percent-encoded '..;x' traversal segment
('%2e%2e' -> '..'): "
+ + "same rejection as the literal '..;x' case above,
decoding does not "
+ + "change the outcome",
+ "http://localhost/private/%2e%2e;x/secret";,
+ "/secret",
+ "/secret",
+ "",
+ ""
+ },
+ {
+ "raw URL path does not even start with the context path
prefix: fall back to "
+ + "container path info instead of an out-of-bounds
substring (previously a 500)",
+ "http://localhost/ctx;x/one";,
+ "/one",
+ "/one",
+ "/context",
+ ""
+ },
+ {
+ "raw URL shorter than the context+servlet path prefix: fall
back to container "
+ + "path info instead of an out-of-bounds substring
(previously a 500)",
+ "http://localhost/a;b";,
+ "/x",
+ "/x",
+ "/context",
+ ""
+ },
+ {
+ "raw URL is not a syntactically valid URI (unencoded space):
URISyntaxException "
+ + "is swallowed, falls back to container path info",
+ "http://localhost/a b;v=1.1",
+ "/a b",
+ "/a b",
+ "",
+ ""
+ },
+ {
+ "container's own getServletPath()/getPathInfo() both return
null (buggy/unusual "
+ + "container): the Sling request wrapper then also
exposes pathInfo=null; "
+ + "must not NPE, path stays null (this is the only row
using "
+ + "servletPath=null instead of \"\")",
+ "http://localhost/context;x=1";,
+ null,
+ null,
+ "/context",
+ null
+ },
+ {
+ "path parameter sits immediately after the context+servlet
prefix, leaving an "
+ + "empty base path once the prefix is stripped: still
re-attached correctly",
+ "http://localhost/context;x=1";,
+ "",
+ ";x=1",
+ "/context",
+ ""
+ },
+ {
+ "raw URL differs from container path info only in case ('/One'
vs '/one'), not "
+ + "just by path parameters: rejected as a near-miss,
container path info wins",
+ "http://localhost/one;x=1";,
+ "/One",
+ "/One",
+ "",
+ ""
+ },
});
}
public InitResourceTest(
- String requestURL, String pathInfo, String expectedResolvePath,
String contextPath, String servletPath) {
- this.requestURL = requestURL;
- this.pathInfo = pathInfo;
- this.expectedResolvePath = expectedResolvePath;
+ String description,
+ String rawRequestURL,
+ String containerPathInfo,
+ String expectedResolvedPath,
+ String contextPath,
+ String servletPath) {
+ this.description = description;
+ this.rawRequestURL = rawRequestURL;
+ this.containerPathInfo = containerPathInfo;
+ this.expectedResolvedPath = expectedResolvedPath;
this.contextPath = contextPath;
this.servletPath = servletPath;
}
@@ -103,12 +286,12 @@ public class InitResourceTest {
context.checking(new Expectations() {
{
allowing(req).getRequestURL();
- will(returnValue(new StringBuffer(requestURL)));
+ will(returnValue(new StringBuffer(rawRequestURL)));
allowing(req).getRequestURI();
allowing(req).getPathInfo();
- will(returnValue(pathInfo));
+ will(returnValue(containerPathInfo));
allowing(req).getContextPath();
will(returnValue(contextPath));
@@ -127,9 +310,14 @@ public class InitResourceTest {
allowing(req).getAttribute(RequestProgressTracker.class.getName());
will(returnValue(null));
- // Verify that the ResourceResolver is called with the
expected path
- allowing(resourceResolver)
- .resolve(with(any(HttpServletRequest.class)),
with(equal(expectedResolvePath)));
+ // Verify that the ResourceResolver is called with the
expected (re-derived or
+ // passed-through) path
+ if (expectedResolvedPath == null) {
+
allowing(resourceResolver).resolve(with(any(HttpServletRequest.class)),
with(aNull(String.class)));
+ } else {
+ allowing(resourceResolver)
+ .resolve(with(any(HttpServletRequest.class)),
with(equal(expectedResolvedPath)));
+ }
allowing(processor).getMaxCallCounter();
will(returnValue(2));
diff --git
a/src/test/java/org/apache/sling/engine/impl/request/StripPathParametersTest.java
b/src/test/java/org/apache/sling/engine/impl/request/StripPathParametersTest.java
new file mode 100644
index 0000000..747bff9
--- /dev/null
+++
b/src/test/java/org/apache/sling/engine/impl/request/StripPathParametersTest.java
@@ -0,0 +1,79 @@
+/*
+ * 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.engine.impl.request;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Direct unit tests for {@link RequestData#stripPathParameters(String)}, the
helper used by
+ * {@link RequestData#initResource} to check whether a path re-derived from
the raw request URL
+ * is equivalent - modulo URL path parameters ({@code ;name=value}) - to the
container-provided
+ * path info.
+ */
+public class StripPathParametersTest {
+
+ @Test
+ public void noPathParameterIsUnchanged() {
+ assertEquals("/one/two", RequestData.stripPathParameters("/one/two"));
+ }
+
+ @Test
+ public void singlePathParameterIsRemoved() {
+ assertEquals("/one", RequestData.stripPathParameters("/one;v=1.1"));
+ }
+
+ @Test
+ public void pathParameterInMiddleSegmentIsRemoved() {
+ assertEquals("/one/two",
RequestData.stripPathParameters("/one;v=1.1/two"));
+ }
+
+ @Test
+ public void multiplePathParametersInSameSegmentAreAllRemoved() {
+ assertEquals("/one/two",
RequestData.stripPathParameters("/one;a=1;b=2/two"));
+ }
+
+ @Test
+ public void trailingPathParameterWithNoFollowingSegmentIsRemoved() {
+ assertEquals("/one", RequestData.stripPathParameters("/one;v=1.1"));
+ // no characters at all after the trailing ';'
+ assertEquals("/one", RequestData.stripPathParameters("/one;"));
+ }
+
+ @Test
+ public void emptyPathParameterValueIsRemoved() {
+ assertEquals("/one/two",
RequestData.stripPathParameters("/one;=/two"));
+ }
+
+ @Test
+ public void leadingPathParameterOnRootIsRemoved() {
+ assertEquals("/one", RequestData.stripPathParameters(";x=1/one"));
+ }
+
+ @Test
+ public void pathParameterConsumingWholeInputYieldsEmptyString() {
+ assertEquals("", RequestData.stripPathParameters(";x=1"));
+ }
+
+ @Test
+ public void emptyStringIsUnchanged() {
+ assertEquals("", RequestData.stripPathParameters(""));
+ }
+}
