Issue #5605 unconsumed input on sendError

jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/modules/FormAuthModule.java

@@ -35,6 +35,7 @@
 import org.eclipse.jetty.security.authentication.DeferredAuthentication;
 import org.eclipse.jetty.security.authentication.LoginCallbackImpl;
 import org.eclipse.jetty.security.authentication.SessionAuthentication;
+import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.UserIdentity;
 import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.URIUtil;
@@ -132,7 +133,6 @@ private void setErrorPage(String path)
     @Override
     public AuthStatus validateRequest(MessageInfo messageInfo, Subject clientSubject, Subject serviceSubject) throws AuthException
     {
-
         HttpServletRequest request = (HttpServletRequest)messageInfo.getRequestMessage();
         HttpServletResponse response = (HttpServletResponse)messageInfo.getResponseMessage();
         String uri = request.getRequestURI();
@@ -173,6 +173,7 @@ public AuthStatus validateRequest(MessageInfo messageInfo, Subject clientSubject
                     }
 
                     response.setContentLength(0);
+                    Request.getBaseRequest(request).getHttpChannel().ensureContentConsumedOrConnectionClose();
                     response.sendRedirect(response.encodeRedirectURL(nuri));
                     return AuthStatus.SEND_CONTINUE;
                 }
@@ -187,6 +188,7 @@ public AuthStatus validateRequest(MessageInfo messageInfo, Subject clientSubject
                 else
                 {
                     response.setContentLength(0);
+                    Request.getBaseRequest(request).getHttpChannel().ensureContentConsumedOrConnectionClose();
                     response.sendRedirect(response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formErrorPage)));
                 }
                 // TODO is this correct response if isMandatory false??? Can
@@ -229,6 +231,7 @@ public AuthStatus validateRequest(MessageInfo messageInfo, Subject clientSubject
             }
 
             response.setContentLength(0);
+            Request.getBaseRequest(request).getHttpChannel().ensureContentConsumedOrConnectionClose();
             response.sendRedirect(response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formLoginPage)));
             return AuthStatus.SEND_CONTINUE;
         }

jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdAuthenticator.java

@@ -300,6 +300,7 @@ public Authentication validateRequest(ServletRequest req, ServletResponse res, b
                             LOG.debug("authenticated {}->{}", openIdAuth, nuri);
 
                         response.setContentLength(0);
+                        baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose();
                         baseResponse.sendRedirect(getRedirectCode(baseRequest.getHttpVersion()), nuri);
                         return openIdAuth;
                     }
@@ -392,6 +393,7 @@ public Authentication validateRequest(ServletRequest req, ServletResponse res, b
             String challengeUri = getChallengeUri(request);
             if (LOG.isDebugEnabled())
                 LOG.debug("challenge {}->{}", session.getId(), challengeUri);
+            baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose();
             baseResponse.sendRedirect(getRedirectCode(baseRequest.getHttpVersion()), challengeUri);
 
             return Authentication.SEND_CONTINUE;
@@ -436,9 +438,11 @@ private void sendError(HttpServletRequest request, HttpServletResponse response,
             {
                 String query = URIUtil.addQueries(ERROR_PARAMETER + "=" + UrlEncoded.encodeString(message), _errorQuery);
                 redirectUri = URIUtil.addPathQuery(URIUtil.addPaths(request.getContextPath(), _errorPath), query);
+                baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose();
                 baseResponse.sendRedirect(getRedirectCode(baseRequest.getHttpVersion()), redirectUri);
             }
 
+            baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose();
             baseResponse.sendRedirect(getRedirectCode(baseRequest.getHttpVersion()), redirectUri);
         }
     }

jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java

@@ -641,7 +641,8 @@ protected boolean checkUserDataPermissions(String pathInContext, Request request
         if (dataConstraint == null || dataConstraint == UserDataConstraint.None)
             return true;
 
-        HttpConfiguration httpConfig = Request.getBaseRequest(request).getHttpChannel().getHttpConfiguration();
+        Request baseRequest = Request.getBaseRequest(request);
+        HttpConfiguration httpConfig = baseRequest.getHttpChannel().getHttpConfiguration();
 
         if (dataConstraint == UserDataConstraint.Confidential || dataConstraint == UserDataConstraint.Integral)
         {
@@ -655,6 +656,7 @@ protected boolean checkUserDataPermissions(String pathInContext, Request request
 
                 String url = URIUtil.newURI(scheme, request.getServerName(), port, request.getRequestURI(), request.getQueryString());
                 response.setContentLength(0);
+                baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose();
                 response.sendRedirect(url);
             }
             else

jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java

@@ -292,6 +292,7 @@ public Authentication validateRequest(ServletRequest req, ServletResponse res, b
 
                     response.setContentLength(0);
                     int redirectCode = (baseRequest.getHttpVersion().getVersion() < HttpVersion.HTTP_1_1.getVersion() ? HttpServletResponse.SC_MOVED_TEMPORARILY : HttpServletResponse.SC_SEE_OTHER);
+                    baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose();
                     baseResponse.sendRedirect(redirectCode, response.encodeRedirectURL(nuri));
                     return formAuth;
                 }
@@ -317,6 +318,7 @@ else if (_dispatch)
                 {
                     LOG.debug("auth failed {}->{}", username, _formErrorPage);
                     int redirectCode = (baseRequest.getHttpVersion().getVersion() < HttpVersion.HTTP_1_1.getVersion() ? HttpServletResponse.SC_MOVED_TEMPORARILY : HttpServletResponse.SC_SEE_OTHER);
+                    baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose();
                     baseResponse.sendRedirect(redirectCode, response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formErrorPage)));
                 }
 
@@ -411,6 +413,7 @@ else if (_dispatch)
             {
                 LOG.debug("challenge {}->{}", session.getId(), _formLoginPage);
                 int redirectCode = (baseRequest.getHttpVersion().getVersion() < HttpVersion.HTTP_1_1.getVersion() ? HttpServletResponse.SC_MOVED_TEMPORARILY : HttpServletResponse.SC_SEE_OTHER);
+                baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose();
                 baseResponse.sendRedirect(redirectCode, response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formLoginPage)));
             }
             return Authentication.SEND_CONTINUE;

jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java

@@ -77,6 +77,7 @@
 import static org.hamcrest.Matchers.in;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.Matchers.startsWith;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
@@ -468,19 +469,13 @@ public static Stream<Arguments> basicScenarios()
             )
         ));
 
-//        rawResponse = _connector.getResponse("GET /ctx/noauth/info HTTP/1.0\r\n\r\n");
-//        assertThat(rawResponse, startsWith("HTTP/1.1 200 OK"));
-
         scenarios.add(Arguments.of(
             new Scenario(
                 "GET /ctx/forbid/info HTTP/1.0\r\n\r\n",
                 HttpStatus.FORBIDDEN_403
             )
         ));
 
-//        rawResponse = _connector.getResponse("GET /ctx/forbid/info HTTP/1.0\r\n\r\n");
-//        assertThat(rawResponse, startsWith("HTTP/1.1 403 Forbidden"));
-
         scenarios.add(Arguments.of(
             new Scenario(
                 "GET /ctx/auth/info HTTP/1.0\r\n\r\n",
@@ -493,9 +488,39 @@ public static Stream<Arguments> basicScenarios()
             )
         ));
 
-//        rawResponse = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n\r\n");
-//        assertThat(rawResponse, startsWith("HTTP/1.1 401 Unauthorized"));
-//        assertThat(rawResponse, containsString("WWW-Authenticate: basic realm=\"TestRealm\""));
+        scenarios.add(Arguments.of(
+            new Scenario(
+                "POST /ctx/auth/info HTTP/1.1\r\n" +
+                    "Host: test\r\n" +
+                    "Content-Length: 10\r\n" +
+                    "\r\n" +
+                    "0123456789",
+                HttpStatus.UNAUTHORIZED_401,
+                (response) ->
+                {
+                    String authHeader = response.get(HttpHeader.WWW_AUTHENTICATE);
+                    assertThat(response.toString(), authHeader, containsString("basic realm=\"TestRealm\""));
+                    assertThat(response.get(HttpHeader.CONNECTION), nullValue());
+                }
+            )
+        ));
+
+        scenarios.add(Arguments.of(
+            new Scenario(
+                "POST /ctx/auth/info HTTP/1.1\r\n" +
+                    "Host: test\r\n" +
+                    "Content-Length: 10\r\n" +
+                    "\r\n" +
+                    "012345",
+                HttpStatus.UNAUTHORIZED_401,
+                (response) ->
+                {
+                    String authHeader = response.get(HttpHeader.WWW_AUTHENTICATE);
+                    assertThat(response.toString(), authHeader, containsString("basic realm=\"TestRealm\""));
+                    assertThat(response.get(HttpHeader.CONNECTION), is("close"));
+                }
+            )
+        ));
 
         scenarios.add(Arguments.of(
             new Scenario(
@@ -511,12 +536,6 @@ public static Stream<Arguments> basicScenarios()
             )
         ));
 
-//        rawResponse = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n" +
-//                "Authorization: Basic " + authBase64("user:wrong") + "\r\n" +
-//                "\r\n");
-//        assertThat(rawResponse, startsWith("HTTP/1.1 401 Unauthorized"));
-//        assertThat(rawResponse, containsString("WWW-Authenticate: basic realm=\"TestRealm\""));
-
         scenarios.add(Arguments.of(
             new Scenario(
                 "GET /ctx/auth/info HTTP/1.0\r\n" +
@@ -526,10 +545,16 @@ public static Stream<Arguments> basicScenarios()
             )
         ));
 
-//        rawResponse = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n" +
-//                "Authorization: Basic " + authBase64("user:password") + "\r\n" +
-//                "\r\n");
-//        assertThat(rawResponse, startsWith("HTTP/1.1 200 OK"));
+        scenarios.add(Arguments.of(
+            new Scenario(
+                "POST /ctx/auth/info HTTP/1.0\r\n" +
+                    "Content-Length: 10\r\n" +
+                    "Authorization: Basic " + authBase64("user:password") + "\r\n" +
+                    "\r\n" +
+                    "0123456789",
+                HttpStatus.OK_200
+            )
+        ));
 
         // == test admin
         scenarios.add(Arguments.of(
@@ -544,10 +569,6 @@ public static Stream<Arguments> basicScenarios()
             )
         ));
 
-//        rawResponse = _connector.getResponse("GET /ctx/admin/info HTTP/1.0\r\n\r\n");
-//        assertThat(rawResponse, startsWith("HTTP/1.1 401 Unauthorized"));
-//        assertThat(rawResponse, containsString("WWW-Authenticate: basic realm=\"TestRealm\""));
-
         scenarios.add(Arguments.of(
             new Scenario(
                 "GET /ctx/admin/info HTTP/1.0\r\n" +
@@ -1007,6 +1028,31 @@ public void testFormPostRedirect() throws Exception
         assertThat(response, containsString("!role"));
     }
 
+    @Test
+    public void testNonFormPostRedirect() throws Exception
+    {
+        _security.setAuthenticator(new FormAuthenticator("/testLoginPage", "/testErrorPage", false));
+        _server.start();
+
+        String response = _connector.getResponse("POST /ctx/auth/info HTTP/1.0\r\n" +
+            "Content-Type: text/plain\r\n" +
+            "Content-Length: 10\r\n" +
+            "\r\n" +
+            "0123456789\r\n");
+        assertThat(response, containsString(" 302 Found"));
+        assertThat(response, containsString("/ctx/testLoginPage"));
+        assertThat(response, not(containsString("Connection: close")));
+
+        response = _connector.getResponse("POST /ctx/auth/info HTTP/1.0\r\n" +
+            "Content-Type: text/plain\r\n" +
+            "Content-Length: 10\r\n" +
+            "\r\n" +
+            "012345\r\n");
+        assertThat(response, containsString(" 302 Found"));
+        assertThat(response, containsString("/ctx/testLoginPage"));
+        assertThat(response, containsString("Connection: close"));
+    }
+
     @Test
     public void testFormNoCookies() throws Exception
     {

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java

@@ -31,14 +31,18 @@
 import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import javax.servlet.DispatcherType;
 import javax.servlet.RequestDispatcher;
 import javax.servlet.ServletException;
 
 import org.eclipse.jetty.http.BadMessageException;
+import org.eclipse.jetty.http.HttpField;
 import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.http.HttpGenerator;
 import org.eclipse.jetty.http.HttpHeader;
+import org.eclipse.jetty.http.HttpHeaderValue;
 import org.eclipse.jetty.http.HttpScheme;
 import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.http.HttpVersion;
@@ -406,7 +410,12 @@ public boolean 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)_request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE);
-                            _response.setStatus(code != null ? code : HttpStatus.INTERNAL_SERVER_ERROR_500);
+                            if (code == null)
+                                code = HttpStatus.INTERNAL_SERVER_ERROR_500;
+                            _response.setStatus(code);
+
+                            // Add Connection:close if we can't consume the input
+                            ensureContentConsumedOrConnectionClose();
 
                             ContextHandler.Context context = (ContextHandler.Context)_request.getAttribute(ErrorHandler.ERROR_CONTEXT);
                             ErrorHandler errorHandler = ErrorHandler.getErrorHandler(getServer(), context == null ? null : context.getContextHandler());
@@ -492,10 +501,18 @@ public boolean handle()
 
                     case COMPLETE:
                     {
-                        if (!_response.isCommitted() && !_request.isHandled() && !_response.getHttpOutput().isClosed())
+                        if (!_response.isCommitted())
                         {
-                            _response.sendError(HttpStatus.NOT_FOUND_404);
-                            break;
+                            if (!_request.isHandled() && !_response.getHttpOutput().isClosed())
+                            {
+                                // The request was not actually handled
+                                _response.sendError(HttpStatus.NOT_FOUND_404);
+                                break;
+                            }
+
+                            // Indicate Connection:close if we can't consume all.
+                            if (_response.getStatus() >= 200)
+                                ensureContentConsumedOrConnectionClose();
                         }
 
                         // RFC 7230, section 3.3.
@@ -511,12 +528,7 @@ public boolean handle()
                                 break;
                             }
                         }
-
-                        // TODO Currently a blocking/aborting consumeAll is done in the handling of the TERMINATED
-                        // TODO Action triggered by the completed callback below.  It would be possible to modify the
-                        // TODO callback to do a non-blocking consumeAll at this point and only call completed when
-                        // TODO that is done.
-
+
                         // Set a close callback on the HttpOutput to make it an async callback
                         _response.completeOutput(Callback.from(() -> _state.completed(null), _state::completed));
 
@@ -545,6 +557,31 @@ public boolean handle()
         return !suspended;
     }
 
+    public void ensureContentConsumedOrConnectionClose()
+    {
+        if (_request.getHttpInput().consumeAll())
+            return;
+        _response.getHttpFields().computeField(HttpHeader.CONNECTION, (h, fields) ->
+        {
+            if (fields == null || fields.isEmpty())
+                return HttpConnection.CONNECTION_CLOSE;
+
+            if (fields.stream().anyMatch(f -> f.contains(HttpHeaderValue.CLOSE.asString())))
+            {
+                if (fields.size() == 1)
+                    return fields.get(0);
+
+                return new HttpField(HttpHeader.CONNECTION, fields.stream()
+                    .flatMap(field -> Stream.of(field.getValues()))
+                    .collect(Collectors.joining(", ")));
+            }
+
+            return new HttpField(HttpHeader.CONNECTION, fields.stream()
+                .flatMap(field -> Stream.of(field.getValues()))
+                .collect(Collectors.joining(", ")) + ", " + HttpHeaderValue.CLOSE.asString());
+        });
+    }
+
     private void dispatch(DispatcherType type, Dispatchable dispatchable) throws IOException, ServletException
     {
         try

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java

@@ -904,8 +904,6 @@ public void sendError(int code, String message)
                 default:
                     throw new IllegalStateException(getStatusStringLocked());
             }
-            if (_outputState != OutputState.OPEN)
-                throw new IllegalStateException("Response is " + _outputState);
 
             response.setStatus(code);
             response.errorClose();