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 symbols as property names #2094

Merged
merged 2 commits into from
Dec 17, 2020
Merged

fix: tolerate symbols as property names #2094

merged 2 commits into from
Dec 17, 2020

Conversation

erights
Copy link
Member

@erights erights commented Dec 17, 2020

If the value of prop is a symbol ${prop} throws whereas ${String(prop)} produces a pleasing diagnostic. Should only be used to generate diagnostic text since the stringified from of a symbol is a valid string name for a property.

See endojs/endo#547
See #2092

@erights
Copy link
Member Author

erights commented Dec 17, 2020

@warner @FUDCo

Please review in particular the semantically significant changes: two places effectively testing that prop is a string that I changed to test whether prop is a string or a symbol.

Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

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

Just to explore this a bit: In my experiments, if foo is a symbol (e.g., foo = Symbol('zot')) and bar is a string (e.g., bar = 'thud'),

`hi ${bar} ho` yields 'hi thud ho' (as one would expect)
`hi ${foo} ho` throws a TypeError (also as one would expect, and the case you're guarding against here)
`hi ${String(bar)} ho` yields 'hi thud ho', which is just like it was without the String() conversion, as desired.
`hi ${String(foo)} ho` yields 'hi Symbol(zot) ho', which is a little weird, but seems like a useful outcome.

This all seems fine, but I want to be sure of your intent. I presume your comment about valid property names is not that Symbol(zot) resembles a property name but that since pretty much any string can be used as a property name things could get goofy if somebody wrote, e.g., obj['Symbol(zot)'] = 47. In that case diagnostic output would look the same as if they had written obj[foo] = 47, but presumably that would merely be confusing output and not a path to play some kind of game that might lead to an exploit. Am I understanding that right?

`ls.qm send(${JSON.stringify(targetSlot)}, ${prop}) -> ${resultVPID}`,
`ls.qm send(${JSON.stringify(targetSlot)}, ${String(
prop,
)}) -> ${resultVPID}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Line breaks inside template strings give me the willies.

Copy link
Member Author

Choose a reason for hiding this comment

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

Am I understanding that right?

Perfectly

Line breaks inside template strings give me the willies.

The computer did it. Blame the computer.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not what you'll say when the robots come for you.

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.

2 participants