-
Notifications
You must be signed in to change notification settings - Fork 27.4k
feat($sanitize): support enhancing white-list #16326
Changes from all 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 |
---|---|---|
|
@@ -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({ | ||
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>'); | ||
}); | ||
|
||
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.
no alphabetic ordering??
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 didn't see any signs of "alphabetic ordering" 🤔
Should I change this code?
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.
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 :-)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.
@petebacondarwin looks like @gkalpak fixed it already
1920420#diff-699beb0d8c48a8257d3d70331bf6138dR19
@gkalpak Thanks
Uh oh!
There was an error while loading. Please reload this page.
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.
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 😞)
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.
Fixed while merging (and also removed unused
isObject
var).