jdaugherty commented on code in PR #16149:
URL: https://github.com/apache/grails-core/pull/16149#discussion_r3918143875
##########
grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/ReflectionUtils.groovy:
##########
@@ -206,7 +206,7 @@ class ReflectionUtils {
static UrlMappingInfo[] matchAllUrlMappings(UrlMappingsHolder
urlMappingsHolder, String requestUrl,
GrailsWebRequest
grailsRequest, HttpServletResponseExtension extension) {
- String method = grailsRequest.currentRequest.method
+ String method = grailsRequest.request.method
Review Comment:
**Effective-method split between security and dispatch (privilege-escalation
surface).** With the filter off (the new default), the security chain still
matches URL mappings against `grailsRequest.request.method` (a plain `POST`),
while `GrailsDispatcherServlet` resolves the `_method` override and
`UrlMappingsHandlerMapping`/`AllowedMethodsHelper` route and authorize on
`HiddenHttpMethod.effectiveMethod(...)`. Combined with the new `POST
/$controller/$id -> update` route, a browser form `POST /book/1` with
`_method=DELETE` is authorized by Spring Security as the `update` mapping, then
executed by the dispatcher as `delete`. If `update` carries a weaker
`@Secured`/rule than `delete`, that is an authorization bypass.
The PR upgrade notes acknowledge that `update` and `delete` are no longer
distinguishable by path; flagging the concrete escalation path so reviewers
weigh it explicitly. Consider publishing the resolved method to the request
before the security chain runs (or documenting a required security-rule change)
so the authorized method and the executed method agree. Otherwise, this is a
CVE that can't be merged.
##########
grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java:
##########
@@ -640,14 +665,23 @@ protected void wrapMethodBodyWithExceptionHandling(final
ClassNode controllerCla
final CatchStatement catchStatement = new CatchStatement(new
Parameter(new ClassNode(Exception.class), caughtExceptionArgumentName),
catchBlockCode);
final Statement methodBody = methodNode.getCode();
+ final BlockStatement codeToHandleAllowedMethods =
getCodeToHandleAllowedMethods(controllerClassNode, methodNode.getName());
+
BlockStatement tryBlock = new BlockStatement();
- BlockStatement codeToHandleAllowedMethods =
getCodeToHandleAllowedMethods(controllerClassNode, methodNode.getName());
- tryBlock.addStatement(codeToHandleAllowedMethods);
+ if (!codeToHandleAllowedMethods.isEmpty()) {
+ tryBlock.addStatement(codeToHandleAllowedMethods);
+ }
tryBlock.addStatement(methodBody);
final TryCatchStatement tryCatchStatement = new
TryCatchStatement(tryBlock, new EmptyStatement());
tryCatchStatement.addCatch(catchStatement);
+ if (codeToHandleAllowedMethods.isEmpty()) {
Review Comment:
**Behavior change: controllers that declare no (or an empty)
`allowedMethods` map no longer write the `ALLOWED_METHODS_HANDLED` request
attribute.** Previously every action generated the `if (attr not set) { ...;
set attr }` wrapper unconditionally, so a restricted action invoked
programmatically or via forward from an *unrestricted* controller had its
method check suppressed (the attribute was already set). With this early-return
the attribute is never set for such controllers, so a forward/chain from an
unrestricted controller into a method-restricted action now runs that action's
`allowedMethods` check against the original request method and can produce an
unexpected `405` mid-request. Worth a test for cross-controller invocation from
an `allowedMethods`-free controller.
##########
grails-web-common/src/main/groovy/org/grails/web/util/WebUtils.java:
##########
@@ -552,4 +564,95 @@ public static boolean
isForwardOrInclude(HttpServletRequest request) {
return isForward(request) || isInclude(request);
}
+ /**
+ * Locate the resolved multipart request for the given request, if there
is one. Normally found by
+ * unwrapping; when the {@code DispatcherServlet} resolved a request
Grails had already bound, the
+ * wrapper sits above it instead, so {@link
#MULTIPART_HTTP_SERVLET_REQUEST_ATTRIBUTE} is consulted too.
+ *
+ * @param request The request
+ * @return The resolved multipart request, or {@code null} when the
request is not multipart
+ */
+ public static MultipartHttpServletRequest
resolveMultipartRequest(HttpServletRequest request) {
+ MultipartHttpServletRequest resolved = getNativeRequest(request,
MultipartHttpServletRequest.class);
+ if (resolved != null) {
+ return resolved;
+ }
+ Object attribute =
request.getAttribute(MULTIPART_HTTP_SERVLET_REQUEST_ATTRIBUTE);
+ return attribute instanceof MultipartHttpServletRequest
multipartRequest ? multipartRequest : null;
+ }
+
+ /**
+ * Check whether the given request declares a multipart content type.
+ *
+ * @param request The request
+ * @return True if the content type is {@code multipart/*}
+ */
+ public static boolean isMultipartContentType(HttpServletRequest request) {
+ String contentType = request.getContentType();
+ return contentType != null &&
contentType.toLowerCase(Locale.ROOT).startsWith("multipart/");
+ }
+
+ /**
+ * Read the servlet parameter map, tolerating a multipart request the
container refuses to parse.
+ *
+ * @param request The request
+ * @return The parameter map, or an empty map when the parameters are
unreadable
+ * @see #readTolerantly(HttpServletRequest, Supplier, Object)
+ */
+ public static Map<String, String[]> readParameterMap(HttpServletRequest
request) {
+ return readTolerantly(request, request::getParameterMap,
Collections.emptyMap());
+ }
+
+ /**
+ * Read a single servlet parameter, tolerating a multipart request the
container refuses to parse.
+ *
+ * @param request The request
+ * @param name The parameter name
+ * @return The parameter value, or {@code null} when it is absent or
unreadable
+ * @see #readTolerantly(HttpServletRequest, Supplier, Object)
+ */
+ public static String readParameter(HttpServletRequest request, String
name) {
+ return readTolerantly(request, () -> request.getParameter(name), null);
+ }
+
+ /**
+ * Read the servlet parameter names, tolerating a multipart request the
container refuses to parse.
+ *
+ * @param request The request
+ * @return The parameter names, or an empty enumeration when they are
unreadable
+ * @see #readTolerantly(HttpServletRequest, Supplier, Object)
+ */
+ public static Enumeration<String> readParameterNames(HttpServletRequest
request) {
+ return readTolerantly(request, request::getParameterNames,
Collections.emptyEnumeration());
+ }
+
+ /**
+ * Perform a request parameter read that must not fail the request when
the container cannot parse a
+ * multipart body.
+ * <p>
+ * A {@code multipart/form-data} request breaching the upload limits fails
the container's part parsing,
+ * and every parameter read on it fails from then on. Grails reads
parameters before, alongside and after
+ * the handler, so throwing from any of them would replace the failure the
application should see with a
+ * secondary one raised where it cannot be handled. The read yields {@code
fallback} instead; the request
+ * still cannot reach a controller, because {@code
DispatcherServlet.checkMultipart} raises the multipart
+ * failure during dispatch. An unreadable parameter on a non-multipart
request still propagates.
+ *
+ * @param request The request
+ * @param read The read to perform
+ * @param fallback The value to use when the parameters are unreadable
+ * @return The read value, or {@code fallback} when the parameters are
unreadable
+ */
+ private static <T> T readTolerantly(HttpServletRequest request,
Supplier<T> read, T fallback) {
+ try {
+ return read.get();
+ }
+ catch (RuntimeException e) {
Review Comment:
**`readTolerantly` swallows *every* `RuntimeException` when the Content-Type
is multipart, not only container part-parse failures.** The only discriminator
is `isMultipartContentType(request)`, so an exception raised for an unrelated
reason on a multipart request (a filter-contributed wrapper vetoing
`getParameterMap()`, an already-consumed/aborted body, etc.) is downgraded to a
DEBUG log and an empty result. If `checkMultipart` then succeeds (e.g. lazy
resolution with the body within limits, or a different failure mode), the
controller runs with silently-empty params and form fields bind as `null`
instead of the request failing. Consider narrowing the catch to the multipart
exception types Spring actually raises for a rejected body (e.g.
`MultipartException` / `MaxUploadSizeExceededException`) rather than all
`RuntimeException`.
##########
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingEvaluator.java:
##########
@@ -135,6 +136,21 @@ public class DefaultUrlMappingEvaluator implements
UrlMappingEvaluator, ClassLoa
private final ConstraintRegistry constraintRegistry;
private final ConstraintsEvaluator constraintsEvaluator;
+ /**
+ * Whether a "resources" mapping should also route a POST to the member
URL at the update action.
+ *
+ * RestfulController has permitted POST for update since #9926 — raised
because AngularJS $resource, and
+ * the clients modelled on it, POST to the member URL to save an existing
object rather than sending a
+ * PUT — but no mapping was ever generated for it, leaving that permission
unreachable.
+ *
+ * Generated only while the hidden HTTP method filter is disabled. In that
mode the filter chain already
+ * sees a form's PUT as a bare POST to this URL, so the route adds no
request shape security had been
+ * able to distinguish; it does add a member URL that answers POST, which
the upgrade notes call out.
+ */
+ private boolean isPostUpdateVariantEnabled() {
+ return grailsApplication != null &&
!HiddenHttpMethod.isServletFilterMode(grailsApplication.getConfig());
Review Comment:
**Filter mode is derived only from the two config properties, not from
whether a hidden-method filter bean is actually registered.** The old
registration used `@ConditionalOnMissingBean(HiddenHttpMethodFilter.class)`, so
an application supplying its own `HiddenHttpMethodFilter` bean was a supported
extension point. `isServletFilterMode()` now checks only
`grails.web.hiddenmethod.filter.enabled` /
`spring.mvc.hiddenmethod.filter.enabled`, so such an app is treated as
*dispatcher* mode: `isPostUpdateVariantEnabled()` returns true and every
`resources` mapping silently gains a `POST /$controller/$id -> update` route
the app never contemplated (and `resolveHiddenHttpMethod` is enabled alongside
its filter). Consider keying the mode on the actual presence of a filter bean,
or documenting that a self-registered filter must also set the property.
##########
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/UrlMappingsHandlerMapping.groovy:
##########
@@ -59,11 +59,46 @@ class UrlMappingsHandlerMapping extends
AbstractHandlerMapping {
public static final String MATCHED_REQUEST = 'org.grails.url.match.info'
+ // Both are stateless, so one shared instance each rather than two
allocations per request.
+ private static final HandlerInterceptor OBSERVATION_ROUTE_HANDLER = new
ObservationRouteHandler()
+ private static final HandlerInterceptor ERROR_HANDLING_HANDLER = new
ErrorHandlingHandler()
+
+ /**
+ * Whether to resolve a "_method" parameter on a POST into the overridden
request method while matching
+ * URL mappings. Set when the hidden HTTP method filter is disabled. The
dispatcher normally wraps the
+ * request with the override before this runs, so this is the fallback for
one that does not.
+ */
+ boolean resolveHiddenHttpMethod = false
+
protected UrlMappingsHolder urlMappingsHolder
+ // Deliberately not UrlPathHelper.defaultInstance: that instance is
read-only, and this field is
+ // protected, so a subclass configuring it (alwaysUseFullPath and friends)
must keep working.
protected UrlPathHelper urlHelper = new UrlPathHelper()
protected MimeTypeResolver mimeTypeResolver
protected HandlerInterceptor[] webRequestHandlerInterceptors
+ /**
+ * The HTTP method to match URL mappings against.
+ *
+ * <p>An override the dispatcher already resolved is honoured wherever it
applies, forwards and includes
+ * included - the servlet filter's wrapper reports the overridden method
for the whole of a request, and
+ * an application is entitled to the same answer in either mode.</p>
+ *
+ * <p>Deriving a fresh override from the parameters is what an internal
dispatch must not do: it inherits
+ * the parameters of the request that started it, so a "_method" the
dispatcher never acted on would go
+ * on selecting an action for every forward after it.</p>
+ */
+ protected String resolveHttpMethod(HttpServletRequest request) {
+ if (!resolveHiddenHttpMethod) {
+ return request.getMethod()
+ }
+ String resolved = HiddenHttpMethod.effectiveMethod(request)
+ if (resolved != request.getMethod() ||
WebUtils.isForwardOrInclude(request)) {
+ return resolved
+ }
+ HiddenHttpMethod.resolveOverride(request) ?: resolved
Review Comment:
**Fallback override is used for routing but never published as
`HiddenHttpMethod.OVERRIDDEN_METHOD_ATTRIBUTE`.** When the dispatcher did not
resolve the override (e.g. a stock `DispatcherServlet` replaces
`GrailsDispatcherServlet`, or the handler mapping is driven standalone), this
branch derives `resolveOverride(request)` and matches mappings against it, but
`effectiveMethod(request)` / `AllowedMethodsHelper` still see the raw `POST`.
The request is then routed to the `update`/`delete` mapping yet fails its
generated `allowedMethods` check with `405`. If this fallback is meant to fully
stand in for the dispatcher, it should also set the overridden-method attribute
so the two agree.
##########
grails-web-common/src/main/groovy/org/grails/web/servlet/mvc/GrailsWebRequest.java:
##########
@@ -118,13 +162,15 @@ public GrailsWebRequest(HttpServletRequest request,
HttpServletResponse response
}
/**
- * Holds a reference to the {@link
org.springframework.web.multipart.MultipartRequest}
+ * Discards the cached params so they are rebuilt and pick up uploaded
files, for when multipart
+ * resolution happens after params were already read.
+ * See <a
href="https://github.com/apache/grails-core/issues/13837";>gh-13837</a>.
*
- * @param multipartRequest The multipart request
+ * @since 8.0
*/
- public void setMultipartRequest(HttpServletRequest multipartRequest) {
- this.multipartRequest = multipartRequest;
- this.originalParams = null; // originalParams will need to be
re-initialized. See https://github.com/apache/grails-core/issues/13837
+ public void multipartRequestResolved() {
Review Comment:
**Public `setMultipartRequest(HttpServletRequest)` was removed outright** —
unlike `getCurrentRequest()` it is neither `@Deprecated` nor mentioned in the
upgrade guide, and its replacement `multipartRequestResolved()` has different
semantics (it takes no request and only nulls the cached params). A plugin or
test harness that previously installed a resolved multipart request via
`webRequest.setMultipartRequest(resolved)` now fails to compile/link with no
drop-in replacement, and even after switching to `multipartRequestResolved()`
must additionally set `WebUtils.MULTIPART_HTTP_SERVLET_REQUEST_ATTRIBUTE` for
the request to be discoverable. Consider keeping a deprecated shim or calling
this out in the upgrade notes alongside `getCurrentRequest()`.
This is a breaking API change with no prior release deprecating the method
and needs fixed.
##########
grails-web-common/src/main/groovy/org/grails/web/util/HiddenHttpMethod.java:
##########
@@ -0,0 +1,144 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.web.util;
+
+import java.util.Locale;
+import java.util.Set;
+
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletRequestWrapper;
+
+import org.springframework.core.env.PropertyResolver;
+import org.springframework.http.HttpMethod;
+
+import grails.config.Settings;
+
+/**
+ * Resolves the hidden HTTP method override a browser form requests through a
{@code _method} parameter.
+ *
+ * <p>Browsers submit only {@code GET} and {@code POST}, so a form that needs
to reach a {@code PUT},
+ * {@code PATCH} or {@code DELETE} route names the method in a request
parameter instead. This is the same
+ * convention {@code org.grails.web.filters.HiddenHttpMethodFilter}
implements, applied inside the dispatcher
+ * rather than ahead of it — deliberately narrower than the filter, which
accepts any method name and also
+ * trusts an {@code X-HTTP-Method-Override} header.
+ *
+ * @since 8.0
+ */
+public final class HiddenHttpMethod {
+
+ /** Default method parameter: <code>_method</code> */
+ public static final String DEFAULT_METHOD_PARAM = "_method";
+
+ /** Spring Boot's equivalent of {@link
Settings#WEB_HIDDEN_METHOD_FILTER_ENABLED}, also false by default. */
+ public static final String SPRING_FILTER_ENABLED =
"spring.mvc.hiddenmethod.filter.enabled";
+
+ /**
+ * Request attribute carrying the method a request asked to be treated as,
published by the dispatcher
+ * when it resolves an override.
+ */
+ public static final String OVERRIDDEN_METHOD_ATTRIBUTE =
HiddenHttpMethod.class.getName() + ".METHOD";
+
+ /**
+ * The only methods a form may ask for: the three a browser cannot submit
itself. Matches the set
+ * Spring's own {@code HiddenHttpMethodFilter} permits, so a POST can
never be turned into a GET.
+ */
+ private static final Set<String> OVERRIDABLE_METHODS =
+ Set.of(HttpMethod.PUT.name(), HttpMethod.PATCH.name(),
HttpMethod.DELETE.name());
+
+ private HiddenHttpMethod() {
+ }
+
+ /**
+ * Whether a servlet filter rewrites the request method, rather than it
being resolved inside the
+ * dispatcher. True when either this application or Spring Boot has asked
for a filter.
+ * <p>
+ * Whenever this returns true a filter really is on the chain, so callers
need not check the context for
+ * one - see {@code GrailsHiddenHttpMethodFilterAutoConfiguration}, which
holds that invariant up.
+ *
+ * @param properties the environment or configuration to read
+ * @return true when a servlet filter performs the override
+ */
+ public static boolean isServletFilterMode(PropertyResolver properties) {
+ return
properties.getProperty(Settings.WEB_HIDDEN_METHOD_FILTER_ENABLED,
Boolean.class, Boolean.FALSE) ||
+ properties.getProperty(SPRING_FILTER_ENABLED, Boolean.class,
Boolean.FALSE);
+ }
+
+ /**
+ * The method this request is being handled as: the override the
dispatcher resolved, when there was one,
+ * and otherwise the request's own method.
+ * <p>
+ * Use this wherever a decision depends on the method the handler was
selected for -- {@code
+ * allowedMethods}, for instance -- rather than on the method the client
actually sent. A servlet filter
+ * doing the override rewrites {@link HttpServletRequest#getMethod()} and
this returns the same answer,
+ * so it is correct in either mode.
+ *
+ * @param request the current request
+ * @return the effective method name, never {@code null}
+ */
+ public static String effectiveMethod(HttpServletRequest request) {
+ Object overridden = request.getAttribute(OVERRIDDEN_METHOD_ATTRIBUTE);
+ return overridden instanceof String method ? method :
request.getMethod();
+ }
+
+ /**
+ * The method this request asks to be treated as, or {@code null} when it
asks for nothing: it is not a
+ * POST, carries no {@code _method} parameter, or names a method that may
not be requested this way.
+ *
+ * @param request the current request
+ * @return the overriding method name in upper case, or {@code null}
+ */
+ public static String resolveOverride(HttpServletRequest request) {
+ if (!HttpMethod.POST.name().equalsIgnoreCase(request.getMethod())) {
+ return null;
+ }
+ String requested = request.getParameter(DEFAULT_METHOD_PARAM);
+ if (requested == null || requested.isBlank()) {
+ return null;
+ }
+ String candidate = requested.toUpperCase(Locale.ROOT);
+ return OVERRIDABLE_METHODS.contains(candidate) ? candidate : null;
Review Comment:
**Silent narrowing of override handling in the new default mode.**
`resolveOverride` only honours a `_method` parameter on a `POST`, restricted to
`PUT`/`PATCH`/`DELETE`; the previously-registered Grails filter also honoured
the `X-HTTP-Method-Override` header and any method name. In default
(dispatcher) mode the header is now ignored and an out-of-set `_method` is
silently dropped rather than applied. This is called out in the upgrade guide,
but it is a silent runtime change for header-based REST clients (e.g. a proxy
that rewrites `DELETE` to `POST + X-HTTP-Method-Override`) — such a client's
`POST /books/1` now matches the new `POST -> update` route and returns `200`
from `update` while the caller believes it issued a `DELETE`. Worth ensuring
this is prominent in the migration notes.
--
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]
