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

Adds escape sequences for '\r', '\n' & '%' characters in Action#SetOutput() #5

Merged
merged 2 commits into from Jun 3, 2020

Conversation

ashutoshgngwr
Copy link
Contributor

@@ -114,6 +114,9 @@ func (c *Action) SetEnv(k, v string) {

// SetOutput sets an output parameter.
func (c *Action) SetOutput(k, v string) {
v = strings.ReplaceAll(v, "%", "%25")
Copy link
Owner

Choose a reason for hiding this comment

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

should we just URL-encode the whole thing?

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that there are two more symbols to add but URL encoding the whole thing will escape a lot more characters I think? I suggest copying the JS/TS version and leaving its link for future reference in case its updated and someone faces the problem?

Copy link
Owner

Choose a reason for hiding this comment

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

SGTM - let me know when you want re-review 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just need a minute.

@sethvargo sethvargo merged commit 866a5c7 into sethvargo:master Jun 3, 2020
@sethvargo
Copy link
Owner

Thanks!

@ashutoshgngwr
Copy link
Contributor Author

@sethvargo Sorry for the incovenience but I just put this to test and it feels like GitHub is not unescaping colon : and comma ,.

See this issue - https://github.com/ashutoshgngwr/test/issues/1
Both comments are generated. First one with these two symbols escaped and latter is unescaped.

Please let me know your inputs on this.

@sethvargo
Copy link
Owner

hmmmmm. okay weird. if you submit a pr to remove those, ill merge it 😄

@github-actions
Copy link

This pull request has been automatically locked since there has not
been any recent activity after it was closed. Please open a new
issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SetOutput() will set incorrect value if it contains any of '\n', '\r' or '%'
2 participants