paulononato/backstage
1
0
Fork 0
Code Issues Pull Requests Actions 47 Packages Projects Releases Wiki Activity

Revert "Merge branch 'master' into bui-links"

Browse Source 
Signed-off-by: Charles de Dreuille <charles.dedreuille@gmail.com>
This commit is contained in:
Charles de Dreuille
2026-01-30 20:57:13 +00:00
parent 76581a5915
commit 4b373cf252
10 changed files with 20 additions and 504 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.changeset/sharp-doodles-retire.md
-5
View File
@@ -1,5 +0,0 @@
---
'@backstage/plugin-techdocs-node': patch
---
Some security fixes
plugins/techdocs-node/src/stages/generate/__fixtures__/mkdocs_with_duplicate_merge_hooks.yml
-12
View File
@@ -1,12 +0,0 @@
# 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
-5
View File
@@ -1,5 +0,0 @@
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
-8
View File
@@ -1,8 +0,0 @@
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
-16
View File
@@ -1,16 +0,0 @@
# 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
+2 -304
View File
@@ -30,13 +30,11 @@ import {
getRepoUrlFromLocationAnnotation,
patchIndexPreBuild,
storeEtagMetadata,
validateDocsDirectory,
validateMkdocsYaml,
} from './helpers';
import {
patchMkdocsYmlPreBuild,
patchMkdocsYmlWithPlugins,
sanitizeMkdocsYml,
} from './mkdocsPatchers';
import yaml from 'js-yaml';
@@ -94,24 +92,6 @@ 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');
@@ -461,6 +441,8 @@ 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 } },
@@ -745,288 +727,4 @@ 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 () => {
mockDir.setContent({
docs: {
'index.md': 'Hello',
},
});
// Create a symlink pointing outside the input directory
await fs.symlink('/tmp', 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 () => {
mockDir.setContent({
docs: {
'index.md': 'Hello',
},
});
// Create a directory symlink pointing outside
await fs.symlink('/tmp', 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
-70
View File
@@ -15,7 +15,6 @@
*/
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';
@@ -267,51 +266,6 @@ 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
@@ -334,7 +288,6 @@ export const validateMkdocsYaml = async (
}
const parsedMkdocsYml: Record<string, any> = mkdocsYml;
if (
parsedMkdocsYml.docs_dir &&
!isChildPath(inputDir, resolvePath(inputDir, parsedMkdocsYml.docs_dir))
@@ -347,29 +300,6 @@ 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
+1 -57
View File
@@ -16,11 +16,7 @@
import fs from 'fs-extra';
import yaml from 'js-yaml';
import { ParsedLocationAnnotation } from '../../helpers';
import {
ALLOWED_MKDOCS_KEYS,
getRepoUrlFromLocationAnnotation,
MKDOCS_SCHEMA,
} from './helpers';
import { getRepoUrlFromLocationAnnotation, MKDOCS_SCHEMA } from './helpers';
import { assertError } from '@backstage/errors';
import { ScmIntegrationRegistry } from '@backstage/integration';
import { LoggerService } from '@backstage/backend-plugin-api';
@@ -184,55 +180,3 @@ 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
-10
View File
@@ -26,14 +26,12 @@ import {
patchIndexPreBuild,
runCommand,
storeEtagMetadata,
validateDocsDirectory,
validateMkdocsYaml,
} from './helpers';
import {
patchMkdocsYmlPreBuild,
patchMkdocsYmlWithPlugins,
sanitizeMkdocsYml,
} from './mkdocsPatchers';
import {
GeneratorBase,
@@ -114,14 +112,6 @@ 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,
yarn.lock
+17 -17
View File
@@ -27589,6 +27589,13 @@ __metadata:
languageName: node
linkType: hard
"cookie-signature@npm:1.0.7, cookie-signature@npm:~1.0.6":
version: 1.0.7
resolution: "cookie-signature@npm:1.0.7"
checksum: 10/1a62808cd30d15fb43b70e19829b64d04b0802d8ef00275b57d152de4ae6a3208ca05c197b6668d104c4d9de389e53ccc2d3bc6bcaaffd9602461417d8c40710
languageName: node
linkType: hard
"cookie-signature@npm:^1.2.1, cookie-signature@npm:^1.2.2":
version: 1.2.2
resolution: "cookie-signature@npm:1.2.2"
@@ -27596,14 +27603,7 @@ __metadata:
languageName: node
linkType: hard
"cookie-signature@npm:~1.0.6, cookie-signature@npm:~1.0.7":
version: 1.0.7
resolution: "cookie-signature@npm:1.0.7"
checksum: 10/1a62808cd30d15fb43b70e19829b64d04b0802d8ef00275b57d152de4ae6a3208ca05c197b6668d104c4d9de389e53ccc2d3bc6bcaaffd9602461417d8c40710
languageName: node
linkType: hard
"cookie@npm:0.7.2, cookie@npm:^0.7.0, cookie@npm:^0.7.1, cookie@npm:^0.7.2, cookie@npm:~0.7.1, cookie@npm:~0.7.2":
"cookie@npm:0.7.2, cookie@npm:^0.7.0, cookie@npm:^0.7.1, cookie@npm:^0.7.2, cookie@npm:~0.7.1":
version: 0.7.2
resolution: "cookie@npm:0.7.2"
checksum: 10/24b286c556420d4ba4e9bc09120c9d3db7d28ace2bd0f8ccee82422ce42322f73c8312441271e5eefafbead725980e5996cc02766dbb89a90ac7f5636ede608f
@@ -28487,7 +28487,7 @@ __metadata:
languageName: node
linkType: hard
"debug@npm:2.6.9, debug@npm:^2.6.0, debug@npm:~2.6.9":
"debug@npm:2.6.9, debug@npm:^2.6.0":
version: 2.6.9
resolution: "debug@npm:2.6.9"
dependencies:
@@ -31059,18 +31059,18 @@ __metadata:
linkType: hard
"express-session@npm:^1.17.1, express-session@npm:^1.17.3":
version: 1.19.0
resolution: "express-session@npm:1.19.0"
version: 1.18.2
resolution: "express-session@npm:1.18.2"
dependencies:
cookie: "npm:~0.7.2"
cookie-signature: "npm:~1.0.7"
debug: "npm:~2.6.9"
cookie: "npm:0.7.2"
cookie-signature: "npm:1.0.7"
debug: "npm:2.6.9"
depd: "npm:~2.0.0"
on-headers: "npm:~1.1.0"
parseurl: "npm:~1.3.3"
safe-buffer: "npm:~5.2.1"
safe-buffer: "npm:5.2.1"
uid-safe: "npm:~2.1.5"
checksum: 10/5f36b51d344ec40adb5b9cd8a915fd694cdaf64832632411d4f6e6de8e9a83d873a20389e9c3cce966fece76a18c0384c5f2763b3ac089f801e8d6e92cb543e6
checksum: 10/1a89a4d3e579e1d2a729dc604c254c3e942b24a279681371c2d8042b1080f9e6de4ca787a091ec1a9178c9f2cc5c19bcb9f3929632d44db8972263e26c2cc4b8
languageName: node
linkType: hard
@@ -45678,7 +45678,7 @@ __metadata:
languageName: node
linkType: hard
"safe-buffer@npm:5.2.1, safe-buffer@npm:>=5.1.0, safe-buffer@npm:^5.0.1, safe-buffer@npm:^5.1.0, safe-buffer@npm:^5.1.1, safe-buffer@npm:^5.1.2, safe-buffer@npm:^5.2.1, safe-buffer@npm:~5.2.0, safe-buffer@npm:~5.2.1":
"safe-buffer@npm:5.2.1, safe-buffer@npm:>=5.1.0, safe-buffer@npm:^5.0.1, safe-buffer@npm:^5.1.0, safe-buffer@npm:^5.1.1, safe-buffer@npm:^5.1.2, safe-buffer@npm:^5.2.1, safe-buffer@npm:~5.2.0":
version: 5.2.1
resolution: "safe-buffer@npm:5.2.1"
checksum: 10/32872cd0ff68a3ddade7a7617b8f4c2ae8764d8b7d884c651b74457967a9e0e886267d3ecc781220629c44a865167b61c375d2da6c720c840ecd73f45d5d9451