jdaugherty commented on code in PR #16768:
URL: https://github.com/apache/grails-core/pull/16768#discussion_r4379591211
##########
grails-doc/src/en/guide/upgrading/upgrading81x.adoc:
##########
@@ -72,3 +72,19 @@ grailsApplication.config.refresh()
`refresh()` is a method of the configuration implementation rather than of the
`Config` interface, so it is a call for dynamically compiled test code and may
change with the implementation. It reads the environment again and merges the
property sources again, which replaces a value the test set on the
configuration directly wherever a property source defines the same key. Call it
again once the variables are removed: the configuration keeps the environment
it last read.
Two smaller differences: variable names are upper-cased with `Locale.ROOT`, as
Spring does, so a key containing `i` matches its upper-cased variable under a
Turkish default locale too; and `getProperty(null)` returns the default value
instead of throwing a `NullPointerException`.
+
+==== 5. The Character Encoding Filter Runs First
+
+Grails 8.1 registers its character encoding filter at
`Ordered.HIGHEST_PRECEDENCE`, the order Spring Boot gives its own, instead of
at -180. Once a filter reads a request parameter, the servlet container has
decoded the request body, as ISO-8859-1 unless an encoding was set by then, so
a filter ordered ahead of the encoding filter that read a parameter garbled
non-ASCII form input for the rest of the request, for example `Café` for
`Café`.
+
+Every filter ordered between `Ordered.HIGHEST_PRECEDENCE + 1` and -181 now
runs after the encoding filter instead of before it, and sees the request
encoding set. That includes asset-pipeline's filter at
`GrailsFilters.ASSET_PIPELINE_FILTER` (-190), a filter at
`GrailsFilters.FIRST`, and filters registered at `Ordered.HIGHEST_PRECEDENCE +
n`:
+
+* In a packaged application (a WAR or an executable jar), asset-pipeline
copies the request's encoding to the precompiled assets it serves, so every
asset response now declares a charset, for example `text/css;charset=UTF-8`,
whether or not `grails.filter.forceEncoding` is set. Browsers use it for
stylesheets and scripts instead of detecting their encoding. Assets served by
asset-pipeline's development runtime still have no charset.
+* A filter that reads a request body sent without a charset through
`getReader()` now decodes it as UTF-8, or as `grails.filter.encoding` says,
instead of as ISO-8859-1.
+* With `grails.filter.forceEncoding: true`, a response such a filter writes
without a charset is now encoded with the forced encoding instead of
ISO-8859-1, and a request encoding such a filter sets is no longer replaced by
the forced one.
+
+A filter registered at exactly `Ordered.HIGHEST_PRECEDENCE` through a
`FilterRegistrationBean` still runs before the encoding filter.
+
+===== Required Actions
+
+For most applications **no changes are required**. Check that packaged
stylesheets and scripts with non-ASCII characters are in the charset their
responses now declare: UTF-8, or the encoding `grails.filter.encoding` names. A
filter that has to run before the encoding filter can be registered at
`Ordered.HIGHEST_PRECEDENCE`; a filter that reads request parameters needs a
later order than that.
Review Comment:
"A filter that has to run before the encoding filter can be registered at
`Ordered.HIGHEST_PRECEDENCE`" is only true for a `FilterRegistrationBean`, as
line 86 and the plugin guide say. Spring Boot adds registration beans to the
filter list ahead of plain `Filter` beans and then sorts the list stably by
order, so at the same order the registration bean runs first. The encoding
filter is itself a plain `Filter` bean (`characterEncodingFilter()` returns the
`OrderedCharacterEncodingFilter`), so a plain `Filter` bean at
`HIGHEST_PRECEDENCE`, through `Ordered` or `@Order`, ties with it and runs
before or after it by bean definition order. Grails' `GrailsCorsFilter` is one
of those: its `getOrder()` returns `HIGHEST_PRECEDENCE`. It reads no
parameters, so nothing breaks, but its place relative to the encoding filter is
no longer decided by the orders.
Please add "through a `FilterRegistrationBean`" here. Both guides now tell
users to rely on that tie, so a test should cover it too, for example a
`FilterRegistrationBean` at `HIGHEST_PRECEDENCE` on `/encoding/*` in app1 that
records `request.characterEncoding`. For the form `CharacterEncodingSpec` posts
without a charset, that is `null` only when the filter runs ahead of the
encoding filter.
##########
grails-web-common/src/main/groovy/org/grails/web/config/http/GrailsFilters.java:
##########
@@ -28,7 +30,14 @@ public enum GrailsFilters {
FIRST,
ASSET_PIPELINE_FILTER,
- CHARACTER_ENCODING_FILTER,
+
+ /**
+ * Runs ahead of every other filter, as Spring Boot's own encoding filter
does. A filter that reads a request
+ * parameter before the encoding is set makes the servlet container decode
the request body with its default
+ * charset (ISO-8859-1), for the rest of the request.
+ */
+ CHARACTER_ENCODING_FILTER(Ordered.HIGHEST_PRECEDENCE),
Review Comment:
With this, `FIRST` is no longer the first Grails filter: the enum lists the
encoding filter third, and it now runs ahead of `FIRST`.
`GrailsSecurityHeadersAutoConfiguration`'s Javadoc still describes the old
arrangement: it says the headers filter is "outermost among the Grails
filters", and its list of filters ordered ahead of `FIRST` names only Spring
Boot's. The encoding filter writes no headers, so the headers filter behaves
the same, but please update that Javadoc. A line of Javadoc on `FIRST` saying
it is the first slot after the encoding filter would also help plugin authors
who pick it to run ahead of Grails.
##########
grails-doc/src/en/guide/upgrading/upgrading81x.adoc:
##########
@@ -72,3 +72,19 @@ grailsApplication.config.refresh()
`refresh()` is a method of the configuration implementation rather than of the
`Config` interface, so it is a call for dynamically compiled test code and may
change with the implementation. It reads the environment again and merges the
property sources again, which replaces a value the test set on the
configuration directly wherever a property source defines the same key. Call it
again once the variables are removed: the configuration keeps the environment
it last read.
Two smaller differences: variable names are upper-cased with `Locale.ROOT`, as
Spring does, so a key containing `i` matches its upper-cased variable under a
Turkish default locale too; and `getProperty(null)` returns the default value
instead of throwing a `NullPointerException`.
+
+==== 5. The Character Encoding Filter Runs First
+
+Grails 8.1 registers its character encoding filter at
`Ordered.HIGHEST_PRECEDENCE`, the order Spring Boot gives its own, instead of
at -180. Once a filter reads a request parameter, the servlet container has
decoded the request body, as ISO-8859-1 unless an encoding was set by then, so
a filter ordered ahead of the encoding filter that read a parameter garbled
non-ASCII form input for the rest of the request, for example `Café` for
`Café`.
+
+Every filter ordered between `Ordered.HIGHEST_PRECEDENCE + 1` and -181 now
runs after the encoding filter instead of before it, and sees the request
encoding set. That includes asset-pipeline's filter at
`GrailsFilters.ASSET_PIPELINE_FILTER` (-190), a filter at
`GrailsFilters.FIRST`, and filters registered at `Ordered.HIGHEST_PRECEDENCE +
n`:
Review Comment:
Every application also has a `FormContentFilter` in this range, at -9900:
Spring Boot's `WebMvcAutoConfiguration` registers `OrderedFormContentFilter`,
and `GrailsFormContentFilterAutoConfiguration` registers one when Boot's is
absent. It parses PUT, PATCH and DELETE form bodies, and for a body sent
without a charset it uses the request's character encoding
(`ServletServerHttpRequest.getHeaders()` adds it to the content type), or UTF-8
when there is none. Ahead of the encoding filter there was none, so it always
decoded those bodies as UTF-8. Now it decodes them as `grails.filter.encoding`
says, as POST forms already were.
That only changes something for an application whose
`grails.filter.encoding` isn't UTF-8, but it's the one filter in this range
that every application has, so I'd give it a bullet. I traced this through the
Spring code and didn't run 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]
