search

supabase / supabase-flutter

Flutter integration for Supabase. This package makes it simple for developers to build secure and scalable products.
https://supabase.com/
MIT License
737 stars 184 forks source link

Unchecked null access in AuthHttpClient #969

Closed AtjonTV closed 3 months ago

AtjonTV commented 4 months ago

Describe the bug Sentry caught this exception right after an AuthApiException, so seemingly during a session refresh.

To Reproduce I am not sure how this can be reproduced, but this exact error in the same constellation was captured twice within 9 days.

The first was on 28th June, second on 7th July.

Expected behavior Graceful handling of possibly-null variables.

Screenshots Failed token refresh:

AuthApiException: AuthException(message: Invalid Refresh Token: Already Used, statusCode: 400)
  File "gotrue_client.dart", line 1206, in GoTrueClient.notifyException
  File "gotrue_client.dart", line 995, in GoTrueClient.recoverSession
  File "<asynchronous suspension>"
  File "supabase_auth.dart", line 90, in SupabaseAuth.recoverSession
  File "<asynchronous suspension>"
  File "cancelable_operation.dart", line 425, in CancelableCompleter.complete.<fn>

Followed by:

_TypeError: Null check operator used on a null value
  File "auth_http_client.dart", line 20, in AuthHttpClient.send
  File "<asynchronous suspension>"
  File "base_client.dart", line 93, in BaseClient._sendUnstreamed
  File "<asynchronous suspension>"
  File "postgrest_builder.dart", line 130, in PostgrestBuilder._execute
  File "<asynchronous suspension>"
  File "postgrest_builder.dart", line 372, in PostgrestBuilder.then

Version (please complete the following information):

├── supabase_flutter 2.5.2
│   ├── supabase 2.1.2
│   │   ├── functions_client 2.1.0
│   │   ├── gotrue 2.6.1
│   │   ├── postgrest 2.1.1
│   │   ├── realtime_client 2.0.4
│   │   ├── storage_client 2.0.1

Additional context I know that the app does not have the latest versions of the dependencies, but the failing code still exists on top-of-main: https://github.com/supabase/supabase-flutter/blob/c65cb6123639d38b2f7fa69b009f29baa8926b01/packages/supabase/lib/src/auth_http_client.dart#L18C23-L22C45

themightychris commented 4 months ago

I always see this the first time I run my app after installing from an APK downloaded from the web. Doesn't happen if I install the same APK via command line or on subsequent runs.

themightychris commented 4 months ago

Actually, the one I'm seeing is slightly different, and happening for fresh Android installs from the Play Store too (but not via shell APK install or VSCode debugging even if I uninstall and clear data first):

E/flutter (29970): [ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: AuthException(message: Invalid Refresh Token: Refresh Token Not Found, statusCode: 400)
E/flutter (29970): #0      GoTrueClient.notifyException (package:gotrue/src/gotrue_client.dart:1190)
E/flutter (29970): #1      GoTrueClient.recoverSession (package:gotrue/src/gotrue_client.dart:979)
E/flutter (29970): <asynchronous suspension>
E/flutter (29970): #2      SupabaseAuth.recoverSession (package:supabase_flutter/src/supabase_auth.dart:90)
E/flutter (29970): <asynchronous suspension>
E/flutter (29970): #3      CancelableCompleter.complete.<anonymous closure> (package:async/src/cancelable_operation.dart:425)
E/flutter (29970): <asynchronous suspension>
AtjonTV commented 4 months ago

We are distributing APKs for cross-device testing using a private F-Droid repository. In this case the App was also "side-loaded" via F-Droid.

Actually looking at what is happening, I think that to solve my "issue" the auth_http_client should check if the variable is null and then throw a error that there is no session. Instead of not doing so and causing a null-check exception.

The other "issue" is the invalid token, as you have in your error too. That is a different error, and should have its own issue.

I think that the invalid session actually doesn't affect our apps' functionality, at least I didn't get any feedback except for the Sentry exception capture.

AtjonTV commented 4 months ago

To further specify what my issue with the logged exception is, it says "_TypeError: Null check operator used on a null value" which isn't helpful.

I propose that AuthHttpClient.send, in the catch block, checks if either currentSession or currentSession.expiresAt is null, and in such case throw a "Cannot refresh nonexistent session" (or similar in wording).

This way a developer at least knows the exception was caused by the fact that a session refresh was triggered even though there was no session.