Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat($sanitize): support enhancing white-list #16326

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 117 additions & 13 deletions src/ngSanitize/sanitize.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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`.
Copy link
Contributor

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?

Copy link
Contributor Author

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:

      $sanitizeProvider.addValidElements({
        htmlVoidElements: ['font-face-uri'],
        svgElements: ['font-face-uri']
      });

*
* <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.
*/
Copy link
Member

Choose a reason for hiding this comment

The 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 $sanitize?

this.addValidElements = function(elements) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's have this method return this to be inline with what we do with other setter methods (helps with chaining).

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.
*/
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's have this method return this as well.

if (!hasBeenInstantiated) {
extend(validAttrs, arrayToMap(attrs, true));
}
return this;
};

//////////////////////////////////////////////////////////////////////////////////////////////////
// Private stuff
//////////////////////////////////////////////////////////////////////////////////////////////////
Expand All @@ -207,6 +299,8 @@ function $SanitizeProvider() {
extend = angular.extend;
forEach = angular.forEach;
isDefined = angular.isDefined;
isArray = angular.isArray;
Copy link
Contributor

Choose a reason for hiding this comment

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

no alphabetic ordering??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see any signs of "alphabetic ordering" 🤔
Should I change this code?

Copy link
Contributor

Choose a reason for hiding this comment

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

The var declarations at the top of the file are listed in alphabetical order; and @gkalpak is usually a stickler for this kind of thing :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@gkalpak gkalpak Feb 6, 2018

Choose a reason for hiding this comment

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

Ts, ts, ts! How could you think I wouldn't have fixed that, @petebacondarwin 😛
😱 😱 😱 😱 😱 I fixed it in one of two places 😱 😱 😱 😱 😱
Thx, @petebacondarwin ❤️ (Sorry I doubted you 😞)

Copy link
Member

Choose a reason for hiding this comment

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

Fixed while merging (and also removed unused isObject var).

isObject = angular.isObject;
lowercase = angular.$$lowercase;
noop = angular.noop;

Expand All @@ -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,
Expand All @@ -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,' +
Expand All @@ -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.
Expand Down
50 changes: 50 additions & 0 deletions test/ngSanitize/sanitizeSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Copy link
Member

Choose a reason for hiding this comment

The 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>');
Copy link
Member

Choose a reason for hiding this comment

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

This is not a block element. Let's merge into the tests above.

});

it('should allow custom white-listed void element', function() {
expectHTML('<foo-input/>').toEqual('<foo-input>');
Copy link
Contributor

@petebacondarwin petebacondarwin Feb 5, 2018

Choose a reason for hiding this comment

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

What happens if the element passed in is not empty? E.g. <foo-input>some content</foo-input>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sanitizer removes closing tag

<foo-input>some content

Copy link
Contributor

Choose a reason for hiding this comment

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

That is interesting...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, but this is how current sanitizer works.

When it meets token <foo-inpupt> it treats this as void element because it's valid case when self-closing element(void) does not have /, after that it meets text some content and we do nothing and finally we meet </foo-input> and because there are no open element before we just remove it.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

cannot or should not?

Copy link
Member

Choose a reason for hiding this comment

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

Cannot and should not 😁
As @maksimr mentioned $sanitize (and browsers) will transform <void-element>foo</void-element> to <void-element>foo (so the content ends up outside the element).

});

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">');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any difference if the attribute is on a blacklisted element?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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() {
Expand All @@ -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>',
Expand Down