fix(telegram): never expose bot token in avatar URL

[paskal]

Summary

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 bot token is a bearer credential for the entire Telegram bot API. The URL flowed into User.Picture and from there:

• Into avatar.Proxy.Put debug logs (both load-success and load-failure lines).
• Into the JWT claims and the user JSON returned to the browser when no AvatarSaver was configured.

Either path leaks the bot token to log-aggregators, anyone who can read the JWT (the user themselves on the device, plus anyone intercepting browser devtools), or third-party observability.

Change

Two parts, both v1 and v2 in single PR:

1. avatar/avatar.go

• Add redactAvatarURL(raw string) string helper — hostname only.
• Use it in the two [DEBUG] saved avatar from %s / [DEBUG] failed to fetch avatar from the orig %s lines so attacker-/credential-bearing path/query never reaches logs.
• Add Proxy.PutContent(userID string, content io.Reader) (avatarURL string, err error) so providers that fetch with credentials can hand the bytes to the store directly, bypassing the URL-fetch path.

2. provider/telegram.go

• In processUpdates, never assign the bot URL to User.Picture. Pass the URL to a new saveTelegramAvatar method that fetches the bytes server-side (the bot URL stays inside one function call), then stores the bytes via the new content-saver interface, returning a clean local proxy URL.
• If the configured AvatarSaver does not implement PutContent (custom external implementation), the avatar is dropped and a [WARN] is logged — we never expose the token to keep the avatar feature working.

Test

• TestSaveTelegramAvatar_BotTokenNeverLogged — unit-level coverage of the helper across success, fallback-without-PutContent, and empty-URL paths.
• TestTelegramProcessUpdates_BotTokenNeverInUserPicture — regression-style property test: drives processUpdates with a TelegramAPI mock that returns a URL containing a secret-bot-token-marker; asserts the marker never lands in user.Picture and never appears in any captured log line.

Confirmed locally that reverting the saveTelegramAvatar redirection makes the property test fail with:

"http://127.0.0.1:NNNNN/file/botsecret-bot-token-marker/photo.jpg"
    should not contain "secret-bot-token-marker"
Messages: User.Picture must not carry the bot token

Full go test -race ./... green in both modules; golangci-lint run --new-from-rev=master 0 issues.

[coveralls]

Coverage Report for CI Build 25587623060

Coverage increased (+0.03%) to 84.897%

Details

• Coverage increased (+0.03%) from the base build.
• Patch coverage: 17 uncovered changes across 2 files (79 of 96 lines covered, 82.29%).
• No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
v2/provider/telegram.go	73	60	82.19%
v2/avatar/avatar.go	23	19	82.61%

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	3410
Covered Lines:	2895
Line Coverage:	84.9%
Coverage Strength:	7.73 hits per line

---

💛 - Coveralls

[umputun]

two real bugs found that need to land before merge, both mirrored across v1 and v2:

1. typed-nil *avatar.Proxy panics on Telegram login. When auth.NewService is created without Opts.AvatarStore, auth.go:135-145 skips initializing res.avatarProxy, so s.avatarProxy ends up as a typed-nil *avatar.Proxy and flows into Params.AvatarSaver. The new type assertion at provider/telegram.go:517 (saver, ok := th.AvatarSaver.(avatarContentSaver)) returns ok=true because interface satisfaction is structural and *avatar.Proxy has the right method set regardless of nil-ness. Then saver.PutContent(userID, resp.Body) invokes the method on a nil receiver, and PutContent (avatar/avatar.go:88) dereferences p.Store → panic. The legacy setAvatar path at provider/service.go:75 had an explicit guard if ava == nil || ava == (*avatar.Proxy)(nil); the new path lost it. Same code in v2.

   Fix: replicate that guard before the assertion, e.g.

   if th.AvatarSaver == nil || th.AvatarSaver == (*avatar.Proxy)(nil) {
       return ""
   }
   saver, ok := th.AvatarSaver.(avatarContentSaver)

   And add a regression test: var p *avatar.Proxy; th.AvatarSaver = p then drive processUpdates, assert no panic + empty Picture + the existing [WARN] log line.

2. double avatar pipeline silently overwrites the real Telegram avatar with an identicon. After processUpdates stores the bytes via PutContent and sets Picture = "<local-proxy-URL>", LoginHandler at provider/telegram.go:306 still unconditionally calls setAvatar(th.AvatarSaver, *authUser, ...), which calls Proxy.Put. That triggers p.load(u.Picture, client), which HTTP-GETs the local URL. In split-DNS / unreachable-internal-Opts.URL deployments, the GET fails, Proxy.Put falls back to genIdenticon at avatar.go:66, and overwrites the Telegram avatar bytes at the same store path with the identicon. The JWT then carries a Picture URL pointing at the identicon. Tests miss it because mockAvatarSaver.Put (provider/oauth2_test.go:365) returns the same canned URL regardless of input. Same code in v2.

   Fix: skip setAvatar in LoginHandler when Picture was already set by saveTelegramAvatar. For example, add a pictureProxied bool on tgAuthRequest, or short-circuit when authUser.Picture is already prefixed by the local proxy URL. Add a test with a real avatar.Proxy whose URL is unreachable to confirm the Telegram avatar is not replaced.

other findings, not blocking:

3. coverage gaps on realistic error paths. The 13 uncovered new lines include the non-200 status branch from the Telegram file API (expired file_id, insufficient bot rights) and the PutContent/Store.Put error branches (disk full, store down). Worth adding two negative-path subtests, ~30 lines each, copy-paste-ready from the existing happy-path test.

4. body-size cap. saveTelegramAvatar streams resp.Body unbounded into resize. Same gap exists in Proxy.Put. Suggest a separate follow-up to add http.MaxBytesReader to both fetch paths symmetrically rather than only here.

5. test fixture lint regression. avatar/avatar_test.go:77 has a deliberate https://x:y@example.com/path?q=1 to verify redactAvatarURL strips userinfo. The gosec G101 check flags it as a hardcoded credential. Either annotate with // #nosec G101 -- deliberate test fixture for userinfo redaction or rewrite the assertion to use a builder that avoids the literal.

6. minor nits:

   • redactAvatarURL doc says it handles "secrets in path or query" but the impl returns hostname only. Tighten doc.
   • bot[A-Za-z0-9:_-]+ regex over-matches words like botFather. Consider /bot[A-Za-z0-9:_-]+/ for path-anchored matching, or update the doc to acknowledge the broader sweep.
   • dumbAvatarSaver test type, informal name; legacyAvatarSaver reads better.
   • the new &http.Client{Timeout: 5 * time.Second} is allocated per call; the existing tgAPI.client could be reused.
   • v1 telegram_test.go has 7 inline comments v2 lacks; mirror or strip.

7. README behavior-change framing. The "Telegram avatar storage" section honestly documents the new contract but undersells the breaking change for custom AvatarSaver implementers. Pre-PR they got Telegram avatars (with embedded token), post-PR they don't. Worth a release-note line explicitly calling out the behavior change for that case, not just a README paragraph.

design choices noted as fine to keep:

• the avatarContentSaver optional-interface duck-typing pattern is idiomatic Go (mirrors http.Flusher, io.WriterTo). Promoting to an exported AvatarContentSaver would lock the contract into the public API for a one-off use case. The README documents the required signature.
• the silent-drop fallback (when AvatarSaver doesn't implement PutContent) with [WARN] is the right trade-off vs leaking the token; documented in README.

[paskal]

addressed both blocking items and most of the non-blocking ones in the last push:

blocking, both fixed in v1+v2:

1. typed-nil *avatar.Proxy panic guard added at provider/telegram.go:saveTelegramAvatar -- explicit if th.AvatarSaver == nil || th.AvatarSaver == (*avatar.Proxy)(nil) before the type assertion. regression test TestSaveTelegramAvatar_TypedNilAvatarSaverDoesNotPanic wires var p *avatar.Proxy; th.AvatarSaver = p and drives saveTelegramAvatar with a reachable URL (so the function would proceed past fetch into PutContent on a nil receiver and panic at p.Store deref without the guard).

2. double-pipeline overwrite fixed in LoginHandler -- the unconditional setAvatar call is now skipped when Picture was already populated by saveTelegramAvatar. regression test TestTelegramLoginHandler_DoesNotOverwriteSavedAvatar uses a real avatar.Proxy with an unreachable URL: "http://127.0.0.1:1"; without the fix the second-pass fetch fails and Proxy.Put silently identicons over the just-stored bytes.

non-blocking, folded in:

3. negative-path coverage added to TestSaveTelegramAvatar_BotTokenNeverLogged -- two new subtests for the non-200 status branch (Telegram file API returning 404 for expired file_id) and the PutContent error branch (new failingAvatarSaver test type whose PutContent returns "disk full"). both assert no token leak in logs.

4. // #nosec G101 -- deliberate test fixture for userinfo redaction annotation on the https://x:y@example.com/... literal in avatar/avatar_test.go:77 (and v2 mirror).

6a. redactAvatarURL doc rewritten -- now explicitly says "returns the hostname only, dropping scheme, userinfo, path, query and fragment" rather than the misleading "preserves secrets" framing.

7. README "Telegram avatar storage" section now has an explicit behaviour-change callout for custom AvatarSaver implementers: pre-PR they got the bot-token-bearing URL via Put, post-PR they don't, and to keep saving Telegram avatars they must implement PutContent.

deferred to follow-ups: body-size cap (#4 -- intentionally separate per your suggestion, touches both telegram and Proxy.Put symmetrically); regex/naming/client-reuse nits (#6b-e -- bikeshed cleanup, separate PR if useful).

go fix + race + lint clean in both modules.

[paskal]

folded all the non-blocking nits into this PR:

1. avatar.Proxy.Put body-size cap (v1+v2) — Proxy.load now buffers the body through io.ReadAll(io.LimitReader(resp.Body, maxAvatarFetchSize+1)) (10 MiB cap), errors on overflow, and Proxy.Put falls back to identicon. Symmetric to the saveTelegramAvatar cap below, addresses review item Auth/Token naming mismatch #4.

2. saveTelegramAvatar body-size cap (v1+v2) — same cap (maxTelegramAvatarSize = 10 << 20), wrapped before PutContent. Test oversized body is rejected and warns operator simulates ~12 MiB upstream. Addresses review item Auth/Token naming mismatch #4.

3. Regex anchoring (v1+v2) — bot[A-Za-z0-9:_-]+ → /bot[A-Za-z0-9:_-]+/ so identifiers like botFather or botanic gardens in narrative log text are no longer over-redacted; replacement preserves the slashes via /bot<redacted>/. New test cases non-path bot identifiers (botFather, botanic) are not over-redacted and bot id without trailing slash is not redacted (anchored regex requires path boundaries) pin the new behaviour. Existing redaction cases still pass byte-for-byte. Addresses review item Fix dev oauth #6 (regex nit).

4. dumbAvatarSaver → legacyAvatarSaver rename in v1+v2 telegram_test.go. Addresses review item Fix dev oauth #6 (naming nit).

skipped intentionally:

• HTTP client reuse on saveTelegramAvatar (review item Fix dev oauth #6 client-reuse) — would require threading a *http.Client through the handler struct; API surface for one-call-per-login optimisation.
• v1 telegram_test.go inline comment parity with v2 (review item Fix dev oauth #6) — bikeshed, not worth a touch.

go fix + race + lint clean in both modules.

[umputun]

both blockers fixed with the exact-shape regression tests I asked for:

1. typed-nil guard at provider/telegram.go:359 (and v2 mirror) matches the legacy setAvatar shape; TestSaveTelegramAvatar_TypedNilAvatarSaverDoesNotPanic drives processUpdates with var p *avatar.Proxy and asserts no panic.
2. LoginHandler now skips setAvatar when Picture was already proxied; TestTelegramLoginHandler_DoesNotOverwriteSavedAvatar uses a real avatar.Proxy with unreachable URL to pin the identicon-overwrite scenario.

non-blocking items: all 5 folded in across the two pushes. Body-size cap is symmetric in Proxy.load + saveTelegramAvatar (both v1 and v2, 10 MiB, +1 overflow trick on LimitReader, [WARN] on drop). Regex anchored to /bot[A-Za-z0-9:_-]+/ with the botFather/botanic and bot1234:abc-def_99 pin tests. Rename + #nosec + doc rewrite + README behavior callout all in. Closing PR #288 by folding it here is the right call.

skipped items (HTTP client reuse, v1/v2 inline-comment parity) are explicitly bikeshed and the justification is correct - reusing tgAPI.client would push API surface for one-call-per-login.

LGTM, thx.