From d0fe5f904e5e9b5cad20f930970b112fac8a33b6 Mon Sep 17 00:00:00 2001
From: Sergei Zharinov <zharinov@users.noreply.github.com>
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 79ca38ddb9..41dff572a5 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 698d9c1173..7ac9ac6f13 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 525c1cdf2a..71aa1257a7 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 dbf3e7a23e..c3cb9a7419 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<string | Buffer | null> {
- 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<void> {
- 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<void> {
- 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<void> {
- 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<void> {
@@ -75,17 +74,14 @@ export async function ensureDir(dirName: string): Promise<void> {
}
}
-export async function ensureLocalDir(dirName: string): Promise<void> {
- const { localDir } = GlobalConfig.get();
- const localDirName = upath.join(localDir, dirName);
- await fs.ensureDir(localDirName);
+export async function ensureLocalDir(dirName: string): Promise<string> {
+ const fullPath = ensureLocalPath(dirName);
+ await fs.ensureDir(fullPath);
+ return fullPath;
}
export async function ensureCacheDir(name: string): Promise<string> {
- 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<string> {
* 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<boolean> {
- const { localDir } = GlobalConfig.get();
+export async function localPathExists(pathName: string): Promise<boolean> {
// 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<string[]> {
- 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<boolean> {
- const { localDir } = GlobalConfig.get();
- return fs
- .stat(upath.join(localDir, pathName))
- .then((s) => s.isFile())
- .catch(() => false);
+export async function localPathIsFile(pathName: string): Promise<boolean> {
+ 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<void> {
- 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<fs.Stats | null> {
- 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<string[]> {
- return fs.readdir(path);
+ const fullPath = ensureCachePath(path);
+ return fs.readdir(fullPath);
}
export async function rmCache(path: string): Promise<void> {
- await fs.rm(path, { recursive: true });
+ const fullPath = ensureCachePath(path);
+ await fs.rm(fullPath, { recursive: true });
}
export async function readCacheFile(fileName: string): Promise<Buffer>;
@@ -230,15 +230,13 @@ export function readCacheFile(
fileName: string,
encoding?: string
): Promise<string | Buffer> {
- 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<void> {
- return fs.outputFile(file, data, options ?? {});
+export function outputCacheFile(file: string, data: unknown): Promise<void> {
+ const filePath = ensureCachePath(file);
+ return fs.outputFile(filePath, data);
}
export async function readSystemFile(fileName: string): Promise<Buffer>;
diff --git a/lib/util/fs/util.spec.ts b/lib/util/fs/util.spec.ts
new file mode 100644
index 0000000000..ae8c7d4c5b
--- /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 0000000000..f63226bf37
--- /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');
+}
--
GitLab