DefaultSavedRequest.doesRequestMatch does not work, when matchingRequestParameterName is set

[furti]

Describe the bug
Some time ago, the following commit 28c0d14 introduced the matchingRequestParameterName parameter for the HttpSessionRequestCache.

After upgrading my application to Spring Boot 3 and Spring Security 6, the saved request is not used anymore.

The problem is, that when the Parameter is set:

1. The Request https://myapp.com?originalParam=value is executed
2. The Request is saved in the Session. Note that queryString is originalParam=value
3. Authentication happens
4. A redirect to the saved request is performed. org.springframework.security.web.savedrequest.DefaultSavedRequest.getRedirectUrl() will append the matchingRequestParameterName to the query string. This is expected, as it is used to check if we need to access the session or not. This means the Redirect to the Browser looks something like this: https://myapp.com?originalParam=value&continue
5. The browser executes the Request https://myapp.com?originalParam=value&continue
6. In org.springframework.security.web.savedrequest.HttpSessionRequestCache.getMatchingRequest() the SavedRequest is compared to the Request from the Browser. This check involves comparing the queryString of the SavedRequest and the HttpServletRequest.

The SavedRequest has the queryString originalParam=value and HttpServletRequest.getQueryString() will return originalParam=value&continue

So the comparison fails, and the SavedRequest is not used.

To Reproduce

1. Perform a request to the application that has a query string
2. Authenticate
3. Now the SavedRequest should match, but it does not.

Expected behavior
The request should match, and the saved request should be picked up by the application.

Sample

I have no sample yet. But maybe you can confirm the bug without a sample. What I found is, that the Unit Test introduced with the above mentioned commit might be a bit misleading. https://github.com/spring-projects/spring-security/blob/main/web/src/test/java/org/springframework/security/web/savedrequest/HttpSessionRequestCacheTests.java#L111

It assumes, that the queryString of both requests is null. But in fact, when the success Parameter is available, also the queryString will be populated by the Servlet Engine.

        @Test
	public void getMatchingRequestWhenMatchingRequestParameterNameSetAndParameterExistThenLookedUp() {
		MockHttpServletRequest request = new MockHttpServletRequest();
		HttpSessionRequestCache cache = new HttpSessionRequestCache();
		cache.setMatchingRequestParameterName("success");
		cache.saveRequest(request, new MockHttpServletResponse());
		MockHttpServletRequest requestToMatch = new MockHttpServletRequest();
		requestToMatch.setParameter("success", "");
		requestToMatch.setSession(request.getSession());
		HttpServletRequest matchingRequest = cache.getMatchingRequest(requestToMatch, new MockHttpServletResponse());
		assertThat(matchingRequest).isNotNull();
	}

If I modify the test, and ensure that both Requests have a query string, the test fails.

    @Test
    public void getMatchingRequestWhenMatchingRequestParameterNameSetAndParameterExistThenLookedUp()
    {
        MockHttpServletRequest request = new MockHttpServletRequest();
        request.setQueryString("param=true");
        HttpSessionRequestCache cache = new HttpSessionRequestCache();
        cache.setMatchingRequestParameterName("success");
        cache.saveRequest(request, new MockHttpServletResponse());
        MockHttpServletRequest requestToMatch = new MockHttpServletRequest();
        requestToMatch.setQueryString("param=true&success");
        requestToMatch.setParameter("success", "");
        requestToMatch.setSession(request.getSession());
        HttpServletRequest matchingRequest = cache.getMatchingRequest(requestToMatch, new MockHttpServletResponse());

        assertThat(matchingRequest).isNotNull();
    }

Edit: Fixed a typo in the last code example. request.setQueryString("param=true&success") should have been requestToMatch.setQueryString("param=true&success");

[jzheaux]

@furti, thanks for the detailed report. I've assigned it to our next maintenance release.

[furti]

@jzheaux thanks for the confirmation. One last question. You assigned the ticket to the 5.8.2 milestone. Does this mean it will be also ported to the 6.0.2 Releae? Because this bug also affects the 6.0.1 Version.

[jzheaux]

It will go out to all affected releases, @furti. Thanks for checking.

In this case, that will be 5.8.x, 6.0.x, and main.