Skip to content

WIP test(pass-style): demonstrate safe promise loophole for @FUDCo#1781

Closed
erights wants to merge 1 commit intomasterfrom
markm-test-promise-loophole
Closed

WIP test(pass-style): demonstrate safe promise loophole for @FUDCo#1781
erights wants to merge 1 commit intomasterfrom
markm-test-promise-loophole

Conversation

@erights
Copy link
Copy Markdown
Contributor

@erights erights commented Sep 20, 2023

In #1275 I loosened the safe-promise.js checks used by passStyleOf in order to accommodate our workaround for the extra symbol-named own properties that Node's current async_hooks might add to promise instances. This checks these possible extra own properties against symbol-named own properties that would otherwise be inherited from Promise.prototype.

However, in doing so I seem to have forgotten that, even without async_hooks, Promise.prototype always has an own property named Symbol.toStringTag. Thus, the safe-promise rules currently allow that, but only if the property's "value" passes further checks. However, apparently by design, this "value" is obtained by a normal property GET (pr[key]), which might invoke a getter, allowing these overriding properties to be accessors. The easiest way for the value to pass the subsequent checks is to be a number.

The getter can be a per-instance function that, when called with a truthy argument, returns something else, like the string @FUDCo needs. The normal builtin toString use of pr[Symbol.toStringTag] expects a string value, which would not pass the test, causing a conflict. However, when pr[Symbol.toStringTag] is instead a number, the builtin toString behavior falls back to printing [object Object] rather than [object Promise], which is harmless enough for the very specialized case in which @FUDCo would use this loophole.

@erights erights requested review from FUDCo and mhofman September 20, 2023 05:17
@erights erights self-assigned this Sep 20, 2023
* ```js
* function destroyTracking(promise, parent) {
* trackPromise(promise, parent);
* trackPromise(promise, parent);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just a drive-by whitespace fix. Otherwise nothing to do with this PR.

t.is(p4[Symbol.toStringTag], 4);
t.is(`${p4}`, '[object Object]');
const getter = Object.getOwnPropertyDescriptor(p4, Symbol.toStringTag).get;
t.is(getter(true), 'I am p4');
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@FUDCo , this is the punchline

@erights erights changed the title test(pass-style): demonstrate safe promise loophole for Chip test(pass-style): demonstrate safe promise loophole for @FUDCo Sep 20, 2023
@erights erights force-pushed the markm-test-promise-loophole branch from c931b08 to 7d96dc9 Compare September 20, 2023 05:23
@erights
Copy link
Copy Markdown
Contributor Author

erights commented Sep 20, 2023

@FUDCo because this PR simply demonstrates that endo already has this odd property, you could utilize it for now without waiting for another endo release cycle. See the TODOs added by the PR.

@erights erights force-pushed the markm-test-promise-loophole branch from 7d96dc9 to 285c409 Compare September 20, 2023 06:01
@erights erights changed the title test(pass-style): demonstrate safe promise loophole for @FUDCo WIP test(pass-style): demonstrate safe promise loophole for @FUDCo Sep 20, 2023
@erights erights force-pushed the markm-test-promise-loophole branch 2 times, most recently from c82f2cf to 3089c82 Compare September 20, 2023 06:05
@erights erights force-pushed the markm-test-promise-loophole branch from 3089c82 to 441dbf1 Compare September 20, 2023 06:11
@erights
Copy link
Copy Markdown
Contributor Author

erights commented Sep 20, 2023

What's the name of the law that all observable behavior, whether documented or not, might get relied on, and thus no longer changeable? If we use this loophole, it'll become one of those, which would be bad. Let's discuss.

@erights
Copy link
Copy Markdown
Contributor Author

erights commented Sep 20, 2023

For debugging-only purposes,

export DEBUG=label-instances`

provokes heap exos and well as virtual and durable objects to have a vref-revealing own @@toStringTag property. For virtual and durable objects, this is set by

https://github.com/Agoric/agoric-sdk/blob/3f47cf03423b46b69851b021a18707b6ce42f056/packages/swingset-liveslots/src/virtualObjectManager.js#L231-L249

  if (LABEL_INSTANCES) {
    // This exposes the vref to userspace, which is a confidentiality hazard
    // as noted in the CONFIDENTIALITY HAZARD NOTE above.
    //
    // Aside from that hazard, the frozen string-valued data property is
    // safe to expose to userspace without enabling a GC sensor.
    // Strings lack identity and cannot be used as keys in WeakMaps.
    // If the property were a accessor property, we'd need to
    // ```js
    //   linkToCohort.set(self, getterFunc);
    //    unweakable.add(getterFunc);
    // ```
    defineProperty(self, Symbol.toStringTag, {
      value: `${proto[Symbol.toStringTag]}#${baseRef}`,
      writable: false,
      enumerable: false,
      configurable: false,
    });
  }

In order for this to work, the passStyleOf remotable checking rules had to explicitly allow this @@toStringTag own data property with a string value, which it does at

((key === Symbol.toStringTag && checkIface(candidate[key], check)) ||

It seems attractive to similarly have the passStyleOf safe-promise checking rules to explicitly allow promises to carry an @@toStringTag own data property with a string value. Then, it could just be a data property and we could avoid all the nonsense with it being an accessor in order to have a number value and a hidden string value.

Since @FUDCo 's need is also debug-only, it further seems attractive to extend the purpose of

export DEBUG=label-instances`

to label some promises with vrefs and krefs refs, using a similar @@toStringTag own data property.

@FUDCo
Copy link
Copy Markdown
Contributor

FUDCo commented Sep 20, 2023

What's the name of the law that all observable behavior, whether documented or not, might get relied on, and thus no longer changeable? If we use this loophole, it'll become one of those, which would be bad. Let's discuss.

Hyrum's Law. https://medium.com/se-101-software-engineering/what-is-the-hyrums-law-and-why-should-software-engineers-care-9bd98fbe74da

I'm really not sure how I feel about this. This particular loophole would actually work just fine for my use case, because the kref for a promise always has the form kpNN. On the other hand, it would bake this pattern in in a weird place and if we should ever decide to change it it will be a royal pain. On the other other hand, what distinguishes one promise kref from another probably wants to be number; what we'd probably change if we ever did is the kp part. On the other other other hand, if we found we needed to encode some additional metadata into the kref (like a type-ish indicator), that would once again put us in a world of hurt. (We do, in fact, have such a piece of metadata in vrefs, which encode whether the reference is to something durable or not).

Another consideration is that our token object for remotables also has a second method that provides the iface string. I'm not sure that's important for promises (presumably they're all, well, promises) but maybe we might use the iface string to indicate what type of thing the promise is allegedly a promise for. But if we wanted that, we'd need a way to add a second method of this sort to the promise token as well.

@erights
Copy link
Copy Markdown
Contributor Author

erights commented Sep 20, 2023

what distinguishes one promise kref from another probably wants to be number;

Look again. The number is just to pass the safe-promise check as it is. The getter parameter is so we can pass through whatever string we'd like.

This would just be until we rev endo. If we rev endo to do #1781 (comment) , then the own @@toStringTag property could be a string-values data property.

@FUDCo
Copy link
Copy Markdown
Contributor

FUDCo commented Sep 20, 2023

OK, I definitely misunderstood this the first time through.

Now that I read it through again much more carefully, my weirdness meter has redlined.

Let me try again to make sure I understand. I may be being overly detailed in this explication because the behavior in question is too bizarre for me to quite believe this is reality:

  • Something, somewhere, for reasons of its own that defy explanation, expects the [Symbol.toStringTag] property of a promise to be a number (or, more pedantically, something that satisfies some unspecified hard to explain mysterious quality that numbers happen to satisfy so for our purposes we'll use numbers).
  • And this despite the whole point of Symbol.toStringTag properties is to enable the provision of strings (rather than, say, numbers) to represent the types of the objects to which they are associated.
  • And you can satisfy this bizarro constraint by having pr[Symbol.toStringTag] be, instead of a value that is a number, a getter that returns a number.
  • That strange thing somewhere that expects pr[Symbol.toStringTag] to be a number will invoke the getter with no parameters, because that's, like, how the engine always invokes getters, that being the nature of getters and whatnot.
  • BUT we, being sneaky and somehow having access to the mysteries of the universe, know that we can extract the getter and just treat it like the function that it actually is, and invoke it ourselves with a parameter that is something other than the undefined it would see if it were being invoked with no arguments (say, true, for example), and the presence of this truthy parameter is used to conditionally return the thing we actually wanted to return in the first place, namely a string.
  • And because we have the getter return a number when the getter is invoked in the normal way means that the promise passes some test intended to ferret out whether it actually is a promise and not some other sneaky thing trying to pretend it's a promise even though it isn't.
  • But we do this sneaky thing not to pretend to be promise, because we actually are one, but to seem to be a promise despite the fact that Symbol.toStringTag can now be made to work somewhat the way it was always supposed to. Yay us.

I can't even.
But, hey, it looks like it works.
I think the Hyrum's Law queasiness is entirely justified.
But, hey, it looks like it works.

@FUDCo
Copy link
Copy Markdown
Contributor

FUDCo commented Sep 20, 2023

Is iface irrelevant in the case of a Promise?

@mhofman
Copy link
Copy Markdown
Contributor

mhofman commented Sep 20, 2023

I am weirded out by this @@toStringTag hackery.

I guess we don't have pseudo-promises yet we could use for this purpose? AFAIK, @FUDCo's constraint is to pass-through values but they don't have to be functional, right?

// TODO Decide whether to close this loophole, which would preclude
// using it to solve Chip's problem.
const unknownKeys = keys.filter(
key => typeof key !== 'symbol' || !hasOwnPropertyOf(Promise.prototype, key),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah this really should have been:

Suggested change
key => typeof key !== 'symbol' || !hasOwnPropertyOf(Promise.prototype, key),
key => {
if (typeof key !== 'symbol') return true;
const protoDesc = getOwnPropertyDescriptor(Promise.prototype, key)
return !(protoDesc && 'set' in protoDesc)
},

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Are all the expected async_hook properties necessarily accessors with a setter? If so, yes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, they're all accessors and meant to catch assignments (override mistake like taming), so guaranteed to have a set.

@erights
Copy link
Copy Markdown
Contributor Author

erights commented Sep 20, 2023

Is iface irrelevant in the case of a Promise?

Yes

@erights
Copy link
Copy Markdown
Contributor Author

erights commented Sep 20, 2023

I am weirded out by this @@toStringTag hackery.

FWIW, me too!

I guess we don't have pseudo-promises yet we could use for this purpose? AFAIK, @FUDCo's constraint is to pass-through values but they don't have to be functional, right?

That was my first thought as well, and where I think the proper fix lies. But @FUDCo explained that he needs something soon, so I came up with this as a first step that happens to work without even revving endo.

@erights
Copy link
Copy Markdown
Contributor Author

erights commented Sep 20, 2023

Now that I read it through again much more carefully, my weirdness meter has redlined.

FWIW, me too!

... too bizarre for me to quite believe this is reality:

But this is JavaScript!

  • Something, somewhere, for reasons of its own that defy explanation, expects the [Symbol.toStringTag] property of a promise to be a ...

The check was unaware of Symbol.toStringTag. It was checking all symbol-named properties on Promise.prototype under the WRONG assumption that these would only be added by NodeJS's async_hooks. So my trick is to override Promise.prototype[Symbol.toStringTag] with a property whose value passes the tests that were written only to validate that the expected async_hooks properties didn't contain anything unexpected and dangerous.

All the rest of your comments are essentially correct.

@erights
Copy link
Copy Markdown
Contributor Author

erights commented Sep 21, 2023

See #1785

@erights
Copy link
Copy Markdown
Contributor Author

erights commented Sep 26, 2023

Closing in favor of #1785

@erights erights closed this Sep 26, 2023
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

Successfully merging this pull request may close these issues.

3 participants