Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Security around allowClientClassCreation #7156

Open
3 tasks done
dblythy opened this issue Feb 1, 2021 · 10 comments
Open
3 tasks done

Improve Security around allowClientClassCreation #7156

dblythy opened this issue Feb 1, 2021 · 10 comments
Labels
type:feature New feature or improvement of existing feature

Comments

@dblythy
Copy link
Member

dblythy commented Feb 1, 2021

New Feature / Enhancement Checklist

Current Limitation

Parse Server is designed so that it can be booted up and tested easily. However, there are some default configurations that are insecure, meaning that when a developer transitions to production, they mightn't be aware of the openings which they haven't fixed.

Feature / Enhancement Description

Just like fileUpload, migrate allowClientClassCreation to default to false, expect for the core classes (such as _User).

Or:

Have 2 start scripts:

npm start:dev // allows for any insecure option, such as allowClientClassCreation or mountPlayground
npm start:prod // overrides insecure options

@mtrezza
Copy link
Member

mtrezza commented Feb 1, 2021

Thanks for suggesting.

I think it makes sense to change the default of allowClientClassCreation.

Following feedback we have received regarding breaking changes, and in light of a missing change policy, I suggest:

  1. Add a warning message if the option is not explicitly set, that its default will change in the future -> this PR.
  2. In a future PR, change the default -> maybe already create a separate PR with a note not to merge before some time in the future.

@dblythy
Copy link
Member Author

dblythy commented Feb 1, 2021

I think the challenge is that for new developers, their servers will respond with an error whenever they try to save their new objects, or even sign up as those classes haven't been created. This could lead to people thinking Parse Server doesn't work.

Maybe we could tie this into @Moumouls PR of having schema defined on startup.

@Moumouls
Copy link
Member

Moumouls commented Feb 1, 2021

Note here, in my schema pr allowClientClass is mandatory disabled, to avoid inconsistency, since the source of ruth become the predefined static schema and not schema into the DB.

@dblythy
Copy link
Member Author

dblythy commented Feb 1, 2021

Hmmm, interesting. Adding on to your point @mtrezza, maybe we could setup the Parse Server Example repo with allowClientClassCreation:true; //do not use this in production so that changing the default doesn't become a stumbling block for new developers

@mtrezza
Copy link
Member

mtrezza commented Feb 1, 2021

I think the challenge is that for new developers, their servers will respond with an error whenever they try to save their new objects, or even sign up as those classes haven't been created. This could lead to people thinking Parse Server doesn't work.

Not sure I can follow, isn't that expected behavior when disallowing client class creation already today?

Note here, in my schema pr allowClientClass is mandatory disabled, to avoid inconsistency, since the source of ruth become the predefined static schema and not schema into the DB.

I took a look at the PR, it seems to disallow schema changes when a static schema is set. It seems intuitive that when setting a static schema it cannot be modified by creating a class from client or Cloud Code, but that is an optional feature, so I'm not sure how that is related to this issue here, which does not involve a static schema.

We should avoid rushing breaking changes, unless they are urgent security fixes. There have been several comments about this and if we ask, we should also listen and take action. Besides that, it is also best practice.

@dblythy
Copy link
Member Author

dblythy commented Feb 1, 2021

It is expected behaviour. My point being, if this is the new configuration, a developer will:

  1. Start a new Parse Server
  2. Attempt a signup / test object save
  3. Get an error as class does not exist
  4. Assume the project is broken

As a new default configuration, it adds a potential hurdles for new developers if they are just trying to play with Parse Server. Parse has always been novice friendly.

I was referring to Antoine's PR as maybe configurations that don't have a schema specified could result to some sort of default schema. I should have made that clear, my mistake.

Although I understand the comments around depreciation and evolution, personally I think any improvements that can be made to improve the default security of the Parse Server configuration are worth the time.

If it's inconvenient that the default configuration sets allowClientClassCreation and mountPlayground to false, I could only imagine how inconvenient it would be if those features were misused on a Parse Server.

@mtrezza
Copy link
Member

mtrezza commented Feb 1, 2021

As a new default configuration, it adds a potential hurdles for new developers if they are just trying to play with Parse Server. Parse has always been novice friendly.

I think it would be enough to add a simple "use these less restrictive setting to play around, but don't use in prod, because..." to the readme > getting started.

personally I think any improvements that can be made to improve the default security of the Parse Server configuration are worth the time.

Sure, worth the time yes, but to make developers aware that their deployment is not secure, we don't want to risk crashing their production server because they didn't notice yet another breaking change or decide for them how urgently they should change their settings.

Parse Server is just one of many components in a stack that requires attention. Fewer braking changes and more time to adapt to them would be more developer friendly. That is why deprecating in a phased manner is also a recurring community feedback. Otherwise we risk developers becoming update-adverse and running on old versions, which doesn't improve security either.

Actually, the SecurityChecks is what is likely most effective in helping developers secure their servers and allows us to gracefully change security relevant defaults over a slightly longer time span.

@mman
Copy link
Contributor

mman commented Feb 1, 2021

Could we perhaps add an option called “environment” defaulting to “development” where client class creation and potentially other things are enabled, and when set to “production” they will get to default safe mode?

@mtrezza
Copy link
Member

mtrezza commented Feb 1, 2021

Not sure about introducing a meta option that sets some other options behind-the-scene. It is not transparent as to which options is sets to which values. It does not help in climbing the learning curve for new developers by obfuscating the Parse Server config.

I think improving the docs could do it. The getting started guide could simply recommend a "development / playground" configuration for Parse Server which could be already pre-set in the parse-server-example repo.

Together with the SecurityChecks feature, it is also clear which options should be changed for a production environment.

@mtrezza
Copy link
Member

mtrezza commented Apr 10, 2021

This issue can be handled quite easily now:

  • With the new Deprecator we can prepare the default change of allowClientClassCreation.

  • With the new Security Check, Parse Server Example can simply contain the less restrictive settings. We can enable Security Check in the example repo and recommend to run them before deploying to production. The Security Check will warn the developer to change allowClientClassCreation.

  • In addition we can add a warning to the Parse Server options docs.

  • In addition we can recommended a config for development in the docs which includes less restrictive settings. A start:dev npm script that includes less restrictive settings may not work in all cases, because Parse Server merges the configs from multiple sources and that may overwrite these env or cli settings.

@mtrezza mtrezza added type:feature New feature or improvement of existing feature and removed type:improvement labels Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or improvement of existing feature
Projects
None yet
Development

No branches or pull requests

4 participants