From 2de1b29c1fa0b9f1bcd6bfc3913e66cdaae76f76 Mon Sep 17 00:00:00 2001
From: Michael Kriese <michael.kriese@visualon.de>
Date: Wed, 12 Apr 2023 17:13:34 +0200
Subject: [PATCH] feat(fs): add `isValidLocalPath` function (#21433)
---
lib/modules/manager/gitlabci/extract.ts | 7 +++++-
lib/util/fs/index.spec.ts | 13 ++++++++++-
lib/util/fs/index.ts | 11 ++++++++-
lib/util/fs/util.spec.ts | 30 ++++++++++++++++++++++++-
lib/util/fs/util.ts | 27 +++++++++++++++++++++-
5 files changed, 83 insertions(+), 5 deletions(-)
diff --git a/lib/modules/manager/gitlabci/extract.ts b/lib/modules/manager/gitlabci/extract.ts
index 6601e91580..acc67acfa1 100644
--- a/lib/modules/manager/gitlabci/extract.ts
+++ b/lib/modules/manager/gitlabci/extract.ts
@@ -1,7 +1,7 @@
import is from '@sindresorhus/is';
import { load } from 'js-yaml';
import { logger } from '../../../logger';
-import { readLocalFile } from '../../../util/fs';
+import { isValidLocalPath, readLocalFile } from '../../../util/fs';
import { trimLeadingSlash } from '../../../util/url';
import type {
ExtractConfig,
@@ -133,6 +133,11 @@ export async function extractAllPackageFiles(
while (filesToExamine.length > 0) {
const file = filesToExamine.pop()!;
+ if (!isValidLocalPath(file)) {
+ logger.debug(`Invalid gitlabci file path ${file}`);
+ continue;
+ }
+
const content = await readLocalFile(file, 'utf8');
if (!content) {
logger.debug(`Empty or non existent gitlabci file ${file}`);
diff --git a/lib/util/fs/index.spec.ts b/lib/util/fs/index.spec.ts
index 2e0af88fe0..e97ebf1a41 100644
--- a/lib/util/fs/index.spec.ts
+++ b/lib/util/fs/index.spec.ts
@@ -17,6 +17,7 @@ import {
getLocalFiles,
getParentDir,
getSiblingFileName,
+ isValidLocalPath,
listCacheDir,
localPathExists,
localPathIsFile,
@@ -205,7 +206,17 @@ describe('util/fs/index', () => {
});
it('returns false', async () => {
- expect(await localPathExists('file.txt')).toBe(false);
+ expect(await localPathExists('file.txt')).toBeFalse();
+ });
+ });
+
+ describe('isLocalPath', () => {
+ it('returns true for valid local path', () => {
+ expect(isValidLocalPath('./foo/...')).toBeTrue();
+ });
+
+ it('returns false', () => {
+ expect(isValidLocalPath('/file.txt')).toBeFalse();
});
});
diff --git a/lib/util/fs/index.ts b/lib/util/fs/index.ts
index 7b76b61db8..5493a6397f 100644
--- a/lib/util/fs/index.ts
+++ b/lib/util/fs/index.ts
@@ -6,7 +6,7 @@ import fs from 'fs-extra';
import upath from 'upath';
import { GlobalConfig } from '../../config/global';
import { logger } from '../../logger';
-import { ensureCachePath, ensureLocalPath } from './util';
+import { ensureCachePath, ensureLocalPath, isValidPath } from './util';
export const pipeline = util.promisify(stream.pipeline);
@@ -120,6 +120,15 @@ export async function localPathExists(pathName: string): Promise<boolean> {
}
}
+/**
+ * Validate local path without throwing.
+ * @param path Path to check
+ * @returns `true` if given `path` is a valid local path, otherwise `false`.
+ */
+export function isValidLocalPath(path: string): boolean {
+ return isValidPath(path, 'localDir');
+}
+
/**
* Tries to find `otherFileName` in the directory where
* `existingFileNameWithPath` is, then in its parent directory, then in the
diff --git a/lib/util/fs/util.spec.ts b/lib/util/fs/util.spec.ts
index f62fff1625..c37d971b06 100644
--- a/lib/util/fs/util.spec.ts
+++ b/lib/util/fs/util.spec.ts
@@ -1,7 +1,7 @@
import upath from 'upath';
import { GlobalConfig } from '../../config/global';
import { FILE_ACCESS_VIOLATION_ERROR } from '../../constants/error-messages';
-import { ensureCachePath, ensureLocalPath } from './util';
+import { ensureCachePath, ensureLocalPath, isValidPath } from './util';
describe('util/fs/util', () => {
const localDir = upath.resolve('/foo');
@@ -45,7 +45,35 @@ describe('util/fs/util', () => {
${'/bar/../foo'}
${'/bar/../../etc/passwd'}
${'/baz'}
+ ${'/baz"'}
`(`ensureCachePath('$path', '${cacheDir}') - throws`, ({ path }) => {
expect(() => ensureCachePath(path)).toThrow(FILE_ACCESS_VIOLATION_ERROR);
});
+
+ it.each`
+ value | expected
+ ${'.'} | ${true}
+ ${'./...'} | ${true}
+ ${'foo'} | ${true}
+ ${'foo/bar'} | ${true}
+ ${'./foo/bar'} | ${true}
+ ${'./foo/bar/...'} | ${true}
+ ${'..'} | ${false}
+ ${'....'} | ${true}
+ ${'./foo/..'} | ${true}
+ ${'./foo/..../bar'} | ${true}
+ ${'./..'} | ${false}
+ ${'\\foo'} | ${false}
+ ${"foo'"} | ${false}
+ ${'fo"o'} | ${false}
+ ${'fo&o'} | ${false}
+ ${'f;oo'} | ${true}
+ ${'f o o'} | ${true}
+ ${'/'} | ${false}
+ ${'/foo'} | ${false}
+ ${'&&'} | ${false}
+ ${';'} | ${true}
+ `('isValidPath($value) == $expected', ({ value, expected }) => {
+ expect(isValidPath(value, 'cacheDir')).toBe(expected);
+ });
});
diff --git a/lib/util/fs/util.ts b/lib/util/fs/util.ts
index 323e0257ff..91ed0f8de0 100644
--- a/lib/util/fs/util.ts
+++ b/lib/util/fs/util.ts
@@ -3,8 +3,12 @@ import { GlobalConfig } from '../../config/global';
import { FILE_ACCESS_VIOLATION_ERROR } from '../../constants/error-messages';
import { logger } from '../../logger';
+// http://www.mtu.edu/umc/services/digital/writing/characters-avoid/
+// We allow spaces, but not newlines
+const restricted = /[[\]#%&{}<>*?\b\n\r\0$!'"@|‘“+^`]/;
+
function assertBaseDir(path: string, baseDir: string): void {
- if (!path.startsWith(upath.resolve(baseDir))) {
+ if (!path.startsWith(baseDir)) {
logger.debug(
{ path, baseDir },
'Preventing access to file outside the base directory'
@@ -14,6 +18,11 @@ function assertBaseDir(path: string, baseDir: string): void {
}
function ensurePath(path: string, key: 'localDir' | 'cacheDir'): string {
+ if (restricted.test(path)) {
+ logger.debug({ path }, 'Preventing access to path with illegal characters');
+ throw new Error(FILE_ACCESS_VIOLATION_ERROR);
+ }
+
const baseDir = upath.resolve(GlobalConfig.get(key)!);
const fullPath = upath.resolve(
upath.isAbsolute(path) ? path : upath.join(baseDir, path)
@@ -29,3 +38,19 @@ export function ensureLocalPath(path: string): string {
export function ensureCachePath(path: string): string {
return ensurePath(path, 'cacheDir');
}
+
+export function isValidPath(
+ path: string,
+ key: 'localDir' | 'cacheDir'
+): boolean {
+ if (restricted.test(path)) {
+ return false;
+ }
+
+ const baseDir = upath.resolve(GlobalConfig.get(key)!);
+ const fullPath = upath.resolve(
+ upath.isAbsolute(path) ? path : upath.join(baseDir, path)
+ );
+
+ return fullPath.startsWith(baseDir);
+}
--
GitLab