matrei commented on code in PR #16760: URL: https://github.com/apache/grails-core/pull/16760#discussion_r4387842890
########## grails-test-examples/app1/src/integration-test/groovy/functionaltests/encoding/CharacterEncodingSpec.groovy: ########## @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package functionaltests.encoding + +import java.net.http.HttpRequest + +import spock.lang.Issue +import spock.lang.Specification +import spock.lang.Tag +import spock.lang.Unroll + +import grails.testing.mixin.integration.Integration +import org.apache.grails.testing.http.client.HttpClientSupport + +/** + * A filter registered at {@code Ordered.HIGHEST_PRECEDENCE + 1} reads a request parameter before Grails sees the + * request. Once a filter reads a parameter, the container has decoded the body, so the encoding filter has to run + * before it. The other cases check which forms the forced request encoding decodes as UTF-8 when they declare + * another charset. + */ +@Integration +@Tag('http-client') +@Issue('https://github.com/apache/grails-core/issues/15921') +class CharacterEncodingSpec extends Specification implements HttpClientSupport { Review Comment: Three places say that a `FilterRegistrationBean` at exactly `Ordered.HIGHEST_PRECEDENCE` runs before Spring Boot's encoding filter: the plugin guide, §20 and the pitfall in the skill. Nothing tests that on 9.0.x. It holds because Boot's filter is a plain `Filter` bean. `ServletContextInitializerBeans` collects registration beans before it adapts plain filter beans, and its sort is stable, so when two orders are equal, the registration bean comes first. That is a Spring Boot implementation detail, and a Boot upgrade could change it without anyone noticing. #16768 has a case for exactly this: `EncodingRecordingFilter` and "runs a filter registered at HIGHEST_PRECEDENCE through a FilterRegistrationBean ahead of the encoding filter". Adding that case here would pin the documented behaviour on 9.0.x. It would also make the three app1 files nearly identical on both branches, so their add/add conflicts become easy to resolve. ########## grails-web-common/src/main/groovy/org/grails/web/config/http/GrailsFilters.java: ########## @@ -28,7 +28,18 @@ public enum GrailsFilters { FIRST, ASSET_PIPELINE_FILTER, + + /** + * The order of the character encoding filter Grails registered before 9.0. + * + * @deprecated since 9.0, for removal. Grails no longer registers a character encoding filter; Spring Boot + * registers one at {@code Ordered.HIGHEST_PRECEDENCE}, ahead of every other filter. The filters after it + * take their orders from their position in this enum, so when it is removed, give them their current orders + * explicitly. + */ + @Deprecated(since = "9.0", forRemoval = true) CHARACTER_ENCODING_FILTER, Review Comment: #16768 makes this `CHARACTER_ENCODING_FILTER(Ordered.HIGHEST_PRECEDENCE)` on 8.1.x. Here it keeps the order derived from its position. If this side wins the merge-up, the constant goes from `Integer.MIN_VALUE` in 8.1 back to -180 in 9.0. The Javadoc's "the order of the character encoding filter Grails registered before 9.0" would then also be wrong for 8.1. I would give it the explicit order here too, and keep the deprecation: ```java @Deprecated(since = "9.0", forRemoval = true) CHARACTER_ENCODING_FILTER(Ordered.HIGHEST_PRECEDENCE), ``` It then still names the order of the encoding filter that actually runs, Spring Boot's. `CHARACTER_ENCODING_FILTER.order + 1` keeps meaning "right after the encoding filter", which matters because the CAS plugin uses that expression on 7.0.x (#16759) and 8.1.x (#16768), and applications may have copied it. The filters after it still take their orders from `ordinal()`, so the note about removal still holds. One catch: `CHARACTER_ENCODING_FILTER.order - 1` wraps around to `Integer.MAX_VALUE`, but that is already true on 8.1 once #16768 is merged. ########## grails-test-examples/app1/src/integration-test/groovy/functionaltests/encoding/CharacterEncodingSpec.groovy: ########## @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package functionaltests.encoding + +import java.net.http.HttpRequest + +import spock.lang.Issue +import spock.lang.Specification +import spock.lang.Tag +import spock.lang.Unroll + +import grails.testing.mixin.integration.Integration +import org.apache.grails.testing.http.client.HttpClientSupport + +/** + * A filter registered at {@code Ordered.HIGHEST_PRECEDENCE + 1} reads a request parameter before Grails sees the + * request. Once a filter reads a parameter, the container has decoded the body, so the encoding filter has to run + * before it. The other cases check which forms the forced request encoding decodes as UTF-8 when they declare + * another charset. + */ +@Integration +@Tag('http-client') +@Issue('https://github.com/apache/grails-core/issues/15921') +class CharacterEncodingSpec extends Specification implements HttpClientSupport { + + void 'decodes a form post as UTF-8 when a filter ahead of Grails reads its parameters'() { + when: 'a form is posted without a charset, through a filter that reads one of its parameters' + def response = httpPost('/encoding/echo', 'text=Caf%C3%A9+%E2%80%93+%F0%9F%8D%82', 'application/x-www-form-urlencoded') + + then: 'the filter and the controller both see the text as it was typed' + response.assertJsonContains(200, [text: 'Café – 🍂', readByFilter: 'Café – 🍂']) + } + + void 'decodes a form post as UTF-8 when it declares another charset'() { + when: 'a form encoded as UTF-8 is posted with ISO-8859-1 declared' + def response = httpPost('/encoding/echo', 'text=Caf%C3%A9', 'application/x-www-form-urlencoded; charset=ISO-8859-1') + + then: 'the forced request encoding wins over the declared charset' + response.assertJsonContains(200, [text: 'Café', readByFilter: 'Café']) + } + + @Unroll Review Comment: Nit: `@Unroll` can go; Spock 2 unrolls data-driven features by default. ########## grails-test-examples/app1/src/integration-test/groovy/functionaltests/encoding/CharacterEncodingSpec.groovy: ########## @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package functionaltests.encoding + +import java.net.http.HttpRequest + +import spock.lang.Issue +import spock.lang.Specification +import spock.lang.Tag +import spock.lang.Unroll + +import grails.testing.mixin.integration.Integration +import org.apache.grails.testing.http.client.HttpClientSupport + +/** + * A filter registered at {@code Ordered.HIGHEST_PRECEDENCE + 1} reads a request parameter before Grails sees the + * request. Once a filter reads a parameter, the container has decoded the body, so the encoding filter has to run + * before it. The other cases check which forms the forced request encoding decodes as UTF-8 when they declare + * another charset. + */ +@Integration +@Tag('http-client') +@Issue('https://github.com/apache/grails-core/issues/15921') +class CharacterEncodingSpec extends Specification implements HttpClientSupport { + + void 'decodes a form post as UTF-8 when a filter ahead of Grails reads its parameters'() { + when: 'a form is posted without a charset, through a filter that reads one of its parameters' + def response = httpPost('/encoding/echo', 'text=Caf%C3%A9+%E2%80%93+%F0%9F%8D%82', 'application/x-www-form-urlencoded') Review Comment: Nit: `httpPostForm` sends exactly this request: it encodes values with `URLEncoder` as UTF-8 and sends `application/x-www-form-urlencoded` without a charset. The input would then read the same as the assertion: ```groovy def response = httpPostForm('/encoding/echo', [text: 'Café – 🍂']) ``` The second case has to stay on `httpPost`, since it declares a charset. ########## grails-test-examples/external-configuration/src/integration-test/groovy/test/app/LegacyEncodingConfigSpec.groovy: ########## @@ -0,0 +1,50 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package test.app + +import grails.testing.mixin.integration.Integration +import org.springframework.test.annotation.DirtiesContext +import org.springframework.web.filter.CharacterEncodingFilter +import spock.lang.Issue +import spock.lang.Specification +import spock.util.environment.RestoreSystemProperties + +/** + * {@code grails.config.locations} files are loaded after Spring Boot's environment post-processors run, and the + * deprecated {@code grails.filter.*} encoding properties in them still configure the encoding filter. + */ +@Integration +@RestoreSystemProperties Review Comment: Nit: a class-level `@RestoreSystemProperties` only wraps each feature, not `setupSpec()`. So `grails.config.locations` stays set to `classpath:legacyEncodingConfig.yml` after this spec. Nothing depends on that today, but `@TestPropertySource` avoids the system property altogether: ```groovy @Integration @TestPropertySource(properties = 'grails.config.locations=classpath:legacyEncodingConfig.yml') @Issue('https://github.com/apache/grails-core/issues/15921') class LegacyEncodingConfigSpec extends Specification { ``` That replaces `@RestoreSystemProperties`, `@DirtiesContext` and `setupSpec()`. The property is in the environment before `ExternalConfigRunListener` reads it, and it gives the context its own cache key. I tried it locally. The spec passes, and it fails without the annotation (UTF-8, forced). The module's other integration specs still pass. ########## grails-doc/src/en/guide/plugins/hookingIntoRuntimeConfiguration.adoc: ########## @@ -384,11 +384,11 @@ However, if you want to control the order of filter registrations you will need myFilter(FilterRegistrationBean) { filter = bean(MyFilter) urlPatterns = ['/*'] - order = Ordered.HIGHEST_PRECEDENCE + order = Ordered.HIGHEST_PRECEDENCE + 1 } ---- -NOTE: Grails' internal registered filters (`GrailsWebRequestFilter`, `HiddenHttpMethodFilter` etc.) are defined by incrementing `HIGHEST_PRECEDENCE` by 10 thus allowing several filters to be inserted before or between Grails' filters. +NOTE: Spring Boot registers the character encoding filter at `Ordered.HIGHEST_PRECEDENCE`, ahead of every other filter. Once any filter reads a request parameter, the servlet container has decoded the request body, so a filter that reads parameters needs a later order than the encoding filter, or form input arrives decoded as ISO-8859-1. A filter registered at exactly `Ordered.HIGHEST_PRECEDENCE` through a `FilterRegistrationBean` runs before it. Grails' own filters (`HiddenHttpMethodFilter`, `GrailsWebRequestFilter` etc.) have orders from -200 to -110, ten apart, ahead of the Spring Security filter chain at -100, which leaves room to insert filters before or between them. Review Comment: Nit: "orders from -200 to -110, ten apart" isn't quite right. Grails' filters are at -200 (`FIRST`, the security headers filter), -190 (asset-pipeline), -170, -160 and -150. Only `GrailsFilters.LAST` is at -110, 40 after the one before it. Perhaps: "have orders from -200 to -150, ten apart, and `GrailsFilters.LAST` is at -110, ahead of the Spring Security filter chain at -100". -- 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]
