From f296f6aa159752cb78b1906c7cd848be519d4a06 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:03:56 +0000 Subject: [PATCH] bug: fix lodash prototype pollution in PUT /chat (CWE-1321) Replace the _.merge of the client-supplied chat message with an allow-listed Object.assign copy, require canDelete to be an own property on the delete guard, and upgrade the vulnerable lodash 4.17.4 dependency to 4.17.21. --- package-lock.json | 14 ++-- package.json | 2 +- routes/index.js | 50 ++++++++++---- tests/chat.prototype-pollution.test.js | 95 ++++++++++++++++++++++++++ 4 files changed, 140 insertions(+), 21 deletions(-) create mode 100644 tests/chat.prototype-pollution.test.js diff --git a/package-lock.json b/package-lock.json index fef2be20353..feca5d35b03 100644 --- a/package-lock.json +++ b/package-lock.json @@ -25,7 +25,7 @@ "hbs": "^4.0.4", "humanize-ms": "1.0.1", "jquery": "^2.2.4", - "lodash": "4.17.4", + "lodash": "4.17.21", "marked": "0.3.5", "method-override": "latest", "moment": "2.15.1", @@ -4192,9 +4192,9 @@ } }, "node_modules/lodash": { - "version": "4.17.4", - "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.4.tgz", - "integrity": "sha1-eCA6TRwyiuHYbcpkYONptX9AVa4=" + "version": "4.17.21", + "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz", + "integrity": "sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==" }, "node_modules/lodash.assign": { "version": "4.2.0", @@ -16104,9 +16104,9 @@ } }, "lodash": { - "version": "4.17.4", - "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.4.tgz", - "integrity": "sha1-eCA6TRwyiuHYbcpkYONptX9AVa4=" + "version": "4.17.21", + "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz", + "integrity": "sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==" }, "lodash.assign": { "version": "4.2.0", diff --git a/package.json b/package.json index d5f9362a36d..83d7d03e918 100644 --- a/package.json +++ b/package.json @@ -31,7 +31,7 @@ "hbs": "^4.0.4", "humanize-ms": "1.0.1", "jquery": "^2.2.4", - "lodash": "4.17.4", + "lodash": "4.17.21", "marked": "0.3.5", "method-override": "latest", "moment": "2.15.1", diff --git a/routes/index.js b/routes/index.js index 6b5455f03e4..7271cff3f0b 100644 --- a/routes/index.js +++ b/routes/index.js @@ -16,8 +16,6 @@ var fileType = require('file-type'); var AdmZip = require('adm-zip'); var fs = require('fs'); -// prototype-pollution -var _ = require('lodash'); exports.index = function (req, res, next) { Todo. @@ -321,10 +319,34 @@ let messages = []; let lastId = 1; function findUser(auth) { + if (typeof auth.name !== 'string' || typeof auth.password !== 'string') { + return undefined; + } + return users.find((u) => u.name === auth.name && u.password === auth.password); } + +// Only these message fields may be supplied by the client, and only as strings. +const MESSAGE_FIELDS = ['text', 'icon']; + +function pickMessageFields(input) { + const picked = {}; + + if (input === null || typeof input !== 'object') { + return picked; + } + + MESSAGE_FIELDS.forEach((field) => { + if (Object.prototype.hasOwnProperty.call(input, field) && + typeof input[field] === 'string') { + picked[field] = input[field]; + } + }); + + return picked; +} /////////////////////////////////////////////////////////////////////////////// exports.chat = { @@ -339,16 +361,17 @@ exports.chat = { return; } - const message = { - // Default message icon. Cen be overwritten by user. - icon: '👋', - }; - - _.merge(message, req.body.message, { - id: lastId++, - timestamp: Date.now(), - userName: user.name, - }); + const message = Object.assign( + { + // Default message icon. Cen be overwritten by user. + icon: '👋', + }, + pickMessageFields(req.body.message), + { + id: lastId++, + timestamp: Date.now(), + userName: user.name, + }); messages.push(message); res.send({ ok: true }); @@ -356,7 +379,8 @@ exports.chat = { delete(req, res) { const user = findUser(req.body.auth || {}); - if (!user || !user.canDelete) { + if (!user || !Object.prototype.hasOwnProperty.call(user, 'canDelete') || + user.canDelete !== true) { res.status(403).send({ ok: false, error: 'Access denied' }); return; } diff --git a/tests/chat.prototype-pollution.test.js b/tests/chat.prototype-pollution.test.js new file mode 100644 index 00000000000..df43ae311d8 --- /dev/null +++ b/tests/chat.prototype-pollution.test.js @@ -0,0 +1,95 @@ +const test = require('tap').test; +const mongoose = require('mongoose'); + +// routes/index.js resolves the Todo/User models at require time. +mongoose.model('Todo', new mongoose.Schema({ content: String, updated_at: Date })); +mongoose.model('User', new mongoose.Schema({ username: String, password: String })); + +const routes = require('../routes'); + +const AUTH = { name: 'user', password: 'pwd' }; + +function fakeRes() { + return { + statusCode: 200, + body: undefined, + status(code) { + this.statusCode = code; + return this; + }, + send(body) { + this.body = body; + return this; + }, + }; +} + +test('chat.add ignores __proto__ in the message payload', (t) => { + const res = fakeRes(); + routes.chat.add({ + body: { + auth: AUTH, + // JSON.parse keeps __proto__ as an own key, exactly like a request body. + message: JSON.parse('{"text":"hi","__proto__":{"polluted":true,"canDelete":true}}'), + }, + }, res); + + t.same(res.body, { ok: true }); + t.equal({}.polluted, undefined, 'Object.prototype is not polluted'); + t.equal({}.canDelete, undefined, 'Object.prototype.canDelete is not set'); + t.end(); +}); + +test('chat.add ignores constructor.prototype in the message payload', (t) => { + const res = fakeRes(); + routes.chat.add({ + body: { + auth: AUTH, + message: { constructor: { prototype: { pollutedToo: true } } }, + }, + }, res); + + t.same(res.body, { ok: true }); + t.equal({}.pollutedToo, undefined, 'Object.prototype is not polluted'); + t.end(); +}); + +test('chat.add only copies allow-listed string fields', (t) => { + const res = fakeRes(); + routes.chat.add({ + body: { + auth: AUTH, + message: { text: 'hello', icon: 'x', id: 999, userName: 'admin', evil: 'nope' }, + }, + }, res); + + const getRes = fakeRes(); + routes.chat.get({}, getRes); + const message = getRes.body[getRes.body.length - 1]; + + t.equal(message.text, 'hello'); + t.equal(message.icon, 'x'); + t.equal(message.userName, 'user', 'userName is assigned server side'); + t.not(message.id, 999, 'id is assigned server side'); + t.equal(message.evil, undefined, 'unknown fields are dropped'); + t.end(); +}); + +test('chat.delete stays denied for the low privileged user', (t) => { + const res = fakeRes(); + routes.chat.delete({ body: { auth: AUTH, messageId: 1 } }, res); + + t.equal(res.statusCode, 403); + t.same(res.body, { ok: false, error: 'Access denied' }); + t.end(); +}); + +test('findUser rejects non string credentials', (t) => { + const res = fakeRes(); + routes.chat.add({ + body: { auth: { name: { $ne: null }, password: { $ne: null } }, message: {} }, + }, res); + + t.equal(res.statusCode, 403); + t.end(); +});