Fix #37564: Review comments, url sanitzer, wrap httprequest in context

modules/util/url.go

@@ -26,3 +26,23 @@ func SanitizeURL(s string) (string, error) {
 	u.User = nil
 	return u.String(), nil
 }
+
+// SanitizeURLForLog returns a redacted form of a URL safe to include in
+// log lines. It strips userinfo (e.g. https://user:pass@…), the query
+// string (which may contain signed-URL credentials such as AWS S3 / GCS /
+// Cloudinary signatures), and the fragment, leaving only scheme+host+path.
+// On a parse failure the placeholder "<unparseable url>" is returned to
+// avoid leaking the raw URL into logs.
+//
+// Unlike SanitizeURL this is intended exclusively for logging: callers
+// that still need to USE the URL (mirroring, indexing, migrations, etc.)
+// should keep using SanitizeURL because they need the query string
+// preserved.
+func SanitizeURLForLog(s string) string {
+	u, err := url.Parse(s)
+	if err != nil {
+		return "<unparseable url>"
+	}
+	redacted := url.URL{Scheme: u.Scheme, Host: u.Host, Path: u.Path}
+	return redacted.String()
+}

modules/util/url_test.go

@@ -0,0 +1,60 @@
+// Copyright 2026 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package util
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSanitizeURLForLog(t *testing.T) {
+	cases := []struct {
+		name string
+		in   string
+		want string
+	}{
+		{
+			name: "plain url is unchanged",
+			in:   "https://avatars.githubusercontent.com/u/9919",
+			want: "https://avatars.githubusercontent.com/u/9919",
+		},
+		{
+			name: "userinfo is stripped",
+			in:   "https://alice:secret@example.com/path/avatar.png",
+			want: "https://example.com/path/avatar.png",
+		},
+		{
+			name: "query string is stripped (signed URL credentials)",
+			in:   "https://bucket.s3.amazonaws.com/avatar.png?X-Amz-Signature=abc123&X-Amz-Expires=3600",
+			want: "https://bucket.s3.amazonaws.com/avatar.png",
+		},
+		{
+			name: "fragment is stripped",
+			in:   "https://example.com/avatar.png#token=xyz",
+			want: "https://example.com/avatar.png",
+		},
+		{
+			name: "userinfo + query + fragment are all stripped together",
+			in:   "https://u:p@host.example.com/p?sig=deadbeef#frag",
+			want: "https://host.example.com/p",
+		},
+		{
+			name: "empty url stays empty",
+			in:   "",
+			want: "",
+		},
+		{
+			name: "unparseable url is replaced with placeholder",
+			// %ZZ is not a valid percent-escape; net/url returns a parse error.
+			in:   "http://example.com/%ZZ",
+			want: "<unparseable url>",
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			assert.Equal(t, c.want, SanitizeURLForLog(c.in))
+		})
+	}
+}

routers/web/auth/oauth.go

@@ -23,6 +23,7 @@ import (
 	"code.gitea.io/gitea/modules/optional"
 	"code.gitea.io/gitea/modules/session"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 	source_service "code.gitea.io/gitea/services/auth/source"
 	"code.gitea.io/gitea/services/auth/source/oauth2"
 	"code.gitea.io/gitea/services/context"
@@ -301,13 +302,20 @@ 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 {
+func oauth2UpdateAvatarIfNeed(ctx *context.Context, rawURL string, u *user_model.User) {
+	if !setting.OAuth2Client.UpdateAvatar || len(rawURL) == 0 {
 		return
 	}
-	req, err := http.NewRequest(http.MethodGet, url, nil)
+	// Compute a redacted URL for log lines BEFORE issuing the request, so we
+	// never accidentally log signed-URL query parameters or userinfo.
+	logURL := util.SanitizeURLForLog(rawURL)
+
+	// Bind the outbound fetch to the inbound request context so the download
+	// is cancelled if the user navigates away / aborts login, and so any
+	// request-level deadline propagates to this fetch.
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, rawURL, nil)
 	if err != nil {
-		log.Warn("oauth2UpdateAvatarIfNeed: invalid avatar URL %q: %v", url, err)
+		log.Warn("oauth2UpdateAvatarIfNeed: invalid avatar URL %q: %v", logURL, err)
 		return
 	}
 	// Some image hosts (e.g. Wikimedia) reject requests using Go's default User-Agent.
@@ -316,22 +324,22 @@ func oauth2UpdateAvatarIfNeed(ctx *context.Context, url string, u *user_model.Us
 
 	resp, err := http.DefaultClient.Do(req)
 	if err != nil {
-		log.Warn("oauth2UpdateAvatarIfNeed: fetch %q failed: %v", url, err)
+		log.Warn("oauth2UpdateAvatarIfNeed: fetch %q failed: %v", logURL, err)
 		return
 	}
 	defer func() { _ = resp.Body.Close() }()
 
 	if resp.StatusCode != http.StatusOK {
-		log.Warn("oauth2UpdateAvatarIfNeed: fetch %q returned status %d", url, resp.StatusCode)
+		log.Warn("oauth2UpdateAvatarIfNeed: fetch %q returned status %d", logURL, resp.StatusCode)
 		return
 	}
 	data, err := io.ReadAll(io.LimitReader(resp.Body, setting.Avatar.MaxFileSize+1))
 	if err != nil {
-		log.Warn("oauth2UpdateAvatarIfNeed: read body from %q failed: %v", url, err)
+		log.Warn("oauth2UpdateAvatarIfNeed: read body from %q failed: %v", logURL, err)
 		return
 	}
 	if int64(len(data)) > setting.Avatar.MaxFileSize {
-		log.Warn("oauth2UpdateAvatarIfNeed: avatar from %q exceeds max size %d", url, setting.Avatar.MaxFileSize)
+		log.Warn("oauth2UpdateAvatarIfNeed: avatar from %q exceeds max size %d", logURL, setting.Avatar.MaxFileSize)
 		return
 	}
 	if err := user_service.UploadAvatar(ctx, u, data); err != nil {