Let oversized uploads reach the application's error handling - #16152
Draft
codeconsole wants to merge 24 commits into
Draft
Let oversized uploads reach the application's error handling#16152codeconsole wants to merge 24 commits into
codeconsole wants to merge 24 commits into
Conversation
The request Grails exposed to controllers, tag libraries and GSPs was replaced with the resolved MultipartHttpServletRequest for file uploads. That discarded every request wrapper contributed after multipart resolution - the hidden HTTP method filter, Spring Security, and any application filter - and required a mutable pointer on GrailsWebRequest plus propagation code to maintain it. The request is now always the outermost request, and multipart capabilities are discovered from its wrapper chain via WebUtils.resolveMultipartRequest. When the DispatcherServlet resolves a request Grails had already bound, the wrapper sits above that request and cannot be reached by unwrapping, so it is also published as a request attribute. - Add WebUtils.resolveMultipartRequest and isMultipartContentType - Add the MultipartRequest read surface to HttpServletRequestExtension so request.getFile(..) and friends keep working, failing loudly rather than returning null when the request is not a resolved multipart request - Populate GrailsParameterMap through discovery rather than an instanceof check - Replace GrailsWebRequest.setMultipartRequest and the multipart branch in getCurrentRequest with multipartRequestResolved, which only invalidates the cached params (apachegh-13837) - Return the processed request from GrailsDispatcherServlet.checkMultipart, so the dispatch runs against it as Spring MVC expects - Delete the unreachable multipart resolution in DefaultUrlMappingInfo, along with the undocumented grails.web.disable.multipart setting - Make the SpringSecurityUtils multipart branch functional again; it read an attribute only the deleted DefaultUrlMappingInfo code ever wrote request instanceof MultipartHttpServletRequest and casts to that type no longer work; documented in the 8.0 upgrade guide.
isMultipartContentType had no production caller - only the test written for it. Condense the six per-method javadoc blocks on the file upload accessors into one.
…ntext GrailsWebRequest built a GrailsApplicationAttributes on every request, through a reflective Constructor.newInstance. That object holds no request state - it caches the beans its own comment calls "used very often" (template engine, GrailsApplication, GroovyPagesUriService, MessageSource, plugin manager) - so building one per request paid for the reflection and then discarded all five caches immediately. It is now created on first use and cached in the servlet context, and rebuilt only if the ApplicationContext it resolved against is no longer current, so a replaced or restarted context (as happens between tests) is never served a stale instance. Its lazily populated fields become volatile now that one instance is shared across request threads. Also stop allocating a UrlPathHelper per request or per call. Spring exposes UrlPathHelper.defaultInstance and none of the four Grails instances were configured, so they can share it.
…ceptors UrlMappingsHandlerMapping.getHandlerExecutionChain re-implemented the loop from AbstractHandlerMapping, so Grails-mapped requests silently missed whatever Spring added to that method later. Currently that is the API version deprecation interceptor, which means the Deprecation, Sunset and Link headers configured by spring.mvc.apiversion.* were never emitted for a Grails-mapped request. It now calls super and inserts the WebRequestInterceptors at the front, which keeps the "OSIV must run first" ordering that motivated the override. The two Grails interceptors are stateless, so they become shared instances instead of two allocations per request, and the @CompileDynamic MappedInterceptor cast helper goes away with the copied loop. Also: - Keep UrlMappingsHandlerMapping's own UrlPathHelper. UrlPathHelper.defaultInstance is read-only and that field is protected, so pointing it at the shared instance would break any subclass configuring it. It is a singleton bean, so there was no per-request allocation to save there anyway. - Restore the previous LocaleContext in GrailsWebRequestFilter rather than clearing it, so a LocaleContext set by a filter outside Grails survives, matching Spring's own RequestContextFilter.
GrailsParameterMap's constructor defensively copied request.getParameterMap() into a LinkedHashMap before walking it. updateNestedKeys only ever reads that map - every put it makes goes into wrappedMap or a nested map it created - so the copy was only ever needed to merge uploaded files in. The servlet map (immutable per the servlet contract) is now walked directly, and copied only when there are multipart files to merge, removing a map allocation and a full entry copy per request for every non-upload request.
A URL mapping cache miss was the most expensive thing in the request path by three orders of magnitude - 1964 ns for a URI only the default mapping serves, against 2.5 ns for a cache hit - because every miss ran a linear scan allocating a Matcher for each of the ~56 compiled patterns in a mid-size application. RegexUrlMapping now records each pattern's slash count at parse time and skips patterns whose segment count rules them out. Every construct convertToRegex emits is bounded to a single path segment except ".*", which comes only from a "**" token, so a pattern without "**" can only match a URI with exactly its slash count or one more, the extra one coming from the trailing "/??" every pattern ends with. Patterns containing "**" are never skipped. Candidates are skipped, never reordered, so the scan still returns the first mapping that matches and declaration precedence is unchanged. This replaces the patternByTokenCount map, which built exactly this index and was never read by anything. The holder computes the URI's slash count once per request rather than once per mapping, and hoists the per-candidate LOG.isDebugEnabled() call out of the three scan loops.
The adapter passed its callback to observe() as `{ -> i.before() } as BooleanSupplier`.
Groovy evaluates that coercion before entering observe(), so it ran even when the
ObservationRegistry is a no-op, and DefaultGroovyMethods.asType routes it through
CachedSAMClass.coerceToSAM to Proxy.newProxyInstance. Every matched interceptor
therefore cost a Closure, a Class[], a ConvertedClosure and a JDK dynamic proxy per
phase per request, with each callback dispatching reflectively through
ConversionHandler rather than calling the interceptor directly.
The phase is now a private enum that dispatches straight to before()/after(), so the
default path is a field read, a no-op check and an interface call. In the compiled
class groovy.lang.Reference references drop from 15 to 0 and the two closure classes
are gone. The observing path is structurally unchanged.
Also caches the logical interceptor name per class rather than recomputing it per
interceptor per phase per request, and reverses the matched-interceptor list in place
rather than copying it - the reversed list is stored back under the request attribute
and read by afterCompletion, so the ordering remains observable and unchanged.
Adds coverage for the observation path, which previously had none, including the
null-registry branch, the no-op registry branch, and error recording.
The controller AST transformer emitted the ALLOWED_METHODS_HANDLED request-attribute guard twice into the same generated wrapper - once from convertToMethodAction and again from wrapMethodBodyWithExceptionHandling - producing two byte-identical blocks where the second could never do anything, because the first had already set the attribute. It also emitted the guard, and its finally-block cleanup, for controllers that declare no allowedMethods at all. An action on a controller with no allowedMethods was paying four dynamic request property gets, two getAttribute, a setAttribute, a removeAttribute and a compareEqual per request for a guard that could never fire. Each request property get goes through an indy callsite and RequestContextHolder, so this was not free. The duplicate emission is removed, and the bookkeeping is now generated only for controllers that declare a non-empty allowedMethods map. Gating it per action rather than per controller looks equivalent but is not: the marker means "an action has already begun handling this request" (apachegh-11444), so an unrestricted action must still set it, or a restricted action it invokes programmatically starts rejecting the request. Controllers that use allowedMethods generate byte-identical code to before. Adds coverage for the command-object path, which had none.
…okup Binding a command object resolved the DataBindingSourceRegistry, the MimeTypeResolver and the GrailsWebDataBinder from the bean factory on every request, each with a containsBean followed by a getBean, and did so twice because bindObjectToInstance runs createDataBindingSource again. Holders.findApplication() is itself a getBean rather than a field read, and was called twice more per bind. These now resolve once per ApplicationContext, held in a single-entry volatile cache. A map keyed by ApplicationContext would retain every context ever seen, since the cached beans reference the context, so a single entry replaced whenever a different context appears is both cheaper and the correct invalidation signal for dev restarts and test contexts. Separately, getBindingIncludeList used getDeclaredField to look up the AST-injected whitelist field and cached the result after the call. For any class the transformer did not touch - inner-class command objects, precompiled classes, plain POJOs - that call threw, control jumped past the caching, and the exception was reconstructed on every subsequent bind. It now uses ReflectionUtils.findField, which returns null, and caches the negative result too, while still requiring the field to be declared on the class itself so an untransformed subclass does not inherit its parent's whitelist.
…ing paths Four lookups repeated per request, all resolving to values that are stable: - Every redirect read the controller's static namespace field reflectively, through a hierarchy walk plus makeAccessible plus Field.get. The in-code comment already noted this was avoidable. Now cached per controller Class; a reloaded class is a different Class object, so a stale namespace cannot be served. - Every redirect allocated a ResponseRedirector and called three setters on it. That object holds only configuration and takes the request, response and arguments per call, so one is now built lazily and reused. Each of its setters clears the cached instance, so a configuration change after the first redirect is still honoured. - Every template render resolved CompositeViewResolver from the bean factory. It is now held in a field, matching how this trait already caches the plugin manager, mime utility and layout selector. - The domain map constructor resolved the GrailsApplication and PersistentEntity, discarded them, then resolved both again to autowire the instance. They are now resolved once and passed down. The redirector is held in an AtomicReference rather than a volatile field: Groovy's trait field remapping drops the volatile modifier, and unlike the other cached values this object is constructed here after its setters run, so it needs safe publication.
GrailsWebRequest.getCurrentRequest() returned the resolved MultipartHttpServletRequest in place of the request Grails was bound to. That substitution is gone, so the method is now literally `return getRequest();`. The two can never disagree. getRequest() is final on Spring's ServletRequestAttributes and fixed at construction, and nothing wraps or replaces the request for the lifetime of a GrailsWebRequest: includes and forwards wrap only the response and dispatch the same request object, layout decoration swaps the response and re-renders against the original request, and async builds a new GrailsWebRequest around the request it is given. The two places that do cope with a later request wrapper avoid this method entirely - multipart through WebUtils.resolveMultipartRequest, and Spring Security by binding a fresh DelegatingGrailsWebRequest. All 65 framework call sites now use getRequest(). The method is deprecated rather than deleted so plugins keep compiling; removing it is a separate decision. - Move the "always the outermost request" note to the class javadoc, where it outlives the deprecated method - Keep getCurrentRequest in DelegatingGrailsWebRequest's @DeleGate exclusions. Delegating it would hand back the request from earlier in the filter chain, which is what that filter exists to prevent. Both reasons the exclusion list exists are now written down - Cover that filter with a spec; it had none - Stop JsonViewTemplateResolverSpec mocking GrailsWebRequest and stubbing getCurrentRequest(). It relied on the deprecated method being the only stubbable request accessor and produced an object whose two accessors disagreed; it now drives a real GrailsWebRequest over a MockHttpServletRequest
Covers controller action invocation (with and without allowedMethods, and a command-object action), the interceptor chain with a no-op and an observing registry, and collectControllerMappings - the uncached wrapper that runs on every request even when the URL mapping cache hits. The existing benchmarks measured GrailsWebRequest construction (12 ns) and multipart resolution (1.7 ns), neither of which is where request time goes.
getRequest() is final on ServletRequestAttributes, so getCurrentRequest() was the only stubbable request accessor on GrailsWebRequest. Tests that mocked it will see framework code take a different path now that it calls getRequest() directly.
The filter sets the locale from the request unconditionally, but restored the previous LocaleContext only on the outermost dispatch. An include or forward therefore left the enclosing request with the locale it had installed, and replaced any TimeZoneAwareLocaleContext with a plain SimpleLocaleContext for the remainder of that request. The restore now happens on every invocation, matching the unconditional set. Only the GrailsWebRequest handling stays branched, since an include restores the previous web request rather than clearing it. This filter had no test coverage; adds one, including a case that fails without the change.
codeconsole
marked this pull request as draft
August 15, 2026 05:55
…8.0.x Upstream landed the mass-assignment hardening (apache#15947) and the clearMissing work (apache#15950), both of which rewrote the DataBindingUtils methods this branch had touched in "Cache the data binding collaborators and the databinding whitelist lookup". The conflict is resolved in favour of upstream everywhere the two overlap, so that the deny-by-default binding behaviour is exactly the one upstream shipped. Superseded by upstream and dropped from this branch: * The whitelist include-list caching in getBindingIncludeList. Upstream's rewrite already caches the negative result behind a NO_BINDING_INCLUDE_LIST sentinel and resolves the runtime bindable names only on a cache miss, and it keys the cache on whether deny-by-default is enabled, which this branch's single cache could not express. The method is taken from upstream verbatim. * The resolveBindingIncludeList helper. Upstream's getField / getPairedField / getStaticListFieldValue replace it and fix the same defect: the lookup no longer lets getDeclaredField throw for a class the AST transform never enhanced, so nothing is owed here any more. The helper also honoured only a whitelist declared on the class itself, whereas upstream deliberately walks the superclass chain, so DataBindingUtilsSpec now asserts that an inherited whitelist applies. Its test of the private include-list cache is dropped: the negative result is still covered through the public binding API, and upstream now keeps two caches rather than the one the test reached into. Kept from this branch: * The ContextBoundBeans cache of the data binding collaborators, which upstream does not touch. * Resolving the GrailsApplication once per bind and passing it down. It now travels through a private bindObjectToDomainInstance overload which runs upstream's include normalisation, so the include.isEmpty() / NO_BINDABLE_PROPERTIES handling and the clearMissing && explicitInclude gating apply on every path, including bindToCollection.
…er tests Holders keeps its application discovery strategies in a static list and consults them in registration order, and tests share a JVM fork. The spec registered its own strategy but did not clear the list first, so a strategy left behind by an earlier test - holding an application context that had since been closed - was asked first and threw IllegalStateException before the spec's strategy was reached. Clearing in setup as well as cleanup makes the spec independent of whatever ran before it.
An upload breaching the configured multipart limits fails when the container parses the request parts, and every parameter read on that request fails with it from then on. Grails reads request parameters twice before the DispatcherServlet runs - HiddenHttpMethodFilter resolves the _method override, and GrailsParameterMap is built by any filter ahead of the dispatch, Spring Security among them. Either read aborted the request inside the filter chain, where no HandlerExceptionResolver can see it, so the application was left with the servlet container's own error page: a raw 413 on Tomcat, with the container's error page dispatch double-faulting on the same unreadable parameters. Both reads now tolerate a multipart request the container refuses to parse, so the failure is left for DispatcherServlet.checkMultipart to raise as a MultipartException during dispatch. The exception reaches the HandlerExceptionResolver chain and the response is rendered through the application's error dispatch. The tolerance is confined to multipart requests - an unreadable parameter map on any other request still propagates - and the parse failure is logged at debug rather than discarded. Such a request cannot reach a controller either way, because the dispatcher rejects it before handler resolution. - Add WebUtils.isMultipartContentType, shared by both call sites - Cover both the tolerated and the still-propagating case in the existing tests apachegh-16145
app1 runs a real embedded container with the Spring Security filter chain in front of it, which is the configuration both halves of apachegh-16145 describe. Neither half had a functional test. - An upload past the configured limit must be rendered by the application's error dispatch rather than by the container. Without the accompanying fix this returns Tomcat's own 413 HTML page. - g:uploadForm(method: 'PUT') emits multipart plus _method, so the override has to keep working for a multipart request rather than being skipped along with the parameter read. The existing specs already cover request.getFile(..) behind the security filter chain, which is the other behaviour the issue reports as broken. apachegh-16145
A multipart body the container refuses to parse - an upload past the configured limits - leaves every parameter read on that request failing from then on. Two of Grails' own reads already tolerated that: the _method override in HiddenHttpMethodFilter, and the GrailsParameterMap constructor. They were not the only ones. ParamsAwareLocaleChangeInterceptor falls back to LocaleChangeInterceptor, which reads the parameter straight off the request, and it runs on every Grails-mapped dispatch - including the container's error dispatch for the very failure being reported. That read threw and took the error page down with it: an oversized upload to an application with a "413" UrlMappings handler returned the container's raw 500 page instead of the mapped response. GrailsExceptionResolver enumerates the request parameters for its log when grails.exceptionresolver.logRequestParameters is set, which defaults to on in development, and GroovyPageView reads showSource when it renders a GSP in development - both on that same error path. The tolerance moves into WebUtils as readParameterMap, readParameter and readParameterNames, so one place decides what an unreadable multipart request yields, and one place keeps the tolerance confined to multipart requests. Every framework read goes through it. Verified against two containers, because the catch is deliberately not container-specific. Tomcat 11 throws org.apache.tomcat.util.http.InvalidParameterException from every parameter accessor; Jetty 12 throws org.eclipse.jetty.http.HttpException$IllegalStateException from getParameterMap while getParameter and getParameterNames succeed. Catching RuntimeException covers both. apachegh-16145
A "500" URL mapping that names a controller is reached by forwarding to it. The forward re-enters the DispatcherServlet, and the forwarded dispatch resolves the same status code mapping again, because the error status attribute is still on the request. An error handler that fails for a reason belonging to the request rather than to the moment - an unparseable multipart body, a missing collaborator - therefore fails again inside its own forward, and that failure resolves straight back into resolveViewOrForward, which forwards to it again. Reproduced in grails-test-examples/app4 with an oversized upload, before the framework's parameter reads were made tolerant: 78,120 nested forwards, StackOverflowError, then OutOfMemoryError while logging the unwind - 1.9 million lines of log for one request. The error handler is no longer forwarded to while a forward to it is already running. The repeat failure is logged and the exception is returned to the DispatcherServlet, which reports it once through the container. The flag is cleared when the forward returns, so an error handler that ran successfully leaves a later, unrelated error on the same request free to use it. The resolver's own request-parameter log is read tolerantly for the same reason: it runs on the error path, on a request whose parameters may be exactly what the container could not read. apachegh-16145
app4 gets a "413" status code mapping onto a controller action, which is what the container's error dispatch has to reach for the failure to be rendered by the application rather than by the container. That dispatch runs the Grails handler mapping and its interceptors on a request whose parameters cannot be read, so it is the case the framework's parameter reads have to survive - and the case that was still broken. The jetty and undertow examples get the same endpoint, because the tolerance is written against RuntimeException rather than a container-specific exception type and that reasoning was untested. Undertow does not behave the same and the spec says so rather than asserting what the others do: it applies the servlet multipart size limit while reading the request entity and refuses the request at the HTTP layer, so no filter, handler or status code mapping runs. The response is a bare 413 with an empty body, and there is nothing the application can contribute to it. apachegh-16145
- The "413" response code mapping that renders it, and what is and is not available to the action it names: the action runs on the container's error dispatch, so params is empty and request.getFile(..) is unavailable - Undertow refusing the request at the HTTP layer, before any application code runs, so no mapping is consulted and the body is empty - The error handler no longer being forwarded to from inside its own forward apachegh-16145
codeconsole
force-pushed
the
fix/multipart-lean-8.0.x
branch
from
August 15, 2026 21:59
e456f0a to
3a76625
Compare
✅ All tests passed ✅🏷️ Commit: 3a76625 Learn more about TestLens at testlens.app. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #16145.
An oversized multipart upload never reaches the application. Grails reads request parameters
before the
DispatcherServletruns —HiddenHttpMethodFilterlooks for_method, andGrailsWebRequestFilterbuildsGrailsParameterMap— and reading any parameter of amultipart/form-datarequest makes the container parse the body. A body past the configuredlimit fails that parse, inside the filter chain, where no
HandlerExceptionResolvercan see it.Spring resolves multipart in
DispatcherServlet.checkMultipart, insidedoDispatch, which iswhy a plain Spring MVC application handles this correctly out of the box. Grails only differs
because it reads parameters first.
So the framework's parameter reads now tolerate a multipart body the container refuses to parse,
and the failure is left for
checkMultipartto raise during dispatch, where Spring routes it tothe exception resolvers and the application's error handling runs. Any non-multipart parameter
failure still propagates unchanged.
Measured
Same 200 KB upload against a 128000-byte limit, same probe, on three containers:
500, raw Tomcat HTML page413,{"error":"Content Too Large","handledBy":"errors.tooLarge"}400"Bad Request" — indistinguishable from a malformed request413, handled by the application413, empty body413, empty body — unchangedUndertow applies the limit while reading the request entity, so no filter or servlet runs and
there is nothing the framework can influence. That limitation is documented and pinned by a spec
so a change in it is noticed.
Also fixed
An error handler that itself fails was forwarded to indefinitely. With a
"413"or"500"controller mapping, the error dispatch re-entered
DispatcherServlet, resolved the same mapping,faulted on the same unreadable request and forwarded again — 78,120 nested frames,
StackOverflowError, thenOutOfMemoryErrorwhile logging the unwind.GrailsExceptionResolverno longer forwards to an error handler while a forward to it is already in progress. This is
independent of multipart: any error handler that throws would have looped.
Notes
<g:uploadForm method="PUT">(multipart plus_method) keeps working; there is a test.dispatcherType=ERROR, emptyparamsand no
request.getFile(..), since the body was never readable. Documented.grails-test-examples.