Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Issue #5605 unconsumed input on sendError #5637

Merged
merged 22 commits into from Nov 18, 2020
Merged

Issue #5605 unconsumed input on sendError #5637

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
0cdc78e
Issue #5605 unconsumed input on sendError
gregw Nov 10, 2020
368b071
Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-56…
gregw Nov 11, 2020
4b934b5
Update from review
gregw Nov 11, 2020
6ea3c05
Update from review
gregw Nov 11, 2020
83ad9cc
Only consume input in COMPLETE if response is >=200 (ie not an upgrad…
gregw Nov 11, 2020
ef560df
Updated to be less adventurous
gregw Nov 12, 2020
8c0e4fe
Updated to be less adventurous
gregw Nov 12, 2020
727b3e1
Testcase for odd sendError(400) issue.
joakime Nov 14, 2020
59611a4
Fix for odd sendError(400) issue.
sbordet Nov 14, 2020
7846e0f
Testcase for odd sendError(400) issue.
joakime Nov 14, 2020
66ea4cc
Always try to consumeAll on all requests
gregw Nov 14, 2020
0518273
Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-56…
gregw Nov 16, 2020
c04f31b
Refinements after testing in 10
gregw Nov 16, 2020
901e6dc
Refinements after testing in 10
gregw Nov 16, 2020
bdbd5d9
Fixed comment from review
gregw Nov 16, 2020
cbaf3bf
Updates from review
gregw Nov 16, 2020
9c6d60d
better consumeAll implementation
gregw Nov 16, 2020
5daecad
update from review
gregw Nov 17, 2020
578a961
update from review
gregw Nov 17, 2020
9a5f987
update from review
gregw Nov 17, 2020
bff386b
update from review
gregw Nov 17, 2020
f3a5868
Merge remote-tracking branch 'origin/jetty-9.4.x-400-gzip' into jetty…
gregw Nov 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 4 additions & 5 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java
View file
Open in desktop
Expand Up @@ -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;
}
gregw marked this conversation as resolved.
Show resolved Hide resolved

if (info.getHttpVersion() == HttpVersion.HTTP_1_0 && _persistent == null && field.contains(HttpHeaderValue.KEEP_ALIVE.asString()))
{
_persistent = true;
}
break;
}

Expand Down
18 changes: 9 additions & 9 deletions jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/modules/FormAuthModule.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -173,7 +173,7 @@ public AuthStatus validateRequest(MessageInfo messageInfo, Subject clientSubject
}

response.setContentLength(0);
response.sendRedirect(response.encodeRedirectURL(nuri));
Request.getBaseRequest(request).getResponse().sendRedirect(HttpServletResponse.SC_MOVED_TEMPORARILY, nuri, true);
return AuthStatus.SEND_CONTINUE;
}
// not authenticated
Expand All @@ -187,7 +187,8 @@ public AuthStatus validateRequest(MessageInfo messageInfo, Subject clientSubject
else
{
response.setContentLength(0);
response.sendRedirect(response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formErrorPage)));
Request.getBaseRequest(request).getResponse().sendRedirect(HttpServletResponse.SC_MOVED_TEMPORARILY,
response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formErrorPage)), true);
}
// TODO is this correct response if isMandatory false??? Can
// that occur?
Expand Down Expand Up @@ -229,14 +230,13 @@ public AuthStatus validateRequest(MessageInfo messageInfo, Subject clientSubject
}

response.setContentLength(0);
response.sendRedirect(response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formLoginPage)));
Request.getBaseRequest(request).getResponse().sendRedirect(
HttpServletResponse.SC_MOVED_TEMPORARILY,
response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formLoginPage)),
true);
return AuthStatus.SEND_CONTINUE;
}
catch (IOException e)
{
throw new AuthException(e.getMessage());
}
catch (UnsupportedCallbackException e)
catch (IOException | UnsupportedCallbackException e)
{
throw new AuthException(e.getMessage());
}
Expand Down
14 changes: 3 additions & 11 deletions jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdAuthenticator.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -300,7 +299,7 @@ public Authentication validateRequest(ServletRequest req, ServletResponse res, b
LOG.debug("authenticated {}->{}", openIdAuth, nuri);

response.setContentLength(0);
baseResponse.sendRedirect(getRedirectCode(baseRequest.getHttpVersion()), nuri);
baseResponse.sendRedirect(nuri, true);
return openIdAuth;
}
}
Expand Down Expand Up @@ -392,7 +391,7 @@ public Authentication validateRequest(ServletRequest req, ServletResponse res, b
String challengeUri = getChallengeUri(request);
if (LOG.isDebugEnabled())
LOG.debug("challenge {}->{}", session.getId(), challengeUri);
baseResponse.sendRedirect(getRedirectCode(baseRequest.getHttpVersion()), challengeUri);
baseResponse.sendRedirect(challengeUri, true);

return Authentication.SEND_CONTINUE;
}
Expand Down Expand Up @@ -436,10 +435,9 @@ private void sendError(HttpServletRequest request, HttpServletResponse response,
{
String query = URIUtil.addQueries(ERROR_PARAMETER + "=" + UrlEncoded.encodeString(message), _errorQuery);
redirectUri = URIUtil.addPathQuery(URIUtil.addPaths(request.getContextPath(), _errorPath), query);
baseResponse.sendRedirect(getRedirectCode(baseRequest.getHttpVersion()), redirectUri);
}

baseResponse.sendRedirect(getRedirectCode(baseRequest.getHttpVersion()), redirectUri);
baseResponse.sendRedirect(redirectUri, true);
}
}

Expand All @@ -461,12 +459,6 @@ public boolean isErrorPage(String pathInContext)
return pathInContext != null && (pathInContext.equals(_errorPath));
}

private static int getRedirectCode(HttpVersion httpVersion)
{
return (httpVersion.getVersion() < HttpVersion.HTTP_1_1.getVersion()
? HttpServletResponse.SC_MOVED_TEMPORARILY : HttpServletResponse.SC_SEE_OTHER);
}

private String getRedirectUri(HttpServletRequest request)
{
final StringBuffer redirectUri = new StringBuffer(128);
Expand Down
5 changes: 3 additions & 2 deletions jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java
View file
Open in desktop
Expand Up @@ -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)
{
Expand All @@ -655,7 +656,7 @@ protected boolean checkUserDataPermissions(String pathInContext, Request request

String url = URIUtil.newURI(scheme, request.getServerName(), port, request.getRequestURI(), request.getQueryString());
response.setContentLength(0);
response.sendRedirect(url);
response.sendRedirect(url, true);
}
else
response.sendError(HttpStatus.FORBIDDEN_403, "!Secure");
Expand Down
10 changes: 3 additions & 7 deletions ...y-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -291,8 +290,7 @@ public Authentication validateRequest(ServletRequest req, ServletResponse res, b
LOG.debug("authenticated {}->{}", formAuth, nuri);

response.setContentLength(0);
int redirectCode = (baseRequest.getHttpVersion().getVersion() < HttpVersion.HTTP_1_1.getVersion() ? HttpServletResponse.SC_MOVED_TEMPORARILY : HttpServletResponse.SC_SEE_OTHER);
baseResponse.sendRedirect(redirectCode, response.encodeRedirectURL(nuri));
baseResponse.sendRedirect(response.encodeRedirectURL(nuri), true);
return formAuth;
}

Expand All @@ -316,8 +314,7 @@ else if (_dispatch)
else
{
LOG.debug("auth failed {}->{}", username, _formErrorPage);
int redirectCode = (baseRequest.getHttpVersion().getVersion() < HttpVersion.HTTP_1_1.getVersion() ? HttpServletResponse.SC_MOVED_TEMPORARILY : HttpServletResponse.SC_SEE_OTHER);
baseResponse.sendRedirect(redirectCode, response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formErrorPage)));
baseResponse.sendRedirect(response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formErrorPage)), true);
}

return Authentication.SEND_FAILURE;
Expand Down Expand Up @@ -410,8 +407,7 @@ else if (_dispatch)
else
{
LOG.debug("challenge {}->{}", session.getId(), _formLoginPage);
int redirectCode = (baseRequest.getHttpVersion().getVersion() < HttpVersion.HTTP_1_1.getVersion() ? HttpServletResponse.SC_MOVED_TEMPORARILY : HttpServletResponse.SC_SEE_OTHER);
baseResponse.sendRedirect(redirectCode, response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formLoginPage)));
baseResponse.sendRedirect(response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(), _formLoginPage)), true);
}
return Authentication.SEND_CONTINUE;
}
Expand Down
124 changes: 101 additions & 23 deletions jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -468,19 +469,13 @@ public static Stream<Arguments> basicScenarios()
)
));

// rawResponse = _connector.getResponse("GET /ctx/noauth/info HTTP/1.0\r\n\r\n");
// assertThat(rawResponse, startsWith("HTTP/1.1 200 OK"));

scenarios.add(Arguments.of(
new Scenario(
"GET /ctx/forbid/info HTTP/1.0\r\n\r\n",
HttpStatus.FORBIDDEN_403
)
));

// rawResponse = _connector.getResponse("GET /ctx/forbid/info HTTP/1.0\r\n\r\n");
// assertThat(rawResponse, startsWith("HTTP/1.1 403 Forbidden"));

scenarios.add(Arguments.of(
new Scenario(
"GET /ctx/auth/info HTTP/1.0\r\n\r\n",
Expand All @@ -493,9 +488,39 @@ public static Stream<Arguments> basicScenarios()
)
));

// rawResponse = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n\r\n");
// assertThat(rawResponse, startsWith("HTTP/1.1 401 Unauthorized"));
// assertThat(rawResponse, containsString("WWW-Authenticate: basic realm=\"TestRealm\""));
scenarios.add(Arguments.of(
new Scenario(
"POST /ctx/auth/info HTTP/1.1\r\n" +
"Host: test\r\n" +
"Content-Length: 10\r\n" +
"\r\n" +
"0123456789",
HttpStatus.UNAUTHORIZED_401,
(response) ->
{
String authHeader = response.get(HttpHeader.WWW_AUTHENTICATE);
assertThat(response.toString(), authHeader, containsString("basic realm=\"TestRealm\""));
assertThat(response.get(HttpHeader.CONNECTION), nullValue());
}
)
));

scenarios.add(Arguments.of(
new Scenario(
"POST /ctx/auth/info HTTP/1.1\r\n" +
"Host: test\r\n" +
"Content-Length: 10\r\n" +
"\r\n" +
"012345",
HttpStatus.UNAUTHORIZED_401,
(response) ->
{
String authHeader = response.get(HttpHeader.WWW_AUTHENTICATE);
assertThat(response.toString(), authHeader, containsString("basic realm=\"TestRealm\""));
assertThat(response.get(HttpHeader.CONNECTION), is("close"));
}
)
));

scenarios.add(Arguments.of(
new Scenario(
Expand All @@ -511,12 +536,6 @@ public static Stream<Arguments> basicScenarios()
)
));

// rawResponse = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n" +
// "Authorization: Basic " + authBase64("user:wrong") + "\r\n" +
// "\r\n");
// assertThat(rawResponse, startsWith("HTTP/1.1 401 Unauthorized"));
// assertThat(rawResponse, containsString("WWW-Authenticate: basic realm=\"TestRealm\""));

scenarios.add(Arguments.of(
new Scenario(
"GET /ctx/auth/info HTTP/1.0\r\n" +
Expand All @@ -526,10 +545,16 @@ public static Stream<Arguments> basicScenarios()
)
));

// rawResponse = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n" +
// "Authorization: Basic " + authBase64("user:password") + "\r\n" +
// "\r\n");
// assertThat(rawResponse, startsWith("HTTP/1.1 200 OK"));
scenarios.add(Arguments.of(
new Scenario(
"POST /ctx/auth/info HTTP/1.0\r\n" +
"Content-Length: 10\r\n" +
"Authorization: Basic " + authBase64("user:password") + "\r\n" +
"\r\n" +
"0123456789",
HttpStatus.OK_200
)
));

// == test admin
scenarios.add(Arguments.of(
Expand All @@ -544,10 +569,6 @@ public static Stream<Arguments> basicScenarios()
)
));

// rawResponse = _connector.getResponse("GET /ctx/admin/info HTTP/1.0\r\n\r\n");
// assertThat(rawResponse, startsWith("HTTP/1.1 401 Unauthorized"));
// assertThat(rawResponse, containsString("WWW-Authenticate: basic realm=\"TestRealm\""));

scenarios.add(Arguments.of(
new Scenario(
"GET /ctx/admin/info HTTP/1.0\r\n" +
Expand Down Expand Up @@ -1007,6 +1028,63 @@ public void testFormPostRedirect() throws Exception
assertThat(response, containsString("!role"));
}

@Test
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"));
}

@Test
public void testFormNoCookies() throws Exception
{
Expand Down