fix(msal-common): use proper 2-arg comparator in getAccountInfoFilteredBy sort#8522
Open
fix(msal-common): use proper 2-arg comparator in getAccountInfoFilteredBy sort#8522
Conversation
…edBy sort The previous sort used a single-argument comparator ((account) => ...), which is incorrect for Array.prototype.sort(). A comparator must take two arguments and return negative, zero, or positive. The 1-arg version never returns 0, breaking stable sort guarantees and causing unpredictable account priority when multiple accounts share the same idTokenClaims presence (e.g. after cross-tenant token acquisition both home and guest profiles have cached ID tokens). Replace with a proper 2-arg comparator that returns 0 when both accounts have (or both lack) idTokenClaims, preserving insertion order.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes incorrect Array.prototype.sort() comparator usage (returning boolean/1-arg comparator) to ensure deterministic/stable ordering when selecting an account in multi-tenant scenarios.
Changes:
- Update
getAccountInfoFilteredByto use a proper 2-argument comparator that returns negative/zero/positive values. - Add a unit test verifying the stable ordering behavior when multiple matching accounts have
idTokenClaims. - Update the associated sample’s account selection sort logic to use a proper 2-argument comparator.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| samples/msal-browser-samples/VanillaJSTestApp2.0/app/customizable-e2e-test/auth.js | Fixes account prioritization sort comparator to be a valid 2-arg comparator. |
| lib/msal-common/test/cache/CacheManager.spec.ts | Adds regression test to ensure stable selection when multiple accounts have idTokenClaims. |
| lib/msal-common/src/cache/CacheManager.ts | Fixes comparator in getAccountInfoFilteredBy to avoid unstable ordering across JS engines. |
| change/@azure-msal-common-b843b148-a865-4af5-adbe-be8d29b8f313.json | Adds required beachball change file (patch) describing the fix. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Re-submission of #8477 (originally authored by @rickygao). CI could not run on the forked PR, so this cherry-picks the same commit from the fork onto a branch in the AzureAD org.
Original PR: #8477
Original commit author: Ruijun Gao (@rickygao)
Summary
Fixes the comparator function passed to Array.prototype.sort() in getAccountInfoFilteredBy. JavaScript's sort() requires a comparator that returns a negative number, zero, or positive number — but the existing code returned a boolean. This caused inconsistent ordering across different JS engines.
Changes