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

Trim leading and trailing whitespace when setting secrets from stdin #5086

Merged
merged 2 commits into from
Jan 29, 2022

Conversation

samcoe
Copy link
Contributor

@samcoe samcoe commented Jan 24, 2022

This PR changes the behavior of the secret set command to trim leading and trailing whitespace from secrets that are input from stdin. The issue this solves only mentions trailing whitespace but I could not think of a reason to not trim leading whitespace as well.

closes #5031

@samcoe samcoe requested a review from a team as a code owner January 24, 2022 09:11
@samcoe samcoe requested review from vilmibm and removed request for a team January 24, 2022 09:11
@samcoe samcoe self-assigned this Jan 24, 2022
@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI Jan 24, 2022
@@ -375,7 +376,7 @@ func getBody(opts *SetOptions) ([]byte, error) {
return nil, fmt.Errorf("failed to read from standard input: %w", err)
}

return body, nil
return bytes.TrimSpace(body), nil
Copy link
Contributor

@mislav mislav Jan 24, 2022

Choose a reason for hiding this comment

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

Since we're trying to mitigate a problem that only happens when using stdin, and is caused by stdin being typically followed by a newline character, could we scope the solution to only those conditions?

  1. Only trim when reading from stdin;
  2. Only trim a single trailing \r?\n and nothing else.

I don't know of realistic scenarios in which preserving all surrounding whitespace will be necessary, but I would like us to avoid doing unnecessary undocumented transformations on the user's input. For example, if someone uses --body "..." to pass a value, I don't see why we should ever change a single byte of that value. With the <<<"..." syntax, however, if the shell magically adds a newline then we can compensate for that by stripping it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup agreed on both points. The location of the current fix is limited to just stdin input so I think that is covered but I will pair it down to just the trailing newline characters.

@samcoe samcoe requested a review from mislav January 24, 2022 20:28
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Fantastic.

The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉 Jan 28, 2022
@samcoe samcoe merged commit 3be4b99 into trunk Jan 29, 2022
The GitHub CLI automation moved this from Needs to be merged 🎉 to Pending Release 🥚 Jan 29, 2022
@samcoe samcoe deleted the trim-whitespace branch January 29, 2022 07:32
@Pencab

This comment was marked as spam.

@github-actions github-actions bot moved this from Pending Release 🥚 to Done 💤 in The GitHub CLI Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
The GitHub CLI
  
Done 💤
Development

Successfully merging this pull request may close these issues.

Secrets don't work when set via stdin
3 participants