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

fix command escaping #302

Merged
merged 1 commit into from
Jan 19, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
45 changes: 45 additions & 0 deletions packages/core/__tests__/command.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,51 @@ describe('@actions/core/src/command', () => {
assertWriteCalls([`::some-command::${os.EOL}`])
})

it('command escapes message', () => {
// Verify replaces each instance, not just first instance
command.issueCommand(
'some-command',
{},
'percent % percent % cr \r cr \r lf \n lf \n'
)
assertWriteCalls([
`::some-command::percent %25 percent %25 cr %0D cr %0D lf %0A lf %0A${os.EOL}`
])

// Verify literal escape sequences
process.stdout.write = jest.fn()
command.issueCommand('some-command', {}, '%25 %25 %0D %0D %0A %0A')
assertWriteCalls([
`::some-command::%2525 %2525 %250D %250D %250A %250A${os.EOL}`
])
})

it('command escapes property', () => {
// Verify replaces each instance, not just first instance
command.issueCommand(
'some-command',
{
name:
'percent % percent % cr \r cr \r lf \n lf \n colon : colon : comma , comma ,'
},
''
)
assertWriteCalls([
`::some-command name=percent %25 percent %25 cr %0D cr %0D lf %0A lf %0A colon %3A colon %3A comma %2C comma %2C::${os.EOL}`
])

// Verify literal escape sequences
process.stdout.write = jest.fn()
command.issueCommand(
'some-command',
{},
'%25 %25 %0D %0D %0A %0A %3A %3A %2C %2C'
)
assertWriteCalls([
`::some-command::%2525 %2525 %250D %250D %250A %250A %253A %253A %252C %252C${os.EOL}`
])
})

it('command with message', () => {
command.issueCommand('some-command', {}, 'some message')
assertWriteCalls([`::some-command::some message${os.EOL}`])
Expand Down
6 changes: 3 additions & 3 deletions packages/core/__tests__/core.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ describe('@actions/core', () => {
})

it('exportVariable escapes variable names', () => {
core.exportVariable('special char var \r\n];', 'special val')
expect(process.env['special char var \r\n];']).toBe('special val')
core.exportVariable('special char var \r\n,:', 'special val')
expect(process.env['special char var \r\n,:']).toBe('special val')
assertWriteCalls([
`::set-env name=special char var %0D%0A%5D%3B::special val${os.EOL}`
`::set-env name=special char var %0D%0A%2C%3A::special val${os.EOL}`
])
})

Expand Down
32 changes: 14 additions & 18 deletions packages/core/src/command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ interface CommandProperties {
* Commands
*
* Command Format:
* ##[name key=value;key=value]message
* ::name key=value,key=value::message
*
* Examples:
* ##[warning]This is the user warning message
* ##[set-secret name=mypassword]definitelyNotAPassword!
* ::warning::This is the message
* ::set-env name=MY_VAR::some value
*/
export function issueCommand(
command: string,
Expand Down Expand Up @@ -62,33 +62,29 @@ class Command {
cmdStr += ','
}

// safely append the val - avoid blowing up when attempting to
// call .replace() if message is not a string for some reason
cmdStr += `${key}=${escape(`${val || ''}`)}`
cmdStr += `${key}=${escapeProperty(val)}`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was reading it and thinking that eventually, the function being exported, it is diable for users to run issueCommand('some-command', {'key,1': 'value'}, 'message') that would break the ::name key1=value1,key2=value2::message formatting. would it be something worth taking into account?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a limitation of the command format - defined by the runner - that commas are not allowed within a key name.

Today this limitation does not present a problem. All commands (and keys) are first party - not extensible.

We can relax this constraint in the future needed. But not required for any current scenarios.

}
}
}
}

cmdStr += CMD_STRING

// safely append the message - avoid blowing up when attempting to
// call .replace() if message is not a string for some reason
const message = `${this.message || ''}`
cmdStr += escapeData(message)

cmdStr += `${CMD_STRING}${escapeData(this.message)}`
return cmdStr
}
}

function escapeData(s: string): string {
return s.replace(/\r/g, '%0D').replace(/\n/g, '%0A')
return (s || '')
.replace(/%/g, '%25')
.replace(/\r/g, '%0D')
.replace(/\n/g, '%0A')
}

function escape(s: string): string {
return s
function escapeProperty(s: string): string {
return (s || '')
.replace(/%/g, '%25')
.replace(/\r/g, '%0D')
.replace(/\n/g, '%0A')
.replace(/]/g, '%5D')
.replace(/;/g, '%3B')
.replace(/:/g, '%3A')
.replace(/,/g, '%2C')
}