codeconsole commented on PR #16183:
URL: https://github.com/apache/grails-core/pull/16183#issuecomment-5363598483
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.**
`AbstractFilterInvocationDefinition.findConfigAttributes` compares a rule's
declared method to the request's actual method
(`grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/web/access/intercept/AbstractFilterInvocationDefinition.groovy:129`):
```groovy
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 `-170`, ahead of the security chain
at `-100`, so `requestMethod` is already `DELETE` by the time this executes.
With the filter disabled it is `POST`, and every Requestmap or `intercept.map`
entry declared with `httpMethod: DELETE` is skipped for a browser form delete.
The action still runs; the rule guarding it simply does not match, and nothing
errors.
**A remedy already sits in the tree, unused.**
`grails.plugin.springsecurity.web.filter.HttpMethodOverrideDetector` reads
`_method` and returns the intended method. It is referenced by nothing but its
own spec — it arrived with the Spring Security plugin merge and was never wired
up, which makes sense, because while the filter is enabled it can only tell you
what `request.getMethod()` already says. Once the filter is off it becomes the
missing piece: it would let the security layer resolve the intended method for
itself.
**One constraint if that is pursued.** Reading a request parameter inside
the security chain re-creates, for `multipart/form-data`, the
pre-authentication body parsing this PR exists to eliminate. That is avoidable
by resolving `_method` only for `application/x-www-form-urlencoded` requests —
a delete form is always form-encoded, and a multipart POST is a file upload,
never a delete. The gap closes without any upload body being touched before
authentication.
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.
--
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]
