Add an additional security brand check to StaticValues#2642
Conversation
🦋 Changeset detectedLatest commit: 05ee1db The changes in this PR will be included in the next version bump. 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 |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultslit-element-list
render
update
update-reflect
lit-html-kitchen-sink
render
update
nop-update
lit-html-repeat
render
update
lit-html-template-heavy
render
update
reactive-element-list
render
update
update-reflect
|
AndrewJakubowicz
left a comment
There was a problem hiding this comment.
LGTM, but should also get a LGTM from someone else with domain knowledge here.
packages/lit-html/src/static.ts
Outdated
| export const unsafeStatic = (value: string) => ({ | ||
| export const unsafeStatic = (value: string): StaticValue => ({ | ||
| ['_$litStatic$']: value, | ||
| r: /_/, |
There was a problem hiding this comment.
It's clever that this is short, but using a RegExp is maybe a little obscure? Also typeof x === {aLiteral} is hghly optimized in JS VMs.
What about:
const staticBrand = Symbol();
export const unsafeStatic = (value: string): StaticValue => ({
['_$litStatic$']: value,
r: staticBrand,
});Then:
if ((typeof (value as Partial<StaticValue>)?.r !== 'symbol')) {There was a problem hiding this comment.
IE11 doesn't have native Symbol, right, so typeof Symbol() === 'symbol' can't work, can it? https://caniuse.com/mdn-javascript_builtins_symbol
There was a problem hiding this comment.
We already require the Symbol polyfill.
Symbol.for() might be good like you mentioned before too, then you can do (value as Partial<StaticValue>)?.r === staticBrand
There was a problem hiding this comment.
Since these are each brand new instances of RegExp, and not a shared value, I'm assuming you're just trying to guard against JSON injections. In that case, you can take advantage of constructor: undefined to get the same benefits without needing an object (or a instanceof check, which is very slow). JSON cannot inject an undefined value, and a non-undefined constructor is present on all objects, so value.constructor === undefined guarantees the value was not constructed from JSON.
There was a problem hiding this comment.
That's a really cool trick @jridgewell! Ended up going with Symbol.for('') for clarity and to avoid hitting VM optimization edge cases.
There was a problem hiding this comment.
Oh, erp, didn't push all my changes, should be there now
AndrewJakubowicz
left a comment
There was a problem hiding this comment.
Awesome! Great comment on the brand.
Similar to #2307