fix(ses): Handle well-known symbol look-alikes #1114
Merged
Conversation
This file contains 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
gibson042
approved these changes
Mar 15, 2022
| if (wellKnownSymbol) { | ||
| return wellKnownSymbol; | ||
| } else { | ||
| return `Unique${String(prop)}`; |
There was a problem hiding this comment.
It would be nice to document the nature of UniqueSymbol(…) (and string representation of property keys in general) somewhere, although I'm not sure where.
There was a problem hiding this comment.
| t.is( | ||
| ArrayProto[RogueSymbolIterator], | ||
| undefined, | ||
| `Well-known Symbol look-alike should have been removed`, |
There was a problem hiding this comment.
Should there also be an assertion about SymbolIterator, e.g. that it is not removed from an object where it is expected?
mhofman
force-pushed
the
mhofman/fix-well-known-symbol-whitelist
branch
2 times, most recently
from
9d2b638
to
6f6fffb
Compare
kriskowal
approved these changes
Mar 16, 2022
packages/ses/src/commons.js
Outdated
| @@ -72,6 +72,8 @@ export const { | |||
| toStringTag: toStringTagSymbol, | |||
| iterator: iteratorSymbol, | |||
| matchAll: matchAllSymbol, | |||
| keyFor: SymbolKeyFor, | |||
| for: SymbolFor, | |||
There was a problem hiding this comment.
Elsewhere, as in arrayPrototype, there’s precedent for camelCase over PascalCase.
There was a problem hiding this comment.
Actually thinking about this more, that means we don't differentiate between static and proto methods?
Edit: answering my own question, with examples like reflectSet, the answer seem to be "no".
mhofman
force-pushed
the
mhofman/fix-well-known-symbol-whitelist
branch
from
6f6fffb
to
899a957
Compare
mhofman
force-pushed
the
mhofman/fix-well-known-symbol-whitelist
branch
from
899a957
to
5139dad
Compare
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.
While working on fixing the
async_hooksissue, I realized that the whitelisting mechanism for well-known symbols can be tricked into accepting a unique symbol that just has the description of the expected well-known symbol, but is actually unique.This PR changes the whitelist parsing logic to generate a map of well-known symbols from the intrinsics and the permit list (similar to marshal's logic), and checks that a symbol matches the well-known symbol when found in the permits of an object. It does so by returning a name in the shape of
UniqueSymbol(description)when finding a symbol that isn't well-known, and only returning the@@nameif the symbol is found in the map. This technically would allow unique symbols to be permitted on intrinsics if named similarly (there is technically a chance of collision with string props, but that was already the case with the@@prefix anyway).Added a unit test for this (renaming the existing test, which seemed to be testing a different file). Verified manually that before the fix, the test would fail (didn't create separate commit with failing test)