Make the Telegram API base URL configurable, and close what that opens

[paskal]

The Telegram provider hard-codes https://api.telegram.org, which means nothing can stand in for the API: no proxy in front of it, and no stub for an end-to-end test. NewTelegramAPIWithBaseURL makes the base configurable, and every request goes through it, avatar downloads included, so the production path stays the code under test. An empty base falls back to the public API and existing callers are unaffected.

That is the feature. Most of this PR is what the feature turned out to require, because moving the base off a constant moves a token-bearing URL across a boundary that used to be frozen, and the answers now come from a host the library does not control.

Six ways the bot token could leave the process, found across your review and two adversarial passes, each fixed with tests in both modules and each mutation-checked.

• The error text on a non-200. parseError interpolated the upstream description raw, so a proxy answering 502 with the request URI in it published the token into the log. Scrubbed by token value, not by URL shape, since the leak is whatever the upstream echoed.
• Encoded echoes defeat an exact substring scrub. A proxy percent-encoding the URI got the token past both the substitution and the /bot<token>/ regex. It now scrubs the escaped forms too and then checks a decoded, case-folded copy, withholding the text entirely if the token is still recoverable. Decoding repeats until the text stops changing, because one pass turns %253A back into %3A.
• A 200 response bypassed every sanitiser. BotInfo accepted whatever the upstream put in result.username, and LoginHandler returns that to an unauthenticated caller in its bot field, so an upstream echoing the request URI published the token through a public endpoint. Every guard was on the error path; this is the success path. The username now has to match Telegram's own shape.
• Redirects hand the token to the destination. Go copies the previous URL into Referer on every hop except https-to-http, and both URL forms carry the token in the path. Verified against net/http with two TLS servers rather than inferred. The public API does not redirect, so these requests now refuse to follow one.
• http://:9000 passed validation, since u.Host is non-empty while u.Hostname() is empty, and an unspecified remote resolves to the local machine.
• The validator checked one string and the code used another. It inspected the parsed URL while requests were built by formatting the original, so a trailing ? set ForceQuery with RawQuery empty and sent GET /tg?/bot<token>/getMe, putting the token in the query string. A trailing # was the mirror image, burying every request in a fragment that is never sent. The accepted value is now rebuilt from the parsed URL.

One behaviour change to accept or reject. Refusing redirects means a proxy that answers with one stops working, where before it worked and leaked. I took refusing as the safer default; making it configurable is a one-line diff.

One place I did not follow your review, deliberately. You asked for "advertised but nil, drop the avatar rather than silently switch transport". That holds only if the capability is advertised conditionally, and avatarHTTPClient is a plain method on *tgAPI, so every instance advertises it. Implemented literally, it dropped Telegram avatars for every caller of NewTelegramAPI, which is all of them, including the repo's own _example/main.go and the README snippet. The nil now falls through to the default client. If you would rather have the behaviour you described, the way to get it is a distinct type from the new constructor so a plain tgAPI fails the assertion; that is a larger diff and the same decision you flagged as one to take deliberately.

Also in here: the constructor rejects a nil client, trims before the empty check so "/" does not become an empty base, and TelegramAPIBaseURL no longer sits between TelegramHandler's doc comment and the type.

[coveralls]

Coverage Report for CI Build 32996973277

Coverage increased (+0.6%) to 86.663%

Details

• Coverage increased (+0.6%) from the base build.
• Patch coverage: 6 uncovered changes across 1 file (115 of 121 lines covered, 95.04%).
• No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
provider/telegram.go	121	115	95.04%

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	3809
Covered Lines:	3301
Line Coverage:	86.66%
Coverage Strength:	9.78 hits per line

---

💛 - Coveralls

[umputun]

last commit only, the other two are #315's.

shape is right, but making the host caller-supplied moves a token-bearing URL across a boundary that used to be frozen at api.telegram.org, and three things behind it weren't built for that.

bot token reaches the error text on a non-200 (:505-506, :523, same in v2). request redacts its own two paths then returns parseError bare, and parseError interpolates the upstream description raw. A proxy answering 502 with {"description":"proxy failed while forwarding /bot<token>/getMe"} gives:

unexpected telegram API status code 502, error: "proxy failed while forwarding /bot1234567:SECRET-TOK_EN-x/getMe"

which reaches the log via Run, GetUpdates and Send. Pls redact the whole error leaving request, scrubbing tg.token rather than trusting the URL-shape regex, since the leak is whatever the upstream echoed. Test with a proxy that echoes the request URI. The :529 comment says the avatarContentSaver hook exists to keep token-bearing URLs out of the avatar path, so this is a hole in a guard that was meant to be there.

base URL isn't validated. TrimSuffix is the whole guard, so https://api.telegram.org@evil.tld resolves to host evil.tld and ships /bot<token>/... there; http:// is accepted too. Config mistake rather than attacker, but same shape as the github provider guard in #311 today. Constructor is unmerged, so return an error: absolute http(s), host required, reject userinfo/query/fragment/opaque, keep an optional path prefix. Reject a nil client while you're there.

avatar download bypasses the caller's client. Bot methods use tg.client at :499, saveTelegramAvatar builds &http.Client{Timeout: 5 * time.Second} at :568. That drops custom CA, client certs, pinning, CheckRedirect and proxy policy, and the default transport picks up proxy env vars instead - so a proxy whose TLS material lives on the passed-in client gives working bot calls and identicons, one [WARN] line. Makes the new godoc's "production path is otherwise identical" untrue for the path the feature exists for.

no interface change needed, the optional-capability pattern is already at :529:

type telegramAvatarClientProvider interface{ avatarHTTPClient() *http.Client }

on tgAPI only, type-asserted in saveTelegramAvatar. Assertion fails, keep today's client; advertised but nil, drop the avatar rather than silently switch transport. TelegramAPI untouched, moq still compiles. For the 5s cap use a child context.WithTimeout and call the client unchanged, so its Transport, CheckRedirect and Jar survive.

one thing to decide rather than fall into: if avatarHTTPClient returns tg.client, plain NewTelegramAPI also starts routing avatar downloads through its caller's client, which is a real behaviour change for existing users. Either take it as a consistency fix and say so, or give tgAPI a separate avatarClient field only the new constructor sets.

test: httptest.NewTLSServer, build with ts.Client(), drive saveTelegramAvatar with a content saver, assert the server saw /file/bot... and the bytes were stored. The current one stops at the URL string, which is why it passes.

small: the empty check runs before the trim, so "/" becomes an empty base and URLs come out as /bot<token>/getMe - trim first. And const TelegramAPIBaseURL sits between // TelegramHandler implements login via telegram and the type, so the type lost its godoc.

the redaction regex itself holds under a custom base, 14 shapes through it and nothing leaked on the build or transport paths; the response path above is the only gap. Both modules identical apart from the jwt v4/v5 block, build and lint clean in both, 13 Telegram tests pass. The two provider/ failures are #315's byte counts.

[paskal]

All three addressed, plus the two small ones.

Token redaction. You are right that redacting by URL shape cannot hold when the leak is whatever the upstream echoed. The token itself is now scrubbed from anything leaving request, so parseError's interpolation of the upstream description is covered whatever it contains. Test uses a server that echoes its own request URI into the description, which is your reproduction; it fails without the scrub.

Base URL validation. Constructor returns an error now: absolute http or https, host required, no userinfo, query, fragment or opaque part, path prefix allowed. Nil client refused. Table covers https://api.telegram.org@evil.tld, a missing scheme, ftp://, the opaque form, an empty host, and query and fragment. The trim also runs before the empty check, so "/" falls back rather than becoming an empty base.

Avatar client. Done through the capability pattern you pointed at, with the 5s cap moved to a child context so Transport, CheckRedirect and Jar survive.

On the decision you flagged rather than falling into: I took the separate-field option. avatarClient is set only by NewTelegramAPIWithBaseURL, so NewTelegramAPI callers keep the transport they have always had on that path and there is no behaviour change for existing users. Advertised-but-nil drops the avatar, as you suggested. The consistency fix is defensible too, but it is a change existing callers did not ask for and this PR is not the place to make it silently.

The test drives saveTelegramAvatar against an httptest.NewTLSServer that only the supplied client can reach, and asserts the stored bytes rather than the URL string. Reverting to the self-built client fails it on the dropped avatar.

Small ones: the const no longer sits between the TelegramHandler godoc and the type.

Both modules build, lint clean at v2.12.2, all Telegram tests pass in each. TestCustomProvider and TestDevProvider still fail here, but on this machine for the port reason, not the byte counts, which are fixed in #315.

[paskal]

Rebased onto master. This was stacked on #315, which I have now closed, so it was carrying that feature's commits and would have merged PartitionedCookies in alongside the Telegram change. It is two commits and nothing else now, and the diff has no cookie code in it.

Why #315 went: your review plus an end-to-end test I should have run first left it fixing something already fixed. With AUTH_SEND_JWT_HEADER=true, remark42's own client already writes those cookies with Partitioned, so cross-domain persistence for the non-OAuth flows works on current master. Against your finding that enabling the flag breaks working OAuth sessions wherever third-party cookies are still permitted, it was not a good trade.

The Go 1.27 byte-count assertions from that PR are split out as #317, test-only, and this branch no longer carries them, so TestCustomProvider and TestDevProvider fail here on 1.27 until that one lands. They pass on 1.26, which is what both workflows pin.

Everything from your review of this PR is unchanged and still in: token scrubbed from anything leaving request, base URL validated with an error-returning constructor, avatar download on the caller's client through the capability pattern with the timeout moved to a context.

[paskal]

Found a regression in my own branch while reviewing it, and pushed the fix.

*tgAPI satisfies telegramAvatarClientProvider whichever constructor built it, because the method is on the type. So an API built with NewTelegramAPI answered the capability with a nil avatarClient, and saveTelegramAvatar read that as "the caller pointed the API elsewhere without giving me a client for it" and dropped the avatar with a warning. NewTelegramAPIWithBaseURL is new in this branch, so NewTelegramAPI is what every existing caller uses, remark42 included: the effect was that Telegram avatars stopped being saved for all of them. On master the download always ran against a default http.Client, so this was a straight behaviour regression, and it contradicted the comment on the field, which says the nil exists so those callers keep the transport they have always had.

The nil now falls through to the default client instead of aborting. A caller who supplied one still gets theirs.

The gap that let it through is the plain one: the branch added a test for the constructor the branch adds and none for the constructor that already existed. Both modules now have TestTelegram_AvatarDownloadFallsBackToTheDefaultClient next to TestTelegram_AvatarDownloadUsesTheSuppliedClient. Mutation-checked: restoring the drop makes the new test fail and leaves the supplied-client one passing, so the two cover different paths.

Still yours to decide, and unchanged by this: whether the avatar download should reuse the API's client at all. You flagged it as something to decide instead of fall into, and I have not tried to settle it here. If you would rather the capability interface went away and the download always used a default client, that is a smaller diff than what is there now and I will cut it.

[paskal]

Two more holes from the same review pass, both in the safeguards the second commit added, both now fixed.

The validator checked one string and the code used another. validateTelegramBaseURL inspected the parsed URL, while request and Avatar built the request by formatting the original string, so any shape that parses to something empty but serializes to something else went between them. https://proxy.example.com/tg? passed, because Go records the trailing ? as ForceQuery and leaves RawQuery empty, and the request then went out as GET /tg?/bot<token>/getMe: the bot token in the query string, which is the part access logs, CDNs and referrer headers capture most eagerly. That is the same class of exposure the validation was added to prevent. https://proxy.example.com/tg# is the mirror image: the whole /bot<token>/getMe becomes a fragment and is never sent, so every call hits /tg and the operator gets a 404 loop with nothing pointing at the base. Both are rejected now, and the accepted value is rebuilt from the parsed URL so the string that was checked is the string that is used.

Exact-substring redaction cannot hold when the upstream picks the encoding. redactToken matched tg.token literally, so a proxy echoing the percent-encoded request URI produced forwarding %2Fbot1234567%3ASECRET…%2FgetMe, which defeats the substitution and also misses botTokenInURLPath, since % is outside its character class. The token is fully recoverable from that. It now scrubs the query- and path-escaped forms as well, and then checks a decoded, case-folded copy of the result: if the token is still recoverable the text is withheld rather than forwarded. Withholding is the right default here, because no fixed set of substitutions covers an encoding the other side chooses.

So the claim in the earlier commit message, that the token is scrubbed from whatever request produces, was true only for a byte-identical echo. It is closer to true now, and the fallback is what makes the difference rather than a longer list of encodings.

Both fixes carry tests in both modules, and both are mutation-checked: removing the ForceQuery and trailing-# checks fails TestTelegram_APIBaseURLRejectsShapesThatMoveTheToken and nothing else, and reverting the redaction hardening fails TestTelegram_APIErrorDoesNotLeakAnEncodedToken and nothing else.

[paskal]

An adversarial review pass over this branch turned up four more ways the bot token leaves the process. All four are fixed and pushed, each with a test in both modules and each mutation-checked. Flagging them individually because the first is the one I would want a second opinion on, and the last carries a behaviour change that is yours to accept or reverse.

A redirect hands the token to the destination. Go copies the previous URL into Referer on every redirect hop except https-to-http, and both URL forms here carry the token in the path, so a proxy that answers with a redirect gives another host the bot token, normally straight into its access log. I verified this against net/http rather than reasoning about it: two TLS test servers, and the target receives Referer: https://host/bot1234567:SECRET-TOK/getMe. validateTelegramBaseURL constrains only where the first request goes, so it cannot help here.

A 200 response can publish the token through a public endpoint. BotInfo accepted whatever the upstream put in result.username, and LoginHandler returns that to an unauthenticated caller in its bot field. An upstream that echoes the request URI therefore publishes the token to anyone who calls /login. Every sanitiser in the branch was on the error path; this is the success path. The username now has to match Telegram's own shape.

The decode check I added last round only decoded once. A double-encoded echo came back as a single-encoded one and stayed just as readable, so %253A defeated it. It now decodes until the text stops changing, with a cap.

http://:9000 passed validation. u.Host is non-empty for that while u.Hostname() is empty, and an unspecified remote resolves to the local machine, so the token could be sent to an unrelated local listener. It checks Hostname() now.

I also routed the two transport-error paths in request through redactToken as well as redactBotURLInErr, since the regex only matches a token sitting inside a /bot.../ path segment and a transport error need not put it there.

The decision I would like you to make. Refusing redirects means a proxy that answers with one stops working, where previously it worked and leaked. I took refusing as the safer default, since the public API does not redirect and a caller-supplied base is precisely where an unexpected one would come from, but it is a behaviour change and making it configurable is a one-line diff if you would rather.

Two things I did not act on. The avatar URL returned by Avatar still contains the token by construction, which is Telegram's design and is consumed inside saveTelegramAvatar without reaching User.Picture. And the capability interface for the avatar client is still the shape you flagged as something to decide instead of fall into; I have not tried to settle that here.

[paskal]

One thing in your review I have deliberately not followed, and it deserves to be called out rather than left for you to spot.

You wrote: Assertion fails, keep today's client; advertised but nil, drop the avatar rather than silently switch transport. I implemented exactly that, and it turned out to break every existing caller.

The reason is that the prescription holds only if the capability is advertised conditionally. avatarHTTPClient is a plain method on *tgAPI, so every tgAPI advertises it, including one from NewTelegramAPI where I leave avatarClient nil on purpose. The assertion therefore always succeeds and "advertised but nil" stops meaning "misconfigured" and starts meaning "built by the plain constructor", which is every caller today since NewTelegramAPIWithBaseURL is new here. The effect was that Telegram avatars silently stopped being saved for all of them, including the repo's own _example/main.go and the README's Telegram snippet, and it contradicted the comment on the field, which says the nil exists so those callers keep the transport they have always had.

So the nil now falls through to the default client instead of aborting, which is your "assertion fails, keep today's client" branch reached by a different route. A caller who supplied a client still gets theirs.

If you would rather have the behaviour you described, the way to get it is to advertise conditionally: give NewTelegramAPIWithBaseURL its own type that embeds tgAPI and carries the method, so a plain tgAPI fails the assertion and only a misconfigured custom-base API reaches the nil branch. That is a slightly larger diff and I did not want to make that choice for you, since it is the same decision you flagged as one to take deliberately.

Both branches are covered now: TestTelegram_AvatarDownloadUsesTheSuppliedClient and TestTelegram_AvatarDownloadFallsBackToTheDefaultClient, in both modules, mutation-checked against each other.

[umputun]

all five from the last round are in: response-path redaction, base URL validation with the error return, the avatar client through the capability with avatarClient set only by the new constructor, the context cap, the TLS test. The trim-before-empty and the godoc placement too. Two things left, one of them blocking.

tokenRecoverable fails open on malformed escaping (provider/telegram.go, same in v2)

it gives up on the first unescape error and returns false, meaning not recoverable, without having looked for the token. The PathUnescape fallback cannot rescue that: both functions reject the same inputs, the mode-specific errors are for encodeHost/encodeZone and the only query/path difference is +. So one bare % anywhere in the upstream text turns the guard off.

that is reachable, because the text is whatever the proxy echoed:

{"description":"forwarding %252Fbot1234567%253ASECRET-TOK_EN-x%252FgetMe at 100% load"}

ReplaceAll misses, token is double-encoded. redactBotURLInErr misses, % is outside /bot[A-Za-z0-9:_-]+/. tokenRecoverable aborts on % l. redactToken sees the message unchanged and returns the error as-is, and it goes out through BotInfo/GetUpdates/Send into the log at :98, :182, :216. Two decodes get the token back.

TestTelegram_APIErrorDoesNotLeakADoubleEncodedToken passes only because its description has no stray %. Append at 100% load to it and it fails.

fix is to fail closed on any decode error. After malformed escaping the function cannot prove the token is absent, so withholding is the only honest answer. Same for the five-layer cap when the text is still changing. Worth pinning with a malformed-percent-plus-double-encoded case in both modules.

redirect refusal overrides a policy the caller set

noRedirect is on request as well as the avatar path, so it replaces CheckRedirect for every call, including NewTelegramAPI callers. :700 says the client's CheckRedirect survives and :369 says the caller's redirect policy is why the avatar download uses their client; :753 replaces it.

refusing is right, Go copies the previous URL into Referer and every URL here carries the token. But make it the default rather than the rule: if the supplied client has a nil CheckRedirect, install the refusal; if it has a hook, that is the operator's explicit policy on a base URL he chose, leave it. Say beside the constructor that allowing a redirect can expose the token through Referer unless the hook strips or refuses it. Test both, and for the second one assert the hook is actually called rather than just still on the struct, since a wrapper in front of it would satisfy the weaker check. That also puts :700 and :369 back in agreement with the code.

minor: validateTelegramBaseURL puts the raw base URL in every rejection, so https://user:pass@proxy.example is rejected for carrying credentials and the error then carries them, into whatever logs the constructor failure. Same for the query case. Name the rejected property, or show a sanitised host, rather than echoing the value.

[paskal]

All three addressed in 591b0c4, both modules.

tokenRecoverable failed open. It now withholds on any decode error, and on the five-layer cap running out while the text is still changing: after either, absence was never established, and withholding is the only answer that cannot leak. TestTelegram_APIErrorWithheldWhenEscapingIsMalformed carries both halves of your example, a double-encoded request URI and a bare %. Against the previous code it fails with the token in the clear:

"unexpected telegram API status code 502, error: \"forwarding
%252Fbot1234567%253ASECRET-TOK_EN-x%252FgetMe at 100% load\"" does not contain "text withheld"

The redirect refusal is now a default rather than a rule. A client arriving with CheckRedirect keeps it; one that expressed nothing gets the refusal. TestTelegram_RedirectPolicyOfTheCallerIsKept asserts the caller's hook ran, by counting its calls rather than checking it is still on the struct, and TestTelegram_APIDoesNotFollowRedirects still passes, so the default is intact. Removing the two-line exemption fails the first and leaves the second green, which is the split you would want. The godoc beside the constructor now says what a permitted redirect exposes through Referer, so :700 and :369 agree with the code again.

The rejection errors no longer echo the value. Each names the property that was wrong; the scheme case keeps the scheme, since that is what makes it fixable and it is not a secret.

go vet and golangci-lint clean in both modules, and the Telegram tests pass. TestCustomProvider and TestDevProvider still fail here with 960 against 1564; they fail identically with my changes stashed, so they are the byte counts #317 carries and not this branch.

[umputun]

fail-closed and the redirect default are both right, and I'm taking them as settled: a hostile upstream already controls whether any description exists, so withholding costs nothing it could not already deny, and CheckRedirect is net/http's own policy surface with the constructor godoc now naming what a permitted hop exposes. The counter in TestTelegram_RedirectPolicyOfTheCallerIsKept is the right shape.

one thing left, in the third fix.

the parse-error branch still echoes the rejected URL (provider/telegram.go:436, same in v2)

u, err := neturl.Parse(baseURL)
if err != nil {
	return nil, fmt.Errorf("invalid telegram api base url: %w", err)
}

*url.Error prints its URL field verbatim, and this runs before the switch, so a base that is both credentialed and malformed never reaches the u.User case:

invalid telegram api base url: parse "https://alice:topsecret@example.com/%zz": invalid URL escape "%zz"

a % in a proxy password does it, as does a control character or an unclosed [. Same for the comment three lines down that says the value is never echoed back.

unwrapping to err.(*url.Error).Err looks like it keeps the useful half, and I thought so too until I ran it: https://proxy.example.com:s3cr3t/tg gives invalid port ":s3cr3t" after host, and the IPv6 zone path leaks input text as well. So the inner error is not safe either. errors.New("telegram api base url is not a valid url") is the answer, since the whole parse boundary is untrusted config.

and the assertion that would have caught it

TestTelegram_APIBaseURLRejectsUnusableValues asserts only assert.Error, so reverting all six errors.New calls back to fmt.Errorf("...: %s", baseURL) leaves everything green. One line in the existing loop covers that and the fix above:

assert.NotContains(t, err.Error(), base)

plus a credentialed-and-malformed case, https://user:pa%zzss@api.telegram.org/, and an invalid port carrying a secret. Both modules.

that's all of it. Everything else in this round checks out: both fail-closed paths land, the redirect branch is right at request and at the avatar download, and the two halves are identical. Rebase onto master when you push, it's conflict-free and clears the six byte-count failures now that #317 is in.

[paskal]

Done in 57bb40d, both modules, rebased onto master.

The parse branch no longer wraps. You are right that unwrapping does not save it: I checked the two shapes you named before changing anything, and https://proxy.example.com:s3cr3t/tg gives invalid port ":s3cr3t" after host from the inner error alone. The whole boundary is untrusted config, so it is now errors.New("telegram api base url is not a valid url") and nothing else. The comment three lines down said the value is never echoed back and was wrong about this branch; it now covers both.

The assertion that would have caught it. TestTelegram_APIBaseURLRejectsUnusableValues requires err.Error() not to contain the base it refused, in the same loop, plus the two shapes that reach the parse branch: https://user:pa%zzss@api.telegram.org/ and the invalid port above.

Reverting every rejection to interpolate the value again now fails seven of seven rejection subtests with the rejection echoed the base url it refused, where before it left the whole case green. That is the property you asked for rather than the six I changed, since it covers any rejection added later.

Both modules identical apart from the jwt block. go test ./provider/ passes in both, and the byte-count failures are gone with #317 in master, as you said. golangci-lint run ./... clean in both.

[umputun]

that closes it. Dropping the parse error is right, and your commit message reaches the invalid-port case on its own, which is the part that makes unwrapping look safe until you try it.

the NotContains across all seven paths is the assertion the whole thing was missing.

lgtm

[paskal]

162baee closes the last one: the request-build error site in saveTelegramAvatar was still going through URL-shape redaction alone, while the fetch failure beside it had been given the token-aware layer — one function holding two halves of a defence that disagreed, under a comment that had just explained why shape alone is not enough there.

It is reachable the same way as its neighbour: the download URL interpolates the file path the metadata response chose, so a path like photo%zz?copy=<token> fails url.Parse inside http.NewRequestWithContext and the *url.Error prints the whole URL. The second copy of the token has no slash-delimited segment around it, so the regex strips nothing.

Both sites now go through the same helper. TestTelegram_AvatarRequestBuildErrorDoesNotLeakTheToken drives an unparseable upstream file path and reads the logged line rather than the redaction helper, so the call site cannot drop it again; against the previous code it fails with the token in the log. I also swept the file for the shape rather than the site — no error reaching a log or a return goes through shape-only redaction now.

For what it is worth on the "would this cause another round" question: three supervised review rounds ran over this branch, on Opus and codex gpt-5.6-sol at high effort. Round 1 raised seven, round 2 raised the one above, and round 3 under a pre-merge profile came back with nothing, all sources reporting. Both modules green, golangci-lint clean in both.