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

Simple object check #17

Open
Gongreg opened this issue Oct 31, 2023 · 6 comments
Open

Simple object check #17

Gongreg opened this issue Oct 31, 2023 · 6 comments

Comments

@Gongreg
Copy link

Gongreg commented Oct 31, 2023

Hello.
First of all thank you for the library, it works really great!

I've just bumped into one issue. In our setup we have iframe & parent window running on same domain and interacting with one another.

One of those interactions is:

const intialState = iframe.someFunction();

const state = create(initialState, ...);

Due to this, even though the returned object is "simple", the Object.getPrototypeOf(value) === Object.prototype check fails.

I can overcome this by using mark functionality:

const state = create(initialState, ..., {  mark: () => "immutable" });

This brings a couple of questions:

  1. Is it okay to simply use mark: () => "immutable",? Will this lead to some side effects? I haven't digged too deep into the source code yet, but couldn't find info in the docs.
  2. Next to mark docs, it says that (AutoFreeze and Patches should both be disabled). Is that really the case? I've tried to use them with mark and it looked like it works fine.
  3. Maybe there is a different kind of "simple" object check that would not break in this crazy scenario?
@unadlib
Copy link
Owner

unadlib commented Nov 1, 2023

hi @Gongreg ,

Is it okay to simply use mark: () => "immutable",? Will this lead to some side effects? I haven't digged too deep into the source code yet, but couldn't find info in the docs.

If you're sure the object structure is all immutable, then you're good to go like this. But be careful, if the object structure has some non-plain objects in it, they might get unexpectedly treated as immutable. Like,

class Foo {
  bar = 1;
}
const initState = { foo: new Foo(), a: { b: 1 } };
const state = create(
  initState,
  (draft) => {
    draft.foo.bar = 2;
  },
  {
    mark: () => 'immutable',
  }
);
expect(state.foo).toBeInstanceOf(Foo);
expect(state.foo).not.toBe(initState.foo);

Any objects in initState that get tweaked will be shallow copied, they've been marked as immutable.


Next to mark docs, it says that (AutoFreeze and Patches should both be disabled). Is that really the case? I've tried to use them with mark and it looked like it works fine.

Typically, when using the mark function, you shouldn't enable AutoFreeze and Patches. It could cause some unnecessary mix-ups, and It's not an equivalent operation patch.

For example,

class Foo {
  bar = 1;
}
const initState = { foo: new Foo(), a: { b: 1 }, time: new Date() };
const [state, patches] = create(
  initState,
  (draft) => {
    draft.foo.bar = 2;
    draft.time.setDate(2);
  },
  {
    enableAutoFreeze: true,
    enablePatches: true,
    mark: (value) => {
      if (value instanceof Foo) return 'immutable';
    },
  }
);
expect(state.foo).toBeInstanceOf(Foo);
expect(state.foo).not.toBe(initState.foo);
expect(state.time).toBe(initState.time);

initState.time is mutable, but the patches don't include its update. So, we can't get an equivalent object structure by applying those patches. But if you use mark: () => "immutable", in that specific case, you can totally enable AutoFreeze and Patches.

I've already updated it in the docs.


Maybe there is a different kind of "simple" object check that would not break in this crazy scenario?

You can use isSimpleObject as your custom mark.

const isSimpleObject = (value) => {
  const prototype = Object.getPrototypeOf(value);
  if (prototype === null) {
    return true;
  }
  const constructor =
    Object.hasOwnProperty.call(prototype, 'constructor') &&
    prototype.constructor;
  return (
    typeof constructor === 'function' &&
    Function.toString.call(constructor) ===
      Object.prototype.constructor.toString()
  );
};

const initialState = iframe.someFunction();

const state = create(initialState, callback, {
  mark: (value) => {
    if (isSimpleObject(value)) {
      return 'immutable';
    }
  },
});

isSimpleObject function can check for simple objects from across iframe or those without inheritance, like Object.create(null).


I'm thinking about adding some potentially useful mark functions to Mutative.

@unadlib
Copy link
Owner

unadlib commented Dec 1, 2023

I've released Mutative v0.7.3, which supports marking simple objects.

For example,

import { create, markSimpleObject } from 'mutative';

const baseState = {
  foo: {
    bar: 'str',
  },
  simpleObject: Object.create(null),
};

const state = create(
  baseState,
  (draft) => {
    draft.foo.bar = 'new str';
    draft.simpleObject.a = 'a';
  },
  {
    mark: markSimpleObject,
  }
);

expect(state.simpleObject).not.toBe(baseState.simpleObject);

@ghost
Copy link

ghost commented Jun 27, 2024

In this conversation I found that NodeJS code creates simple objects everywhere (in the form of { __proto__: null } see example) so that they don't inherit a polluted Object prototype (for security). They even have an eslint rule to enforce it.

With that in mind, and not knowing what type of data a developer might stick into one of my drafted states, wouldn't it be safer to always check if (prototype === null) by default and mark it as immutable? (Or to just make the whole markSimpleObject mark the default behavior?)

Also, am I protected against this if strict: true during development, even without mark: markSimpleObject?

@unadlib
Copy link
Owner

unadlib commented Jun 29, 2024

hi @waynebloss , mark: markSimpleObject or enabling strict mode does not protect the Object prototype. The mark function only defines the nature of a shallow copy or marks data.

The main issue is that protecting the Object prototype is very costly, and it may vary depending on the browsers, requiring unnecessary overhead. Moreover, prototype chain pollution is not common.

Specifically, there are multiple ways to access the prototype chain, such as: Object.getPrototypeOf(objectOrDraft), objectOrDraft.__proto__, objectOrDraft.constructor.prototype, objectOrDraft.isPrototypeOf.__proto__.__proto__, and so on.

Therefore, I think it is not very necessary for us to do this, don't you think so?

@ghost
Copy link

ghost commented Jun 29, 2024

@unadlib I was not trying to protect against object prototype pollution - I was surprised when I observed that objects created with Object.create(null) or { __proto__: null }, which live inside in the base state are modified during create.

image

@ghost
Copy link

ghost commented Jun 30, 2024

I tried it out and I see that strict mode does catch the issue. Thanks!

e.g.

> var baseState = { a: {}, b: Object.create(null) };
undefined
> var [draft, finalize] = create(baseState, { enablePatches: true, strict: true });
undefined
> draft.a.name = "a";
'a'
> draft.b.name = "b";
Uncaught:
Error: Strict mode: Mutable data cannot be accessed directly, please use 'unsafe(callback)' wrap.
    at checkReadable
    at Object.get

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