-
Notifications
You must be signed in to change notification settings - Fork 27.4k
feat($sanitize): support enhancing white-list #16326
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,9 @@ var $sanitizeMinErr = angular.$$minErr('$sanitize'); | |
var bind; | ||
var extend; | ||
var forEach; | ||
var isArray; | ||
var isDefined; | ||
var isObject; | ||
var lowercase; | ||
var noop; | ||
var nodeContains; | ||
|
@@ -144,9 +146,11 @@ var htmlSanitizeWriter; | |
* Creates and configures {@link $sanitize} instance. | ||
*/ | ||
function $SanitizeProvider() { | ||
var hasBeenInstantiated = false; | ||
var svgEnabled = false; | ||
|
||
this.$get = ['$$sanitizeUri', function($$sanitizeUri) { | ||
hasBeenInstantiated = true; | ||
if (svgEnabled) { | ||
extend(validElements, svgElements); | ||
} | ||
|
@@ -187,7 +191,7 @@ function $SanitizeProvider() { | |
* </div> | ||
* | ||
* @param {boolean=} flag Enable or disable SVG support in the sanitizer. | ||
* @returns {boolean|ng.$sanitizeProvider} Returns the currently configured value if called | ||
* @returns {boolean|$sanitizeProvider} Returns the currently configured value if called | ||
* without an argument or self for chaining otherwise. | ||
*/ | ||
this.enableSvg = function(enableSvg) { | ||
|
@@ -199,6 +203,94 @@ function $SanitizeProvider() { | |
} | ||
}; | ||
|
||
|
||
/** | ||
* @ngdoc method | ||
* @name $sanitizeProvider#addValidElements | ||
* @kind function | ||
* | ||
* @description | ||
* Extends the built-in lists of valid HTML/SVG elements, i.e. elements that are considered safe | ||
* and are not stripped off during sanitization. You can extend the following lists: | ||
* | ||
* - `htmlElements`: A list of elements (tag names) to extend the current list of safe HTML | ||
* elements. HTML elements considered safe will not be removed during sanitization. All other | ||
* elements will be stripped off. | ||
* | ||
* - `htmlVoidElements`: This is similar to `htmlElements`, but in addition allows the specified | ||
* elements to have no end tag (similar to HTML | ||
* [void elements](https://rawgit.com/w3c/html/html5.1-2/single-page.html#void-elements)). | ||
* | ||
* - `svgElements`: This is similar to `htmlElements`, but for SVG elements. This list is only | ||
* taken into account if SVG is {@link ngSanitize.$sanitizeProvider#enableSvg enabled} for | ||
* `$sanitize`. | ||
* | ||
* <div class="alert alert-warning"> | ||
* This method must be called during the {@link angular.Module#config config} phase. Once the | ||
* `$sanitize` service has been instantiated, this method has no effect. | ||
* </div> | ||
* | ||
* @param {Array<String>|Object} elements - A list of valid HTML elements or an object with one or | ||
* more of the following properties: | ||
* - **htmlElements** - `{Array<String>}` - A list of elements to extend the current list of | ||
* HTML elements. | ||
* - **htmlVoidElements** - `{Array<String>}` - A list of elements to extend the current list of | ||
* void HTML elements; i.e. elements that do not have an end tag. | ||
* - **svgElements** - `{Array<String>}` - A list of elements to extend the current list of SVG | ||
* elements. The list of SVG elements is only taken into account if SVG is | ||
* {@link ngSanitize.$sanitizeProvider#enableSvg enabled} for `$sanitize`. | ||
* | ||
* Passing an array (`[...]`) is equivalent to passing `{htmlElements: [...]}`. | ||
* | ||
* @return {$sanitizeProvider} Returns self for chaining. | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need more docs (and also mention that it has no effect after the service has been instantiated). @maksimr, can you take a first stab at documenting what this method does and "valid elements/attributes" are and how they are used by |
||
this.addValidElements = function(elements) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's have this method return |
||
if (!hasBeenInstantiated) { | ||
if (isArray(elements)) { | ||
elements = {htmlElements: elements}; | ||
} | ||
|
||
addElementsTo(svgElements, elements.svgElements); | ||
addElementsTo(voidElements, elements.htmlVoidElements); | ||
addElementsTo(validElements, elements.htmlVoidElements); | ||
addElementsTo(validElements, elements.htmlElements); | ||
} | ||
|
||
return this; | ||
}; | ||
|
||
|
||
/** | ||
* @ngdoc method | ||
* @name $sanitizeProvider#addValidAttrs | ||
* @kind function | ||
* | ||
* @description | ||
* Extends the built-in list of valid attributes, i.e. attributes that are considered safe and are | ||
* not stripped off during sanitization. | ||
* | ||
* **Note**: | ||
* The new attributes will not be treated as URI attributes, which means their values will not be | ||
* sanitized as URIs using `$compileProvider`'s | ||
* {@link ng.$compileProvider#aHrefSanitizationWhitelist aHrefSanitizationWhitelist} and | ||
* {@link ng.$compileProvider#imgSrcSanitizationWhitelist imgSrcSanitizationWhitelist}. | ||
* | ||
* <div class="alert alert-warning"> | ||
* This method must be called during the {@link angular.Module#config config} phase. Once the | ||
* `$sanitize` service has been instantiated, this method has no effect. | ||
* </div> | ||
* | ||
* @param {Array<String>} attrs - A list of valid attributes. | ||
* | ||
* @returns {$sanitizeProvider} Returns self for chaining. | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be worth documenting that the added attributes will not be treated as URI attributes, i.e. their values will not be sanitized as URIs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs more docs as well. You could also move most of the param description into the main description of the function. |
||
this.addValidAttrs = function(attrs) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's have this method return |
||
if (!hasBeenInstantiated) { | ||
extend(validAttrs, arrayToMap(attrs, true)); | ||
} | ||
return this; | ||
}; | ||
|
||
////////////////////////////////////////////////////////////////////////////////////////////////// | ||
// Private stuff | ||
////////////////////////////////////////////////////////////////////////////////////////////////// | ||
|
@@ -207,6 +299,8 @@ function $SanitizeProvider() { | |
extend = angular.extend; | ||
forEach = angular.forEach; | ||
isDefined = angular.isDefined; | ||
isArray = angular.isArray; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no alphabetic ordering?? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't see any signs of "alphabetic ordering" 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @petebacondarwin looks like @gkalpak fixed it already 1920420#diff-699beb0d8c48a8257d3d70331bf6138dR19 @gkalpak Thanks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed while merging (and also removed unused |
||
isObject = angular.isObject; | ||
lowercase = angular.$$lowercase; | ||
noop = angular.noop; | ||
|
||
|
@@ -230,36 +324,36 @@ function $SanitizeProvider() { | |
|
||
// Safe Void Elements - HTML5 | ||
// http://dev.w3.org/html5/spec/Overview.html#void-elements | ||
var voidElements = toMap('area,br,col,hr,img,wbr'); | ||
var voidElements = stringToMap('area,br,col,hr,img,wbr'); | ||
|
||
// Elements that you can, intentionally, leave open (and which close themselves) | ||
// http://dev.w3.org/html5/spec/Overview.html#optional-tags | ||
var optionalEndTagBlockElements = toMap('colgroup,dd,dt,li,p,tbody,td,tfoot,th,thead,tr'), | ||
optionalEndTagInlineElements = toMap('rp,rt'), | ||
var optionalEndTagBlockElements = stringToMap('colgroup,dd,dt,li,p,tbody,td,tfoot,th,thead,tr'), | ||
optionalEndTagInlineElements = stringToMap('rp,rt'), | ||
optionalEndTagElements = extend({}, | ||
optionalEndTagInlineElements, | ||
optionalEndTagBlockElements); | ||
|
||
// Safe Block Elements - HTML5 | ||
var blockElements = extend({}, optionalEndTagBlockElements, toMap('address,article,' + | ||
var blockElements = extend({}, optionalEndTagBlockElements, stringToMap('address,article,' + | ||
'aside,blockquote,caption,center,del,dir,div,dl,figure,figcaption,footer,h1,h2,h3,h4,h5,' + | ||
'h6,header,hgroup,hr,ins,map,menu,nav,ol,pre,section,table,ul')); | ||
|
||
// Inline Elements - HTML5 | ||
var inlineElements = extend({}, optionalEndTagInlineElements, toMap('a,abbr,acronym,b,' + | ||
var inlineElements = extend({}, optionalEndTagInlineElements, stringToMap('a,abbr,acronym,b,' + | ||
'bdi,bdo,big,br,cite,code,del,dfn,em,font,i,img,ins,kbd,label,map,mark,q,ruby,rp,rt,s,' + | ||
'samp,small,span,strike,strong,sub,sup,time,tt,u,var')); | ||
|
||
// SVG Elements | ||
// https://wiki.whatwg.org/wiki/Sanitization_rules#svg_Elements | ||
// Note: the elements animate,animateColor,animateMotion,animateTransform,set are intentionally omitted. | ||
// They can potentially allow for arbitrary javascript to be executed. See #11290 | ||
var svgElements = toMap('circle,defs,desc,ellipse,font-face,font-face-name,font-face-src,g,glyph,' + | ||
var svgElements = stringToMap('circle,defs,desc,ellipse,font-face,font-face-name,font-face-src,g,glyph,' + | ||
'hkern,image,linearGradient,line,marker,metadata,missing-glyph,mpath,path,polygon,polyline,' + | ||
'radialGradient,rect,stop,svg,switch,text,title,tspan'); | ||
|
||
// Blocked Elements (will be stripped) | ||
var blockedElements = toMap('script,style'); | ||
var blockedElements = stringToMap('script,style'); | ||
|
||
var validElements = extend({}, | ||
voidElements, | ||
|
@@ -268,17 +362,17 @@ function $SanitizeProvider() { | |
optionalEndTagElements); | ||
|
||
//Attributes that have href and hence need to be sanitized | ||
var uriAttrs = toMap('background,cite,href,longdesc,src,xlink:href,xml:base'); | ||
var uriAttrs = stringToMap('background,cite,href,longdesc,src,xlink:href,xml:base'); | ||
|
||
var htmlAttrs = toMap('abbr,align,alt,axis,bgcolor,border,cellpadding,cellspacing,class,clear,' + | ||
var htmlAttrs = stringToMap('abbr,align,alt,axis,bgcolor,border,cellpadding,cellspacing,class,clear,' + | ||
'color,cols,colspan,compact,coords,dir,face,headers,height,hreflang,hspace,' + | ||
'ismap,lang,language,nohref,nowrap,rel,rev,rows,rowspan,rules,' + | ||
'scope,scrolling,shape,size,span,start,summary,tabindex,target,title,type,' + | ||
'valign,value,vspace,width'); | ||
|
||
// SVG attributes (without "id" and "name" attributes) | ||
// https://wiki.whatwg.org/wiki/Sanitization_rules#svg_Attributes | ||
var svgAttrs = toMap('accent-height,accumulate,additive,alphabetic,arabic-form,ascent,' + | ||
var svgAttrs = stringToMap('accent-height,accumulate,additive,alphabetic,arabic-form,ascent,' + | ||
'baseProfile,bbox,begin,by,calcMode,cap-height,class,color,color-rendering,content,' + | ||
'cx,cy,d,dx,dy,descent,display,dur,end,fill,fill-rule,font-family,font-size,font-stretch,' + | ||
'font-style,font-variant,font-weight,from,fx,fy,g1,g2,glyph-name,gradientUnits,hanging,' + | ||
|
@@ -299,14 +393,24 @@ function $SanitizeProvider() { | |
svgAttrs, | ||
htmlAttrs); | ||
|
||
function toMap(str, lowercaseKeys) { | ||
var obj = {}, items = str.split(','), i; | ||
function stringToMap(str, lowercaseKeys) { | ||
return arrayToMap(str.split(','), lowercaseKeys); | ||
} | ||
|
||
function arrayToMap(items, lowercaseKeys) { | ||
var obj = {}, i; | ||
for (i = 0; i < items.length; i++) { | ||
obj[lowercaseKeys ? lowercase(items[i]) : items[i]] = true; | ||
} | ||
return obj; | ||
} | ||
|
||
function addElementsTo(elementsMap, newElements) { | ||
if (newElements && newElements.length) { | ||
extend(elementsMap, arrayToMap(newElements)); | ||
} | ||
} | ||
|
||
/** | ||
* Create an inert document that contains the dirty HTML that needs sanitizing | ||
* Depending upon browser support we use one of three strategies for doing this. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -293,10 +293,56 @@ describe('HTML', function() { | |
expect(doc).toEqual('<p><img src="x"></p>'); | ||
})); | ||
|
||
describe('Custom white-list support', function() { | ||
|
||
var $sanitizeProvider; | ||
beforeEach(module(function(_$sanitizeProvider_) { | ||
$sanitizeProvider = _$sanitizeProvider_; | ||
|
||
$sanitizeProvider.addValidElements(['foo']); | ||
$sanitizeProvider.addValidElements({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need a test for passing an array instead of an object. |
||
htmlElements: ['foo-button', 'foo-video'], | ||
htmlVoidElements: ['foo-input'], | ||
svgElements: ['foo-svg'] | ||
}); | ||
$sanitizeProvider.addValidAttrs(['foo']); | ||
})); | ||
|
||
it('should allow custom white-listed element', function() { | ||
expectHTML('<foo></foo>').toEqual('<foo></foo>'); | ||
expectHTML('<foo-button></foo-button>').toEqual('<foo-button></foo-button>'); | ||
expectHTML('<foo-video></foo-video>').toEqual('<foo-video></foo-video>'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not a |
||
}); | ||
|
||
it('should allow custom white-listed void element', function() { | ||
expectHTML('<foo-input/>').toEqual('<foo-input>'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if the element passed in is not empty? E.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sanitizer removes closing tag
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is interesting... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, but this is how current sanitizer works. When it meets token There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will make it more explicit in the docs that void elements cannot have content. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cannot or should not? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cannot and should not 😁 |
||
}); | ||
|
||
it('should allow custom white-listed void element to be used with closing tag', function() { | ||
expectHTML('<foo-input></foo-input>').toEqual('<foo-input>'); | ||
}); | ||
|
||
it('should allow custom white-listed attribute', function() { | ||
expectHTML('<foo-input foo="foo"/>').toEqual('<foo-input foo="foo">'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any difference if the attribute is on a blacklisted element? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sanitizer just remove a blacklisted element |
||
}); | ||
|
||
it('should ignore custom white-listed SVG element if SVG disabled', function() { | ||
expectHTML('<foo-svg></foo-svg>').toEqual(''); | ||
}); | ||
|
||
it('should not allow add custom element after service has been instantiated', inject(function($sanitize) { | ||
$sanitizeProvider.addValidElements(['bar']); | ||
expectHTML('<bar></bar>').toEqual(''); | ||
})); | ||
}); | ||
|
||
describe('SVG support', function() { | ||
|
||
beforeEach(module(function($sanitizeProvider) { | ||
$sanitizeProvider.enableSvg(true); | ||
$sanitizeProvider.addValidElements({ | ||
svgElements: ['font-face-uri'] | ||
}); | ||
})); | ||
|
||
it('should accept SVG tags', function() { | ||
|
@@ -314,6 +360,10 @@ describe('HTML', function() { | |
|
||
}); | ||
|
||
it('should allow custom white-listed SVG element', function() { | ||
expectHTML('<font-face-uri></font-face-uri>').toEqual('<font-face-uri></font-face-uri>'); | ||
}); | ||
|
||
it('should sanitize SVG xlink:href attribute values', function() { | ||
expectHTML('<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"><a xlink:href="javascript:alert()"></a></svg>') | ||
.toBeOneOf('<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"><a></a></svg>', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it is not possible to have "void svg" elements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and No, because in sanitizer no type like "void svg" I didn't add such property, but if you know how works sanitizer you could do something like this: