Parse-server: Query Field Stripping

[DanielSinclair]

History

In the 2.3.0 release, 01b05b0 introduced the ability to strip user-sensitive information from queries on the _User class. By default, the email field is protected, meaning that it is only returned from master-key or owner queries. Other fields can be specified upon configuration with the userSensitiveFields array.

Upon release, it was described in 2 ways:

• Breaking Change: email field is not returned anymore for Parse.User queries. (Provided only on the user itself if provided).

• New Feature: Adds ability to strip sensitive data from _User responses, strips emails by default, thanks to Arthur Cinader

Use Cases

This feature can be very useful in a few different instances. Until it was introduced, user preferences, for example, would need to be stored in a separate class and referenced via a pointer. The referenced instance of the ACL controlled class would configure a Role that allowed only the owning user to query it. With the ability to strip sensitive information, this functionality can be accomplished within the _User class without much overhead and engineering. It's a safer approach, and that's good.

Breaking Change

With Parse.com, authenticated users could query the _User class with the expectation of receiving an email address for each user. This was okay. Yes, there could be issues where the functionality could be abused, dumping all of your users email addresses. However, this exact issue was the driving force for a more robust Security-first design, which later become the ACL and Role system. If information in the _User class was deemed at-risk, basic ACLs and Roles could protect them from prying eyes.

In an app I worked on, we protected _User information by limiting the _User scope to those within a trusted network. In this trusted network, it was okay to view other users' email address. Those that were not authenticated or belonging to a separate network could not query for the other users, let alone view their email address.

This was a common practice with expected functionality. There are a number of Parse Community tutorials that describe this approach. If I recall correctly, the blog posts introducing Roles use this as a driving example. It was a very common problem, with a well known solution.

Until 2.3.0, this worked as expected, and as documented.

Implementation

Although I agree that this feature can be useful, I find the implementation to be flawed. As it stands, the default email field cannot be overridden. The defaults are concatenated with the overrides.

userSensitiveFields = Array.from(new Set(userSensitiveFields.concat(
       defaults.userSensitiveFields,
       userSensitiveFields
)));

This is a blocker, and it is problematic.

Let's Improve It

I left a comment on the original commit asking about ways in which this can be resolved and improved. I was instructed to create a pull request.

Yes, I could create a pull request, but my pull request would not be the best for the community. I want to open a discussion about the best ways in which this feature can be improved, before I build the wrong thing. I don't want to end up back where we started, breaking another developer's app.

So let me ask again: what can we do to improve it?

I see a few different paths that we could take. One of them is to resolve the concatenation issue and to properly document the feature. Another is to rethink it from the ground up. Why is the stripping ability limited to the _User class? Aren't there instances where you would want to protect fields in other classes? What if this functionality was introduced into the ACL & Role systems, on a system-wide approach. What could this look like? How could it be best implemented?

Let's talk about it.

[DanielSinclair]

cc @acinader @flovilmart @natanrolnik @drew-gross

[flovilmart]

@DanielSinclair you seem pretty darn sure that parse.com ever let slipped the emails. And I'm quite confident of the opposite.

Also, it seems that you took a lot of time writing this long backstory for something that would be solved by a one liner through a Pull request.

Closing, awaiting PR.

[DanielSinclair]

I came across this issue from a thread that confirms the opposite. Like I said, let's have a discussion — that's how we solve issues correctly. cc: @OpConTech @natanrolnik @bcootner

[flovilmart]

Again, open a PR, we'll evaluate from there.

thanks.

On my side the position is firm and the default has to be secure. There's no discussion to have on that. I you wish to introduce a different behavior, you know what to do.

[acinader]

Hi @DanielSinclair.

I tested the parse.com behavior, but I only tested an anonymous REST request, so there may be other use cases where an email could be returned.

I think that the "right" solution to improve on the status quo is to allow email (and any other sensitive fields) to be returned not only to the owner or mater key (which is the current state), but also to be able to be returned to a user that belongs to a role that has been "duly authorized" to get back personally identifiable information.

The change to facilitate this should be pretty straightforward, after which it would need to have tests and some documentation

If you're game to take it on, I'd be glad to review your pr.

[flovilmart]

The change to facilitate this should be pretty straightforward

A new Class Level Permission? Not sure. If anything, I'd let anyone override the restricedFields array, and make it's setup insecure with big fat warnings each time a email leaks out to the public that doesn't belong to the user.

Because we don't have rate limiting or else, those DB would be exposed, too bad for wanting to do reckless things.

[acinader]

My line of thought was to have a configuration pair with a name like "Privileged Roles" and a value that is an array of role names.

at the point that we check if a user is the owner of the record or if the master key is being used, we could add another check to see if the user's role is in the Privileged Roles array. Unless I am missing something, this would be a straightforward way to make an admin role that can see email addresses in an admin ui built for that purpose.

Obviously, my $.02 is that I don't think that making "leaking" configurable or optional to anonymous users is something we should facilitate no matter how big the warning ;). and my suggestion above may be a reasonable compromise????

[flovilmart]

I'm not for coupling roles that are DB driven and dynamic with this behavior. That opens this feature of 'elevated' roles and permission. The more you give the more we'll be asked.

I'm tired of that issue, as you mentioned, this was on par with parse.com.

How I see it now:

1. Allow full override of the array
2. Close that and never come back to it, if one wants to run insecurely, he can still fork.

I know that's extreme, but all of that need to be maintained and be comprehensible. I hope you get my PoV, that may be a bit extreme but I believe we spent already too much energy trying fix a design flow that actually people complain about.

[flovilmart]

ALSO, worth mentioning that one can very well add a beforeSave(User) to set the email to another field etc... If One really want to expose the emails publicly.

[acinader]

Given those two options, I think that '2' is the easy choice and I support closing this issue and not revisiting it again.

[OpConTech]

The problem is that right up until the Parse.com closed the iOS SDK would allow the read of _User email field.

I am 100% sure of this. Coming to Parse Server, I had two choices re-write a bunch of code in my app and re-push to Apple or downgrade Parse Server to the previous version which allows email fields to be read. In my case it would of have been nice if this blocked email field was cleanly documented and there was a config switch for in index.js. Instead I spend days reading change logs and trying to understand this functionality that I have never run into before at Parse.com.

I completed agree that it is more secure to protect the client by lock down even the read access for the email field in _User but that should be up to the developer writing the app. Parse.com locked down the email field as a "special" field up in the server - except for read access. Probably, because with 650,000 apps Parse.com found it is reasonable that there could be an app or apps where it would be ok to not have that field locked down - e.g. Corporate app that is not distributed for public use.

I hope this history of what I experienced when firing up Parse Server is helpful to this conversation.

[acinader]

hm. so it seems clear from various comments on this thread and others that on parse.com there was a facility for allowing privileged roles of some sort to see pii. While I did check the simple case, I didn't look very hard when investigating how parse.com handled this issue.....

[masonlee]

As Parse Server user running into this issue, my vote would be for @flovilmart 's "1.) Allow full override of the array". As it stands now, "['email']" is not really the "default" value of the userSensitiveFields config (as it is labeled "default" in lib/cli/definitions/parse-server.js). Instead it is a permanent restriction. I believe that with every the other config setting, overriding the "default" replaces the entire value. Thus making it overridable would both solve people's problems and make the notion of defaults more congruous.

I can understand that many apps are social and need to share the User objects widely and securely. However, other apps simply lock down the Find permission on User table, and desire to make them Findable only to custom "admin" roles. I would like to do the latter.

(Lastly, thank you guys, from the bottom of my heart, for maintaining this awesome project!)

[jrmeurer]

@masonlee -- Try this:

I put the following code in my index.js file, before spinning up the ParseServer.

var $parse_defaults = require('./node_modules/parse-server/lib/defaults.js'); $parse_defaults.default.userSensitiveFields = [];

Not 100% best practice, but it's a great hack that doesn't require forking the repo... and I'll definitely know if it bricks my server (in the future) when I run locally.

[masonlee]

@jrmeurer Thank you for this tip!