The Wayback Machine - https://web.archive.org/web/20220108201932/https://github.com/go-gitea/gitea/pull/17482
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

Jump to bottom

Only allow webhook to send requests to allowed hosts #17482

Merged
merged 12 commits into from Nov 1, 2021
Merged

Only allow webhook to send requests to allowed hosts #17482

merged 12 commits into from Nov 1, 2021

Conversation

@wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Oct 29, 2021 •
edited

For security reasons, the webhook should only send requests to allowed hosts.

This PR introduces a setting option:

  • ALLOWED_HOST_LIST: external: Webhook can only call allowed hosts for security reasons. Comma separated list: loopback, private, external, or *, or CIDR list (1.2.3.0/8), or wildcard hosts (*.mydomain.com)

ps: I added a log message log.Info("AppURL(ROOT_URL): %s", setting.AppURL) during starting. It is very trivial (but helpful) so I think it's fine to put it here. There are more comments in code.
@wxiaoguang wxiaoguang force-pushed the fix-webhook-request branch 5 times, most recently from e4f9871 to d6358bc Oct 29, 2021
@wxiaoguang wxiaoguang force-pushed the fix-webhook-request branch from d6358bc to e9618db Oct 29, 2021
delvh
delvh reviewed Oct 29, 2021
View changes
custom/conf/app.example.ini Outdated Show resolved Hide resolved
cmd/web.go Outdated Show resolved Hide resolved
docs/content/doc/advanced/config-cheat-sheet.en-us.md Outdated Show resolved Hide resolved
modules/setting/webhook.go Outdated Show resolved Hide resolved
services/webhook/deliver.go Outdated Show resolved Hide resolved
lafriks
lafriks reviewed Oct 29, 2021
View changes
services/webhook/deliver_test.go Outdated Show resolved Hide resolved
@wxiaoguang
Copy link
Contributor Author

@wxiaoguang wxiaoguang commented Oct 30, 2021

Re-worked the documents, unit tests and comments.

@wxiaoguang wxiaoguang force-pushed the fix-webhook-request branch from 150f4a4 to f4b13c2 Oct 30, 2021
@wxiaoguang wxiaoguang added this to the 1.16.0 milestone Oct 30, 2021
@codecov-commenter
Copy link

@codecov-commenter codecov-commenter commented Oct 30, 2021 •
edited

Codecov Report

Merging #17482 (e134bf2) into main (4e8a817) will increase coverage by 0.01%.
The diff coverage is 69.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #17482      +/-   ##
==========================================
+ Coverage   45.49%   45.51%   +0.01%     
==========================================
  Files         791      793       +2     
  Lines       88717    88772      +55     
==========================================
+ Hits        40364    40404      +40     
- Misses      41842    41855      +13     
- Partials     6511     6513       +2
Impacted Files Coverage Δ
cmd/web.go 0.00% <0.00%> (ø)
services/webhook/deliver.go 45.98% <14.28%> (-2.04%) ⬇️
modules/util/net.go 83.33% <83.33%> (ø)
modules/hostmatcher/hostmatcher.go 86.04% <86.04%> (ø)
modules/migrations/migrate.go 26.19% <100.00%> (-1.60%) ⬇️
modules/setting/webhook.go 62.50% <100.00%> (+2.50%) ⬆️
modules/log/event.go 60.64% <0.00%> (-1.86%) ⬇️
modules/queue/workerpool.go 48.09% <0.00%> (-1.15%) ⬇️
modules/queue/queue_bytefifo.go 59.28% <0.00%> (-0.60%) ⬇️
services/pull/pull.go 41.78% <0.00%> (-0.41%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e8a817...e134bf2. Read the comment docs.

@wxiaoguang wxiaoguang requested review from lafriks and zeripath Oct 30, 2021
delvh
delvh approved these changes Oct 30, 2021
View changes
lunny
lunny reviewed Oct 31, 2021
View changes
modules/util/net.go Show resolved Hide resolved
@wxiaoguang wxiaoguang requested a review from lunny Oct 31, 2021
@wxiaoguang wxiaoguang merged commit 599ff1c into go-gitea:main Nov 1, 2021
2 checks passed
@techknowlogick
Copy link
Member

@techknowlogick techknowlogick commented Nov 1, 2021

@wxiaoguang please send backport :)

wxiaoguang added a commit to wxiaoguang/gitea that referenced this issue Nov 1, 2021
@wxiaoguang
Only allow webhook to send requests to allowed hosts (backport go-git…
1867d8a 
…ea#17482)
@wxiaoguang wxiaoguang mentioned this pull request Nov 1, 2021
Merged
@wxiaoguang wxiaoguang deleted the fix-webhook-request branch Nov 1, 2021
@wxiaoguang wxiaoguang mentioned this pull request Nov 3, 2021
Merged
zeripath pushed a commit that referenced this issue Nov 6, 2021
@wxiaoguang
Only allow webhook to send requests to allowed hosts (#17482) (#17510)
20ae184 
Backport #17482

* Only allow webhook to send requests to allowed hosts (backport #17482)

* use ALLOWED_HOST_LIST=* for default to keep the legacy behavior in 1.15.x
@wxiaoguang wxiaoguang mentioned this pull request Nov 10, 2021
Merged
melegiul added a commit to netzbegruenung/gitea that referenced this issue Nov 11, 2021
@wxiaoguang @melegiul
Only allow webhook to send requests to allowed hosts (go-gitea#17482)
3fbf72f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@6543 6543

@techknowlogick techknowlogick

@delvh delvh

@lafriks lafriks

@zeripath zeripath

@lunny lunny

Assignees
No one assigned
Labels
backport/done backport/v1.15 kind/breaking kind/enhancement kind/security lgtm/done
Milestone
1.16.0
Linked issues

Successfully merging this pull request may close these issues.

None yet
9 participants
@wxiaoguang @codecov-commenter @techknowlogick @lunny @lafriks @6543 @delvh @zeripath @GiteaBot