jdaugherty commented on code in PR #16311:
URL: https://github.com/apache/grails-core/pull/16311#discussion_r3931061288
##########
grails-interceptors/src/main/groovy/grails/artefact/Interceptor.groovy:
##########
@@ -89,25 +90,17 @@ 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)
+ String uri =
UrlPathHelper.defaultInstance.getPathWithinApplication(request)
def matchedInfo =
request.getAttribute(UrlMappingsHandlerMapping.MATCHED_REQUEST)
UrlMappingInfo grailsMappingInfo = (UrlMappingInfo) matchedInfo
for (Matcher matcher in allMatchers) {
- boolean matchUri = matcher.doesMatch(uri, grailsMappingInfo,
req.method)
- boolean matchNoCtxUri = matcher.doesMatch(noCtxUri,
grailsMappingInfo, req.method)
-
- if (matcher.isExclude() && matchUri && matchNoCtxUri) {
- // Exclude interceptors are special because with only one of
the conditions being false the interceptor
- // won't be applied to the request
- return true
- } else if (!matcher.isExclude() && (matchUri || (checkNoCtxUri &&
matchNoCtxUri))) {
+ boolean matches = matcher instanceof UrlMappingMatcher
+ ? ((UrlMappingMatcher) matcher).doesMatch(uri,
grailsMappingInfo, request.method, request.contextPath)
+ : matcher.doesMatch(uri, grailsMappingInfo, request.method)
+ if (matches) {
Review Comment:
Two things about this dispatch.
`grails.interceptors.Matcher` is public API, but the context-path-aware
overload only exists on the internal `UrlMappingMatcher`, so a third-party
`Matcher` gets no context path — and, less visibly, a different `uri` argument
than before (decoded and application-relative, where it used to be the raw
context-prefixed request URI). A default method on `Matcher` would keep that
contract explicit instead of leaving it to an `instanceof`.
Second, this is a per-request hot path:
`GrailsInterceptorHandlerInterceptorAdapter.preHandle` calls
`doesMatch(request)` once per interceptor, each call does a full
`getPathWithinApplication` (semicolon scan, decode, sanitize), and then every
matcher does another `canonicalizePath` on top. That is N + N*M decode passes
per request where there were previously none. Canonicalizing once here and
passing the result down would fix both that and the double decode in
`UrlMappingMatcher`.
`grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/interceptors/UrlMappingMatcherBenchmark.java`
is worth a run either way.
##########
grails-interceptors/src/main/groovy/grails/artefact/Interceptor.groovy:
##########
@@ -89,25 +90,17 @@ 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)
+ String uri =
UrlPathHelper.defaultInstance.getPathWithinApplication(request)
Review Comment:
`UrlPathHelper.getRequestUri()` consults
`jakarta.servlet.include.request_uri` before it falls back to
`getRequestURI()`, so this changes which URI is matched during an include
sub-dispatch. Grails re-runs the interceptor chain on those (that is why
`UrlMappingUtils.includeForUrl` clears `MATCHED_REQUEST` and the
once-per-request filter marker before `dispatcher.include`), so an interceptor
that used to see the outer page URI now sees the included one.
Measured with `match(uri: '/admin/**')` on a request for `/admin/dashboard`:
| | 8.0.x | this branch |
| --- | --- | --- |
| normal request | true | true |
| during `<g:include>` of `/stats/index` | true | **false** |
The effect is that interceptors decorating a page stop running for anything
pulled in through `<g:include>`. If matching the included URI is the intent it
should be deliberate and pinned by a test; otherwise this could fall back to
`request.requestURI` when `WebUtils.isInclude(request)`.
##########
grails-interceptors/src/main/groovy/grails/artefact/Interceptor.groovy:
##########
@@ -89,25 +90,17 @@ 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)
+ String uri =
UrlPathHelper.defaultInstance.getPathWithinApplication(request)
Review Comment:
Separate point on the same line: `UriUtils.decode` throws
`IllegalArgumentException` on a malformed escape, and
`UrlPathHelper.decodeInternal` only catches `UnsupportedCharsetException`, so
that exception escapes here. `canonicalizePath` in `UrlMappingMatcher` guards
it, but none of the three new `getPathWithinApplication(request)` call sites do.
Request URI `/foo%`:
| | 8.0.x | this branch |
| --- | --- | --- |
| `Interceptor.doesMatch()` | false | throws `IllegalArgumentException` |
| `IpAddressFilter.doFilter` | request allowed | throws |
| compat `AntPathRequestMatcher.matches` | false | throws |
Most containers reject these before dispatch, but
`doesMatch(HttpServletRequest)` is public API and is driven directly from
application interceptor unit tests via `withRequest(uri: ...)`, and a security
filter throwing mid-chain is a worse failure mode than not matching. The
try/catch already used in `canonicalizePath` would cover all three.
##########
grails-spring-security/compat/build.gradle:
##########
@@ -52,8 +52,15 @@ dependencies {
compileOnly 'jakarta.servlet:jakarta.servlet-api'
compileOnly 'org.apache.groovy:groovy'
compileOnly 'org.slf4j:slf4j-api'
+
+ testImplementation 'jakarta.servlet:jakarta.servlet-api'
+ testImplementation 'org.springframework:spring-test'
+ testImplementation 'org.spockframework:spock-core'
+
+ testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine'
}
apply {
from rootProject.layout.projectDirectory.file('gradle/docs-config.gradle')
+ from rootProject.layout.projectDirectory.file('gradle/test-config.gradle')
Review Comment:
`gradle/test-config.gradle` disables `Test` tasks when
`onlySpringSecurityTests` is set, and that is the flag the "Spring Security
Tests" workflow job runs with, so this module's new spec is skipped in that job:
```
./gradlew :grails-spring-security-compat:test -PonlySpringSecurityTests
> Task :grails-spring-security-compat:test SKIPPED
```
It still runs in the core job so the coverage is not lost, but the sibling
`grails-spring-security/plugin` applies
`gradle/spring-security-test-config.gradle`, which gates on
`skipSpringSecurityTests` / `onlyCoreTests` instead. Using that file here would
put the spec in the job that owns this code.
##########
grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/web/filter/IpAddressFilter.groovy:
##########
@@ -138,6 +134,19 @@ class IpAddressFilter extends GenericFilterBean {
false
}
+ protected String getPathWithinApplication(HttpServletRequest request) {
+ String forwardUri =
request.getAttribute(WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE) as String
+ if (forwardUri) {
+ return
urlPathHelper.removeSemicolonContent(urlPathHelper.getPathWithinApplication(new
HttpServletRequestWrapper(request) {
+ @Override
+ String getRequestURI() {
+ forwardUri
+ }
+ }))
+ }
+
urlPathHelper.removeSemicolonContent(urlPathHelper.getPathWithinApplication(request))
+ }
+
Review Comment:
The wrapper does not take effect when an include attribute is present:
`UrlPathHelper.getRequestUri()` reads `jakarta.servlet.include.request_uri`
before it calls `getRequestURI()`, so the override is never consulted and the
restricted forward URI is skipped entirely.
With restriction `/admin/**`, `forward.request_uri = /admin/x`,
`include.request_uri = /public` and request URI `/public`:
| | 8.0.x | this branch |
| --- | --- | --- |
| result | 404, chain not invoked | **200, chain invoked** |
Stripping the context path off `forwardUri` and running
`removeSemicolonContent` plus the decode on the string directly would avoid
both this and the per-request wrapper allocation. The decode needs the same
`IllegalArgumentException` guard as `canonicalizePath` too: `/foo%` currently
throws out of `doFilter`, where before it simply did not match.
##########
grails-spring-security/plugin/src/test/groovy/grails/plugin/springsecurity/web/filter/IpAddressFilterSpec.groovy:
##########
@@ -189,4 +190,42 @@ class IpAddressFilterSpec extends AbstractUnitSpec {
0 == chainCount
}
+
+ void 'doFilter canonicalizes restricted paths'() {
+ given:
+ filter.allowLocalhost = false
+ filter.ipRestrictions = [[pattern: '/admin/**', access: '10.0.0.0/8']]
+ int chainCount = 0
+ def chain = [doFilter: { req, res -> chainCount++ }] as FilterChain
+ request.remoteAddr = '192.168.1.123'
+
+ when:
+ request.requestURI = requestUri
+ response.reset()
+ filter.doFilter(request, response, chain)
+
+ then:
+ response.status == 404
+ chainCount == 0
+
+ where:
+ requestUri << ['/admin;x=1/deleteUser', '/admin%3Bx=1/deleteUser',
'/%61dmin/deleteUser']
+ }
+
+ void 'doFilter canonicalizes forwarded restricted paths'() {
+ given:
+ filter.allowLocalhost = false
+ filter.ipRestrictions = [[pattern: '/admin/**', access: '10.0.0.0/8']]
+ def chain = Mock(FilterChain)
+ request.remoteAddr = '192.168.1.123'
+ request.requestURI = '/public'
+ request.setAttribute(WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE,
'/admin;x=1/deleteUser')
+
+ when:
+ filter.doFilter(request, response, chain)
+
+ then:
+ response.status == 404
+ 0 * chain.doFilter(_, _)
+ }
Review Comment:
Could these grow two more cases? Both regress as things stand:
- `forward.request_uri` set together with `include.request_uri` — the
forward URI is not evaluated at all, so a restricted path is allowed through.
- a request URI with a malformed escape such as `/foo%` — `doFilter` now
throws instead of falling through.
##########
grails-spring-security/compat/src/main/groovy/org/springframework/security/web/util/matcher/AntPathRequestMatcher.groovy:
##########
@@ -47,11 +48,7 @@ class AntPathRequestMatcher implements RequestMatcher {
if (httpMethod && !httpMethod.equalsIgnoreCase(request.method)) {
return false
}
- def path = request.requestURI ?: '/'
- def contextPath = request.contextPath
- if (contextPath && path.startsWith(contextPath)) {
- path = path.substring(contextPath.length())
- }
+ def path =
UrlPathHelper.defaultInstance.removeSemicolonContent(UrlPathHelper.defaultInstance.getPathWithinApplication(request))
?: '/'
Review Comment:
Worth noting this class is on the core security path rather than only a shim
for application code: `GrailsSecurityFilterChain` and
`ChannelFilterInvocationSecurityMetadataSourceFactoryBean` both resolve to it,
so this changes which filter chain a request selects.
Three deltas beyond the intended fix, measured against 8.0.x with
`contextPath = '/app'` and pattern `/admin/**`:
| request URI | 8.0.x | this branch |
| --- | --- | --- |
| `/app/foo%` | false | throws `IllegalArgumentException` |
| `/APP/admin/x` | false | true — `getRemainingPath` compares the context
path with `ignoreCase = true` |
| `/app/public` with `include.request_uri = /app/admin/x` | false | true |
And the mirror of the last row: pattern `/public/**` against that same
request goes true to false. The throw is the one I would guard first here,
since it aborts chain selection rather than declining to match.
##########
grails-interceptors/src/main/groovy/org/grails/plugins/web/interceptors/UrlMappingMatcher.groovy:
##########
@@ -99,6 +110,30 @@ class UrlMappingMatcher implements Matcher {
return false
}
+ private String canonicalizePath(String uri) {
+ if (uri == null || uri.isEmpty()) {
+ return '/'
+ }
+ String path = UrlPathHelper.defaultInstance.removeSemicolonContent(uri)
+ try {
+ path = UriUtils.decode(path, StandardCharsets.UTF_8)
+ } catch (IllegalArgumentException ignored) {
+ // keep the semicolon-stripped form when the URI is malformed
+ }
+ path = UrlPathHelper.defaultInstance.removeSemicolonContent(path)
+ path ?: '/'
Review Comment:
This decodes and strips semicolons a second time. `Interceptor.doesMatch`
already hands in the output of `UrlPathHelper.getPathWithinApplication`, which
has done both. So the matcher's path is not the path dispatch routes on, which
is the desync this PR is closing, just moved one step along:
| request | path `UrlMappingsHandlerMapping` routes on | path the matcher
sees | 8.0.x | this branch |
| --- | --- | --- | --- | --- |
| `/%2561dmin/x` vs `match(uri: '/admin/**')` | `/%61dmin/x` | `/admin/x` |
false | **true** |
| `/health%3Bx` vs `matchAll().excludes(uri: '/health')` | `/health;x` |
`/health` | interceptor runs | **interceptor skipped** |
The second row is the one that matters: over-normalizing on the exclude side
is fail-open, and `%3B` is a literal semicolon inside a path segment rather
than a matrix parameter, so stripping it after decoding is wrong independently
of the double decode.
The decode here is also hardcoded to UTF-8 while `UrlPathHelper` uses
`request.getCharacterEncoding()` with an ISO-8859-1 fallback, so the two passes
can disagree on charset as well.
Suggest canonicalizing exactly once: either skip this for the 4-arg
overload, or have `Interceptor` pass `request.requestURI` and let this method
own the single decode.
##########
grails-interceptors/src/main/groovy/org/grails/plugins/web/interceptors/UrlMappingMatcher.groovy:
##########
@@ -99,6 +110,30 @@ class UrlMappingMatcher implements Matcher {
return false
}
+ private String canonicalizePath(String uri) {
+ if (uri == null || uri.isEmpty()) {
+ return '/'
+ }
+ String path = UrlPathHelper.defaultInstance.removeSemicolonContent(uri)
+ try {
+ path = UriUtils.decode(path, StandardCharsets.UTF_8)
+ } catch (IllegalArgumentException ignored) {
+ // keep the semicolon-stripped form when the URI is malformed
+ }
+ path = UrlPathHelper.defaultInstance.removeSemicolonContent(path)
+ path ?: '/'
+ }
+
+ private boolean matchesPattern(String pattern, String path, String
contextPath) {
+ if (pathMatcher.match(pattern, path)) {
+ return true
+ }
+ if (contextPath && contextPath != '/' && (pattern == contextPath ||
pattern.startsWith(contextPath + '/'))) {
+ return pathMatcher.match(pattern.substring(contextPath.length())
?: '/', path)
+ }
+ false
+ }
Review Comment:
Stripping the context path off the *pattern* covers the 10857 cases, but it
is not equivalent to the old "match against contextPath + path" rule for
patterns that do not literally start with the context path. With
`server.servlet.context-path=/app`, `match(uri: '/*/*')` matched `/app/save`
before and no longer does.
That old match was accidental so I am not arguing to keep it, but it is a
silent semantic change and nothing in the suite pins it either way.
##########
grails-interceptors/src/test/groovy/grails/artefact/InterceptorSpec.groovy:
##########
@@ -267,6 +267,19 @@ class InterceptorSpec extends Specification {
'/foo/bar' | true
}
+ void "Test URI matching uses the canonical request path"() {
+ given:
+ def interceptor = new TestAdminUriInterceptor()
+ def webRequest = GrailsWebMockUtil.bindMockWebRequest(new
MockServletContext(), new MockHttpServletRequest('', requestUri), new
MockHttpServletResponse())
+
webRequest.request.setAttribute(UrlMappingsHandlerMapping.MATCHED_REQUEST, new
ForwardUrlMappingInfo(controllerName: 'admin', action: 'deleteUser'))
+
+ expect:
+ interceptor.doesMatch()
+
+ where:
+ requestUri << ['/admin;x=1/deleteUser', '/%61dmin/deleteUser']
+ }
+
Review Comment:
The case this changes most for existing applications is not covered here: an
interceptor combining `match(uri:)` with `excludes(uri:)` could never fire
under a non-root context path before, because the old `isExclude() && matchUri
&& matchNoCtxUri` gate also required the pattern to match the context-prefixed
URI.
```groovy
match(uri: '/api/**').excludes(uri: '/api/health') // contextPath '/app',
request '/app/api/orders'
// 8.0.x: false, this branch: true
```
That is the correct behaviour, but it means applications deployed at a
context path start executing interceptors that have silently never run, so it
deserves a case here and a release note.
Also uncovered, and both currently changing behaviour: matching during an
include sub-dispatch, and a request URI with a malformed percent escape.
--
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]
