Skip to content

fix(sso): bound and synchronize the Entra ID state map held in the session - #3222

Open
marevol wants to merge 1 commit into
masterfrom
entraid-state-map-bounds
Open

fix(sso): bound and synchronize the Entra ID state map held in the session#3222
marevol wants to merge 1 commit into
masterfrom
entraid-state-map-bounds

Conversation

@marevol

@marevol marevol commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Problem

storeStateInSession() kept pending authorization attempts in a plain HashMap under the
session's entraidStates attribute:

Map<String, StateData> stateMap = (Map<String, StateData>) session.getAttribute(STATES);
if (stateMap == null) {
    stateMap = new HashMap<>();
    session.setAttribute(STATES, stateMap);
}
...
stateMap.put(state, stateData);

Unbounded growth. Every redirect to the authorization endpoint adds one entry, 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 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 null attribute and create their own
map. The loser's state is then unreachable when its callback returns, so that login fails with
could not validate state. Concurrent put on a shared HashMap has the usual resize hazards on
top.

Fix

  • getStateMap(HttpSession) — creates the map once, synchronized on the session, and returns
    a ConcurrentHashMap. A plain HashMap left behind by a session that predates this change is
    migrated in place, so an in-flight login survives the upgrade.
  • removeExpiredStates(Map) — the TTL pruning, now shared by the write path and the read
    path. A session that never completes a login gets pruned on every new attempt.
  • removeOldestStates(Map, int) — caps the map at maxStates (default 10, settable via
    LastaDi 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, a
live state is returned and consumed exactly once.

Tests

EntraIdAuthenticator's unit test class, 6 new tests, using a SystemHelper with a controllable
clock so expiry is deterministic:

  • test_storeStateInSession_dropsExpiredStatesOnWrite
  • test_storeStateInSession_capsTheNumberOfLiveStates — 20 writes with maxStates=3 leave the 3
    newest
  • test_getStateMap_migratesALegacyHashMap — the migrated map is the one stored back on the
    session, so later writes are not lost
  • test_getStateMap_isCreatedOnceUnderConcurrentAccess — 8 threads must all get the same instance
  • test_removeStateFromSession_stillDropsExpiredStates
  • test_removeStateFromSession_returnsAndConsumesALiveState

Falsification: with the two pruning calls removed from storeStateInSession, the first two tests
fail.

Tests run: 137, Failures: 0, Errors: 0, Skipped: 0

(the whole org.codelibs.fess.sso package)

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant