matrei commented on PR #16311: URL: https://github.com/apache/grails-core/pull/16311#issuecomment-5597492117
## AI Review Findings Reviewed together with the follow-up at https://github.com/jamesfredley/grails-core/pull/6 (three commits on top of `f0108c28ed`). The premise of this PR is right: `UrlMappingsHandlerMapping.getHandlerInternal()` routes on a default `UrlPathHelper.getPathWithinApplication(request)`, so URI matchers must see that same path. The PR head approximates that path instead of reusing it, and the approximation reopens the same class of inconsistency. The follow-up fixes every item below and should be merged into this branch before this PR merges. Verified against the Spring 7.0.9 source: `UrlPathHelper.decodeAndCleanUriString` runs `removeSemicolonContent`, then `decodeRequestString`, then `getSanitizedPath`, and `getRequestUri` reads `jakarta.servlet.include.request_uri` before falling back to `getRequestURI()`. ### High: The path is canonicalized twice, which is fail-open on `excludes(uri:)` References: - `grails-interceptors/src/main/groovy/grails/artefact/Interceptor.groovy:93` - `grails-interceptors/src/main/groovy/org/grails/plugins/web/interceptors/UrlMappingMatcher.groovy:113-124` `Interceptor.doesMatch` passes the output of `getPathWithinApplication` to `UrlMappingMatcher`, and `canonicalizePath` then decodes it again and strips semicolons again. Dispatch removes path parameters before decoding, so `/health%3Bx` is dispatched as `/health;x`, but the matcher sees `/health` and `matchAll().excludes(uri: '/health')` skips the interceptor for a path that is not `/health`. | request | dispatched path | matcher path (PR head) | |---|---|---| | `/health%3Bx` | `/health;x` | `/health` | | `/%2561dmin/x` | `/%61dmin/x` | `/admin/x` | Fixed in the follow-up by canonicalizing once in `Interceptor.doesMatch` and passing the result to every matcher through a new default method `Matcher.doesMatch(uri, info, method, contextPath)`. `UrlMappingMatcher` no longer decodes or strips anything. ### High: An encoded semicolon is treated as a matrix-parameter delimiter References: - `grails-interceptors/src/main/groovy/org/grails/plugins/web/interceptors/UrlMappingMatcher.groovy:121` - `grails-spring-security/compat/src/test/groovy/org/springframework/security/web/util/matcher/AntPathRequestMatcherSpec.groovy:38` - `grails-spring-security/plugin/src/test/groovy/grails/plugin/springsecurity/web/filter/IpAddressFilterSpec.groovy:212` Two test rows pin `/admin%3Bx=1/deleteUser` as matching `/admin/**`. Jakarta Servlet 6.0 §3.5.2 removes path parameters per segment before percent-decoding and RFC 3986 §2.2 says a reserved character and its escape are not equivalent, so that request is dispatched to controller `admin;x=1`, which does not exist. Matching it against `/admin/**` over-matches on `match` and is fail-open on `excludes`. The follow-up flips both expectations. The raw `;x=1` and `%61` rows, which are the finding this PR set out to fix, are unchanged. ### High: A malformed percent escape throws out of all three matchers References: - `grails-interceptors/src/main/groovy/grails/artefact/Interceptor.groovy:93` - `grails-spring-security/compat/src/main/groovy/org/springframework/security/web/util/matcher/AntPathRequestMatcher.groovy:51` - `grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/web/filter/IpAddressFilter.groovy:140-149` `UriUtils.decode` throws `IllegalArgumentException` on `/foo%`, and `UrlPathHelper.decodeInternal` only catches `UnsupportedCharsetException`. None of the three new `getPathWithinApplication` call sites guard it, so `Interceptor.doesMatch`, `IpAddressFilter.doFilter` and `AntPathRequestMatcher.matches` all throw where 8.0.x declined to match. `doesMatch(HttpServletRequest)` is public API driven from application unit tests, and a security filter throwing mid-chain is a worse failure mode than not matching. The follow-up falls back to a `UrlPathHelper` with `urlDecode = false` at all three sites. ### High: `IpAddressFilter` forward wrapper is bypassed by an include attribute References: - `grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/web/filter/IpAddressFilter.groovy:137-149` The wrapper overrides `getRequestURI()`, but `UrlPathHelper.getRequestUri` reads `jakarta.servlet.include.request_uri` first, so when an include attribute is present the forward URI is never consulted and a restricted forwarded path is let through. With restriction `/admin/**`, `forward.request_uri = /admin/x`, `include.request_uri = /public`: 8.0.x returns 404, this branch invokes the chain. The follow-up canonicalizes the forward URI string directly (semicolon content removed, decoded once, forward context path stripped case-insensitively) with no per-request wrapper. ### Medium: Compat spec is skipped in the Spring Security CI job References: - `grails-spring-security/compat/build.gradle:65` `gradle/test-config.gradle` disables `Test` tasks under `-PonlySpringSecurityTests`, which is what the Spring Security workflow job runs with, so the new `AntPathRequestMatcherSpec` only runs in the core job. The follow-up switches the module to `gradle/spring-security-test-config.gradle` and guards that script's `integrationTest` block with `pluginManager.withPlugin(...)` so a library module can apply it. ### Medium: Documentation References: - `grails-doc/src/en/guide/theWebLayer/interceptors/interceptorMatching.adoc:67` - `grails-doc/src/en/guide/upgrading/upgrading80x.adoc` The guide still describes `uri` as "The URI of the request". The semantics are now decoded, matrix-parameter-stripped and application-relative, with context-prefixed patterns accepted for compatibility. Two behaviour changes need an upgrade note: interceptors pairing `match(uri:)` with any `excludes(...)` were inert under a non-root context path and now run, and `match(uri: '/*/*')` no longer matches `/app/save` under context path `/app`. The follow-up adds both, plus `ip.adoc` coverage. ## Follow-up (jamesfredley#6) findings All three module suites pass on the follow-up head: | Module | Tests | Failures | |---|---|---| | grails-interceptors | 89 | 0 | | grails-spring-security-compat | 16 | 0 | | grails-spring-security (plugin) | 215 | 0 | ### Medium: Upgrade note number collides after rebase References: - `grails-doc/src/en/guide/upgrading/upgrading80x.adoc:3101` The PR base is 116 commits behind 8.0.x, which now has sections through 57. A trial merge is conflict-free but leaves two sections numbered 54. Renumber to 58 when rebasing. ### Low: Forwarded `ipRestrictions` under a context path should be in the upgrade note References: - `grails-doc/src/en/guide/security/securityPlugins/springSecurity/core/ip.adoc:51` - `grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/web/filter/IpAddressFilter.groovy:146-166` 8.0.x never stripped the context path from `forward.request_uri`, so a forwarded restricted path under a context path was fail-open. The follow-up enforces it, which is right, but it is a behaviour change on a security filter and neither doc mentions it. ### Low: Nits - `Interceptor.doesMatch` allocates a new `UrlPathHelper` on every malformed-escape fallback; the compat matcher and the IP filter hold a raw helper once. Do the same here. - The forward branch in `IpAddressFilter.getPathWithinApplication` skips `getSanitizedPath`, so `//` is not collapsed for forwarded URIs. Harmless because `AntPathMatcher` ignores empty segments, but it is the one place the forward path deviates from the direct path. - `bindRequest` sits between feature methods in `InterceptorSpec`; move it to the bottom with the other helpers. - The new compat spec pins that `AntPathRequestMatcher` is case-insensitive by default. That is pre-existing, but Spring Security's original defaulted to case-sensitive, and this class selects filter chains. Worth a deliberate decision. ## Questions for maintainers - This PR targets 8.0.x only. Is a backport to the 7.x lines intended? - Includes are now matched on the included path, so `match(uri: '/admin/**')` no longer fires for `<g:include>` sub-dispatches inside an admin page. The upgrade note covers it and it is consistent with `match(controller:)`, but it is the one change that could surprise an application relying on the old double-fire. -- 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]
