From bbfb793ea587dc6696badf7c613d8290fbe75ee1 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Fri, 8 May 2026 20:47:04 +0200 Subject: [PATCH] fix(telegram): never expose bot token in avatar URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tgAPI.Avatar returned a URL with the bot token embedded in its path: https://api.telegram.org/file/bot{TOKEN}/photos/file_X.jpg The token is a bearer credential for the entire bot API. The URL flowed into User.Picture and from there: * Into avatar.Proxy.Put debug logs ("[DEBUG] saved avatar from " and the corresponding load-failure line) regardless of whether avatar saving succeeded. * Into the JWT claims and the user JSON returned to the browser when no AvatarSaver was configured (User.Picture is in the User struct). Either path leaks the bot token to anyone with log access, anyone who can read the JWT (the user themselves on the device, plus anyone intercepting browser/devtools), or any third-party observability stack. Two-part fix in v1 and v2: 1. avatar/avatar.go: redact the URL in Put's two debug log lines via a new redactAvatarURL helper (hostname only). Add Proxy.PutContent so pre-fetched bytes can be saved without the URL-fetch round trip. 2. provider/telegram.go: in processUpdates, never assign the bot URL to User.Picture. Pass it to a new saveTelegramAvatar method that fetches the bytes server-side and stores them via the new content- saver interface (avatar.Proxy implements it). The call returns a clean local proxy URL or "" — whatever lands in Picture is safe to log and to send to the client. A graceful fallback path warns and drops the avatar when the configured AvatarSaver does not implement PutContent (custom external implementations) — never exposes the token to satisfy the avatar feature. Tests in both modules: * TestSaveTelegramAvatar_BotTokenNeverLogged — unit-level table for the helper covering the success, fallback-without-PutContent and empty-URL paths. * TestTelegramProcessUpdates_BotTokenNeverInUserPicture — regression test for the property: drive processUpdates with a mock that returns a URL containing a bot-token marker; assert the marker never lands in user.Picture and never appears in any captured log line. Reverting the saveTelegramAvatar redirection makes this test fail with a clear assertion message. --- README.md | 23 +++ avatar/avatar.go | 49 ++++- avatar/avatar_test.go | 53 +++++ middleware/auth_test.go | 2 +- provider/oauth2_test.go | 11 + provider/sender/email_test.go | 2 +- provider/telegram.go | 132 +++++++++++- provider/telegram_test.go | 339 ++++++++++++++++++++++++++++++- v2/avatar/avatar.go | 49 ++++- v2/avatar/avatar_test.go | 51 +++++ v2/middleware/auth_test.go | 2 +- v2/provider/oauth2_test.go | 11 + v2/provider/sender/email_test.go | 2 +- v2/provider/telegram.go | 132 +++++++++++- v2/provider/telegram_test.go | 308 +++++++++++++++++++++++++++- 15 files changed, 1124 insertions(+), 42 deletions(-) diff --git a/README.md b/README.md index 50da934..50f465f 100644 --- a/README.md +++ b/README.md @@ -377,6 +377,29 @@ go func() { service.AddCustomHandler(&telegram) ``` +#### Avatar handling + +Telegram's file API requires the bot token in the URL path +(`https://api.telegram.org/file/bot/...`), so the URL is a bearer +credential and must never reach the JWT, the user JSON, or any debug log. +The provider fetches the avatar bytes server-side and stores them via +`AvatarSaver`, returning a clean local proxy URL. + +This requires `AvatarSaver` to support direct content saving. +`avatar.Proxy` (the default returned by `service.AvatarProxy()`) does -- no +extra setup needed. **Custom `AvatarSaver` implementations** that don't +implement `PutContent(userID string, content io.Reader) (string, error)` +will see an empty `User.Picture` for Telegram logins, and the avatar proxy +will fall back to identicon. To preserve avatars with a custom saver, +implement that method alongside `Put`. + +> **Behaviour change for custom `AvatarSaver`:** before this change, custom +> savers received the raw Telegram file URL (which included the bot token) +> via `Put`. After this change they no longer do, because that URL is a +> bearer credential. Implementations that relied on receiving and refetching +> that URL must now implement `PutContent` to keep saving Telegram avatars, +> or accept that Telegram users get identicons. + Now all your users have to do is click one of the following links and press **start** `tg://resolve?domain=&start=` or `https://t.me//?start=` diff --git a/avatar/avatar.go b/avatar/avatar.go index bbfb363..488dfe6 100644 --- a/avatar/avatar.go +++ b/avatar/avatar.go @@ -11,6 +11,7 @@ import ( "image/png" "io" "net/http" + "net/url" "strconv" "strings" "time" @@ -26,6 +27,12 @@ import ( // http.sniffLen is 512 bytes which is how much we need to read to detect content type const sniffLen = 512 +// maxAvatarFetchSize bounds the bytes read from a remote avatar URL. 10 MiB is +// generous for any reasonable avatar (Telegram caps photo at 5 MiB; Gravatar is +// much smaller); the cap protects Proxy.Put against an upstream sending an +// unbounded body that would exhaust process memory inside resize. +const maxAvatarFetchSize = 10 << 20 + // Proxy provides http handler for avatars from avatar.Store // On user login token will call Put and it will retrieve and save picture locally. type Proxy struct { @@ -61,7 +68,7 @@ func (p *Proxy) Put(u token.User, client *http.Client) (avatarURL string, err er body, err := p.load(u.Picture, client) if err != nil { - p.Logf("[DEBUG] failed to fetch avatar from the orig %s, %v", u.Picture, err) + p.Logf("[DEBUG] failed to fetch avatar from the orig %s, %v", redactAvatarURL(u.Picture), err) return genIdenticon(u.ID) } @@ -76,10 +83,36 @@ func (p *Proxy) Put(u token.User, client *http.Client) (avatarURL string, err er return "", err } - p.Logf("[DEBUG] saved avatar from %s to %s, user %q", u.Picture, avatarID, u.Name) + p.Logf("[DEBUG] saved avatar from %s to %s, user %q", redactAvatarURL(u.Picture), avatarID, u.Name) + return p.URL + p.RoutePath + "/" + avatarID, nil +} + +// PutContent stores already-fetched avatar bytes via the underlying Store and returns +// the proxied URL. It exists so providers that authenticate with credentials embedded +// in the upstream URL (e.g. Telegram bot file API: /file/bot{TOKEN}/...) can fetch the +// content themselves and avoid exposing the credential to Put's URL-fetching path — +// where it would land in u.Picture, debug logs, and the user JSON returned to clients. +func (p *Proxy) PutContent(userID string, content io.Reader) (avatarURL string, err error) { + avatarID, err := p.Store.Put(userID, p.resize(content, p.ResizeLimit)) + if err != nil { + return "", err + } + p.Logf("[DEBUG] saved avatar bytes to %s, user %q", avatarID, userID) return p.URL + p.RoutePath + "/" + avatarID, nil } +// redactAvatarURL returns the hostname only, dropping scheme, userinfo, path, +// query and fragment. This is enough to keep avatar URLs identifiable in logs +// while ensuring credentials carried in any of those parts (e.g. Telegram bot +// tokens, time-limited signed-URL tokens, basic-auth in userinfo) don't reach +// log destinations. On parse failure a sentinel is returned. +func redactAvatarURL(raw string) string { + if u, err := url.Parse(raw); err == nil && u.Hostname() != "" { + return u.Hostname() + } + return "" +} + // load avatar from remote url and return body. Caller has to close the reader func (p *Proxy) load(url string, client *http.Client) (rc io.ReadCloser, err error) { // load avatar from remote location @@ -98,7 +131,17 @@ func (p *Proxy) load(url string, client *http.Client) (rc io.ReadCloser, err err return nil, fmt.Errorf("failed to get avatar from the orig, status %s", resp.Status) } - return resp.Body, nil + // buffer the body up to the cap to fail fast on oversized inputs. + // Reading +1 byte beyond the cap distinguishes "exactly cap" from "too big". + body, err := io.ReadAll(io.LimitReader(resp.Body, maxAvatarFetchSize+1)) + _ = resp.Body.Close() + if err != nil { + return nil, fmt.Errorf("failed to read avatar body: %w", err) + } + if int64(len(body)) > maxAvatarFetchSize { + return nil, fmt.Errorf("avatar body exceeds %d bytes", maxAvatarFetchSize) + } + return io.NopCloser(bytes.NewReader(body)), nil } // Handler returns token routes for given provider diff --git a/avatar/avatar_test.go b/avatar/avatar_test.go index 0052ff4..d599f15 100644 --- a/avatar/avatar_test.go +++ b/avatar/avatar_test.go @@ -58,6 +58,34 @@ func TestAvatar_Put(t *testing.T) { assert.Equal(t, int64(21), fi.Size()) } +func TestAvatar_PutContent(t *testing.T) { + defer func() { _ = os.RemoveAll("/tmp/avatars.put-content.test/") }() + p := Proxy{RoutePath: "/avatar", URL: "http://localhost:8080", Store: NewLocalFS("/tmp/avatars.put-content.test"), L: logger.Std} + got, err := p.PutContent("user1", strings.NewReader("png-bytes")) + require.NoError(t, err) + assert.Equal(t, "http://localhost:8080/avatar/b3daa77b4c04a9551b8781d03191fe098f325e67.image", got) + fi, err := os.Stat("/tmp/avatars.put-content.test/30/b3daa77b4c04a9551b8781d03191fe098f325e67.image") + require.NoError(t, err) + assert.Greater(t, fi.Size(), int64(0)) +} + +func TestAvatar_RedactAvatarURL(t *testing.T) { + cases := []struct { + in, want string + }{ + {in: "https://api.telegram.org/file/botSECRET/photo.jpg", want: "api.telegram.org"}, + {in: "https://x:y@example.com/path?q=1", want: "example.com"}, // #nosec G101 -- deliberate test fixture for userinfo redaction + {in: "", want: ""}, + {in: "/local/path", want: ""}, + {in: "://malformed", want: ""}, + } + for _, c := range cases { + t.Run(c.in, func(t *testing.T) { + assert.Equal(t, c.want, redactAvatarURL(c.in)) + }) + } +} + func TestAvatar_PutIdenticon(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { log.Print("request: ", r.URL.Path) @@ -103,6 +131,31 @@ func TestAvatar_PutFailed(t *testing.T) { assert.Equal(t, int64(992), fi.Size()) } +func TestAvatar_PutCapsBodySize(t *testing.T) { + // upstream that streams indefinitely past the cap; without + // the maxAvatarFetchSize check resize would consume all bytes. + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "image/*") + w.WriteHeader(http.StatusOK) + buf := make([]byte, 64<<10) + for i := 0; i < (maxAvatarFetchSize/len(buf))+32; i++ { + if _, err := w.Write(buf); err != nil { + return + } + } + })) + defer ts.Close() + + dir := t.TempDir() + p := Proxy{RoutePath: "/avatar", URL: "http://localhost:8080", Store: NewLocalFS(dir), L: logger.NoOp} + client := &http.Client{Timeout: 5 * time.Second} + + u := token.User{ID: "user1", Name: "huge avatar", Picture: ts.URL + "/pic.png"} + res, err := p.Put(u, client) + require.NoError(t, err, "Put falls back to identicon on capped fetch failure") + assert.Contains(t, res, "/avatar/", "still returns a proxy URL via identicon fallback") +} + func TestAvatar_Routes(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/middleware/auth_test.go b/middleware/auth_test.go index 1acae6e..44fc80b 100644 --- a/middleware/auth_test.go +++ b/middleware/auth_test.go @@ -340,7 +340,7 @@ func TestBasicAdminUserDoesNotLogPassword(t *testing.T) { const secret = "super-secret-attempted-passwd" var buf strings.Builder a := makeTestAuth(t) - a.L = logger.Func(func(format string, args ...interface{}) { + a.L = logger.Func(func(format string, args ...any) { fmt.Fprintf(&buf, format, args...) }) diff --git a/provider/oauth2_test.go b/provider/oauth2_test.go index 1c16047..77610d9 100644 --- a/provider/oauth2_test.go +++ b/provider/oauth2_test.go @@ -369,3 +369,14 @@ func (m *mockAvatarSaver) Put(u token.User, _ *http.Client) (avatarURL string, e return "http://example.com/fake.png", nil } + +// PutContent satisfies the avatarContentSaver type assertion in telegram.go so +// the test mock behaves like avatar.Proxy and the bot-token-aware path can be +// exercised. The body is drained but the saver returns the same canned URL it +// would for a regular Put, so existing tests asserting on Picture stay valid. +func (m *mockAvatarSaver) PutContent(_ string, content io.Reader) (avatarURL string, err error) { + if _, e := io.Copy(io.Discard, content); e != nil { + return "", e + } + return "http://example.com/ava12345.png", nil +} diff --git a/provider/sender/email_test.go b/provider/sender/email_test.go index f02c162..6a2ee5a 100644 --- a/provider/sender/email_test.go +++ b/provider/sender/email_test.go @@ -59,7 +59,7 @@ func TestEmail_New(t *testing.T) { func TestEmail_SendDoesNotLogBody(t *testing.T) { const secretBody = "Confirmation token: super-secret-magic-link-XYZ" var buf strings.Builder - capturing := logger.Func(func(format string, args ...interface{}) { + capturing := logger.Func(func(format string, args ...any) { fmt.Fprintf(&buf, format, args...) }) p := EmailParams{Host: "127.0.0.2", Port: 25, From: "from@example.com", diff --git a/provider/telegram.go b/provider/telegram.go index 33a8bb1..8da1f38 100644 --- a/provider/telegram.go +++ b/provider/telegram.go @@ -3,13 +3,16 @@ package provider //go:generate moq --out telegram_moq_test.go . TelegramAPI import ( + "bytes" "context" "crypto/sha1" "encoding/json" + "errors" "fmt" "io" "net/http" neturl "net/url" + "regexp" "strings" "sync" "sync/atomic" @@ -19,6 +22,7 @@ import ( "github.com/go-pkgz/rest" "github.com/golang-jwt/jwt" + "github.com/go-pkgz/auth/avatar" "github.com/go-pkgz/auth/logger" authtoken "github.com/go-pkgz/auth/token" ) @@ -186,11 +190,18 @@ func (th *TelegramHandler) processUpdates(ctx context.Context, updates *telegram id := th.ProviderName + "_" + authtoken.HashID(sha1.New(), fmt.Sprint(update.Message.Chat.ID)) + // avatarURL embeds the bot token in its path + // (https://api.telegram.org/file/bot{TOKEN}/...). Never store it in + // User.Picture: it would leak through avatar.Proxy.Put logs and, when + // no avatar saver is configured, into the JWT and on to the client. + // Fetch the bytes here and hand them to the avatar store directly. + picture := th.saveTelegramAvatar(ctx, id, avatarURL) + authRequest.confirmed = true authRequest.user = &authtoken.User{ ID: id, Name: update.Message.Chat.Name, - Picture: avatarURL, + Picture: picture, } th.requests.Lock() @@ -294,10 +305,19 @@ func (th *TelegramHandler) LoginHandler(w http.ResponseWriter, r *http.Request) return } - u, err := setAvatar(th.AvatarSaver, *authUser, &http.Client{Timeout: 5 * time.Second}) - if err != nil { - rest.SendErrorJSON(w, r, th.L, http.StatusInternalServerError, err, "failed to save avatar to proxy") - return + // when saveTelegramAvatar already populated Picture with a local proxy + // URL, skip the URL-fetching avatar pipeline. Letting setAvatar run + // here would have it call Proxy.Put which re-fetches Picture; in + // split-DNS / unreachable-internal-Opts.URL deployments that fetch + // fails and the identicon fallback would silently overwrite the + // stored Telegram bytes with an identicon at the same store path. + u := *authUser + if u.Picture == "" { + u, err = setAvatar(th.AvatarSaver, *authUser, &http.Client{Timeout: 5 * time.Second}) + if err != nil { + rest.SendErrorJSON(w, r, th.L, http.StatusInternalServerError, err, "failed to save avatar to proxy") + return + } } claims := authtoken.Claims{ @@ -455,12 +475,12 @@ func (tg *tgAPI) request(ctx context.Context, method string, data any) error { req, err := http.NewRequestWithContext(ctx, "GET", url, http.NoBody) if err != nil { - return fmt.Errorf("failed to create request: %w", err) + return fmt.Errorf("failed to create request: %w", redactBotURLInErr(err)) } resp, err := tg.client.Do(req) if err != nil { - return fmt.Errorf("failed to send request: %w", err) + return fmt.Errorf("failed to send request: %w", redactBotURLInErr(err)) } defer resp.Body.Close() //nolint gosec // we don't care about response body @@ -485,3 +505,101 @@ func (tg *tgAPI) parseError(r io.Reader, statusCode int) error { } return fmt.Errorf("unexpected telegram API status code %d, error: %q", statusCode, tgErr.Description) } + +// avatarContentSaver matches the optional method on AvatarSaver implementations +// that can store already-fetched bytes (avatar.Proxy provides one). Used by the +// Telegram provider to avoid passing a bot-token-bearing URL through the +// URL-fetching avatar pipeline. +type avatarContentSaver interface { + PutContent(userID string, content io.Reader) (string, error) +} + +// saveTelegramAvatar fetches the avatar bytes from a bot-token-bearing Telegram +// URL and stores them via th.AvatarSaver, returning a clean local proxy URL. +// The bot URL is consumed entirely inside this function so it never reaches +// User.Picture, JWT claims, or any debug log of the user object. Returns "" +// when the avatar cannot be saved (no URL, no compatible saver, or fetch +// failure) — the caller treats that as "no picture" and the avatar pipeline +// falls back to identicon as usual. +func (th *TelegramHandler) saveTelegramAvatar(ctx context.Context, userID, avatarURL string) string { + if avatarURL == "" { + return "" + } + // guard against typed-nil *avatar.Proxy. auth.go skips initializing + // res.avatarProxy when Opts.AvatarStore is unset, so AvatarSaver can be + // a non-nil interface wrapping a nil *avatar.Proxy. The type assertion + // below would still succeed (interface satisfaction is structural), but + // PutContent on a nil receiver panics on the first p.Store deref. + if th.AvatarSaver == nil || th.AvatarSaver == (*avatar.Proxy)(nil) { + th.Logf("[WARN] telegram avatar dropped: AvatarSaver is not configured") + return "" + } + saver, ok := th.AvatarSaver.(avatarContentSaver) + if !ok { + // fallback intentionally drops the picture rather than expose the bot + // token; warn so operators can wire a content-aware saver if they want + // telegram avatars saved + th.Logf("[WARN] telegram avatar dropped: configured AvatarSaver does not support direct content save") + return "" + } + req, err := http.NewRequestWithContext(ctx, http.MethodGet, avatarURL, http.NoBody) + if err != nil { + th.Logf("[WARN] telegram avatar fetch request build failed: %v", redactBotURLInErr(err)) + return "" + } + client := &http.Client{Timeout: 5 * time.Second} + resp, err := client.Do(req) + if err != nil { + th.Logf("[WARN] telegram avatar fetch failed: %v", redactBotURLInErr(err)) + return "" + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + th.Logf("[WARN] telegram avatar fetch returned status %d", resp.StatusCode) + return "" + } + // cap body size to protect PutContent from an unbounded upstream response. + // Telegram caps photos at 5 MiB; 10 MiB is generous headroom while still + // bounding worst-case memory. + body, err := io.ReadAll(io.LimitReader(resp.Body, maxTelegramAvatarSize+1)) + if err != nil { + th.Logf("[WARN] telegram avatar read failed: %v", err) + return "" + } + if int64(len(body)) > maxTelegramAvatarSize { + th.Logf("[WARN] telegram avatar dropped: body exceeds %d bytes", maxTelegramAvatarSize) + return "" + } + picture, err := saver.PutContent(userID, bytes.NewReader(body)) + if err != nil { + th.Logf("[WARN] telegram avatar save failed: %v", err) + return "" + } + return picture +} + +const maxTelegramAvatarSize = 10 << 20 + +// botTokenInURLPath matches the bot-token segment of a Telegram URL anchored +// between path slashes ("/botTOKEN/..."). The leading and trailing slashes +// avoid matching unrelated identifiers that happen to start with "bot" (e.g. +// the username "botFather" appearing elsewhere in a log line). Replacement +// preserves the slashes via "/bot/" to keep surrounding URL +// structure intact for diagnostics. +var botTokenInURLPath = regexp.MustCompile(`/bot[A-Za-z0-9:_-]+/`) + +// redactBotURLInErr returns the error with any embedded Telegram bot-token +// segment in URL paths replaced by "bot". net/http's *url.Error +// stringifies as `Op "URL": Err`, so a transport failure on a URL like +// https://api.telegram.org/file/bot/... otherwise prints the token +// verbatim. +func redactBotURLInErr(err error) error { + if err == nil { + return nil + } + redacted := botTokenInURLPath.ReplaceAllString(err.Error(), "/bot/") + if redacted == err.Error() { + return err + } + return errors.New(redacted) +} diff --git a/provider/telegram_test.go b/provider/telegram_test.go index 6d07d50..3180abe 100644 --- a/provider/telegram_test.go +++ b/provider/telegram_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "io" "net/http" "net/http/httptest" "net/url" @@ -13,7 +14,10 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/go-pkgz/auth/avatar" + "github.com/go-pkgz/auth/logger" authtoken "github.com/go-pkgz/auth/token" ) @@ -121,15 +125,6 @@ func TestTelegramConfirmedRequest(t *testing.T) { return &upd, nil }, - AvatarFunc: func(ctx context.Context, userID int) (string, error) { - select { - case <-ctx.Done(): - return "", ctx.Err() - default: - } - assert.Equal(t, 313131313, userID) - return "http://t.me/avatar.png", nil - }, SendFunc: func(ctx context.Context, id int, text string) error { select { case <-ctx.Done(): @@ -143,6 +138,24 @@ func TestTelegramConfirmedRequest(t *testing.T) { BotInfoFunc: botInfoFunc, } + // stand up a tiny server that serves the avatar bytes — the + // saveTelegramAvatar helper does an HTTP GET on whatever URL Avatar + // returns; using a real, reachable URL keeps the bot-token redaction + // logic exercised end-to-end. + avatarSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("png-bytes")) + })) + defer avatarSrv.Close() + m.AvatarFunc = func(ctx context.Context, userID int) (string, error) { + select { + case <-ctx.Done(): + return "", ctx.Err() + default: + } + assert.Equal(t, 313131313, userID) + return avatarSrv.URL + "/file/botSECRET-TOKEN/photo.jpg", nil + } + tg, cleanup := setupHandler(t, m) defer cleanup() @@ -195,6 +208,314 @@ func TestTelegramConfirmedRequest(t *testing.T) { assert.Equal(t, `{"error":"request is not found"}`+"\n", w.Body.String()) } +func TestRedactBotURLInErr(t *testing.T) { + tests := []struct { + name string + in error + want string + }{ + {name: "nil error", in: nil, want: ""}, + {name: "error without bot pattern is unchanged", in: fmt.Errorf("plain error"), want: "plain error"}, + {name: "bot token in URL is redacted", in: fmt.Errorf(`Get "https://api.telegram.org/file/bot1234567:SECRET-TOK_EN-x/photo.jpg": dial tcp`), + want: `Get "https://api.telegram.org/file/bot/photo.jpg": dial tcp`}, + {name: "wrapped error with bot pattern is redacted", + in: fmt.Errorf("wrap: %w", fmt.Errorf(`Get "https://api.telegram.org/bot1234:abc-def_99/getMe": context deadline`)), + want: `wrap: Get "https://api.telegram.org/bot/getMe": context deadline`}, + {name: "non-path bot identifiers (botFather, botanic) are not over-redacted", + in: fmt.Errorf("user @botFather mentioned in chat about botanic gardens"), + want: `user @botFather mentioned in chat about botanic gardens`}, + {name: "bot id without trailing slash is not redacted (anchored regex requires path boundaries)", + in: fmt.Errorf(`reference to bot1234:abc-def_99 in narrative text`), + want: `reference to bot1234:abc-def_99 in narrative text`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := redactBotURLInErr(tt.in) + if tt.in == nil { + assert.Nil(t, got) + return + } + require.NotNil(t, got) + assert.Equal(t, tt.want, got.Error()) + }) + } +} + +func TestSaveTelegramAvatar_TransportErrorDoesNotLeakBotToken(t *testing.T) { + const botToken = "1234567:SECRET-bot-token-do-not-log" + var logBuf strings.Builder + captureLog := logger.Func(func(format string, args ...any) { + fmt.Fprintf(&logBuf, format, args...) + }) + + th := &TelegramHandler{ + AvatarSaver: &mockAvatarSaver{}, + L: captureLog, + } + // unreachable host -- forces http.Client.Do to return a *url.Error whose + // default stringification embeds the full URL (including bot token). + got := th.saveTelegramAvatar(context.Background(), "user1", + "https://api.telegram.invalid/file/bot"+botToken+"/photo.jpg") + assert.Equal(t, "", got) + assert.NotContains(t, logBuf.String(), botToken, + "bot token must not appear in log even when http.Client.Do returns an error containing the URL") +} + +func TestSaveTelegramAvatar_BotTokenNeverLogged(t *testing.T) { + const botToken = "1234567:SECRET-bot-token-do-not-log" + var logBuf strings.Builder + captureLog := logger.Func(func(format string, args ...any) { + fmt.Fprintf(&logBuf, format, args...) + }) + + avatarSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("png")) + })) + defer avatarSrv.Close() + + t.Run("returns proxy URL via PutContent and does not leak bot token", func(t *testing.T) { + logBuf.Reset() + th := &TelegramHandler{ + AvatarSaver: &mockAvatarSaver{}, + L: captureLog, + } + got := th.saveTelegramAvatar(context.Background(), "user1", + avatarSrv.URL+"/file/bot"+botToken+"/photo.jpg") + assert.Equal(t, "http://example.com/ava12345.png", got) + assert.NotContains(t, logBuf.String(), botToken, "bot token must never appear in logs") + }) + + t.Run("returns empty when saver lacks PutContent and warns operator", func(t *testing.T) { + logBuf.Reset() + th := &TelegramHandler{ + AvatarSaver: legacyAvatarSaver{}, + L: captureLog, + } + got := th.saveTelegramAvatar(context.Background(), "user1", + avatarSrv.URL+"/file/bot"+botToken+"/photo.jpg") + assert.Equal(t, "", got, "Picture must be empty rather than carry bot URL") + assert.NotContains(t, logBuf.String(), botToken, "bot token must never appear in logs") + assert.Contains(t, logBuf.String(), "telegram avatar dropped") + }) + + t.Run("empty avatar URL returns empty without fetch attempt", func(t *testing.T) { + logBuf.Reset() + th := &TelegramHandler{AvatarSaver: &mockAvatarSaver{}, L: captureLog} + got := th.saveTelegramAvatar(context.Background(), "user1", "") + assert.Equal(t, "", got) + }) + + t.Run("non-200 status from Telegram file API returns empty", func(t *testing.T) { + logBuf.Reset() + failSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "expired file_id", http.StatusNotFound) + })) + defer failSrv.Close() + + th := &TelegramHandler{AvatarSaver: &mockAvatarSaver{}, L: captureLog} + got := th.saveTelegramAvatar(context.Background(), "user1", + failSrv.URL+"/file/bot"+botToken+"/photo.jpg") + assert.Equal(t, "", got) + assert.Contains(t, logBuf.String(), "telegram avatar fetch returned status 404") + assert.NotContains(t, logBuf.String(), botToken, "bot token must never appear in logs") + }) + + t.Run("PutContent error returns empty and warns operator", func(t *testing.T) { + logBuf.Reset() + th := &TelegramHandler{ + AvatarSaver: &failingAvatarSaver{}, + L: captureLog, + } + got := th.saveTelegramAvatar(context.Background(), "user1", + avatarSrv.URL+"/file/bot"+botToken+"/photo.jpg") + assert.Equal(t, "", got) + assert.Contains(t, logBuf.String(), "telegram avatar save failed") + assert.NotContains(t, logBuf.String(), botToken, "bot token must never appear in logs") + }) + + t.Run("oversized body is rejected and warns operator", func(t *testing.T) { + logBuf.Reset() + bigSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + buf := make([]byte, 64<<10) + for i := 0; i < (maxTelegramAvatarSize/len(buf))+32; i++ { + if _, err := w.Write(buf); err != nil { + return + } + } + })) + defer bigSrv.Close() + + th := &TelegramHandler{AvatarSaver: &mockAvatarSaver{}, L: captureLog} + got := th.saveTelegramAvatar(context.Background(), "user1", + bigSrv.URL+"/file/bot"+botToken+"/photo.jpg") + assert.Equal(t, "", got) + assert.Contains(t, logBuf.String(), "telegram avatar dropped: body exceeds") + assert.NotContains(t, logBuf.String(), botToken, "bot token must never appear in logs") + }) +} + +// failingAvatarSaver implements both Put and PutContent but its PutContent +// returns an error -- exercises the saver-side failure branch in +// saveTelegramAvatar without going through a real avatar.Proxy. +type failingAvatarSaver struct{} + +func (failingAvatarSaver) Put(authtoken.User, *http.Client) (string, error) { return "", nil } +func (failingAvatarSaver) PutContent(string, io.Reader) (string, error) { return "", fmt.Errorf("disk full") } + +type legacyAvatarSaver struct{} + +func (legacyAvatarSaver) Put(authtoken.User, *http.Client) (string, error) { return "", nil } + +// TestSaveTelegramAvatar_TypedNilAvatarSaverDoesNotPanic guards against the +// case where Opts.AvatarStore is unset. auth.go skips initializing +// res.avatarProxy then, so AvatarSaver ends up as a typed-nil *avatar.Proxy +// (non-nil interface wrapping a nil pointer). The avatarContentSaver type +// assertion would still succeed because interface satisfaction is structural, +// and PutContent on a nil receiver would panic on the first p.Store deref. +// The guard returns "" with a warn log instead. +func TestSaveTelegramAvatar_TypedNilAvatarSaverDoesNotPanic(t *testing.T) { + avatarSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("png-bytes")) + })) + defer avatarSrv.Close() + + var nilProxy *avatar.Proxy + th := &TelegramHandler{AvatarSaver: nilProxy, L: logger.NoOp} + // reachable URL so that without the typed-nil guard the function would + // proceed past the fetch into PutContent on a nil receiver and panic + // inside avatar.Proxy at p.Store deref. + got := th.saveTelegramAvatar(context.Background(), "user1", avatarSrv.URL+"/file/botSECRET/photo.jpg") + assert.Equal(t, "", got, "typed-nil AvatarSaver must short-circuit before fetch + PutContent") +} + +// TestTelegramLoginHandler_DoesNotOverwriteSavedAvatar guards against the +// double-pipeline regression: after saveTelegramAvatar stores the bytes and +// sets Picture to a local proxy URL, LoginHandler used to call setAvatar +// which re-fetches Picture. In split-DNS / unreachable-internal-Opts.URL +// deployments the fetch fails and Proxy.Put silently overwrites the just- +// stored Telegram avatar with an identicon at the same store path. This +// test wires a real avatar.Proxy with an unreachable Opts.URL and asserts +// the second-pass fetch does not happen (Picture survives intact). +func TestTelegramLoginHandler_DoesNotOverwriteSavedAvatar(t *testing.T) { + dir := t.TempDir() + store := avatar.NewLocalFS(dir) + // unreachable URL so any Proxy.Put re-fetch would fail and trigger + // identicon fallback at the same store path + proxy := &avatar.Proxy{ + Store: store, + URL: "http://127.0.0.1:1", + RoutePath: "/avatar", + L: logger.NoOp, + } + + avatarSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("png-bytes")) + })) + defer avatarSrv.Close() + + const tok = "registered-token" + th := &TelegramHandler{ + ProviderName: "telegram", + Telegram: &TelegramAPIMock{ + AvatarFunc: func(context.Context, int) (string, error) { + return avatarSrv.URL + "/file/botSECRET/photo.jpg", nil + }, + SendFunc: func(context.Context, int, string) error { return nil }, + BotInfoFunc: botInfoFunc, + }, + AvatarSaver: proxy, + L: logger.NoOp, + SuccessMsg: "ok", + } + th.requests.data = map[string]tgAuthRequest{ + tok: {expires: time.Now().Add(time.Hour)}, + } + + updates := &telegramUpdate{} + require.NoError(t, json.Unmarshal([]byte(fmt.Sprintf(getUpdatesResp, tok)), updates)) + th.processUpdates(context.Background(), updates) + + got := th.requests.data[tok] + require.NotNil(t, got.user, "request must be confirmed") + pictureFromUpdate := got.user.Picture + require.NotEmpty(t, pictureFromUpdate, "telegram path must populate Picture via PutContent") + require.True(t, strings.HasPrefix(pictureFromUpdate, "http://127.0.0.1:1/avatar/"), + "Picture should point at proxy URL, got: %s", pictureFromUpdate) + + jwtSvc := authtoken.NewService(authtoken.Opts{ + SecretReader: authtoken.SecretFunc(func(string) (string, error) { return "secret", nil }), + }) + th.TokenService = jwtSvc + rr := httptest.NewRecorder() + req, err := http.NewRequest("GET", "/login?token="+tok, http.NoBody) + require.NoError(t, err) + th.LoginHandler(rr, req) + require.Equal(t, http.StatusOK, rr.Code) + assert.Contains(t, rr.Body.String(), pictureFromUpdate, "Picture must round-trip unchanged from saveTelegramAvatar") +} + +// TestTelegramProcessUpdates_BotTokenNeverInUserPicture is the regression-style +// property test the user asked for: drives the full update-processing flow with +// a TelegramAPI mock that hands back a URL containing a bot-token-like marker, +// then asserts (a) the marker never lands in authRequest.user.Picture and +// (b) the marker never appears in any captured log line. +// +// Reverting the saveTelegramAvatar redirection (i.e. assigning avatarURL +// directly to user.Picture) makes assertion (a) fail. Reverting the avatar +// proxy log redaction makes assertion (b) fail when the avatar pipeline +// actually fetches the URL. Either way the test will scream loudly. +func TestTelegramProcessUpdates_BotTokenNeverInUserPicture(t *testing.T) { + const botToken = "secret-bot-token-marker" + var logBuf strings.Builder + var logMu sync.Mutex + captureLog := logger.Func(func(format string, args ...any) { + logMu.Lock() + defer logMu.Unlock() + fmt.Fprintf(&logBuf, format+"\n", args...) + }) + + avatarSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("png")) + })) + defer avatarSrv.Close() + + m := &TelegramAPIMock{ + AvatarFunc: func(context.Context, int) (string, error) { + return avatarSrv.URL + "/file/bot" + botToken + "/photo.jpg", nil + }, + SendFunc: func(context.Context, int, string) error { return nil }, + BotInfoFunc: botInfoFunc, + } + + const tok = "registered-token" + th := &TelegramHandler{ + ProviderName: "telegram", + Telegram: m, + AvatarSaver: &mockAvatarSaver{}, + L: captureLog, + SuccessMsg: "ok", + } + th.requests.data = map[string]tgAuthRequest{ + tok: {expires: time.Now().Add(time.Hour)}, + } + + updates := &telegramUpdate{} + err := json.Unmarshal(fmt.Appendf(nil, getUpdatesResp, tok), updates) + require.NoError(t, err) + + th.processUpdates(context.Background(), updates) + + got := th.requests.data[tok] + require.NotNil(t, got.user, "auth request should have been confirmed") + assert.NotContains(t, got.user.Picture, botToken, "User.Picture must not carry the bot token") + + logMu.Lock() + logged := logBuf.String() + logMu.Unlock() + assert.NotContains(t, logged, botToken, "no log line must contain the bot token") +} + func TestTelegramLogout(t *testing.T) { m := &TelegramAPIMock{ GetUpdatesFunc: func(ctx context.Context) (*telegramUpdate, error) { diff --git a/v2/avatar/avatar.go b/v2/avatar/avatar.go index 9147ebb..2aaecd2 100644 --- a/v2/avatar/avatar.go +++ b/v2/avatar/avatar.go @@ -11,6 +11,7 @@ import ( "image/png" "io" "net/http" + "net/url" "strconv" "strings" "time" @@ -26,6 +27,12 @@ import ( // http.sniffLen is 512 bytes which is how much we need to read to detect content type const sniffLen = 512 +// maxAvatarFetchSize bounds the bytes read from a remote avatar URL. 10 MiB is +// generous for any reasonable avatar (Telegram caps photo at 5 MiB; Gravatar is +// much smaller); the cap protects Proxy.Put against an upstream sending an +// unbounded body that would exhaust process memory inside resize. +const maxAvatarFetchSize = 10 << 20 + // Proxy provides http handler for avatars from avatar.Store // On user login token will call Put and it will retrieve and save picture locally. type Proxy struct { @@ -61,7 +68,7 @@ func (p *Proxy) Put(u token.User, client *http.Client) (avatarURL string, err er body, err := p.load(u.Picture, client) if err != nil { - p.Logf("[DEBUG] failed to fetch avatar from the orig %s, %v", u.Picture, err) + p.Logf("[DEBUG] failed to fetch avatar from the orig %s, %v", redactAvatarURL(u.Picture), err) return genIdenticon(u.ID) } @@ -76,10 +83,36 @@ func (p *Proxy) Put(u token.User, client *http.Client) (avatarURL string, err er return "", err } - p.Logf("[DEBUG] saved avatar from %s to %s, user %q", u.Picture, avatarID, u.Name) + p.Logf("[DEBUG] saved avatar from %s to %s, user %q", redactAvatarURL(u.Picture), avatarID, u.Name) + return p.URL + p.RoutePath + "/" + avatarID, nil +} + +// PutContent stores already-fetched avatar bytes via the underlying Store and returns +// the proxied URL. It exists so providers that authenticate with credentials embedded +// in the upstream URL (e.g. Telegram bot file API: /file/bot{TOKEN}/...) can fetch the +// content themselves and avoid exposing the credential to Put's URL-fetching path — +// where it would land in u.Picture, debug logs, and the user JSON returned to clients. +func (p *Proxy) PutContent(userID string, content io.Reader) (avatarURL string, err error) { + avatarID, err := p.Store.Put(userID, p.resize(content, p.ResizeLimit)) + if err != nil { + return "", err + } + p.Logf("[DEBUG] saved avatar bytes to %s, user %q", avatarID, userID) return p.URL + p.RoutePath + "/" + avatarID, nil } +// redactAvatarURL returns the hostname only, dropping scheme, userinfo, path, +// query and fragment. This is enough to keep avatar URLs identifiable in logs +// while ensuring credentials carried in any of those parts (e.g. Telegram bot +// tokens, time-limited signed-URL tokens, basic-auth in userinfo) don't reach +// log destinations. On parse failure a sentinel is returned. +func redactAvatarURL(raw string) string { + if u, err := url.Parse(raw); err == nil && u.Hostname() != "" { + return u.Hostname() + } + return "" +} + // load avatar from remote url and return body. Caller has to close the reader func (p *Proxy) load(url string, client *http.Client) (rc io.ReadCloser, err error) { // load avatar from remote location @@ -98,7 +131,17 @@ func (p *Proxy) load(url string, client *http.Client) (rc io.ReadCloser, err err return nil, fmt.Errorf("failed to get avatar from the orig, status %s", resp.Status) } - return resp.Body, nil + // buffer the body up to the cap to fail fast on oversized inputs. + // Reading +1 byte beyond the cap distinguishes "exactly cap" from "too big". + body, err := io.ReadAll(io.LimitReader(resp.Body, maxAvatarFetchSize+1)) + _ = resp.Body.Close() + if err != nil { + return nil, fmt.Errorf("failed to read avatar body: %w", err) + } + if int64(len(body)) > maxAvatarFetchSize { + return nil, fmt.Errorf("avatar body exceeds %d bytes", maxAvatarFetchSize) + } + return io.NopCloser(bytes.NewReader(body)), nil } // Handler returns token routes for given provider diff --git a/v2/avatar/avatar_test.go b/v2/avatar/avatar_test.go index 2f379d2..154b33e 100644 --- a/v2/avatar/avatar_test.go +++ b/v2/avatar/avatar_test.go @@ -58,6 +58,34 @@ func TestAvatar_Put(t *testing.T) { assert.Equal(t, int64(21), fi.Size()) } +func TestAvatar_PutContent(t *testing.T) { + defer func() { _ = os.RemoveAll("/tmp/avatars.put-content.test/") }() + p := Proxy{RoutePath: "/avatar", URL: "http://localhost:8080", Store: NewLocalFS("/tmp/avatars.put-content.test"), L: logger.Std} + got, err := p.PutContent("user1", strings.NewReader("png-bytes")) + require.NoError(t, err) + assert.Equal(t, "http://localhost:8080/avatar/b3daa77b4c04a9551b8781d03191fe098f325e67.image", got) + fi, err := os.Stat("/tmp/avatars.put-content.test/30/b3daa77b4c04a9551b8781d03191fe098f325e67.image") + require.NoError(t, err) + assert.Greater(t, fi.Size(), int64(0)) +} + +func TestAvatar_RedactAvatarURL(t *testing.T) { + cases := []struct { + in, want string + }{ + {in: "https://api.telegram.org/file/botSECRET/photo.jpg", want: "api.telegram.org"}, + {in: "https://x:y@example.com/path?q=1", want: "example.com"}, // #nosec G101 -- deliberate test fixture for userinfo redaction + {in: "", want: ""}, + {in: "/local/path", want: ""}, + {in: "://malformed", want: ""}, + } + for _, c := range cases { + t.Run(c.in, func(t *testing.T) { + assert.Equal(t, c.want, redactAvatarURL(c.in)) + }) + } +} + func TestAvatar_PutIdenticon(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { log.Print("request: ", r.URL.Path) @@ -103,6 +131,29 @@ func TestAvatar_PutFailed(t *testing.T) { assert.Equal(t, int64(992), fi.Size()) } +func TestAvatar_PutCapsBodySize(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "image/*") + w.WriteHeader(http.StatusOK) + buf := make([]byte, 64<<10) + for i := 0; i < (maxAvatarFetchSize/len(buf))+32; i++ { + if _, err := w.Write(buf); err != nil { + return + } + } + })) + defer ts.Close() + + dir := t.TempDir() + p := Proxy{RoutePath: "/avatar", URL: "http://localhost:8080", Store: NewLocalFS(dir), L: logger.NoOp} + client := &http.Client{Timeout: 5 * time.Second} + + u := token.User{ID: "user1", Name: "huge avatar", Picture: ts.URL + "/pic.png"} + res, err := p.Put(u, client) + require.NoError(t, err, "Put falls back to identicon on capped fetch failure") + assert.Contains(t, res, "/avatar/", "still returns a proxy URL via identicon fallback") +} + func TestAvatar_Routes(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/v2/middleware/auth_test.go b/v2/middleware/auth_test.go index eaae31e..3d5ba54 100644 --- a/v2/middleware/auth_test.go +++ b/v2/middleware/auth_test.go @@ -339,7 +339,7 @@ func TestBasicAdminUserDoesNotLogPassword(t *testing.T) { const secret = "super-secret-attempted-passwd" var buf strings.Builder a := makeTestAuth(t) - a.L = logger.Func(func(format string, args ...interface{}) { + a.L = logger.Func(func(format string, args ...any) { fmt.Fprintf(&buf, format, args...) }) diff --git a/v2/provider/oauth2_test.go b/v2/provider/oauth2_test.go index bb6ecb0..e10419e 100644 --- a/v2/provider/oauth2_test.go +++ b/v2/provider/oauth2_test.go @@ -458,3 +458,14 @@ func (m *mockAvatarSaver) Put(u token.User, _ *http.Client) (avatarURL string, e return "http://example.com/fake.png", nil } + +// PutContent satisfies the avatarContentSaver type assertion in telegram.go so +// the test mock behaves like avatar.Proxy and the bot-token-aware path can be +// exercised. The body is drained but the saver returns the same canned URL it +// would for a regular Put, so existing tests asserting on Picture stay valid. +func (m *mockAvatarSaver) PutContent(_ string, content io.Reader) (avatarURL string, err error) { + if _, e := io.Copy(io.Discard, content); e != nil { + return "", e + } + return "http://example.com/ava12345.png", nil +} diff --git a/v2/provider/sender/email_test.go b/v2/provider/sender/email_test.go index b888336..112f1e1 100644 --- a/v2/provider/sender/email_test.go +++ b/v2/provider/sender/email_test.go @@ -59,7 +59,7 @@ func TestEmail_New(t *testing.T) { func TestEmail_SendDoesNotLogBody(t *testing.T) { const secretBody = "Confirmation token: super-secret-magic-link-XYZ" var buf strings.Builder - capturing := logger.Func(func(format string, args ...interface{}) { + capturing := logger.Func(func(format string, args ...any) { fmt.Fprintf(&buf, format, args...) }) p := EmailParams{Host: "127.0.0.2", Port: 25, From: "from@example.com", diff --git a/v2/provider/telegram.go b/v2/provider/telegram.go index 3f1f274..a54f3d7 100644 --- a/v2/provider/telegram.go +++ b/v2/provider/telegram.go @@ -3,13 +3,16 @@ package provider //go:generate moq --out telegram_moq_test.go . TelegramAPI import ( + "bytes" "context" "crypto/sha1" "encoding/json" + "errors" "fmt" "io" "net/http" neturl "net/url" + "regexp" "strings" "sync" "sync/atomic" @@ -19,6 +22,7 @@ import ( "github.com/go-pkgz/rest" "github.com/golang-jwt/jwt/v5" + "github.com/go-pkgz/auth/v2/avatar" "github.com/go-pkgz/auth/v2/logger" authtoken "github.com/go-pkgz/auth/v2/token" ) @@ -186,11 +190,18 @@ func (th *TelegramHandler) processUpdates(ctx context.Context, updates *telegram id := th.ProviderName + "_" + authtoken.HashID(sha1.New(), fmt.Sprint(update.Message.Chat.ID)) + // avatarURL embeds the bot token in its path + // (https://api.telegram.org/file/bot{TOKEN}/...). Never store it in + // User.Picture: it would leak through avatar.Proxy.Put logs and, when + // no avatar saver is configured, into the JWT and on to the client. + // Fetch the bytes here and hand them to the avatar store directly. + picture := th.saveTelegramAvatar(ctx, id, avatarURL) + authRequest.confirmed = true authRequest.user = &authtoken.User{ ID: id, Name: update.Message.Chat.Name, - Picture: avatarURL, + Picture: picture, } th.requests.Lock() @@ -294,10 +305,19 @@ func (th *TelegramHandler) LoginHandler(w http.ResponseWriter, r *http.Request) return } - u, err := setAvatar(th.AvatarSaver, *authUser, &http.Client{Timeout: 5 * time.Second}) - if err != nil { - rest.SendErrorJSON(w, r, th.L, http.StatusInternalServerError, err, "failed to save avatar to proxy") - return + // when saveTelegramAvatar already populated Picture with a local proxy + // URL, skip the URL-fetching avatar pipeline. Letting setAvatar run + // here would have it call Proxy.Put which re-fetches Picture; in + // split-DNS / unreachable-internal-Opts.URL deployments that fetch + // fails and the identicon fallback would silently overwrite the + // stored Telegram bytes with an identicon at the same store path. + u := *authUser + if u.Picture == "" { + u, err = setAvatar(th.AvatarSaver, *authUser, &http.Client{Timeout: 5 * time.Second}) + if err != nil { + rest.SendErrorJSON(w, r, th.L, http.StatusInternalServerError, err, "failed to save avatar to proxy") + return + } } claims := authtoken.Claims{ @@ -455,12 +475,12 @@ func (tg *tgAPI) request(ctx context.Context, method string, data any) error { req, err := http.NewRequestWithContext(ctx, "GET", url, http.NoBody) if err != nil { - return fmt.Errorf("failed to create request: %w", err) + return fmt.Errorf("failed to create request: %w", redactBotURLInErr(err)) } resp, err := tg.client.Do(req) if err != nil { - return fmt.Errorf("failed to send request: %w", err) + return fmt.Errorf("failed to send request: %w", redactBotURLInErr(err)) } defer resp.Body.Close() //nolint gosec // we don't care about response body @@ -485,3 +505,101 @@ func (tg *tgAPI) parseError(r io.Reader, statusCode int) error { } return fmt.Errorf("unexpected telegram API status code %d, error: %q", statusCode, tgErr.Description) } + +// avatarContentSaver matches the optional method on AvatarSaver implementations +// that can store already-fetched bytes (avatar.Proxy provides one). Used by the +// Telegram provider to avoid passing a bot-token-bearing URL through the +// URL-fetching avatar pipeline. +type avatarContentSaver interface { + PutContent(userID string, content io.Reader) (string, error) +} + +// saveTelegramAvatar fetches the avatar bytes from a bot-token-bearing Telegram +// URL and stores them via th.AvatarSaver, returning a clean local proxy URL. +// The bot URL is consumed entirely inside this function so it never reaches +// User.Picture, JWT claims, or any debug log of the user object. Returns "" +// when the avatar cannot be saved (no URL, no compatible saver, or fetch +// failure) — the caller treats that as "no picture" and the avatar pipeline +// falls back to identicon as usual. +func (th *TelegramHandler) saveTelegramAvatar(ctx context.Context, userID, avatarURL string) string { + if avatarURL == "" { + return "" + } + // guard against typed-nil *avatar.Proxy. auth.go skips initializing + // res.avatarProxy when Opts.AvatarStore is unset, so AvatarSaver can be + // a non-nil interface wrapping a nil *avatar.Proxy. The type assertion + // below would still succeed (interface satisfaction is structural), but + // PutContent on a nil receiver panics on the first p.Store deref. + if th.AvatarSaver == nil || th.AvatarSaver == (*avatar.Proxy)(nil) { + th.Logf("[WARN] telegram avatar dropped: AvatarSaver is not configured") + return "" + } + saver, ok := th.AvatarSaver.(avatarContentSaver) + if !ok { + // fallback intentionally drops the picture rather than expose the bot + // token; warn so operators can wire a content-aware saver if they want + // telegram avatars saved + th.Logf("[WARN] telegram avatar dropped: configured AvatarSaver does not support direct content save") + return "" + } + req, err := http.NewRequestWithContext(ctx, http.MethodGet, avatarURL, http.NoBody) + if err != nil { + th.Logf("[WARN] telegram avatar fetch request build failed: %v", redactBotURLInErr(err)) + return "" + } + client := &http.Client{Timeout: 5 * time.Second} + resp, err := client.Do(req) + if err != nil { + th.Logf("[WARN] telegram avatar fetch failed: %v", redactBotURLInErr(err)) + return "" + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + th.Logf("[WARN] telegram avatar fetch returned status %d", resp.StatusCode) + return "" + } + // cap body size to protect PutContent from an unbounded upstream response. + // Telegram caps photos at 5 MiB; 10 MiB is generous headroom while still + // bounding worst-case memory. + body, err := io.ReadAll(io.LimitReader(resp.Body, maxTelegramAvatarSize+1)) + if err != nil { + th.Logf("[WARN] telegram avatar read failed: %v", err) + return "" + } + if int64(len(body)) > maxTelegramAvatarSize { + th.Logf("[WARN] telegram avatar dropped: body exceeds %d bytes", maxTelegramAvatarSize) + return "" + } + picture, err := saver.PutContent(userID, bytes.NewReader(body)) + if err != nil { + th.Logf("[WARN] telegram avatar save failed: %v", err) + return "" + } + return picture +} + +const maxTelegramAvatarSize = 10 << 20 + +// botTokenInURLPath matches the bot-token segment of a Telegram URL anchored +// between path slashes ("/botTOKEN/..."). The leading and trailing slashes +// avoid matching unrelated identifiers that happen to start with "bot" (e.g. +// the username "botFather" appearing elsewhere in a log line). Replacement +// preserves the slashes via "/bot/" to keep surrounding URL +// structure intact for diagnostics. +var botTokenInURLPath = regexp.MustCompile(`/bot[A-Za-z0-9:_-]+/`) + +// redactBotURLInErr returns the error with any embedded Telegram bot-token +// segment in URL paths replaced by "bot". net/http's *url.Error +// stringifies as `Op "URL": Err`, so a transport failure on a URL like +// https://api.telegram.org/file/bot/... otherwise prints the token +// verbatim. +func redactBotURLInErr(err error) error { + if err == nil { + return nil + } + redacted := botTokenInURLPath.ReplaceAllString(err.Error(), "/bot/") + if redacted == err.Error() { + return err + } + return errors.New(redacted) +} diff --git a/v2/provider/telegram_test.go b/v2/provider/telegram_test.go index fcaa2d3..a48cd82 100644 --- a/v2/provider/telegram_test.go +++ b/v2/provider/telegram_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "io" "net/http" "net/http/httptest" "net/url" @@ -13,7 +14,10 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/go-pkgz/auth/v2/avatar" + "github.com/go-pkgz/auth/v2/logger" authtoken "github.com/go-pkgz/auth/v2/token" ) @@ -121,15 +125,6 @@ func TestTelegramConfirmedRequest(t *testing.T) { return &upd, nil }, - AvatarFunc: func(ctx context.Context, userID int) (string, error) { - select { - case <-ctx.Done(): - return "", ctx.Err() - default: - } - assert.Equal(t, 313131313, userID) - return "http://t.me/avatar.png", nil - }, SendFunc: func(ctx context.Context, id int, text string) error { select { case <-ctx.Done(): @@ -143,6 +138,20 @@ func TestTelegramConfirmedRequest(t *testing.T) { BotInfoFunc: botInfoFunc, } + avatarSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("png-bytes")) + })) + defer avatarSrv.Close() + m.AvatarFunc = func(ctx context.Context, userID int) (string, error) { + select { + case <-ctx.Done(): + return "", ctx.Err() + default: + } + assert.Equal(t, 313131313, userID) + return avatarSrv.URL + "/file/botSECRET-TOKEN/photo.jpg", nil + } + tg, cleanup := setupHandler(t, m) defer cleanup() @@ -195,6 +204,287 @@ func TestTelegramConfirmedRequest(t *testing.T) { assert.Equal(t, `{"error":"request is not found"}`+"\n", w.Body.String()) } +func TestRedactBotURLInErr(t *testing.T) { + tests := []struct { + name string + in error + want string + }{ + {name: "nil error", in: nil, want: ""}, + {name: "error without bot pattern is unchanged", in: fmt.Errorf("plain error"), want: "plain error"}, + {name: "bot token in URL is redacted", in: fmt.Errorf(`Get "https://api.telegram.org/file/bot1234567:SECRET-TOK_EN-x/photo.jpg": dial tcp`), + want: `Get "https://api.telegram.org/file/bot/photo.jpg": dial tcp`}, + {name: "wrapped error with bot pattern is redacted", + in: fmt.Errorf("wrap: %w", fmt.Errorf(`Get "https://api.telegram.org/bot1234:abc-def_99/getMe": context deadline`)), + want: `wrap: Get "https://api.telegram.org/bot/getMe": context deadline`}, + {name: "non-path bot identifiers (botFather, botanic) are not over-redacted", + in: fmt.Errorf("user @botFather mentioned in chat about botanic gardens"), + want: `user @botFather mentioned in chat about botanic gardens`}, + {name: "bot id without trailing slash is not redacted (anchored regex requires path boundaries)", + in: fmt.Errorf(`reference to bot1234:abc-def_99 in narrative text`), + want: `reference to bot1234:abc-def_99 in narrative text`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := redactBotURLInErr(tt.in) + if tt.in == nil { + assert.Nil(t, got) + return + } + require.NotNil(t, got) + assert.Equal(t, tt.want, got.Error()) + }) + } +} + +func TestSaveTelegramAvatar_TransportErrorDoesNotLeakBotToken(t *testing.T) { + const botToken = "1234567:SECRET-bot-token-do-not-log" + var logBuf strings.Builder + captureLog := logger.Func(func(format string, args ...any) { + fmt.Fprintf(&logBuf, format, args...) + }) + + th := &TelegramHandler{ + AvatarSaver: &mockAvatarSaver{}, + L: captureLog, + } + got := th.saveTelegramAvatar(context.Background(), "user1", + "https://api.telegram.invalid/file/bot"+botToken+"/photo.jpg") + assert.Equal(t, "", got) + assert.NotContains(t, logBuf.String(), botToken, + "bot token must not appear in log even when http.Client.Do returns an error containing the URL") +} + +func TestSaveTelegramAvatar_BotTokenNeverLogged(t *testing.T) { + const botToken = "1234567:SECRET-bot-token-do-not-log" + var logBuf strings.Builder + captureLog := logger.Func(func(format string, args ...any) { + fmt.Fprintf(&logBuf, format, args...) + }) + + avatarSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("png")) + })) + defer avatarSrv.Close() + + t.Run("returns proxy URL via PutContent and does not leak bot token", func(t *testing.T) { + logBuf.Reset() + th := &TelegramHandler{ + AvatarSaver: &mockAvatarSaver{}, + L: captureLog, + } + got := th.saveTelegramAvatar(context.Background(), "user1", + avatarSrv.URL+"/file/bot"+botToken+"/photo.jpg") + assert.Equal(t, "http://example.com/ava12345.png", got) + assert.NotContains(t, logBuf.String(), botToken, "bot token must never appear in logs") + }) + + t.Run("returns empty when saver lacks PutContent and warns operator", func(t *testing.T) { + logBuf.Reset() + th := &TelegramHandler{ + AvatarSaver: legacyAvatarSaver{}, + L: captureLog, + } + got := th.saveTelegramAvatar(context.Background(), "user1", + avatarSrv.URL+"/file/bot"+botToken+"/photo.jpg") + assert.Equal(t, "", got, "Picture must be empty rather than carry bot URL") + assert.NotContains(t, logBuf.String(), botToken, "bot token must never appear in logs") + assert.Contains(t, logBuf.String(), "telegram avatar dropped") + }) + + t.Run("empty avatar URL returns empty without fetch attempt", func(t *testing.T) { + logBuf.Reset() + th := &TelegramHandler{AvatarSaver: &mockAvatarSaver{}, L: captureLog} + got := th.saveTelegramAvatar(context.Background(), "user1", "") + assert.Equal(t, "", got) + }) + + t.Run("non-200 status from Telegram file API returns empty", func(t *testing.T) { + logBuf.Reset() + failSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "expired file_id", http.StatusNotFound) + })) + defer failSrv.Close() + + th := &TelegramHandler{AvatarSaver: &mockAvatarSaver{}, L: captureLog} + got := th.saveTelegramAvatar(context.Background(), "user1", + failSrv.URL+"/file/bot"+botToken+"/photo.jpg") + assert.Equal(t, "", got) + assert.Contains(t, logBuf.String(), "telegram avatar fetch returned status 404") + assert.NotContains(t, logBuf.String(), botToken, "bot token must never appear in logs") + }) + + t.Run("PutContent error returns empty and warns operator", func(t *testing.T) { + logBuf.Reset() + th := &TelegramHandler{ + AvatarSaver: &failingAvatarSaver{}, + L: captureLog, + } + got := th.saveTelegramAvatar(context.Background(), "user1", + avatarSrv.URL+"/file/bot"+botToken+"/photo.jpg") + assert.Equal(t, "", got) + assert.Contains(t, logBuf.String(), "telegram avatar save failed") + assert.NotContains(t, logBuf.String(), botToken, "bot token must never appear in logs") + }) + + t.Run("oversized body is rejected and warns operator", func(t *testing.T) { + logBuf.Reset() + bigSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + buf := make([]byte, 64<<10) + for i := 0; i < (maxTelegramAvatarSize/len(buf))+32; i++ { + if _, err := w.Write(buf); err != nil { + return + } + } + })) + defer bigSrv.Close() + + th := &TelegramHandler{AvatarSaver: &mockAvatarSaver{}, L: captureLog} + got := th.saveTelegramAvatar(context.Background(), "user1", + bigSrv.URL+"/file/bot"+botToken+"/photo.jpg") + assert.Equal(t, "", got) + assert.Contains(t, logBuf.String(), "telegram avatar dropped: body exceeds") + assert.NotContains(t, logBuf.String(), botToken, "bot token must never appear in logs") + }) +} + +type legacyAvatarSaver struct{} + +func (legacyAvatarSaver) Put(authtoken.User, *http.Client) (string, error) { return "", nil } + +// failingAvatarSaver implements both Put and PutContent but its PutContent +// returns an error -- exercises the saver-side failure branch in +// saveTelegramAvatar without going through a real avatar.Proxy. +type failingAvatarSaver struct{} + +func (failingAvatarSaver) Put(authtoken.User, *http.Client) (string, error) { return "", nil } +func (failingAvatarSaver) PutContent(string, io.Reader) (string, error) { return "", fmt.Errorf("disk full") } + +func TestSaveTelegramAvatar_TypedNilAvatarSaverDoesNotPanic(t *testing.T) { + avatarSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("png-bytes")) + })) + defer avatarSrv.Close() + + var nilProxy *avatar.Proxy + th := &TelegramHandler{AvatarSaver: nilProxy, L: logger.NoOp} + got := th.saveTelegramAvatar(context.Background(), "user1", avatarSrv.URL+"/file/botSECRET/photo.jpg") + assert.Equal(t, "", got, "typed-nil AvatarSaver must short-circuit before fetch + PutContent") +} + +func TestTelegramLoginHandler_DoesNotOverwriteSavedAvatar(t *testing.T) { + dir := t.TempDir() + store := avatar.NewLocalFS(dir) + proxy := &avatar.Proxy{ + Store: store, + URL: "http://127.0.0.1:1", + RoutePath: "/avatar", + L: logger.NoOp, + } + + avatarSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("png-bytes")) + })) + defer avatarSrv.Close() + + const tok = "registered-token" + th := &TelegramHandler{ + ProviderName: "telegram", + Telegram: &TelegramAPIMock{ + AvatarFunc: func(context.Context, int) (string, error) { + return avatarSrv.URL + "/file/botSECRET/photo.jpg", nil + }, + SendFunc: func(context.Context, int, string) error { return nil }, + BotInfoFunc: botInfoFunc, + }, + AvatarSaver: proxy, + L: logger.NoOp, + SuccessMsg: "ok", + } + th.requests.data = map[string]tgAuthRequest{ + tok: {expires: time.Now().Add(time.Hour)}, + } + + updates := &telegramUpdate{} + require.NoError(t, json.Unmarshal([]byte(fmt.Sprintf(getUpdatesResp, tok)), updates)) + th.processUpdates(context.Background(), updates) + + got := th.requests.data[tok] + require.NotNil(t, got.user, "request must be confirmed") + pictureFromUpdate := got.user.Picture + require.NotEmpty(t, pictureFromUpdate, "telegram path must populate Picture via PutContent") + require.True(t, strings.HasPrefix(pictureFromUpdate, "http://127.0.0.1:1/avatar/"), + "Picture should point at proxy URL, got: %s", pictureFromUpdate) + + jwtSvc := authtoken.NewService(authtoken.Opts{ + SecretReader: authtoken.SecretFunc(func(string) (string, error) { return "secret", nil }), + }) + th.TokenService = jwtSvc + rr := httptest.NewRecorder() + req, err := http.NewRequest("GET", "/login?token="+tok, http.NoBody) + require.NoError(t, err) + th.LoginHandler(rr, req) + require.Equal(t, http.StatusOK, rr.Code) + assert.Contains(t, rr.Body.String(), pictureFromUpdate, "Picture must round-trip unchanged from saveTelegramAvatar") +} + +// TestTelegramProcessUpdates_BotTokenNeverInUserPicture is the regression-style +// property test the user asked for: drives the full update-processing flow with +// a TelegramAPI mock that hands back a URL containing a bot-token-like marker, +// then asserts (a) the marker never lands in authRequest.user.Picture and +// (b) the marker never appears in any captured log line. +func TestTelegramProcessUpdates_BotTokenNeverInUserPicture(t *testing.T) { + const botToken = "secret-bot-token-marker" + var logBuf strings.Builder + var logMu sync.Mutex + captureLog := logger.Func(func(format string, args ...any) { + logMu.Lock() + defer logMu.Unlock() + fmt.Fprintf(&logBuf, format+"\n", args...) + }) + + avatarSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("png")) + })) + defer avatarSrv.Close() + + m := &TelegramAPIMock{ + AvatarFunc: func(context.Context, int) (string, error) { + return avatarSrv.URL + "/file/bot" + botToken + "/photo.jpg", nil + }, + SendFunc: func(context.Context, int, string) error { return nil }, + BotInfoFunc: botInfoFunc, + } + + const tok = "registered-token" + th := &TelegramHandler{ + ProviderName: "telegram", + Telegram: m, + AvatarSaver: &mockAvatarSaver{}, + L: captureLog, + SuccessMsg: "ok", + } + th.requests.data = map[string]tgAuthRequest{ + tok: {expires: time.Now().Add(time.Hour)}, + } + + updates := &telegramUpdate{} + err := json.Unmarshal(fmt.Appendf(nil, getUpdatesResp, tok), updates) + require.NoError(t, err) + + th.processUpdates(context.Background(), updates) + + got := th.requests.data[tok] + require.NotNil(t, got.user, "auth request should have been confirmed") + assert.NotContains(t, got.user.Picture, botToken, "User.Picture must not carry the bot token") + + logMu.Lock() + logged := logBuf.String() + logMu.Unlock() + assert.NotContains(t, logged, botToken, "no log line must contain the bot token") +} + func TestTelegramLogout(t *testing.T) { m := &TelegramAPIMock{ GetUpdatesFunc: func(ctx context.Context) (*telegramUpdate, error) {