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

Anonymize secrets data that will end up in report #559

Closed
plocket opened this issue May 10, 2022 · 7 comments
Closed

Anonymize secrets data that will end up in report #559

plocket opened this issue May 10, 2022 · 7 comments
Assignees
Labels
priority A combination of urgency and impact security

Comments

@plocket
Copy link
Collaborator

plocket commented May 10, 2022

Does that mean complete anonymity, or do we include the first three chars of email and the last three chars of password, etc?

@plocket plocket added the priority A combination of urgency and impact label May 10, 2022
@BryceStevenWilley
Copy link
Collaborator

IMO given that reports are public for public github repos, they should be completely anonymous, just for extra safety. I'm also not sure this needs to be required, as most emails that I give to tests are fake emails, such as example@example.com, and don't need that to be censored. I don't think I'd suggest using live emails in an automated test, especially since we can't verify if the emails been sent from our side.

@michaelhofrichter
Copy link
Collaborator

I just noticed this as well in troubleshooting a failed report. Step one of my tests is to log in to the server using two of the github secrets. I just checked the test results and noticed that it listed my username and password in plaintext.

@BryceStevenWilley
Copy link
Collaborator

Realizing that this might be a smaller scope than I thought, I thought we were trying to censor all data entered into email and password boxes, but this seems to be an issue in just one line?

@michaelhofrichter I'd be curious about your thoughts here. IMO it's easier (and safer) to just not print that data at all, instead of censoring it by:

  1. replacing all characters (except the @ in the email maybe) and still leaking the length of the email / password
  2. printing the same length ******** password string, which is misleading if someone really is trying to see if their password is what they expected it to be.

Would you rather have just "Logged in ... successfully", or would you rather have some ********@*** to make it clear it's the email sign in?

@plocket
Copy link
Collaborator Author

plocket commented May 21, 2022

Jus to clarify, this is just for the login step.

@plocket
Copy link
Collaborator Author

plocket commented May 21, 2022

Also, if we did show some of the names, we definitely wouldn't use the length of the actual secrets. We'd use three stars no matter what.

It occurs to me that we also want to avoid taking a screenshot of the failed page since one of the inputs might be right. That means there's no way for the developer to confirm what they put in.

Maybe that's just the way it has to be. They can always re-enter the secrets to make sure they've put the right ones in.

[We might want to think about how to handle screenshots of errors for setting other secrets. If a secret is set in an input field and the page errors, there will be a screenshot of it. Maybe we can disable screenshots until the page navigates away where that's handled.]

@plocket plocket self-assigned this May 22, 2022
plocket added a commit that referenced this issue May 22, 2022
@michaelhofrichter
Copy link
Collaborator

@BryceStevenWilley Sorry for the delay in my responses. I certainly don't need a length or anything to show in the report. If the password's wrong, I'm going to have to update the secret. I certainly can't edit it.

plocket added a commit that referenced this issue May 23, 2022
* Fix #559, improve security for secrets

* Clarify comment about hiding secret values

Co-authored-by: Bryce Willey <Bryce.Steven.Willey@gmail.com>

Co-authored-by: Bryce Willey <Bryce.Steven.Willey@gmail.com>
@plocket
Copy link
Collaborator Author

plocket commented May 23, 2022

Closed by #562

@plocket plocket closed this as completed May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority A combination of urgency and impact security
Projects
None yet
Development

No branches or pull requests

3 participants