fix: scope ClearNonCurrentSessions to current user only#21331
Open
harshinsecurity wants to merge 1 commit intosmartcontractkit:developfrom
Open
fix: scope ClearNonCurrentSessions to current user only#21331harshinsecurity wants to merge 1 commit intosmartcontractkit:developfrom
harshinsecurity wants to merge 1 commit intosmartcontractkit:developfrom
Conversation
ClearNonCurrentSessions previously deleted every session in the table except the caller's current session ID, regardless of which user owned them. This meant that when any user changed their password, all other users were logged out. Fix by adding a subquery that resolves the owning user's email from the current session ID and constraining the DELETE to only that user's sessions. This preserves the existing interface signature. Affected backends: localauth (sessions), ldapauth (ldap_sessions), oidcauth (oidc_sessions). Reported-by: hi@harshinsecurity.in
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: scope
ClearNonCurrentSessionsto current user onlyProblem
When any user changes their password (via
PATCH /v2/user/passwordor theupdateUserPasswordGraphQL mutation),ClearNonCurrentSessionsis called to invalidate the user's other sessions. However, the SQL query deletes every session in the table except the caller's current one — including sessions belonging to completely different users.Affected code (all three authentication backends)
core/sessions/localauth/orm.gocore/sessions/ldapauth/ldap.gocore/sessions/oidcauth/oidc.goIn all three cases the query filters only on session ID, with no
WHEREclause constraining the deletion to the current user's email. The result is that when User A changes their password, User B, User C, etc. are all logged out.Call sites
This function is invoked from two places:
UserController.updateUserPasswordincore/web/user_controller.go(line 348)Resolver.UpdateUserPasswordincore/web/resolver/mutation.go(line 961)Both pass only the current session ID, and neither has any additional user-scoping logic.
Impact
NewPassword ≠ OldPassword, so an attacker can repeatedly set the same password, wiping all other sessions each time.What is NOT affected
sessionstable and is unaffected. Users authenticating via API tokens (X-API-Key/X-API-Secretheaders) will continue to work normally.Practical severity
Low-to-Medium. Most production Chainlink nodes are single-user, and programmatic access uses API tokens (unaffected). However, multi-user deployments do exist, and the behavior is clearly unintended — the function's own doc comment says "removes all sessions but the id passed in," which implies intent to clear only the caller's own sessions.
Fix
Adds an
AND email = (SELECT email FROM <table> WHERE id = $1)subquery to all three implementations. This scopes the deletion to sessions owned by the same user as the current session, without changing theClearNonCurrentSessionsinterface signature.After fix
core/sessions/localauth/orm.gocore/sessions/ldapauth/ldap.gocore/sessions/oidcauth/oidc.goWhy a subquery instead of adding an email parameter
The
ClearNonCurrentSessionsmethod is defined in theAuthenticationProviderinterface (core/sessions/authentication.go), and is called from both the REST controller and the GraphQL resolver. Changing the function signature would require updating the interface, all three implementations, both call sites, and the mock. The subquery approach achieves the same result with zero interface changes — the session ID already contains enough information to resolve the owning user.Files changed
core/sessions/localauth/orm.goDELETEto current user's email via subquerycore/sessions/ldapauth/ldap.goDELETEto current user'suser_emailvia subquerycore/sessions/oidcauth/oidc.goDELETEto current user'suser_emailvia subqueryHow to verify
PATCH /v2/user/passwordContact
Reported by: hi@harshinsecurity.in