fix(compiler): do not unquote CSS values (#49460)

alan-agius4 · atscott · commit 1829542aeabd · 2023-03-28T11:35:38.000-07:00
Currently we are unsafely unquoting CSS values which in some cases causes valid values to become invalid and invalid values to become valid. Example: ```html <div style="width:&quot;1px;&quot;"></div> ``` In the above case, `width` has an invalid value of `"1px"`, however the compiler will transform it to `1px` which makes it valid. On the other hand, in the below case ```html <div style="content:&quot;foo&quot;"></div> ``` `content` has a valid value of `"foo"`, but since the compiler unwraps it to `foo` it becomes invalid. For correctness, we should not remove quotes. ```js const div = document.createElement('div'); div.style.width='"1px"'; div.style.content='foo'; div.style.width; // '' div.style.content; // '' div.style.width='1px'; div.style.content='"foo"'; div.style.width; // '1px' div.style.content; // '"foo"' ``` More information about values can be found https://www.w3.org/TR/CSS21/syndata.html#value-def-identifier PR Close #49460
diff --git a/packages/compiler/src/render3/view/style_parser.ts b/packages/compiler/src/render3/view/style_parser.ts
@@ -6,6 +6,9 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+// Any changes here should be ported to the Angular Domino fork.
+// https://github.com/angular/domino/blob/main/lib/style_parser.js
+
 const enum Char {
   OpenParen = 40,
   CloseParen = 41,
@@ -39,7 +42,6 @@ export function parse(value: string): string[] {
   let valueStart = 0;
   let propStart = 0;
   let currentProp: string|null = null;
-  let valueHasQuotes = false;
   while (i < value.length) {
     const token = value.charCodeAt(i++) as Char;
     switch (token) {
@@ -52,7 +54,6 @@ export function parse(value: string): string[] {
       case Char.QuoteSingle:
         // valueStart needs to be there since prop values don't
         // have quotes in CSS
-        valueHasQuotes = valueHasQuotes || valueStart > 0;
         if (quote === Char.QuoteNone) {
           quote = Char.QuoteSingle;
         } else if (quote === Char.QuoteSingle && value.charCodeAt(i - 1) !== Char.BackSlash) {
@@ -61,7 +62,6 @@ export function parse(value: string): string[] {
         break;
       case Char.QuoteDouble:
         // same logic as above
-        valueHasQuotes = valueHasQuotes || valueStart > 0;
         if (quote === Char.QuoteNone) {
           quote = Char.QuoteDouble;
         } else if (quote === Char.QuoteDouble && value.charCodeAt(i - 1) !== Char.BackSlash) {
@@ -77,38 +77,23 @@ export function parse(value: string): string[] {
       case Char.Semicolon:
         if (currentProp && valueStart > 0 && parenDepth === 0 && quote === Char.QuoteNone) {
           const styleVal = value.substring(valueStart, i - 1).trim();
-          styles.push(currentProp, valueHasQuotes ? stripUnnecessaryQuotes(styleVal) : styleVal);
+          styles.push(currentProp, styleVal);
           propStart = i;
           valueStart = 0;
           currentProp = null;
-          valueHasQuotes = false;
         }
         break;
     }
   }
 
   if (currentProp && valueStart) {
     const styleVal = value.slice(valueStart).trim();
-    styles.push(currentProp, valueHasQuotes ? stripUnnecessaryQuotes(styleVal) : styleVal);
+    styles.push(currentProp, styleVal);
   }
 
   return styles;
 }
 
-export function stripUnnecessaryQuotes(value: string): string {
-  const qS = value.charCodeAt(0);
-  const qE = value.charCodeAt(value.length - 1);
-  if (qS == qE && (qS == Char.QuoteSingle || qS == Char.QuoteDouble)) {
-    const tempValue = value.substring(1, value.length - 1);
-    // special case to avoid using a multi-quoted string that was just chomped
-    // (e.g. `font-family: "Verdana", "sans-serif"`)
-    if (tempValue.indexOf('\'') == -1 && tempValue.indexOf('"') == -1) {
-      value = tempValue;
-    }
-  }
-  return value;
-}
-
 export function hyphenate(value: string): string {
   return value
       .replace(
diff --git a/packages/compiler/test/render3/style_parser_spec.ts b/packages/compiler/test/render3/style_parser_spec.ts
@@ -5,7 +5,7 @@
  * Use of this source code is governed by an MIT-style license that can be
  * found in the LICENSE file at https://angular.io/license
  */
-import {hyphenate, parse as parseStyle, stripUnnecessaryQuotes} from '../../src/render3/view/style_parser';
+import {hyphenate, parse as parseStyle} from '../../src/render3/view/style_parser';
 
 describe('style parsing', () => {
   it('should parse empty or blank strings', () => {
@@ -31,16 +31,9 @@ describe('style parsing', () => {
     expect(result).toEqual(['width', '333px', 'height', '666px', 'opacity', '0.5']);
   });
 
-  it('should chomp out start/end quotes', () => {
-    const result = parseStyle(
-        'content: "foo"; opacity: \'0.5\'; font-family: "Verdana", Helvetica, "sans-serif"');
-    expect(result).toEqual(
-        ['content', 'foo', 'opacity', '0.5', 'font-family', '"Verdana", Helvetica, "sans-serif"']);
-  });
-
   it('should not mess up with quoted strings that contain [:;] values', () => {
     const result = parseStyle('content: "foo; man: guy"; width: 100px');
-    expect(result).toEqual(['content', 'foo; man: guy', 'width', '100px']);
+    expect(result).toEqual(['content', '"foo; man: guy"', 'width', '100px']);
   });
 
   it('should not mess up with quoted strings that contain inner quote values', () => {
@@ -64,22 +57,15 @@ describe('style parsing', () => {
     expect(result).toEqual(['border-width', '200px']);
   });
 
-  describe('quote chomping', () => {
-    it('should remove the start and end quotes', () => {
-      expect(stripUnnecessaryQuotes('\'foo bar\'')).toEqual('foo bar');
-      expect(stripUnnecessaryQuotes('"foo bar"')).toEqual('foo bar');
-    });
-
-    it('should not remove quotes if the quotes are not at the start and end', () => {
-      expect(stripUnnecessaryQuotes('foo bar')).toEqual('foo bar');
-      expect(stripUnnecessaryQuotes('   foo bar   ')).toEqual('   foo bar   ');
-      expect(stripUnnecessaryQuotes('\'foo\' bar')).toEqual('\'foo\' bar');
-      expect(stripUnnecessaryQuotes('foo "bar"')).toEqual('foo "bar"');
+  describe('should not remove quotes', () => {
+    it('from string data types', () => {
+      const result = parseStyle('content: "foo"');
+      expect(result).toEqual(['content', '"foo"']);
     });
 
-    it('should not remove quotes if there are inner quotes', () => {
-      const str = '"Verdana", "Helvetica"';
-      expect(stripUnnecessaryQuotes(str)).toEqual(str);
+    it('that changes the value context from invalid to valid', () => {
+      const result = parseStyle('width: "1px"');
+      expect(result).toEqual(['width', '"1px"']);
     });
   });
 
diff --git a/packages/core/test/acceptance/styling_spec.ts b/packages/core/test/acceptance/styling_spec.ts
@@ -123,14 +123,15 @@ describe('styling', () => {
 
       const div = fixture.nativeElement.querySelector('div');
       expect(getSortedClassName(div)).toEqual('HOST_STATIC_1 HOST_STATIC_2 STATIC');
-      expect(getSortedStyle(div)).toEqual('color: blue; font-family: c2;');
+      // Chrome will remove quotes but Firefox and Domino do not.
+      expect(getSortedStyle(div)).toMatch(/color: blue; font-family: "?c2"?;/);
     });
 
     it('should support hostBindings inheritance', () => {
       @Component({template: `<div my-host-bindings class="STATIC" style="color: blue;"></div>`})
       class Cmp {
       }
-      @Directive({host: {'class': 'SUPER_STATIC', 'style': 'font-family: "super"; width: "1px";'}})
+      @Directive({host: {'class': 'SUPER_STATIC', 'style': 'font-family: "super";'}})
       class SuperDir {
       }
       @Directive({
@@ -149,7 +150,29 @@ describe('styling', () => {
       // Browsers keep the '"' around the font name, but Domino removes it some we do search and
       // replace. Yes we could do `replace(/"/g, '')` but that fails on android.
       expect(getSortedStyle(div).replace('"', '').replace('"', ''))
-          .toEqual('color: blue; font-family: host font; width: 1px;');
+          .toEqual('color: blue; font-family: host font;');
+    });
+
+    it('should apply style properties that require quote wrapping', () => {
+      @Component({
+        selector: 'test-style-quoting',
+        template: `
+          <div style="content: &quot;foo&quot;"></div>
+          <div style='content: "foo"'></div>
+          <div style="content: 'foo'"></div>
+        `
+      })
+      class Cmp {
+      }
+
+      TestBed.configureTestingModule({declarations: [Cmp]});
+      const fixture = TestBed.createComponent(Cmp);
+      fixture.detectChanges();
+
+      const divElements = fixture.nativeElement.querySelectorAll('div');
+      expect(getSortedStyle(divElements[0])).toBe('content: "foo";');
+      expect(getSortedStyle(divElements[1])).toBe('content: "foo";');
+      expect(getSortedStyle(divElements[2])).toMatch(/content: ["']foo["'];/);
     });
 
     it('should apply template classes in correct order', () => {
