jamesfredley opened a new pull request, #16330: URL: https://github.com/apache/grails-core/pull/16330
> Relocated from [jamesfredley/grails-core#6](https://github.com/jamesfredley/grails-core/pull/6). That follow-up was opened on an archive fork by mistake; Grails PRs belong on `apache/grails-core`. Parent: apache/grails-core#16311. Follow-up to apache/grails-core#16311 addressing the review there. Three commits on top of `f0108c28ed`: the first addresses the inline review, the second aligns the two Spring Security matchers with dispatch and documents the specification basis, the third narrows the upgrade note on which interceptor shape was inert under a context path. ## Why The PR's premise is right: URI matchers must see the path that `UrlMappingsHandlerMapping` routes on, otherwise a request can reach a controller without its interceptors or restrictions. Dispatch has resolved that path with Spring's `UrlPathHelper.getPathWithinApplication` since Grails 3 (2014). The PR head approximated that path instead of reusing it, and the approximation reopened the same class of inconsistency in three ways: 1. **The path was canonicalized twice.** `Interceptor` decoded once via `UrlPathHelper`, then `UrlMappingMatcher.canonicalizePath` decoded again and stripped semicolons again. RFC 3986 section 2.4 is explicit: "Implementations must not percent-encode or decode the same string more than once." The effect was fail-open on the exclude side: `/health%3Bx` is dispatched as `/health;x`, but `matchAll().excludes(uri: '/health')` saw `/health` and skipped the interceptor. 2. **An encoded semicolon was treated as a matrix-parameter delimiter.** Jakarta Servlet 6.0 section 3.5.2 removes path parameters per segment *before* percent-decoding, and RFC 3986 section 2.2 says a reserved character and its percent-encoded form "are not equivalent". So `/admin%3Bx=1/deleteUser` is dispatched as `/admin;x=1/deleteUser` (controller `admin;x=1`, which does not exist), not as `/admin/deleteUser`. The PR head's post-decode strip matched it anyway. Two test expectations encoding that over-match are flipped here. 3. **The two security matchers used their own path resolution**, so they still diverged from dispatch at the edges: a malformed escape threw out of `doFilter` and `matches` where 8.0.x declined to match, and the `IpAddressFilter` wrapper that supplied the forward URI was bypassed whenever an include attribute was present, letting a restricted forwarded path through. The specifications settle every case, and the fix is to reuse dispatch's resolution exactly rather than re-implement it: - Servlet 6.0 section 3.5.2: path parameters (`;name=value`) are removed per segment, then each segment is percent-decoded once; illegal `%` sequences, encoded `/`, encoded dot segments and control characters must be rejected with 400. - RFC 3986 section 2.3: unreserved characters and their escapes are equivalent (`/%61dmin` is `/admin`) and "should be decoded ... by URI normalizers". This is the original finding f003. - RFC 3986 section 2.2: reserved characters and their escapes are not equivalent (`%3B` is a literal semicolon). - RFC 3986 section 2.4: decode exactly once; a literal `%` must be `%25`, so `/foo%` is not a valid URI. - RFC 3986 section 6.2.2.1 and RFC 9110 section 4.2.3: paths are case-sensitive. Two behaviours are inherited from Spring and deviate from the last point; both are kept deliberately and documented in the `Interceptor` Javadoc and the upgrade note. `UrlPathHelper` compares the context path case-insensitively, and it matches the included URI during a `RequestDispatcher` include (Spring's own `ServletRequestPathUtils.parse`, which Spring Security 7's `PathPatternRequestMatcher` builds on, does the same). Dispatch does both, so the matchers must too: if a container delivered `/APP/admin/x` under context path `/app`, dispatch would route it to `AdminController`, and a matcher stripping the context path case-sensitively would select the wrong filter chain. Consistency with dispatch is the security property; RFC parity on its own is not. The review also found that the old `isExclude() && matchUri && matchNoCtxUri` gate made one specific shape inert under a non-root context path: a matcher pairing a `uri` pattern with any `excludes(...)` (URI, controller, action or closure), because the gate required the pattern to match both the context-prefixed and the context-relative URI, which `/api/**` cannot do. `matchAll()` with excludes, name-based matchers, `match(uri:)` without excludes, and patterns like `/**` were unaffected and always ran. The PR fixed that shape implicitly; this branch pins it with tests for both a URI and a controller exclude and calls it out in the upgrade note, since an interceptor written that way starts executing under a context path. ## Changes - **Canonicalize once.** `Interceptor.doesMatch(request)` computes the application path with `UrlPathHelper.getPathWithinApplication` once per request and passes it to every matcher through a new default method `Matcher.doesMatch(uri, info, method, contextPath)`. `UrlMappingMatcher` no longer decodes or strips semicolons. The `instanceof UrlMappingMatcher` dispatch is gone and third-party `Matcher` implementations get the documented contract plus the context path. - **All three matchers resolve the path exactly as dispatch does.** `Interceptor`, `IpAddressFilter` and the compat `AntPathRequestMatcher` call `UrlPathHelper.getPathWithinApplication(request)`, with a guarded fallback that matches the undecoded path when an escape is illegal, so no matcher throws mid-chain. - **`IpAddressFilter` forwarded requests** canonicalize the forward URI string the same way (path parameters removed, decoded once, decoded forward context path stripped case-insensitively), with no per-request wrapper, so the include attribute can no longer bypass a forwarded restriction. - **Compat module test wiring** moves to `gradle/spring-security-test-config.gradle`; the script's `integrationTest` block is guarded with `pluginManager.withPlugin('org.apache.grails.gradle.grails-integration-test')`, the plugin that registers the task, so a library module can apply it. The compat spec now runs in the Spring Security CI job. - **Docs.** `interceptorMatching.adoc` documents the `uri` semantics and their specification basis; `upgrading80x.adoc` gains a section covering the context-path `match`+`excludes` change, the `/*/*` change, includes, custom matchers and the two documented deviations; `ip.adoc` notes canonical matching. ## Tests Reproducers for every behaviour called out in review: both fail-open double-decode rows, include sub-dispatch (all three call sites), malformed escapes (all three call sites), `match(uri:)` + `excludes(uri:)` under a context path, the `/*/*` pin, request-charset parity with dispatch, custom `Matcher` receiving the canonical path and context path, forward + include in `IpAddressFilter`, case-insensitive context-path stripping for direct and forwarded paths, and empty-segment collapsing in compat. Two expectations from the original PR are flipped: `/admin%3Bx=1/deleteUser` (compat) and `/admin%3Bx=1/deleteUser` (`IpAddressFilterSpec`) are no longer matched by `/admin/**`, for the reason in point 2 above. Every raw `;x=1` and `%61` expectation is unchanged. ## Verification - Full local run of `./gradlew clean aggregateViolations :grails-test-report:check --continue` on JDK 21 with test caching disabled: 14,793 tests, 0 failures, 0 errors, 339 pre-existing skips; 1,062 test tasks executed, none from cache. Checkstyle, CodeNarc, PMD and SpotBugs aggregate reports all clean. (The Micronaut example island is pruned on JDK 21 by `settings.gradle`, as in the Java 21 CI jobs.) - Downstream `grails-test-examples/app1` `UriMatchingInterceptorSpec` and `BookInterceptorSpec` pass. - Compat spec executes under `-PonlySpringSecurityTests` and is skipped under `-PskipSpringSecurityTests`; plugin modules still receive the `integrationTest` configuration. `UrlMappingMatcherBenchmark` (1 fork, 5 iterations, ns/op, lower is better): | | matchUriPattern | rejectNonMatchingUriPattern | |---|---|---| | PR base (`f0108c28ed^`) | 79.05 ± 5.30 | 18.16 ± 1.85 | | PR head | 82.67 ± 2.69 | 21.82 ± 2.04 | | this branch | 79.09 ± 6.71 | 16.35 ± 0.16 | The matcher is back to base speed; the larger saving (one canonicalization per request instead of N + N·M) is in `Interceptor.doesMatch`, which the benchmark does not exercise. -- 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]
