feat: Prevent Parse Server start in case of unknown option in server configuration

[vivekjoshi556]

Pull Request

• Report security issues confidentially.
• Any contribution is under this license.
• Link this pull request to an issue.

Issue

Closes: #8938

Approach

Created a simple function that checks provided configuration keys against a list of possible keys (retrieved from the definitions file at src/Options/Definition.js). If any provided key is not valid, the function logs a warning message.

• Add tests
• Add changes to documentation (guides, repository pages, code comments)
• Add security check
• Add new Parse Error codes to Parse JS SDK

[parse-github-assistant]

Thanks for opening this pull request!

[mtrezza]

Could you please separate the test into distinct tests, each with their own specific description, for example these scenarios:
a) no warning logged if no unknown option is set
b) warning logged if unknown option is set on root { unknown: 1 }
c) ... on sub object { a: { unknown: 1 }}
d) 2 warnings logged on 2 invalid options

Could you also please rename the warning msg to:

“The following Parse Server option is not recognized”, instead of “key”.

Also, I believe the server shouldn’t even start up if an invalid option has been set, because it is likely configured incorrectly, which could affect data integrity, security, app performance or user experience.

[vivekjoshi556]

Hello @mtrezza,

For tests, the given test includes these 3 conditions that you mentioned.
b) warning logged if unknown option is set on root { unknown: 1 }
c) ... on sub object { a: { unknown: 1 }}
d) 2 warnings logged on 2 invalid options

Is there something else that you want me to do in that respect?
I'll make other changes ASAP.

[mtrezza]

Yes, could you please refactor the test into individual tests, one for reach scenario? The purpose of a test is to test a specific scenario that is described in the test title. So that it's easier to debug if a specific test fails, than a composite test that contains many smaller tests. The repetitive code can be moved into the describe section, like the spies.

[vivekjoshi556]

Hi @mtrezza,

Before I was checking for config variables based on the keys of defnitions from Definitions.js, but now I realize that there are several other keys that are being used that are not mentioned in those defnitions: level, loggerController, filesController, userController, pushController, pushWorker, ....
To handle all these cases is there a single place where I can get name of all these variables that need to be handled?

[mtrezza]

Not sure, I think all these options should be somewhere in the code; but I'm not sure if there is a comprehensive list of all options. Maybe the definitions file of the Parse Server options is the best source for that?

[vivekjoshi556]

Not sure, I think all these options should be somewhere in the code; but I'm not sure if there is a comprehensive list of all options. Maybe the definitions file of the Parse Server options is the best source for that?

The definitions files doesn't give most of the variables.
I can look at all the warnings that I am getting and add them manually. Do you think that it would be right way to go here?

[mtrezza]

I believe we need a solution that doesn't require maintaining a separate list if we already have a list, like the definitions. What is missing in the definitions file, for example?

[vivekjoshi556]

I believe we need a solution that doesn't require maintaining a separate list if we already have a list, like the definitions. What is missing in the definitions file, for example?

state, loggerController, hasPushScheduledSupport, authDataManager, masterKeyIpsStore, maintenanceKeyIpsStore

to name some.

[mtrezza]

Are these really Parse Server options?

I mean the main issue is, if they are not in the definitions file, then they are not (poorly) documented Parse Server options, because the docs for them is auto generated from the definitions file.

If they are really undocumented then I suggest to add them to the definitions file, which is fairly easy to do. You could take a look at the existing options and then just add them. There are instructions on how to do that here.

[vivekjoshi556]

Yes, I understand but at one point or another they are expected in the config variable. They are also used in test files as well which is causing a lot of log messages during tests.
hasPushScheduledSupport -> src/StatusHandler.js

[mtrezza]

Yes, so I believe the definitions file is the actual source to determine whether an option key is valid or not. If an options is missing there then that's likely a bug and we could simply add it, as part of this feature. I assume they are not many?

[vivekjoshi556]

I ran tests, some of them didn't pass, but I got warning messages for these keys:

cacheController, loggerController, filesController, 3, masterKeyIpsStore, pushController, exampleKey, rateLimits, 5, userController, state, hasPushScheduledSupport, version, _mount, retryWrites, 4, databaseController, authDataManager, parseGraphQLController, schemaCache, RateLimitZone, 0, liveQueryController, database, generateEmailVerifyTokenExpiresAt, generateSessionExpiresAt, analyticsController, maintenanceKeyIpsStore, patternValidator, 2, hooksController, SchemaCacheTtl, level, hasPushSupport, pushWorker, applicationId, 1, pushControllerQueue

[mtrezza]

The items _mount and the numbers 0, etc make me think that the keys you are checking are not the right ones.

[vivekjoshi556]

Sorry for late reply. You were right there was a slight mistake that numbers were getting printed. But I looked into other keys and the locations they are set:

_mount
level
Controllers, hasPushSupport, pushWorker, pushControllerQueue, authDataManager, schemaCache
state
masterKeyIpsStore
maintenanceKeyIpsStore
rateLimits

and some others keys.

[mtrezza]

Yes, so whatever is missing and is really a Parse Server option may need to to be added to the definitions file. That's easy, see the link I shared previously. I doubt that _mount is a Parse Server option, it looks more like an internal variable. You may need to add a list for excluded keys because we know these are internal keys.

[vivekjoshi556]

Hey @mtrezza,

I'm done with this one. Please review and let me know if something else needs to be changed.

[mtrezza]

Could you resolve the conflicts?

[vivekjoshi556]

@mtrezza, Conflicts resolved.

[codecov]

Codecov Report

Attention: Patch coverage is 97.22222% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 94.14%. Comparing base (f1469c6) to head (7ac43a5).

Files	Patch %	Lines
src/Config.js	75.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            alpha    #8987   +/-   ##
=======================================
  Coverage   94.13%   94.14%           
=======================================
  Files         186      186           
  Lines       14687    14722   +35     
=======================================
+ Hits        13826    13860   +34     
- Misses        861      862    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mtrezza]

Yes, sounds good!

[mtrezza]

Very nice! This looks much cleaner now. Let's wait for the CI to pass, then this is good to merge?

[vivekjoshi556]

Very nice! This looks much cleaner now. Let's wait for the CI to pass, then this is good to merge?

yes it is done from my end.

[parseplatformorg]

🎉 This change has been released in version 7.1.0-alpha.5

[parseplatformorg]

🎉 This change has been released in version 7.1.0-beta.1

[parseplatformorg]

🎉 This change has been released in version 7.1.0