Skip to content

Conversation

@lachlan-roberts
Copy link
Contributor

See issue #12697

Doing a Response.writeError() will try to invoke the servlet ErrorHandler, which can do an ERROR dispatch. This does not work properly if it is not done from within the scope of the ServletChannel.

  • Add checks inside the ErrorHandler implementations to stop dispatching to error pages unless we have already started the ServletChannel.
  • Authenticators now use the ServletChannel mechanism to do a sendError() when the ServletChannel starts handling the request. It uses a new static method ErrorHandler.writeError which is overriden by the servlet ErrorHandlers.

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>
Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>
Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>
Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>
@lachlan-roberts lachlan-roberts marked this pull request as ready for review January 14, 2025 06:48
@janbartel
Copy link
Contributor

@lachlan-roberts this is quite a hefty refactor of ee10 error handling code, of publically available methods - are you sure we shouldn't just be doing this in ee11 only?

@lachlan-roberts
Copy link
Contributor Author

@janbartel I have aligned the EE10 ErrorHandler with changes from the EE11 one.

I think maybe the changes can be reduced, but I wanted the EE10 ErrorHandler to extend the core ErrorHandler like the EE11 one does, so that I could introduce this new ErrorHandler.writeError method.

If we really don't want to change the EE10 ErrorHandler I could introduce a new interface which both EE10 and EE11 ErrorHandlers could extend.

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>
@lachlan-roberts
Copy link
Contributor Author

@gregw @janbartel ready for review

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a few documentation changes

I also restarted CI, to get a clean build

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>
@lachlan-roberts lachlan-roberts requested a review from gregw January 28, 2025 03:35
@gregw
Copy link
Contributor

gregw commented Jan 28, 2025

Hmm AsyncMiddleMan tests failing here as well. So probably unrelated.

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor niggle

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>
value += ", charset=\"" + charset.name() + "\"";
res.getHeaders().put(HttpHeader.WWW_AUTHENTICATE.asString(), value);

// Don't use AuthenticationState.writeError, to avoid possibility of doing a Servlet error dispatch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a known specific set of cases/codes for which we want to avoid doing a Servlet error dispatch? If so, can we codify in AuthenticationState.writeError and just always call it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its just based on the use case rather than the code. I don't see why 401 couldn't be dispatched to a servlet error handler in a different case.

Although we only ever use AuthenticationState.writeError with 403 status codes.

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>
@lachlan-roberts lachlan-roberts merged commit 6b00af8 into jetty-12.1.x Apr 3, 2025
10 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Jetty 12.1.0 - (FROZEN) Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

4 participants