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

maksimr · 2017-11-11T13:29:23Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Improve functionality of ngSanitize. More information you could find here - #5900

What is the current behavior? (You can also link to an open issue here)
Hardcoded white-list

What is the new behavior (if this is a feature change)?
Allow adding valid elements and attributes

Does this PR introduce a breaking change?

No

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:

gkalpak

If we are doing this (I am not sure how this would affect security), we need to better document what "valid elements/attributes" are and how they are used by $sanitize.

gkalpak · 2017-11-13T12:11:11Z

src/ngSanitize/sanitize.js

+
+    if (angular.isObject(elements)) {
+      _validElements = [];
+      angular.forEach(['voidElements', 'inlineElements', 'blockElements', 'svgElements'], function(elementsType) {


This doesn't take into account whether svgEnabled is true or false.

gkalpak · 2017-11-13T12:13:48Z

src/ngSanitize/sanitize.js

+   * @name $sanitizeProvider#addValidElements
+   * @kind function
+   *
+   * @param {Array|Object=} elements List of valid elements.


Why the =? This is not optional afaict.

Copy-paste (shame on me)

gkalpak · 2017-11-13T12:18:22Z

src/ngSanitize/sanitize.js

+   * @kind function
+   *
+   * @param {Array} attrs List of valid attributes.
+   */


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.

gkalpak · 2017-11-13T12:20:11Z

test/ngSanitize/sanitizeSpec.js

+  describe('Custom white-list support', function() {
+    beforeEach(module(function($sanitizeProvider) {
+      $sanitizeProvider.addValidElements({
+        inlineElements: ['button'],


We need tests for all supported categories (e.g. blockElements, svgElements).

gkalpak · 2017-11-13T12:21:35Z

test/ngSanitize/sanitizeSpec.js

@@ -280,6 +280,29 @@ describe('HTML', function() {
    expect(doc).toEqual('<p><img src="x"></p>');
  }));

+  describe('Custom white-list support', function() {
+    beforeEach(module(function($sanitizeProvider) {
+      $sanitizeProvider.addValidElements({


We need a test for passing an array instead of an object.

gkalpak · 2017-11-13T12:22:06Z

test/ngSanitize/sanitizeSpec.js

+      expectHTML('<button></button>').toEqual('<button></button>');
+    });
+
+    it('should correct handle custom white-listed void element', function() {


correct --> correctly

gkalpak · 2017-11-13T12:23:53Z

test/ngSanitize/sanitizeSpec.js

+      $sanitizeProvider.addValidAttrs(['type']);
+    }));
+
+    it('should not ignore custom white-listed element', function() {


"should not ignore" is not very straight forward. I would change it to something like "should allow".

gkalpak · 2017-11-13T12:24:43Z

test/ngSanitize/sanitizeSpec.js

+        voidElements: ['input']
+      });
+
+      $sanitizeProvider.addValidAttrs(['type']);


type is already a valid attribute.

gkalpak · 2017-11-13T12:32:58Z

src/ngSanitize/sanitize.js

+          if (elementsType === 'voidElements') {
+            angular.extend(voidElements, toMap(elements.voidElements.join(',')));
+          }
+          _validElements = _validElements.concat(elements[elementsType]);


Since we treat most categories (except for voidElements and svgElements) the same, I am wondering if it would be more straight forward to use a simpler public API, e.g.:

{ htmlElements: [...], htmlVoidElements: [...], svgElements: [...], }

Looks good to me

Narretz · 2017-11-13T17:56:01Z

One problem with this is that theortically any 3rd party module can add a config block that changes the whitelist, possibly allowing things you don't want to have in your app.

maksimr · 2017-11-13T20:02:58Z

If we are doing this (I am not sure how this would affect security), we need to better document what "valid elements/attributes" are and how they are used by $sanitize

@gkalpak any documentation text welcome

One problem with this is that theortically any 3rd party module can add a config block that changes the whitelist, possibly allowing things you don't want to have in your app.

@Narretz yours suggestions?
Theoretically, any 3rd party module can replace $sanitize service

Narretz · 2017-11-15T09:50:54Z

It's true that the sanitizer can be replaced completely. Maybe we should add a note about the security implications of this, though.

maksimr · 2017-11-16T11:56:29Z

@gkalpak is it ok now?

Thanks

gkalpak · 2017-11-21T13:34:47Z

src/ngSanitize/sanitize.js

+   * @name $sanitizeProvider#addValidAttrs
+   * @kind function
+   *
+   * @param {Array} attrs List of valid attributes. The added attributes will not be treated as URI attributes


I would expand a bit on the non-URI part:

[...] The added attributes will not be treated as URI attributes, which means their values will not sanitized as URIs using the aHrefSanitizationWhitelist and imgSrcSanitizationWhitelist of {@link ng.$compileProvider $compileProvider}.

gkalpak · 2017-11-21T13:54:57Z

src/ngSanitize/sanitize.js

+        }
+
+        if (elementsType === 'svgElements') {
+          angular.extend(svgElements, toMap(elements[elementsType].join(',')));


Extending svgElements has no effect if called after the service has been instantiated. This can be unintuitive (because theoretically it is possible to use provider methods after a service has been instantiated and in many cases everything works as expected).

I think it would be better to either throw or make it a no-op after instantiation (and document that).

As I see enableSvg also doesn't work after instantiation.

@gkalpak

I think it would be better to either throw or make it a no-op after instantiation (and document that).

How can I check that service has been instantiated? (without additional variable)

You can't (without additional variable) 😁

gkalpak · 2017-11-21T13:58:53Z

test/ngSanitize/sanitizeSpec.js

@@ -280,10 +280,51 @@ describe('HTML', function() {
    expect(doc).toEqual('<p><img src="x"></p>');
  }));

+  describe('Custom white-list support', function() {
+    beforeEach(module(function($sanitizeProvider) {
+      $sanitizeProvider.addValidElements(['form']);


In order to make sure this test does indeed test what it is supposed to (now and in the future), it might be better to add arbitrary elements (that do not exist in the HTML5 spec) to ensure that they are not already considered valid.

gkalpak · 2017-11-21T13:59:45Z

test/ngSanitize/sanitizeSpec.js

+    }));
+
+    it('should allow custom white-listed element', function() {
+      expectHTML('<form></form>').toEqual('<form></form>');


There should be a test that this is not allowed as voidElement.

gkalpak · 2017-11-21T14:01:49Z

test/ngSanitize/sanitizeSpec.js

+    });
+
+    it('should allow custom white-listed void element', function() {
+      expectHTML('<input/>').toEqual('<input>');


There should be a test that this is also allowed as non-void element.

I have added check

expectHTML('<input/>').not.toEqual('<input></input>');

No, I meant something like expectHTML('<input></input>') (which is accepted atm).

🤔 I didn't get your point.

There should be a test that this is also allowed as non-void element.

We already have such test
https://github.com/angular/angular.js/pull/16326/files#diff-85da78d5073ece90913c9e733cfcdef4R306

Yes, the test above not about input but I don't think that input tag is something special.
I have renamed input to foo-input to emphasize that name of the tag isn't matter

That test is for a custom non-void element.
There should be a test that custom void element can be used as <foo-input></foo-input>.

🆙 Done

https://github.com/angular/angular.js/pull/16326/files#diff-85da78d5073ece90913c9e733cfcdef4R314

gkalpak

Looking good. We're getting close 😃

gkalpak · 2017-12-05T09:37:08Z

src/ngSanitize/sanitize.js

+   * @kind function
+   *
+   * @param {Array|Object} elements List of valid elements.
+   */


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?

gkalpak · 2017-12-05T09:38:05Z

src/ngSanitize/sanitize.js

+   * @name $sanitizeProvider#addValidElements
+   * @kind function
+   *
+   * @param {Array|Object} elements List of valid elements.


This should mention the "shape" of the Object version.

gkalpak · 2017-12-05T09:39:21Z

src/ngSanitize/sanitize.js

+      addElementsTo(validElements, elements['htmlElements']);
+    }
+
+    function addElementsTo(elementsMap, newElements) {


There is no reason for this function to be inside the closure. Move it outside to avoid re-creating it on every invokation.

gkalpak · 2017-12-05T09:45:29Z

src/ngSanitize/sanitize.js

+
+    function addElementsTo(elementsMap, newElements) {
+      if (newElements && newElements.length) {
+        return angular.extend(elementsMap, toMap(newElements.join(',')));


The return value is ignored. You can drop it.
It also seems redundant to join() items only to split them right away (inside toMap()). Can you refactor toMap() to avoid that? Something like:

+function stringToMap(str, lowercaseKeys) { + return arrayToMap(str.split(','), lowercaseKeys); +} -function toMap(str, lowercaseKeys) { - var obj = {}, items = str.split(','), i; +function arrayToMap(items, lowercaseKeys) { + var obj = {}, i; for (i = 0; i < items.length; i++) { obj[lowercaseKeys ? lowercase(items[i]) : items[i]] = true; } return obj; }

Also, use the extend function from the closure here and elsewhere (see below the Private stuff comment).
Do the same for other helpers (isArray, isObject). (Define the ones that are not defined already in "Private stuff".)

gkalpak · 2017-12-05T09:54:58Z

src/ngSanitize/sanitize.js

+   *
+   * @param {Array|Object} elements List of valid elements.
+   */
+  this.addValidElements = function(elements) {


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

gkalpak · 2017-12-05T09:59:14Z

src/ngSanitize/sanitize.js

+   *
+   * @param {Array} attrs List of valid attributes The added attributes will not be treated as URI attributes, which means their values will
+   * not sanitized as URIs using the aHrefSanitizationWhitelist and imgSrcSanitizationWhitelist of {@link ng.$compileProvider $compileProvider}.
+   */


This needs more docs as well. You could also move most of the param description into the main description of the function.

gkalpak · 2017-12-05T10:00:35Z

test/ngSanitize/sanitizeSpec.js

+
+    it('should not allow add custom element after service has been instantiated', inject(function($sanitize) {
+      $sanitizeProvider.addValidElements(['bar']);
+      expect($sanitize).toBeDefined();


This assertion is unnecessary.

gkalpak · 2017-12-05T10:02:46Z

test/ngSanitize/sanitizeSpec.js

+    });
+
+    it('should allow custom white-listed block element', function() {
+      expectHTML('<foo-video></foo-video>').toEqual('<foo-video></foo-video>');


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

gkalpak · 2017-12-05T10:02:59Z

test/ngSanitize/sanitizeSpec.js

+      expectHTML('<foo></foo>').toEqual('<foo></foo>');
+    });
+
+    it('should allow custom white-listed inline element', function() {


This is not necessarily a inline element. Just call it element 😃
And merge it with the above test.

gkalpak · 2017-12-05T10:04:27Z

test/ngSanitize/sanitizeSpec.js

+      expectHTML('<foo-input/>').toEqual('<foo-input>');
+    });
+
+    it('should correct handle void element', function() {


Let's change this to:
should allow custom white-listed void element to be used with closing tag

maksimr · 2017-12-07T21:23:01Z

@gkalpak could you provide documentation for these methods

#16326 (comment)
#16326 (comment)

maksimr · 2017-12-21T20:13:20Z

@gkalpak ping

googlebot · 2018-02-05T13:43:24Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-02-05T13:43:24Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

gkalpak · 2018-02-05T13:46:57Z

I've rebased this on master, made some small changes and added docs.

petebacondarwin · 2018-02-05T14:12:04Z

src/ngSanitize/sanitize.js

@@ -207,6 +299,8 @@ function $SanitizeProvider() {
  extend = angular.extend;
  forEach = angular.forEach;
  isDefined = angular.isDefined;
+  isArray = angular.isArray;


no alphabetic ordering??

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

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 :-)

@petebacondarwin looks like @gkalpak fixed it already

1920420#diff-699beb0d8c48a8257d3d70331bf6138dR19

@gkalpak Thanks

~~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 😞)

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

petebacondarwin · 2018-02-05T14:15:58Z

test/ngSanitize/sanitizeSpec.js

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


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

sanitizer removes closing tag

<foo-input>some content

That is interesting...

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.

I will make it more explicit in the docs that void elements cannot have content.

cannot or should not?

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

petebacondarwin · 2018-02-05T14:17:15Z

test/ngSanitize/sanitizeSpec.js

+    });
+
+    it('should allow custom white-listed attribute', function() {
+      expectHTML('<foo-input foo="foo"/>').toEqual('<foo-input foo="foo">');


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

sanitizer just remove a blacklisted element

petebacondarwin · 2018-02-05T14:17:57Z

src/ngSanitize/sanitize.js

+   *
+   * - `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`.


I guess it is not possible to have "void svg" elements?

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'] });

petebacondarwin

LGTM - a couple of unimportant questions.

Narretz · 2018-02-05T15:08:59Z

So we don't need an extra notice that this method should be used carefully, because adding more elements can jeopardize the security of the app?

petebacondarwin · 2018-02-05T16:21:58Z

I assume the purpose of this feature is to support HTML5 custom web elements?

maksimr · 2018-02-05T17:29:22Z

I assume the purpose of this feature is to support HTML5 custom web elements?

Not only, in our case we want support for checkboxes.

Narretz · 2018-02-05T19:45:37Z

It can also be used to add elements / attributes where we haven't decided if they should be whitelisted: #13282

gkalpak · 2018-02-06T11:45:35Z

So we don't need an extra notice that this method should be used carefully, because adding more elements can jeopardize the security of the app?

A good thing is that you can't "overwrite" the blockedElements list. So, even adding script or style (which are currently blocked) will not make them "safe". But you can add other elements and you can definitely add attributes.
I'll add a note to the docs.

Not only, in our case we want support for checkboxes.

@maksimr, can you elaborate? What do you mean by "checkboxes"?

maksimr · 2018-02-06T11:51:41Z

@gkalpak sure,

"checkboxes" is <input type=checkbox/>

gkalpak · 2018-02-06T12:05:02Z

Oh, OK. With this change you will have to whitelist all input elements (so be careful 😃).

gkalpak · 2018-02-06T12:08:26Z

Updated the docs. Will merge once Travis is green.
(If you want to TAL, now is your chance, @petebacondarwin @Narretz 😃)

Fixes #5900 Closes #16326

googlebot added the cla: yes label Nov 11, 2017

gkalpak suggested changes Nov 13, 2017

View reviewed changes

gkalpak suggested changes Nov 21, 2017

View reviewed changes

gkalpak added component: ngSanitize frequency: low needs: review severity: inconvenient type: feature labels Dec 5, 2017

gkalpak added this to the Backlog milestone Dec 5, 2017

gkalpak suggested changes Dec 5, 2017

View reviewed changes

gkalpak self-assigned this Feb 2, 2018

maksimr and others added 2 commits February 5, 2018 15:41

feat($sanitize): support enhancing white-list

0b51380

fixup! feat($sanitize): support enhancing white-list

1920420

googlebot added cla: no and removed cla: yes labels Feb 5, 2018

gkalpak approved these changes Feb 5, 2018

View reviewed changes

petebacondarwin reviewed Feb 5, 2018

View reviewed changes

petebacondarwin approved these changes Feb 5, 2018

View reviewed changes

fixup! fixup! feat($sanitize): support enhancing white-list

fc05208

gkalpak pushed a commit that referenced this pull request Feb 6, 2018

feat($sanitize): support enhancing elements/attributes white-lists

ee8e05c

Fixes #5900 Closes #16326

gkalpak closed this in 8d6ac5f Feb 6, 2018

This was referenced Apr 18, 2018

Update angular in package.json from 1.6.9 to 1.6.10 ui-router/sample-app-angular-hybrid#69

Closed

Update 4 dependencies from npm JetBrains/ring-ui#226

Merged

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

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

Uh oh!

Conversation

maksimr commented Nov 11, 2017

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narretz commented Nov 13, 2017

Uh oh!

maksimr commented Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Narretz commented Nov 15, 2017

Uh oh!

maksimr commented Nov 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maksimr Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gkalpak Nov 13, 2017 •

edited

Loading

maksimr commented Nov 13, 2017 •

edited

Loading

gkalpak Nov 21, 2017 •

edited

Loading

maksimr Dec 4, 2017 •

edited

Loading