Conversation
There was a problem hiding this comment.
Pull Request Overview
Refactors the large db-controller.js into multiple focused controller modules while maintaining existing exports and behavior.
- Splits controllers into 13 ES6 modules (utils, crud, delete, update, putUpdate, patchUpdate, patchSet, patchUnset, overwrite, bulk, history, release, gog)
- Updates
db-controller.jsto aggregate and re-export these modules - Adds a test for optimistic locking in
overwrite
Reviewed Changes
Copilot reviewed 14 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| db-controller-backup.js | Imports and re-exports all controller modules |
| controllers/utils.js | Core utility and helper functions |
| controllers/crud.js | Create and read operations |
| controllers/delete.js | Soft-delete and history healing |
| controllers/update.js | Aggregates update operations |
| controllers/putUpdate.js | PUT-based update and import |
| controllers/patchUpdate.js | PATCH update logic |
| controllers/patchSet.js | PATCH set logic |
| controllers/patchUnset.js | PATCH unset logic |
| controllers/overwrite.js | Overwrite with optimistic locking |
| controllers/bulk.js | Bulk create and update operations |
| controllers/history.js | History and HEAD request handlers |
| controllers/release.js | Release operations and tree management |
| controllers/gog.js | Gallery of Glosses endpoints |
Comments suppressed due to low confidence (1)
controllers/utils.js:73
- The
generateSlugIdhelper isn’t directly covered by existing tests. Adding unit tests for slug generation collision and error paths would improve confidence.
const generateSlugId = async function(slug_id="", next){
| const skip = parseInt(req.query.skip ?? 0) | ||
| let err = { message: `` } | ||
| // This request can only be made my Gallery of Glosses production apps. | ||
| if (!agentID === "61043ad4ffce846a83e700dd") { |
There was a problem hiding this comment.
The check !agentID === "..." always evaluates incorrectly. It should be agentID !== "61043ad4ffce846a83e700dd" to enforce the intended comparison.
| if (!agentID === "61043ad4ffce846a83e700dd") { | |
| if (agentID !== "61043ad4ffce846a83e700dd") { |
| const fragmentSet = new Set(witnessFragments) | ||
| witnessFragments = Array.from(fragmentSet.values()) |
There was a problem hiding this comment.
Using a Set on an array of objects won't dedupe by object content but by reference. Consider deduplicating by a unique key (e.g., @id) to avoid redundant entries.
| const fragmentSet = new Set(witnessFragments) | |
| witnessFragments = Array.from(fragmentSet.values()) | |
| const fragmentMap = new Map() | |
| witnessFragments.forEach(fragment => { | |
| if (fragment['@id']) { | |
| fragmentMap.set(fragment['@id'], fragment) | |
| } | |
| }) | |
| witnessFragments = Array.from(fragmentMap.values()) |
where credit is due
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return | ||
| } | ||
| try { | ||
| let matches = await db.find(props).limit(limit).skip(skip).toArray() |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, we need to ensure that the user-provided input (req.body) is sanitized or validated before being used in the database query. Specifically:
- Use MongoDB's
$eqoperator to ensure that the user input is treated as a literal value and not as a query object. - Alternatively, validate that
propscontains only literal values (e.g., strings, numbers) and does not include any special query operators.
The best approach in this case is to validate props to ensure it contains only literal values. This ensures that the query is safe while preserving the intended functionality.
| @@ -78,2 +78,13 @@ | ||
| } | ||
| // Validate props to ensure it contains only literal values | ||
| for (const key in props) { | ||
| if (typeof props[key] !== "string" && typeof props[key] !== "number" && typeof props[key] !== "boolean") { | ||
| let err = { | ||
| message: `Invalid value for key '${key}'. Only string, number, or boolean values are allowed.`, | ||
| status: 400 | ||
| } | ||
| next(createExpressError(err)) | ||
| return | ||
| } | ||
| } | ||
| try { |
| res.set("Content-Type", "application/json; charset=utf-8") | ||
| let props = req.body | ||
| try { | ||
| let matches = await db.find(props).toArray() |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the vulnerability, we need to ensure that the user-provided req.body is sanitized or validated before being used in the MongoDB query. The best approach is to enforce that props is a literal value and not a query object with special operators. This can be achieved by:
- Validating the structure of
req.bodyto ensure it contains only expected fields and values. - Using MongoDB's
$eqoperator to treat user input as literal values.
The fix involves modifying the queryHeadRequest function to validate req.body and use the $eq operator in the query. This ensures that untrusted input is interpreted as literal values, mitigating the risk of NoSQL injection.
| @@ -118,4 +118,16 @@ | ||
| let props = req.body | ||
| if (typeof props !== 'object' || Array.isArray(props)) { | ||
| let err = { | ||
| "message": "Invalid query format. Request body must be a JSON object.", | ||
| "status": 400 | ||
| } | ||
| next(createExpressError(err)) | ||
| return | ||
| } | ||
| try { | ||
| let matches = await db.find(props).toArray() | ||
| let sanitizedProps = {} | ||
| for (let key in props) { | ||
| sanitizedProps[key] = { $eq: props[key] } | ||
| } | ||
| let matches = await db.find(sanitizedProps).toArray() | ||
| if (matches.length) { |
| * This is the because some of the @ids have different RERUM URL patterns on them. | ||
| **/ | ||
| //All the children of this object will have its @id in __rerum.history.prime | ||
| ls_versions = await db.find({ "__rerum.history.prime": rootObj['@id'] }).toArray() |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, we need to ensure that the user-controlled data (rootObj['@id']) is sanitized or validated before being used in the MongoDB query. The best approach is to use the $eq operator to enforce that the input is treated as a literal value. Additionally, we can validate the input to ensure it is a string or a valid identifier before constructing the query.
Steps to fix:
- Modify the query in
getAllVersions(line 212 ofcontrollers/utils.js) to use the$eqoperator. - Add a validation step to ensure that
rootObj['@id']is a string or a valid identifier before using it in the query.
| @@ -211,3 +211,6 @@ | ||
| //All the children of this object will have its @id in __rerum.history.prime | ||
| ls_versions = await db.find({ "__rerum.history.prime": rootObj['@id'] }).toArray() | ||
| if (typeof rootObj['@id'] !== 'string') { | ||
| throw new Error("Invalid @id: must be a string"); | ||
| } | ||
| ls_versions = await db.find({ "__rerum.history.prime": { $eq: rootObj['@id'] } }).toArray() | ||
| //The root object is a version, prepend it in |
| safe_descendant.__rerum.releases.previous = releasing["@id"] | ||
| let result | ||
| try { | ||
| result = await db.replaceOne({ "_id": d_id }, safe_descendant) |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, we need to ensure that the _id field used in the query object { "_id": d_id } is sanitized or validated to prevent NoSQL injection. The best approach is to use MongoDB's $eq operator to ensure that the value is treated as a literal and not as a query object. Additionally, we should validate that d_id is a string or a valid ObjectID before using it in the query.
Changes will be made in the establishReleasesTree function in controllers/utils.js to:
- Use the
$eqoperator in the query object. - Validate that
d_idis a string or a valid ObjectID.
| @@ -311,2 +311,6 @@ | ||
| let d_id = safe_descendant._id | ||
| if (typeof d_id !== "string" && !ObjectID.isValid(d_id)) { | ||
| console.error("Invalid _id detected:", d_id) | ||
| return false | ||
| } | ||
| safe_descendant.__rerum.releases.previous = releasing["@id"] | ||
| @@ -314,3 +318,3 @@ | ||
| try { | ||
| result = await db.replaceOne({ "_id": d_id }, safe_descendant) | ||
| result = await db.replaceOne({ "_id": { $eq: d_id } }, safe_descendant) | ||
| } |
| } | ||
| let result | ||
| try { | ||
| result = await db.replaceOne({ "_id": a_id }, safe_ancestor) |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, we need to ensure that the user-controlled input is sanitized or validated before being used in the MongoDB query. Specifically:
- Use MongoDB's
$eqoperator to ensure that the_idfield is treated as a literal value and not as a query object. - Alternatively, validate the
_idfield to ensure it is a valid string or ObjectID before using it in the query.
The best approach in this case is to use the $eq operator in the query object. This ensures that the input is treated as a literal value, mitigating the risk of NoSQL injection.
| @@ -314,3 +314,3 @@ | ||
| try { | ||
| result = await db.replaceOne({ "_id": d_id }, safe_descendant) | ||
| result = await db.replaceOne({ "_id": { $eq: d_id } }, safe_descendant) | ||
| } | ||
| @@ -334,3 +334,3 @@ | ||
| try { | ||
| result = await db.replaceOne({ "_id": a_id }, safe_ancestor) | ||
| result = await db.replaceOne({ "_id": { $eq: a_id } }, safe_ancestor) | ||
| } |
| } | ||
| let result | ||
| try { | ||
| result = await db.replaceOne({ "_id": d_id }, safe_descendant) |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, we need to ensure that the d_id value used in the query object { "_id": d_id } is properly validated or sanitized. Since MongoDB queries are susceptible to NoSQL injection, we can use the $eq operator to ensure that the _id field is treated as a literal value. This approach prevents malicious input from being interpreted as a query object.
Steps to implement the fix:
- Modify the query on line 384 in
controllers/utils.jsto use the$eqoperator for the_idfield. - Ensure that the
d_idvalue is validated to confirm it is a valid MongoDB ObjectID or a string, depending on the expected format.
| @@ -383,3 +383,3 @@ | ||
| try { | ||
| result = await db.replaceOne({ "_id": d_id }, safe_descendant) | ||
| result = await db.replaceOne({ "_id": { $eq: d_id } }, safe_descendant) | ||
| } |
| safe_ancestor.__rerum.releases.next = ancestorNextArray | ||
| let result | ||
| try { | ||
| result = await db.replaceOne({ "_id": a_id }, safe_ancestor) |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
|
boo: |
|
Looks like we're catching 500 instead of 409... |
Great news team. I think this is actually a Tiny problem™ and not the RERUM errors: fetch(OVERWRITE_URL, {
method: 'PUT',
body: JSON.stringify(obj),
headers: {
'Content-Type': 'application/json; charset=utf-8'
}
})
.then(response => {
if (response.ok) { return response.json() }
throw response
})
.then(resultObj => {
delete resultObj.new_obj_state
_customEvent("rerum-result", `URI ${uri} overwritten. See resulting object below:`, resultObj)
})
.catch(err => {
_customEvent("rerum-error", "There was an error trying to overwrite object at " + uri, {}, err)
}) |
|
recall we changed TinyPen, but not TinyThings |
|
Success:
|
thehabes
left a comment
thehabes
left a comment
There was a problem hiding this comment.
Manually tested using TinyNode. Did a limited amount of looking through the new controllers to make sure it copied the code out correctly, did not notice any errors while using it.
🎯 Refactor monolithic db-controller.js into modular controller structure
📋 Summary
This PR addresses the technical debt of the large, monolithic
db-controller.jsfile by breaking it down into smaller, logically grouped modules. The refactoring improves code maintainability, readability, and follows separation of concerns principles.🔄 Changes Made
New Controller Modules Created:
controllers/utils.js- Utility functions (idNegotiation, generateSlugId, etc.)controllers/crud.js- CRUD operations (create, query, id)controllers/delete.js- Delete operations and history helperscontrollers/update.js- Update operations aggregatorcontrollers/putUpdate.js- PUT update and import operationscontrollers/patchUpdate.js- PATCH update operationscontrollers/patchSet.js- PATCH set operationscontrollers/patchUnset.js- PATCH unset operationscontrollers/overwrite.js- Overwrite operations with optimistic lockingcontrollers/bulk.js- Bulk operations (bulkCreate, bulkUpdate)controllers/history.js- History operations (since, history, head requests)controllers/release.js- Release operationscontrollers/gog.js- Gallery of Glosses endpointsUpdated Files:
db-controller.js- Now serves as a clean aggregator that imports and re-exports from new modulesroutes/__tests__/overwrite-optimistic-locking.test.js- Added test for optimistic lockingBackup Files:
db-controller-backup.js- Complete backup of original filedb-controller.js.backup- Additional backup for safety🎯 Key Benefits
🔒 Backward Compatibility
🧪 Testing
📁 File Structure
🚀 Technical Implementation
👨💻 Contributors
🔍 Review Notes
This refactoring maintains 100% backward compatibility while significantly improving code organization. The changes are primarily structural - moving code from one large file into multiple smaller, focused files without altering the core business logic.
Ready for Review! 🎉