matrei commented on PR #16149:
URL: https://github.com/apache/grails-core/pull/16149#issuecomment-5471155801
# AI Review
Compared the checked-out branch at `a192c75725` with `origin/8.0.x` at
`4309d5b876`.
## Findings
### 1. High: Dispatcher-level hidden-method overrides are not propagated to
`GrailsWebRequest`
References:
-
`grails-web-mvc/src/main/groovy/org/grails/web/servlet/mvc/GrailsDispatcherServlet.groovy:115-122`
-
`grails-web-common/src/main/groovy/org/grails/web/servlet/mvc/GrailsWebRequest.java:269-270`
-
`grails-web-common/src/main/groovy/grails/web/api/ServletAttributes.groovy:47-49`
`checkMultipart()` returns a wrapper reporting `PUT`, `PATCH`, or `DELETE`,
but the `GrailsWebRequest` was already bound to the original request and
continues returning it. The override is only exposed through an attribute
consumed by `HiddenHttpMethod.effectiveMethod()`.
As a result:
- URL mapping resolution can select `PUT`, `PATCH`, or `DELETE`.
- `allowedMethods` sees the overridden method.
- `controller.request.method` still reports `POST`.
- `ServletRenderContext.getHttpMethod()` still reports `POST`.
- Command-object initialization still branches on `POST`.
- Method-keyed mapping names still use `POST`.
This contradicts the implementation comments and documentation claiming that
the Grails request API agrees with the selected route. The existing test
already expects `request.method == 'PUT'` in
`grails-test-examples/app1/src/integration-test/groovy/functionaltests/fileupload/FileUploadSpec.groovy:334-337`,
but the current lifecycle supplies the original request to the controller.
This should block approval until the request method authority is unified, or
the behavior is explicitly redesigned and all affected APIs are updated.
### 2. High: Multipart form fields can be lost from `params`
Reference:
-
`grails-web-common/src/main/groovy/grails/web/servlet/mvc/GrailsParameterMap.java:99-117`
The new code reads ordinary parameters from the outer request, then
discovers the inner `MultipartHttpServletRequest` but only merges its uploaded
files. It does not merge the multipart wrapper's parameter map.
Spring's `StandardMultipartHttpServletRequest` explicitly documents that
servlet containers are not guaranteed to expose multipart text parts through
the native request's `getParameterMap()`. Its own `getParameterMap()` merges
those parts. Therefore, with a container exhibiting that behavior, fields such
as `description`, `category`, or `_method` disappear from Grails `params`.
The existing `uploadWithMetadata` integration test expects multipart text
fields to be present, but coverage uses only the normal container path and does
not verify the outer-wrapper/native-parameter-map combination.
`GrailsParameterMap` should merge parameters from the resolved multipart
request as well as files, while retaining the tolerant behavior for an
unparseable oversized request.
### 3. High: The URL-mapping fallback resolves hidden methods during
forwards and includes
References:
-
`grails-web-mvc/src/main/groovy/org/grails/web/servlet/mvc/GrailsDispatcherServlet.groovy:93,115`
-
`grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/UrlMappingsHandlerMapping.groovy:89-94,178`
The dispatcher deliberately disables hidden-method resolution for
forwards/includes. However, `UrlMappingsHandlerMapping.resolveHttpMethod()`
still reads `_method` for every mapping lookup whenever
`resolveHiddenHttpMethod` is enabled.
A forwarded or included `POST` carrying `_method=DELETE` can therefore be
matched as `DELETE`, even though the dispatcher did not apply the override.
This can produce inconsistent routing and `allowedMethods` behavior, and may
let inherited request parameters influence an internal dispatch's action
selection.
The handler mapping should apply the same dispatch-type guard as the
dispatcher, or the dispatcher and mapping behavior should be made intentionally
consistent.
### 4. High: Generic PUT/PATCH forms no longer preserve their requested
method
Reference:
-
`grails-gsp/plugin/src/main/groovy/org/grails/plugins/web/taglib/FormTagLib.groovy:519-525`
With the filter disabled, `_method` is suppressed for every PUT/PATCH form,
not just forms targeting a resource member URL. This changes:
```groovy
<g:form url="/admin/update" method="PUT">
```
from a POST containing `_method=PUT` into a plain POST. Existing custom
PUT/PATCH-only mappings consequently stop matching.
The newly generated POST member route only compensates for `resources`
mappings. It cannot preserve arbitrary application mappings. The form tag
either needs to retain `_method` for non-resource forms or determine that the
target is covered by the new resource fallback.
### 5. High: Bare member POST introduces a new mutation route with
potentially different authorization semantics
References:
-
`grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingEvaluator.java:147-155,960-967`
-
`grails-rest-transforms/src/main/groovy/grails/rest/RestfulController.groovy:44`
The branch adds:
```text
POST /books/$id -> update
```
for every `resources` mapping when filter mode is disabled. A bare POST to a
member URL can now invoke `update`, whereas it did not previously.
The PR rationale says this does not widen authorization because security
sees both requests as POST. However, applications commonly distinguish
collection POST and member POST, and may have authorization rules that permit
collection POST while unintentionally allowing member POST. This is a new write
route and should be treated as a security-sensitive behavior change.
The upgrade documentation currently understates this consequence.
### 6. Medium: `SpringSecurityUtils.isAjax()` still performs an unsafe
multipart parameter read
Reference:
-
`grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/SpringSecurityUtils.groovy:303-310`
The method directly calls:
```groovy
request.getParameter('ajax')
```
before the tolerant multipart handling. For an oversized multipart request,
this can fail inside the Spring Security filter chain, before
`DispatcherServlet.checkMultipart()` can route the multipart exception through
Grails error handling.
The direct parameter read should use `WebUtils.readParameter()`, and Spring
Security integration coverage should include oversized multipart requests
handled by authentication and access-denied flows.
### 7. Medium: `HttpMethodOverrideDetector` remains inconsistent with the
tolerant parameter path
Reference:
-
`grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/web/filter/HttpMethodOverrideDetector.groovy:46-52`
This Spring Security extension point still calls `request.getParameter()`
directly. If configured ahead of dispatcher multipart handling, an oversized
multipart request can fail in the filter chain rather than reaching the
intended application error pipeline.
No default production call site was found, so this is primarily an extension
compatibility risk, but it should either use the tolerant helper or be
documented as intentionally outside the new behavior.
### 8. Medium: Interceptor post-processing now mutates request state in place
Reference:
-
`grails-interceptors/src/main/groovy/org/grails/plugins/web/interceptors/GrailsInterceptorHandlerInterceptorAdapter.groovy:118-120`
The previous implementation created a reversed copy of the
matched-interceptor list. The new implementation reverses the request attribute
in place.
A second `postHandle()` call now reverses the list back, changing callback
order between invocations. The existing spec already invokes `postHandle()`
twice, but only checks the model/view result, not the callback order. Nested
dispatches, forwards/includes, async redispatches, or custom lifecycle
invocations can expose this difference.
The list should be copied before reversal, or the lifecycle should
explicitly guarantee that `postHandle()` is called only once.
## Additional review notes
- The branch is clean and is 53 commits ahead of `origin/8.0.x` at the
refreshed head `a192c75725`.
- The GORM/native-ID changes visible in the branch are already present in
`origin/8.0.x`; they are not additional PR changes.
- The captured CI state was not fully green. CodeQL reported 11 new
high-severity alerts, and the captured merge state was unstable.
- Patch coverage was approximately 69.7%, with 134 changed lines not covered.
- The most important missing coverage is a full request-path test combining
filter ordering, multipart resolution, dispatcher method wrapping, URL mapping,
`GrailsWebRequest`, controller request access, `allowedMethods`, command-object
binding, REST rendering, and Spring Security.
--
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]
