From ab5cc376fac968c0643d1b624fb37d3117d399de Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 24 Jun 2021 17:27:05 +0200 Subject: [PATCH] backend-common,cli-common: new utilites for safely resolving child paths Signed-off-by: Patrik Oldsberg --- .changeset/loud-pets-hang.md | 6 ++ .changeset/many-penguins-clap.md | 5 ++ .changeset/silent-avocados-beam.md | 5 ++ .changeset/six-weeks-shop.md | 5 ++ packages/backend-common/api-report.md | 6 ++ packages/backend-common/src/paths.ts | 26 +++++++ packages/cli-common/api-report.md | 3 + packages/cli-common/src/index.ts | 1 + packages/cli-common/src/isChildPath.test.ts | 74 +++++++++++++++++++ packages/cli-common/src/isChildPath.ts | 33 +++++++++ .../lib/bundler/LinkedPackageResolvePlugin.ts | 2 +- packages/cli/src/lib/bundler/config.ts | 3 +- packages/cli/src/lib/bundler/paths.ts | 17 ----- .../src/stages/generate/helpers.ts | 7 +- .../builtin/fetch/cookiecutter.test.ts | 2 +- .../actions/builtin/fetch/cookiecutter.ts | 9 +-- .../actions/builtin/fetch/plain.test.ts | 2 +- .../scaffolder/actions/builtin/fetch/plain.ts | 11 +-- 18 files changed, 175 insertions(+), 42 deletions(-) create mode 100644 .changeset/loud-pets-hang.md create mode 100644 .changeset/many-penguins-clap.md create mode 100644 .changeset/silent-avocados-beam.md create mode 100644 .changeset/six-weeks-shop.md create mode 100644 packages/cli-common/src/isChildPath.test.ts create mode 100644 packages/cli-common/src/isChildPath.ts diff --git a/.changeset/loud-pets-hang.md b/.changeset/loud-pets-hang.md new file mode 100644 index 0000000000..52e5c8a7dd --- /dev/null +++ b/.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 diff --git a/.changeset/many-penguins-clap.md b/.changeset/many-penguins-clap.md new file mode 100644 index 0000000000..be88536af6 --- /dev/null +++ b/.changeset/many-penguins-clap.md @@ -0,0 +1,5 @@ +--- +'@backstage/cli-common': patch +--- + +Add new `isChildPath` export diff --git a/.changeset/silent-avocados-beam.md b/.changeset/silent-avocados-beam.md new file mode 100644 index 0000000000..b3f2796b86 --- /dev/null +++ b/.changeset/silent-avocados-beam.md @@ -0,0 +1,5 @@ +--- +'@backstage/cli': patch +--- + +Use new `isChildPath` export from `@backstage/cli-common` diff --git a/.changeset/six-weeks-shop.md b/.changeset/six-weeks-shop.md new file mode 100644 index 0000000000..3adb37d144 --- /dev/null +++ b/.changeset/six-weeks-shop.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': patch +--- + +Add new `isChildPath` and `resolveSafeChildPath` exports diff --git a/packages/backend-common/api-report.md b/packages/backend-common/api-report.md index fcddb8dda5..6b9e7bbbe0 100644 --- a/packages/backend-common/api-report.md +++ b/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; @@ -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; diff --git a/packages/backend-common/src/paths.ts b/packages/backend-common/src/paths.ts index efe8e027c8..ffe99fe762 100644 --- a/packages/backend-common/src/paths.ts +++ b/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 }; diff --git a/packages/cli-common/api-report.md b/packages/cli-common/api-report.md index bcdc023c22..7141d27f3a 100644 --- a/packages/cli-common/api-report.md +++ b/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; diff --git a/packages/cli-common/src/index.ts b/packages/cli-common/src/index.ts index d36314ba35..1f4f17fe9f 100644 --- a/packages/cli-common/src/index.ts +++ b/packages/cli-common/src/index.ts @@ -15,4 +15,5 @@ */ export { findPaths } from './paths'; +export { isChildPath } from './isChildPath'; export type { Paths } from './paths'; diff --git a/packages/cli-common/src/isChildPath.test.ts b/packages/cli-common/src/isChildPath.test.ts new file mode 100644 index 0000000000..4c0d94f07c --- /dev/null +++ b/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'); + }); + }); +}); diff --git a/packages/cli-common/src/isChildPath.ts b/packages/cli-common/src/isChildPath.ts new file mode 100644 index 0000000000..f48c92db32 --- /dev/null +++ b/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; +} diff --git a/packages/cli/src/lib/bundler/LinkedPackageResolvePlugin.ts b/packages/cli/src/lib/bundler/LinkedPackageResolvePlugin.ts index 2360078a18..aa6a6b4e2e 100644 --- a/packages/cli/src/lib/bundler/LinkedPackageResolvePlugin.ts +++ b/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. diff --git a/packages/cli/src/lib/bundler/config.ts b/packages/cli/src/lib/bundler/config.ts index 89f677109d..75e94af716 100644 --- a/packages/cli/src/lib/bundler/config.ts +++ b/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'; diff --git a/packages/cli/src/lib/bundler/paths.ts b/packages/cli/src/lib/bundler/paths.ts index 01fdcdefe5..d465b3010f 100644 --- a/packages/cli/src/lib/bundler/paths.ts +++ b/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; diff --git a/packages/techdocs-common/src/stages/generate/helpers.ts b/packages/techdocs-common/src/stages/generate/helpers.ts index 718ee9f924..086377b4ef 100644 --- a/packages/techdocs-common/src/stages/generate/helpers.ts +++ b/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.`, diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/cookiecutter.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/cookiecutter.test.ts index 123f5e0ebb..3ce3052956 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/cookiecutter.test.ts +++ b/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/, ); }); }); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/cookiecutter.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/cookiecutter.ts index 9679adffa8..62dfc20125 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/cookiecutter.ts +++ b/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); }, }); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/plain.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/plain.test.ts index 2f6caf8c97..1a4428e3f0 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/plain.test.ts +++ b/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/, ); }); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/plain.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/plain.ts index 8f7eefb657..9cac6d5ab8 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/plain.ts +++ b/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,