refactor(core): move sanitization into core directory #22540

mhevery · 2018-03-01T23:09:21Z

This is in preparation of having Ivy have sanitization inline.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

vicb · 2018-03-02T06:49:56Z

packages/core/src/render3/instructions.ts

+/**
+ * Function used to sanitize the value before writing it into the renderer.
+ */
+export type SanitizerFn = (value: any) => string;


Stringifyer ? (more generic)

But it is specifically use for sanitization. I feel like strigifier is too generic.

Question is: should "But it is specifically use for sanitization" be "I specifically use for sanitization" ?

vicb · 2018-03-02T06:50:52Z

packages/core/src/render3/instructions.ts

@@ -687,16 +692,17 @@ export function elementEnd() {
 * @param any value The attribute is removed when value is `null` or `undefined`.
 *                  Otherwise the attribute value is set to the stringified value.
 */
-export function elementAttribute(index: number, name: string, value: any): void {
+export function elementAttribute(


update API docs

vicb · 2018-03-02T06:52:21Z

packages/core/src/render3/instructions.ts

@@ -725,6 +732,9 @@ export function elementProperty<T>(index: number, propName: string, value: T | N
    tNode.inputs = generatePropertyAliases(node.flags, BindingDirection.Input);
  }

+  if (sanitizerFn != null) {
+    value = sanitizerFn(stringify(value)) as any;


type SanitizerFn = (value: any) => string;
-> do not stringify ? elementAttribute doesn't above

No, since properties get passed in without strigifing, such as when parent component is passing data to child component. We add the sanitization only when compiler detects img and property src.

Not sure to understand why you stringify(value) then ?
Could you add a comment in the code ?

Sorry maybe I don't understand your question. Can you ask you question in more detail?

vicb · 2018-03-02T06:54:02Z

packages/core/src/render3/instructions.ts

  if (value !== NO_CHANGE) {
    const lElement = data[index] as LElementNode;
    if (value == null) {
      isProceduralRenderer(renderer) ?
          renderer.removeStyle(lElement.native, styleName, RendererStyleFlags3.DashCase) :
          lElement.native.style.removeProperty(styleName);
    } else {
-      const strValue = suffix ? stringify(value) + suffix : stringify(value);
+      let strValue = sanitizerFn == null ? stringify(value) : sanitizerFn(value);
+      if (suffix) strValue = strValue + suffix;


should the (stringify(value) + prefix) be sanitized instead ?

it can't be. Since by appending suffix it becomes string and by that point bypass tokens would be stringified.

it can be or it should be ?
suffix='px" script="evil()"'

we control the suffix, so we know that suffix is safe. You can't get into situation where adding suffix would make the content unsafe.

isn't this suppose to become a public API ?

vicb · 2018-03-02T06:55:22Z

packages/core/src/sanitization/html_sanitizer.ts

+    for (let i = 0; i < elAttrs.length; i++) {
+      const elAttr = elAttrs.item(i);
+      const attrName = elAttr.name;
+      let value = elAttr.value;


move value after the if { continue }

vicb · 2018-03-02T06:57:10Z

packages/core/src/sanitization/html_sanitizer.ts

@@ -156,12 +157,12 @@ class SanitizingHtmlSerializer {
      this.buf.push('="');


while updating this code push(a, b, c) would save some bytes

vicb · 2018-03-02T06:58:43Z

packages/core/src/sanitization/html_sanitizer.ts

      }
    }
  }
 }
+
+function getTemplateContent(el: Node): Node|null {
+  return 'content' in el && isTemplateElement(el) ? (<any>el).content : null;


(<any>el).content could be el.content if isTemplateElement returns el is HTMLTemplateElement

vicb · 2018-03-02T06:59:45Z

packages/core/src/sanitization/html_sanitizer.ts

+function getTemplateContent(el: Node): Node|null {
+  return 'content' in el && isTemplateElement(el) ? (<any>el).content : null;
+}
+function isTemplateElement(el: Node): boolean {


returns el is HTMLTemplateElement ?

vicb · 2018-03-02T07:06:58Z

packages/core/src/sanitization/sanitization.ts

+}
+
+/**
+ * An `html` sanitizer which converts untrusted `html` string into trusted string by removing


which converts untrusted 'html' **string** not a string (any)

vicb · 2018-03-02T07:07:48Z

packages/core/src/sanitization/sanitization.ts

+ * @returns `html` string which is safe to display to user, because all of the dangerous javascript
+ * and urls have been removed.
+ */
+export function htmlSanitizer(unsafeHtml: any): string {


should this be called sanitizeHtml() (the other could be html.sanitize())

changed for all methods

vicb · 2018-03-02T07:07:59Z

packages/core/src/sanitization/sanitization.ts

+}
+
+/**
+ * A `style` sanitizer which converts untrusted `style` string into trusted string by removing


ditto string

vicb · 2018-03-02T07:08:46Z

packages/core/src/sanitization/sanitization.ts

+}
+
+/**
+ * A `url` sanitizer which converts untrusted `url` string into trusted string by removing dangerous


vicb · 2018-03-02T07:12:43Z

packages/core/src/sanitization/style_sanitizer.ts

@@ -98,7 +96,7 @@ export function sanitizeStyle(value: string): string {
  }

  if (isDevMode()) {
-    getDOM().log(
+    console.warn(


console && console.warn ?

Why guard? All system should have it. Also if console is not found that console && .. will throw you have to say typeof console !== 'undefined' && console.warn and we don't do it anywhere else.

There is one place I have on top of my head https://github.com/angular/angular/blob/master/packages/common/src/directives/ng_for_of.ts#L105 - probably wrong there too...

vicb · 2018-03-02T07:12:52Z

packages/core/src/sanitization/url_sanitizer.ts

@@ -51,7 +49,7 @@ export function sanitizeUrl(url: string): string {
  if (url.match(SAFE_URL_PATTERN) || url.match(DATA_URL_PATTERN)) return url;

  if (isDevMode()) {
-    getDOM().log(`WARNING: sanitizing unsafe URL value ${url} (see http://g.co/ng/security#xss)`);
+    console.warn(`WARNING: sanitizing unsafe URL value ${url} (see http://g.co/ng/security#xss)`);


same comment

vicb · 2018-03-02T07:13:11Z

packages/core/test/render3/compiler_canonical/sanitize_spec.ts

+
+describe('compiler sanitization', () => {
+  type $boolean$ = boolean;
+  fit('should translate DOM structure', () => {


vicb · 2018-03-02T07:14:38Z

packages/core/test/render3/compiler_canonical/sanitize_spec.ts

+            $r3$.ɵE(2, 'script');
+            $r3$.ɵe();
+          }
+          $r3$.ɵp(0, 'innerHTML', $r3$.ɵb(ctx.innerHTML), $r3$.ɵhtmlSanitizer);


shorter name for ɵsanitizers ?

we could, they are pretty rare since most properties will not have them.

vicb · 2018-03-02T07:18:10Z

Second commit is not really/only: "feat(ivy): add canonical spec for sanitization" ?

mhevery · 2018-03-02T18:25:10Z

@pkozlowski-opensource This is only sort of breaking change since Render2 still goes through injection. Only Ivy with Render3 would have it fixed. Configuration and tree shaking are the opposing forces which we are trying to balance.

mhevery

PTAL

mhevery · 2018-03-02T18:52:47Z

packages/core/src/render3/instructions.ts

+/**
+ * Function used to sanitize the value before writing it into the renderer.
+ */
+export type SanitizerFn = (value: any) => string;


But it is specifically use for sanitization. I feel like strigifier is too generic.

mhevery · 2018-03-02T19:36:58Z

packages/core/src/render3/instructions.ts

@@ -687,16 +692,17 @@ export function elementEnd() {
 * @param any value The attribute is removed when value is `null` or `undefined`.
 *                  Otherwise the attribute value is set to the stringified value.
 */
-export function elementAttribute(index: number, name: string, value: any): void {
+export function elementAttribute(


mhevery · 2018-03-02T19:40:18Z

packages/core/src/render3/instructions.ts

@@ -725,6 +732,9 @@ export function elementProperty<T>(index: number, propName: string, value: T | N
    tNode.inputs = generatePropertyAliases(node.flags, BindingDirection.Input);
  }

+  if (sanitizerFn != null) {
+    value = sanitizerFn(stringify(value)) as any;


No, since properties get passed in without strigifing, such as when parent component is passing data to child component. We add the sanitization only when compiler detects img and property src.

mhevery · 2018-03-02T19:41:08Z

packages/core/src/render3/instructions.ts

  if (value !== NO_CHANGE) {
    const lElement = data[index] as LElementNode;
    if (value == null) {
      isProceduralRenderer(renderer) ?
          renderer.removeStyle(lElement.native, styleName, RendererStyleFlags3.DashCase) :
          lElement.native.style.removeProperty(styleName);
    } else {
-      const strValue = suffix ? stringify(value) + suffix : stringify(value);
+      let strValue = sanitizerFn == null ? stringify(value) : sanitizerFn(value);
+      if (suffix) strValue = strValue + suffix;


it can't be. Since by appending suffix it becomes string and by that point bypass tokens would be stringified.

mhevery · 2018-03-02T19:43:55Z

packages/core/src/sanitization/html_sanitizer.ts

+    for (let i = 0; i < elAttrs.length; i++) {
+      const elAttr = elAttrs.item(i);
+      const attrName = elAttr.name;
+      let value = elAttr.value;


mhevery · 2018-03-02T19:51:06Z

packages/core/src/sanitization/sanitization.ts

+ * @returns `html` string which is safe to display to user, because all of the dangerous javascript
+ * and urls have been removed.
+ */
+export function htmlSanitizer(unsafeHtml: any): string {


changed for all methods

mhevery · 2018-03-02T19:54:12Z

packages/core/src/sanitization/style_sanitizer.ts

@@ -98,7 +96,7 @@ export function sanitizeStyle(value: string): string {
  }

  if (isDevMode()) {
-    getDOM().log(
+    console.warn(


Why guard? All system should have it. Also if console is not found that console && .. will throw you have to say typeof console !== 'undefined' && console.warn and we don't do it anywhere else.

mhevery · 2018-03-02T19:54:20Z

packages/core/src/sanitization/url_sanitizer.ts

@@ -51,7 +49,7 @@ export function sanitizeUrl(url: string): string {
  if (url.match(SAFE_URL_PATTERN) || url.match(DATA_URL_PATTERN)) return url;

  if (isDevMode()) {
-    getDOM().log(`WARNING: sanitizing unsafe URL value ${url} (see http://g.co/ng/security#xss)`);
+    console.warn(`WARNING: sanitizing unsafe URL value ${url} (see http://g.co/ng/security#xss)`);


same comment

mhevery · 2018-03-02T19:54:27Z

packages/core/test/render3/compiler_canonical/sanitize_spec.ts

+
+describe('compiler sanitization', () => {
+  type $boolean$ = boolean;
+  fit('should translate DOM structure', () => {


mhevery · 2018-03-02T19:54:52Z

packages/core/test/render3/compiler_canonical/sanitize_spec.ts

+            $r3$.ɵE(2, 'script');
+            $r3$.ɵe();
+          }
+          $r3$.ɵp(0, 'innerHTML', $r3$.ɵb(ctx.innerHTML), $r3$.ɵhtmlSanitizer);


we could, they are pretty rare since most properties will not have them.

vicb · 2018-03-02T21:22:06Z

packages/core/src/render3/instructions.ts

 *                  Otherwise the attribute value is set to the stringified value.
+ * @param sanitizerFn An optional function which is responsible for sanitizing the `value`.


ie @param stringifier An optional function used to transform the value typically used for sanitization

fixed all references

vicb · 2018-03-02T21:27:16Z

packages/core/src/sanitization/html_sanitizer.ts

      }
    }
  }
 }
+
+function getTemplateContent(el: Node): Node|null {
+  return 'content' in el && isTemplateElement(el) ? el.content : null;


Not sure why 'content' in el is required here (on top of isTemplateElement )
Add a comment ?

No idea, it is copy paste from DOM adapter. Maybe some browser don't have content?

vicb · 2018-03-02T21:29:36Z

packages/core/src/sanitization/inert_body.ts

      if (attrName === 'xmlns:ns1' || attrName.indexOf('ns1:') === 0) {
-        this.DOM.removeAttribute(el, attrName);
+        el.removeAttribute(attrName);
+        i--;  // compensate for the removal of attribute.


please no. loop backward instead ?

good idea, fixed

vicb · 2018-03-02T21:34:30Z

packages/core/src/render3/instructions.ts

 */
 export function elementStyle<T>(
    index: number, styleName: string, value: T | NO_CHANGE, suffix?: string | null,
    sanitizerFn?: SanitizerFn): void {
+  debugger;


vicb · 2018-03-03T05:09:40Z

packages/core/test/render3/render_util.ts

+       * @param updateBlock Optional instructions which go after the creation block:
+       *          `if (creationMode) { ... } __here__`.
+       */
+      constructor(private createBlock: () => void, updateBlock?: () => void) {


formatting is off here

vicb · 2018-03-03T05:10:04Z

packages/core/test/render3/render_util.ts

+       *          `if (creationMode) { ... } __here__`.
+       */
+      constructor(private createBlock: () => void, updateBlock?: () => void) {
+    this.updateBlock = updateBlock || function() {};


default value ?

vicb · 2018-03-03T05:10:53Z

packages/core/test/render3/render_util.ts

+
+  hostNode: LElementNode;
+
+  updateBlock: () => void;


vicb · 2018-03-03T05:12:07Z

packages/core/test/render3/render_util.ts

+ * - maintaining the template state between invocations,
+ * - access to the render `html`.
+ */
+export class TemplateFixture {


nice !
That was the kind of helper I had in mind

I thought you would. We can really add lots of features on top of the fixture to make testing easier.

mhevery · 2018-03-03T18:15:16Z

http://test/OCL:187738698:BASE:187901321:1520280371053:3f416658

This is in preparation of having Ivy have sanitization inline.

By providing a top level sanitization methods (rather than service) the compiler can generate calls into the methods only when needed. This makes the methods tree shakable.

vicb · 2018-03-06T18:25:11Z

packages/core/src/render3/instructions.ts

  if (value !== NO_CHANGE) {
    const lElement = data[index] as LElementNode;
    if (value == null) {
      isProceduralRenderer(renderer) ?
          renderer.removeStyle(lElement.native, styleName, RendererStyleFlags3.DashCase) :
          lElement.native.style.removeProperty(styleName);
    } else {
-      const strValue = suffix ? stringify(value) + suffix : stringify(value);
+      let strValue = stringifier == null ? stringify(value) : stringifier(value);
+      if (suffix) strValue = strValue + (stringifier == null ? suffix : stringifier(suffix));


What about:

if (stringifier == null) { v = stringify(value); } else { v = suffix ? stringifier(stringify(value) + suffix) : stringifier(value); }

If suffix is the unit, this should work fine ?

vicb · 2018-03-06T18:29:15Z

packages/core/src/sanitization/sanitization.ts

+}
+
+/**
+ * An `html` sanitizer which converts untrusted `html` **string** into trusted string by removing


left as is per our discussion

) By providing a top level sanitization methods (rather than service) the compiler can generate calls into the methods only when needed. This makes the methods tree shakable. PR Close #22540

This is in preparation of having Ivy have sanitization inline. PR Close angular#22540

…ular#22540) By providing a top level sanitization methods (rather than service) the compiler can generate calls into the methods only when needed. This makes the methods tree shakable. PR Close angular#22540

angular-automatic-lock-bot · 2019-09-13T09:52:56Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Mar 1, 2018

alexeagle added the area: core Issues related to the framework runtime label Mar 2, 2018

vicb reviewed Mar 2, 2018

View reviewed changes

mhevery commented Mar 2, 2018

View reviewed changes

mhevery force-pushed the spec_sanitize branch 3 times, most recently from 40b598f to 6cc895d Compare March 2, 2018 21:16

vicb reviewed Mar 2, 2018

View reviewed changes

mhevery force-pushed the spec_sanitize branch from 6cc895d to c995dec Compare March 2, 2018 21:31

vicb reviewed Mar 2, 2018

View reviewed changes

mhevery changed the title ~~refactor(core): move sanitization into common directory~~ refactor(core): move sanitization into core directory Mar 2, 2018

mhevery force-pushed the spec_sanitize branch from fc9c5de to 8fa7bdd Compare March 3, 2018 05:04

vicb reviewed Mar 3, 2018

View reviewed changes

mhevery force-pushed the spec_sanitize branch from 8fa7bdd to e28f6b4 Compare March 3, 2018 05:10

vicb reviewed Mar 3, 2018

View reviewed changes

mhevery added the target: major This PR is targeted for the next major release label Mar 3, 2018

mhevery force-pushed the spec_sanitize branch from c27050c to 3e60aa3 Compare March 5, 2018 22:44

mhevery added 2 commits March 5, 2018 15:02

refactor(core): move sanitization into core

5bb6e11

This is in preparation of having Ivy have sanitization inline.

feat(ivy): provide sanitization methods which can be tree shaken

99dfb79

By providing a top level sanitization methods (rather than service) the compiler can generate calls into the methods only when needed. This makes the methods tree shakable.

mhevery force-pushed the spec_sanitize branch from 3e60aa3 to 99dfb79 Compare March 5, 2018 23:04

mhevery added 2 commits March 5, 2018 16:09

fixup! feat(ivy): provide sanitization methods which can be tree shaken

a22e3be

fixup! feat(ivy): provide sanitization methods which can be tree shaken

87b7ac5

mhevery force-pushed the spec_sanitize branch from 2eea87d to 87b7ac5 Compare March 6, 2018 01:06

vicb reviewed Mar 6, 2018

View reviewed changes

vicb approved these changes Mar 6, 2018

View reviewed changes

mhevery added the action: merge The PR is ready for merge by the caretaker label Mar 6, 2018

fixup! feat(ivy): provide sanitization methods which can be tree shaken

dee3447

mhevery force-pushed the spec_sanitize branch from b7c5fc7 to dee3447 Compare March 6, 2018 23:46

kara closed this in 538f1d9 Mar 8, 2018

marclaval mentioned this pull request Mar 9, 2018

fix(ivy): elements properties should not be stringified #22683

Closed

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018

refactor(core): move sanitization into core (angular#22540)

edefe6e

This is in preparation of having Ivy have sanitization inline. PR Close angular#22540

justinappler mentioned this pull request May 15, 2018

Throw Node is not defined when server-side rendering #23280

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

		@@ -156,12 +157,12 @@ class SanitizingHtmlSerializer {
		this.buf.push('="');

		* Otherwise the attribute value is set to the stringified value.
		* @param sanitizerFn An optional function which is responsible for sanitizing the `value`.

refactor(core): move sanitization into core directory #22540

refactor(core): move sanitization into core directory #22540

Conversation

mhevery commented Mar 1, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb Mar 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb commented Mar 2, 2018

mhevery commented Mar 2, 2018

mhevery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb Mar 2, 2018 •

edited

mhevery commented Mar 3, 2018 •

edited