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 2e0ca88 SLING-13368 don't log sensitive information on the
DefaulErrorHandler (#99)
2e0ca88 is described below
commit 2e0ca88c9fb5d6d4e172b11384b49fc749431b74
Author: Jörg Hoh <[email protected]>
AuthorDate: Wed Sep 30 10:29:56 2026 +0200
SLING-13368 don't log sensitive information on the DefaulErrorHandler (#99)
* do not send a stacktrace if no ErrorHandler is registered
* do not print the server info string if no ErrorHandler is registered
---
.../sling/engine/impl/DefaultErrorHandler.java | 49 +++------
.../engine/impl/SlingRequestProcessorImpl.java | 11 --
.../sling/engine/impl/DefaultErrorHandlerTest.java | 118 +++++++++++++++++++++
3 files changed, 133 insertions(+), 45 deletions(-)
diff --git
a/src/main/java/org/apache/sling/engine/impl/DefaultErrorHandler.java
b/src/main/java/org/apache/sling/engine/impl/DefaultErrorHandler.java
index 8320bf2..3cf96c4 100644
--- a/src/main/java/org/apache/sling/engine/impl/DefaultErrorHandler.java
+++ b/src/main/java/org/apache/sling/engine/impl/DefaultErrorHandler.java
@@ -28,7 +28,6 @@ import org.apache.sling.api.SlingHttpServletRequest;
import org.apache.sling.api.SlingHttpServletResponse;
import org.apache.sling.api.SlingJakartaHttpServletRequest;
import org.apache.sling.api.SlingJakartaHttpServletResponse;
-import org.apache.sling.api.request.RequestProgressTracker;
import org.apache.sling.api.request.ResponseUtil;
import org.apache.sling.api.servlets.ErrorHandler;
import org.apache.sling.api.servlets.JakartaErrorHandler;
@@ -49,8 +48,6 @@ public class DefaultErrorHandler implements
JakartaErrorHandler {
/** default log */
private final Logger log = LoggerFactory.getLogger(getClass());
- private volatile String serverInfo = ProductInfoProvider.PRODUCT_NAME;
-
/** Use this if not null, and if that fails output a report about that
failure */
private volatile JakartaErrorHandler delegate;
@@ -59,10 +56,6 @@ public class DefaultErrorHandler implements
JakartaErrorHandler {
private volatile ServiceReference<?> errorHandlerRef;
private volatile ServiceReference<?> jakartaErrorHandlerRef;
- void setServerInfo(final String serverInfo) {
- this.serverInfo = (serverInfo != null) ? serverInfo :
ProductInfoProvider.PRODUCT_NAME;
- }
-
@SuppressWarnings("deprecation")
public synchronized void setDelegate(final ServiceReference<?> ref, final
ErrorHandler eh) {
if (eh != null) {
@@ -146,9 +139,8 @@ public class DefaultErrorHandler implements
JakartaErrorHandler {
* Backend implementation of the HttpServletResponse.sendError methods.
* <p>
* This implementation resets the response before sending back a
- * standardized response which just conveys the status, the message (either
- * provided or a message derived from the status code), and server
- * information.
+ * standardized response which just conveys the status and the message
+ * (either provided or a message derived from the status code).
* <p>
* This method logs error and does not write back and response data if the
* response has already been committed.
@@ -171,6 +163,11 @@ public class DefaultErrorHandler implements
JakartaErrorHandler {
return;
}
+ log.warn(
+ "handleError: No ErrorHandler service registered; every Sling
instance should have one. "
+ + "Falling back to the minimal built-in error response
for status {}",
+ status);
+
if (message == null) {
message = "HTTP ERROR:" + String.valueOf(status);
} else {
@@ -185,8 +182,10 @@ public class DefaultErrorHandler implements
JakartaErrorHandler {
* <p>
* This implementation resets the response before sending back a
* standardized response which just conveys the status as 500/INTERNAL
- * SERVER ERROR, the message from the throwable, the stacktrace, and server
- * information.
+ * SERVER ERROR and the message from the throwable. The exception's
+ * stacktrace and the {@code RequestProgressTracker} dump are not sent to
+ * the client; they are only available in the server-side log (see the
+ * caller of this method).
* <p>
* This method logs error and does not write back and response data if the
* response has already been committed.
@@ -209,6 +208,9 @@ public class DefaultErrorHandler implements
JakartaErrorHandler {
return;
}
+ log.warn("handleError: No ErrorHandler service registered; every Sling
instance should have one. "
+ + "Falling back to the minimal built-in error response.");
+
sendError(status, throwable.getMessage(), throwable, request,
response);
}
@@ -254,28 +256,7 @@ public class DefaultErrorHandler implements
JakartaErrorHandler {
}
pw.println("</p>");
- if (throwable != null) {
- final PrintWriter escapingWriter = new
PrintWriter(ResponseUtil.getXmlEscapingWriter(pw));
- pw.println("<h3>Exception stacktrace:</h3>");
- pw.println("<pre>");
- pw.flush();
- throwable.printStackTrace(escapingWriter);
- escapingWriter.flush();
- pw.println("</pre>");
-
- final RequestProgressTracker tracker =
- ((SlingJakartaHttpServletRequest)
request).getRequestProgressTracker();
- pw.println("<h3>Request Progress:</h3>");
- pw.println("<pre>");
- pw.flush();
- tracker.dump(new PrintWriter(escapingWriter));
- escapingWriter.flush();
- pw.println("</pre>");
- }
-
- pw.println("<hr /><address>");
- pw.println(ResponseUtil.escapeXml(serverInfo));
- pw.println("</address></body></html>");
+ pw.println("</body></html>");
// commit the response
response.flushBuffer();
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 48137e0..1cd2ac5 100644
--- a/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java
+++ b/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java
@@ -33,7 +33,6 @@ import jakarta.servlet.DispatcherType;
import jakarta.servlet.FilterChain;
import jakarta.servlet.RequestDispatcher;
import jakarta.servlet.Servlet;
-import jakarta.servlet.ServletContext;
import jakarta.servlet.ServletException;
import jakarta.servlet.ServletRequest;
import jakarta.servlet.ServletResponse;
@@ -65,7 +64,6 @@ import
org.apache.sling.engine.impl.filter.RequestSlingFilterChain;
import org.apache.sling.engine.impl.filter.ServletFilterManager;
import
org.apache.sling.engine.impl.filter.ServletFilterManager.FilterChainType;
import org.apache.sling.engine.impl.filter.SlingComponentFilterChain;
-import org.apache.sling.engine.impl.helper.SlingServletContext;
import org.apache.sling.engine.impl.parameters.ParameterSupport;
import org.apache.sling.engine.impl.parameters.SlingParameterParseException;
import org.apache.sling.engine.impl.request.ContentData;
@@ -156,15 +154,6 @@ public class SlingRequestProcessorImpl implements
SlingRequestProcessor {
this.disableCheckCompliantGetUserPrincipal =
config.disable_spec_compliant_getuserprincipal();
}
- @Reference(target = SlingServletContext.TARGET, policy =
ReferencePolicy.DYNAMIC, updated = "bindServletContext")
- void bindServletContext(final ServletContext servletContext) {
- this.errorHandler.setServerInfo(servletContext.getServerInfo());
- }
-
- void unbindServletContext(final ServletContext servletContext) {
- // nothing to do here, but DS requires this method
- }
-
@Reference(
name = "JakartaErrorHandler",
cardinality = ReferenceCardinality.OPTIONAL,
diff --git
a/src/test/java/org/apache/sling/engine/impl/DefaultErrorHandlerTest.java
b/src/test/java/org/apache/sling/engine/impl/DefaultErrorHandlerTest.java
new file mode 100644
index 0000000..7ba3b55
--- /dev/null
+++ b/src/test/java/org/apache/sling/engine/impl/DefaultErrorHandlerTest.java
@@ -0,0 +1,118 @@
+/*
+ * 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.io.PrintWriter;
+import java.io.StringWriter;
+
+import org.apache.sling.api.SlingJakartaHttpServletRequest;
+import org.apache.sling.api.SlingJakartaHttpServletResponse;
+import org.apache.sling.api.request.RequestProgressTracker;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * Tests for {@link DefaultErrorHandler}: with no {@code ErrorHandler}/
+ * {@code JakartaErrorHandler} service bound, the built-in error response must
+ * not leak the exception's stacktrace, the {@link RequestProgressTracker}
+ * dump, or the server/JVM/OS fingerprint footer to the client.
+ */
+public class DefaultErrorHandlerTest {
+
+ private DefaultErrorHandler handler;
+ private SlingJakartaHttpServletRequest request;
+ private SlingJakartaHttpServletResponse response;
+ private StringWriter responseBody;
+ private RequestProgressTracker tracker;
+
+ @Before
+ public void setup() throws Exception {
+ handler = new DefaultErrorHandler();
+
+ tracker = mock(RequestProgressTracker.class);
+
+ request = mock(SlingJakartaHttpServletRequest.class);
+ when(request.getRequestURI()).thenReturn("/content/test");
+ when(request.getRequestProgressTracker()).thenReturn(tracker);
+
+ response = mock(SlingJakartaHttpServletResponse.class);
+ responseBody = new StringWriter();
+ when(response.getWriter()).thenReturn(new PrintWriter(responseBody));
+ }
+
+ @Test
+ public void
testHandleThrowableWithoutDelegateDoesNotLeakStacktraceOrTracker() throws
Exception {
+ final Exception cause = new IllegalStateException("some internal
detail: /etc/secret-path");
+
+ handler.handleError(cause, request, response);
+
+ verify(response).setStatus(500);
+ responseBody.flush();
+ final String body = responseBody.toString();
+
+ // the exception's stacktrace must never be written to the response
+ assertFalse(
+ "response body must not contain a stacktrace frame",
+ body.contains("at
org.apache.sling.engine.impl.DefaultErrorHandlerTest"));
+ assertFalse("response body must not mention the stacktrace section",
body.contains("Exception stacktrace"));
+
+ // the RequestProgressTracker must never be dumped into the response
+ assertFalse("response body must not contain the tracker dump section",
body.contains("Request Progress"));
+ verify(tracker,
never()).dump(org.mockito.ArgumentMatchers.any(PrintWriter.class));
+
+ // the server/JVM/OS fingerprint footer must never be written either
+ assertFalse("response body must not contain a server/address footer",
body.contains("<address>"));
+
+ // a minimal, generic error page is still rendered
+ assertTrue(body.contains("RequestURI="));
+ }
+
+ @Test
+ public void testHandleStatusWithoutDelegateStillRendersMessage() throws
Exception {
+ handler.handleError(404, "not found", request, response);
+
+ verify(response).setStatus(404);
+ responseBody.flush();
+ final String body = responseBody.toString();
+ assertTrue(body.contains("not found"));
+ assertFalse("response body must not contain a server/address footer",
body.contains("<address>"));
+ }
+
+ @Test
+ public void testHandleThrowableWithDelegateDoesNotUseFallback() throws
Exception {
+ final org.apache.sling.api.servlets.JakartaErrorHandler delegate =
+ mock(org.apache.sling.api.servlets.JakartaErrorHandler.class);
+ handler.setDelegate(null, delegate);
+
+ final Exception cause = new IllegalStateException("boom");
+ handler.handleError(cause, request, response);
+
+ verify(delegate).handleError(cause, request, response);
+ // the built-in fallback must not have written anything to the response
+ responseBody.flush();
+ assertTrue(responseBody.toString().isEmpty());
+ }
+}
