This is an automated email from the ASF dual-hosted git repository. joerghoh pushed a commit to branch SLING-13364 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-engine.git
commit 50f8ae73d09ad19eb80b0342324f6eea2713383c Author: Joerg Hoh <[email protected]> AuthorDate: Fri Sep 25 12:34:15 2026 +0200 SLING-13364 if the parameter parsing fails throw a statuscode 400 --- .../engine/impl/SlingRequestProcessorImpl.java | 8 ++ .../engine/impl/parameters/ParameterSupport.java | 34 +++-- .../impl/parameters/RequestPartsIterator.java | 4 +- .../parameters/SlingParameterParseException.java | 36 +++++ .../engine/impl/SlingRequestProcessorImplTest.java | 155 +++++++++++++++++++++ .../impl/parameters/ParameterSupportTest.java | 62 +++++++++ .../impl/parameters/RequestPartsIteratorTest.java | 114 +++++++++++++++ 7 files changed, 403 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java b/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java index 27451b6..48137e0 100644 --- a/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java +++ b/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java @@ -67,6 +67,7 @@ import org.apache.sling.engine.impl.filter.ServletFilterManager.FilterChainType; import org.apache.sling.engine.impl.filter.SlingComponentFilterChain; import org.apache.sling.engine.impl.helper.SlingServletContext; import org.apache.sling.engine.impl.parameters.ParameterSupport; +import org.apache.sling.engine.impl.parameters.SlingParameterParseException; import org.apache.sling.engine.impl.request.ContentData; import org.apache.sling.engine.impl.request.DispatchingInfo; import org.apache.sling.engine.impl.request.RequestData; @@ -311,6 +312,13 @@ public class SlingRequestProcessorImpl implements SlingRequestProcessor { log.debug("service: Resource {} not found", rnfe.getResource()); handleError(HttpServletResponse.SC_NOT_FOUND, rnfe.getMessage(), request, response); + } catch (final SlingParameterParseException sppe) { + // the request parameters could not be parsed completely/within + // configured limits; the request itself is malformed, not the + // server, so send this exception as a 400 status + log.debug("service: Failed parsing request parameters", sppe); + handleError(HttpServletResponse.SC_BAD_REQUEST, sppe.getMessage(), request, response); + } catch (final SlingException se) { // send this exception as is (albeit unwrapping and wrapped // exception. diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java index 98bed3c..a4f0631 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java @@ -123,6 +123,12 @@ public class ParameterSupport { private boolean requestDataUsed; + /** + * Set when parsing the request parameters failed. The failure is cached + * and rethrown on every subsequent parameter access. + */ + private SlingParameterParseException parseFailure; + /** * Returns the {@code ParameterSupport} instance supporting request * parameter for the give {@code request}. For a single request only a @@ -240,6 +246,9 @@ public class ParameterSupport { } private ParameterMap getRequestParameterMapInternal() { + if (this.parseFailure != null) { + throw this.parseFailure; + } if (this.postParameterMap == null) { // SLING-508 Try to force servlet container to decode parameters @@ -272,11 +281,11 @@ public class ParameterSupport { Util.parseQueryString(input, Util.ENCODING_DIRECT, parameters, false); addContainerParameters = checkForAdditionalParameters; } catch (IllegalArgumentException e) { - this.log.error("getRequestParameterMapInternal: Error parsing request", e); + throw failParse("Error parsing query string", e); } catch (UnsupportedEncodingException e) { throw new SlingUnsupportedEncodingException(e); } catch (IOException e) { - this.log.error("getRequestParameterMapInternal: Error parsing request", e); + throw failParse("Error parsing query string", e); } useFallback = false; } else { @@ -294,11 +303,11 @@ public class ParameterSupport { Util.parseQueryString(input, encoding, parameters, false); addContainerParameters = checkForAdditionalParameters; } catch (IllegalArgumentException e) { - this.log.error("getRequestParameterMapInternal: Error parsing request", e); + throw failParse("Error parsing request body", e); } catch (UnsupportedEncodingException e) { throw new SlingUnsupportedEncodingException(e); } catch (IOException e) { - this.log.error("getRequestParameterMapInternal: Error parsing request", e); + throw failParse("Error parsing request body", e); } this.requestDataUsed = true; useFallback = false; @@ -316,8 +325,7 @@ public class ParameterSupport { this.log.debug( "getRequestParameterMapInternal: Iterator<javax.servlet.http.Part> available as request attribute named request-parts-iterator"); } catch (final FileUploadException | IOException e) { - this.log.error( - "getRequestParameterMapInternal: Error parsing multipart streamed request", e); + throw failParse("Error parsing multipart streamed request", e); } // The request data has been passed to the RequestPartsIterator, hence from a RequestParameter // pov its been used, and must not be used again. @@ -347,6 +355,16 @@ public class ParameterSupport { return this.postParameterMap; } + /** + * Records a request-parameter parse failure and returns the exception to + * throw. Parse errors must be request-fatal. + */ + private SlingParameterParseException failParse(final String message, final Exception cause) { + this.log.error("getRequestParameterMapInternal: {}", message, cause); + this.parseFailure = new SlingParameterParseException(message, cause); + return this.parseFailure; + } + /** * Checks to see if there is an upload mode header or uploadmode parameter indicating the request is * to be streamed from the client to the server. @@ -431,12 +449,12 @@ public class ParameterSupport { upload.setFileCountMax(ParameterSupport.maxFileCount); final RequestContext rc = this.getMultiPartContext(); - // Parse the request + // Parse the request. A FileUploadException is request-fatal. List<?> /* FileItem */ items = null; try { items = upload.parseRequest(rc); } catch (FileUploadException fue) { - this.log.error("parseMultiPartPost: Error parsing request", fue); + throw failParse("Error parsing multipart request", fue); } if (items != null && items.size() > 0) { diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java b/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java index f018b63..ffad79f 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java @@ -63,8 +63,8 @@ public class RequestPartsIterator implements Iterator<Part> { return itemIterator.hasNext(); } catch (final FileUploadException | IOException e) { LOG.error("hasNext Item failed cause:" + e.getMessage(), e); + throw new SlingParameterParseException("Error reading next part from the request stream", e); } - return false; } @Override @@ -73,8 +73,8 @@ public class RequestPartsIterator implements Iterator<Part> { return new StreamedRequestPart(itemIterator.next()); } catch (final FileUploadException | IOException e) { LOG.error("next Item failed cause:" + e.getMessage(), e); + throw new SlingParameterParseException("Error reading next part from the request stream", e); } - return null; } @Override diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/SlingParameterParseException.java b/src/main/java/org/apache/sling/engine/impl/parameters/SlingParameterParseException.java new file mode 100644 index 0000000..6315fee --- /dev/null +++ b/src/main/java/org/apache/sling/engine/impl/parameters/SlingParameterParseException.java @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.engine.impl.parameters; + +/** + * Unchecked exception indicating that the request parameters could not be + * parsed completely and within the configured limits. Request processing must + * not continue with a silently truncated parameter map. + * <p> + * Deliberately not an {@code IllegalArgumentException} so it is never + * swallowed by lenient parse-error handling. + */ +public class SlingParameterParseException extends IllegalStateException { + + private static final long serialVersionUID = 1L; + + public SlingParameterParseException(final String message, final Throwable cause) { + super(message, cause); + } +} diff --git a/src/test/java/org/apache/sling/engine/impl/SlingRequestProcessorImplTest.java b/src/test/java/org/apache/sling/engine/impl/SlingRequestProcessorImplTest.java new file mode 100644 index 0000000..2a4a160 --- /dev/null +++ b/src/test/java/org/apache/sling/engine/impl/SlingRequestProcessorImplTest.java @@ -0,0 +1,155 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.engine.impl; + +import java.io.PrintWriter; +import java.io.StringWriter; +import java.lang.reflect.Field; + +import jakarta.servlet.Servlet; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.apache.sling.api.SlingJakartaHttpServletRequest; +import org.apache.sling.api.SlingJakartaHttpServletResponse; +import org.apache.sling.api.request.RequestProgressTracker; +import org.apache.sling.api.resource.Resource; +import org.apache.sling.api.resource.ResourceMetadata; +import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.api.servlets.ServletResolver; +import org.apache.sling.engine.impl.filter.FilterHandle; +import org.apache.sling.engine.impl.filter.ServletFilterManager; +import org.apache.sling.engine.impl.filter.ServletFilterManager.FilterChainType; +import org.apache.sling.engine.impl.parameters.SlingParameterParseException; +import org.jetbrains.annotations.NotNull; +import org.junit.Before; +import org.junit.Test; + +import static jakarta.servlet.http.HttpServletResponse.SC_BAD_REQUEST; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Tests for {@link SlingRequestProcessorImpl}, in particular the + * {@code SlingParameterParseException} to HTTP 400 mapping performed in + * {@code doProcessRequest}. + */ +public class SlingRequestProcessorImplTest { + + private SlingRequestProcessorImpl processor; + private ServletFilterManager filterManager; + private SlingJakartaHttpServletRequest request; + private SlingJakartaHttpServletResponse response; + private StringWriter responseBody; + + @Before + public void setup() throws Exception { + processor = new SlingRequestProcessorImpl(); + + filterManager = mock(ServletFilterManager.class); + when(filterManager.getFilters(FilterChainType.ERROR)).thenReturn(new FilterHandle[0]); + setField(processor, "filterManager", filterManager); + + request = mock(SlingJakartaHttpServletRequest.class); + when(request.getRequestProgressTracker()).thenReturn(mock(RequestProgressTracker.class)); + when(request.getRequestURI()).thenReturn("/test"); + + response = mock(SlingJakartaHttpServletResponse.class); + responseBody = new StringWriter(); + when(response.getWriter()).thenReturn(new PrintWriter(responseBody)); + } + + private static void setField(final Object target, final String name, final Object value) throws Exception { + Field field = target.getClass().getDeclaredField(name); + field.setAccessible(true); + field.set(target, value); + } + + @Test + public void testHandleErrorWithNullMessageStillSetsStatus() throws Exception { + processor.handleError(SC_BAD_REQUEST, null, request, response); + + verify(response).setStatus(SC_BAD_REQUEST); + responseBody.flush(); + assertTrue(responseBody.toString().contains(String.valueOf(SC_BAD_REQUEST))); + } + + /** + * End-to-end regression test for SLING-13364: a + * {@link SlingParameterParseException} raised while servicing a request + * (here simulated by the resolved servlet, standing in for the parameter + * parsing that {@code RequestData.service} triggers indirectly) must be + * caught by {@code doProcessRequest} and mapped to a 400 response, + * instead of propagating as a server error or being swallowed. + */ + @Test + public void testDoProcessRequestMapsParameterParseExceptionToBadRequest() throws Exception { + final Servlet servlet = mock(Servlet.class); + doThrow(new SlingParameterParseException("Error parsing query string", new IllegalArgumentException("bad"))) + .when(servlet) + .service(any(ServletRequest.class), any(ServletResponse.class)); + + final Resource resource = getMockedResource("/content/test"); + + final ResourceResolver resourceResolver = mock(ResourceResolver.class); + when(resourceResolver.resolve(any(HttpServletRequest.class), anyString())) + .thenReturn(resource); + + final ServletResolver servletResolver = mock(ServletResolver.class); + when(servletResolver.resolve(any(SlingJakartaHttpServletRequest.class))).thenReturn(servlet); + setField(processor, "servletResolver", servletResolver); + + // no request/component filters: the chain falls straight through to + // the resolved servlet, whose service() call raises the exception + when(filterManager.getFilters(FilterChainType.REQUEST)).thenReturn(new FilterHandle[0]); + when(filterManager.getFilters(FilterChainType.COMPONENT)).thenReturn(new FilterHandle[0]); + + final HttpServletRequest httpServletRequest = mock(HttpServletRequest.class); + when(httpServletRequest.getRequestURI()).thenReturn("/content/test"); + when(httpServletRequest.getRequestURL()).thenReturn(new StringBuffer("http://localhost/content/test";)); + when(httpServletRequest.getContextPath()).thenReturn(""); + when(httpServletRequest.getServletPath()).thenReturn(""); + when(httpServletRequest.getMethod()).thenReturn("GET"); + + final HttpServletResponse httpServletResponse = mock(HttpServletResponse.class); + final StringWriter writer = new StringWriter(); + when(httpServletResponse.getWriter()).thenReturn(new PrintWriter(writer)); + + processor.doProcessRequest(httpServletRequest, httpServletResponse, resourceResolver); + + verify(httpServletResponse).setStatus(SC_BAD_REQUEST); + writer.flush(); + assertTrue(writer.toString().contains("Error parsing query string")); + } + + private static @NotNull Resource getMockedResource(final @NotNull String path) { + final Resource resource = mock(Resource.class); + when(resource.getPath()).thenReturn(path); + final ResourceMetadata resourceMetadata = mock(ResourceMetadata.class); + when(resource.getResourceMetadata()).thenReturn(resourceMetadata); + when(resourceMetadata.getResolutionPathInfo()).thenReturn(path); + return resource; + } +} diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java index 3a7b241..3a290ba 100644 --- a/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java +++ b/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java @@ -30,6 +30,7 @@ import org.junit.Test; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -99,6 +100,67 @@ public class ParameterSupportTest { assertNull(parameterSupport.getParameter("a")); } + @Test(expected = SlingParameterParseException.class) + public void testMalformedQueryStringIsRejected() { + final HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getMethod()).thenReturn("GET"); + // a malformed escape aborts the parse after 'a' was already added; + // 'c' must not be silently dropped while processing continues + when(request.getQueryString()).thenReturn("a=1&b=%zz&c=3"); + + final ParameterSupport support = ParameterSupport.getInstance(request); + support.getParameter("c"); + } + + @Test(expected = SlingParameterParseException.class) + public void testMalformedQueryStringFailureIsCachedAcrossAccessors() { + final HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getMethod()).thenReturn("GET"); + when(request.getQueryString()).thenReturn("a=1&b=%zz&c=3"); + + final ParameterSupport support = ParameterSupport.getInstance(request); + try { + support.getParameter("c"); + fail("Expected the first access to throw SlingParameterParseException"); + } catch (SlingParameterParseException expected) { + // expected on first access; the point of this test is that the + // failure must be cached and rethrown on the *next* access too - + // even via a different accessor method - instead of silently + // continuing with a partial map + } + support.getRequestParameterMap(); + } + + @Test(expected = SlingParameterParseException.class) + public void testMalformedFormEncodedBodyIsRejected() throws IOException { + final HttpServletRequest request = postRequest("application/x-www-form-urlencoded", "a=1&b=%zz&c=3"); + + final ParameterSupport support = ParameterSupport.getInstance(request); + support.getParameter("c"); + } + + @Test(expected = SlingParameterParseException.class) + public void testMalformedMultipartBodyIsRejected() throws IOException { + // "multipart/form-data" without a boundary parameter is structurally + // malformed and rejected outright by commons-fileupload + final HttpServletRequest request = postRequest("multipart/form-data", "irrelevant body content"); + + final ParameterSupport support = ParameterSupport.getInstance(request); + support.getParameter("anything"); + } + + @Test(expected = SlingParameterParseException.class) + public void testMalformedStreamedMultipartBodyIsRejected() throws IOException { + // same malformed multipart body, but with streamed upload mode + // requested - the RequestPartsIterator construction itself must fail + // closed instead of silently producing an empty part sequence + final HttpServletRequest request = postRequest("multipart/form-data", "irrelevant body content"); + when(request.getHeader(ParameterSupport.SLING_UPLOADMODE_HEADER)).thenReturn(ParameterSupport.STREAM_UPLOAD); + + final ParameterSupport support = ParameterSupport.getInstance(request); + support.getParameter("anything"); + } + private static HttpServletRequest postRequest(final String contentType, final String body) throws IOException { final HttpServletRequest request = mock(HttpServletRequest.class); when(request.getMethod()).thenReturn("POST"); diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/RequestPartsIteratorTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/RequestPartsIteratorTest.java new file mode 100644 index 0000000..8a05730 --- /dev/null +++ b/src/test/java/org/apache/sling/engine/impl/parameters/RequestPartsIteratorTest.java @@ -0,0 +1,114 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.engine.impl.parameters; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.lang.reflect.Field; + +import org.apache.commons.fileupload.FileItemIterator; +import org.apache.commons.fileupload.FileUploadException; +import org.apache.commons.fileupload.RequestContext; +import org.junit.Test; + +import static org.junit.Assert.assertFalse; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Regression tests for SLING-13364: a stream turning malformed + * mid-body must surface as an error from {@link RequestPartsIterator} + * instead of silently truncating the part sequence. + */ +public class RequestPartsIteratorTest { + + @Test(expected = SlingParameterParseException.class) + public void testHasNextIsRejectedOnFileUploadException() throws Exception { + final RequestPartsIterator iterator = newIterator(); + final FileItemIterator delegate = mock(FileItemIterator.class); + when(delegate.hasNext()).thenThrow(new FileUploadException("boom")); + injectDelegate(iterator, delegate); + + iterator.hasNext(); + } + + @Test(expected = SlingParameterParseException.class) + public void testHasNextIsRejectedOnIOException() throws Exception { + final RequestPartsIterator iterator = newIterator(); + final FileItemIterator delegate = mock(FileItemIterator.class); + when(delegate.hasNext()).thenThrow(new IOException("connection reset")); + injectDelegate(iterator, delegate); + + iterator.hasNext(); + } + + @Test(expected = SlingParameterParseException.class) + public void testNextIsRejectedOnFileUploadException() throws Exception { + final RequestPartsIterator iterator = newIterator(); + final FileItemIterator delegate = mock(FileItemIterator.class); + when(delegate.next()).thenThrow(new FileUploadException("boom")); + injectDelegate(iterator, delegate); + + iterator.next(); + } + + @Test(expected = SlingParameterParseException.class) + public void testNextIsRejectedOnIOException() throws Exception { + final RequestPartsIterator iterator = newIterator(); + final FileItemIterator delegate = mock(FileItemIterator.class); + when(delegate.next()).thenThrow(new IOException("connection reset")); + injectDelegate(iterator, delegate); + + iterator.next(); + } + + @Test + public void testHasNextPassesThroughCleanEndOfStream() throws Exception { + // sanity check: a well-formed, empty multipart body must not trigger + // the reject-on-error path at all + final RequestPartsIterator iterator = newIterator(); + + assertFalse(iterator.hasNext()); + } + + /** + * Builds a real {@link RequestPartsIterator} over a trivial, well-formed, + * empty multipart body so that construction itself succeeds; the + * commons-fileupload delegate is then swapped out via reflection to + * simulate a stream failing mid-read, which is otherwise very hard to + * trigger deterministically with a hand-crafted byte stream. + */ + private static RequestPartsIterator newIterator() throws FileUploadException, IOException { + final String boundary = "X"; + final String body = "--" + boundary + "--\r\n"; + final RequestContext context = mock(RequestContext.class); + when(context.getContentType()).thenReturn("multipart/form-data; boundary=" + boundary); + when(context.getCharacterEncoding()).thenReturn("UTF-8"); + when(context.getContentLength()).thenReturn(body.length()); + when(context.getInputStream()).thenReturn(new ByteArrayInputStream(body.getBytes(Util.ENCODING_DIRECT))); + return new RequestPartsIterator(context); + } + + private static void injectDelegate(final RequestPartsIterator iterator, final FileItemIterator delegate) + throws Exception { + final Field field = RequestPartsIterator.class.getDeclaredField("itemIterator"); + field.setAccessible(true); + field.set(iterator, delegate); + } +}
