Shape validation for input with a field named `constructor` will throw an error.

[danbruce]

Describe the bug Schema validation for any object shape throws an error if the input being validated contains a field named constructor.

To Reproduce

const yup = require("yup");
const schema = yup.object().shape({});                                                                                
schema.validate({ constructor: "bar" }).then(res => console.log(res));

Results in this trace:

/project/node_modules/yup/lib/object.js:155
        field = field.resolve(innerOptions);

TypeError: field.resolve is not a function
    at /project/node_modules/yup/lib/object.js:155:23
    at Array.forEach (<anonymous>)
    at ObjectSchema._cast (/project/node_modules/yup/lib/object.js:145:11)
    at ObjectSchema._validate (/project/node_modules/yup/lib/mixed.js:215:20)
    at ObjectSchema._validate (/project/node_modules/yup/lib/object.js:187:47)
    at ObjectSchema.validate (/project/node_modules/yup/lib/mixed.js:260:19)
    at Object.<anonymous> (/project/yup-error-demonstration.js:4:8)
    at Module._compile (internal/modules/cjs/loader.js:701:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:712:10)
    at Module.load (internal/modules/cjs/loader.js:600:32)

Expected behavior I expect constructor to not be considered a special field name for object validation.

Platform (please complete the following information):

• Tested with node.js v10.15.3.

Additional context If the developer is using yup to validate form or query data on the backend the only way to mitigate this error is to sanitize the fields before sending them to yup. But a major feature of using yup is the noUnknown object to strip out unwanted fields, which cannot be relied upon if the input payload can be crafted to throw an error in yup.

[mastacheata]

That's a limitation of ECMAScript/JS, not yup.

In ECMAScript/JS constructor is specified in the Object Prototype, therefore you can't use it as an object property unless you want to override the object's constructor.

See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/constructor

[danbruce]

@mastacheata Thanks for the documentation link.

Shouldn't yup still handle this though? It's still an unexpected error being thrown by yup when trying to validate a shape.

In my case, I use yup to validate req.query/req.post from a node.js web app and the attacker managed to get an unexpected error thrown by sending a field named constructor. Are you implying yup should not be trusted to validate user input? That seems to be a primary use case for this library.

[mastacheata]

You're passing executable code to the library, that's not what User Input should look like.

schema.validate('{ constructor: "bar" }').then(res => console.log(res));

^^ This is what user input looks like. You pass a string to yup and it will validate it. If you pass a code structure to yup, it is assumed that it was generated under your control. Allowing a user to enter something and then execute that "as is" is neither what form validation nor your own XSS/sanitization mechanisms should ever allow to happen.

[danbruce]

Shouldn't there at least be a warning or documentation though? That if you are sending user input directly to yup, that it is up to you to remove special fields for objects like this?

That is, you are responsible for sanitizing your inputs....before handing them to your validation and sanitizing library...

Or yup could check for these special cases and handle them more gracefully.

[mastacheata]

It looks like you are constructing a case that isn't gonna happen in real life to me. (But I'm not the developer of yup, just a fellow user)

If someone enters { constructor: "bar" } in a form and you want to validate that field using yup, you get a string. If you personally named one of your input fields "constructor" and pass all your form field values as one big object to yup, that's gonna have side effects in the language. It's perfectly valid to pass a custom object, a class instance or even any self-defined type to yup, but the validation logic only applies to common variable types and stuff that behaves similarly enough.

At least that's what I understand. - Again: I'm not speaking on behalf of the yup maintainer(s)/developer(s). I haven't even contributed to this project at all. Just sharing what I know from experience as a developer working with this library.

[danbruce]

My use case is more like this:

const yup = require("yup");
const schema = yup.object().shape({... define my shape here ...}).noUnknown();                                                                                
schema.validate(req.query).then(queryParms => { I can use the validated query params here});

But right now, when someone hits the web server with a query parameter named constructor it blows up yup's validator...Now of course I can strip that field out, but it seems to me that a library who's goal is to validate user input should be responsible for this, not my code.

[mastacheata]

I only use yup for frontend validation, so maybe that's why my perspective is a little different. But I never thought about passing the raw request to my backend validation either. Yup expects you to collect only those fields you'd like to validate yourself and pass them as an input to validate or pass each field on it's own to a corresponding test method of your shape. (i.e. yup.number().test('10a'))

My guess is that this is also how 99.999% of all yup users are using the library and how the library is meant to be used. Your use case is obviously not anticipated, although it is possible to use the library that way.

[danbruce]

The noUnknown method for objects allows you to use yup to strip out those additional fields you don't care about. It's a killer feature actually!

[mastacheata]

Ahh, sorry, I had overlooked you mentioning that feature as it wasn't included in the original demo code. 🤦‍♂

Hmm, now this is different. The ignore/skip happens AFTER casting the field to MixedSchema instead of before.

Sorry, I've reached a dead end here. I still think this noUnknown is the wrong way around for your situation. You should pick only the known instead of filtering out unknowns. The noUnknown still has to iterate over the unknown fields and mark them as ignored, which is potentially dangerous or at least error prone as we can see here.

[jquense]

I'm a bit unclear whether this falls into "thats how the language works" or not on this case. I need to try it out a bit. I'm pretty surprised this is the behavior, yup does try to avoid these cases generally by using objects without prototypes which it does do here, so a bit unclear why this is still happening

[jquense]

ah i see this is bug in lodash's cloneDeepWith function