fix: let signInSilently() complete instead of always timing out - #578
Dumindu-Kanchana wants to merge 2 commits into
Conversation
Silent sign-in and check-session mark their requests through the state, and replacing it with instance_<id> dropped that marker. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The provider in the hidden iframe saw the shared session and returned before the response reached the parent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes preserve caller-provided state when an instance ID is added to authorize requests. The React provider also handles an initialized silent-sign-in iframe for its instance before continuing session resumption. ChangesSilent sign-in
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The silent sign-in flow is ready to merge based on the verified callback handling and state propagation changes. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The normal silent-renewal flow is restored, but delayed or overlapping responses can still update authentication state after the initiating attempt has failed. Existing instance checks and PKCE constrain code acceptance; the review did not establish an account-takeover path. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Purpose
signInSilently()could never resolve with a user, so every silent renewal ended at its 10 s timeout withfalse. Two causes, one per commit:@asgardeo/javascript:getAuthorizeRequestUrlParamsreplaced the caller'sstatewithinstance_<id>whenever an instance ID was set, andgetSignInUrlalways sets one. Thesign-in-silentlymarker was dropped (state=instance_0_request_N), so the iframe never recognised the response as a silent sign-in. The state now keeps both parts, e.g.instance_0_sign-in-silently_request_N. That still matcheshasCalledForThisInstanceInUrlandextractPkceStorageKeyFromState, and a request without a caller state is unchanged (instance_0_request_N).@asgardeo/react: with the marker back, the app loaded inside the hidden iframe still didn't hand the response over when the parent was signed in. The iframe shares the parent's session storage, soAsgardeoProvidersaw an active session and returned before reachingsignIn({callOnlyOnRedirect: true}). The provider now checks for a silent sign-in callback for its own instance first and callssignInSilently(), which posts the code to the parent throughreceivePromptNoneResponse.Verification: a React SPA on
@asgardeo/react0.25.14 with these builds swapped in, Chrome 154, and a 401 forced on one API call to triggersignInSilently(), traced with the Chrome DevTools Protocol:stateinstance_0_request_1instance_0_sign-in-silently_request_4check_session_signed_inwith the code, ~1.4 s after the redirectPOST /oauth2/token→ 200signInSilently()falseafter ~10 sUnit tests:
javascript464/464 (2 new),browser4/4,react29/29.AsgardeoProviderhas no test harness in the package, so the React change is covered by the manual run above.Not changed here: the Vue provider (
packages/vue/src/providers/AsgardeoProvider.ts) has the same signed-in short-circuit ahead of its callback handling and likely needs the same guard. I haven't tested it.Related Issues
Fixes #577
Related PRs
Checklist
packages/javascript/src/utils/__tests__/getAuthorizeRequestUrlParams.test.tsSecurity checks
🤖 Generated with Claude Code