From 375dd689358fd97ed40d79c47d6d445a51ac1601 Mon Sep 17 00:00:00 2001 From: Thomas Boop Date: Mon, 6 Apr 2020 15:57:27 -0400 Subject: [PATCH 1/7] Be more lenient in accepting command inputs --- packages/core/__tests__/command.test.ts | 15 ++++++++ packages/core/__tests__/core.test.ts | 47 ++++++++++++++++++++++-- packages/core/src/command.ts | 4 +-- packages/core/src/core.ts | 48 +++++++++++++++++++------ 4 files changed, 99 insertions(+), 15 deletions(-) diff --git a/packages/core/__tests__/command.test.ts b/packages/core/__tests__/command.test.ts index 182e145cb3..6580acb832 100644 --- a/packages/core/__tests__/command.test.ts +++ b/packages/core/__tests__/command.test.ts @@ -112,6 +112,21 @@ describe('@actions/core/src/command', () => { `::some-command prop1=value 1,prop2=value 2,prop3=value 3::${os.EOL}` ]) }) + + it('should not crash on bad user input', () => { + command.issueCommand( + 'some-command', + { + prop1: ({test: 'object'} as unknown) as string, + prop2: (123 as unknown) as string, + prop3: (true as unknown) as string + }, + ({test: 'object'} as unknown) as string + ) + assertWriteCalls([ + `::some-command prop1=[object Object],prop2=123,prop3=true::[object Object]${os.EOL}` + ]) + }) }) // Assert that process.stdout.write calls called only with the given arguments. diff --git a/packages/core/__tests__/core.test.ts b/packages/core/__tests__/core.test.ts index 020f4a51b9..a5048e176b 100644 --- a/packages/core/__tests__/core.test.ts +++ b/packages/core/__tests__/core.test.ts @@ -54,6 +54,16 @@ describe('@actions/core', () => { assertWriteCalls([`::set-env name=my var2::var val%0D%0A${os.EOL}`]) }) + it('exportVariable handles boolean inputs', () => { + core.exportVariable('my var', true) + assertWriteCalls([`::set-env name=my var::true${os.EOL}`]) + }) + + it('exportVariable handles number inputs', () => { + core.exportVariable('my var', 5) + assertWriteCalls([`::set-env name=my var::5${os.EOL}`]) + }) + it('setSecret produces the correct command', () => { core.setSecret('secret val') assertWriteCalls([`::add-mask::secret val${os.EOL}`]) @@ -104,18 +114,35 @@ describe('@actions/core', () => { assertWriteCalls([`::set-output name=some output::some value${os.EOL}`]) }) - it('setFailure sets the correct exit code and failure message', () => { + it('setOutput handles bools', () => { + core.setOutput('some output', false) + assertWriteCalls([`::set-output name=some output::false${os.EOL}`]) + }) + + it('setOutput handles numbers', () => { + core.setOutput('some output', 1.01) + assertWriteCalls([`::set-output name=some output::1.01${os.EOL}`]) + }) + + it('setFailed sets the correct exit code and failure message', () => { core.setFailed('Failure message') expect(process.exitCode).toBe(core.ExitCode.Failure) assertWriteCalls([`::error::Failure message${os.EOL}`]) }) - it('setFailure escapes the failure message', () => { + it('setFailed escapes the failure message', () => { core.setFailed('Failure \r\n\nmessage\r') expect(process.exitCode).toBe(core.ExitCode.Failure) assertWriteCalls([`::error::Failure %0D%0A%0Amessage%0D${os.EOL}`]) }) + it('setFailed handles Error', () => { + const message = 'this is my error message' + core.setFailed(new Error(message)) + expect(process.exitCode).toBe(core.ExitCode.Failure) + assertWriteCalls([`::error::Error: ${message}${os.EOL}`]) + }) + it('error sets the correct error message', () => { core.error('Error message') assertWriteCalls([`::error::Error message${os.EOL}`]) @@ -126,6 +153,12 @@ describe('@actions/core', () => { assertWriteCalls([`::error::Error message%0D%0A%0A${os.EOL}`]) }) + it('error handles an error object', () => { + const message = 'this is my error message' + core.error(new Error(message)) + assertWriteCalls([`::error::Error: ${message}${os.EOL}`]) + }) + it('warning sets the correct message', () => { core.warning('Warning') assertWriteCalls([`::warning::Warning${os.EOL}`]) @@ -174,6 +207,16 @@ describe('@actions/core', () => { assertWriteCalls([`::save-state name=state_1::some value${os.EOL}`]) }) + it('saveState handles numbers', () => { + core.saveState('state_1', 1) + assertWriteCalls([`::save-state name=state_1::1${os.EOL}`]) + }) + + it('saveState handles bools', () => { + core.saveState('state_1', true) + assertWriteCalls([`::save-state name=state_1::true${os.EOL}`]) + }) + it('getState gets wrapper action state', () => { expect(core.getState('TEST_1')).toBe('state_val') }) diff --git a/packages/core/src/command.ts b/packages/core/src/command.ts index 9bdfd9c41c..1ea142f60f 100644 --- a/packages/core/src/command.ts +++ b/packages/core/src/command.ts @@ -62,13 +62,13 @@ class Command { cmdStr += ',' } - cmdStr += `${key}=${escapeProperty(val)}` + cmdStr += `${key}=${escapeProperty(`${val || ''}`)}` } } } } - cmdStr += `${CMD_STRING}${escapeData(this.message)}` + cmdStr += `${CMD_STRING}${escapeData(`${this.message || ''}`)}` return cmdStr } } diff --git a/packages/core/src/core.ts b/packages/core/src/core.ts index 9072f46a79..dfdb95097b 100644 --- a/packages/core/src/core.ts +++ b/packages/core/src/core.ts @@ -35,9 +35,13 @@ export enum ExitCode { * @param name the name of the variable to set * @param val the value of the variable */ -export function exportVariable(name: string, val: string): void { - process.env[name] = val - issueCommand('set-env', {name}, val) +export function exportVariable( + name: string, + val: string | boolean | number +): void { + const convertedVal = sanitizeInput(val) + process.env[name] = convertedVal + issueCommand('set-env', {name}, convertedVal) } /** @@ -80,8 +84,11 @@ export function getInput(name: string, options?: InputOptions): string { * @param name name of the output to set * @param value value to store */ -export function setOutput(name: string, value: string): void { - issueCommand('set-output', {name}, value) +export function setOutput( + name: string, + value: string | boolean | number +): void { + issueCommand('set-output', {name}, sanitizeInput(value)) } //----------------------------------------------------------------------- @@ -93,9 +100,9 @@ export function setOutput(name: string, value: string): void { * When the action exits it will be with an exit code of 1 * @param message add error issue message */ -export function setFailed(message: string): void { +export function setFailed(message: string | Error): void { process.exitCode = ExitCode.Failure - error(message) + error(sanitizeInput(message)) } //----------------------------------------------------------------------- @@ -121,8 +128,8 @@ export function debug(message: string): void { * Adds an error issue * @param message error issue message */ -export function error(message: string): void { - issue('error', message) +export function error(message: string | Error): void { + issue('error', sanitizeInput(message)) } /** @@ -191,8 +198,11 @@ export async function group(name: string, fn: () => Promise): Promise { * @param name name of the state to store * @param value value to store */ -export function saveState(name: string, value: string): void { - issueCommand('save-state', {name}, value) +export function saveState( + name: string, + value: string | number | boolean +): void { + issueCommand('save-state', {name}, sanitizeInput(value)) } /** @@ -204,3 +214,19 @@ export function saveState(name: string, value: string): void { export function getState(name: string): string { return process.env[`STATE_${name}`] || '' } + +/** + * Sanatizes an input into a string so it can be passed into issueCommand + * @param input Input parameter to sanitize into a string + */ +function sanitizeInput(input: any): string { + if (input == null) { + return '' + } else if (typeof input === 'string' || input instanceof String) { + return input as string + } else if (input instanceof Error) { + return (input as Error).toString() + } else { + return JSON.stringify(input) + } +} From 2e67724e73adf3b6f47ce0f28d863176b181ed55 Mon Sep 17 00:00:00 2001 From: Thomas Boop Date: Mon, 6 Apr 2020 16:43:54 -0400 Subject: [PATCH 2/7] Fix Linting --- packages/core/src/core.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/core/src/core.ts b/packages/core/src/core.ts index dfdb95097b..a73aece929 100644 --- a/packages/core/src/core.ts +++ b/packages/core/src/core.ts @@ -216,16 +216,17 @@ export function getState(name: string): string { } /** - * Sanatizes an input into a string so it can be passed into issueCommand - * @param input Input parameter to sanitize into a string + * Sanatizes an input into a string so it can be passed into issueCommand safely + * @param input input to sanitize into a string */ +// eslint-disable-next-line @typescript-eslint/no-explicit-any function sanitizeInput(input: any): string { if (input == null) { return '' } else if (typeof input === 'string' || input instanceof String) { return input as string } else if (input instanceof Error) { - return (input as Error).toString() + return input.toString() } else { return JSON.stringify(input) } From b86cd20f76e388acdc186ac46ec3268f8d128b3c Mon Sep 17 00:00:00 2001 From: Thomas Boop Date: Mon, 6 Apr 2020 16:45:24 -0400 Subject: [PATCH 3/7] Add comments for safely converting to string --- packages/core/src/command.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/core/src/command.ts b/packages/core/src/command.ts index 1ea142f60f..c2284974b0 100644 --- a/packages/core/src/command.ts +++ b/packages/core/src/command.ts @@ -61,13 +61,15 @@ class Command { } else { cmdStr += ',' } - + // safely escape the property - avoid blowing up when attempting to + // call .replace() if property is not a string for some reason cmdStr += `${key}=${escapeProperty(`${val || ''}`)}` } } } } - + // safely append the message - avoid blowing up when attempting to + // call .replace() if message is not a string for some reason cmdStr += `${CMD_STRING}${escapeData(`${this.message || ''}`)}` return cmdStr } From 0c741045cb3d89b275100b7fb245e1ee0f6c3d1e Mon Sep 17 00:00:00 2001 From: Thomas Boop Date: Wed, 8 Apr 2020 16:50:23 -0400 Subject: [PATCH 4/7] Handle any as a part of issuecommand --- packages/core/__tests__/command.test.ts | 4 +-- packages/core/src/command.ts | 32 ++++++++++++----- packages/core/src/core.ts | 46 +++++++------------------ 3 files changed, 39 insertions(+), 43 deletions(-) diff --git a/packages/core/__tests__/command.test.ts b/packages/core/__tests__/command.test.ts index 6580acb832..42ebd03f38 100644 --- a/packages/core/__tests__/command.test.ts +++ b/packages/core/__tests__/command.test.ts @@ -113,7 +113,7 @@ describe('@actions/core/src/command', () => { ]) }) - it('should not crash on bad user input', () => { + it('should handle issueing commands for non-string objects', () => { command.issueCommand( 'some-command', { @@ -124,7 +124,7 @@ describe('@actions/core/src/command', () => { ({test: 'object'} as unknown) as string ) assertWriteCalls([ - `::some-command prop1=[object Object],prop2=123,prop3=true::[object Object]${os.EOL}` + `::some-command prop1={\"test\"%3A\"object\"},prop2=123,prop3=true::{\"test\":\"object\"}${os.EOL}` ]) }) }) diff --git a/packages/core/src/command.ts b/packages/core/src/command.ts index c2284974b0..321cc9ddd1 100644 --- a/packages/core/src/command.ts +++ b/packages/core/src/command.ts @@ -2,8 +2,11 @@ import * as os from 'os' // For internal use, subject to change. +// We use any as a valid input type +/* eslint-disable @typescript-eslint/no-explicit-any */ + interface CommandProperties { - [key: string]: string + [key: string]: any } /** @@ -19,7 +22,7 @@ interface CommandProperties { export function issueCommand( command: string, properties: CommandProperties, - message: string + message: any ): void { const cmd = new Command(command, properties, message) process.stdout.write(cmd.toString() + os.EOL) @@ -63,27 +66,40 @@ class Command { } // safely escape the property - avoid blowing up when attempting to // call .replace() if property is not a string for some reason - cmdStr += `${key}=${escapeProperty(`${val || ''}`)}` + cmdStr += `${key}=${escapeProperty(val)}` } } } } // safely append the message - avoid blowing up when attempting to // call .replace() if message is not a string for some reason - cmdStr += `${CMD_STRING}${escapeData(`${this.message || ''}`)}` + cmdStr += `${CMD_STRING}${escapeData(this.message)}` return cmdStr } } -function escapeData(s: string): string { - return (s || '') +/** + * Sanatizes an input into a string so it can be passed into issueCommand safely + * @param input input to sanitize into a string + */ +export function toCommandValue(input: any): string { + if (input === null || input === undefined) { + return '' + } else if (typeof input === 'string' || input instanceof String) { + return input as string + } + return JSON.stringify(input) +} + +function escapeData(s: any): string { + return toCommandValue(s) .replace(/%/g, '%25') .replace(/\r/g, '%0D') .replace(/\n/g, '%0A') } -function escapeProperty(s: string): string { - return (s || '') +function escapeProperty(s: any): string { + return toCommandValue(s) .replace(/%/g, '%25') .replace(/\r/g, '%0D') .replace(/\n/g, '%0A') diff --git a/packages/core/src/core.ts b/packages/core/src/core.ts index a73aece929..5aa0e3cfb9 100644 --- a/packages/core/src/core.ts +++ b/packages/core/src/core.ts @@ -1,4 +1,4 @@ -import {issue, issueCommand} from './command' +import {issue, issueCommand, toCommandValue} from './command' import * as os from 'os' import * as path from 'path' @@ -33,13 +33,11 @@ export enum ExitCode { /** * Sets env variable for this action and future actions in the job * @param name the name of the variable to set - * @param val the value of the variable + * @param val the value of the variable. Will be converted to a string via JSON.stringify */ -export function exportVariable( - name: string, - val: string | boolean | number -): void { - const convertedVal = sanitizeInput(val) +// eslint-disable-next-line @typescript-eslint/no-explicit-any +export function exportVariable(name: string, val: any): void { + const convertedVal = toCommandValue(val) process.env[name] = convertedVal issueCommand('set-env', {name}, convertedVal) } @@ -82,13 +80,11 @@ export function getInput(name: string, options?: InputOptions): string { * Sets the value of an output. * * @param name name of the output to set - * @param value value to store + * @param value value to store. Will be converted to a string via JSON.stringify */ -export function setOutput( - name: string, - value: string | boolean | number -): void { - issueCommand('set-output', {name}, sanitizeInput(value)) +// eslint-disable-next-line @typescript-eslint/no-explicit-any +export function setOutput(name: string, value: any): void { + issueCommand('set-output', {name}, value) } //----------------------------------------------------------------------- @@ -102,7 +98,8 @@ export function setOutput( */ export function setFailed(message: string | Error): void { process.exitCode = ExitCode.Failure - error(sanitizeInput(message)) + + error(message) } //----------------------------------------------------------------------- @@ -129,7 +126,7 @@ export function debug(message: string): void { * @param message error issue message */ export function error(message: string | Error): void { - issue('error', sanitizeInput(message)) + issue('error', message instanceof Error ? message.toString() : message) } /** @@ -202,7 +199,7 @@ export function saveState( name: string, value: string | number | boolean ): void { - issueCommand('save-state', {name}, sanitizeInput(value)) + issueCommand('save-state', {name}, value) } /** @@ -214,20 +211,3 @@ export function saveState( export function getState(name: string): string { return process.env[`STATE_${name}`] || '' } - -/** - * Sanatizes an input into a string so it can be passed into issueCommand safely - * @param input input to sanitize into a string - */ -// eslint-disable-next-line @typescript-eslint/no-explicit-any -function sanitizeInput(input: any): string { - if (input == null) { - return '' - } else if (typeof input === 'string' || input instanceof String) { - return input as string - } else if (input instanceof Error) { - return input.toString() - } else { - return JSON.stringify(input) - } -} From 3ab3ad1e8bfa1bb74abeca3a1a99bb630b8e39ca Mon Sep 17 00:00:00 2001 From: Thomas Boop Date: Thu, 9 Apr 2020 10:42:52 -0400 Subject: [PATCH 5/7] Add Errors to warnings as well --- packages/core/__tests__/core.test.ts | 6 ++++++ packages/core/src/core.ts | 16 ++++++++-------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/packages/core/__tests__/core.test.ts b/packages/core/__tests__/core.test.ts index a5048e176b..d10e0590d8 100644 --- a/packages/core/__tests__/core.test.ts +++ b/packages/core/__tests__/core.test.ts @@ -169,6 +169,12 @@ describe('@actions/core', () => { assertWriteCalls([`::warning::%0D%0Awarning%0A${os.EOL}`]) }) + it('warning handles an error object', () => { + const message = 'this is my error message' + core.warning(new Error(message)) + assertWriteCalls([`::warning::Error: ${message}${os.EOL}`]) + }) + it('startGroup starts a new group', () => { core.startGroup('my-group') assertWriteCalls([`::group::my-group${os.EOL}`]) diff --git a/packages/core/src/core.ts b/packages/core/src/core.ts index 5aa0e3cfb9..2e2dba4ef6 100644 --- a/packages/core/src/core.ts +++ b/packages/core/src/core.ts @@ -33,7 +33,7 @@ export enum ExitCode { /** * Sets env variable for this action and future actions in the job * @param name the name of the variable to set - * @param val the value of the variable. Will be converted to a string via JSON.stringify + * @param val the value of the variable. Non-string values will be converted to a string via JSON.stringify */ // eslint-disable-next-line @typescript-eslint/no-explicit-any export function exportVariable(name: string, val: any): void { @@ -80,7 +80,7 @@ export function getInput(name: string, options?: InputOptions): string { * Sets the value of an output. * * @param name name of the output to set - * @param value value to store. Will be converted to a string via JSON.stringify + * @param value value to store. Non-string values will be converted to a string via JSON.stringify */ // eslint-disable-next-line @typescript-eslint/no-explicit-any export function setOutput(name: string, value: any): void { @@ -123,7 +123,7 @@ export function debug(message: string): void { /** * Adds an error issue - * @param message error issue message + * @param message error issue message. Errors will be converted to string via toString() */ export function error(message: string | Error): void { issue('error', message instanceof Error ? message.toString() : message) @@ -131,10 +131,10 @@ export function error(message: string | Error): void { /** * Adds an warning issue - * @param message warning issue message + * @param message warning issue message. Errors will be converted to string via toString() */ -export function warning(message: string): void { - issue('warning', message) +export function warning(message: string | Error): void { + issue('warning', message instanceof Error ? message.toString() : message) } /** @@ -193,11 +193,11 @@ export async function group(name: string, fn: () => Promise): Promise { * Saves state for current action, the state can only be retrieved by this action's post job execution. * * @param name name of the state to store - * @param value value to store + * @param value value to store. Non-string values will be converted to a string via JSON.stringify */ export function saveState( name: string, - value: string | number | boolean + value: any ): void { issueCommand('save-state', {name}, value) } From 8b12f528eff9634bd6ed3180f0032b8819ffdcf2 Mon Sep 17 00:00:00 2001 From: Thomas Boop Date: Thu, 9 Apr 2020 12:54:14 -0400 Subject: [PATCH 6/7] Fix lint/style issues --- packages/core/__tests__/command.test.ts | 2 +- packages/core/src/core.ts | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/core/__tests__/command.test.ts b/packages/core/__tests__/command.test.ts index 42ebd03f38..da3df23a4e 100644 --- a/packages/core/__tests__/command.test.ts +++ b/packages/core/__tests__/command.test.ts @@ -124,7 +124,7 @@ describe('@actions/core/src/command', () => { ({test: 'object'} as unknown) as string ) assertWriteCalls([ - `::some-command prop1={\"test\"%3A\"object\"},prop2=123,prop3=true::{\"test\":\"object\"}${os.EOL}` + `::some-command prop1={"test"%3A"object"},prop2=123,prop3=true::{"test":"object"}${os.EOL}` ]) }) }) diff --git a/packages/core/src/core.ts b/packages/core/src/core.ts index 2e2dba4ef6..85fc547a96 100644 --- a/packages/core/src/core.ts +++ b/packages/core/src/core.ts @@ -195,10 +195,8 @@ export async function group(name: string, fn: () => Promise): Promise { * @param name name of the state to store * @param value value to store. Non-string values will be converted to a string via JSON.stringify */ -export function saveState( - name: string, - value: any -): void { +// eslint-disable-next-line @typescript-eslint/no-explicit-any +export function saveState(name: string, value: any): void { issueCommand('save-state', {name}, value) } From ba12683b69b0f7a36654add66f3b3de467d35f94 Mon Sep 17 00:00:00 2001 From: Thomas Boop Date: Thu, 9 Apr 2020 16:56:44 -0400 Subject: [PATCH 7/7] Remove unneeded comments --- packages/core/src/command.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/core/src/command.ts b/packages/core/src/command.ts index 321cc9ddd1..57067a1a52 100644 --- a/packages/core/src/command.ts +++ b/packages/core/src/command.ts @@ -64,15 +64,13 @@ class Command { } else { cmdStr += ',' } - // safely escape the property - avoid blowing up when attempting to - // call .replace() if property is not a string for some reason + cmdStr += `${key}=${escapeProperty(val)}` } } } } - // safely append the message - avoid blowing up when attempting to - // call .replace() if message is not a string for some reason + cmdStr += `${CMD_STRING}${escapeData(this.message)}` return cmdStr }