This is an automated email from the ASF dual-hosted git repository. joerghoh pushed a commit to branch SLING-13371 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-engine.git
commit 02aefc77ab4c1e70736d848a0ae6fa50da981c51 Author: Joerg Hoh <[email protected]> AuthorDate: Tue Sep 29 23:30:16 2026 +0200 SLING-13371 Fix path resolution divergence for ';' path parameters --- .../sling/engine/impl/request/RequestData.java | 60 +++++- .../engine/impl/request/InitResourceTest.java | 232 +++++++++++++++++++-- .../impl/request/StripPathParametersTest.java | 79 +++++++ 3 files changed, 342 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..8fae95b 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,31 @@ 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 != null && 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) { + // ignore and use the container provided path info } } @@ -240,6 +258,34 @@ 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) { + if (path.indexOf(';') < 0) { + return path; + } + final StringBuilder builder = new StringBuilder(path.length()); + for (int i = 0; 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("")); + } +}
