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

src/main/java/org/apache/sling/engine/impl/Config.java

@@ -91,4 +91,17 @@
 
     @AttributeDefinition(name = "Servlet Name", description = "Optional name for the Sling main servlet registered by this component")
     String servlet_name();
-}
+
+    @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 overrides",
+            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;
+}

src/main/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapper.java

@@ -0,0 +1,73 @@
+/*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ ~ 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.util.Arrays;
+import java.util.Optional;
+
+import org.apache.sling.api.SlingException;
+import org.apache.sling.api.SlingHttpServletResponse;
+import org.apache.sling.engine.impl.request.RequestData;
+import org.jetbrains.annotations.NotNull;
+
+public class IncludeNoContentTypeOverrideResponseWrapper extends IncludeResponseWrapper {
+
+    private final RequestData requestData;
+
+    /**
+     * Wraps a response object and throws a {@link RuntimeException} if {@link #setContentType(String)} is called with
+     * a value that would override the response's previously set value.
+     *
+     * @param requestData     the request data object
+     * @param wrappedResponse the response to be wrapped
+     */
+    public IncludeNoContentTypeOverrideResponseWrapper(@NotNull RequestData requestData,
+                                                       @NotNull SlingHttpServletResponse wrappedResponse) {
+        super(wrappedResponse);
+        this.requestData = requestData;
+    }
+
+    @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.",
+                        requestData.getActiveServletName(),
+                        currentMime.get(),
+                        setMime.get(),
+                        Config.PID,
+                        "sling.includes.checkcontenttype"
+                );
+                requestData.getRequestProgressTracker().log("ERROR: " + message);
+                throw new ContentTypeChangeException(message);
+            }
+        }
+    }
+
+    private static class ContentTypeChangeException extends SlingException {
+        protected ContentTypeChangeException(String text) {
+            super(text);
+        }
+    }
+}

src/main/java/org/apache/sling/engine/impl/SlingHttpServletRequestImpl.java

@@ -124,7 +124,14 @@ public RequestDispatcher getRequestDispatcher(Resource resource) {
     @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 RequestDispatcher getRequestDispatcher(String path) {
     @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;
     }
 
     /**

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 void modified(final Config config) {
         // 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 void doProcessRequest(final HttpServletRequest servletRequest,
         }
 
         // 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 void processComponent(SlingHttpServletRequest request,
      * @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,16 @@ public void dispatchRequest(final ServletRequest request,
             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, cResponse
+                    );
+                }
+            }
+            processComponent(cRequest, cResponse, type);
         } finally {
             requestData.resetContent(oldContentData);
         }

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 RequestDispatcher getRequestDispatcher(String path) {
             return null;
         }
 
-        return new SlingRequestDispatcher(path, null);
+        return new SlingRequestDispatcher(path, null, protectHeadersOnInclude, checkContentTypeOnInclude);
     }
 
     /**

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 int getServletCallCount() {
         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

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 @@ private void dispatch(final ServletRequest request, final ServletResponse respon
         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);
     }
 
     /**