Copilot commented on code in PR #16330:
URL: https://github.com/apache/grails-core/pull/16330#discussion_r3969386750
##########
grails-spring-security/compat/src/main/groovy/org/springframework/security/web/util/matcher/AntPathRequestMatcher.groovy:
##########
@@ -48,12 +51,36 @@ class AntPathRequestMatcher implements RequestMatcher {
if (httpMethod && !httpMethod.equalsIgnoreCase(request.method)) {
return false
}
- def path =
UrlPathHelper.defaultInstance.removeSemicolonContent(UrlPathHelper.defaultInstance.getPathWithinApplication(request))
?: '/'
- def candidate = caseSensitive ? path : path.toLowerCase(Locale.ENGLISH)
- def matcherPattern = caseSensitive ? pattern :
pattern.toLowerCase(Locale.ENGLISH)
+ String path = getPathWithinApplication(request)
+ String candidate = caseSensitive ? path :
path.toLowerCase(Locale.ENGLISH)
+ String matcherPattern = caseSensitive ? pattern :
pattern.toLowerCase(Locale.ENGLISH)
Review Comment:
`path` is assumed non-null, but if
`UrlPathHelper.getPathWithinApplication(request)` ever returns `null` (or an
empty value in some servlet/request edge cases), this will throw (e.g.,
`toLowerCase`). Previously this code explicitly defaulted to `'/'`. To preserve
prior behavior and avoid NPEs, add a root-path fallback (e.g., normalize `path`
to `'/'` when blank/null) either in `getPathWithinApplication` or immediately
after the call.
##########
grails-doc/src/en/guide/theWebLayer/interceptors/interceptorMatching.adoc:
##########
@@ -64,4 +64,24 @@ All named arguments except for `uri` accept either a String
or a Regex expressio
* `controller` - The name of the controller
* `action` - The name of the action
* `method` - The HTTP method
-* `uri` - The URI of the request. If this argument is used then all other
arguments will be ignored and only this will be used.
+* `uri` - The path of the request within the application. If this argument is
used then all other arguments will be ignored and only this will be used.
+
+The `uri` argument is matched against the same path that URL mappings use to
select the controller: percent-encoded characters are decoded, matrix
parameters (`;name=value`) are removed, and the context path is not included.
When an application is deployed under the context path `/app`, a request for
`/app/%61dmin;x=1/users` is matched as `/admin/users`, so the following
interceptor applies to it:
Review Comment:
The docs describe the canonical form, and they mention include-path
matching, but they don’t mention the other deliberate deviation documented
elsewhere in this PR: the context path being compared case-insensitively
(inherited from Spring/dispatch). To avoid user confusion (especially for
`server.servlet.context-path` setups), consider adding a brief note here
consistent with the `Interceptor` Javadoc and upgrade guide.
##########
grails-doc/src/en/guide/theWebLayer/interceptors/interceptorMatching.adoc:
##########
@@ -64,4 +64,24 @@ All named arguments except for `uri` accept either a String
or a Regex expressio
* `controller` - The name of the controller
* `action` - The name of the action
* `method` - The HTTP method
-* `uri` - The URI of the request. If this argument is used then all other
arguments will be ignored and only this will be used.
+* `uri` - The path of the request within the application. If this argument is
used then all other arguments will be ignored and only this will be used.
+
+The `uri` argument is matched against the same path that URL mappings use to
select the controller: percent-encoded characters are decoded, matrix
parameters (`;name=value`) are removed, and the context path is not included.
When an application is deployed under the context path `/app`, a request for
`/app/%61dmin;x=1/users` is matched as `/admin/users`, so the following
interceptor applies to it:
+
+[source,groovy]
+----
+class AdminInterceptor {
+ AdminInterceptor() {
+ match(uri: '/admin/**')
+ .excludes(uri: '/admin/health')
+ }
+
+ boolean before() {
+ ...
+ }
+}
+----
+
+This is the canonical form defined by RFC 3986 and the Jakarta Servlet
specification: path parameters are removed from each segment before
percent-decoding, so an encoded semicolon (`%3B`) is a literal character rather
than a parameter delimiter, and the path is decoded exactly once.
+
+A pattern that starts with the context path, such as `match(uri:
'/app/admin/**')`, is also accepted for backwards compatibility. Requests
dispatched through `<g:include>` are matched against the included path, which
is the path the include is dispatched to.
Review Comment:
The docs describe the canonical form, and they mention include-path
matching, but they don’t mention the other deliberate deviation documented
elsewhere in this PR: the context path being compared case-insensitively
(inherited from Spring/dispatch). To avoid user confusion (especially for
`server.servlet.context-path` setups), consider adding a brief note here
consistent with the `Interceptor` Javadoc and upgrade guide.
##########
grails-interceptors/src/main/groovy/grails/artefact/Interceptor.groovy:
##########
@@ -90,17 +107,21 @@ trait Interceptor implements ResponseRenderer,
ResponseRedirector, RequestForwar
allMatchers << matcher
}
- String uri =
UrlPathHelper.defaultInstance.getPathWithinApplication(request)
-
- 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:
In the exception path, a new `UrlPathHelper` is allocated per request. Since
this code can run in a hot path under repeated malformed inputs, consider
reusing a cached/static `UrlPathHelper` instance configured with
`urlDecode=false` (similar to the approach in `AntPathRequestMatcher` and
`IpAddressFilter`) to avoid repeated allocations and reduce GC pressure.
##########
grails-interceptors/src/main/groovy/org/grails/plugins/web/interceptors/UrlMappingMatcher.groovy:
##########
@@ -67,17 +70,17 @@ class UrlMappingMatcher implements Matcher {
doesMatch(uri, info, method, null)
}
+ @Override
boolean doesMatch(String uri, UrlMappingInfo info, String method, String
contextPath) {
boolean hasUriPatterns = !uriPatterns.isEmpty()
- String path = canonicalizePath(uri)
- boolean isExcluded = this.isExcluded(path, info, contextPath)
+ boolean isExcluded = this.isExcluded(uri, info, contextPath)
if (matchAll && !isExcluded) return true
if (!isExcluded) {
if (hasUriPatterns) {
for (pattern in uriPatterns) {
- if (matchesPattern(pattern, path, contextPath)) {
+ if (matchesPattern(pattern, uri, contextPath)) {
return true
}
Review Comment:
This method now matches against the passed-in `uri` without the previous
`canonicalizePath` normalization (which converted `null`/empty to `'/'`). If
any call site supplies `null` or `''` (e.g., root requests or atypical servlet
containers/mocks), `isExcluded`/pattern matching may misbehave or throw.
Consider restoring the minimal normalization at the start of this method (treat
`null`/empty as `'/'`) while still avoiding any decoding/semicolon stripping.
##########
grails-interceptors/src/main/groovy/org/grails/plugins/web/interceptors/UrlMappingMatcher.groovy:
##########
@@ -67,17 +70,17 @@ class UrlMappingMatcher implements Matcher {
doesMatch(uri, info, method, null)
}
+ @Override
boolean doesMatch(String uri, UrlMappingInfo info, String method, String
contextPath) {
boolean hasUriPatterns = !uriPatterns.isEmpty()
- String path = canonicalizePath(uri)
- boolean isExcluded = this.isExcluded(path, info, contextPath)
+ boolean isExcluded = this.isExcluded(uri, info, contextPath)
if (matchAll && !isExcluded) return true
Review Comment:
This method now matches against the passed-in `uri` without the previous
`canonicalizePath` normalization (which converted `null`/empty to `'/'`). If
any call site supplies `null` or `''` (e.g., root requests or atypical servlet
containers/mocks), `isExcluded`/pattern matching may misbehave or throw.
Consider restoring the minimal normalization at the start of this method (treat
`null`/empty as `'/'`) while still avoiding any decoding/semicolon stripping.
--
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]
