Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

decode object with null prototype #865

Closed
bhamon opened this issue May 7, 2024 · 3 comments
Closed

decode object with null prototype #865

bhamon opened this issue May 7, 2024 · 3 comments

Comments

@bhamon
Copy link

bhamon commented May 7, 2024

I got a weird behavior with GraphQL args parsing: the decode function produces an incorrect value with their args object, but works fine with the same exact object copy/pasted from the console.
After a lot of digging and low level debug comparisons it seems like they (graphql-js) instanciate a null prototype object to prevent prototype leak during introspection (graphql/graphql-js#504 (comment)).

Those null prototype objects seem to break the decode function.

Here is a simple reproduction case:

import {Type} from '@sinclair/typebox';
import {Value} from '@sinclair/typebox/value';

const FooTransform = Type.Transform(Type.String())
  .Decode(v => new Date(v))
  .Encode(v => v.toISOString());

const ArgsSchema = Type.Object({
  foo: FooTransform
});

const obj1 = {foo: new Date().toISOString()};
console.log(Value.Check(ArgsSchema, obj1));
console.log(Value.Decode(ArgsSchema, obj1));

const obj2 = Object.create(null);
obj2.foo = new Date().toISOString();
console.log(Value.Check(ArgsSchema, obj2));
console.log(Value.Decode(ArgsSchema, obj2));

In my case, the objects with null prototype are nested at multiple paths inside a bigger objet. The overall parsing works without complaining, but the returned value breaks its own statically inferred type.

I'm curious to know your opinion on this one.

In the meantime I will make a deep copy of this args object before decoding.

@sinclairzx81
Copy link
Owner

@bhamon Hi,

Currently, the Decode function has a minimum requirement that the value being passed match the following criteria.

  • Be an instance of Object.prototype
  • Not be an instance of a class constructor
const obj1 = { foo: new Date().toISOString() }
console.log(Value.Check(ArgsSchema, obj1))
console.log(Value.Decode(ArgsSchema, obj1))

const obj2 = Object.create(null) // not a Object.prototype
obj2.foo = new Date().toISOString()
console.log(Value.Check(ArgsSchema, obj2))
console.log(Value.Decode(ArgsSchema, obj2))

const obj3: any = new Map() // Object.constructor.name !== 'Object'
obj3.foo = new Date().toISOString()
console.log(Value.Check(ArgsSchema, obj3))
console.log(Value.Decode(ArgsSchema, obj3))

// { foo: 2024-05-08T03:51:07.532Z }
// true
// [Object: null prototype] { foo: '2024-05-08T03:51:07.541Z' }
// true
// Map(0) { foo: '2024-05-08T03:51:07.542Z' }

The "no class constructor" criteria is mostly there as TB cannot be assured it can reconstruct a class instance with interior members (noting internal private class members cannot be cloned). The Object.prototype requirement is mostly there to ensure properties are enumerable.

Do you think Decode should support the Object.create(null) case? Could the graphql-js instance be patched to be made compatible with the current critiera?

Let me know your thoughts.
S

@CraigMacomber
Copy link

CraigMacomber commented May 9, 2024

Do you think Decode should support the Object.create(null) case?

I think such support would be nice as I think there are good reasons to use null prototypes with data someone might want to use with typebox.

I like to explicitly use null prototypes when dealing with data typed like Records to avoid various issues like the risk of false positives if someone uses "in" (ex: "toString" in {} is true).

Doing this also can avoid risk of prototype pollution attacks. Imagine code trying to merge two deep object hierarchies while one explicitly has an own value under a key __proto__, and the other code incorrectly tests for such a value using "in", resulting in merging the data from on object's __proto__ object into Object.prototype. Using a null prototype fixes this risk, which is a common source of real security issues.

MDN's page on Object calls out both this record/map like use-case and the risk of prototype pollution I mentioned above.

Thus I think it there are good reasons to use object with null prototypes, especially with record style data (where user data is used as keys), which is something that typebox has excellent support for. Therefore it seems like it would be nice for typebox to support null prototypes.

While I do use typebox and null prototype objects in the same project, I haven't actually used them together and hit this issue, but I reasonably could have, and would have been surprised that it didn't just work.

Also as this is my first post here, thanks for your work on TypeBox: I have both found it very useful as a library, but also very informative as something to learn from while working on my own typescript based strongly typed schema system.

@sinclairzx81
Copy link
Owner

@CraigMacomber Hi,

I like to explicitly use null prototypes when dealing with data typed like Records to avoid various issues like the risk of false positives if someone uses "in" (ex: "toString" in {} is true). ........ Doing this also can avoid risk of prototype pollution attacks. Imagine code trying to merge two deep object hierarchies while one explicitly has an own value under a key proto, and the other code incorrectly tests for such a value using "in", resulting in merging the data from on object's proto object into Object.prototype. Using a null prototype fixes this risk, which is a common source of real security issues.

Thanks for the detailed write up + rationale. This seems reasonable to me, and when looking at the OP's original use case, I guess it is somewhat surprising TB wasn't able to decode (or encode) in this scenario (the OPs use case is very reasonable). I do think TB was being overly strict in terms of it's definition of what could be reasonably decoded. As such, I've just pushed an update on 0.32.30 to extend it's definition to support objects with null prototypes.

Also as this is my first post here, thanks for your work on TypeBox: I have both found it very useful as a library, but also very informative as something to learn from while working on my own typescript based strongly typed schema system.

Ah thanks :)


@bhamon Hi,

As noted above, I've pushed an update to fix this on 0.32.30. Will close up this issue for now, but if you do run into any problems, feel free to ping on this thread.

All the best
S

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants