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 01a28e6c57..0be2189c2f 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 @@ -21,26 +21,58 @@ import com.opensymphony.xwork2.ActionInvocation; import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.interceptor.AbstractInterceptor; +import org.apache.commons.lang3.StringUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.apache.struts2.ModelDriven; import org.apache.struts2.ServletActionContext; import org.apache.struts2.rest.handler.ContentTypeHandler; import javax.servlet.http.HttpServletRequest; +import java.io.FilterReader; +import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; +import java.io.Reader; /** * Uses the content handler to apply the request body to the action */ 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 int maxLength = DEFAULT_MAX_LENGTH; @Inject public void setContentTypeHandlerSelector(ContentTypeHandlerManager selector) { this.selector = selector; } + @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); @@ -51,12 +83,87 @@ public String intercept(ActionInvocation invocation) throws Exception { } if (request.getContentLength() > 0) { - final String encoding = request.getCharacterEncoding(); - InputStream is = request.getInputStream(); - InputStreamReader reader = encoding == null ? new InputStreamReader(is) : new InputStreamReader(is, encoding); - handler.toObject(invocation, reader, target); + BoundedReader reader = new BoundedReader(openBodyReader(request), maxLength); + try { + handler.toObject(invocation, reader, target); + } catch (Exception e) { + if (reader.limitExceeded()) { + throw requestBodyTooLarge(); + } + throw e; + } + if (reader.limitExceeded()) { + throw requestBodyTooLarge(); + } } return invocation.invoke(); } + 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); + } + + /** + * 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 0000000000..5f1e8ef5c2 --- /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 cb47b6a939..f10cc0e781 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 @@ -35,4 +35,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 f680489924..9cbb906cc4 100644 --- a/plugins/rest/src/main/resources/struts-plugin.xml +++ b/plugins/rest/src/main/resources/struts-plugin.xml @@ -40,6 +40,7 @@ + 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 ed1f7e8e72..ba2176b4f1 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 com.opensymphony.xwork2.ActionSupport; import junit.framework.TestCase; -import java.io.InputStreamReader; +import javax.servlet.ReadListener; +import javax.servlet.ServletInputStream; +import java.io.IOException; +import java.io.Reader; +import java.util.Arrays; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; @@ -72,8 +76,8 @@ public boolean matches(Object[] args) { 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(); @@ -83,8 +87,7 @@ public void testRequestWithEncodingAscii() throws Exception { 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); @@ -98,7 +101,7 @@ public boolean matches(Object[] args) { 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() @@ -112,7 +115,7 @@ public boolean matches(Object[] args) { mockContentTypeHandler.verify(); } - public void testRequestWithEncodingUtf() throws Exception { + public void testRequestWithEncodingUtf8() throws Exception { final Charset charset = StandardCharsets.UTF_8; ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); @@ -123,8 +126,7 @@ public void testRequestWithEncodingUtf() throws Exception { 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); @@ -138,7 +140,7 @@ public boolean matches(Object[] args) { 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() @@ -151,4 +153,338 @@ public boolean matches(Object[] args) { mockActionInvocation.verify(); mockContentTypeHandler.verify(); } + + public void testBodyOverLimitIsRejectedBeforeActionRuns() throws Exception { + ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + 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.setMaxLength("8"); + + assertEquals("12345678", interceptAndCaptureBody(interceptor, new MockHttpServletRequest(), + "12345678".getBytes(StandardCharsets.US_ASCII))); + } + + public void testBodyOverLimitIsNotReadToTheEnd() throws Exception { + ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + 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.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.setMaxLength("0"); + + assertEquals("abc", interceptAndCaptureBody(interceptor, new MockHttpServletRequest(), + "abc".getBytes(StandardCharsets.US_ASCII))); + } + + public void testHandlerThatIgnoresTheReaderLeavesBodyUnread() throws Exception { + ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + + 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(); + } + + public void testBlankMaxLengthKeepsDefault() throws Exception { + ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + interceptor.setMaxLength(" "); + + byte[] body = new byte[64 * 1024]; + Arrays.fill(body, (byte) 'x'); + assertEquals(body.length, interceptAndCaptureBody(interceptor, new MockHttpServletRequest(), body).length()); + } + + public void testHandlerThatSwallowsTheLimitIsStillRejected() throws Exception { + ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + interceptor.setMaxLength("8"); + + Mock mockActionInvocation = new Mock(ActionInvocation.class); + mockActionInvocation.expectAndReturn("getAction", new ActionSupport()); + Mock swallowingHandler = new Mock(ContentTypeHandler.class); + swallowingHandler.expect("toObject", new AnyConstraintMatcher() { + public boolean matches(Object[] args) { + try { + readFully((Reader) args[1]); + } catch (RuntimeException swallowed) { + // a handler that hides the reader's failure must not let the action run + } + return true; + } + }); + interceptor.setContentTypeHandlerSelector(selectorReturning((ContentTypeHandler) swallowingHandler.proxy())); + + 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) { + // action never invoked: no "invoke" expectation was set + } + mockActionInvocation.verify(); + } + + public void testHandlerFailureUnderTheLimitPropagatesUnchanged() throws Exception { + ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + interceptor.setMaxLength("8"); + + Mock mockActionInvocation = new Mock(ActionInvocation.class); + mockActionInvocation.expectAndReturn("getAction", new ActionSupport()); + IllegalStateException handlerFailure = new IllegalStateException("malformed"); + Mock failingHandler = new Mock(ContentTypeHandler.class); + failingHandler.expect("toObject", new AnyConstraintMatcher() { + public boolean matches(Object[] args) { + throw handlerFailure; + } + }); + interceptor.setContentTypeHandlerSelector(selectorReturning((ContentTypeHandler) failingHandler.proxy())); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setContent("abc".getBytes(StandardCharsets.US_ASCII)); + ActionContext.of() + .withActionMapping(new ActionMapping()) + .withServletRequest(request) + .bind(); + + try { + interceptor.intercept((ActionInvocation) mockActionInvocation.proxy()); + fail("expected the handler's own exception"); + } catch (IllegalStateException e) { + assertSame(handlerFailure, e); + } + } + + public void testSkippingPastTheLimitIsRejected() throws Exception { + ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + interceptor.setMaxLength("8"); + + Mock mockActionInvocation = new Mock(ActionInvocation.class); + mockActionInvocation.expectAndReturn("getAction", new ActionSupport()); + Mock skippingHandler = new Mock(ContentTypeHandler.class); + skippingHandler.expect("toObject", new AnyConstraintMatcher() { + public boolean matches(Object[] args) { + try { + ((Reader) args[1]).skip(Long.MAX_VALUE); + } catch (IOException e) { + throw new IllegalStateException(e); + } + return true; + } + }); + interceptor.setContentTypeHandlerSelector(selectorReturning((ContentTypeHandler) skippingHandler.proxy())); + + 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) { + // skipped input counts against the limit like read input + } + 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); + } + } }