From 73489bbf4f23f91223d0e318dd8ec000dee41e11 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Wed, 26 Aug 2026 21:54:57 +0100 Subject: [PATCH] Make the Telegram API base URL settable, and close what that opens The bot API host was fixed at api.telegram.org, reachable only through an unexported field the package's own tests set. Two consequences: an operator behind a proxy had no way in, and the Telegram notifier was unreachable from any test unwilling to talk to the live API. remark42 hit the second one. Its Telegram auth now points at a stub through go-pkgz/auth, which gained the same option, but the notify service still reaches the public API, and the update dispatcher takes its requester from that service, so the subscription flow cannot be covered by a browser test at all. APIURL takes the base and the "bot" segment is appended here, so a caller passes https://proxy.example.com and requests come out as https://proxy.example.com/bot/, matching what Telegram serves and what go-pkgz/auth's own option accepts. Moving a token-bearing URL across a boundary that used to be frozen is what the rest of this is for. The base is validated rather than trimmed. "https://api.telegram.org@evil.tld" is a valid URL whose host is evil.tld, and every request built from the base carries the bot token in its path, so it would ship the token there. Absolute http or https, host required, no userinfo, query, fragment or opaque part; a path prefix is allowed for a proxy mounted under one. No rejection echoes the value, since it is configuration that can carry credentials in its userinfo or a secret where the port belongs, and the parse error is dropped rather than wrapped because *url.Error prints the URL it was given. Errors are scrubbed of the token itself, not only of *url.Error's URL field. The upstream decides an API error's text, and parseError interpolated its description raw, so something standing in for Telegram could echo the request URI back and put the token in a caller's log. The scrub covers the raw and encoded forms and then checks a decoded copy, withholding the text when the token cannot be shown absent from it: after a decode failure nothing was established, and withholding is the only answer that cannot leak. It applies only to a value shaped like a bot token, ":", since blanking a short arbitrary string out of a diagnostic corrupts more than it protects. Redirects are refused. Go copies the previous URL into Referer on any hop that is not https-to-http, so following one hands the destination the token. Telegram does not redirect; something standing in for it can. Each guard fails against the thing it names: reverting the validation fails eight of the rejection cases, dropping the redaction fails two, following redirects fails the referer case, and answering "not recoverable" on a decode error releases a double-encoded token. One thing this does not change: the HTTP client is still built here, so a proxy whose TLS material lives on a caller-supplied client cannot be used yet. --- telegram.go | 159 ++++++++++++++++++++++++++++++++++++++++++++--- telegram_test.go | 125 +++++++++++++++++++++++++++++++++++++ 2 files changed, 276 insertions(+), 8 deletions(-) diff --git a/telegram.go b/telegram.go index 62a8456..63a0a41 100644 --- a/telegram.go +++ b/telegram.go @@ -26,7 +26,13 @@ type TelegramParams struct { Timeout time.Duration // http client timeout ErrorMsg, SuccessMsg string // messages for successful and unsuccessful subscription requests to bot - apiPrefix string // changed only in tests + // APIURL points the client at something other than the public bot API, for a proxy standing in + // front of Telegram or a test double answering as it. Empty means the public API. Every request + // carries the bot token in its path, so this is validated rather than trusted: see + // validateTelegramBaseURL for what is refused and why. + APIURL string + + apiPrefix string // derived from APIURL, or the public API } // Telegram notifications client @@ -78,8 +84,20 @@ const tgCleanupInterval = time.Minute * 5 func NewTelegram(params TelegramParams) (*Telegram, error) { res := Telegram{TelegramParams: params} - if res.apiPrefix == "" { + switch { + case res.APIURL != "": + base, err := validateTelegramBaseURL(res.APIURL) + if err != nil { + return nil, err + } + // the "bot" literal belongs to the API's own shape rather than to the operator's base, so + // it is appended here: a caller passes https://proxy.example.com and the URLs come out as + // https://proxy.example.com/bot/, which is what Telegram itself serves + res.apiPrefix = base + "/bot" + case res.apiPrefix == "": res.apiPrefix = telegramAPIPrefix + default: + // a prefix set directly, which the package's own tests do } if res.Timeout == 0 { res.Timeout = telegramTimeOut @@ -450,6 +468,45 @@ func (t *Telegram) botInfo(ctx context.Context) (*TelegramBotInfo, error) { return resp.Result, nil } +// validateTelegramBaseURL checks a caller-supplied API base and returns it without its trailing +// slash. Every request built from it carries the bot token in its path, so a base that resolves +// somewhere unintended ships the token there: "https://api.telegram.org@evil.tld" is a valid URL +// whose host is evil.tld, and a bare scheme or an opaque form silently produces a request to +// somewhere else again. A path prefix is allowed, for a proxy mounted under one. +// +// No rejection echoes the value. It is configuration that can carry credentials in its userinfo, a +// secret in its query or a password where the port belongs, and the error travels to whatever logs +// the constructor failure. The parse error is dropped for the same reason: *url.Error prints the +// URL it was given, and the inner error quotes the input back as well. +func validateTelegramBaseURL(baseURL string) (string, error) { + baseURL = strings.TrimRight(baseURL, "/") + + u, err := neturl.Parse(baseURL) + if err != nil { + return "", errors.New("telegram api url is not a valid url") + } + + switch { + case u.Opaque != "": + return "", errors.New("telegram api url must not be opaque") + case u.Scheme != "http" && u.Scheme != "https": + return "", fmt.Errorf("telegram api url must be http or https, got %q", u.Scheme) + case u.Hostname() == "": + // u.Host is non-empty for "http://:9000", which resolves to the local machine + return "", errors.New("telegram api url must have a host") + case u.User != nil: + return "", errors.New("telegram api url must not carry userinfo") + case u.RawQuery != "" || u.ForceQuery: + return "", errors.New("telegram api url must not carry a query") + case u.Fragment != "" || strings.HasSuffix(baseURL, "#"): + return "", errors.New("telegram api url must not carry a fragment") + } + + // rebuilt from what was checked, so the string validated is the string used + u.Path = strings.TrimRight(u.Path, "/") + return u.String(), nil +} + // Request makes a request to the Telegram API and return the result func (t *Telegram) Request(ctx context.Context, method string, b []byte, data any) error { return repeater.NewFixed(3, time.Millisecond*250).Do(ctx, func() error { @@ -469,7 +526,16 @@ func (t *Telegram) Request(ctx context.Context, method string, b []byte, data an req.Header.Set("Content-Type", "application/json; charset=utf-8") } - client := http.Client{Timeout: t.Timeout} + // refusing redirects: Go copies the previous URL into Referer on any hop that is not + // https-to-http, and every URL here carries the bot token in its path, so following one + // hands the destination the token, usually into its access log. Telegram does not redirect; + // something standing in for it can + client := http.Client{ + Timeout: t.Timeout, + CheckRedirect: func(_ *http.Request, _ []*http.Request) error { + return errors.New("refusing to follow a telegram api redirect: the bot token travels in the URL") + }, + } resp, err := client.Do(req) if err != nil { return fmt.Errorf("failed to send request: %w", t.redactToken(err)) @@ -477,7 +543,7 @@ func (t *Telegram) Request(ctx context.Context, method string, b []byte, data an defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return t.parseError(resp.Body, resp.StatusCode) + return t.redactToken(t.parseError(resp.Body, resp.StatusCode)) } if err = json.NewDecoder(resp.Body).Decode(data); err != nil { @@ -488,14 +554,91 @@ func (t *Telegram) Request(ctx context.Context, method string, b []byte, data an }) } -// redactToken hides the bot token in the URL of *url.Error returned by the http client, -// as the token is a part of every API URL and otherwise leaks into the logs of the caller printing the error +// redactToken removes the bot token from an error before it reaches a caller's log. +// +// Two routes, and the second only exists once APIURL can point somewhere the operator chose. The +// token is part of every API URL, so a transport failure carries it in *url.Error's URL field. And +// the upstream decides the text of an API error: something standing in for Telegram can echo the +// request URI into its description, in whatever encoding it likes, so scrubbing the token itself is +// what holds rather than matching a URL shape. func (t *Telegram) redactToken(err error) error { + if err == nil || t.Token == "" { + return err + } + var urlErr *neturl.Error - if t.Token == "" || !errors.As(err, &urlErr) || !strings.Contains(urlErr.URL, t.Token) { + if errors.As(err, &urlErr) && strings.Contains(urlErr.URL, t.Token) { + return &neturl.Error{ + Op: urlErr.Op, URL: strings.ReplaceAll(urlErr.URL, t.Token, ""), Err: urlErr.Err, + } + } + + // the text scrub is for something that could actually be a bot token. Telegram issues them as + // ":", and blanking a short arbitrary string out of a diagnostic corrupts more + // than it protects: a token of "404" would turn every "status code 404" into "status code + // ". The URL-field redaction above stays unconditional, since there the token is + // whatever the caller configured and the field is nothing else + if !looksLikeBotToken(t.Token) { return err } - return &neturl.Error{Op: urlErr.Op, URL: strings.ReplaceAll(urlErr.URL, t.Token, ""), Err: urlErr.Err} + + msg := err.Error() + for _, form := range []string{t.Token, neturl.QueryEscape(t.Token), neturl.PathEscape(t.Token)} { + msg = strings.ReplaceAll(msg, form, "") + } + + // substitution only catches encodings we thought of, and the upstream picks the encoding, so the + // result is checked once more against a decoded copy. If the token is still recoverable by any + // of those routes the text goes rather than the token stays + if tokenRecoverable(msg, t.Token) { + return errors.New("unexpected telegram API error, text withheld: the bot token could not be ruled out of it") + } + + if msg == err.Error() { + return err + } + return errors.New(msg) +} + +// looksLikeBotToken reports whether token has the shape Telegram issues, ":". Used +// to keep the text scrub off values that cannot be one, where it would damage diagnostics for +// nothing +func looksLikeBotToken(token string) bool { + id, secret, found := strings.Cut(token, ":") + if !found || len(secret) < 8 { + return false + } + if _, err := strconv.Atoi(id); err != nil { + return false + } + return true +} + +// tokenRecoverable reports whether token can still be read out of msg after undoing the encodings +// an upstream might have applied to it. +// +// Fails closed: after a decode error, or after the cap runs out while the text is still changing, +// absence was never established, and withholding is the only answer that cannot leak. One stray "%" +// in whatever the upstream echoed is enough to reach that. +func tokenRecoverable(msg, token string) bool { + lowered := strings.ToLower(token) + seen := msg + for i := 0; i < 5; i++ { + if strings.Contains(strings.ToLower(seen), lowered) { + return true + } + next, err := neturl.QueryUnescape(seen) + if err != nil { + return true + } + if next == seen { + // decoding has converged and every form has been examined, so the token is absent + // rather than undecided + return false + } + seen = next + } + return true } func (t *Telegram) parseError(r io.Reader, statusCode int) error { diff --git a/telegram_test.go b/telegram_test.go index 464c920..08bddef 100644 --- a/telegram_test.go +++ b/telegram_test.go @@ -738,3 +738,128 @@ func mockTelegramServer(h http.HandlerFunc) *httptest.Server { return httptest.NewServer(mux) } + +const fixtureToken = "1234567:SECRET-TOK_EN-x" + +// TestTelegram_APIURLRejectsUnusableValues covers the validation a caller-supplied base needs. +// Every request built from it carries the bot token in its path, so a base that resolves somewhere +// unintended ships the token there, and TrimRight alone is not a guard. +func TestTelegram_APIURLRejectsUnusableValues(t *testing.T) { + // every value here exists to be refused; the userinfo one is a url whose host is evil.tld, + // which is exactly what the validator has to catch + for name, base := range map[string]string{ //nolint:gosec // G101: urls to be refused, not credentials + "userinfo redirects the host": "https://api.telegram.org@evil.tld", + "no scheme": "api.telegram.org", + "wrong scheme": "ftp://api.telegram.org", + "opaque": "https:api.telegram.org", + "no host": "https://", + "port without a host": "http://:9000", + "carries a query": "https://api.telegram.org?a=b", + "carries a fragment": "https://api.telegram.org#x", + "credentialed and malformed": "https://user:pa%zzss@api.telegram.org", + "secret in place of a port": "https://proxy.example.com:s3cr3t/tg", + } { + t.Run(name, func(t *testing.T) { + _, err := NewTelegram(TelegramParams{Token: fixtureToken, APIURL: base}) + require.Error(t, err, "%s has to be refused", base) + + // the refusal is logged by whoever built the client, and the value it refused is + // untrusted configuration: without this every rejection could interpolate the base + // back and stay green + assert.NotContains(t, err.Error(), strings.TrimRight(base, "/"), + "the rejection echoed the base url it refused") + }) + } +} + +// TestTelegram_APIURLIsUsedForRequests pins that a valid base actually redirects the calls, path +// prefix included, and that the "bot" segment is the API's own shape rather than the caller's +// problem. +func TestTelegram_APIURLIsUsedForRequests(t *testing.T) { + const token = fixtureToken + + var seen string + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + seen = r.URL.Path + _, _ = fmt.Fprint(w, `{"ok":true,"result":{"username":"somebot"}}`) + })) + defer ts.Close() + + tg, err := NewTelegram(TelegramParams{Token: token, APIURL: ts.URL + "/tg/"}) + require.NoError(t, err) + assert.Equal(t, "somebot", tg.username) + assert.Equal(t, "/tg/bot"+token+"/getMe", seen, + "the request did not go through the configured base, or the bot segment was lost") +} + +// TestTelegram_APIErrorDoesNotLeakTheToken covers the route the settable base opens. The upstream +// decides an error's text, so something standing in for Telegram can echo the request URI into its +// description, and the shape-matching redaction that came before only ever looked at *url.Error's +// URL field. +func TestTelegram_APIErrorDoesNotLeakTheToken(t *testing.T) { + const token = fixtureToken + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadGateway) + // the echo is the point: an upstream putting the request uri into its own error text is + // the route this test exists for + _, _ = fmt.Fprintf(w, `{"description":%q}`, "forwarding "+r.URL.RequestURI()) //nolint:gosec // G705: the response is read by this test, never rendered + })) + defer ts.Close() + + _, err := NewTelegram(TelegramParams{Token: token, APIURL: ts.URL}) + require.Error(t, err) + assert.NotContains(t, err.Error(), token, "the api error carried the bot token") +} + +// TestTelegram_APIErrorWithheldWhenEscapingIsMalformed covers the direction tokenRecoverable has to +// fail in: a bare "%" stops the decoder before it reaches an encoded copy of the token, and +// answering "not recoverable" there would release text whose encodings were never undone. +func TestTelegram_APIErrorWithheldWhenEscapingIsMalformed(t *testing.T) { + const token = fixtureToken + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadGateway) + doubled := neturl.QueryEscape(neturl.QueryEscape(r.URL.RequestURI())) + _, _ = fmt.Fprintf(w, `{"description":%q}`, "forwarding "+doubled+" at 100% load") + })) + defer ts.Close() + + _, err := NewTelegram(TelegramParams{Token: token, APIURL: ts.URL}) + require.Error(t, err) + assert.Contains(t, err.Error(), "text withheld", "an undecodable description was released") + + text := err.Error() + for i := 0; i < 5; i++ { + assert.NotContains(t, text, token, "the token is recoverable after %d decoding passes", i) + next, decErr := neturl.QueryUnescape(text) + if decErr != nil || next == text { + break + } + text = next + } +} + +// TestTelegram_DoesNotFollowRedirects covers the other route out. Go copies the previous URL into +// Referer on an https-to-https hop, and every URL here carries the token in its path, so following +// a redirect hands the destination the token. +func TestTelegram_DoesNotFollowRedirects(t *testing.T) { + const token = fixtureToken + + var refererSeen string + dest := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + refererSeen = r.Header.Get("Referer") + _, _ = fmt.Fprint(w, `{"ok":true,"result":{"username":"somebot"}}`) + })) + defer dest.Close() + + src := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, dest.URL+"/next", http.StatusFound) + })) + defer src.Close() + + _, err := NewTelegram(TelegramParams{Token: token, APIURL: src.URL}) + require.Error(t, err, "the redirect was followed") + assert.Empty(t, refererSeen, "the redirect target was reached and saw Referer %q", refererSeen) + assert.NotContains(t, err.Error(), token, "the refusal itself leaked the token") +}