matrei commented on PR #16759:
URL: https://github.com/apache/grails-core/pull/16759#issuecomment-6419439247

   I'd keep the global reorder out of 7.0.x and only take the CAS part for the 
patch.
   
   Moving `CHARACTER_ENCODING_FILTER` to `HIGHEST_PRECEDENCE` is the right end 
state, but going by the code it changes filter ordering for every app, not just 
CAS users:
   
   - Any filter ordered between `HIGHEST_PRECEDENCE + 1` and -181 now runs 
after the encoding filter instead of before it. That includes asset-pipeline 
(-190), anything at `GrailsFilters.FIRST`, and the common `HIGHEST_PRECEDENCE + 
n` pattern.
   - With `grails.filter.forceEncoding: true`, responses from those filters now 
get the forced encoding, so assets served by asset-pipeline pick up 
`;charset=UTF-8`. A filter that sets its own request encoding now also wins 
over the forced one, where the forced one used to win.
   - A filter ahead of -180 that reads the body with `getReader()` now gets 
UTF-8 instead of ISO-8859-1.
   
   Most of that is arguably better behaviour, but it is a behaviour change, and 
the upgrade-guide section the PR adds says as much. I think it fits the next 
minor (or #16760 on 9.0.x) better than a patch.
   
   The CAS change on its own fixes #15921. I tried it on a 7.2.4 port of the 
sample app from the issue: overriding only 
`singleSignOutFilterRegistrationBean` with `order = 
GrailsFilters.CHARACTER_ENCODING_FILTER.order + 1` makes a form POST of `®ñ` 
come through intact, without touching the encoding filter. That only affects 
apps with CAS single sign-out enabled.
   
   So my suggestion for 7.0.x is the CAS order change plus 
`SingleSignOutEncodingSpec`, and the `GrailsFilters` change, 
`CharacterEncodingSpec` and the upgrade note go to the next minor.
   
   Until a fix is released, there is also a workaround that doesn't depend on 
filter order. It sets the container's default request encoding, so the body is 
decoded as UTF-8 whichever filter reads it first. I tested it on 7.2.4 and 
8.0.0 (it needs Servlet 4.0+, so it won't work on Grails 3):
   
   ```java
   @Configuration
   public class RequestEncodingConfiguration {
   
       @Bean
       ServletContextInitializer defaultRequestEncoding() {
           return servletContext ->
               servletContext.setRequestCharacterEncoding("UTF-8");
       }
   }
   ```
   
   Grails doesn't scan for `@Configuration` classes by default, so it also 
needs `@Import(RequestEncodingConfiguration)` on the `Application` class. 
Without that, the bean is never registered and nothing warns about it.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to