snyk · basti-snyk · Jun 23, 2026
@@ -124,7 +124,15 @@ func Test_DefaultOAuthProvider_storageCallbackUpdatesCredentialsAndNotifies(t *t
 	assert.Empty(t, receivedAuthParams.ApiUrl)
 }
 
-func Test_RegisterOAuthStorageBridge_UsesDefaultUnlockedCallback(t *testing.T) {
+// Test_RegisterOAuthStorageBridge_CallbackDoesNotBlockSet verifies that the OAuth
+// storage bridge callback enqueues asynchronously so that storageWithCallbacks.Set
+// returns immediately without blocking, and the token eventually propagates to the
+// configuration. The old test held serviceImpl.m to assert non-blocking, but the
+// worker now also acquires m before writing credentials (to provide a happens-before
+// guarantee against concurrent synchronous UpdateCredentials calls). Holding m
+// externally would deadlock the worker. The invariant — that Set() is non-blocking —
+// is preserved because the callback only performs a non-blocking channel send.
+func Test_RegisterOAuthStorageBridge_CallbackDoesNotBlockSet(t *testing.T) {
 	engine, tokenService := testutil.UnitTestWithEngine(t)
 	conf := engine.GetConfiguration()
 	storageWithCallbacks, err := storage2.NewStorageWithCallbacks(storage2.WithStorageFile(filepath.Join(t.TempDir(), "testStorage")))
@@ -140,8 +148,6 @@ func Test_RegisterOAuthStorageBridge_UsesDefaultUnlockedCallback(t *testing.T) {
 		notification.NewNotifier(),
 		testutil.DefaultConfigResolver(engine),
 	)
-	serviceImpl, ok := service.(*AuthenticationServiceImpl)
-	require.True(t, ok)
 
 	RegisterOAuthStorageBridge(storageWithCallbacks, service)
 
@@ -154,13 +160,10 @@ func Test_RegisterOAuthStorageBridge_UsesDefaultUnlockedCallback(t *testing.T) {
 	require.NoError(t, err)
 	rotatedToken := string(rotatedTokenBytes)
 
-	serviceImpl.m.Lock()
-	defer serviceImpl.m.Unlock()
-
 	require.NoError(t, storageWithCallbacks.Set(auth.CONFIG_KEY_OAUTH_TOKEN, rotatedToken))
 	require.Eventually(t, func() bool {
 		return config.GetToken(conf) == rotatedToken
-	}, 2*time.Second, time.Millisecond)
+	}, 5*time.Second, time.Millisecond)
 }
 
 func Test_RegisterOAuthStorageBridge_EmitsSecretSafeLogMarkers(t *testing.T) {

@@ -63,6 +63,15 @@ type AuthenticationService interface {
 	// but before the $/snyk.hasAuthenticated notification is sent to the IDE.
 	// This allows callers to perform setup (e.g. fetching feature flags) while the token
 	// is available but before the IDE reacts to the authentication event.
+	//
+	// Lock-state contract: the hook is invoked WITHOUT a.m held when called from
+	// the credentialUpdateWorker (async path), but WITH a.m held when called from
+	// the synchronous path (UpdateCredentials / Authenticate / Logout via
+	// updateCredentials → runPostMutationEffects). The hook MUST be safe to run
+	// in either state. In particular, the hook must NOT call back into any method
+	// that attempts to acquire a.m (e.g. UpdateCredentials, Logout), because
+	// sync.RWMutex is not reentrant: doing so from the synchronous path will
+	// deadlock.
 	SetPostCredentialUpdateHook(hook func())
 
 	// AuthURL retrieves the authentication URL

@@ -27,6 +27,7 @@ import (
 	"reflect"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/erni27/imcache"
@@ -55,6 +56,11 @@ type credentialUpdate struct {
 	token            string
 	sendNotification bool
 	updateApiUrl     bool
+	// generation is the value of syncGeneration at enqueue time. The worker
+	// discards updates whose generation is older than the current syncGeneration,
+	// preventing a stale async clear from overwriting a token set by a later
+	// synchronous UpdateCredentials call.
+	generation uint64
 }
 
 type AuthenticationServiceImpl struct {
@@ -90,6 +96,21 @@ type AuthenticationServiceImpl struct {
 	credentialUpdateChan chan credentialUpdate
 	// credentialUpdateCancel cancels the credential update worker on shutdown.
 	credentialUpdateCancel context.CancelFunc
+	// syncGeneration is a monotonic counter used to detect stale async credential
+	// updates. QueueCredentialUpdate captures it at enqueue time; the worker
+	// discards any update whose generation is older than the current value,
+	// preventing a stale OAuth-bridge clear from overwriting a token that was
+	// synchronously set by a later applyToken / IDE configuration change.
+	//
+	// Invariant: syncGeneration is incremented ONLY inside updateCredentials,
+	// which is always called while a.m is held (by UpdateCredentials, Authenticate,
+	// and Logout). The worker reads it under a.m as well, giving a proper
+	// happens-before guarantee. Any new code that increments syncGeneration MUST
+	// also hold a.m; failing to do so breaks the stale-discard guarantee and is
+	// a data race. Reads/writes use atomic operations to satisfy the race detector
+	// across the lock boundary (worker acquires a.m before the load, but
+	// QueueCredentialUpdate reads without the lock).
+	syncGeneration uint64
 }
 
 func NewAuthenticationService(engine workflow.Engine, tokenService types.TokenService, authProviders AuthenticationProvider, errorReporter error_reporting.ErrorReporter, notifier noti.Notifier, configResolver types.ConfigResolverInterface) AuthenticationService {
@@ -122,25 +143,61 @@ func (a *AuthenticationServiceImpl) AuthURL(ctx context.Context) string {
 
 // credentialUpdateWorker processes credential updates sequentially from the channel.
 // This prevents race conditions where older tokens overwrite newer ones during rapid rotations.
+//
+// Lock-scope discipline: a.m is held ONLY for the minimal critical section — the
+// generation check and the token mutation. It is released BEFORE running
+// post-mutation effects (postCredentialUpdateHook, GetGlobalOrganization prime,
+// notification send). This ensures that a hook that calls back into any
+// a.m-locking method (e.g. UpdateCredentials, Logout) does NOT deadlock the
+// worker goroutine. sync.RWMutex is not reentrant, so holding a.m across an
+// arbitrary caller-supplied hook was a latent deadlock surface.
+//
+// The worker acquires a.m before checking the generation counter so that any
+// concurrent synchronous credential write — which increments syncGeneration
+// under a.m — has already finished by the time the worker evaluates the
+// generation, giving a proper happens-before guarantee.
 func (a *AuthenticationServiceImpl) credentialUpdateWorker(ctx context.Context) {
 	for {
 		select {
 		case <-ctx.Done():
 			return
 		case update := <-a.credentialUpdateChan:
-			a.updateCredentials(update.token, update.sendNotification, update.updateApiUrl)
+			// Critical section: generation check + token mutation only.
+			a.m.Lock()
+			if update.generation < atomic.LoadUint64(&a.syncGeneration) {
+				a.engine.GetLogger().Debug().
+					Uint64("update_generation", update.generation).
+					Uint64("current_generation", atomic.LoadUint64(&a.syncGeneration)).
+					Bool("token_empty", update.token == "").
+					Msg("credential update worker: discarding stale async update superseded by synchronous credential write")
+				a.m.Unlock()
+				continue
+			}
+			applied := a.applyTokenMutationLocked(update.token, update.updateApiUrl)
+			a.m.Unlock()
+
+			// Post-mutation effects run WITHOUT a.m so that hooks which call
+			// back into UpdateCredentials / Logout do not self-deadlock.
+			if applied {
+				a.runPostMutationEffects(update.token, update.sendNotification, update.updateApiUrl)
+			}
 		}
 	}
 }
 
 // QueueCredentialUpdate queues a credential update for sequential processing.
 // This is used by the OAuth storage bridge callback to serialize updates.
+// The current syncGeneration is captured at enqueue time; the worker discards
+// the update if a later synchronous UpdateCredentials call has already incremented
+// the generation past the captured value.
 func (a *AuthenticationServiceImpl) QueueCredentialUpdate(token string, sendNotification bool, updateApiUrl bool) {
+	gen := atomic.LoadUint64(&a.syncGeneration)
 	select {
 	case a.credentialUpdateChan <- credentialUpdate{
 		token:            token,
 		sendNotification: sendNotification,
 		updateApiUrl:     updateApiUrl,
+		generation:       gen,
 	}:
 	default:
 		a.engine.GetLogger().Warn().
@@ -513,18 +570,45 @@ func (a *AuthenticationServiceImpl) UpdateCredentials(newToken string, sendNotif
 	a.updateCredentials(newToken, sendNotification, updateApiUrl)
 }
 
+// updateCredentials is the combined credential-write path for callers that
+// already hold a.m (Authenticate, Logout, UpdateCredentials). It increments
+// syncGeneration so that any async update queued at the previous generation is
+// considered stale by the credentialUpdateWorker, then applies the token
+// mutation and runs the post-mutation effects (hook, GlobalOrg prime,
+// notifier) while still holding a.m (pre-existing behavior for these callers;
+// the hook-deadlock risk described in Finding 2 only applied to the worker,
+// which is now fixed separately via applyTokenMutationLocked +
+// runPostMutationEffects).
 func (a *AuthenticationServiceImpl) updateCredentials(newToken string, sendNotification bool, updateApiUrl bool) {
+	// Increment syncGeneration BEFORE the token write, while holding a.m.
+	// This supersedes any async update that was enqueued at the previous
+	// generation: when the worker acquires a.m, it will see
+	// update.generation < syncGeneration and discard the stale update.
+	// Applies to all three callers: UpdateCredentials, authenticate, logout.
+	atomic.AddUint64(&a.syncGeneration, 1)
+
+	if !a.applyTokenMutationLocked(newToken, updateApiUrl) {
+		return
+	}
+	a.runPostMutationEffects(newToken, sendNotification, updateApiUrl)
+}
+
+// applyTokenMutationLocked performs the lock-critical part of a credential
+// update: token write, cache invalidation, and notifDedup reset. It MUST be
+// called with a.m held (either a.m.Lock from a synchronous caller, or from
+// the worker's critical section). Returns true if the mutation was applied
+// (token changed or updateApiUrl forced a refresh), false if nothing changed.
+func (a *AuthenticationServiceImpl) applyTokenMutationLocked(newToken string, updateApiUrl bool) bool {
 	conf := a.engine.GetConfiguration()
 	oldToken := config.GetToken(conf)
 	if oldToken == newToken && !updateApiUrl {
-		return
+		return false
 	}
 
 	a.engine.GetLogger().Debug().
 		Str("method", "AuthenticationService.updateCredentials").
 		Bool("old_token_empty", oldToken == "").
 		Bool("new_token_empty", newToken == "").
-		Bool("send_notification", sendNotification).
 		Bool("update_api_url", updateApiUrl).
 		Str("authentication_method", string(config.GetAuthenticationMethodFromConfig(conf))).
 		Msg("auth credentials update requested")
@@ -541,7 +625,7 @@ func (a *AuthenticationServiceImpl) updateCredentials(newToken string, sendNotif
 				Bool("current_token_empty", config.GetToken(conf) == "").
 				Str("authentication_method", string(config.GetAuthenticationMethodFromConfig(conf))).
 				Msg("auth credentials update skipped because token was not applied")
-			return
+			return false
 		}
 		// Reset the notification cooldown so the user gets immediate feedback after changing credentials
 		a.notifDedup.Lock()
@@ -550,6 +634,17 @@ func (a *AuthenticationServiceImpl) updateCredentials(newToken string, sendNotif
 		a.notifDedup.Unlock()
 	}
 
+	return true
+}
+
+// runPostMutationEffects runs the post-credential-write effects: the
+// postCredentialUpdateHook, the GlobalOrg prime, and the notification send.
+// For the credentialUpdateWorker these run WITHOUT a.m held, preventing the
+// hook from deadlocking the worker if it calls back into any a.m-locking
+// method (UpdateCredentials, Logout, Provider, …). For the synchronous callers
+// (UpdateCredentials, authenticate, logout) a.m is still held at call time —
+// that is pre-existing behavior and not changed here.
+func (a *AuthenticationServiceImpl) runPostMutationEffects(newToken string, sendNotification bool, updateApiUrl bool) {
 	a.postCredentialUpdateHookMu.RLock()
 	postCredentialUpdateHook := a.postCredentialUpdateHook
 	a.postCredentialUpdateHookMu.RUnlock()
@@ -570,6 +665,7 @@ func (a *AuthenticationServiceImpl) updateCredentials(newToken string, sendNotif
 	}
 
 	if sendNotification {
+		conf := a.engine.GetConfiguration()
 		apiUrl := ""
 		if updateApiUrl {
 			apiUrl = a.configResolver.GetString(types.SettingApiEndpoint, nil)