fix: tolerate empty func.prototype#1221
Merged
Merged
Conversation
Jack-Works
approved these changes
Jun 18, 2022
kriskowal
reviewed
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`); |
Contributor
Author
There was a problem hiding this comment.
Hmm. Unless the prototype: 'undefined' property is explicitly defined in the whitelist?
Contributor
Author
There was a problem hiding this comment.
In any case, for present purposes, I prefer that it always warn.
kriskowal
approved these changes
Jun 18, 2022
zloirock
reviewed
Jun 21, 2022
|
|
||
| test('tolerate empty prototype', t => { | ||
| t.assert('prototype' in Array.prototype.push); | ||
| t.is(Array.prototype.push.prototype, undefined); |
There was a problem hiding this comment.
Array.prototype.push.prototype here is not set to undefined, so it's an object, no?
Contributor
There was a problem hiding this comment.
1 task
erights
added a commit
that referenced
this pull request
Nov 13, 2024
Closes: #2598 Refs: #2563 #2334 #1221 ## Description #1221 was supposed to make ses tolerate undeletable `func.prototype` properties that should be absent, so long as they could be set to `undefined` instead, making them harmless. This tolerance came with a warning to flag the remaining non-conformance. However #2598 explains why #1221 sometimes fails to do this. #1221 did come with a test, but it fell into the case where #1221 works, which is a non-toplevel function. #2563 (and #2334 ?) fell into the trap explained by #2598 and untested by #1221, which is an undeletable `func.prototype` on a top-level instrinsic. As a result, #2563 currently contains a workaround for #2598 which this PR would make unnecessary. This PR fixes the problem by factoring out the `func.prototype`-tolerant property deletion into a separate `cauterizeProperty` function which it calls from both places. This PR also adds the test that was missing from #1221 , having first checked that the test detects #2598 when run without the rest of this PR. If this PR gets merged before #2563, then #2563's workaround for #2598 can first be removed before it is merged. - [ ] TODO should pass a genuine reporter in to all calls to `cauterizeProperty`. @kriskowal , please advise how intrinsics.js should arrange to do so. ### Security Considerations Allowing a `func.prototype` property that really shouldn't be there seems safe, so long as it is safely set to `undefined` first, which this PR does, and then checks that it has done so. ### Scaling Considerations none ### Documentation Considerations generally, this would be one less thing to worry about, and thus one less thing that needs to be documented for most users. ### Testing Considerations Adds the test that was missing from #1221 that let #2598 go unnoticed until #2563 ### Compatibility Considerations Should be none. ### Upgrade Considerations Should be none.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See zloirock/core-js#1092
If a primordial method is whitelisted as
fn, then it is not supposed to haveprototypeproperty. However, as explained by zloirock/core-js#1092 , there is some desire to runcore-jsas a vetted shim, i.e., a shim run beforelockdownthat 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 undeletableprototypeproperty.With this PR, our whitelisting, on encountering such a function, instead sets its
prototypetoundefinedand then checks that it has done so. If so, then it issues a warning but allows the whitelisting to continue.