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

Matrix webhook doesn't send access token in Authorization header on replay #19872

Closed
jellykells opened this issue Jun 2, 2022 · 2 comments · Fixed by #20926
Closed

Matrix webhook doesn't send access token in Authorization header on replay #19872

jellykells opened this issue Jun 2, 2022 · 2 comments · Fixed by #20926
Labels

Comments

@jellykells
Copy link

Description

If a Matrix webhook fails and it is replayed, the access token is not sent in the Authorization header. The resulting header sent is Authorization: Bearer

Gitea Version

1.16.8

Can you reproduce the bug on the Gitea demo site?

No

Log Gist

https://gist.github.com/jellykells/68fdaba33b2a68c2a3edec59a34b5b40

Screenshots

No response

Git Version

2.17.1

Operating System

Linux 4.15.0

How are you running Gitea?

Binary downloaded from GitHub and run with systemd

Database

MySQL

@oliverpool
Copy link
Contributor

From a quick analysis, here is what I think happens:

There is a PR to better manage such Authorization header, which should fix this: #20267 (the PR is initially aimed only for gitea webhooks, but I hope that it will be reconsidered :)

lafriks pushed a commit that referenced this issue Nov 3, 2022
_This is a different approach to #20267, I took the liberty of adapting
some parts, see below_

## Context

In some cases, a weebhook endpoint requires some kind of authentication.
The usual way is by sending a static `Authorization` header, with a
given token. For instance:

- Matrix expects a `Bearer <token>` (already implemented, by storing the
header cleartext in the metadata - which is buggy on retry #19872)
- TeamCity #18667
- Gitea instances #20267
- SourceHut https://man.sr.ht/graphql.md#authentication-strategies (this
is my actual personal need :)

## Proposed solution

Add a dedicated encrypt column to the webhook table (instead of storing
it as meta as proposed in #20267), so that it gets available for all
present and future hook types (especially the custom ones #19307).

This would also solve the buggy matrix retry #19872.

As a first step, I would recommend focusing on the backend logic and
improve the frontend at a later stage. For now the UI is a simple
`Authorization` field (which could be later customized with `Bearer` and
`Basic` switches):


![2022-08-23-142911](https://user-images.githubusercontent.com/3864879/186162483-5b721504-eef5-4932-812e-eb96a68494cc.png)

The header name is hard-coded, since I couldn't fine any usecase
justifying otherwise.

## Questions

- What do you think of this approach? @justusbunsi @Gusted @silverwind 
- ~~How are the migrations generated? Do I have to manually create a new
file, or is there a command for that?~~
- ~~I started adding it to the API: should I complete it or should I
drop it? (I don't know how much the API is actually used)~~

## Done as well:

- add a migration for the existing matrix webhooks and remove the
`Authorization` logic there


_Closes #19872_

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: Gusted <williamzijl7@hotmail.com>
Co-authored-by: delvh <dev.lh@web.de>
@oliverpool
Copy link
Contributor

FYI, the PR should be included in the 1.19 release (in a couple of months).

@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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants