Wrong SecurityContext with async-mode-enabled: true results

[ncioj10]

Describe the bug
The Spring Security Context obtained by the OncePerRequestFilter is wrong when upgrading to 11.1.0 with async-mode-enabled: true by default.
This can lead to very serious security concerns as the context is also not cleared correctly so requests get sometimes authorized with credentials from other users.

To Reproduce
Create a Filter and try to access the context with SecurityContextHolder within the dataFetchers.

Expected behavior
The Security Context should contain the correct context.

[oliemansm]

Thanks for reporting @ncioj10! Just noticed the same thing. The problem is caused by this snippet in HttpRequestInvokerImpl in graphql-java-servlet:

    asyncContext.start(
        () -> {
          FutureExecutionResult futureResult = invoke(invocationInput, request, response);
          futureHolder.set(futureResult);
          handle(futureResult, request, response, listenerHandler)
              .thenAccept(it -> asyncContext.complete());
        });

We probably need to introduce something like an AsyncContextTaskDecorator in that library, and then provide an autoconfigured AsyncSecurityContextTaskDecorator implementation of it here if Spring Security is on the class path that wraps that runnable to propagate the SecurityContext.

[ncioj10]

As this has some security implications I would propose to not set it as default value if possible as a first step?

As for the fix: I'm not too deep into the code of graphql-java-servlet, but I think one of the problems will be that Spring can take care of this if you have it running on a separate thread, but in this case we're running on the same Thread only in completable futures, so it might get a little interesting to set this without producing interference as it sets it globally?

[oliemansm]

You'll have to stick with 11.0.0 for now to not have this behavior or just disable async-mode through props as you've already figured out.

It's not running on the same Thread actually. In the snippet above, the asyncContext.start(Runnable) method starts that lambda as a Thread. So having that Runnable wrapped with the decorator I descirbed that copies the SecurityContext before starting and clears it again after should do the trick.

[ncioj10]

Yes, I'd just try to mitigate problems for those who update the library and did not notice the change. Otherwise it would make sense to update the Readme so it warns about it.

I see, you mean something like this as fix right: spring-projects/spring-security#6856 (comment) ?

[ncioj10]

Sorry, wrong button

[oliemansm]

Yes exactly. Good point about the readme. I'll add a warning at the top and update the release notes too.

[ssprang]

Shouldn't the executing thread be wrapped in a org.springframework.security.task.DelegatingSecurityContextAsyncTaskExecutor instead of introducing a AsyncContextTaskDecorator?

[oliemansm]

I understand the thought, but graphql-java-servlet is independent of Spring, so that's not an option unfortunately.

[ssprang]

Is there a way to globally specify the Executor for the AsyncDataFetcher? That way I could specify one that decorates threads with a DelegatingSecurityContext..

val executor = ThreadPoolTaskExecutor()
// Set parallelism explicitly, default is 1
executor.corePoolSize = 5
// Max pool size (threads to scale up to if queue is full)
executor.maxPoolSize = 10
// Possibility to set queue capacity, default is Integer.MAX_VALUE
// executor.setQueueCapacity(500)
executor.setThreadNamePrefix("async-data-fetching-")
executor.setTaskDecorator { runnable -> DelegatingSecurityContextRunnable(runnable) }
executor.initialize()
....
// Pass executor for the AsyncDataFetcher somehow

[oliemansm]

That ThreadPoolTaskExecutor is again a Spring construct. Reading this article sheds some insight and raises the question whether we should be using this in the first place, or maybe like @ncioj10 suggested we should at least disable it by default. The article is from 2012, but I just ran a test and it's currently still using the HTTP worker thread pool to run these async Threads on.

See: https://dzone.com/articles/limited-usefulness

[ssprang]

Sorry, my example was too specific. It doesn't really matter which executor I pass in, I just wonder if there's a way to specify it myself because then I have the possibility to decide stuff like amount of threads and wrap it in a DelegatingSecurityContext in my Spring project.

[oliemansm]

So far I haven't found a way to define the thread pool executor to be used when running AsyncContext.start. And the latter is necessary to be able to benefit from the timeout option. If we can find a way to do that then we can make it configurable of course. Any help trying to find that way is appreciated... 😬