HttpSessionSecurityContextRepository fails to create a session because of the deferred security context support

[sbegaudeau]

Describe the bug

Short version

The new support for deferred security context makes DelegatingSecurityContextRepository#loadContext call HttpSessionSecurityContextRepository#loadDeferredContext while HttpSessionSecurityContextRepository#loadContext would have been called before. The response wrapper is thus not configured by HttpSessionSecurityContextRepository#loadContext when the context is loaded but only when the context is to be saved. As a result, when that wrapper is finally created, the response is now committed, it did not have the ability to act as an OnCommittedResponseWrapper and it can't create a session anymore, it's too late.

Long version

I am using the oauth2Login() in my security configuration and there seems to be a change in behavior caused by this commit introducing the support for deferred SecurityContext which makes my application throw an exception with Spring Security 6.0.0 while it worked with Spring Security 5.7.3.

I am using the OAuth2 support to log in with Github and everything works fine with regard to the communication with Github but by migrating to Spring Security 6.0.0, after the authentication, new requests cannot find the security context and return a 401.

I have thus used http.securityContext((securityContext) -> securityContext.requireExplicitSave(false)); to restore the existing behavior of Spring Security 5.7 but now the code is producing the following error:

w.c.HttpSessionSecurityContextRepository : Failed to create a session, as response has been committed. Unable to store SecurityContext.

From what I have understood, in my application with Spring Security 5.7.3, the SecurityContextPersistenceFilter used the HttpSessionSecurityContextRepository and the call to SecurityContext contextBeforeChainExecution = this.repo.loadContext(holder); would thus call HttpSessionSecurityContextRepository#loadContext. In that method, a wrapper was added to both the request and the response:

SaveToSessionResponseWrapper wrappedResponse = new SaveToSessionResponseWrapper(response, request, httpSession != null, context);
wrappedResponse.setSecurityContextHolderStrategy(this.securityContextHolderStrategy);
requestResponseHolder.setResponse(wrappedResponse);
requestResponseHolder.setRequest(new SaveToSessionRequestWrapper(request, wrappedResponse));

As a result, when the success handler of my OAuth2 login was called, it could perform the redirection with:

this.getRedirectStrategy().sendRedirect(request, response, redirectUri);

Now the problem is that, this call to sendRedirect is executed by an OnCommittedResponseWrapper, which is the HeaderWriterResponse (as before) and underneath I don't have the SaveToSessionResponseWrapper anymore. In Spring Security 5.7, this wrapper would save the context by executing the following code before the response was committed:

@Override
protected void onResponseCommitted() {
	saveContext(this.securityContextHolderStrategy.getContext());
	this.contextSaved = true;
}

Now instead, the call to HttpSessionSecurityContextRepository#loadDeferredContext caused by DelegatingSecurityContextRepository#loadContext does not create this wrapper anymore. A wrapper is only created later thanks to the call to:

this.repo.saveContext(contextAfterChainExecution, holder.getRequest(), holder.getResponse());

By the SecurityContextPersistenceFilter after the response has been committed. Now the method DelegatingSecurityContextRepository#saveContext will call HttpSessionSecurityContextRepository#saveContext which will create a new response wrapper but long after the response has been committed:

public void saveContext(SecurityContext context, HttpServletRequest request, HttpServletResponse response) {
	SaveContextOnUpdateOrErrorResponseWrapper responseWrapper = WebUtils.getNativeResponse(response, SaveContextOnUpdateOrErrorResponseWrapper.class);
	if (responseWrapper == null) {
		boolean httpSessionExists = request.getSession(false) != null;
		SecurityContext initialContext = this.securityContextHolderStrategy.createEmptyContext();
		responseWrapper = new SaveToSessionResponseWrapper(response, request, httpSessionExists, initialContext);
	}
	responseWrapper.saveContext(context);
}

When this new instance of the response wrapper will try to do its job, the response would have been committed already and the exception would appear. As a result, the authentication does not work.

Expected behavior
The use of HttpSessionSecurityContextRepository should create the same wrapper as before to save the security context in the session before the response is committed. I don't know if HttpSessionSecurityContextRepository#loadDeferredContext can do it since we don't have access to the response since it is "lost" by the DelegatingSecurityContextRepository in:

@Override
public SecurityContext loadContext(HttpRequestResponseHolder requestResponseHolder) {
	return loadDeferredContext(requestResponseHolder.getRequest()).get();
}

[rwinch]

Hello @sbegaudeau! Thank you for the detailed write up. I have responded below.

I am using the OAuth2 support to log in with Github and everything works fine with regard to the communication with Github but by migrating to Spring Security 6.0.0, after the authentication, new requests cannot find the security context and return a 401.

I am unable to reproduce this with a simple sample application. You can find directions for setup in the latest commit. Can you update the sample to reproduce the issue you are seeing?

w.c.HttpSessionSecurityContextRepository : Failed to create a session, as response has been committed. Unable to store SecurityContext.

I'm able to reproduce this error and believe that DelegatingSecurityContextRepository needs to implement the loadContext(HttpRequestResponseHolder) method by invoking the same method on the delegates. Can you see if the following works around your issue?

Start by creating a new PatchedDelegatingSecurityContextRepository class.

package example;

import java.util.Arrays;
import java.util.List;

import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;

import org.springframework.security.core.context.DeferredSecurityContext;
import org.springframework.security.core.context.SecurityContext;
import org.springframework.security.web.context.HttpRequestResponseHolder;
import org.springframework.security.web.context.SecurityContextRepository;
import org.springframework.util.Assert;

public class PatchedDelegatingSecurityContextRepository implements SecurityContextRepository {

	private final List<SecurityContextRepository> delegates;

	public PatchedDelegatingSecurityContextRepository(SecurityContextRepository... delegates) {
		this(Arrays.asList(delegates));
	}

	public PatchedDelegatingSecurityContextRepository(List<SecurityContextRepository> delegates) {
		Assert.notEmpty(delegates, "delegates cannot be empty");
		this.delegates = delegates;
	}

	@Override
	public SecurityContext loadContext(HttpRequestResponseHolder requestResponseHolder) {
		SecurityContext result = null;
		for (SecurityContextRepository delegate : this.delegates) {
			SecurityContext delegateResult = delegate.loadContext(requestResponseHolder);
			if (result == null || delegate.containsContext(requestResponseHolder.getRequest())) {
				result = delegateResult;
			}
		}
		return result;
	}

	@Override
	public DeferredSecurityContext loadDeferredContext(HttpServletRequest request) {
		DeferredSecurityContext deferredSecurityContext = null;
		for (SecurityContextRepository delegate : this.delegates) {
			if (deferredSecurityContext == null) {
				deferredSecurityContext = delegate.loadDeferredContext(request);
			}
			else {
				DeferredSecurityContext next = delegate.loadDeferredContext(request);
				deferredSecurityContext = new DelegatingDeferredSecurityContext(deferredSecurityContext, next);
			}
		}
		return deferredSecurityContext;
	}

	@Override
	public void saveContext(SecurityContext context, HttpServletRequest request, HttpServletResponse response) {
		for (SecurityContextRepository delegate : this.delegates) {
			delegate.saveContext(context, request, response);
		}
	}

	@Override
	public boolean containsContext(HttpServletRequest request) {
		for (SecurityContextRepository delegate : this.delegates) {
			if (delegate.containsContext(request)) {
				return true;
			}
		}
		return false;
	}

	static final class DelegatingDeferredSecurityContext implements DeferredSecurityContext {

		private final DeferredSecurityContext previous;

		private final DeferredSecurityContext next;

		DelegatingDeferredSecurityContext(DeferredSecurityContext previous, DeferredSecurityContext next) {
			this.previous = previous;
			this.next = next;
		}

		@Override
		public SecurityContext get() {
			SecurityContext securityContext = this.previous.get();
			if (!this.previous.isGenerated()) {
				return securityContext;
			}
			return this.next.get();
		}

		@Override
		public boolean isGenerated() {
			return this.previous.isGenerated() && this.next.isGenerated();
		}

	}

}

Then in your security configuration add:

PatchedDelegatingSecurityContextRepository repository =
			new PatchedDelegatingSecurityContextRepository(new RequestAttributeSecurityContextRepository(), new HttpSessionSecurityContextRepository());
http
	.securityContext(sc -> sc
		.requireExplicitSave(false)
		.securityContextRepository(repository)
	)
	// ...

If that fixes the second issue, then we can get a fix pushed to Spring Security. However, I'd like to understand the first error as well.

[sbegaudeau]

Thanks for the response, I have tested your PatchedDelegatingSecurityContextRepository and it does properly restore the wrapper and everything works nicely after that.

However, I'd like to understand the first error as well

Well, I have looked into it since it seems strange now that I think about it. I have deactivated the requireExplicitSave(false) to restore the default behavior of spring-security 6.0.0. I did not have the time to test it with your simple sample application but I can offer additional information on what I am seeing for the moment.

In my application, I am relying on spring-graphql and from what I can see, it appears that everything is working nicely on the spring-security side but once I'm inside spring-graphql there seems to be a call to SecurityContextHolder#setContext which does not match the new recommendation of the documentation since it does not call SecurityContextRepository#saveContext. The issue seems to come from their SecurityContextThreadLocalAccessor$DelegateAccessor, visible here.

From what I understand, after the successful authentication using OAuth2, my frontend is redirecting the user to the homepage /, from there my JavaScript is performing some requests to /api/graphql to retrieve some information. The first request is a success since AuthorizationFilter#getAuthentication finds the OAuth2AuthenticationToken while the second one always find a AnonymousAuthenticationToken since a security context is not found and AnonymousAuthenticationFilter ends up creating an anonymous token which then fails to match the rules of my security configuration:

http.authorizeHttpRequests((authz) -> {
    authz.requestMatchers("/api/graphql").authenticated();
    authz.requestMatchers("/**").permitAll();
});

It seems that the first GraphQL request will always work and the second one will always fail so my frontend always end up receiving a 401 and thus it redirects my user on the login page. Between the two, Spring GraphQL always changes the value in the SecurityContextHolder. So I suspect that by using SecurityContextHolder.setContext((SecurityContext) value) without the SecurityContextRepository, Spring GraphQL may be overwriting something which breaks after that.

Would that be possible?

[oldshensheep]

I have the same issue with this and acting weird.

1. goto http://127.0.0.1:8080/who
   RESULT: anonymousUser
2. goto http://127.0.0.1:8080/login-not-working
   RESULT: a random UUID, e.g. 8b128bd6-e84c-4433-ab14-2869ce3c4677
3. goto http://127.0.0.1:8080/who
   RESULT: anonymousUser Expect a random UUID, the same as step 2
4. goto http://127.0.0.1:8080/login
   RESULT: a random UUID, e.g. 561e6e58-714d-4fe4-afde-8349ceb91973
5. goto http://127.0.0.1:8080/who
   RESULT: a random UUID, the same as step 4.

I have set requireExplicitSave(false). But I also Have to call securityContextRepository.saveContext(context, request, response); Otherwise login won't work

BUT, if run step 4 run before step 2, everything works fine.

Full repo: https://github.com/oldshensheep/shiny-octo-memory/tree/spring-security-issue-12314
Only a single class.

@Configuration
@RestController
@SpringBootApplication
public class DemoApplication {

    public static void main(String[] args) {
        SpringApplication.run(DemoApplication.class, args);
        System.out.println("DemoApplication started");
    }

    private SecurityContextRepository securityContextRepository = new HttpSessionSecurityContextRepository();

    @Bean
    public SecurityFilterChain filterChain(HttpSecurity http) throws Exception {

        http
                .csrf().disable()
                .authorizeHttpRequests((authz) -> authz
                        .anyRequest().permitAll())
                .securityContext((context) -> context
                        .requireExplicitSave(false))
                .formLogin().disable();
        return http.build();
    }

    @GetMapping("/who")
    public String getName() {
        return SecurityContextHolder.getContext().getAuthentication().getName();
    }

    @GetMapping("/login")
    public String login(HttpServletRequest request, HttpServletResponse response) {
        var context = SecurityContextHolder.createEmptyContext();
        var authentication = new UsernamePasswordAuthenticationToken(UUID.randomUUID(), "password");
        context.setAuthentication(authentication);
        SecurityContextHolder.setContext(context);
        securityContextRepository.saveContext(context, request, response);
        return SecurityContextHolder.getContext().getAuthentication().getName();
    }

    @GetMapping("/login-not-working")
    public String login2(HttpServletRequest request, HttpServletResponse response) {
        var context = SecurityContextHolder.createEmptyContext();
        var authentication = new UsernamePasswordAuthenticationToken(UUID.randomUUID(), "password");
        context.setAuthentication(authentication);
        SecurityContextHolder.setContext(context);
        // securityContextRepository.saveContext(context, request, response);
        return SecurityContextHolder.getContext().getAuthentication().getName();
    }

}

[marcusdacoregio]

Hi @oldshensheep, have you tried Rob's workaround?

[oldshensheep]

@marcusdacoregio
I have tried, and it's working.

[marcusdacoregio]

Hi @sbegaudeau,

I was able to simulate your scenario using GraphQL, and the problem has been fixed via this issue #11962.

The problem is that Spring GraphQL handles the request asynchronously, making use of the ASYNC dispatch, and the SecurityContextHolderFilter wasn't being invoked again for that dispatch. This problem has been fixed in Spring Security 6.0.1, therefore I recommend you upgrade to the latest Spring Boot version which is 3.0.4.

Can you upgrade the version and tell us if that fixes your problem?

[sbegaudeau]

Hi @sbegaudeau,

I was able to simulate your scenario using GraphQL, and the problem has been fixed via this issue #11962.

The problem is that Spring GraphQL handles the request asynchronously, making use of the ASYNC dispatch, and the SecurityContextHolderFilter wasn't being invoked again for that dispatch. This problem has been fixed in Spring Security 6.0.1, therefore I recommend you upgrade to the latest Spring Boot version which is 3.0.4.

Can you upgrade the version and tell us if that fixes your problem?

Hi,

Sorry for the delay, I had to find some time to update my application but I've switched to Spring Boot 3.0.5 and everything is working as expected. Thanks for the fix 👍