Skip to content

Commit f917b67

Browse files
committed
refactor: remove polynomial regular expression
This change updates the critical css processor to remove the need for a Polynomial regular expression. Addresses: https://github.com/angular/angular-cli/security/code-scanning/51 Closes #25742
1 parent e8240d7 commit f917b67

File tree

5 files changed

+38
-22
lines changed

5 files changed

+38
-22
lines changed

packages/angular/ssr/src/common-engine.ts

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -98,22 +98,16 @@ export class CommonEngine {
9898
...this.providers,
9999
];
100100

101-
let doc = opts.document;
102-
if (!doc && opts.documentFilePath) {
103-
doc = await this.getDocument(opts.documentFilePath);
101+
let document = opts.document;
102+
if (!document && opts.documentFilePath) {
103+
document = await this.getDocument(opts.documentFilePath);
104104
}
105105

106-
if (doc) {
106+
if (document) {
107107
extraProviders.push({
108108
provide: INITIAL_CONFIG,
109109
useValue: {
110-
document: inlineCriticalCss
111-
? // Workaround for https://github.com/GoogleChromeLabs/critters/issues/64
112-
doc.replace(
113-
/ media="print" onload="this\.media='.+?'"(?: ngCspMedia=".+")?><noscript><link .+?><\/noscript>/g,
114-
'>',
115-
)
116-
: doc,
110+
document,
117111
url: opts.url,
118112
},
119113
});

packages/angular/ssr/src/inline-css-processor.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,8 @@ interface PartialHTMLElement {
6262
hasAttribute(name: string): boolean;
6363
removeAttribute(name: string): void;
6464
appendChild(child: PartialHTMLElement): void;
65+
remove(): void;
66+
name: string;
6567
textContent: string;
6668
tagName: string | null;
6769
children: PartialHTMLElement[];
@@ -138,6 +140,17 @@ class CrittersExtended extends Critters {
138140
* that makes it work with Angular's CSP APIs.
139141
*/
140142
private embedLinkedStylesheetOverride: EmbedLinkedStylesheetFn = async (link, document) => {
143+
if (link.getAttribute('media') === 'print' && link.next?.name === 'noscript') {
144+
// Workaround for https://github.com/GoogleChromeLabs/critters/issues/64
145+
// NB: this is only needed for the webpack based builders.
146+
const media = link.getAttribute('onload')?.match(MEDIA_SET_HANDLER_PATTERN);
147+
if (media) {
148+
link.removeAttribute('onload');
149+
link.setAttribute('media', media[1]);
150+
link?.next?.remove();
151+
}
152+
}
153+
141154
const returnValue = await this.initialEmbedLinkedStylesheet(link, document);
142155
const cspNonce = this.findCspNonce(document);
143156

packages/angular_devkit/build_angular/src/builders/prerender/render-worker.ts

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -85,15 +85,7 @@ async function render({
8585
? 'index.original.html'
8686
: indexFile;
8787
const browserIndexInputPath = path.join(outputPath, indexBaseName);
88-
let document = await fs.promises.readFile(browserIndexInputPath, 'utf8');
89-
90-
if (inlineCriticalCss) {
91-
// Workaround for https://github.com/GoogleChromeLabs/critters/issues/64
92-
document = document.replace(
93-
/ media="print" onload="this\.media='.+?'"(?: ngCspMedia=".+")?><noscript><link .+?><\/noscript>/g,
94-
'>',
95-
);
96-
}
88+
const document = await fs.promises.readFile(browserIndexInputPath, 'utf8');
9789

9890
const platformProviders: StaticProvider[] = [
9991
{

packages/angular_devkit/build_angular/src/builders/prerender/works_spec.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,5 +210,9 @@ describe('Prerender Builder', () => {
210210
expect(content).toMatch(
211211
/<style>p{color:#000}<\/style><link rel="stylesheet" href="styles\.\w+\.css" media="print" onload="this\.media='all'">/,
212212
);
213+
214+
// Validate that critters does not process already critical css inlined stylesheets.
215+
expect(content).not.toContain(`onload="this.media='print'">`);
216+
expect(content).not.toContain(`media="print"></noscript>`);
213217
});
214218
});

packages/angular_devkit/build_angular/src/utils/index-file/inline-critical-css.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import * as fs from 'fs';
9+
import { readFile } from 'node:fs/promises';
1010

1111
const Critters: typeof import('critters').default = require('critters');
1212

@@ -58,6 +58,8 @@ interface PartialHTMLElement {
5858
hasAttribute(name: string): boolean;
5959
removeAttribute(name: string): void;
6060
appendChild(child: PartialHTMLElement): void;
61+
remove(): void;
62+
name: string;
6163
textContent: string;
6264
tagName: string | null;
6365
children: PartialHTMLElement[];
@@ -122,14 +124,25 @@ class CrittersExtended extends Critters {
122124
public override readFile(path: string): Promise<string> {
123125
const readAsset = this.optionsExtended.readAsset;
124126

125-
return readAsset ? readAsset(path) : fs.promises.readFile(path, 'utf-8');
127+
return readAsset ? readAsset(path) : readFile(path, 'utf-8');
126128
}
127129

128130
/**
129131
* Override of the Critters `embedLinkedStylesheet` method
130132
* that makes it work with Angular's CSP APIs.
131133
*/
132134
private embedLinkedStylesheetOverride: EmbedLinkedStylesheetFn = async (link, document) => {
135+
if (link.getAttribute('media') === 'print' && link.next?.name === 'noscript') {
136+
// Workaround for https://github.com/GoogleChromeLabs/critters/issues/64
137+
// NB: this is only needed for the webpack based builders.
138+
const media = link.getAttribute('onload')?.match(MEDIA_SET_HANDLER_PATTERN);
139+
if (media) {
140+
link.removeAttribute('onload');
141+
link.setAttribute('media', media[1]);
142+
link?.next?.remove();
143+
}
144+
}
145+
133146
const returnValue = await this.initialEmbedLinkedStylesheet(link, document);
134147
const cspNonce = this.findCspNonce(document);
135148

0 commit comments

Comments
 (0)