feat: Ensure Database Password Security Check Covers All Possible URIs

[pavan-dulam]

Pull Request

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

Issue

Closes: #8833

Approach

Addressed issue #8833 where the database password security check was not checking all possible URIs. Updated the CheckGroupDatabase class to handle cases where the database adapter is not defined in the configuration object, ensuring compatibility with configurations that use config.databaseURI instead. Added error handling with descriptive error messages for password security requirements. Updated issue tracker accordingly.

• 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!

[pavan-dulam]

Hi @mtrezza, Please review at your earliest convenience. Please let me know in case of any improvements.
Thanks

[mtrezza]

CI fails

[codecov]

Codecov Report

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

Project coverage is 93.75%. Comparing base (f1469c6) to head (187e440).
Report is 8 commits behind head on alpha.

Files	Patch %	Lines
src/Security/CheckGroups/CheckGroupDatabase.js	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha    #9078      +/-   ##
==========================================
- Coverage   94.13%   93.75%   -0.38%     
==========================================
  Files         186      186              
  Lines       14687    14727      +40     
==========================================
- Hits        13826    13808      -18     
- Misses        861      919      +58     

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

[mtrezza]

Please see CI and coverage report

[mtrezza]

What is the behavior of Parse Server if both are defined? Is there a way to remove this logic and access the DB URI in another way? Because maybe no matter how the URI is defined, I would imagine it ends up in the same place?

[pavan-dulam]

In the current implementation of both are defined then the adaptor uri will take precedence. And if the adapter uri is not defined then it will go for db uri.
Please let me know if the logic needs to change, And what will be the correct logic
Thanks!

[mtrezza]

In the current implementation of both are defined then the adaptor uri will take precedence.

Is that so? It seems that Parse Server throws an error if both are defined:

parse-server/src/Controllers/index.js

Lines 155 to 161 in 2420024

	if (
	(databaseOptions ||
	(databaseURI && databaseURI !== defaults.databaseURI) ||
	collectionPrefix !== defaults.collectionPrefix) &&
	databaseAdapter
	) {
	throw 'You cannot specify both a databaseAdapter and a databaseURI/databaseOptions/collectionPrefix.';

I think - if possible - we should not replicate the logic here that is already defined in the code above. I suggest to pull the DB URI directly from the database controller. Let getDatabaseController handle the logic and only check the URI of the controller, so that it doesn't matter how it is set.

[pavan-dulam]

parse-server/src/Controllers/index.js

Lines 160 to 167 in 2420024

	) {
	throw 'You cannot specify both a databaseAdapter and a databaseURI/databaseOptions/collectionPrefix.';
	} else if (!databaseAdapter) {
	databaseAdapter = getDatabaseAdapter(databaseURI, collectionPrefix, databaseOptions);
	} else {
	databaseAdapter = loadAdapter(databaseAdapter);
	}
	return new DatabaseController(databaseAdapter, options);

Got it, so here's what I'm thinking: In the above else if condition we can add one more condition to check if adaptor is assigned, if not we can assign the database URI to the adapter. This way, regardless of how the URI is set, it'll be considered. What do you think? Sounds like a plan to me!

[mtrezza]

This sounds like duplicating the logic? If the other logic changes, then this security check won't be accurate anymore because the duplicated logic will be different. Why not just get the DB controller and check the URI?

[pavan-dulam]

Just a quick question for clarification: I took a look at the DB Controller class, but I couldn't spot any DBURI there. Could you help me understand how I should go about pulling the URI? Thanks for your help!

parse-server/src/Controllers/DatabaseController.js

Lines 387 to 403 in 2420024

	class DatabaseController {
	adapter: StorageAdapter;
	schemaCache: any;
	schemaPromise: ?Promise<SchemaController.SchemaController>;
	_transactionalSession: ?any;
	options: ParseServerOptions;
	idempotencyOptions: any;
	constructor(adapter: StorageAdapter, options: ParseServerOptions) {
	this.adapter = adapter;
	this.options = options || {};
	this.idempotencyOptions = this.options.idempotencyOptions || {};
	// Prevent mutable this.schema, otherwise one request could use
	// multiple schemas, so instead use loadSchema to get a schema.
	this.schemaPromise = null;
	this._transactionalSession = null;
	this.options = options;

[mtrezza]

Did you try like it's already done: databaseAdapter._uri; where _uri is an internal var? But you would not get it from the config that is passed when initializing Parse Server but from the initialized Parse Server. Btw, the internal var _uri is not on the superclass StorageAdapter, but in the subclasses like MongoStorageAdapter. I think for now we can just assume that every storage adapter has an internal _uri var, because I at least cannot think of an adapter that would not require a URI. Even though it's not the superclass.