fix(token-providers): fromSso 30s refresh throttle leaks across unrelated SSO sessions - #8267
Open
Adityaj0 wants to merge 1 commit into
Open
fix(token-providers): fromSso 30s refresh throttle leaks across unrelated SSO sessions#8267Adityaj0 wants to merge 1 commit into
Adityaj0 wants to merge 1 commit into
Conversation
…ated SSO sessions lastRefreshAttemptTime was a single Date at module scope, shared by every fromSso() provider instance in the process regardless of which profile or sso_session it targets. Refreshing one profile's expired token set this global timestamp, so refreshing a completely unrelated, independently expired profile/session within the next 30 seconds was silently skipped -- the code fell through to validateTokenExpiry() on the still-stale token and threw "Token is expired", even though that session had never actually been rate-limited and no refresh attempt had been made for it. Key the throttle by sso_session name instead of using one shared timestamp, so it still protects a single session from being hammered with refresh calls but no longer blocks unrelated sessions from refreshing in the same window.
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.
Motivation and Context
fixes #8266
fromSso's 30-second "don't hammer the OIDC refresh endpoint" throttle uses a singleDatedeclared at module scope:This timestamp is shared by every
fromSsoprovider in the process, regardless of profile/sso_session. Refreshing profile A's expired token sets it; if profile B — an unrelated profile on a differentsso_session, also independently expired — is resolved within the next 30 seconds, its refresh is silently skipped and it falls through tovalidateTokenExpiry()on its own still-stale token, throwingToken is expiredeven though B was never actually rate-limited.Description
Replaced the single shared
Datewith aMap<string, number>keyed byssoSessionName:This preserves the original intent (don't hammer refresh calls for the same session within 30 seconds — including across multiple
fromSso()instances pointed at the same session, which is correct and desirable) while no longer blocking unrelated sessions from refreshing in the same window.Testing
Added
does not skip refresh for a different profile/sso_session within the same 30 secondstofromSso.spec.ts, exercising two profiles (differentsso_sessions, both independently expired) against a single importedfromSsomodule (novi.resetModules()between the two calls, matching real single-process usage). Confirmed it fails against the pre-fix code (git stashthe fix, rerun — the second profile incorrectly returns its stale unrefreshed token instead of a real refresh) and passes with the fix.Also independently verified against the real, byte-for-byte source (only import specifiers redirected to stub dependency files) with a standalone driver simulating two profiles/sessions:
And confirmed the original same-session throttle behavior is unchanged (a second call for the same session within 30s still returns the existing token / throws if it's expired, without triggering a duplicate refresh call).
Types of changes
Checklist