This is an automated email from the ASF dual-hosted git repository.
joerghoh pushed a commit to branch master
in repository
https://gitbox.apache.org/repos/asf/sling-org-apache-sling-engine.git
The following commit(s) were added to refs/heads/master by this push:
new 3cf85e6 SLING-12661 stick with the first violation for recursive
calls (#56)
3cf85e6 is described below
commit 3cf85e6f0a00d4a90520e414dad3f078bffcf7b6
Author: Remo Liechti <[email protected]>
AuthorDate: Thu Mar 13 15:15:54 2025 +0100
SLING-12661 stick with the first violation for recursive calls (#56)
* SLING-12661 use last 500 messages only for potential origins matching
* Apply suggestions from code review
* SLING-12661 rework duplicated test code
* SLING-12661 move test method to previous location
* SLING-12661 rename test method
* SLING-12661 stop analyzing for succeeding violations once the first
violation is found, especially useful for recursive calls
* SLING-12661 add stacktrace whenviolation is detected
* SLING-12661 remove xss from naming
* SLING-12661 review comments
* SLING-12661 remove duplicated infromation string
---------
Co-authored-by: Jörg Hoh <[email protected]>
---
.../sling/engine/impl/ContentTypeHeaderState.java | 29 +++++++++++++++++
.../engine/impl/SlingHttpServletResponseImpl.java | 25 ++++++++++++++-
.../engine/impl/SlingRequestProcessorImpl.java | 36 ++++++++++++++++++----
.../impl/SlingHttpServletResponseImplTest.java | 15 +++++++++
4 files changed, 98 insertions(+), 7 deletions(-)
diff --git
a/src/main/java/org/apache/sling/engine/impl/ContentTypeHeaderState.java
b/src/main/java/org/apache/sling/engine/impl/ContentTypeHeaderState.java
new file mode 100644
index 0000000..b7c305f
--- /dev/null
+++ b/src/main/java/org/apache/sling/engine/impl/ContentTypeHeaderState.java
@@ -0,0 +1,29 @@
+/*
+ * 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;
+
+/**
+ * This enumeration defines states hold in a ThreadLocal to indicate a
+ * previously detected violation to change the content type header.
+ */
+public enum ContentTypeHeaderState {
+ UNSET,
+ VIOLATED,
+ NOT_VIOLATED
+}
diff --git
a/src/main/java/org/apache/sling/engine/impl/SlingHttpServletResponseImpl.java
b/src/main/java/org/apache/sling/engine/impl/SlingHttpServletResponseImpl.java
index ad57faf..8fb5b9b 100644
---
a/src/main/java/org/apache/sling/engine/impl/SlingHttpServletResponseImpl.java
+++
b/src/main/java/org/apache/sling/engine/impl/SlingHttpServletResponseImpl.java
@@ -48,6 +48,8 @@ import org.slf4j.LoggerFactory;
public class SlingHttpServletResponseImpl extends HttpServletResponseWrapper
implements SlingHttpServletResponse {
+ private static final String CALL_STACK_MESSAGE = "Call stack causing the
content type override violation: ";
+
private static final Logger LOG =
LoggerFactory.getLogger(SlingHttpServletResponseImpl.class);
// this regex matches TIMER_START{ followed by any characters except },
and then
@@ -312,6 +314,15 @@ public class SlingHttpServletResponseImpl extends
HttpServletResponseWrapper imp
}
}
+ private String getCurrentStackTrace() {
+ StackTraceElement[] stackTraceElements =
Thread.currentThread().getStackTrace();
+ StringBuilder stackTraceBuilder = new StringBuilder();
+ for (StackTraceElement element : stackTraceElements) {
+
stackTraceBuilder.append(element.toString()).append(System.lineSeparator());
+ }
+ return stackTraceBuilder.toString();
+ }
+
@Override
public void setContentType(final String type) {
if (!isInclude()) {
@@ -321,14 +332,17 @@ public class SlingHttpServletResponseImpl extends
HttpServletResponseWrapper imp
if (message.isPresent()) {
if (isCheckContentTypeOnInclude()) {
requestData.getRequestProgressTracker().log("ERROR: " +
message.get());
+ LOG.error(CALL_STACK_MESSAGE + getCurrentStackTrace());
throw new ContentTypeChangeException(message.get());
}
if (isProtectHeadersOnInclude()) {
LOG.error(message.get());
+ LOG.error(CALL_STACK_MESSAGE + getCurrentStackTrace());
requestData.getRequestProgressTracker().log("ERROR: " +
message.get());
return;
}
LOG.warn(message.get());
+ LOG.warn(CALL_STACK_MESSAGE + getCurrentStackTrace());
requestData.getRequestProgressTracker().log("WARN: " +
message.get());
super.setContentType(type);
} else {
@@ -345,8 +359,15 @@ public class SlingHttpServletResponseImpl extends
HttpServletResponseWrapper imp
* @return an optional message to log
*/
private Optional<String> checkContentTypeOverride(@Nullable String
contentType) {
+ if (requestData.getSlingRequestProcessor().getContentTypeHeaderState()
== ContentTypeHeaderState.VIOLATED) {
+ // return immediatly as the content type header has already been
violated
+ // prevoiously, no more checks needed
+ return Optional.empty();
+ }
+
String currentContentType = getContentType();
if (contentType == null) {
+
requestData.getSlingRequestProcessor().setContentTypeHeaderState(ContentTypeHeaderState.VIOLATED);
return Optional.of(getMessage(currentContentType, null));
} else {
Optional<String> currentMime = currentContentType == null
@@ -356,6 +377,7 @@ public class SlingHttpServletResponseImpl extends
HttpServletResponseWrapper imp
if (currentMime.isPresent()
&& setMime.isPresent()
&& !currentMime.get().equals(setMime.get())) {
+
requestData.getSlingRequestProcessor().setContentTypeHeaderState(ContentTypeHeaderState.VIOLATED);
return Optional.of(getMessage(currentContentType,
contentType));
}
}
@@ -363,7 +385,8 @@ public class SlingHttpServletResponseImpl extends
HttpServletResponseWrapper imp
}
private List<String> getLastMessagesOfProgressTracker() {
- // Collect the last MAX_NR_OF_MESSAGES messages from the
RequestProgressTracker to prevent excessive memory
+ // Collect the last MAX_NR_OF_MESSAGES messages from the
RequestProgressTracker
+ // to prevent excessive memory
// consumption errors when close to infinite recursive calls are made
int nrOfOriginalMessages = 0;
boolean gotCut = false;
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 144c529..82f2734 100644
--- a/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java
+++ b/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java
@@ -117,6 +117,9 @@ public class SlingRequestProcessorImpl implements
SlingRequestProcessor {
private volatile boolean checkContentTypeOnInclude;
private volatile boolean disableCheckCompliantGetUserPrincipal;
+ private static final ThreadLocal<ContentTypeHeaderState>
contentTypeHeaderState =
+ ThreadLocal.withInitial(() -> ContentTypeHeaderState.UNSET);
+
@Activate
public void activate(final Config config) {
this.modified(config);
@@ -206,7 +209,8 @@ public class SlingRequestProcessorImpl implements
SlingRequestProcessor {
if (resourceResolver == null || sr == null) {
// Dependencies are missing
// In this case we must not use the Sling error handling
infrastructure but
- // just return a 503 status response handled by the servlet
container environment
+ // just return a 503 status response handled by the servlet
container
+ // environment
final int status = HttpServletResponse.SC_SERVICE_UNAVAILABLE;
String errorMessage = "Required service missing (";
@@ -236,6 +240,13 @@ public class SlingRequestProcessorImpl implements
SlingRequestProcessor {
final SlingHttpServletResponse response =
requestData.getSlingResponse();
try {
+ if (getContentTypeHeaderState() != ContentTypeHeaderState.UNSET) {
+ log.error(
+ "Content Type Header state has not been cleared
properly, is set to {}",
+ getContentTypeHeaderState());
+ }
+ setContentTypeHeaderState(ContentTypeHeaderState.NOT_VIOLATED);
+
// initialize the request data - resolve resource and servlet
final Resource resource =
requestData.initResource(resourceResolver);
requestData.initServlet(resource, sr);
@@ -307,6 +318,8 @@ public class SlingRequestProcessorImpl implements
SlingRequestProcessor {
if (localBean != null) {
localBean.addRequestData(requestData);
}
+
+ setContentTypeHeaderState(ContentTypeHeaderState.UNSET);
}
}
@@ -328,7 +341,9 @@ public class SlingRequestProcessorImpl implements
SlingRequestProcessor {
// ---------- SlingRequestProcessor interface
/**
- * @see
org.apache.sling.engine.SlingRequestProcessor#processRequest(javax.servlet.http.HttpServletRequest,
javax.servlet.http.HttpServletResponse,
org.apache.sling.api.resource.ResourceResolver)
+ * @see
org.apache.sling.engine.SlingRequestProcessor#processRequest(javax.servlet.http.HttpServletRequest,
+ * javax.servlet.http.HttpServletResponse,
+ * org.apache.sling.api.resource.ResourceResolver)
*/
@Override
public void processRequest(
@@ -379,10 +394,11 @@ public class SlingRequestProcessorImpl implements
SlingRequestProcessor {
/**
* Dispatches the request on behalf of the
* {@link org.apache.sling.engine.impl.request.SlingRequestDispatcher}.
- * @param request The request
- * @param response The response
- * @param resource The resource
- * @param resolvedURL Request path info
+ *
+ * @param request The request
+ * @param response The response
+ * @param resource The resource
+ * @param resolvedURL Request path info
* @param DispatchingInfo dispatching info
*/
public void dispatchRequest(
@@ -523,4 +539,12 @@ public class SlingRequestProcessorImpl implements
SlingRequestProcessor {
log.debug("getMimeType: MimeTypeService not available, cannot resolve
mime type for {}", name);
return null;
}
+
+ public ContentTypeHeaderState getContentTypeHeaderState() {
+ return contentTypeHeaderState.get();
+ }
+
+ public void setContentTypeHeaderState(ContentTypeHeaderState newState) {
+ contentTypeHeaderState.set(newState);
+ }
}
diff --git
a/src/test/java/org/apache/sling/engine/impl/SlingHttpServletResponseImplTest.java
b/src/test/java/org/apache/sling/engine/impl/SlingHttpServletResponseImplTest.java
index 98eb7b8..4bf21ff 100644
---
a/src/test/java/org/apache/sling/engine/impl/SlingHttpServletResponseImplTest.java
+++
b/src/test/java/org/apache/sling/engine/impl/SlingHttpServletResponseImplTest.java
@@ -39,6 +39,7 @@ import org.mockito.Mockito;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.atMostOnce;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
@@ -117,6 +118,9 @@ public class SlingHttpServletResponseImplTest {
when(requestData.getRequestProgressTracker()).thenReturn(requestProgressTracker);
when(requestData.getActiveServletName()).thenReturn(ACTIVE_SERVLET_NAME);
+ final SlingRequestProcessorImpl requestProcessor =
mock(SlingRequestProcessorImpl.class);
+
when(requestData.getSlingRequestProcessor()).thenReturn(requestProcessor);
+
ArrayList<String> logMessagesList = new
ArrayList<>(Arrays.asList(logMessages));
when(requestProgressTracker.getMessages()).thenAnswer(invocation ->
logMessagesList.iterator());
info.setProtectHeadersOnInclude(true);
@@ -135,6 +139,8 @@ public class SlingHttpServletResponseImplTest {
Mockito.verify(orig, never()).setLocale(null);
Mockito.verify(orig, never()).setBufferSize(4500);
+ Mockito.verify(requestProcessor,
atMostOnce()).setContentTypeHeaderState(Mockito.any());
+
ArgumentCaptor<String> logCaptor =
ArgumentCaptor.forClass(String.class);
verify(requestProgressTracker, times(1)).log(logCaptor.capture());
return logCaptor.getValue();
@@ -214,6 +220,9 @@ public class SlingHttpServletResponseImplTest {
final HttpServletResponse include = new
SlingHttpServletResponseImpl(requestData, orig);
when(requestData.getActiveServletName()).thenReturn(ACTIVE_SERVLET_NAME);
+ final SlingRequestProcessorImpl requestProcessor =
mock(SlingRequestProcessorImpl.class);
+
when(requestData.getSlingRequestProcessor()).thenReturn(requestProcessor);
+
Throwable throwable = null;
try {
include.setContentType("application/json");
@@ -257,6 +266,9 @@ public class SlingHttpServletResponseImplTest {
ArrayList<String> logMessagesList = new
ArrayList<>(Arrays.asList(logMessages));
when(requestProgressTracker.getMessages()).thenAnswer(invocation ->
logMessagesList.iterator());
+ final SlingRequestProcessorImpl requestProcessor =
mock(SlingRequestProcessorImpl.class);
+
when(requestData.getSlingRequestProcessor()).thenReturn(requestProcessor);
+
final HttpServletResponse include = new
SlingHttpServletResponseImpl(requestData, orig);
when(requestData.getActiveServletName()).thenReturn(ACTIVE_SERVLET_NAME);
include.setContentType("application/json");
@@ -288,6 +300,9 @@ public class SlingHttpServletResponseImplTest {
when(orig.getContentType()).thenReturn("application/json");
when(requestData.getRequestProgressTracker()).thenReturn(requestProgressTracker);
+ final SlingRequestProcessorImpl requestProcessor =
mock(SlingRequestProcessorImpl.class);
+
when(requestData.getSlingRequestProcessor()).thenReturn(requestProcessor);
+
final HttpServletResponse include = new
SlingHttpServletResponseImpl(requestData, orig);
when(requestData.getActiveServletName()).thenReturn(ACTIVE_SERVLET_NAME);
include.setContentType("application/json");
