feat: secure wrap xlink #875
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
Benchmark resultsBase commit: lwc-engine-benchmark
|
caridy
reviewed
Dec 4, 2018
| @@ -1,4 +1,4 @@ | |||
| import { registerTemplate } from "lwc"; | |||
| import { registerTemplate, sanitizeXLink } from "lwc"; | |||
There was a problem hiding this comment.
this should be called sanitize with signature: (tagName: string, attrName: string, value: any)
There was a problem hiding this comment.
The tag name is probably not enough here, we also need the namespace as well.
function sanatizeAttribute(tagName: string, namespaceUri: string, attrName: string, attrValue: any): void
There was a problem hiding this comment.
since we were only concerned about sanitizing xlink function, i kept it explicit for locker. Do we anticipate more attributes that may need to be sanitized?
caridy
reviewed
Dec 4, 2018
| @@ -20,7 +20,7 @@ function tmpl($api, $cmp, $slotset, $ctx) { | |||
| "use", | |||
| { | |||
| attrs: { | |||
| "xlink:href": "/x" | |||
| "xlink:href": sanitizeXLink("/x") | |||
There was a problem hiding this comment.
this expression should be sanitize("use", "xlink:href", "/x")
There was a problem hiding this comment.
Do you foresee do that for all the attributes? Or only the one that we flagged as potential security issues?
There was a problem hiding this comment.
at this point i was only implementing the flagged one, wasn't sure there was more?
caridy
reviewed
Dec 4, 2018
| @@ -60,6 +61,10 @@ export function isIdReferencingAttribute(attrName: string): boolean { | |||
| return ID_REFERENCING_ATTRIBUTES_SET.has(attrName); | |||
| } | |||
|
|
|||
| export function isXLinkAttribute(attrName: string): boolean { | |||
There was a problem hiding this comment.
make this a generic method that receives the tagName and the attrName, and determine if that requires sanitizing
There was a problem hiding this comment.
i made the sanitizeAttribute function generic as requested. However, this particular function merely checks if this attribute is indeed a 'xlink:href'.
caridy
reviewed
Dec 4, 2018
| @@ -3,7 +3,7 @@ import _nsBar from "ns/bar"; | |||
| import _nsBuzz from "ns/buzz"; | |||
| import _nsTable from "ns/table"; | |||
| import _nsInput from "ns/input"; | |||
| import { registerTemplate } from "lwc"; | |||
| import { registerTemplate, sanitizeXLink } from "lwc"; | |||
There was a problem hiding this comment.
how is this going to work? who is responsible to populate this function? aura loader?
There was a problem hiding this comment.
Locker will be responsible for overriding the value.
There was a problem hiding this comment.
We also are going to add a sanitizeXLink no-op function on the lwc in the event of off-core usage where locker won't be available.
caridy
reviewed
Dec 4, 2018
| @@ -77,7 +77,7 @@ function tmpl($api, $cmp, $slotset, $ctx) { | |||
| "use", | |||
| { | |||
| attrs: { | |||
| "xlink:href": "xx" | |||
| "xlink:href": sanitizeAttribute("use", "http://www.w3.org/2000/svg", "xlink:href", "xx") | |||
There was a problem hiding this comment.
how are you planning to resolve the namespaceUri?
There was a problem hiding this comment.
Is that something that the parser is producing already?
There was a problem hiding this comment.
yes this information is already collected.
caridy
reviewed
Dec 4, 2018
| @@ -386,6 +397,13 @@ function transform( | |||
| if (isIdReferencingAttribute(attr.name)) { | |||
| return generateScopedIdFunctionForIdRefAttr(attr.value); | |||
| } | |||
|
|
|||
| if (isXLinkAttribute(attr.name) && namespaceURI === SVG_NAMESPACE_URI) { | |||
There was a problem hiding this comment.
eventually this should be abstracted, but I'm fine with this
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
|
LGTM so far |
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
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.
Details
Wrap the xlink:href into 'sanitize' function which will get patched by locker during runtime. When ran off-core, the function simply returns incoming value.
template compiler snippet for svg:
TODO: still need to handle expression values and check for svg
Does this PR introduce a breaking change?