-
Notifications
You must be signed in to change notification settings - Fork 6.2k
SecurityContextHolderThreadLocalAccessor should not share SecurityContext instance across threads #18210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 6.5.x
Are you sure you want to change the base?
SecurityContextHolderThreadLocalAccessor should not share SecurityContext instance across threads #18210
Conversation
`SecurityContextHolderThreadLocalAccessor` currently propagates the same `SecurityContext` to other threads when Micrometer Context Propagation is used. This leads to unintended sharing of mutable state and can cause authentication to leak between threads. This change updates the accessor to create a new `SecurityContext` for the target thread while reusing only the `Authentication` value. Each thread now receives its own `SecurityContext` instance, preventing cross-thread interference and aligning with recommended `SecurityContext` usage. Signed-off-by: Tadaya Tsuyukubo <tadaya@ttddyy.net>
| public void setValue(SecurityContext securityContext) { | ||
| Assert.notNull(securityContext, "securityContext cannot be null"); | ||
| SecurityContextHolder.setContext(securityContext); | ||
| SecurityContext newContext = SecurityContextHolder.createEmptyContext(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question : Can we apply singleton concept here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole point of this PR is not to share the SecurityContext, so making it a singleton defeats the purpose.
| Assert.notNull(securityContext, "securityContext cannot be null"); | ||
| SecurityContextHolder.setContext(securityContext); | ||
| SecurityContext newContext = SecurityContextHolder.createEmptyContext(); | ||
| newContext.setAuthentication(securityContext.getAuthentication()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newContext.setAuthentication(Objects.requireNonNull(securityContext.getAuthentication()))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spring projects use Assert, which throws IllegalArgumentException instead of NullPointerException.
We encountered an issue where authentication was being mixed across threads.
During our analysis, we discovered that
SecurityContextHolderThreadLocalAccessorpropagates the sameSecurityContextto other threads when using Micrometer Context Propagation.Below is a simplified example that demonstrates the problem:
When Micrometer context propagation is in use, the current implementation effectively shares the
SecurityContextinstance between threads. This is generally not recommended and can cause subtle and hard-to-diagnose behavior, such as authentication leakage across threads.We understand that swapping the
Authenticationdirectly is rarely a good practice. However, if a newSecurityContexthad been used for the different thread, it would have been isolated and harmless. The underlying issue happens only because the sameSecurityContextinstance is shared across threads.Proposed Change
This PR updates
SecurityContextHolderThreadLocalAccessorto create a newSecurityContextwhenever a thread switch happens, and propagte only theAuthentication.I have targetted the PR to
6.5.xbranch as I consider it is a bug and should be fixed in there and up.