This is an automated email from the ASF dual-hosted git repository. radu pushed a commit to branch issue/SLING-11722 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-engine.git
commit 8f4ea4153bc6224940b634ba0b03d0b9a2d26121 Author: Radu Cotescu <[email protected]> AuthorDate: Thu Dec 8 14:45:23 2022 +0100 SLING-11722 - The SlingRequestDispatcher doesn't correctly implement the RequestDispatcher API * added two new configuration options to the Sling Main Servlet: sling.includes.protectheaders and sling.includes.checkcontenttype * the former doesn't allow included servlets to set a status code or response headers, whereas the latter also makes the Sling Engine throw a RuntimeException every time an included servlet tries to override the Content-Type response header * sling.includes.protectheaders can be overridden per include, using the RequestDispatcherOptions --- .../java/org/apache/sling/engine/impl/Config.java | 15 +++- ...ncludeNoContentTypeOverrideResponseWrapper.java | 74 ++++++++++++++++++++ .../engine/impl/SlingHttpServletRequestImpl.java | 18 ++++- .../engine/impl/SlingRequestProcessorImpl.java | 28 ++++++-- .../engine/impl/helper/SlingServletContext.java | 7 +- .../sling/engine/impl/request/RequestData.java | 18 ++++- .../impl/request/SlingRequestDispatcher.java | 27 +++++--- ...deNoContentTypeOverrideResponseWrapperTest.java | 81 ++++++++++++++++++++++ .../impl/filter/AbstractSlingFilterChainTest.java | 2 +- .../engine/impl/request/InitResourceTest.java | 2 +- .../sling/engine/impl/request/RequestDataTest.java | 2 +- 11 files changed, 253 insertions(+), 21 deletions(-) diff --git a/src/main/java/org/apache/sling/engine/impl/Config.java b/src/main/java/org/apache/sling/engine/impl/Config.java index 62bc4c3..042bd4c 100644 --- a/src/main/java/org/apache/sling/engine/impl/Config.java +++ b/src/main/java/org/apache/sling/engine/impl/Config.java @@ -91,4 +91,17 @@ public @interface Config { @AttributeDefinition(name = "Servlet Name", description = "Optional name for the Sling main servlet registered by this component") String servlet_name(); -} \ No newline at end of file + + @AttributeDefinition(name = "Protect Headers on Includes", + description = "When enabled, servlets included via the RequestDispatcher will not be able to change the " + + "response status code or set headers. Any attempt to make a change is ignored. This behaviour can " + + "be overridden per include via the 'protectHeadersOnInclude' RequestDispatcherOptions key.") + boolean sling_includes_protectheaders() default false; + + @AttributeDefinition(name = "Check Content-Type overwrites", + description = "When enabled, in addition to not allowing servlets included via the RequestDispatcher to " + + "change the response status code or set headers, it will also check explicit overrides of the " + + "Content-Type header and will make the Sling Engine throw a RuntimeException when such an " + + "override is detected.") + boolean sling_includes_checkcontenttype() default false; +} diff --git a/src/main/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapper.java b/src/main/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapper.java new file mode 100644 index 0000000..d0dbb10 --- /dev/null +++ b/src/main/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapper.java @@ -0,0 +1,74 @@ +/*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + ~ 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 org.apache.sling.api.SlingException; +import org.apache.sling.api.SlingHttpServletResponse; +import org.apache.sling.api.request.RequestProgressTracker; +import org.jetbrains.annotations.NotNull; + +import java.util.Arrays; +import java.util.Optional; + +public class IncludeNoContentTypeOverrideResponseWrapper extends IncludeResponseWrapper { + + private final RequestProgressTracker requestProgressTracker; + private final String activeServletName; + + /** + * Wraps a response object and throws a {@link RuntimeException} if {@link #setContentType(String)} is called with + * a value that would override the response's previous set value. + * + * @param wrappedResponse the response to be wrapped + */ + public IncludeNoContentTypeOverrideResponseWrapper(@NotNull RequestProgressTracker requestProgressTracker, + @NotNull String activeServletName, + @NotNull SlingHttpServletResponse wrappedResponse) { + super(wrappedResponse); + this.requestProgressTracker = requestProgressTracker; + this.activeServletName = activeServletName; + } + + @Override + public void setContentType(String type) { + String contentTypeString = getContentType(); + if (contentTypeString != null) { + Optional<String> currentMime = Arrays.stream(contentTypeString.split(";")).findFirst(); + Optional<String> setMime = Arrays.stream(type.split(";")).findFirst(); + if (currentMime.isPresent() && setMime.isPresent() && !currentMime.get().equals(setMime.get())) { + String message = String.format( + "Servlet %s tried to override the 'Content-Type' header from '%s' to '%s', however the %s forbids this via the %s configuration property.", + activeServletName, + currentMime.get(), + setMime.get(), + Config.PID, + "sling.includes.checkcontenttype" + ); + requestProgressTracker.log(message); + throw new ContentTypeChangeException(message); + } + } + } + + private static class ContentTypeChangeException extends SlingException { + protected ContentTypeChangeException(String text) { + super(text); + } + } +} diff --git a/src/main/java/org/apache/sling/engine/impl/SlingHttpServletRequestImpl.java b/src/main/java/org/apache/sling/engine/impl/SlingHttpServletRequestImpl.java index 13cfa43..8d02dc8 100644 --- a/src/main/java/org/apache/sling/engine/impl/SlingHttpServletRequestImpl.java +++ b/src/main/java/org/apache/sling/engine/impl/SlingHttpServletRequestImpl.java @@ -124,7 +124,14 @@ public class SlingHttpServletRequestImpl extends HttpServletRequestWrapper imple @Override public RequestDispatcher getRequestDispatcher(Resource resource, RequestDispatcherOptions options) { - return (resource != null) ? new SlingRequestDispatcher(resource, options) : null; + return (resource != null) ? + new SlingRequestDispatcher( + resource, + options, + requestData.protectHeadersOnInclude(), + requestData.checkContentTypeOnInclude() + ) + : null; } /** @@ -141,7 +148,14 @@ public class SlingHttpServletRequestImpl extends HttpServletRequestWrapper imple @Override public RequestDispatcher getRequestDispatcher(String path, RequestDispatcherOptions options) { - return (path != null) ? new SlingRequestDispatcher(path, options) : null; + return (path != null) ? + new SlingRequestDispatcher( + path, + options, + requestData.protectHeadersOnInclude(), + requestData.checkContentTypeOnInclude() + ) + : null; } /** 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 564126c..e5a9ab3 100644 --- a/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java +++ b/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java @@ -113,6 +113,9 @@ public class SlingRequestProcessorImpl implements SlingRequestProcessor { private volatile List<StaticResponseHeader> additionalResponseHeaders = Collections.emptyList(); + private volatile boolean protectHeadersOnInclude; + private volatile boolean checkContentTypeOnInclude; + @Activate public void activate(final Config config) { this.errorHandler.setServerInfo(this.slingServletContext.getServerInfo()); @@ -140,6 +143,8 @@ public class SlingRequestProcessorImpl implements SlingRequestProcessor { // configure the request limits this.maxInclusionCounter = config.sling_max_inclusions(); this.maxCallCounter = config.sling_max_calls(); + this.protectHeadersOnInclude = config.sling_includes_protectheaders(); + this.checkContentTypeOnInclude = config.sling_includes_checkcontenttype(); } @Reference(name = "ErrorHandler", cardinality=ReferenceCardinality.OPTIONAL, policy = ReferencePolicy.DYNAMIC, unbind = "unsetErrorHandler") @@ -204,7 +209,7 @@ public class SlingRequestProcessorImpl implements SlingRequestProcessor { } // setting the Sling request and response - final RequestData requestData = new RequestData(this, servletRequest, servletResponse); + final RequestData requestData = new RequestData(this, servletRequest, servletResponse, protectHeadersOnInclude, checkContentTypeOnInclude); final SlingHttpServletRequest request = requestData.getSlingRequest(); final SlingHttpServletResponse response = requestData.getSlingResponse(); @@ -365,17 +370,19 @@ public class SlingRequestProcessorImpl implements SlingRequestProcessor { * @param resolvedURL Request path info * @param include Is this an include (or forward) ? * @param protectHeadersOnInclude Should the headers be protected on include? + * @param checkContentTypeOnInclude Should we prevent changing the Content-Type on include? */ public void dispatchRequest(final ServletRequest request, final ServletResponse response, final Resource resource, final RequestPathInfo resolvedURL, final boolean include, - final boolean protectHeadersOnInclude) throws IOException, ServletException { + final boolean protectHeadersOnInclude, + final boolean checkContentTypeOnInclude) throws IOException, ServletException { // we need a SlingHttpServletRequest/SlingHttpServletResponse tupel // to continue final SlingHttpServletRequest cRequest = RequestData.toSlingHttpServletRequest(request); - final SlingHttpServletResponse cResponse = RequestData.toSlingHttpServletResponse(response); + SlingHttpServletResponse cResponse = RequestData.toSlingHttpServletResponse(response); // get the request data (and btw check the correct type) final RequestData requestData = RequestData.getRequestData(cRequest); @@ -390,8 +397,19 @@ public class SlingRequestProcessorImpl implements SlingRequestProcessor { FilterChainType type = include ? FilterChainType.INCLUDE : FilterChainType.FORWARD; - - processComponent(cRequest, include && protectHeadersOnInclude ? new IncludeResponseWrapper(cResponse) : cResponse, type); + if (include) { + if (protectHeadersOnInclude) { + cResponse = new IncludeResponseWrapper(cResponse); + } + if (checkContentTypeOnInclude) { + cResponse = new IncludeNoContentTypeOverrideResponseWrapper( + requestData.getRequestProgressTracker(), + requestData.getActiveServletName(), + cResponse + ); + } + } + processComponent(cRequest, cResponse, type); } finally { requestData.resetContent(oldContentData); } diff --git a/src/main/java/org/apache/sling/engine/impl/helper/SlingServletContext.java b/src/main/java/org/apache/sling/engine/impl/helper/SlingServletContext.java index 3fc1423..3b60d35 100644 --- a/src/main/java/org/apache/sling/engine/impl/helper/SlingServletContext.java +++ b/src/main/java/org/apache/sling/engine/impl/helper/SlingServletContext.java @@ -120,12 +120,17 @@ public class SlingServletContext implements ServletContext, ServletContextListen private volatile ServiceRegistration<ServletContext> registration; + private final boolean protectHeadersOnInclude; + private final boolean checkContentTypeOnInclude; + @Activate public SlingServletContext(final Config config, final BundleContext bundleContext, @Reference final ProductInfoProvider infoProvider) { this.bundleContext = bundleContext; this.productInfoProvider = infoProvider; + this.protectHeadersOnInclude = config.sling_includes_protectheaders(); + this.checkContentTypeOnInclude = config.sling_includes_checkcontenttype(); this.setup(config); } @@ -415,7 +420,7 @@ public class SlingServletContext implements ServletContext, ServletContextListen return null; } - return new SlingRequestDispatcher(path, null); + return new SlingRequestDispatcher(path, null, protectHeadersOnInclude, checkContentTypeOnInclude); } /** diff --git a/src/main/java/org/apache/sling/engine/impl/request/RequestData.java b/src/main/java/org/apache/sling/engine/impl/request/RequestData.java index b4e979c..c622a81 100644 --- a/src/main/java/org/apache/sling/engine/impl/request/RequestData.java +++ b/src/main/java/org/apache/sling/engine/impl/request/RequestData.java @@ -107,6 +107,10 @@ public class RequestData { /** The original servlet Servlet Response object */ private final SlingHttpServletResponse slingResponse; + private final boolean protectHeadersOnInclude; + + private final boolean checkContentTypeOnInclude; + /** The parameter support class */ private ParameterSupport parameterSupport; @@ -151,13 +155,17 @@ public class RequestData { } public RequestData(SlingRequestProcessorImpl slingRequestProcessor, - HttpServletRequest request, HttpServletResponse response) { + HttpServletRequest request, HttpServletResponse response, + boolean protectHeadersOnInclude, + boolean checkContentTypeOnInclude) { this.startTimestamp = System.currentTimeMillis(); this.slingRequestProcessor = slingRequestProcessor; this.servletRequest = request; this.servletResponse = response; + this.protectHeadersOnInclude = protectHeadersOnInclude; + this.checkContentTypeOnInclude = checkContentTypeOnInclude; this.slingRequest = new SlingHttpServletRequestImpl(this, this.servletRequest); @@ -582,6 +590,14 @@ public class RequestData { return servletCallCounter; } + public boolean protectHeadersOnInclude() { + return protectHeadersOnInclude; + } + + public boolean checkContentTypeOnInclude() { + return checkContentTypeOnInclude; + } + /** * Returns {@code true} if the number of {@code RequestDispatcher.include} * calls has been reached within the given request. That maximum number may diff --git a/src/main/java/org/apache/sling/engine/impl/request/SlingRequestDispatcher.java b/src/main/java/org/apache/sling/engine/impl/request/SlingRequestDispatcher.java index c83ea08..5006cfb 100644 --- a/src/main/java/org/apache/sling/engine/impl/request/SlingRequestDispatcher.java +++ b/src/main/java/org/apache/sling/engine/impl/request/SlingRequestDispatcher.java @@ -43,22 +43,31 @@ public class SlingRequestDispatcher implements RequestDispatcher { private Resource resource; - private RequestDispatcherOptions options; + private final RequestDispatcherOptions options; - private String path; + private final String path; - public SlingRequestDispatcher(String path, RequestDispatcherOptions options) { + private final boolean protectHeadersOnInclude; + private final boolean checkContentTypeOnInclude; + + public SlingRequestDispatcher(String path, RequestDispatcherOptions options, + boolean protectHeadersOnInclude, + boolean checkContentTypeOnInclude) { this.path = path; this.options = options; this.resource = null; + this.protectHeadersOnInclude = protectHeadersOnInclude; + this.checkContentTypeOnInclude = checkContentTypeOnInclude; } - public SlingRequestDispatcher(Resource resource, - RequestDispatcherOptions options) { - + public SlingRequestDispatcher(Resource resource, RequestDispatcherOptions options, + boolean protectHeadersOnInclude, + boolean checkContentTypeOnInclude) { this.resource = resource; this.options = options; this.path = resource.getPath(); + this.protectHeadersOnInclude = protectHeadersOnInclude; + this.checkContentTypeOnInclude = checkContentTypeOnInclude; } @Override @@ -214,9 +223,11 @@ public class SlingRequestDispatcher implements RequestDispatcher { SlingRequestPathInfo info = getMergedRequestPathInfo(cRequest); requestProgressTracker.log( "Including resource {0} ({1})", resource, info); - final boolean protectHeaders = this.options != null ? this.options.isProtectHeadersOnInclude() : false; + boolean protectHeaders = this.options != null ? + Boolean.parseBoolean(this.options.getOrDefault(RequestDispatcherOptions.OPT_PROTECT_HEADERS_ON_INCLUDE, String.valueOf(this.protectHeadersOnInclude))) + : this.protectHeadersOnInclude; rd.getSlingRequestProcessor().dispatchRequest(request, response, resource, - info, include, protectHeaders); + info, include, protectHeaders, this.checkContentTypeOnInclude); } /** diff --git a/src/test/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapperTest.java b/src/test/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapperTest.java new file mode 100644 index 0000000..b0d4879 --- /dev/null +++ b/src/test/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapperTest.java @@ -0,0 +1,81 @@ +/*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + ~ 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 org.apache.sling.api.SlingHttpServletResponse; +import org.apache.sling.api.request.RequestProgressTracker; +import org.junit.Test; + +import javax.servlet.ServletException; +import java.io.IOException; + +import static org.junit.Assert.*; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class IncludeNoContentTypeOverrideResponseWrapperTest { + + private static final String ACTIVE_SERVLET_NAME = "activeServlet"; + + @Test + public void testContentTypeOverrideThrows() throws ServletException, IOException { + RequestProgressTracker requestProgressTracker = mock(RequestProgressTracker.class); + SlingHttpServletResponse response = mock(SlingHttpServletResponse.class); + when(response.getContentType()).thenReturn("text/html"); + + + IncludeNoContentTypeOverrideResponseWrapper wrapper = new IncludeNoContentTypeOverrideResponseWrapper( + requestProgressTracker, ACTIVE_SERVLET_NAME, response + ); + + Throwable throwable = null; + try { + wrapper.setContentType("application/json"); + } catch (Throwable t) { + throwable = t; + } + assertNotNull(throwable); + assertEquals("Servlet activeServlet tried to override the 'Content-Type' header from 'text/html' to " + + "'application/json', however the org.apache.sling.engine.impl.SlingMainServlet forbids this via the " + + "sling.includes.checkcontenttype configuration property.", throwable.getMessage()); + } + + @Test + public void testContentTypeOverrideLenient() throws ServletException, IOException { + RequestProgressTracker requestProgressTracker = mock(RequestProgressTracker.class); + SlingHttpServletResponse response = mock(SlingHttpServletResponse.class); + when(response.getContentType()).thenReturn("text/html"); + + + IncludeNoContentTypeOverrideResponseWrapper wrapper = new IncludeNoContentTypeOverrideResponseWrapper( + requestProgressTracker, ACTIVE_SERVLET_NAME, response + ); + + Throwable throwable = null; + try { + wrapper.setContentType("text/html;utf-8"); + } catch (Throwable t) { + throwable = t; + } + assertNull(throwable); + assertEquals("text/html", wrapper.getContentType()); + } + +} diff --git a/src/test/java/org/apache/sling/engine/impl/filter/AbstractSlingFilterChainTest.java b/src/test/java/org/apache/sling/engine/impl/filter/AbstractSlingFilterChainTest.java index ee9ec30..a2631d9 100644 --- a/src/test/java/org/apache/sling/engine/impl/filter/AbstractSlingFilterChainTest.java +++ b/src/test/java/org/apache/sling/engine/impl/filter/AbstractSlingFilterChainTest.java @@ -65,7 +65,7 @@ public class AbstractSlingFilterChainTest extends AbstractFilterTest { }; HttpServletRequest httpReq = whateverRequest(); final RequestData requestData = new RequestData(new SlingRequestProcessorImpl(), httpReq, - context.mock(HttpServletResponse.class)); + context.mock(HttpServletResponse.class), false, false); final SlingHttpServletRequestImpl req = new SlingHttpServletRequestImpl(requestData, httpReq); boolean illegalStateCaught = false; try { diff --git a/src/test/java/org/apache/sling/engine/impl/request/InitResourceTest.java b/src/test/java/org/apache/sling/engine/impl/request/InitResourceTest.java index 5cdc380..8163de8 100644 --- a/src/test/java/org/apache/sling/engine/impl/request/InitResourceTest.java +++ b/src/test/java/org/apache/sling/engine/impl/request/InitResourceTest.java @@ -118,7 +118,7 @@ public class InitResourceTest { will(returnValue(Collections.emptyList())); }}); - requestData = new RequestData(processor, req, resp); + requestData = new RequestData(processor, req, resp, false, false); } @Test diff --git a/src/test/java/org/apache/sling/engine/impl/request/RequestDataTest.java b/src/test/java/org/apache/sling/engine/impl/request/RequestDataTest.java index 2979937..7fc9d2c 100644 --- a/src/test/java/org/apache/sling/engine/impl/request/RequestDataTest.java +++ b/src/test/java/org/apache/sling/engine/impl/request/RequestDataTest.java @@ -101,7 +101,7 @@ public class RequestDataTest { }}); - requestData = new RequestData(processor, req, resp) { + requestData = new RequestData(processor, req, resp, false, false) { @Override public ContentData getContentData() { return contentData;
