jamesfredley commented on code in PR #16332:
URL: https://github.com/apache/grails-core/pull/16332#discussion_r3970757404
##########
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:
Thanks. This allocates one helper only on the illegal-percent-escape path
that results in a 400-class URI. That is not a practical denial-of-service
issue, and sharing the helper would be a style optimization rather than a
correctness requirement for this PR.
##########
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:
Thanks. The forwarded branch removes semicolon content and decodes but does
not apply duplicate-slash collapse. That is a small difference for uncommon
forwarded URIs, not the decode-once and matrix-parameter bug addressed here. We
can consider it as an optional follow-up, but it is not a blocker for this PR.
##########
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:
Thanks. Production and the spec both use org.grails.web.util.WebUtils.
Grails WebUtils extends Spring WebUtils, so these are the same attribute keys
and the test is exercising the production constants correctly.
--
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]
