You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by cz...@apache.org on 2022/11/26 13:33:57 UTC
[sling-org-apache-sling-engine] branch master updated: SLING-11702 : Prevent wrong handling of error handlers
This is an automated email from the ASF dual-hosted git repository.
cziegeler 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 e43285f SLING-11702 : Prevent wrong handling of error handlers
e43285f is described below
commit e43285f3a7896a62b3ce1bf397b4c5852b66583a
Author: Carsten Ziegeler <cz...@apache.org>
AuthorDate: Sat Nov 26 14:33:50 2022 +0100
SLING-11702 : Prevent wrong handling of error handlers
---
.../sling/engine/impl/DefaultErrorHandler.java | 149 ++++++++++++---------
.../engine/impl/SlingRequestProcessorImpl.java | 72 +++++-----
.../sling/engine/impl/DefaultErrorHandlerTest.java | 62 +++++++++
3 files changed, 185 insertions(+), 98 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 f1b38c1..a973826 100644
--- a/src/main/java/org/apache/sling/engine/impl/DefaultErrorHandler.java
+++ b/src/main/java/org/apache/sling/engine/impl/DefaultErrorHandler.java
@@ -68,6 +68,15 @@ public class DefaultErrorHandler implements ErrorHandler {
// don't include Throwable in the response, gives too much information
final String m = "Error handler failed:" + t.getClass().getName();
log.error(m, t);
+
+ if (response.isCommitted()) {
+ log.error(
+ "handleError: Response already committed; cannot send error "
+ + originalStatus + " : " + originalMessage);
+ return;
+ }
+ // reset the response to clear headers and body
+ response.reset();
sendError(originalStatus, originalMessage, null, request, response);
}
@@ -90,9 +99,17 @@ public class DefaultErrorHandler implements ErrorHandler {
final SlingHttpServletRequest request,
final SlingHttpServletResponse response)
throws IOException {
+ if (response.isCommitted()) {
+ log.error(
+ "handleError: Response already committed; cannot send error "
+ + status + " : " + message);
+ return;
+ }
+ // reset the response to clear headers and body
+ response.reset();
// If we have a delegate let it handle the error
- if(delegate != null) {
+ if (delegate != null) {
try {
delegate.handleError(status, message, request, response);
} catch(Exception e) {
@@ -128,9 +145,18 @@ public class DefaultErrorHandler implements ErrorHandler {
final SlingHttpServletRequest request,
final SlingHttpServletResponse response)
throws IOException {
+ if (response.isCommitted()) {
+ log.error(
+ "handleError: Response already committed; cannot send error "
+ + throwable.getMessage(), throwable);
+ return;
+ }
+ // reset the response to clear headers and body
+ response.reset();
+
final int status = HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
// If we have a delegate let it handle the error
- if(delegate != null) {
+ if (delegate != null) {
try {
delegate.handleError(throwable, request, response);
} catch(Exception e) {
@@ -151,74 +177,63 @@ public class DefaultErrorHandler implements ErrorHandler {
final HttpServletRequest request,
final HttpServletResponse response)
throws IOException {
+ // error situation
+ final String servletName = (String) request.getAttribute(ERROR_SERVLET_NAME);
+ String requestURI = (String) request.getAttribute(ERROR_REQUEST_URI);
+ if (requestURI == null) {
+ requestURI = request.getRequestURI();
+ }
- if (response.isCommitted()) {
- log.error(
- "handleError: Response already committed; cannot send error "
- + status + message, throwable);
- } else {
-
- // error situation
- final String servletName = (String) request.getAttribute(ERROR_SERVLET_NAME);
- String requestURI = (String) request.getAttribute(ERROR_REQUEST_URI);
- if (requestURI == null) {
- requestURI = request.getRequestURI();
- }
-
- // reset anything in the response first
- response.reset();
-
- // set the status, content type and encoding
- response.setStatus(status);
- response.setContentType("text/html; charset=UTF-8");
-
- final PrintWriter pw = response.getWriter();
- pw.println("<html><head><title>");
+ // set the status, content type and encoding
+ response.setStatus(status);
+ response.setContentType("text/html; charset=UTF-8");
+
+ final PrintWriter pw = response.getWriter();
+ pw.println("<html><head><title>");
+ pw.println(ResponseUtil.escapeXml(message));
+ pw.println("</title></head><body><h1>");
+ if (throwable != null) {
+ pw.println(ResponseUtil.escapeXml(throwable.toString()));
+ } else if (message != null) {
pw.println(ResponseUtil.escapeXml(message));
- pw.println("</title></head><body><h1>");
- if (throwable != null) {
- pw.println(ResponseUtil.escapeXml(throwable.toString()));
- } else if (message != null) {
- pw.println(ResponseUtil.escapeXml(message));
- } else {
- pw.println("Internal error (no Exception to report)");
- }
- pw.println("</h1><p>");
- pw.print("RequestURI=");
- pw.println(ResponseUtil.escapeXml(request.getRequestURI()));
- if (servletName != null) {
- pw.println("</p><p>Servlet=");
- pw.println(ResponseUtil.escapeXml(servletName));
- }
- 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 = ((SlingHttpServletRequest) request).getRequestProgressTracker();
- pw.println("<h3>Request Progress:</h3>");
- pw.println("<pre>");
- pw.flush();
- tracker.dump(new PrintWriter(escapingWriter));
- escapingWriter.flush();
- pw.println("</pre>");
- }
+ } else {
+ pw.println("Internal error (no Exception to report)");
+ }
+ pw.println("</h1><p>");
+ pw.print("RequestURI=");
+ pw.println(ResponseUtil.escapeXml(request.getRequestURI()));
+ if (servletName != null) {
+ pw.println("</p><p>Servlet=");
+ pw.println(ResponseUtil.escapeXml(servletName));
+ }
+ 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 = ((SlingHttpServletRequest) 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("<hr /><address>");
+ pw.println(ResponseUtil.escapeXml(serverInfo));
+ pw.println("</address></body></html>");
- // commit the response
- response.flushBuffer();
- // close the response (SLING-2724)
- pw.close();
- }
+ // commit the response
+ response.flushBuffer();
+ // close the response (SLING-2724)
+ pw.close();
}
}
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 6b6aced..564126c 100644
--- a/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java
+++ b/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java
@@ -39,6 +39,7 @@ import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
+import org.apache.sling.api.SlingConstants;
import org.apache.sling.api.SlingException;
import org.apache.sling.api.SlingHttpServletRequest;
import org.apache.sling.api.SlingHttpServletResponse;
@@ -207,6 +208,7 @@ public class SlingRequestProcessorImpl implements SlingRequestProcessor {
final SlingHttpServletRequest request = requestData.getSlingRequest();
final SlingHttpServletResponse response = requestData.getSlingResponse();
+ final boolean isInclude = request.getAttribute(SlingConstants.ATTR_INCLUDE_CONTEXT_PATH) != null;
try {
// initialize the request data - resolve resource and servlet
final Resource resource = requestData.initResource(resourceResolver);
@@ -227,11 +229,16 @@ public class SlingRequestProcessorImpl implements SlingRequestProcessor {
// send this exception as a 404 status
log.debug("service: Resource {} not found", rnfe.getResource());
- handleError(HttpServletResponse.SC_NOT_FOUND, rnfe.getMessage(),
- request, response);
+ if ( isInclude ) {
+ throw rnfe;
+ }
+ handleError(HttpServletResponse.SC_NOT_FOUND, rnfe.getMessage(), request, response);
} catch (final SlingException se) {
+ if ( isInclude ) {
+ throw se;
+ }
// if we have request data and a non-null active servlet name
// we assume, that this is the name of the causing servlet
if (requestData.getActiveServletName() != null) {
@@ -250,6 +257,9 @@ public class SlingRequestProcessorImpl implements SlingRequestProcessor {
} catch (final AccessControlException ace) {
+ if ( isInclude ) {
+ throw ace;
+ }
// SLING-319 if anything goes wrong, send 403/FORBIDDEN
log.debug(
"service: Authenticated user {} does not have enough rights to executed requested action",
@@ -264,6 +274,16 @@ public class SlingRequestProcessorImpl implements SlingRequestProcessor {
} catch (final Throwable t) {
+ if ( isInclude ) {
+ if ( t instanceof RuntimeException ) {
+ throw (RuntimeException)t;
+ }
+ if ( t instanceof Error ) {
+ throw (Error)t;
+ }
+ throw new SlingException(t.getMessage(), t);
+ }
+
// if we have request data and a non-null active servlet name
// we assume, that this is the name of the causing servlet
if (requestData.getActiveServletName() != null) {
@@ -379,45 +399,35 @@ public class SlingRequestProcessorImpl implements SlingRequestProcessor {
// ---------- Error Handling with Filters
- void handleError(final int status, final String message,
+ private void handleError(final FilterChain errorFilterChain,
final SlingHttpServletRequest request,
- SlingHttpServletResponse response) throws IOException {
-
- // wrap the response ensuring getWriter will fall back to wrapping
- // the response output stream if reset does not reset this
- response = new ErrorResponseWrapper(response);
-
- final FilterHandle[] filters = filterManager.getFilters(FilterChainType.ERROR);
- FilterChain processor = new ErrorFilterChainStatus(filters, errorHandler, status, message);
- request.getRequestProgressTracker().log(
- "Applying " + FilterChainType.ERROR + " filters");
+ final SlingHttpServletResponse response)
+ throws IOException {
+ request.getRequestProgressTracker().log("Applying " + FilterChainType.ERROR + " filters");
try {
- processor.doFilter(request, response);
- } catch (ServletException se) {
+ // wrap the response ensuring getWriter will fall back to wrapping
+ // the response output stream if reset does not reset this
+ errorFilterChain.doFilter(request, new ErrorResponseWrapper(response));
+ } catch (final ServletException se) {
throw new SlingServletException(se);
}
}
- // just rethrow the exception as explained in the class comment
- private void handleError(final Throwable throwable,
+ void handleError(final int status, final String message,
final SlingHttpServletRequest request,
- SlingHttpServletResponse response) throws IOException {
-
- // wrap the response ensuring getWriter will fall back to wrapping
- // the response output stream if reset does not reset this
- response = new ErrorResponseWrapper(response);
-
+ final SlingHttpServletResponse response) throws IOException {
final FilterHandle[] filters = filterManager.getFilters(FilterChainType.ERROR);
- FilterChain processor = new ErrorFilterChainThrowable(filters, errorHandler, throwable);
- request.getRequestProgressTracker().log(
- "Applying " + FilterChainType.ERROR + " filters");
+ final FilterChain processor = new ErrorFilterChainStatus(filters, errorHandler, status, message);
+ this.handleError(processor, request, response);
+ }
- try {
- processor.doFilter(request, response);
- } catch (ServletException se) {
- throw new SlingServletException(se);
- }
+ private void handleError(final Throwable throwable,
+ final SlingHttpServletRequest request,
+ final SlingHttpServletResponse response) throws IOException {
+ final FilterHandle[] filters = filterManager.getFilters(FilterChainType.ERROR);
+ final FilterChain processor = new ErrorFilterChainThrowable(filters, errorHandler, throwable);
+ this.handleError(processor, request, response);
}
private static class ErrorResponseWrapper extends
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..be48632
--- /dev/null
+++ b/src/test/java/org/apache/sling/engine/impl/DefaultErrorHandlerTest.java
@@ -0,0 +1,62 @@
+/*
+ * 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 static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+
+import java.io.IOException;
+
+import org.apache.sling.api.SlingHttpServletResponse;
+import org.apache.sling.engine.servlets.ErrorHandler;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+public class DefaultErrorHandlerTest {
+
+ @Test public void testResponseCommitted() throws IOException {
+ final DefaultErrorHandler handler = new DefaultErrorHandler();
+ final ErrorHandler errorHandler = Mockito.mock(ErrorHandler.class);
+ handler.setDelegate(errorHandler);
+ final SlingHttpServletResponse response = Mockito.mock(SlingHttpServletResponse.class);
+ Mockito.when(response.isCommitted()).thenReturn(true);
+
+ handler.handleError(new Exception(), null, response);
+ handler.handleError(500, "message", null, response);
+
+ Mockito.verify(errorHandler, never()).handleError(any(Throwable.class), eq(null), eq(response));
+ Mockito.verify(errorHandler, never()).handleError(anyInt(), anyString(), eq(null), eq(response));
+ }
+
+ @Test public void testResponseNotCommitted() throws IOException {
+ final DefaultErrorHandler handler = new DefaultErrorHandler();
+ final ErrorHandler errorHandler = Mockito.mock(ErrorHandler.class);
+ handler.setDelegate(errorHandler);
+ final SlingHttpServletResponse response = Mockito.mock(SlingHttpServletResponse.class);
+ Mockito.when(response.isCommitted()).thenReturn(false);
+
+ handler.handleError(new Exception(), null, response);
+ Mockito.verify(errorHandler, times(1)).handleError(any(Throwable.class), eq(null), eq(response));
+ handler.handleError(500, "message", null, response);
+ Mockito.verify(errorHandler, times(1)).handleError(anyInt(), anyString(), eq(null), eq(response));
+ }
+}