Skip to content

Fix encoding for Matrix Webhooks#37190

Merged
wxiaoguang merged 12 commits intogo-gitea:mainfrom
bircni:fix-matrix
Apr 13, 2026
Merged

Fix encoding for Matrix Webhooks#37190
wxiaoguang merged 12 commits intogo-gitea:mainfrom
bircni:fix-matrix

Conversation

@bircni
Copy link
Copy Markdown
Member

@bircni bircni commented Apr 12, 2026

url.PathEscape unnecessarily encodes ! to %21, causing Matrix homeservers to reject the request with 401. Replace %21 back to ! after escaping.

Fixes #36012

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 12, 2026
@bircni bircni requested a review from wxiaoguang April 12, 2026 14:32
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Apr 12, 2026 •
edited
Loading

There are different standards: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent#description

Only ! is special here?

The characters on the second line are characters that may be part of the URI syntax, and are only escaped by encodeURIComponent(). Both encodeURI() and encodeURIComponent() do not encode the characters -.!~*'(), known as "unreserved marks", which do not have a reserved purpose but are allowed in a URI "as is". (See RFC2396)

@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Apr 12, 2026

btw: is it possible to find some "matrix official documents" for this encoding?

@bircni
Copy link
Copy Markdown
Member Author

bircni commented Apr 12, 2026

Good point — : also needs to be preserved since Matrix room IDs are of the form !localpart:server and url.PathEscape encodes : as %3A too. Updated to restore both.

For documentation: the Matrix spec defines room IDs at https://spec.matrix.org/latest/appendices/#room-ids. The underlying reason both characters are safe to leave unencoded is RFC 3986 §3.3 — ! is a sub-delimiter and : is explicitly listed as a valid pchar, so neither requires percent-encoding in a path segment.

Attribution: Nicolas (bircni) with claude-sonnet-4-6

@bircni @claude
Also restore %3A to : in Matrix room ID URL path
16801d1 
Matrix room IDs are of the form !localpart:server. Both ! and : are
valid unencoded path segment characters per RFC 3986, but url.PathEscape
encodes both. Restore : alongside ! so the full room ID is preserved.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bircni
Copy link
Copy Markdown
Member Author

bircni commented Apr 12, 2026

Maybe also interesting: https://spec.matrix.org/latest/appendices/#matrix-uri-scheme

@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 Apr 12, 2026
@bircni bircni marked this pull request as ready for review April 12, 2026 14:54
@bircni bircni added the backport/v1.26 This PR should be backported to Gitea 1.26 label Apr 12, 2026
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Apr 12, 2026

Wait, %3A can be kept:

Matrix does a mess for the encoding .......

image

@TheFox0x7
Copy link
Copy Markdown
Contributor

TheFox0x7 commented Apr 12, 2026 •
edited
Loading

Those are matrix.to links. It might not be the same as direct ones. And for the comment maybe stick to !opaque_id:domain naming?

wxiaoguang
wxiaoguang reviewed Apr 12, 2026
View reviewed changes
Comment thread routers/web/repo/setting/webhook.go Outdated
@wxiaoguang
Apply suggestion from @wxiaoguang
13c9bf7 
Signed-off-by: wxiaoguang <wxiaoguang@gmail.com>
@bircni
Copy link
Copy Markdown
Member Author

bircni commented Apr 12, 2026

thanks @wxiaoguang - I was too fast xD

@bircni bircni added this to the 1.26.0 milestone Apr 12, 2026
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Apr 12, 2026

Those are matrix.to links. It might not be the same as direct ones. And for the comment maybe stick to !opaque_id:domain naming?

I am not sure, maybe need to find some real matrix users to try.

@wxiaoguang wxiaoguang modified the milestones: 1.26.0, 1.27.0 Apr 12, 2026
@TheFox0x7
Copy link
Copy Markdown
Contributor

TheFox0x7 commented Apr 12, 2026 •
edited
Loading

Those are matrix.to links. It might not be the same as direct ones. And for the comment maybe stick to !opaque_id:domain naming?

I am not sure, maybe need to find some real matrix users to try.

well it's less broken than before and if it is still broken someone will complain.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 12, 2026
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Apr 12, 2026

Maybe it needs to migrate to the latest API, and the encoding problem might should have been fixed.

https://spec.matrix.org/v1.18/client-server-api/#sending-events-to-a-room

@bircni
Copy link
Copy Markdown
Member Author

bircni commented Apr 12, 2026

Maybe it needs to migrate to the latest API, and the encoding problem might should have been fixed.

https://spec.matrix.org/v1.18/client-server-api/#sending-events-to-a-room

Yes looks like our implementation is kinda old - but this should be solved in another issue and by someone who has matrix knowledge

@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Apr 12, 2026

r0 API is like this, indeed, : should be also be kept .....

image

@bircni
Copy link
Copy Markdown
Member Author

bircni commented Apr 12, 2026

r0 API is like this, indeed, : should be also be kept .....

Thats why I was confused when you said we don't need lol

@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Apr 12, 2026

OK, latest API .....

image

@bircni
Copy link
Copy Markdown
Member Author

bircni commented Apr 12, 2026

this is so confusing the deeper you dive

@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Apr 12, 2026

r0 API is like this, indeed, : should be also be kept .....

Thats why I was confused when you said we don't need lol

Well, just beucase their API design is a mess .... there is no reason to reject encoded characters like ! in real world .....

@bircni bircni added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 12, 2026
@wxiaoguang wxiaoguang enabled auto-merge (squash) April 13, 2026 17:27
@wxiaoguang wxiaoguang merged commit 6eae042 into go-gitea:main Apr 13, 2026
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 13, 2026
@bircni bircni deleted the fix-matrix branch April 13, 2026 18:12
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Apr 13, 2026
@bircni @claude
@wxiaoguang @GiteaBot
Fix encoding for Matrix Webhooks (go-gitea#37190)
d9d85b3 
`url.PathEscape` unnecessarily encodes ! to %21, causing Matrix
homeservers to reject the request with 401. Replace %21 back to ! after
escaping.

Fixes go-gitea#36012

---------

Signed-off-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@GiteaBot GiteaBot mentioned this pull request Apr 13, 2026
Merged
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Apr 13, 2026
silverwind pushed a commit that referenced this pull request Apr 13, 2026
@GiteaBot @bircni
@claude @wxiaoguang
Fix encoding for Matrix Webhooks (#37190) (#37201)
d1be5c3 
Backport #37190 by @bircni

`url.PathEscape` unnecessarily encodes ! to %21, causing Matrix
homeservers to reject the request with 401. Replace %21 back to ! after
escaping.

Fixes #36012

Signed-off-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Nicolas <bircni@icloud.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 14, 2026
@zjjhot
Merge branch 'main' into mainzjj
4f68a03 
* main:
  Add comment for the design of "user activity time" (go-gitea#37195)
  fix(api): handle missing base branch in PR commits API (go-gitea#37193)
  Refactor htmx and fetch-action related code (go-gitea#37186)
  Fix encoding for Matrix Webhooks (go-gitea#37190)
  Always show owner/repo name in compare page dropdowns (go-gitea#37172)
  fix(api): handle fork-only commits in compare API (go-gitea#37185)
  Improve Contributing docs and set a release schedule (go-gitea#37109)
  Update Nix flake (go-gitea#37183)
  Remove outdated RunUser logic (go-gitea#37180)
  Refactor flash message and remove SanitizeHTML template func (go-gitea#37179)
  Indicate form field readonly via background (go-gitea#37175)
  Remove dead CSS rules (go-gitea#37173)
  Fix flaky `TestCatFileBatch/QueryTerminated` test (go-gitea#37159)
  Implement logout redirection for reverse proxy auth setups (go-gitea#36085)
  Add missing `//nolint:depguard` (go-gitea#37162)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@TheFox0x7 TheFox0x7 TheFox0x7 approved these changes

Assignees

No one assigned

Labels

backport/done All backports for this PR have been created backport/v1.26 This PR should be backported to Gitea 1.26 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore.

Projects

None yet

Milestone

1.27.0

Development

Successfully merging this pull request may close these issues.

matrix webhook encodes ! and # rendreing the links unusable by matrix

4 participants

@bircni @wxiaoguang @TheFox0x7 @GiteaBot