Skip to content

Commit

Permalink
fix(compiler): incorrectly encapsulating @import containing colons an…
Browse files Browse the repository at this point in the history 
…d semicolons (angular#38716)

At a high level, the current shadow DOM shim logic works by escaping the content of a CSS rule
(e.g. `div {color: red;}` becomes `div {%BLOCK%}`), using a regex to parse out things like the
selector and the rule body, and then re-adding the content after the selector has been modified.
The problem is that the regex has to be very broad in order capture all of the different use cases,
which can cause it to match strings suffixed with a semi-colon in some places where it shouldn't,
like this URL from Google Fonts `https://fonts.googleapis.com/css2?family=Roboto:wght@400;500&display=swap`.
Most of the time this is fine, because the logic that escapes the rule content to `%BLOCK%` will
have converted it to something that won't be matched by the regex. However, it breaks down for rules
like `@import` which don't have a body, but can still have quoted content with characters that can
match the regex.

These changes resolve the issue by making a second pass over the escaped string and replacing all
of the remaining quoted content with `%QUOTED%` before parsing it with the regex. Once everything
has been processed, we make a final pass where we restore the quoted content.

In a previous iteration of this PR, I went with a shorter approach which narrowed down the
regex so that it doesn't capture rules without a body. It fixed the issue, but it also ended
up breaking some of the more contrived unit test cases. I decided not to pursue it further, because
we would've ended up with a very long and brittle regex that likely would've broken in even weirder
ways.

Fixes angular#38587.

PR Close angular#38716
  • Loading branch information
@crisbeto @atscott
crisbeto authored and atscott committed Oct 9, 2020
1 parent ca4ef61 commit 7f689a2
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 41 deletions.
99 changes: 58 additions & 41 deletions packages/compiler/src/shadow_css.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -555,66 +555,83 @@ function extractCommentsWithHash(input: string): string[] {
return input.match(_commentWithHashRe) || [];
}

const _ruleRe = /(\s*)([^;\{\}]+?)(\s*)((?:{%BLOCK%}?\s*;?)|(?:\s*;))/g;
const _curlyRe = /([{}])/g;
const OPEN_CURLY = '{';
const CLOSE_CURLY = '}';
const BLOCK_PLACEHOLDER = '%BLOCK%';
const QUOTE_PLACEHOLDER = '%QUOTED%';
const _ruleRe = /(\s*)([^;\{\}]+?)(\s*)((?:{%BLOCK%}?\s*;?)|(?:\s*;))/g;
const _quotedRe = /%QUOTED%/g;
const CONTENT_PAIRS = new Map([['{', '}']]);
const QUOTE_PAIRS = new Map([[`"`, `"`], [`'`, `'`]]);

export class CssRule {
constructor(public selector: string, public content: string) {}
}

export function processRules(input: string, ruleCallback: (rule: CssRule) => CssRule): string {
const inputWithEscapedBlocks = escapeBlocks(input);
const inputWithEscapedQuotes = escapeBlocks(input, QUOTE_PAIRS, QUOTE_PLACEHOLDER);
const inputWithEscapedBlocks =
escapeBlocks(inputWithEscapedQuotes.escapedString, CONTENT_PAIRS, BLOCK_PLACEHOLDER);
let nextBlockIndex = 0;
return inputWithEscapedBlocks.escapedString.replace(_ruleRe, function(...m: string[]) {
const selector = m[2];
let content = '';
let suffix = m[4];
let contentPrefix = '';
if (suffix && suffix.startsWith('{' + BLOCK_PLACEHOLDER)) {
content = inputWithEscapedBlocks.blocks[nextBlockIndex++];
suffix = suffix.substring(BLOCK_PLACEHOLDER.length + 1);
contentPrefix = '{';
}
const rule = ruleCallback(new CssRule(selector, content));
return `${m[1]}${rule.selector}${m[3]}${contentPrefix}${rule.content}${suffix}`;
});
let nextQuoteIndex = 0;
return inputWithEscapedBlocks.escapedString
.replace(
_ruleRe,
(...m: string[]) => {
const selector = m[2];
let content = '';
let suffix = m[4];
let contentPrefix = '';
if (suffix && suffix.startsWith('{' + BLOCK_PLACEHOLDER)) {
content = inputWithEscapedBlocks.blocks[nextBlockIndex++];
suffix = suffix.substring(BLOCK_PLACEHOLDER.length + 1);
contentPrefix = '{';
}
const rule = ruleCallback(new CssRule(selector, content));
return `${m[1]}${rule.selector}${m[3]}${contentPrefix}${rule.content}${suffix}`;
})
.replace(_quotedRe, () => inputWithEscapedQuotes.blocks[nextQuoteIndex++]);
}

class StringWithEscapedBlocks {
constructor(public escapedString: string, public blocks: string[]) {}
}

function escapeBlocks(input: string): StringWithEscapedBlocks {
const inputParts = input.split(_curlyRe);
function escapeBlocks(
input: string, charPairs: Map<string, string>, placeholder: string): StringWithEscapedBlocks {
const resultParts: string[] = [];
const escapedBlocks: string[] = [];
let bracketCount = 0;
let currentBlockParts: string[] = [];
for (let partIndex = 0; partIndex < inputParts.length; partIndex++) {
const part = inputParts[partIndex];
if (part == CLOSE_CURLY) {
bracketCount--;
}
if (bracketCount > 0) {
currentBlockParts.push(part);
} else {
if (currentBlockParts.length > 0) {
escapedBlocks.push(currentBlockParts.join(''));
resultParts.push(BLOCK_PLACEHOLDER);
currentBlockParts = [];
let openCharCount = 0;
let nonBlockStartIndex = 0;
let blockStartIndex = -1;
let openChar: string|undefined;
let closeChar: string|undefined;
for (let i = 0; i < input.length; i++) {
const char = input[i];
if (char === '\\') {
i++;
} else if (char === closeChar) {
openCharCount--;
if (openCharCount === 0) {
escapedBlocks.push(input.substring(blockStartIndex, i));
resultParts.push(placeholder);
nonBlockStartIndex = i;
blockStartIndex = -1;
openChar = closeChar = undefined;
}
resultParts.push(part);
}
if (part == OPEN_CURLY) {
bracketCount++;
} else if (char === openChar) {
openCharCount++;
} else if (openCharCount === 0 && charPairs.has(char)) {
openChar = char;
closeChar = charPairs.get(char);
openCharCount = 1;
blockStartIndex = i + 1;
resultParts.push(input.substring(nonBlockStartIndex, blockStartIndex));
}
}
if (currentBlockParts.length > 0) {
escapedBlocks.push(currentBlockParts.join(''));
resultParts.push(BLOCK_PLACEHOLDER);
if (blockStartIndex !== -1) {
escapedBlocks.push(input.substring(blockStartIndex));
resultParts.push(placeholder);
} else {
resultParts.push(input.substring(nonBlockStartIndex));
}
return new StringWithEscapedBlocks(resultParts.join(''), escapedBlocks);
}
40 changes: 40 additions & 0 deletions packages/compiler/test/shadow_css_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,28 @@ import {normalizeCSS} from '@angular/platform-browser/testing/src/browser_util';
expect(css).toEqual('@import url("a"); div[contenta] {}');
});

it('should shim rules with quoted content after @import', () => {
const styleStr = '@import url("a"); div {background-image: url("a.jpg"); color: red;}';
const css = s(styleStr, 'contenta');
expect(css).toEqual(
'@import url("a"); div[contenta] {background-image:url("a.jpg"); color:red;}');
});

it('should pass through @import directives whose URL contains colons and semicolons', () => {
const styleStr =
'@import url("https://fonts.googleapis.com/css2?family=Roboto:wght@400;500&display=swap");';
const css = s(styleStr, 'contenta');
expect(css).toEqual(styleStr);
});

it('should shim rules after @import with colons and semicolons', () => {
const styleStr =
'@import url("https://fonts.googleapis.com/css2?family=Roboto:wght@400;500&display=swap"); div {}';
const css = s(styleStr, 'contenta');
expect(css).toEqual(
'@import url("https://fonts.googleapis.com/css2?family=Roboto:wght@400;500&display=swap"); div[contenta] {}');
});

it('should leave calc() unchanged', () => {
const styleStr = 'div {height:calc(100% - 55px);}';
const css = s(styleStr, 'contenta');
Expand Down Expand Up @@ -312,6 +334,24 @@ import {normalizeCSS} from '@angular/platform-browser/testing/src/browser_util';
expect(s('/*# sourceMappingURL=data:x */b {c}/*# sourceURL=xxx */', 'contenta'))
.toEqual('b[contenta] {c}/*# sourceMappingURL=data:x *//*# sourceURL=xxx */');
});

it('should shim rules with quoted content', () => {
const styleStr = 'div {background-image: url("a.jpg"); color: red;}';
const css = s(styleStr, 'contenta');
expect(css).toEqual('div[contenta] {background-image:url("a.jpg"); color:red;}');
});

it('should shim rules with an escaped quote inside quoted content', () => {
const styleStr = 'div::after { content: "\\"" }';
const css = s(styleStr, 'contenta');
expect(css).toEqual('div[contenta]::after { content:"\\""}');
});

it('should shim rules with curly braces inside quoted content', () => {
const styleStr = 'div::after { content: "{}" }';
const css = s(styleStr, 'contenta');
expect(css).toEqual('div[contenta]::after { content:"{}"}');
});
});

describe('processRules', () => {
Expand Down

0 comments on commit 7f689a2

Please sign in to comment.