-
Notifications
You must be signed in to change notification settings - Fork 168
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
Comments
@bhamon Hi, Currently, the Decode function has a minimum requirement that the value being passed match the following criteria.
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. |
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: 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 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. |
@CraigMacomber Hi,
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.
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 |
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:
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.
The text was updated successfully, but these errors were encountered: