[v0.31.0-rc.5] The internal function `_stringify` has a problem

[devcaeg]

The internal function _stringify has a problem. It may happen that constructor is undefined, so when trying to access constructor.name an error is thrown because name cannot be accessed since constructor is `undefined.

https://github.com/fabian-hiller/valibot/blob/2bdb2c8cb2228cc9d11776a60ba7e380caaece97/library/src/utils/_stringify/_stringify.ts#L10-L20

[devcaeg]

Perhaps the solution is to use Object.prototype.toString.call(input).slice(8, -1) this will return Object, String, Number, Function, etc.

[fabian-hiller]

Thank you for reporting this issue. How can I reproduce it? I thought due to type === 'object' and Object.getPrototypeOf(input) it is always defined because every object has a constructor.

[devcaeg]

For example, if an object is created with Object.create(null) the constructor will not be defined. This also happens with objects whose prototype has been modified.

const test1 = Object.create(null);
console.log(typeof test1, test1.constructor); // "object", "undefined"

function Empty() {}
Empty.prototype = Object.create(null);

const test2 = new Empty();
console.log(typeof test2, test2.constructor); // "object", "undefined"

[fabian-hiller]

Nice catch! Thank you! Something like this could work:

type = (input && Object.getPrototypeOf(input)?.constructor.name) || 'null';

[devcaeg]

Almost, it should actually be type = (input && Object.getPrototypeOf(input)?.constructor?.name) || 'null'; Since Object.getPrototypeOf(input) can be undefined or it can also be an empty object {}.

[devcaeg]

I have tested this change in my project but this change brings other problems. As you can see, the input actually complies with schema but safeParse says it does not.

const Empty = function () {};
Empty.prototype = Object.create(null);

const schema = object({
  id: string(),
});

const input = new Empty();
input.id = 'abc';

const validation = safeParse(schema, input);

console.log(validation);

{
  typed: false,
  success: false,
  output: {
    id: "123",
  },
  issues: [
    {
      kind: "schema",
      type: "object",
      input: [Object ...],
      expected: "Object",
      received: "null",
      message: "Invalid type: Expected Object but received null",
      requirement: undefined,
      path: undefined,
      issues: undefined,
      lang: undefined,
      abortEarly: undefined,
      abortPipeEarly: undefined,
      skipPipe: undefined,
    }
  ],
}

[devcaeg]

Although it may seem like an isolated case where constructor is not defined, in a real project, especially when using multiple libraries, this issue tends to occur frequently.

[devcaeg]

Sorry for so many comments, but I think issue #602 is related to this problem. Since if we force type to be Object you will receive the error Invalid type: Expected Object but received Object.

[fabian-hiller]

Almost, it should actually be type = (input && Object.getPrototypeOf(input)?.constructor?.name) || 'null'; Since Object.getPrototypeOf(input) can be undefined or it can also be an empty object {}.

Can you provide me sample input where Object.getPrototypeOf(input).constructor is undefined?

I have tested this change in my project but this change brings other problems. As you can see, the input actually complies with schema but safeParse says it does not.

This is probably related to our input.constructor === Object check. What do you think about my comment here?

[devcaeg]

Yes, the problem is this line.

https://github.com/fabian-hiller/valibot/blob/56ed176822a652959b8e1341fca287105baf728f/library/src/schemas/object/object.ts#L98

Here we could use Object.prototype.toString.call(input).slice(8, -1) === 'Object' and you could remove the typeof and input.

if (Object.prototype.toString.call(input).slice(8, -1) === 'Object') {

[devcaeg]

Can you provide me sample input where Object.getPrototypeOf(input).constructor is undefined?

Sorry, it is not undefiend it is null.

const test1 = Object.create(null);
console.log(Object.getPrototypeOf(test1)); // null

function Empty() {}
Empty.prototype = Object.create(null);

const test2 = new Empty();
console.log(Object.getPrototypeOf(test2)); // {}

[fabian-hiller]

Here we could use Object.prototype.toString.call(input).slice(8, -1) === 'Object' and you could remove the typeof and input.

I don't know if I want to change the code this way but I will think about it. If we stick to the current implementation we probably should change input.constructor === Object to Object.getPrototypeOf(input)?.constructor === Object.

Can you provide me sample input where Object.getPrototypeOf(input).constructor is undefined?

Sorry, it is not undefiend it is null.

I mean Object.getPrototypeOf(input).constructor and not Object.getPrototypeOf(input). You recommended to add a second ? to Object.getPrototypeOf(input)?.constructor?.name and I want to understand why do you think that this is necessary.

[devcaeg]

I mean Object.getPrototypeOf(input).constructor and not Object.getPrototypeOf(input). You recommended to add a second ? to Object.getPrototypeOf(input)?.constructor?.name and I want to understand why do you think that this is necessary.

The first ? is useful in cases where Object.getPrototypeOf(input) is null but there are also cases where Object.getPrototypeOf(input) is an empty object {} so the first ? has no effect, therefore trying to access the name property of the constructor will throw error since the constructor property does not exist on an empty object {}. For this reason, it is necessary to use two ?.

function Empty() {}
Empty.prototype = Object.create(null);

const test2 = new Empty();

Object.getPrototypeOf(test2); // {}
Object.getPrototypeOf(test2)?.constructor; // undefiend
Object.getPrototypeOf(test2)?.constructor.name; // TypeError: Cannot read properties of undefined (reading 'name')
Object.getPrototypeOf(test2)?.constructor?.name; // undefined

[devcaeg]

I don't know if I want to change the code this way but I will think about it. If we stick to the current implementation we probably should change input.constructor === Object to Object.getPrototypeOf(input)?.constructor === Object.

The problem with using Object.getPrototypeOf(input)?.constructor === Object is that it does not solve the problem in several cases, for example, in the two examples I mentioned above, and it will not solve it in Vercel Edge either.

For example, the fast-querystring library uses

function Empty() {}
Empty.prototype = Object.create(null);

so if with Valibot you wanted to validate this, it would actually give an error.

[EltonLobo07]

As far I as know, Object.getPrototypeOf can return a valid object or null, thats why you will need the first optional chaining operator. [[Prototype]].constructor can be anything as programmers have the freedom to set it (check the example below). It is not common but it is possible. So the second optional chanining operator can help in those uncommon cases.

class Box<T> {
    #value: T;

    constructor(value: T) {
        this.#value = value;
    }

    getValue() {
        return this.#value;
    }

    setValue(newValue: T) {
        this.#value = newValue;
    }
}

const box = new Box("Hello world");
Object.setPrototypeOf(box, {constructor: null});
_stringify(box); // Runtime error

[fabian-hiller]

In conclusion, it seems to be best to remove the plain object check and only check input && typeof input === 'object', as there are so many possible edge cases. What is your opinion on this? This is how we implemented it before v0.31.0.

[fabian-hiller]

I think I will remove it for now until we find a better solution.

[fabian-hiller]

The only solution I see is your recommendation @devcaeg. We could change the type check in object to Object.prototype.toString.call(input).slice(8, -1) === 'Object'. Are there any drawbacks? Can developers manipulate what this returns?

Should we also change _stringify to type = input ? Object.prototype.toString.call(input).slice(8, -1) : 'null'? How should we proceed with other schemas like array? Should we change the type checking to this approach as well to minimize the bundle size due to better compression?

[EltonLobo07]

My reply https://github.com/fabian-hiller/valibot/issues/608#issuecomment-2135423650 - Yes, I agree. I think input && typeof input === 'object' is the only check that cannot be manipulated by developers. It will always work as expected. But by doing so, specific object types will also get accepted (like Array, Date and so on), which is fine because technically specific objects still belong to the general object type. I would recommend using input && typeof input === 'object' as a temporary change until we can find a better solution.

[EltonLobo07]

The only solution I see is your recommendation @devcaeg. We could change the type check in object to Object.prototype.toString.call(input).slice(8, -1) === 'Object'. Are there any drawbacks? Can developers manipulate what this returns?

Should we also change _stringify to type = input ? Object.prototype.toString.call(input).slice(8, -1) : 'null'? How should we proceed with other schemas like array? Should we change the type checking to this approach as well to minimize the bundle size due to better compression?

toString checks are unreliable as developers can customize the string returned by the toString function call by setting [Symbol.toStringTag] as shown in the examples below. So I don't think relying on toString checks is a good idea.

class Box<T> {
    #value: T;

    constructor(value: T) {
        this.#value = value;
    }

    getValue() {
        return this.#value;
    }

    setValue(newValue: T) {
        this.#value = newValue;
    }

    get [Symbol.toStringTag]() {
        return "SomethingBox";
    }
}

// Custom object type
console.log(Object.prototype.toString.call(new Box("Hello world"))); // [object SomethingBox]

const arrayPrototype = Object.getPrototypeOf([]);
arrayPrototype[Symbol.toStringTag] = "xyz";

// JS provided complex object type
console.log(Object.prototype.toString.call([1, 2, 3])); // [object xyz]

// JS provided plain object
const message = {
    value: "Hi",
    [Symbol.toStringTag]: "Message"
};

console.log(Object.prototype.toString.call(message)); // [object Message]

[devcaeg]

Perhaps it helps to see what Zod does.

https://github.com/colinhacks/zod/blob/62b7842c46dd7f1004f41e2220284abb75378fb7/src/helpers/util.ts#L166

[fabian-hiller]

Due to our small bundle size, Zods solution does not work for us. 🫤

[fabian-hiller]

This is fixed in v0.31.0-rc.6