Merge pull request #8234 from backstage/rugvip/noresolve

scaffolder-backend: replace all usage and prevent new usage of path.resolve

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'],
+      },
+    },
+  ],
 };

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),
       );
     },

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<string[]> {
   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];
     }),
   );

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,

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(

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');