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

Add support for X-Hub-Signature headers in webhooks #8473

Closed
wants to merge 7 commits into from

Conversation

rafelbev
Copy link

Send a new header X-Hub-Signature using the sha1 format

Renames existing Signature to Signature256 since we need to generate to HMACs from the same secret

Fix #7788

@@ -572,7 +574,8 @@ type HookTask struct {
UUID string
Type HookTaskType
URL string `xorm:"TEXT"`
Signature string `xorm:"TEXT"`
Copy link
Member

Choose a reason for hiding this comment

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

We need a database migration

This comment was marked as outdated.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 12, 2019
var err error

switch x.Dialect().DriverName() {
case "mysql":
Copy link
Member

Choose a reason for hiding this comment

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

Two things. 1. Rename columns is tricky (for example SQLite also needs to be added to this migration) so perhaps instead of a rename we keep signature column with same content we just have the strict field be renamed, and then add the new sha column. 2. Could you add the adding of the new column to this migration, while it is added by the default migration,sometimes we’ve been bitten by changing a column that doesn’t yet exist so usually we request explicit add of new columns.

Apologies if I am unclear, I am on mobile and can give better details when I get to a computer. Please ask any questions.

Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

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

imho this would be much better

@@ -572,7 +574,8 @@ type HookTask struct {
UUID string
Type HookTaskType
URL string `xorm:"TEXT"`
Signature string `xorm:"TEXT"`
SignatureSha1 string `xorm:"TEXT"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SignatureSha1 string `xorm:"TEXT"`
Signature string `xorm:"TEXT"`

@@ -572,7 +574,8 @@ type HookTask struct {
UUID string
Type HookTaskType
URL string `xorm:"TEXT"`
Signature string `xorm:"TEXT"`
SignatureSha1 string `xorm:"TEXT"`
SignatureSha256 string `xorm:"TEXT"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SignatureSha256 string `xorm:"TEXT"`
SignatureSha1 string `xorm:"TEXT"`

Copy link
Member

Choose a reason for hiding this comment

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

There still needs to be fields:

Signature       string `xorm:"TEXT"`
SignatureSha1   string `xorm:"TEXT"`

@lafriks lafriks added the type/enhancement An improvement of existing functionality label Oct 12, 2019
@lafriks lafriks added this to the 1.11.0 milestone Oct 12, 2019
rafelbev and others added 2 commits October 12, 2019 12:55
Co-Authored-By: Lauris BH <lauris@nix.lv>
Co-Authored-By: Lauris BH <lauris@nix.lv>
@lunny
Copy link
Member

lunny commented Oct 14, 2019

I would like @lafriks 's change. So that your migrations will be simpler to only use Sync2.

@lunny
Copy link
Member

lunny commented Nov 16, 2019

Please resolve the conflicts.

@techknowlogick techknowlogick modified the milestones: 1.11.0, 1.x.x Dec 12, 2019
@stale
Copy link

stale bot commented Feb 10, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale
Copy link

stale bot commented Apr 10, 2020

This pull request has been automatically closed because of inactivity. You can re-open it if needed.

@stale stale bot closed this Apr 10, 2020
@lunny lunny removed this from the 1.x.x milestone Apr 19, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/stale lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add X-Hub-Signature header to webhook deliveries
5 participants