Parse-server: keeping Personal Info personal....

[acinader]

When doing queries on the user object, the response contains all fields. So if your user table has email addresses in it, I can pick your app id out of your ios app and the hit the rest endpoint and get all of your users email addresses. I'd like to be able fix this for our app, and maybe I should fix it for parse-server in general?

The basic idea is: if your not querying your own record or not using the master key, then don't return fields in the user table that are "sensitive". Simple enough?

I don't think that I can use any existing functionality to restrict which fields are returned. So what I did was make a beforeFind hook and then select just the 'whitelist' fields. That works fine.

So the problem that remains is for get requests (i.e. an id is provided) instead of find requests.

I'm game to submit a pr to apply the beforeFind hook to the get which seems like the right thing to me, OR i could add a beforeGet hook if that's what folks thinks would be better.

OR better yet, I am just missing something obvious :).

[flovilmart]

I think I recall the email addresses we removed when querying on parse.com, I still have valid accounts there and I could definitely look for it.

[acinader]

@flovilmart well, in our case, we've added more than just email that is sensitive (zip, phone which are both legally PII), so it would seem that having something general that could even be used for other classes would be useful. For me, I'm not the interested in trying to get field level permissions at this point, but at least being able to do it with hooks....in which case I need to get my hands on get requests :).

[acinader]

but yes, if this was done on parse.com, it would be good for us to strip the email address when not a master or self query of the user table, which I'd be glad to wrap into this effort (though prolly two sep pr as it wouldn't use the same method.....).

[flovilmart]

yep, stripping email on user responses should be done!

[acinader]

on it.

[FridaySG]

Yeah, this can be a pretty nasty problem for many reasons. Would be nice if we could build more security into Parse now that it's open-sourced. Like some sort of mechanism that only allows users to connect from specific client applications and things of that sort.

Returning to the original issue though, I crossed this bridge by creating a "PersonalInfo" class and setting the ACL's read/write restricted to the user in which they belong. Any sensitive data is saved in there.

[flovilmart]

That'a usually what i'd recommend. Use _User for login and publicly available info, or lock it down it your app don't need it. Use other objects for private info.

[mebmichael]

I appreciate the concern for security, but although this is a small change, it introduces a breaking change. Tagging it in release "2.3.0" is inconsistent with npm semantic versioning, unless I'm mistaken. (https://docs.npmjs.com/getting-started/semantic-versioning). Is it the intent of this package to disregard a versioning scheme that many rely on?

[flovilmart]

There are other changes and refactorings that made the 2.3.0. It doesn't break the REST API, so we didn't tag it 3.0.0.
Also, the email key should have been probably hidden to match parse.com behavior.

If you are concerned about the release cycle and tagging, you can very well chip in and contribute yourself.

As a rule of thumb:

Patches: bug fixes, non breaking internal changes
Minor: breaking internal changes (config etc...)
Major: breaking REST API changes.

You should probably semver for automatic update on patches (~> 2.2.0) or like we do, fix all dependencies.

[acinader]

@mebmichael just to chime in since this was my change.

a) I was blissfully unaware, really, of the semvar rules despite having read them, so thanks for pointing it out now that I have some context that is relevant to help me understand the implications.

b) with regards to this change, it would probably be more accurate to call this a regression than a breaking change. Parse.com did not leak pii. I discovered the issue when i was doing some testing and was moderately panicked as I am aware that leaking pii has the potential to expose companies legally. I had a fix merged in by @flovilmart within 36 hours of my discovery.

If we weren't in the process of packaging up 2.3 at the time, I would have advocated for releasing a patch (which still might arguably be the right thing to do now so the fix is more widely distributed.)

Finally, I'd also argue that any functionality that depends on making people's email public without their knowledge should be broken and is probably illegal in at least some jurisdictions (like the EU).

Maybe none of that impacts how it should be versioned in your mind, and I'd be open to hearing your thoughts on it.

So its after the fact at this point, but knowing what I was thinking, what would you suggest would have been the right course of action?

Best and thanks again for a thought provoking question.

[mebmichael]

@flovilmart I agree that this change should definitely have been made ASAP, though I would argue that for some client APIs, removal of a field _is_ a breaking change (i.e., JavaScript). Were you ever able to determine if parse.com excludes the email field? Anyway, thanks for the response and the info :)

@acinader Thanks so much for providing the fix (and for the thoughtful response)! That's hugely appreciated. Yes, I was mostly wondering why things suddenly disappeared in my app when only a minor version changed, and it took a little digging to get to the answer (and more digging after that to get to the reason behind the answer). However, I'm kind of glad that it happened because I'm now aware of the issue you brought up, though I could have easily missed that small detail. I suppose if it's seen more as a patch than a feature, that makes much more sense. Anyway, thanks again for the response!

[acinader]

I can confirm that parse.com does not leak email from the user rest request. my change brought us into compliance with the parse.com behavior.

[Amex22]

Hi everyone !
I'm fighting for many days to understand why I don't get back my users email as before and just find that thread.

Well, I do understand the security thing of @acinader but as @mebmichael says, that is a breaking change for some apps and I spend many days before finding this.

As I don't want to set the masterKey in my app, is there a way to add a CloudCode hook so I can check the request.user roles and add the missing field in the response if the user role is something like Admin ?

Thanks.

[Amex22]

Tried something like this with no luck :(

Parse.Cloud.beforeFind('_User', function(request) { request.master = true; });

[virtualtoy]

Is there an easy way to include email to user queries? userSensitiveFields is concatenated internally with default ['email']. I'd expect that parse server respects userSensitiveFields if it's specified.
@Amex22 I experience exactly same problem and I had to monkey-patch parse server. Then I strip sensitive fields in afterFind hook for all users except admins.

[acinader]

@virtualtoy I made it always strip email on purpose. Not saying I did the right thing, but I did do it intentionally. My two thoughts when making that choice:

1. it mimics what parse.com did
2. publicly "leaking" emails is bad (actually illegal in some jurisdictions) so if a developer using parse needs access to emails for certain roles, etc, then they should have to implement cloud code to get at them, because "making" it simple so find no longer strips it will result in "leaking" that info to anyone who hits the rest api.

Your "monkey" patch where the strip of userSensitiveFields can be conditional based on the logged in user's role is a nice solution if it were configurable, like adding another key to the config allowedToSeeSensitiveFields or something. If you're interested in submitting a PR to do that, I'd be glad to review it and help.

[virtualtoy]

@acinader I totally understand your intentions. Privacy is important, but there's use cases where current solution fails. Let's discuss it first maybe? Probably additional keys are not needed and userSensitiveFields alone is enough, so if user wants to include email they simply provide userSensitiveFields: [] (if not provided then default ['email'] will be used). I suppose that user who provides it knows consequences and takes additional steps to prevent personal info leaking. And also probably show them big red warning when server starts and emails are included into queries

[flovilmart]

@virtualtoy that means that anyone using your app, can pretty much dump your database of emails. Not sure you realize how serious that is.

[virtualtoy]

@flovilmart there's so many ways to screw things, he-he. I explained already that in my app email is stripped in afterFind hook for everyone except admins.

[acinader]

@virtualtoy right, and my suggestion is just to make what that role is configurable and then it would be generally useful enough to add to the codebase -- otherwise its too specific to your code base.

[virtualtoy]

@acinader not sure how making default userSensitiveFields: ['email'], omitting concatenation and showing warning is specific to my code base

[acinader]

oh, that's just a non starter IMHO.

what would be reasonable though would be allowing certain roles to not have email be filtered out, and a pr to do that would be welcome. As I understand it, you have already done and tested much of this work, so I thought you might want to get it mainlined so it's easier for you to keep up with releases going forward.