From f9352ab606367cd9efc6ff048915c70ed3013b7f Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 24 Nov 2021 19:56:44 +0100 Subject: [PATCH] scaffolder-backend: removed all usaged and prevent new usage of path.resolve Signed-off-by: Patrik Oldsberg --- .changeset/serious-pens-tease.md | 5 +++ plugins/scaffolder-backend/.eslintrc.js | 33 +++++++++++++++++++ .../actions/builtin/catalog/write.ts | 4 +-- .../scaffolder/actions/builtin/debug/log.ts | 4 +-- .../actions/builtin/fetch/helpers.ts | 2 +- .../actions/builtin/fetch/template.ts | 8 ++--- .../builtin/publish/githubPullRequest.ts | 3 +- 7 files changed, 48 insertions(+), 11 deletions(-) create mode 100644 .changeset/serious-pens-tease.md diff --git a/.changeset/serious-pens-tease.md b/.changeset/serious-pens-tease.md new file mode 100644 index 0000000000..7759ba3bc0 --- /dev/null +++ b/.changeset/serious-pens-tease.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend': patch +--- + +Removed all usages of `path.resolve` in order to ensure that template paths are resolved in a safe way. diff --git a/plugins/scaffolder-backend/.eslintrc.js b/plugins/scaffolder-backend/.eslintrc.js index 19c9ad7395..2632b5e089 100644 --- a/plugins/scaffolder-backend/.eslintrc.js +++ b/plugins/scaffolder-backend/.eslintrc.js @@ -1,8 +1,41 @@ +const parent = require('@backstage/cli/config/eslint.backend'); + module.exports = { extends: [require.resolve('@backstage/cli/config/eslint.backend')], ignorePatterns: ['sample-templates/'], rules: { 'no-console': 0, // Permitted in console programs 'new-cap': ['error', { capIsNew: false }], // Because Express constructs things e.g. like 'const r = express.Router()' + // Usage of path.resolve is extra sensitive in the scaffolder, so forbid it in non-test code + 'no-restricted-imports': [ + 'error', + { + ...parent.rules['no-restricted-imports'][1], + paths: [ + { + name: 'path', + importNames: ['resolve'], + message: + 'Do not use path.resolve, use `resolveSafeChildPath` from `@backstage/backend-common` instead as it prevents security issues', + }, + ], + }, + ], + 'no-restricted-syntax': parent.rules['no-restricted-syntax'].concat([ + { + message: + 'Do not use path.resolve, use `resolveSafeChildPath` from `@backstage/backend-common` instead as it prevents security issues', + selector: + 'MemberExpression[object.name="path"][property.name="resolve"]', + }, + ]), }, + overrides: [ + { + files: ['*.test.*', 'src/setupTests.*', 'dev/**'], + rules: { + 'no-restricted-imports': parent.rules['no-restricted-imports'], + }, + }, + ], }; diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/catalog/write.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/catalog/write.ts index 45481b47e6..b813244020 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/catalog/write.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/catalog/write.ts @@ -15,10 +15,10 @@ */ import fs from 'fs-extra'; -import { resolve as resolvePath } from 'path'; import { createTemplateAction } from '../../createTemplateAction'; import * as yaml from 'yaml'; import { Entity } from '@backstage/catalog-model'; +import { resolveSafeChildPath } from '@backstage/backend-common'; export function createCatalogWriteAction() { return createTemplateAction<{ name?: string; entity: Entity }>({ @@ -42,7 +42,7 @@ export function createCatalogWriteAction() { const { entity } = ctx.input; await fs.writeFile( - resolvePath(ctx.workspacePath, 'catalog-info.yaml'), + resolveSafeChildPath(ctx.workspacePath, 'catalog-info.yaml'), yaml.stringify(entity), ); }, diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.ts index 9bf9b7fb90..36ea842a66 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.ts @@ -15,7 +15,7 @@ */ import { readdir, stat } from 'fs-extra'; -import { relative, resolve } from 'path'; +import { relative, join } from 'path'; import { createTemplateAction } from '../../createTemplateAction'; /** @@ -68,7 +68,7 @@ export async function recursiveReadDir(dir: string): Promise { const subdirs = await readdir(dir); const files = await Promise.all( subdirs.map(async subdir => { - const res = resolve(dir, subdir); + const res = join(dir, subdir); return (await stat(res)).isDirectory() ? recursiveReadDir(res) : [res]; }), ); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/helpers.ts index 3946110d42..b56bab0d53 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/helpers.ts @@ -19,7 +19,7 @@ import { JsonValue } from '@backstage/types'; import { InputError } from '@backstage/errors'; import { ScmIntegrations } from '@backstage/integration'; import fs from 'fs-extra'; -import * as path from 'path'; +import path from 'path'; export async function fetchContents({ reader, diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts index 47bfcccfdf..54a2c25cf3 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { resolve as resolvePath, extname } from 'path'; +import { extname } from 'path'; import { resolveSafeChildPath, UrlReader } from '@backstage/backend-common'; import { InputError } from '@backstage/errors'; import { ScmIntegrations } from '@backstage/integration'; @@ -114,7 +114,7 @@ export function createFetchTemplateAction(options: { ctx.logger.info('Fetching template content from remote URL'); const workDir = await ctx.createTemporaryDirectory(); - const templateDir = resolvePath(workDir, 'template'); + const templateDir = resolveSafeChildPath(workDir, 'template'); const targetPath = ctx.input.targetPath ?? './'; const outputDir = resolveSafeChildPath(ctx.workspacePath, targetPath); @@ -240,7 +240,7 @@ export function createFetchTemplateAction(options: { if (renderFilename) { localOutputPath = templater.renderString(localOutputPath, context); } - const outputPath = resolvePath(outputDir, localOutputPath); + const outputPath = resolveSafeChildPath(outputDir, localOutputPath); // variables have been expanded to make an empty file name // this is due to a conditional like if values.my_condition then file-name.txt else empty string so skip if (outputDir === outputPath) { @@ -259,7 +259,7 @@ export function createFetchTemplateAction(options: { ); await fs.ensureDir(outputPath); } else { - const inputFilePath = resolvePath(templateDir, location); + const inputFilePath = resolveSafeChildPath(templateDir, location); if (await isBinaryFile(inputFilePath)) { ctx.logger.info( diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts index 0cceb47d71..3a9dd01686 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts @@ -15,7 +15,6 @@ */ import fs from 'fs-extra'; -import path from 'path'; import { parseRepoUrl, isExecutable } from './util'; import { @@ -197,7 +196,7 @@ export const createPublishGithubPullRequestAction = ({ const fileContents = await Promise.all( localFilePaths.map(filePath => { - const absPath = path.resolve(fileRoot, filePath); + const absPath = resolveSafeChildPath(fileRoot, filePath); const base64EncodedContent = fs .readFileSync(absPath) .toString('base64');