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

fix(marshal): reject getters in pass-by-ref, even if it returns a function #2438

Merged
merged 2 commits into from
Feb 16, 2021

Conversation

warner
Copy link
Member

@warner warner commented Feb 16, 2021

This doesn't pass the typescript check yet, because it complains that I'm using a (potential) Symbol to do a property lookup:

  const descs = getOwnPropertyDescriptors(val);
  const keys = ownKeys(descs); // enumerable-and-not, string-or-Symbol
  keys.forEach(key => {
    assert(!descs[key].get, X`cannot serialize objects with getters like ${q(String(key))} in ${val}`);
    assert.typeof(
      val[key],
      'function',
      X`cannot serialize objects with non-methods like ${q(
        String(key),
      )} in ${val}`,
    );
  });

But that's exactly what I need to do: I want to reject getters in Symbol-named properties too. I don't need to look at the property name (pass-by-reference doesn't care), so want something that returns key+value tuples like Object.entries().forEach([_key, desc] .., except that Object.entries ignores Symbol-named properties, as does Object.values().

Any ideas how I can appease typescript?

@warner warner added the marshal package: marshal label Feb 16, 2021
@warner warner requested a review from erights February 16, 2021 03:07
@dckc
Copy link
Member

dckc commented Feb 16, 2021

I suggest using entries rather than keys and descs[key]:

  const descs = getOwnPropertyDescriptors(val);
  Object.entries(descs).forEach(([key, desc]) => {
    assert(!desc.get, X`cannot serialize objects with getters like ${q(String(key))} in ${val}`);

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

packages/marshal/src/marshal.js Outdated Show resolved Hide resolved
packages/marshal/src/marshal.js Outdated Show resolved Hide resolved
packages/marshal/test/test-marshal.js Show resolved Hide resolved
@erights
Copy link
Member

erights commented Feb 16, 2021

I suggest using entries rather than keys and descs[key]:
...

@dckc Using entries was exactly the mistake I made that set us down this long road. What I did not understand is how restrictive entries is. It only enumerates string-named enumerable own properties. It does not enumerate symbol-named own properties.

packages/marshal/src/marshal.js Outdated Show resolved Hide resolved
@erights
Copy link
Member

erights commented Feb 16, 2021

I suggest using entries rather than keys and descs[key]:
...

@dckc Using entries was exactly the mistake I made that set us down this long road. What I did not understand is how restrictive entries is. It only enumerates string-names enumerable own properties. It does not enumerate symbol-named own properties.

The new comments at

* Some edge cases to be aware of
* * If any of the original properties were accessors, `Object.entries`
* will cause its `getter` to be called and will use the resulting
* value.
* * No matter whether the original property was an accessor, writable,
* or configurable, all the properties of the returned object will be
* writable, configurable, data properties.
* * No matter what the original object may have inherited from, and
* no matter whether it was a special kind of object such as an array,
* the returned object will always be a plain object inheriting directly
* from `Object.prototype` and whose state is only these new mapped
* own properties.
* * The caller-provided mapping function can map the entry to a new entry
* with a different key, in which case the new object will have those
* keys as its property names rather than the original's.
*
* Typical usage will be to apply `objectMap` to a pass-by-copy record, i.e.,
* and object for which `passStyleOf(original) === 'copyRecord'`. For these,
* none of these edge cases arise. If the mapping does not introduce
* symbol-named properties, then the result, once hardened, will also be a
* pass-by-copy record.

is a good example both of how tricky all this is in JavaScript, and how great it is that our pass-by-copy objects avoid these hazards.

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good

…ction

The prohibit-accessors check was improved by using `!('get' in descs[key])`.
This should catch properties with a setter but not a getter, which will have
a descriptor with `get: undefined`. Although this happens to be caught
elsewhere, (because such a property reads as a non-function, triggering the
"cannot serialize objects with non-methods" clause.

closes #2436
@warner warner force-pushed the 2436-reject-getters branch from 3d4ddba to f5ec70e Compare February 16, 2021 08:07
@warner warner enabled auto-merge (squash) February 16, 2021 08:17
@warner warner merged commit b9368b6 into master Feb 16, 2021
@warner warner deleted the 2436-reject-getters branch February 16, 2021 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
marshal package: marshal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants