ianstormtaylor / superstruct

A simple and composable way to validate data in JavaScript (and TypeScript).
https://docs.superstructjs.org
MIT License
6.95k stars 224 forks source link

Defaulted values are not correctly copied for `record` type data. #1238

Closed MaddyGuthridge closed 1 month ago

MaddyGuthridge commented 3 months ago

When providing a default value for a record-type struct, the default value is not properly copied, meaning that it is at risk of subtle pass-by-reference bugs.

Consider this example:

import { create, defaulted, record, string } from 'superstruct';

// BAD: array is passed by reference
// =================================
const DefaultedRecord = defaulted(
  record(string(), string()),
  {},
);

const recordA = create(undefined, DefaultedRecord);
const recordB = create(undefined, DefaultedRecord);

recordA.name = 'maddy';

console.log(recordB.name);
// Expect: undefined
// Receive: "maddy"

// OK: object is created through function
// ======================================

const DefaultedRecordFn = defaulted(
  record(string(), string()),
  () => ({}),
);

const recordFnA = create(undefined, DefaultedRecordFn);
const recordFnB = create(undefined, DefaultedRecordFn);

recordFnA.name = 'george';

console.log(recordFnB.name);
// Expect: undefined
// Receive: undefined

The current behaviour of a defaulted(record(...), ...) is different compared to that of defaulted(object(...), ...) and defaulted(array(...), ...) which both appear to copy their input data.

yeoffrey commented 2 months ago

Thanks for the report @MaddyGuthridge! I've tested this locally and I get the same issue.

After testing this a bit I think the easiest solution for this is to call structredClone on the default value in the defaulted coercion if its an object. For primatives this shouldn't happen because of the way that JS handles assignments.

export function defaulted<T, S>(
  struct: Struct<T, S>,
  fallback: any,
  options: {
    strict?: boolean
  } = {}
): Struct<T, S> {
  return coerce(struct, unknown(), (x) => {
    // To avoid a pass-by-reference bug, we'll clone objects when encountered
    // here, but the for performance avoid cloning primatives and functions
    const f =
      typeof fallback === 'function'
        ? fallback()
        : typeof fallback === 'object'
          ? structuredClone(fallback)
          : fallback

WDYT? cc: @arturmuller

arturmuller commented 2 months ago

@MaddyGuthridge thank you for the report!

@yeoffrey hmm. structuredClone destroys an object's prototype chain, meaning that if someone uses some custom class as the default value, they will unexpectedly get a plain object on the other end.

Using a custom class is probably a very rare use case, but also doesn't seem impossible.

I personally exclusively use POJOs with Superstruct, but... I guess someone somewhere could be doing something that this change could completely break.

I am honestly just leaning towards a warning in the docs, pointing people towards using the factory function signature for non primitive objects. My guess is that it was added exactly for this reason, just not properly documented.

yeoffrey commented 2 months ago

@arturmuller Alternatively we could accept only a callback function. That would be a breaking change but to me this feels like a bit of a foot gun 😅

Up to you!

MaddyGuthridge commented 2 months ago

I may actually have a suggestion for this: a library maintained by one of my coworkers jewire has some code that it uses to clone JS objects whilst preserving prototype chains (link to the source code). Perhaps you could use something similar?

yeoffrey commented 2 months ago

I may actually have a suggestion for this: a library maintained by one of my coworkers jewire has some code that it uses to clone JS objects whilst preserving prototype chains (link to the source code). Perhaps you could use something similar?

This could be a good solution. I could try modifying #1248 to include that code. Thanks for the link!

arturmuller commented 1 month ago

This has been fixed by changes introduced by #1151 which was just released in v2.0.2. Thank you for the bug report!