fix!: security#38406
Conversation
Co-authored-by: TheFox0x7 <thefox0x7@gmail.com> Co-authored-by: techknowlogick <techknowlogick@gitea.io> Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com> Assisted-by: Claude Code:claude-opus-4-8 Assisted-by: Codex
|
See internal repo for individual changes. Docs are synced |
Signed-off-by: wxiaoguang <wxiaoguang@gmail.com>
There was a problem hiding this comment.
Pull request overview
Security hardening across multiple subsystems (web/API token scope confinement, SSRF defenses, disclosure fixes, and input-bounds DoS mitigations), with broad integration/unit test coverage to prevent regressions.
Changes:
- Add/strengthen SSRF protections (migration fetches, OAuth2 avatar fetch, OpenID discovery, mirror re-validation, outbound proxy path checks).
- Tighten token-scope enforcement (prevent scope escalation on token minting, enforce public-only confinement across more API/web surfaces).
- Fix multiple access-control and disclosure gaps (go-get default branch leak, webhook authorization header leak, label/attachment scoping, watches/stars clearing on visibility changes, Actions org-level listing access filtering, input bounds).
Reviewed changes
Copilot reviewed 84 out of 84 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/repo_home_token_scope_test.go | Adds integration coverage for repo-home scope enforcement under Basic-auth PATs. |
| tests/integration/pull_update_test.go | Verifies public-only tokens can’t update PRs with private head repos. |
| tests/integration/oauth_test.go | Ensures OIDC userinfo enforces read:user scope. |
| tests/integration/oauth_avatar_test.go | Adjusts integration test to allow loopback when testing avatar fetches. |
| tests/integration/mirror_pull_test.go | Adds SSRF re-validation test for pull mirrors. |
| tests/integration/issue_label_scope_test.go | Adds web-side label scoping test to prevent cross-repo label oracle. |
| tests/integration/goget_test.go | Adds tests preventing default-branch disclosure via go-get. |
| tests/integration/feed_user_test.go | Adds tests ensuring public-only tokens don’t surface private activity in feeds/profile activity. |
| tests/integration/auth_oauth2_test.go | Extends OAuth reactivation gating coverage (incl. no-refresh-token sources). |
| tests/integration/api_user_email_test.go | Ensures email management API respects feature restrictions. |
| tests/integration/api_token_test.go | Tests token scope escalation prevention and public-only inheritance. |
| tests/integration/api_repo_hook_test.go | Ensures webhook authorization header is never returned by API. |
| tests/integration/api_public_only_test.go | Adds public-only restriction case for limited-visibility owners. |
| tests/integration/api_packages_npm_test.go | Adds test for bounded npm dist-tag request body size. |
| tests/integration/api_packages_generic_test.go | Adds public-only restriction tests for packages under limited owners. |
| tests/integration/api_issue_tracked_time_test.go | Ensures nonexistent user in tracked-time API returns 404 (no panic/500). |
| tests/integration/api_issue_label_test.go | Tests cross-repo label delete returns consistent “not found”. |
| tests/integration/api_actions_run_test.go | Tests org Actions run/job listing access control + public-only admin confinement. |
| tests/integration/actions_schedule_test.go | Adjusts schedule mirror sync test to allow local networks in test setup. |
| services/webhook/main_test.go | Updates webhook tests to use global security allow-list initialization. |
| services/webhook/general.go | Removes authorization header from webhook API responses. |
| services/webhook/deliver.go | Switches webhook delivery allow-list to [security] ALLOWED_HOST_LIST. |
| services/repository/repository.go | Clears watches when repo becomes private (matching star clearing). |
| services/repository/repository_test.go | Adds test ensuring updateRepository clears watches on private transition. |
| services/org/org.go | Clears stars and watches when org visibility becomes non-public. |
| services/org/org_test.go | Adds coverage for clearing watches/stars on org visibility changes. |
| services/mirror/mirror_pull.go | Re-validates mirror remote URL on every sync against migration allow/block rules. |
| services/migrations/http_client.go | Enforces allow/block list checks on the proxy path (pre-dial) to prevent SSRF via proxy. |
| services/migrations/gitea_uploader.go | Uses migration HTTP client for patch/asset fetches (redirect-safe SSRF confinement). |
| services/migrations/dump.go | Uses migration HTTP client for release asset and PR patch fetches. |
| services/context/permission.go | Expands public-only confinement to deny repos under non-public owners. |
| services/context/api.go | Updates APIContext repo-access check to use the shared public-only deny logic. |
| services/auth/auth_token.go | Revokes auth token on hash mismatch (rotate/compromise handling). |
| services/auth/auth_token_test.go | Updates tests to assert token revocation on hash mismatch. |
| services/actions/task.go | Releases claimed tasks using a fresh bounded context to avoid stranded jobs. |
| services/actions/task_test.go | Adds coverage for the fresh-context task release helper. |
| routers/web/user/setting/applications.go | Enforces public-only inheritance when minting tokens via web settings. |
| routers/web/user/profile.go | Downgrades “show private activity” when authenticated via public-only PAT. |
| routers/web/repo/view_home.go | Requires repo-read scope for web repo home when authenticated via scoped PAT. |
| routers/web/repo/issue_label.go | Scopes label lookup to repo/org to prevent enumeration oracles. |
| routers/web/goget.go | Prevents go-get from leaking default branch to unauthorized/scopeless/public-only callers. |
| routers/web/feed/profile.go | Downgrades feed privacy when authenticated via public-only PAT. |
| routers/web/auth/oauth2_provider.go | Enforces read:user scope for OIDC userinfo responses. |
| routers/web/auth/oauth.go | Adds SSRF-protected avatar fetch client using allow-list + proxy-path validation. |
| routers/web/auth/oauth_test.go | Adds tests validating avatar client loopback/link-local restrictions and allow-list behavior. |
| routers/api/v1/user/email.go | Honors manage-credentials feature restriction in email APIs. |
| routers/api/v1/user/app.go | Prevents token-authenticated token minting from exceeding parent scopes; enforces public-only inheritance. |
| routers/api/v1/shared/action.go | Restricts owner-scoped Actions listing to repos accessible by caller; public-only admin confinement. |
| routers/api/v1/repo/pull.go | Blocks PR update when public-only token can’t access the private head repo. |
| routers/api/v1/repo/issue_tracked_time.go | Returns 404 when specified user doesn’t exist. |
| routers/api/v1/repo/issue_label.go | Scopes label delete to repo/org and unifies not-found responses. |
| routers/api/v1/api.go | Extends public-only package restrictions to limited-visibility owners. |
| routers/api/packages/npm/npm.go | Bounds dist-tag request-body reads and returns 413 when exceeded. |
| routers/api/packages/api.go | Extends public-only restrictions for package owners to include limited visibility. |
| modules/uri/uri.go | Adds OpenWithClient for redirect-safe, caller-controlled HTTP fetching. |
| modules/uri/uri_test.go | Tests that OpenWithClient blocks redirects via validating transports. |
| modules/setting/webhook.go | Bridges deprecated webhook allow-list config into global security allow-list (temporary). |
| modules/setting/security.go | Introduces [security] ALLOWED_HOST_LIST with explicit-set tracking for bridging logic. |
| modules/setting/security_test.go | Adds default-value coverage for new security allow-list setting. |
| modules/private/internal.go | Restricts internal API TLS-skip-verify to loopback/unix and fixes ServerName selection. |
| modules/private/internal_test.go | Adds tests for internal API TLS verification and server-name behavior. |
| modules/packages/debian/metadata.go | Bounds decompressed control tar; avoids O(n²) Description accumulation. |
| modules/packages/debian/metadata_test.go | Adds complexity guard test for Description parsing. |
| modules/packages/arch/metadata.go | Bounds Arch package file-list accumulation to prevent metadata amplification. |
| modules/packages/arch/metadata_test.go | Adds test ensuring oversized file lists are rejected. |
| modules/hostmatcher/http.go | Adds proxy-path target validation to prevent SSRF bypass via configured proxies. |
| modules/hostmatcher/http_test.go | Adds tests for the new proxy-path validation logic. |
| modules/auth/openid/openid.go | Forces OpenID discovery/verification through external-only SSRF-protected client. |
| modules/auth/openid/openid_test.go | Adds test ensuring OpenID discovery won’t reach loopback/internal hosts. |
| models/repo/user_repo.go | Excludes public repos under non-public owners from public star/watch listings. |
| models/repo/user_repo_test.go | Adds coverage for hiding limited-owner repos in public star/watch listings. |
| models/repo/repo_list.go | Adds “public repo under public owner” condition + Actions-accessible repo subquery helpers. |
| models/repo/repo_list_test.go | Adds coverage for Actions-accessible repo filtering with public-only confinement. |
| models/issues/issue_update.go | Blocks cross-repo (or already-linked) attachment relinking by UUID; handles legacy missing RepoID. |
| models/issues/issue_update_test.go | Adds test rejecting cross-repo attachment relinking. |
| models/issues/comment.go | Applies cross-repo/already-linked attachment guards to comment attachments too. |
| models/issues/comment_test.go | Updates test data to satisfy new cross-repo attachment guard. |
| models/auth/access_token_scope.go | Adds EnforcePublicOnlyFrom to propagate public-only restriction to child tokens. |
| models/auth/access_token_scope_test.go | Adds tests for EnforcePublicOnlyFrom. |
| models/asymkey/ssh_key_test.go | Adds oversized SSH public key test; renames test to Go naming convention. |
| models/asymkey/ssh_key_parse.go | Adds input length bound + avoids O(n²) SSH2 accumulation by using a Builder. |
| models/actions/run_list.go | Adds accessible-repo subquery restriction support for runs listing. |
| models/actions/run_job_list.go | Adds accessible-repo subquery restriction support for jobs listing. |
| custom/conf/app.example.ini | Documents new [security] ALLOWED_HOST_LIST and removes deprecated webhook key docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
# Conflicts: # services/webhook/deliver.go
| // checkProxyTarget applies the allow/block lists to a proxied request's target. Unlike the DialContext | ||
| // path, the proxy resolves DNS itself, so the target IP is not known here. We match wildcard patterns and | ||
| // IP literals directly, and for a bare hostname resolve it so IP-based builtins (loopback, private, | ||
| // external) and CIDRs can be evaluated. This is best-effort: the proxy re-resolves the name, so a | ||
| // rebinding TOCTOU remains, but it prevents a proxy from reaching an obviously disallowed target. | ||
| func checkProxyTarget(usage string, allowList, blockList *HostMatchList, hostPort string) error { |
There was a problem hiding this comment.
Do we really need to handle "proxy" SSRF?
Isn't it proxy-server's duty to make sure it only handle valid requests?
There was a problem hiding this comment.
The reason to keep it: ALLOWED_HOST_LIST is a Gitea policy, and without this it's silently bypassed the moment an unrelated proxy is configured — a plain forwarding proxy will fetch whatever target we hand it. This keeps the setting meaning the same with or without a proxy.
It's intentionally best-effort (can't fully stop DNS-rebinding since the proxy re-resolves), but it reliably blocks the cases that matter: IP literals (169.254.169.254, 10/8, ::1), CIDR/wildcard rules, obvious internal names — at the cost of one DNS lookup on non-hot paths.
|
I am done @wxiaoguang |
|
@bircni can you please mention the changed config options and their replacements in the PR description? |
@TheFox0x7 could you describe it? |
Addresses a batch of privately reported security issues, grouped by area: