Skip to content

Issue #12720 - fix servlet error dispatch for errors generated by SecurityHandler #13693

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

Open
wants to merge 6 commits into
base: jetty-12.1.x
Choose a base branch
from
+185 −64
Open

Issue #12720 - fix servlet error dispatch for errors generated by SecurityHandler #13693

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
be33798
Issue #12720 - fix servlet error dispatch for 403's generated from Se…
lachlan-roberts Oct 10, 2025
735f3c7
Remove expectation of !authorized in tests
lachlan-roberts Oct 13, 2025
ed8cd86
PR #13693 - Remove expectation of !authorized in tests
lachlan-roberts Oct 17, 2025
c0f394f
PR #13693 - changes from review
lachlan-roberts Oct 21, 2025
acb911a
PR #13693 - changes from review
lachlan-roberts Oct 29, 2025
ec1c28e
Merge remote-tracking branch 'origin/jetty-12.1.x' into fix/jetty-12.…
joakime Nov 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 80 additions & 35 deletions jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/SecurityHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ public abstract class SecurityHandler extends Handler.Wrapper implements Configu
private int _sessionMaxInactiveIntervalOnAuthentication = 0;
private AuthenticationState.Deferred _deferred;

private record RequestResponse(Request request, Response response) {}

static
{
TypeUtil.serviceStream(ServiceLoader.load(Authenticator.Factory.class))
Expand Down Expand Up @@ -488,7 +490,7 @@ public boolean handle(Request request, Response response, Callback callback) thr

if (constraint.getAuthorization() == Authorization.FORBIDDEN)
{
Response.writeError(request, response, callback, HttpStatus.FORBIDDEN_403);
doWriteError(request, response, callback, HttpStatus.FORBIDDEN_403);
return true;
}

Expand Down Expand Up @@ -520,44 +522,14 @@ public boolean handle(Request request, Response response, Callback callback) thr

if (authenticationState instanceof AuthenticationState.ServeAs serveAs)
{
HttpURI uri = request.getHttpURI();
request = serveAs.wrap(request);
if (!uri.equals(request.getHttpURI()))
{
// URI is replaced, so filter out all metadata for the old URI
response.getHeaders().put(HttpHeader.CACHE_CONTROL.asString(), HttpHeaderValue.NO_CACHE.asString());
response.getHeaders().putDate(HttpHeader.EXPIRES.asString(), 1);
HttpFields.Mutable headers = new HttpFields.Mutable.Wrapper(response.getHeaders())
{
@Override
public HttpField onAddField(HttpField field)
{
if (field.getHeader() == null)
return field;
return switch (field.getHeader())
{
case CACHE_CONTROL, PRAGMA, ETAG, EXPIRES, LAST_MODIFIED, AGE -> null;
default -> field;
};
}
};

response = new Response.Wrapper(request, response)
{
@Override
public HttpFields.Mutable getHeaders()
{
return headers;
}
};
}

RequestResponse result = serveAsWrap(request, response, serveAs);
request = result.request;
response = result.response;
authenticationState = _deferred;
}
else if (mustValidate && !isAuthorized(constraint, authenticationState))
{
Response.writeError(request, response, callback, HttpStatus.FORBIDDEN_403, "!authorized");
return true;
return doWriteError(request, response, callback, HttpStatus.FORBIDDEN_403);
}
else if (authenticationState == null)
{
Expand Down Expand Up @@ -589,6 +561,79 @@ else if (authenticationState == null)
}
}

private boolean doWriteError(Request request, Response response, Callback callback, int status)
{
AuthenticationState authenticationState = AuthenticationState.writeError(request, response, callback, status);
if (authenticationState instanceof AuthenticationState.ServeAs serveAs)
{
RequestResponse result = serveAsWrap(request, response, serveAs);
request = result.request;
response = result.response;
authenticationState = _deferred;

AuthenticationState.setAuthenticationState(request, authenticationState);
IdentityService.Association association =
(authenticationState instanceof AuthenticationState.Succeeded user)
? _identityService.associate(user.getUserIdentity(), null) : null;

try
{
//process the request by other handlers
return getHandler().handle(_authenticator.prepareRequest(request, authenticationState), response, callback);
}
catch (Throwable t)
{
Response.writeError(request, response, callback, HttpStatus.INTERNAL_SERVER_ERROR_500, t.getMessage());
}
finally
{
if (association == null && authenticationState instanceof AuthenticationState.Deferred deferred)
association = deferred.getAssociation();
if (association != null)
association.close();
}
}

return true;
}

private RequestResponse serveAsWrap(Request request, Response response, AuthenticationState.ServeAs serveAs)
{
HttpURI uri = request.getHttpURI();
request = serveAs.wrap(request);
if (!uri.equals(request.getHttpURI()))
{
// URI is replaced, so filter out all metadata for the old URI
response.getHeaders().put(HttpHeader.CACHE_CONTROL.asString(), HttpHeaderValue.NO_CACHE.asString());
response.getHeaders().putDate(HttpHeader.EXPIRES.asString(), 1);
HttpFields.Mutable headers = new HttpFields.Mutable.Wrapper(response.getHeaders())
{
@Override
public HttpField onAddField(HttpField field)
{
if (field.getHeader() == null)
return field;
return switch (field.getHeader())
{
case CACHE_CONTROL, PRAGMA, ETAG, EXPIRES, LAST_MODIFIED, AGE -> null;
default -> field;
};
}
};

response = new Response.Wrapper(request, response)
{
@Override
public HttpFields.Mutable getHeaders()
{
return headers;
}
};
}

return new RequestResponse(request, response);
}

public static SecurityHandler getCurrentSecurityHandler()
{
ContextHandler contextHandler = ContextHandler.getCurrentContextHandler();
Expand Down
2 changes: 0 additions & 2 deletions ...-core/jetty-security/src/test/java/org/eclipse/jetty/security/BasicAuthenticatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@ public void testBasic() throws Exception

response = _connector.getResponse("GET /ctx/admin/user HTTP/1.0\r\nAuthorization: %s\r\n\r\n".formatted(BasicAuthenticator.authorization("user", "password")));
assertThat(response, containsString("HTTP/1.1 403 Forbidden"));
assertThat(response, containsString("!authorized"));
assertThat(response, not(containsString("OK")));

response = _connector.getResponse("GET /ctx/admin/user HTTP/1.0\r\nAuthorization: %s\r\n\r\n".formatted(BasicAuthenticator.authorization("admin", "password")));
Expand All @@ -153,7 +152,6 @@ public void testBasic() throws Exception

response = _connector.getResponse("GET /ctx/known/user HTTP/1.0\r\nAuthorization: %s\r\n\r\n".formatted(BasicAuthenticator.authorization("user", "password")));
assertThat(response, containsString("HTTP/1.1 403 Forbidden"));
assertThat(response, containsString("!authorized"));
assertThat(response, not(containsString("OK")));
}

Expand Down
2 changes: 0 additions & 2 deletions ...y-core/jetty-security/src/test/java/org/eclipse/jetty/security/FormAuthenticatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,6 @@ public void testLoginQuery() throws Exception

response = _connector.getResponse("GET /ctx/admin/user HTTP/1.0\r\nHost:host:8888\r\nCookie: JSESSIONID=" + sessionId + "\r\n\r\n");
assertThat(response, containsString("HTTP/1.1 403 Forbidden"));
assertThat(response, containsString("!authorized"));
assertThat(response, not(containsString("OK")));

response = _connector.getResponse("GET /ctx/any/user HTTP/1.0\r\nHost:host:8888\r\nCookie: JSESSIONID=" + unsafeSessionId + "\r\n\r\n");
Expand Down Expand Up @@ -282,7 +281,6 @@ public void testLoginForm() throws Exception

response = _connector.getResponse("GET /ctx/admin/user HTTP/1.0\r\nHost:host:8888\r\nCookie: JSESSIONID=" + sessionId + "\r\n\r\n");
assertThat(response, containsString("HTTP/1.1 403 Forbidden"));
assertThat(response, containsString("!authorized"));
assertThat(response, not(containsString("OK")));

response = _connector.getResponse("GET /ctx/any/user HTTP/1.0\r\nHost:host:8888\r\nCookie: JSESSIONID=" + unsafeSessionId + "\r\n\r\n");
Expand Down
39 changes: 39 additions & 0 deletions ...y-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ErrorPageTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.http.HttpTester;
import org.eclipse.jetty.logging.StacklessLogging;
import org.eclipse.jetty.security.Constraint;
import org.eclipse.jetty.security.SecurityHandler;
import org.eclipse.jetty.server.Handler;
import org.eclipse.jetty.server.LocalConnector;
import org.eclipse.jetty.server.Request;
Expand Down Expand Up @@ -2067,6 +2069,43 @@ public void testProtectedTarget() throws Exception
assertThat(response.getContent(), containsString("ERROR_MESSAGE: Not Found"));
}

@Test
public void testForbidden() throws Exception
{
ServletContextHandler contextHandler = new ServletContextHandler();
contextHandler.setContextPath("/ctx");
contextHandler.setProtectedTargets(new String[] {"/WEB-INF", "/META-INF"});
contextHandler.addServlet(ErrorDumpServlet.class, "/error/*");
contextHandler.addServlet(new OkServlet(), "/*");

SecurityHandler.PathMapped securityHandler = new SecurityHandler.PathMapped();
securityHandler.put("/*", Constraint.ANY_USER);
contextHandler.setSecurityHandler(securityHandler);

ErrorPageErrorHandler errorPageErrorHandler = new ErrorPageErrorHandler();
contextHandler.setErrorHandler(errorPageErrorHandler);
errorPageErrorHandler.addErrorPage(403, "/error/403");

startServer(contextHandler);

String rawRequest = """
GET /ctx/WEB-INF/anything HTTP/1.1\r
Host: test\r
Connection: close\r
Accept: */*\r
Accept-Charset: *\r
\r
""";

String rawResponse = _connector.getResponse(rawRequest);
assertThat(rawResponse, startsWith("HTTP/1.1 403 Forbidden"));
HttpTester.Response response = HttpTester.parseResponse(rawResponse);
assertThat(response.getStatus(), is(403));
assertThat(response.getContent(), containsString("ERROR_PAGE: /403"));
assertThat(response.getContent(), containsString("ERROR_MESSAGE: Forbidden"));
assertThat(response.getContent(), containsString("ERROR_CODE: 403"));
}

public static class ErrorDumpServlet extends HttpServlet
{
@Override
Expand Down
24 changes: 12 additions & 12 deletions ...ty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/security/ConstraintTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,7 +802,7 @@ public static Stream<Arguments> basicScenarios()
"GET /ctx/admin/info HTTP/1.0\r\n" +
"Authorization: Basic " + authBase64("user:password") + "\r\n" +
"\r\n",
HttpStatus.FORBIDDEN_403, response -> assertThat(response.getContent(), containsString("!authorized"))
HttpStatus.FORBIDDEN_403, response -> assertThat(response.getContent(), containsString("Forbidden"))
)
));

Expand Down Expand Up @@ -1100,7 +1100,7 @@ public void testFormDispatch() throws Exception
"Cookie: JSESSIONID=" + session + "\r\n" +
"\r\n");
assertThat(response, startsWith("HTTP/1.1 403"));
assertThat(response, containsString("!authorized"));
assertThat(response, containsString("Forbidden"));
}

@Test
Expand Down Expand Up @@ -1163,7 +1163,7 @@ public void testFormRedirect() throws Exception
"Cookie: JSESSIONID=" + session + "\r\n" +
"\r\n");
assertThat(response, startsWith("HTTP/1.1 403"));
assertThat(response, containsString("!authorized"));
assertThat(response, containsString("Forbidden"));
assertThat(response, not(containsString("JSESSIONID=" + session)));
}

Expand Down Expand Up @@ -1322,7 +1322,7 @@ public void testFormPostRedirect() throws Exception
"Cookie: JSESSIONID=" + session + "\r\n" +
"\r\n");
assertThat(response, startsWith("HTTP/1.1 403"));
assertThat(response, containsString("!authorized"));
assertThat(response, containsString("Forbidden"));
}

@Test
Expand Down Expand Up @@ -1439,7 +1439,7 @@ public void testFormNoCookies() throws Exception
response = _connector.getResponse("GET /ctx/admin/info;jsessionid=" + session + ";other HTTP/1.0\r\n" +
"\r\n");
assertThat(response, startsWith("HTTP/1.1 403"));
assertThat(response, containsString("!authorized"));
assertThat(response, containsString("Forbidden"));
}

/**
Expand Down Expand Up @@ -1667,7 +1667,7 @@ public void testStrictBasic() throws Exception
"\r\n");

assertThat(response, startsWith("HTTP/1.1 403 "));
assertThat(response, containsString("!authorized"));
assertThat(response, containsString("Forbidden"));

response = _connector.getResponse("GET /ctx/admin/info HTTP/1.0\r\n" +
"Authorization: Basic " + authBase64("admin:password") + "\r\n" +
Expand Down Expand Up @@ -1726,13 +1726,13 @@ public void testStrictFormDispatch()
"Cookie: JSESSIONID=" + session + "\r\n" +
"\r\n");
assertThat(response, startsWith("HTTP/1.1 403"));
assertThat(response, containsString("!authorized"));
assertThat(response, containsString("Forbidden"));

response = _connector.getResponse("GET /ctx/admin/info HTTP/1.0\r\n" +
"Cookie: JSESSIONID=" + session + "\r\n" +
"\r\n");
assertThat(response, startsWith("HTTP/1.1 403"));
assertThat(response, containsString("!authorized"));
assertThat(response, containsString("Forbidden"));

// log in again as user2
response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n\r\n");
Expand Down Expand Up @@ -1760,7 +1760,7 @@ public void testStrictFormDispatch()
"Cookie: JSESSIONID=" + session + "\r\n" +
"\r\n");
assertThat(response, startsWith("HTTP/1.1 403"));
assertThat(response, containsString("!authorized"));
assertThat(response, containsString("Forbidden"));

// log in again as admin
response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n\r\n");
Expand Down Expand Up @@ -1833,13 +1833,13 @@ public void testStrictFormRedirect() throws Exception
"Cookie: JSESSIONID=" + session + "\r\n" +
"\r\n");
assertThat(response, startsWith("HTTP/1.1 403"));
assertThat(response, containsString("!authorized"));
assertThat(response, containsString("Forbidden"));

response = _connector.getResponse("GET /ctx/admin/info HTTP/1.0\r\n" +
"Cookie: JSESSIONID=" + session + "\r\n" +
"\r\n");
assertThat(response, startsWith("HTTP/1.1 403"));
assertThat(response, containsString("!authorized"));
assertThat(response, containsString("Forbidden"));

// log in again as user2
response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n\r\n");
Expand Down Expand Up @@ -1868,7 +1868,7 @@ public void testStrictFormRedirect() throws Exception
"Cookie: JSESSIONID=" + session + "\r\n" +
"\r\n");
assertThat(response, startsWith("HTTP/1.1 403"));
assertThat(response, containsString("!authorized"));
assertThat(response, containsString("Forbidden"));

//log in as user3, who doesn't have a valid role, but we are checking a constraint
//of ** which just means they have to be authenticated
Expand Down
39 changes: 39 additions & 0 deletions ...y-ee11/jetty-ee11-servlet/src/test/java/org/eclipse/jetty/ee11/servlet/ErrorPageTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.http.HttpTester;
import org.eclipse.jetty.logging.StacklessLogging;
import org.eclipse.jetty.security.Constraint;
import org.eclipse.jetty.security.SecurityHandler;
import org.eclipse.jetty.server.Handler;
import org.eclipse.jetty.server.LocalConnector;
import org.eclipse.jetty.server.Request;
Expand Down Expand Up @@ -2069,6 +2071,43 @@ public void testProtectedTarget() throws Exception
assertThat(response.getContent(), containsString("ERROR_MESSAGE: Not Found"));
}

@Test
public void testForbidden() throws Exception
{
ServletContextHandler contextHandler = new ServletContextHandler();
contextHandler.setContextPath("/ctx");
contextHandler.setProtectedTargets(new String[] {"/WEB-INF", "/META-INF"});
contextHandler.addServlet(ErrorDumpServlet.class, "/error/*");
contextHandler.addServlet(new OkServlet(), "/*");

SecurityHandler.PathMapped securityHandler = new SecurityHandler.PathMapped();
securityHandler.put("/*", Constraint.ANY_USER);
contextHandler.setSecurityHandler(securityHandler);

ErrorPageErrorHandler errorPageErrorHandler = new ErrorPageErrorHandler();
contextHandler.setErrorHandler(errorPageErrorHandler);
errorPageErrorHandler.addErrorPage(403, "/error/403");

startServer(contextHandler);

String rawRequest = """
GET /ctx/WEB-INF/anything HTTP/1.1\r
Host: test\r
Connection: close\r
Accept: */*\r
Accept-Charset: *\r
\r
""";

String rawResponse = _connector.getResponse(rawRequest);
assertThat(rawResponse, startsWith("HTTP/1.1 403 Forbidden"));
HttpTester.Response response = HttpTester.parseResponse(rawResponse);
assertThat(response.getStatus(), is(403));
assertThat(response.getContent(), containsString("ERROR_PAGE: /403"));
assertThat(response.getContent(), containsString("ERROR_MESSAGE: Forbidden"));
assertThat(response.getContent(), containsString("ERROR_CODE: 403"));
}

public static class ErrorDumpServlet extends HttpServlet
{
@Override
Expand Down
Loading