Replace FORWARD and INCLUDE with ERROR in default DispatcherTypes

[cemo]

I had trouble about default DispatcherTypes a few times and always found root cause in the context of default DispatcherTypes. Given the next release is a suitable time in terms of semver, please change the default values. What I have in my mind is replacing FORWARD, INCLUDE with ERROR.

Consider Spring Security Filter, Spring Session Filter, HiddenHttpMethodFilter, CharacterEncodingFilter etc... Which one needs to be filtered again in a FORWARD or INCLUDE request? But all these filters must be filtered in an ERROR request as well.

Here is the default values:

static final EnumSet<DispatcherType> ASYNC_DISPATCHER_TYPES = EnumSet.of(
            DispatcherType.FORWARD, DispatcherType.INCLUDE, DispatcherType.REQUEST,
            DispatcherType.ASYNC);

static final EnumSet<DispatcherType> NON_ASYNC_DISPATCHER_TYPES = EnumSet
            .of(DispatcherType.FORWARD, DispatcherType.INCLUDE, DispatcherType.REQUEST);

[cemo]

Kindly ping @philwebb :) I am afraid of being too late for next milestone which seems last milestone.

[dsyer]

The main argument we have seen in favour of adding (or replacing) ERROR dispatches is so that an error page can use state that is set up by a filter (e.g. the authenticated user in the case of the security filter). In general, we feel that relying on any state being present in an error page is probably a bad idea, and could easily lead to another error that cannot be processed (a stackoverflow seems the most likely outcome in that case). So we don't want to change the default dispatcher types. You can already change the individual ones by configuring the filters yourself, or (in the case of Spring Security) through configuration properties.

Regarding whether or not the existing defaults make sense, I'm not sure, but wouldn't it be important to have the security and session context available in a FORWARD? If so we should leave the defaults as they are. Maybe I'm missing something?

[cemo]

I think that it is a very good idea to know who has the error. There was some comments on this topic by @rwinch on #4505 (comment). Honestly I can not understand and how this can easily lead another error. Can you explain with an example please?

Also INCLUDE definitely should be considered to be removed from defaults. I can not find a place where INCLUDE is making sense. Included JSP pages which are including other JSP pages can easily create unpleasant errors.

FORWARD's usage is also similar to INCLUDE. They are internal requests and should be definitely considered to be removed.

I think that the best thing would be take some filters from Spring Boot and discuss their usage. We can start from Spring Security Filter, Spring Session Filter, HiddenHttpMethodFilter, CharacterEncodingFilter

[cemo]

This is a long time standing issue but I believe that this is still a pitfall. I believe that revising this issue can greatly simplify some errors users hit.

Here is another one #7348. Changing default DispatcherTypes completely seems more convenient to me.

gentle ping @philwebb @wilkinsona

[wilkinsona]

Here is another one #7348. Changing default DispatcherTypes completely seems more convenient to me.

@cemo IMO, you're conflating two separate issues. One auto-configured filter that does not have the correct dispatcher types is not a reason for changing the dispatcher types that are used by all filters by default. If we are going to change the defaults, then I am in favour of aligning with the servlet spec which would make REQUEST and nothing else the default.

[cemo]

I believe that the current configuration has 2 mistake:

1. Using INCLUDE and FORWARD as a default
   The current state of the configuration is dangerous in terms of performance. It is using INCLUDE as a default. All java technologies using INCLUDE as a dispatcher type will refilter INCLUDE requests. This means a JSP page using include directives will be filtered again by with all filters. FORWARD is also similar to INCLUDE. Which INCLUDE and FORWARD request should be filtered again by any filter. I really do not know. I counted all filter came to my mind and could not find. What we are doing right now is that changing their dispatching types.
2. Not using ERROR as a default.
   ERROR indicates a real error in java world. This usually ends with an error page. Again all filters I know must be used in this dispatching type. For example we are using user-id in error pages to feed some 3rd party tools to indicate an error.

Shortly I believe this decision should be made by analyzing current filters. Aligning with current servlet spec will not solve whole problems. I think that ERROR should be included in this list.

If you want to give a try to check these filters came to my mind:

• Spring Security Filter
• Spring Session Filter
• HiddenHttpMethodFilter
• CharacterEncodingFilter
• ResourceUrlEncodingFilter

Which one do you think that is not suitable for ERROR? There are also some comments here #4505 (comment) for Spring Security from @rwinch.
#7348 has same configuration problem. Defaulting ERROR would solve in the first place.

[rwinch]

I think it makes sense to change SecurityFilterAutoConfiguration to use the ERROR dispatch type by default. The ERROR dispatch type is also the default of Spring Security's Servlet Java Config.

The reason the ERROR dispatch type makes sense for Spring Security is many times navigation is based on the permissions of the current user. Without Spring Security being populated, the error page may not have meaningful navigation.

@cemo The concern for invoking Spring Security (or other Filters) on error handling is a valid concern. If there is a service down that Spring Security leverages, the error page will no longer be displayed (because you are depending on Spring Security). That said, users can handle this by having a Filter that try/catches the entire FilterChain or acknowledge that things are broken and you cannot always have your custom error page