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

Secret should be removed from webhook body by default #8436

Closed
guillep2k opened this issue Oct 9, 2019 · 0 comments
Closed

Secret should be removed from webhook body by default #8436

guillep2k opened this issue Oct 9, 2019 · 0 comments
Labels
pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! type/enhancement An improvement of existing functionality

Comments

@guillep2k
Copy link
Member

Gitea version: 1.10.0-dev 3810fa4

Since header signatures were introduced in #6428, I think the default behavior should be to remove the secret from the body of the webhook request, as it is exposed in plain text and defeats the purpose of the signature. It's usually not a problem when the connection is https, but many struggle to configure that kind of setup, especially on internal servers that can't get a Letsencrypt certificate.

p.SetSecret(w.Secret)

I know this is a breaking change, so I propose an app.ini setting and default it to not include the secret in the body. The admin can change it if they need the old behavior. An ini setting will not be enough if the users need by-repo configuration, though.

@lafriks lafriks added type/enhancement An improvement of existing functionality pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! labels Oct 9, 2019
@lafriks lafriks added this to the 1.11.0 milestone Oct 9, 2019
@techknowlogick techknowlogick modified the milestones: 1.11.0, 1.x.x Dec 30, 2019
@lunny lunny removed this from the 1.x.x milestone Apr 13, 2022
@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
pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! type/enhancement An improvement of existing functionality
Projects
None yet
Development

No branches or pull requests

4 participants