From d0fe5f904e5e9b5cad20f930970b112fac8a33b6 Mon Sep 17 00:00:00 2001 From: Sergei Zharinov Date: Thu, 14 Jul 2022 10:53:26 +0300 Subject: [PATCH] feat(fs): Scope checks for filesystem functions (#16511) --- lib/constants/error-messages.ts | 1 + .../manager/npm/post-update/yarn.spec.ts | 5 +- lib/util/fs/index.spec.ts | 135 +++++++----------- lib/util/fs/index.ts | 90 ++++++------ lib/util/fs/util.spec.ts | 50 +++++++ lib/util/fs/util.ts | 33 +++++ 6 files changed, 182 insertions(+), 132 deletions(-) create mode 100644 lib/util/fs/util.spec.ts create mode 100644 lib/util/fs/util.ts diff --git a/lib/constants/error-messages.ts b/lib/constants/error-messages.ts index 79ca38ddb9fe8f..41dff572a5a93b 100644 --- a/lib/constants/error-messages.ts +++ b/lib/constants/error-messages.ts @@ -41,6 +41,7 @@ export const NO_VULNERABILITY_ALERTS = 'no-vulnerability-alerts'; // Manager Error export const MANAGER_LOCKFILE_ERROR = 'lockfile-error'; +export const FILE_ACCESS_VIOLATION_ERROR = 'file-access-violation-error'; // Host error export const EXTERNAL_HOST_ERROR = 'external-host-error'; diff --git a/lib/modules/manager/npm/post-update/yarn.spec.ts b/lib/modules/manager/npm/post-update/yarn.spec.ts index 698d9c117344c6..7ac9ac6f13c026 100644 --- a/lib/modules/manager/npm/post-update/yarn.spec.ts +++ b/lib/modules/manager/npm/post-update/yarn.spec.ts @@ -300,9 +300,8 @@ describe('modules/manager/npm/post-update/yarn', () => { // subdirectory isolated workspaces to work with Yarn 2+. expect(res.lockFile).toBe(''); expect(fs.outputFile).toHaveBeenCalledTimes(1); - expect(fs.outputFile).toHaveBeenCalledWith( - 'some-dir/sub_workspace/yarn.lock', - '' + expect(mockedFunction(fs.outputFile).mock.calls[0][0]).toEndWith( + 'some-dir/sub_workspace/yarn.lock' ); expect(fixSnapshots(execSnapshots)).toMatchSnapshot(); } diff --git a/lib/util/fs/index.spec.ts b/lib/util/fs/index.spec.ts index 525c1cdf2ad32c..71aa1257a76305 100644 --- a/lib/util/fs/index.spec.ts +++ b/lib/util/fs/index.spec.ts @@ -2,8 +2,7 @@ import _findUp from 'find-up'; import fs from 'fs-extra'; import tmp, { DirectoryResult } from 'tmp-promise'; import { join } from 'upath'; -import { envMock } from '../../../test/exec-util'; -import { env, mockedFunction } from '../../../test/util'; +import { mockedFunction } from '../../../test/util'; import { GlobalConfig } from '../../config/global'; import { chmodLocalFile, @@ -37,17 +36,32 @@ jest.mock('find-up'); const findUp = mockedFunction(_findUp); describe('util/fs/index', () => { - let dirResult: DirectoryResult; + let localDirResult: DirectoryResult; + let localDir: string; + + let cacheDirResult: DirectoryResult; + let cacheDir: string; + + let tmpDirResult: DirectoryResult; let tmpDir: string; beforeEach(async () => { - GlobalConfig.set({ localDir: '' }); - dirResult = await tmp.dir({ unsafeCleanup: true }); - tmpDir = dirResult.path; + localDirResult = await tmp.dir({ unsafeCleanup: true }); + localDir = localDirResult.path; + + cacheDirResult = await tmp.dir({ unsafeCleanup: true }); + cacheDir = cacheDirResult.path; + + tmpDirResult = await tmp.dir({ unsafeCleanup: true }); + tmpDir = tmpDirResult.path; + + GlobalConfig.set({ localDir, cacheDir }); }); afterEach(async () => { - await dirResult.cleanup(); + await localDirResult?.cleanup(); + await cacheDirResult?.cleanup(); + await tmpDirResult?.cleanup(); }); describe('getParentDir', () => { @@ -87,23 +101,24 @@ describe('util/fs/index', () => { describe('readLocalFile', () => { it('reads buffer', async () => { - expect(await readLocalFile(__filename)).toBeInstanceOf(Buffer); + await fs.outputFile(`${localDir}/file.txt`, 'foobar'); + const res = await readLocalFile('file.txt'); + expect(res).toBeInstanceOf(Buffer); }); it('reads string', async () => { - expect(typeof (await readLocalFile(__filename, 'utf8'))).toBe('string'); + await fs.outputFile(`${localDir}/file.txt`, 'foobar'); + const res = await readLocalFile('file.txt', 'utf8'); + expect(res).toBe('foobar'); }); - it('does not throw', async () => { - // Does not work on FreeBSD: https://nodejs.org/docs/latest-v10.x/api/fs.html#fs_fs_readfile_path_options_callback - expect(await readLocalFile(__dirname)).toBeNull(); + it('returns null if file is not found', async () => { + expect(await readLocalFile('foobar')).toBeNull(); }); }); describe('writeLocalFile', () => { it('outputs file', async () => { - const localDir = tmpDir; - GlobalConfig.set({ localDir }); await writeLocalFile('foo/bar/file.txt', 'foobar'); const path = `${localDir}/foo/bar/file.txt`; @@ -114,8 +129,6 @@ describe('util/fs/index', () => { describe('deleteLocalFile', () => { it('deletes file', async () => { - const localDir = tmpDir; - GlobalConfig.set({ localDir }); const filePath = `${localDir}/foo/bar/file.txt`; await fs.outputFile(filePath, 'foobar'); @@ -127,8 +140,6 @@ describe('util/fs/index', () => { describe('renameLocalFile', () => { it('renames file', async () => { - const localDir = tmpDir; - GlobalConfig.set({ localDir }); const sourcePath = `${localDir}/foo.txt`; const targetPath = `${localDir}/bar.txt`; await fs.outputFile(sourcePath, 'foobar'); @@ -143,8 +154,6 @@ describe('util/fs/index', () => { describe('ensureDir', () => { it('creates directory', async () => { - const localDir = tmpDir; - GlobalConfig.set({ localDir }); const path = `${localDir}/foo/bar`; await ensureDir(path); @@ -155,8 +164,6 @@ describe('util/fs/index', () => { describe('ensureLocalDir', () => { it('creates local directory', async () => { - const localDir = tmpDir; - GlobalConfig.set({ localDir }); const path = `${localDir}/foo/bar`; await ensureLocalDir('foo/bar'); @@ -166,30 +173,11 @@ describe('util/fs/index', () => { }); describe('ensureCacheDir', () => { - function setupMock(root: string): { - dirFromEnv: string; - dirFromConfig: string; - } { - const dirFromEnv = join(root, join('/bar/others/bundler')); - const dirFromConfig = join(root, join('/bar')); - - jest.resetAllMocks(); - env.getChildProcessEnv.mockReturnValueOnce({ - ...envMock.basic, - }); - - GlobalConfig.set({ - cacheDir: join(dirFromConfig), - }); - - return { dirFromEnv, dirFromConfig }; - } - it('prefers environment variables over global config', async () => { - const { dirFromEnv } = setupMock(tmpDir); const res = await ensureCacheDir('bundler'); - expect(res).toEqual(dirFromEnv); - expect(await fs.pathExists(dirFromEnv)).toBeTrue(); + const path = join(cacheDir, 'others/bundler'); + expect(res).toEqual(path); + expect(await fs.pathExists(path)).toBeTrue(); }); }); @@ -202,25 +190,22 @@ describe('util/fs/index', () => { describe('localPathExists', () => { it('returns true for file', async () => { - expect(await localPathExists(__filename)).toBeTrue(); + const path = `${localDir}/file.txt`; + await fs.outputFile(path, 'foobar'); + expect(await localPathExists('file.txt')).toBeTrue(); }); it('returns true for directory', async () => { - expect(await localPathExists(getParentDir(__filename))).toBeTrue(); + expect(await localPathExists('.')).toBeTrue(); }); it('returns false', async () => { - expect(await localPathExists(__filename.replace('.ts', '.txt'))).toBe( - false - ); + expect(await localPathExists('file.txt')).toBe(false); }); }); describe('findLocalSiblingOrParent', () => { it('returns path for file', async () => { - const localDir = tmpDir; - GlobalConfig.set({ localDir }); - await writeLocalFile('crates/one/Cargo.toml', 'foo'); await writeLocalFile('Cargo.lock', 'bar'); @@ -252,8 +237,6 @@ describe('util/fs/index', () => { describe('readLocalDirectory', () => { it('returns dir content', async () => { - const localDir = tmpDir; - GlobalConfig.set({ localDir }); await writeLocalFile('test/Cargo.toml', ''); await writeLocalFile('test/Cargo.lock', ''); @@ -272,8 +255,6 @@ describe('util/fs/index', () => { }); it('return empty array for non existing directory', async () => { - const localDir = tmpDir; - GlobalConfig.set({ localDir }); await expect(readLocalDirectory('somedir')).rejects.toThrow(); }); @@ -287,10 +268,10 @@ describe('util/fs/index', () => { describe('createCacheWriteStream', () => { it('creates write stream', async () => { - const path = `${tmpDir}/file.txt`; + const path = `${cacheDir}/file.txt`; await fs.outputFile(path, 'foo'); - const stream = createCacheWriteStream(path); + const stream = createCacheWriteStream('file.txt'); expect(stream).toBeInstanceOf(fs.WriteStream); const write = new Promise((resolve, reject) => { @@ -304,17 +285,19 @@ describe('util/fs/index', () => { describe('localPathIsFile', () => { it('returns true for file', async () => { - expect(await localPathIsFile(__filename)).toBeTrue(); + const path = `${localDir}/file.txt`; + await fs.outputFile(path, 'foo'); + expect(await localPathIsFile('file.txt')).toBeTrue(); }); it('returns false for directory', async () => { - expect(await localPathIsFile(__dirname)).toBeFalse(); + const path = `${localDir}/foobar`; + await fs.mkdir(path); + expect(await localPathIsFile(path)).toBeFalse(); }); it('returns false for non-existing path', async () => { - expect( - await localPathIsFile(__filename.replace('.ts', '.txt')) - ).toBeFalse(); + expect(await localPathIsFile(`${localDir}/foobar`)).toBeFalse(); }); }); @@ -344,8 +327,6 @@ describe('util/fs/index', () => { describe('chmodLocalFile', () => { it('changes file mode', async () => { - const localDir = tmpDir; - GlobalConfig.set({ localDir }); await writeLocalFile('foo', 'bar'); let stat = await statLocalFile('foo'); const oldMode = stat!.mode & 0o777; @@ -363,9 +344,6 @@ describe('util/fs/index', () => { describe('statLocalFile', () => { it('returns stat object', async () => { - const localDir = tmpDir; - GlobalConfig.set({ localDir }); - expect(await statLocalFile('foo')).toBeNull(); await writeLocalFile('foo', 'bar'); @@ -377,19 +355,15 @@ describe('util/fs/index', () => { describe('listCacheDir', () => { it('lists directory', async () => { - const cacheDir = tmpDir; - GlobalConfig.set({ cacheDir }); await fs.outputFile(`${cacheDir}/foo/bar.txt`, 'foobar'); - expect(await listCacheDir(`${cacheDir}/foo`)).toEqual(['bar.txt']); + expect(await listCacheDir('foo')).toEqual(['bar.txt']); }); }); describe('rmCache', () => { it('removes cache dir', async () => { - const cacheDir = tmpDir; - GlobalConfig.set({ cacheDir }); await fs.outputFile(`${cacheDir}/foo/bar/file.txt`, 'foobar'); - await rmCache(`${cacheDir}/foo/bar`); + await rmCache(`foo/bar`); expect(await fs.pathExists(`${cacheDir}/foo/bar/file.txt`)).toBeFalse(); expect(await fs.pathExists(`${cacheDir}/foo/bar`)).toBeFalse(); }); @@ -397,13 +371,9 @@ describe('util/fs/index', () => { describe('readCacheFile', () => { it('reads file', async () => { - const cacheDir = tmpDir; - GlobalConfig.set({ cacheDir }); await fs.outputFile(`${cacheDir}/foo/bar/file.txt`, 'foobar'); - expect(await readCacheFile(`${cacheDir}/foo/bar/file.txt`, 'utf8')).toBe( - 'foobar' - ); - expect(await readCacheFile(`${cacheDir}/foo/bar/file.txt`)).toEqual( + expect(await readCacheFile(`foo/bar/file.txt`, 'utf8')).toBe('foobar'); + expect(await readCacheFile(`foo/bar/file.txt`)).toEqual( Buffer.from('foobar') ); }); @@ -411,9 +381,8 @@ describe('util/fs/index', () => { describe('outputCacheFile', () => { it('outputs file', async () => { - const file = join(tmpDir, 'some-file'); - await outputCacheFile(file, 'foobar'); - const res = await fs.readFile(file, 'utf8'); + await outputCacheFile('file.txt', 'foobar'); + const res = await fs.readFile(`${cacheDir}/file.txt`, 'utf8'); expect(res).toBe('foobar'); }); }); diff --git a/lib/util/fs/index.ts b/lib/util/fs/index.ts index dbf3e7a23ea2c2..c3cb9a7419d1cf 100644 --- a/lib/util/fs/index.ts +++ b/lib/util/fs/index.ts @@ -3,10 +3,10 @@ import util from 'util'; import is from '@sindresorhus/is'; import findUp from 'find-up'; import fs from 'fs-extra'; -import type { WriteFileOptions } from 'fs-extra'; import upath from 'upath'; import { GlobalConfig } from '../../config/global'; import { logger } from '../../logger'; +import { ensureCachePath, ensureLocalPath } from './util'; export const pipeline = util.promisify(stream.pipeline); @@ -31,8 +31,7 @@ export async function readLocalFile( fileName: string, encoding?: string ): Promise { - const { localDir } = GlobalConfig.get(); - const localFileName = upath.join(localDir, fileName); + const localFileName = ensureLocalPath(fileName); try { const fileContent = encoding ? await fs.readFile(localFileName, encoding) @@ -48,15 +47,14 @@ export async function writeLocalFile( fileName: string, fileContent: string | Buffer ): Promise { - const { localDir } = GlobalConfig.get(); - const localFileName = upath.join(localDir, fileName); + const localFileName = ensureLocalPath(fileName); await fs.outputFile(localFileName, fileContent); } export async function deleteLocalFile(fileName: string): Promise { - const { localDir } = GlobalConfig.get(); + const localDir = GlobalConfig.get('localDir'); if (localDir) { - const localFileName = upath.join(localDir, fileName); + const localFileName = ensureLocalPath(fileName); await fs.remove(localFileName); } } @@ -65,8 +63,9 @@ export async function renameLocalFile( fromFile: string, toFile: string ): Promise { - const { localDir } = GlobalConfig.get(); - await fs.move(upath.join(localDir, fromFile), upath.join(localDir, toFile)); + const fromPath = ensureLocalPath(fromFile); + const toPath = ensureLocalPath(toFile); + await fs.move(fromPath, toPath); } export async function ensureDir(dirName: string): Promise { @@ -75,17 +74,14 @@ export async function ensureDir(dirName: string): Promise { } } -export async function ensureLocalDir(dirName: string): Promise { - const { localDir } = GlobalConfig.get(); - const localDirName = upath.join(localDir, dirName); - await fs.ensureDir(localDirName); +export async function ensureLocalDir(dirName: string): Promise { + const fullPath = ensureLocalPath(dirName); + await fs.ensureDir(fullPath); + return fullPath; } export async function ensureCacheDir(name: string): Promise { - const cacheDirName = upath.join( - GlobalConfig.get('cacheDir'), - `others/${name}` - ); + const cacheDirName = ensureCachePath(`others/${name}`); await fs.ensureDir(cacheDirName); return cacheDirName; } @@ -96,17 +92,19 @@ export async function ensureCacheDir(name: string): Promise { * without risk of that information leaking to other repositories/users. */ export function privateCacheDir(): string { - const { cacheDir } = GlobalConfig.get(); + const cacheDir = GlobalConfig.get('cacheDir'); return upath.join(cacheDir, '__renovate-private-cache'); } -export function localPathExists(pathName: string): Promise { - const { localDir } = GlobalConfig.get(); +export async function localPathExists(pathName: string): Promise { // Works for both files as well as directories - return fs - .stat(upath.join(localDir, pathName)) - .then((s) => !!s) - .catch(() => false); + const path = ensureLocalPath(pathName); + try { + const s = await fs.stat(path); + return !!s; + } catch (_) { + return false; + } } /** @@ -142,22 +140,24 @@ export async function findLocalSiblingOrParent( * Get files by name from directory */ export async function readLocalDirectory(path: string): Promise { - const { localDir } = GlobalConfig.get(); - const localPath = upath.join(localDir, path); + const localPath = ensureLocalPath(path); const fileList = await fs.readdir(localPath); return fileList; } export function createCacheWriteStream(path: string): fs.WriteStream { - return fs.createWriteStream(path); + const fullPath = ensureCachePath(path); + return fs.createWriteStream(fullPath); } -export function localPathIsFile(pathName: string): Promise { - const { localDir } = GlobalConfig.get(); - return fs - .stat(upath.join(localDir, pathName)) - .then((s) => s.isFile()) - .catch(() => false); +export async function localPathIsFile(pathName: string): Promise { + const path = ensureLocalPath(pathName); + try { + const s = await fs.stat(path); + return s.isFile(); + } catch (_) { + return false; + } } /** @@ -196,16 +196,14 @@ export function chmodLocalFile( fileName: string, mode: string | number ): Promise { - const localDir = GlobalConfig.get('localDir'); - const fullFileName = upath.join(localDir, fileName); + const fullFileName = ensureLocalPath(fileName); return fs.chmod(fullFileName, mode); } export async function statLocalFile( fileName: string ): Promise { - const localDir = GlobalConfig.get('localDir'); - const fullFileName = upath.join(localDir, fileName); + const fullFileName = ensureLocalPath(fileName); try { return await fs.stat(fullFileName); } catch (_) { @@ -214,11 +212,13 @@ export async function statLocalFile( } export function listCacheDir(path: string): Promise { - return fs.readdir(path); + const fullPath = ensureCachePath(path); + return fs.readdir(fullPath); } export async function rmCache(path: string): Promise { - await fs.rm(path, { recursive: true }); + const fullPath = ensureCachePath(path); + await fs.rm(fullPath, { recursive: true }); } export async function readCacheFile(fileName: string): Promise; @@ -230,15 +230,13 @@ export function readCacheFile( fileName: string, encoding?: string ): Promise { - return encoding ? fs.readFile(fileName, encoding) : fs.readFile(fileName); + const fullPath = ensureCachePath(fileName); + return encoding ? fs.readFile(fullPath, encoding) : fs.readFile(fullPath); } -export function outputCacheFile( - file: string, - data: unknown, - options?: WriteFileOptions | string -): Promise { - return fs.outputFile(file, data, options ?? {}); +export function outputCacheFile(file: string, data: unknown): Promise { + const filePath = ensureCachePath(file); + return fs.outputFile(filePath, data); } export async function readSystemFile(fileName: string): Promise; diff --git a/lib/util/fs/util.spec.ts b/lib/util/fs/util.spec.ts new file mode 100644 index 00000000000000..ae8c7d4c5b9b70 --- /dev/null +++ b/lib/util/fs/util.spec.ts @@ -0,0 +1,50 @@ +import { GlobalConfig } from '../../config/global'; +import { FILE_ACCESS_VIOLATION_ERROR } from '../../constants/error-messages'; +import { ensureCachePath, ensureLocalPath } from './util'; + +describe('util/fs/util', () => { + const localDir = '/foo'; + const cacheDir = '/bar'; + + beforeAll(() => { + GlobalConfig.set({ localDir, cacheDir }); + }); + + test.each` + path | fullPath + ${''} | ${`${localDir}`} + ${'baz'} | ${`${localDir}/baz`} + ${'/baz'} | ${`${localDir}/baz`} + `(`ensureLocalPath('$path', '$fullPath')`, ({ path, fullPath }) => { + expect(ensureLocalPath(path)).toBe(fullPath); + }); + + test.each` + path + ${'..'} + ${'../etc/passwd'} + ${'/foo/../bar'} + ${'/foo/../../etc/passwd'} + `(`ensureLocalPath('$path', '${localDir}') - throws`, ({ path }) => { + expect(() => ensureLocalPath(path)).toThrow(FILE_ACCESS_VIOLATION_ERROR); + }); + + test.each` + path | fullPath + ${''} | ${`${cacheDir}`} + ${'baz'} | ${`${cacheDir}/baz`} + ${'/baz'} | ${`${cacheDir}/baz`} + `(`ensureCachePath('$path', '$fullPath')`, ({ path, fullPath }) => { + expect(ensureCachePath(path)).toBe(fullPath); + }); + + test.each` + path + ${'..'} + ${'../etc/passwd'} + ${'/bar/../foo'} + ${'/bar/../../etc/passwd'} + `(`ensureCachePath('$path', '${cacheDir}') - throws`, ({ path }) => { + expect(() => ensureCachePath(path)).toThrow(FILE_ACCESS_VIOLATION_ERROR); + }); +}); diff --git a/lib/util/fs/util.ts b/lib/util/fs/util.ts new file mode 100644 index 00000000000000..f63226bf37176c --- /dev/null +++ b/lib/util/fs/util.ts @@ -0,0 +1,33 @@ +import upath from 'upath'; +import { GlobalConfig } from '../../config/global'; +import { FILE_ACCESS_VIOLATION_ERROR } from '../../constants/error-messages'; +import { logger } from '../../logger'; + +function assertBaseDir(path: string, baseDir: string): void { + if (!path.startsWith(upath.resolve(baseDir))) { + logger.warn( + { path, baseDir }, + 'Preventing access to file outside the base directory' + ); + throw new Error(FILE_ACCESS_VIOLATION_ERROR); + } +} + +function ensurePath(path: string, key: 'localDir' | 'cacheDir'): string { + const baseDir = upath.resolve(GlobalConfig.get(key)!); + let fullPath = path; + if (fullPath.startsWith(baseDir)) { + fullPath = fullPath.replace(baseDir, ''); + } + fullPath = upath.resolve(upath.join(baseDir, fullPath)); + assertBaseDir(fullPath, baseDir); + return fullPath; +} + +export function ensureLocalPath(path: string): string { + return ensurePath(path, 'localDir'); +} + +export function ensureCachePath(path: string): string { + return ensurePath(path, 'cacheDir'); +}