From 3306137d204c6eff6ed7e3217ae8c6ca709f943e Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Fri, 4 Apr 2025 19:55:56 -0500 Subject: [PATCH 1/6] feat: add secure JSON parsing with prototype poisoning handling --- lib/types/json.js | 4 +++- package.json | 1 + test/json.js | 26 ++++++++++++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/lib/types/json.js b/lib/types/json.js index 078ce710..60ba6bcf 100644 --- a/lib/types/json.js +++ b/lib/types/json.js @@ -18,6 +18,7 @@ var isFinished = require('on-finished').isFinished var read = require('../read') var typeis = require('type-is') var { getCharset, normalizeOptions } = require('../utils') +const secureJson = require('secure-json-parse') /** * Module exports. @@ -55,6 +56,7 @@ function json (options) { var reviver = options?.reviver var strict = options?.strict !== false + const protoPoisoning = options?.onProtoPoisoning || 'ignore' function parse (body) { if (body.length === 0) { @@ -74,7 +76,7 @@ function json (options) { try { debug('parse json') - return JSON.parse(body, reviver) + return secureJson.parse(body, reviver, { protoAction: protoPoisoning }) } catch (e) { throw normalizeJsonSyntaxError(e, { message: e.message, diff --git a/package.json b/package.json index b5dc8303..5d5bebf6 100644 --- a/package.json +++ b/package.json @@ -17,6 +17,7 @@ "on-finished": "^2.4.1", "qs": "^6.14.0", "raw-body": "^3.0.0", + "secure-json-parse": "^4.0.0", "type-is": "^2.0.1" }, "devDependencies": { diff --git a/test/json.js b/test/json.js index e679ac57..00e232ca 100644 --- a/test/json.js +++ b/test/json.js @@ -721,6 +721,32 @@ describe('bodyParser.json()', function () { test.expect(413, done) }) }) + + describe("prototype poisoning", function () { + it('should parse __proto__ when protoAction is set to ignore', function (done) { + request(createServer({ onProtoPoisoning: 'ignore' })) + .post('/') + .set('Content-Type', 'application/json') + .send('{"user":"tobi","__proto__":{"x":7}}') + .expect(200, '{"user":"tobi","__proto__":{"x":7}}', done) + }) + + it('should throw when protoAction is set to error', function (done) { + request(createServer({ onProtoPoisoning: 'error' })) + .post('/') + .set('Content-Type', 'application/json') + .send('{"user":"tobi","__proto__":{"x":7}}') + .expect(400, '[entity.parse.failed] Object contains forbidden prototype property', done) + }) + + it('should remove prototype poisoning when protoAction is set to remove', function (done) { + request(createServer({ onProtoPoisoning: 'remove' })) + .post('/') + .set('Content-Type', 'application/json') + .send('{"user":"tobi","__proto__":{"x":7}}') + .expect(200, '{"user":"tobi"}', done) + }) + }) }) function createServer (opts) { From 45f30e1425e96ebef8bca91e562d90dcdac1194f Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Fri, 4 Apr 2025 20:07:23 -0500 Subject: [PATCH 2/6] fix: downgrade secure-json-parse to version 3.0.2 for compatibility --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 5d5bebf6..9f75bd8c 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,7 @@ "on-finished": "^2.4.1", "qs": "^6.14.0", "raw-body": "^3.0.0", - "secure-json-parse": "^4.0.0", + "secure-json-parse": "^3.0.2", "type-is": "^2.0.1" }, "devDependencies": { From 6371f5b4aabc41a6367490655652df4ea6c670b1 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Fri, 4 Apr 2025 20:19:34 -0500 Subject: [PATCH 3/6] docs: add documentation for onProtoPoisoning option in README --- README.md | 4 ++++ test/json.js | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 9fcd4c6f..6a2a253e 100644 --- a/README.md +++ b/README.md @@ -116,6 +116,10 @@ The `verify` option, if supplied, is called as `verify(req, res, buf, encoding)` where `buf` is a `Buffer` of the raw request body and `encoding` is the encoding of the request. The parsing can be aborted by throwing an error. +##### onProtoPoisoning + +Defines what action must be taken when parsing a JSON object with `__proto__` + ### bodyParser.raw([options]) Returns middleware that parses all bodies as a `Buffer` and only looks at diff --git a/test/json.js b/test/json.js index 00e232ca..fdc3540b 100644 --- a/test/json.js +++ b/test/json.js @@ -722,7 +722,7 @@ describe('bodyParser.json()', function () { }) }) - describe("prototype poisoning", function () { + describe('prototype poisoning', function () { it('should parse __proto__ when protoAction is set to ignore', function (done) { request(createServer({ onProtoPoisoning: 'ignore' })) .post('/') @@ -738,7 +738,7 @@ describe('bodyParser.json()', function () { .send('{"user":"tobi","__proto__":{"x":7}}') .expect(400, '[entity.parse.failed] Object contains forbidden prototype property', done) }) - + it('should remove prototype poisoning when protoAction is set to remove', function (done) { request(createServer({ onProtoPoisoning: 'remove' })) .post('/') From 6b4e150240ff9dd378600386392fe0c1b7d8941b Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sat, 26 Apr 2025 19:57:59 -0500 Subject: [PATCH 4/6] feat: enhance JSON parsing options to include constructor poisoning handling Signed-off-by: Sebastian Beltran --- lib/types/json.js | 4 ++-- test/json.js | 32 +++++++++++++++++++++++++++++--- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/lib/types/json.js b/lib/types/json.js index 60ba6bcf..a8e4723c 100644 --- a/lib/types/json.js +++ b/lib/types/json.js @@ -56,7 +56,7 @@ function json (options) { var reviver = options?.reviver var strict = options?.strict !== false - const protoPoisoning = options?.onProtoPoisoning || 'ignore' + const poisoningOptions = { onProtoPoisoning: options?.onProto?.onProtoPoisoning || 'ignore', onConstructorPoisoning: options?.onProto?.onConstructorPoisoning || 'ignore' } function parse (body) { if (body.length === 0) { @@ -76,7 +76,7 @@ function json (options) { try { debug('parse json') - return secureJson.parse(body, reviver, { protoAction: protoPoisoning }) + return secureJson.parse(body, reviver, { protoAction: poisoningOptions.onProtoPoisoning, constructorAction: poisoningOptions.onConstructorPoisoning }) } catch (e) { throw normalizeJsonSyntaxError(e, { message: e.message, diff --git a/test/json.js b/test/json.js index fdc3540b..15f146e1 100644 --- a/test/json.js +++ b/test/json.js @@ -724,7 +724,7 @@ describe('bodyParser.json()', function () { describe('prototype poisoning', function () { it('should parse __proto__ when protoAction is set to ignore', function (done) { - request(createServer({ onProtoPoisoning: 'ignore' })) + request(createServer({ onProto: { onProtoPoisoning: 'ignore' }})) .post('/') .set('Content-Type', 'application/json') .send('{"user":"tobi","__proto__":{"x":7}}') @@ -732,7 +732,7 @@ describe('bodyParser.json()', function () { }) it('should throw when protoAction is set to error', function (done) { - request(createServer({ onProtoPoisoning: 'error' })) + request(createServer({ onProto: { onProtoPoisoning: 'error' }})) .post('/') .set('Content-Type', 'application/json') .send('{"user":"tobi","__proto__":{"x":7}}') @@ -740,13 +740,39 @@ describe('bodyParser.json()', function () { }) it('should remove prototype poisoning when protoAction is set to remove', function (done) { - request(createServer({ onProtoPoisoning: 'remove' })) + request(createServer({ onProto: { onProtoPoisoning: 'remove' }})) .post('/') .set('Content-Type', 'application/json') .send('{"user":"tobi","__proto__":{"x":7}}') .expect(200, '{"user":"tobi"}', done) }) }) + + describe('constructor poisoning', function () { + it('should parse constructor when protoAction is set to ignore', function (done) { + request(createServer({ onProto: { onConstructorPoisoning: 'ignore' }})) + .post('/') + .set('Content-Type', 'application/json') + .send('{"user":"tobi","constructor":{"prototype":{"bar":"baz"}}}') + .expect(200, '{"user":"tobi","constructor":{"prototype":{"bar":"baz"}}}', done) + }) + + it('should throw when protoAction is set to error', function (done) { + request(createServer({ onProto: { onConstructorPoisoning: 'error' }})) + .post('/') + .set('Content-Type', 'application/json') + .send('{"user":"tobi","constructor":{"prototype":{"bar":"baz"}}}') + .expect(400, '[entity.parse.failed] Object contains forbidden prototype property', done) + }) + + it('should remove prototype poisoning when protoAction is set to remove', function (done) { + request(createServer({ onProto: { onConstructorPoisoning: 'remove' }})) + .post('/') + .set('Content-Type', 'application/json') + .send('{"user":"tobi","constructor":{"prototype":{"bar":"baz"}}}') + .expect(200, '{"user":"tobi"}', done) + }) + }) }) function createServer (opts) { From 548f2c79965ca550f30486583dd837abf4f45767 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sat, 26 Apr 2025 20:02:06 -0500 Subject: [PATCH 5/6] test: fix formatting in prototype and constructor poisoning tests --- test/json.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/json.js b/test/json.js index 15f146e1..2a760f5a 100644 --- a/test/json.js +++ b/test/json.js @@ -724,7 +724,7 @@ describe('bodyParser.json()', function () { describe('prototype poisoning', function () { it('should parse __proto__ when protoAction is set to ignore', function (done) { - request(createServer({ onProto: { onProtoPoisoning: 'ignore' }})) + request(createServer({ onProto: { onProtoPoisoning: 'ignore' } })) .post('/') .set('Content-Type', 'application/json') .send('{"user":"tobi","__proto__":{"x":7}}') @@ -732,7 +732,7 @@ describe('bodyParser.json()', function () { }) it('should throw when protoAction is set to error', function (done) { - request(createServer({ onProto: { onProtoPoisoning: 'error' }})) + request(createServer({ onProto: { onProtoPoisoning: 'error' } })) .post('/') .set('Content-Type', 'application/json') .send('{"user":"tobi","__proto__":{"x":7}}') @@ -740,7 +740,7 @@ describe('bodyParser.json()', function () { }) it('should remove prototype poisoning when protoAction is set to remove', function (done) { - request(createServer({ onProto: { onProtoPoisoning: 'remove' }})) + request(createServer({ onProto: { onProtoPoisoning: 'remove' } })) .post('/') .set('Content-Type', 'application/json') .send('{"user":"tobi","__proto__":{"x":7}}') @@ -750,7 +750,7 @@ describe('bodyParser.json()', function () { describe('constructor poisoning', function () { it('should parse constructor when protoAction is set to ignore', function (done) { - request(createServer({ onProto: { onConstructorPoisoning: 'ignore' }})) + request(createServer({ onProto: { onConstructorPoisoning: 'ignore' } })) .post('/') .set('Content-Type', 'application/json') .send('{"user":"tobi","constructor":{"prototype":{"bar":"baz"}}}') @@ -758,7 +758,7 @@ describe('bodyParser.json()', function () { }) it('should throw when protoAction is set to error', function (done) { - request(createServer({ onProto: { onConstructorPoisoning: 'error' }})) + request(createServer({ onProto: { onConstructorPoisoning: 'error' } })) .post('/') .set('Content-Type', 'application/json') .send('{"user":"tobi","constructor":{"prototype":{"bar":"baz"}}}') @@ -766,7 +766,7 @@ describe('bodyParser.json()', function () { }) it('should remove prototype poisoning when protoAction is set to remove', function (done) { - request(createServer({ onProto: { onConstructorPoisoning: 'remove' }})) + request(createServer({ onProto: { onConstructorPoisoning: 'remove' } })) .post('/') .set('Content-Type', 'application/json') .send('{"user":"tobi","constructor":{"prototype":{"bar":"baz"}}}') From 78926a7e47053123c96c4194585e3a30e3acc935 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sat, 26 Apr 2025 20:07:41 -0500 Subject: [PATCH 6/6] update docs --- README.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 6a2a253e..b2ebf67f 100644 --- a/README.md +++ b/README.md @@ -116,10 +116,16 @@ The `verify` option, if supplied, is called as `verify(req, res, buf, encoding)` where `buf` is a `Buffer` of the raw request body and `encoding` is the encoding of the request. The parsing can be aborted by throwing an error. -##### onProtoPoisoning +##### onProto + +###### onProtoPoisoning Defines what action must be taken when parsing a JSON object with `__proto__` +###### onConstructorPoisoning + +Defines what action must be taken when parsing a JSON object with `constructor.prototype` key + ### bodyParser.raw([options]) Returns middleware that parses all bodies as a `Buffer` and only looks at