HttpSessionRequestCache#getMatchingRequest passes decoded Request URL to UriComponentsBuilder

[ClausMie]

Hi there!
Thanks for taking a look at this issue. Please let me know if you require more information.
Let's discuss!

Describe the bug

HttpSessionRequestCache#getMatchingRequest relies on org.springframework.web.util.UriComponentsBuilder#fromUriString(String) to be able to handle decoded % characters.

To Reproduce

The org.springframework.web.util.UriComponentsBuilder#fromUriString(String) cannot handle a % followed by characters that result in false %-encoding.

@Test
void unencodedPercent(){
  assertThatRuntimeException().isThrownBy(() -> UriComponentsBuilder.fromUriString("/30 % off"));
}

This leads to a org.springframework.web.util.InvalidUrlException: Bad path when such a request gets to

spring-security/web/src/main/java/org/springframework/security/web/savedrequest/HttpSessionRequestCache.java

Line 107 in ec3cc66

|| !UriComponentsBuilder.fromUriString(UrlUtils.buildRequestUrl(request))

An encoded request https://example.com/myapp/discounts/30%20%25%20off is returned decoded by

spring-security/web/src/main/java/org/springframework/security/web/util/UrlUtils.java

Lines 91 to 94 in ec3cc66

	* @return the decoded URL, excluding any server name, context path or servlet path
	*
	*/
	public static String buildRequestUrl(HttpServletRequest r) {

The UriComponentsBuilder cannot handle this decoded %

Expected behavior
My request does not result in an InvalidUrlException.

[mkleine]

In fact every request containing a properly encoded % sign followed by a non-hex character will cause an exception in line

spring-security/web/src/main/java/org/springframework/security/web/savedrequest/HttpSessionRequestCache.java

Line 107 in ec3cc66

|| !UriComponentsBuilder.fromUriString(UrlUtils.buildRequestUrl(request))

as the path of the URI to be checked is decoded as documented

spring-security/web/src/main/java/org/springframework/security/web/util/UrlUtils.java

Line 85 in ec3cc66

* But the requestURI is not decoded, whereas the servletPath and pathInfo are

The decoded URI could be parsed using WHAT_WG parser type but that's not configurable at that line. @marcusdacoregio can you confirm this analysis? You introduced this line in 18e8836

[mkleine]

Dear Sprint Security Team, could someone please take a look at this problem? This issue is affecting us in production and we're unsure how to get around it.

[ClausMie]

Hi @Kehrlann, do you maybe have an idea about how to solve this issue?

[Kehrlann]

Hey @ClausMie @mkleine - thanks for reaching out.

I'll look into it further, but here are some obvious preliminary thoughts. You may have considered them already, but worth asking.

1. Consider removing the request cache

Are your users redirected to a default page when they log in? In case you don't need the redirect post-authentication feature, turn it off with:

http
        // ...
        .requestCache(RequestCacheConfigurer::disable)

Or use the NullRequestCache

2. Consider setting matchingRequestParameterName to null

If you set matchingRequestParameterName to null, then the request cache will be looked up regardless of the request (i.e. when there is no ?continue param).

http
        // ...
        .requestCache(rc -> {
            var requestCache = new HttpSessionRequestCache();
            requestCache.setMatchingRequestParameterName(null);
            rc.requestCache(requestCache);
        })

However, this will look up the request cache on every request, including public resources (e.g. favicon). This means reading the session, which may be expensive depending on your setup. See gh-6125 .

3. Extend HttpSessionRequestCache

If the above does not work, you could extend HttpSessionRequestCache, and re-implement getMatchingRequest with a better matching alg.
You will also need to copy the code from SavedRequestAwareWrapper if you need the full features of replaying the saved request (e.g. if you want to replay POST requests).
If you just need the redirection, returning null is fine:

http
        // ...
        .requestCache(rc -> {
            var requestCache = new HttpSessionRequestCache() {
                @Override
                public HttpServletRequest getMatchingRequest(HttpServletRequest request, HttpServletResponse response) {
                    return null;
                }
            };
            rc.requestCache(requestCache);
        })

[ClausMie]

Hi @Kehrlann
Thanks for getting back to us so quickly!
Disabling the request cache works for us for now. Thank you for the code pointer.

[Kehrlann]

From further investigation: this appeared in Security 6.4 following an upgrade to Framework 6.2 which changed their URI parsing logic.