diff --git a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpApiV2ProxyHttpServletRequest.java b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpApiV2ProxyHttpServletRequest.java index f318b7277..ddf321aaa 100644 --- a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpApiV2ProxyHttpServletRequest.java +++ b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpApiV2ProxyHttpServletRequest.java @@ -156,8 +156,9 @@ public String getMethod() { @Override public String getPathInfo() { - String pathInfo = cleanUri(request.getRawPath()); - return decodeRequestPath(pathInfo, LambdaContainerHandler.getContainerConfig()); + // Must be the same canonical form filter matching uses, otherwise a path can be spelled so that filter + // selection and servlet resolution disagree about which resource is being requested. + return canonicalizePath(request.getRawPath()); } @Override diff --git a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpServletRequest.java b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpServletRequest.java index 0029ee2d4..b292d1dd2 100644 --- a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpServletRequest.java +++ b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpServletRequest.java @@ -38,6 +38,7 @@ import jakarta.ws.rs.core.MediaType; import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.UnsupportedEncodingException; import java.net.URLDecoder; @@ -47,6 +48,9 @@ import java.util.*; import java.util.stream.Collectors; import java.util.stream.Stream; +import java.nio.charset.StandardCharsets; +import java.util.ArrayDeque; +import java.util.Deque; /** @@ -757,17 +761,137 @@ protected Locale parseLanguageTag(String languageTag) { return new Locale(language, country); } - static String decodeRequestPath(String requestPath, ContainerConfig config) { + + /** + * The request URI with the context path removed, exactly as it arrived: not decoded, not normalized. The context + * path is configured rather than client-supplied, so it is excluded from anything that inspects what the client + * actually sent. + * @param request The incoming request + * @return The raw request path relative to the context + */ + public static String contextRelativeRequestUri(HttpServletRequest request) { + String uri = request.getRequestURI(); + String contextPath = request.getContextPath(); + if (uri != null && contextPath != null && !contextPath.isEmpty() && uri.startsWith(contextPath)) { + return uri.substring(contextPath.length()); + } + return (uri == null || uri.isEmpty() ? "/" : uri); + } + + + /** + * Produces the canonical form of a request path: percent-decoded exactly once and then normalized. This is the + * form every routing and authorization decision must use, so that no spelling of a path can make two decisions + * disagree about which resource is being requested. + * + * Decoding is deliberately not delegated to URLDecoder, which implements form encoding and would + * turn a literal "+" in a path segment into a space. + * @param path A request path + * @return The decoded, normalized path, always starting with "/" and never ending with one + */ + public static String canonicalizePath(final String path) { + if (path == null || path.isEmpty()) { + return "/"; + } + return normalizePathSegments(decodePath(path)); + } + + + /** + * Percent-decodes a path exactly once, leaving dot segments in place. Code that rejects suspicious input needs + * this form rather than the canonical one: canonicalizePath resolves "." and ".." away, which hides + * a traversal attempt from any validator looking for it. + * + * Escape sequences are gathered and decoded as a group using the configured URI encoding, so a multi-byte + * character spanning several escapes decodes correctly. Literal characters are copied across untouched, which + * also means a surrogate pair is never split. + * @param path A request path + * @return The path with escape sequences decoded, dot segments untouched + */ + public static String decodePath(final String path) { + if (path == null || path.indexOf('%') < 0) { + return path; + } + + Charset charset = uriCharset(); + StringBuilder decoded = new StringBuilder(path.length()); + ByteArrayOutputStream pending = new ByteArrayOutputStream(); + for (int i = 0; i < path.length(); i++) { + char current = path.charAt(i); + if (current == '%' && i + 2 < path.length()) { + int high = Character.digit(path.charAt(i + 1), 16); + int low = Character.digit(path.charAt(i + 2), 16); + if (high >= 0 && low >= 0) { + pending.write((high << 4) + low); + i += 2; + continue; + } + } + flushDecodedBytes(pending, decoded, charset); + decoded.append(current); + } + flushDecodedBytes(pending, decoded, charset); + + return decoded.toString(); + } + + + private static void flushDecodedBytes(ByteArrayOutputStream pending, StringBuilder out, Charset charset) { + if (pending.size() > 0) { + out.append(new String(pending.toByteArray(), charset)); + pending.reset(); + } + } + + + /** + * The charset configured for decoding request URIs, falling back to UTF-8 if it is unset or not supported. + * Never throws, because this runs on every request including malformed ones. + */ + private static Charset uriCharset() { + ContainerConfig config = LambdaContainerHandler.getContainerConfig(); + String configured = (config == null ? null : config.getUriEncoding()); + if (configured == null || configured.isEmpty()) { + return StandardCharsets.UTF_8; + } try { - return URLDecoder.decode(requestPath, config.getUriEncoding()); - } catch (UnsupportedEncodingException ex) { - log.error("Could not URL decode the request path, configured encoding not supported: {}", SecurityUtils.encode(config.getUriEncoding())); - // we do not fail at this. - return requestPath; + return Charset.forName(configured); + } catch (Exception e) { + log.warn("Configured uriEncoding is not supported, falling back to UTF-8"); + return StandardCharsets.UTF_8; + } + } + + + /** + * Collapses empty segments and resolves "." and ".." segments. Traversal above the root is contained rather than + * rejected, so that a path can never normalize to something outside the application. + */ + private static String normalizePathSegments(final String path) { + Deque segments = new ArrayDeque<>(); + for (String segment : path.split("/", -1)) { + if (segment.isEmpty() || ".".equals(segment)) { + continue; + } + if ("..".equals(segment)) { + segments.pollLast(); + continue; + } + segments.addLast(segment); + } + + if (segments.isEmpty()) { + return "/"; } + StringBuilder normalized = new StringBuilder(); + for (String segment : segments) { + normalized.append("/").append(segment); + } + return normalized.toString(); } + static String cleanUri(String uri) { String finalUri = (uri == null ? "/" : uri); if (finalUri.equals("/")) { diff --git a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpServletRequestWrapper.java b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpServletRequestWrapper.java index 58d7282ba..b3902ab2e 100644 --- a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpServletRequestWrapper.java +++ b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpServletRequestWrapper.java @@ -27,6 +27,7 @@ import java.util.Map; import static com.amazonaws.serverless.proxy.internal.servlet.AwsProxyHttpServletRequest.cleanUri; +import static com.amazonaws.serverless.proxy.internal.servlet.AwsHttpServletRequest.canonicalizePath; public class AwsHttpServletRequestWrapper implements HttpServletRequest { private HttpServletRequest originalRequest; @@ -86,8 +87,9 @@ public String getMethod() { @Override public String getPathInfo() { - String pathInfo = cleanUri(newPath); - return AwsHttpServletRequest.decodeRequestPath(pathInfo, LambdaContainerHandler.getContainerConfig()); + // Same canonical form the wrapped request and filter matching use. This is reached on async dispatch, where + // AwsProxyRequestDispatcher resolves the servlet from getPathInfo, so it has to agree with filter selection. + return canonicalizePath(newPath); } @Override diff --git a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsProxyHttpServletRequest.java b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsProxyHttpServletRequest.java index c2a257d34..381aee090 100644 --- a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsProxyHttpServletRequest.java +++ b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsProxyHttpServletRequest.java @@ -175,8 +175,9 @@ public String getMethod() { @Override public String getPathInfo() { - String pathInfo = cleanUri(request.getPath()); - return decodeRequestPath(pathInfo, LambdaContainerHandler.getContainerConfig()); + // Must be the same canonical form filter matching uses, otherwise a path can be spelled so that filter + // selection and servlet resolution disagree about which resource is being requested. + return canonicalizePath(request.getPath()); } diff --git a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsServletContext.java b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsServletContext.java index 94dcaf440..a7ce5bd9a 100644 --- a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsServletContext.java +++ b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsServletContext.java @@ -223,18 +223,30 @@ public RequestDispatcher getNamedDispatcher(String s) { } public Servlet getServletForPath(String path) { - String[] pathParts = path.split("/"); + // getPathInfo() is null whenever the servlet path covered the whole request, so callers can legitimately + // pass null here. Treat it as the root rather than dereferencing it. + String targetPath = (path == null ? "/" : path); + String[] pathParts = targetPath.split("/"); for (AwsServletRegistration reg : servletRegistrations.values()) { for (String p : reg.getMappings()) { if ("".equals(p) || "/".equals(p) || "/*".equals(p)) { return reg.getServlet(); } // if I have no path and I haven't matched something now I'll just move on to the next - if ("".equals(path) || "/".equals(path)) { + if ("".equals(targetPath) || "/".equals(targetPath)) { continue; } String[] regParts = p.split("/"); for (int i = 0; i < regParts.length; i++) { + if (i >= pathParts.length) { + // The request has fewer segments than this mapping. A trailing wildcard still matches the + // empty remainder - the servlet spec has "/a/*" match "/a" - but anything else cannot, and + // walking further would read past the end of the request path. + if ("*".equals(regParts[i])) { + return reg.getServlet(); + } + break; + } if (!regParts[i].equals(pathParts[i]) && !"*".equals(regParts[i])) { break; } diff --git a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/FilterChainManager.java b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/FilterChainManager.java index 4917b4aaa..4c3af0f9d 100644 --- a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/FilterChainManager.java +++ b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/FilterChainManager.java @@ -89,7 +89,25 @@ public abstract class FilterChainManagerFilterChainHolder object that can be used to apply the filters to the request */ FilterChainHolder getFilterChain(final HttpServletRequest request, Servlet servlet) { - String targetPath = request.getRequestURI(); + // Filter selection and servlet resolution used to read the path from different sources, which is the bypass + // this fixes. They cannot just be pointed at one source, because consumers downstream disagree about which + // form they route on, so a filter applies if its url-pattern matches under either of two spellings. + // + // The canonical path is the one servlet resolution here uses, since getPathInfo() is decoded and normalized. + // The decoded-but-not-normalized path is needed because Spring MVC matches on the undecoded request URI and + // leaves dot segments to a servlet container that does not exist in Lambda. "/admin/../public/info" still + // reaches an "/admin/**" handler even though its canonical form is "/public/info", so matching on the + // canonical path alone leaves that request reaching an admin handler with its filter skipped. + // + // Matching under either spelling over-selects, so a filter may run for a request that is ultimately routed + // elsewhere. Under-selecting is the authorization bypass, so that is the direction chosen. + String rawPath = AwsHttpServletRequest.contextRelativeRequestUri(request); + String decodedPath = AwsHttpServletRequest.decodePath(rawPath); + String canonicalPath = request.getPathInfo(); + if (canonicalPath == null || canonicalPath.isEmpty()) { + canonicalPath = AwsHttpServletRequest.canonicalizePath(rawPath); + } + String targetPath = canonicalPath + "|" + decodedPath; DispatcherType type = request.getDispatcherType(); // only return the cached result if the filter list hasn't changed in the meanwhile @@ -119,8 +137,9 @@ FilterChainHolder getFilterChain(final HttpServletRequest request, Servlet servl continue; } for (String path : holder.getRegistration().getUrlPatternMappings()) { - if (pathMatches(targetPath, path)) { + if (pathMatches(canonicalPath, path) || pathMatches(decodedPath, path)) { chainHolder.addFilter(holder); + break; } } @@ -205,19 +224,24 @@ private void putFilterChainCache(final DispatcherType type, final String targetP * @return true if the given mapping path can apply to the target, false otherwise. */ boolean pathMatches(final String target, final String mapping) { + // Matching is case-insensitive throughout. The exact-equality check below always lowercased both sides while + // the segment comparison further down was case-sensitive, so the two halves of this method disagreed and a + // differently-cased path could skip its filter. Over-selecting a filter is fail-safe; under-selecting one is + // an authorization bypass, so both paths now compare case-insensitively. + String finalTarget = target.toLowerCase(Locale.ENGLISH); + String finalMapping = mapping.toLowerCase(Locale.ENGLISH); + // easiest case, they are exactly the same - if (target.toLowerCase(Locale.ENGLISH).equals(mapping.toLowerCase(Locale.ENGLISH))) { + if (finalTarget.equals(finalMapping)) { return true; } - String finalTarget = target; - String finalMapping = mapping; // strip first slash - if (target.startsWith("/")) { - finalTarget = target.replaceFirst("/", ""); + if (finalTarget.startsWith("/")) { + finalTarget = finalTarget.replaceFirst("/", ""); } - if (mapping.startsWith("/")) { - finalMapping = mapping.replaceFirst("/", ""); + if (finalMapping.startsWith("/")) { + finalMapping = finalMapping.replaceFirst("/", ""); } String[] targetParts = finalTarget.split(PATH_PART_SEPARATOR); diff --git a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/filters/UrlPathValidator.java b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/filters/UrlPathValidator.java index aadb26efd..da456de8f 100644 --- a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/filters/UrlPathValidator.java +++ b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/filters/UrlPathValidator.java @@ -17,6 +17,7 @@ import jakarta.servlet.*; import jakarta.servlet.annotation.WebFilter; +import com.amazonaws.serverless.proxy.internal.servlet.AwsHttpServletRequest; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import java.io.IOException; @@ -72,8 +73,12 @@ public void init(FilterConfig filterConfig) throws ServletException { @Override public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain) throws IOException, ServletException { - // the getPathInfo method of the AwsProxyHttpServletRequest returns the request path with the correct base path stripped - String path = ((HttpServletRequest)servletRequest).getPathInfo(); + // This filter has to see the path decoded but NOT normalized. getPathInfo resolves dot segments, which + // would hide a traversal attempt from the checks below, and the raw URI leaves escapes encoded, so + // "/%2e%2e/x" would not register as "..". The context path is excluded because it contributes slashes + // without contributing dot segments, which loosens the ratio check further down. + HttpServletRequest httpRequest = (HttpServletRequest) servletRequest; + String path = AwsHttpServletRequest.decodePath(stripContextPath(httpRequest)); if (path == null) { setErrorResponse(servletResponse); return; @@ -139,4 +144,18 @@ private int countStrings(String needle, String haystack) { } return stringCount; } + + /** + * The request URI with the context path removed, still encoded. The context path is configured rather than + * client-supplied, so including it would only dilute the checks applied to the part the client controls. + */ + private static String stripContextPath(HttpServletRequest request) { + String uri = request.getRequestURI(); + String contextPath = request.getContextPath(); + if (uri != null && contextPath != null && !contextPath.isEmpty() && uri.startsWith(contextPath)) { + return uri.substring(contextPath.length()); + } + return uri; + } + } diff --git a/aws-serverless-java-container-core/src/test/java/com/amazonaws/serverless/proxy/internal/servlet/AwsServletContextServletForPathTest.java b/aws-serverless-java-container-core/src/test/java/com/amazonaws/serverless/proxy/internal/servlet/AwsServletContextServletForPathTest.java new file mode 100644 index 000000000..7541f4a14 --- /dev/null +++ b/aws-serverless-java-container-core/src/test/java/com/amazonaws/serverless/proxy/internal/servlet/AwsServletContextServletForPathTest.java @@ -0,0 +1,107 @@ +package com.amazonaws.serverless.proxy.internal.servlet; + +import org.junit.jupiter.api.Test; + +import jakarta.servlet.ServletRegistration; +import jakarta.servlet.http.HttpServlet; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +/** + * Regression tests for AwsServletContext.getServletForPath. + * + * The matching loop was bounded by the length of the mapping while indexing into the request path, + * so any request with fewer segments than a registered mapping read past the end of the path array and threw + * ArrayIndexOutOfBoundsException. The method is reached on every request through + * SpringBootLambdaContainerHandler, SpringLambdaContainerHandler and + * AwsProxyRequestDispatcher, and its argument is getPathInfo(), which is null whenever the + * servlet path covered the whole request. + */ +public class AwsServletContextServletForPathTest { + + private static class NamedServlet extends HttpServlet { + private final String id; + + NamedServlet(String id) { + this.id = id; + } + + String getId() { + return id; + } + } + + private AwsServletContext contextWithMapping(String... mappings) { + AwsServletContext ctx = new AwsServletContext(null); + ServletRegistration.Dynamic reg = ctx.addServlet("deep", new NamedServlet("deep")); + reg.addMapping(mappings); + return ctx; + } + + /** The crash: request path shorter than the mapping. Previously ArrayIndexOutOfBoundsException. */ + @Test + void getServletForPath_pathShorterThanMapping_doesNotThrow() { + AwsServletContext ctx = contextWithMapping("/first/second/third"); + assertDoesNotThrow(() -> ctx.getServletForPath("/first")); + assertNull(ctx.getServletForPath("/first")); + } + + /** Same crash one segment further in, to show it is not an off-by-one at a single depth. */ + @Test + void getServletForPath_pathShorterThanMappingByOne_doesNotThrow() { + AwsServletContext ctx = contextWithMapping("/first/second/third"); + assertDoesNotThrow(() -> ctx.getServletForPath("/first/second")); + assertNull(ctx.getServletForPath("/first/second")); + } + + /** A null path is legitimate - getPathInfo() returns null when the servlet path covered everything. */ + @Test + void getServletForPath_nullPath_doesNotThrow() { + AwsServletContext ctx = contextWithMapping("/first/second"); + assertDoesNotThrow(() -> ctx.getServletForPath(null)); + assertNull(ctx.getServletForPath(null)); + } + + /** A null path must still reach a catch-all servlet, exactly as "/" does. */ + @Test + void getServletForPath_nullPathWithCatchAllMapping_returnsServlet() { + AwsServletContext ctx = new AwsServletContext(null); + NamedServlet root = new NamedServlet("root"); + ctx.addServlet("root", root).addMapping("/*"); + assertEquals(root, ctx.getServletForPath(null)); + assertEquals(root, ctx.getServletForPath("/")); + } + + /** Per the servlet spec "/first/*" matches "/first" itself, not only its children. */ + @Test + void getServletForPath_wildcardMatchesBareMappingPrefix() { + AwsServletContext ctx = contextWithMapping("/first/*"); + assertEquals("deep", ((NamedServlet) ctx.getServletForPath("/first")).getId()); + assertEquals("deep", ((NamedServlet) ctx.getServletForPath("/first/second")).getId()); + } + + /** Existing prefix behaviour must be unchanged: a mapping matches deeper paths under it. */ + @Test + void getServletForPath_longerPathStillMatches() { + AwsServletContext ctx = contextWithMapping("/first"); + assertEquals("deep", ((NamedServlet) ctx.getServletForPath("/first")).getId()); + assertEquals("deep", ((NamedServlet) ctx.getServletForPath("/first/second/third")).getId()); + } + + /** An unrelated path must still miss. */ + @Test + void getServletForPath_unrelatedPath_returnsNull() { + AwsServletContext ctx = contextWithMapping("/first/second"); + assertNull(ctx.getServletForPath("/other")); + assertNull(ctx.getServletForPath("/other/second")); + } + + /** Empty path keeps its existing behaviour of not matching a scoped mapping. */ + @Test + void getServletForPath_emptyPath_returnsNull() { + AwsServletContext ctx = contextWithMapping("/first/second"); + assertNull(ctx.getServletForPath("")); + } +} diff --git a/aws-serverless-java-container-core/src/test/java/com/amazonaws/serverless/proxy/internal/servlet/FilterChainManagerPathBypassTest.java b/aws-serverless-java-container-core/src/test/java/com/amazonaws/serverless/proxy/internal/servlet/FilterChainManagerPathBypassTest.java new file mode 100644 index 000000000..97d10483b --- /dev/null +++ b/aws-serverless-java-container-core/src/test/java/com/amazonaws/serverless/proxy/internal/servlet/FilterChainManagerPathBypassTest.java @@ -0,0 +1,316 @@ +package com.amazonaws.serverless.proxy.internal.servlet; + +import com.amazonaws.serverless.proxy.internal.testutils.AwsProxyRequestBuilder; +import com.amazonaws.serverless.proxy.internal.testutils.MockLambdaContext; +import com.amazonaws.serverless.proxy.internal.testutils.MockServlet; +import com.amazonaws.services.lambda.runtime.Context; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import jakarta.servlet.DispatcherType; +import jakarta.servlet.Filter; +import jakarta.servlet.FilterChain; +import jakarta.servlet.FilterConfig; +import jakarta.servlet.FilterRegistration; +import jakarta.servlet.ServletContext; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; + +import java.io.IOException; +import java.util.EnumSet; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Regression tests for the filter-selection authorization bypass: filter url-pattern matching used the raw, + * undecoded path from getRequestURI() while servlet resolution used the decoded path from + * getPathInfo(). A request for a percent-encoded equivalent of a protected path therefore failed to + * select the filter mapped to that path but still routed to the servlet mapped to it. + * + * Filter selection must be performed on the same decoded, normalized path that servlet resolution uses, so that no + * encoding of a path can cause the two to disagree. + */ +public class FilterChainManagerPathBypassTest { + private static final Context lambdaContext = new MockLambdaContext(); + + private ServletContext servletContext; + private AwsFilterChainManager chainManager; + + @BeforeEach + public void setUp() { + servletContext = new AwsServletContext(null); + FilterRegistration.Dynamic adminFilter = servletContext.addFilter("AdminFilter", new MockFilter()); + adminFilter.addMappingForUrlPatterns(EnumSet.of(DispatcherType.REQUEST), true, "/admin/*"); + servletContext.addServlet("adminServlet", new MockServlet()).addMapping("/admin/*"); + chainManager = new AwsFilterChainManager((AwsServletContext) servletContext); + } + + private int filterCountFor(String path) { + AwsProxyHttpServletRequest req = new AwsProxyHttpServletRequest( + new AwsProxyRequestBuilder(path, "GET").build(), lambdaContext, null + ); + req.setServletContext(servletContext); + return chainManager.getFilterChain(req, null).filterCount(); + } + + private jakarta.servlet.Servlet servletForPath(String path) { + AwsProxyHttpServletRequest req = new AwsProxyHttpServletRequest( + new AwsProxyRequestBuilder(path, "GET").build(), lambdaContext, null + ); + req.setServletContext(servletContext); + return ((AwsServletContext) servletContext).getServletForPath(req.getPathInfo()); + } + + /** Control: the plain protected path selects the filter. If this fails the test setup is wrong. */ + @Test + void filterChain_plainProtectedPath_selectsFilter() { + assertEquals(1, filterCountFor("/admin/secret")); + } + + /** Control: an unrelated path does not select the filter, i.e. the mapping is actually scoped. */ + @Test + void filterChain_unrelatedPath_doesNotSelectFilter() { + assertEquals(0, filterCountFor("/public/info")); + } + + /** + * The reported bypass. "/%61dmin/secret" decodes to "/admin/secret", which is what servlet resolution sees, + * so filter selection must see it too. + */ + @Test + void filterChain_percentEncodedFirstCharacter_stillSelectsFilter() { + assertEquals(1, filterCountFor("/%61dmin/secret")); + } + + /** Same bypass, encoding a character in the middle of the protected segment. */ + @Test + void filterChain_percentEncodedMiddleCharacter_stillSelectsFilter() { + assertEquals(1, filterCountFor("/adm%69n/secret")); + } + + /** Every character of the protected segment encoded. */ + @Test + void filterChain_fullyEncodedSegment_stillSelectsFilter() { + assertEquals(1, filterCountFor("/%61%64%6d%69%6e/secret")); + } + + /** Uppercase percent-encoding hex digits must decode identically to lowercase. */ + @Test + void filterChain_uppercaseHexEncoding_stillSelectsFilter() { + assertEquals(1, filterCountFor("/%41dmin/secret")); + } + + /** + * An encoded path separator collapses to "/admin/secret" once decoded. Servlet resolution decodes it, so + * filter selection must not treat it as a single opaque segment. + */ + @Test + void filterChain_encodedPathSeparator_stillSelectsFilter() { + assertEquals(1, filterCountFor("/admin%2Fsecret")); + } + + /** A dot segment that normalizes back into the protected path must still select the filter. */ + @Test + void filterChain_dotSegmentTraversal_stillSelectsFilter() { + assertEquals(1, filterCountFor("/public/../admin/secret")); + } + + /** + * Double encoding must decode exactly once, matching what servlet resolution does. "/%2561dmin" decodes to + * "/%61dmin", which is not "/admin", so the filter legitimately does not apply. + */ + @Test + void filterChain_doubleEncoded_decodesExactlyOnce() { + assertEquals(0, filterCountFor("/%2561dmin/secret")); + } + + /** A malformed percent sequence must not throw; the request still has to be routed. */ + @Test + void filterChain_malformedEncoding_doesNotThrow() { + filterCountFor("/%zz/secret"); + filterCountFor("/admin%"); + filterCountFor("/admin%2"); + } + + /** + * The matcher lowercases both sides on its exact-equality fast path but compares segments case-sensitively, + * so the two halves of the same method disagreed. Matching is case-insensitive throughout, which is the + * fail-safe direction: a filter may run when it need not, never the reverse. + */ + @Test + void filterChain_mixedCasePath_stillSelectsFilter() { + assertEquals(1, filterCountFor("/ADMIN/secret")); + assertEquals(1, filterCountFor("/Admin/secret")); + } + + /** The cache must not let an encoded request poison or satisfy the entry for a different decoded path. */ + @Test + void filterChain_cacheKeyedOnCanonicalPath_encodedAndPlainAgree() { + assertEquals(1, filterCountFor("/%61dmin/secret")); + assertEquals(1, filterCountFor("/admin/secret")); + assertEquals(0, filterCountFor("/public/info")); + assertEquals(1, filterCountFor("/admin/secret")); + } + + /** + * A dot segment that steps OUT of the protected prefix must still select that prefix's filter. The canonical + * form is "/public", but Spring MVC matches on the undecoded request URI and leaves dot segments to a servlet + * container that does not exist here, so "/admin/**" still reaches an admin handler. Selecting the filter for + * the decoded-but-not-normalized spelling as well is what stops that from being a bypass: matching on the + * canonical path alone leaves this request reaching the admin handler unfiltered. + */ + @Test + void filterChain_dotSegmentLeadingOutOfProtectedPath_stillSelectsFilter() { + assertEquals(1, filterCountFor("/admin/x/../../public")); + } + + /** Same, with the dot segments percent-encoded. */ + @Test + void filterChain_encodedDotSegmentLeadingOut_stillSelectsFilter() { + assertEquals(1, filterCountFor("/admin/x/%2e%2e/%2e%2e/public")); + assertEquals(1, filterCountFor("/admin/secret%2F..%2F..%2Fpublic")); + } + + /** + * The mirror case: a dot segment that lands ON a protected path must select that path's filter. This is the + * direction a decode-only canonicalization would miss. + */ + @Test + void filterChain_dotSegmentLeadingIntoProtectedPath_selectsFilter() { + assertEquals(1, filterCountFor("/public/x/../../admin/secret")); + assertNotNull(servletForPath("/public/x/../../admin/secret")); + } + + /** + * The invariant that matters: filter selection must never UNDER-select. If any spelling of a path could reach a + * servlet mapped under a protected prefix, the filter for that prefix has to run. Over-selection is permitted + * and expected, because consumers downstream disagree about which form of the path they route on, so predicting + * a single winner is what produced this bug class in the first place. + */ + @Test + void filterSelectionNeverUnderSelects() { + for (String path : new String[]{ + "/admin/secret", + "/%61dmin/secret", + "/adm%69n/secret", + "/ADMIN/secret", + "/admin%2Fsecret", + "/admin/x/../../public", + "/admin/x/%2e%2e/%2e%2e/public", + "/admin/secret%2F..%2F..%2Fpublic", + "/%61dmin/../public", + "/public/x/../../admin/secret", + "/admin//secret", + "/./admin/secret", + "/../../admin/secret" + }) { + assertTrue(filterCountFor(path) > 0, + "no filter selected for a spelling that can reach the protected prefix: " + path); + } + } + + /** A path with no relationship to the protected prefix under any spelling must not select its filter. */ + @Test + void filterSelection_unrelatedPathsAreNotOverSelected() { + assertEquals(0, filterCountFor("/public/info")); + assertEquals(0, filterCountFor("/public/x/y/z")); + assertEquals(0, filterCountFor("/")); + } + + + /** + * AwsHttpServletRequestWrapper is used on async dispatch, and AwsProxyRequestDispatcher resolves the servlet + * from its getPathInfo. It therefore has to report the same canonical path as the request it wraps, otherwise + * the async path reintroduces the filter/servlet disagreement. + */ + @Test + void requestWrapper_reportsTheSameCanonicalPathAsTheWrappedRequest() { + for (String path : new String[]{ + "/admin/secret", "/%61dmin/secret", "/admin/x/../../public", "/admin//secret", "/./admin/secret"}) { + AwsProxyHttpServletRequest original = new AwsProxyHttpServletRequest( + new AwsProxyRequestBuilder("/unrelated", "GET").build(), lambdaContext, null); + original.setServletContext(servletContext); + AwsHttpServletRequestWrapper wrapped = new AwsHttpServletRequestWrapper(original, path); + assertEquals(AwsHttpServletRequest.canonicalizePath(path), wrapped.getPathInfo(), + "wrapper disagrees with canonicalizePath for " + path); + } + } + + + /** The canonicalization helper itself, independent of request plumbing. */ + @Test + void canonicalize_decodesAndNormalizes() { + assertEquals("/admin/secret", AwsHttpServletRequest.canonicalizePath("/%61dmin/secret")); + assertEquals("/admin/secret", AwsHttpServletRequest.canonicalizePath("/admin%2Fsecret")); + assertEquals("/admin/secret", AwsHttpServletRequest.canonicalizePath("/public/../admin/secret")); + assertEquals("/admin/secret", AwsHttpServletRequest.canonicalizePath("/admin//secret")); + assertEquals("/admin/secret", AwsHttpServletRequest.canonicalizePath("/./admin/secret")); + assertEquals("/%61dmin/secret", AwsHttpServletRequest.canonicalizePath("/%2561dmin/secret")); + } + + /** + * URLDecoder.decode applies form encoding, which turns "+" into a space. Path segments must not be decoded + * that way, so a literal "+" has to survive canonicalization. + */ + @Test + void canonicalize_plusIsNotTreatedAsSpace() { + assertEquals("/admin+user/secret", AwsHttpServletRequest.canonicalizePath("/admin+user/secret")); + assertEquals("/admin user/secret", AwsHttpServletRequest.canonicalizePath("/admin%20user/secret")); + } + + /** + * Characters outside the BMP are stored as surrogate pairs. Encoding each half separately replaces the + * character with "??", and because getPathInfo() returns the canonical path the corruption would reach the + * application. Only triggered when the path also contains a '%', since otherwise decoding is skipped. + */ + @Test + void canonicalize_nonBmpCharactersSurviveDecoding() { + assertEquals("/\uD83D\uDE00/admin", AwsHttpServletRequest.canonicalizePath("/\uD83D\uDE00/%61dmin")); + assertEquals("/\uD83D\uDE00/admin", AwsHttpServletRequest.canonicalizePath("/\uD83D\uDE00/admin")); + // CJK Extension B, also outside the BMP + assertEquals("/\uD840\uDC0B/admin", AwsHttpServletRequest.canonicalizePath("/\uD840\uDC0B/%61dmin")); + } + + /** BMP characters were never affected, but pin them so the surrogate handling cannot regress them. */ + @Test + void canonicalize_bmpMultiByteCharactersSurviveDecoding() { + assertEquals("/caf\u00e9/admin", AwsHttpServletRequest.canonicalizePath("/caf\u00e9/%61dmin")); + assertEquals("/\u4f60\u597d/admin", AwsHttpServletRequest.canonicalizePath("/\u4f60\u597d/%61dmin")); + } + + /** An unpaired surrogate is malformed input and must not throw. */ + @Test + void canonicalize_loneSurrogateDoesNotThrow() { + AwsHttpServletRequest.canonicalizePath("/\uD83D/%61dmin"); + AwsHttpServletRequest.canonicalizePath("/%61dmin/\uD83D"); + AwsHttpServletRequest.canonicalizePath("/\uDE00/%61dmin"); + } + + + /** Canonicalization must never escape the root via excess dot segments. */ + @Test + void canonicalize_traversalAboveRootIsContained() { + assertTrue(AwsHttpServletRequest.canonicalizePath("/../../admin/secret").startsWith("/admin")); + assertEquals("/", AwsHttpServletRequest.canonicalizePath("/../..")); + } + + private static class MockFilter implements Filter { + @Override + public void init(FilterConfig filterConfig) throws ServletException { + } + + @Override + public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain) + throws IOException, ServletException { + chain.doFilter(req, res); + } + + @Override + public void destroy() { + } + } +} diff --git a/aws-serverless-java-container-core/src/test/java/com/amazonaws/serverless/proxy/internal/servlet/filters/UrlPathValidatorTraversalTest.java b/aws-serverless-java-container-core/src/test/java/com/amazonaws/serverless/proxy/internal/servlet/filters/UrlPathValidatorTraversalTest.java new file mode 100644 index 000000000..bfc073ac6 --- /dev/null +++ b/aws-serverless-java-container-core/src/test/java/com/amazonaws/serverless/proxy/internal/servlet/filters/UrlPathValidatorTraversalTest.java @@ -0,0 +1,91 @@ +package com.amazonaws.serverless.proxy.internal.servlet.filters; + +import com.amazonaws.serverless.proxy.internal.LambdaContainerHandler; +import com.amazonaws.serverless.proxy.internal.servlet.AwsHttpServletRequest; +import com.amazonaws.serverless.proxy.internal.servlet.AwsHttpServletResponse; +import com.amazonaws.serverless.proxy.internal.servlet.AwsProxyHttpServletRequest; +import com.amazonaws.serverless.proxy.internal.testutils.AwsProxyRequestBuilder; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; + +/** + * UrlPathValidator rejects traversal attempts up front. It needs the path decoded but not normalized: the raw URI + * leaves "%2e%2e" unrecognizable as "..", and the canonical path has already resolved the dot segments away. + */ +public class UrlPathValidatorTraversalTest { + + @AfterEach + public void reset() { + LambdaContainerHandler.getContainerConfig().setServiceBasePath(null); + LambdaContainerHandler.getContainerConfig().setUseStageAsServletContext(false); + } + + private int statusFor(String path) { + AwsProxyHttpServletRequest req = + new AwsProxyHttpServletRequest(new AwsProxyRequestBuilder(path, "GET").build(), null, null); + AwsHttpServletResponse resp = new AwsHttpServletResponse(req, null); + UrlPathValidator validator = new UrlPathValidator(); + final boolean[] chainCalled = {false}; + try { + validator.init(null); + validator.doFilter(req, resp, (rq, rs) -> chainCalled[0] = true); + } catch (Exception e) { + throw new RuntimeException(e); + } + return resp.getStatus(); + } + + /** Plain traversal was always rejected. */ + @Test + void plainTraversal_isRejected() { + assertEquals(UrlPathValidator.DEFAULT_ERROR_CODE, statusFor("../..")); + } + + /** Percent-encoded traversal must be rejected too. Reading the raw URI misses this entirely. */ + @Test + void percentEncodedTraversal_isRejected() { + assertEquals(UrlPathValidator.DEFAULT_ERROR_CODE, statusFor("/%2e%2e/%2e%2e/x")); + } + + /** Uppercase escapes decode the same way. */ + @Test + void uppercaseEncodedTraversal_isRejected() { + assertEquals(UrlPathValidator.DEFAULT_ERROR_CODE, statusFor("/%2E%2E/%2E%2E/x")); + } + + /** Mixed plain and encoded dots form a traversal too. */ + @Test + void mixedEncodingTraversal_isRejected() { + assertEquals(UrlPathValidator.DEFAULT_ERROR_CODE, statusFor("/.%2e/.%2e/x")); + } + + /** + * A configured base path contributes slashes but never dot segments, which loosened the ratio check. The + * decision must not depend on how the context path is configured. + */ + @Test + void configuredBasePath_doesNotWeakenTheCheck() { + int withoutContext = statusFor("/../x"); + LambdaContainerHandler.getContainerConfig().setServiceBasePath("/prod"); + int withContext = statusFor("/../x"); + assertEquals(withoutContext, withContext, + "the verdict changed when a context path was configured"); + } + + /** An ordinary path must still be allowed through. */ + @Test + void ordinaryPath_isAllowed() { + assertNotEquals(UrlPathValidator.DEFAULT_ERROR_CODE, statusFor("/admin/secret")); + assertNotEquals(UrlPathValidator.DEFAULT_ERROR_CODE, statusFor("/admin/a.b/secret")); + } + + /** The decode-only helper leaves dot segments in place, unlike canonicalizePath. */ + @Test + void decodePath_leavesDotSegmentsInPlace() { + assertEquals("/../../x", AwsHttpServletRequest.decodePath("/%2e%2e/%2e%2e/x")); + assertEquals("/admin/secret", AwsHttpServletRequest.canonicalizePath("/%2e%2e/admin/secret")); + } +} diff --git a/aws-serverless-java-container-springboot4/src/test/java/com/amazonaws/serverless/proxy/spring/FilterAuthorizationBypassTest.java b/aws-serverless-java-container-springboot4/src/test/java/com/amazonaws/serverless/proxy/spring/FilterAuthorizationBypassTest.java new file mode 100644 index 000000000..929fa5d65 --- /dev/null +++ b/aws-serverless-java-container-springboot4/src/test/java/com/amazonaws/serverless/proxy/spring/FilterAuthorizationBypassTest.java @@ -0,0 +1,169 @@ +package com.amazonaws.serverless.proxy.spring; + +import com.amazonaws.serverless.proxy.internal.testutils.AwsProxyRequestBuilder; +import com.amazonaws.serverless.proxy.internal.testutils.MockLambdaContext; +import com.amazonaws.serverless.proxy.model.AwsProxyResponse; +import com.amazonaws.serverless.proxy.spring.filterauthapp.AdminAuthorizationFilter; +import com.amazonaws.serverless.proxy.spring.filterauthapp.AdminWildcardController; +import com.amazonaws.serverless.proxy.spring.filterauthapp.FilterAuthApplication; +import com.amazonaws.serverless.proxy.spring.filterauthapp.LambdaHandler; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; + +import java.util.Arrays; +import java.util.Collection; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * End-to-end regression test for the filter-selection authorization bypass, driven through the shipped Lambda entry + * point rather than the filter matcher in isolation. + * + * The application protects /admin/* with a deny-by-default servlet Filter that never calls chain.doFilter, so the + * response is unambiguous: 403 with FORBIDDEN means the filter was selected and ran, and 200 with TOP_SECRET means it + * was skipped and the protected handler served the request. + * + * Before the fix, filter url-pattern matching used the raw undecoded path from getRequestURI() while servlet + * resolution used the decoded path from getPathInfo(), so an encoded or dot-segment spelling of /admin/secret + * returned TOP_SECRET with the filter never invoked. + */ +public class FilterAuthorizationBypassTest { + private final MockLambdaContext lambdaContext = new MockLambdaContext(); + + private LambdaHandler handler; + + public static Collection data() { + return Arrays.asList(new Object[]{"API_GW", "ALB", "HTTP_API"}); + } + + private AwsProxyResponse get(String reqType, String path) { + handler = new LambdaHandler(reqType); + AdminAuthorizationFilter.resetInvocations(); + return handler.handleRequest(new AwsProxyRequestBuilder(path, "GET"), lambdaContext); + } + + private void assertProtected(String reqType, String path) { + AwsProxyResponse resp = get(reqType, path); + assertEquals(403, resp.getStatusCode(), + "expected the filter to protect " + path + " on " + reqType + " but got " + resp.getBody()); + assertEquals(AdminAuthorizationFilter.FORBIDDEN_BODY, resp.getBody()); + assertNotEquals(FilterAuthApplication.SECRET_BODY, resp.getBody()); + assertTrue(AdminAuthorizationFilter.getInvocations() > 0, + "the filter was never invoked for " + path + " on " + reqType); + } + + /** Control: the plain protected path is blocked. If this fails, the app is not wired as the test assumes. */ + @MethodSource("data") + @ParameterizedTest + void plainProtectedPath_isBlocked(String reqType) { + assertProtected(reqType, "/admin/secret"); + } + + /** Control: the filter mapping really is scoped, so an unprotected path is served normally. */ + @MethodSource("data") + @ParameterizedTest + void unprotectedPath_isServed(String reqType) { + AwsProxyResponse resp = get(reqType, "/public/info"); + assertEquals(200, resp.getStatusCode()); + assertEquals(FilterAuthApplication.PUBLIC_BODY, resp.getBody()); + assertEquals(0, AdminAuthorizationFilter.getInvocations()); + } + + /** The reported bypass: %61 is an encoded "a", so this is /admin/secret. */ + @MethodSource("data") + @ParameterizedTest + void percentEncodedFirstCharacter_isBlocked(String reqType) { + assertProtected(reqType, "/%61dmin/secret"); + } + + /** The same bypass with the encoding in the middle of the protected segment. */ + @MethodSource("data") + @ParameterizedTest + void percentEncodedMiddleCharacter_isBlocked(String reqType) { + assertProtected(reqType, "/adm%69n/secret"); + } + + /** Every character of the protected segment encoded. */ + @MethodSource("data") + @ParameterizedTest + void fullyEncodedSegment_isBlocked(String reqType) { + assertProtected(reqType, "/%61%64%6d%69%6e/secret"); + } + + /** Uppercase hex digits must decode the same as lowercase. */ + @MethodSource("data") + @ParameterizedTest + void uppercaseHexEncoding_isBlocked(String reqType) { + assertProtected(reqType, "/%41dmin/secret"); + } + + /** + * A dot-segment detour that resolves back into the protected path, and which needs no encoding at all. Spring + * normalizes this to /admin/secret when routing, so filter selection has to see it the same way. + */ + @MethodSource("data") + @ParameterizedTest + void dotSegmentTraversal_isBlocked(String reqType) { + assertProtected(reqType, "/public/../admin/secret"); + } + + /** + * Spellings that step OUT of the protected prefix. The canonical form is "/public/info", but Spring MVC matches + * on the undecoded request URI and leaves dot segments to a servlet container that does not exist here, so + * "/admin/**" still reaches an admin handler. Filter selection has to apply for the decoded-but-not-normalized + * spelling too, since matching on the canonical path alone leaves this reaching the handler unfiltered. + */ + @MethodSource("data") + @ParameterizedTest + void dotSegmentSteppingOutOfProtectedPrefix_isBlocked(String reqType) { + assertProtected(reqType, "/admin/../public/info"); + assertProtected(reqType, "/admin/x/../../public/info"); + } + + /** The same, with the separators and dot segments percent-encoded. */ + @MethodSource("data") + @ParameterizedTest + void encodedSeparatorSteppingOutOfProtectedPrefix_isBlocked(String reqType) { + assertProtected(reqType, "/admin/secret%2F..%2F..%2Fpublic%2Finfo"); + } + + /** No spelling may reach the wildcard admin handler without its filter. */ + @MethodSource("data") + @ParameterizedTest + void wildcardAdminHandlerIsNeverReachedUnfiltered(String reqType) { + for (String path : new String[]{ + "/admin/../public/info", + "/admin/x/../../public/info", + "/admin/secret%2F..%2F..%2Fpublic%2Finfo", + "/%61dmin/../public/info"}) { + AwsProxyResponse resp = get(reqType, path); + assertNotEquals(AdminWildcardController.WILDCARD_SECRET, resp.getBody(), + "the wildcard admin handler was reached unfiltered via " + path + " on " + reqType); + } + } + + + /** The protected body must never be returned for any spelling of the protected path. */ + @MethodSource("data") + @ParameterizedTest + void noSpellingOfProtectedPathLeaksTheSecret(String reqType) { + for (String path : new String[]{ + "/admin/secret", + "/%61dmin/secret", + "/adm%69n/secret", + "/%61%64%6d%69%6e/secret", + "/%41dmin/secret", + "/public/../admin/secret", + "/admin%2Fsecret", + "/ADMIN/secret", + "/admin//secret", + "/./admin/secret" + }) { + AwsProxyResponse resp = get(reqType, path); + assertNotEquals(FilterAuthApplication.SECRET_BODY, resp.getBody(), + "the secret leaked via " + path + " on " + reqType); + } + } +} diff --git a/aws-serverless-java-container-springboot4/src/test/java/com/amazonaws/serverless/proxy/spring/filterauthapp/AdminAuthorizationFilter.java b/aws-serverless-java-container-springboot4/src/test/java/com/amazonaws/serverless/proxy/spring/filterauthapp/AdminAuthorizationFilter.java new file mode 100644 index 000000000..ce367d2e9 --- /dev/null +++ b/aws-serverless-java-container-springboot4/src/test/java/com/amazonaws/serverless/proxy/spring/filterauthapp/AdminAuthorizationFilter.java @@ -0,0 +1,41 @@ +package com.amazonaws.serverless.proxy.spring.filterauthapp; + +import jakarta.servlet.Filter; +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; +import jakarta.servlet.http.HttpServletResponse; + +import java.io.IOException; +import java.util.concurrent.atomic.AtomicInteger; + +/** + * A deny-by-default authorization filter, mapped to the protected path only. It never calls + * chain.doFilter, so the response tells you unambiguously whether the filter was selected for the + * request: 403 means it ran, and the protected body coming back means it did not. + */ +public class AdminAuthorizationFilter implements Filter { + public static final String FORBIDDEN_BODY = "FORBIDDEN"; + + private static final AtomicInteger INVOCATIONS = new AtomicInteger(0); + + public static int getInvocations() { + return INVOCATIONS.get(); + } + + public static void resetInvocations() { + INVOCATIONS.set(0); + } + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + INVOCATIONS.incrementAndGet(); + HttpServletResponse httpResponse = (HttpServletResponse) response; + httpResponse.setStatus(403); + httpResponse.setContentType("text/plain"); + httpResponse.getWriter().write(FORBIDDEN_BODY); + // deliberately not calling chain.doFilter - the request stops here + } +} diff --git a/aws-serverless-java-container-springboot4/src/test/java/com/amazonaws/serverless/proxy/spring/filterauthapp/AdminWildcardController.java b/aws-serverless-java-container-springboot4/src/test/java/com/amazonaws/serverless/proxy/spring/filterauthapp/AdminWildcardController.java new file mode 100644 index 000000000..899cc0a7d --- /dev/null +++ b/aws-serverless-java-container-springboot4/src/test/java/com/amazonaws/serverless/proxy/spring/filterauthapp/AdminWildcardController.java @@ -0,0 +1,15 @@ +package com.amazonaws.serverless.proxy.spring.filterauthapp; + +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.RestController; + +/** Mirrors the reviewer's scenario: an admin handler whose pattern can match a raw, un-normalized path. */ +@RestController +public class AdminWildcardController { + public static final String WILDCARD_SECRET = "WILDCARD_TOP_SECRET"; + + @GetMapping("/admin/**") + public String anyAdmin() { + return WILDCARD_SECRET; + } +} diff --git a/aws-serverless-java-container-springboot4/src/test/java/com/amazonaws/serverless/proxy/spring/filterauthapp/FilterAuthApplication.java b/aws-serverless-java-container-springboot4/src/test/java/com/amazonaws/serverless/proxy/spring/filterauthapp/FilterAuthApplication.java new file mode 100644 index 000000000..ebe3e5b8a --- /dev/null +++ b/aws-serverless-java-container-springboot4/src/test/java/com/amazonaws/serverless/proxy/spring/filterauthapp/FilterAuthApplication.java @@ -0,0 +1,41 @@ +package com.amazonaws.serverless.proxy.spring.filterauthapp; + +import org.springframework.boot.autoconfigure.SpringBootApplication; +import org.springframework.boot.web.servlet.FilterRegistrationBean; +import org.springframework.context.annotation.Bean; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.RestController; + +/** + * A plain Spring Boot servlet application whose only protection on /admin/* is a servlet Filter registered with a + * path-scoped url-pattern. This is the configuration the reported authorization bypass affects - hand-rolled filter + * authorization, as opposed to Spring Security. + */ +// No Spring Security auto-configuration to exclude here: Boot 4 moved those classes out of +// org.springframework.boot.autoconfigure.security.servlet, so nothing secures this app implicitly and the +// path-scoped filter below is the only protection. The springboot3 copy of this class does need the exclusion. +@SpringBootApplication +@RestController +public class FilterAuthApplication { + public static final String SECRET_BODY = "TOP_SECRET"; + public static final String PUBLIC_BODY = "PUBLIC_OK"; + + @GetMapping("/admin/secret") + public String adminSecret() { + return SECRET_BODY; + } + + @GetMapping("/public/info") + public String publicInfo() { + return PUBLIC_BODY; + } + + @Bean + public FilterRegistrationBean adminAuthorizationFilter() { + FilterRegistrationBean registration = new FilterRegistrationBean<>(); + registration.setFilter(new AdminAuthorizationFilter()); + registration.addUrlPatterns("/admin/*"); + registration.setName("adminAuthorizationFilter"); + return registration; + } +} diff --git a/aws-serverless-java-container-springboot4/src/test/java/com/amazonaws/serverless/proxy/spring/filterauthapp/LambdaHandler.java b/aws-serverless-java-container-springboot4/src/test/java/com/amazonaws/serverless/proxy/spring/filterauthapp/LambdaHandler.java new file mode 100644 index 000000000..65ccf159b --- /dev/null +++ b/aws-serverless-java-container-springboot4/src/test/java/com/amazonaws/serverless/proxy/spring/filterauthapp/LambdaHandler.java @@ -0,0 +1,62 @@ +package com.amazonaws.serverless.proxy.spring.filterauthapp; + +import com.amazonaws.serverless.exceptions.ContainerInitializationException; +import com.amazonaws.serverless.proxy.InitializationWrapper; +import com.amazonaws.serverless.proxy.internal.testutils.AwsProxyRequestBuilder; +import com.amazonaws.serverless.proxy.model.AwsProxyRequest; +import com.amazonaws.serverless.proxy.model.AwsProxyResponse; +import com.amazonaws.serverless.proxy.model.HttpApiV2ProxyRequest; +import com.amazonaws.serverless.proxy.spring.SpringBootLambdaContainerHandler; +import com.amazonaws.serverless.proxy.spring.SpringBootProxyHandlerBuilder; +import com.amazonaws.services.lambda.runtime.Context; +import com.amazonaws.services.lambda.runtime.RequestHandler; + +/** + * Drives requests through the shipped Lambda entry point, the same one a deployed function executes. + */ +public class LambdaHandler implements RequestHandler { + private static SpringBootLambdaContainerHandler handler; + private static SpringBootLambdaContainerHandler httpApiHandler; + private String type; + + public LambdaHandler(String reqType) { + type = reqType; + try { + switch (type) { + case "API_GW": + case "ALB": + handler = new SpringBootProxyHandlerBuilder() + .defaultProxy() + .initializationWrapper(new InitializationWrapper()) + .servletApplication() + .springBootApplication(FilterAuthApplication.class) + .buildAndInitialize(); + break; + case "HTTP_API": + httpApiHandler = new SpringBootProxyHandlerBuilder() + .defaultHttpApiV2Proxy() + .initializationWrapper(new InitializationWrapper()) + .servletApplication() + .springBootApplication(FilterAuthApplication.class) + .buildAndInitialize(); + break; + } + } catch (ContainerInitializationException e) { + e.printStackTrace(); + } + } + + @Override + public AwsProxyResponse handleRequest(AwsProxyRequestBuilder awsProxyRequest, Context context) { + switch (type) { + case "API_GW": + return handler.proxy(awsProxyRequest.build(), context); + case "ALB": + return handler.proxy(awsProxyRequest.alb().build(), context); + case "HTTP_API": + return httpApiHandler.proxy(awsProxyRequest.toHttpApiV2Request(), context); + default: + throw new RuntimeException("Unknown request type: " + type); + } + } +}