Deprecate and remove shared secret from webhook payloads #5496

@stale[bot] commented on GitHub (Aug 9, 2020):

This issue has been automatically marked as stale because it has not had recent activity. I am here to help clear issues left open even if solved or waiting for more insight. This issue will be closed if no further activity occurs during the next 2 weeks. If the issue is still valid just add a comment to keep it alive. Thank you for your contributions.

@stale[bot] commented on GitHub (Aug 9, 2020): This issue has been automatically marked as stale because it has not had recent activity. I am here to help clear issues left open even if solved or waiting for more insight. This issue will be closed if no further activity occurs during the next 2 weeks. If the issue is still valid just add a comment to keep it alive. Thank you for your contributions.

GiteaMirror commented

@JustAnotherArchivist commented on GitHub (Aug 9, 2020):

Yes, this is still valid.

@JustAnotherArchivist commented on GitHub (Aug 9, 2020): Yes, this is still valid.

GiteaMirror commented

@stale[bot] commented on GitHub (Oct 10, 2020):

This issue has been automatically marked as stale because it has not had recent activity. I am here to help clear issues left open even if solved or waiting for more insight. This issue will be closed if no further activity occurs during the next 2 weeks. If the issue is still valid just add a comment to keep it alive. Thank you for your contributions.

@stale[bot] commented on GitHub (Oct 10, 2020): This issue has been automatically marked as stale because it has not had recent activity. I am here to help clear issues left open even if solved or waiting for more insight. This issue will be closed if no further activity occurs during the next 2 weeks. If the issue is still valid just add a comment to keep it alive. Thank you for your contributions.

GiteaMirror commented

@zeripath commented on GitHub (Oct 10, 2020):

AFAICS this is still present but it needs a security tag.

I don't understand completely why this was ever put there, and therefore I don't know why it hasn't been removed - but clearly it needs to be dealt with.

Now this is too late for 1.13 but I think we can just deprecate the secret from 1.13 so that it will be gone from 1.14 and anyone requiring it just has to do something about it.

@zeripath commented on GitHub (Oct 10, 2020): AFAICS this is still present but it needs a security tag. I don't understand completely why this was ever put there, and therefore I don't know why it hasn't been removed - but clearly it needs to be dealt with. Now this is too late for 1.13 but I think we can just deprecate the secret from 1.13 so that it will be gone from 1.14 and anyone requiring it just has to do something about it.

GiteaMirror commented

@rakshith-ravi commented on GitHub (Oct 14, 2020):

@zeripath shouldn't this be moved to 1.14.0 milestone instead of 1.14? (Why do we even have 1.14 in the first place?) Just making sure there's no duplicate milestones created

@rakshith-ravi commented on GitHub (Oct 14, 2020): @zeripath shouldn't this be moved to `1.14.0` milestone instead of `1.14`? (Why do we even have `1.14` in the first place?) Just making sure there's no duplicate milestones created

GiteaMirror commented

@JustAnotherArchivist commented on GitHub (Oct 14, 2020):

Shouldn't this be on the 1.13.0 milestone until the deprecation warnings are in? Just trying to make sure this isn't delayed by another version due to this.

@JustAnotherArchivist commented on GitHub (Oct 14, 2020): Shouldn't this be on the 1.13.0 milestone until the deprecation warnings are in? Just trying to make sure this isn't delayed by another version due to this.

GiteaMirror commented

@techknowlogick commented on GitHub (Oct 14, 2020):

@JustAnotherArchivist we will mention in 1.13.0 blog post

@techknowlogick commented on GitHub (Oct 14, 2020): @JustAnotherArchivist we will mention in 1.13.0 blog post

GiteaMirror commented

@JustAnotherArchivist commented on GitHub (Oct 14, 2020):

I see. I feel like a warning should also be added to the webhook docs and config page.

@JustAnotherArchivist commented on GitHub (Oct 14, 2020): I see. I feel like a warning should also be added to the webhook docs and config page.

GiteaMirror commented

@techknowlogick commented on GitHub (Oct 14, 2020):

That's reasonable. Would you be open to making a PR :)

@techknowlogick commented on GitHub (Oct 14, 2020): That's reasonable. Would you be open to making a PR :)

GiteaMirror commented

@JustAnotherArchivist commented on GitHub (Oct 14, 2020):

Sure. I'll need some guidance on how to integrate it into the web UI though.

Also, I noticed that the secret is also included on Gogs webhooks. It appears like Gogs itself didn't have it in the payload since early 2017 (https://github.com/gogs/gogs/issues/3692), but that should be verified first to avoid breaking existing hooks.

@JustAnotherArchivist commented on GitHub (Oct 14, 2020): Sure. I'll need some guidance on how to integrate it into the web UI though. Also, I noticed that the secret is also included on Gogs webhooks. It appears like Gogs itself didn't have it in the payload since early 2017 (https://github.com/gogs/gogs/issues/3692), but that should be verified first to avoid breaking existing hooks.

GiteaMirror commented

@JustAnotherArchivist commented on GitHub (Oct 27, 2020):

Checklist:

Add deprecation notice to the webhook docs in v1.13.0 – done in #13329/#13330
~~Add deprecation warning to the webhook configuration page in v1.13.0~~ – not necessary, see below
Add deprecation notice in v1.13.0 release notes/announcement – @techknowlogick did this.
Verify that removing the field on Gogs webhooks is safe (see https://github.com/go-gitea/gitea/issues/11755#issuecomment-708702261)
Remove field on master branch before v1.14 (including docs update)
Breaking change notice in v1.14.0 release notes/announcement

@JustAnotherArchivist commented on GitHub (Oct 27, 2020): Checklist: - [x] Add deprecation notice to the webhook docs in v1.13.0 – done in #13329/#13330 - [x] ~~Add deprecation warning to the webhook configuration page in v1.13.0~~ – not necessary, see below - [x] Add deprecation notice in v1.13.0 release notes/announcement – @techknowlogick did this. - [ ] Verify that removing the field on Gogs webhooks is safe (see <https://github.com/go-gitea/gitea/issues/11755#issuecomment-708702261>) - [ ] Remove field on master branch before v1.14 (including docs update) - [ ] Breaking change notice in v1.14.0 release notes/announcement

GiteaMirror commented

@zeripath commented on GitHub (Oct 27, 2020):

Add deprecation warning to the webhook configuration page in v1.13.0 – please advise how such a warning should be integrated into the web UI; I assume there are already elements defined for warnings/errors but currently don't have time to dig into the source code and find this myself.

I don't think there's a sensible place for that - the webhook page simply links to gitea.io. I would guess that means we don't need to do anything about that.

@zeripath commented on GitHub (Oct 27, 2020): > Add deprecation warning to the webhook configuration page in v1.13.0 – please advise how such a warning should be integrated into the web UI; I assume there are already elements defined for warnings/errors but currently don't have time to dig into the source code and find this myself. I don't think there's a sensible place for that - the webhook page simply links to gitea.io. I would guess that means we don't need to do anything about that.

GiteaMirror commented

@JustAnotherArchivist commented on GitHub (Oct 27, 2020):

Thinking about it again, yeah, I guess you're right. The secret itself isn't being deprecated, only its inclusion in the payload, and the web interface makes no attempt to describe what the payload is (except that you can look at the payloads on delivered hooks).

@JustAnotherArchivist commented on GitHub (Oct 27, 2020): Thinking about it again, yeah, I guess you're right. The secret itself isn't being deprecated, only its inclusion in the payload, and the web interface makes no attempt to describe what the payload is (except that you can look at the payloads on delivered hooks).

GiteaMirror commented

427b8a8589/scm/driver/gitea/webhook.go (L62-L72)

@CirnoT commented on GitHub (Oct 28, 2020):

Just FYI this will require appropriate PR for go-scm for Drone

@CirnoT commented on GitHub (Oct 28, 2020): Just FYI this will require appropriate PR for `go-scm` for Drone https://github.com/drone/go-scm/blob/427b8a85897c892148801824760bc66d3a3cdcdb/scm/driver/gitea/webhook.go#L62-L72

GiteaMirror commented

@techknowlogick commented on GitHub (Oct 28, 2020):

@CirnoT 427b8a8589/scm/driver/gitea/webhook.go (L76) should already be suitable, no?

@techknowlogick commented on GitHub (Oct 28, 2020): @CirnoT https://github.com/drone/go-scm/blob/427b8a85897c892148801824760bc66d3a3cdcdb/scm/driver/gitea/webhook.go#L76 should already be suitable, no?

GiteaMirror commented

@CirnoT commented on GitHub (Oct 28, 2020):

Ah sorry, yes. That's && there not ||.

Also it uses FormValue so it looks for ?secret= instead of actually examining Gitea's webhook body, meh... I suppose an ugly workaround for very old Gitea versions?

The comment on line 70 is misleading actually since it mentions "and secret is in payload" but the actual implementation fetches the secret from query parameter, not payload.

@CirnoT commented on GitHub (Oct 28, 2020): Ah sorry, yes. That's `&&` there not `||`. Also it uses `FormValue` so it looks for `?secret=` instead of actually examining Gitea's webhook body, meh... I suppose an ugly workaround for very old Gitea versions? * The comment on line 70 is misleading actually since it mentions "and secret is in payload" but the actual implementation fetches the secret from query parameter, not payload.

GiteaMirror commented

@techknowlogick commented on GitHub (Oct 28, 2020):

@CirnoT no worries, I added in support for the signature checking via header so I was worried I missed something haha.

FormValue is from before my PR so I don't think that needs changing from querystring to webhook body, as that means it has been working that way since the start

@techknowlogick commented on GitHub (Oct 28, 2020): @CirnoT no worries, I added in support for the signature checking via header so I was worried I missed something haha. `FormValue` is from before my PR so I don't think that needs changing from querystring to webhook body, as that means it has been working that way since the start

GiteaMirror commented

@JustAnotherArchivist commented on GitHub (Oct 28, 2020):

I believe secret was never in the query parameters at all. It was originally implemented in Gogs in commit e573855a and was in the payload back then already.

@JustAnotherArchivist commented on GitHub (Oct 28, 2020): I believe `secret` was never in the query parameters at all. It was originally implemented in Gogs in commit e573855a and was in the payload back then already.

GiteaMirror commented

@CirnoT commented on GitHub (Oct 29, 2020):

I believe secret was never in the query parameters at all. It was originally implemented in Gogs in commit e573855 and was in the payload back then already.

It's part of URI that Drone installs as webhook; like I said, most likely workaround from when there was no support for signature in Gitea

@CirnoT commented on GitHub (Oct 29, 2020): > I believe secret was never in the query parameters at all. It was originally implemented in Gogs in commit e573855 and was in the payload back then already. It's part of URI that Drone installs as webhook; like I said, most likely workaround from when there was no support for signature in Gitea

GiteaMirror commented

@JustAnotherArchivist commented on GitHub (Dec 20, 2020):

@techknowlogick It appears this was missed on the release notes and blog post unless I'm blind?

@JustAnotherArchivist commented on GitHub (Dec 20, 2020): @techknowlogick It appears this was missed on the release notes and blog post unless I'm blind?

GiteaMirror commented