Spring Security NPE in HttpSessionSecurityContextRepository.isTransientAuthentication

Describe the bug On our systems we see a NullPointerException for cached static resources with spring-security-web-5.3.3:

java.lang.NullPointerException: null
    at org.springframework.security.web.context.HttpSessionSecurityContextRepository.isTransientAuthentication(HttpSessionSecurityContextRepository.java:444) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
    at org.springframework.security.web.context.HttpSessionSecurityContextRepository.access$300(HttpSessionSecurityContextRepository.java:82) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
    at org.springframework.security.web.context.HttpSessionSecurityContextRepository$SaveToSessionResponseWrapper.createNewSessionIfAllowed(HttpSessionSecurityContextRepository.java:389) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
    at org.springframework.security.web.context.HttpSessionSecurityContextRepository$SaveToSessionResponseWrapper.saveContext(HttpSessionSecurityContextRepository.java:363) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
    at org.springframework.security.web.context.SaveContextOnUpdateOrErrorResponseWrapper.onResponseCommitted(SaveContextOnUpdateOrErrorResponseWrapper.java:85) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
    at org.springframework.security.web.util.OnCommittedResponseWrapper.doOnResponseCommitted(OnCommittedResponseWrapper.java:252) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
    at org.springframework.security.web.util.OnCommittedResponseWrapper.checkContentLength(OnCommittedResponseWrapper.java:242) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
    at org.springframework.security.web.util.OnCommittedResponseWrapper.access$200(OnCommittedResponseWrapper.java:34) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
    at org.springframework.security.web.util.OnCommittedResponseWrapper$SaveContextServletOutputStream.write(OnCommittedResponseWrapper.java:644) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
    at org.springframework.security.web.util.OnCommittedResponseWrapper$SaveContextServletOutputStream.write(OnCommittedResponseWrapper.java:645) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
    at org.springframework.util.FastByteArrayOutputStream.writeTo(FastByteArrayOutputStream.java:249) ~[spring-core-5.2.7.RELEASE.jar:5.2.7.RELEASE]
    at org.springframework.web.util.ContentCachingResponseWrapper.copyBodyToResponse(ContentCachingResponseWrapper.java:215) ~[spring-web-5.2.7.RELEASE.jar:5.2.7.RELEASE]
    at org.springframework.web.util.ContentCachingResponseWrapper.copyBodyToResponse(ContentCachingResponseWrapper.java:199) ~[spring-web-5.2.7.RELEASE.jar:5.2.7.RELEASE]
    at org.springframework.web.filter.ShallowEtagHeaderFilter.updateResponse(ShallowEtagHeaderFilter.java:132) ~[spring-web-5.2.7.RELEASE.jar:5.2.7.RELEASE]
    at org.springframework.web.filter.ShallowEtagHeaderFilter.doFilterInternal(ShallowEtagHeaderFilter.java:109) ~[spring-web-5.2.7.RELEASE.jar:5.2.7.RELEASE]
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:119) ~[spring-web-5.2.7.RELEASE.jar:5.2.7.RELEASE]
    at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1618) ~[jetty-servlet-9.4.28.v20200408.jar:9.4.28.v20200408]
    at org.springframework.security.web.authentication.preauth.AbstractPreAuthenticatedProcessingFilter.doFilter(AbstractPreAuthenticatedProcessingFilter.java:124) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
    [...]

This seems to happen because the authentication returned by SecurityContextHolder.getContext() is null.

To Reproduce Unknown, because the error does not happen often.

Expected behavior I am not sure if the authentication may be null at all, but instead of a NullPointerException resulting in an "Internal Server Error" I would expect a plain "Unauthorized".

Comment From: jzheaux

@stovocor, I agree that a NullPointerException in this case is not desirable.

Since isTransientAuthentication cannot determine whether or not the authentication is transient in this case, I think it's reasonable to have it return false.

Comment From: marcusdacoregio

@stovocor, thank you for pointing this out.

I was looking into the code and the authentication is checked for null before the method is called:

if (authentication == null || trustResolver.isAnonymous(authentication)) { // authentication is being checked for null
    if (logger.isDebugEnabled()) {
        logger.debug("SecurityContext is empty or contents are anonymous - context will not be stored in HttpSession.");
    }

    if (httpSession != null && authBeforeExecution != null) {
        // SEC-1587 A non-anonymous context may still be in the session
        // SEC-1735 remove if the contextBeforeExecution was not anonymous
        httpSession.removeAttribute(springSecurityContextKey);
    }
    return;
}

if (httpSession == null) {
    httpSession = createNewSessionIfAllowed(context); // the method that is throwing NPE call
}

So in the scenario where the authentication is null it shouldreturn from this method and not reach the createNewSessionIfAllowed.

It seems to be a concurrency issue where the authentication is getting removed after the authentication == null test and before the createNewSessionIfAllowed method call.

To narrow the problem down, could you provide a sample that replicates the behavior?

Comment From: windmueller

To narrow the problem down, could you provide a sample that replicates the behavior?

I am very sorry, but as I mentioned in the original bugreport, I am not able to reproduce this problem and it occurs rarely.

Comment From: marcusdacoregio

@stovocor I see, thanks for the feedback. We are planning to work on a more general solution that would address more scenarios. @jzheaux do we have already an issue open for that?

Comment From: jzheaux

Yes, https://github.com/spring-projects/spring-security/issues/9634

Comment From: marcusdacoregio

Hi @stovocor, we have now a proposed PR #9813 to address this problem.

Comment From: marcusdacoregio

Hello @stovocor, the PR is now merged and is aimed at the milestone 5.6.0-M1. Thanks for the report and feel free to discuss here if you want anything else.