This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5723-rest-body-limit in repository https://gitbox.apache.org/repos/asf/struts.git
commit 6f40627a28307dee4876fdffaae6abf4a751132c Author: Lukasz Lenart <[email protected]> AuthorDate: Fri Sep 11 12:56:13 2026 +0200 WW-5723 feat(rest): bound the request body read in ContentTypeInterceptor The REST plugin handed request.getInputStream() to the content-type handler with no length limit, while the JSON plugin bounds the same read with struts.json.maxLength and CspReportAction with struts.csp.report.maxSize. Apply the same limit here. Add struts.rest.content.maxLength (default 2097152, matching the JSON plugin) as a framework constant injected into the interceptor, so it is set before the interceptor stack runs and behaves identically on both maintenance lines. Blank, non-numeric or sub-1 values are ignored with a warning and the default kept, as CspReportAction does. No upper cap: unlike CspReportAction nothing is pre-allocated, so a large value costs nothing until a body that size arrives. The bound is enforced on the read itself, not on Content-Length: the handler receives a FilterReader that counts characters and fails once the limit is passed. Reading lazily means handlers that never touch the reader (HTML, form-urlencoded, multipart) leave the body untouched for the action, exactly as before, and Jackson keeps streaming rather than parsing from a buffer. Handlers wrap the reader's failure in their own types (Jackson passes IOException through, XStream wraps in StreamException, Juneau in ParseException), so intercept() consults the reader's flag after the call and throws RequestBodyTooLargeException regardless of what propagated. A handler that swallows the failure still fails closed: the flag is checked on the normal return path too, and the action is never invoked. The dedicated exception type lets an application map it to a 413 via exception-mapping without catching every StrutsException. The getContentLength() > 0 gate is unchanged. Two existing tests asserted the handler received an InputStreamReader and read its encoding from it. They now assert the decoded content instead; the ASCII case becomes ISO-8859-1 so the assertion actually discriminates between honouring the request charset and ignoring it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01QmdzKqRSSMyCsdgnv2ubiy --- .../struts2/rest/ContentTypeInterceptor.java | 110 ++++++++- .../struts2/rest/RequestBodyTooLargeException.java | 32 +++ .../org/apache/struts2/rest/RestConstants.java | 1 + plugins/rest/src/main/resources/struts-plugin.xml | 1 + .../struts2/rest/ContentTypeInterceptorTest.java | 253 ++++++++++++++++++++- 5 files changed, 382 insertions(+), 15 deletions(-) diff --git a/plugins/rest/src/main/java/org/apache/struts2/rest/ContentTypeInterceptor.java b/plugins/rest/src/main/java/org/apache/struts2/rest/ContentTypeInterceptor.java index 17ccdf2de..e814e5dfc 100644 --- a/plugins/rest/src/main/java/org/apache/struts2/rest/ContentTypeInterceptor.java +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/ContentTypeInterceptor.java @@ -27,6 +27,7 @@ import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; import org.apache.struts2.ServletActionContext; import org.apache.struts2.rest.handler.ContentTypeHandler; import org.apache.commons.lang3.BooleanUtils; +import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -34,8 +35,11 @@ import jakarta.servlet.http.HttpServletRequest; import java.beans.BeanInfo; import java.beans.Introspector; import java.beans.PropertyDescriptor; +import java.io.FilterReader; +import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; +import java.io.Reader; import java.lang.reflect.Array; import java.lang.reflect.Method; import java.util.ArrayList; @@ -62,9 +66,12 @@ public class ContentTypeInterceptor extends AbstractInterceptor { private static final Logger LOG = LogManager.getLogger(ContentTypeInterceptor.class); + public static final int DEFAULT_MAX_LENGTH = 2_097_152; + private ContentTypeHandlerManager selector; private ParameterAuthorizer parameterAuthorizer; private boolean requireAnnotations = false; + private int maxLength = DEFAULT_MAX_LENGTH; @Inject public void setContentTypeHandlerSelector(ContentTypeHandlerManager selector) { @@ -81,6 +88,27 @@ public class ContentTypeInterceptor extends AbstractInterceptor { this.requireAnnotations = BooleanUtils.toBoolean(requireAnnotations); } + @Inject(value = RestConstants.REST_CONTENT_MAX_LENGTH, required = false) + public void setMaxLength(String maxLength) { + if (StringUtils.isBlank(maxLength)) { + return; + } + int length; + try { + length = Integer.parseInt(maxLength.trim()); + } catch (NumberFormatException e) { + LOG.warn("Ignoring non-numeric {} value: {}, keeping {}", + RestConstants.REST_CONTENT_MAX_LENGTH, maxLength, this.maxLength); + return; + } + if (length < 1) { + LOG.warn("Ignoring out-of-range {} value: {}, expected 1 or more, keeping {}", + RestConstants.REST_CONTENT_MAX_LENGTH, length, this.maxLength); + return; + } + this.maxLength = length; + } + public String intercept(ActionInvocation invocation) throws Exception { HttpServletRequest request = ServletActionContext.getRequest(); ContentTypeHandler handler = selector.getHandlerForRequest(request); @@ -91,19 +119,35 @@ public class ContentTypeInterceptor extends AbstractInterceptor { } if (request.getContentLength() > 0) { - applyRequestBody(invocation, handler, target, openBodyReader(request)); + BoundedReader reader = new BoundedReader(openBodyReader(request), maxLength); + try { + applyRequestBody(invocation, handler, target, reader); + } catch (Exception e) { + if (reader.limitExceeded()) { + throw requestBodyTooLarge(); + } + throw e; + } + if (reader.limitExceeded()) { + throw requestBodyTooLarge(); + } } return invocation.invoke(); } - private static InputStreamReader openBodyReader(HttpServletRequest request) throws java.io.IOException { + private RequestBodyTooLargeException requestBodyTooLarge() { + return new RequestBodyTooLargeException("Request body exceeds maximum allowed length (" + + maxLength + "). Use " + RestConstants.REST_CONTENT_MAX_LENGTH + " to increase the limit."); + } + + private static InputStreamReader openBodyReader(HttpServletRequest request) throws IOException { String encoding = request.getCharacterEncoding(); InputStream is = request.getInputStream(); return encoding == null ? new InputStreamReader(is) : new InputStreamReader(is, encoding); } private void applyRequestBody(ActionInvocation invocation, ContentTypeHandler handler, Object target, - InputStreamReader reader) throws Exception { + Reader reader) throws Exception { if (!requireAnnotations) { // Direct deserialization (backward compat when requireAnnotations is not enabled). handler.toObject(invocation, reader, target); @@ -122,7 +166,7 @@ public class ContentTypeInterceptor extends AbstractInterceptor { * for the call duration. */ private void applyWithAuthorizationContext(ActionInvocation invocation, ContentTypeHandler handler, Object target, - InputStreamReader reader) throws java.io.IOException { + Reader reader) throws IOException { Object action = invocation.getAction(); Object resolvedTarget = parameterAuthorizer.resolveTarget(action); org.apache.struts2.interceptor.parameter.ParameterAuthorizationContext.bind( @@ -141,7 +185,7 @@ public class ContentTypeInterceptor extends AbstractInterceptor { * unauthorized property is nulled out, so skipping is the safer choice). */ private void applyTwoPhaseDeserialize(ActionInvocation invocation, ContentTypeHandler handler, Object target, - InputStreamReader reader) throws Exception { + Reader reader) throws Exception { Object freshInstance = createFreshInstance(target.getClass()); if (freshInstance == null) { LOG.warn("REST body rejected: requireAnnotations=true but [{}] has no no-arg constructor; " @@ -378,4 +422,60 @@ public class ContentTypeInterceptor extends AbstractInterceptor { return true; } + /** + * Stops the handler at {@code struts.rest.content.maxLength} characters. The handler may wrap the + * {@link IOException} thrown here in its own type, so {@link #intercept} consults + * {@link #limitExceeded()} afterwards rather than relying on what propagates. + */ + private static final class BoundedReader extends FilterReader { + + private final int limit; + private long consumed; + private boolean limitExceeded; + + BoundedReader(Reader in, int limit) { + super(in); + this.limit = limit; + } + + @Override + public int read() throws IOException { + int c = super.read(); + if (c != -1) { + consumed(1); + } + return c; + } + + @Override + public int read(char[] buf, int off, int len) throws IOException { + int n = super.read(buf, off, len); + if (n > 0) { + consumed(n); + } + return n; + } + + @Override + public long skip(long n) throws IOException { + long skipped = super.skip(n); + if (skipped > 0) { + consumed(skipped); + } + return skipped; + } + + private void consumed(long n) throws IOException { + consumed += n; + if (consumed > limit) { + limitExceeded = true; + throw new IOException("Request body exceeds " + limit + " characters"); + } + } + + boolean limitExceeded() { + return limitExceeded; + } + } + } diff --git a/plugins/rest/src/main/java/org/apache/struts2/rest/RequestBodyTooLargeException.java b/plugins/rest/src/main/java/org/apache/struts2/rest/RequestBodyTooLargeException.java new file mode 100644 index 000000000..5f1e8ef5c --- /dev/null +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/RequestBodyTooLargeException.java @@ -0,0 +1,32 @@ +/* + * 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.struts2.rest; + +import org.apache.struts2.StrutsException; + +/** + * Thrown by {@link ContentTypeInterceptor} when a request body exceeds + * {@code struts.rest.content.maxLength}. + */ +public class RequestBodyTooLargeException extends StrutsException { + + public RequestBodyTooLargeException(String message) { + super(message); + } +} diff --git a/plugins/rest/src/main/java/org/apache/struts2/rest/RestConstants.java b/plugins/rest/src/main/java/org/apache/struts2/rest/RestConstants.java index d2675ecee..2fd772120 100644 --- a/plugins/rest/src/main/java/org/apache/struts2/rest/RestConstants.java +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/RestConstants.java @@ -36,4 +36,5 @@ public class RestConstants { public static final String REST_MAPPER_PUT_CONTINUE_METHOD_NAME = "struts.mapper.putContinueMethodName"; public static final String STRUTS_REST_NAMESPACE = "struts.rest.namespace"; public static final String REST_VALIDATION_FAILURE_STATUS_CODE = "struts.rest.validationFailureStatusCode"; + public static final String REST_CONTENT_MAX_LENGTH = "struts.rest.content.maxLength"; } diff --git a/plugins/rest/src/main/resources/struts-plugin.xml b/plugins/rest/src/main/resources/struts-plugin.xml index 73ad21146..c6bb08e0e 100644 --- a/plugins/rest/src/main/resources/struts-plugin.xml +++ b/plugins/rest/src/main/resources/struts-plugin.xml @@ -41,6 +41,7 @@ <constant name="struts.rest.logger" value="true" /> <constant name="struts.rest.defaultErrorResultName" value="default-error" /> <constant name="struts.rest.anySetter.requireAnnotations" value="false" /> + <constant name="struts.rest.content.maxLength" value="2097152" /> <constant name="struts.mapper.class" value="rest" /> <constant name="struts.mapper.idParameterName" value="id" /> <constant name="struts.action.extension" value="xhtml,,xml,json" /> diff --git a/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorTest.java b/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorTest.java index a14d407ee..3983ded65 100644 --- a/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorTest.java +++ b/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorTest.java @@ -26,7 +26,11 @@ import org.apache.struts2.ActionInvocation; import org.apache.struts2.ActionSupport; import junit.framework.TestCase; -import java.io.InputStreamReader; +import jakarta.servlet.ReadListener; +import jakarta.servlet.ServletInputStream; +import java.io.IOException; +import java.io.Reader; +import java.util.Arrays; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; @@ -74,8 +78,8 @@ public class ContentTypeInterceptorTest extends TestCase { mockContentTypeHandler.verify(); } - public void testRequestWithEncodingAscii() throws Exception { - final Charset charset = StandardCharsets.US_ASCII; + public void testRequestWithEncodingLatin1() throws Exception { + final Charset charset = StandardCharsets.ISO_8859_1; ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); interceptor.setParameterAuthorizer((parameterName, target, action) -> true); @@ -86,8 +90,7 @@ public class ContentTypeInterceptorTest extends TestCase { Mock mockContentTypeHandler = new Mock(ContentTypeHandler.class); mockContentTypeHandler.expect("toObject", new AnyConstraintMatcher() { public boolean matches(Object[] args) { - InputStreamReader in = (InputStreamReader) args[1]; - return charset.equals(Charset.forName(in.getEncoding())); + return "caf\u00e9".equals(readFully((Reader) args[1])); } }); mockActionInvocation.expectAndReturn("invoke", Action.SUCCESS); @@ -101,7 +104,7 @@ public class ContentTypeInterceptorTest extends TestCase { interceptor.setContentTypeHandlerSelector((ContentTypeHandlerManager) mockContentTypeHandlerManager.proxy()); MockHttpServletRequest request = new MockHttpServletRequest(); - request.setContent(new byte[] {1}); + request.setContent("caf\u00e9".getBytes(charset)); request.setCharacterEncoding(charset.name()); ActionContext.of() @@ -115,7 +118,7 @@ public class ContentTypeInterceptorTest extends TestCase { mockContentTypeHandler.verify(); } - public void testRequestWithEncodingUtf() throws Exception { + public void testRequestWithEncodingUtf8() throws Exception { final Charset charset = StandardCharsets.UTF_8; ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); @@ -127,8 +130,7 @@ public class ContentTypeInterceptorTest extends TestCase { Mock mockContentTypeHandler = new Mock(ContentTypeHandler.class); mockContentTypeHandler.expect("toObject", new AnyConstraintMatcher() { public boolean matches(Object[] args) { - InputStreamReader in = (InputStreamReader) args[1]; - return charset.equals(Charset.forName(in.getEncoding())); + return "caf\u00e9".equals(readFully((Reader) args[1])); } }); mockActionInvocation.expectAndReturn("invoke", Action.SUCCESS); @@ -142,7 +144,7 @@ public class ContentTypeInterceptorTest extends TestCase { interceptor.setContentTypeHandlerSelector((ContentTypeHandlerManager) mockContentTypeHandlerManager.proxy()); MockHttpServletRequest request = new MockHttpServletRequest(); - request.setContent(new byte[] {1}); + request.setContent("caf\u00e9".getBytes(charset)); request.setCharacterEncoding(charset.name()); ActionContext.of() @@ -233,4 +235,235 @@ public class ContentTypeInterceptorTest extends TestCase { mockActionInvocation.verify(); mockContentTypeHandler.verify(); } + + public void testBodyOverLimitIsRejectedBeforeActionRuns() throws Exception { + ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + interceptor.setParameterAuthorizer((parameterName, target, action) -> true); + interceptor.setMaxLength("8"); + + Mock mockActionInvocation = new Mock(ActionInvocation.class); + mockActionInvocation.expectAndReturn("getAction", new ActionSupport()); + interceptor.setContentTypeHandlerSelector(selectorReturning(readingHandler())); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setContent("123456789".getBytes(StandardCharsets.US_ASCII)); + + ActionContext.of() + .withActionMapping(new ActionMapping()) + .withServletRequest(request) + .bind(); + + try { + interceptor.intercept((ActionInvocation) mockActionInvocation.proxy()); + fail("expected " + RequestBodyTooLargeException.class.getSimpleName()); + } catch (RequestBodyTooLargeException expected) { + assertTrue(expected.getMessage().contains(RestConstants.REST_CONTENT_MAX_LENGTH)); + } + mockActionInvocation.verify(); + } + + public void testBodyAtLimitIsPassedToHandlerInFull() throws Exception { + ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + interceptor.setParameterAuthorizer((parameterName, target, action) -> true); + interceptor.setMaxLength("8"); + + assertEquals("12345678", interceptAndCaptureBody(interceptor, new MockHttpServletRequest(), + "12345678".getBytes(StandardCharsets.US_ASCII))); + } + + public void testBodyOverLimitIsNotReadToTheEnd() throws Exception { + ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + interceptor.setParameterAuthorizer((parameterName, target, action) -> true); + interceptor.setMaxLength("8"); + + Mock mockActionInvocation = new Mock(ActionInvocation.class); + mockActionInvocation.expectAndReturn("getAction", new ActionSupport()); + interceptor.setContentTypeHandlerSelector(selectorReturning(readingHandler())); + + byte[] body = new byte[1024 * 1024]; + Arrays.fill(body, (byte) 'x'); + CountingRequest request = new CountingRequest(); + request.setContent(body); + + ActionContext.of() + .withActionMapping(new ActionMapping()) + .withServletRequest(request) + .bind(); + + try { + interceptor.intercept((ActionInvocation) mockActionInvocation.proxy()); + fail("expected " + RequestBodyTooLargeException.class.getSimpleName()); + } catch (RequestBodyTooLargeException expected) { + assertTrue("read " + request.bytesRead + " of " + body.length + " bytes", + request.bytesRead < body.length); + } + } + + public void testNonNumericMaxLengthKeepsDefault() throws Exception { + ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + interceptor.setParameterAuthorizer((parameterName, target, action) -> true); + interceptor.setMaxLength("lots"); + + byte[] body = new byte[64 * 1024]; + Arrays.fill(body, (byte) 'x'); + assertEquals(body.length, interceptAndCaptureBody(interceptor, new MockHttpServletRequest(), body).length()); + } + + public void testMaxLengthBelowOneKeepsDefault() throws Exception { + ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + interceptor.setParameterAuthorizer((parameterName, target, action) -> true); + interceptor.setMaxLength("0"); + + assertEquals("abc", interceptAndCaptureBody(interceptor, new MockHttpServletRequest(), + "abc".getBytes(StandardCharsets.US_ASCII))); + } + + public void testHandlerThatIgnoresTheReaderLeavesBodyUnread() throws Exception { + ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + interceptor.setParameterAuthorizer((parameterName, target, action) -> true); + + Mock mockActionInvocation = new Mock(ActionInvocation.class); + mockActionInvocation.expectAndReturn("invoke", Action.SUCCESS); + mockActionInvocation.expectAndReturn("getAction", new ActionSupport()); + Mock mockContentTypeHandler = new Mock(ContentTypeHandler.class); + mockContentTypeHandler.expect("toObject", new AnyConstraintMatcher() { + public boolean matches(Object[] args) { + return true; + } + }); + Mock mockContentTypeHandlerManager = new Mock(ContentTypeHandlerManager.class); + mockContentTypeHandlerManager.expectAndReturn("getHandlerForRequest", new AnyConstraintMatcher() { + public boolean matches(Object[] args) { + return true; + } + }, mockContentTypeHandler.proxy()); + interceptor.setContentTypeHandlerSelector((ContentTypeHandlerManager) mockContentTypeHandlerManager.proxy()); + + CountingRequest request = new CountingRequest(); + request.setContent("raw body the action may want to read itself".getBytes(StandardCharsets.US_ASCII)); + + ActionContext.of() + .withActionMapping(new ActionMapping()) + .withServletRequest(request) + .bind(); + + interceptor.intercept((ActionInvocation) mockActionInvocation.proxy()); + assertEquals(0, request.bytesRead); + mockActionInvocation.verify(); + } + + /** + * A handler that reads the body the way the real ones do, and surfaces the reader's failure in its + * own exception type as Jackson, XStream and Juneau each do. + */ + private static ContentTypeHandler readingHandler() { + Mock handler = new Mock(ContentTypeHandler.class); + handler.expect("toObject", new AnyConstraintMatcher() { + public boolean matches(Object[] args) { + readFully((Reader) args[1]); + return true; + } + }); + return (ContentTypeHandler) handler.proxy(); + } + + private static ContentTypeHandlerManager selectorReturning(ContentTypeHandler handler) { + Mock selector = new Mock(ContentTypeHandlerManager.class); + selector.expectAndReturn("getHandlerForRequest", new AnyConstraintMatcher() { + public boolean matches(Object[] args) { + return true; + } + }, handler); + return (ContentTypeHandlerManager) selector.proxy(); + } + + private static String interceptAndCaptureBody(ContentTypeInterceptor interceptor, MockHttpServletRequest request, + byte[] body) throws Exception { + String[] captured = new String[1]; + Mock mockActionInvocation = new Mock(ActionInvocation.class); + mockActionInvocation.expectAndReturn("invoke", Action.SUCCESS); + mockActionInvocation.expectAndReturn("getAction", new ActionSupport()); + Mock mockContentTypeHandler = new Mock(ContentTypeHandler.class); + mockContentTypeHandler.expect("toObject", new AnyConstraintMatcher() { + public boolean matches(Object[] args) { + captured[0] = readFully((Reader) args[1]); + return true; + } + }); + Mock mockContentTypeHandlerManager = new Mock(ContentTypeHandlerManager.class); + mockContentTypeHandlerManager.expectAndReturn("getHandlerForRequest", new AnyConstraintMatcher() { + public boolean matches(Object[] args) { + return true; + } + }, mockContentTypeHandler.proxy()); + interceptor.setContentTypeHandlerSelector((ContentTypeHandlerManager) mockContentTypeHandlerManager.proxy()); + + request.setContent(body); + ActionContext.of() + .withActionMapping(new ActionMapping()) + .withServletRequest(request) + .bind(); + + interceptor.intercept((ActionInvocation) mockActionInvocation.proxy()); + mockContentTypeHandler.verify(); + mockActionInvocation.verify(); + return captured[0]; + } + + /** Counts the bytes the interceptor actually pulls from the request stream. */ + private static final class CountingRequest extends MockHttpServletRequest { + long bytesRead; + + @Override + public ServletInputStream getInputStream() { + ServletInputStream delegate = super.getInputStream(); + return new ServletInputStream() { + @Override + public int read() throws IOException { + int b = delegate.read(); + if (b != -1) { + bytesRead++; + } + return b; + } + + @Override + public int read(byte[] buf, int off, int len) throws IOException { + int n = delegate.read(buf, off, len); + if (n > 0) { + bytesRead += n; + } + return n; + } + + @Override + public boolean isFinished() { + return delegate.isFinished(); + } + + @Override + public boolean isReady() { + return delegate.isReady(); + } + + @Override + public void setReadListener(ReadListener readListener) { + delegate.setReadListener(readListener); + } + }; + } + } + + private static String readFully(Reader reader) { + try { + StringBuilder out = new StringBuilder(); + int c; + while ((c = reader.read()) != -1) { + out.append((char) c); + } + return out.toString(); + } catch (IOException e) { + throw new IllegalStateException(e); + } + } }
