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

Jump to bottom

Save entity process transactional #7627

Draft
wants to merge 18 commits into
base: production
Choose a base branch
from
Draft

Save entity process transactional #7627

wants to merge 18 commits into from
+1,641 −403

Conversation

daneryl
Copy link
Collaborator

@daneryl daneryl commented Jan 31, 2025

fixes #7519

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 31, 2025
View reviewed changes
app/api/odm/MultiTenantMongooseModel.ts
@@ -33,81 +34,142 @@
}

findById(id: any, select?: any) {
return this.dbForCurrentTenant().findById(id, select, { lean: true });
const session = dbSessionContext.getSession();
return this.dbForCurrentTenant().findById(id, select, {

Check failure

Code scanning / CodeQL

Database query built from user-controlled sources High

This query object depends on a
user-provided value
Loading
.
This query object depends on a
user-provided value
Loading
.
This query object depends on a
user-provided value
Loading
.
This query object depends on a
user-provided value
Loading
.
This query object depends on a
user-provided value
Loading
.
This query object depends on a
user-provided value
Loading
.
This query object depends on a
user-provided value
Loading
.
This query object depends on a
user-provided value
Loading
.
This query object depends on a
user-provided value
Loading
.
This query object depends on a
user-provided value
Loading
.
This query object depends on a
user-provided value
Loading
.
This query object depends on a
user-provided value
Loading
.
This query object depends on a
user-provided value
Loading
.

Copilot Autofix AI 4 days ago

To fix the problem, we need to ensure that the id parameter is sanitized before being used in the MongoDB query. This can be achieved by using the $eq operator to ensure that the user input is interpreted as a literal value and not as a query object. This change should be made in the findById method of the MongooseModelWrapper class in the app/api/odm/MultiTenantMongooseModel.ts file.

Suggested changeset 1
app/api/odm/MultiTenantMongooseModel.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/api/odm/MultiTenantMongooseModel.ts b/app/api/odm/MultiTenantMongooseModel.ts
--- a/app/api/odm/MultiTenantMongooseModel.ts
+++ b/app/api/odm/MultiTenantMongooseModel.ts
@@ -37,3 +37,3 @@
     const session = dbSessionContext.getSession();
-    return this.dbForCurrentTenant().findById(id, select, {
+    return this.dbForCurrentTenant().findById({ _id: { $eq: id } }, select, {
       lean: true,
EOF
@@ -37,3 +37,3 @@
const session = dbSessionContext.getSession();
return this.dbForCurrentTenant().findById(id, select, {
return this.dbForCurrentTenant().findById({ _id: { $eq: id } }, select, {
lean: true,
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
app/api/odm/MultiTenantMongooseModel.ts
return this.dbForCurrentTenant().deleteMany(query);
async deleteMany(query: UwaziFilterQuery<DataType<T>>, options: any = {}) {
const session = dbSessionContext.getSession();
return this.dbForCurrentTenant().deleteMany(query, {

Check failure

Code scanning / CodeQL

Database query built from user-controlled sources High

This query object depends on a
user-provided value
Loading
.
This query object depends on a
user-provided value
Loading
.

Copilot Autofix AI 4 days ago

To fix the problem, we need to ensure that the user-provided data is interpreted as a literal value and not as a query object. This can be achieved by using the $eq operator in the MongoDB query. This approach ensures that the user input is treated as a literal value, preventing any potential NoSQL injection attacks.

We will modify the deleteMany method in the MongooseModelWrapper class to use the $eq operator for the _id field. This change will ensure that the _id is treated as a literal value in the query.

Suggested changeset 1
app/api/odm/MultiTenantMongooseModel.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/api/odm/MultiTenantMongooseModel.ts b/app/api/odm/MultiTenantMongooseModel.ts
--- a/app/api/odm/MultiTenantMongooseModel.ts
+++ b/app/api/odm/MultiTenantMongooseModel.ts
@@ -125,3 +125,4 @@
     const session = dbSessionContext.getSession();
-    return this.dbForCurrentTenant().deleteMany(query, {
+    const sanitizedQuery = { _id: { $eq: query._id } };
+    return this.dbForCurrentTenant().deleteMany(sanitizedQuery, {
       ...options,
EOF
@@ -125,3 +125,4 @@
const session = dbSessionContext.getSession();
return this.dbForCurrentTenant().deleteMany(query, {
const sanitizedQuery = { _id: { $eq: query._id } };
return this.dbForCurrentTenant().deleteMany(sanitizedQuery, {
...options,
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@daneryl daneryl force-pushed the saveEntity_process_transactional branch from ca4fae2 to f91c11d Compare January 31, 2025 12:59
@daneryl daneryl force-pushed the saveEntity_process_transactional branch from f91c11d to a6f4bd8 Compare February 3, 2025 05:22
@daneryl daneryl force-pushed the saveEntity_process_transactional branch from e96c82c to 76bf90c Compare February 4, 2025 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@daneryl