fix: only hexadecimal strings allowed if length of string is 24 #10010
Conversation
test/index.test.js
Outdated
| @@ -812,5 +812,26 @@ describe('mongoose module:', function() { | |||
| assert.ok(!movie.genre); | |||
| }); | |||
| }); | |||
| it('should prevent non-hexadecimal strings (gh-9996)', function() { | |||
| return co(function* () { | |||
There was a problem hiding this comment.
This test works just fine, I'd however change a few things about it to make the intention clear.
A) We don't need the connection, nor the saving here, we can get rid of them and test only isValidObjectId(...) since the changes are related to it.
B) I'd write explicit assertions instead of implicit ones (such as saving here), and show exactly why the bad string is bad, I need to read until the third from last character (t) to figure out why. Instead we could write something obviously bad such as 'z'.repeat(24).
Here's an example:
const badIdString = 'z'.repeat(24);
assert.deepStrictEqual(mongoose.isValidObjectId(badIdString), false);
const goodIdString = '1'.repeat(12);
assert.deepStrictEqual(mongoose.isValidObjectId(goodIdString), true);
There was a problem hiding this comment.
I agree with @AbdelrahmanHafez 's suggestions 👍
| if (typeof v === 'string' && v.length === 12) { | ||
| return true; | ||
| } | ||
| if (typeof v === 'string' && v.length === 24 && /^[a-f0-9]*$/.test(v)) { |
There was a problem hiding this comment.
You also need to add a similar check on line 992, there's another place where we check length === 24
No description provided.