fix(security): gate SVGs in static/img and static/favicons at the site origin

scripts/validate-architecture-assets.mjs

@@ -12,6 +12,26 @@ import { collectError, reportAndExit } from './lib/validate-utils.mjs';
 import { findActiveContent } from './lib/svg-active-content.mjs';
 
 const root = fileURLToPath(new URL('..', import.meta.url));
+
+// Mirrors MIRRORABLE_ASSET_EXTENSIONS in scripts/import-architectures.mjs.
+// static/ is published verbatim at the site origin, so a file the browser
+// executes as markup or script must never be present here.
+const ALLOWED_ASSET_EXTENSIONS = new Set([
+  '.avif',
+  '.gif',
+  '.jpeg',
+  '.jpg',
+  '.png',
+  '.svg',
+  '.webp',
+]);
+
+// static/img additionally serves the legacy favicon.ico. ICO is a raster
+// container that no browser parses as markup or script, so it is safe at the
+// origin. It stays out of ALLOWED_ASSET_EXTENSIONS because that set mirrors
+// what the importer will mirror, and the importer never writes an .ico.
+const SITE_CHROME_EXTENSIONS = new Set([...ALLOWED_ASSET_EXTENSIONS, '.ico']);
+
 // Every directory here is published verbatim at the site origin, so every one
 // gets the security gate (extension allow-list, symlink rejection, SVG active
 // content). The gate is scoped by where the bytes are *served from*, not by
@@ -22,40 +42,60 @@ const root = fileURLToPath(new URL('..', import.meta.url));
 // Diagram-quality checks (viewBox, raster bloat, editor metadata) apply only
 // to architecture diagrams; mirrored cncf/artwork icons and award logos are
 // kept byte-faithful apart from the security gate.
+//
+// static/img and static/favicons hold site chrome - the footer logo, the
+// favicons - rather than imported assets, but they are served from the same
+// origin as everything else, so they carry the same security gate. static/img
+// is walked shallowly because its image subdirectories are listed above, each
+// with its own quality setting.
+//
+// static/fonts and the static/ root (robots.txt, manifest.json, .nojekyll) are
+// deliberately outside the gate: they hold no SVG, and their extensions are
+// legitimately outside the image allow-list.
 const assetDirs = [
   { dir: join(root, 'static/img/architectures'), quality: true },
   { dir: join(root, 'static/img/cncf-projects'), quality: false },
   { dir: join(root, 'static/img/awards'), quality: false },
+  {
+    dir: join(root, 'static/img'),
+    quality: false,
+    recurse: false,
+    extensions: SITE_CHROME_EXTENSIONS,
+  },
+  {
+    dir: join(root, 'static/favicons'),
+    quality: false,
+    extensions: SITE_CHROME_EXTENSIONS,
+  },
 ];
 const shouldFix = process.argv.includes('--fix');
 
-// Mirrors MIRRORABLE_ASSET_EXTENSIONS in scripts/import-architectures.mjs.
-// static/ is published verbatim at the site origin, so a file the browser
-// executes as markup or script must never be present here.
-const ALLOWED_ASSET_EXTENSIONS = new Set([
-  '.avif',
-  '.gif',
-  '.jpeg',
-  '.jpg',
-  '.png',
-  '.svg',
-  '.webp',
-]);
+// Paths that are asset roots in their own right. The shallow static/img walk
+// must not report on them whatever they turn out to be on disk: each one is
+// already handled by its own entry above, which decides for itself whether a
+// symlink is an error and whether a non-directory is skipped. Without this the
+// shallow walk would duplicate the symlink error and would additionally reject
+// a regular file sitting where a root is expected.
+const assetRootPaths = new Set(assetDirs.map(({ dir }) => dir));
 
 const issues = [];
 const fixed = [];
 
-function walk(dir) {
+function walk(dir, recurse = true) {
   return readdirSync(dir, { withFileTypes: true }).flatMap((entry) => {
     const path = join(dir, entry.name);
+    if (assetRootPaths.has(path)) return [];
     // A symlink in published assets can point anywhere in the repository (or
     // outside it) and would be followed by readers and --fix writers, so its
     // presence is itself an error rather than something to validate through.
+    // Checked before the directory branch, so a symlinked directory is still
+    // reported in a shallow walk.
     if (entry.isSymbolicLink()) {
       record(path, 'error', 'is a symbolic link; symlinks are not allowed');
       return [];
     }
-    return entry.isDirectory() ? walk(path) : [path];
+    if (entry.isDirectory()) return recurse ? walk(path) : [];
+    return [path];
   });
 }
 
@@ -161,7 +201,7 @@ function validateSvg(path, quality) {
   }
 }
 
-function validateAsset(path, quality) {
+function validateAsset(path, quality, extensions) {
   const rel = relative(root, path);
   const stats = statSync(path);
   const maxSize = 2 * 1024 * 1024; // 2 MB
@@ -174,7 +214,7 @@ function validateAsset(path, quality) {
   }
 
   const extension = extname(path).toLowerCase();
-  if (!ALLOWED_ASSET_EXTENSIONS.has(extension)) {
+  if (!extensions.has(extension)) {
     record(
       path,
       'error',
@@ -188,23 +228,25 @@ function validateAsset(path, quality) {
   }
 }
 
-const assets = assetDirs.flatMap(({ dir, quality }) => {
-  const kind = assetRootKind(dir);
-  // Fail loudly rather than skipping: a silently unvalidated asset root ships
-  // unchecked SVGs from the site origin.
-  if (kind === 'symlink') {
-    record(
-      dir,
-      'error',
-      'asset directory is a symbolic link; symlinks are not allowed',
-    );
-    return [];
-  }
-  if (kind !== 'directory') return [];
-  return walk(dir).map((path) => ({ path, quality }));
-});
-for (const { path, quality } of assets) {
-  validateAsset(path, quality);
+const assets = assetDirs.flatMap(
+  ({ dir, quality, recurse = true, extensions = ALLOWED_ASSET_EXTENSIONS }) => {
+    const kind = assetRootKind(dir);
+    // Fail loudly rather than skipping: a silently unvalidated asset root ships
+    // unchecked SVGs from the site origin.
+    if (kind === 'symlink') {
+      record(
+        dir,
+        'error',
+        'asset directory is a symbolic link; symlinks are not allowed',
+      );
+      return [];
+    }
+    if (kind !== 'directory') return [];
+    return walk(dir, recurse).map((path) => ({ path, quality, extensions }));
+  },
+);
+for (const { path, quality, extensions } of assets) {
+  validateAsset(path, quality, extensions);
 }
 
 if (fixed.length) {

tests/validate-architecture-assets.test.mjs

@@ -356,3 +356,72 @@ test('warns on an asset larger than 2 MB but still passes', () => {
   assert.match(result.stderr, /large\.svg: asset is \d+\.\d\d MB/);
   assert.match(result.stdout, /Validated 1 architecture asset/);
 });
+
+// static/img and static/favicons hold site chrome — the footer logo, the
+// favicon set — rather than imported assets. They are served from the same
+// origin as the diagrams, so a browser that opens one of their SVGs directly
+// executes any script it carries; they were outside the gate until #690.
+
+test('rejects active content in a site-chrome SVG under static/img', () => {
+  const logo = 'static/img/cncf_logo_white.svg';
+  const svg = VALID_SVG.replace('<rect', '<script>alert(1)</script><rect');
+  const result = runScriptWithFixtures(SCRIPT, { [logo]: svg });
+  assert.equal(result.status, 1);
+  assert.match(result.stderr, /cncf_logo_white\.svg: active content/);
+});
+
+test('rejects active content in a favicon SVG under static/favicons', () => {
+  const icon = 'static/favicons/favicon.svg';
+  const svg = VALID_SVG.replace(
+    '<rect',
+    '<a xlink:href="javascript:alert(1)"/><rect',
+  );
+  const result = runScriptWithFixtures(SCRIPT, { [icon]: svg });
+  assert.equal(result.status, 1);
+  assert.match(result.stderr, /favicon\.svg: active content/);
+});
+
+test('rejects a non-image file in static/img', () => {
+  const result = runScriptWithFixtures(SCRIPT, {
+    'static/img/page.html': '<html>x</html>',
+  });
+  assert.equal(result.status, 1);
+  assert.match(result.stderr, /\.html is not an allowed asset type/);
+});
+
+test('accepts the legacy favicon.ico that static/img serves', () => {
+  // ICO is a raster container no browser parses as markup, so it is safe at
+  // the origin even though the importer never mirrors one.
+  const result = runScriptWithFixtures(SCRIPT, {
+    'static/img/favicon.ico': 'not-really-an-icon',
+  });
+  assert.equal(result.status, 0, result.stderr);
+  assert.match(result.stdout, /Validated 1 architecture asset/);
+});
+
+test('walks static/img shallowly so diagram-quality checks stay scoped', () => {
+  // static/img is walked without recursion because its image subdirectories
+  // are gated as their own roots. A viewBox-less logo sitting directly in
+  // static/img must therefore pass, while the same file under
+  // static/img/architectures fails the diagram-quality gate.
+  const svg = '<svg xmlns="http://www.w3.org/2000/svg"><rect/></svg>';
+  const chrome = runScriptWithFixtures(SCRIPT, { 'static/img/logo.svg': svg });
+  assert.equal(chrome.status, 0, chrome.stderr);
+
+  const diagram = runScriptWithFixtures(SCRIPT, svgFixture(svg));
+  assert.equal(diagram.status, 1);
+  assert.match(diagram.stderr, /missing viewBox/);
+});
+
+test('reports a symlinked directory sitting directly in static/img', () => {
+  // The shallow walk must still reject symlinks: the symlink check runs
+  // before the directory branch, so a link that would otherwise be skipped
+  // for not being recursed into is still a finding.
+  const result = runScriptWithFixtures(
+    SCRIPT,
+    { 'static/img/architectures/example/diagram.svg': VALID_SVG },
+    { symlinks: { 'static/img/elsewhere': 'architectures/example' } },
+  );
+  assert.equal(result.status, 1);
+  assert.match(result.stderr, /elsewhere: is a symbolic link/);
+});