pocketbase / dart-sdk

PocketBase Dart SDK
https://pub.dev/packages/pocketbase
MIT License
511 stars 51 forks source link

TypeError in `authWithPassword()` if server reponse is 308 "Permanent Redirect" #59

Closed gOATiful closed 1 month ago

gOATiful commented 3 months ago

Hi everyone,

I tried to log in to a pocketbase server via the authWithPassword() method. The firewall in front of the PB instance redirected me and I got a _TypeError (type 'String' is not a subtype of type 'Map<String, dynamic>?' in type cast) exception. I tracked it down and found that the following cast is causing the issue: https://github.com/pocketbase/dart-sdk/blob/d7c0c877587befce623608c899be657bd2f710e2/lib/src/services/record_service.dart#L226

This happens because client.send() can also return a string due to this line: https://github.com/pocketbase/dart-sdk/blob/d7c0c877587befce623608c899be657bd2f710e2/lib/src/client.dart#L232

I will submit a pull request shortly with a proposed solution.

Best and thank you

ganigeorgiev commented 3 months ago

I don't exactly understand your use case but usually I'd expect the Dart HTTP client to automatically follow redirects. If the resulting page of the redirect is a plain text and not a json then Yes, it would fail but I'm not sure if this is something that the SDK should deal with.

As mentioned in your PR, I'll think a little more on it after the refactoring (there are no ETAs).

gOATiful commented 3 months ago

I guess you are right with the automatic follow part, but in my case the server responded with a different (default) endpoint, therefore following the redirect ends in a crash (_TypeError from above) caused by the PB-sdk. Since this a lib internal error, I would expect to get something "outside" to deal with it, other than trying to catch a cast-error that should not happen anyways 🙈 Best

ganigeorgiev commented 3 months ago

The fix in your PR is not OK because the above error is not related to the 308 redirect. It could happen every time when you have a reverse-proxy or anything else intercepting the requests and that returns a non standard PocketBase response.

The more proper fix for the TypeError I guess would be to wrap all type assertions in a helper like assertOrThrowClientException in order to normalize the thrown exceptions, but again this is not a standard operational behavior as the SDK expects all API responses to be a valid PocketBase json corresponding to the specific endpoint.

As mentioned earlier, I'll think a little more on it after the refactoring, but for now remains a very low priority.

ganigeorgiev commented 1 month ago

Graceful fallback value assert was implemented in the develop branch together with some other changes related to the upcoming PocketBase v0.23.0 release.

Note that even with the above change there is no guarantee that this will cover every case because there is nothing preventing the proxy returning valid json but with custom/unknown fields (and I'm not willing to add checks whether each response field matches with the expected PocketBase API data as this will be unmaintainable and completely unnecessary in my opinion).