Issue #5605 unconsumed input on sendError (#5637)

* Issue #5605 unconsumed input on sendError Add Connection:close if content can't be consumed during a sendError. Processed after the request has returned to the container. Signed-off-by: Greg Wilkins <[email protected]> * Update from review + Add close on all uncommitted requests when content cannot be consumed. * Update from review + fixed comment + space comma * Only consume input in COMPLETE if response is >=200 (ie not an upgrade or similar) * Updated to be less adventurous I do not think it was valid to always consumeAll in COMPLETE as this could break upgrades with both 101s and 200s Instead I have reverted to having this consumeAll logic only: + in sendError once control has passed back to the container and we are about to generate an error page. + in front of all the sendRedirection that we do without calling the application first. Extra tests also added * Updated to be less adventurous reverted test * Testcase for odd sendError(400) issue. Signed-off-by: Joakim Erdfelt <[email protected]> * Fix for odd sendError(400) issue. Signed-off-by: Simone Bordet <[email protected]> * Testcase for odd sendError(400) issue. Signed-off-by: Joakim Erdfelt <[email protected]> * Always try to consumeAll on all requests * Refinements after testing in 10 * Refinements after testing in 10 Fixed test * Fixed comment from review * Updates from review + added redirect methods that consumeAll + ensureContentConsumedOrConnectionClose renamed to ensureConsumeAllOrNotPersistent + ensureConsumeAllOrNotPersistent now handles HTTP/1.0 and HTTP/1.1 differently * better consumeAll implementation * update from review + better javadoc + filter out keep-alive + added more tests * update from review + better javadoc * update from review + fixed form redirection test for http 1.0 and 1.1 * update from review + HttpGenerator removes keep-alive if close present + Use isRedirection Co-authored-by: Joakim Erdfelt <[email protected]> Co-authored-by: Simone Bordet <[email protected]>
jetty · Nov 18, 2020 · 14f94f7 · 14f94f7
1 parent 1448444
commit 14f94f7
Show file tree

Hide file tree

Showing 20 changed files with 662 additions and 117 deletions.
diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java
@@ -23,6 +23,8 @@
 import java.nio.ByteBuffer;
 import java.util.Arrays;
 import java.util.function.Supplier;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import org.eclipse.jetty.http.HttpTokens.EndOfContent;
 import org.eclipse.jetty.util.ArrayTrie;
@@ -658,17 +660,23 @@ else if (contentLength != field.getLongValue())
 
                         case CONNECTION:
                         {
-                            putTo(field, header);
+                            boolean keepAlive = field.contains(HttpHeaderValue.KEEP_ALIVE.asString());
+                            if (keepAlive && info.getHttpVersion() == HttpVersion.HTTP_1_0 && _persistent == null)
+                            {
+                                _persistent = true;
+                            }
                             if (field.contains(HttpHeaderValue.CLOSE.asString()))
                             {
                                 close = true;
                                 _persistent = false;
                             }
-
-                            if (info.getHttpVersion() == HttpVersion.HTTP_1_0 && _persistent == null && field.contains(HttpHeaderValue.KEEP_ALIVE.asString()))
+                            if (keepAlive && _persistent == Boolean.FALSE)
                             {
-                                _persistent = true;
+                                field = new HttpField(HttpHeader.CONNECTION,
+                                    Stream.of(field.getValues()).filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s))
+                                        .collect(Collectors.joining(", ")));
                             }
+                            putTo(field, header);
                             break;
                         }
 

diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpGeneratorServerTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpGeneratorServerTest.java
@@ -862,4 +862,20 @@ public void testConnectionKeepAliveWithAdditionalCustomValue() throws Exception
         assertThat(headers, containsString(HttpHeaderValue.KEEP_ALIVE.asString()));
         assertThat(headers, containsString(customValue));
     }
+
+    @Test
+    public void testKeepAliveWithClose() throws Exception
+    {
+        HttpGenerator generator = new HttpGenerator();
+        HttpFields fields = new HttpFields();
+        fields.put(HttpHeader.CONNECTION,
+            HttpHeaderValue.KEEP_ALIVE.asString() + ", other, " + HttpHeaderValue.CLOSE.asString());
+        MetaData.Response info = new MetaData.Response(HttpVersion.HTTP_1_0, 200, "OK", fields, -1);
+        ByteBuffer header = BufferUtil.allocate(4096);
+        HttpGenerator.Result result = generator.generateResponse(info, false, header, null, null, true);
+        assertSame(HttpGenerator.Result.FLUSH, result);
+        String headers = BufferUtil.toString(header);
+        assertThat(headers, containsString("Connection: other, close\r\n"));
+        assertThat(headers, not(containsString("keep-alive")));
+    }
 }
diff --git a/jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/modules/FormAuthModule.java b/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,7 +173,7 @@ public AuthStatus validateRequest(MessageInfo messageInfo, Subject clientSubject
                     }
 
                     response.setContentLength(0);
-                    response.sendRedirect(response.encodeRedirectURL(nuri));
+                    Request.getBaseRequest(request).getResponse().sendRedirect(HttpServletResponse.SC_MOVED_TEMPORARILY, nuri, true);
                     return AuthStatus.SEND_CONTINUE;
                 }
                 // not authenticated
@@ -187,7 +187,8 @@ public AuthStatus validateRequest(MessageInfo messageInfo, Subject clientSubject
                 else
                 {
                     response.setContentLength(0);
-                    response.sendRedirect(response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formErrorPage)));
+                    Request.getBaseRequest(request).getResponse().sendRedirect(HttpServletResponse.SC_MOVED_TEMPORARILY,
+                        response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formErrorPage)), true);
                 }
                 // TODO is this correct response if isMandatory false??? Can
                 // that occur?
@@ -229,14 +230,13 @@ public AuthStatus validateRequest(MessageInfo messageInfo, Subject clientSubject
             }
 
             response.setContentLength(0);
-            response.sendRedirect(response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formLoginPage)));
+            Request.getBaseRequest(request).getResponse().sendRedirect(
+                HttpServletResponse.SC_MOVED_TEMPORARILY,
+                response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formLoginPage)),
+                true);
             return AuthStatus.SEND_CONTINUE;
         }
-        catch (IOException e)
-        {
-            throw new AuthException(e.getMessage());
-        }
-        catch (UnsupportedCallbackException e)
+        catch (IOException | UnsupportedCallbackException e)
         {
             throw new AuthException(e.getMessage());
         }

diff --git a/jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdAuthenticator.java b/jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdAuthenticator.java
@@ -30,7 +30,6 @@
 import javax.servlet.http.HttpSession;
 
 import org.eclipse.jetty.http.HttpMethod;
-import org.eclipse.jetty.http.HttpVersion;
 import org.eclipse.jetty.http.MimeTypes;
 import org.eclipse.jetty.security.LoginService;
 import org.eclipse.jetty.security.ServerAuthException;
@@ -300,7 +299,7 @@ public Authentication validateRequest(ServletRequest req, ServletResponse res, b
                             LOG.debug("authenticated {}->{}", openIdAuth, nuri);
 
                         response.setContentLength(0);
-                        baseResponse.sendRedirect(getRedirectCode(baseRequest.getHttpVersion()), nuri);
+                        baseResponse.sendRedirect(nuri, true);
                         return openIdAuth;
                     }
                 }
@@ -392,7 +391,7 @@ public Authentication validateRequest(ServletRequest req, ServletResponse res, b
             String challengeUri = getChallengeUri(request);
             if (LOG.isDebugEnabled())
                 LOG.debug("challenge {}->{}", session.getId(), challengeUri);
-            baseResponse.sendRedirect(getRedirectCode(baseRequest.getHttpVersion()), challengeUri);
+            baseResponse.sendRedirect(challengeUri, true);
 
             return Authentication.SEND_CONTINUE;
         }
@@ -436,10 +435,9 @@ 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);
-                baseResponse.sendRedirect(getRedirectCode(baseRequest.getHttpVersion()), redirectUri);
             }
 
-            baseResponse.sendRedirect(getRedirectCode(baseRequest.getHttpVersion()), redirectUri);
+            baseResponse.sendRedirect(redirectUri, true);
         }
     }
 
@@ -461,12 +459,6 @@ public boolean isErrorPage(String pathInContext)
         return pathInContext != null && (pathInContext.equals(_errorPath));
     }
 
-    private static int getRedirectCode(HttpVersion httpVersion)
-    {
-        return (httpVersion.getVersion() < HttpVersion.HTTP_1_1.getVersion()
-            ? HttpServletResponse.SC_MOVED_TEMPORARILY : HttpServletResponse.SC_SEE_OTHER);
-    }
-
     private String getRedirectUri(HttpServletRequest request)
     {
         final StringBuffer redirectUri = new StringBuffer(128);

diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java b/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,7 +656,7 @@ protected boolean checkUserDataPermissions(String pathInContext, Request request
 
                 String url = URIUtil.newURI(scheme, request.getServerName(), port, request.getRequestURI(), request.getQueryString());
                 response.setContentLength(0);
-                response.sendRedirect(url);
+                response.sendRedirect(url, true);
             }
             else
                 response.sendError(HttpStatus.FORBIDDEN_403, "!Secure");

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
@@ -35,7 +35,6 @@
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpHeaderValue;
 import org.eclipse.jetty.http.HttpMethod;
-import org.eclipse.jetty.http.HttpVersion;
 import org.eclipse.jetty.http.MimeTypes;
 import org.eclipse.jetty.security.ServerAuthException;
 import org.eclipse.jetty.security.UserAuthentication;
@@ -291,8 +290,7 @@ public Authentication validateRequest(ServletRequest req, ServletResponse res, b
                     LOG.debug("authenticated {}->{}", formAuth, nuri);
 
                     response.setContentLength(0);
-                    int redirectCode = (baseRequest.getHttpVersion().getVersion() < HttpVersion.HTTP_1_1.getVersion() ? HttpServletResponse.SC_MOVED_TEMPORARILY : HttpServletResponse.SC_SEE_OTHER);
-                    baseResponse.sendRedirect(redirectCode, response.encodeRedirectURL(nuri));
+                    baseResponse.sendRedirect(response.encodeRedirectURL(nuri), true);
                     return formAuth;
                 }
 
@@ -316,8 +314,7 @@ else if (_dispatch)
                 else
                 {
                     LOG.debug("auth failed {}->{}", username, _formErrorPage);
-                    int redirectCode = (baseRequest.getHttpVersion().getVersion() < HttpVersion.HTTP_1_1.getVersion() ? HttpServletResponse.SC_MOVED_TEMPORARILY : HttpServletResponse.SC_SEE_OTHER);
-                    baseResponse.sendRedirect(redirectCode, response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formErrorPage)));
+                    baseResponse.sendRedirect(response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formErrorPage)), true);
                 }
 
                 return Authentication.SEND_FAILURE;
@@ -410,8 +407,7 @@ else if (_dispatch)
             else
             {
                 LOG.debug("challenge {}->{}", session.getId(), _formLoginPage);
-                int redirectCode = (baseRequest.getHttpVersion().getVersion() < HttpVersion.HTTP_1_1.getVersion() ? HttpServletResponse.SC_MOVED_TEMPORARILY : HttpServletResponse.SC_SEE_OTHER);
-                baseResponse.sendRedirect(redirectCode, response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formLoginPage)));
+                baseResponse.sendRedirect(response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formLoginPage)), true);
             }
             return Authentication.SEND_CONTINUE;
         }