use in to check for prototype violation

[gurgunday]

It's 36% faster and provides clearer intent

Benchmark

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[Uzlopak]

Well. hmm. why not just do const body = { __proto__ }

or more performant:

const Body = function Body () { }
Body.prototype = Object.create(null)


const body = new Body()

Then you can allow even "bad" names.

[gurgunday]

I really prefer the second one or simply doing body = Object.create(null)

WDYT?

[Uzlopak]

Object.create(null) is super slow.

imho this should be discussed. same as we did with delvedor/find-my-way#333

Well. hmm. i should write that benchmark i promised few weeks ago for find-my-way.

[gurgunday]

Wow, the performance difference is huge 😄

[Uzlopak]

@mcollina
@climba03003
@Eomm

How about making the body null prototype?

[climba03003]

in will look through the prototype chain, so a no in here

[gurgunday]

Yeah, but Object.prototype is already the last step of the prototype chain

Its prototype is null and immutable: Object.prototype.__proto__ === null

So equivalent to getOwnPropertyDescriptor in terms of behavior

[gurgunday]

Then you can allow even "bad" names.

@Uzlopak not sure anymore about that... I don't think it should be possible with multipart but if someone sneaks in a custom proto object then you certainly have prototype poisoning

The checks should stay even if we make it a null proto object

function NullObject () {}
NullObject.prototype = Object.create(null)
// {}
a = new NullObject()
// NullObject {}
a
// NullObject {}[[Prototype]]: ObjectNo properties
a['__proto__'] = {shh: 5}
// {shh: 5}
b = Object.assign({}, a)
// {}
b.shh
// 5

[Eomm]

How about making the body null prototype?

I think was already planned for fastify v5. isn't it?

[Uzlopak]

I think this was referring for parameters object. idk.

[gurgunday]

I think just to align the multipart parser with the default JSON parser, we should make req.body's prototype a null-prototype object in a future PR

[gurgunday]

Can I persuade someone to approve? 😁

To reiterate, Object.prototype cannot have a non-null __proto__ since it's the end of the prototype chain, so using in here is equivalent to getOwnPropertyDescriptor - but faster

This should be a free performance improvement 🤠

[Uzlopak]

Well. Do we had before this PR unit tests covering prototype pollution checks?

[gurgunday]

We have checks for proto pollution:

fastify-multipart/test/multipart-security.test.js

Lines 16 to 105 in 109a116

	test('should not allow __proto__ as file name', function (t) {
	t.plan(4)
	const fastify = Fastify()
	t.teardown(fastify.close.bind(fastify))
	fastify.register(multipart)
	fastify.post('/', async function (req, reply) {
	t.ok(req.isMultipart())
	try {
	await req.file()
	reply.code(200).send()
	} catch (error) {
	t.ok(error instanceof fastify.multipartErrors.PrototypeViolationError)
	reply.code(500).send()
	}
	})
	fastify.listen({ port: 0 }, async function () {
	// request
	const form = new FormData()
	const opts = {
	protocol: 'http:',
	hostname: 'localhost',
	port: fastify.server.address().port,
	path: '/',
	headers: form.getHeaders(),
	method: 'POST'
	}
	const req = http.request(opts, (res) => {
	t.equal(res.statusCode, 500)
	res.resume()
	res.on('end', () => {
	t.pass('res ended successfully')
	})
	})
	const rs = fs.createReadStream(filePath)
	form.append('__proto__', rs)
	form.pipe(req)
	})
	})
	test('should not allow __proto__ as field name', function (t) {
	t.plan(4)
	const fastify = Fastify()
	t.teardown(fastify.close.bind(fastify))
	fastify.register(multipart)
	fastify.post('/', async function (req, reply) {
	t.ok(req.isMultipart())
	try {
	await req.file()
	reply.code(200).send()
	} catch (error) {
	t.ok(error instanceof fastify.multipartErrors.PrototypeViolationError)
	reply.code(500).send()
	}
	})
	fastify.listen({ port: 0 }, async function () {
	// request
	const form = new FormData()
	const opts = {
	protocol: 'http:',
	hostname: 'localhost',
	port: fastify.server.address().port,
	path: '/',
	headers: form.getHeaders(),
	method: 'POST'
	}
	const req = http.request(opts, (res) => {
	t.equal(res.statusCode, 500)
	res.resume()
	res.on('end', () => {
	t.pass('res ended successfully')
	})
	})
	form.append('__proto__', 'world')
	form.pipe(req)
	})
	})

Let me add a few more

[Uzlopak]

Ah, i think there is also another issuer with using in. As @climba03003 wrote, it means we go up the prototype chain.

If we dont use a null object this means, that we use Object. Object has e.g.'hasOwnProperty, which is a set to enumerable false. So if you use Object.keys(body), you dont see it. So if multipart has a key named hasOwnProperty it is no issue, because we can overwrite it, as Object.prototype.hasOwnProperty.call(body, 'hasOwnProperty') returns false. And Object.keys(body) will contain hasOwnProperty.

If this PR lands, it will make it impossible to use keys like hasOwnProperty.

That is my assessment of @climba03003 remark. I am not at my PC to check it.

Can you check it please?

Maybe this can be done if we switch to NullObject body.

[gurgunday]

No, it is much more simpler than that

master (OLD) already blocks these, we are already checking if a name is IN Object.property by applying getOwnPropertyDescriptor directly on Object.prototype with the field name

For the prototype chain, ECMAScript defines that Object.property is the last step of the prototype chain, its prototype's value is null and cannot go further – as an example, try modifying Object.prototype.__proto__, it will result in an error

So instead of calling getOwnPropertyDescriptor, we are simply using the in keyword, which, yes looks at the prototype chain, but it's already the last step! It's completely the same in terms of semantics

[Uzlopak]

Ah, i see.

@climba03003
Can you please revise your review based on the comments of @gurgunday ?

[Uzlopak]

Approving, but please dont merge before we did not convinced @climba03003

[climba03003]

Yes, after a few test Object.prototype is allowed to modify.