-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix command escaping #302
Conversation
also related to actions/runner#267 |
There is a fix in actions toolkit: actions/toolkit#302, apply it here as well
// 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)}` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
backport of actions/toolkit#302
fixes #301