You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@grails.apache.org by "codeconsole (via GitHub)" <gi...@apache.org> on 2026/10/25 03:11:08 UTC
[PR] Run the character encoding filter ahead of every other filter [grails-core]
codeconsole opened a new pull request, #16759:
URL: https://github.com/apache/grails-core/pull/16759
Fixes #15921.
Once any filter reads a request parameter, the servlet container decodes the request body, and it uses ISO-8859-1 unless a character encoding has been set by then. Grails registers its character encoding filter at order -180, so a filter ordered ahead of it that reads a parameter garbles every non-ASCII form field for the rest of the request: a controller receives `Café – ðŸ‚` for `Café – 🍂`. The CAS plugin's single sign-out filter is one such filter. It is registered at `Ordered.HIGHEST_PRECEDENCE` and reads `logoutRequest` from every POST, so any app with single sign-out enabled (the default) has this bug, which is what #15921 reports.
Spring Boot registers its own encoding filter at `Ordered.HIGHEST_PRECEDENCE` for this reason. This PR does the same for Grails':
- `GrailsFilters.CHARACTER_ENCODING_FILTER` is now `Ordered.HIGHEST_PRECEDENCE`.
- The CAS plugin registers its single sign-out filter one order after the encoding filter. Moving the encoding filter alone isn't enough: a `FilterRegistrationBean` at the same order still runs first. I checked this by putting CAS back at `HIGHEST_PRECEDENCE`, and the CAS reproducer below failed again.
One side effect: with `grails.filter.forceEncoding: true`, the response encoding is now set before filters that used to run ahead of the encoding filter, including on responses those filters write themselves. The upgrade notes mention it.
#### Tests
- app1: `CharacterEncodingSpec` posts a form, with no charset, through a filter at `HIGHEST_PRECEDENCE + 1` that reads one of its parameters, and checks that the filter and the controller both get the text as typed.
- CAS example: `SingleSignOutEncodingSpec` posts `Café – 🍂` to an echo action with single sign-out enabled.
Both specs fail without the fix (`Café – ðŸ‚`) and pass with it. These also pass: `codeStyle`, `rat`, the `grails-web-common` and `grails-controllers` tests, app1's integration tests, and the CAS example's.
#### Docs
- The filter-ordering section of the plugin guide now says where the encoding filter sits. Its example moved to `HIGHEST_PRECEDENCE + 1`.
- The upgrade guide has a new section, "21. The character encoding filter runs first".
#### Merging up
8.0.x restructured the CAS example, which now runs a CAS container and has its specs in `grails.plugin.springsecurity.cas.test`. At the 7.2.x → 8.0.x merge, `EchoController` and `SingleSignOutEncodingSpec` need to move into that structure.
For 9.0.x, a separate PR (link to follow) drops Grails' encoding filter in favour of Spring Boot's.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Register CAS single sign-out after the character encoding filter [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty merged PR #16759:
URL: https://github.com/apache/grails-core/pull/16759
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Run the character encoding filter ahead of every other filter [grails-core]
Posted by "matrei (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Run the character encoding filter ahead of every other filter [grails-core]
Posted by "matrei (via GitHub)" <gi...@apache.org>.
matrei commented on PR #16759:
URL: https://github.com/apache/grails-core/pull/16759#issuecomment-6419783494
Following up on the three points above: I tested them on 7.0.17 (asset-pipeline 5.0.35). I ran a production WAR four ways: with the stock filter order and with the encoding filter at `HIGHEST_PRECEDENCE` (redefining the `characterEncodingFilter` bean, which is all the `GrailsFilters` change does), each with `forceEncoding` off and on. All three hold, but I understated the asset change: it doesn't need `forceEncoding`.
- **Ordering:** probe filters at `HIGHEST_PRECEDENCE + 1`, `GrailsFilters.FIRST`, -191 and -181 ran before the encoding filter on stock and after it with this PR. A `FilterRegistrationBean` at exactly `HIGHEST_PRECEDENCE` still runs first.
- **Assets:** with this PR, every asset response gets `;charset=UTF-8` (`image/x-icon;charset=UTF-8`, `image/svg+xml;charset=UTF-8`, `text/css;charset=UTF-8`, `text/javascript;charset=UTF-8`), also with `forceEncoding` off, which is the default. asset-pipeline's filter [reads the request's character encoding](https://github.com/wondrify/asset-pipeline/blob/77aac07346ef965cb8f86fddf0b6e2d7711ceb78/asset-pipeline-grails/src/main/groovy/asset/pipeline/AssetPipelineFilter.groovy#L49) and [sets it on the response](https://github.com/wondrify/asset-pipeline/blob/77aac07346ef965cb8f86fddf0b6e2d7711ceb78/asset-pipeline-grails/src/main/groovy/asset/pipeline/AssetPipelineFilter.groovy#L95). On stock it runs at -190, before the encoding filter, so the request encoding is still null and no charset is added. This affects every app that uses asset-pipeline, not just the ones with `forceEncoding: true`.
- **Responses written by early filters:** with `forceEncoding: true`, a filter at `HIGHEST_PRECEDENCE + 1` that writes `text/plain` without a charset now sends UTF-8 instead of ISO-8859-1. With it off, nothing changes.
- **Filters that set their own request encoding:** with `forceEncoding: true`, a filter that sets `windows-1252` used to be overridden by the forced UTF-8. With this PR, its `windows-1252` wins. With it off, nothing changes.
- **`getReader()`:** a filter at `HIGHEST_PRECEDENCE + 1` reading a JSON body sent without a charset got `®ñ` on stock and `®ñ` with this PR.
Browsers ignore a charset on images, but on CSS and JS it overrides their own detection. That's one more reason I'd keep the global move out of 7.0.x. Either way, the upgrade note should mention it, since it currently only covers `forceEncoding: true`.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Register CAS single sign-out after the character encoding filter [grails-core]
Posted by "codeconsole (via GitHub)" <gi...@apache.org>.
codeconsole commented on PR #16759:
URL: https://github.com/apache/grails-core/pull/16759#issuecomment-6420446056
Thanks, both. I've taken the split: this PR now only registers the single sign-out filter at `GrailsFilters.CHARACTER_ENCODING_FILTER.order + 1`, with `SingleSignOutEncodingSpec`, and the move is #16768 on 8.1.x, since 7.1.x, 7.2.x and 8.0.x would all ship it in a patch. Its upgrade note covers the points measured above. One refinement on assets: asset-pipeline adds the charset only to precompiled assets in a packaged app; its development runtime sets none, which I checked in app1 on 8.1.x. #16760's 9.0 note now mentions the asset charset too.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Run the character encoding filter ahead of every other filter [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on PR #16759:
URL: https://github.com/apache/grails-core/pull/16759#issuecomment-6418114557
I'm ok with this change, but this is a breaking change for ordering of filters. @matrei do you agree it's safe to merge for 7.0.x?
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org