ajv-validator / ajv-validator/ajv

required/properties/dependencies treat Object.prototype property names (toString, constructor, __proto__) as present in the data

Aperta
#2,664 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub
Lingua principale
TypeScript
Stelle
14.8k
Fork
1k
Metriche di merge delle PR
Nessuna PR unita negli ultimi 30g

Descrizione

**What version of Ajv are you using? Does the issue happen if you use the latest version?**

`ajv@8.20.0` (latest). Also reproduces on `master` at `f177fe32`.

**Ajv options object**

```javascript
const options = {} // defaults
```

**JSON Schema**

```json
{"type": "object", "required": ["toString"]}
```

**Sample data**

```json
{}
```

**Your code**

```javascript
const Ajv = require("ajv")
const ajv = new Ajv(options)
console.log(ajv.validate(schema, data))
```

**Validation result, data AFTER validation, error messages**

```
true
```

Data is unchanged. No errors.

**What results did you expect?**

`false`. `{}` does not have a `toString` property — `Object.keys({})`, `for (const k in {})` and `JSON.stringify({})` all agree that it has no properties at all.

The cause is that `required` (and `properties` and `dependencies`) detect presence with `data[prop] !== undefined`, which is true for every object inheriting from `Object.prototype` when the property name is one of `Object.prototype`'s own members (`toString`, `constructor`, `valueOf`, `hasOwnProperty`, `__proto__`, ...).

Three more cases of the same defect:

```javascript
// 2. valid data rejected
ajv.validate({type: "object", properties: {constructor: {type: "number"}}}, {})
// -> false
// errors: [{"instancePath":"/constructor","schemaPath":"#/properties/constructor/type",
// "keyword":"type","params":{"type":"number"},"message":"must be number"}]
// The `constructor` subschema is applied to the inherited `Object` function.

// 3. invalid data accepted
ajv.validate({type: "object", required: ["__proto__"]}, {})
// -> true

// 4. dependencies triggered by an absent property
ajv.validate({type: "object", dependencies: {toString: ["x"]}}, {})
// -> false
```

Why I think this is a bug rather than intended behaviour:

1. **`required` accepts data that is missing a required property.** This is the direction that has not been discussed before (see prior art below) and it is a false negative from the one keyword whose entire job is to reject such data.
2. **Ajv's keywords disagree with each other about what "present" means.** `additionalProperties` uses `for...in`, so with `{"additionalProperties": false}` Ajv correctly treats `toString` as absent from `{}` — while `required` treats it as present in the very same object.
3. **It contradicts the documented semantics of `ownProperties`.** [options.md](https://ajv.js.org/options.html#ownproperties) says *"By default Ajv iterates over all **enumerable** object properties"*. `Object.prototype`'s members are not enumerable.
4. **The JSON-Schema-Test-Suite forbids it, in non-optional tests.** `tests//required.json` and `tests//properties.json` both have a group `"...properties whose names are Javascript object property names"` with the comment *"Ensure JS implementations don't universally consider e.g. `__proto__` to always be present in an object."* Ajv fails 4 of the `required.json` cases on draft-06, draft-07, draft-2019-09 and draft-2020-12. On draft-07 those 4 are, apart from 3 pre-existing `$ref` failures, the only non-optional failures left.

Ajv's own CI does not see these groups because `spec/JSON-Schema-Test-Suite` is pinned at a Nov 2021 commit that predates them.

**Prior art, and the reason I am opening this anyway**

#1045 ("Can't validate json if it has `constructor` property", 2019, v6) is exactly case 2 above. It was closed with *"see option `ownProperties`"*, and #197 shows `ownProperties` was designed deliberately to cover `properties`/`required`/`dependencies`. So this is a re-opening of ground that was already ruled on, and I want to be upfront about that rather than pretend otherwise.

What I think has changed since that ruling:

- the official test suite now has non-optional tests forbidding this on every draft (they did not exist in 2019);
- case 1 (`required` accepting data missing the property) was never raised in #1045;
- `ownProperties: true` is a wider change than the problem requires — it also stops inherited *enumerable* properties from validating, which is the behaviour the option exists to control. A user who wants inherited enumerable properties to validate has no way to also get correct `required` handling of `toString`.

**Are you going to resolve the issue?**

Yes — I have a fix ready and will open a PR referencing this issue.

The approach: a small runtime helper that treats a property as present if it is an own property, or an inherited *enumerable* property. It is only consulted for the ~12 names in `Object.getOwnPropertyNames(Object.prototype)`; every other property name keeps the current `!== undefined` test, and the helper itself short-circuits on non-`Object.prototype` names so that the `$data` and `loopRequired` code paths (where the name is not known at compile time) behave identically. `ownProperties: true` is untouched.

Generated code for schemas that do not mention one of those names is byte-for-byte unchanged (verified over 400 random schemas plus the pinned draft-07 suite), and the full test suite passes on all 16 option combinations plus standalone.

If you would rather not change the default behaviour, please say so and I will close the PR — I would rather ask than not raise it. A narrower variant is also possible: fix only `required`/`dependencies` (the false-negative direction and all 4 suite failures) and leave `properties` exactly as #1045 ruled.

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.