Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #12505 - Server to Servlet Error Handling #12586

Open
wants to merge 13 commits into
base: jetty-12.1.x
Choose a base branch
from
Open

Issue #12505 - Server to Servlet Error Handling #12586

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
37ab820
Issue #12505 server to servlet api error handling
janbartel Nov 21, 2024
564e3ee
WIP
janbartel Nov 22, 2024
c9ee387
WIP
janbartel Nov 22, 2024
21f4f68
WIP
janbartel Nov 27, 2024
84892b7
Servlet context errors handled by ServletHandler
gregw Nov 27, 2024
119a930
Merge remote-tracking branch 'origin/jetty-12.1.x' into jetty-12.1.x-…
gregw Nov 27, 2024
bcf274d
Servlet context errors handled by ServletHandler
gregw Nov 27, 2024
135eb61
Better signalling of context error
gregw Nov 27, 2024
84d39e7
checkstyle
gregw Nov 27, 2024
585aeed
fixed ee11
gregw Nov 27, 2024
db3f051
fixed ee11
gregw Nov 27, 2024
111c3fb
updates from review
gregw Nov 28, 2024
5257bba
updates from review
gregw Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 44 additions & 10 deletions jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/Dispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import org.eclipse.jetty.http.HttpURI;
import org.eclipse.jetty.http.pathmap.MatchedResource;
import org.eclipse.jetty.io.WriterOutputStream;
import org.eclipse.jetty.server.handler.ErrorHandler;
import org.eclipse.jetty.util.Fields;
import org.eclipse.jetty.util.IO;
import org.eclipse.jetty.util.StringUtil;
Expand All @@ -58,10 +59,10 @@ public class Dispatcher implements RequestDispatcher
* Dispatch include attribute names
*/
public static final String __FORWARD_PREFIX = "jakarta.servlet.forward.";

/**
* Name of original request attribute
*/
*/
public static final String __ORIGINAL_REQUEST = "org.eclipse.jetty.originalRequest";

public static final String JETTY_INCLUDE_HEADER_PREFIX = "org.eclipse.jetty.server.include.";
Expand Down Expand Up @@ -316,15 +317,15 @@ public String getRequestURI()
@Override
public StringBuffer getRequestURL()
{
return _uri == null ? super.getRequestURL() : new StringBuffer(HttpURI.build(_uri).query(null).scheme(super.getScheme()).host(super.getServerName()).port(super.getServerPort()).asString());
return _uri == null ? super.getRequestURL() : new StringBuffer(HttpURI.build(_uri).query(null).scheme(super.getScheme()).host(super.getServerName()).port(super.getServerPort()).asString());
}

@Override
public Object getAttribute(String name)
{
if (name == null)
return null;

//Servlet Spec 9.4.2 no forward attributes if a named dispatcher
if (_named != null && name.startsWith(__FORWARD_PREFIX))
return null;
Expand Down Expand Up @@ -356,7 +357,9 @@ public Object getAttribute(String name)
return originalRequest == null ? _httpServletRequest : originalRequest;
}
// Forward should hide include.
case RequestDispatcher.INCLUDE_MAPPING, RequestDispatcher.INCLUDE_SERVLET_PATH, RequestDispatcher.INCLUDE_PATH_INFO, RequestDispatcher.INCLUDE_REQUEST_URI, RequestDispatcher.INCLUDE_CONTEXT_PATH, RequestDispatcher.INCLUDE_QUERY_STRING ->
case RequestDispatcher.INCLUDE_MAPPING, RequestDispatcher.INCLUDE_SERVLET_PATH,
RequestDispatcher.INCLUDE_PATH_INFO, RequestDispatcher.INCLUDE_REQUEST_URI,
RequestDispatcher.INCLUDE_CONTEXT_PATH, RequestDispatcher.INCLUDE_QUERY_STRING ->
{
return null;
}
Expand All @@ -380,11 +383,11 @@ public Object getAttribute(String name)
public Enumeration<String> getAttributeNames()
{
ArrayList<String> names = new ArrayList<>(Collections.list(super.getAttributeNames()));

//Servlet Spec 9.4.2 no forward attributes if a named dispatcher
if (_named != null)
return Collections.enumeration(names);

names.add(RequestDispatcher.FORWARD_REQUEST_URI);
names.add(RequestDispatcher.FORWARD_SERVLET_PATH);
names.add(RequestDispatcher.FORWARD_PATH_INFO);
Expand Down Expand Up @@ -416,7 +419,7 @@ public Object getAttribute(String name)
{
if (name == null)
return null;

//Servlet Spec 9.3.1 no include attributes if a named dispatcher
if (_named != null && name.startsWith(__INCLUDE_PREFIX))
return null;
Expand All @@ -440,7 +443,7 @@ public Enumeration<String> getAttributeNames()
ArrayList<String> names = new ArrayList<>(Collections.list(super.getAttributeNames()));
if (_named != null)
return Collections.enumeration(names);

names.add(RequestDispatcher.INCLUDE_MAPPING);
names.add(RequestDispatcher.INCLUDE_SERVLET_PATH);
names.add(RequestDispatcher.INCLUDE_PATH_INFO);
Expand All @@ -462,7 +465,7 @@ private static class IncludeResponse extends HttpServletResponseWrapper
ServletOutputStream _servletOutputStream;
PrintWriter _printWriter;
PrintWriter _mustFlush;

public IncludeResponse(HttpServletResponse response)
{
super(response);
Expand Down Expand Up @@ -753,9 +756,12 @@ public Enumeration<String> getAttributeNames()

private class ErrorRequest extends ParameterRequestWrapper
{
private final HttpServletRequest _httpServletRequest;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is oddly aligned. I'm surprised checkstyle let it go through.


public ErrorRequest(HttpServletRequest httpRequest)
{
super(httpRequest);
_httpServletRequest = httpRequest;
}

@Override
Expand Down Expand Up @@ -797,6 +803,34 @@ public StringBuffer getRequestURL()
.port(getServerPort())
.asString());
}

@Override
public Object getAttribute(String name)
{
return switch (name)
{
case ERROR_REQUEST_URI -> _httpServletRequest.getRequestURI();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could call getRequest and cast it to HttpServletRequest instead of introducing the _httpServletRequest field.

case ERROR_STATUS_CODE -> super.getAttribute(ErrorHandler.ERROR_STATUS);
case ERROR_MESSAGE -> super.getAttribute(ErrorHandler.ERROR_MESSAGE);
case ERROR_SERVLET_NAME -> super.getAttribute(ErrorHandler.ERROR_ORIGIN);
case ERROR_EXCEPTION -> super.getAttribute(ErrorHandler.ERROR_EXCEPTION);
case ERROR_EXCEPTION_TYPE ->
{
Object err = super.getAttribute(ErrorHandler.ERROR_EXCEPTION);
yield err == null ? null : err.getClass();
}
default -> super.getAttribute(name);
};
}

@Override
public Enumeration<String> getAttributeNames()
{
// TODO add all names?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you add all the names present in getAttribute()'s switch?

List<String> names = new ArrayList<>(List.of(ERROR_REQUEST_URI, ERROR_STATUS_CODE, ERROR_MESSAGE));
names.addAll(Collections.list(super.getAttributeNames()));
return Collections.enumeration(names);
}
}

@Override
Expand Down
17 changes: 8 additions & 9 deletions jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ErrorHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import java.util.Map;
import java.util.stream.Collectors;

import jakarta.servlet.RequestDispatcher;
import jakarta.servlet.ServletException;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
Expand Down Expand Up @@ -128,7 +127,7 @@ public boolean handle(Request request, Response response, Callback callback) thr
}
}

String message = (String)request.getAttribute(Dispatcher.ERROR_MESSAGE);
String message = (String)request.getAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_MESSAGE);
if (message == null)
message = HttpStatus.getMessage(response.getStatus());
generateAcceptableResponse(servletContextRequest, httpServletRequest, httpServletResponse, response.getStatus(), message);
Expand Down Expand Up @@ -416,9 +415,9 @@ protected void writeErrorPageMessage(HttpServletRequest request, Writer writer,
htmlRow(writer, "MESSAGE", message);
if (isShowServlet())
{
htmlRow(writer, "SERVLET", request.getAttribute(Dispatcher.ERROR_SERVLET_NAME));
htmlRow(writer, "SERVLET", request.getAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_ORIGIN));
}
Throwable cause = (Throwable)request.getAttribute(Dispatcher.ERROR_EXCEPTION);
Throwable cause = (Throwable)request.getAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_EXCEPTION);
while (cause != null)
{
htmlRow(writer, "CAUSED BY", cause);
Expand Down Expand Up @@ -451,9 +450,9 @@ protected void writeErrorPlain(HttpServletRequest request, PrintWriter writer, i
writer.printf("MESSAGE: %s%n", message);
if (isShowServlet())
{
writer.printf("SERVLET: %s%n", request.getAttribute(Dispatcher.ERROR_SERVLET_NAME));
writer.printf("SERVLET: %s%n", request.getAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_ORIGIN));
}
Throwable cause = (Throwable)request.getAttribute(Dispatcher.ERROR_EXCEPTION);
Throwable cause = (Throwable)request.getAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_EXCEPTION);
while (cause != null)
{
writer.printf("CAUSED BY %s%n", cause);
Expand All @@ -467,8 +466,8 @@ protected void writeErrorPlain(HttpServletRequest request, PrintWriter writer, i

protected void writeErrorJson(HttpServletRequest request, PrintWriter writer, int code, String message)
{
Throwable cause = (Throwable)request.getAttribute(Dispatcher.ERROR_EXCEPTION);
Object servlet = request.getAttribute(Dispatcher.ERROR_SERVLET_NAME);
Throwable cause = (Throwable)request.getAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_EXCEPTION);
Object servlet = request.getAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_ORIGIN);
Map<String, String> json = new HashMap<>();

json.put("url", request.getRequestURI());
Expand All @@ -492,7 +491,7 @@ protected void writeErrorJson(HttpServletRequest request, PrintWriter writer, in

protected void writeErrorPageStacks(HttpServletRequest request, Writer writer) throws IOException
{
Throwable th = (Throwable)request.getAttribute(RequestDispatcher.ERROR_EXCEPTION);
Throwable th = (Throwable)request.getAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_EXCEPTION);
if (th != null)
{
writer.write("<h3>Caused by:</h3><pre>");
Expand Down
7 changes: 4 additions & 3 deletions ...etty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ErrorPageErrorHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public String getErrorPage(HttpServletRequest request)
PageLookupTechnique pageSource = null;

Class<?> matchedThrowable = null;
Throwable error = (Throwable)request.getAttribute(Dispatcher.ERROR_EXCEPTION);
Throwable error = (Throwable)request.getAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_EXCEPTION);
Throwable cause = error;

// Walk the cause hierarchy
Expand Down Expand Up @@ -98,6 +98,7 @@ public String getErrorPage(HttpServletRequest request)
{
request.setAttribute(Dispatcher.ERROR_EXCEPTION, unwrapped);
request.setAttribute(Dispatcher.ERROR_EXCEPTION_TYPE, unwrapped.getClass());
request.setAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_EXCEPTION, unwrapped);
}
}

Expand All @@ -108,7 +109,7 @@ public String getErrorPage(HttpServletRequest request)
pageSource = PageLookupTechnique.STATUS_CODE;

// look for an exact code match
errorStatusCode = (Integer)request.getAttribute(Dispatcher.ERROR_STATUS_CODE);
errorStatusCode = (Integer)request.getAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_STATUS);
if (errorStatusCode != null)
{
errorPage = _errorPages.get(Integer.toString(errorStatusCode));
Expand Down Expand Up @@ -149,7 +150,7 @@ public String getErrorPage(HttpServletRequest request)
dbg.append(" (using matched Throwable ");
dbg.append(matchedThrowable.getName());
dbg.append(" / actually thrown as ");
Throwable originalThrowable = (Throwable)request.getAttribute(Dispatcher.ERROR_EXCEPTION);
Throwable originalThrowable = (Throwable)request.getAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_EXCEPTION);
dbg.append(originalThrowable.getClass().getName());
dbg.append(')');
LOG.debug(dbg.toString(), cause);
Expand Down
2 changes: 1 addition & 1 deletion ...ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ResourceServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,7 +780,7 @@ protected void writeHttpError(Request coreRequest, Response coreResponse, Callba
if (isIncluded(request))
return;
if (cause != null)
request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, cause);
request.setAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_EXCEPTION, cause);
response.sendError(statusCode, reason);
}
catch (IOException e)
Expand Down
4 changes: 2 additions & 2 deletions ...-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ public void handle()

// the following is needed as you cannot trust the response code and reason
// as those could have been modified after calling sendError
Integer code = (Integer)_servletContextRequest.getAttribute(RequestDispatcher.ERROR_STATUS_CODE);
Integer code = (Integer)_servletContextRequest.getAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_STATUS);
if (code == null)
code = HttpStatus.INTERNAL_SERVER_ERROR_500;
getServletContextResponse().setStatus(code);
Expand All @@ -472,7 +472,7 @@ public void handle()
if (!_httpInput.consumeAvailable())
ResponseUtils.ensureNotPersistent(_servletContextRequest, _servletContextRequest.getServletContextResponse());

ContextHandler.ScopedContext context = (ContextHandler.ScopedContext)_servletContextRequest.getAttribute(ErrorHandler.ERROR_CONTEXT);
ContextHandler.ScopedContext context = (ContextHandler.ScopedContext)_servletContextRequest.getAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_CONTEXT);
Request.Handler errorHandler = ErrorHandler.getErrorHandler(getServer(), context == null ? null : context.getContextHandler());

// If we can't have a body or have no ErrorHandler, then create a minimal error response.
Expand Down
17 changes: 15 additions & 2 deletions .../jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import static jakarta.servlet.RequestDispatcher.ERROR_REQUEST_URI;
import static jakarta.servlet.RequestDispatcher.ERROR_SERVLET_NAME;
import static jakarta.servlet.RequestDispatcher.ERROR_STATUS_CODE;
import static org.eclipse.jetty.server.handler.ErrorHandler.ERROR_STATUS;

/**
* holder of the state of request-response cycle.
Expand Down Expand Up @@ -423,6 +424,16 @@ public Action handling()
throw new IllegalStateException(getStatusStringLocked());
_initial = true;
_state = State.HANDLING;
if (_servletChannel.getResponse().getStatus() != 0)
{
if (_servletChannel.getRequest().getAttribute(ERROR_STATUS) instanceof Integer errorCode)
{
_servletChannel.getServletRequestState().sendError(errorCode, null);
_requestState = RequestState.BLOCKING;
_sendError = false;
return Action.SEND_ERROR;
}
}
return Action.DISPATCH;

case WOKEN:
Expand Down Expand Up @@ -1022,7 +1033,6 @@ else if (cause instanceof UnavailableException)

public void sendError(int code, String message)
{
// This method is called by Response.sendError to organise for an error page to be generated when it is possible:
// + The response is reset and temporarily closed.
// + The details of the error are saved as request attributes
// + The _sendError boolean is set to true so that an ERROR_DISPATCH action will be generated:
Expand Down Expand Up @@ -1067,12 +1077,15 @@ public void sendError(int code, String message)
// Set Jetty Specific Attributes.
request.setAttribute(ErrorHandler.ERROR_CONTEXT, servletContextRequest.getServletContext());
request.setAttribute(ErrorHandler.ERROR_MESSAGE, message);
request.setAttribute(ErrorHandler.ERROR_STATUS, code);
request.setAttribute(ERROR_STATUS, code);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was ERROR_STATUS statically imported? Other constants are not.

request.setAttribute(ErrorHandler.ERROR_ORIGIN, servletContextRequest.getServletName());

_sendError = true;
if (_event != null)
{
Throwable cause = (Throwable)request.getAttribute(ERROR_EXCEPTION);
if (cause == null)
cause = (Throwable)request.getAttribute(ErrorHandler.ERROR_EXCEPTION);
if (cause != null)
_event.addThrowable(cause);
}
Expand Down
11 changes: 9 additions & 2 deletions ...etty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
import org.eclipse.jetty.ee10.servlet.security.ConstraintAware;
import org.eclipse.jetty.ee10.servlet.security.ConstraintMapping;
import org.eclipse.jetty.ee10.servlet.security.ConstraintSecurityHandler;
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.http.HttpURI;
import org.eclipse.jetty.http.pathmap.MatchedResource;
import org.eclipse.jetty.io.IOResources;
Expand Down Expand Up @@ -1147,7 +1148,6 @@ protected ContextRequest wrapRequest(Request request, Response response)
decodedPathInContext = URIUtil.decodePath(getContext().getPathInContext(request.getHttpURI().getCanonicalPath()));
matchedResource = _servletHandler.getMatchedServlet(decodedPathInContext);


if (matchedResource == null)
return wrapNoServlet(request, response);
ServletHandler.MappedServlet mappedServlet = matchedResource.getResource();
Expand Down Expand Up @@ -1196,7 +1196,14 @@ protected boolean handleByContextHandler(String pathInContext, ContextRequest re
boolean initialDispatch = request instanceof ServletContextRequest;
if (!initialDispatch)
return false;
return super.handleByContextHandler(pathInContext, request, response, callback);

if (isProtectedTarget(pathInContext))
{
// Do nothing here other than set the error status so that the ServletHandler will handle as if a sendError
request.setAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_STATUS, 404);
response.setStatus(HttpStatus.NOT_FOUND_404);
}
return false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now different to ee11 - is that on purpose or an omission?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in ee11 required a change to the ErrorPageMapper API and implementation, that I do not want to do in EE10, so just doing the simplest solution here. In EE11, I have tried to clean up the ErrorPageMapper API somewhat... but I think more could be done (maybe a different PR?)

}

@Override
Expand Down
Loading