Validates CSS path before loading the file (#155)

* Prevent security concern from preload media option * Update yarn.lock * Validates CSS path before loading the file * Modify isSubpath logic
GoogleChromeLabs · Feb 23, 2024 · 2e8cbe8 · 2e8cbe8
1 parent 6a100b7
commit 2e8cbe8
Show file tree

Hide file tree

Showing 5 changed files with 97 additions and 1 deletion.
diff --git a/packages/critters/src/index.js b/packages/critters/src/index.js
@@ -27,7 +27,7 @@ import {
   applyMarkedSelectors,
   validateMediaQuery
 } from './css';
-import { createLogger } from './util';
+import { createLogger, isSubpath } from './util';
 
 /**
  * The mechanism to use for lazy-loading stylesheets.
@@ -253,6 +253,10 @@ export default class Critters {
     }
 
     const filename = path.resolve(outputPath, normalizedPath);
+    // Check if the resolved path is valid
+    if (!isSubpath(outputPath, filename)) {
+      return undefined;
+    }
 
     let sheet;
 

diff --git a/packages/critters/src/util.js b/packages/critters/src/util.js
@@ -1,4 +1,5 @@
 import chalk from 'chalk';
+import path from 'path';
 
 const LOG_LEVELS = ['trace', 'debug', 'info', 'warn', 'error', 'silent'];
 
@@ -38,3 +39,7 @@ export function createLogger(logLevel) {
     return logger;
   }, {});
 }
+
+export function isSubpath(basePath, currentPath) {
+  return !path.relative(basePath, currentPath).startsWith('..');
+}
diff --git a/packages/critters/test/__snapshots__/critters.test.js.snap b/packages/critters/test/__snapshots__/critters.test.js.snap
@@ -64,3 +64,30 @@ exports[`Critters Run on HTML file 1`] = `
 </html>
 "
 `;
+
+exports[`Critters Skip invalid path 1`] = `
+"<html>
+  <link>
+    <title>Testing</title>
+    <style>h1{color:blue}p{color:purple}.contents{padding:50px;text-align:center}.input-field{padding:10px}.custom-element::part(tab){color:#0c0dcc;border-bottom:transparent solid 2px}.custom-element::part(tab):hover:active{background-color:#0c0d33;color:#ffffff}.custom-element::part(tab):focus{box-shadow:0 0 0 1px #0a84ff inset, 0 0 0 1px #0a84ff,
+    0 0 0 4px rgba(10, 132, 255, 0.3)}div:is(:hover, .active){color:#000}div:is(.selected, :hover){color:#fff}</style><link rel=\\"preload\\" href=\\"styles.css\\" as=\\"style\\">
+    <link rel=\\"stylesheet\\" href=\\"../../../../../style3.css\\">
+  
+  <body>
+    <div class=\\"container\\">
+      <header>
+        <div class=\\"banner\\">Lorem ipsum dolor sit amet</div>
+      </header>
+    </div>
+    <div class data-critters-container>
+      <h1>Hello World!</h1>
+      <p>This is a paragraph</p>
+      <input class=\\"input-field\\">
+      <div class=\\"contents\\"></div>
+      <div class=\\"selected active\\"></div>
+    </div>
+    <footer class></footer>
+  <link rel=\\"stylesheet\\" href=\\"styles.css\\"></body>
+</html>
+"
+`;
diff --git a/packages/critters/test/critters.test.js b/packages/critters/test/critters.test.js
@@ -126,4 +126,42 @@ describe('Critters', () => {
     const result = await critters.process(html);
     expect(result).toMatchSnapshot();
   });
+
+  test('Skip invalid path', async () => {
+    const consoleSpy = jest.spyOn(console, 'warn');
+
+    const critters = new Critters({
+      reduceInlineStyles: false,
+      path: path.join(__dirname, 'src')
+    });
+
+    const html = fs.readFileSync(
+      path.join(__dirname, 'src/subpath-validation.html'),
+      'utf8'
+    );
+
+    const result = await critters.process(html);
+    expect(consoleSpy).not.toHaveBeenCalledWith(
+      expect.stringContaining('Unable to locate stylesheet')
+    );
+    expect(result).toMatchSnapshot();
+  });
+
+  it('should not load stylesheets outside of the base path', async () => {
+    const critters = new Critters({ path: '/var/www' });
+    jest.spyOn(critters, 'readFile');
+    await critters.process(`
+        <html>
+            <head>
+                <link rel=stylesheet href=/file.css>
+                <link rel=stylesheet href=/../../../company-secrets/secret.css>
+            </head>
+            <body></body>
+        </html>
+    `);
+    expect(critters.readFile).toHaveBeenCalledWith('/var/www/file.css');
+    expect(critters.readFile).not.toHaveBeenCalledWith(
+      '/company-secrets/secret.css'
+    );
+  });
 });
diff --git a/packages/critters/test/src/subpath-validation.html b/packages/critters/test/src/subpath-validation.html
@@ -0,0 +1,22 @@
+<html>
+  <link>
+    <title>Testing</title>
+    <link rel="stylesheet" href="styles.css"></link>
+    <link rel="stylesheet" href="../../../../../style3.css"></link>
+  </head>
+  <body>
+    <div class="container">
+      <header>
+        <div class="banner">Lorem ipsum dolor sit amet</div>
+      </header>
+    </div>
+    <div class="" data-critters-container>
+      <h1>Hello World!</h1>
+      <p>This is a paragraph</p>
+      <input class="input-field" />
+      <div class="contents"></div>
+      <div class="selected active"></div>
+    </div>
+    <footer class=""></footer>
+  </body>
+</html>