Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Be more lenient in accepting command inputs #405

Merged
merged 7 commits into from
Apr 9, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/core/__tests__/command.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
47 changes: 45 additions & 2 deletions packages/core/__tests__/core.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}`])
Expand Down Expand Up @@ -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}`])
Expand All @@ -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}`])
Expand Down Expand Up @@ -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')
})
Expand Down
10 changes: 6 additions & 4 deletions packages/core/src/command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,16 @@ class Command {
} else {
cmdStr += ','
}

cmdStr += `${key}=${escapeProperty(val)}`
// safely escape the property - avoid blowing up when attempting to
thboop marked this conversation as resolved.
Show resolved Hide resolved
thboop marked this conversation as resolved.
Show resolved Hide resolved
thboop marked this conversation as resolved.
Show resolved Hide resolved
// call .replace() if property is not a string for some reason
cmdStr += `${key}=${escapeProperty(`${val || ''}`)}`
}
}
}
}

cmdStr += `${CMD_STRING}${escapeData(this.message)}`
// 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
}
}
Expand Down
49 changes: 38 additions & 11 deletions packages/core/src/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

/**
Expand Down Expand Up @@ -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))
}

//-----------------------------------------------------------------------
Expand All @@ -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))
}

//-----------------------------------------------------------------------
Expand All @@ -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 {
thboop marked this conversation as resolved.
Show resolved Hide resolved
issue('error', sanitizeInput(message))
}

/**
Expand Down Expand Up @@ -191,8 +198,11 @@ export async function group<T>(name: string, fn: () => Promise<T>): Promise<T> {
* @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
thboop marked this conversation as resolved.
Show resolved Hide resolved
): void {
issueCommand('save-state', {name}, sanitizeInput(value))
}

/**
Expand All @@ -204,3 +214,20 @@ 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 safely
* @param input input to sanitize into a string
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function sanitizeInput(input: any): string {
thboop marked this conversation as resolved.
Show resolved Hide resolved
thboop marked this conversation as resolved.
Show resolved Hide resolved
if (input == null) {
thboop marked this conversation as resolved.
Show resolved Hide resolved
return ''
} else if (typeof input === 'string' || input instanceof String) {
return input as string
} else if (input instanceof Error) {
thboop marked this conversation as resolved.
Show resolved Hide resolved
return input.toString()
} else {
return JSON.stringify(input)
thboop marked this conversation as resolved.
Show resolved Hide resolved
}
}