Sitelet https://github.com/angular/angular/commit/52a0c6b
Skip to content

Commit 52a0c6b

Browse files
crisbetoatscott
crisbeto
authored and
atscott
committed
fix(compiler): incorrectly encapsulating @import containing colons and semicolons (#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 #38587. PR Close #38716
1 parent 8913aee commit 52a0c6b

2 files changed

Lines changed: 98 additions & 41 deletions

File tree

‎packages/compiler/src/shadow_css.ts‎

Lines changed: 58 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -555,66 +555,83 @@ function extractCommentsWithHash(input: string): string[] {
555555
return input.match(_commentWithHashRe) || [];
556556
}
557557

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

564565
export class CssRule {
565566
constructor(public selector: string, public content: string) {}
566567
}
567568

568569
export function processRules(input: string, ruleCallback: (rule: CssRule) => CssRule): string {
569-
const inputWithEscapedBlocks = escapeBlocks(input);
570+
const inputWithEscapedQuotes = escapeBlocks(input, QUOTE_PAIRS, QUOTE_PLACEHOLDER);
571+
const inputWithEscapedBlocks =
572+
escapeBlocks(inputWithEscapedQuotes.escapedString, CONTENT_PAIRS, BLOCK_PLACEHOLDER);
570573
let nextBlockIndex = 0;
571-
return inputWithEscapedBlocks.escapedString.replace(_ruleRe, function(...m: string[]) {
572-
const selector = m[2];
573-
let content = '';
574-
let suffix = m[4];
575-
let contentPrefix = '';
576-
if (suffix && suffix.startsWith('{' + BLOCK_PLACEHOLDER)) {
577-
content = inputWithEscapedBlocks.blocks[nextBlockIndex++];
578-
suffix = suffix.substring(BLOCK_PLACEHOLDER.length + 1);
579-
contentPrefix = '{';
580-
}
581-
const rule = ruleCallback(new CssRule(selector, content));
582-
return `${m[1]}${rule.selector}${m[3]}${contentPrefix}${rule.content}${suffix}`;
583-
});
574+
let nextQuoteIndex = 0;
575+
return inputWithEscapedBlocks.escapedString
576+
.replace(
577+
_ruleRe,
578+
(...m: string[]) => {
579+
const selector = m[2];
580+
let content = '';
581+
let suffix = m[4];
582+
let contentPrefix = '';
583+
if (suffix && suffix.startsWith('{' + BLOCK_PLACEHOLDER)) {
584+
content = inputWithEscapedBlocks.blocks[nextBlockIndex++];
585+
suffix = suffix.substring(BLOCK_PLACEHOLDER.length + 1);
586+
contentPrefix = '{';
587+
}
588+
const rule = ruleCallback(new CssRule(selector, content));
589+
return `${m[1]}${rule.selector}${m[3]}${contentPrefix}${rule.content}${suffix}`;
590+
})
591+
.replace(_quotedRe, () => inputWithEscapedQuotes.blocks[nextQuoteIndex++]);
584592
}
585593

586594
class StringWithEscapedBlocks {
587595
constructor(public escapedString: string, public blocks: string[]) {}
588596
}
589597

590-
function escapeBlocks(input: string): StringWithEscapedBlocks {
591-
const inputParts = input.split(_curlyRe);
598+
function escapeBlocks(
599+
input: string, charPairs: Map<string, string>, placeholder: string): StringWithEscapedBlocks {
592600
const resultParts: string[] = [];
593601
const escapedBlocks: string[] = [];
594-
let bracketCount = 0;
595-
let currentBlockParts: string[] = [];
596-
for (let partIndex = 0; partIndex < inputParts.length; partIndex++) {
597-
const part = inputParts[partIndex];
598-
if (part == CLOSE_CURLY) {
599-
bracketCount--;
600-
}
601-
if (bracketCount > 0) {
602-
currentBlockParts.push(part);
603-
} else {
604-
if (currentBlockParts.length > 0) {
605-
escapedBlocks.push(currentBlockParts.join(''));
606-
resultParts.push(BLOCK_PLACEHOLDER);
607-
currentBlockParts = [];
602+
let openCharCount = 0;
603+
let nonBlockStartIndex = 0;
604+
let blockStartIndex = -1;
605+
let openChar: string|undefined;
606+
let closeChar: string|undefined;
607+
for (let i = 0; i < input.length; i++) {
608+
const char = input[i];
609+
if (char === '\\') {
610+
i++;
611+
} else if (char === closeChar) {
612+
openCharCount--;
613+
if (openCharCount === 0) {
614+
escapedBlocks.push(input.substring(blockStartIndex, i));
615+
resultParts.push(placeholder);
616+
nonBlockStartIndex = i;
617+
blockStartIndex = -1;
618+
openChar = closeChar = undefined;
608619
}
609-
resultParts.push(part);
610-
}
611-
if (part == OPEN_CURLY) {
612-
bracketCount++;
620+
} else if (char === openChar) {
621+
openCharCount++;
622+
} else if (openCharCount === 0 && charPairs.has(char)) {
623+
openChar = char;
624+
closeChar = charPairs.get(char);
625+
openCharCount = 1;
626+
blockStartIndex = i + 1;
627+
resultParts.push(input.substring(nonBlockStartIndex, blockStartIndex));
613628
}
614629
}
615-
if (currentBlockParts.length > 0) {
616-
escapedBlocks.push(currentBlockParts.join(''));
617-
resultParts.push(BLOCK_PLACEHOLDER);
630+
if (blockStartIndex !== -1) {
631+
escapedBlocks.push(input.substring(blockStartIndex));
632+
resultParts.push(placeholder);
633+
} else {
634+
resultParts.push(input.substring(nonBlockStartIndex));
618635
}
619636
return new StringWithEscapedBlocks(resultParts.join(''), escapedBlocks);
620637
}

‎packages/compiler/test/shadow_css_spec.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,28 @@ import {normalizeCSS} from '@angular/platform-browser/testing/src/browser_util';
283283
expect(css).toEqual('@import url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fangular%2Fangular%2Fcommit%2F%26quot%3Ba%26quot%3B); div[contenta] {}');
284284
});
285285

286+
it('should shim rules with quoted content after @import', () => {
287+
const styleStr = '@import url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fangular%2Fangular%2Fcommit%2F%26quot%3Ba%26quot%3B); div {background-image: url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fangular%2Fangular%2Fcommit%2F%26quot%3Ba.jpg%26quot%3B); color: red;}';
288+
const css = s(styleStr, 'contenta');
289+
expect(css).toEqual(
290+
'@import url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fangular%2Fangular%2Fcommit%2F%26quot%3Ba%26quot%3B); div[contenta] {background-image:url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fangular%2Fangular%2Fcommit%2F%26quot%3Ba.jpg%26quot%3B); color:red;}');
291+
});
292+
293+
it('should pass through @import directives whose URL contains colons and semicolons', () => {
294+
const styleStr =
295+
'@import url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fangular%2Fangular%2Fcommit%2F%26quot%3Bhttps%3A%2F%2Ffonts.googleapis.com%2Fcss2%3Ffamily%3DRoboto%3Awght%40400%3B500%26amp%3Bdisplay%3Dswap%26quot%3B);';
296+
const css = s(styleStr, 'contenta');
297+
expect(css).toEqual(styleStr);
298+
});
299+
300+
it('should shim rules after @import with colons and semicolons', () => {
301+
const styleStr =
302+
'@import url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fangular%2Fangular%2Fcommit%2F%26quot%3Bhttps%3A%2F%2Ffonts.googleapis.com%2Fcss2%3Ffamily%3DRoboto%3Awght%40400%3B500%26amp%3Bdisplay%3Dswap%26quot%3B); div {}';
303+
const css = s(styleStr, 'contenta');
304+
expect(css).toEqual(
305+
'@import url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fangular%2Fangular%2Fcommit%2F%26quot%3Bhttps%3A%2F%2Ffonts.googleapis.com%2Fcss2%3Ffamily%3DRoboto%3Awght%40400%3B500%26amp%3Bdisplay%3Dswap%26quot%3B); div[contenta] {}');
306+
});
307+
286308
it('should leave calc() unchanged', () => {
287309
const styleStr = 'div {height:calc(100% - 55px);}';
288310
const css = s(styleStr, 'contenta');
@@ -312,6 +334,24 @@ import {normalizeCSS} from '@angular/platform-browser/testing/src/browser_util';
312334
expect(s('/*# sourceMappingURL=data:x */b {c}/*# sourceURL=xxx */', 'contenta'))
313335
.toEqual('b[contenta] {c}/*# sourceMappingURL=data:x *//*# sourceURL=xxx */');
314336
});
337+
338+
it('should shim rules with quoted content', () => {
339+
const styleStr = 'div {background-image: url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fangular%2Fangular%2Fcommit%2F%26quot%3Ba.jpg%26quot%3B); color: red;}';
340+
const css = s(styleStr, 'contenta');
341+
expect(css).toEqual('div[contenta] {background-image:url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fangular%2Fangular%2Fcommit%2F%26quot%3Ba.jpg%26quot%3B); color:red;}');
342+
});
343+
344+
it('should shim rules with an escaped quote inside quoted content', () => {
345+
const styleStr = 'div::after { content: "\\"" }';
346+
const css = s(styleStr, 'contenta');
347+
expect(css).toEqual('div[contenta]::after { content:"\\""}');
348+
});
349+
350+
it('should shim rules with curly braces inside quoted content', () => {
351+
const styleStr = 'div::after { content: "{}" }';
352+
const css = s(styleStr, 'contenta');
353+
expect(css).toEqual('div[contenta]::after { content:"{}"}');
354+
});
315355
});
316356

317357
describe('processRules', () => {

0 commit comments

Comments
 (0)