backend-common,cli-common: new utilites for safely resolving child paths

Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>

.changeset/loud-pets-hang.md

@@ -0,0 +1,6 @@
+---
+'@backstage/techdocs-common': patch
+'@backstage/plugin-scaffolder-backend': patch
+---
+
+Use new utilities from `@backstage/backend-common` for safely resolving child paths

.changeset/many-penguins-clap.md

@@ -0,0 +1,5 @@
+---
+'@backstage/cli-common': patch
+---
+
+Add new `isChildPath` export

.changeset/silent-avocados-beam.md

@@ -0,0 +1,5 @@
+---
+'@backstage/cli': patch
+---
+
+Use new `isChildPath` export from `@backstage/cli-common`

.changeset/six-weeks-shop.md

@@ -0,0 +1,5 @@
+---
+'@backstage/backend-common': patch
+---
+
+Add new `isChildPath` and `resolveSafeChildPath` exports

packages/backend-common/api-report.md

@@ -16,6 +16,7 @@ import { GithubCredentialsProvider } from '@backstage/integration';
 import { GitHubIntegration } from '@backstage/integration';
 import { GitLabIntegration } from '@backstage/integration';
 import * as http from 'http';
+import { isChildPath } from '@backstage/cli-common';
 import { JsonValue } from '@backstage/config';
 import { Knex } from 'knex';
 import { Logger } from 'winston';
@@ -252,6 +253,8 @@ export class GitlabUrlReader implements UrlReader {
     toString(): string;
 }
 
+export { isChildPath }
+
 // @public
 export function loadBackendConfig(options: Options): Promise<Config>;
 
@@ -294,6 +297,9 @@ export function requestLoggingHandler(logger?: Logger): RequestHandler;
 // @public
 export function resolvePackagePath(name: string, ...paths: string[]): string;
 
+// @public
+export function resolveSafeChildPath(base: string, path: string): string;
+
 // @public (undocumented)
 export type RunContainerOptions = {
     imageName: string;

packages/backend-common/src/paths.ts

@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+import { isChildPath } from '@backstage/cli-common';
+import { NotAllowedError } from '@backstage/errors';
 import { resolve as resolvePath } from 'path';
 
 /**
@@ -32,3 +34,27 @@ export function resolvePackagePath(name: string, ...paths: string[]) {
 
   return resolvePath(req.resolve(`${name}/package.json`), '..', ...paths);
 }
+
+/**
+ * Resolves a target path from a base path while guaranteeing that the result is
+ * a path that point to or within the base path. This is useful for resolving
+ * paths from user input, as it otherwise opens up for vulnerabilities.
+ *
+ * @param base The base directory to resolve the path from.
+ * @param path The target path, relative or absolute
+ * @returns A path that is guaranteed to point to or within the base path.
+ */
+export function resolveSafeChildPath(base: string, path: string): string {
+  const targetPath = resolvePath(base, path);
+
+  if (!isChildPath(base, targetPath)) {
+    throw new NotAllowedError(
+      'Relative path is not allowed to refer to a directory outside its parent',
+    );
+  }
+
+  return targetPath;
+}
+
+// Re-export isChildPath so that backend packages don't need to depend on cli-common
+export { isChildPath };

packages/cli-common/api-report.md

@@ -7,6 +7,9 @@
 // @public
 export function findPaths(searchDir: string): Paths;
 
+// @public
+export function isChildPath(base: string, path: string): boolean;
+
 // @public (undocumented)
 export type Paths = {
     ownDir: string;

packages/cli-common/src/index.ts

@@ -15,4 +15,5 @@
  */
 
 export { findPaths } from './paths';
+export { isChildPath } from './isChildPath';
 export type { Paths } from './paths';

packages/cli-common/src/isChildPath.test.ts

@@ -0,0 +1,74 @@
+/*
+ * Copyright 2021 The Backstage Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { posix, win32 } from 'path';
+
+describe('isChildPath', () => {
+  it('should check child posix paths', () => {
+    jest.isolateModules(() => {
+      jest.setMock('path', posix);
+      const { isChildPath } = require('./isChildPath');
+
+      expect(isChildPath('/', '/')).toBe(true);
+      expect(isChildPath('/x', '/x')).toBe(true);
+      expect(isChildPath('/x', '/x/y')).toBe(true);
+      expect(isChildPath('/x', '/x/x')).toBe(true);
+      expect(isChildPath('/x', '/x/y/z')).toBe(true);
+      expect(isChildPath('/x/y', '/x/y/z')).toBe(true);
+      expect(isChildPath('/x/y/z', '/x/y/z')).toBe(true);
+      expect(isChildPath('/x/a b c/z', '/x/a b c/z')).toBe(true);
+      expect(isChildPath('/', '/ yz')).toBe(true);
+
+      expect(isChildPath('/x', '/y')).toBe(false);
+      expect(isChildPath('/x', '/')).toBe(false);
+      expect(isChildPath('/x', '/x y')).toBe(false);
+      expect(isChildPath('/x y', '/x yz')).toBe(false);
+      expect(isChildPath('/ yz', '/')).toBe(false);
+      expect(isChildPath('/x', '/')).toBe(false);
+
+      jest.dontMock('path');
+    });
+  });
+
+  it('should check child win32 paths', () => {
+    jest.isolateModules(() => {
+      jest.setMock('path', win32);
+      const { isChildPath } = require('./isChildPath');
+
+      expect(isChildPath('/x', '/x')).toBe(true);
+      expect(isChildPath('/x', '/x/y')).toBe(true);
+      expect(isChildPath('/x', '/x/x')).toBe(true);
+      expect(isChildPath('/x', '/x/y/z')).toBe(true);
+      expect(isChildPath('/x/y', '/x/y/z')).toBe(true);
+      expect(isChildPath('/x/y/z', '/x/y/z')).toBe(true);
+      expect(isChildPath('Z:', 'Z:')).toBe(true);
+      expect(isChildPath('C:/', 'c:/')).toBe(true);
+      expect(isChildPath('C:/x', 'C:/x')).toBe(true);
+      expect(isChildPath('C:/x', 'c:/x')).toBe(true);
+      expect(isChildPath('C:/x', 'C:/x/y')).toBe(true);
+      expect(isChildPath('d:/x', 'D:/x/y')).toBe(true);
+
+      expect(isChildPath('/x', '/y')).toBe(false);
+      expect(isChildPath('/x', '/')).toBe(false);
+      expect(isChildPath('C:/', 'D:/')).toBe(false);
+      expect(isChildPath('C:/x', 'D:/x')).toBe(false);
+      expect(isChildPath('D:/x', 'CD:/x')).toBe(false);
+      expect(isChildPath('D:/x', 'D:/y')).toBe(false);
+
+      jest.dontMock('path');
+    });
+  });
+});

packages/cli-common/src/isChildPath.ts

@@ -0,0 +1,33 @@
+/*
+ * Copyright 2021 The Backstage Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { relative, isAbsolute } from 'path';
+
+/**
+ * Checks if path is the same as or a child path of base.
+ */
+export function isChildPath(base: string, path: string): boolean {
+  const relativePath = relative(base, path);
+  if (relativePath === '') {
+    // The same directory
+    return true;
+  }
+
+  const outsideBase = relativePath.startsWith('..'); // not outside base
+  const differentDrive = isAbsolute(relativePath); // on Windows, this means dir is on a different drive from base.
+
+  return !outsideBase && !differentDrive;
+}

packages/cli/src/lib/bundler/LinkedPackageResolvePlugin.ts

@@ -16,7 +16,7 @@
 
 import { resolve as resolvePath } from 'path';
 import { ResolvePlugin } from 'webpack';
-import { isChildPath } from './paths';
+import { isChildPath } from '@backstage/cli-common';
 import { LernaPackage } from './types';
 
 // Enables proper resolution of packages when linking in external packages.

packages/cli/src/lib/bundler/config.ts

@@ -22,9 +22,10 @@ import ModuleScopePlugin from 'react-dev-utils/ModuleScopePlugin';
 import StartServerPlugin from 'start-server-webpack-plugin';
 import webpack from 'webpack';
 import nodeExternals from 'webpack-node-externals';
+import { isChildPath } from '@backstage/cli-common';
 import { optimization } from './optimization';
 import { Config } from '@backstage/config';
-import { BundlingPaths, isChildPath } from './paths';
+import { BundlingPaths } from './paths';
 import { transforms } from './transforms';
 import { LinkedPackageResolvePlugin } from './LinkedPackageResolvePlugin';
 import { BundlingOptions, BackendBundlingOptions, LernaPackage } from './types';

packages/cli/src/lib/bundler/paths.ts

@@ -15,25 +15,8 @@
  */
 
 import fs from 'fs-extra';
-import path from 'path';
 import { paths } from '../paths';
 
-/**
- * Checks if dir is the same as or a child of base.
- */
-export function isChildPath(base: string, dir: string): boolean {
-  const relativePath = path.relative(base, dir);
-  if (relativePath === '') {
-    // The same directory
-    return true;
-  }
-
-  const outsideBase = relativePath.startsWith('..'); // not outside base
-  const differentDrive = path.isAbsolute(relativePath); // on Windows, this means dir is on a different drive from base.
-
-  return !outsideBase && !differentDrive;
-}
-
 export type BundlingPathsOptions = {
   // bundle entrypoint, e.g. 'src/index'
   entry: string;

packages/techdocs-common/src/stages/generate/helpers.ts

@@ -15,6 +15,7 @@
  */
 
 import { Entity } from '@backstage/catalog-model';
+import { isChildPath } from '@backstage/backend-common';
 import { spawn } from 'child_process';
 import fs from 'fs-extra';
 import yaml, { DEFAULT_SCHEMA, Type } from 'js-yaml';
@@ -23,7 +24,6 @@ import { Logger } from 'winston';
 import { ParsedLocationAnnotation } from '../../helpers';
 import { RemoteProtocol } from '../prepare/types';
 import { SupportedGeneratorKey } from './types';
-import { resolve as resolvePath } from 'path';
 
 // TODO: Implement proper support for more generators.
 export function getGeneratorKey(entity: Entity): SupportedGeneratorKey {
@@ -178,10 +178,7 @@ export const validateMkdocsYaml = async (
     schema: MKDOCS_SCHEMA,
   });
 
-  if (
-    mkdocsYml.docs_dir &&
-    !resolvePath(inputDir, mkdocsYml.docs_dir).startsWith(inputDir)
-  ) {
+  if (mkdocsYml.docs_dir && !isChildPath(inputDir, mkdocsYml.docs_dir)) {
     throw new Error(
       `docs_dir configuration value in mkdocs can't be an absolute directory or start with ../ for security reasons.
        Use relative paths instead which are resolved relative to your mkdocs.yml file location.`,

plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/cookiecutter.test.ts

@@ -180,7 +180,7 @@ describe('fetch:cookiecutter', () => {
         },
       }),
     ).rejects.toThrow(
-      /targetPath may not specify a path outside the working directory/,
+      /Relative path is not allowed to refer to a directory outside its parent/,
     );
   });
 });

plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/cookiecutter.ts

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { UrlReader } from '@backstage/backend-common';
+import { UrlReader, resolveSafeChildPath } from '@backstage/backend-common';
 import { JsonObject } from '@backstage/config';
 import { InputError } from '@backstage/errors';
 import { ScmIntegrations } from '@backstage/integration';
@@ -138,12 +138,7 @@ export function createFetchCookiecutterAction(options: {
 
       // Finally move the template result into the task workspace
       const targetPath = ctx.input.targetPath ?? './';
-      const outputPath = resolvePath(ctx.workspacePath, targetPath);
-      if (!outputPath.startsWith(ctx.workspacePath)) {
-        throw new InputError(
-          `Fetch action targetPath may not specify a path outside the working directory`,
-        );
-      }
+      const outputPath = resolveSafeChildPath(ctx.workspacePath, targetPath);
       await fs.copy(resultDir, outputPath);
     },
   });

plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/plain.test.ts

@@ -62,7 +62,7 @@ describe('fetch:plain', () => {
         },
       }),
     ).rejects.toThrow(
-      /Fetch action targetPath may not specify a path outside the working directory/,
+      /Relative path is not allowed to refer to a directory outside its parent/,
     );
   });
 

plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/plain.ts

@@ -14,9 +14,7 @@
  * limitations under the License.
  */
 
-import path from 'path';
-import { UrlReader } from '@backstage/backend-common';
-import { InputError } from '@backstage/errors';
+import { UrlReader, resolveSafeChildPath } from '@backstage/backend-common';
 import { ScmIntegrations } from '@backstage/integration';
 import { fetchContents } from './helpers';
 import { createTemplateAction } from '../../createTemplateAction';
@@ -56,12 +54,7 @@ export function createFetchPlainAction(options: {
 
       // Finally move the template result into the task workspace
       const targetPath = ctx.input.targetPath ?? './';
-      const outputPath = path.resolve(ctx.workspacePath, targetPath);
-      if (!outputPath.startsWith(ctx.workspacePath)) {
-        throw new InputError(
-          `Fetch action targetPath may not specify a path outside the working directory`,
-        );
-      }
+      const outputPath = resolveSafeChildPath(ctx.workspacePath, targetPath);
 
       await fetchContents({
         reader,