From ef801bb661338b7dea3ad4c03f51e2a7f38e2610 Mon Sep 17 00:00:00 2001 From: pandareen <7270563+pandareen@users.noreply.github.com> Date: Fri, 15 May 2026 23:52:36 +0530 Subject: [PATCH] fix(auth): set User-Agent on avatar fetch and sync avatar on link-account register (#37564) (#37588) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fixes [go-gitea/gitea#37564](https://github.com/go-gitea/gitea/issues/37564): when an OIDC provider returns a `picture` claim, Gitea is supposed to download that image as the user's avatar (if `[oauth2_client] UPDATE_AVATAR = true`). Two latent bugs prevented this from working consistently: 1. **Default Go User-Agent rejected by some image hosts.** `oauth2UpdateAvatarIfNeed` used `http.Get`, which sends `User-Agent: Go-http-client/1.1`. Hosts like `upload.wikimedia.org` reject that UA with `403`, and every error path silently returned, so the user was left with an identicon and **no log line** to diagnose the issue. 2. **Link-account *register* path skipped avatar sync.** First-time OIDC sign-ins where auto-registration is disabled (or required a username/password retype) go through `LinkAccountPostRegister`, which created the user but never called `oauth2SignInSync`. So the avatar / full name / SSH keys from the IdP were dropped on the floor for those users, even though the existing-account-link path (`oauth2LinkAccount`) and the auto-register path (`handleOAuth2SignIn`) both already did the sync. ## Changes - `routers/web/auth/oauth.go` — `oauth2UpdateAvatarIfNeed` now uses `http.NewRequest` + `http.DefaultClient.Do`, sets `User-Agent: Gitea `, and logs every failure path at `Warn` (invalid URL, fetch error, non-200, body read error, oversize body, upload error). No silent failures. - `routers/web/auth/linkaccount.go` — `LinkAccountPostRegister` now calls `oauth2SignInSync` after a successful user creation, mirroring the auto-register and link-existing-account flows. - `tests/integration/oauth_avatar_test.go` — new `TestOAuth2AvatarFromPicture` integration test with five sub-cases: - `AutoRegister_FetchesAvatarFromPictureWithGiteaUA` — happy path, asserts `use_custom_avatar=true`, an avatar hash is set, exactly one HTTP request was made, and the request carried a `Gitea ` UA. The mock server enforces the UA prefix to mirror real-world hosts that reject Go's default UA. - `AutoRegister_NonOK_DoesNotUpdateAvatar` — server returns 403; user's avatar must remain unset. - `AutoRegister_EmptyPicture_NoFetch` — empty `picture` claim must not trigger any HTTP request. - `AutoRegister_UpdateAvatarFalse_NoFetch` — `UPDATE_AVATAR=false` must not trigger any HTTP request. - `LinkAccountRegister_FetchesAvatarFromPicture` — guards the `linkaccount.go` fix; without the new `oauth2SignInSync` call this assertion fails. ## Related - Upstream issue: go-gitea/gitea#37564 -------------------------------------------- AI Editor was used in this PR --------- Signed-off-by: silverwind Co-authored-by: silverwind Co-authored-by: Claude (Opus 4.7) Co-authored-by: wxiaoguang Co-authored-by: Nicolas --- modules/log/logger_impl.go | 21 ++++++ modules/log/logger_test.go | 7 ++ routers/web/auth/linkaccount.go | 5 ++ routers/web/auth/oauth.go | 52 ++++++++++----- tests/integration/oauth_avatar_test.go | 92 ++++++++++++++++++++++++++ tests/integration/oauth_test.go | 9 +++ 6 files changed, 171 insertions(+), 15 deletions(-) create mode 100644 tests/integration/oauth_avatar_test.go diff --git a/modules/log/logger_impl.go b/modules/log/logger_impl.go index 551c1454aa9..b15dc2f28dd 100644 --- a/modules/log/logger_impl.go +++ b/modules/log/logger_impl.go @@ -5,6 +5,7 @@ package log import ( "context" + "net/url" "reflect" "runtime" "strings" @@ -226,6 +227,8 @@ func (l *LoggerImpl) Log(skip int, event *Event, format string, logArgs ...any) } } else if ls := asLogStringer(v); ls != nil { msgArgs[i] = logStringFormatter{v: ls} + } else if str, ok := v.(string); ok { + msgArgs[i] = protectSensitiveInfo(str) } } @@ -235,6 +238,24 @@ func (l *LoggerImpl) Log(skip int, event *Event, format string, logArgs ...any) l.SendLogEvent(event) } +func protectSensitiveInfo(s string) string { + u, err := url.Parse(s) + if err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" { + return s + } + q := u.Query() + for _, vals := range q { + for i := range vals { + vals[i] = "_" + } + } + masked := &url.URL{Scheme: u.Scheme, Host: u.Host, Path: u.Path, RawQuery: q.Encode()} + if u.User != nil { + masked.User = url.User("_masked_") + } + return masked.String() +} + func (l *LoggerImpl) GetLevel() Level { return Level(l.level.Load()) } diff --git a/modules/log/logger_test.go b/modules/log/logger_test.go index a74139dc51c..6e38610dddc 100644 --- a/modules/log/logger_test.go +++ b/modules/log/logger_test.go @@ -177,3 +177,10 @@ func TestLoggerExpressionFilter(t *testing.T) { assert.Equal(t, []string{"foo\n", "foo bar\n", "by filename\n"}, w1.FetchLogs()) } + +func TestProtectSensitiveInfo(t *testing.T) { + assert.Empty(t, protectSensitiveInfo("")) + assert.Equal(t, "mailto:user@example.com", protectSensitiveInfo("mailto:user@example.com")) + assert.Equal(t, "https://example.com", protectSensitiveInfo("https://example.com")) + assert.Equal(t, "https://_masked_@example.com/path?k=_", protectSensitiveInfo("https://u:p@example.com/path?k=v#hash")) +} diff --git a/routers/web/auth/linkaccount.go b/routers/web/auth/linkaccount.go index ea3aa3f7cae..ad44c5c6713 100644 --- a/routers/web/auth/linkaccount.go +++ b/routers/web/auth/linkaccount.go @@ -253,6 +253,11 @@ func LinkAccountPostRegister(ctx *context.Context) { return } + oauth2SignInSync(ctx, linkAccountData.AuthSourceID, u, linkAccountData.GothUser) + if ctx.Written() { + return + } + authSource, err := auth.GetSourceByID(ctx, linkAccountData.AuthSourceID) if err != nil { ctx.ServerError("GetSourceByID", err) diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index 8645aedbdee..d42ec88ac68 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -13,6 +13,7 @@ import ( "net/url" "sort" "strings" + "time" "code.gitea.io/gitea/models/auth" user_model "code.gitea.io/gitea/models/user" @@ -301,21 +302,42 @@ func showLinkingLogin(ctx *context.Context, authSourceID int64, gothUser goth.Us ctx.Redirect(setting.AppSubURL + "/user/link_account") } -func oauth2UpdateAvatarIfNeed(ctx *context.Context, url string, u *user_model.User) { - if setting.OAuth2Client.UpdateAvatar && len(url) > 0 { - resp, err := http.Get(url) - if err == nil { - defer func() { - _ = resp.Body.Close() - }() - } - // ignore any error - if err == nil && resp.StatusCode == http.StatusOK { - data, err := io.ReadAll(io.LimitReader(resp.Body, setting.Avatar.MaxFileSize+1)) - if err == nil && int64(len(data)) <= setting.Avatar.MaxFileSize { - _ = user_service.UploadAvatar(ctx, u, data) - } - } +var oauth2AvatarHTTPClient = &http.Client{Timeout: 30 * time.Second} + +func oauth2UpdateAvatarIfNeed(ctx *context.Context, avatarURL string, u *user_model.User) { + if !setting.OAuth2Client.UpdateAvatar || len(avatarURL) == 0 { + return + } + req, err := http.NewRequestWithContext(ctx, http.MethodGet, avatarURL, nil) + if err != nil { + log.Warn("invalid avatar URL %q: %v", avatarURL, err) + return + } + // Some hosts (e.g. Wikimedia) reject Go's default User-Agent. + req.Header.Set("User-Agent", "Gitea "+setting.AppVer) + + resp, err := oauth2AvatarHTTPClient.Do(req) + if err != nil { + log.Warn("fetch %q failed: %v", avatarURL, err) + return + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + log.Warn("fetch %q returned status %d", avatarURL, resp.StatusCode) + return + } + data, err := io.ReadAll(io.LimitReader(resp.Body, setting.Avatar.MaxFileSize+1)) + if err != nil { + log.Warn("read body from %q failed: %v", avatarURL, err) + return + } + if int64(len(data)) > setting.Avatar.MaxFileSize { + log.Warn("avatar from %q exceeds max size %d", avatarURL, setting.Avatar.MaxFileSize) + return + } + if err := user_service.UploadAvatar(ctx, u, data); err != nil { + log.Warn("UploadAvatar for user %q failed: %v", u.Name, err) } } diff --git a/tests/integration/oauth_avatar_test.go b/tests/integration/oauth_avatar_test.go new file mode 100644 index 00000000000..d86f2f2cbe4 --- /dev/null +++ b/tests/integration/oauth_avatar_test.go @@ -0,0 +1,92 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "net/http" + "testing" + + auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" + "code.gitea.io/gitea/modules/web" + "code.gitea.io/gitea/routers/web/auth" + "code.gitea.io/gitea/services/auth/source/oauth2" + "code.gitea.io/gitea/services/context" + "code.gitea.io/gitea/tests" + + "github.com/markbates/goth" + "github.com/markbates/goth/gothic" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestOAuth2AvatarFromPicture(t *testing.T) { + defer tests.PrepareTestEnv(t)() + defer test.MockVariableValue(&setting.OAuth2Client.UpdateAvatar, true)() + + mockServer := createOAuth2MockProvider() + defer mockServer.Close() + addOAuth2Source(t, "test-oidc-avatar", oauth2.Source{ + Provider: "openidConnect", + ClientID: "test-client-id", + OpenIDConnectAutoDiscoveryURL: mockServer.URL + "/.well-known/openid-configuration", + }) + authSource, err := auth_model.GetActiveOAuth2SourceByAuthName(t.Context(), "test-oidc-avatar") + require.NoError(t, err) + providerName := authSource.Cfg.(*oauth2.Source).Provider + + t.Run("AutoRegister", func(t *testing.T) { + defer test.MockVariableValue(&setting.OAuth2Client.Username, "")() + defer test.MockVariableValue(&setting.OAuth2Client.EnableAutoRegistration, true)() + defer test.MockVariableValue(&gothic.CompleteUserAuth, func(res http.ResponseWriter, req *http.Request) (goth.User, error) { + return goth.User{ + Provider: providerName, + UserID: "oidc-user-ua-pic", + Email: "oidc-user-ua-pic@example.com", + Name: "OIDC UA Pic", + AvatarURL: mockServer.URL + "/avatar.png", + }, nil + })() + + req := NewRequest(t, "GET", "/user/oauth2/test-oidc-avatar/callback?code=XYZ&state=XYZ") + emptyTestSession(t).MakeRequest(t, req, http.StatusSeeOther) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{LoginName: "oidc-user-ua-pic"}) + assert.True(t, user.UseCustomAvatar, "avatar must sync (requires Gitea UA)") + assert.NotEmpty(t, user.Avatar) + }) + + t.Run("LinkAccountRegister", func(t *testing.T) { + const newUserName = "oidc-link-register" + defer web.RouteMockReset() + web.RouteMock(web.MockAfterMiddlewares, func(ctx *context.Context) { + require.NoError(t, auth.Oauth2SetLinkAccountData(ctx, auth.LinkAccountData{ + AuthSourceID: authSource.ID, + GothUser: goth.User{ + Provider: providerName, + UserID: "oidc-link-register-sub", + Email: "oidc-link-register-a@example.com", + Name: "OIDC Link Register", + AvatarURL: mockServer.URL + "/avatar.png", + }, + })) + }) + + req := NewRequestWithValues(t, "POST", "/user/link_account_signup", map[string]string{ + "user_name": newUserName, + "email": "oidc-link-register-b@example.com", + "password": "AVeryStrongPassword!1", + "retype": "AVeryStrongPassword!1", + }) + emptyTestSession(t).MakeRequest(t, req, http.StatusSeeOther) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{LowerName: newUserName}) + require.Equal(t, auth_model.OAuth2, user.LoginType) + assert.True(t, user.UseCustomAvatar, "register-link flow must sync avatar from `picture` claim") + assert.NotEmpty(t, user.Avatar) + }) +} diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index 5ec5fa7dcd3..90ff785ac1e 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -7,6 +7,8 @@ import ( "bytes" "encoding/base64" "fmt" + "image" + "image/png" "io" "net/http" "net/http/httptest" @@ -1062,6 +1064,13 @@ func createOAuth2MockProvider() *httptest.Server { var mockServer *httptest.Server mockServer = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { + case "/avatar.png": + if !strings.HasPrefix(r.Header.Get("User-Agent"), "Gitea ") { + http.Error(w, "user agent doesn't match", http.StatusForbidden) + return + } + w.Header().Set("Content-Type", "image/png") + _ = png.Encode(w, image.NewRGBA(image.Rect(0, 0, 8, 8))) case "/.well-known/openid-configuration": _, _ = w.Write([]byte(`{ "issuer": "` + mockServer.URL + `",