search

IdentityModel / IdentityModel.AspNetCore.OAuth2Introspection

ASP.NET Core authentication handler for OAuth 2.0 token introspection
Apache License 2.0
147 stars 66 forks source link

Potential issue with unawaited UpdateClientAssertion event #160

Open ovska opened 2 years ago

ovska commented 2 years ago

https://github.com/IdentityModel/IdentityModel.AspNetCore.OAuth2Introspection/blob/main/src/OAuth2IntrospectionHandler.cs#L211

The OnUpdateClientAssertion event is invoked without an await, probably because it is inside a lock. This can potentially cause hard to diagnose bugs if the event is assigned to something that requires awaiting. For example the current HttpContext is one of the properties, and using it after the request is ended can throw object disposed exceptions.

My suggestion is to replace the lock with a SemaphoreSlim which works fine in async context as well, and the lifetime of OAuth2IntrospectionOptions shouldn't be a problem as SemaphoreSlim wouldn't need to be disposed in this case.

Tangentially, ConfigureAwait(false) is missing from some all awaits on events. Is this intentional or an oversight?

leastprivilege commented 2 years ago

My understanding is that ASP.NET Core has no synchronization context - so ConfigureAwait(false) is not required.

Would you agree?

ovska commented 2 years ago

Correct, was just wondering why it was used in some awaits and not all :)

leastprivilege commented 2 years ago

I guess because it is ultimately an old code base..

leastprivilege commented 2 years ago

@brockallen could you have a look when time permits.

@ovska do you want to propose a PR?

ovska commented 2 years ago

Here's more or less the minimal fix I could come up with: #164