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

Value.Clone corrupts Object instance #1186

Closed
bitofbreeze opened this issue Feb 22, 2025 · 3 comments
Closed

Value.Clone corrupts Object instance #1186

bitofbreeze opened this issue Feb 22, 2025 · 3 comments

Comments

@bitofbreeze
Copy link

I am trying to validate a Cloudflare Worker env context, which binds a ProxyStub instance to it . I have a schema like:

Type.Object({
  KV: Type.Unsafe<KVNamespace>(Type.Object({})),
})

and then I proceed to do the Parse steps:

console.log("original", value.KV);
const cloned = Value.Clone(value); // clone because value ops can be mutable
console.log("cloned", cloned.KV);
const defaulted = Value.Default(EnvType, cloned); // initialize defaults for value
console.log("defaulted", defaulted.KV);
const converted = Value.Convert(EnvType, defaulted); // convert mismatched types for value
console.log("converted", converted.KV);
const cleaned = Value.Clean(EnvType, converted); // remove unknown properties
console.log("cleaned", cleaned.KV);
const decoded = Value.Decode(EnvType, cleaned); // run decode transforms (optional)
console.log("decoded", decoded.KV);

which outputs:

[app] original ProxyStub { name: 'KvNamespace', poisoned: false }
[app] cloned {}
[app] defaulted {}
[app] converted {}
[app] cleaned {}
[app] decoded {}

As you can see, the cloning step seems to corrupt the value. I have tested doing a simple shallow clone in JS via the spread operator, and it preserves the binding correctly, so I am not sure what Value.Clone does that destroys it.

Simply removing the Clone step outputs the following:

[app] original ProxyStub { name: 'KvNamespace', poisoned: false }
[app] defaulted ProxyStub { name: 'KvNamespace', poisoned: false }
[app] converted ProxyStub { name: 'KvNamespace', poisoned: false }
[app] cleaned ProxyStub { name: 'KvNamespace', poisoned: false }
[app] decoded ProxyStub { name: 'KvNamespace', poisoned: false }

which confirms the Clone step is the offender.

@sinclairzx81
Copy link
Owner

sinclairzx81 commented Feb 23, 2025

@bitofbreeze Hi,

I have tested doing a simple shallow clone in JS via the spread operator

Interesting. The minimum requirements for Clone is that the properties and symbols of an object are enumerable

Try the following and let me know what it prints.

const cloned = structuredClone(value.KV)
const symbols = Object.getOwnPropertySymbols(value.KV)
const keys = Object.getOwnPropertyNames(value.KV)

console.log({ cloned, symbols, keys })

If keys returns an empty array, it means the KV object is non-enumerable (so probably can't be cloned), but this seems at odds with you being able to use the spread operator to apply a shallow property clone. This said, if you don't need to Clone the value before applying operations to it, you can use a configurable Parse and omit the Clone operation.

const result = Value.Parse([  
  // 'Clone', // omit
  'Clean',    // ... and keep these
  'Default',
  'Convert',
  'Assert',
  'Decode'
], schema, value.KV)

Let me know what you see for the above cloned, symbols and keys return values.
Keep me posted
S

@bitofbreeze
Copy link
Author

bitofbreeze commented Feb 23, 2025

@sinclairzx81 structuredClone gives #<Object> could not be cloned., which looks to be due to this being a Proxy instance.

The other two give { symbols: [], keys: [] }

Thanks for the tip about just not cloning. Also to clarify, I mean using the spread on the entire value not value.KV. Using it on just value.KV gives just {}

Maybe you could add this check in the Clone method: util.types.isProxy(value) and not clone the property in that case? This is from "node:util".

@sinclairzx81
Copy link
Owner

@bitofbreeze Hiya,


Maybe you could add this check in the Clone method: util.types.isProxy(value) and not clone the property in that case? This is from "node:util".

It won't be possible for TypeBox make calls to runtime specific functions (be it Node, Deno, Bun or even Browser API's) as this would couple TypeBox to that runtime. You can however override the default Clone to perform the isProxy check in the following way.

import { Value, ParseRegistry } from '@sinclair/typebox/value'

ParseRegistry.Set('Clone', (_schema, _references, value) => {
  return util.types.isProxy(value) ? value : Value.Clone(value)
})
const result = Value.Parse([  
  'Clone',    // ... uses clone override above
  'Clean',    
  'Default',
  'Convert',
  'Assert',
  'Decode'
], schema, value.KV)

Object could not be cloned., which looks to be due to this being a Proxy instance

Indeed. The problem does stem from the way the KV proxy as been configured. TypeBox mandates that an object must be at least enumerable in order to be cloned. The developers of KV might have intentionally made the proxy non-enumerable for security reasons (it's not possible for a malicious script to enumerate keys and read potentially sensitive information that may be stored within the KV object if the object is not enumerable).

This said, you could try ask the maintainers of the KV Proxy to consider a providing some mechanism to implement getOwnPropertyDescriptor which would allow the KV proxy to be enumerated. It's unlikely, but you never know.

Consider the following example

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

// virtual object
const value = new Proxy({}, {
  ownKeys: () => ['x', 'y', 'z'],
  get: (_, key) => {
    return (
      key === 'x' ? 1 :
      key === 'y' ? 2 :
      key === 'z' ? 3 :
      undefined
    )
  },
  // required for clone
  getOwnPropertyDescriptor: () => {
    return { 
      enumerable: true, 
      configurable: true 
    }
  }
})

// enumerable test

console.log(Object.keys(value))                  // ['x', 'y', 'z']
console.log(Object.getOwnPropertyNames(value))   // ['x', 'y', 'z']
console.log(Object.getOwnPropertySymbols(value)) // []

// clone test

const A = { ...value }         // ok 
const B =  Value.Clone(value)  // ok - when enumerable
const C = structuredClone(     
  Object.fromEntries(Object.entries(value)) // enumerable to structuredClone
)

console.log({ A, B, C })
// {
//   A: { x: 1, y: 2, z: 3 },
//   B: { x: 1, y: 2, z: 3 },
//   C: { x: 1, y: 2, z: 3 }
// }

I think I will close out this issue because there's not much TypeBox can do to Clone a non-enumerable object. If you want to implement conditional Clone via util.types.isProxy have a look at the ParseRegistry override example. Alternatively you may wish to setup a specific ParseKV function specifically to handle values stored there.

import { TSchema, StaticDecode } from '@sinclair/typebox'
import { Value } from '@sinclair/typebox/value'
import { Type } from '@sinclair/typebox'


function ParseKV<Type extends TSchema, Result = StaticDecode<Type>>(type: Type, value: unknown): Result {
  return Value.Parse(['Clean', 'Default', 'Convert','Assert', 'Decode'], type, value) as never
}

const result = ParseKV(Type.Unsafe<KVNamespace>(Type.Unknown()), value.KV)

Happy to answer additional questions on this thread if you need, but will close this issue out for now.
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

2 participants