feat: disable the hidden HTTP method filter by default - #16183
feat: disable the hidden HTTP method filter by default#16183codeconsole wants to merge 5 commits into
Conversation
…he filter is off Grails registers HiddenHttpMethodFilter unconditionally, rewriting a POST into a PUT, PATCH or DELETE when the request carries a _method parameter or an X-HTTP-Method-Override header. There is no way to turn it off, and the parameter read happens before the dispatcher runs: because a MultipartConfig is attached to the dispatcher servlet registration, Tomcat resolves it at filter time, so getParameter() on a multipart/form-data POST parses the whole body — temporary files and all — before any routing or authorization decision, and outside GrailsDispatcherServlet's MultipartException handling. Add grails.web.hiddenmethod.filter.enabled, defaulting to true so existing applications are unchanged. When it is false the override is not lost: it moves into the dispatcher, which resolves it after multipart handling — so a multipart body is parsed once, by the dispatcher — and after the filter chain, which therefore sees the request's real POST method. GrailsDispatcherServlet resolves the override in checkMultipart and publishes the wrapped request through GrailsWebRequest, so everything reading the request through the Grails API agrees on the method: a controller's 'request', allowedMethods, interceptors, and URL mapping resolution. Without that the generated allowedMethods check would still read POST and a form submit routed to 'delete' would be rejected with a 405. UrlMappingsHandlerMapping resolves the same override independently, keeping mapping resolution correct on its own terms. Both delegate to org.grails.web.util.HiddenHttpMethod so the rules cannot drift. The mapping-time resolution is deliberately narrower than the filter it stands in for: it reads only the _method parameter, not the X-HTTP-Method-Override header any client can set, and accepts only PUT, PATCH and DELETE — the three methods a browser form cannot submit itself — matching the set Spring's own HiddenHttpMethodFilter permits. The filter applies any method name it is given, including GET. Forms, scaffolded views and GSP templates are unchanged in either mode. Also fixes a startup failure new in 8.0: Boot's WebMvcAutoConfiguration registers its own filter under the same 'hiddenHttpMethodFilter' bean name and keys its @ConditionalOnMissingBean on Spring's filter type, which the Grails FilterRegistrationBean does not satisfy, so spring.mvc.hiddenmethod.filter.enabled=true failed application startup with a BeanDefinitionOverrideException. Grails' registration now backs off when Boot's property is explicitly enabled. In 7.x @EnableWebMvc kept Boot's bean from existing, so the collision arrived with its removal in 8.0. Claude-Session: https://claude.ai/code/session_01Pwd8dRc4WWHEPpbgrmxZmn
19f55a3 to
d17f416
Compare
codeconsole
changed the title
|
Note for anyone weighing the authorization risk called out above — it is concrete rather than theoretical, and it has a known remedy. Where it bites. if (requestMethod && iu.httpMethod && iu.httpMethod != HttpMethod.valueOf(requestMethod)) {
log.debug "Request '{} {}' doesn't match '{} {}'", requestMethod, url, iu.httpMethod, iu.pattern
continue
}With the filter enabled it runs at order A remedy already sits in the tree, unused. One constraint if that is pursued. Reading a request parameter inside the security chain re-creates, for Deliberately not part of this PR: different module, security-sensitive, and it deserves its own review. Raising it here so the risk is not mistaken for an unsolvable one. |
|
Added The bug. The five isolated Wiring both properties to the test source set brings 40 tests back into the build. 36 pass. The other four, and a defect they expose. They assert the validation-error branch of a controller action — an invalid domain instance should re-render the against @Entity
class Probe {
String title
static constraints = { title blank: false }
}The constraint is registered and That is pre-existing and unrelated to this PR — it was simply invisible while the tasks reported |
The previous commit made the filter optional and moved the override into the dispatcher when it was switched off. This makes that the default, because the reasons to switch it off apply to every application rather than a few. Reading a request parameter before the dispatcher runs is not free: a MultipartConfigElement is attached to the dispatcher servlet registration, so the container resolves it at filter time and reading _method on a multipart/form-data POST parses the entire body, writing its temporary files, before the request has been routed or authenticated. The filter is order -170 and the Spring Security chain is -100, so an unauthenticated request can cause uploads to be written to disk. Spring Boot disabled its equivalent filter in 2.2 for the same reason — it "causes early consumption of a request body if the body may contain parameters". For browser forms to keep working without the filter, two things change. A 'resources' mapping now also generates POST /$controller/$id -> update. RestfulController has declared update: ['PUT', 'POST'] since apache#9926 — raised because AngularJS $resource, and the clients modelled on it, POST to the member URL to save an existing object — but no route was ever generated, so that permission has been unreachable through a resources block. Generating it is also what lets a form submit reach update with no parameter at all. <g:form method="PUT"> therefore stops emitting _method and submits a plain POST to the same URL; the action attribute is unchanged, so no template needs editing. method="DELETE" still emits it, because delete and update share a URL and the parameter is what distinguishes them. method="PATCH" behaves as PUT, since RestfulController.patch() delegates to update(). In the new default _method appears in exactly one place: a delete form. Setting grails.web.hiddenmethod.filter.enabled back to true restores the Grails 7 behaviour exactly, including the X-HTTP-Method-Override header and the unrestricted method names, and suppresses the POST member route so a resources block generates the mappings it did before. The behavioural change to review when upgrading: servlet filters and the Spring Security chain now see a form delete as a bare POST to the member URL, so a rule matching DELETE /books/** no longer fires for it, and the URL does not distinguish update from delete. This is the one consequence that fails silently and it leads the upgrade note. Tests and the URL mappings report are updated for the added route, and FormTagLibResourceTests gains the delete coverage it never had — now the only place _method is expected. Claude-Session: https://claude.ai/code/session_01Pwd8dRc4WWHEPpbgrmxZmn
The five isolated Test tasks in grails-test-suite-uber were registered without testClassesDirs or classpath. A manually registered Test task inherits neither from the test source set, so each task resolved no candidate classes and reported NO-SOURCE. Because the main test task excludes exactly these patterns, the isolated classes ran nowhere: isolatedTestsOne, isolatedTestsTwo, isolatedRestRendererTests, isolatedPersonTests, isolatedRestfulControllerTests Wiring both properties to the test source set brings 40 tests back into the build. 36 pass. The remaining four assert the validation-error branch of a controller action — that an invalid domain instance re-renders the create or edit view — and are marked @PendingFeature because domain validation is not enforced in this unit-test harness: constraints are registered, and constrainedProperties reports them, but validate() returns true even for a null value on a property whose nullable constraint defaults to false. The invalid instance is therefore valid, the action takes its success path, and the assertions never see the error view. That is a pre-existing defect, unrelated to this change and merely revealed by it; the annotation fails the build if the behaviour is fixed and the tests start passing, so it cannot outlive the bug. Claude-Session: https://claude.ai/code/session_01Pwd8dRc4WWHEPpbgrmxZmn
…handler-mapping # Conflicts: # grails-doc/src/en/guide/upgrading/upgrading80x.adoc
7cdb385 to
75fb4ed
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16183 +/- ##
==================================================
+ Coverage 53.7450% 53.7628% +0.0179%
- Complexity 19821 19846 +25
==================================================
Files 2086 2087 +1
Lines 99667 99712 +45
Branches 17603 17616 +13
==================================================
+ Hits 53566 53608 +42
- Misses 38434 38435 +1
- Partials 7667 7669 +2
🚀 New features to boost your workflow:
|
…handler-mapping # Conflicts: # grails-doc/src/en/guide/upgrading/upgrading80x.adoc
✅ All tests passed ✅Test SummaryCI / Functional Tests (Java 21, indy=false, shard 1) > :grails-test-examples-app1:integrationTest
🏷️ Commit: 6e58f8b Learn more about TestLens at testlens.app/docs. |
|
We removed the dispatch actions in Grails 8 already (deprecated in Grails 7). Is _method just left over from that? I had no idea you could even change the method for a form - why wouldn't you either set the form to the intended method or just set the formmethod? It seems like _method is from an earlier implementation when there weren't built in constructs to change the method in html5. |
No, it is for <g:form method="PUT" action="/books">creates <form method="POST" action="/books">
<input type="hidden" name="_method" value="PUT">then HiddenMethodFilter wraps the entire request so the entire filter stack (including Spring Security thinks the request is This PR keeps the actual request and uses I a follow up PR would also be to not use If you only use POST and GET in your forms, this PR will only help you not forcing multipart resolution at the beginning of the filter stack |
2 participants
Description
Grails registers a servlet filter that rewrites a
POSTinto aPUT,PATCHorDELETEwhen the request carries a_methodparameter or anX-HTTP-Method-Overrideheader. It runs ahead of the dispatcher — and ahead of the Spring Security filter chain — and there is no way to turn it off.This PR disables it by default and resolves the override inside the dispatcher instead. Browser forms keep working with no template changes.
No approved issue exists, so — background:
Why the default should change
The filter reads a request parameter before the dispatcher runs.
ControllersAutoConfigurationattaches aMultipartConfigElementto the dispatcher servlet registration, and the container resolves it at filter time, so reading_methodon amultipart/form-dataPOST forces the container to parse the entire body — writing its temporary files — before the request has been routed or authenticated.GrailsFilters.HIDDEN_HTTP_METHOD_FILTERis order-170; the Spring Security chain is-100. An unauthenticated request can cause uploads to be written to disk.Spring Boot disabled its equivalent in 2.2 for the same reason — it "causes early consumption of a request body if the body may contain parameters" — and reported the filter taking up to 80% of processing time on some endpoints.
The Grails filter is also wider than Spring's:
_methodparameterX-HTTP-Method-OverrideheaderPUT,PATCH,DELETEGETPUT,PATCH,DELETEWhat changes
GrailsDispatcherServletresolves the override incheckMultipart— after multipart resolution and after the filter chain — and publishes the wrapped request throughGrailsWebRequest. That publication is what makes it work end to end: a controller'srequest,allowedMethods, interceptors and URL mapping resolution all read through the Grails API and therefore agree on the method.UrlMappingsHandlerMappingresolves the same override independently so mapping resolution is correct on its own terms; both delegate to a neworg.grails.web.util.HiddenHttpMethod. Requests with no override are returned untouched, leaving multipart handling exactly as it was.POST /$controller/$id→updateis now generated by aresourcesmapping.RestfulControllerhas declaredupdate: ['PUT', 'POST']since #9926 — raised because AngularJS$resource, and the client libraries modelled on it, POST to the member URL to save an existing object — but no route was ever generated, so that permission has been unreachable for nine years. This is also what lets a form submit without a_methodparameter at all.<g:form method="PUT">stops emitting_method. It submits a plainPOSTto the same URL, which the route above answers — theactionattribute is unchanged.method="DELETE"still emits it, becausedeleteandupdateshare a URL and the parameter is what distinguishes them.method="PATCH"behaves asPUT, sinceRestfulController.patch()delegates toupdate().So in the new default,
_methodappears in exactly one place: a delete form.Restoring the old behaviour is one property, which also suppresses the
POSTmember route so aresourcesblock generates exactly the mappings it does today:Also fixed: a startup failure that is new in 8.0
Boot's
WebMvcAutoConfigurationregisters its own filter under the samehiddenHttpMethodFilterbean name, and its@ConditionalOnMissingBeankeys onorg.springframework.web.filter.HiddenHttpMethodFilter, which the GrailsFilterRegistrationBeandoes not satisfy. With bean-definition overriding disabled by default,spring.mvc.hiddenmethod.filter.enabled=truefailed application startup with aBeanDefinitionOverrideException. Grails' registration now backs off. This could not occur in 7.x, where@EnableWebMvckept Boot's bean from existing.The one thing that fails silently in the unlikely that you would have such a specific url mapping rule
Spring Security now receives the actual request urls instead of the fake ones created by HttpHiddenMethodFilter
Servlet filters and the Spring Security chain now see a form delete as a bare
POST /books/1, because the rewrite happens inside the dispatcher. A security rule matchingDELETE /books/**will no longer fire, and since the URL is unchanged,updateanddeletecannot be distinguished by path either. This is called out at the top of the upgrade-guide section, and it is the strongest argument for reviewers who may prefer to ship the property in 8.0 and defer the default flip to 9.0 — that is a one-line change to this PR if the team prefers it.A note on scope
This flips a default for every application on upgrade. 8.0 is the release where that is permitted, and the alternative — shipping the property defaulted to
true— leaves every app paying pre-authentication multipart parsing that Spring Boot decided against six years ago. I have written it as the flip, but the decision is the team's and the property makes either choice cheap.Testing
HiddenHttpMethodSpec— resolution rules, including that the header is not read andGET/TRACE/unknown names are refusedGrailsDispatcherServletHiddenMethodSpec— the dispatched request andGrailsWebRequest.currentRequestboth report the overridden method (whatallowedMethodsreads), and a request without an override is returned untouchedHiddenHttpMethodHandlerMappingSpec— form POSTs reachingPUT/PATCH/DELETEroutes, the$resource-style POST reachingupdatewith no parameter, and nothing generated when the filter is enabledControllersAutoConfigurationSpec— default registration, the restore property, the Boot collision, user-bean back-offFormTagLibResourceTests— updated for the new default, plusdeletecoverage that did not previously existRestfulResourceMappingSpec,AnsiConsoleUrlMappingsRendererSpec— mapping counts and the rendered mappings report updated for the added routeDocumentation
grails-docupgrade guide section 48 and the REST guide's Linking to Resources page.Generative AI tooling (Claude Code) was used in preparing this contribution, in line with the ASF policy on generative tooling. All changes were reviewed and verified against the project's test and style gates by the submitter.
https://claude.ai/code/session_01Pwd8dRc4WWHEPpbgrmxZmn