Add opt-in sharedScope for multi-instance MFE support#170
Add opt-in sharedScope for multi-instance MFE support#170aziobakas wants to merge 1 commit intokeycloakify:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an optional window-scoped shared global context for OIDC state and a new exported function Changes
Sequence DiagramsequenceDiagram
participant Bundle as Bundle
participant Window as Window Object
participant Shared as Shared State
participant OIDC as OIDC Factory
Bundle->>Window: earlyInit(sharedScope: "window")
alt first bundle enables shared scope
Window->>Window: create SHARED_MEMO_KEY container & shared state
Window-->>Bundle: returns shared state
Bundle->>Window: log warning about shared global context
else subsequent bundle
Window-->>Bundle: return existing shared state & shouldLoadApp
end
Bundle->>OIDC: createOidc() (calls getOidcMemoMap)
OIDC->>Window: enableSharedGlobalContext() (store memo map on window)
OIDC-->>Bundle: register exports (evtIframeAuthResponse, redirectAuthResponse, clear)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/earlyInit.ts (1)
79-85:listenerRegisteredfield is set but never checked.The
SharedStateincludeslistenerRegistered: booleanwhich is always set totrue(lines 262, 276), but it's never read or checked. This field could be useful for:
- Preventing duplicate message listener registration
- Coordinating between bundles to determine which one "owns" the listener
Consider either removing it if unused, or utilizing it to address the race condition mentioned above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/earlyInit.ts` around lines 79 - 85, SharedState currently declares listenerRegistered but never reads it; either remove the field or use it to guard listener setup: in the code paths that register the iframe/message listener (the places that currently set listenerRegistered = true), first check SharedState.listenerRegistered and skip registration if already true, then set it to true immediately after successful registration and set it back to false when the listener is removed (or when clearRedirectAuthResponse runs). Use the SharedState.listenerRegistered flag to prevent duplicate registration of the message/event listener (and to coordinate ownership between bundles) by checking it before calling addEventListener / Evt.subscribe and updating it when you install/uninstall the handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/earlyInit.ts`:
- Around line 127-130: The async then-callbacks on prModuleCreateOidc can
register different exports first, causing Bundle 2's registerExports_earlyInit
to point at a local evtIframeAuthResponse while Bundle 1 posts to its own
listener; make registration idempotent or ensure both bundles use the shared Evt
instance: modify registerExports_earlyInit (and/or the enableSharedGlobalContext
flow) to check a shared registry flag or existing.evtIframeAuthResponse and
skip/return early if exports are already registered, or eagerly populate
existing.evtIframeAuthResponse in shared state before either
prModuleCreateOidc.then runs so both registrations reference the same Evt;
target symbols: prModuleCreateOidc.then, registerExports_earlyInit,
enableSharedGlobalContext, exports_earlyInit, existing.evtIframeAuthResponse,
and the message listener that posts iframe auth responses.
- Around line 253-264: The shared state captures the current (undefined) values
of evtIframeAuthResponse and redirectAuthResponse, causing stale references when
the local variables are later lazily initialized; to fix, eagerly create
evtIframeAuthResponse (call createEvt() and assign to the local variable) before
calling setSharedState so the same Evt instance is stored, and change
clearRedirectAuthResponse to call setSharedState with redirectAuthResponse:
undefined (and also update the local variable) instead of only mutating the
local variable; update the block around sharedScope === "window" and the message
listener logic that uses evtIframeAuthResponse, ensuring setSharedState always
stores the live references for evtIframeAuthResponse and redirectAuthResponse.
---
Nitpick comments:
In `@src/core/earlyInit.ts`:
- Around line 79-85: SharedState currently declares listenerRegistered but never
reads it; either remove the field or use it to guard listener setup: in the code
paths that register the iframe/message listener (the places that currently set
listenerRegistered = true), first check SharedState.listenerRegistered and skip
registration if already true, then set it to true immediately after successful
registration and set it back to false when the listener is removed (or when
clearRedirectAuthResponse runs). Use the SharedState.listenerRegistered flag to
prevent duplicate registration of the message/event listener (and to coordinate
ownership between bundles) by checking it before calling addEventListener /
Evt.subscribe and updating it when you install/uninstall the handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 901243d3-42b5-4043-bf71-686e6915a6ab
📒 Files selected for processing (2)
src/core/createOidc.tssrc/core/earlyInit.ts
aziobakas
force-pushed
the
feat/shared-scope-multi-instance
branch
from
09203dc to
0ec4886
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/core/earlyInit.ts (2)
264-273:⚠️ Potential issue | 🔴 CriticalCritical:
clearRedirectAuthResponsedoesn't update shared state, causing stale data for subsequent bundles.The
clearRedirectAuthResponsecallback (lines 269-271) only clears the localredirectAuthResponsevariable. It does not updateexisting.redirectAuthResponsein the shared state object.If Bundle 2 consumes and clears the redirect auth response,
existing.redirectAuthResponseretains the stale value. Any subsequent bundle (Bundle 3+) will incorrectly see the already-consumed auth response.🐛 Proposed fix: Update shared state when clearing
if (sharedScope === "window") { + const sharedState: SharedEarlyInitState = { + shouldLoadApp: true, + evtIframeAuthResponse, + redirectAuthResponse, + clearRedirectAuthResponse: () => { + redirectAuthResponse = undefined; + sharedState.redirectAuthResponse = undefined; + } + }; - setSharedState({ - shouldLoadApp: true, - evtIframeAuthResponse, - redirectAuthResponse, - clearRedirectAuthResponse: () => { - redirectAuthResponse = undefined; - } - }); + setSharedState(sharedState); }This ensures the shared state object's
redirectAuthResponseproperty is also cleared.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/earlyInit.ts` around lines 264 - 273, The clearRedirectAuthResponse callback only clears the local redirectAuthResponse variable; update it so it also clears the shared state's redirectAuthResponse via setSharedState. Modify the clearRedirectAuthResponse implementation (the function passed into setSharedState) to both set redirectAuthResponse = undefined and call setSharedState to update the shared object (clearing existing.redirectAuthResponse) so subsequent bundles don't see stale data.
120-136:⚠️ Potential issue | 🔴 CriticalCritical: Bundle 2's Evt instance can diverge from Bundle 1's message listener.
When Bundle 2 calls
getEvtIframeAuthResponse()(line 124), it creates an Evt inexisting.evtIframeAuthResponse. However, Bundle 1's message listener at line 224 posts to its localevtIframeAuthResponsevariable, notexisting.evtIframeAuthResponse.Since
evtIframeAuthResponseisundefinedwhensetSharedStateis called (line 267), and the listener lazily creates a separate Evt, iframe auth messages will be posted to a different Evt than the one Bundle 2 subscribes to.🐛 Proposed fix: Eagerly create Evt and ensure listener uses shared reference
In
oidcEarlyInit_nonMemoized, eagerly create the Evt and ensure it's the same instance used by the listener and stored in shared state:if (shouldLoadApp) { - let evtIframeAuthResponse: Evt<AuthResponse> | undefined = undefined; + const evtIframeAuthResponse = createEvt<AuthResponse>(); { // ... message listener setup ... window.addEventListener( "message", event => { // ... validation ... - (evtIframeAuthResponse ??= createEvt()).post(authResponse); + evtIframeAuthResponse.post(authResponse); }, // ... ); } exports_earlyInit = { shouldLoadApp: true, getEvtIframeAuthResponse: () => { - return (evtIframeAuthResponse ??= createEvt()); + return evtIframeAuthResponse; }, // ... };This ensures the same Evt instance is stored in shared state and used by the listener.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/earlyInit.ts` around lines 120 - 136, The Evt for iframe auth is created lazily causing Bundle 1's message listener to post to a different Evt than Bundle 2 subscribes to; to fix, eagerly create a single Evt instance in oidcEarlyInit_nonMemoized (call createEvt once and assign it to existing.evtIframeAuthResponse), use that same local variable for the message listener (instead of creating a new Evt there), and have getEvtIframeAuthResponse simply return that shared instance; also ensure setSharedState stores this same existing.evtIframeAuthResponse so both bundles reference the identical Evt.
🧹 Nitpick comments (2)
src/core/earlyInit.ts (2)
77-79: Consider extracting shared constants to a common module.
SHARED_MEMO_KEYis duplicated here and increateOidc.ts(as noted in the comment). Duplicating magic strings risks silent divergence if one is updated without the other.💡 Suggestion: Extract to shared constants
Create a shared constants file (e.g.,
src/core/sharedScopeConstants.ts):export const SHARED_MEMO_KEY = "__oidc_spa_shared_prOidcByConfigId__"; export const SHARED_EARLY_INIT_KEY = "__oidc_spa_shared_earlyInit__";Then import from both
earlyInit.tsandcreateOidc.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/earlyInit.ts` around lines 77 - 79, Extract the duplicated magic strings into a single shared constant module and import them where needed: create a new file exporting SHARED_MEMO_KEY and SHARED_EARLY_INIT_KEY, then replace the local declarations in earlyInit.ts and the ones in createOidc.ts with imports from that shared module; ensure the exported names exactly match the existing symbols (SHARED_MEMO_KEY, SHARED_EARLY_INIT_KEY) so all references (e.g., usages inside functions in earlyInit.ts and createOidc.ts) continue to work without other changes.
134-134: Consider storingsessionRestorationMethodin shared state for consistency.Bundle 2 uses its own
params?.sessionRestorationMethod, but if Bundle 1 already created the shared OIDC instance, Bundle 2's preference will be ignored. This could lead to confusion when developers specify different values across bundles.Consider either:
- Storing
sessionRestorationMethodinSharedEarlyInitStateand reusing Bundle 1's value, or- Documenting that only the first bundle's
sessionRestorationMethodtakes effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/earlyInit.ts` at line 134, The code currently reads sessionRestorationMethod from params when initializing an OIDC instance, which means later bundles' params are ignored if SharedEarlyInitState already contains an OIDC; update SharedEarlyInitState to include a sessionRestorationMethod field and, in the OIDC initialization path (where sessionRestorationMethod: params?.sessionRestorationMethod is passed), read and persist the value into SharedEarlyInitState when the OIDC is first created so subsequent bundles reuse that stored sessionRestorationMethod; alternatively (if you prefer documentation only) add clear docs to state that the first bundle's params.sessionRestorationMethod wins and don't change runtime behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/earlyInit.ts`:
- Around line 264-273: The clearRedirectAuthResponse callback only clears the
local redirectAuthResponse variable; update it so it also clears the shared
state's redirectAuthResponse via setSharedState. Modify the
clearRedirectAuthResponse implementation (the function passed into
setSharedState) to both set redirectAuthResponse = undefined and call
setSharedState to update the shared object (clearing
existing.redirectAuthResponse) so subsequent bundles don't see stale data.
- Around line 120-136: The Evt for iframe auth is created lazily causing Bundle
1's message listener to post to a different Evt than Bundle 2 subscribes to; to
fix, eagerly create a single Evt instance in oidcEarlyInit_nonMemoized (call
createEvt once and assign it to existing.evtIframeAuthResponse), use that same
local variable for the message listener (instead of creating a new Evt there),
and have getEvtIframeAuthResponse simply return that shared instance; also
ensure setSharedState stores this same existing.evtIframeAuthResponse so both
bundles reference the identical Evt.
---
Nitpick comments:
In `@src/core/earlyInit.ts`:
- Around line 77-79: Extract the duplicated magic strings into a single shared
constant module and import them where needed: create a new file exporting
SHARED_MEMO_KEY and SHARED_EARLY_INIT_KEY, then replace the local declarations
in earlyInit.ts and the ones in createOidc.ts with imports from that shared
module; ensure the exported names exactly match the existing symbols
(SHARED_MEMO_KEY, SHARED_EARLY_INIT_KEY) so all references (e.g., usages inside
functions in earlyInit.ts and createOidc.ts) continue to work without other
changes.
- Line 134: The code currently reads sessionRestorationMethod from params when
initializing an OIDC instance, which means later bundles' params are ignored if
SharedEarlyInitState already contains an OIDC; update SharedEarlyInitState to
include a sessionRestorationMethod field and, in the OIDC initialization path
(where sessionRestorationMethod: params?.sessionRestorationMethod is passed),
read and persist the value into SharedEarlyInitState when the OIDC is first
created so subsequent bundles reuse that stored sessionRestorationMethod;
alternatively (if you prefer documentation only) add clear docs to state that
the first bundle's params.sessionRestorationMethod wins and don't change runtime
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e6123a9-9aa1-4f1a-a5dd-48ab51032d14
📒 Files selected for processing (2)
src/core/createOidc.tssrc/core/earlyInit.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/createOidc.ts
aziobakas
force-pushed
the
feat/shared-scope-multi-instance
branch
from
0ec4886 to
c229b98
Compare
When sharedScope: 'window' is passed to oidcEarlyInit, the OIDC instance memoization map and earlyInit state are stored on window instead of module scope. This allows multiple bundles of oidc-spa (from different micro-frontend remotes) to share a single OIDC instance for the same issuerUri + clientId combination. The shared memo map is created synchronously in oidcEarlyInit to avoid race conditions with createOidc. The first bundle to call oidcEarlyInit with sharedScope proceeds normally and stores its state on window. Subsequent bundles detect the existing state and reuse it, skipping duplicate listener registration and callback processing. Default behavior is unchanged (module-scoped, no window globals). Closes keycloakify#169
aziobakas
force-pushed
the
feat/shared-scope-multi-instance
branch
from
c229b98 to
47547d9
Compare
|
Thank you for your contribution @aziobakas, However, I won’t be merging this PR. The initialization sequence is both highly intricate and security-critical. We need to ensure that it works reliably in the general case and does not introduce any potential attack vectors. For example, if multiple copies of Additionally, anything that is global must be attached to the We also need to ensure that an attacker cannot hijack the initialization sequence to force micro-frontend-like behavior in a non-micro-frontend setup. For these reasons, this PR is superseded by: I have released a candidate version. Could you test it? Best, |
2 participants
Summary
Adds an opt-in
sharedScope: "window"parameter tooidcEarlyInitthat stores the OIDC instance memoization map and earlyInit state onwindowinstead of module scope. This allows multiple bundles of oidc-spa (from different micro-frontend remotes) to share a single OIDC instance for the sameissuerUri+clientId.Related issue: #169
Changes
earlyInit.tssharedScope: "window"option onParamsOfEarlyInitwindowsynchronously (avoids race conditions withcreateOidc)windowcreateOidc.tsgetOidcMemoMap()function checkswindowfirst, falls back to module-scoped mapprOidcByConfigIdreads go through this functiondExports_earlyInit,dExports_tokenSubstitution, etc.) stays module-scoped since each bundle needs its ownHow it works
oidcEarlyInit({ sharedScope: "window" })for each remotewindowand proceeds normallywindow, reuses it, skips duplicate workcreateOidc,getOidcMemoMap()returns the shared mapcreateOidcfinds the first remote's instance (cache hit)Security
Usage
Summary by CodeRabbit