From 4beda9cbc00ba6eefe387a937c21087ccb8ee9df Mon Sep 17 00:00:00 2001 From: Cory Miller <13227161+cory-miller@users.noreply.github.com> Date: Mon, 8 Aug 2022 14:16:39 -0400 Subject: [PATCH] Merge pull request from GHSA-7r3h-m5j6-3q42 * use uuid as our multiline env delimiter * remove extra fn * Fix version * also throw error if delimiter is found in name or value * move delimiter and uuid to global var in test * upgrade uuid to newest version * remove spy variable * Update packages/core/src/core.ts Co-authored-by: Thomas Boop <52323235+thboop@users.noreply.github.com> * Update packages/core/src/core.ts Co-authored-by: Thomas Boop <52323235+thboop@users.noreply.github.com> --- packages/core/RELEASES.md | 3 ++ packages/core/__tests__/core.test.ts | 44 ++++++++++++++++++++++++++-- packages/core/package-lock.json | 35 +++++++++++++++++++--- packages/core/package.json | 8 +++-- packages/core/src/core.ts | 13 +++++++- 5 files changed, 92 insertions(+), 11 deletions(-) diff --git a/packages/core/RELEASES.md b/packages/core/RELEASES.md index 4e0c3a7b..c8def6fc 100644 --- a/packages/core/RELEASES.md +++ b/packages/core/RELEASES.md @@ -1,5 +1,8 @@ # @actions/core Releases +### 1.9.1 +- Randomize delimiter when calling `core.exportVariable` + ### 1.9.0 - Added `toPosixPath`, `toWin32Path` and `toPlatformPath` utilities [#1102](https://github.com/actions/toolkit/pull/1102) diff --git a/packages/core/__tests__/core.test.ts b/packages/core/__tests__/core.test.ts index 7ffcc6c9..fd7fd572 100644 --- a/packages/core/__tests__/core.test.ts +++ b/packages/core/__tests__/core.test.ts @@ -4,6 +4,9 @@ import * as path from 'path' import * as core from '../src/core' import {HttpClient} from '@actions/http-client' import {toCommandProperties} from '../src/utils' +import * as uuid from 'uuid' + +jest.mock('uuid') /* eslint-disable @typescript-eslint/unbound-method */ @@ -41,6 +44,9 @@ const testEnvVars = { GITHUB_ENV: '' } +const UUID = '9b1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d' +const DELIMITER = `ghadelimiter_${UUID}` + describe('@actions/core', () => { beforeAll(() => { const filePath = path.join(__dirname, `test`) @@ -54,6 +60,14 @@ describe('@actions/core', () => { process.env[key] = testEnvVars[key as keyof typeof testEnvVars] } process.stdout.write = jest.fn() + + jest.spyOn(uuid, 'v4').mockImplementation(() => { + return UUID + }) + }) + + afterEach(() => { + jest.restoreAllMocks() }) it('legacy exportVariable produces the correct command and sets the env', () => { @@ -91,7 +105,7 @@ describe('@actions/core', () => { core.exportVariable('my var', 'var val') verifyFileCommand( command, - `my var<<_GitHubActionsFileCommandDelimeter_${os.EOL}var val${os.EOL}_GitHubActionsFileCommandDelimeter_${os.EOL}` + `my var<<${DELIMITER}${os.EOL}var val${os.EOL}${DELIMITER}${os.EOL}` ) }) @@ -101,7 +115,7 @@ describe('@actions/core', () => { core.exportVariable('my var', true) verifyFileCommand( command, - `my var<<_GitHubActionsFileCommandDelimeter_${os.EOL}true${os.EOL}_GitHubActionsFileCommandDelimeter_${os.EOL}` + `my var<<${DELIMITER}${os.EOL}true${os.EOL}${DELIMITER}${os.EOL}` ) }) @@ -111,10 +125,34 @@ describe('@actions/core', () => { core.exportVariable('my var', 5) verifyFileCommand( command, - `my var<<_GitHubActionsFileCommandDelimeter_${os.EOL}5${os.EOL}_GitHubActionsFileCommandDelimeter_${os.EOL}` + `my var<<${DELIMITER}${os.EOL}5${os.EOL}${DELIMITER}${os.EOL}` ) }) + it('exportVariable does not allow delimiter as value', () => { + const command = 'ENV' + createFileCommandFile(command) + + expect(() => { + core.exportVariable('my var', `good stuff ${DELIMITER} bad stuff`) + }).toThrow(`Unexpected input: value should not contain the delimiter "${DELIMITER}"`) + + const filePath = path.join(__dirname, `test/${command}`) + fs.unlinkSync(filePath) + }) + + it('exportVariable does not allow delimiter as name', () => { + const command = 'ENV' + createFileCommandFile(command) + + expect(() => { + core.exportVariable(`good stuff ${DELIMITER} bad stuff`, 'test') + }).toThrow(`Unexpected input: name should not contain the delimiter "${DELIMITER}"`) + + const filePath = path.join(__dirname, `test/${command}`) + fs.unlinkSync(filePath) + }) + it('setSecret produces the correct command', () => { core.setSecret('secret val') assertWriteCalls([`::add-mask::secret val${os.EOL}`]) diff --git a/packages/core/package-lock.json b/packages/core/package-lock.json index 2453168a..119f418e 100644 --- a/packages/core/package-lock.json +++ b/packages/core/package-lock.json @@ -1,18 +1,20 @@ { "name": "@actions/core", - "version": "1.9.0", + "version": "1.9.1", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@actions/core", - "version": "1.8.1", + "version": "1.9.1", "license": "MIT", "dependencies": { - "@actions/http-client": "^2.0.1" + "@actions/http-client": "^2.0.1", + "uuid": "^8.3.2" }, "devDependencies": { - "@types/node": "^12.0.2" + "@types/node": "^12.0.2", + "@types/uuid": "^8.3.4" } }, "node_modules/@actions/http-client": { @@ -29,6 +31,12 @@ "integrity": "sha512-5tabW/i+9mhrfEOUcLDu2xBPsHJ+X5Orqy9FKpale3SjDA17j5AEpYq5vfy3oAeAHGcvANRCO3NV3d2D6q3NiA==", "dev": true }, + "node_modules/@types/uuid": { + "version": "8.3.4", + "resolved": "https://registry.npmjs.org/@types/uuid/-/uuid-8.3.4.tgz", + "integrity": "sha512-c/I8ZRb51j+pYGAu5CrFMRxqZ2ke4y2grEBO5AUjgSkSk+qT2Ea+OdWElz/OiMf5MNpn2b17kuVBwZLQJXzihw==", + "dev": true + }, "node_modules/tunnel": { "version": "0.0.6", "resolved": "https://registry.npmjs.org/tunnel/-/tunnel-0.0.6.tgz", @@ -36,6 +44,14 @@ "engines": { "node": ">=0.6.11 <=0.7.0 || >=0.7.3" } + }, + "node_modules/uuid": { + "version": "8.3.2", + "resolved": "https://registry.npmjs.org/uuid/-/uuid-8.3.2.tgz", + "integrity": "sha512-+NYs2QeMWy+GWFOEm9xnn6HCDp0l7QBD7ml8zLUmJ+93Q5NF0NocErnwkTkXVFNiX3/fpC6afS8Dhb/gz7R7eg==", + "bin": { + "uuid": "dist/bin/uuid" + } } }, "dependencies": { @@ -53,10 +69,21 @@ "integrity": "sha512-5tabW/i+9mhrfEOUcLDu2xBPsHJ+X5Orqy9FKpale3SjDA17j5AEpYq5vfy3oAeAHGcvANRCO3NV3d2D6q3NiA==", "dev": true }, + "@types/uuid": { + "version": "8.3.4", + "resolved": "https://registry.npmjs.org/@types/uuid/-/uuid-8.3.4.tgz", + "integrity": "sha512-c/I8ZRb51j+pYGAu5CrFMRxqZ2ke4y2grEBO5AUjgSkSk+qT2Ea+OdWElz/OiMf5MNpn2b17kuVBwZLQJXzihw==", + "dev": true + }, "tunnel": { "version": "0.0.6", "resolved": "https://registry.npmjs.org/tunnel/-/tunnel-0.0.6.tgz", "integrity": "sha512-1h/Lnq9yajKY2PEbBadPXj3VxsDDu844OnaAo52UVmIzIvwwtBPIuNvkjuzBlTWpfJyUbG3ez0KSBibQkj4ojg==" + }, + "uuid": { + "version": "8.3.2", + "resolved": "https://registry.npmjs.org/uuid/-/uuid-8.3.2.tgz", + "integrity": "sha512-+NYs2QeMWy+GWFOEm9xnn6HCDp0l7QBD7ml8zLUmJ+93Q5NF0NocErnwkTkXVFNiX3/fpC6afS8Dhb/gz7R7eg==" } } } diff --git a/packages/core/package.json b/packages/core/package.json index 4816c6aa..45341c3a 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -1,6 +1,6 @@ { "name": "@actions/core", - "version": "1.9.0", + "version": "1.9.1", "description": "Actions core lib", "keywords": [ "github", @@ -36,9 +36,11 @@ "url": "https://github.com/actions/toolkit/issues" }, "dependencies": { - "@actions/http-client": "^2.0.1" + "@actions/http-client": "^2.0.1", + "uuid": "^8.3.2" }, "devDependencies": { - "@types/node": "^12.0.2" + "@types/node": "^12.0.2", + "@types/uuid": "^8.3.4" } } diff --git a/packages/core/src/core.ts b/packages/core/src/core.ts index e4f8bf95..85bdcb56 100644 --- a/packages/core/src/core.ts +++ b/packages/core/src/core.ts @@ -4,6 +4,7 @@ import {toCommandProperties, toCommandValue} from './utils' import * as os from 'os' import * as path from 'path' +import { v4 as uuidv4 } from 'uuid' import {OidcClient} from './oidc-utils' @@ -86,7 +87,17 @@ export function exportVariable(name: string, val: any): void { const filePath = process.env['GITHUB_ENV'] || '' if (filePath) { - const delimiter = '_GitHubActionsFileCommandDelimeter_' + const delimiter = `ghadelimiter_${uuidv4()}` + + // These should realistically never happen, but just in case someone finds a way to exploit uuid generation let's not allow keys or values that contain the delimiter. + if (name.includes(delimiter)) { + throw new Error(`Unexpected input: name should not contain the delimiter "${delimiter}"`) + } + + if (convertedVal.includes(delimiter)) { + throw new Error(`Unexpected input: value should not contain the delimiter "${delimiter}"`) + } + const commandValue = `${name}<<${delimiter}${os.EOL}${convertedVal}${os.EOL}${delimiter}` issueFileCommand('ENV', commandValue) } else {