Skip to content

fix: do not treat Object.prototype properties as present in the data - #2665

Open
maximilliangrand wants to merge 1 commit into
ajv-validator:masterfrom
maximilliangrand:fix/object-prototype-property-names
Open

fix: do not treat Object.prototype properties as present in the data#2665
maximilliangrand wants to merge 1 commit into
ajv-validator:masterfrom
maximilliangrand:fix/object-prototype-property-names

Conversation

@maximilliangrand

Copy link
Copy Markdown

What issue does this pull request resolve?

It resolves #2664, and fixes #1045 (properties with a constructor key rejects {}) as part of the same defect.

required, properties and dependencies detect a property with data[prop] !== undefined, which is true for every object inheriting from Object.prototype when the name is one of Object.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 shows ownProperties was designed deliberately to cover these keywords. #2664 sets out why I think it is worth revisiting: the required false-negative direction was never raised in #1045; the JSON-Schema-Test-Suite has since added non-optional groups forbidding this on every draft; and additionalProperties (which uses for...in) already disagrees with required about whether toString is present in {}. If you would rather keep the ownProperties answer, 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):

const Ajv = require("ajv")
const ajv = new Ajv()

ajv.validate({type: "object", required: ["toString"]}, JSON.parse("{}"))
// -> true   (expected false)

ajv.validate({type: "object", properties: {constructor: {type: "number"}}}, JSON.parse("{}"))
// -> false  (expected true)
// errors: [{"instancePath":"/constructor","schemaPath":"#/properties/constructor/type",
//           "keyword":"type","params":{"type":"number"},"message":"must be number"}]

ajv.validate({type: "object", required: ["__proto__"]}, JSON.parse("{}"))
// -> true   (expected false)

Root causelib/vocabularies/code.ts, propertyInData() / noPropertyInData():

const cond = _`${data}${getProperty(property)} !== undefined`
return ownProperties ? _`${cond} && ${isOwnProperty(gen, data, property)}` : cond

data.toString / data.constructor / data.__proto__ resolve on Object.prototype for any JSON-parsed object, so they are never undefined. These two helpers are the presence test for required (validation/required.ts), properties (applicator/properties.ts), dependencies/dependentRequired (applicator/dependencies.ts) and JTD properties.

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 !== undefined test. For those names it treats a property as present if it is an own property, or an inherited enumerable one.

// lib/runtime/hasProperty.ts
const objectProtoProps = new Set(Object.getOwnPropertyNames(Object.prototype))

export function isObjectProtoProperty(prop: string): boolean {
  return objectProtoProps.has(prop)
}

export default function hasProperty(data: object, prop: string): boolean {
  if (!objectProtoProps.has(prop)) return true
  if (Object.prototype.hasOwnProperty.call(data, prop)) return true
  let proto = Object.getPrototypeOf(data)
  while (proto !== null) {
    if (Object.prototype.hasOwnProperty.call(proto, prop)) {
      return Object.prototype.propertyIsEnumerable.call(proto, prop)
    }
    proto = Object.getPrototypeOf(proto)
  }
  return false
}

The call is only generated when the name is an Object.prototype property, or when it is not known at compile time ($data, or required compiled 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 returns true immediately for anything that cannot be inherited from Object.prototype, so the same schema and data validate identically under default options, under loopRequired: 1, and via $data. There is a regression test for exactly that. The ownProperties: true branch 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.prototype name — 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.validateCode for a corpus of 655 schemas (400 seeded-random schemas + every schema in the pinned draft-07 suite that does not mention an Object.prototype name), against a rebuilt baseline and against this branch:

$ diff code_base.txt code_fixed.txt && echo IDENTICAL
IDENTICAL

For a schema that does hit the new path:

if(((data.toString === undefined) || (!(func2(data, "toString")))) && (missing0 = "toString")){ ... }
if(data.constructor !== undefined && func2(data, "constructor")){ ... }
// standalone: const func2 = require("ajv/dist/runtime/hasProperty").default;

Conformance. Harness, so the numbers are reproducible: JSON-Schema-Test-Suite at 6648e81, non-optional tests only (tests/<draft>/*.json, not tests/<draft>/optional/), one Ajv instance per test group with {strict: false, validateFormats: false, allowUnionTypes: true, logger: false}, remotes/ preloaded via addSchema under http://localhost:1234/, entry points dist/ajv.js for draft-06/07 and dist/2019.js / dist/2020.js. Failure counts:

draft before after
draft-06 12 8
draft-07 8 4
draft-2019-09 28 24
draft-2020-12 62 58

The delta is the same four cases on every draft — the whole required.json group "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 ($ref sibling-keyword and sibling-$id base-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 a dependencies group. Run by spec/schema-tests.spec.ts across the full option matrix and through withStandalone, 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 $data path, the loopRequired path, 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 with ownProperties: false; ownProperties: true unchanged).
  • One clarifying sentence added to docs/options.md under ownProperties.

Fail → pass on a true rebuilt baseline (git checkout HEAD~1 -- lib/, helper deleted, npm run build, dist/runtime/hasProperty.js confirmed absent):

new unit spec       : 3 passing, 4 failing   ->  7 passing, 0 failing
spec/schema-tests   : 273 passing, 6 failing ->  279 passing, 0 failing

(the 3 that already pass on the baseline are the three "existing behaviour is preserved" tests, which is the point of them)

Full suite:

baseline (new test files removed) : 7612 passing, 350 pending, 0 failing
this branch                       : 7635 passing, 350 pending, 0 failing
this branch, AJV_FULL_TEST=true   : 7635 passing, 350 pending, 0 failing
npm run build / json-tests / prettier:check / eslint : all clean

7635 − 7612 = 23 = 7 new unit cases + 16 new JSON test cases.


Is there anything that requires more attention while reviewing?

Yes — four things.

  1. This changes default validation behaviour. Schemas with a toString/constructor/valueOf/__proto__ property name will validate differently. Probably a minor rather than a patch.

  2. 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 matching if (key === "__proto__") continue in dependencies.ts). I did not touch that: un-filtering it would make useDefaults emit data.__proto__ = <default>, i.e. set the object's prototype. So the suite's properties.json case "__proto__ not valid" still fails. To be clear about the direction of travel, it passed before this PR, but only by accident — the sibling constructor subschema was being wrongly applied to the inherited Object function 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.

  3. useDefaults is not covered by this change, and I left it that way. lib/compile/validate/defaults.ts has 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.

  4. required in loop / $data mode calls the helper for every property name. Where the name is not a compile-time string I cannot tell in advance whether it is an Object.prototype name, so the call is always emitted. The helper's first line is a Set lookup that returns immediately for other names, so the cost is one Set.has per 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 propertyInData half (i.e. fixing only noPropertyInData, which serves required and dependencies) still fixes all four JSON-Schema-Test-Suite failures and the false-negative direction, and leaves properties exactly 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, propertyIsEnumerable and Set, and the code.es5 option 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 than ajv-formats. I also did not bump the spec/JSON-Schema-Test-Suite submodule — it is pinned at Nov 2021 and does not contain these groups, so I added equivalent cases under spec/tests/issues/ rather than pulling in ~700 commits of unrelated new tests.

`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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant