jdaugherty commented on PR #16311: URL: https://github.com/apache/grails-core/pull/16311#issuecomment-5588799711
All of my suggested feedback is implemented in a PR for this branch: https://github.com/jamesfredley/grails-core/pull/6 While implementing the feedback I checked what the specifications actually say, and it changed two details: First, Jakarta Servlet 6.0 §3.5.2 removes path parameters per segment before percent-decoding, RFC 3986 §2.2 says a reserved character and its percent-encoded form "are not equivalent", and §2.4 says implementations "must not percent-encode or decode the same string more than once". So %3B is a literal semicolon: /admin%3Bx=1/deleteUser is dispatched as /admin;x=1/deleteUser (controller admin;x=1) and never reaches AdminController. The second semicolon strip after decoding in this PR's canonicalizePath matched it against /admin/** anyway, which over-matches on match and is fail-open on excludes (/health%3Bx skipping matchAll().excludes(uri: '/health')). The follow-up removes that strip and flips the two test expectations that encoded it. The raw ;x=1 and %61 cases are unchanged and still match, which is the finding this PR set out to fix. Second, in my review I listed the case-insensitive context path (/APP/admin/x) and the include attribute as deltas against 8.0.x, and my first cut of the follow-up had the two security matchers doing their own case-sensitive strip on requestURI to stay close to 8.0.x. I reversed that. RFC 3986 §6.2.2.1 does say paths are case-sensitive, but UrlMappingsHandlerMapping has resolved the path with Spring's UrlPathHelper.getPathWithinApplication since Grails 3, and that compares the context path case-insensitively and honors include.request_uri (Spring's ServletRequestPathUtils, which Spring Security 7's PathPatternRequestMatcher builds on, does the same). If a container ever delivers /APP/admin/x under /app, dispatch routes it to AdminController, so a matcher that strips case-sensitively selects the wrong chain, which is exactly the desync this PR is about. Consistency with dispatch is the security property; RFC parity on its own isn't. All three matchers now call getPathWithinApplica tion, and the two deviations from the RFC are documented as deliberate, with the reason, in the Interceptor Javadoc and the upgrade note. One more correction to my own review: the "interceptors that silently never ran under a context path" case is narrower than I described. Only a matcher pairing a uri pattern with an excludes(...) was inert; matchAll() with excludes, name-based matchers and match(uri:) without excludes always ran. The release note says so and both variants are pinned. -- 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]
