From 7677462195d21ca16fc7c6ba713e53f3589efee4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Old=C5=99ich=20Jedli=C4=8Dka?= Date: Thu, 9 Jul 2026 19:39:43 +0200 Subject: [PATCH 1/2] Add tests reproducing gh-18059 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Micrometer Context Propagation invokes ThreadLocalAccessor.getValue() when capturing a snapshot and when clearing thread-locals for a scope. SecurityContextHolderThreadLocalAccessor materializes the deferred SecurityContext in getValue(), so snapshot capture triggers session loading; when that load itself subscribes to a Mono (e.g. Lettuce connecting to Redis), propagation re-enters the accessor and recurses until StackOverflowError. Issue gh-18059 Co-Authored-By: Claude Fable 5 Signed-off-by: Oldřich Jedlička --- ...ContextHolderThreadLocalAccessorTests.java | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessorTests.java b/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessorTests.java index e04d0277ff5..d91954ed81f 100644 --- a/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessorTests.java +++ b/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessorTests.java @@ -16,9 +16,14 @@ package org.springframework.security.core.context; +import java.util.concurrent.atomic.AtomicInteger; + +import io.micrometer.context.ContextSnapshot; +import io.micrometer.context.ContextSnapshotFactory; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import reactor.util.context.Context; import org.springframework.security.authentication.TestingAuthenticationToken; @@ -90,4 +95,63 @@ public void setValueWhenSecurityContextSetThenClearsSecurityContextHolder() { assertThat(SecurityContextHolder.getContext()).isEqualTo(emptyContext); } + // gh-18059 + @Test + public void captureAllWhenDeferredContextSetThenDoesNotInvokeSupplier() { + SecurityContext securityContext = new SecurityContextImpl(new TestingAuthenticationToken("user", "password")); + AtomicInteger invocationCount = new AtomicInteger(); + SecurityContextHolder.setDeferredContext(() -> { + invocationCount.incrementAndGet(); + return securityContext; + }); + + ContextSnapshotFactory factory = ContextSnapshotFactory.builder().build(); + factory.captureAll(); + + assertThat(invocationCount.get()).as("snapshot capture must not invoke the deferred supplier").isZero(); + } + + // gh-18059 + @Test + public void setThreadLocalsFromWhenContextMissingKeyThenDoesNotInvokeSupplier() { + // The branch from the issue's stack trace: clearMissing=true and no + // SecurityContext key in the Reactor Context invokes accessor.getValue() + // to save the previous value before clearing. + SecurityContext securityContext = new SecurityContextImpl(new TestingAuthenticationToken("user", "password")); + AtomicInteger invocationCount = new AtomicInteger(); + SecurityContextHolder.setDeferredContext(() -> { + invocationCount.incrementAndGet(); + return securityContext; + }); + + ContextSnapshotFactory factory = ContextSnapshotFactory.builder().clearMissing(true).build(); + try (ContextSnapshot.Scope scope = factory.setThreadLocalsFrom(Context.empty())) { + assertThat(invocationCount.get()).as("entering the scope must not invoke the deferred supplier").isZero(); + } + + assertThat(invocationCount.get()).as("leaving the scope must not invoke the deferred supplier").isZero(); + } + + // gh-18059 + @Test + public void captureAllWhenDeferredContextReentersPropagationThenDoesNotRecurse() { + // Models the issue's Lettuce/Redis flow: materializing the deferred context + // itself triggers Mono.subscribe -> context propagation -> accessor.getValue(). + // Bounded so a regression fails the assertion instead of a StackOverflowError. + SecurityContext securityContext = new SecurityContextImpl(new TestingAuthenticationToken("user", "password")); + ContextSnapshotFactory factory = ContextSnapshotFactory.builder().build(); + AtomicInteger invocationCount = new AtomicInteger(); + SecurityContextHolder.setDeferredContext(() -> { + if (invocationCount.incrementAndGet() <= 3) { + factory.captureAll(); + } + return securityContext; + }); + + factory.captureAll(); + + assertThat(invocationCount.get()).as("snapshot capture must not invoke the deferred supplier, even reentrantly") + .isZero(); + } + } From a052bdb9ee68f38b026d8fb181c4b54e7215740d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Old=C5=99ich=20Jedli=C4=8Dka?= Date: Thu, 9 Jul 2026 19:46:52 +0200 Subject: [PATCH 2/2] Propagate deferred SecurityContext supplier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SecurityContextHolderThreadLocalAccessor now propagates the deferred Supplier instead of materializing the SecurityContext during Micrometer context snapshot capture. Materializing could load the session from a store whose client subscribes to a Mono, which re-entered the accessor through context propagation and recursed until StackOverflowError. The new SecurityContextHolderStrategy.peekDeferredContext() returns the stored Supplier without invoking it and without auto-creating or storing a context, so snapshot capture has no side effects. getValue() keeps the pre-existing behavior of treating an empty SecurityContext as absent, applied only when the context is already materialized (an uninvoked deferred Supplier is propagated uninspected). The default peekDeferredContext() implementation materializes the current context eagerly, preserving pre-7.1 behavior for custom strategies. setDeferredContext() no longer re-wraps an already-validated Supplier, so micrometer's save/restore cycle does not accumulate wrappers. Closes gh-18059 Co-Authored-By: Claude Fable 5 Signed-off-by: Oldřich Jedlička --- .../core/context/ConstantSupplier.java | 42 +++++ .../GlobalSecurityContextHolderStrategy.java | 8 + ...eadLocalSecurityContextHolderStrategy.java | 20 ++- ...isteningSecurityContextHolderStrategy.java | 10 ++ .../core/context/NotNullSupplier.java | 44 +++++ .../SecurityContextHolderStrategy.java | 19 +++ ...urityContextHolderThreadLocalAccessor.java | 27 ++- ...eadLocalSecurityContextHolderStrategy.java | 20 ++- ...balSecurityContextHolderStrategyTests.java | 64 ++++++++ ...calSecurityContextHolderStrategyTests.java | 54 ++++++ ...ingSecurityContextHolderStrategyTests.java | 15 ++ .../SecurityContextHolderStrategyTests.java | 89 ++++++++++ ...ContextHolderThreadLocalAccessorTests.java | 154 +++++++++++++++++- ...calSecurityContextHolderStrategyTests.java | 54 ++++++ docs/modules/ROOT/pages/whats-new.adoc | 1 + 15 files changed, 592 insertions(+), 29 deletions(-) create mode 100644 core/src/main/java/org/springframework/security/core/context/ConstantSupplier.java create mode 100644 core/src/main/java/org/springframework/security/core/context/NotNullSupplier.java create mode 100644 core/src/test/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategyTests.java create mode 100644 core/src/test/java/org/springframework/security/core/context/SecurityContextHolderStrategyTests.java diff --git a/core/src/main/java/org/springframework/security/core/context/ConstantSupplier.java b/core/src/main/java/org/springframework/security/core/context/ConstantSupplier.java new file mode 100644 index 00000000000..b695bfd4121 --- /dev/null +++ b/core/src/main/java/org/springframework/security/core/context/ConstantSupplier.java @@ -0,0 +1,42 @@ +/* + * Copyright 2004-present the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.core.context; + +import java.util.function.Supplier; + +import org.springframework.util.Assert; + +/** + * A {@link Supplier} of an already-materialized {@link SecurityContext}. Marker class so + * {@link SecurityContextHolderThreadLocalAccessor} can inspect the value without + * triggering deferred materialization. + */ +final class ConstantSupplier implements Supplier { + + private final SecurityContext context; + + ConstantSupplier(SecurityContext context) { + Assert.notNull(context, "context cannot be null"); + this.context = context; + } + + @Override + public SecurityContext get() { + return this.context; + } + +} diff --git a/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java index ded1a611b24..e477eeb36c5 100644 --- a/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java @@ -16,6 +16,8 @@ package org.springframework.security.core.context; +import java.util.function.Supplier; + import org.jspecify.annotations.Nullable; import org.springframework.util.Assert; @@ -52,6 +54,12 @@ public void setContext(SecurityContext context) { contextHolder = context; } + @Override + public @Nullable Supplier peekDeferredContext() { + SecurityContext context = contextHolder; + return (context != null) ? new ConstantSupplier(context) : null; + } + @Override public SecurityContext createEmptyContext() { return new SecurityContextImpl(); diff --git a/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java index e64d9058d1f..1403bdb9e63 100644 --- a/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java @@ -18,6 +18,8 @@ import java.util.function.Supplier; +import org.jspecify.annotations.Nullable; + import org.springframework.util.Assert; /** @@ -46,8 +48,7 @@ public SecurityContext getContext() { public Supplier getDeferredContext() { Supplier result = contextHolder.get(); if (result == null) { - SecurityContext context = createEmptyContext(); - result = () -> context; + result = new ConstantSupplier(createEmptyContext()); contextHolder.set(result); } return result; @@ -56,17 +57,15 @@ public Supplier getDeferredContext() { @Override public void setContext(SecurityContext context) { Assert.notNull(context, "Only non-null SecurityContext instances are permitted"); - contextHolder.set(() -> context); + contextHolder.set(new ConstantSupplier(context)); } @Override public void setDeferredContext(Supplier deferredContext) { Assert.notNull(deferredContext, "Only non-null Supplier instances are permitted"); - Supplier notNullDeferredContext = () -> { - SecurityContext result = deferredContext.get(); - Assert.notNull(result, "A Supplier returned null and is not allowed."); - return result; - }; + Supplier notNullDeferredContext = (deferredContext instanceof NotNullSupplier + || deferredContext instanceof ConstantSupplier) ? deferredContext + : new NotNullSupplier(deferredContext); contextHolder.set(notNullDeferredContext); } @@ -75,4 +74,9 @@ public SecurityContext createEmptyContext() { return new SecurityContextImpl(); } + @Override + public @Nullable Supplier peekDeferredContext() { + return contextHolder.get(); + } + } diff --git a/core/src/main/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategy.java index 771684c786d..e3c36faa47f 100644 --- a/core/src/main/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategy.java @@ -21,6 +21,8 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Supplier; +import org.jspecify.annotations.Nullable; + import org.springframework.util.Assert; /** @@ -150,6 +152,14 @@ public Supplier getDeferredContext() { return this.delegate.getDeferredContext(); } + /** + * {@inheritDoc} + */ + @Override + public @Nullable Supplier peekDeferredContext() { + return this.delegate.peekDeferredContext(); + } + /** * {@inheritDoc} */ diff --git a/core/src/main/java/org/springframework/security/core/context/NotNullSupplier.java b/core/src/main/java/org/springframework/security/core/context/NotNullSupplier.java new file mode 100644 index 00000000000..a1e73e6e099 --- /dev/null +++ b/core/src/main/java/org/springframework/security/core/context/NotNullSupplier.java @@ -0,0 +1,44 @@ +/* + * Copyright 2004-present the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.core.context; + +import java.util.function.Supplier; + +import org.springframework.util.Assert; + +/** + * Wraps a {@link Supplier} to reject {@code null} results. Marker class so + * {@link SecurityContextHolderStrategy} can skip re-wrapping on round-trip — without it, + * {@code setDeferredContext(getDeferredContext())} would accumulate one wrapper per call. + */ +final class NotNullSupplier implements Supplier { + + private final Supplier delegate; + + NotNullSupplier(Supplier delegate) { + Assert.notNull(delegate, "delegate cannot be null"); + this.delegate = delegate; + } + + @Override + public SecurityContext get() { + SecurityContext result = this.delegate.get(); + Assert.notNull(result, "A Supplier returned null and is not allowed."); + return result; + } + +} diff --git a/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderStrategy.java index 6b23733667e..fbf3a999347 100644 --- a/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderStrategy.java @@ -18,6 +18,8 @@ import java.util.function.Supplier; +import org.jspecify.annotations.Nullable; + /** * A strategy for storing security context information against a thread. * @@ -51,6 +53,23 @@ default Supplier getDeferredContext() { return this::getContext; } + /** + * Obtains the current context as a {@link Supplier} if one is associated with the + * current thread, or {@code null} otherwise. Used by Micrometer Context Propagation + * during snapshot capture, so implementations SHOULD return the stored + * {@link Supplier} without invoking it and without auto-creating or storing a default + * context as a side effect. + *

+ * The default implementation preserves the pre-7.1 propagation behavior: it + * materializes the current context eagerly and returns a {@link Supplier} of the + * captured context. + * @return the current context's {@link Supplier}, or {@code null} if none is set + * @since 7.1.1 + */ + default @Nullable Supplier peekDeferredContext() { + return new ConstantSupplier(getContext()); + } + /** * Sets the current context. * @param context to the new argument (should never be null, although diff --git a/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessor.java b/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessor.java index 170f260c19a..acbffd7f04a 100644 --- a/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessor.java +++ b/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessor.java @@ -16,6 +16,8 @@ package org.springframework.security.core.context; +import java.util.function.Supplier; + import io.micrometer.context.ThreadLocalAccessor; import org.jspecify.annotations.Nullable; @@ -30,12 +32,17 @@ * {@link SecurityContext} in Servlet applications. It is automatically registered with * the {@link io.micrometer.context.ContextRegistry} through the * {@link java.util.ServiceLoader} mechanism when context-propagation is on the classpath. + *

+ * The propagated value is the deferred {@link Supplier}, so capture does not materialize + * the {@link SecurityContext}; the supplier is invoked lazily on the consuming thread. An + * already-materialized {@link SecurityContext} that equals the empty context is treated + * as absent and is not propagated. * * @author Steve Riesenberg * @since 6.5 * @see io.micrometer.context.ContextRegistry */ -public final class SecurityContextHolderThreadLocalAccessor implements ThreadLocalAccessor { +public final class SecurityContextHolderThreadLocalAccessor implements ThreadLocalAccessor> { @Override public Object key() { @@ -43,17 +50,21 @@ public Object key() { } @Override - public @Nullable SecurityContext getValue() { - SecurityContext securityContext = SecurityContextHolder.getContext(); - SecurityContext emptyContext = SecurityContextHolder.createEmptyContext(); - - return !securityContext.equals(emptyContext) ? securityContext : null; + public @Nullable Supplier getValue() { + Supplier deferred = SecurityContextHolder.getContextHolderStrategy().peekDeferredContext(); + // The empty check is only possible when the context is already materialized; + // invoking a deferred supplier here would reintroduce gh-18059. + if (deferred instanceof ConstantSupplier constant + && constant.get().equals(SecurityContextHolder.createEmptyContext())) { + return null; + } + return deferred; } @Override - public void setValue(SecurityContext securityContext) { + public void setValue(Supplier securityContext) { Assert.notNull(securityContext, "securityContext cannot be null"); - SecurityContextHolder.setContext(securityContext); + SecurityContextHolder.getContextHolderStrategy().setDeferredContext(securityContext); } @Override diff --git a/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java index f4fb5689c02..27ba2a6ddc2 100644 --- a/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java @@ -18,6 +18,8 @@ import java.util.function.Supplier; +import org.jspecify.annotations.Nullable; + import org.springframework.util.Assert; /** @@ -47,8 +49,7 @@ public SecurityContext getContext() { public Supplier getDeferredContext() { Supplier result = contextHolder.get(); if (result == null) { - SecurityContext context = createEmptyContext(); - result = () -> context; + result = new ConstantSupplier(createEmptyContext()); contextHolder.set(result); } return result; @@ -57,17 +58,15 @@ public Supplier getDeferredContext() { @Override public void setContext(SecurityContext context) { Assert.notNull(context, "Only non-null SecurityContext instances are permitted"); - contextHolder.set(() -> context); + contextHolder.set(new ConstantSupplier(context)); } @Override public void setDeferredContext(Supplier deferredContext) { Assert.notNull(deferredContext, "Only non-null Supplier instances are permitted"); - Supplier notNullDeferredContext = () -> { - SecurityContext result = deferredContext.get(); - Assert.notNull(result, "A Supplier returned null and is not allowed."); - return result; - }; + Supplier notNullDeferredContext = (deferredContext instanceof NotNullSupplier + || deferredContext instanceof ConstantSupplier) ? deferredContext + : new NotNullSupplier(deferredContext); contextHolder.set(notNullDeferredContext); } @@ -76,4 +75,9 @@ public SecurityContext createEmptyContext() { return new SecurityContextImpl(); } + @Override + public @Nullable Supplier peekDeferredContext() { + return contextHolder.get(); + } + } diff --git a/core/src/test/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategyTests.java b/core/src/test/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategyTests.java new file mode 100644 index 00000000000..a3bca8f3b41 --- /dev/null +++ b/core/src/test/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategyTests.java @@ -0,0 +1,64 @@ +/* + * Copyright 2004-present the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.core.context; + +import java.util.function.Supplier; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import org.springframework.security.core.Authentication; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +class GlobalSecurityContextHolderStrategyTests { + + GlobalSecurityContextHolderStrategy strategy = new GlobalSecurityContextHolderStrategy(); + + @AfterEach + void clearContext() { + this.strategy.clearContext(); + } + + // gh-18059 + @Test + void peekDeferredContextWhenEmptyThenReturnsNull() { + assertThat(this.strategy.peekDeferredContext()).isNull(); + } + + // gh-18059 + @Test + void peekDeferredContextWhenSetThenReturnsSupplierOfSameContext() { + Authentication authentication = mock(Authentication.class); + SecurityContext context = new SecurityContextImpl(authentication); + this.strategy.setContext(context); + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(context); + } + + // gh-18059 + @Test + void peekDeferredContextWhenContextAutoCreatedThenReturnsSupplierOfSameContext() { + SecurityContext context = this.strategy.getContext(); + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(context); + } + +} diff --git a/core/src/test/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategyTests.java b/core/src/test/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategyTests.java index e1961240de6..0cb45a20bce 100644 --- a/core/src/test/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategyTests.java +++ b/core/src/test/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategyTests.java @@ -81,4 +81,58 @@ void getContextWhenEmptyThenReturnsSameInstance() { assertThat(this.strategy.getContext().getAuthentication()).isEqualTo(authentication); } + @Test + void setDeferredContextWhenAlreadyWrappedThenDoesNotRewrap() { + Authentication authentication = mock(Authentication.class); + this.strategy.setDeferredContext(() -> new SecurityContextImpl(authentication)); + Supplier wrapped = this.strategy.getDeferredContext(); + this.strategy.setDeferredContext(wrapped); + assertThat(this.strategy.getDeferredContext()).isSameAs(wrapped); + } + + // gh-18059 + @Test + void setDeferredContextWhenAlreadyMaterializedThenDoesNotRewrap() { + Authentication authentication = mock(Authentication.class); + this.strategy.setContext(new SecurityContextImpl(authentication)); + Supplier materialized = this.strategy.getDeferredContext(); + this.strategy.setDeferredContext(materialized); + assertThat(this.strategy.getDeferredContext()).isSameAs(materialized); + } + + // gh-18059 + @Test + void peekDeferredContextWhenNotSetThenReturnsNull() { + assertThat(this.strategy.peekDeferredContext()).isNull(); + } + + // gh-18059 + @Test + void peekDeferredContextWhenContextAutoCreatedThenReturnsSupplierOfSameContext() { + SecurityContext context = this.strategy.getContext(); + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(context); + } + + // gh-18059 + @Test + void peekDeferredContextWhenContextSetThenReturnsSupplierOfSameContext() { + Authentication authentication = mock(Authentication.class); + SecurityContext context = new SecurityContextImpl(authentication); + this.strategy.setContext(context); + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(context); + } + + // gh-18059 + @Test + void peekDeferredContextWhenDeferredContextSetThenReturnsSameSupplierWithoutInvoking() { + Supplier deferredContext = mock(Supplier.class); + this.strategy.setDeferredContext(deferredContext); + assertThat(this.strategy.peekDeferredContext()).isSameAs(this.strategy.getDeferredContext()); + verifyNoInteractions(deferredContext); + } + } diff --git a/core/src/test/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategyTests.java b/core/src/test/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategyTests.java index 22a9e6a596e..733972624b2 100644 --- a/core/src/test/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategyTests.java +++ b/core/src/test/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategyTests.java @@ -134,4 +134,19 @@ public void constructorWhenNullListenerThenIllegalArgument() { (SecurityContextChangedListener) null)); } + // gh-18059 + @Test + public void peekDeferredContextDelegatesToUnderlyingStrategy() { + SecurityContextHolderStrategy delegate = mock(SecurityContextHolderStrategy.class); + SecurityContextChangedListener listener = mock(SecurityContextChangedListener.class); + SecurityContextHolderStrategy strategy = new ListeningSecurityContextHolderStrategy(delegate, listener); + + given(delegate.peekDeferredContext()).willReturn(null); + assertThat(strategy.peekDeferredContext()).isNull(); + + Supplier supplier = () -> new SecurityContextImpl(); + given(delegate.peekDeferredContext()).willReturn(supplier); + assertThat(strategy.peekDeferredContext()).isSameAs(supplier); + } + } diff --git a/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderStrategyTests.java b/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderStrategyTests.java new file mode 100644 index 00000000000..9e271541fae --- /dev/null +++ b/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderStrategyTests.java @@ -0,0 +1,89 @@ +/* + * Copyright 2004-present the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.core.context; + +import java.util.function.Supplier; + +import org.jspecify.annotations.Nullable; +import org.junit.jupiter.api.Test; + +import org.springframework.security.core.Authentication; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +/** + * Tests for the {@link SecurityContextHolderStrategy} default methods. + */ +class SecurityContextHolderStrategyTests { + + LegacyStrategy strategy = new LegacyStrategy(); + + // gh-18059 + @Test + void peekDeferredContextWhenNotSetThenReturnsSupplierOfMaterializedContext() { + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(this.strategy.getContext()); + } + + // gh-18059 + @Test + void peekDeferredContextWhenContextSetThenReturnsSupplierOfCapturedContext() { + Authentication authentication = mock(Authentication.class); + SecurityContext context = new SecurityContextImpl(authentication); + this.strategy.setContext(context); + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + // the default captures the value eagerly, so it is not a live view + this.strategy.clearContext(); + assertThat(deferred.get()).isSameAs(context); + } + + /** + * A pre-5.8 style implementation relying on the interface default methods. + */ + static final class LegacyStrategy implements SecurityContextHolderStrategy { + + private @Nullable SecurityContext context; + + @Override + public void clearContext() { + this.context = null; + } + + @Override + public SecurityContext getContext() { + if (this.context == null) { + this.context = createEmptyContext(); + } + return this.context; + } + + @Override + public void setContext(SecurityContext context) { + this.context = context; + } + + @Override + public SecurityContext createEmptyContext() { + return new SecurityContextImpl(); + } + + } + +} diff --git a/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessorTests.java b/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessorTests.java index d91954ed81f..7bcbdb189e3 100644 --- a/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessorTests.java +++ b/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessorTests.java @@ -16,8 +16,16 @@ package org.springframework.security.core.context; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; +import io.micrometer.context.ContextExecutorService; import io.micrometer.context.ContextSnapshot; import io.micrometer.context.ContextSnapshotFactory; import org.junit.jupiter.api.AfterEach; @@ -29,6 +37,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyNoInteractions; /** * Tests for {@link SecurityContextHolderThreadLocalAccessor}. @@ -60,21 +70,67 @@ public void getValueWhenSecurityContextHolderNotSetThenReturnsNull() { } @Test - public void getValueWhenSecurityContextHolderSetThenReturnsSecurityContext() { + public void getValueWhenSecurityContextHolderSetThenReturnsSupplierOfSecurityContext() { SecurityContext securityContext = SecurityContextHolder.createEmptyContext(); securityContext.setAuthentication(new TestingAuthenticationToken("user", "password")); SecurityContextHolder.setContext(securityContext); - assertThat(this.threadLocalAccessor.getValue()).isSameAs(securityContext); + Supplier deferred = this.threadLocalAccessor.getValue(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(securityContext); } + // gh-18059 + @Test + public void getValueWhenContextAutoCreatedThenReturnsNull() { + SecurityContextHolder.getContext(); + assertThat(this.threadLocalAccessor.getValue()).isNull(); + } + + // gh-18059 + @Test + public void getValueWhenEmptyContextSetThenReturnsNull() { + SecurityContextHolder.setContext(SecurityContextHolder.createEmptyContext()); + assertThat(this.threadLocalAccessor.getValue()).isNull(); + } + + // gh-18059 + @Test + public void getValueWhenAutoCreatedContextModifiedThenReturnsSupplier() { + SecurityContext securityContext = SecurityContextHolder.getContext(); + securityContext.setAuthentication(new TestingAuthenticationToken("user", "password")); + Supplier deferred = this.threadLocalAccessor.getValue(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(securityContext); + } + + // gh-18059 @Test - public void setValueWhenSecurityContextThenSetsSecurityContextHolder() { + @SuppressWarnings("unchecked") + public void getValueWhenDeferredContextSetThenDoesNotInvokeSupplier() { + Supplier deferredContext = mock(Supplier.class); + SecurityContextHolder.setDeferredContext(deferredContext); + Supplier result = this.threadLocalAccessor.getValue(); + assertThat(result).isNotNull(); + verifyNoInteractions(deferredContext); + } + + @Test + public void setValueWhenSupplierThenSetsSecurityContextHolder() { SecurityContext securityContext = SecurityContextHolder.createEmptyContext(); securityContext.setAuthentication(new TestingAuthenticationToken("user", "password")); - this.threadLocalAccessor.setValue(securityContext); + this.threadLocalAccessor.setValue(() -> securityContext); assertThat(SecurityContextHolder.getContext()).isSameAs(securityContext); } + // gh-18059 + @Test + @SuppressWarnings("unchecked") + public void setValueWhenSupplierThenDoesNotInvokeSupplier() { + Supplier deferredContext = mock(Supplier.class); + this.threadLocalAccessor.setValue(deferredContext); + verifyNoInteractions(deferredContext); + } + @Test public void setValueWhenNullThenThrowsIllegalArgumentException() { // @formatter:off @@ -123,12 +179,14 @@ public void setThreadLocalsFromWhenContextMissingKeyThenDoesNotInvokeSupplier() invocationCount.incrementAndGet(); return securityContext; }); + Supplier before = this.threadLocalAccessor.getValue(); ContextSnapshotFactory factory = ContextSnapshotFactory.builder().clearMissing(true).build(); try (ContextSnapshot.Scope scope = factory.setThreadLocalsFrom(Context.empty())) { - assertThat(invocationCount.get()).as("entering the scope must not invoke the deferred supplier").isZero(); + assertThat(this.threadLocalAccessor.getValue()).as("cleared inside the scope").isNull(); } + assertThat(this.threadLocalAccessor.getValue()).as("restored on scope close, not re-wrapped").isSameAs(before); assertThat(invocationCount.get()).as("leaving the scope must not invoke the deferred supplier").isZero(); } @@ -154,4 +212,90 @@ public void captureAllWhenDeferredContextReentersPropagationThenDoesNotRecurse() .isZero(); } + // gh-18059 + @Test + public void getValueWhenDeferredContextReentersAccessorThenDoesNotRecurseAndReturnsCallableSupplier() { + SecurityContext securityContext = new SecurityContextImpl(new TestingAuthenticationToken("user", "password")); + AtomicInteger invocationCount = new AtomicInteger(); + Supplier reentrantSupplier = () -> { + // Bound recursion so a regression fails the assertion instead of + // crashing the JVM with StackOverflowError. + if (invocationCount.incrementAndGet() > 1) { + return securityContext; + } + Supplier nested = this.threadLocalAccessor.getValue(); + if (nested != null) { + nested.get(); + } + return securityContext; + }; + SecurityContextHolder.setDeferredContext(reentrantSupplier); + + Supplier result = this.threadLocalAccessor.getValue(); + + assertThat(result).isNotNull(); + assertThat(invocationCount.get()).isZero(); + assertThat(result.get()).isSameAs(securityContext); + } + + // gh-18059 + @Test + public void contextPropagationToWorkerThreadDoesNotMaterializeDeferredSupplier() + throws ExecutionException, InterruptedException, TimeoutException { + // Cross-thread propagation via ContextExecutorService: micrometer captures on + // submit() and applies the snapshot on the worker thread. + SecurityContext securityContext = new SecurityContextImpl(new TestingAuthenticationToken("user", "password")); + AtomicInteger invocationCount = new AtomicInteger(); + Supplier deferredSupplier = () -> { + invocationCount.incrementAndGet(); + return securityContext; + }; + + ExecutorService raw = Executors.newSingleThreadExecutor(); + ContextSnapshotFactory factory = ContextSnapshotFactory.builder().build(); + ExecutorService propagating = ContextExecutorService.wrap(raw, factory); + try { + assertThat(raw.submit(this.threadLocalAccessor::getValue).get(5, TimeUnit.SECONDS)) + .as("worker thread initially has no SecurityContext") + .isNull(); + + SecurityContextHolder.setDeferredContext(deferredSupplier); + Supplier captured = this.threadLocalAccessor.getValue(); + + AtomicInteger invocationsAtTaskStart = new AtomicInteger(-1); + AtomicReference> workerSupplier = new AtomicReference<>(); + AtomicReference workerContext = new AtomicReference<>(); + AtomicInteger invocationsAfterMaterialization = new AtomicInteger(-1); + + propagating.submit(() -> { + invocationsAtTaskStart.set(invocationCount.get()); + workerSupplier.set(this.threadLocalAccessor.getValue()); + workerContext.set(SecurityContextHolder.getContext()); + invocationsAfterMaterialization.set(invocationCount.get()); + }).get(5, TimeUnit.SECONDS); + + assertThat(invocationsAtTaskStart.get()) + .as("micrometer's capture+apply path must not invoke the deferred supplier") + .isZero(); + assertThat(workerSupplier.get()) + .as("worker thread receives the same supplier instance captured on the submitter") + .isSameAs(captured); + assertThat(workerContext.get()) + .as("SecurityContextHolder.getContext() on the worker materializes the supplier") + .isSameAs(securityContext); + assertThat(invocationsAfterMaterialization.get()) + .as("materialization on the worker invokes the supplier exactly once") + .isOne(); + + // Probe via the unwrapped executor so this submission does not propagate. + assertThat(raw.submit(this.threadLocalAccessor::getValue).get(5, TimeUnit.SECONDS)) + .as("worker thread's prior (empty) state must be restored after the task") + .isNull(); + } + finally { + propagating.shutdown(); + raw.shutdownNow(); + } + } + } diff --git a/core/src/test/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategyTests.java b/core/src/test/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategyTests.java index 23c238ac732..79de7897909 100644 --- a/core/src/test/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategyTests.java +++ b/core/src/test/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategyTests.java @@ -81,4 +81,58 @@ void getContextWhenEmptyThenReturnsSameInstance() { assertThat(this.strategy.getContext().getAuthentication()).isEqualTo(authentication); } + @Test + void setDeferredContextWhenAlreadyWrappedThenDoesNotRewrap() { + Authentication authentication = mock(Authentication.class); + this.strategy.setDeferredContext(() -> new SecurityContextImpl(authentication)); + Supplier wrapped = this.strategy.getDeferredContext(); + this.strategy.setDeferredContext(wrapped); + assertThat(this.strategy.getDeferredContext()).isSameAs(wrapped); + } + + // gh-18059 + @Test + void setDeferredContextWhenAlreadyMaterializedThenDoesNotRewrap() { + Authentication authentication = mock(Authentication.class); + this.strategy.setContext(new SecurityContextImpl(authentication)); + Supplier materialized = this.strategy.getDeferredContext(); + this.strategy.setDeferredContext(materialized); + assertThat(this.strategy.getDeferredContext()).isSameAs(materialized); + } + + // gh-18059 + @Test + void peekDeferredContextWhenNotSetThenReturnsNull() { + assertThat(this.strategy.peekDeferredContext()).isNull(); + } + + // gh-18059 + @Test + void peekDeferredContextWhenContextAutoCreatedThenReturnsSupplierOfSameContext() { + SecurityContext context = this.strategy.getContext(); + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(context); + } + + // gh-18059 + @Test + void peekDeferredContextWhenContextSetThenReturnsSupplierOfSameContext() { + Authentication authentication = mock(Authentication.class); + SecurityContext context = new SecurityContextImpl(authentication); + this.strategy.setContext(context); + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(context); + } + + // gh-18059 + @Test + void peekDeferredContextWhenDeferredContextSetThenReturnsSameSupplierWithoutInvoking() { + Supplier deferredContext = mock(Supplier.class); + this.strategy.setDeferredContext(deferredContext); + assertThat(this.strategy.peekDeferredContext()).isSameAs(this.strategy.getDeferredContext()); + verifyNoInteractions(deferredContext); + } + } diff --git a/docs/modules/ROOT/pages/whats-new.adoc b/docs/modules/ROOT/pages/whats-new.adoc index 76314d2c876..7931fdcea2e 100644 --- a/docs/modules/ROOT/pages/whats-new.adoc +++ b/docs/modules/ROOT/pages/whats-new.adoc @@ -5,6 +5,7 @@ * https://github.com/spring-projects/spring-security/pull/18634[gh-18634] - Added javadoc:org.springframework.security.util.matcher.InetAddressMatcher[] * https://github.com/spring-projects/spring-security/issues/18960[gh-18960] - Added xref:servlet/authentication/mfa.adoc#all-factors-anyof[AllRequiredFactorsAuthorizationManager.anyOf] +* https://github.com/spring-projects/spring-security/issues/18059[gh-18059] - `SecurityContextHolderThreadLocalAccessor` now propagates the deferred `SecurityContext` supplier without materializing it during context snapshot capture == Web * https://github.com/spring-projects/spring-security/issues/18755[gh-18755] - Include `charset` in `WWW-Authenticate` header