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 a0b08897dbbbf32760d55fb1b7e8b7a88391dd35 Author: Radu Cotescu <[email protected]> AuthorDate: Tue Dec 13 12:11:16 2022 +0100 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 --- .../java/org/apache/sling/engine/impl/Config.java | 8 +++--- ...ncludeNoContentTypeOverrideResponseWrapper.java | 31 +++++++++++++++------- ...deNoContentTypeOverrideResponseWrapperTest.java | 27 ++++++++++++++++--- 3 files changed, 48 insertions(+), 18 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 b47fdfd..16854a4 100644 --- a/src/main/java/org/apache/sling/engine/impl/Config.java +++ b/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 @@ public @interface Config { 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; } diff --git a/src/main/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapper.java b/src/main/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapper.java index da1c7d0..ed0aef8 100644 --- a/src/main/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapper.java +++ b/src/main/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapper.java @@ -23,10 +23,11 @@ import java.util.Optional; 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 class IncludeNoContentTypeOverrideResponseWrapper extends IncludeResponse 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); diff --git a/src/test/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapperTest.java b/src/test/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapperTest.java index 89367ba..2bd063b 100644 --- a/src/test/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapperTest.java +++ b/src/test/java/org/apache/sling/engine/impl/IncludeNoContentTypeOverrideResponseWrapperTest.java @@ -26,7 +26,8 @@ import org.junit.Test; 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 class IncludeNoContentTypeOverrideResponseWrapperTest { } @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 class IncludeNoContentTypeOverrideResponseWrapperTest { } 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()); } }
