fix(sso): bound and synchronize the Entra ID state map held in the session - #3222
Open
marevol wants to merge 1 commit into
Open
fix(sso): bound and synchronize the Entra ID state map held in the session#3222marevol wants to merge 1 commit into
marevol wants to merge 1 commit into
Conversation
…ssion Every redirect to the authorization endpoint stored one entry in a plain HashMap under the session's entraidStates attribute, and expired entries were cleared only inside removeStateFromSession -- that is, only when a callback actually arrived. A client that keeps starting logins without finishing one grew that map without bound. The map was also created and mutated without synchronization. Two concurrent login attempts in the same session could each see a null attribute and create their own map; the loser's state was then unreachable when its callback came back, so that login failed with "could not validate state". - getStateMap() creates the map once, synchronized on the session, and returns a ConcurrentHashMap. A plain HashMap left by a session that predates this change is migrated in place. - removeExpiredStates() is now shared by the write and the read path, so a session that never completes a login still gets pruned. - removeOldestStates() caps the map at maxStates (default 10, settable), keeping the most recent attempts. removeStateFromSession() keeps its behaviour: expired states are dropped, a live state is returned and consumed exactly once.
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.
Problem
storeStateInSession()kept pending authorization attempts in a plainHashMapunder thesession's
entraidStatesattribute:Unbounded growth. Every redirect to the authorization endpoint adds one entry, and expired
entries were cleared only inside
removeStateFromSession()— that is, only when a callbackactually arrived. A client that keeps starting logins without finishing one grows the map without
bound for the whole session lifetime, and nothing caps it.
Not synchronized. Two concurrent login attempts in the same session (two tabs, or a page that
triggers the SSO endpoint more than once) can each read a
nullattribute and create their ownmap. The loser's state is then unreachable when its callback returns, so that login fails with
could not validate state. Concurrentputon a sharedHashMaphas the usual resize hazards ontop.
Fix
getStateMap(HttpSession)— creates the map once, synchronized on the session, and returnsa
ConcurrentHashMap. A plainHashMapleft behind by a session that predates this change ismigrated in place, so an in-flight login survives the upgrade.
removeExpiredStates(Map)— the TTL pruning, now shared by the write path and the readpath. A session that never completes a login gets pruned on every new attempt.
removeOldestStates(Map, int)— caps the map atmaxStates(default 10, settable viaLastaDi like the other knobs on this component), keeping the most recent attempts, which are the
ones a user can still complete.
removeStateFromSession()keeps its behaviour and gets shorter: expired states are dropped, alive state is returned and consumed exactly once.
Tests
EntraIdAuthenticator's unit test class, 6 new tests, using aSystemHelperwith a controllableclock so expiry is deterministic:
test_storeStateInSession_dropsExpiredStatesOnWritetest_storeStateInSession_capsTheNumberOfLiveStates— 20 writes withmaxStates=3leave the 3newest
test_getStateMap_migratesALegacyHashMap— the migrated map is the one stored back on thesession, so later writes are not lost
test_getStateMap_isCreatedOnceUnderConcurrentAccess— 8 threads must all get the same instancetest_removeStateFromSession_stillDropsExpiredStatestest_removeStateFromSession_returnsAndConsumesALiveStateFalsification: with the two pruning calls removed from
storeStateInSession, the first two testsfail.
(the whole
org.codelibs.fess.ssopackage)