at_client "crashes" when decryptionService.decrypt() returns a null

[cconstab]

[gkc] Update 2023-02-20

• The fatal bug was fixed in 2022.
• However we have not yet tackled the follow-on issue of how the contacts widget should deal with atSigns which no longer exist.
• Created ticket in at_widgets which will need an architecture discussion.
• Closing this issue.

Describe the bug
This was originally noticed in spacechat with some @ signs working just fine and others not logging in or not seeing chats.

To Reproduce
Steps to reproduce the behavior:

1. First run an app that uses the at_contacts_flutter
2. Then have an @ sign that has at least some odd data (@ colin always fails)
3. Crash is shown in the logs but the app keeps running

I/flutter (14990): @colin
I/flutter (14990): INFO|2022-03-31 13:52:21.367113|At Onboarding Flutter|Onboarding...! 
I/flutter (14990): SEVERE|2022-03-31 13:52:21.701471|SelfKeyDecryption|Error while decrypting value: Invalid argument(s): Invalid or corrupted pad block 
E/flutter (14990): [ERROR:flutter/lib/ui/ui_dart_state.cc(209)] Unhandled Exception: type 'Null' is not a subtype of type 'String' in type cast
E/flutter (14990): #0      GetResponseTransformer.transform (package:at_client/src/transformer/response_transformer/get_response_transformer.dart:38:69)
E/flutter (14990): <asynchronous suspension>
E/flutter (14990): #1      AtContactsImpl.get (package:at_contact/src/at_contacts_impl.dart:73:19)
E/flutter (14990): <asynchronous suspension>
E/flutter (14990): #2      AtContactsImpl.listContacts (package:at_contact/src/at_contacts_impl.dart:153:19)
E/flutter (14990): <asynchronous suspension>
E/flutter (14990): #3      ContactService.initContactsService (package:spacesignal/app/modules/contacts/controllers/contact_service.dart:131:25)
E/flutter (14990): <asynchronous suspension>
E/flutter (14990): #4      OnbordingScreenState.build.<anonymous closure>.<anonymous closure>.<anonymous closure>.<anonymous closure> (package:spacesignal/app/modules/home/views/onboarding.dart:229:43)
E/flutter (14990): <asynchronous suspension>
E/flutter (14990):

Expected behavior
The library should not error

Were you using an @‎application when the bug was found?

• spacechat

Additional context
This is a low level issue so could have impact across other places

[cconstab]

After digging into the problem that causes the issue is in :-

https://github.com/atsign-foundation/at_client_sdk/blob/trunk/at_client/lib/src/transformer/response_transformer/get_response_transformer.dart

lines 38/39

    // For public and cached public keys, data is not encrypted.
    // Decrypt the data, for other keys
    if (!(decodedResponse['key'].startsWith('public:')) &&
        !(decodedResponse['key'].startsWith('cached:public:'))) {
      var decryptionService = AtKeyDecryptionManager.get(tuple.one,
          AtClientManager.getInstance().atClient.getCurrentAtSign()!);
      atValue.value =
          await decryptionService.decrypt(tuple.one, atValue.value) as String;
    }

I put in a null check and the app now works as it should

    // For public and cached public keys, data is not encrypted.
    // Decrypt the data, for other keys
    if (!(decodedResponse['key'].startsWith('public:')) &&
        !(decodedResponse['key'].startsWith('cached:public:'))) {
      var decryptionService = AtKeyDecryptionManager.get(tuple.one,
          AtClientManager.getInstance().atClient.getCurrentAtSign()!);
// temp fix 
       var temp = await decryptionService.decrypt(tuple.one, atValue.value);
       if (temp != null){
         atValue.value = temp.toString();
       }
    }

[cconstab]

Although that fixes things it is probably not the ideal solution.. Would appreciate your thoughts on the best way to fix the bug.

@VJag @gkc @murali-shris @kalluriramkumar

[cconstab]

With this 'fix' in place still get plenty of errors from the contact lib @sarika01 / @murali-shris

My guess is that these are contact entries that we used to be able to decrypt but cannot now as the @ sign keys changed for some reason..

If that is the case I think we should remove (carefully :-)) the contact if is not longer decryptable ... What do you think ?

I/flutter (17415): SEVERE|2022-03-31 16:31:45.446241|AtContactsImpl|Invalid JSON. Unexpected character found in JSON : NFmJEPvYE50SNgnZujZKZNAuPmnZwcLRuAgcmP4vhJM7c8PnVLPWflpzsC+rv0rpSYDqQTVQUpSu0H2YGgouuRvptA/b/ZSKmc4bG4/IpTlG5eQZ3/K3fUBWte6qWcV3xkmECeWvkbEatN2NhQ/SHaE5Ztjj7yT/6NlISl/Ig2okfMWdKYdVL/0aZidccHbIksoBmF3fMJKwRuOnpHHqVG+Wscz5YE11vZ6MIiGKxlvBr7VEH1+6e2A5xh704ayQM+P+IKj7P20fi1Mb1tz2BPSqL96gPOfjSmRdop1NrpnygkRUbKRg0O/Md2at3NRwlde4wbcC48hrZ4V/tuRxVrjKk48DfdZC9N+lDP3WPMpSES92i4o8Lc3tmJiSgD0A
I/flutter (17415): SEVERE|2022-03-31 16:31:45.446935|AtContactsImpl|Invalid atsign contact found @AtKey{key: atconnections.deepburntorange.colin.at_contact.spacesignal, sharedWith: null, sharedBy: @colin, namespace: spacesignal, metadata: Metadata{ttl: null, ttb: null, ttr: null,ccd: null, isPublic: false, isHidden: false, availableAt : null, expiresAt : null, refreshAt : null, createdAt : null,updatedAt : null,isBinary : false, isEncrypted : null, isCached : false, dataSignature: null, sharedKeyStatus: null}, isRef: false} : Exception: Invalid JSON found

[VJag]

It is possible to do remove a 'no longer valid' contact.

One way to do that is:

1. Self key decryption throws the exception back to app

2. Give an option to the widget to decide if the contact can be deleted or not

@OverRide
Future<List> listContacts({bool removeInvalid = false}) async

3. Handle the exception in the contacts library when contact is no longer decryptable

   try {
   contact = await get(atsign, getAtKey: atKey);
   } on Exception catch (e) {
   logger.severe('Invalid atsign contact found @$key : $e');
   if(removeInvalid) {
   await delete(atsign);
   }
   }

The reason we see these errors all the time is that we have not yet removed that contact.

[VJag]

I think even better would be to return a bad contact back so that the "user will not have silently removed without any information" kind of experience.

For that we have to do the following:

1. Introduce isValid on AtContact defaulting it to true
2. In the contacts library we catch the exception during decryption failure and still add the contact to the return set with @sign and isValid populated to false

Now the app can still show the contact as invalid and the user can have options to "delete" or "delete and add again" etc..
However, till the contact is deleted we will continue to see the app logs being flooded with error messages.

I personally prefer this approach where the Contact is returned back as a bad one.

[murali-shris]

Fix in at_client
#459

[VJag]

@sarika01 @cconstab Fix is 1st piece of the puzzle. We have to brainstorm an approach to deal with it on the side of the contacts.

[gkc]

[gkc] Update

• The fatal bug was fixed in 2022.
• However we have not yet tackled the follow-on issue of how the contacts widget should deal with atSigns which no longer exist.
• Created ticket in at_widgets which will need an architecture discussion.
• Closing this issue.