diff --git a/api/src/org/labkey/api/ApiModule.java b/api/src/org/labkey/api/ApiModule.java index 3289b981362..4f235184f10 100644 --- a/api/src/org/labkey/api/ApiModule.java +++ b/api/src/org/labkey/api/ApiModule.java @@ -116,6 +116,7 @@ import org.labkey.api.reader.ExcelLoader; import org.labkey.api.reader.JSONDataLoader; import org.labkey.api.reader.MapLoader; +import org.labkey.api.reader.StrictBoundedReader; import org.labkey.api.reader.TabLoader; import org.labkey.api.reports.model.ViewCategoryManager; import org.labkey.api.reports.report.ReportType; @@ -439,6 +440,7 @@ public void registerServlets(ServletContext servletCtx) SimpleFilter.InClauseTestCase.class, SimpleFilter.SqlClauseTestCase.class, SqlScanner.TestCase.class, + StrictBoundedReader.TestCase.class, StringExpressionFactory.TestCase.class, StringUtilsLabKey.TestCase.class, SubfolderWriter.TestCase.class, diff --git a/api/src/org/labkey/api/action/BaseApiAction.java b/api/src/org/labkey/api/action/BaseApiAction.java index 516bbe61de1..4f9c9364e18 100644 --- a/api/src/org/labkey/api/action/BaseApiAction.java +++ b/api/src/org/labkey/api/action/BaseApiAction.java @@ -365,8 +365,11 @@ protected FormAndErrors
populateJacksonForm() throws Exception // Ideally, ObjectReader would handle the Object case as well, but currently readValue() throws with "end-of-input" exception if (Object.class != c) { - ObjectReader reader = getObjectReader(c); - form = reader.readValue(getViewContext().getRequest().getInputStream()); + ObjectReader objectReader = getObjectReader(c); + try (Reader requestReader = openRequestReader()) + { + form = objectReader.readValue(requestReader); + } } else { @@ -385,7 +388,7 @@ protected FormAndErrors populateJacksonForm() throws Exception errors = new NullSafeBindException(new Object(), "form"); errors.reject(SpringActionController.ERROR_MSG, "Error binding property: " + x.getMessage()); } - catch (JsonProcessingException x) + catch (JsonProcessingException | StrictBoundedReader.LimitExceededException x) { // Bad JSON throw new BadRequestException(x.getMessage(), x); @@ -516,20 +519,24 @@ protected long getMaximumJsonInputLength() if (request == null) return null; + try (Reader reader = openRequestReader()) + { + JSONTokener tokener = new JSONTokener(reader); + return tokener.more() ? new JSONObject(tokener) : null; + } + } + + private Reader openRequestReader() throws IOException + { + HttpServletRequest request = getViewContext().getRequest(); String characterEncoding = request.getCharacterEncoding(); if (characterEncoding == null) characterEncoding = StringUtilsLabKey.DEFAULT_CHARSET.name(); - long maxLength = getMaximumJsonInputLength(); - // Issue 53699: Use request.getInputStream() instead of request.getReader() to // avoid BufferUnderflowException when processing multibyte character JSON payloads. - try (Reader streamReader = new InputStreamReader(request.getInputStream(), characterEncoding); - Reader jsonReader = maxLength > 0 ? new StrictBoundedReader(streamReader, maxLength) : streamReader) - { - JSONTokener tokener = new JSONTokener(jsonReader); - return tokener.more() ? new JSONObject(tokener) : null; - } + Reader streamReader = new InputStreamReader(request.getInputStream(), characterEncoding); + return maxLength > 0 ? new StrictBoundedReader(streamReader, maxLength) : streamReader; } private BindException populateForm(@Nullable JSONObject jsonObj, FORM form) diff --git a/api/src/org/labkey/api/reader/StrictBoundedReader.java b/api/src/org/labkey/api/reader/StrictBoundedReader.java index 463829344df..2510f0c79db 100644 --- a/api/src/org/labkey/api/reader/StrictBoundedReader.java +++ b/api/src/org/labkey/api/reader/StrictBoundedReader.java @@ -1,7 +1,14 @@ package org.labkey.api.reader; +import org.jetbrains.annotations.NotNull; +import org.jspecify.annotations.NonNull; +import org.junit.Assert; +import org.junit.Test; +import org.labkey.api.test.TestWhen; + import java.io.IOException; import java.io.Reader; +import java.io.StringReader; /** * Restricts the Reader to a character limit. Throws an exception when the limit is exceeded. @@ -19,12 +26,13 @@ public static class LimitExceededException extends IOException public LimitExceededException(String message) { super(message); - } } - public StrictBoundedReader(Reader in, long maxChars) + @SuppressWarnings("NullableProblems") + public StrictBoundedReader(@NotNull Reader in, long maxChars) { + //noinspection ConstantValue if (in == null) { throw new NullPointerException("Reader cannot be null"); @@ -54,7 +62,7 @@ public int read() throws IOException } @Override - public int read(char[] cbuf, int off, int len) throws IOException + public int read(char @NonNull [] cbuf, int off, int len) throws IOException { ensureOpen(); if (charsRead >= maxChars) @@ -96,4 +104,151 @@ public void close() throws IOException closed = true; } } + + @TestWhen(TestWhen.When.BVT) + public static class TestCase extends Assert + { + @Test + public void testSingleCharReadWithinLimit() throws IOException + { + try (StrictBoundedReader reader = new StrictBoundedReader(new StringReader("hello"), 10)) + { + assertEquals('h', reader.read()); + assertEquals('e', reader.read()); + assertEquals('l', reader.read()); + } + } + + @Test + public void testBulkReadWithinLimit() throws IOException + { + try (StrictBoundedReader reader = new StrictBoundedReader(new StringReader("hello"), 10)) + { + char[] buf = new char[5]; + int n = reader.read(buf, 0, 5); + assertEquals(5, n); + assertEquals("hello", new String(buf, 0, n)); + } + } + + @Test + public void testBulkReadTruncatedToLimit() throws IOException + { + // When requesting more chars than the limit allows, the read is capped at remaining + try (StrictBoundedReader reader = new StrictBoundedReader(new StringReader("hello world"), 5)) + { + char[] buf = new char[20]; + int n = reader.read(buf, 0, 20); + assertEquals(5, n); + assertEquals("hello", new String(buf, 0, n)); + } + } + + @Test + public void testSingleReadThrowsAfterLimitReached() throws IOException + { + try (StrictBoundedReader reader = new StrictBoundedReader(new StringReader("hello"), 3)) + { + assertEquals('h', reader.read()); + assertEquals('e', reader.read()); + assertEquals('l', reader.read()); + try + { + var _ = reader.read(); + fail("Expected LimitExceededException"); + } + catch (LimitExceededException e) + { + // expected + } + } + } + + @Test + public void testBulkReadThrowsAfterLimitReached() throws IOException + { + try (StrictBoundedReader reader = new StrictBoundedReader(new StringReader("hello world"), 5)) + { + char[] buf = new char[20]; + var _ = reader.read(buf, 0, 20); + try + { + var _ = reader.read(buf, 0, 1); + fail("Expected LimitExceededException"); + } + catch (LimitExceededException e) + { + // expected + } + } + } + + @Test + public void testZeroLimitThrowsImmediately() throws IOException + { + try (StrictBoundedReader reader = new StrictBoundedReader(new StringReader("hello"), 0)) + { + try + { + var _ = reader.read(); + fail("Expected LimitExceededException"); + } + catch (LimitExceededException e) + { + // expected + } + } + } + + @Test + public void testEofReturnedBeforeLimit() throws IOException + { + try (StrictBoundedReader reader = new StrictBoundedReader(new StringReader("hi"), 100)) + { + assertEquals('h', reader.read()); + assertEquals('i', reader.read()); + assertEquals(-1, reader.read()); + } + } + + @Test + public void testBulkEofReturnedBeforeLimit() throws IOException + { + try (StrictBoundedReader reader = new StrictBoundedReader(new StringReader("hi"), 100)) + { + char[] buf = new char[10]; + assertEquals(2, reader.read(buf, 0, 10)); + assertEquals(-1, reader.read(buf, 0, 10)); + } + } + + @Test(expected = NullPointerException.class) + public void testNullReaderThrows() + { + //noinspection DataFlowIssue,resource + new StrictBoundedReader(null, 10); + } + + @Test(expected = IllegalArgumentException.class) + public void testNegativeLimitThrows() + { + new StrictBoundedReader(new StringReader("hello"), -1); + } + + @Test(expected = IOException.class) + public void testReadAfterCloseThrows() throws IOException + { + StrictBoundedReader reader = new StrictBoundedReader(new StringReader("hello"), 10); + reader.close(); + var _ = reader.read(); + } + + @Test + public void testDoubleCloseIsIdempotent() throws IOException + { + StrictBoundedReader reader = new StrictBoundedReader(new StringReader("hello"), 10); + reader.close(); + reader.close(); // should not throw + } + } } diff --git a/devtools/src/org/labkey/devtools/DevtoolsModule.java b/devtools/src/org/labkey/devtools/DevtoolsModule.java index b918e6f01a7..342373df071 100644 --- a/devtools/src/org/labkey/devtools/DevtoolsModule.java +++ b/devtools/src/org/labkey/devtools/DevtoolsModule.java @@ -29,8 +29,11 @@ import org.labkey.devtools.authentication.TestSsoController; import org.labkey.devtools.authentication.TestSsoProvider; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.List; +import java.util.Set; import java.util.function.Supplier; public class DevtoolsModule extends CodeOnlyModule @@ -75,6 +78,16 @@ public void doStartup(ModuleContext moduleContext) @Override public @NotNull Collection>> getIntegrationTestFactories() { - return Collections.singletonList(new JspTestCase("/org/labkey/devtools/test/JspTestCaseTest.jsp")); + List>> list = new ArrayList<>(super.getIntegrationTestFactories()); + list.add(new JspTestCase("/org/labkey/devtools/test/JspTestCaseTest.jsp")); + return list; + } + + @Override + public @NotNull Set> getIntegrationTests() + { + return Set.of( + TestController.JsonInputLimitTest.class + ); } } \ No newline at end of file diff --git a/devtools/src/org/labkey/devtools/TestController.java b/devtools/src/org/labkey/devtools/TestController.java index 5c8cf796240..4e7bc2c726e 100644 --- a/devtools/src/org/labkey/devtools/TestController.java +++ b/devtools/src/org/labkey/devtools/TestController.java @@ -19,12 +19,19 @@ import jakarta.servlet.http.HttpServletResponse; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; +import org.junit.Assert; +import org.junit.Test; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiSimpleResponse; import org.labkey.api.action.ConfirmAction; import org.labkey.api.action.FormArrayList; import org.labkey.api.action.FormViewAction; +import org.labkey.api.action.JsonInputLimit; +import org.labkey.api.action.Marshal; +import org.labkey.api.action.Marshaller; +import org.labkey.api.action.MutatingApiAction; import org.labkey.api.action.ReadOnlyApiAction; +import org.labkey.api.action.SimpleApiJsonForm; import org.labkey.api.action.SimpleResponse; import org.labkey.api.action.SimpleViewAction; import org.labkey.api.action.SpringActionController; @@ -32,12 +39,14 @@ import org.labkey.api.data.ContainerManager; import org.labkey.api.mcp.AbstractAgentAction; import org.labkey.api.mcp.McpService; +import org.labkey.api.security.AdminConsoleAction; import org.labkey.api.security.CSRF; import org.labkey.api.security.MethodsAllowed; import org.labkey.api.security.RequiresLogin; import org.labkey.api.security.RequiresNoPermission; import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.RequiresSiteAdmin; +import org.labkey.api.security.permissions.AdminOperationsPermission; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.InsertPermission; @@ -52,6 +61,7 @@ import org.labkey.api.util.HtmlString; import org.labkey.api.util.HtmlStringBuilder; import org.labkey.api.util.PageFlowUtil; +import org.labkey.api.util.TestContext; import org.labkey.api.util.URLHelper; import org.labkey.api.view.ActionURL; import org.labkey.api.view.HtmlView; @@ -61,6 +71,7 @@ import org.labkey.api.view.NotFoundException; import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.ViewContext; +import org.labkey.api.view.ViewServlet; import org.labkey.api.view.template.ClientDependency; import org.labkey.api.view.template.PageConfig; import org.labkey.api.wiki.WikiService; @@ -68,6 +79,7 @@ import org.springframework.ai.vectorstore.SimpleVectorStore; import org.springframework.ai.vectorstore.VectorStore; import org.springframework.dao.PessimisticLockingFailureException; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.validation.ObjectError; @@ -1404,4 +1416,93 @@ public boolean handlePost(Object o, BindException errors) } } } + + @AdminConsoleAction(AdminOperationsPermission.class) + @JsonInputLimit(50) + public static class TestJsonObjectInputLimitAction extends MutatingApiAction + { + @Override + public ApiResponse execute(SimpleApiJsonForm form, BindException errors) + { + return new ApiSimpleResponse("success", true); + } + } + + @AdminConsoleAction(AdminOperationsPermission.class) + @JsonInputLimit(50) + @Marshal(Marshaller.Jackson) + public static class TestJacksonInputLimitAction extends MutatingApiAction + { + @Override + public ApiResponse execute(TestJsonInputLimitForm form, BindException errors) + { + return new ApiSimpleResponse("success", true); + } + } + + public static class TestJsonInputLimitForm + { + private String value; + + public String getValue() { return value; } + public void setValue(String value) { this.value = value; } + } + + public static class JsonInputLimitTest extends Assert + { + // 14 chars — well under the 50-char limit on the test actions + private static final String UNDER_LIMIT = "{\"value\":\"ok\"}"; + // 62 chars — over the 50-char limit + private static final String OVER_LIMIT = "{\"value\":\"" + "x".repeat(50) + "\"}"; + private static final Map JSON_HEADERS = Map.of("Content-Type", "application/json"); + + @Test + public void testJsonObjectInputLimitOk() throws Exception + { + MockHttpServletResponse response = ViewServlet.POST( + new ActionURL(TestJsonObjectInputLimitAction.class, ContainerManager.getRoot()), + TestContext.get().getUser(), + JSON_HEADERS, + UNDER_LIMIT + ); + assertEquals("Expected 200 for request within limit", HttpServletResponse.SC_OK, response.getStatus()); + } + + @Test + public void testJsonObjectInputLimitExceeded() throws Exception + { + MockHttpServletResponse response = ViewServlet.POST( + new ActionURL(TestJsonObjectInputLimitAction.class, ContainerManager.getRoot()), + TestContext.get().getUser(), + JSON_HEADERS, + OVER_LIMIT + ); + assertEquals("Expected 400 for request exceeding limit", HttpServletResponse.SC_BAD_REQUEST, response.getStatus()); + } + + @Test + public void testJacksonInputLimitOk() throws Exception + { + MockHttpServletResponse response = ViewServlet.POST( + new ActionURL(TestJacksonInputLimitAction.class, ContainerManager.getRoot()), + TestContext.get().getUser(), + JSON_HEADERS, + UNDER_LIMIT + ); + assertEquals("Expected 200 for request within limit", HttpServletResponse.SC_OK, response.getStatus()); + } + + @Test + public void testJacksonInputLimitExceeded() throws Exception + { + MockHttpServletResponse response = ViewServlet.POST( + new ActionURL(TestJacksonInputLimitAction.class, ContainerManager.getRoot()), + TestContext.get().getUser(), + JSON_HEADERS, + OVER_LIMIT + ); + assertEquals("Expected 400 for request exceeding limit", HttpServletResponse.SC_BAD_REQUEST, response.getStatus()); + } + } + }