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) {