Copilot commented on code in PR #16332:
URL: https://github.com/apache/grails-core/pull/16332#discussion_r3969787285
##########
grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/web/filter/IpAddressFilter.groovy:
##########
@@ -138,6 +134,44 @@ class IpAddressFilter extends GenericFilterBean {
false
}
+ /**
+ * Resolves the path that restriction patterns are matched against, in the
form Grails URL mapping dispatch
+ * resolves it: path parameters removed per segment before
percent-decoding (Jakarta Servlet 6.0 section 3.5.2),
+ * decoded exactly once (RFC 3986 section 2.4) and relative to the context
path, so an encoded or matrix-parameter
+ * variant of a restricted path is subject to the same restriction. For a
forwarded request the original request
+ * URI is checked, canonicalized the same way. Like dispatch, and unlike
RFC 3986 section 6.2.2.1, the context
+ * path is compared case-insensitively. A URI with an illegal percent
escape, which a Servlet 6.0 container
+ * rejects with 400 before the filter chain runs, is matched undecoded
rather than aborting the chain.
+ */
+ protected String getPathWithinApplication(HttpServletRequest request) {
+ String forwardUri =
request.getAttribute(WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE) as String
+ if (!forwardUri) {
+ try {
+ return urlPathHelper.getPathWithinApplication(request)
+ } catch (IllegalArgumentException ignored) {
+ return rawUrlPathHelper.getPathWithinApplication(request)
+ }
+ }
+ String path = urlPathHelper.removeSemicolonContent(forwardUri)
+ String contextPath =
(request.getAttribute(WebUtils.FORWARD_CONTEXT_PATH_ATTRIBUTE) as String) ?:
request.contextPath
+ try {
+ path = urlPathHelper.decodeRequestString(request, path)
+ contextPath = urlPathHelper.decodeRequestString(request,
contextPath)
+ } catch (IllegalArgumentException ignored) {
+ // illegal percent escape: match the undecoded path
+ }
+ if (contextPath && contextPath != '/' && path.regionMatches(true, 0,
contextPath, 0, contextPath.length())) {
+ path = path.substring(contextPath.length())
+ }
+ path ?: '/'
+ }
+
Review Comment:
In the forwarded-URI branch, the code removes matrix params and decodes but
does not appear to apply the same path sanitization that
`UrlPathHelper.getPathWithinApplication(request)` applies (notably collapsing
duplicate slashes as referenced elsewhere in this PR’s tests/docs). This can
reintroduce a dispatch/matcher inconsistency specifically for forwarded
requests. Consider applying the same sanitization rules used by `UrlPathHelper`
(e.g., duplicate-slash collapse / path sanitization) to `forwardUri` before
matching restrictions, so forward matching stays aligned with dispatch
semantics.
##########
grails-interceptors/src/main/groovy/grails/artefact/Interceptor.groovy:
##########
@@ -89,25 +107,21 @@ trait Interceptor implements ResponseRenderer,
ResponseRedirector, RequestForwar
allMatchers << matcher
}
- HttpServletRequest req = request
- String ctxPath = req.contextPath
- String uri = req.requestURI
- String noCtxUri = uri - ctxPath
- boolean checkNoCtxUri = ctxPath && uri.startsWith(ctxPath)
-
- def matchedInfo =
request.getAttribute(UrlMappingsHandlerMapping.MATCHED_REQUEST)
-
- UrlMappingInfo grailsMappingInfo = (UrlMappingInfo) matchedInfo
+ String uri
+ try {
+ uri =
UrlPathHelper.defaultInstance.getPathWithinApplication(request)
+ } catch (IllegalArgumentException ignored) {
+ // illegal percent escape: match the undecoded path rather than
fail
+ UrlPathHelper rawPathHelper = new UrlPathHelper()
+ rawPathHelper.urlDecode = false
+ uri = rawPathHelper.getPathWithinApplication(request)
+ }
Review Comment:
The exception path allocates and configures a new `UrlPathHelper` each time
an illegal percent escape is encountered. Since this is attacker-controlled
input, repeated requests could create avoidable allocation pressure. Consider
reusing a static/shared `UrlPathHelper` configured with `urlDecode = false`
(similar to the approach used in `AntPathRequestMatcher`) to keep behavior
consistent and avoid per-request object creation.
##########
grails-spring-security/plugin/src/test/groovy/grails/plugin/springsecurity/web/filter/IpAddressFilterSpec.groovy:
##########
@@ -21,6 +21,8 @@ package grails.plugin.springsecurity.web.filter
import jakarta.servlet.FilterChain
import grails.plugin.springsecurity.AbstractUnitSpec
+import org.grails.web.util.WebUtils
+import spock.lang.Unroll
Review Comment:
This spec uses `org.grails.web.util.WebUtils` constants for forward/include
attribute keys. To avoid accidental mismatch with the production filter’s
attribute keys (which are often sourced from Spring’s
`org.springframework.web.util.WebUtils` or `jakarta.servlet.RequestDispatcher`
constants), consider importing and using the same constants source as the
production code (or `RequestDispatcher`), so the test is guaranteed to set the
exact attributes the filter reads.
--
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]
