Skip to content

Mask access token in migration forms #22232

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

Closed
wants to merge 1 commit into from

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Dec 23, 2022

Mask the field without the downsides of type=password. It might be argued that there should be a "reveal" button, but this is too tricky to implement and I see that as the job of the browser, and I guess some mobile ones will provide such a button. Also, disable autocomplete so these tokens do not end up in form history.

Fixes: #22174
Fixes: #22175

Firefox:
Screenshot 2022-12-23 at 23 14 22

Chrome:
Screenshot 2022-12-23 at 23 14 37

@silverwind silverwind added type/enhancement An improvement of existing functionality topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! labels Dec 23, 2022
@silverwind silverwind added this to the 1.19.0 milestone Dec 23, 2022
@silverwind
Copy link
Member Author

Questions:

  1. Should the webhook auth header field be included? I'm leaning towards yes but the content there is not only a token. Users might have a hard time inputting the exact header without seeing the typed characters
  2. Any other token fields I've missed?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 23, 2022
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 23, 2022
@silverwind silverwind marked this pull request as draft December 26, 2022 19:01
@lunny
Copy link
Member

lunny commented Feb 4, 2023

Is this still a draft?

@wxiaoguang
Copy link
Contributor

I know some people like to Copy&Paste non-ASCII chars into the password input, to make their password "more secure". Which is pretty valid.

Then for fun, you will see this with this patch -- which is not a general approach.

image

@yardenshoham yardenshoham modified the milestones: 1.19.0, 1.20.0 Feb 22, 2023
@yardenshoham yardenshoham added the outdated/backport/v1.19 This PR should be backported to Gitea 1.19 label Feb 22, 2023
@silverwind
Copy link
Member Author

Forgot this PR. Will check out the emoji issue.

@silverwind
Copy link
Member Author

silverwind commented Mar 17, 2023

Actually, let's close this in favor of #22175, the firefox workaround is too much of a hack.

Proper solution should be type=password, we just need to figure out how to suppress the browser from offering a "Save Password" prompt. Some other forms like webhook already use type=password and there is no prompt, so it must be possible somehow.

@silverwind silverwind closed this Mar 17, 2023
@silverwind silverwind removed this from the 1.20.0 milestone Mar 17, 2023
@silverwind silverwind removed the outdated/backport/v1.19 This PR should be backported to Gitea 1.19 label Mar 17, 2023
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrating: Auth token is not properly protected in frontend
6 participants