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.

Sign up for GitHub

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

feat: Ensure Database Password Security Check Covers All Possible URIs #9078

Open
wants to merge 6 commits into
base: alpha
Choose a base branch
from
Open

feat: Ensure Database Password Security Check Covers All Possible URIs #9078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
be0a456
feat:Ensure Database Password Security Check Covers All Possible URIs
pavan-dulam Apr 11, 2024
d7a8230
feat: Ensure Database Password Security Check Covers All Possible URIs
pavan-dulam Apr 11, 2024
51df099
Merge branches 'feature/8833-database-password-security-check' and 'f…
pavan-dulam Apr 11, 2024
51af7de
Merge branch 'alpha' into feature/8833-database-password-security-check
mtrezza Apr 15, 2024
4e9fa64
fix: resolve lint no-unused-vars
pavan-dulam Apr 26, 2024
187e440
fix: removed unnecessary check
pavan-dulam Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions spec/SecurityCheckGroups.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,20 +62,36 @@ describe('Security Check Groups', () => {
expect(group.checks().length).toBeGreaterThan(0);
});

it('checks succeed correctly', async () => {
it('checks succeed correctly with database adapter defined', async () => {
const config = Config.get(Parse.applicationId);
config.database.adapter._uri = 'protocol://user:aMoreSecur3Passwor7!@example.com';
const group = new CheckGroupDatabase();
await group.run();
expect(group.checks()[0].checkState()).toBe(CheckState.success);
});

it('checks fail correctly', async () => {
it('checks succeed correctly with databaseURI defined', async () => {
const config = Config.get(Parse.applicationId);
config.databaseURI = 'protocol://user:insecure@example.com';
const group = new CheckGroupDatabase();
await group.run();
expect(group.checks()[0].checkState()).toBe(CheckState.success);
});

it('checks fail correctly with database adapter defined', async () => {
const config = Config.get(Parse.applicationId);
config.database.adapter._uri = 'protocol://user:insecure@example.com';
const group = new CheckGroupDatabase();
await group.run();
expect(group.checks()[0].checkState()).toBe(CheckState.fail);
});

it('checks fail correctly with databaseURI defined', async () => {
const config = Config.get(Parse.applicationId);
config.databaseURI = 'protocol://user:insecure@example.com';
const group = new CheckGroupDatabase();
await group.run();
expect(group.checks()[0].checkState()).toBe(CheckState.fail);
});
});
});
9 changes: 8 additions & 1 deletion src/Security/CheckGroups/CheckGroupDatabase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,15 @@
}
setChecks() {
const config = Config.get(Parse.applicationId);
let databaseUrl;
const databaseAdapter = config.database.adapter;
const databaseUrl = databaseAdapter._uri;
if (databaseAdapter) {
// If database adapter is defined, use its URI
databaseUrl = databaseAdapter._uri;
} else if (config.databaseURI) {

Check warning on line 22 in src/Security/CheckGroups/CheckGroupDatabase.js

View check run for this annotation

Codecov / codecov/patch

src/Security/CheckGroups/CheckGroupDatabase.js#L22 

Added line #L22 was not covered by tests
// If database adapter is not defined, fallback to config.databaseURI
databaseUrl = config.databaseURI;

Check warning on line 24 in src/Security/CheckGroups/CheckGroupDatabase.js

View check run for this annotation

Codecov / codecov/patch

src/Security/CheckGroups/CheckGroupDatabase.js#L24 

Added line #L24 was not covered by tests
}

Check failure on line 25 in src/Security/CheckGroups/CheckGroupDatabase.js

View workflow job for this annotation

GitHub Actions / Lint 

Trailing spaces not allowed
Comment on lines +19 to +25
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

@mtrezza mtrezza Apr 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

@mtrezza mtrezza Apr 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Member

@mtrezza mtrezza Apr 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return [
new Check({
title: 'Secure database password',
Expand Down
Loading