Skip to content

Commit 6f42484

Browse files
RemoLiechtiRemoLiechti
committed
SLING-12844 check content type violations for committed responses if they where not commited due to sendError or sendRedirect
1 parent c02bb4d commit 6f42484

File tree

2 files changed

+108
-50
lines changed

2 files changed

+108
-50
lines changed

‎src/main/java/org/apache/sling/engine/impl/SlingJakartaHttpServletResponseImpl.java

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,15 @@
4848
public class SlingJakartaHttpServletResponseImpl extends HttpServletResponseWrapper
4949
implements SlingJakartaHttpServletResponse {
5050

51+
/**
52+
* The reason why the response was committed. This is used to determine whether
53+
* the change to the content type header can be ignored or not.
54+
*/
55+
public static enum CommitReason {
56+
SEND_ERROR,
57+
SEND_REDIRECT
58+
}
59+
5160
private static final String CALL_STACK_MESSAGE = "Call stack causing the content type override violation: ";
5261

5362
private static final Logger LOG = LoggerFactory.getLogger(SlingJakartaHttpServletResponseImpl.class);
@@ -77,14 +86,16 @@ public static class WriterAlreadyClosedException extends IllegalStateException {
7786

7887
private final boolean firstSlingResponse;
7988

89+
private CommitReason committedReason;
90+
8091
public SlingJakartaHttpServletResponseImpl(RequestData requestData, HttpServletResponse response) {
8192
super(response);
8293
this.requestData = requestData;
8394
this.firstSlingResponse = !(response instanceof SlingJakartaHttpServletResponse);
8495

8596
if (firstSlingResponse) {
86-
for (final StaticResponseHeader mapping :
87-
requestData.getSlingRequestProcessor().getAdditionalResponseHeaders()) {
97+
for (final StaticResponseHeader mapping : requestData.getSlingRequestProcessor()
98+
.getAdditionalResponseHeaders()) {
8899
response.addHeader(mapping.getResponseHeaderName(), mapping.getResponseHeaderValue());
89100
}
90101
}
@@ -269,6 +280,7 @@ public void addIntHeader(final String name, final int value) {
269280
@Override
270281
public void sendRedirect(final String location) throws IOException {
271282
if (!this.isProtectHeadersOnInclude()) {
283+
this.committedReason = CommitReason.SEND_REDIRECT;
272284
super.sendRedirect(location);
273285
}
274286
}
@@ -305,7 +317,10 @@ private String getCurrentStackTrace() {
305317

306318
@Override
307319
public void setContentType(final String type) {
308-
if (this.isCommitted() || !isInclude()) {
320+
boolean isCommitedDueToSendErrorOrRedirect = this.isCommitted()
321+
&& (CommitReason.SEND_ERROR.equals(this.committedReason)
322+
|| CommitReason.SEND_REDIRECT.equals(this.committedReason));
323+
if (isCommitedDueToSendErrorOrRedirect || !isInclude()) {
309324
super.setContentType(type);
310325
} else {
311326
Optional<String> message = checkContentTypeOverride(type);
@@ -369,8 +384,7 @@ private List<String> getLastMessagesOfProgressTracker() {
369384
// consumption errors when close to infinite recursive calls are made
370385
int nrOfOriginalMessages = 0;
371386
boolean gotCut = false;
372-
Iterator<String> messagesIterator =
373-
requestData.getRequestProgressTracker().getMessages();
387+
Iterator<String> messagesIterator = requestData.getRequestProgressTracker().getMessages();
374388
LinkedList<String> lastMessages = new LinkedList<>();
375389
while (messagesIterator.hasNext()) {
376390
nrOfOriginalMessages++;
@@ -449,8 +463,8 @@ private String findUnmatchedTimerStarts() {
449463
private String getMessage(@Nullable String currentContentType, @Nullable String setContentType) {
450464
String unmatchedStartTimers = findUnmatchedTimerStarts();
451465

452-
String allMessages =
453-
getLastMessagesOfProgressTracker().stream().collect(Collectors.joining(System.lineSeparator()));
466+
String allMessages = getLastMessagesOfProgressTracker().stream()
467+
.collect(Collectors.joining(System.lineSeparator()));
454468

455469
if (!isCheckContentTypeOnInclude()) {
456470
return String.format(
@@ -497,6 +511,8 @@ public void sendError(int status, String message) throws IOException {
497511
if (!this.isProtectHeadersOnInclude()) {
498512
checkCommitted();
499513

514+
this.committedReason = CommitReason.SEND_ERROR;
515+
500516
final SlingRequestProcessorImpl eh = getRequestData().getSlingRequestProcessor();
501517
eh.handleError(status, message, requestData.getSlingRequest(), this);
502518
}
@@ -771,8 +787,7 @@ private String map(String url) {
771787
}
772788

773789
private String removeContextPath(final String path) {
774-
final String contextPath =
775-
this.getRequestData().getSlingRequest().getContextPath().concat("/");
790+
final String contextPath = this.getRequestData().getSlingRequest().getContextPath().concat("/");
776791
if (contextPath.length() > 1 && path.startsWith(contextPath)) {
777792
return path.substring(contextPath.length() - 1);
778793
}

‎src/test/java/org/apache/sling/engine/impl/SlingHttpServletResponseImplTest.java

Lines changed: 84 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -49,61 +49,104 @@ public class SlingHttpServletResponseImplTest {
4949

5050
private static final String ACTIVE_SERVLET_NAME = "activeServlet";
5151
String[] logMessages = {
52-
"0 TIMER_START{Request Processing}",
53-
"6 COMMENT timer_end format is {<elapsed microseconds>,<timer name>} <optional message>",
54-
"17 LOG Method=GET, PathInfo=null",
55-
"20 TIMER_START{handleSecurity}",
56-
"2104 TIMER_END{2081,handleSecurity} authenticator org.apache.sling.auth.core.impl.SlingAuthenticator@6367091e returns true",
57-
"2478 TIMER_START{ResourceResolution}",
58-
"2668 TIMER_END{189,ResourceResolution} URI=/content/slingshot.html resolves to Resource=JcrNodeResource, type=slingshot/Home, superType=null, path=/content/slingshot",
59-
"2678 LOG Resource Path Info: SlingRequestPathInfo: path='/content/slingshot', selectorString='null', extension='html', suffix='null'",
60-
"2678 TIMER_START{ServletResolution}",
61-
"2683 TIMER_START{resolveServlet(/content/slingshot)}",
62-
"3724 TIMER_END{1040,resolveServlet(/content/slingshot)} Using servlet /libs/slingshot/Home/html.jsp",
63-
"3727 TIMER_END{1047,ServletResolution} URI=/content/slingshot.html handled by Servlet=/libs/slingshot/Home/html.jsp",
64-
"3736 LOG Applying REQUESTfilters",
65-
"3751 LOG Calling filter: com.composum.sling.nodes.mount.remote.RemoteRequestFilter",
66-
"4722 TIMER_START{/libs/slingshot/Component/head.html.jsp#1}",
67-
"3757 LOG Calling filter: org.apache.sling.i18n.impl.I18NFilter",
68-
"4859 TIMER_END{135,/libs/slingshot/Component/head.html.jsp#1}",
69-
"3765 LOG Calling filter: org.apache.sling.engine.impl.debug.RequestProgressTrackerLogFilter",
70-
"2678 TIMER_START{ServletResolution}",
71-
"2683 TIMER_START{resolveServlet(/content/slingshot)}",
72-
"2678 TIMER_START{ServletResolution}",
73-
"2683 TIMER_START{resolveServlet(/content/slingshot)}",
74-
"3724 TIMER_END{1040,resolveServlet(/content/slingshot)} Using servlet /libs/slingshot/Home/html.jsp",
75-
"3727 TIMER_END{1047,ServletResolution} URI=/content/slingshot.html handled by Servlet=/libs/slingshot/Home/html.jsp",
76-
"3724 TIMER_END{1040,resolveServlet(/content/slingshot)} Using servlet /libs/slingshot/Home/html.jsp",
77-
"3727 TIMER_END{1047,ServletResolution} URI=/content/slingshot.html handled by Servlet=/libs/slingshot/Home/html.jsp",
78-
"3774 LOG Applying Componentfilters",
79-
"3797 TIMER_START{/libs/slingshot/Home/html.jsp#0}",
80-
"3946 LOG Adding bindings took 18 microseconds",
81-
"4405 LOG Including resource JcrNodeResource, type=slingshot/Home, superType=null, path=/content/slingshot (SlingRequestPathInfo: path='/content/slingshot', selectorString='head', extension='html', suffix='null')",
82-
"4414 TIMER_START{resolveServlet(/content/slingshot)}",
83-
"4670 TIMER_END{253,resolveServlet(/content/slingshot)} Using servlet /libs/slingshot/Component/head.html.jsp",
84-
"4673 LOG Applying Includefilters",
85-
"4722 TIMER_START{/libs/slingshot/Component/head.html.jsp#1}",
86-
"4749 LOG Adding bindings took 4 microseconds"
52+
"0 TIMER_START{Request Processing}",
53+
"6 COMMENT timer_end format is {<elapsed microseconds>,<timer name>} <optional message>",
54+
"17 LOG Method=GET, PathInfo=null",
55+
"20 TIMER_START{handleSecurity}",
56+
"2104 TIMER_END{2081,handleSecurity} authenticator org.apache.sling.auth.core.impl.SlingAuthenticator@6367091e returns true",
57+
"2478 TIMER_START{ResourceResolution}",
58+
"2668 TIMER_END{189,ResourceResolution} URI=/content/slingshot.html resolves to Resource=JcrNodeResource, type=slingshot/Home, superType=null, path=/content/slingshot",
59+
"2678 LOG Resource Path Info: SlingRequestPathInfo: path='/content/slingshot', selectorString='null', extension='html', suffix='null'",
60+
"2678 TIMER_START{ServletResolution}",
61+
"2683 TIMER_START{resolveServlet(/content/slingshot)}",
62+
"3724 TIMER_END{1040,resolveServlet(/content/slingshot)} Using servlet /libs/slingshot/Home/html.jsp",
63+
"3727 TIMER_END{1047,ServletResolution} URI=/content/slingshot.html handled by Servlet=/libs/slingshot/Home/html.jsp",
64+
"3736 LOG Applying REQUESTfilters",
65+
"3751 LOG Calling filter: com.composum.sling.nodes.mount.remote.RemoteRequestFilter",
66+
"4722 TIMER_START{/libs/slingshot/Component/head.html.jsp#1}",
67+
"3757 LOG Calling filter: org.apache.sling.i18n.impl.I18NFilter",
68+
"4859 TIMER_END{135,/libs/slingshot/Component/head.html.jsp#1}",
69+
"3765 LOG Calling filter: org.apache.sling.engine.impl.debug.RequestProgressTrackerLogFilter",
70+
"2678 TIMER_START{ServletResolution}",
71+
"2683 TIMER_START{resolveServlet(/content/slingshot)}",
72+
"2678 TIMER_START{ServletResolution}",
73+
"2683 TIMER_START{resolveServlet(/content/slingshot)}",
74+
"3724 TIMER_END{1040,resolveServlet(/content/slingshot)} Using servlet /libs/slingshot/Home/html.jsp",
75+
"3727 TIMER_END{1047,ServletResolution} URI=/content/slingshot.html handled by Servlet=/libs/slingshot/Home/html.jsp",
76+
"3724 TIMER_END{1040,resolveServlet(/content/slingshot)} Using servlet /libs/slingshot/Home/html.jsp",
77+
"3727 TIMER_END{1047,ServletResolution} URI=/content/slingshot.html handled by Servlet=/libs/slingshot/Home/html.jsp",
78+
"3774 LOG Applying Componentfilters",
79+
"3797 TIMER_START{/libs/slingshot/Home/html.jsp#0}",
80+
"3946 LOG Adding bindings took 18 microseconds",
81+
"4405 LOG Including resource JcrNodeResource, type=slingshot/Home, superType=null, path=/content/slingshot (SlingRequestPathInfo: path='/content/slingshot', selectorString='head', extension='html', suffix='null')",
82+
"4414 TIMER_START{resolveServlet(/content/slingshot)}",
83+
"4670 TIMER_END{253,resolveServlet(/content/slingshot)} Using servlet /libs/slingshot/Component/head.html.jsp",
84+
"4673 LOG Applying Includefilters",
85+
"4722 TIMER_START{/libs/slingshot/Component/head.html.jsp#1}",
86+
"4749 LOG Adding bindings took 4 microseconds"
8787
};
8888

8989
@Test
90-
public void testNoViolationChecksOnCommitedResponse() {
90+
public void testNoViolationChecksOnCommittedResponseWhenSendRedirect() throws IOException {
9191
final SlingJakartaHttpServletResponse orig = Mockito.mock(SlingJakartaHttpServletResponse.class);
9292
Mockito.when(orig.isCommitted()).thenReturn(true);
9393

9494
final RequestData requestData = mock(RequestData.class);
9595
final DispatchingInfo info = new DispatchingInfo(DispatcherType.INCLUDE);
9696
when(requestData.getDispatchingInfo()).thenReturn(info);
97-
info.setProtectHeadersOnInclude(true);
9897

9998
final SlingJakartaHttpServletResponseImpl include = new SlingJakartaHttpServletResponseImpl(requestData, orig);
10099
SlingJakartaHttpServletResponseImpl spyInclude = Mockito.spy(include);
101100

101+
spyInclude.sendRedirect("somewhere");
102+
103+
spyInclude.setContentType("someOtherType");
104+
Mockito.verify(orig, times(1)).setContentType(Mockito.any());
105+
Mockito.verify(spyInclude, never()).checkContentTypeOverride(Mockito.any());
106+
}
107+
108+
@Test
109+
public void testNoViolationChecksOnCommittedResponseWhenSendError() throws IOException {
110+
final SlingJakartaHttpServletResponse orig = Mockito.mock(SlingJakartaHttpServletResponse.class);
111+
112+
final RequestData requestData = mock(RequestData.class);
113+
final DispatchingInfo info = new DispatchingInfo(DispatcherType.INCLUDE);
114+
when(requestData.getDispatchingInfo()).thenReturn(info);
115+
when(requestData.getSlingRequestProcessor()).thenReturn(mock(SlingRequestProcessorImpl.class));
116+
117+
final SlingJakartaHttpServletResponseImpl include = new SlingJakartaHttpServletResponseImpl(requestData, orig);
118+
SlingJakartaHttpServletResponseImpl spyInclude = Mockito.spy(include);
119+
120+
spyInclude.sendError(501);
121+
// send error will eventually commit the response, let's mock this
122+
Mockito.when(orig.isCommitted()).thenReturn(true);
123+
102124
spyInclude.setContentType("someOtherType");
103125
Mockito.verify(orig, times(1)).setContentType(Mockito.any());
104126
Mockito.verify(spyInclude, never()).checkContentTypeOverride(Mockito.any());
105127
}
106128

129+
@Test
130+
public void testViolationChecksOnCommittedResponses() throws IOException {
131+
final SlingJakartaHttpServletResponse orig = Mockito.mock(SlingJakartaHttpServletResponse.class);
132+
Mockito.when(orig.isCommitted()).thenReturn(true);
133+
134+
final RequestData requestData = mock(RequestData.class);
135+
final DispatchingInfo info = new DispatchingInfo(DispatcherType.INCLUDE);
136+
when(requestData.getDispatchingInfo()).thenReturn(info);
137+
when(requestData.getSlingRequestProcessor()).thenReturn(mock(SlingRequestProcessorImpl.class));
138+
final RequestProgressTracker rpt = mock(RequestProgressTracker.class);
139+
when(rpt.getMessages()).thenReturn(new ArrayList<String>().iterator());
140+
when(requestData.getRequestProgressTracker()).thenReturn(rpt);
141+
142+
final SlingJakartaHttpServletResponseImpl include = new SlingJakartaHttpServletResponseImpl(requestData, orig);
143+
SlingJakartaHttpServletResponseImpl spyInclude = Mockito.spy(include);
144+
145+
spyInclude.setContentType("someOtherType");
146+
Mockito.verify(orig, times(1)).setContentType(Mockito.any());
147+
Mockito.verify(spyInclude, Mockito.times(1)).checkContentTypeOverride(Mockito.any());
148+
}
149+
107150
@Test
108151
public void testReset() {
109152
final SlingJakartaHttpServletResponse originalResponse = mock(SlingJakartaHttpServletResponse.class);
@@ -112,8 +155,8 @@ public void testReset() {
112155
when(requestData.getDispatchingInfo()).thenReturn(dispatchingInfo);
113156
dispatchingInfo.setProtectHeadersOnInclude(true);
114157

115-
final HttpServletResponse includeResponse =
116-
new SlingJakartaHttpServletResponseImpl(requestData, originalResponse);
158+
final HttpServletResponse includeResponse = new SlingJakartaHttpServletResponseImpl(requestData,
159+
originalResponse);
117160

118161
when(originalResponse.isCommitted()).thenReturn(false);
119162
includeResponse.reset();
@@ -134,8 +177,8 @@ public void testResetOnError() {
134177

135178
// Simulate an error dispatching scenario on a uncommitted response
136179
DispatchingInfo dispatchingInfo = new DispatchingInfo(DispatcherType.ERROR);
137-
final HttpServletResponse includeResponse =
138-
new SlingJakartaHttpServletResponseImpl(requestData, originalResponse);
180+
final HttpServletResponse includeResponse = new SlingJakartaHttpServletResponseImpl(requestData,
181+
originalResponse);
139182
dispatchingInfo.setProtectHeadersOnInclude(true);
140183
when(requestData.getDispatchingInfo()).thenReturn(dispatchingInfo);
141184
when(originalResponse.isCommitted()).thenReturn(false);

0 commit comments

Comments
 (0)