codeconsole commented on code in PR #16149:
URL: https://github.com/apache/grails-core/pull/16149#discussion_r3918633199
##########
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:
Confirmed and fixed in a39a3502. The mechanism is as you describe:
`AnnotationFilterInvocationDefinition` resolves the URL to a controller/action
inside the chain via `matchAllUrlMappings`, which matched on
`grailsRequest.request.method`. So `POST /book/1` with `_method=DELETE`
resolved to the `update` mapping - reachable at all only because this branch
generates that POST route - was authorized against `update`, then executed as
`delete`.
Matching now resolves the override first, so the action security authorizes
is the action that runs. It closes in the safe direction: adding `_method` can
only select the stricter rule, never a weaker one, because both sides read the
same resolution. Under the servlet filter the request already reports the
overridden method and it resolves to that. Two tests in `ReflectionUtilsSpec`
assert the method the mapping lookup is handed, with and without an override.
##########
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:
Fixed in a39a3502 - the fallback now publishes what it resolves, so
`effectiveMethod` and `allowedMethods` agree with the route it picked. Without
it the request reached `delete` and was then refused a 405 for the POST it
arrived as, exactly as you describe. Test added.
##########
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:
You are right, and it is fixed in a39a3502 - every action writes the
attribute again. The optimization looked safe from inside the controller being
compiled, which is the flaw: the action that *reads* the attribute is in
whichever controller is entered second, so it is not knowable from the one
being compiled.
##########
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:
Fixed in a39a3502. `setMultipartRequest(..)` is back, `@Deprecated(since =
"8.0")`, publishing its argument as
`WebUtils.MULTIPART_HTTP_SERVLET_REQUEST_ATTRIBUTE` and discarding the cached
params, so an existing caller keeps working. Upgrade guide 54.4 now covers it
beside `getCurrentRequest()`, with the
attribute-plus-`multipartRequestResolved()` form for callers who want to move
off it.
##########
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:
Kept the broad catch but made it non-silent, in a39a3502. Narrowing to
`MultipartException` would lose the cases this exists for - the container fails
the parameter read itself, and Tomcat and Jetty do not all surface that as a
Spring exception. So the fallback still applies to any `RuntimeException` on a
multipart request, but only a `MultipartException` is the expected case:
anything else is logged at warn rather than debug, so an unrelated failure is
visible instead of passing as empty params.
##########
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:
Documented rather than detected, in a39a3502. Keying the mode on bean
presence means resolving a filter bean from three call sites during evaluation,
one of which runs while bean definitions are still being registered, so I would
rather not make mode resolution order-dependent in this PR. The upgrade note
now says plainly that an application registering its own
`HiddenHttpMethodFilter` must set the property too, otherwise it gets its
filter *and* dispatcher mode, including the generated POST member route.
##########
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:
Made prominent in a39a3502. The note now spells out the silent case rather
than only stating the narrowing: a proxy or SDK that rewrites `DELETE` to
`POST` plus `X-HTTP-Method-Override` has its `POST /books/1` answered by
`update` with a 200 while the caller believes it issued a `DELETE`, and the
remedy - enable the filter for an application such a client talks to - is
stated with it.
--
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]
