Add basic tests for prototype safety

[ChALkeR]

Language-specific implementation details shouldn't affect property existence checks.
Validators should not traverse JS prototype chain.

If a validator supports applying default values from the schema, it should pass with that both enabled or disabled.

Testing that might detect significant issues in e.g. JS implementations (although I attempted to target Lua as well, but that's completely untested against any Lua impl).

This test is generated by a generator of test 5 blocks, here: https://gist.github.com/ChALkeR/38c2753f9420feccbaac036b83bd51c0
Should I include the generator somehow, perhaps? If so, where should I place it?

[ChALkeR]

Included the generator in test-generator dir, as discussed in Slack. Self-contained.

[chalker@xps JSON-Schema-Test-Suite]$ ./test-generator/prototype-safe.js | shasum
d531471c5e2ca0cafc15da22018e65a972bcf40b  -
[chalker@xps JSON-Schema-Test-Suite]$ find . -name prototype-safe.json | xargs shasum
d531471c5e2ca0cafc15da22018e65a972bcf40b  ./tests/draft2019-09/prototype-safe.json
d531471c5e2ca0cafc15da22018e65a972bcf40b  ./tests/draft7/prototype-safe.json
d531471c5e2ca0cafc15da22018e65a972bcf40b  ./tests/draft4/prototype-safe.json
d531471c5e2ca0cafc15da22018e65a972bcf40b  ./tests/draft6/prototype-safe.json

[awwright]

Do we really need almost a thousand lines of tests (per directory) to test for this?

It seems reasonable to me to check for common bugs, even language-specific ones, but could we make do with a single test in properties.json?

[ChALkeR]

@awwright No, these test for a series of failure points, not just a single bug.

I can remove the ones that are targeting Lua though, as those weren't checked against any actual Lua validator and I'm not sure if they are useful.

The js ones found different issues in several js implementations though, I would prefer not to remove them.

[ChALkeR]

@awwright I removed Lua tests. This is now ~600 lines.

[ChALkeR]

@awwright This is a demonstration of how badly js impls fail on this: ChALkeR/json-schema-benchmark@0f1b068
One fails so badly that it affects other validators though global side-effects. In default configuration.

This was also a security vulnerability is ajv leading to code execution: https://github.com/ajv-validator/ajv/releases/tag/v6.12.3
I delayed this PR specifically until ajv was fixed enough so this wouldn't be a vulnerability anymore.

I know only two three js-based validators that pass -- schemasafe, hyperjump, and jassi. Of those, hyperjump fixed the issues because of the tests in this pr.

[awwright]

It seems like most of these tests are testing the same thing. For example, the outcome isn't going to change because you used an array instead of an object.

Is there any case where one of the tests failing is not predictive of the other tests failing? I.e. is it feasible to reduce this to a single schema being tested, with a positive and negative case, in properties.json?

Was the code execution because of how ajv compiles to ECMAScript code? I poked around with this idea some time ago and concluded since you can't store functions in JSON, it's pretty difficult do do anything malicious.

[awwright]

I mean, to reiterate my earlier point, I think it's OK to test for a common platform-specific or language-specific bug; and if you're adding a second test, it's either testing for something completely different, or it's testing for something that the first one is likely to pick up (in which case, it's not really testing for a "common" bug).

[ChALkeR]

Was the code execution because of how ajv compiles to ECMAScript code? I poked around with this idea some time ago and concluded since you can't store functions in JSON, it's pretty difficult do do anything malicious.

There were multiple issues in ajv each separately leading to code execution. One of them was directly related to this + useDefaults option -- that was enough to gain RCE.

Re: duplication -- if you check ChALkeR/json-schema-benchmark@0f1b068, multiple impls fail different subsets of these tests.
There might be e.g. checks specifically for certain property keys (like __proto__) -- that is not enough to make this work and hence other tests shouldn't be excluded.

I don't think this could be reduced to a single one -- that might slip some kind of errors through, which is might be dangerous given the security nature of this.

[ChALkeR]

@awwright It would be possible to merge this into one huge schema with subschemas for properties and test everything against it (in properties), but I don't see how would that be better than the current approach.

[ChALkeR]

Here, I split the commit linked above in four parts to make it more readable:
ChALkeR/json-schema-benchmark@e94479f...e991828

1. ChALkeR/json-schema-benchmark@032715f -- immediate action of this test
2. ChALkeR/json-schema-benchmark@84d43a6 -- first-order side effects
3. ChALkeR/json-schema-benchmark@bb4f07a -- one validator breaks other validators via second-order effects
4. ChALkeR/json-schema-benchmark@e991828 -- second-order side effects

There, it's easy to see in the first commit that different validators fail different subsets of these tests.

[ChALkeR]

@awwright Specifically, djv fails only the length test. It can't be removed.
For comparison, ajv doesn't fail length test but fails others.

I don't recommend removing any tests from here based on guesses how they should automatically pass if others pass.

[ChALkeR]

Here is a list of failures minus one validator.

Note that this is condensed, and some validators different tests inside them -- this is a list of schemas which caused failures.
Also this is just the Does not see block, Default value block is different (and causes different failures).

Expand

ajv	constructor	as number
ajv	constructor	as object
ajv	constructor	via required
ajv	__proto__	as number
ajv	__proto__	as object
ajv	__proto__	via required
ajv	toString	as number
ajv	toString	as object
ajv	toString	via required
@cfworker/json-schema	constructor	as number
@cfworker/json-schema	constructor	as object
@cfworker/json-schema	constructor	via required
@cfworker/json-schema	__proto__	as number
@cfworker/json-schema	__proto__	via required
@cfworker/json-schema	toString	as number
@cfworker/json-schema	toString	as object
@cfworker/json-schema	toString	via required
djv	length	as object
is-my-json-valid	constructor	as number
is-my-json-valid	constructor	as object
is-my-json-valid	constructor	via required
is-my-json-valid	length	as object
is-my-json-valid	__proto__	as number
is-my-json-valid	__proto__	as object
is-my-json-valid	__proto__	via required
is-my-json-valid	toString	as number
is-my-json-valid	toString	as object
is-my-json-valid	toString	via required
jjv	constructor	via required
jjv	length	via required
jjv	__proto__	via required
jjv	toString	via required
jjv	x	via required
jsck	constructor	via required
jsck	__proto__	via required
jsck	toString	via required
jsen	constructor	as number
jsen	constructor	as object
jsen	constructor	via required
jsen	__proto__	as number
jsen	__proto__	via required
jsen	toString	as number
jsen	toString	as object
jsen	toString	via required
json-model	constructor	as number
json-model	constructor	as object
json-model	constructor	via required
json-model	__proto__	as number
json-model	__proto__	as object
json-model	__proto__	via required
json-model	toString	as number
json-model	toString	as object
json-model	toString	via required
json-schema-library	constructor	as number
json-schema-library	constructor	as object
json-schema-library	constructor	via required
json-schema-library	__proto__	as number
json-schema-library	__proto__	via required
json-schema-library	toString	as number
json-schema-library	toString	as object
json-schema-library	toString	via required
jsonschema	constructor	via required
jsonschema	__proto__	via required
jsonschema	toString	via required
json-schema-validator-generator	constructor	as number
json-schema-validator-generator	constructor	as object
json-schema-validator-generator	constructor	via required
json-schema-validator-generator	length	via required
json-schema-validator-generator	__proto__	as number
json-schema-validator-generator	__proto__	via required
json-schema-validator-generator	toString	as number
json-schema-validator-generator	toString	as object
json-schema-validator-generator	toString	via required
json-schema-validator-generator	x	via required
JSV	constructor	as number
JSV	constructor	as object
JSV	constructor	via required
JSV	length	via required
JSV	__proto__	as number
JSV	__proto__	as object
JSV	__proto__	via required
JSV	toString	as number
JSV	toString	as object
JSV	toString	via required
JSV	x	via required
request-validator	constructor	as number
request-validator	constructor	as object
request-validator	constructor	via required
request-validator	__proto__	as number
request-validator	__proto__	as object
request-validator	__proto__	via required
request-validator	toString	as number
request-validator	toString	as object
request-validator	toString	via required
revalidator	constructor	as number
revalidator	constructor	as object
revalidator	constructor	via required
revalidator	length	as object
revalidator	length	via required
revalidator	__proto__	as number
revalidator	__proto__	as object
revalidator	__proto__	via required
revalidator	toString	as number
revalidator	toString	as object
revalidator	toString	via required
revalidator	x	via required
schemasaurus	constructor	as number
schemasaurus	constructor	as object
schemasaurus	length	as object
schemasaurus	__proto__	as number
schemasaurus	__proto__	as object
schemasaurus	toString	as number
schemasaurus	toString	as object
skeemas	constructor	as number
skeemas	constructor	as object
skeemas	constructor	via required
skeemas	__proto__	as number
skeemas	__proto__	via required
skeemas	toString	as number
skeemas	toString	as object
skeemas	toString	via required
themis	constructor	via required
themis	length	via required
themis	__proto__	via required
themis	toString	via required
themis	x	via required
tv4	constructor	via required
tv4	__proto__	via required
tv4	toString	via required
z-schema	constructor	via required
z-schema	__proto__	as number
z-schema	__proto__	as object
z-schema	__proto__	via required
z-schema	toString	via required

[awwright]

What does "via required" mean here?

[ChALkeR]

@awwright: It's a part of the test description

Generator:

• https://github.com/json-schema-org/JSON-Schema-Test-Suite/pull/414/files#diff-ead01d1d13a0064fb83121e5edd6a7abR109-R111
• https://github.com/json-schema-org/JSON-Schema-Test-Suite/pull/414/files#diff-ead01d1d13a0064fb83121e5edd6a7abR43-R60

Test json file:

• https://github.com/json-schema-org/JSON-Schema-Test-Suite/pull/414/files#diff-03509272e7e82a2031f5914474ad3282R44-R48

Seems that descriptions in Default value blocks are off, I'll fix that.
But that doesn't affect the list above, as that's from the Does not see blocks.
Upd: ah, not, it affects those. Will fix the table in a moment.
Upd2: fixed, thanks! List in #414 (comment) updated.

[jdesrosiers]

Do we really need almost a thousand lines of tests (per directory) to test for this?

This was my impression as well. When I ran these tests on my implementation, I had 56 errors. There were only three places in my code that needed changes. 56 test failures for 3 bugs indicates that there is a lot of redundancy in these tests.

However, just because these tests are redundant the way my implementation is designed, doesn't necessarily mean they are going to be redundant for another implementation that makes different design decisions. So, it's hard to say what the right balance is.

Although I'm grateful that these tests found bugs in my implementation, I don't think the official test suite should be this exhaustive for language specific tests. Every implementation should have their own tests in addition to the official suite that tests their API and the quirks of the language they are using.

My suggestion would be to add a few tests to the main suite that should cover most prototype safety issues. Three tests would have been sufficient for me and I didn't consider prototype safety at all when implementing, so I think 3-10 tests should be sufficient to catch all issues in all but the most poorly designed code. The full exhaustive set of tests (or at least the generation script) can be included as an optional suite for js implementations who want to be really really sure they're not missing anything.

[ChALkeR]

@jdesrosiers This now has 30 tests. For comparison, #385 introduces 172 tests.

Different implementations have different bugs, as can be seen in the list above.
Some fail only one or three tests. It would be hard to reduce this and something might get missed.

It's testing not one failure point but many, and as you mentioned, that was 3 bugs, not one, in a single impl.

[awwright]

@jdesrosiers Would you be able to show me the fix that you implemented?

[jdesrosiers]

@awwright

For the required keyword, the fix was simply to use hasOwnProperty instead of in. The other two places were using Object.create(null) instead of {} to create new objects. This creates an object with a null prototype which fixes all the issues with properties named __proto__.

[jdesrosiers]

@ChALkeR

This now has 30 tests.

This isn't 30 tests. There are 30 subjects that each have 12 tests.

For comparison, #385 introduces 172 tests.

Yep, I find that problematic as well.

It would be hard to reduce this and something might get missed.

You have all that data on which implementations are failing which tests. You should be able to use that data to determine which are the most valuable tests. Like I said, I'm not worried about catching every possible language specific error in the main test suite. I really don't think it should have hundreds of tests that only apply to JavaScript. Having a few of the most valuable tests is fine. Having the exhaustive set of tests available in a separate area (similar to "optional" tests) is fine too.

That's my opinion, but I don't care enough argue about it. Whatever you all decide to do with this is fine with me.

[awwright]

So it sounds like we can pick up on the vast majority of problems with 2-3 tests for ECMAScript and Lua each, then.

[Julian]

Perhaps there's an "easy" compromise to be made here -- think we just need to make a call.

Specifically:

• Add the 2-3 or however many tests that we think cover the majority of the problems directly to the suite
• Still merge and ship the script to generate the full spectrum of cases, such that "at-risk" implementations can run the generator and then run all of its outputted cases

Does that sound agreeable? Probably relevant for #385 as well if we make it "policy" for how we handle large numbers of generated duplicated tests.

[Julian]

Going to take the silence as "yes" :D -- @ChALkeR can you pick a smallish subset of these (e.g. 3 that cover 80% of the possible issues), submit those to the regular suite, and then obviously keep the generating script, but probably we need to document how to run it (and who should run it) in a sentence or two in the README?

[Julian]

Any chance anyone who benefited from these is willing to do ^ (pick 3-5)? Otherwise will likely pick whatever few using my less experienced Javascript eye so we can close & merge.

[Julian]

I've extracted a subset of these (10-15) and pushed them as 597b1fb (across all the drafts). Roughly I pared down to one schema per property, and combined them into one test case. If there's further improvements needed please speak up in a future issue or PR, otherwise closing this! Thanks for submitting them.