Copilot commented on code in PR #16146:
URL: https://github.com/apache/grails-core/pull/16146#discussion_r3775048523
##########
grails-web-mvc/src/main/groovy/org/grails/web/filters/HiddenHttpMethodFilter.java:
##########
@@ -71,14 +71,22 @@ protected void doFilterInternal(HttpServletRequest request,
HttpServletResponse
}
protected String getHttpMethodOverride(HttpServletRequest request) {
- String httpMethod = request.getParameter(methodParam);
+ String httpMethod = null;
+ if (!isMultipart(request)) {
+ httpMethod = request.getParameter(methodParam);
+ }
Review Comment:
This disables `_method` overrides for every multipart form, so existing
multipart PUT/PATCH/DELETE forms are now dispatched as POST. The newly
registered `MultipartFilter` runs at `GrailsFilters.FIRST`, before this filter,
so multipart parsing has already gone through the configured resolver; restore
parameter lookup rather than removing this supported behavior.
##########
grails-web-mvc/src/main/groovy/org/grails/web/servlet/mvc/GrailsWebRequestFilter.java:
##########
@@ -58,6 +59,9 @@ protected void doFilterInternal(HttpServletRequest request,
HttpServletResponse
boolean isIncludeOrForward = WebUtils.isForwardOrInclude(request);
GrailsWebRequest previous = isIncludeOrForward ?
GrailsWebRequest.lookup(request) : null;
GrailsWebRequest webRequest = new GrailsWebRequest(request, response,
getServletContext());
+ if (request instanceof MultipartHttpServletRequest) {
+ webRequest.setMultipartRequest(request);
+ }
Review Comment:
Checking only the outer request misses a multipart request hidden by an
earlier wrapper. `HiddenHttpMethodFilter` runs before this filter and wraps
requests when the override header is present, so downstream code using the
bound `GrailsWebRequest` can still see a non-multipart request until the
dispatcher runs. Resolve the native multipart request here, as the dispatcher
does.
##########
grails-web-mvc/src/main/groovy/org/grails/web/servlet/mvc/GrailsDispatcherServlet.groovy:
##########
@@ -89,6 +100,7 @@ class GrailsDispatcherServlet extends DispatcherServlet
implements ServletContex
if (webRequest != null) {
webRequest.multipartRequest = processedRequest
}
+ return processedRequest
Review Comment:
None of the new dispatcher multipart paths is exercised by the added tests.
Add coverage for an active `GrailsWebRequest` behind a nested request wrapper
and for `checkMultipart` returning and storing a newly resolved request, since
these are the paths that preserve valid uploads through security wrappers.
##########
grails-test-suite-uber/src/test/groovy/org/grails/web/filters/HiddenHttpMethodFilterTests.groovy:
##########
@@ -71,4 +70,23 @@ class HiddenHttpMethodFilterTests {
assertEquals "DELETE", method
}
+
+ @Test
+ void testMultipartRequestDoesNotParseParameters() {
+ def filter = new HiddenHttpMethodFilter()
+ def req = new MockHttpServletRequest() {
+ @Override
+ String getParameter(String name) {
+ throw new IllegalStateException('Multipart request parameters
must not be parsed by this filter')
+ }
+ }
+ req.contentType = 'multipart/form-data; boundary=test'
+ req.method = 'POST'
+ def res = new MockHttpServletResponse()
+ String method = null
+
+ filter.doFilter(req, res, { req2, res2 -> method = req2.method } as
FilterChain)
+
+ assertEquals('POST', method)
Review Comment:
This assertion codifies the multipart `_method` regression: a multipart form
containing `_method=DELETE` must still reach the chain as DELETE after the
earlier multipart resolver has wrapped it. Replace this with a resolved
multipart request carrying `_method` and retain a regression test that proves
no container-level parsing occurs before resolution.
##########
grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/ControllersAutoConfiguration.java:
##########
@@ -101,6 +102,20 @@ public CharacterEncodingFilter characterEncodingFilter() {
return characterEncodingFilter;
}
+ @Bean
+ @ConditionalOnMissingBean(MultipartFilter.class)
Review Comment:
This condition does not detect the common customization
`FilterRegistrationBean<MultipartFilter>` because the bean itself is a
registration wrapper, not a `MultipartFilter`. The auto-configuration can
therefore register a second multipart filter (or collide on the natural
`multipartFilter` bean name); include `FilterRegistrationBean` as a
parameterized container so user registrations back this bean off.
This issue also appears in the following locations of the same file:
- line 109
- line 115
--
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]
