From 0cdc78e3a6089d8021269ed7e480dab0ad21b896 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 10 Nov 2020 22:35:42 +0100 Subject: [PATCH 01/19] 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 --- .../org/eclipse/jetty/server/HttpChannel.java | 32 ++++++++++++++- .../jetty/server/ErrorHandlerTest.java | 40 +++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index feb2e36c76bc..bd867fce0f73 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/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,33 @@ 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); + + // Close the connection if we can't consume the input + if (!_request.getHttpInput().consumeAll()) + { + _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(", ")) + ",close"); + }); + } ContextHandler.Context context = (ContextHandler.Context)_request.getAttribute(ErrorHandler.ERROR_CONTEXT); ErrorHandler errorHandler = ErrorHandler.getErrorHandler(getServer(), context == null ? null : context.getContextHandler()); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java index 539266db9df4..4f104b390fb2 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java @@ -233,6 +233,46 @@ public void test404HtmlAccept() throws Exception assertContent(response); } + @Test + public void test404Post() throws Exception + { + String rawResponse = connector.getResponse( + "POST / HTTP/1.1\r\n" + + "Host: Localhost\r\n" + + "Accept: text/html\r\n" + + "Content-Length: 10\r\n" + + "\r\n" + + "0123456789"); + + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + + assertThat("Response status code", response.getStatus(), is(404)); + assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0)); + assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1")); + assertThat(response.getField(HttpHeader.CONNECTION), nullValue()); + assertContent(response); + } + + @Test + public void test404PostCantConsume() throws Exception + { + String rawResponse = connector.getResponse( + "POST / HTTP/1.1\r\n" + + "Host: Localhost\r\n" + + "Accept: text/html\r\n" + + "Content-Length: 100\r\n" + + "\r\n" + + "0123456789"); + + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + + assertThat("Response status code", response.getStatus(), is(404)); + assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0)); + assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1")); + assertThat(response.getField(HttpHeader.CONNECTION).getValue(), is("close")); + assertContent(response); + } + @Test public void testMoreSpecificAccept() throws Exception { From 4b934b5262c3dd1da794bf3bfe7e3f15e92912ea Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 11 Nov 2020 13:55:33 +0100 Subject: [PATCH 02/19] Update from review + Add close on all uncommitted requests when content cannot be consumed. --- .../org/eclipse/jetty/server/HttpChannel.java | 68 ++++++++++--------- .../jetty/server/HttpChannelState.java | 2 - .../jetty/server/AsyncRequestReadTest.java | 1 + 3 files changed, 38 insertions(+), 33 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index bd867fce0f73..4be9a8265fc6 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -414,29 +414,9 @@ public boolean handle() code = HttpStatus.INTERNAL_SERVER_ERROR_500; _response.setStatus(code); - // Close the connection if we can't consume the input + // Add Connection:close if we can't consume the input if (!_request.getHttpInput().consumeAll()) - { - _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(", ")) + ",close"); - }); - } + ensureConnectionClose(); ContextHandler.Context context = (ContextHandler.Context)_request.getAttribute(ErrorHandler.ERROR_CONTEXT); ErrorHandler errorHandler = ErrorHandler.getErrorHandler(getServer(), context == null ? null : context.getContextHandler()); @@ -522,10 +502,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; + } + // If content has not been consumed and we can't consume it now without blocking, then + // then ensure we signal that the connection will be closed. + if (!_request.getHttpInput().consumeAll()) + ensureConnectionClose(); } // RFC 7230, section 3.3. @@ -541,12 +529,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)); @@ -575,6 +558,29 @@ public boolean handle() return !suspended; } + private void ensureConnectionClose() + { + _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(", ")) + ",close"); + }); + } + private void dispatch(DispatcherType type, Dispatchable dispatchable) throws IOException, ServletException { try diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java index 4caf8106a1b7..be4849ae8fce 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java +++ b/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(); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncRequestReadTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncRequestReadTest.java index c3b86047569c..78f0b695b0f2 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncRequestReadTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncRequestReadTest.java @@ -328,6 +328,7 @@ public void testPartialReadThenClose() throws Exception BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream())); assertThat(in.readLine(), containsString("HTTP/1.1 200 OK")); + assertThat(in.readLine(), containsString("Connection: close")); assertThat(in.readLine(), containsString("Content-Length:")); assertThat(in.readLine(), containsString("Server:")); in.readLine(); From 6ea3c05b99a790c4be696e91742bf55ad455697b Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 11 Nov 2020 17:43:29 +0100 Subject: [PATCH 03/19] Update from review + fixed comment + space comma --- .../src/main/java/org/eclipse/jetty/server/HttpChannel.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index 4be9a8265fc6..f4241635cb2b 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -510,7 +510,7 @@ public boolean handle() _response.sendError(HttpStatus.NOT_FOUND_404); break; } - // If content has not been consumed and we can't consume it now without blocking, then + // If content has not been consumed and we can't consume it now without blocking // then ensure we signal that the connection will be closed. if (!_request.getHttpInput().consumeAll()) ensureConnectionClose(); @@ -577,7 +577,7 @@ private void ensureConnectionClose() return new HttpField(HttpHeader.CONNECTION, fields.stream() .flatMap(field -> Stream.of(field.getValues())) - .collect(Collectors.joining(", ")) + ",close"); + .collect(Collectors.joining(", ")) + ", " + HttpHeaderValue.CLOSE.asString()); }); } From 83ad9ccb0e3f8303a22d4234d3662c86602f01fe Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 11 Nov 2020 19:42:54 +0100 Subject: [PATCH 04/19] Only consume input in COMPLETE if response is >=200 (ie not an upgrade or similar) --- .../src/main/java/org/eclipse/jetty/server/HttpChannel.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index f4241635cb2b..5c157c95e0e0 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -512,7 +512,7 @@ public boolean handle() } // If content has not been consumed and we can't consume it now without blocking // then ensure we signal that the connection will be closed. - if (!_request.getHttpInput().consumeAll()) + if (_response.getStatus() >= 200 && !_request.getHttpInput().consumeAll()) ensureConnectionClose(); } From ef560df09d08922df2ee4200e5b4b53afd788c01 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 12 Nov 2020 12:44:35 +0100 Subject: [PATCH 05/19] 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 --- .../jaspi/modules/FormAuthModule.java | 5 +- .../security/openid/OpenIdAuthenticator.java | 4 + .../security/ConstraintSecurityHandler.java | 4 +- .../authentication/FormAuthenticator.java | 3 + .../jetty/security/ConstraintTest.java | 92 ++++++++++++++----- .../org/eclipse/jetty/server/HttpChannel.java | 22 ++--- .../jetty/server/handler/ContextHandler.java | 1 + .../handler/SecuredRedirectHandler.java | 1 + 8 files changed, 93 insertions(+), 39 deletions(-) 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 index eb8c3d997948..c6e9b45be122 100644 --- 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,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; } 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 index b606d9b585e4..ad24c6893968 100644 --- 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 @@ -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); } } 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 index 6a7e99cf8b99..d46c03d99724 100644 --- 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,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 diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java b/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java index d5d70eab4435..ab262c1a81f2 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java +++ b/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; diff --git a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java b/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java index 8d6af8c29b39..0abe8bf4e7bf 100644 --- a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java +++ b/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,9 +469,6 @@ public static Stream 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", @@ -478,9 +476,6 @@ public static Stream basicScenarios() ) )); -// 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 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 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 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 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 { diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index 5c157c95e0e0..d12e90695dec 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -415,8 +415,7 @@ public boolean handle() _response.setStatus(code); // Add Connection:close if we can't consume the input - if (!_request.getHttpInput().consumeAll()) - ensureConnectionClose(); + ensureContentConsumedOrConnectionClose(); ContextHandler.Context context = (ContextHandler.Context)_request.getAttribute(ErrorHandler.ERROR_CONTEXT); ErrorHandler errorHandler = ErrorHandler.getErrorHandler(getServer(), context == null ? null : context.getContextHandler()); @@ -502,18 +501,11 @@ public boolean handle() case COMPLETE: { - if (!_response.isCommitted()) + if (!_response.isCommitted() && !_request.isHandled() && !_response.getHttpOutput().isClosed()) { - if (!_request.isHandled() && !_response.getHttpOutput().isClosed()) - { - // The request was not actually handled - _response.sendError(HttpStatus.NOT_FOUND_404); - break; - } - // If content has not been consumed and we can't consume it now without blocking - // then ensure we signal that the connection will be closed. - if (_response.getStatus() >= 200 && !_request.getHttpInput().consumeAll()) - ensureConnectionClose(); + // The request was not actually handled + _response.sendError(HttpStatus.NOT_FOUND_404); + break; } // RFC 7230, section 3.3. @@ -558,8 +550,10 @@ public boolean handle() return !suspended; } - private void ensureConnectionClose() + public void ensureContentConsumedOrConnectionClose() { + if (_request.getHttpInput().consumeAll()) + return; _response.getHttpFields().computeField(HttpHeader.CONNECTION, (h, fields) -> { if (fields == null || fields.isEmpty()) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index f2cfab1c4f71..2216cd1b1173 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -1240,6 +1240,7 @@ public boolean checkContext(final String target, final Request baseRequest, fina { // context request must end with / baseRequest.setHandled(true); + baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose(); if (baseRequest.getQueryString() != null) response.sendRedirect(baseRequest.getRequestURI() + "/?" + baseRequest.getQueryString()); else diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/SecuredRedirectHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/SecuredRedirectHandler.java index a35796f27ba2..e0f0ec565506 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/SecuredRedirectHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/SecuredRedirectHandler.java @@ -62,6 +62,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques String url = URIUtil.newURI(scheme, baseRequest.getServerName(), port, baseRequest.getRequestURI(), baseRequest.getQueryString()); response.setContentLength(0); + baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose(); response.sendRedirect(url); } else From 8c0e4fe204022d17d4075e49e914de2abfc9ce27 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 12 Nov 2020 17:05:06 +0100 Subject: [PATCH 06/19] Updated to be less adventurous reverted test --- .../test/java/org/eclipse/jetty/server/AsyncRequestReadTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncRequestReadTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncRequestReadTest.java index 78f0b695b0f2..c3b86047569c 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncRequestReadTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncRequestReadTest.java @@ -328,7 +328,6 @@ public void testPartialReadThenClose() throws Exception BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream())); assertThat(in.readLine(), containsString("HTTP/1.1 200 OK")); - assertThat(in.readLine(), containsString("Connection: close")); assertThat(in.readLine(), containsString("Content-Length:")); assertThat(in.readLine(), containsString("Server:")); in.readLine(); From 727b3e18e98ea08a66090f8a282a399ad00fdf56 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Fri, 13 Nov 2020 18:43:03 -0600 Subject: [PATCH 07/19] Testcase for odd sendError(400) issue. Signed-off-by: Joakim Erdfelt --- .../jetty/test/GzipWithSendErrorTest.java | 166 ++++++++++++++++++ 1 file changed, 166 insertions(+) create mode 100644 tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java diff --git a/tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java b/tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java new file mode 100644 index 000000000000..2a6c0c938bac --- /dev/null +++ b/tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java @@ -0,0 +1,166 @@ +// +// ======================================================================== +// Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.test; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.net.URI; +import java.util.zip.GZIPOutputStream; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.client.util.BytesContentProvider; +import org.eclipse.jetty.http.HttpHeader; +import org.eclipse.jetty.http.HttpMethod; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.server.handler.gzip.GzipHandler; +import org.eclipse.jetty.servlet.ServletContextHandler; +import org.eclipse.jetty.util.IO; +import org.eclipse.jetty.util.component.LifeCycle; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.junit.jupiter.api.Assertions.assertEquals; + +public class GzipWithSendErrorTest +{ + private Server server; + private HttpClient client; + + @BeforeEach + public void setup() throws Exception + { + server = new Server(); + + ServerConnector connector = new ServerConnector(server); + connector.setPort(0); + server.addConnector(connector); + + GzipHandler gzipHandler = new GzipHandler(); + gzipHandler.setInflateBufferSize(4096); + + ServletContextHandler contextHandler = new ServletContextHandler(); + contextHandler.setContextPath("/"); + + contextHandler.addServlet(PostServlet.class, "/submit"); + contextHandler.addServlet(FailServlet.class, "/fail"); + + gzipHandler.setHandler(contextHandler); + server.setHandler(gzipHandler); + server.start(); + + client = new HttpClient(); + client.start(); + } + + @AfterEach + public void teardown() + { + LifeCycle.stop(client); + LifeCycle.stop(server); + } + + /** + * Make 3 requests on the same connection. + *

+ * Normal POST with 200 response, POST which results in 400, POST with 200 response. + *

+ */ + @Test + public void testGzipNormalErrorNormal() throws Exception + { + URI serverURI = server.getURI(); + + ContentResponse response; + + response = client.newRequest(serverURI.resolve("/submit")) + .method(HttpMethod.POST) + .header(HttpHeader.CONTENT_ENCODING, "gzip") + .header(HttpHeader.ACCEPT_ENCODING, "gzip") + .content(new BytesContentProvider("text/plain", compressed("normal-A"))) + .send(); + + assertEquals(200, response.getStatus(), "Response status on /submit (normal-A)"); + assertEquals("normal-A", response.getContentAsString(), "Response content on /submit (normal-A)"); + + response = client.newRequest(serverURI.resolve("/fail")) + .method(HttpMethod.POST) + .header(HttpHeader.CONTENT_ENCODING, "gzip") + .header(HttpHeader.ACCEPT_ENCODING, "gzip") + .content(new BytesContentProvider("text/plain", compressed("normal-B"))) + .send(); + + assertEquals(400, response.getStatus(), "Response status on /fail (normal-B)"); + assertThat("Response content on /fail (normal-B)", response.getContentAsString(), containsString("Error 400 Bad Request")); + + response = client.newRequest(serverURI.resolve("/submit")) + .method(HttpMethod.POST) + .header(HttpHeader.CONTENT_ENCODING, "gzip") + .header(HttpHeader.ACCEPT_ENCODING, "gzip") + .content(new BytesContentProvider("text/plain", compressed("normal-C"))) + .send(); + + assertEquals(200, response.getStatus(), "Response status on /submit (normal-C)"); + assertEquals("normal-C", response.getContentAsString(), "Response content on /submit (normal-C)"); + } + + private byte[] compressed(String content) throws IOException + { + try (ByteArrayOutputStream baos = new ByteArrayOutputStream(); + GZIPOutputStream gzipOut = new GZIPOutputStream(baos)) + { + gzipOut.write(content.getBytes(UTF_8)); + gzipOut.finish(); + return baos.toByteArray(); + } + } + + public static class PostServlet extends HttpServlet + { + @Override + protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException + { + resp.setCharacterEncoding("utf-8"); + resp.setContentType("text/plain"); + resp.setHeader("X-Servlet", req.getServletPath()); + + String reqBody = IO.toString(req.getInputStream(), UTF_8); + resp.getWriter().append(reqBody); + } + } + + public static class FailServlet extends HttpServlet + { + @Override + protected void service(HttpServletRequest req, HttpServletResponse resp) throws IOException + { + resp.setHeader("X-Servlet", req.getServletPath()); + // intentionally do not read request body here. + resp.sendError(400); + } + } +} From 59611a4661f527ae5273990f7f62301f2ffb1ea6 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Sat, 14 Nov 2020 11:59:02 +0100 Subject: [PATCH 08/19] Fix for odd sendError(400) issue. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/server/HttpInput.java | 18 +++++++++++++++--- .../handler/gzip/GzipHttpInputInterceptor.java | 6 ++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java index 3eabed665d39..4f04b8c32fa9 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java @@ -154,13 +154,14 @@ public void recycle() { synchronized (_inputQ) { - if (_content != null) - _content.failed(null); + Throwable failure = fail(_intercepted, null); + _intercepted = null; + failure = fail(_content, failure); _content = null; Content item = _inputQ.poll(); while (item != null) { - item.failed(null); + failure = fail(item, failure); item = _inputQ.poll(); } _listener = null; @@ -176,6 +177,17 @@ public void recycle() } } + private Throwable fail(Content content, Throwable failure) + { + if (content != null) + { + if (failure == null) + failure = new IOException("unconsumed input"); + content.failed(failure); + } + return failure; + } + /** * @return The current Interceptor, or null if none set */ diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpInputInterceptor.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpInputInterceptor.java index 2419f6e23d9a..1ebc4f7056a1 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpInputInterceptor.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpInputInterceptor.java @@ -55,6 +55,12 @@ public void succeeded() { _decoder.release(chunk); } + + @Override + public void failed(Throwable x) + { + _decoder.release(chunk); + } }; } From 7846e0fd39991aa714fdd0d8db91e2866760e19d Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Fri, 13 Nov 2020 18:43:03 -0600 Subject: [PATCH 09/19] Testcase for odd sendError(400) issue. Signed-off-by: Joakim Erdfelt --- .../jetty/test/GzipWithSendErrorTest.java | 166 ++++++++++++++++++ 1 file changed, 166 insertions(+) create mode 100644 tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java diff --git a/tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java b/tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java new file mode 100644 index 000000000000..2a6c0c938bac --- /dev/null +++ b/tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java @@ -0,0 +1,166 @@ +// +// ======================================================================== +// Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.test; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.net.URI; +import java.util.zip.GZIPOutputStream; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.client.util.BytesContentProvider; +import org.eclipse.jetty.http.HttpHeader; +import org.eclipse.jetty.http.HttpMethod; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.server.handler.gzip.GzipHandler; +import org.eclipse.jetty.servlet.ServletContextHandler; +import org.eclipse.jetty.util.IO; +import org.eclipse.jetty.util.component.LifeCycle; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.junit.jupiter.api.Assertions.assertEquals; + +public class GzipWithSendErrorTest +{ + private Server server; + private HttpClient client; + + @BeforeEach + public void setup() throws Exception + { + server = new Server(); + + ServerConnector connector = new ServerConnector(server); + connector.setPort(0); + server.addConnector(connector); + + GzipHandler gzipHandler = new GzipHandler(); + gzipHandler.setInflateBufferSize(4096); + + ServletContextHandler contextHandler = new ServletContextHandler(); + contextHandler.setContextPath("/"); + + contextHandler.addServlet(PostServlet.class, "/submit"); + contextHandler.addServlet(FailServlet.class, "/fail"); + + gzipHandler.setHandler(contextHandler); + server.setHandler(gzipHandler); + server.start(); + + client = new HttpClient(); + client.start(); + } + + @AfterEach + public void teardown() + { + LifeCycle.stop(client); + LifeCycle.stop(server); + } + + /** + * Make 3 requests on the same connection. + *

+ * Normal POST with 200 response, POST which results in 400, POST with 200 response. + *

+ */ + @Test + public void testGzipNormalErrorNormal() throws Exception + { + URI serverURI = server.getURI(); + + ContentResponse response; + + response = client.newRequest(serverURI.resolve("/submit")) + .method(HttpMethod.POST) + .header(HttpHeader.CONTENT_ENCODING, "gzip") + .header(HttpHeader.ACCEPT_ENCODING, "gzip") + .content(new BytesContentProvider("text/plain", compressed("normal-A"))) + .send(); + + assertEquals(200, response.getStatus(), "Response status on /submit (normal-A)"); + assertEquals("normal-A", response.getContentAsString(), "Response content on /submit (normal-A)"); + + response = client.newRequest(serverURI.resolve("/fail")) + .method(HttpMethod.POST) + .header(HttpHeader.CONTENT_ENCODING, "gzip") + .header(HttpHeader.ACCEPT_ENCODING, "gzip") + .content(new BytesContentProvider("text/plain", compressed("normal-B"))) + .send(); + + assertEquals(400, response.getStatus(), "Response status on /fail (normal-B)"); + assertThat("Response content on /fail (normal-B)", response.getContentAsString(), containsString("Error 400 Bad Request")); + + response = client.newRequest(serverURI.resolve("/submit")) + .method(HttpMethod.POST) + .header(HttpHeader.CONTENT_ENCODING, "gzip") + .header(HttpHeader.ACCEPT_ENCODING, "gzip") + .content(new BytesContentProvider("text/plain", compressed("normal-C"))) + .send(); + + assertEquals(200, response.getStatus(), "Response status on /submit (normal-C)"); + assertEquals("normal-C", response.getContentAsString(), "Response content on /submit (normal-C)"); + } + + private byte[] compressed(String content) throws IOException + { + try (ByteArrayOutputStream baos = new ByteArrayOutputStream(); + GZIPOutputStream gzipOut = new GZIPOutputStream(baos)) + { + gzipOut.write(content.getBytes(UTF_8)); + gzipOut.finish(); + return baos.toByteArray(); + } + } + + public static class PostServlet extends HttpServlet + { + @Override + protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException + { + resp.setCharacterEncoding("utf-8"); + resp.setContentType("text/plain"); + resp.setHeader("X-Servlet", req.getServletPath()); + + String reqBody = IO.toString(req.getInputStream(), UTF_8); + resp.getWriter().append(reqBody); + } + } + + public static class FailServlet extends HttpServlet + { + @Override + protected void service(HttpServletRequest req, HttpServletResponse resp) throws IOException + { + resp.setHeader("X-Servlet", req.getServletPath()); + // intentionally do not read request body here. + resp.sendError(400); + } + } +} From 66ea4cccf25f0b60225c1b77ec1176d9159d81cf Mon Sep 17 00:00:00 2001 From: gregw Date: Sat, 14 Nov 2020 14:09:54 +0100 Subject: [PATCH 10/19] Always try to consumeAll on all requests --- .../eclipse/jetty/server/HttpConnection.java | 25 +++---------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index 33ae537c3ca3..a7bfa6807bd6 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -408,28 +408,11 @@ public void onCompleted() // close to seek EOF _parser.close(); } - else if (_parser.inContentState() && _generator.isPersistent()) + else if (!_input.consumeAll()) { - // Try to progress without filling. - parseRequestBuffer(); - if (_parser.inContentState()) - { - // If we are async, then we have problems to complete neatly - if (_input.isAsync()) - { - if (LOG.isDebugEnabled()) - LOG.debug("{}unconsumed input while async {}", _parser.isChunking() ? "Possible " : "", this); - _channel.abort(new IOException("unconsumed input")); - } - else - { - if (LOG.isDebugEnabled()) - LOG.debug("{}unconsumed input {}", _parser.isChunking() ? "Possible " : "", this); - // Complete reading the request - if (!_input.consumeAll()) - _channel.abort(new IOException("unconsumed input")); - } - } + if (LOG.isDebugEnabled()) + LOG.debug("{}unconsumed input {}", _parser.isChunking() ? "Possible " : "", this); + _channel.abort(new IOException("unconsumed input")); } // Reset the channel, parsers and generator From c04f31b4897b2ebfdcde2169e9ef12fb749b9cd3 Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 16 Nov 2020 11:40:10 +0100 Subject: [PATCH 11/19] Refinements after testing in 10 --- .../org/eclipse/jetty/server/HttpChannel.java | 15 +++++++++++---- .../org/eclipse/jetty/server/HttpConnection.java | 3 ++- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index d12e90695dec..77a87f77dac8 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -501,11 +501,18 @@ public boolean handle() case COMPLETE: { - if (!_response.isCommitted() && !_request.isHandled() && !_response.getHttpOutput().isClosed()) + if (!_response.isCommitted()) { - // The request was not actually handled - _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. diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index a7bfa6807bd6..d48ab94835e0 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -408,7 +408,8 @@ public void onCompleted() // close to seek EOF _parser.close(); } - else if (!_input.consumeAll()) + // else abort if we can't consume all + else if (_generator.isPersistent() && !_input.consumeAll()) { if (LOG.isDebugEnabled()) LOG.debug("{}unconsumed input {}", _parser.isChunking() ? "Possible " : "", this); From 901e6dcdd6676ed745ee3853d787b90f5abe1020 Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 16 Nov 2020 11:46:14 +0100 Subject: [PATCH 12/19] Refinements after testing in 10 Fixed test --- .../test/java/org/eclipse/jetty/server/AsyncRequestReadTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncRequestReadTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncRequestReadTest.java index c3b86047569c..78f0b695b0f2 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncRequestReadTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncRequestReadTest.java @@ -328,6 +328,7 @@ public void testPartialReadThenClose() throws Exception BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream())); assertThat(in.readLine(), containsString("HTTP/1.1 200 OK")); + assertThat(in.readLine(), containsString("Connection: close")); assertThat(in.readLine(), containsString("Content-Length:")); assertThat(in.readLine(), containsString("Server:")); in.readLine(); From bdbd5d9dde092715bed1a985ab76d12502a92e34 Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 16 Nov 2020 13:10:04 +0100 Subject: [PATCH 13/19] Fixed comment from review --- .../src/main/java/org/eclipse/jetty/server/HttpChannel.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index 77a87f77dac8..f2a3b001cf94 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -414,7 +414,11 @@ public boolean handle() code = HttpStatus.INTERNAL_SERVER_ERROR_500; _response.setStatus(code); - // Add Connection:close if we can't consume the input + // The handling of the original dispatch failed and we are now going to either generate + // and error page ourselves or dispatch for an error page. If there is content left over + // from the failed dispatch, then we try to consume it here and if we fail we add a + // Connection:close. This can't be deferred to COMPLETE as the response will committed by + // then by this sendError handling. ensureContentConsumedOrConnectionClose(); ContextHandler.Context context = (ContextHandler.Context)_request.getAttribute(ErrorHandler.ERROR_CONTEXT); From cbaf3bfbd6399eab2c83ac44227bf1631e084837 Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 16 Nov 2020 15:02:31 +0100 Subject: [PATCH 14/19] Updates from review + added redirect methods that consumeAll + ensureContentConsumedOrConnectionClose renamed to ensureConsumeAllOrNotPersistent + ensureConsumeAllOrNotPersistent now handles HTTP/1.0 and HTTP/1.1 differently --- .../jaspi/modules/FormAuthModule.java | 19 ++--- .../security/openid/OpenIdAuthenticator.java | 18 +---- .../security/ConstraintSecurityHandler.java | 3 +- .../authentication/FormAuthenticator.java | 13 +--- .../org/eclipse/jetty/server/HttpChannel.java | 76 +++++++++++++------ .../eclipse/jetty/server/HttpConnection.java | 2 +- .../org/eclipse/jetty/server/Response.java | 30 ++++++++ .../jetty/server/handler/ContextHandler.java | 9 +-- .../handler/SecuredRedirectHandler.java | 3 +- .../jetty/server/ErrorHandlerTest.java | 46 ++++++++++- 10 files changed, 148 insertions(+), 71 deletions(-) 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 index c6e9b45be122..71e0243bda55 100644 --- 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 @@ -173,8 +173,7 @@ public AuthStatus validateRequest(MessageInfo messageInfo, Subject clientSubject } response.setContentLength(0); - Request.getBaseRequest(request).getHttpChannel().ensureContentConsumedOrConnectionClose(); - response.sendRedirect(response.encodeRedirectURL(nuri)); + Request.getBaseRequest(request).getResponse().sendRedirect(HttpServletResponse.SC_MOVED_TEMPORARILY, nuri, true); return AuthStatus.SEND_CONTINUE; } // not authenticated @@ -188,8 +187,8 @@ 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))); + 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? @@ -231,15 +230,13 @@ public AuthStatus validateRequest(MessageInfo messageInfo, Subject clientSubject } response.setContentLength(0); - Request.getBaseRequest(request).getHttpChannel().ensureContentConsumedOrConnectionClose(); - 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 index ad24c6893968..030a2211b9c6 100644 --- 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,8 +299,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); + baseResponse.sendRedirect(nuri, true); return openIdAuth; } } @@ -393,8 +391,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); + baseResponse.sendRedirect(challengeUri, true); return Authentication.SEND_CONTINUE; } @@ -438,12 +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); - baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose(); - baseResponse.sendRedirect(getRedirectCode(baseRequest.getHttpVersion()), redirectUri); } - baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose(); - baseResponse.sendRedirect(getRedirectCode(baseRequest.getHttpVersion()), redirectUri); + baseResponse.sendRedirect(redirectUri, true); } } @@ -465,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 index d46c03d99724..97d0013e16f3 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java +++ b/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java @@ -656,8 +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); + response.sendRedirect(url, true); } else response.sendError(HttpStatus.FORBIDDEN_403, "!Secure"); diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java b/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java index ab262c1a81f2..4829f666c657 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java +++ b/jetty-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,9 +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); - baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose(); - baseResponse.sendRedirect(redirectCode, response.encodeRedirectURL(nuri)); + baseResponse.sendRedirect(response.encodeRedirectURL(nuri), true); return formAuth; } @@ -317,9 +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); - baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose(); - baseResponse.sendRedirect(redirectCode, response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formErrorPage))); + baseResponse.sendRedirect(response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formErrorPage)), true); } return Authentication.SEND_FAILURE; @@ -412,9 +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); - baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose(); - baseResponse.sendRedirect(redirectCode, response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formLoginPage))); + baseResponse.sendRedirect(response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formLoginPage)), true); } return Authentication.SEND_CONTINUE; } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index f2a3b001cf94..94e2e223096d 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -58,6 +58,7 @@ import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.SharedBlockingCallback.Blocker; +import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; import org.eclipse.jetty.util.thread.Scheduler; @@ -415,11 +416,11 @@ public boolean handle() _response.setStatus(code); // The handling of the original dispatch failed and we are now going to either generate - // and error page ourselves or dispatch for an error page. If there is content left over + // and error response ourselves or dispatch for an error page. If there is content left over // from the failed dispatch, then we try to consume it here and if we fail we add a - // Connection:close. This can't be deferred to COMPLETE as the response will committed by - // then by this sendError handling. - ensureContentConsumedOrConnectionClose(); + // Connection:close. This can't be deferred to COMPLETE as the response will be committed + // by then. + ensureConsumeAllOrNotPersistent(); ContextHandler.Context context = (ContextHandler.Context)_request.getAttribute(ErrorHandler.ERROR_CONTEXT); ErrorHandler errorHandler = ErrorHandler.getErrorHandler(getServer(), context == null ? null : context.getContextHandler()); @@ -516,7 +517,7 @@ public boolean handle() // Indicate Connection:close if we can't consume all. if (_response.getStatus() >= 200) - ensureContentConsumedOrConnectionClose(); + ensureConsumeAllOrNotPersistent(); } // RFC 7230, section 3.3. @@ -561,29 +562,58 @@ public boolean handle() return !suspended; } - public void ensureContentConsumedOrConnectionClose() + public void ensureConsumeAllOrNotPersistent() { - if (_request.getHttpInput().consumeAll()) - return; - _response.getHttpFields().computeField(HttpHeader.CONNECTION, (h, fields) -> + switch (_request.getHttpVersion()) { - if (fields == null || fields.isEmpty()) - return HttpConnection.CONNECTION_CLOSE; + case HTTP_1_0: + if (_request.getHttpInput().consumeAll()) + return; - if (fields.stream().anyMatch(f -> f.contains(HttpHeaderValue.CLOSE.asString()))) - { - if (fields.size() == 1) - return fields.get(0); + // Remove any keep-alive value in Connection headers + _response.getHttpFields().computeField(HttpHeader.CONNECTION, (h, fields) -> + { + if (fields == null || fields.isEmpty()) + return null; + String v = fields.stream() + .flatMap(field -> Stream.of(field.getValues()).filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s))) + .collect(Collectors.joining(", ")); + if (StringUtil.isEmpty(v)) + return null; + + return new HttpField(HttpHeader.CONNECTION, v); + }); + break; - return new HttpField(HttpHeader.CONNECTION, fields.stream() - .flatMap(field -> Stream.of(field.getValues())) - .collect(Collectors.joining(", "))); - } + case HTTP_1_1: + if (_request.getHttpInput().consumeAll()) + return; - return new HttpField(HttpHeader.CONNECTION, fields.stream() - .flatMap(field -> Stream.of(field.getValues())) - .collect(Collectors.joining(", ")) + ", " + HttpHeaderValue.CLOSE.asString()); - }); + // Add close value to Connection headers + _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()); + }); + break; + + default: + break; + } } private void dispatch(DispatcherType type, Dispatchable dispatchable) throws IOException, ServletException diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index d48ab94835e0..87dc0893c4ca 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -412,7 +412,7 @@ public void onCompleted() else if (_generator.isPersistent() && !_input.consumeAll()) { if (LOG.isDebugEnabled()) - LOG.debug("{}unconsumed input {}", _parser.isChunking() ? "Possible " : "", this); + LOG.debug("unconsumed input {} {}", this, _parser); _channel.abort(new IOException("unconsumed input")); } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java index 7d82d5318097..d366d487d262 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java @@ -497,6 +497,36 @@ public void sendProcessing() throws IOException */ public void sendRedirect(int code, String location) throws IOException { + sendRedirect(code, location, false); + } + + /** + * Sends a response with one of the 300 series redirection codes. + * + * @param location the location to send in {@code Location} headers + * @param consumeAll if True, consume any HTTP/1 request input before doing the redirection. If the input cannot + * be consumed without blocking, then add a `Connection: close` header to the response. + * @throws IOException if unable to send the redirect + */ + public void sendRedirect(String location, boolean consumeAll) throws IOException + { + sendRedirect(getHttpChannel().getRequest().getHttpVersion().getVersion() < HttpVersion.HTTP_1_1.getVersion() + ? HttpServletResponse.SC_MOVED_TEMPORARILY : HttpServletResponse.SC_SEE_OTHER, location, consumeAll); + } + + /** + * Sends a response with one of the 300 series redirection codes. + * + * @param code the redirect status code + * @param location the location to send in {@code Location} headers + * @param consumeAll if True, consume any HTTP/1 request input before doing the redirection. If the input cannot + * be consumed without blocking, then add a `Connection: close` header to the response. + * @throws IOException if unable to send the redirect + */ + public void sendRedirect(int code, String location, boolean consumeAll) throws IOException + { + if (consumeAll) + getHttpChannel().ensureConsumeAllOrNotPersistent(); if ((code < HttpServletResponse.SC_MULTIPLE_CHOICES) || (code >= HttpServletResponse.SC_BAD_REQUEST)) throw new IllegalArgumentException("Not a 3xx redirect code"); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index 2216cd1b1173..d28d94297d29 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -1240,11 +1240,10 @@ public boolean checkContext(final String target, final Request baseRequest, fina { // context request must end with / baseRequest.setHandled(true); - baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose(); - if (baseRequest.getQueryString() != null) - response.sendRedirect(baseRequest.getRequestURI() + "/?" + baseRequest.getQueryString()); - else - response.sendRedirect(baseRequest.getRequestURI() + "/"); + baseRequest.getResponse().sendRedirect( + HttpServletResponse.SC_MOVED_TEMPORARILY, + baseRequest.getRequestURI() + (baseRequest.getQueryString() == null ? "/" : ("/?" + baseRequest.getQueryString())), + true); return false; } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/SecuredRedirectHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/SecuredRedirectHandler.java index e0f0ec565506..204feae76505 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/SecuredRedirectHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/SecuredRedirectHandler.java @@ -62,8 +62,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques String url = URIUtil.newURI(scheme, baseRequest.getServerName(), port, baseRequest.getRequestURI(), baseRequest.getQueryString()); response.setContentLength(0); - baseRequest.getHttpChannel().ensureContentConsumedOrConnectionClose(); - response.sendRedirect(url); + baseRequest.getResponse().sendRedirect(HttpServletResponse.SC_MOVED_TEMPORARILY, url, true); } else { diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java index 4f104b390fb2..1e241d0ddf30 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java @@ -234,7 +234,28 @@ public void test404HtmlAccept() throws Exception } @Test - public void test404Post() throws Exception + public void test404PostHttp10() throws Exception + { + String rawResponse = connector.getResponse( + "POST / HTTP/1.0\r\n" + + "Host: Localhost\r\n" + + "Accept: text/html\r\n" + + "Content-Length: 10\r\n" + + "Connection: keep-alive\r\n" + + "\r\n" + + "0123456789"); + + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + + assertThat("Response status code", response.getStatus(), is(404)); + assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0)); + assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1")); + assertThat("Response Content-Type", response.get(HttpHeader.CONNECTION), is("keep-alive")); + assertContent(response); + } + + @Test + public void test404PostHttp11() throws Exception { String rawResponse = connector.getResponse( "POST / HTTP/1.1\r\n" + @@ -254,7 +275,28 @@ public void test404Post() throws Exception } @Test - public void test404PostCantConsume() throws Exception + public void test404PostCantConsumeHttp10() throws Exception + { + String rawResponse = connector.getResponse( + "POST / HTTP/1.0\r\n" + + "Host: Localhost\r\n" + + "Accept: text/html\r\n" + + "Content-Length: 100\r\n" + + "Connection: keep-alive\r\n" + + "\r\n" + + "0123456789"); + + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + + assertThat("Response status code", response.getStatus(), is(404)); + assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0)); + assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1")); + assertThat(response.getField(HttpHeader.CONNECTION).getValue(), nullValue()); + assertContent(response); + } + + @Test + public void test404PostCantConsumeHttp11() throws Exception { String rawResponse = connector.getResponse( "POST / HTTP/1.1\r\n" + From 9c6d60d6932ff266404930661ae09d9eb1881b93 Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 16 Nov 2020 17:27:47 +0100 Subject: [PATCH 15/19] better consumeAll implementation --- .../org/eclipse/jetty/server/HttpInput.java | 49 ++++++++++++------- .../jetty/server/ErrorHandlerTest.java | 28 +++++------ .../eclipse/jetty/server/HttpInputTest.java | 6 ++- 3 files changed, 50 insertions(+), 33 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java index 3eabed665d39..a5edfca1a105 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java @@ -672,29 +672,44 @@ public boolean eof() public boolean consumeAll() { - synchronized (_inputQ) + while (true) { - try + synchronized (_inputQ) { - while (true) + if (_intercepted != null) { - Content item = nextContent(); - if (item == null) - break; // Let's not bother blocking + _intercepted.skip(_intercepted.remaining()); + consume(_intercepted); + } - skip(item, item.remaining()); + if (_content != null) + { + _content.skip(_content.remaining()); + consume(_content); } - if (isFinished()) - return !isError(); - _state = EARLY_EOF; - return false; - } - catch (Throwable e) - { - LOG.debug(e); - _state = new ErrorState(e); - return false; + Content content = _inputQ.poll(); + while (content != null) + { + consume(content); + content = _inputQ.poll(); + } + + if (_state instanceof EOFState) + return !(_state instanceof ErrorState); + + try + { + produceContent(); + if (_content == null && _intercepted == null && _inputQ.isEmpty()) + return false; + } + catch (Throwable e) + { + LOG.debug(e); + _state = new ErrorState(e); + return false; + } } } } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java index 1e241d0ddf30..11faa3fc8037 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java @@ -247,10 +247,10 @@ public void test404PostHttp10() throws Exception HttpTester.Response response = HttpTester.parseResponse(rawResponse); - assertThat("Response status code", response.getStatus(), is(404)); - assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0)); - assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1")); - assertThat("Response Content-Type", response.get(HttpHeader.CONNECTION), is("keep-alive")); + assertThat(response.getStatus(), is(404)); + assertThat(response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0)); + assertThat(response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1")); + assertThat(response.get(HttpHeader.CONNECTION), is("keep-alive")); assertContent(response); } @@ -267,9 +267,9 @@ public void test404PostHttp11() throws Exception HttpTester.Response response = HttpTester.parseResponse(rawResponse); - assertThat("Response status code", response.getStatus(), is(404)); - assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0)); - assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1")); + assertThat(response.getStatus(), is(404)); + assertThat(response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0)); + assertThat(response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1")); assertThat(response.getField(HttpHeader.CONNECTION), nullValue()); assertContent(response); } @@ -288,10 +288,10 @@ public void test404PostCantConsumeHttp10() throws Exception HttpTester.Response response = HttpTester.parseResponse(rawResponse); - assertThat("Response status code", response.getStatus(), is(404)); - assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0)); - assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1")); - assertThat(response.getField(HttpHeader.CONNECTION).getValue(), nullValue()); + assertThat(response.getStatus(), is(404)); + assertThat(response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0)); + assertThat(response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1")); + assertThat(response.getField(HttpHeader.CONNECTION), nullValue()); assertContent(response); } @@ -308,9 +308,9 @@ public void test404PostCantConsumeHttp11() throws Exception HttpTester.Response response = HttpTester.parseResponse(rawResponse); - assertThat("Response status code", response.getStatus(), is(404)); - assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0)); - assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1")); + assertThat(response.getStatus(), is(404)); + assertThat(response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0)); + assertThat(response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1")); assertThat(response.getField(HttpHeader.CONNECTION).getValue(), is("close")); assertContent(response); } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpInputTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpInputTest.java index 809d2c3acb02..134a6af81c51 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpInputTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpInputTest.java @@ -498,7 +498,8 @@ public void testConsumeAll() throws Exception assertThat(_in.read(), equalTo((int)'A')); assertFalse(_in.consumeAll()); - assertThat(_in.getContentConsumed(), equalTo(8L)); + assertThat(_in.getContentConsumed(), equalTo(1L)); + assertThat(_in.getContentReceived(), equalTo(8L)); assertThat(_history.poll(), equalTo("Content succeeded AB")); assertThat(_history.poll(), equalTo("Content succeeded CD")); @@ -520,7 +521,8 @@ public void testConsumeAllEOF() throws Exception assertThat(_in.read(), equalTo((int)'A')); assertTrue(_in.consumeAll()); - assertThat(_in.getContentConsumed(), equalTo(8L)); + assertThat(_in.getContentConsumed(), equalTo(1L)); + assertThat(_in.getContentReceived(), equalTo(8L)); assertThat(_history.poll(), equalTo("Content succeeded AB")); assertThat(_history.poll(), equalTo("Content succeeded CD")); From 5daecad494a70d12262c71f9a07d4dec11cabd2a Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 17 Nov 2020 10:10:27 +0100 Subject: [PATCH 16/19] update from review + better javadoc + filter out keep-alive + added more tests --- .../org/eclipse/jetty/http/HttpGenerator.java | 9 ++- .../org/eclipse/jetty/server/HttpChannel.java | 16 +++-- .../org/eclipse/jetty/server/Response.java | 4 +- .../jetty/server/handler/ContextHandler.java | 3 +- .../jetty/server/ErrorHandlerTest.java | 2 + .../eclipse/jetty/server/ResponseTest.java | 72 +++++++++++++++++++ 6 files changed, 93 insertions(+), 13 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 index 8be21bff0d3c..5a8a347c9085 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java @@ -659,16 +659,15 @@ else if (contentLength != field.getLongValue()) case CONNECTION: { putTo(field, header); + if (info.getHttpVersion() == HttpVersion.HTTP_1_0 && _persistent == null && field.contains(HttpHeaderValue.KEEP_ALIVE.asString())) + { + _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())) - { - _persistent = true; - } break; } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index 94e2e223096d..c98bd5a3f4b0 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -598,16 +598,22 @@ public void ensureConsumeAllOrNotPersistent() if (fields.stream().anyMatch(f -> f.contains(HttpHeaderValue.CLOSE.asString()))) { if (fields.size() == 1) - return fields.get(0); + { + HttpField f = fields.get(0); + if (HttpConnection.CONNECTION_CLOSE.equals(f)) + return f; + } return new HttpField(HttpHeader.CONNECTION, fields.stream() - .flatMap(field -> Stream.of(field.getValues())) + .flatMap(field -> Stream.of(field.getValues()).filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s))) .collect(Collectors.joining(", "))); } - return new HttpField(HttpHeader.CONNECTION, fields.stream() - .flatMap(field -> Stream.of(field.getValues())) - .collect(Collectors.joining(", ")) + ", " + HttpHeaderValue.CLOSE.asString()); + return new HttpField(HttpHeader.CONNECTION, + Stream.concat(fields.stream() + .flatMap(field -> Stream.of(field.getValues()).filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s))), + Stream.of(HttpHeaderValue.CLOSE.asString())) + .collect(Collectors.joining(", "))); }); break; diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java index d366d487d262..99615391f8f2 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java @@ -501,7 +501,7 @@ public void sendRedirect(int code, String location) throws IOException } /** - * Sends a response with one of the 300 series redirection codes. + * Sends a response with a HTTP version appropriate 30x redirection. * * @param location the location to send in {@code Location} headers * @param consumeAll if True, consume any HTTP/1 request input before doing the redirection. If the input cannot @@ -515,7 +515,7 @@ public void sendRedirect(String location, boolean consumeAll) throws IOException } /** - * Sends a response with one of the 300 series redirection codes. + * Sends a response with a given redirection code. * * @param code the redirect status code * @param location the location to send in {@code Location} headers diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index d28d94297d29..f917aad6c960 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -1240,9 +1240,10 @@ public boolean checkContext(final String target, final Request baseRequest, fina { // context request must end with / baseRequest.setHandled(true); + String queryString = baseRequest.getQueryString(); baseRequest.getResponse().sendRedirect( HttpServletResponse.SC_MOVED_TEMPORARILY, - baseRequest.getRequestURI() + (baseRequest.getQueryString() == null ? "/" : ("/?" + baseRequest.getQueryString())), + baseRequest.getRequestURI() + (queryString == null ? "/" : ("/?" + queryString)), true); return false; } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java index 11faa3fc8037..d79c8624aee4 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java @@ -262,6 +262,7 @@ public void test404PostHttp11() throws Exception "Host: Localhost\r\n" + "Accept: text/html\r\n" + "Content-Length: 10\r\n" + + "Connection: keep-alive\r\n" + // This is not need by HTTP/1.1 but sometimes sent anyway "\r\n" + "0123456789"); @@ -303,6 +304,7 @@ public void test404PostCantConsumeHttp11() throws Exception "Host: Localhost\r\n" + "Accept: text/html\r\n" + "Content-Length: 100\r\n" + + "Connection: keep-alive\r\n" + // This is not need by HTTP/1.1 but sometimes sent anyway "\r\n" + "0123456789"); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java index 1cabd60b38db..8be5277d4e74 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java @@ -1293,6 +1293,78 @@ public void testFlushAfterFullContent() throws Exception output.flush(); } + @Test + public void testEnsureConsumeAllOrNotPersistentHttp10() throws Exception + { + Response response = getResponse(); + response.getHttpChannel().getRequest().setHttpVersion(HttpVersion.HTTP_1_0); + response.getHttpChannel().ensureConsumeAllOrNotPersistent(); + assertThat(response.getHttpFields().get(HttpHeader.CONNECTION), nullValue()); + + response = getResponse(); + response.getHttpChannel().getRequest().setHttpVersion(HttpVersion.HTTP_1_0); + response.setHeader(HttpHeader.CONNECTION, "keep-alive"); + response.getHttpChannel().ensureConsumeAllOrNotPersistent(); + assertThat(response.getHttpFields().get(HttpHeader.CONNECTION), nullValue()); + + response = getResponse(); + response.getHttpChannel().getRequest().setHttpVersion(HttpVersion.HTTP_1_0); + response.setHeader(HttpHeader.CONNECTION, "before"); + response.getHttpFields().add(HttpHeader.CONNECTION, "foo, keep-alive, bar"); + response.getHttpFields().add(HttpHeader.CONNECTION, "after"); + response.getHttpChannel().ensureConsumeAllOrNotPersistent(); + assertThat(response.getHttpFields().get(HttpHeader.CONNECTION), is("before, foo, bar, after")); + + response = getResponse(); + response.getHttpChannel().getRequest().setHttpVersion(HttpVersion.HTTP_1_0); + response.setHeader(HttpHeader.CONNECTION, "close"); + response.getHttpChannel().ensureConsumeAllOrNotPersistent(); + assertThat(response.getHttpFields().get(HttpHeader.CONNECTION), is("close")); + } + + @Test + public void testEnsureConsumeAllOrNotPersistentHttp11() throws Exception + { + Response response = getResponse(); + response.getHttpChannel().getRequest().setHttpVersion(HttpVersion.HTTP_1_1); + response.getHttpChannel().ensureConsumeAllOrNotPersistent(); + assertThat(response.getHttpFields().get(HttpHeader.CONNECTION), is("close")); + + response = getResponse(); + response.getHttpChannel().getRequest().setHttpVersion(HttpVersion.HTTP_1_1); + response.setHeader(HttpHeader.CONNECTION, "keep-alive"); + response.getHttpChannel().ensureConsumeAllOrNotPersistent(); + assertThat(response.getHttpFields().get(HttpHeader.CONNECTION), is("close")); + + response = getResponse(); + response.getHttpChannel().getRequest().setHttpVersion(HttpVersion.HTTP_1_1); + response.setHeader(HttpHeader.CONNECTION, "close"); + response.getHttpChannel().ensureConsumeAllOrNotPersistent(); + assertThat(response.getHttpFields().get(HttpHeader.CONNECTION), is("close")); + + response = getResponse(); + response.getHttpChannel().getRequest().setHttpVersion(HttpVersion.HTTP_1_1); + response.setHeader(HttpHeader.CONNECTION, "before, close, after"); + response.getHttpChannel().ensureConsumeAllOrNotPersistent(); + assertThat(response.getHttpFields().get(HttpHeader.CONNECTION), is("before, close, after")); + + response = getResponse(); + response.getHttpChannel().getRequest().setHttpVersion(HttpVersion.HTTP_1_1); + response.setHeader(HttpHeader.CONNECTION, "before"); + response.getHttpFields().add(HttpHeader.CONNECTION, "middle, close"); + response.getHttpFields().add(HttpHeader.CONNECTION, "after"); + response.getHttpChannel().ensureConsumeAllOrNotPersistent(); + assertThat(response.getHttpFields().get(HttpHeader.CONNECTION), is("before, middle, close, after")); + + response = getResponse(); + response.getHttpChannel().getRequest().setHttpVersion(HttpVersion.HTTP_1_1); + response.setHeader(HttpHeader.CONNECTION, "one"); + response.getHttpFields().add(HttpHeader.CONNECTION, "two"); + response.getHttpFields().add(HttpHeader.CONNECTION, "three"); + response.getHttpChannel().ensureConsumeAllOrNotPersistent(); + assertThat(response.getHttpFields().get(HttpHeader.CONNECTION), is("one, two, three, close")); + } + private Response getResponse() { _channel.recycle(); From 578a961a38d5bbec61341e62ca83d72105c20a66 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 17 Nov 2020 10:15:06 +0100 Subject: [PATCH 17/19] update from review + better javadoc --- .../src/main/java/org/eclipse/jetty/server/HttpInput.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java index a5edfca1a105..2fe1b0f2005b 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java @@ -670,6 +670,12 @@ public boolean eof() return addContent(EOF_CONTENT); } + /** + * Consume all available content without blocking. + * Raw content is counted in the {@link #getContentReceived()} statistics, but + * is not intercepted nor counted in the {@link #getContentConsumed()} statistics + * @return True if EOF was reached, false otherwise. + */ public boolean consumeAll() { while (true) From 9a5f987418f4bf4eb39535f24e0a9819569a000f Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 17 Nov 2020 10:39:31 +0100 Subject: [PATCH 18/19] update from review + fixed form redirection test for http 1.0 and 1.1 --- .../jetty/security/ConstraintTest.java | 34 ++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java b/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java index 0abe8bf4e7bf..9dfd30810200 100644 --- a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java +++ b/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java @@ -1029,27 +1029,59 @@ public void testFormPostRedirect() throws Exception } @Test - public void testNonFormPostRedirect() throws Exception + public void testNonFormPostRedirectHttp10() 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" + + "Connection: keep-alive\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"))); + assertThat(response, containsString("Connection: keep-alive")); response = _connector.getResponse("POST /ctx/auth/info HTTP/1.0\r\n" + + "Host: localhost\r\n" + "Content-Type: text/plain\r\n" + + "Connection: keep-alive\r\n" + "Content-Length: 10\r\n" + "\r\n" + "012345\r\n"); assertThat(response, containsString(" 302 Found")); assertThat(response, containsString("/ctx/testLoginPage")); + assertThat(response, not(containsString("Connection: keep-alive"))); + } + + @Test + public void testNonFormPostRedirectHttp11() throws Exception + { + _security.setAuthenticator(new FormAuthenticator("/testLoginPage", "/testErrorPage", false)); + _server.start(); + + String response = _connector.getResponse("POST /ctx/auth/info HTTP/1.1\r\n" + + "Host: test\r\n" + + "Content-Type: text/plain\r\n" + + "Content-Length: 10\r\n" + + "\r\n" + + "0123456789\r\n"); + assertThat(response, containsString(" 303 See Other")); + assertThat(response, containsString("/ctx/testLoginPage")); + assertThat(response, not(containsString("Connection: close"))); + + response = _connector.getResponse("POST /ctx/auth/info HTTP/1.1\r\n" + + "Host: test\r\n" + + "Host: localhost\r\n" + + "Content-Type: text/plain\r\n" + + "Content-Length: 10\r\n" + + "\r\n" + + "012345\r\n"); + assertThat(response, containsString(" 303 See Other")); + assertThat(response, containsString("/ctx/testLoginPage")); assertThat(response, containsString("Connection: close")); } From bff386b9a5d9fce5aca86a0c6428053fb9350324 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 17 Nov 2020 15:18:07 +0100 Subject: [PATCH 19/19] update from review + HttpGenerator removes keep-alive if close present + Use isRedirection --- .../org/eclipse/jetty/http/HttpGenerator.java | 13 +++++++++++-- .../jetty/http/HttpGeneratorServerTest.java | 16 ++++++++++++++++ .../java/org/eclipse/jetty/server/Response.java | 2 +- 3 files changed, 28 insertions(+), 3 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 index 5a8a347c9085..53b75d3c0823 100644 --- 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,8 +660,8 @@ else if (contentLength != field.getLongValue()) case CONNECTION: { - putTo(field, header); - if (info.getHttpVersion() == HttpVersion.HTTP_1_0 && _persistent == null && field.contains(HttpHeaderValue.KEEP_ALIVE.asString())) + boolean keepAlive = field.contains(HttpHeaderValue.KEEP_ALIVE.asString()); + if (keepAlive && info.getHttpVersion() == HttpVersion.HTTP_1_0 && _persistent == null) { _persistent = true; } @@ -668,6 +670,13 @@ else if (contentLength != field.getLongValue()) close = true; _persistent = false; } + if (keepAlive && _persistent == Boolean.FALSE) + { + 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 index bb32ae7331aa..eff069f1c301 100644 --- 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-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java index 99615391f8f2..9866eee125de 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java @@ -527,7 +527,7 @@ public void sendRedirect(int code, String location, boolean consumeAll) throws I { if (consumeAll) getHttpChannel().ensureConsumeAllOrNotPersistent(); - if ((code < HttpServletResponse.SC_MULTIPLE_CHOICES) || (code >= HttpServletResponse.SC_BAD_REQUEST)) + if (!HttpStatus.isRedirection(code)) throw new IllegalArgumentException("Not a 3xx redirect code"); if (!isMutable())