diff --git a/.changeset/sharp-doodles-retire.md b/.changeset/sharp-doodles-retire.md new file mode 100644 index 0000000000..2062b7ec34 --- /dev/null +++ b/.changeset/sharp-doodles-retire.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-techdocs-node': patch +--- + +Some security fixes diff --git a/plugins/techdocs-node/src/stages/generate/__fixtures__/mkdocs_with_duplicate_merge_hooks.yml b/plugins/techdocs-node/src/stages/generate/__fixtures__/mkdocs_with_duplicate_merge_hooks.yml new file mode 100644 index 0000000000..145412e730 --- /dev/null +++ b/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 diff --git a/plugins/techdocs-node/src/stages/generate/__fixtures__/mkdocs_with_hooks.yml b/plugins/techdocs-node/src/stages/generate/__fixtures__/mkdocs_with_hooks.yml new file mode 100644 index 0000000000..333ba6db36 --- /dev/null +++ b/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 diff --git a/plugins/techdocs-node/src/stages/generate/__fixtures__/mkdocs_with_merge_key_hooks.yml b/plugins/techdocs-node/src/stages/generate/__fixtures__/mkdocs_with_merge_key_hooks.yml new file mode 100644 index 0000000000..f3b5dbdc93 --- /dev/null +++ b/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 diff --git a/plugins/techdocs-node/src/stages/generate/__fixtures__/mkdocs_with_parser_differential_hooks.yml b/plugins/techdocs-node/src/stages/generate/__fixtures__/mkdocs_with_parser_differential_hooks.yml new file mode 100644 index 0000000000..42f574dcaf --- /dev/null +++ b/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 diff --git a/plugins/techdocs-node/src/stages/generate/helpers.test.ts b/plugins/techdocs-node/src/stages/generate/helpers.test.ts index 8a066e2c42..fd428fd23a 100644 --- a/plugins/techdocs-node/src/stages/generate/helpers.test.ts +++ b/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(); + }); + }); }); diff --git a/plugins/techdocs-node/src/stages/generate/helpers.ts b/plugins/techdocs-node/src/stages/generate/helpers.ts index fa0311b47c..1b0813f18d 100644 --- a/plugins/techdocs-node/src/stages/generate/helpers.ts +++ b/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 = 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 => { + 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 diff --git a/plugins/techdocs-node/src/stages/generate/mkdocsPatchers.ts b/plugins/techdocs-node/src/stages/generate/mkdocsPatchers.ts index 804c06f129..cd506d3b28 100644 --- a/plugins/techdocs-node/src/stages/generate/mkdocsPatchers.ts +++ b/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 = {}; + for (const key of ALLOWED_MKDOCS_KEYS) { + if (key in mkdocsYml) { + sanitized[key] = (mkdocsYml as Record)[key]; + } + } + + // Clear the original object and copy sanitized values back + for (const key of Object.keys(mkdocsYml)) { + delete (mkdocsYml as Record)[key]; + } + Object.assign(mkdocsYml, sanitized); + + // Always rewrite to ensure clean YAML output (resolves merge keys, anchors, etc.) + return true; + }); +}; diff --git a/plugins/techdocs-node/src/stages/generate/techdocs.ts b/plugins/techdocs-node/src/stages/generate/techdocs.ts index 077963b6c7..ed357c5c81 100644 --- a/plugins/techdocs-node/src/stages/generate/techdocs.ts +++ b/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,