Skip to content

fix: post auth hook locking#1214

Open
ShawkyZ wants to merge 6 commits intorefactor/IDE-1786_folder-config-refactoringfrom
fix/postAuthhook
Open

fix: post auth hook locking#1214
ShawkyZ wants to merge 6 commits intorefactor/IDE-1786_folder-config-refactoringfrom
fix/postAuthhook

Conversation

@ShawkyZ
Copy link
Copy Markdown
Collaborator

@ShawkyZ ShawkyZ commented Apr 15, 2026

Description

Fix: move postCredentialUpdateHook execution outside the auth lock

The postCredentialUpdateHook (used to populate feature flags after login) was running inside updateCredentials, which is called from authenticate() while a.m.Lock() is held. The hook makes HTTP calls to fetch feature flags and SAST settings for every workspace folder. This means:

  • The auth service's RWMutex write lock was held during multiple network round-trips
  • All concurrent IsAuthenticated() callers (which need RLock) were blocked
  • Any code in the hook's call chain that transitively called back into the auth service would deadlock

Fix: Remove the hook call from updateCredentials and move it to Authenticate(), where it runs after the lock is released. The hook is still injectable from outside via SetPostCredentialUpdateHook — the only change is when it executes relative to the lock.

Before:

Authenticate() {
    a.m.Lock()
    └─ authenticate()
       └─ updateCredentials()
          ├─ store credentials
          ├─ postCredentialUpdateHook()  ← network I/O under lock
          └─ send notification
    a.m.Unlock()
}

After:

Authenticate() {
    a.m.Lock()
    └─ authenticate()
       └─ updateCredentials()
          ├─ store credentials
          └─ send notification
    a.m.Unlock()
    postCredentialUpdateHook()  ← network I/O, NO lock held
}

The test is updated to verify the hook runs outside the lock by calling IsAuthenticated() from within the hook — which would deadlock under the old code.

Checklist

  • Tests added and all succeed
  • Regenerated mocks, etc. (make generate)
  • Linted (make lint-fix)
  • README.md updated, if user-facing
  • License file updated, if new 3rd-party dependency is introduced

@snyk-io
Copy link
Copy Markdown

snyk-io bot commented Apr 15, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@ShawkyZ ShawkyZ marked this pull request as ready for review April 15, 2026 13:50
@ShawkyZ ShawkyZ requested review from a team as code owners April 15, 2026 13:50
@ShawkyZ ShawkyZ changed the title Fix/post authhook fix: post auth hook locking Apr 15, 2026
a.notifDedup.Unlock()
}

if a.postCredentialUpdateHook != nil && newToken != "" {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you remove it?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was added by a previous PR and wasn't necessary since we have the recovery on a global level.

a.previousAuthCtxCancelFunc()
a.m.Unlock()

if a.postCredentialUpdateHook != nil && token != "" && err == nil {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this now extracted?

Copy link
Copy Markdown
Collaborator Author

@ShawkyZ ShawkyZ Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't want the post hook to be inside the mutex lock, since post hook will trigger a bunch of network calls for fetching FFs. This will affect other calls like IsAuthenticated

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot
Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Deadlock Risk 🔴 [critical]

The Authenticate function replaces defer a.m.Unlock() with a manual a.m.Unlock() call after a.authenticate(ctx). If a.authenticate(ctx) or any of its internal logic (such as configureProviders or sendAuthenticationAnalytics) panics, the mutex a.m will remain locked indefinitely, deadlocking all subsequent calls to IsAuthenticated, Provider, and Logout.

a.m.Lock()

a.previousAuthCtxCancelFuncMu.Lock()
ctx, a.previousAuthCtxCancelFunc = context.WithCancel(ctx)
a.previousAuthCtxCancelFuncMu.Unlock()

token, err = a.authenticate(ctx)
hook := a.postCredentialUpdateHook
a.previousAuthCtxCancelFunc()
a.m.Unlock()
Race Condition 🟠 [major]

Moving the postCredentialUpdateHook execution to the end of Authenticate ensures it runs after the lock is released, but also after updateCredentials has already sent the $/snyk.hasAuthenticated notification to the IDE. As documented in domain/ide/command/login.go, this hook must run before the notification to ensure feature flags are populated before the IDE triggers an automatic scan. This change re-introduces the race condition where scans fail with 'Snyk Code is not enabled'.

if hook != nil && token != "" && err == nil {
	hook()
}
📚 Repository Context Analyzed

This review considered 12 relevant code sections from 10 files (average relevance: 0.92)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants