From 8c94ee876c7a5995e0df2090cbfcdf832b4ae56f Mon Sep 17 00:00:00 2001 From: pandareen <7270563+pandareen@users.noreply.github.com> Date: Thu, 7 May 2026 22:44:05 +0530 Subject: [PATCH] Fix #37564: Review comments, url sanitzer, wrap httprequest in context --- modules/util/url.go | 20 +++++++++++++ modules/util/url_test.go | 60 +++++++++++++++++++++++++++++++++++++++ routers/web/auth/oauth.go | 24 ++++++++++------ 3 files changed, 96 insertions(+), 8 deletions(-) create mode 100644 modules/util/url_test.go diff --git a/modules/util/url.go b/modules/util/url.go index dc31441ca8..2fde9890eb 100644 --- a/modules/util/url.go +++ b/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 "" 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 "" + } + redacted := url.URL{Scheme: u.Scheme, Host: u.Host, Path: u.Path} + return redacted.String() +} diff --git a/modules/util/url_test.go b/modules/util/url_test.go new file mode 100644 index 0000000000..e922d0cf28 --- /dev/null +++ b/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: "", + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + assert.Equal(t, c.want, SanitizeURLForLog(c.in)) + }) + } +} diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index ce5b863c38..7b41ae53d0 100644 --- a/routers/web/auth/oauth.go +++ b/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 {