fix: do not treat Object.prototype properties as present in the data - #2665
Open
maximilliangrand wants to merge 1 commit into
Open
fix: do not treat Object.prototype properties as present in the data#2665maximilliangrand wants to merge 1 commit into
maximilliangrand wants to merge 1 commit into
Conversation
`required`, `properties` and `dependencies` detect a property with
`data[prop] !== undefined`, which is true for every object that inherits
from Object.prototype when the property name is `toString`, `constructor`,
`__proto__`, `valueOf` etc. As a result `{"required": ["toString"]}` was
satisfied by `{}` and `{"properties": {"constructor": {"type": "number"}}}`
rejected `{}` (ajv-validator#1045).
Properties of Object.prototype are not enumerable, so under the documented
`ownProperties: false` semantics ("iterates over all enumerable object
properties") they should never be considered present unless they are own
properties of the data.
Presence of a property with one of these names is now checked with a runtime
function that returns true for an own property or an inherited enumerable
one, and true for any property name that is not a property of Object.prototype.
The call is only generated when the property name is a property of
Object.prototype, or when the name is not known at compile time ($data or
`required` compiled into a loop) - generated code for all other schemas is
unchanged, and the run time check gives the same result on both paths.
This fixes the "required properties whose names are Javascript object
property names" test group of JSON-Schema-Test-Suite (all drafts).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What issue does this pull request resolve?
It resolves #2664, and fixes #1045 (
propertieswith aconstructorkey rejects{}) as part of the same defect.required,propertiesanddependenciesdetect a property withdata[prop] !== undefined, which is true for every object inheriting fromObject.prototypewhen the name is one ofObject.prototype's own members. So{"required": ["toString"]}is satisfied by{}— a keyword whose job is to reject data accepting data that is missing the property.I am aware #1045 was closed in 2019 with "see option
ownProperties", and that #197 showsownPropertieswas designed deliberately to cover these keywords. #2664 sets out why I think it is worth revisiting: therequiredfalse-negative direction was never raised in #1045; the JSON-Schema-Test-Suite has since added non-optional groups forbidding this on every draft; andadditionalProperties(which usesfor...in) already disagrees withrequiredabout whethertoStringis present in{}. If you would rather keep theownPropertiesanswer, please close this — it is a default-behaviour change and it is your call.What changes did you make?
Reproduction on the published release (
npm i ajv@8.20.0):Root cause —
lib/vocabularies/code.ts,propertyInData()/noPropertyInData():data.toString/data.constructor/data.__proto__resolve onObject.prototypefor any JSON-parsed object, so they are neverundefined. These two helpers are the presence test forrequired(validation/required.ts),properties(applicator/properties.ts),dependencies/dependentRequired(applicator/dependencies.ts) and JTDproperties.Fix — a new runtime helper. To be precise about what it does and does not do: it special-cases the ~12 names in
Object.getOwnPropertyNames(Object.prototype)and leaves every other property name on exactly the current!== undefinedtest. For those names it treats a property as present if it is an own property, or an inherited enumerable one.The call is only generated when the name is an
Object.prototypeproperty, or when it is not known at compile time ($data, orrequiredcompiled into a loop). The!objectProtoProps.has(prop)guard is what makes those two cases agree: on the dynamic paths the helper is called for every name, and returnstrueimmediately for anything that cannot be inherited fromObject.prototype, so the same schema and data validate identically under default options, underloopRequired: 1, and via$data. There is a regression test for exactly that. TheownProperties: truebranch is untouched.I am deliberately not claiming this implements the documented "all enumerable properties" rule universally. An inherited non-enumerable property whose name is not an
Object.prototypename — a class prototype method, say — still counts as present, as it does today. Applying the enumerability rule to every property name would be a much larger behaviour change and would cost a function call on every property check.Generated code for every other schema is unchanged. I dumped
validate.source.validateCodefor a corpus of 655 schemas (400 seeded-random schemas + every schema in the pinned draft-07 suite that does not mention anObject.prototypename), against a rebuilt baseline and against this branch:For a schema that does hit the new path:
Conformance. Harness, so the numbers are reproducible: JSON-Schema-Test-Suite at
6648e81, non-optional tests only (tests/<draft>/*.json, nottests/<draft>/optional/), one Ajv instance per test group with{strict: false, validateFormats: false, allowUnionTypes: true, logger: false},remotes/preloaded viaaddSchemaunderhttp://localhost:1234/, entry pointsdist/ajv.jsfor draft-06/07 anddist/2019.js/dist/2020.js. Failure counts:The delta is the same four cases on every draft — the whole
required.jsongroup"required properties whose names are Javascript object property names"(none of the properties mentioned,__proto__ present,toString present,constructor present). Every remaining failure is pre-existing and unrelated to this change ($refsibling-keyword and sibling-$idbase-URI cases,dynamicRef,unevaluatedItems/unevaluatedProperties,vocabulary,enum,definitions); I confirmed their counts are unchanged but did not investigate them.Tests
spec/tests/issues/1045_js_object_property_names.json— 16 cases mirroring the official suite groups plus adependenciesgroup. Run byspec/schema-tests.spec.tsacross the full option matrix and throughwithStandalone, so both the normal compiler and standalone code generation are covered.spec/issues/1045_object_prototype_property_names.spec.ts— 7 cases:required,properties, the$datapath, theloopRequiredpath, the static/dynamic agreement regression test described above, and — as CONTRIBUTING asks — the existing behaviour with the option off and on (inherited enumerable properties still validate withownProperties: false;ownProperties: trueunchanged).docs/options.mdunderownProperties.Fail → pass on a true rebuilt baseline (
git checkout HEAD~1 -- lib/, helper deleted,npm run build,dist/runtime/hasProperty.jsconfirmed absent):(the 3 that already pass on the baseline are the three "existing behaviour is preserved" tests, which is the point of them)
Full suite:
7635 − 7612 = 23 = 7 new unit cases + 16 new JSON test cases.
Is there anything that requires more attention while reviewing?
Yes — four things.
This changes default validation behaviour. Schemas with a
toString/constructor/valueOf/__proto__property name will validate differently. Probably a minor rather than a patch.properties: {"__proto__": ...}is still ignored, so one suite case still fails.allSchemaProperties()deliberately filters__proto__out of schema maps (lib/vocabularies/code.ts, with the matchingif (key === "__proto__") continueindependencies.ts). I did not touch that: un-filtering it would makeuseDefaultsemitdata.__proto__ = <default>, i.e. set the object's prototype. So the suite'sproperties.jsoncase"__proto__ not valid"still fails. To be clear about the direction of travel, it passed before this PR, but only by accident — the siblingconstructorsubschema was being wrongly applied to the inheritedObjectfunction and produced an error for an unrelated reason. Net, that group goes from 1 real pass + 3 accidental passes to 6 real passes and 1 real failure. Happy to follow up separately if you want the__proto__schema key honoured.useDefaultsis not covered by this change, and I left it that way.lib/compile/validate/defaults.tshas its own presence test (${childData} === undefined) which I did not touch, so with{useDefaults: true}and{"properties": {"constructor": {"default": 7}}}against{}, the default is still not assigned. I verified this is identical before and after the change (data after validation is{}in both cases), so it is not a regression — but it does mean Ajv now has two different answers to "is this property present", and a reviewer will reasonably ask. I judged fixing it out of scope for a bug fix; say the word and I will include it.requiredin loop /$datamode calls the helper for every property name. Where the name is not a compile-time string I cannot tell in advance whether it is anObject.prototypename, so the call is always emitted. The helper's first line is aSetlookup that returns immediately for other names, so the cost is oneSet.hasper property check on those two paths — but I have not benchmarked it. Everything else is byte-identical.If you want this scoped down: dropping the
propertyInDatahalf (i.e. fixing onlynoPropertyInData, which servesrequiredanddependencies) still fixes all four JSON-Schema-Test-Suite failures and the false-negative direction, and leavespropertiesexactly as #1045 ruled. Say the word and I will cut it back.What I could not verify: browser/karma tests (no Chrome in this environment; the change uses only
Object.getPrototypeOf,hasOwnProperty,propertyIsEnumerableandSet, and thecode.es5option is exercised by the passing full matrix, but real-browser execution is untested); benchmarks (no timing measurements taken); Node 18/20/22/24 (I ran on Node 26 only — CI will cover the rest); plugins other thanajv-formats. I also did not bump thespec/JSON-Schema-Test-Suitesubmodule — it is pinned at Nov 2021 and does not contain these groups, so I added equivalent cases underspec/tests/issues/rather than pulling in ~700 commits of unrelated new tests.