SLING-11722 - The SlingRequestDispatcher doesn't correctly implement …

…the RequestDispatcher API

* the IncludeNoContentTypeOverrideResponseWrapper will only report on Content-Type overrides,
but will not prevent included servlets from setting other headers or the status code

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

@@ -18,6 +18,8 @@
  */
 package org.apache.sling.engine.impl;
 
+import org.apache.sling.api.SlingHttpServletRequest;
+import org.apache.sling.api.SlingHttpServletResponse;
 import org.osgi.service.metatype.annotations.AttributeDefinition;
 import org.osgi.service.metatype.annotations.ObjectClassDefinition;
 
@@ -99,9 +101,7 @@
     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.")
+            description = "When enabled, it will 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

@@ -23,10 +23,11 @@
 
 import org.apache.sling.api.SlingException;
 import org.apache.sling.api.SlingHttpServletResponse;
+import org.apache.sling.api.wrappers.SlingHttpServletResponseWrapper;
 import org.apache.sling.engine.impl.request.RequestData;
 import org.jetbrains.annotations.NotNull;
 
-public class IncludeNoContentTypeOverrideResponseWrapper extends IncludeResponseWrapper {
+public class IncludeNoContentTypeOverrideResponseWrapper extends SlingHttpServletResponseWrapper {
 
     private final RequestData requestData;
 
@@ -47,24 +48,34 @@ public IncludeNoContentTypeOverrideResponseWrapper(@NotNull RequestData requestD
     public void setContentType(String type) {
         String contentTypeString = getContentType();
         if (contentTypeString != null) {
+            if (type == null) {
+                String message = getMessage(contentTypeString, "null");
+                requestData.getRequestProgressTracker().log("ERROR: " + message);
+                throw new ContentTypeChangeException(message);
+            }
             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"
-                );
+                String message = getMessage(contentTypeString, type);
                 requestData.getRequestProgressTracker().log("ERROR: " + message);
                 throw new ContentTypeChangeException(message);
             }
+            getResponse().setContentType(type);
         }
     }
 
+    private String getMessage(String currentContentType, String setContentType) {
+        return 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(),
+                currentContentType,
+                setContentType,
+                Config.PID,
+                "sling.includes.checkcontenttype"
+        );
+    }
+
     private static class ContentTypeChangeException extends SlingException {
         protected ContentTypeChangeException(String text) {
             super(text);

src/test/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapperTest.java

@@ -26,7 +26,8 @@
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -61,13 +62,18 @@ public void testContentTypeOverrideThrows() {
     }
 
     @Test
-    public void testContentTypeOverrideLenient() {
+    public void testContentTypeOverrideLenient() throws Throwable {
         RequestData requestData = mock(RequestData.class);
         when(requestData.getActiveServletName()).thenReturn(ACTIVE_SERVLET_NAME);
         RequestProgressTracker requestProgressTracker = mock(RequestProgressTracker.class);
         when(requestData.getRequestProgressTracker()).thenReturn(requestProgressTracker);
         SlingHttpServletResponse response = mock(SlingHttpServletResponse.class);
         when(response.getContentType()).thenReturn("text/html");
+        doAnswer(invocationOnMock -> {
+            String setContentType = invocationOnMock.getArgument(0);
+            when(response.getContentType()).thenReturn(setContentType);
+            return null;
+        }).when(response).setContentType(anyString());
 
 
         IncludeNoContentTypeOverrideResponseWrapper wrapper = new IncludeNoContentTypeOverrideResponseWrapper(requestData,
@@ -80,8 +86,21 @@ public void testContentTypeOverrideLenient() {
         } catch (Throwable t) {
             throwable = t;
         }
-        assertNull(throwable);
-        assertEquals("text/html", wrapper.getContentType());
+        if (throwable != null) {
+            throw throwable;
+        }
+        assertEquals("text/html;utf-8", wrapper.getContentType());
+
+        try {
+            wrapper.setContentType(null);
+        } catch (Throwable t) {
+            throwable = t;
+        }
+        assertNotNull(throwable);
+        assertEquals("Servlet activeServlet tried to override the 'Content-Type' header from 'text/html;utf-8' to " +
+                "'null', however the org.apache.sling.engine.impl.SlingMainServlet forbids this via the sling" +
+                ".includes.checkcontenttype configuration property.", throwable.getMessage());
+        assertEquals("text/html;utf-8", wrapper.getContentType());
     }
 
 }