[Ready for payment][$250] Add some basic runtime type validation in Onyx

[roryabraham]

coming from https://github.com/Expensify/Expensify/issues/363913...

Problem

It is possible to queue incorrect Onyx updates from Auth. Twice now, we have seen an array used in place of an object in an Onyx update in Auth, which when merged completely removes the object and replaces it with an array.

Solution

While we recognize that there a number of more elegant solutions to this (compile time type validation of Onyx updates in the back-end being a prime candidate), we should apply a simple solution to prevent simple mistakes from having catastrophic effects, as we have seen occur in production twice already.

What this means is - let's update Onyx such that:

• it will not apply any updates where we try to merge an array into an object, or vice-versa
• it will log an alert if we do try to do that (except on dev, of course)

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0152270829f1a6f929
• Upwork Job ID: 1777413537488273408
• Last Price Increase: 2024-04-08

Issue Owner

Current Issue Owner: @kadiealexander

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0152270829f1a6f929

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)

[melvin-bot]

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[ShridharGoel]

Proposal

Please re-state the problem that we are trying to solve in this issue.

We need to validate if existing items and new changes have the same type before merging in Onyx.

What is the root cause of that problem?

New change.

What changes do you think we should make in order to solve the problem?

Below is the pseudo code which needs to be added in merge method in react-native-onyx:

OnyxUtils.get(key).then(existingValue => {
  const existingValueType = Array.isArray(existingValue) ? 'array' : 'object';
  const incomingValueType = Array.isArray(changes) ? 'array' : 'object';
  if (existingValueType !== incomingValueType) {
      // Log an alert for the mismatched types except on dev
      return Promise.resolve();
  }

   // Continue with the merge

}).catch(exception => {
    // Log the exception here
    return Promise.resolve();
});
       

For set:

function set<TKey extends OnyxKey>(key: TKey, value: OnyxEntry<KeyValueMapping[TKey]>): Promise<void[]> {
  OnyxUtils.get(key).then(existingValue => {
    const existingValueType = Array.isArray(existingValue) ? 'array' : 'object';
    const incomingValueType = Array.isArray(changes) ? 'array' : 'object';

    if (existingValueType != incomingValueType) {
        // Log an alert for the mismatched types except on dev
        return Promise.resolve();
    }

   // Continue with existing logic
  })

}

For mergeCollection, change the existingKeys logic to below:

const existingKeys = []

keys.forEach((key) => {
    if (persistedKeys.has(key)) {
        OnyxUtils.get(key).then((existingValue) => {
            const existingValueType = Array.isArray(existingValue) ? 'array' : 'object';
            const incomingValueType = Array.isArray(mergedCollection[key]) ? 'array' : 'object';
            if (existingValueType === incomingValueType) {
                existingKeys.push(key)
            } else {
                // Log an alert for the mismatched types except on dev
            }
        }).catch(exception => {
            // Log the exception here
        });
    }
});

For multiSet, this would be the updated method:

function multiSet(data: Partial<NullableKeyValueMapping>): Promise<void[]> {
    const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(data);

    const updatePromises = keyValuePairs.map(([key, value]) => {
        const prevValue = cache.getValue(key, false);

        const existingValueType = Array.isArray(prevValue) ? 'array' : 'object';
        const incomingValueType = Array.isArray(value) ? 'array' : 'object';
        if (existingValueType !== incomingValueType) {
            // Log an alert for the mismatched types except on dev
            return null
        }

        // Update cache and optimistically inform subscribers on the next tick
        cache.set(key, value);
        return OnyxUtils.scheduleSubscriberUpdate(key, value, prevValue);
    });

    return Storage.multiSet(keyValuePairs)
        .catch((error) => OnyxUtils.evictStorageAndRetry(error, multiSet, data))
        .then(() => {
            OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MULTI_SET, undefined, data);
            return Promise.all(updatePromises.filter(promise => promise !== null));
        });
}

[mananjadhav]

@ShridharGoel Wouldn't we need to check if it's an Array for the existingValue as well? I am assuming we're using the existing block with OnyxUtils.get(key).then(existingValue.

@roryabraham

1. The following is true for all/any environment right?

it will not apply any updates where we try to merge an array into an object, or vice-versa

2. Would Logger.logAlert suffice for the mismatch? do we need any sendActionToDevTools?

[ShridharGoel]

Proposal

Updated to use similar check for existing value and changes.

[dominictb]

Proposal

Please re-state the problem that we are trying to solve in this issue.

We want to:

• It will not apply any updates where we try to merge an array into an object, or vice-versa
• It will log an alert if we do try to do that (except on dev, of course)

What is the root cause of that problem?

We don't have such functionality in Onyx yet

What changes do you think we should make in order to solve the problem?

A. Updates for Onyx.merge:

First there're a few considerations:

• There're 3 cases possible for a change:
  • Nullish (null, undefined)
  • Object
  • Array
    If existing value is Array, it can be used with Nullish/Array change, but not Object change. If existing value is Object, it can be used with Nullish/Object change, but not Array change.

So just checking Array.isArray and compare both is not enough, we need to check the Nullish case as well.

We can define a method isUpdateCompatibleWithExistingValue to validate that the update and the existing value are compatible:

function isUpdateCompatibleWithExistingValue(value, existingValue): boolean {
    if (existingValue && value && Array.isArray(existingValue) !== Array.isArray(value)) {
        return false;
    }
    return true;
}

• The changes are batched into an array, we should only remove the invalid changes, not discard the full batch just because one of the update happens to be of the wrong type.

With the above considerations, here're the pseudo code of the changes we can make in Onyx here to filter out the invalid change, Log an alert, and check the change type compatibility properly:

const validChanges = mergeQueue[key].filter((change) => isUpdateCompatibleWithExistingValue(change, existingValue));

if (validChanges.length !== mergeQueue[key].length) {
    Logger.logAlert(`A warning occurred while applying merge for key: ${key}, Warning: Trying to merge array to object or vice versa`);
}

if (!validChanges.length) {
    return undefined;
}

Then below that, when we use mergeQueue[key] like in here, we should change to use validChanges instead.

B. Updates for Onyx.set
In here, we should get the existing value, then check if the new update is compatible with the existing value, if not, early return.
(we can alternatively get the existing value from cache.getValue so it can be synchronous)

OnyxUtils.get(key).then(existingValue => {
        if (!isUpdateCompatibleWithExistingValue(value, existingValue)) {
            Logger.logAlert(`A warning occurred while applying set for key: ${key}, Warning: Trying to set array to object or vice versa`);
            return Promise.resolve();
        }
        
        // now we do the Onyx.set as now

C. Updates for Onyx.mergeCollection

• In here, modify so getCachedCollection accepts an optional collectionMemberKeys param, if it's passed in, we'll use it, if not, we'll use our own existing collectionMemberKeys here
• In here, get the collection value of existingKeys by using getCachedCollection and pass existingKeys as collectionMemberKeys
• In here, we check that mergedCollection[key] is compatible with the existing value from existingKeys above, if they are not compatible, Log a warning by using Logger.logAlert, and do not set to obj (so that update will be ignored)

What alternative solutions did you explore? (Optional)

For A., an alternative is to check update compatibility right in here before we push the changes to the mergeQueue. We can get the existing value by using cache.getValue and check the compatibility using isUpdateCompatibleWithExistingValue, if they are not compatible, log a warning using Logger.logAlert and early return, so the changes will never end up in the merge queue.

[judy-hopps-12345]

### Proposal

Please re-state the problem that we are trying to solve in this issue.
Add some basic runtime type validation in Onyx #39852

What is the root cause of that problem?
The validation of the datatype for Onyx update will be done in the backend but double validation in the frontend is helpful in order to prevent simple mistakes.

What changes do you think we should make in order to solve the problem?
The functions for Onyx update such as Onyx.set(), Onyx.update() are performed by write(..., onyxData, ...) function in the @libs/API/index.ts.
As the parameter for Onyx, we can see a variable - onyxData(which is consist of OptimisticData and onyxDataWithoutOptimisticData). Here, OptimisticData is enough to compare the types of old and new.

There is the example of optimisticData variable below. (refer to tunction signIn())

const optimisticData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: ONYXKEYS.ACCOUNT,
            value: {
                ...CONST.DEFAULT_ACCOUNT_DATA,
                isLoading: true,
                loadingForm: twoFactorAuthCode ? CONST.FORMS.VALIDATE_TFA_CODE_FORM : CONST.FORMS.VALIDATE_CODE_FORM,
            },
        },
    ];

We can get the old value from "key" property, and compare it with "value" property.

const promise1 = new Promise((resolve, reject) => {
    if (oldValueType !== newValueType) {
        Logger.logAlert();
        Promise.reject();
    } else {
        // Do the original action
        Promise.resolve();
    }
});
Promise.all([promise1]).then((values) => {
    console.log(values);
});

What alternative solutions did you explore? (Optional)
N/A

[mananjadhav]

I think @ShridharGoel's proposal makes sense. Pending response from @roryabraham .

🎀 👀 🎀 C+ reviewed.

[melvin-bot]

Current assignee @roryabraham is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[dominictb]

@mananjadhav That proposal won't work in many cases, as mentioned in my proposal.

Some examples where that solution will cause regressions:

• When we try to clear an Array key (we won't be able to clear)
• When there're many updates in update array, but only one of them is invalid
• It will only work with the first changes, since it uses the changes variable, not the mergeQueue[key] (which is the batch of changes that we apply)

Can you double check on that?

cc @roryabraham

[ShridharGoel]

When there're many updates in update array, but only one of them is invalid

At once we pass either an object or array to merge. So, if the existing value is an object and the new value is not, we'll not continue with the merge, and vice-versa.

[ShridharGoel]

We don't have to discard specific array elements.

[dominictb]

At once we pass either an object or array to merge. So, if the existing value is an object and the new value is not, we'll not continue with the merge, and vice-versa.

@ShridharGoel So is it correct that you're suggesting to run OnyxUtils.get(key).then(existingValue => to check the existingValue for every change? (without batching changes)

[james-tindal]

Any solution is likely to conflict with Expensify/react-native-onyx#523

[roryabraham]

The following is true for all/any environment right?

correct

Would Logger.logAlert suffice for the mismatch?

yes

[roryabraham]

Unfortunately the E/App PR to upgrade Onyx was reverted due to a regression from an unrelated Oynx PR. I've asked someone else to redo the upgrade PR, so there's nothing more to do here except for wait for Onyx to be fixed and upgraded again for this to go to staging.

[kadiealexander]

Not overdue.

[mananjadhav]

@roryabraham Are we expecting any updates on the Onyx version?

[dominictb]

@mananjadhav @roryabraham I saw the blocker issue was closed. Should we move forward with this issue? Thanks!

[mananjadhav]

Waiting for an update from @roryabraham on this one.

[mananjadhav]

Quick bump @roryabraham. Did you get chance to look at the previous comments?

[dominictb]

@roryabraham Can you move this issue to the next step? Thanks!

[mananjadhav]

Just checked and it seems @roryabraham is OOO. Let's wait for him to come back by this week, else will ask to reassign.

[roryabraham]

Ok, so this is definitely out in production right now, and I think it was deployed in #42772.

So that means it's been on prod for about a week. Given the unrelated delays, I suggest we pay this out right away and then close this.

[mananjadhav]

@kadiealexander this is ready for payout. Can you update the payment summary?

[kadiealexander]

Payouts due:

• Contributor: $250 @dominictb
• Contributor+: $250 @mananjadhav (Newdot)

Upwork job is here.

@dominictb please apply on Upwork, I couldn't find your account.

[mananjadhav]

@kadiealexander I don't need the Upwork payout. I have requested on NewDot.

[kadiealexander]

Withdrawn, thanks Manan!

[dominictb]

@kadiealexander I submitted the application on Upwork, my profile link is https://www.upwork.com/freelancers/~01f70bed1934fd35d5

Thank you

[JmillsExpensify]

$250 approved for @mananjadhav