Issue #12697 - resolve use of Response.writeError in authenticators

Signed-off-by: Lachlan Roberts <[email protected]>
jetty · Jan 10, 2025 · 7f31cb4 · 7f31cb4
1 parent 65495d7
commit 7f31cb4
Show file tree

Hide file tree

Showing 18 changed files with 93 additions and 109 deletions.
diff --git a/jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/AuthenticationState.java b/jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/AuthenticationState.java
@@ -23,6 +23,7 @@
 import org.eclipse.jetty.security.internal.DeferredAuthenticationState;
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.Response;
+import org.eclipse.jetty.server.handler.ErrorHandler;
 import org.eclipse.jetty.util.Callback;
 
 /**
@@ -179,6 +180,20 @@ static boolean logout(Request request, Response response)
         return false;
     }
 
+    static AuthenticationState writeError(Request request, Response response, Callback callback, int code)
+    {
+        if (request.getContext().getErrorHandler() instanceof ErrorHandler errorHandler)
+        {
+            if (errorHandler.writeError(request, response, callback, HttpStatus.FORBIDDEN_403))
+                return AuthenticationState.SEND_FAILURE;
+            else
+                return new AuthenticationState.ServeAs(request.getHttpURI());
+        }
+
+        Response.writeError(request, response, callback, HttpStatus.FORBIDDEN_403);
+        return AuthenticationState.SEND_FAILURE;
+    }
+
     /**
      * A successful Authentication with User information.
      */

diff --git a/...-security/src/main/java/org/eclipse/jetty/security/authentication/BasicAuthenticator.java b/...-security/src/main/java/org/eclipse/jetty/security/authentication/BasicAuthenticator.java
@@ -87,6 +87,8 @@ public AuthenticationState validateRequest(Request req, Response res, Callback c
         if (charset != null)
             value += ", charset=\"" + charset.name() + "\"";
         res.getHeaders().put(HttpHeader.WWW_AUTHENTICATE.asString(), value);
+
+        // Don't use AuthenticationState.writeError, to avoid possibility of doing a Servlet error dispatch.
         Response.writeError(req, res, callback, HttpStatus.UNAUTHORIZED_401);
         return AuthenticationState.CHALLENGE;
     }

diff --git a/...security/src/main/java/org/eclipse/jetty/security/authentication/DigestAuthenticator.java b/...security/src/main/java/org/eclipse/jetty/security/authentication/DigestAuthenticator.java
@@ -180,8 +180,9 @@ else if (n == 0)
                     "\", algorithm=MD5" +
                     ", qop=\"auth\"" +
                     ", stale=" + stale);
-            Response.writeError(req, res, callback, HttpStatus.UNAUTHORIZED_401);
 
+            // Don't use AuthenticationState.writeError, to avoid possibility of doing a Servlet error dispatch.
+            Response.writeError(req, res, callback, HttpStatus.UNAUTHORIZED_401);
             return AuthenticationState.CHALLENGE;
         }
 

diff --git a/...y-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java b/...y-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java
@@ -365,7 +365,7 @@ private AuthenticationState dispatch(String path, Request request, Response resp
     private AuthenticationState sendError(Request request, Response response, Callback callback)
     {
         if (_formErrorPage == null)
-            Response.writeError(request, response, callback, HttpStatus.FORBIDDEN_403);
+            return AuthenticationState.writeError(request, response, callback, HttpStatus.FORBIDDEN_403);
         else if (_dispatch)
             return dispatch(_formErrorPage, request, response, callback);
         else

diff --git a/...security/src/main/java/org/eclipse/jetty/security/authentication/SPNEGOAuthenticator.java b/...security/src/main/java/org/eclipse/jetty/security/authentication/SPNEGOAuthenticator.java
@@ -185,6 +185,7 @@ else if (httpSession != null)
     private void sendChallenge(Request req, Response res, Callback callback, String token)
     {
         setSpnegoToken(res, token);
+        // Don't use AuthenticationState.writeError, to avoid possibility of doing a Servlet error dispatch.
         Response.writeError(req, res, callback, HttpStatus.UNAUTHORIZED_401);
     }
 

diff --git a/...y/src/main/java/org/eclipse/jetty/security/authentication/SslClientCertAuthenticator.java b/...y/src/main/java/org/eclipse/jetty/security/authentication/SslClientCertAuthenticator.java
@@ -56,11 +56,8 @@ public String getAuthenticationType()
     public AuthenticationState validateRequest(Request req, Response res, Callback callback) throws ServerAuthException
     {
         if (!(req.getAttribute(EndPoint.SslSessionData.ATTRIBUTE) instanceof EndPoint.SslSessionData sslSessionData))
-        {
-            Response.writeError(req, res, callback, HttpStatus.FORBIDDEN_403);
-            return AuthenticationState.SEND_FAILURE;
-        }
-
+            return AuthenticationState.writeError(req, res, callback, HttpStatus.FORBIDDEN_403);
+
         X509Certificate[] certs = sslSessionData.peerCertificates();
 
         try
@@ -106,10 +103,7 @@ public AuthenticationState validateRequest(Request req, Response res, Callback c
             }
 
             if (!AuthenticationState.Deferred.isDeferred(res))
-            {
-                Response.writeError(req, res, callback, HttpStatus.FORBIDDEN_403);
-                return AuthenticationState.SEND_FAILURE;
-            }
+                return AuthenticationState.writeError(req, res, callback, HttpStatus.FORBIDDEN_403);
 
             return null;
         }

diff --git a/...ty-ee10-jaspi/src/main/java/org/eclipse/jetty/ee10/security/jaspi/JaspiAuthenticator.java b/...ty-ee10-jaspi/src/main/java/org/eclipse/jetty/ee10/security/jaspi/JaspiAuthenticator.java
@@ -233,8 +233,7 @@ public AuthenticationState validateRequest(JaspiMessageInfo messageInfo) throws
             }
             if (authStatus == AuthStatus.FAILURE)
             {
-                Response.writeError(messageInfo.getBaseRequest(), messageInfo.getBaseResponse(), messageInfo.getCallback(), HttpServletResponse.SC_FORBIDDEN);
-                return AuthenticationState.SEND_FAILURE;
+                return AuthenticationState.writeError(messageInfo.getBaseRequest(), messageInfo.getBaseResponse(), messageInfo.getCallback(), HttpServletResponse.SC_FORBIDDEN);
             }
             // should not happen
             throw new IllegalStateException("No AuthStatus returned");

diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ErrorHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ErrorHandler.java
@@ -47,9 +47,17 @@ public ErrorHandler()
     @Override
     public boolean writeError(Request request, Response response, Callback callback, int code)
     {
-        response.setStatus(code);
-        request.setAttribute(ERROR_STATUS, code);
-        return false;
+        // If we have not entered the servlet channel we should trigger a sendError for when we do enter the servlet channel.
+        ServletContextRequest servletContextRequest = Request.as(request, ServletContextRequest.class);
+        boolean enteredServletChannel = servletContextRequest.getServletChannel().getCallback() != null;
+        if (!enteredServletChannel)
+        {
+            response.setStatus(code);
+            request.setAttribute(ERROR_STATUS, code);
+            return false;
+        }
+
+        return super.writeError(request, response, callback, code);
     }
 
     @Override

diff --git a/...etty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java b/...etty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java
@@ -1199,13 +1199,14 @@ protected boolean handleByContextHandler(String pathInContext, ContextRequest re
 
         if (isProtectedTarget(pathInContext))
         {
-            // At this point we have not entered the state machine of the ServletChannelState, so we do nothing here
-            // other than to set the error status attribute (and also status). When execution proceeds normally into the
-            // state machine the request will be treated as an error. Note that we set both the error status and request
-            // status because the request status is cheaper to check than the error status attribute.
-            request.setAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_STATUS, 404);
-            response.setStatus(HttpStatus.NOT_FOUND_404);
+            // If we have a Servlet ErrorHandler, then writeError will return false, and it will signal
+            // the ServletChannel to trigger a sendError() when it is started.
+            if (request.getContext().getErrorHandler() instanceof org.eclipse.jetty.server.handler.ErrorHandler errorHandler)
+                return errorHandler.writeError(request, response, callback, HttpStatus.NOT_FOUND_404);
+            Response.writeError(request, response, callback, HttpStatus.NOT_FOUND_404);
+            return true;
         }
+
         return false;
     }
 

diff --git a/...rc/test/java/org/eclipse/jetty/ee10/websocket/jakarta/tests/ServerUpgradeRequestTest.java b/...rc/test/java/org/eclipse/jetty/ee10/websocket/jakarta/tests/ServerUpgradeRequestTest.java
@@ -124,8 +124,7 @@ public AuthenticationState validateRequest(Request request, Response response, C
             if (user != null)
                 return new UserAuthenticationSucceeded(getAuthenticationType(), user);
 
-            Response.writeError(request, response, callback, HttpStatus.FORBIDDEN_403);
-            return AuthenticationState.SEND_FAILURE;
+            return AuthenticationState.writeError(request, response, callback, HttpStatus.FORBIDDEN_403);
         }
     }
 

diff --git a/...-tests/src/test/java/org/eclipse/jetty/ee10/websocket/tests/ServerUpgradeRequestTest.java b/...-tests/src/test/java/org/eclipse/jetty/ee10/websocket/tests/ServerUpgradeRequestTest.java
@@ -134,8 +134,7 @@ public AuthenticationState validateRequest(Request request, Response response, C
             if (user != null)
                 return new UserAuthenticationSucceeded(getAuthenticationType(), user);
 
-            Response.writeError(request, response, callback, HttpStatus.FORBIDDEN_403);
-            return AuthenticationState.SEND_FAILURE;
+            return AuthenticationState.writeError(request, response, callback, HttpStatus.FORBIDDEN_403);
         }
     }
 

diff --git a/...ty-ee11-jaspi/src/main/java/org/eclipse/jetty/ee11/security/jaspi/JaspiAuthenticator.java b/...ty-ee11-jaspi/src/main/java/org/eclipse/jetty/ee11/security/jaspi/JaspiAuthenticator.java
@@ -233,8 +233,7 @@ public AuthenticationState validateRequest(JaspiMessageInfo messageInfo) throws
             }
             if (authStatus == AuthStatus.FAILURE)
             {
-                Response.writeError(messageInfo.getBaseRequest(), messageInfo.getBaseResponse(), messageInfo.getCallback(), HttpServletResponse.SC_FORBIDDEN);
-                return AuthenticationState.SEND_FAILURE;
+                return AuthenticationState.writeError(messageInfo.getBaseRequest(), messageInfo.getBaseResponse(), messageInfo.getCallback(), HttpServletResponse.SC_FORBIDDEN);
             }
             // should not happen
             throw new IllegalStateException("No AuthStatus returned");

diff --git a/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ErrorHandler.java b/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ErrorHandler.java
@@ -47,9 +47,17 @@ public ErrorHandler()
     @Override
     public boolean writeError(Request request, Response response, Callback callback, int code)
     {
-        response.setStatus(code);
-        request.setAttribute(ERROR_STATUS, code);
-        return false;
+        // If we have not entered the servlet channel we should trigger a sendError for when we do enter the servlet channel.
+        ServletContextRequest servletContextRequest = Request.as(request, ServletContextRequest.class);
+        boolean enteredServletChannel = servletContextRequest.getServletChannel().getCallback() != null;
+        if (!enteredServletChannel)
+        {
+            response.setStatus(code);
+            request.setAttribute(ERROR_STATUS, code);
+            return false;
+        }
+
+        return super.writeError(request, response, callback, code);
     }
 
     @Override

diff --git a/...etty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletContextHandler.java b/...etty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletContextHandler.java
@@ -114,7 +114,6 @@
 import org.slf4j.LoggerFactory;
 
 import static jakarta.servlet.ServletContext.TEMPDIR;
-import static org.eclipse.jetty.server.handler.ErrorHandler.ERROR_STATUS;
 
 /**
  * Servlet Context.
@@ -1198,18 +1197,11 @@ protected boolean handleByContextHandler(String pathInContext, ContextRequest re
 
         if (isProtectedTarget(pathInContext))
         {
-            if (getErrorHandler() instanceof ErrorHandler.ErrorPageMapper mapper)
-            {
-                ErrorHandler.ErrorPageMapper.ErrorPage errorPage = mapper.getErrorPage(404, null);
-                if (errorPage != null)
-                {
-                    // Do nothing here other than set the error status so that the ServletHandler will handle as if a sendError
-                    request.setAttribute(ERROR_STATUS, 404);
-                    response.setStatus(HttpStatus.NOT_FOUND_404);
-                    return false;
-                }
-            }
-            Response.writeError(request, response, callback, HttpStatus.NOT_FOUND_404, null);
+            // If we have a Servlet ErrorHandler, then writeError will return false, and it will signal
+            // the ServletChannel to trigger a sendError() when it is started.
+            if (request.getContext().getErrorHandler() instanceof org.eclipse.jetty.server.handler.ErrorHandler errorHandler)
+                return errorHandler.writeError(request, response, callback, HttpStatus.NOT_FOUND_404);
+            Response.writeError(request, response, callback, HttpStatus.NOT_FOUND_404);
             return true;
         }
         return false;

diff --git a/...rc/test/java/org/eclipse/jetty/ee11/websocket/jakarta/tests/ServerUpgradeRequestTest.java b/...rc/test/java/org/eclipse/jetty/ee11/websocket/jakarta/tests/ServerUpgradeRequestTest.java
@@ -124,8 +124,7 @@ public AuthenticationState validateRequest(Request request, Response response, C
             if (user != null)
                 return new UserAuthenticationSucceeded(getAuthenticationType(), user);
 
-            Response.writeError(request, response, callback, HttpStatus.FORBIDDEN_403);
-            return AuthenticationState.SEND_FAILURE;
+            return AuthenticationState.writeError(request, response, callback, HttpStatus.FORBIDDEN_403);
         }
     }
 

diff --git a/...-tests/src/test/java/org/eclipse/jetty/ee11/websocket/tests/ServerUpgradeRequestTest.java b/...-tests/src/test/java/org/eclipse/jetty/ee11/websocket/tests/ServerUpgradeRequestTest.java
@@ -134,8 +134,7 @@ public AuthenticationState validateRequest(Request request, Response response, C
             if (user != null)
                 return new UserAuthenticationSucceeded(getAuthenticationType(), user);
 
-            Response.writeError(request, response, callback, HttpStatus.FORBIDDEN_403);
-            return AuthenticationState.SEND_FAILURE;
+            return AuthenticationState.writeError(request, response, callback, HttpStatus.FORBIDDEN_403);
         }
     }
 

diff --git a/...s/jetty-ethereum/src/main/java/org/eclipse/jetty/security/siwe/EthereumAuthenticator.java b/...s/jetty-ethereum/src/main/java/org/eclipse/jetty/security/siwe/EthereumAuthenticator.java
@@ -513,26 +513,22 @@ protected AuthenticationState handleNonceRequest(Request request, Response respo
         return AuthenticationState.CHALLENGE;
     }
 
-    private boolean validateSignInWithEthereumToken(SignInWithEthereumToken siwe, SignedMessage signedMessage, Request request, Response response, Callback callback)
+    private AuthenticationState validateSignInWithEthereumToken(SignInWithEthereumToken siwe, SignedMessage signedMessage, Request request, Response response, Callback callback)
     {
         Session session = request.getSession(false);
         if (siwe == null)
-        {
-            sendError(request, response, callback, "failed to parse SIWE message");
-            return false;
-        }
+            return sendError(request, response, callback, "failed to parse SIWE message");;
 
         try
         {
             siwe.validate(signedMessage, nonce -> redeemNonce(session, nonce), _domains, _chainIds);
         }
         catch (Throwable t)
         {
-            sendError(request, response, callback, t.getMessage());
-            return false;
+            return sendError(request, response, callback, t.getMessage());
         }
 
-        return true;
+        return null;
     }
 
     @Override
@@ -552,10 +548,7 @@ public AuthenticationState validateRequest(Request request, Response response, C
             {
                 session = request.getSession(true);
                 if (session == null)
-                {
-                    sendError(request, response, callback, "session could not be created");
-                    return AuthenticationState.SEND_FAILURE;
-                }
+                    return sendError(request, response, callback, "session could not be created");
             }
 
             if (isNonceRequest(uri))
@@ -568,10 +561,14 @@ public AuthenticationState validateRequest(Request request, Response response, C
                 // Parse and validate SIWE Message.
                 SignedMessage signedMessage = parseMessage(request, response, callback);
                 if (signedMessage == null)
-                    return AuthenticationState.SEND_FAILURE;
+                    return sendError(request, response, callback, "failed to read SIWE message");
                 SignInWithEthereumToken siwe = SignInWithEthereumToken.from(signedMessage.message());
-                if (siwe == null || !validateSignInWithEthereumToken(siwe, signedMessage, request, response, callback))
-                    return AuthenticationState.SEND_FAILURE;
+                if (siwe == null)
+                    return sendError(request, response, callback, "failed to parse SIWE message");
+
+                AuthenticationState authenticationState = validateSignInWithEthereumToken(siwe, signedMessage, request, response, callback);
+                if (authenticationState != null)
+                    return authenticationState;
 
                 String address = siwe.address();
                 UserIdentity user = login(address, null, request, response);
@@ -592,8 +589,7 @@ public AuthenticationState validateRequest(Request request, Response response, C
                     return formAuth;
                 }
 
-                sendError(request, response, callback, "auth failed");
-                return AuthenticationState.SEND_FAILURE;
+                return sendError(request, response, callback, "auth failed");
             }
 
             // Look for cached authentication in the Session.
@@ -605,21 +601,21 @@ public AuthenticationState validateRequest(Request request, Response response, C
                     !_loginService.validate(((AuthenticationState.Succeeded)authenticationState).getUserIdentity()))
                 {
                     if (LOG.isDebugEnabled())
-                        LOG.debug("auth revoked {}", authenticationState);
+                        LOG.debug("authentication revoked {}", authenticationState);
                     logoutWithoutRedirect(request, response);
-                    return AuthenticationState.SEND_FAILURE;
+                    return sendError(request, response, callback, "authentication revoked");
                 }
 
                 if (LOG.isDebugEnabled())
-                    LOG.debug("auth {}", authenticationState);
+                    LOG.debug("authenticationState {}", authenticationState);
                 return authenticationState;
             }
 
             // If we can't send challenge.
             if (AuthenticationState.Deferred.isDeferred(response))
             {
                 if (LOG.isDebugEnabled())
-                    LOG.debug("auth deferred {}", session.getId());
+                    LOG.debug("authentication deferred {}", session.getId());
                 return null;
             }
 
@@ -667,23 +663,17 @@ public AuthenticationState validateRequest(Request request, Response response, C
      * @param callback the callback.
      * @param message the reason for the error or null.
      */
-    private void sendError(Request request, Response response, Callback callback, String message)
+    private AuthenticationState sendError(Request request, Response response, Callback callback, String message)
     {
         if (LOG.isDebugEnabled())
             LOG.debug("Authentication FAILED: {}", message);
 
         if (_errorPath == null)
         {
-            if (LOG.isDebugEnabled())
-                LOG.debug("auth failed 403");
-            if (response != null)
-                Response.writeError(request, response, callback, HttpStatus.FORBIDDEN_403, message);
+            return AuthenticationState.writeError(request, response, callback, HttpStatus.FORBIDDEN_403);
         }
         else
         {
-            if (LOG.isDebugEnabled())
-                LOG.debug("auth failed {}", _errorPath);
-
             String contextPath = Request.getContextPath(request);
             String redirectUri = URIUtil.addPaths(contextPath, _errorPath);
             if (message != null)
@@ -695,6 +685,7 @@ private void sendError(Request request, Response response, Callback callback, St
             int redirectCode = request.getConnectionMetaData().getHttpVersion().getVersion() < HttpVersion.HTTP_1_1.getVersion()
                 ? HttpStatus.MOVED_TEMPORARILY_302 : HttpStatus.SEE_OTHER_303;
             Response.sendRedirect(request, response, callback, redirectCode, redirectUri, true);
+            return AuthenticationState.SEND_FAILURE;
         }
     }