Skip to content

fix: let signInSilently() complete instead of always timing out - #578

Open
Dumindu-Kanchana wants to merge 2 commits into
asgardeo:mainfrom
Dumindu-Kanchana:fix/silent-sign-in-state
Open

Dumindu-Kanchana wants to merge 2 commits into
asgardeo:mainfrom
Dumindu-Kanchana:fix/silent-sign-in-state

Conversation

@Dumindu-Kanchana

@Dumindu-Kanchana Dumindu-Kanchana commented Sep 30, 2026 •

Copy link
Copy Markdown

Purpose

signInSilently() could never resolve with a user, so every silent renewal ended at its 10 s timeout with false. Two causes, one per commit:

  1. @asgardeo/javascript: getAuthorizeRequestUrlParams replaced the caller's state with instance_<id> whenever an instance ID was set, and getSignInUrl always sets one. The sign-in-silently marker 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 matches hasCalledForThisInstanceInUrl and extractPkceStorageKeyFromState, and a request without a caller state is unchanged (instance_0_request_N).
  2. @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, so AsgardeoProvider saw an active session and returned before reaching signIn({callOnlyOnRedirect: true}). The provider now checks for a silent sign-in callback for its own instance first and calls signInSilently(), which posts the code to the parent through receivePromptNoneResponse.

Verification: a React SPA on @asgardeo/react 0.25.14 with these builds swapped in, Chrome 154, and a 401 forced on one API call to trigger signInSilently(), traced with the Chrome DevTools Protocol:

Before After
Authorize state instance_0_request_1 instance_0_sign-in-silently_request_4
Iframe → parent message none check_session_signed_in with the code, ~1.4 s after the redirect
Token exchange none POST /oauth2/token → 200
signInSilently() false after ~10 s resolves; the retried API call returned 200 ~4.5 s after the 401

Unit tests: javascript 464/464 (2 new), browser 4/4, react 29/29. AsgardeoProvider has 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

  • N/A

Checklist

  • Followed the CONTRIBUTING guidelines.
  • Manual test round performed and verified.
  • Documentation provided. (Add links if there are any)
  • Unit tests provided. (Add links if there are any)
    • packages/javascript/src/utils/__tests__/getAuthorizeRequestUrlParams.test.ts

Security checks

🤖 Generated with Claude Code

Dumindu-Kanchana and others added 2 commits September 30, 2026 14:02
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>
@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: a00533ca-e272-43fb-937f-f2b2fe073196

📥 Commits

Reviewing files that changed from the base of the PR and between 3487684 and b8d96a9.

📒 Files selected for processing (4)
  • .changeset/silent-sign-in-state.md
  • packages/javascript/src/utils/__tests__/getAuthorizeRequestUrlParams.test.ts
  • packages/javascript/src/utils/getAuthorizeRequestUrlParams.ts
  • packages/react/src/contexts/Asgardeo/AsgardeoProvider.tsx

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Silent sign-in

Layer / File(s) Summary
Preserve caller state in authorize requests
packages/javascript/src/utils/getAuthorizeRequestUrlParams.ts, packages/javascript/src/utils/__tests__/getAuthorizeRequestUrlParams.test.ts, .changeset/silent-sign-in-state.md
Authorize state now combines the instance prefix with non-empty caller state. Tests cover instance-only and combined state values. The changeset records patch releases for both packages and describes the silent-sign-in changes.
Handle the silent-sign-in iframe before session resumption
packages/react/src/contexts/Asgardeo/AsgardeoProvider.tsx
Before the existing signed-in check, the provider checks whether silent sign-in is initialized and the current URL belongs to its instance. If both conditions hold, it awaits asgardeo.signInSilently() and returns.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to b8d96

The silent sign-in flow is ready to merge based on the verified callback handling and state propagation changes.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to b8d96

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

  • Medium · reliability · inferred: The restored callback path can update stored authentication after its initiating renewal has timed out, or race another attempt. The unchanged listener survives the 10-second timeout and accepts signed-in messages without an expected-state or in-flight check; successful exchange persists credentials even though the original promise may already have resolved false. The PR makes genuine iframe responses reach this incomplete ownership lifecycle. This is a session-state failure-containment concern, not a verified account-takeover finding.
Security review details

Security Blast Radius

  • inferred — The demonstrated outcome is mutation of the receiving SDK client's session and credentials. Multiple outstanding listeners share the page's message stream without request-state filtering, so ownership is not isolated between attempts at the receiver. Acceptance of a code still depends on the receiving client's token configuration and applicable PKCE enforcement; wider tenant or service compromise is not established.

Security Findings and Attack Paths

  • inferred — A genuine callback arriving after the timeout can reach the retained listener, start token exchange, and persist a successful response despite the caller having received false. Overlapping or duplicate messages can also start exchanges before listener removal. These paths affect authentication-state failure containment without requiring a forged authorization code.

Trust Boundaries and Controls

  • observed — The iframe specifies the parent's origin for outbound messages, but the receiving listener checks neither MessageEvent.origin nor MessageEvent.source. This receiver condition predates the PR, and parent-side renewal already installed that listener in base. It is therefore an existing trust-boundary weakness, not independently demonstrated as a newly introduced cross-origin attack path.

Resilience and Maintainability Implications

  • observed — Signed-out responses and settled token exchanges remove their listener; timeout only resolves false. Iframe flag reset and navigation clean up the producer but do not terminate the parent's stale listener or prevent concurrent exchanges already started.

Hardening Proposals

  • proposed — Bind each callback to its expected request state and live operation, suppress duplicate exchange initiation, and terminate listener ownership on every terminal path. Define whether an exchange already in flight at timeout may commit credentials, rather than allowing promise settlement and session mutation to diverge.
  • proposed — Validate inbound message origin and the expected iframe source before processing either signed-in or signed-out responses. This addresses the pre-existing receiver weakness separately from the PR's callback restoration.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed PR #578 addresses the active coding requirements in [#577]. getAuthorizeRequestUrlParams now combines instance_<id> with the caller state, and the added test verifies `instance_0_sign-in-silently_…
Out of Scope Changes check ✅ Passed The reviewed changes stay within [#577]. The state changes and regression tests support silent-sign-in and check-session state handling. The React provider change supports the issue's iframe callback …
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3…
Title check ✅ Passed The title clearly and concisely describes the primary fix: allowing signInSilently() to complete instead of timing out.
Description check ✅ Passed The description follows the repository template and provides detailed purpose, issue linkage, implementation context, verification results, tests, and security confirmation. Documentation and secure-c…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
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.

signInSilently() always resolves false: getAuthorizeRequestUrlParams overwrites the "sign-in-silently" state with instance_<id>

1 participant