fix(ses): enablements grow monotonically#3129
Merged
Conversation
🦋 Changeset detectedLatest commit: 48016c4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
335c632 to
bb86fc3
Compare
a8685aa to
38ea294
Compare
gibson042
approved these changes
Mar 16, 2026
Member
gibson042
left a comment
There was a problem hiding this comment.
I think we should also cover monotonicity via test assertions, e.g.
/**
* Matches any non-empty string that consists exclusively of ASCII
* letter/digit/underscore/percent sign and does not start with a digit.
* Percent sign is allowed for unnamed primordials such as `%ObjectPrototype%`.
*/
const identifierLikePatt = /^[a-zA-Z_%][a-zA-Z_%0-9]*$/i;
/** @type {Map<symbol, string>} */
const builtinSymbols = new Map(
Reflect.ownKeys(Symbol).flatMap(key => {
const value = Symbol[key];
return typeof value === 'symbol' ? [[value, key]] : [];
}),
);
/** @type {(symbol: symbol) => string} */
const stringifySymbol = symbol => {
const builtinSymbolName = builtinSymbols.get(symbol);
if (builtinSymbolName) {
return builtinSymbolName.match(identifierLikePatt)
? `Symbol.${builtinSymbolName}`
: `Symbol[${JSON.stringify(builtinSymbolName)}]`;
}
const key = Symbol.keyFor(symbol);
return key !== undefined
? `Symbol.for(${JSON.stringify(key)})`
: `Symbol(${JSON.stringify(symbol.description)})`;
};
/**
* Assert that some enablement value is a valid relaxation of a base enablement.
* `true` may be relaxed only to `true`, "*" may be relaxed to `true` or "*",
* and a base record may be relaxed to `true`, "*", or a record that includes a
* superset of the base properties in which each property of the relaxation is
* either absent from the base element or is (recursively) a valid
* relaxation of the corresponding base enablement.
*/
const assertEnablementsRelaxation = (t, base, relaxation, path = '') => {
// Relaxing to `true` is always acceptable.
if (relaxation === true) return;
// Otherwise, relaxation must either preserve `true`/"*" or be recursively
// acceptable.
if (base === true || base === '*') {
t.is(
relaxation,
base,
`relaxation must preserve ${JSON.stringify(base)} at ${path || 'top-level'}`,
);
return;
}
t.is(
base === null ? 'null' : typeof base,
'object',
`base enablement at ${path || 'top-level'} must be \`true\`, "*", or a record`,
);
if (relaxation === '*') return;
t.is(
relaxation === null ? 'null' : typeof relaxation,
'object',
`relaxed enablement at ${path || 'top-level'} must be \`true\`, "*", or a record`,
);
const baseKeys = Reflect.ownKeys(base);
const relaxationKeys = Reflect.ownKeys(relaxation);
const missingKeys = baseKeys.filter(k => !relaxationKeys.includes(k));
t.deepEqual(
missingKeys,
[],
`relaxation must not omit base properties at ${path || 'top-level'}`,
);
for (const key of baseKeys) {
const pathSuffix =
typeof key === 'symbol'
? `[${stringifySymbol(key)}]`
: key.match(identifierLikePatt)
? `.${key}`
: `[${JSON.stringify(key)}]`;
const subPath = `${path}${pathSuffix}`.replace(/^[.]/, '');
const relaxationEnablement = Object.hasOwn(relaxation, key)
? relaxation[key]
: undefined;
assertEnablementsRelaxation(t, base[key], relaxationEnablement, subPath);
}
};
test('moderateEnablements relaxes minEnablements', t => {
assertEnablementsRelaxation(t, minEnablements, moderateEnablements);
});
test('severeEnablements relaxes moderateEnablements', t => {
assertEnablementsRelaxation(t, moderateEnablements, severeEnablements);
});
kriskowal
approved these changes
Mar 16, 2026
Contributor
Author
|
@gibson042 , #3129 (review) done. Thanks! |
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
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.
Closes: #XXXX
Refs: #XXXX
Description
Problem originally reported by @boneskull .
overrideTaming: 'moderate'includesoverrideTaming: 'min'.Previously
overrideTaming: 'min'correctly enabledIterator.prototype.constructorto be overridden by assignment, but due to an oversight,overrideTaming: 'moderate'did not. Now it does.To make such mistakes less likely, this PR also adopts a style where all records within larger enablements triple-dot the corresponding record from a smaller enablement, if present.
Security Considerations
Beyond fixing an observable bug, none. Everything is equally safe before and after this PR.
Scaling Considerations
none
Documentation Considerations
none
Testing Considerations
Tested.
Tests also modified to follow enablements being tested.
New test that the larger enablements are relaxations (as defined in packages/ses/test/enable-property-overrides-relaxation.test.js) of smaller enablements.
Compatibility Considerations
Iterator appears starting in Node 22. In my first attempt, tests broke in Node 22 because of this.
Upgrade Considerations
none.