Skip to content

Commit 8d44a04

Browse files
bewestCopilot
bewest
and
Copilot
committed
feat(devicestatus): return 400 for invalid _id format
Add validation for _id field in devicestatus API: - POST: validates each document's _id before storage - DELETE: validates _id parameter (allows wildcard '*') Accepts: undefined, null, or 24-character hex string Rejects: UUIDs, short strings, numbers, objects with 400 Bad Request Previously, invalid _id values were silently stored as strings instead of ObjectIds, causing inconsistent data and query issues. Tests added for all validation cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 32b1d70 commit 8d44a04

3 files changed

Lines changed: 216 additions & 14 deletions

File tree

‎lib/api/devicestatus/index.js‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,33 @@ const { query } = require('express');
66
const _take = require('lodash/take');
77
const _ = require('lodash');
88

9+
/**
10+
* Validate MongoDB ObjectId format.
11+
* Accepts: undefined, null, or 24-character hex string.
12+
* Rejects: anything else (UUIDs, short strings, numbers, objects).
13+
* @param {*} id - The _id value to validate
14+
* @returns {boolean} - true if valid or empty, false if invalid format
15+
*/
16+
function isValidObjectId(id) {
17+
if (id === undefined || id === null) return true; // Will auto-generate
18+
if (typeof id !== 'string') return false;
19+
return /^[a-fA-F0-9]{24}$/.test(id);
20+
}
21+
22+
/**
23+
* Validate _id field for each document in an array.
24+
* @param {Array} docs - Array of documents to validate
25+
* @returns {Object|null} - null if all valid, or {index, id} of first invalid
26+
*/
27+
function findInvalidId(docs) {
28+
for (var i = 0; i < docs.length; i++) {
29+
if (!isValidObjectId(docs[i]._id)) {
30+
return { index: i, id: docs[i]._id };
31+
}
32+
}
33+
return null;
34+
}
35+
936
function configure (app, wares, ctx, env) {
1037
var express = require('express')
1138
, api = express.Router();
@@ -74,6 +101,13 @@ function configure (app, wares, ctx, env) {
74101
statuses = [statuses];
75102
}
76103

104+
// Validate _id fields before storage (return 400 on invalid)
105+
var invalid = findInvalidId(statuses);
106+
if (invalid) {
107+
return res.sendJSONStatus(res, consts.HTTP_BAD_REQUEST,
108+
'Invalid _id format', 'Must be 24-character hex string or omit for auto-generation. Got: ' + String(invalid.id));
109+
}
110+
77111
// Purify each devicestatus object
78112
for (var i = 0; i < statuses.length; i++) {
79113
ctx.purifier.purifyObject(statuses[i]);
@@ -119,6 +153,12 @@ function configure (app, wares, ctx, env) {
119153
}
120154

121155
api.delete('/devicestatus/:id', ctx.authorization.isPermitted('api:devicestatus:delete'), function(req, res, next) {
156+
// Validate _id parameter (unless wildcard)
157+
if (req.params.id !== '*' && !isValidObjectId(req.params.id)) {
158+
return res.sendJSONStatus(res, consts.HTTP_BAD_REQUEST,
159+
'Invalid _id format', 'Must be 24-character hex string. Got: ' + String(req.params.id));
160+
}
161+
122162
if (!req.query.find) {
123163
req.query.find = {
124164
_id: req.params.id

‎tests/api.devicestatus.test.js‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,4 +96,157 @@ describe('Devicestatus API', function ( ) {
9696
}
9797
});
9898
});
99+
100+
// _id validation tests (prevent silent data corruption and ensure 400 on invalid)
101+
describe('_id validation', function() {
102+
103+
it('should return 400 for POST with invalid UUID _id', function(done) {
104+
var status_with_uuid = {
105+
"_id": "my-uuid-12345",
106+
"device": "test-device",
107+
"created_at": "2024-01-01T00:00:00Z"
108+
};
109+
110+
request(self.app)
111+
.post('/api/devicestatus/')
112+
.set('api-secret', known || '')
113+
.send(status_with_uuid)
114+
.expect(400)
115+
.expect(function(response) {
116+
response.body.should.have.property('status', 400);
117+
response.body.should.have.property('message');
118+
response.body.message.should.match(/Invalid _id format/i);
119+
})
120+
.end(done);
121+
});
122+
123+
it('should return 400 for POST with short _id', function(done) {
124+
var status_short_id = {
125+
"_id": "abc",
126+
"device": "test-device",
127+
"created_at": "2024-01-01T00:00:00Z"
128+
};
129+
130+
request(self.app)
131+
.post('/api/devicestatus/')
132+
.set('api-secret', known || '')
133+
.send(status_short_id)
134+
.expect(400)
135+
.end(done);
136+
});
137+
138+
it('should return 400 for DELETE with invalid _id', function(done) {
139+
request(self.app)
140+
.delete('/api/devicestatus/invalid-uuid-here')
141+
.set('api-secret', known || '')
142+
.expect(400)
143+
.expect(function(response) {
144+
response.body.message.should.match(/Invalid _id format/i);
145+
})
146+
.end(done);
147+
});
148+
149+
it('should accept POST with valid 24-hex _id', function(done) {
150+
// Use a unique ID that doesn't conflict with other tests
151+
var testId = 'bbbbbbbbbbbbbbbbbbbbbbbb';
152+
var status_valid_id = {
153+
"_id": testId,
154+
"device": "test-device-valid",
155+
"created_at": "2024-01-02T00:00:00Z"
156+
};
157+
158+
// First, try to delete any existing document with this _id (cleanup from previous runs)
159+
request(self.app)
160+
.delete('/api/devicestatus/' + testId)
161+
.set('api-secret', known || '')
162+
.end(function() {
163+
// Ignore errors (document may not exist)
164+
request(self.app)
165+
.post('/api/devicestatus/')
166+
.set('api-secret', known || '')
167+
.send(status_valid_id)
168+
.expect(200)
169+
.expect(function(response) {
170+
response.body.should.be.an.Array();
171+
response.body.length.should.equal(1);
172+
response.body[0]._id.should.equal(testId);
173+
})
174+
.end(function(err) {
175+
if (err) return done(err);
176+
// Clean up
177+
request(self.app)
178+
.delete('/api/devicestatus/' + testId)
179+
.set('api-secret', known || '')
180+
.expect(200)
181+
.end(done);
182+
});
183+
});
184+
});
185+
186+
it('should accept POST without _id (auto-generate)', function(done) {
187+
var status_no_id = {
188+
"device": "test-device-autogen",
189+
"created_at": "2024-01-03T00:00:00Z"
190+
};
191+
192+
request(self.app)
193+
.post('/api/devicestatus/')
194+
.set('api-secret', known || '')
195+
.send(status_no_id)
196+
.expect(200)
197+
.expect(function(response) {
198+
response.body.should.be.an.Array();
199+
response.body.length.should.equal(1);
200+
response.body[0].should.have.property('_id');
201+
// Verify auto-generated _id is valid ObjectId format
202+
response.body[0]._id.toString().should.match(/^[a-fA-F0-9]{24}$/);
203+
})
204+
.end(function(err, res) {
205+
if (err) return done(err);
206+
// Clean up
207+
var createdId = res.body[0]._id;
208+
request(self.app)
209+
.delete('/api/devicestatus/' + createdId)
210+
.set('api-secret', known || '')
211+
.expect(200)
212+
.end(done);
213+
});
214+
});
215+
216+
it('should return 400 for array POST with one invalid _id', function(done) {
217+
var statuses_mixed = [
218+
{ "device": "device1", "created_at": "2024-01-01T00:00:00Z" },
219+
{ "_id": "bad-uuid", "device": "device2", "created_at": "2024-01-02T00:00:00Z" }
220+
];
221+
222+
request(self.app)
223+
.post('/api/devicestatus/')
224+
.set('api-secret', known || '')
225+
.send(statuses_mixed)
226+
.expect(400)
227+
.expect(function(response) {
228+
response.body.message.should.match(/Invalid _id format/i);
229+
})
230+
.end(done);
231+
});
232+
233+
it('should allow DELETE with wildcard _id', function(done) {
234+
// First insert a test record
235+
request(self.app)
236+
.post('/api/devicestatus/')
237+
.set('api-secret', known || '')
238+
.send({ "device": "delete-wildcard-test", "created_at": "2020-01-01T00:00:00Z" })
239+
.expect(200)
240+
.end(function(err) {
241+
if (err) return done(err);
242+
// Wildcard delete with date filter should work
243+
request(self.app)
244+
.delete('/api/devicestatus/*')
245+
.query('find[created_at][$lte]=2020-01-02')
246+
.set('api-secret', known || '')
247+
.expect(200)
248+
.end(done);
249+
});
250+
});
251+
});
99252
});

‎tests/api.profiles.test.js‎

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -213,31 +213,40 @@ describe('Profiles API', function ( ) {
213213
});
214214

215215
it('should accept POST with valid 24-hex _id', function(done) {
216+
// Use a unique ID that doesn't conflict with other tests
217+
var testId = 'aaaaaaaaaaaaaaaaaaaaaaaa';
216218
var profile_valid_id = {
217-
"_id": "507f1f77bcf86cd799439011",
219+
"_id": testId,
218220
"defaultProfile": "Default",
219221
"store": { "Default": { "dia": 3 } },
220222
"startDate": "2024-10-19T23:00:00.000Z"
221223
};
222224

225+
// First, try to delete any existing document with this _id (cleanup from previous runs)
223226
request(self.app)
224-
.post('/api/profile/')
227+
.delete('/api/profile/' + testId)
225228
.set('api-secret', known || '')
226-
.send(profile_valid_id)
227-
.expect(200)
228-
.expect(function(response) {
229-
response.body.should.be.an.Array();
230-
response.body.length.should.equal(1);
231-
response.body[0]._id.should.equal('507f1f77bcf86cd799439011');
232-
})
233-
.end(function(err) {
234-
if (err) return done(err);
235-
// Clean up: delete the profile we just created
229+
.end(function() {
230+
// Ignore errors (document may not exist)
236231
request(self.app)
237-
.delete('/api/profile/507f1f77bcf86cd799439011')
232+
.post('/api/profile/')
238233
.set('api-secret', known || '')
234+
.send(profile_valid_id)
239235
.expect(200)
240-
.end(done);
236+
.expect(function(response) {
237+
response.body.should.be.an.Array();
238+
response.body.length.should.equal(1);
239+
response.body[0]._id.should.equal(testId);
240+
})
241+
.end(function(err) {
242+
if (err) return done(err);
243+
// Clean up: delete the profile we just created
244+
request(self.app)
245+
.delete('/api/profile/' + testId)
246+
.set('api-secret', known || '')
247+
.expect(200)
248+
.end(done);
249+
});
241250
});
242251
});
243252

0 commit comments

Comments
 (0)