chore: small tweaks

Signed-off-by: benjdlambert <ben@blam.sh>

.changeset/sharp-doodles-retire.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-techdocs-node': patch
+---
+
+Some security fixes

plugins/techdocs-node/src/stages/generate/__fixtures__/mkdocs_with_duplicate_merge_hooks.yml

@@ -0,0 +1,12 @@
+# This file uses duplicate merge keys (<<) which causes parser differential:
+# - js-yaml: hooks: null (first merge key wins)
+# - PyYAML: hooks: [hello.py] (last merge key wins)
+site_name: Test
+plugins:
+  - techdocs-core
+array_base: &array_base
+  hooks: [hello.py]
+null_base: &null_base
+  hooks: null
+<<: *null_base
+<<: *array_base

plugins/techdocs-node/src/stages/generate/__fixtures__/mkdocs_with_hooks.yml

@@ -0,0 +1,5 @@
+site_name: Test site name
+site_description: Test site description
+hooks:
+  - hooks/my_hook.py
+  - hooks/another_hook.py

plugins/techdocs-node/src/stages/generate/__fixtures__/mkdocs_with_merge_key_hooks.yml

@@ -0,0 +1,8 @@
+site_name: Test
+plugins:
+  - techdocs-core
+extra:
+  array_hooks: &array_hooks
+    hooks:
+      - custom_hook.py
+<<: *array_hooks

plugins/techdocs-node/src/stages/generate/__fixtures__/mkdocs_with_parser_differential_hooks.yml

@@ -0,0 +1,16 @@
+# This file intentionally uses duplicate merge keys (<<) which is technically
+# invalid YAML, but is accepted by most parsers with different behaviors:
+# - js-yaml: uses the first merge key value (hooks: null)
+# - PyYAML: uses the last merge key value (hooks: [custom_hook.py])
+# This tests that sanitization catches hooks regardless of parser interpretation.
+site_name: Test
+plugins:
+  - techdocs-core
+extra:
+  null_hooks: &null_hooks
+    hooks: null
+  array_hooks: &array_hooks
+    hooks:
+      - custom_hook.py
+<<: *null_hooks
+<<: *array_hooks

plugins/techdocs-node/src/stages/generate/helpers.test.ts

@@ -30,11 +30,13 @@ import {
   getRepoUrlFromLocationAnnotation,
   patchIndexPreBuild,
   storeEtagMetadata,
+  validateDocsDirectory,
   validateMkdocsYaml,
 } from './helpers';
 import {
   patchMkdocsYmlPreBuild,
   patchMkdocsYmlWithPlugins,
+  sanitizeMkdocsYml,
 } from './mkdocsPatchers';
 import yaml from 'js-yaml';
 
@@ -92,6 +94,24 @@ const mkdocsYmlWithAdditionalPluginsWithConfig = fs.readFileSync(
 const mkdocsYmlWithEnvTag = fs.readFileSync(
   resolvePath(__filename, '../__fixtures__/mkdocs_with_env_tag.yml'),
 );
+const mkdocsYmlWithHooks = fs.readFileSync(
+  resolvePath(__filename, '../__fixtures__/mkdocs_with_hooks.yml'),
+);
+const mkdocsYmlWithMergeKeyHooks = fs.readFileSync(
+  resolvePath(__filename, '../__fixtures__/mkdocs_with_merge_key_hooks.yml'),
+);
+const mkdocsYmlWithParserDifferentialHooks = fs.readFileSync(
+  resolvePath(
+    __filename,
+    '../__fixtures__/mkdocs_with_parser_differential_hooks.yml',
+  ),
+);
+const mkdocsYmlWithDuplicateMergeHooks = fs.readFileSync(
+  resolvePath(
+    __filename,
+    '../__fixtures__/mkdocs_with_duplicate_merge_hooks.yml',
+  ),
+);
 const mockLogger = mockServices.logger.mock();
 const warn = jest.spyOn(mockLogger, 'warn');
 
@@ -441,8 +461,6 @@ describe('helpers', () => {
       };
       expect(parsedYml.plugins).toHaveLength(4);
       expect(parsedYml.plugins).toContain('techdocs-core');
-      // we want our original object with its properties to be preserved, and for the basic string form of the plugin
-      // to NOT be added as well.
       expect(parsedYml.plugins).not.toContain('custom-plugin');
       expect(parsedYml.plugins).toContainEqual({
         'custom-plugin': { with: { configuration: 1 } },
@@ -727,4 +745,310 @@ describe('helpers', () => {
       ).resolves.toBeUndefined();
     });
   });
+
+  describe('sanitizeMkdocsYml', () => {
+    beforeEach(() => {
+      warn.mockClear();
+      mockDir.setContent({
+        'mkdocs_with_hooks.yml': mkdocsYmlWithHooks,
+        'mkdocs.yml': mkdocsYml,
+      });
+    });
+
+    it('should remove disallowed keys from mkdocs.yml and log a warning', async () => {
+      await sanitizeMkdocsYml(
+        mockDir.resolve('mkdocs_with_hooks.yml'),
+        mockLogger,
+      );
+
+      const updatedMkdocsYml = await fs.readFile(
+        mockDir.resolve('mkdocs_with_hooks.yml'),
+      );
+      const parsedYml = yaml.load(updatedMkdocsYml.toString()) as {
+        hooks?: string[];
+        site_name: string;
+      };
+      expect(parsedYml.hooks).toBeUndefined();
+      expect(parsedYml.site_name).toBe('Test site name');
+      expect(warn).toHaveBeenCalledWith(
+        expect.stringContaining(
+          'Removed the following unsupported configuration keys from mkdocs.yml: hooks',
+        ),
+      );
+    });
+
+    it('should not modify mkdocs.yml when no disallowed keys are present', async () => {
+      await sanitizeMkdocsYml(mockDir.resolve('mkdocs.yml'), mockLogger);
+
+      const updatedMkdocsYml = await fs.readFile(mockDir.resolve('mkdocs.yml'));
+      const parsedYml = yaml.load(updatedMkdocsYml.toString()) as {
+        hooks?: string[];
+        site_name: string;
+      };
+      expect(parsedYml.hooks).toBeUndefined();
+      expect(parsedYml.site_name).toBe('Test site name');
+      expect(warn).not.toHaveBeenCalled();
+    });
+
+    it('should remove multiple disallowed keys and list them all in the warning', async () => {
+      const mkdocsWithMultipleDisallowed = `site_name: Test
+hooks:
+  - hook.py
+some_unknown_key: value
+another_unknown: true
+`;
+      mockDir.setContent({
+        'mkdocs_multiple.yml': mkdocsWithMultipleDisallowed,
+      });
+
+      await sanitizeMkdocsYml(
+        mockDir.resolve('mkdocs_multiple.yml'),
+        mockLogger,
+      );
+
+      const updatedMkdocsYml = await fs.readFile(
+        mockDir.resolve('mkdocs_multiple.yml'),
+      );
+      const parsedYml = yaml.load(updatedMkdocsYml.toString()) as Record<
+        string,
+        unknown
+      >;
+      expect(parsedYml.hooks).toBeUndefined();
+      expect(parsedYml.some_unknown_key).toBeUndefined();
+      expect(parsedYml.another_unknown).toBeUndefined();
+      expect(parsedYml.site_name).toBe('Test');
+      expect(warn).toHaveBeenCalledWith(
+        expect.stringMatching(
+          /Removed the following unsupported configuration keys.*hooks.*some_unknown_key.*another_unknown|Removed the following unsupported configuration keys.*hooks.*another_unknown.*some_unknown_key/,
+        ),
+      );
+    });
+
+    it('should remove hooks introduced via YAML merge keys', async () => {
+      mockDir.setContent({
+        'mkdocs_merge_keys.yml': mkdocsYmlWithMergeKeyHooks,
+      });
+
+      await sanitizeMkdocsYml(
+        mockDir.resolve('mkdocs_merge_keys.yml'),
+        mockLogger,
+      );
+
+      const updatedMkdocsYml = await fs.readFile(
+        mockDir.resolve('mkdocs_merge_keys.yml'),
+      );
+      const parsedYml = yaml.load(updatedMkdocsYml.toString()) as Record<
+        string,
+        unknown
+      >;
+
+      expect(parsedYml.hooks).toBeUndefined();
+      expect(parsedYml.site_name).toBe('Test');
+      expect(parsedYml.plugins).toEqual(['techdocs-core']);
+      expect(updatedMkdocsYml.toString()).not.toContain('<<:');
+
+      expect(warn).toHaveBeenCalledWith(
+        expect.stringContaining(
+          'Removed the following unsupported configuration keys from mkdocs.yml: hooks',
+        ),
+      );
+    });
+
+    it('should remove hooks when parsers interpret duplicate merge keys differently', async () => {
+      mockDir.setContent({
+        'mkdocs_parser_diff.yml': mkdocsYmlWithParserDifferentialHooks,
+      });
+
+      await sanitizeMkdocsYml(
+        mockDir.resolve('mkdocs_parser_diff.yml'),
+        mockLogger,
+      );
+
+      const updatedMkdocsYml = await fs.readFile(
+        mockDir.resolve('mkdocs_parser_diff.yml'),
+      );
+      const parsedYml = yaml.load(updatedMkdocsYml.toString()) as Record<
+        string,
+        unknown
+      >;
+
+      expect(parsedYml.hooks).toBeUndefined();
+      expect(parsedYml.site_name).toBe('Test');
+      expect(parsedYml.plugins).toEqual(['techdocs-core']);
+      expect(parsedYml.extra).toBeDefined();
+      expect(updatedMkdocsYml.toString()).not.toContain('<<:');
+
+      expect(warn).toHaveBeenCalledWith(
+        expect.stringContaining(
+          'Removed the following unsupported configuration keys from mkdocs.yml: hooks',
+        ),
+      );
+    });
+
+    it('should remove hooks with duplicate merge keys and top-level anchors', async () => {
+      mockDir.setContent({
+        'mkdocs_duplicate_merge.yml': mkdocsYmlWithDuplicateMergeHooks,
+      });
+
+      await sanitizeMkdocsYml(
+        mockDir.resolve('mkdocs_duplicate_merge.yml'),
+        mockLogger,
+      );
+
+      const updatedMkdocsYml = await fs.readFile(
+        mockDir.resolve('mkdocs_duplicate_merge.yml'),
+      );
+      const parsedYml = yaml.load(updatedMkdocsYml.toString()) as Record<
+        string,
+        unknown
+      >;
+
+      expect(parsedYml.hooks).toBeUndefined();
+      expect(parsedYml.site_name).toBe('Test');
+      expect(parsedYml.plugins).toEqual(['techdocs-core']);
+      expect(updatedMkdocsYml.toString()).not.toContain('<<:');
+
+      expect(warn).toHaveBeenCalledWith(expect.stringContaining('hooks'));
+    });
+  });
+
+  describe('validateDocsDirectory', () => {
+    it('should pass for a valid docs directory with no symlinks', async () => {
+      mockDir.setContent({
+        docs: {
+          'index.md': 'Hello',
+          'guide.md': 'Guide content',
+        },
+      });
+
+      await expect(
+        validateDocsDirectory(mockDir.resolve('docs'), mockDir.path),
+      ).resolves.toBeUndefined();
+    });
+
+    it('should pass for symlinks pointing within the input directory', async () => {
+      mockDir.setContent({
+        docs: {
+          'index.md': 'Hello',
+        },
+        'other.md': 'Other content',
+      });
+
+      // Create a symlink within the input directory
+      await fs.symlink(
+        mockDir.resolve('other.md'),
+        mockDir.resolve('docs/link.md'),
+      );
+
+      await expect(
+        validateDocsDirectory(mockDir.resolve('docs'), mockDir.path),
+      ).resolves.toBeUndefined();
+    });
+
+    it('should reject symlinks pointing outside the input directory', async () => {
+      const anotherMockDir = createMockDirectory();
+
+      mockDir.setContent({
+        docs: {
+          'index.md': 'Hello',
+        },
+      });
+
+      anotherMockDir.setContent({
+        tmp: {
+          secret: 'password',
+        },
+      });
+
+      // Create a symlink pointing outside the input directory
+      await fs.symlink(
+        anotherMockDir.resolve('tmp/secret'),
+        mockDir.resolve('docs/escape.md'),
+      );
+
+      await expect(
+        validateDocsDirectory(mockDir.resolve('docs'), mockDir.path),
+      ).rejects.toThrow(/not allowed to refer to a location outside/i);
+    });
+
+    it('should reject symlinks to sensitive files like /etc/passwd', async () => {
+      mockDir.setContent({
+        docs: {
+          'index.md': 'Hello',
+        },
+      });
+
+      // Create a symlink to /etc/passwd
+      await fs.symlink('/etc/passwd', mockDir.resolve('docs/passwd.md'));
+
+      await expect(
+        validateDocsDirectory(mockDir.resolve('docs'), mockDir.path),
+      ).rejects.toThrow(/not allowed to refer to a location outside/i);
+    });
+
+    it('should reject symlinks in nested directories', async () => {
+      mockDir.setContent({
+        docs: {
+          'index.md': 'Hello',
+          nested: {
+            'page.md': 'Nested page',
+          },
+        },
+      });
+
+      // Create a symlink in a nested directory pointing outside
+      await fs.symlink('/etc/passwd', mockDir.resolve('docs/nested/escape.md'));
+
+      await expect(
+        validateDocsDirectory(mockDir.resolve('docs'), mockDir.path),
+      ).rejects.toThrow(/not allowed to refer to a location outside/i);
+    });
+
+    it('should reject directory symlinks pointing outside', async () => {
+      const anotherMockDir = createMockDirectory();
+
+      mockDir.setContent({
+        docs: {
+          'index.md': 'Hello',
+        },
+      });
+
+      anotherMockDir.setContent({
+        tmp: {
+          secret: 'password',
+        },
+      });
+
+      // Create a directory symlink pointing outside
+      await fs.symlink(
+        anotherMockDir.path,
+        mockDir.resolve('docs/external-dir'),
+      );
+
+      await expect(
+        validateDocsDirectory(mockDir.resolve('docs'), mockDir.path),
+      ).rejects.toThrow(/not allowed to refer to a location outside/i);
+    });
+
+    it('should pass for directory symlinks within input directory', async () => {
+      mockDir.setContent({
+        docs: {
+          'index.md': 'Hello',
+        },
+        assets: {
+          'image.png': 'binary content',
+        },
+      });
+
+      // Create a directory symlink within input directory
+      await fs.symlink(
+        mockDir.resolve('assets'),
+        mockDir.resolve('docs/assets'),
+      );
+
+      await expect(
+        validateDocsDirectory(mockDir.resolve('docs'), mockDir.path),
+      ).resolves.toBeUndefined();
+    });
+  });
 });

plugins/techdocs-node/src/stages/generate/helpers.ts

@@ -15,6 +15,7 @@
  */
 
 import { isChildPath, LoggerService } from '@backstage/backend-plugin-api';
+import { NotAllowedError } from '@backstage/errors';
 import { Entity } from '@backstage/catalog-model';
 import { assertError, ForwardedError } from '@backstage/errors';
 import { ScmIntegrationRegistry } from '@backstage/integration';
@@ -266,6 +267,51 @@ export const getMkdocsYml = async (
   };
 };
 
+/**
+ * Allowlist of MkDocs configuration keys supported by TechDocs.
+ *
+ * @see https://www.mkdocs.org/user-guide/configuration/
+ */
+export const ALLOWED_MKDOCS_KEYS = new Set([
+  // Site information
+  'site_name',
+  'site_url',
+  'site_description',
+  'site_author',
+  // Repository
+  'repo_url',
+  'repo_name',
+  'edit_uri',
+  'edit_uri_template',
+  // Build directories
+  'docs_dir',
+  'site_dir',
+  // Documentation layout
+  'nav',
+  'exclude_docs',
+  'not_in_nav',
+  // Build settings
+  'theme',
+  'plugins',
+  'markdown_extensions',
+  'extra',
+  'extra_css',
+  'extra_templates',
+  // Preview controls
+  'use_directory_urls',
+  'strict',
+  'dev_addr',
+  'watch',
+  // Metadata
+  'copyright',
+  'remote_branch',
+  'remote_name',
+  'validation',
+  // Deprecated
+  'google_analytics',
+  'INHERIT',
+]);
+
 /**
  * Validating mkdocs config file for incorrect/insecure values
  * Throws on invalid configs
@@ -288,6 +334,7 @@ export const validateMkdocsYaml = async (
   }
 
   const parsedMkdocsYml: Record<string, any> = mkdocsYml;
+
   if (
     parsedMkdocsYml.docs_dir &&
     !isChildPath(inputDir, resolvePath(inputDir, parsedMkdocsYml.docs_dir))
@@ -300,6 +347,29 @@ export const validateMkdocsYaml = async (
   return parsedMkdocsYml.docs_dir;
 };
 
+/**
+ * Validates that the docs directory doesn't contain symlinks pointing outside
+ * the input directory. This prevents path traversal attacks where malicious
+ * symlinks could be used to read arbitrary files from the host filesystem.
+ *
+ * @param docsDir - The docs directory to validate (absolute path)
+ * @param inputDir - The root input directory that symlinks must stay within
+ */
+export const validateDocsDirectory = async (
+  docsDir: string,
+  inputDir: string,
+): Promise<void> => {
+  const files = await getFileTreeRecursively(docsDir);
+
+  for (const file of files) {
+    if (!isChildPath(inputDir, file)) {
+      throw new NotAllowedError(
+        `Path ${file} is not allowed to refer to a location outside ${inputDir}`,
+      );
+    }
+  }
+};
+
 /**
  * Update docs/index.md file before TechDocs generator uses it to generate docs site,
  * falling back to docs/README.md or README.md in case a default docs/index.md

plugins/techdocs-node/src/stages/generate/mkdocsPatchers.ts

@@ -16,7 +16,11 @@
 import fs from 'fs-extra';
 import yaml from 'js-yaml';
 import { ParsedLocationAnnotation } from '../../helpers';
-import { getRepoUrlFromLocationAnnotation, MKDOCS_SCHEMA } from './helpers';
+import {
+  ALLOWED_MKDOCS_KEYS,
+  getRepoUrlFromLocationAnnotation,
+  MKDOCS_SCHEMA,
+} from './helpers';
 import { assertError } from '@backstage/errors';
 import { ScmIntegrationRegistry } from '@backstage/integration';
 import { LoggerService } from '@backstage/backend-plugin-api';
@@ -180,3 +184,55 @@ export const patchMkdocsYmlWithPlugins = async (
     return changesMade;
   });
 };
+
+/**
+ * Sanitize mkdocs.yml by keeping only allowed configuration keys.
+ *
+ * TechDocs only supports a subset of MkDocs configuration options.
+ * This function reconstructs the config with only allowed keys,
+ * discarding everything else. This approach ensures that any unknown
+ * or potentially dangerous configuration options are not passed to MkDocs.
+ *
+ * The file is always rewritten to ensure YAML features like merge keys
+ * and anchors are resolved into plain configuration.
+ *
+ * @param mkdocsYmlPath - Absolute path to mkdocs.yml or equivalent of a docs site
+ * @param logger - A logger instance
+ */
+export const sanitizeMkdocsYml = async (
+  mkdocsYmlPath: string,
+  logger: LoggerService,
+) => {
+  await patchMkdocsFile(mkdocsYmlPath, logger, mkdocsYml => {
+    // Identify keys that will be removed for logging
+    const removedKeys = Object.keys(mkdocsYml).filter(
+      key => !ALLOWED_MKDOCS_KEYS.has(key),
+    );
+
+    if (removedKeys.length > 0) {
+      logger.warn(
+        `Removed the following unsupported configuration keys from mkdocs.yml: ${removedKeys.join(
+          ', ',
+        )}. ` +
+          `TechDocs only supports a subset of MkDocs configuration options.`,
+      );
+    }
+
+    // Build a new object with only allowed keys
+    const sanitized: Record<string, unknown> = {};
+    for (const key of ALLOWED_MKDOCS_KEYS) {
+      if (key in mkdocsYml) {
+        sanitized[key] = (mkdocsYml as Record<string, unknown>)[key];
+      }
+    }
+
+    // Clear the original object and copy sanitized values back
+    for (const key of Object.keys(mkdocsYml)) {
+      delete (mkdocsYml as Record<string, unknown>)[key];
+    }
+    Object.assign(mkdocsYml, sanitized);
+
+    // Always rewrite to ensure clean YAML output (resolves merge keys, anchors, etc.)
+    return true;
+  });
+};

plugins/techdocs-node/src/stages/generate/techdocs.ts

@@ -26,12 +26,14 @@ import {
   patchIndexPreBuild,
   runCommand,
   storeEtagMetadata,
+  validateDocsDirectory,
   validateMkdocsYaml,
 } from './helpers';
 
 import {
   patchMkdocsYmlPreBuild,
   patchMkdocsYmlWithPlugins,
+  sanitizeMkdocsYml,
 } from './mkdocsPatchers';
 import {
   GeneratorBase,
@@ -112,6 +114,14 @@ export class TechdocsGenerator implements GeneratorBase {
     // validate the docs_dir first
     const docsDir = await validateMkdocsYaml(inputDir, content);
 
+    // Remove unsupported configuration keys
+    await sanitizeMkdocsYml(mkdocsYmlPath, childLogger);
+
+    // Validate that no symlinks in the docs directory point outside the input directory
+    // This prevents path traversal attacks where malicious symlinks could leak host files
+    const resolvedDocsDir = path.join(inputDir, docsDir ?? 'docs');
+    await validateDocsDirectory(resolvedDocsDir, inputDir);
+
     if (parsedLocationAnnotation) {
       await patchMkdocsYmlPreBuild(
         mkdocsYmlPath,