pushDevices bad data cause errors for other players

[Alys]

A player's pushDevices array can have bad data in it, as shown in the example below - the first object in the array is correct, the others are not.

"pushDevices": [
    {
        "regId": "fzvFqZketcetcetcetcygb2Q",
        "type": "android",
        "createdAt": "2019-03-13T21:04:36.573Z",
        "updatedAt": "2019-03-13T21:04:36.573Z"
    },
    {
        "updatedAt": "2019-03-13T21:04:20.973Z"
    },
    {
        "updatedAt": "2019-03-13T21:04:20.973Z"
    },
]

When there's bad data, certain actions in Habitica become impossible due to errors thrown by the code that tries to use the array. This affects the player with the bad data of course but can also affect other players. The error at the bottom of this comment occurred when a guild leader tried to remove an inactive guild member. The guild member's pushDevices array had the bad data in it and that was stopping the guild leader from removing them. Being unable to remove a party or guild member is particularly troublesome because the leader of a private group might need to remove someone for privacy / personal safety reasons, and a Group Plan leader must be allowed to reliably remove members otherwise they'll continue to be charged for them.

Because bad data can affect players who don't have the bug in their own account, I think it's worth adjusting the API's code to make errors like this non-fatal when possible. I'm proposing that any code in the API that uses the pushDevices array be modified so that it ignores any entries that aren't valid.

I'll leave this as suggestion-discussion for a few days and then if there's no objections from Habitica's staff, I'll mark it as help wanted.

Once (if) it's marked as help wanted, a contributor can find the code that uses the array by doing git grep pushDevices however I believe the two main files are:

• https://github.com/HabitRPG/habitica/blob/develop/website/server/libs/pushNotifications.js
• https://github.com/HabitRPG/habitica/blob/develop/website/server/controllers/api-v3/pushNotifications.js

You'll want to check though to be sure I haven't missed important files, and also to see the current tests which will help you create new tests.

{
"method":"POST",
"originalUrl":"/api/v3/groups/12345678-4fa5-4e94-8114-123456789abc/removeMember/12345678-18b7-4fd7-ba30-123456789abc?message=",
"headers":{...},"body":{},
"httpCode":400,"isHandledError":true,
"fullError":{
    "errors":{
        "pushDevices.0.type":{
            "message":"Path `type` is required.","name":"ValidatorError",
            "properties":{"message":"Path `type` is required.","type":"required","path":"type"},
            "kind":"required",
            "path":"type"
        },
        "pushDevices.0.regId":{
            "message":"Path `regId` is required.","name":"ValidatorError",
            "properties":{"message":"Path `regId` is required.","type":"required","path":"regId"},
            "kind":"required",
            "path":"regId"
        },
    },
    "_message":"User validation failed",
    "message":"User validation failed: pushDevices.0.type: Path `type` is required......",
    "name":"ValidationError"
},
"timestamp":"2020-02-15T19:43:06.818Z",
"level":"warn",
"message":"ValidationError: User validation failed: pushDevices.0.type: Path `type` is required....\n
    at new ValidationError (/usr/src/habitrpg/node_modules/mongoose/lib/error/validation.js:31:11)\n
    at model.Document.invalidate (/usr/src/habitrpg/node_modules/mongoose/lib/document.js:2490:32)\n
    at EmbeddedDocument.invalidate (/usr/src/habitrpg/node_modules/mongoose/lib/types/embedded.js:291:29)\n
    at /usr/src/habitrpg/node_modules/mongoose/lib/document.js:2339:17\n
    at /usr/src/habitrpg/node_modules/mongoose/lib/schematype.js:1064:9\n
    at processTicksAndRejections (internal/process/task_queues.js:75:11)"
}

[artur-borys]

Since error was thrown at .../groups/.../removeMember maybe a problem is there. From my understanding, Mongoose validates documents before saving them, so maybe here, in groups.js link

EDIT: Maybe wrap member.save() in a try...catch and remove invalid pushDevices elements and then try again to remove any links with Group

EDIT2: I can see that before this commit (link) reqId and type were not required in pushDevices, so maybe a single query that will look for users created before that commit and remove invalid elements from their pushDevices?

[Alys]

Removing a member from a group is one example of the problem. There are likely to be other actions that throw errors (certainly for the player who has the bug and possibly for other players who interact with them), so that's why the files that refer to pushDevices directly might be the best place to fix this.

EDIT:

psuhDevices schema is here: https://github.com/HabitRPG/habitica/blob/0e590c362318e6e5b8fc525df60ecaa77f64c02d/website/server/models/user/schema.js#L629 https://github.com/HabitRPG/habitica/blob/ebbec57b297f63765ba3db57d0cea4d1f671f49b/website/server/models/pushDevice.js#L6-L20

~The schema for pushDevices doesn't specify details so that's why invalid data can be saved: github.com/HabitRPG/habitica/blob/0e590c362318e6e5b8fc525df60ecaa77f64c02d/website/server/models/user/schema.js#L629-L633 I'd guess that the different mobile platforms need to store different data so that's why it's not coded in the schema.~

[artur-borys]

@Alys Right now PushDeviceSchema sets a required option on reqId and type, but before the commit I've mentioned above, it didn't, so that's why there might be some users that have invalid data and any attempt to save will throw an error.

[Alys]

Oh for hecks sakes, I completely misread the schema even after pasting the link to it into my comment. Yeah, the schema specifies that certain parts of a pushDevices object must exist. I'll edit my previous comments for clarity. Thanks very much @artur-borys !

[paglias]

I think what is happening here is very similar to what happened with UserNotifications and a similar solution could be used in this case, see https://github.com/HabitRPG/habitica/blob/develop/website/server/models/userNotification.js#L98

[paglias]

Although the solution linked above cleans up the data so that it isn't returned to the user it never changes what's is saved in the database (which instead it should be doing). Also there's https://github.com/HabitRPG/habitica/pull/11825 which could be mixed with this one

[aristizabal95]

Does the malformed data always looks that way? Missing the RegId and type properties? I haven't done anything about push notifications, but if my intuition is correct, any pushDevice instance missing this information is useless, right? This is the information that's used to reference a specific device, and if missing then that entry is nonsensical.

My guess is that the error happens because when removing a user from a party/guild the schema does some cleanup to remove the ability to receive notifications from that party/guild to the user devices. Probably doing some sort of fetch like getting the join between the party Id and the device Id (this is sql jargon, I haven't really worked with nosql). When doing so the error occurs, because that entry has no device Id in the first place. This would mean that trying to create or search any entry that requires the pushDevice's regId should generate an error.

My two suggestions out of ignorance would be:

• Deleting the malformed entries referring to this pushDevices. If they are nonsensical to begin with, then it also makes no sense to store unusable data in the database.
• Making it so that if a pushDevice doesn't have a regId then it should be ignored. Maybe checking for undefined.

Not sure if this is of any help, but I thought I would share my thoughts about the issue :)

[paglias]

AFAIK they're always empty objects except for the updatedAt and sometimes createdAt properties which are simply metadata and are completely useless in this case.

Making it so that if a pushDevice doesn't have a regId then it should be ignored. Maybe checking for undefined.

The problem with this is finding out why it happens at all since the schema is there and has to be respected, if you try to push an empty object into user.pushDevices and then save it it should not work

Given the existence of this invalid data we should:

• write a migration to remove it
• if we can find the source of the issue fix it, otherwise when saving the user we should remove invalid data like we do for notifications which have a similar issue https://github.com/HabitRPG/habitica/blob/develop/website/server/models/user/hooks.js#L261-L262

The second point is the easier to fix and would be the perfect candidate for a PR if you want to give it a look @aristizabal95

[aristizabal95]

@paglias I see, but is there proof the creation of malformed pushDevices is an ongoing problem? I've been looking at the code and everything looks alright! The controller does some validation here https://github.com/HabitRPG/habitica/blob/f8bab04a0e66a819f6d7ca5b210d27d991446bf3/website/server/controllers/api-v3/pushNotifications.js#L28-L32 As well as validation inside the pushDevice schema, as stated by @Alys on previous comments. https://github.com/HabitRPG/habitica/blob/ebbec57b297f63765ba3db57d0cea4d1f671f49b/website/server/models/pushDevice.js#L6-L20

There are also tests demonstrating how trying to create pushDevices missing the required regId& type would throw an error here https://github.com/HabitRPG/habitica/blob/f8bab04a0e66a819f6d7ca5b210d27d991446bf3/test/api/v3/integration/user/POST-user_push_device.test.js#L15-L31

Maybe checking the database and looking if there are pushDevices that are malformed and created after the PR that added validation would point out if this is in fact something to be fixed first. If it is, I'd be happy to do a PR ignoring malformed pushDevices as a temporary fix.

[Alys]

@aristizabal95 That's an excellent suggestion, thank you.

I've done a search over players who cronned in the past couple of days and found one with a malformed pushDevices object (below; now fixed for that player). That player created their account at 2019-10-18, which was after the commit that added validation, so the validation is not preventing bad entries from occurring (I do not know why).

My feeling is that it might be difficult to find the cause of the malformed data, and there might be more than one cause, and even if we fixed all cause(s) now there might be more in future. Because of that, and because this bug is damaging for players whose own accounts don't have the bug, I think a PR ignoring malformed pushDevices is a good idea. Copying from paglias's post:

when saving the user we should remove invalid data like we do for notifications which have a similar issue https://github.com/HabitRPG/habitica/blob/develop/website/server/models/user/hooks.js#L261-L262

@aristizabal95 I've marked this as in progress for you, although if you've changed your mind about wanting to work on it for any reason, just let us know and I'll change it back to help wanted.

{
    "pushDevices": [
        {
            "updatedAt": "2020-02-20T13:51:20.435Z"
        },
        {
            "regId": "esP....iK-",
            "type": "android",
            "createdAt": "2020-02-20T13:51:39.476Z",
            "updatedAt": "2020-02-20T13:51:39.476Z"
        }
    ]

[aristizabal95]

so the validation is not preventing bad entries from occurring (I do not know why)

This just got pretty interesting out of a sudden! And sure, you're totally right. Making this silently fail is necessary no matter what. I'll get working on it then!

[aristizabal95]

So I just reproduced the error by editing the database using the mongo command. I've never used mongo before, so I have to ask: does mongo applies schema validation un collection.updateOne()? Because it gave me no trouble while adding the malformed pushDevices to a test user.

[paglias]

@aristizabal95 you're right that modifying the database directly skips all validation because that is applied on the server not directly on the database. But all changes to documents in the database go through the server so it must be another bug since we never save direclty to the database without passing through the validations on the server

[aristizabal95]

So I think I figured out where the problem is, and I may need some guidance into finding the best solution.

When removing a user, the API has to update the member document so that it no longer includes the group/guild. https://github.com/HabitRPG/habitica/blob/bad148148cb362e4f65161893362aadcef329dee/website/server/controllers/api-v3/groups.js#L1000-L1003

Because of the schema validation the member object cannot be saved. On the other hand, the group is being successfully saved, which is also causing the memberCount to gradually decrease, even though no member was removed.

this happened to me in my tests after a few tries of removing the member.

[aristizabal95]

The thing is, if my conclusion is correct (which I tested by only running member.save() on the removeGroupMember function), then the user with malformed pushDevices cannot do anything that requires something similar to member.save()!! Now, I don't know if there's a way to specify which properties of the member to update (something like member.save(only: "guilds")). Or if something has to be done with the schema so that it doesn't complain on member update (not sure if that's a good idea).

[paglias]

@aristizabal95 I think the solution we adopted for notifications (which have the same problem of invalid data ending up in the database despite validation) can be used here as well.

Basically in the user's pre-save hook (a function that is run when an user is saved) we would remove any invalid data, making sure that the document can be saved.

Here https://github.com/HabitRPG/habitica/blob/develop/website/server/models/user/hooks.js#L250-L262 you can see how it's done for notifications (there's some extra code), but something like this should work:

  // Filter notifications, remove unvalid and not necessary,
  // handle the ones that have special requirements
  if ( // Make sure all the necessary data is loaded
    this.isDirectSelected('pushDevices')
  ) {
    this.pushDevices = this.pushDevices.filter(pushDevice => {
      // Remove corrupt push devices
      if (!pushDevice || !pushDevice.type || !pushDevice.regId) return false;

      // Keep all the others
      return true;
    });

This will remove existing invalid push devices when the user is saved.

While to make sure it doesn't cause any issue when the user document is loaded we want to modify the way the push devices are returned from the API by modifying the toJSON function similarly to how it's done for notifications.

In the user's toJSON transformation function https://github.com/HabitRPG/habitica/blob/develop/website/server/models/user/hooks.js#L28-L31 we call this method on the notification's model https://github.com/HabitRPG/habitica/blob/develop/website/server/models/userNotification.js#L100-L109 that removed anything not valid.

Does it make sense?

[aristizabal95]

Makes total sense! Thanks for the explanation, I had seen this from your previous comment but wasn't able to see what exactly was it doing to fix it. I'll implement it then.

[aristizabal95]

Even though I implemented the changes there has been no improvements. I think the schema.pre('save') hook is not running because validation happens before this.

I copy-pasted the filtering inside the pre validation hook and printing this did show that the malformed push devices were gone, yet it seems this modifications are not carried when mongoose validates, since the error message is still showing.

I'll post a PR with WIP so that maybe some more feedback can be given, but I think my lack of knowledge in mongoose is starting to show at this point. If after a few tries I don't manage to get this working I think it would be best to give this to someone with more experience in mongoose. I don't want to slow down the process that much.

[paglias]

no worries @aristizabal95 :) post the PR and I'll give it a look.

to reproduce the bug you edited the db directly, right?

[aristizabal95]

Yes, I used the test user to create a guild and added another user with malformed push devices. All of that modifying the db directly