New OncePerRequestFilter behavior breaks RequestContextFilter on Jetty after sendError

[svschouw-bb]

After the change to OncePerRequestFilter in #22989, when using a RequestContextFilter, the request context will no longer be available after sendError(...) completes in Jetty. This means that the usage of any @Scope("request") beans in places like the afterCompletion in interceptors or on the return path of filters will not work, nor will more general usage of RequestContextHolder.getRequestAttributes() work.

The problem is that the change in OncePerRequestFilter causes the filter to be processed again, even though it is a OncePerRequestFilter. And the RequestContextFilter is not designed to be nested. The finally block clears all request attributes, even when attributes were set before the filter started.

I'm not really sure what the best solution would be. Maybe introduce a OncePerRequestFilter.shouldNotFilterOnNestedErrorDispatch() or something. Or maybe make RequestContextFilter restore the previous request context, instead of clearing everything (it seems like FrameworkServlet does this too)? Or both.

Affected version: 5.1.8

[sbrannen]

@rstoyanchev, this appears to be a possible regression introduced in conjunction with #22989. Can you take a look at it?

[rstoyanchev]

@svschouw-bb, thanks for reporting this and apologies for the disruption.

the change in OncePerRequestFilter causes the filter to be processed again, even though it is a OncePerRequestFilter

The filter was created long before the Servlet API had a formal concept of different dispatcher types. Nowadays it's more like once per top-level REQUEST, ASYNC, or ERROR dispatch. Jetty however shreds the assumption those are top-level.

Taking a step back we can say once per top-level dispatch, i.e. filter only the first invocation of OncePerRequestFilter on the current stack regardless of whether it's REQUEST -> FORWARD -> FORWARD, or REQUEST -> ERROR, or some other variation.

@rwinch can you explain why it was important in your case for #22989 to be invoked on an ERROR dispatch that's already nested within a REQUEST dispatch? Presumably any initialization done by the filter has already been done for the current thread.

We could either roll back this change, or at best turn it into an opt-in mechanism with something like shouldNotFilterNestedErrorDispatch(), set to true by default.

[svschouw-bb]

As far as I can tell it was to mirror the change in spring-projects/spring-session#1308. The problem there was that they use a OncePerRequestFilter to wrap the request object and set a request attribute. Later code, when it saw the request attribute assumed the request was wrapped, but it wasn't, since an ERROR dispatch needs to be done with the original request and response objects. In this way an ERROR dispatch differs most significantly from a FORWARD dispatch.

[rstoyanchev]

Okay but I still don't understand why the request isn't wrapped? The behavior in Jetty is that the error dispatch is performed within the current stack. That means we are still within the try-finally of SecurityContextPersistenceFilter where the context is cleared and after which the request will no longer be wrapped.

[svschouw-bb]

From section 10.9.3 of the Servlet 3.1 book:

The error page mechanism operates on the original unwrapped/unfiltered request and response objects created by the container. The mechanism described in Section 6.2.5, “Filters and the RequestDispatcher” may be used to specify filters that are applied before an error response is generated.

So Jetty properly dispatches the request on the original request and response objects, causing the wrapping to be undone.

There is nothing (as far as I can see, I didn't check thoroughly) which says the attributes should be cleared, and Jetty doesn't clear hem.

I guess a problem is that Jetty only applies the "unwrapped" portion of the specification, and not the "unfiltered" portion. So it could be considered a Jetty bug.

[rwinch]

@rstoyanchev As mentioned in the previous comment, the issue is that error is a new dispatch and the request is no longer wrapped. This means that the request is not wrapped again on error dispatch. Here are some additional details spring-projects/spring-session#1308 (comment)

[rstoyanchev]

Thanks for the pointer, @svschouw-bb. Indeed the error dispatch needs to be performed with the original request and response which explains why they are unwrapped.

So for a nested ERROR dispatch:

1. Filters that establish a ThreadLocal context must ignore it to avoid clearing the context too early.
2. Filters that wrap the request/response must re-apply the wrapping.
3. Filters that do both must do both of the above.

The Spring Framework has three filters that fall under 1) (OSIV, OEIV, and RequestContext) and one that falls under 2) (forwarded headers), and the rest don't process ERROR dispatches. There is nothing in the Spring Framework for 3), but SecurityContextPersistenceFilter is an example.

My suggestion is to add shouldNotFilterNestedErrorDispatch() returning true by default in which case filters like 1) work as expected. For filters that return false, the current solution would apply (i.e. use a separate request attribute for ERROR dispatches), and such filters would need to be careful to only re-apply wrapping but otherwise leave ThreadLocal context as is. So they'll need to be aware that an ERROR dispatch can be nested but I guess that's where the opt part comes.

I see this as the best compromise in the given situation and the most we can do. Does it seem acceptable?

[svschouw-bb]

Not sure if it was implied, but for (1) it would say they need to ignore nested error calls (i.e.: just the regular OncePerRequestFilter behavior), since it's possible they get an ERROR dispatch before the first regular REQUEST dispatch even happened (if a prior filter already does a sendError(...)).

Other than that it sounds good to me.

(Also, as for the RequestContextFilter to which this issue originally applied: maybe it's possible to restore the original request/locale context instead of always resetting it, just like the FrameworkServlet does.)

[rstoyanchev]

I see no reason for RequestContextFilter to opt into processing a nested ERROR dispatch, only to then have to detect a previous context + restore it at the end. There is nothing to be gained that I can see, only additional work to be done.

Essentially unless you opt into processing a nested ERROR dispatch, you will not be called a second time as long as long as you're still within a previous Filter invocation on the call stack.

As for the case of sendError before a Filter is reached, that should work fine since the "alreadyFiltered" attribute would not have been set yet.

[rwinch]

I'm concerned with how complex the OncePerRequestFilter is becoming. What does it now mean to be once per request? It now has variations on how it can behave for wrapping the request vs setting up a ThreadLocal. I wonder if we could should provide different abstractions for the different scenarios vs making sure the Filter is setup correctly.

For example have a RequestWrapperFilter which works for wrapping the request once. Perhaps it even uses something like WebUtils.getNativeRequest to determine if it needs wrapped vs trying to be invoked correctly. Of course, I'm not sure what sort of overhead we should expect traversing through the wrapped requests in a real application. Would that be significant overhead?

[rstoyanchev]

I'll experiment with a base class to help ensure the request (and/or response) remain wrapped. Maybe that would solve the original problem in a different way, and there is no need to re-invoke doFilterInternal for a nested ERROR dispatch at all.

[rstoyanchev]

Here is what I've come up with. There is a new protected method in OncePerRequestFilter called doFilterNestedErrorDispatch that is invoked separately and only during a nested ERROR dispatch, i.e. where we are still in the filter chain and it is an ERROR dispatch. This gives sub-classes a chance to wrap the request and response again and delegate to the filter chain.

I did consider creating some sort of RequestWrappingFilter but didn't find a good solution there. Separate methods to wrap the request and/or response aren't ideal, since if you want to do both, it's best to do them in one method, and such a method cannot return both wrapped.

I've also updated the ForwardedHeaderFilter to have as an example. If this looks okay I'll prepare this further to push into master but I wanted some confirmation that this will work for Spring Security and Spring Session, @rwinch?

[rwinch]

Curious if the request is then wrapped twice in Tomcat for error dispatch since the attributes are cleared out?

[rstoyanchev]

It shouldn't be wrapped twice. In Tomcat or whenever the ERROR dispatch is not nested, the filter chain is exited all the way.

[rstoyanchev]

The new doFilterNestedErrorDispatch method is now in while the change from #22989 that caused the regression has been removed.

@rwinch this means there is a need for a follow-up for spring-projects/spring-session#1308 to ensure affected filters implement the new method.

By the way I checked Tomcat and Jetty behavior in case of an error, regarding double wrapping:

• For Tomcat, doFilterInternal is called once for REQUEST and once for ERROR. This matches long existing behavior since the ERROR dispatch is a completely separate filter chain invocation. No double wrapping occurs.
• For Jetty, doFilterInternal is called for REQUEST, and doFilterNestedErrorDispatch is called for ERROR, allowing wrapping in case of a nested ERROR dispatch.