diff --git a/src/utilities/fileUtils.test.ts b/src/utilities/fileUtils.test.ts index fa9ae246..91d3aa3a 100644 --- a/src/utilities/fileUtils.test.ts +++ b/src/utilities/fileUtils.test.ts @@ -3,12 +3,16 @@ import * as fileUtils from './fileUtils.js' import * as yaml from 'js-yaml' import fs from 'node:fs' +import os from 'node:os' import * as path from 'path' import {K8sObject} from '../types/k8sObject.js' const sampleYamlUrl = 'https://raw.githubusercontent.com/kubernetes/website/main/content/en/examples/controllers/nginx-deployment.yaml' describe('File utils', () => { + beforeAll(() => { + process.env.GITHUB_WORKSPACE ??= process.cwd() + }) test('correctly parses a yaml file from a URL', async () => { const tempFile = await fileUtils.writeYamlFromURLToFile(sampleYamlUrl, 0) const fileContents = fs.readFileSync(tempFile).toString() @@ -53,7 +57,7 @@ describe('File utils', () => { expect(testSearch).toHaveLength(10) expectedManifests.forEach((fileName) => { if (fileName.startsWith('test/unit')) { - expect(testSearch).toContain(fileName) + expect(testSearch).toContain(path.resolve(fileName)) } else { expect(fileName.includes(fileUtils.urlFileKind)).toBe(true) expect(fileName.startsWith(fileUtils.getTempDirectory())) @@ -105,20 +109,366 @@ describe('File utils', () => { fileUtils.writeYamlFromURLToFile(badUrl, 0) ).rejects.toBeTruthy() }) -}) -describe('moving files to temp', () => { - it('correctly moves the contents of a file to the temporary directory', () => { - vi.spyOn(fs, 'writeFileSync').mockImplementation(() => {}) - vi.spyOn(fs, 'readFileSync').mockImplementation((filename) => { - return 'test contents' - }) - const originalFilePath = path.join('path', 'in', 'repo') + it('rejects manifest inputs that resolve outside the workspace', async () => { + const originalWs = process.env.GITHUB_WORKSPACE + const ws = fs.mkdtempSync(path.join(os.tmpdir(), 'ws-')) + const outside = fs.mkdtempSync(path.join(os.tmpdir(), 'outside-')) + fs.writeFileSync(path.join(outside, 'secrets.yaml'), 'api_key: x') + process.env.GITHUB_WORKSPACE = ws + try { + await expect( + fileUtils.getFilesFromDirectoriesAndURLs([outside]) + ).rejects.toThrow(/outside the workspace/) + await expect( + fileUtils.getFilesFromDirectoriesAndURLs([ + path.join(outside, 'secrets.yaml') + ]) + ).rejects.toThrow(/outside the workspace/) + } finally { + if (originalWs === undefined) delete process.env.GITHUB_WORKSPACE + else process.env.GITHUB_WORKSPACE = originalWs + fs.rmSync(ws, {recursive: true, force: true}) + fs.rmSync(outside, {recursive: true, force: true}) + } + }) - const output = fileUtils.moveFileToTmpDir(originalFilePath) - - expect(output).toEqual( - path.join(fileUtils.getTempDirectory(), '/path/in/repo') - ) + it('rejects symlinks inside a directory that escape the workspace', async () => { + const originalWs = process.env.GITHUB_WORKSPACE + const ws = fs.mkdtempSync(path.join(os.tmpdir(), 'ws-')) + const outside = fs.mkdtempSync(path.join(os.tmpdir(), 'outside-')) + const escapeTarget = path.join(outside, 'passwd.yaml') + fs.writeFileSync(escapeTarget, 'root:x:0:0') + const dir = path.join(ws, 'manifests') + fs.mkdirSync(dir) + fs.symlinkSync(escapeTarget, path.join(dir, 'escape.yaml')) + process.env.GITHUB_WORKSPACE = ws + try { + await expect( + fileUtils.getFilesFromDirectoriesAndURLs([dir]) + ).rejects.toThrow(/outside the workspace/) + } finally { + if (originalWs === undefined) delete process.env.GITHUB_WORKSPACE + else process.env.GITHUB_WORKSPACE = originalWs + fs.rmSync(ws, {recursive: true, force: true}) + fs.rmSync(outside, {recursive: true, force: true}) + } + }) +}) + +describe('moveFileToTmpDir', () => { + let workspace: string + let originalWorkspace: string | undefined + let originalTemp: string | undefined + let originalCwd: string + let tmpDir: string + + beforeEach(() => { + originalWorkspace = process.env.GITHUB_WORKSPACE + originalTemp = process.env.RUNNER_TEMP + originalCwd = process.cwd() + workspace = fs.mkdtempSync(path.join(os.tmpdir(), 'ws-')) + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'rt-')) + process.env.GITHUB_WORKSPACE = workspace + process.env.RUNNER_TEMP = tmpDir + }) + + afterEach(() => { + process.chdir(originalCwd) + if (originalWorkspace === undefined) delete process.env.GITHUB_WORKSPACE + else process.env.GITHUB_WORKSPACE = originalWorkspace + if (originalTemp === undefined) delete process.env.RUNNER_TEMP + else process.env.RUNNER_TEMP = originalTemp + fs.rmSync(workspace, {recursive: true, force: true}) + fs.rmSync(tmpDir, {recursive: true, force: true}) + }) + + it('copies a workspace file to RUNNER_TEMP using a basename-only destination', () => { + const src = path.join(workspace, 'svc.yaml') + fs.writeFileSync(src, 'kind: Service') + + const out = fileUtils.moveFileToTmpDir(src) + + expect(fs.realpathSync(path.dirname(out))).toBe(fs.realpathSync(tmpDir)) + expect(path.basename(out)).toMatch(/^svc_\d+_\d+\.yaml$/) + expect(fs.readFileSync(out).toString()).toBe('kind: Service') + }) + + it('rejects relative traversal that escapes the workspace', () => { + const outside = fs.mkdtempSync(path.join(os.tmpdir(), 'outside-')) + fs.writeFileSync(path.join(outside, 'secrets.yaml'), 'api_key: x') + process.chdir(workspace) + const rel = path.relative(workspace, path.join(outside, 'secrets.yaml')) + expect(() => fileUtils.moveFileToTmpDir(rel)).toThrow( + /outside the workspace/ + ) + fs.rmSync(outside, {recursive: true, force: true}) + }) + + it('does not collide when two inputs share a basename', () => { + const a = path.join(workspace, 'a') + const b = path.join(workspace, 'b') + fs.mkdirSync(a) + fs.mkdirSync(b) + fs.writeFileSync(path.join(a, 'svc.yaml'), 'A') + fs.writeFileSync(path.join(b, 'svc.yaml'), 'B') + + const outA = fileUtils.moveFileToTmpDir(path.join(a, 'svc.yaml')) + const outB = fileUtils.moveFileToTmpDir(path.join(b, 'svc.yaml')) + + expect(outA).not.toBe(outB) + expect(fs.readFileSync(outA).toString()).toBe('A') + expect(fs.readFileSync(outB).toString()).toBe('B') + }) +}) + +describe('assertPathWithinWorkspace', () => { + let workspace: string + let outside: string + let originalWorkspace: string | undefined + let originalCwd: string + + beforeEach(() => { + originalWorkspace = process.env.GITHUB_WORKSPACE + originalCwd = process.cwd() + workspace = fs.mkdtempSync(path.join(os.tmpdir(), 'ws-')) + outside = fs.mkdtempSync(path.join(os.tmpdir(), 'outside-')) + process.env.GITHUB_WORKSPACE = workspace + }) + + afterEach(() => { + process.chdir(originalCwd) + if (originalWorkspace === undefined) { + delete process.env.GITHUB_WORKSPACE + } else { + process.env.GITHUB_WORKSPACE = originalWorkspace + } + fs.rmSync(workspace, {recursive: true, force: true}) + fs.rmSync(outside, {recursive: true, force: true}) + }) + + it('returns the resolved path for files inside the workspace', () => { + const inside = path.join(workspace, 'a.yaml') + fs.writeFileSync(inside, 'kind: X') + const result = fileUtils.assertPathWithinWorkspace(inside) + expect(result).toBe(fs.realpathSync(inside)) + }) + + it('accepts workspace files whose basename starts with ..', () => { + const inside = path.join(workspace, '..bar.yaml') + fs.writeFileSync(inside, 'kind: X') + expect(fileUtils.assertPathWithinWorkspace(inside)).toBe( + fs.realpathSync(inside) + ) + }) + + it('throws for relative traversal paths that escape the workspace', () => { + const target = path.join(outside, 'secrets.yaml') + fs.writeFileSync(target, 'api_key: secret') + const rel = path.relative(workspace, target) + process.chdir(workspace) + expect(() => fileUtils.assertPathWithinWorkspace(rel)).toThrow( + /outside the workspace/ + ) + }) + + it('resolves relative paths against GITHUB_WORKSPACE even when CWD differs', () => { + const inside = path.join(workspace, 'manifest.yaml') + fs.writeFileSync(inside, 'kind: X') + // Deliberately chdir somewhere unrelated so a process.cwd()-based + // resolver would either reject or resolve to the wrong place. + process.chdir(os.tmpdir()) + const result = fileUtils.assertPathWithinWorkspace('manifest.yaml') + expect(result).toBe(fs.realpathSync(inside)) + }) + + it('throws for absolute paths outside the workspace', () => { + const target = path.join(outside, 'secrets.yaml') + fs.writeFileSync(target, 'api_key: secret') + expect(() => fileUtils.assertPathWithinWorkspace(target)).toThrow( + /outside the workspace/ + ) + }) + + it('throws when a symlink inside the workspace points outside', () => { + const target = path.join(outside, 'secrets.yaml') + fs.writeFileSync(target, 'api_key: secret') + const link = path.join(workspace, 'evil.yaml') + fs.symlinkSync(target, link) + expect(() => fileUtils.assertPathWithinWorkspace(link)).toThrow( + /outside the workspace/ + ) + }) + + it('throws a clear error for missing files', () => { + const missing = path.join(workspace, 'nope.yaml') + expect(() => fileUtils.assertPathWithinWorkspace(missing)).toThrow( + /does not exist or is not readable/ + ) + }) + + it('skips containment when GITHUB_WORKSPACE is unset', () => { + delete process.env.GITHUB_WORKSPACE + const target = path.join(outside, 'whatever.yaml') + fs.writeFileSync(target, 'kind: X') + expect(fileUtils.assertPathWithinWorkspace(target)).toBe(target) + }) +}) + +import {EventEmitter} from 'node:events' +import {PassThrough} from 'node:stream' +import * as https from 'node:https' + +const httpsState = vi.hoisted(() => ({impl: null as any})) + +vi.mock('https', async (importOriginal) => { + const actual = await importOriginal() + const get = (...args: any[]) => + httpsState.impl ? httpsState.impl(...args) : (actual.get as any)(...args) + return { + ...actual, + default: {...actual, get}, + get + } +}) + +describe('writeYamlFromURLToFile error handling', () => { + let tempDir: string + let originalRunnerTemp: string | undefined + + beforeEach(() => { + originalRunnerTemp = process.env.RUNNER_TEMP + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'url-fetch-')) + process.env.RUNNER_TEMP = tempDir + }) + + afterEach(() => { + httpsState.impl = null + vi.restoreAllMocks() + if (originalRunnerTemp === undefined) { + delete process.env.RUNNER_TEMP + } else { + process.env.RUNNER_TEMP = originalRunnerTemp + } + fs.rmSync(tempDir, {recursive: true, force: true}) + }) + + // Wait one tick so cleanupAndReject's async fs.rm callback can fire + // before the test inspects the temp directory. + const waitForCleanup = () => + new Promise((r) => setImmediate(() => setImmediate(r))) + + function mockHttpsGet( + makeResponse: () => { + response: EventEmitter & { + statusCode?: number + statusMessage?: string + pipe: PassThrough['pipe'] + resume: () => void + } + requestEmitter: EventEmitter + } + ) { + httpsState.impl = ((url: string, cb?: any) => { + const {response, requestEmitter} = makeResponse() + if (cb) setImmediate(() => cb(response)) + return requestEmitter as any + }) as any + } + + it('rejects on HTTP 500 without writing a file', async () => { + const requestEmitter = new EventEmitter() + const response = Object.assign(new PassThrough(), { + statusCode: 500, + statusMessage: 'Server Error', + resume() { + /* drain */ + } + }) + mockHttpsGet(() => ({response: response as any, requestEmitter})) + + await expect( + fileUtils.writeYamlFromURLToFile('https://example.com/x.yaml', 99) + ).rejects.toThrow(/Server Error/) + }) + + it('rejects when the response stream errors mid-download', async () => { + const requestEmitter = new EventEmitter() + const response = Object.assign(new PassThrough(), { + statusCode: 200, + statusMessage: 'OK', + resume() {} + }) + mockHttpsGet(() => ({response: response as any, requestEmitter})) + + const p = fileUtils.writeYamlFromURLToFile( + 'https://example.com/y.yaml', + 100 + ) + setImmediate(() => response.emit('error', new Error('socket reset'))) + await expect(p).rejects.toThrow(/socket reset/) + }) + + it('rejects on request-level errors', async () => { + const requestEmitter = new EventEmitter() + const response = Object.assign(new PassThrough(), { + statusCode: 200, + resume() {} + }) + mockHttpsGet(() => ({response: response as any, requestEmitter})) + + const p = fileUtils.writeYamlFromURLToFile( + 'https://example.com/z.yaml', + 101 + ) + setImmediate(() => requestEmitter.emit('error', new Error('DNS failure'))) + await expect(p).rejects.toThrow(/DNS failure/) + }) + + it('removes temp file when verification fails', async () => { + const requestEmitter = new EventEmitter() + const response = Object.assign(new PassThrough(), { + statusCode: 200, + statusMessage: 'OK' + }) + mockHttpsGet(() => ({response: response as any, requestEmitter})) + + const before = new Set(fs.readdirSync(tempDir)) + const p = fileUtils.writeYamlFromURLToFile( + 'https://example.com/bad.yaml', + 200 + ) + // Stream a YAML document missing required k8s fields so verifyYaml fails. + setImmediate(() => { + response.end('not: a-real-manifest\n') + }) + await expect(p).rejects.toThrow(/missing fields|failed to parse/) + await waitForCleanup() + const after = fs.readdirSync(tempDir).filter((f) => !before.has(f)) + expect(after).toEqual([]) + }) + + it('removes temp file on mid-stream response error', async () => { + const requestEmitter = new EventEmitter() + const response = Object.assign(new PassThrough(), { + statusCode: 200, + statusMessage: 'OK', + resume() {} + }) + mockHttpsGet(() => ({response: response as any, requestEmitter})) + + const before = new Set(fs.readdirSync(tempDir)) + const p = fileUtils.writeYamlFromURLToFile( + 'https://example.com/midstream.yaml', + 201 + ) + setImmediate(() => { + response.write('kind: Foo\n') + response.emit('error', new Error('socket reset')) + }) + await expect(p).rejects.toThrow(/socket reset/) + await waitForCleanup() + const after = fs.readdirSync(tempDir).filter((f) => !before.has(f)) + expect(after).toEqual([]) }) }) diff --git a/src/utilities/fileUtils.ts b/src/utilities/fileUtils.ts index b70565a9..361a0945 100644 --- a/src/utilities/fileUtils.ts +++ b/src/utilities/fileUtils.ts @@ -11,10 +11,56 @@ import {K8sObject} from '../types/k8sObject.js' export const urlFileKind = 'urlfile' +let moveCounter = 0 + export function getTempDirectory(): string { return process.env['RUNNER_TEMP'] || os.tmpdir() } +// Exported for tests. Validates that `inputPath` resolves (after symlink +// resolution) to a location inside GITHUB_WORKSPACE. When GITHUB_WORKSPACE +// is not set (e.g. local dev / unit tests), the check is skipped — callers +// that write to RUNNER_TEMP still get protection from basename-only +// destinations. +export function assertPathWithinWorkspace(inputPath: string): string { + const workspace = process.env.GITHUB_WORKSPACE + if (!workspace) { + core.warning( + 'GITHUB_WORKSPACE is not set; skipping manifest path containment check' + ) + return inputPath + } + const resolvedWorkspace = fs.realpathSync(path.resolve(workspace)) + // Resolve relative inputs against the workspace (not process.cwd()), so + // a relative `manifests:` input is interpreted consistently regardless of + // whether a prior step changed the working directory. Absolute paths are + // passed through unchanged and still validated below. + const absoluteInput = path.isAbsolute(inputPath) + ? inputPath + : path.resolve(resolvedWorkspace, inputPath) + let resolvedInput: string + try { + resolvedInput = fs.realpathSync(absoluteInput) + } catch (e) { + throw new Error( + `manifest path ${inputPath} does not exist or is not readable: ${e}` + ) + } + const rel = path.relative(resolvedWorkspace, resolvedInput) + if ( + rel === '' || + (rel !== '..' && + !rel.startsWith('..' + path.sep) && + !path.isAbsolute(rel)) + ) { + return resolvedInput + } + throw new Error( + `manifest path ${inputPath} resolves to ${resolvedInput}, ` + + `which is outside the workspace ${resolvedWorkspace}` + ) +} + export function writeObjectsToFile(inputObjects: any[]): string[] { const newFilePaths = [] @@ -64,22 +110,20 @@ export function writeManifestToFile( } export function moveFileToTmpDir(originalFilepath: string) { + const safeSource = assertPathWithinWorkspace(originalFilepath) const tempDirectory = getTempDirectory() - const newPath = path.join(tempDirectory, originalFilepath) + const ext = path.extname(safeSource) + const base = path.basename(safeSource, ext) + const uniqueName = `${base}_${getCurrentTime()}_${moveCounter++}${ext}` + const newPath = path.join(tempDirectory, uniqueName) core.debug(`reading original contents from path: ${originalFilepath}`) - const contents = fs.readFileSync(originalFilepath).toString() + const contents = fs.readFileSync(safeSource) - const dirName = path.dirname(newPath) - if (!fs.existsSync(dirName)) { - core.debug(`path ${dirName} doesn't exist yet, making new dir...`) - fs.mkdirSync(dirName, {recursive: true}) - } core.debug(`writing contents to new path ${newPath}`) - fs.writeFileSync(path.join(newPath), contents) + fs.writeFileSync(newPath, contents) core.debug(`moved contents from ${originalFilepath} to ${newPath}`) - return newPath } @@ -109,15 +153,20 @@ export async function getFilesFromDirectoriesAndURLs( `encountered error trying to pull YAML from URL ${fileName}: ${e}` ) } - } else if (fs.lstatSync(fileName).isDirectory()) { - recurisveManifestGetter(fileName).forEach((file) => { + continue + } + + const safePath = assertPathWithinWorkspace(fileName) + + if (fs.lstatSync(safePath).isDirectory()) { + recurisveManifestGetter(safePath).forEach((file) => { fullPathSet.add(file) }) } else if ( - getFileExtension(fileName) === 'yml' || - getFileExtension(fileName) === 'yaml' + getFileExtension(safePath) === 'yml' || + getFileExtension(safePath) === 'yaml' ) { - fullPathSet.add(fileName) + fullPathSet.add(safePath) } else { core.debug( `Detected non-manifest file, ${fileName}, continuing... ` @@ -140,24 +189,33 @@ export async function writeYamlFromURLToFile( ): Promise { return new Promise((resolve, reject) => { https - .get(url, async (response) => { + .get(url, (response) => { const code = response.statusCode ?? 0 if (code >= 400) { + response.resume() reject( - Error( + new Error( `received response status ${response.statusMessage} from url ${url}` ) ) + return } const targetPath = getNewTempManifestFileName( urlFileKind, fileNumber.toString() ) - // save the file to disk - const fileWriter = fs - .createWriteStream(targetPath) - .on('finish', () => { + // Once the write stream is created the file exists on disk; + // route all post-stream rejections through this helper so we + // don't leave truncated YAML in RUNNER_TEMP for later tooling + // to pick up. Do NOT unlink on the success path. + const cleanupAndReject = (err: unknown) => { + fs.rm(targetPath, {force: true}, () => reject(err)) + } + const fileWriter = fs.createWriteStream(targetPath) + fileWriter.on('error', cleanupAndReject) + fileWriter.on('finish', () => { + try { const verification = verifyYaml(targetPath, url) if (succeeded(verification)) { core.debug( @@ -167,15 +225,16 @@ export async function writeYamlFromURLToFile( ) resolve(targetPath) } else { - reject(verification.error) + cleanupAndReject(new Error(verification.error)) } - }) - + } catch (e) { + cleanupAndReject(e) + } + }) + response.on('error', cleanupAndReject) response.pipe(fileWriter) }) - .on('error', (error) => { - reject(error) - }) + .on('error', reject) }) } @@ -199,7 +258,7 @@ function verifyYaml(filepath: string, url: string): Errorable { } for (const obj of inputObjects) { - if (!obj.kind || !obj.apiVersion || !obj.metadata) { + if (obj == null || !obj.kind || !obj.apiVersion || !obj.metadata) { return { succeeded: false, error: `failed to parse manifest from ${url}: missing fields` @@ -221,7 +280,7 @@ function recurisveManifestGetter(dirName: string): string[] { getFileExtension(fileName) === 'yml' || getFileExtension(fileName) === 'yaml' ) { - toRet.push(path.join(dirName, fileName)) + toRet.push(assertPathWithinWorkspace(fnwd)) } else { core.debug(`Detected non-manifest file, ${fileName}, continuing... `) }