what happens if Object.defineProperty is unavailable, like in ES3 engines?

I've been thinking about that a bit. It might be good to check for the existence of Object.defineProperty and fall back to regular assignment.

This means it would still be possible to modify prototypes in pre-ES5 environments. That seems fine in practice. Some browsers will fall back, but Node won't have to. iojs and Node 0.8 have Object.defineProperty available.

(To be frank, Node 0.8 has bigger security concerns anyway)

should this not be undefined in that case?

Yes, definitely.

t.deepEqual(target.__proto__, { george: 1 }) would also work

For sure!

The reason for the var name = '__proto__' gymnastics: the linter discourages both target.__proto__ (no-proto) and target['__proto__'] (dot-notation). After some thought, I want to drop the gymnastics and use a comment to disable no-proto on that line.

Perhaps instead of this, we could do var malicious = { fred: 1 }; if (Object.defineProperty) { Object.defineProperty(malicious, '__proto__', { value: { george: 1 } }; }?

JSON.parse is just one way a malicious/surprising object could appear, but we should try to test this in earlier v8s too.

I like that a lot. It's doing something I didn't expect in 0.8, however:

> var malicious = { fred: 1 }
undefined
> Object.defineProperty(malicious, '__proto__', { value: { george: 1 }, enumerable: true })
{ fred: 1, [__proto__]: { george: 1 } }
> malicious.__proto__
{}
> malicious.hasOwnProperty('__proto__')
true
> Object.keys(malicious)
[ 'fred', '__proto__' ]
> malicious[Object.keys(malicious)[0]]
1
> malicious[Object.keys(malicious)[1]]
{}

(in newer Nodes, it does this instead):

> Object.defineProperty(malicious, '__proto__', { value: { george: 1 }, enumerable: true })
{ fred: 1, __proto__: { george: 1 } }
> malicious.__proto__
{ george: 1 }

interesting - it seems in node 0.8, Object.getOwnPropertyDescriptor(malicious, '__proto__').value gives the expected output. This seems to be a bug in these node versions; but it'd be fine to use this approach in the tests.

That's very interesting. It might not be fixable from the test side alone. The library code would also have to do copy = Object.getOwnPropertyDescriptor(options, name).value instead of copy = options[name] to get the real value of an own __proto__ on Node 0.8.

Pure druthers, I'd change the test to use Object.defineProperty() instead of JSON.parse(), but continue to skip this test case on 0.8 and 0.10.

Alternatively, this could go into the library and the test would pass on 0.8:

var getProperty = function getProperty(obj, name) {
	if (name === '__proto__') {
		if (!hasOwn.call(obj, name)) {
			return undefined;
		} else if (Object.getOwnPropertyDescriptor) {
			return Object.getOwnPropertyDescriptor(obj, name).value;
		}
	}

	return obj[name];
};

The performance characteristics of Object.getOwnPropertyDescriptor() aren't ideal, but I trust that branch wouldn't run often in production.

What do you think?

I don't think performance is particularly important here anyways :-) let's go for it.