fix(auth): parse expires_in as num in Session.fromJson - #1716
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesNumeric JSON normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR makes a localized fix to safely parse numeric authentication fields across WebAssembly and adds tests; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 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 |
spydon
left a comment
There was a problem hiding this comment.
Lgtm, thanks for you contribution!
I will cherry-pick this to a hotfix branch and release it later this afternoon (since main now is tracking v3)
Parse expires_in using (json['expires_in'] as num?)?.toInt() so that Session.fromJson handles double values crossing JS interop (such as BroadcastChannel cross-tab sync) on Wasm without throwing. Fixes supabase#1687
6881338 to
5d278b9
Compare
## What Hotfix for the `gotrue` 2.27.x line, backporting the Wasm `Session.fromJson` crash fix from #1716. The base is `release/gotrue-2.27.x`, a maintenance branch cut at the `gotrue-v2.27.1` tag. It cannot target `main`, because `main` has since renamed the package to `supabase_auth` (#1697) and renamed the public API (#1712), so a PR against `main` would read as reverting everything merged since the tag. Resolves #1687 for the 2.x line. ## The bug `Session.fromJson` cast `json['expires_in']` straight to `int?`. That map does not always come from `jsonDecode`. `GoTrueClient._mayStartBroadcastChannel` also feeds it payloads that crossed the JavaScript interop boundary through `dartify()` in `broadcast_web.dart`, where every JavaScript number arrives as a `double`. Under `dart2js` this was invisible, because Dart `int` and `double` share a JavaScript `Number` at runtime, so `3600.0 as int?` succeeded. Under `dart2wasm` they are distinct runtime types and the cast throws: ``` TypeError: type 'double' is not a subtype of type 'int?' in type cast ``` The `json.decode(json.encode(dataMap))` round trip in `broadcast_web.dart` does not rescue this: `3600.0` encodes to `"3600.0"` and decodes back to a `double`. Because the throw happened inside the `BroadcastChannel` message listener, outside the setup `try`/`catch`, the rest of the listener was skipped. No `_saveSession` or `_removeSession` ran, and `notifyAllSubscribers` never fired, so receiving tabs silently failed to synchronize login, logout, and token refresh. ## The fix `expires_in` is now parsed as `(json['expires_in'] as num?)?.toInt()`, which accepts `int`, `double`, and `null`. `JwtPayload.fromJson` (`exp`, `nbf`, `iat`) and `OAuthClientListResponse.fromJson` (`nextPage`, `lastPage`, `total`) get the same treatment for their numeric fields. I traced the rest of the reachable surface. `dartify()` is called in exactly one place in the repository, and the only types built from that data are `Session`, `User`, `UserIdentity`, and `Factor`. `User.fromJson` has no numeric fields, its timestamps are ISO 8601 strings, so after this change nothing reachable from the interop boundary casts to `int`. Everything else in the workspace decodes from a string through `dart:convert`, where integer literals stay `int` on every backend. ## Pipeline fixes The tag this branch is frozen at no longer builds against the current toolchain, so the second commit carries three unrelated fixes needed to get a green run. All three were verified to be pre-existing drift rather than fallout from this change, by comparing against #1717, an equivalent change on `main` whose run passed minutes apart. - Flutter stable now ships AGP 9, which rejects the example app's old Gradle DSL. The example's Gradle configuration is ported from `main` (AGP 8.13.1 to 9.1.0, Gradle 8.13 to 9.3.1, Kotlin 2.1.20 to 2.4.0). The example is `publish_to: none`, so nothing published changes. - `dart analyze --fatal-infos` now reports `use_super_parameters` on `SupabaseStorageClient`. `main` resolved this as part of the fetch layer refactor in #1647, which gave `StorageBucketApi` a stored client field. At this tag the superclass stores nothing, so the local field is still needed and the lint is suppressed instead of backporting that refactor. - The compliance workflow validates against `supabase/sdk@main`, whose canonical capability identifiers keep moving, so a branch frozen at an old release can never satisfy them. Its `pull_request` trigger is now scoped to pull requests that target `main`. ## Release notes `melos version` on this branch proposes `gotrue` 2.27.2, plus `supabase` 2.16.1 and `supabase_flutter` 2.17.2 as dependency cascades. Those cascades are required, not incidental: the published `supabase` 2.16.0 pins `gotrue: 2.27.1` exactly and `supabase_flutter` 2.17.1 pins `supabase: 2.16.0` exactly, so publishing `gotrue` alone would reach nobody using the higher level packages. Note that `release-tag.yml` only triggers on pushes to `main`, so merging the version pull request into this maintenance branch will not create the tags. They need to be pushed manually, or that workflow needs a `workflow_dispatch` trigger, before `release-publish.yml` can run against `gotrue-v2.27.2`. ## Testing - `dart pub get` resolves the workspace cleanly. - `dart analyze lib test` in `packages/gotrue`: no issues. - `dart analyze --fatal-infos packages/storage_client`: no issues. - `dart test test/src/types/session_test.dart test/src/helper_test.dart`: 57 passing, including two new tests covering a `double` `expires_in` and `double` `exp`, `nbf`, and `iat`. - `dart format`: clean. - The Android build is verified by CI only, it was not built locally.
#1720) ## What kind of change does this PR introduce? Test fix for a flaky WASM CI failure on main. ## What is the current behavior? `Expired session emits exception when no auto refresh` waits a fixed 100ms and then asserts that the next event on `onAuthStateChange` is an error: ```dart await Future.delayed(const Duration(milliseconds: 100)); await expectLater( Supabase.instance.client.auth.onAuthStateChange, emitsError(isA<AuthException>()), ); ``` The session recovery flow emits `initialSession`, then `signedOut` (with `signOutReason: sessionExpired`), and only after the logout HTTP call fails does the `AuthException` reach the stream. Since `ReplaySubject` replays only the latest event, the test passes only when the whole flow, including the HTTP roundtrip, completes within the 100ms budget. On a slow runner the subscription lands between `signedOut` and the error, the replayed event is a data event, and `emitsError` fails: ``` Which: emitted • AuthState(event: signedOut, session: null, fromBroadcast: false, signOutReason: sessionExpired) ``` This is exactly the failure seen on the main WASM runs starting with the run for #1716. The race is unrelated to that commit: reducing the delay reproduces the identical failure on 29286f4, the commit before it, and both failing CI runs executed concurrently on loaded runners while all passing runs did not. ## What is the new behavior? The test drops the fixed delay and skips past data events until the error arrives, which is deterministic for every interleaving: ```dart await expectLater( Supabase.instance.client.auth.onAuthStateChange, emitsThrough(emitsError(isA<AuthException>())), ); ``` Verified locally on the VM and with `flutter test --platform chrome --wasm`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Updated expired-session authentication coverage to account for intermediate sign-out events and event replay. * Improved validation of eventual authentication errors without relying on fixed timing delays. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What
Fixes an issue where
Session.fromJsonthrew a runtimeTypeErroron Web when compiled with WebAssembly (flutter build web --wasm) during cross-tab authentication synchronization.Resolves #1687
Root Cause Analysis
In
packages/supabase_auth/lib/src/types/session.dart,Session.fromJsonpreviously castjson['expires_in']directly toint?:While JSON parsed by Dart's native
dart:convertjsonDecodedeserializes integer literals asint,Session.fromJsonis also invoked directly with map payloads from JavaScript interop inAuthClient._mayStartBroadcastChannel(viabroadcast_web.dart):In Dart JS-interop (
dartify()), all JavaScript numbers cross the boundary asdoublevalues (e.g.3600.0).dart2js: Dart integers and doubles share an underlying JavaScriptNumberrepresentation at runtime, so3600.0 as int?succeeded without throwing.dart2wasm& Dart VM:intanddoubleare distinct runtime types. Attempting to cast3600.0 as int?results in a runtime error:Because this exception occurred inside the
BroadcastChannelonMessageevent listener (outside the initial channel setuptry/catchguard), all remaining operations in the listener were aborted:_saveSession(session)/_removeSession()was skipped.notifyAllSubscribers(event, session: session, broadcast: false)was skipped.Consequently, receiving browser tabs silently failed to synchronize session state (login, logout, token refresh), causing unexpected sign-outs or stale tokens when switching between tabs.
Changes
1.
Session.fromJsonIn
packages/supabase_auth/lib/src/types/session.dart:expiresInparsing to(json['expires_in'] as num?)?.toInt(). This safely accommodatesint,double, andnullwithout throwing a type cast error.2. Sibling Model Defensive Updates
For consistency and resilience against numbers crossing JS boundaries:
packages/supabase_auth/lib/src/types/jwt.dart: UpdatedJwtPayload.fromJsonto parseexp,nbf, andiatclaims using(json['...'] as num?)?.toInt().packages/supabase_auth/lib/src/auth_admin_oauth_api.dart: UpdatedOAuthClientListResponse.fromJsonto parsenextPage,lastPage, andtotalusing(json['...'] as num?)?.toInt().3. Unit Tests
packages/supabase_auth/test/src/types/session_test.dartverifying thatSession.fromJsonparses adoubleexpires_in(e.g.3600.0) toint3600.packages/supabase_auth/test/src/helper_test.dartverifying thatJwtPayload.fromJsoncorrectly parsesdoubletimestamps forexp,nbf, andiat.Testing & Verification
dart test test/src/types/session_test.dart test/src/helper_test.dartinpackages/supabase_auth(all 59 tests passing).dart run melos analyzeacross all 12 packages and 8 example projects with zero errors or warnings (SUCCESS).Summary by CodeRabbit