fix: passStyleOf full input validation #1250
Conversation
|
Ready for review @mhofman Regarding Agoric/agoric-sdk#4333, I verified that I failed to check that the tagRecord is frozen. On remotable recognition, I made no other changes, because it now seems complete. Could you verify that there is no remaining laxity in remotable recognition? If there is, This PR should not close out Agoric/agoric-sdk#4333 . Thanks. |
erights
changed the title
There was a problem hiding this comment.
I did a quick pass through, and given this is not trying to guard against attacks through isPromise checks, this looks fine. We'll have to review this and other promise checks once we can have a safe Promise brand check, depending on which layer that check is implemented.
| // because we only get here if `ifPromise(pr)` already passed. | ||
| // eslint-disable-next-line prettier/prettier | ||
| (keys = ownKeys(/** @type {Promise} pr */(pr))).length === 0, | ||
| X`{pr} - Must not have any own properties: ${q(keys)}`, |
There was a problem hiding this comment.
| X`{pr} - Must not have any own properties: ${q(keys)}`, | |
| X`${pr} - Must not have any own properties: ${q(keys)}`, |
| // required for the TypeScript case syntax. We know this case is safe | ||
| // because we only get here if `ifPromise(pr)` already passed. | ||
| // eslint-disable-next-line prettier/prettier | ||
| (keys = ownKeys(/** @type {Promise} pr */(pr))).length === 0, |
There was a problem hiding this comment.
This check ends up too strict in the presence of the node async_hooks symbols added to some promise instances.
see https://github.com/Agoric/agoric-sdk/runs/7980370884?check_suite_focus=true#step:9:1589
|
Yup, this was noticed by @kriskowal while merging into agoric-sdk. Working on a PR right now. |
Fixes Agoric/agoric-sdk#4333
See Agoric/agoric-sdk#9
See #1126
I needed to fix this first, because I need to change the rules for recognizing remotables #1251 , in order to allow FarClasses.