Skip to content

Commit

Permalink
fix(ses): fix #2598 with cauterizeProperty reuse
Browse files Browse the repository at this point in the history
  • Loading branch information
erights committed Nov 4, 2024
1 parent 08d3945 commit 578772d
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 30 deletions.
51 changes: 51 additions & 0 deletions packages/ses/src/cauterize-property.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { objectHasOwnProperty } from './commons.js';

/**
* @import {Reporter} from './reporting-types.js'
*/

/**
*
* @param {object} obj
* @param {PropertyKey} prop
* @param {boolean} known
* @param {string} subPath
* @param {Reporter} reporter
* @returns {void}
*/
export const cauterizeProperty = (
obj,
prop,
known,
subPath,
{ warn, error },
) => {
// Either the object lacks a permit or the object doesn't match the
// permit.
// If the permit is specifically false, not merely undefined,
// this is a property we expect to see because we know it exists in
// some environments and we have expressly decided to exclude it.
// Any other disallowed property is one we have not audited and we log
// that we are removing it so we know to look into it, as happens when
// the language evolves new features to existing intrinsics.
if (!known) {
warn(`Removing ${subPath}`);
}
try {
delete obj[prop];
} catch (err) {
if (objectHasOwnProperty(obj, prop)) {
if (typeof obj === 'function' && prop === 'prototype') {
obj.prototype = undefined;
if (obj.prototype === undefined) {
warn(`Tolerating undeletable ${subPath} === undefined`);
return;
}
}
error(`failed to delete ${subPath}`, err);
} else {
error(`deleting ${subPath} threw`, err);
}
throw err;
}
};
11 changes: 10 additions & 1 deletion packages/ses/src/intrinsics.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { cauterizeProperty } from './cauterize-property.js';
import {
TypeError,
WeakSet,
Expand Down Expand Up @@ -100,7 +101,15 @@ export const makeIntrinsicsCollector = () => {
}
const namePrototype = permit.prototype;
if (!namePrototype) {
throw TypeError(`${name}.prototype property not permitted`);
cauterizeProperty(
intrinsic,
'prototype',
false,
`${name}.prototype`,
console, // TODO should be a proper Reporter
);
// eslint-disable-next-line no-continue
continue;
}
if (
typeof namePrototype !== 'string' ||
Expand Down
34 changes: 5 additions & 29 deletions packages/ses/src/permits-intrinsics.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ import {
ownKeys,
symbolKeyFor,
} from './commons.js';
import { cauterizeProperty } from './cauterize-property.js';

/**
* @import {Reporter} from './reporting-types.js'
Expand Down Expand Up @@ -279,35 +280,10 @@ export default function removeUnpermittedIntrinsics(
const subPermit = getSubPermit(obj, permit, propString);

if (!subPermit || !isAllowedProperty(subPath, obj, prop, subPermit)) {
// Either the object lacks a permit or the object doesn't match the
// permit.
// If the permit is specifically false, not merely undefined,
// this is a property we expect to see because we know it exists in
// some environments and we have expressly decided to exclude it.
// Any other disallowed property is one we have not audited and we log
// that we are removing it so we know to look into it, as happens when
// the language evolves new features to existing intrinsics.
if (subPermit !== false) {
warn(`Removing ${subPath}`);
}
try {
delete obj[prop];
} catch (err) {
if (prop in obj) {
if (typeof obj === 'function' && prop === 'prototype') {
obj.prototype = undefined;
if (obj.prototype === undefined) {
warn(`Tolerating undeletable ${subPath} === undefined`);
// eslint-disable-next-line no-continue
continue;
}
}
error(`failed to delete ${subPath}`, err);
} else {
error(`deleting ${subPath} threw`, err);
}
throw err;
}
cauterizeProperty(obj, prop, subPermit === false, subPath, {
warn,
error,
});
}
}
}
Expand Down
24 changes: 24 additions & 0 deletions packages/ses/test/tolerate-empty-prototype-toplevel.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/* global globalThis */
import test from 'ava';
import '../index.js';

// See https://github.com/zloirock/core-js/issues/1092
// See https://github.com/endojs/endo/issues/2598
const originalEscape = globalThis.escape;
globalThis.escape = function escape(...args) {
return Reflect.apply(originalEscape, this, args);
};

lockdown();

test('tolerate empty escape.prototype', t => {
t.is(globalThis.escape, escape);
t.assert('prototype' in escape);
t.is(escape.prototype, undefined);
t.deepEqual(Object.getOwnPropertyDescriptor(escape, 'prototype'), {
value: undefined,
writable: !!harden.isFake,
enumerable: false,
configurable: false,
});
});
3 changes: 3 additions & 0 deletions packages/ses/test/tolerate-empty-prototype.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ import test from 'ava';
import '../index.js';

// See https://github.com/zloirock/core-js/issues/1092
// Does not detect https://github.com/endojs/endo/issues/2598 because
// `push` is not toplevel.
// See tolerate-empty-prototype-toplevel.test.js
const originalPush = Array.prototype.push;
// eslint-disable-next-line no-extend-native
Array.prototype.push = function push(...args) {
Expand Down

0 comments on commit 578772d

Please sign in to comment.