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.

[mtrezza]

You mean to the /src/Options/index.js file? Could you give an example how that would look like?

[vivekjoshi556]

not the index.js file but the Definitions file that is created finally.

For example in /src/Options/index.js file
customUrls: ?PagesCustomUrlsOptions; // PagesCustomUrlsOption is another interface.

In the definitions files it will come out as:
customUrls: {
env: 'PARSE_SERVER_PAGES_CUSTOM_URLS',
help: 'The URLs to the custom pages.',
action: parsers.objectParser,
type: 'PagesCustomUrlsOptions', // With this extra key.
default: {},
}

[mtrezza]

Got it, I assume you would parse the type in buildConfigDefinitions.js and then just add it to the output file? But how would you do that, given that there are other more complex type definitions like:

sendUserEmailVerification: ?(boolean | void);
userSensitiveFields: ?(string[]);
allowOrigin: ?StringOrStringArray;

Even if you ignore any types that have non-letter chars, you would still end up with StringOrStringArray which is not an interface but a type.

I see there is a const nestedOptionTypes in buildConfigDefinitions.js that you could use to identify an interface of sub-keys. So instead of a type: 'PagesCustomUrlsOptions', maybe you could add a child / sub: 'PagesCustomUrlsOptions', which will only be set if there is a child or sub-interface. So that feature would not generally indicate the type, but specifically indicate the sub interface.

[vivekjoshi556]

Yes that is what I was suggesting as well. SO is it all right then? So I go ahead with this?

[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