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: tolerate empty func.prototype #1221

Merged
merged 2 commits into from Jun 18, 2022
Merged

Conversation

erights
Copy link
Contributor

@erights erights commented Jun 18, 2022

See zloirock/core-js#1092

If a primordial method is whitelisted as fn, then it is not supposed to have prototype property. However, as explained by zloirock/core-js#1092 , there is some desire to run core-js as a vetted shim, i.e., a shim run before lockdown that does not break any of the invariants that SES depends on. However, the core-js shim also targets ES5, and there's no good option in ES5 for making a replacement primordial function that does not have an undeletable prototype property.

With this PR, our whitelisting, on encountering such a function, instead sets its prototype to undefined and then checks that it has done so. If so, then it issues a warning but allows the whitelisting to continue.

@erights erights requested a review from kriskowal June 18, 2022 01:05
@erights erights self-assigned this Jun 18, 2022
Comment on lines +290 to +292
if (obj.prototype === undefined) {
// eslint-disable-next-line @endo/no-polymorphic-call
console.warn(`Tolerating undeletable ${subPath} === undefined`);
Copy link
Member

Choose a reason for hiding this comment

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

Will this not always warn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Unless the prototype: 'undefined' property is explicitly defined in the whitelist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In any case, for present purposes, I prefer that it always warn.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see the context.

@erights erights merged commit 4da7742 into master Jun 18, 2022
@erights erights deleted the markm-tolerate-empty-prototype branch June 18, 2022 03:02

test('tolerate empty prototype', t => {
t.assert('prototype' in Array.prototype.push);
t.is(Array.prototype.push.prototype, undefined);
Copy link

@zloirock zloirock Jun 21, 2022

Choose a reason for hiding this comment

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

Array.prototype.push.prototype here is not set to undefined, so it's an object, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Ah, clear.

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.

None yet

4 participants