Conversation
This commit introduces the ability to use a webhook poller instead of the default long polling mechanism. Key changes: - Added `WebhookURL`, `ListenAddress`, `TLSKeyFile`, and `TLSCertFile` configuration options to `bote.Config` in `options.go`. These can also be set via environment variables. - Updated `bot.go` to initialize a `tele.Webhook` poller when webhook options are provided. It supports configurations with and without direct TLS handling (for use behind a reverse proxy). - Updated `README.md` to document the new webhook configuration options and usage. - Added an example of webhook configuration in `example/main.go`. - Added unit tests in `bote_test.go` to verify that the correct poller (LongPoller or Webhook) is initialized based on the provided configuration and that webhook settings are correctly applied.
This commit fixes the webhook TLS configuration in `bot.go` and updates the corresponding tests in `bote_test.go`. Previously, `webhook.KeyFile` and `webhook.Endpoint.PublicKey` were being set directly. The corrected implementation now uses the `webhook.TLS` field (a `*tele.WebhookTLS` struct) to set the key and certificate file paths for the HTTPS listener, and `webhook.Endpoint.Cert` to provide the public certificate to Telegram for self-signed certificate validation, aligning with `telebot.v4` library's expected structure. The tests have been updated to assert the values in these corrected fields. This change should address potential build or runtime errors related to webhook TLS setup.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This commit updates the test assertions in `bote_test.go` for the webhook poller initialization logic, specifically for the NoTLS case.
- In the "WebhookPoller_NoTLS" test case:
- Assertions now correctly check for `webhookPoller.TLS == nil`.
- Assertions now correctly check for `webhookPoller.Endpoint.Cert == ""` (empty string).
- Removed outdated assertions for fields that were previously part of an incorrect TLS configuration approach.
- Other test assertions for DefaultLongPoller, WebhookPoller_WithTLS, and invalid configurations were reviewed and confirmed to be consistent with the current implementation.
These changes ensure the tests accurately reflect the expected state of the webhook poller configuration as per `telebot.v4`'s API.
WalkthroughWebhook support was added to the bot, allowing configuration via new fields in the config struct and corresponding environment variables. The bot initialization logic now selects between long polling and webhook polling based on these settings, with appropriate TLS handling. Tests and documentation were updated to cover webhook configuration and validation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Config
participant Bot
participant Telegram
User->>Config: Set WebhookURL and TLS options
Config->>Bot: Pass config during initialization
Bot->>Bot: Validate config (webhook fields, TLS, etc.)
alt WebhookURL set
Bot->>Bot: Create Webhook poller (with/without TLS)
Telegram-->>Bot: Send updates via webhook
else
Bot->>Bot: Create LongPoller
Bot->>Telegram: Poll for updates
end
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
||
| To configure webhooks, you need to set the following fields in the `bote.Config` struct or use their corresponding environment variables: | ||
|
|
||
| - **`WebhookURL`** (`string`): The publicly accessible HTTPS URL for your webhook. Telegram will send updates to this URL. |
There was a problem hiding this comment.
🔒 Security improvement
Model confidence: very high (90-100%)
Issue priority: must be fixed immediately 🔴
Critical: Missing Webhook Secret Token Configuration in Documentation
The newly added 'Webhook Configuration' section in the README omits the crucial secret_token parameter recommended by Telegram for webhook security. This token is essential for verifying that incoming updates originate from Telegram's servers, preventing unauthorized requests and potential denial-of-service attacks. Without explicit guidance to configure this, users may deploy webhooks vulnerable to external manipulation, compromising the bot's integrity and reliability. This is a fundamental security measure for any public-facing webhook.
💡 Suggestion
Enhance the bote.Config struct to include a WebhookSecretToken field and ensure the underlying bote framework validates this token against the X-Telegram-Bot-Api-Secret-Token header. Update the README to clearly document WebhookSecretToken, its purpose, and the importance of using a strong, randomly generated value. Additionally, for comprehensive webhook management, consider documenting MaxConnections (to control concurrent connections) and AllowedUpdates (to filter update types, reducing unnecessary traffic), although secret_token is the most critical omission.
- **`TLSKeyFile`** (`string`, optional): Path to your TLS private key file. Required if you want the bot to handle HTTPS directly.
- Environment variable: `BOTE_TLS_KEY_FILE`
- **`TLSCertFile`** (`string`, optional): Path to your TLS public certificate file. Required if you want the bot to handle HTTPS directly.
- Environment variable: `BOTE_TLS_CERT_FILE`
+ - **`WebhookSecretToken`** (`string`, optional): A secret token to verify the authenticity of incoming updates. Telegram will send this token in the `X-Telegram-Bot-Api-Secret-Token` header.
+ - Environment variable: `BOTE_WEBHOOK_SECRET_TOKEN`
+ - **Importance**: Crucial for security. Use a strong, random string to prevent unauthorized requests.
+ - **`MaxConnections`** (`int`, optional): The maximum number of concurrent HTTPS connections to the webhook.
+ - Environment variable: `BOTE_MAX_CONNECTIONS`
+ - Defaults to `40` (Telegram's default).
+ - **`AllowedUpdates`** (`[]string`, optional): A JSON-serialized list of the update types you want your bot to receive.
+ - Environment variable: `BOTE_ALLOWED_UPDATES`
+ - Defaults to all update types.
| Token: token, | ||
| Poller: tele.NewMiddlewarePoller(poller, b.middleware), | ||
| Client: &http.Client{Timeout: 2 * opts.Config.LPTimeout}, | ||
| Client: &http.Client{Timeout: 2 * opts.Config.LPTimeout}, // Consider if this timeout is appropriate for webhooks |
There was a problem hiding this comment.
🚀 Performance improvement
Model confidence: very high (90-100%)
Issue priority: should be fixed soon 🟡
Inappropriate HTTP Client Timeout for Webhook Mode
The http.Client used by telebot.NewBot has its Timeout set to 2 * opts.Config.LPTimeout. While LPTimeout (Long Polling Timeout) is relevant for long polling, this http.Client is used for all Telegram API calls (sending messages, editing, deleting, and crucially, setting up the webhook itself) regardless of the polling method. Tying the general API client timeout to a long polling specific timeout is semantically incorrect and can lead to issues. If LPTimeout is very short (e.g., 5s for quick polling cycles), the client timeout becomes 10s, which might be too short for some Telegram API calls under network congestion or high load, leading to spurious API errors. Conversely, if LPTimeout is very long, it might unnecessarily delay startup or API calls. This coupling introduces an arbitrary dependency and can negatively impact the reliability of API interactions.
💡 Suggestion
Decouple the http.Client timeout from LPTimeout. Introduce a specific configuration option (e.g., opts.Config.HTTPClientTimeout) for the general API client timeout, allowing it to be configured independently based on typical API response expectations. This improves robustness, clarity, and allows for more precise control over API call timeouts.
package bote
import (
// ...
"time"
)
// In Options struct:
type Config struct {
// ... existing fields ...
LPTimeout time.Duration
HTTPClientTimeout time.Duration // New field
}
// In prepareOpts function (or similar configuration setup):
func prepareOpts(opts Options) (Options, error) {
// ...
if opts.Config.HTTPClientTimeout == 0 { // Set a reasonable default if not provided
opts.Config.HTTPClientTimeout = 30 * time.Second
}
// ...
return opts, nil
}
// In newBaseBot function, replace the Client timeout line:
func newBaseBot(token string, opts Options) (*baseBot, error) {
// ...
bot, err := tele.NewBot(tele.Settings{
Token: token,
Poller: tele.NewMiddlewarePoller(poller, b.middleware),
Client: &http.Client{Timeout: opts.Config.HTTPClientTimeout}, // Use dedicated timeout
Verbose: opts.Config.Debug,
// ...
})
// ...
}
| webhook := &tele.Webhook{ | ||
| Listen: opts.Config.ListenAddress, | ||
| Endpoint: &tele.WebhookEndpoint{PublicURL: opts.Config.WebhookURL}, | ||
| } |
There was a problem hiding this comment.
🔒 Security improvement
Model confidence: high (70-90%)
Issue priority: should be fixed soon 🟡
Missing Webhook Secret Token for Enhanced Security
Telegram webhooks support a secret_token that can be used to verify the authenticity of incoming updates. When a secret_token is configured, Telegram includes an X-Telegram-Bot-Api-Secret-Token header in webhook requests. The bot should then verify this header against its configured secret. This mechanism helps prevent denial-of-service attacks or spoofed updates from unauthorized sources, ensuring that only legitimate updates from Telegram are processed. The current implementation configures the webhook but does not utilize this security feature, making the webhook endpoint potentially vulnerable to unauthenticated requests if the WebhookURL is ever compromised or guessed.
💡 Suggestion
Add a SecretToken field to the Config struct. If provided, pass this token to the tele.Webhook configuration. This will enable telebot to automatically verify the X-Telegram-Bot-Api-Secret-Token header, significantly enhancing the security and integrity of the webhook endpoint. The secret token should be a strong, randomly generated string.
package bote
// In Options struct:
type Config struct {
// ... existing fields ...
WebhookURL string
SecretToken string // New field
}
// In newBaseBot function, inside the webhook configuration block:
func newBaseBot(token string, opts Options) (*baseBot, error) {
// ...
if opts.Config.WebhookURL != "" {
webhook := &tele.Webhook{
Listen: opts.Config.ListenAddress,
Endpoint: &tele.WebhookEndpoint{PublicURL: opts.Config.WebhookURL},
Secret: opts.Config.SecretToken, // Pass the secret token here
}
// ... rest of webhook config ...
poller = webhook
}
// ...
}
| Listen: opts.Config.ListenAddress, | ||
| Endpoint: &tele.WebhookEndpoint{PublicURL: opts.Config.WebhookURL}, | ||
| } | ||
| if opts.Config.TLSKeyFile != "" && opts.Config.TLSCertFile != "" { |
There was a problem hiding this comment.
🔄 Other issue
Model confidence: high (70-90%)
Issue priority: could be fixed later 🟢
Lack of Explicit File Existence/Readability Checks for TLS Certificates
When configuring TLS for webhooks, the TLSKeyFile and TLSCertFile paths are provided. The current code only checks if these string paths are non-empty. It does not verify if the files actually exist or are readable. While telebot's underlying HTTP server (which uses net/http) will eventually return an error if the files are invalid or inaccessible, this error might occur later during startup, be less specific, or lead to a silent failure to start the webhook with TLS. Failing early with a clear, specific error message (e.g., "TLS key file not found") improves debugging, system reliability, and reduces the time to identify misconfigurations.
💡 Suggestion
Before constructing the tele.Webhook with TLS paths, add explicit checks to verify the existence and readability of opts.Config.TLSKeyFile and opts.Config.TLSCertFile using os.Stat. Return a wrapped error with a descriptive message if any file is missing or inaccessible. Additionally, ensure that if WebhookURL is set, both TLS files are provided if TLS is intended, preventing partial configurations.
package bote
import (
// ...
"os"
"github.com/maxbolgarin/errm"
)
// In newBaseBot function, inside the webhook TLS configuration block:
func newBaseBot(token string, opts Options) (*baseBot, error) {
// ...
if opts.Config.WebhookURL != "" {
webhook := &tele.Webhook{
Listen: opts.Config.ListenAddress,
Endpoint: &tele.WebhookEndpoint{PublicURL: opts.Config.WebhookURL},
}
if opts.Config.TLSKeyFile != "" && opts.Config.TLSCertFile != "" {
// Check if key file exists and is readable
if _, err := os.Stat(opts.Config.TLSKeyFile); os.IsNotExist(err) {
return nil, errm.Wrap(err, "TLS key file not found", "path", opts.Config.TLSKeyFile)
}
// Check if cert file exists and is readable
if _, err := os.Stat(opts.Config.TLSCertFile); os.IsNotExist(err) {
return nil, errm.Wrap(err, "TLS certificate file not found", "path", opts.Config.TLSCertFile)
}
webhook.TLS = &tele.WebhookTLS{
Key: opts.Config.TLSKeyFile,
Cert: opts.Config.TLSCertFile,
}
webhook.Endpoint.Cert = opts.Config.TLSCertFile
} else if opts.Config.TLSKeyFile != "" || opts.Config.TLSCertFile != "" {
// If WebhookURL is set, but only one TLS file is provided, it's likely a misconfiguration.
return nil, errm.New("both TLSKeyFile and TLSCertFile must be provided for webhook TLS")
}
poller = webhook
}
// ...
}
| cfg.UserCacheCapacity = lang.Check(cfg.UserCacheCapacity, 10000) | ||
| cfg.UserCacheTTL = lang.Check(cfg.UserCacheTTL, 24*time.Hour) | ||
|
|
||
| if cfg.WebhookURL != "" { |
There was a problem hiding this comment.
🔄 Other issue
Model confidence: very high (90-100%)
Issue priority: should be fixed soon 🟡
Ambiguous Bot Polling Mode Configuration
The Config struct now includes fields for webhook setup (WebhookURL, ListenAddress, TLSKeyFile, TLSCertFile), implying the bot can operate in webhook mode. However, the Options struct also has a Poller field. If opts.Poller is nil (which is the default unless WithTestMode is used), telebot's internal logic will default to a LongPoller. There is no explicit mechanism within prepareOpts or the bot's overall design to manage the mutual exclusivity or precedence between long polling and webhook modes. This can lead to a situation where webhook configuration is provided, but the bot still defaults to long polling, or where an explicitly provided LongPoller conflicts with webhook settings, causing unexpected behavior or misconfiguration at runtime.
💡 Suggestion
The responsibility for instantiating the correct tele.Poller (either tele.LongPoller or tele.Webhook) should be clearly defined and handled in the bot's main constructor (e.g., NewBote function). The prepareOpts function should primarily validate the consistency of the Options and Config. If opts.Config.WebhookURL is set, prepareOpts should ensure that any explicitly provided opts.Poller is not a tele.LongPoller. If no WebhookURL is set and opts.Poller is nil, prepareOpts should default it to tele.LongPoller to ensure a functional bot setup.
func prepareOpts(opts Options) (Options, error) {
err := opts.Config.prepareAndValidate()
if err != nil {
return opts, errm.Wrap(err, "prepare and validate config")
}
// ... (existing logger, userDB, msgs setup)
// Validate Poller consistency with WebhookURL configuration and set default if needed
if opts.Config.WebhookURL != "" {
// If WebhookURL is set, an explicitly provided Poller should not be a LongPoller.
// The actual tele.Webhook poller will be instantiated by the bot's core logic (e.g., in NewBote).
if opts.Poller != nil {
if _, isLongPoller := opts.Poller.(*tele.LongPoller); isLongPoller {
return opts, errm.New("explicitly provided LongPoller conflicts with webhook URL configuration")
}
}
} else {
// If no WebhookURL, and no poller is provided, default to LongPoller.
// This ensures the bot always has a default polling mechanism if not using webhooks.
if opts.Poller == nil {
opts.Poller = &tele.LongPoller{Timeout: opts.Config.LPTimeout}
}
}
return opts, nil
}
| return errm.Wrap(err, "invalid webhook url") | ||
| } | ||
| cfg.ListenAddress = lang.Check(cfg.ListenAddress, ":8443") | ||
| if (cfg.TLSKeyFile == "") != (cfg.TLSCertFile == "") { |
There was a problem hiding this comment.
🔄 Other issue
Model confidence: high (70-90%)
Issue priority: should be fixed soon 🟡
Insufficient TLS Key/Cert File Validation
The prepareAndValidate function correctly checks that TLSKeyFile and TLSCertFile are either both provided or both empty. However, it does not verify if the specified file paths actually exist on the filesystem or if the application has the necessary permissions to read them. If these files are missing, corrupted, or inaccessible, the bot will fail to start its webhook server at runtime, leading to a service outage. The error will occur later during the telebot.Webhook poller setup, making debugging harder as the configuration was considered "valid" during the initial prepareAndValidate call.
💡 Suggestion
Add checks using os.Stat to verify the existence and readability of the TLS key and certificate files when WebhookURL is configured and TLS files are provided. This provides an early failure mechanism during configuration loading, preventing runtime crashes and improving diagnostics by failing fast with a clear error message.
func (cfg *Config) prepareAndValidate() error {
if err := env.Parse(cfg); err != nil {
return err
}
// ... existing checks ...
if cfg.WebhookURL != "" {
if _, err := url.ParseRequestURI(cfg.WebhookURL); err != nil {
return errm.Wrap(err, "invalid webhook url")
}
cfg.ListenAddress = lang.Check(cfg.ListenAddress, ":8443")
if (cfg.TLSKeyFile == "") != (cfg.TLSCertFile == "") {
return errm.New("tls key and cert files must be both provided or both empty")
}
// NEW: Validate existence and readability of TLS files if provided
if cfg.TLSKeyFile != "" { // Implies TLSCertFile is also not empty due to previous check
if _, err := os.Stat(cfg.TLSKeyFile); os.IsNotExist(err) {
return errm.Wrapf(err, "TLS key file not found: %s", cfg.TLSKeyFile)
} else if err != nil { // Other errors like permission denied
return errm.Wrapf(err, "failed to access TLS key file %s", cfg.TLSKeyFile)
}
if _, err := os.Stat(cfg.TLSCertFile); os.IsNotExist(err) {
return errm.Wrapf(err, "TLS certificate file not found: %s", cfg.TLSCertFile)
} else if err != nil {
return errm.Wrapf(err, "failed to access TLS certificate file %s", cfg.TLSCertFile)
}
}
}
return nil
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
README.md (1)
133-154: LGTM with minor grammar fix needed.The webhook configuration documentation is comprehensive and well-structured, covering:
- All configuration fields and environment variables
- TLS usage scenarios
- Network accessibility considerations
Apply this diff to fix the grammar issue identified by static analysis:
-If your bot is directly exposed to the internet and you want it to handle HTTPS encryption itself, provide both `TLSKeyFile` and `TLSCertFile`. +If your bot is directly exposed to the internet, and you want it to handle HTTPS encryption itself, provide both `TLSKeyFile` and `TLSCertFile`.bot.go (1)
567-567: Consider webhook-specific HTTP timeout configuration.The comment raises a valid concern about HTTP client timeout appropriateness for webhooks. Consider:
- Webhooks typically need different timeout characteristics than long polling
- Long polling uses longer timeouts by design, while webhooks should be responsive
- The current
2 * opts.Config.LPTimeoutmay be too long for webhook scenariosConsider implementing webhook-specific timeout configuration:
-Client: &http.Client{Timeout: 2 * opts.Config.LPTimeout}, // Consider if this timeout is appropriate for webhooks +Client: &http.Client{Timeout: lang.If(opts.Config.WebhookURL != "", 30*time.Second, 2*opts.Config.LPTimeout)},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)bot.go(1 hunks)bote_test.go(1 hunks)example/main.go(1 hunks)options.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
bot.go (1)
options.go (1)
Config(82-158)
🪛 LanguageTool
README.md
[uncategorized] ~151-~151: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... bot is directly exposed to the internet and you want it to handle HTTPS encryption ...
(COMMA_COMPOUND_SENTENCE)
🪛 GitHub Actions: Go
bote_test.go
[error] 1025-1025: TestContextOperations failed: expected text '/test with arguments' but got empty string.
[error] 1259-1259: TestBotInitialization_PollerSelection/WebhookPoller_InvalidConfig_MissingURL failed: expected an error due to invalid webhook configuration (missing URL) but got nil.
🔇 Additional comments (5)
options.go (3)
5-5: LGTM: Import addition for URL validation.The
net/urlimport is correctly added to support webhook URL validation.
142-157: LGTM: Well-structured webhook configuration fields.The webhook configuration fields are well-designed with:
- Clear documentation for each field
- Proper environment variable mapping
- Sensible defaults where applicable
The field structure supports both scenarios: direct TLS handling and reverse proxy setups.
221-229: LGTM: Robust webhook validation logic.The validation logic properly:
- Validates webhook URL format using
url.ParseRequestURI- Sets default listen address when webhook URL is provided
- Ensures TLS key and cert files are both provided or both empty
This prevents misconfiguration and provides clear error messages.
example/main.go (1)
20-37: LGTM: Comprehensive webhook configuration examples.The commented examples effectively demonstrate:
- Webhook setup behind a reverse proxy (HTTP termination)
- Direct TLS handling by the bot
- Clear explanations of when to use each approach
These examples provide practical guidance for users implementing webhook support.
bot.go (1)
542-562: LGTM: Well-implemented webhook poller selection.The webhook implementation correctly:
- Falls back to long polling when no webhook URL is provided
- Configures webhook with proper listen address and public URL
- Handles TLS configuration when both key and cert files are provided
- Sets the endpoint certificate for Telegram verification
The conditional logic is clean and handles all scenarios appropriately.
| func TestBotInitialization_PollerSelection(t *testing.T) { | ||
| // Test Case 1: Default (Long Poller) | ||
| t.Run("DefaultLongPoller", func(t *testing.T) { | ||
| boteBot, err := bote.New("test_token", bote.WithTestMode(NewTestPoller())) // Using TestMode to avoid actual API calls & provide a poller | ||
| assert.NoError(t, err) | ||
| if assert.NotNil(t, boteBot) { | ||
| teleBot := boteBot.Bot() | ||
| if assert.NotNil(t, teleBot) && assert.NotNil(t, teleBot.Poller) { | ||
| // The poller is wrapped by MiddlewarePoller, so we need to check the actual poller inside it. | ||
| // This requires inspecting the MiddlewarePoller's Poller field, which might not be directly accessible. | ||
| // For now, we assume that if WebhookURL is not set, it defaults to LongPoller. | ||
| // A more robust check would involve reflection or a getter for the wrapped poller if telebot provides it. | ||
| // As direct type assertion on teleBot.Poller will fail because it's a *tele.MiddlewarePoller. | ||
| // We check if it's *not* a WebhookPoller as an indirect way for now. | ||
| // Ideally, bote.Bot could expose its configured poller type or tele.MiddlewarePoller could expose its inner poller. | ||
| // For the purpose of this test, we'll create a bot *without* WithTestMode's poller to inspect the default. | ||
| opts := bote.Options{ | ||
| Config: bote.Config{TestMode: true}, // Keep TestMode to prevent actual polling | ||
| } | ||
| defaultBot, err := bote.NewWithOptions("test_token", opts) | ||
| assert.NoError(t, err) | ||
| if assert.NotNil(t, defaultBot) { | ||
| actualTeleBot := defaultBot.Bot() | ||
| if assert.NotNil(t, actualTeleBot) && assert.NotNil(t, actualTeleBot.Poller) { | ||
| middlewarePoller, ok := actualTeleBot.Poller.(*tele.MiddlewarePoller) | ||
| if assert.True(t, ok, "Poller should be MiddlewarePoller") { | ||
| assert.IsType(t, &tele.LongPoller{}, middlewarePoller.Poller, "Underlying poller should be LongPoller") | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| // Test Case 2: Webhook Poller (No TLS) | ||
| t.Run("WebhookPoller_NoTLS", func(t *testing.T) { | ||
| opts := bote.Options{ | ||
| Config: bote.Config{ | ||
| WebhookURL: "https://test.com/webhook", | ||
| ListenAddress: "127.0.0.1:8080", | ||
| TestMode: true, // Important to prevent actual network operations | ||
| }, | ||
| } | ||
| boteBot, err := bote.NewWithOptions("test_token", opts) | ||
| assert.NoError(t, err) | ||
| if assert.NotNil(t, boteBot) { | ||
| teleBot := boteBot.Bot() | ||
| if assert.NotNil(t, teleBot) && assert.NotNil(t, teleBot.Poller) { | ||
| middlewarePoller, ok := teleBot.Poller.(*tele.MiddlewarePoller) | ||
| if assert.True(t, ok, "Poller should be MiddlewarePoller") { | ||
| webhookPoller, ok := middlewarePoller.Poller.(*tele.Webhook) | ||
| if assert.True(t, ok, "Underlying poller should be Webhook") { | ||
| assert.Equal(t, "127.0.0.1:8080", webhookPoller.Listen) | ||
| if assert.NotNil(t, webhookPoller.Endpoint) { | ||
| assert.Equal(t, "https://test.com/webhook", webhookPoller.Endpoint.PublicURL) | ||
| assert.Empty(t, webhookPoller.Endpoint.Cert, "Endpoint.Cert should be empty for NoTLS case") | ||
| } | ||
| assert.Nil(t, webhookPoller.TLS, "TLS config should be nil for NoTLS case") | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| // Test Case 3: Webhook Poller (With TLS) | ||
| t.Run("WebhookPoller_WithTLS", func(t *testing.T) { | ||
| opts := bote.Options{ | ||
| Config: bote.Config{ | ||
| WebhookURL: "https://test.com/webhook_tls", | ||
| ListenAddress: "127.0.0.1:8443", | ||
| TLSCertFile: "test_cert.pem", | ||
| TLSKeyFile: "test_key.pem", | ||
| TestMode: true, // Important | ||
| }, | ||
| } | ||
| boteBot, err := bote.NewWithOptions("test_token", opts) | ||
| assert.NoError(t, err) | ||
| if assert.NotNil(t, boteBot) { | ||
| teleBot := boteBot.Bot() | ||
| if assert.NotNil(t, teleBot) && assert.NotNil(t, teleBot.Poller) { | ||
| middlewarePoller, ok := teleBot.Poller.(*tele.MiddlewarePoller) | ||
| if assert.True(t, ok, "Poller should be MiddlewarePoller") { | ||
| webhookPoller, ok := middlewarePoller.Poller.(*tele.Webhook) | ||
| if assert.True(t, ok, "Underlying poller should be Webhook") { | ||
| assert.Equal(t, "127.0.0.1:8443", webhookPoller.Listen) | ||
| if assert.NotNil(t, webhookPoller.Endpoint) { | ||
| assert.Equal(t, "https://test.com/webhook_tls", webhookPoller.Endpoint.PublicURL) | ||
| assert.Equal(t, "test_cert.pem", webhookPoller.Endpoint.Cert) // Corrected: Was PublicKey | ||
| } | ||
| if assert.NotNil(t, webhookPoller.TLS) { // Corrected: Check TLS struct | ||
| assert.Equal(t, "test_key.pem", webhookPoller.TLS.Key) // Corrected: Was KeyFile | ||
| assert.Equal(t, "test_cert.pem", webhookPoller.TLS.Cert) // Corrected: New check for TLS.Cert | ||
| } else { | ||
| assert.Fail(t, "webhookPoller.TLS should not be nil when TLSKeyFile and TLSCertFile are provided") | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| // Test Case 4: Invalid Webhook Config (e.g., TLS files set, URL missing) | ||
| // This relies on validation in options.go's prepareAndValidate | ||
| t.Run("WebhookPoller_InvalidConfig_MissingURL", func(t *testing.T) { | ||
| opts := bote.Options{ | ||
| Config: bote.Config{ | ||
| // WebhookURL is missing | ||
| ListenAddress: "127.0.0.1:8443", | ||
| TLSCertFile: "test_cert.pem", | ||
| TLSKeyFile: "test_key.pem", | ||
| TestMode: true, | ||
| }, | ||
| } | ||
| _, err := bote.NewWithOptions("test_token", opts) | ||
| assert.Error(t, err, "Expected an error due to invalid webhook configuration (missing URL)") | ||
| // Check if the error message indicates the URL issue, if possible and stable. | ||
| // For now, just checking for any error is sufficient as specific error messages can be brittle. | ||
| }) | ||
|
|
||
| t.Run("WebhookPoller_InvalidConfig_OnlyOneTLSFile_Key", func(t *testing.T) { | ||
| opts := bote.Options{ | ||
| Config: bote.Config{ | ||
| WebhookURL: "https://test.com/webhook", | ||
| ListenAddress: "127.0.0.1:8443", | ||
| TLSKeyFile: "test_key.pem", | ||
| // TLSCertFile is missing | ||
| TestMode: true, | ||
| }, | ||
| } | ||
| _, err := bote.NewWithOptions("test_token", opts) | ||
| assert.Error(t, err, "Expected an error due to invalid webhook configuration (missing cert file)") | ||
| }) | ||
|
|
||
| t.Run("WebhookPoller_InvalidConfig_OnlyOneTLSFile_Cert", func(t *testing.T) { | ||
| opts := bote.Options{ | ||
| Config: bote.Config{ | ||
| WebhookURL: "https://test.com/webhook", | ||
| TLSCertFile: "test_cert.pem", | ||
| // TLSKeyFile is missing | ||
| TestMode: true, | ||
| }, | ||
| } | ||
| _, err := bote.NewWithOptions("test_token", opts) | ||
| assert.Error(t, err, "Expected an error due to invalid webhook configuration (missing key file)") | ||
| }) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix test failures and improve webhook validation tests.
The webhook poller selection tests are comprehensive, but there are two critical issues that need to be addressed:
-
Missing URL validation test failure (line 1259): The test expects an error when webhook URL is missing but TLS files are provided, but no error occurs. This suggests the validation logic in
options.gomay not be working correctly for this scenario. -
Context operations test failure (line 1025): The text handling test is failing, which may indicate issues with the test setup or context implementation.
Issue 1: Fix webhook validation logic
The validation should reject configurations where TLS files are provided without a webhook URL. Review the validation logic in options.go:
if cfg.WebhookURL != "" {
// existing validation
+} else if cfg.TLSKeyFile != "" || cfg.TLSCertFile != "" {
+ return errm.New("tls files provided without webhook url")
}Issue 2: Fix context text handling in tests
The text handling test is failing because the received text is empty. This suggests the context isn't properly parsing the text from the update.
Run the following script to investigate the validation logic:
🏁 Script executed:
#!/bin/bash
# Verify the webhook validation logic handles missing URL with TLS files
ast-grep --pattern 'if cfg.WebhookURL != "" {
$$$
}'Length of output: 541
🏁 Script executed:
#!/bin/bash
# Extract lines around the context operations test failure
sed -n '1000,1100p' bote_test.goLength of output: 2362
🏁 Script executed:
#!/bin/bash
# Find declarations of textWg and callbackWg in bote_test.go
grep -nE 'textWg|callbackWg' bote_test.goLength of output: 347
🏁 Script executed:
#!/bin/bash
# Extract setup and handlers for text and callback tests
sed -n '850,972p' bote_test.goLength of output: 2679
🏁 Script executed:
#!/bin/bash
# Extract handler registrations in TestContextOperations
start_line=$(grep -n 'func TestContextOperations' -n bote_test.go | cut -d: -f1)
end_line=$((start_line + 150))
sed -n "${start_line},${end_line}p" bote_test.goLength of output: 3567
🏁 Script executed:
#!/bin/bash
# Locate NewTestPoller and its SendTextMessage implementation
grep -R "func NewTestPoller" -n .
grep -R "type testPoller" -n .
grep -R "SendTextMessage" -n .
# Locate Context interface and Text() method implementations
grep -R "type Context" -n .
grep -R "func .*Text() string" -n .Length of output: 1052
🏁 Script executed:
#!/bin/bash
# Extract implementations of Text, ButtonID, Data, and DataParsed from context.go
sed -n '200,300p' context.goLength of output: 2639
🏁 Script executed:
#!/bin/bash
# Extract the testPoller definition and SendTextMessage implementation
sed -n '1,120p' bote_test.goLength of output: 2765
Fix webhook URL validation and test poller message‐ID
-
Add validation in options.go to reject TLS files when no WebhookURL is set. For example, after the existing
if cfg.WebhookURL != "" { … }
add:
} else if cfg.TLSKeyFile != "" || cfg.TLSCertFile != "" { return errm.New("tls files provided without webhook url") } -
Ensure the test poller assigns a non‐zero Message.ID so Text() returns the payload. In bote_test.go, update SendTextMessage:
func (p *testPoller) SendTextMessage(from tele.User, text string) { - p.SendUpdate(tele.Update{ - Message: &tele.Message{ - Sender: &from, - Text: text, - }, - }) + p.SendUpdate(tele.Update{ + Message: &tele.Message{ + ID: int(time.Now().Unix()), // ensure non-zero id + Sender: &from, + Text: text, + }, + }) }
These two changes should resolve the pipeline failures in the webhook validation tests and the text‐handling test.
🧰 Tools
🪛 GitHub Actions: Go
[error] 1259-1259: TestBotInitialization_PollerSelection/WebhookPoller_InvalidConfig_MissingURL failed: expected an error due to invalid webhook configuration (missing URL) but got nil.
🤖 Prompt for AI Agents
In options.go, add validation to reject TLSCertFile and TLSKeyFile if WebhookURL
is empty by checking if TLS files are set without a WebhookURL and returning an
error. In bote_test.go, update the test poller's SendTextMessage method to
assign a non-zero Message.ID so that calling Text() on the message returns the
payload correctly. These fixes will ensure webhook configuration validation is
correct and the test poller behaves as expected for text message handling.
📝 List of changes| File | Type | Diff | Description ||---|---|---|---| |
📝 List of changes
|
📝 List of changes
|
|
|
||
| To configure webhooks, you need to set the following fields in the `bote.Config` struct or use their corresponding environment variables: | ||
|
|
||
| - **`WebhookURL`** (`string`): The publicly accessible HTTPS URL for your webhook. Telegram will send updates to this URL. |
There was a problem hiding this comment.
🔒 Security improvement
Model confidence: very high (90-100%)
Issue priority: must be fixed immediately 🔴
Missing Webhook Secret Token Configuration
The documentation for webhook configuration omits the secret_token parameter, a crucial security feature provided by Telegram for verifying the authenticity of incoming webhook requests. Without configuring and validating this token, the bot's webhook endpoint is vulnerable to spoofed requests from any source, potentially leading to unauthorized command execution, data manipulation, or denial-of-service attacks by exhausting bot resources with fake updates. This is a fundamental security best practice for any public-facing webhook.
💡 Suggestion
Introduce a WebhookSecret field in the bote.Config struct (and its corresponding environment variable) to allow users to configure a shared secret. Clearly explain its purpose as a mechanism to verify that incoming updates truly originate from Telegram. Users should be instructed to generate a strong, random secret and configure it both in the bot and when calling Telegram's setWebhook API.
- **`ListenAddress`** (`string`): The IP address and port the bot will listen on for incoming webhook requests (e.g., `:8443` or `0.0.0.0:8080`).
- Environment variable: `BOTE_LISTEN_ADDRESS`
- Defaults to `:8443` if `WebhookURL` is set and `ListenAddress` is not.
+ - **`WebhookSecret`** (`string`, optional): A secret token to be sent with every webhook update. This helps verify that the update truly comes from Telegram. It should be a strong, random string.
+ - Environment variable: `BOTE_WEBHOOK_SECRET`
- **`TLSKeyFile`** (`string`, optional): Path to your TLS private key file. Required if you want the bot to handle HTTPS directly.
- Environment variable: `BOTE_TLS_KEY_FILE`
- **`TLSCertFile`** (`string`, optional): Path to your TLS public certificate file. Required if you want the bot to handle HTTPS directly.
- Environment variable: `BOTE_TLS_CERT_FILE`
| - If your bot is directly exposed to the internet and you want it to handle HTTPS encryption itself, provide both `TLSKeyFile` and `TLSCertFile`. | ||
| - If your bot is behind a reverse proxy (like Nginx or Caddy) that handles HTTPS termination, you typically don't need to set `TLSKeyFile` and `TLSCertFile` in the bot's configuration. The reverse proxy would forward plain HTTP traffic to the `ListenAddress` of the bot. | ||
|
|
||
| Make sure your `WebhookURL` is correctly pointing to where your bot is listening, and that your firewall/network configuration allows Telegram servers to reach your `ListenAddress`. |
There was a problem hiding this comment.
🔒 Security improvement
Model confidence: high (70-90%)
Issue priority: should be fixed soon 🟡
Lack of Specific Recommendation for IP Whitelisting
While the documentation correctly advises on general firewall configuration, it does not explicitly recommend whitelisting Telegram's known IP ranges for incoming webhook requests. This omission leaves the webhook endpoint unnecessarily exposed to the entire internet, increasing the attack surface. Implementing IP whitelisting is a strong security measure to ensure that only legitimate requests from Telegram's servers can reach the bot, significantly mitigating risks of DDoS attacks, port scanning, or malicious requests from unauthorized sources.
💡 Suggestion
Augment the network configuration advice to explicitly recommend configuring firewalls to accept incoming connections on the ListenAddress only from Telegram's official IP ranges. Advise users on where to find these ranges (e.g., Telegram Bot API documentation) as they are subject to change.
Make sure your `WebhookURL` is correctly pointing to where your bot is listening, and that your firewall/network configuration allows Telegram servers to reach your `ListenAddress`.
+ For enhanced security, it is highly recommended to configure your firewall to only accept incoming connections on the `ListenAddress` from Telegram's official IP ranges. These ranges are subject to change and can typically be found in Telegram's official Bot API documentation or by querying their API.
| Listen: opts.Config.ListenAddress, | ||
| Endpoint: &tele.WebhookEndpoint{PublicURL: opts.Config.WebhookURL}, | ||
| } | ||
| if opts.Config.TLSKeyFile != "" && opts.Config.TLSCertFile != "" { |
There was a problem hiding this comment.
🚨 Critical issue
Model confidence: very high (90-100%)
Issue priority: must be fixed immediately 🔴
Missing TLS Certificate/Key File Validation for Webhook Configuration
The code now supports webhook mode with optional TLS. While it checks if TLSKeyFile and TLSCertFile paths are provided, it does not validate if these files actually exist or are readable on the filesystem. If a user configures webhook mode with TLS and provides invalid, non-existent, or inaccessible file paths, the tele.Webhook server will likely fail to initialize or start when bot.Start(poller) is called. This can lead to a silent failure where the bot appears to start but cannot receive updates, or a runtime crash, making the system unreliable and difficult to debug in production.
💡 Suggestion
Before assigning the TLSKeyFile and TLSCertFile to the webhook configuration, perform explicit checks for their existence and readability using os.Stat. If a file is not found or is inaccessible, return an informative error early during the bot's initialization phase. This prevents runtime failures, provides clear feedback to the operator, and ensures the bot starts in a fully functional state.
import (
"fmt"
"os"
"time"
)
// Assuming this code is within a function that returns (*tele.Bot, error)
// ...
} else {
webhook := &tele.Webhook{
Listen: opts.Config.ListenAddress,
Endpoint: &tele.WebhookEndpoint{PublicURL: opts.Config.WebhookURL},
}
if opts.Config.TLSKeyFile != "" && opts.Config.TLSCertFile != "" {
// Validate TLS key file existence and readability
if _, err := os.Stat(opts.Config.TLSKeyFile); os.IsNotExist(err) {
return nil, fmt.Errorf("TLS key file not found or inaccessible: %s", opts.Config.TLSKeyFile)
}
// Validate TLS certificate file existence and readability
if _, err := os.Stat(opts.Config.TLSCertFile); os.IsNotExist(err) {
return nil, fmt.Errorf("TLS certificate file not found or inaccessible: %s", opts.Config.TLSCertFile)
}
webhook.TLS = &tele.WebhookTLS{
Key: opts.Config.TLSKeyFile,
Cert: opts.Config.TLSCertFile,
}
// Endpoint.Cert is used by Telegram to verify the certificate presented by the bot.
// It should be the public certificate file.
webhook.Endpoint.Cert = opts.Config.TLSCertFile
}
poller = webhook
}
// ... rest of the function
| Token: token, | ||
| Poller: tele.NewMiddlewarePoller(poller, b.middleware), | ||
| Client: &http.Client{Timeout: 2 * opts.Config.LPTimeout}, | ||
| Client: &http.Client{Timeout: 2 * opts.Config.LPTimeout}, // Consider if this timeout is appropriate for webhooks |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model confidence: high (70-90%)
Issue priority: should be fixed soon 🟡
Suboptimal HTTP Client Timeout Derivation for Webhook Mode
The http.Client timeout for the bot's outgoing requests to the Telegram API is currently set to 2 * opts.Config.LPTimeout. While this derivation might be suitable for long polling, it's not ideal or semantically correct for webhook-based deployments. In webhook mode, LPTimeout (Long Polling Timeout) is irrelevant, yet it's still used to determine the timeout for outgoing API calls. This could lead to an unnecessarily long or short timeout for API interactions, potentially causing delays or premature timeouts for critical operations like sending large files or receiving responses from Telegram. The accompanying comment is also misleading, suggesting it's related to incoming webhook handling.
💡 Suggestion
Decouple the http.Client timeout from LPTimeout. Introduce a dedicated configuration option (e.g., opts.Config.APITimeout) for the bot's outgoing API client, allowing it to be configured independently of the polling mechanism. This provides greater flexibility and allows for fine-tuning based on actual API usage patterns and network conditions. Update the comment to accurately reflect that this timeout applies to the bot's outgoing HTTP client for Telegram API calls.
// In opts.Config struct (example):
// type BotConfig struct {
// LPTimeout time.Duration // For Long Polling (if used)
// APITimeout time.Duration // New: For outgoing Telegram API calls
// WebhookURL string
// // ... other fields
// }
// In bot.go initialization:
// ...
var apiTimeout time.Duration
// Prefer a dedicated API timeout if configured
if opts.Config.APITimeout > 0 {
apiTimeout = opts.Config.APITimeout
} else {
// Fallback for existing configurations or if APITimeout is not set
apiTimeout = 2 * opts.Config.LPTimeout // Still uses LPTimeout as a fallback
if apiTimeout == 0 {
apiTimeout = 30 * time.Second // Sensible default if no timeout is specified
}
}
bot := &tele.Bot{
// ... other bot fields
Client: &http.Client{Timeout: apiTimeout}, // Timeout for outgoing requests to Telegram API
}
// ... rest of the function
| Config: bote.Config{ | ||
| WebhookURL: "https://test.com/webhook_tls", | ||
| ListenAddress: "127.0.0.1:8443", | ||
| TLSCertFile: "test_cert.pem", |
There was a problem hiding this comment.
⚠️ Potential bug
Model confidence: very high (90-100%)
Issue priority: should be fixed soon 🟡
Tests Dependent on External Files for TLS Configuration
The webhook poller tests for TLS (lines 1215-1216, 1269, 1282) use hardcoded string literals for certificate and key file paths ('test_cert.pem', 'test_key.pem'). This creates an implicit dependency on the existence and accessibility of these specific files in the test execution environment. Such dependencies make tests fragile, non-portable, and prone to flakiness in different development or CI/CD environments where these files might be missing, have incorrect permissions, or be located elsewhere. Robust unit/integration tests should be self-contained and not rely on external file system state.
💡 Suggestion
To ensure test reliability and portability, create temporary dummy certificate and key files within the test function using t.TempDir(). This guarantees that the necessary files are always available during the test run and are automatically cleaned up afterward, eliminating external dependencies and making tests self-contained.
import (
"os"
"path/filepath"
"testing"
"github.com/stretchr/testify/assert"
tele "gopkg.in/telebot.v3"
)
func TestBotInitialization_PollerSelection(t *testing.T) {
// ... (other test cases)
t.Run("WebhookPoller_WithTLS", func(t *testing.T) {
// Create temporary dummy cert and key files
tempDir := t.TempDir()
certFilePath := filepath.Join(tempDir, "test_cert.pem")
keyFilePath := filepath.Join(tempDir, "test_key.pem")
err := os.WriteFile(certFilePath, []byte("dummy_cert_content"), 0644)
assert.NoError(t, err, "Failed to create dummy cert file")
err = os.WriteFile(keyFilePath, []byte("dummy_key_content"), 0644)
assert.NoError(t, err, "Failed to create dummy key file")
opts := bote.Options{
Config: bote.Config{
WebhookURL: "https://test.com/webhook_tls",
ListenAddress: "127.0.0.1:8443",
TLSCertFile: certFilePath, // Use temporary file path
TLSKeyFile: keyFilePath, // Use temporary file path
TestMode: true,
},
}
boteBot, err := bote.NewWithOptions("test_token", opts)
assert.NoError(t, err)
// ... rest of the assertions for webhookPoller.Endpoint.Cert and webhookPoller.TLS
})
// Apply similar changes to other TLS-related invalid config tests:
// WebhookPoller_InvalidConfig_MissingURL
// WebhookPoller_InvalidConfig_OnlyOneTLSFile_Key
// WebhookPoller_InvalidConfig_OnlyOneTLSFile_Cert
}
| func TestBotInitialization_PollerSelection(t *testing.T) { | ||
| // Test Case 1: Default (Long Poller) | ||
| t.Run("DefaultLongPoller", func(t *testing.T) { | ||
| boteBot, err := bote.New("test_token", bote.WithTestMode(NewTestPoller())) // Using TestMode to avoid actual API calls & provide a poller |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model confidence: high (70-90%)
Issue priority: could be fixed later 🟢
Redundant Bot Initialization in Default Poller Test
The DefaultLongPoller test case unnecessarily initializes a boteBot using bote.New with a NewTestPoller() option (line 1148). Immediately after, it creates a second defaultBot using bote.NewWithOptions (line 1164) to actually test the default long poller behavior. The first boteBot is never used for its intended purpose of verifying the default poller, making its creation redundant and potentially confusing to future readers. This adds unnecessary overhead and obfuscates the test's true intent.
💡 Suggestion
Remove the initial, unused bote.New call. Directly create the bot instance that is specifically configured for testing the default long poller behavior. This simplifies the test logic, improves clarity, and removes unnecessary object instantiation.
func TestBotInitialization_PollerSelection(t *testing.T) {
t.Run("DefaultLongPoller", func(t *testing.T) {
opts := bote.Options{
Config: bote.Config{TestMode: true}, // Keep TestMode to prevent actual polling
}
boteBot, err := bote.NewWithOptions("test_token", opts) // Directly create the bot for default poller test
assert.NoError(t, err)
// Flatten assertions for better readability
assert.NotNil(t, boteBot)
teleBot := boteBot.Bot()
assert.NotNil(t, teleBot)
assert.NotNil(t, teleBot.Poller)
middlewarePoller, ok := teleBot.Poller.(*tele.MiddlewarePoller)
assert.True(t, ok, "Poller should be MiddlewarePoller")
assert.IsType(t, &tele.LongPoller{}, middlewarePoller.Poller, "Underlying poller should be LongPoller")
})
// ... (other test cases)
}
| } | ||
| boteBot, err := bote.NewWithOptions("test_token", opts) | ||
| assert.NoError(t, err) | ||
| if assert.NotNil(t, boteBot) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model confidence: medium (40-70%)
Issue priority: could be fixed later 🟢
Excessive Nested Assertions Reduce Test Readability
The test cases, particularly the webhook poller tests (e.g., lines 1190-1206, 1224-1243), employ deeply nested if assert.NotNil(t, obj) { ... } and if assert.True(t, ok) { ... } blocks. While functionally correct, this nesting significantly reduces test readability and makes it harder to quickly grasp the test's intent or debug failures. testify/assert functions are designed to report failures to t and typically stop the current sub-test execution upon failure, making deep nesting often unnecessary and counterproductive.
💡 Suggestion
Flatten the assertion structure by relying on the default failure-reporting behavior of assert functions. If an assertion fails, the subsequent code in that t.Run block will not execute, which is usually the desired behavior. This improves readability, reduces visual clutter, and makes tests easier to maintain and debug.
func TestBotInitialization_PollerSelection(t *testing.T) {
// ... (other test cases)
t.Run("WebhookPoller_NoTLS", func(t *testing.T) {
opts := bote.Options{
Config: bote.Config{
WebhookURL: "https://test.com/webhook",
ListenAddress: "127.0.0.1:8080",
TestMode: true,
},
}
boteBot, err := bote.NewWithOptions("test_token", opts)
assert.NoError(t, err)
assert.NotNil(t, boteBot)
teleBot := boteBot.Bot()
assert.NotNil(t, teleBot)
assert.NotNil(t, teleBot.Poller)
middlewarePoller, ok := teleBot.Poller.(*tele.MiddlewarePoller)
assert.True(t, ok, "Poller should be MiddlewarePoller")
webhookPoller, ok := middlewarePoller.Poller.(*tele.Webhook)
assert.True(t, ok, "Underlying poller should be Webhook")
assert.Equal(t, "127.0.0.1:8080", webhookPoller.Listen)
assert.NotNil(t, webhookPoller.Endpoint)
assert.Equal(t, "https://test.com/webhook", webhookPoller.Endpoint.PublicURL)
assert.Empty(t, webhookPoller.Endpoint.Cert, "Endpoint.Cert should be empty for NoTLS case")
assert.Nil(t, webhookPoller.TLS, "TLS config should be nil for NoTLS case")
})
// ... (other test cases)
}
| if _, err := url.ParseRequestURI(cfg.WebhookURL); err != nil { | ||
| return errm.Wrap(err, "invalid webhook url") | ||
| } | ||
| cfg.ListenAddress = lang.Check(cfg.ListenAddress, ":8443") |
There was a problem hiding this comment.
⚠️ Potential bug
Model confidence: very high (90-100%)
Issue priority: should be fixed soon 🟡
ListenAddress default is conditionally applied based on WebhookURL presence
The ListenAddress field is only assigned its default value ":8443" if cfg.WebhookURL is not empty. This creates an implicit dependency where the listener address might not be configured if the WebhookURL (which could be an outbound URL for sending webhooks) is not specified. If ListenAddress is intended for an inbound webhook listener, its default should be applied unconditionally or based on its own specific logic, independent of WebhookURL. This can lead to unexpected server startup failures or incorrect listening behavior if WebhookURL is optional or serves a different purpose.
💡 Suggestion
Decouple the default assignment of ListenAddress from the WebhookURL validation. Apply the default for ListenAddress unconditionally if it's empty, or tie it to the component that uses ListenAddress (e.g., if a webhook server is enabled) rather than an unrelated configuration.
func (cfg *Options) Validate() error {
// ... other validations ...
// Apply default for ListenAddress unconditionally if it's empty.
// This assumes ListenAddress is for an inbound server that should always have a default.
cfg.ListenAddress = lang.Check(cfg.ListenAddress, ":8443")
if cfg.WebhookURL != "" {
// Original WebhookURL validation remains here
if _, err := url.ParseRequestURI(cfg.WebhookURL); err != nil {
return errm.Wrap(err, "invalid webhook url")
}
}
if (cfg.TLSKeyFile == "") != (cfg.TLSCertFile == "") {
return errm.New("tls key and cert files must be both provided or both empty")
}
// ... rest of the validation ...
return nil
}
| if (cfg.TLSKeyFile == "") != (cfg.TLSCertFile == "") { | ||
| return errm.New("tls key and cert files must be both provided or both empty") | ||
| } | ||
| } |
There was a problem hiding this comment.
🔄 Other issue
Model confidence: high (70-90%)
Issue priority: should be fixed soon 🟡
Missing file existence checks for TLS key and certificate files
The configuration checks if TLSKeyFile and TLSCertFile are both provided or both empty. However, it does not verify if the specified file paths actually exist on the filesystem or if the application has the necessary permissions to read them. This can lead to a runtime error when the application attempts to load the TLS configuration, rather than failing early and gracefully during configuration validation. Failing early provides a better user experience and simplifies debugging.
💡 Suggestion
After validating that both TLS file paths are either present or absent, add checks to confirm the existence of the files using os.Stat. This ensures that the application can find and potentially read the files before attempting to start the TLS listener.
import "os"
// ... existing code ...
func (cfg *Options) Validate() error {
// ... existing validations ...
if (cfg.TLSKeyFile == "") != (cfg.TLSCertFile == "") {
return errm.New("tls key and cert files must be both provided or both empty")
}
// Only check existence if files are provided
if cfg.TLSKeyFile != "" {
if _, err := os.Stat(cfg.TLSKeyFile); os.IsNotExist(err) {
return errm.Wrapf(err, "TLS key file not found: %s", cfg.TLSKeyFile)
}
}
if cfg.TLSCertFile != "" {
if _, err := os.Stat(cfg.TLSCertFile); os.IsNotExist(err) {
return errm.Wrapf(err, "TLS cert file not found: %s", cfg.TLSCertFile)
}
}
return nil
}
| cfg.UserCacheTTL = lang.Check(cfg.UserCacheTTL, 24*time.Hour) | ||
|
|
||
| if cfg.WebhookURL != "" { | ||
| if _, err := url.ParseRequestURI(cfg.WebhookURL); err != nil { |
There was a problem hiding this comment.
🔒 Security improvement
Model confidence: high (70-90%)
Issue priority: must be fixed immediately 🔴
WebhookURL validation does not mitigate Server-Side Request Forgery (SSRF)
The WebhookURL is validated using url.ParseRequestURI, which only checks for a syntactically valid URI. If this WebhookURL is used by the application to make outbound requests (e.g., sending notifications to an external webhook endpoint), an attacker who can control or influence this configuration value could potentially direct the application to make requests to internal services, metadata endpoints (e.g., AWS EC2 metadata service 169.254.169.254), or other sensitive internal resources. This is known as Server-Side Request Forgery (SSRF) and can lead to information disclosure, unauthorized actions, or network scanning.
💡 Suggestion
Implement a robust allow-list or deny-list approach for WebhookURL destinations. For external webhooks, typically only http or https schemes should be allowed. Additionally, prevent requests to private IP ranges (RFC1918 addresses), loopback addresses, and link-local addresses. This involves resolving the hostname and checking the resolved IP addresses against a list of forbidden ranges.
import (
"net"
"net/url"
"strings"
// "errm" and "lang" are assumed to be available
)
// isPrivateIP checks if an IP address is a private, loopback, or link-local address.
func isPrivateIP(ip net.IP) bool {
if ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() {
return true
}
// RFC1918 private networks
if ip4 := ip.To4(); ip4 != nil {
if ip4[0] == 10 { // 10.0.0.0/8
return true
}
if ip4[0] == 172 && (ip4[1] >= 16 && ip4[1] <= 31) { // 172.16.0.0/12
return true
}
if ip4[0] == 192 && ip4[1] == 168 { // 192.168.0.0/16
return true
}
}
// Add IPv6 private ranges if necessary (e.g., fc00::/7 - Unique Local Addresses)
return false
}
func (cfg *Options) Validate() error {
// ... other validations ...
if cfg.WebhookURL != "" {
parsedURL, err := url.ParseRequestURI(cfg.WebhookURL)
if err != nil {
return errm.Wrap(err, "invalid webhook url format")
}
// 1. Scheme validation for outbound requests
if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" {
return errm.Newf("unsupported webhook URL scheme: %s. Only http and https are allowed.", parsedURL.Scheme)
}
// 2. Hostname resolution and IP validation (SSRF prevention)
if parsedURL.Hostname() == "" {
return errm.New("webhook URL must have a hostname")
}
ips, err := net.LookupIP(parsedURL.Hostname())
if err != nil {
// Handle temporary DNS resolution issues gracefully, or fail if critical
return errm.Wrapf(err, "failed to resolve webhook URL hostname: %s", parsedURL.Hostname())
}
for _, ip := range ips {
if isPrivateIP(ip) {
return errm.Newf("webhook URL resolves to a private IP address, which is forbidden: %s (%s)", parsedURL.Hostname(), ip.String())
}
}
}
// ... rest of the validation ...
return nil
}
| To configure webhooks, you need to set the following fields in the `bote.Config` struct or use their corresponding environment variables: | ||
|
|
||
| - **`WebhookURL`** (`string`): The publicly accessible HTTPS URL for your webhook. Telegram will send updates to this URL. | ||
| - Environment variable: `BOTE_WEBHOOK_URL` |
There was a problem hiding this comment.
🔒 Security improvement
Model confidence: very high (90-100%)
Issue priority: should be fixed soon 🟡
Missing Webhook Secret Token Recommendation for Security
The newly added documentation for webhook configuration does not mention the importance of using a secret_token in the WebhookURL or as a separate configuration option. Telegram's setWebhook API allows setting a secret_token (via the secret_token parameter), which the bot should then verify in incoming webhook requests by checking the X-Telegram-Bot-Api-Secret-Token HTTP header. Without this verification, any party knowing the WebhookURL can send arbitrary data to the bot's endpoint, potentially leading to denial-of-service, resource exhaustion, or unexpected behavior if the bot doesn't validate the source of the updates. This is a critical security measure to ensure that only legitimate updates from Telegram are processed, preventing webhook spoofing and unauthorized interactions.
💡 Suggestion
Enhance the webhook configuration documentation by adding a recommendation to include a secret_token in the bote.Config struct. Clearly explain its purpose for verifying the origin of webhook requests and emphasize that this token should be a strong, randomly generated string. The bote framework should internally handle setting this token with Telegram and verifying it on incoming requests.
--- a/README.md
+++ b/README.md
@@ -138,6 +138,9 @@
- **`WebhookURL`** (`string`): The publicly accessible HTTPS URL for your webhook. Telegram will send updates to this URL.
- Environment variable: `BOTE_WEBHOOK_URL`
- **`ListenAddress`** (`string`): The IP address and port the bot will listen on for incoming webhook requests (e.g., `:8443` or `0.0.0.0:8080`).
- Environment variable: `BOTE_LISTEN_ADDRESS`
- Defaults to `:8443` if `WebhookURL` is set and `ListenAddress` is not.
+- **`SecretToken`** (`string`, optional): A secret token to be sent with every webhook update. Used by Telegram and expected by your bot to verify the origin of updates.
+ - Environment variable: `BOTE_SECRET_TOKEN`
+ - **Security Note**: Highly recommended for production deployments to prevent webhook spoofing. This token should be a strong, randomly generated string.
- **`TLSKeyFile`** (`string`, optional): Path to your TLS private key file. Required if you want the bot to handle HTTPS directly.
- Environment variable: `BOTE_TLS_KEY_FILE`
- **`TLSCertFile`** (`string`, optional): Path to your TLS public certificate file. Required if you want the bot to handle HTTPS directly.
| if opts.Poller != nil { | ||
| poller = opts.Poller | ||
| } | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model confidence: very high (90-100%)
Issue priority: should be fixed soon 🟡
Incomplete Webhook Configuration Validation and Implicit Defaults
The newly introduced webhook setup logic directly uses opts.Config.WebhookURL and opts.Config.ListenAddress without comprehensive validation. An invalid WebhookURL (e.g., malformed, or non-HTTPS in production) can lead to runtime errors or security risks. Furthermore, if WebhookURL is provided but ListenAddress is empty, the underlying telebot library implicitly defaults to listening on ":80" or ":443". This implicit behavior can cause unexpected port conflicts, require elevated privileges, or result in the bot failing to start predictably, making deployment and debugging challenging.
💡 Suggestion
Implement explicit validation for WebhookURL (e.g., ensuring it's a valid URL and uses HTTPS for production environments). Additionally, provide clear, configurable defaults for ListenAddress or make it a mandatory field when WebhookURL is set, to avoid reliance on implicit telebot defaults. This validation and defaulting should ideally be handled within the prepareOpts function to centralize configuration logic.
import (
// ... existing imports ...
"net/url" // Add this import to bot.go or options.go
)
// In prepareOpts(opts Options) (Options, error) function:
// (Assuming prepareOpts is where configuration options are prepared and validated)
// ... existing prepareOpts logic ...
// Webhook specific validation and defaults
if opts.Config.WebhookURL != "" {
parsedURL, err := url.ParseRequestURI(opts.Config.WebhookURL)
if err != nil {
return opts, errm.Wrap(err, "invalid WebhookURL format")
}
// Enforce HTTPS for production webhooks. Consider adding a flag for local dev HTTP.
if parsedURL.Scheme != "https" {
return opts, errm.New("WebhookURL must use HTTPS scheme for production environments")
}
if opts.Config.ListenAddress == "" {
// Explicitly set a default ListenAddress to avoid telebot's implicit defaults.
if opts.Config.TLSKeyFile != "" && opts.Config.TLSCertFile != "" {
opts.Config.ListenAddress = ":8443" // Standard HTTPS webhook port
} else {
opts.Config.ListenAddress = ":8080" // Common HTTP port for webhooks behind a proxy
}
}
}
return opts, nil
| Token: token, | ||
| Poller: tele.NewMiddlewarePoller(poller, b.middleware), | ||
| Client: &http.Client{Timeout: 2 * opts.Config.LPTimeout}, | ||
| Client: &http.Client{Timeout: 2 * opts.Config.LPTimeout}, // Consider if this timeout is appropriate for webhooks |
There was a problem hiding this comment.
🚀 Performance improvement
Model confidence: high (70-90%)
Issue priority: could be fixed later 🟢
http.Client Timeout Mismatch for Webhook Mode
The http.Client used by telebot for its outgoing Telegram API requests (e.g., sending messages, setting webhook info) is configured with a timeout derived from opts.Config.LPTimeout (Long Poller Timeout). In webhook mode, long polling is not active, making LPTimeout semantically irrelevant for outgoing API calls. Reusing this timeout can lead to either excessively long blocking calls (if LPTimeout is large) or premature timeouts (if LPTimeout is small) for critical Telegram API interactions, impacting bot responsiveness and reliability.
💡 Suggestion
Introduce a dedicated configuration option (e.g., APICallTimeout) within Options.Config specifically for the http.Client timeout. This allows for independent and appropriate tuning of outgoing API request behavior, ensuring robust communication with the Telegram API regardless of the chosen update mechanism.
// In options.go (add to Config struct):
type Config {
// ... existing fields ...
APICallTimeout time.Duration // New field for outgoing API calls
}
// In prepareOpts(opts Options) (Options, error) function:
// (Set a sensible default for the new field)
if opts.Config.APICallTimeout == 0 {
opts.Config.APICallTimeout = 15 * time.Second // A reasonable default for outgoing Telegram API requests
}
// In newBaseBot(token string, opts Options) (*baseBot, error) function, modify line 567:
Client: &http.Client{Timeout: opts.Config.APICallTimeout},
| func TestBotInitialization_PollerSelection(t *testing.T) { | ||
| // Test Case 1: Default (Long Poller) | ||
| t.Run("DefaultLongPoller", func(t *testing.T) { | ||
| boteBot, err := bote.New("test_token", bote.WithTestMode(NewTestPoller())) // Using TestMode to avoid actual API calls & provide a poller |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model confidence: high (70-90%)
Issue priority: could be fixed later 🟢
Redundant Bot Initialization in Default Poller Test
In the 'DefaultLongPoller' subtest, the initial bote.New call on line 1148 explicitly provides a NewTestPoller() via bote.WithTestMode. This bypasses the actual default poller selection logic that the test aims to verify. The subsequent block (lines 1161-1174) correctly creates a bot without an explicit poller to test the default behavior. The initial call is therefore redundant and slightly misleading regarding the test's intent.
💡 Suggestion
Remove the initial bote.New call and its associated assertions (lines 1148-1150) within the 'DefaultLongPoller' subtest. The subsequent code block (lines 1161-1174) already correctly tests the default LongPoller selection, making the first initialization unnecessary. This will improve the clarity and conciseness of the test.
func TestBotInitialization_PollerSelection(t *testing.T) {
t.Run("DefaultLongPoller", func(t *testing.T) {
// Remove the following lines:
// boteBot, err := bote.New("test_token", bote.WithTestMode(NewTestPoller()))
// assert.NoError(t, err)
// if assert.NotNil(t, boteBot) {
// teleBot := boteBot.Bot()
// if assert.NotNil(t, teleBot) && assert.NotNil(t, teleBot.Poller) {
// ... (rest of the original code for default poller test)
opts := bote.Options{
Config: bote.Config{TestMode: true},
}
defaultBot, err := bote.NewWithOptions("test_token", opts)
assert.NoError(t, err)
if assert.NotNil(t, defaultBot) {
actualTeleBot := defaultBot.Bot()
if assert.NotNil(t, actualTeleBot) && assert.NotNil(t, actualTeleBot.Poller) {
middlewarePoller, ok := actualTeleBot.Poller.(*tele.MiddlewarePoller)
if assert.True(t, ok, "Poller should be MiddlewarePoller") {
assert.IsType(t, &tele.LongPoller{}, middlewarePoller.Poller, "Underlying poller should be LongPoller")
}
}
}
// }
// }
})
// ... (rest of the test cases)
}
|
|
||
| To configure webhooks, you need to set the following fields in the `bote.Config` struct or use their corresponding environment variables: | ||
|
|
||
| - **`WebhookURL`** (`string`): The publicly accessible HTTPS URL for your webhook. Telegram will send updates to this URL. |
There was a problem hiding this comment.
🔒 Security improvement
Model confidence: very high (90-100%)
Issue priority: must be fixed immediately 🔴
Missing Webhook Secret Token Configuration
The documentation for webhook configuration omits a critical security feature: the secret_token parameter. Telegram's setWebhook method allows a secret_token to be set, which Telegram then includes in the X-Telegram-Bot-Api-Secret-Token header for every incoming webhook update. Verifying this token is crucial to ensure that webhook requests originate from Telegram and not from malicious actors attempting to spoof requests. Without this, the bot's webhook endpoint is vulnerable to unauthenticated requests, potentially leading to denial-of-service, data manipulation, or unauthorized command execution if the bot's logic isn't robustly secured otherwise. This is a fundamental security best practice for Telegram webhooks.
💡 Suggestion
Add SecretToken as a mandatory configuration field for webhooks. The bot's implementation must then validate the X-Telegram-Bot-Api-Secret-Token header against this configured value for every incoming request. Requests with a missing or mismatched token should be rejected immediately. This significantly enhances the security posture of the webhook endpoint.
--- a/README.md
+++ b/README.md
@@ -137,6 +137,9 @@
- **`WebhookURL`** (`string`): The publicly accessible HTTPS URL for your webhook. Telegram will send updates to this URL.
- Environment variable: `BOTE_WEBHOOK_URL`
+- **`SecretToken`** (`string`, optional): A secret token that Telegram will send in the `X-Telegram-Bot-Api-Secret-Token` header. Used to verify the authenticity of webhook requests.
+ - Environment variable: `BOTE_SECRET_TOKEN`
+ - **Security Best Practice**: Always set a strong, random secret token to prevent spoofed webhook requests.
- **`ListenAddress`** (`string`): The IP address and port the bot will listen on for incoming webhook requests (e.g., `:8443` or `0.0.0.0:8080`).
- Environment variable: `BOTE_LISTEN_ADDRESS`
- Defaults to `:8443` if `WebhookURL` is set and `ListenAddress` is not.
| - If your bot is directly exposed to the internet and you want it to handle HTTPS encryption itself, provide both `TLSKeyFile` and `TLSCertFile`. | ||
| - If your bot is behind a reverse proxy (like Nginx or Caddy) that handles HTTPS termination, you typically don't need to set `TLSKeyFile` and `TLSCertFile` in the bot's configuration. The reverse proxy would forward plain HTTP traffic to the `ListenAddress` of the bot. | ||
|
|
||
| Make sure your `WebhookURL` is correctly pointing to where your bot is listening, and that your firewall/network configuration allows Telegram servers to reach your `ListenAddress`. |
There was a problem hiding this comment.
🔒 Security improvement
Model confidence: high (70-90%)
Issue priority: should be fixed soon 🟡
Missing Recommendation for IP Whitelisting
The documentation correctly advises ensuring firewall/network configuration allows Telegram servers to reach the ListenAddress. However, it misses a crucial security hardening step: recommending that users whitelist Telegram's official IP ranges for incoming webhook traffic. Telegram publishes a list of IP addresses from which webhook updates originate. Restricting inbound traffic to only these known IP ranges significantly reduces the attack surface, preventing arbitrary internet hosts from attempting to connect to the webhook endpoint, even if a secret token is used.
💡 Suggestion
Add a recommendation to configure firewalls to only accept incoming connections on the ListenAddress from Telegram's official IP ranges. Provide a link or instructions on where to find these ranges (e.g., via Telegram's getWebhookInfo method or official documentation). This is a defense-in-depth measure that complements the secret_token.
--- a/README.md
+++ b/README.md
@@ -152,4 +152,6 @@
Make sure your `WebhookURL` is correctly pointing to where your bot is listening, and that your firewall/network configuration allows Telegram servers to reach your `ListenAddress`.
+**Security Recommendation**: For enhanced security, configure your firewall to only accept incoming connections on the `ListenAddress` from Telegram's official IP ranges. These ranges can be obtained via the `getWebhookInfo` API method or Telegram's documentation.
+
| - **`ListenAddress`** (`string`): The IP address and port the bot will listen on for incoming webhook requests (e.g., `:8443` or `0.0.0.0:8080`). | ||
| - Environment variable: `BOTE_LISTEN_ADDRESS` | ||
| - Defaults to `:8443` if `WebhookURL` is set and `ListenAddress` is not. | ||
| - **`TLSKeyFile`** (`string`, optional): Path to your TLS private key file. Required if you want the bot to handle HTTPS directly. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model confidence: medium (40-70%)
Issue priority: could be fixed later 🟢
Ambiguity in TLS File Requirement for Direct HTTPS
The documentation states that TLSKeyFile and TLSCertFile are 'Required if you want the bot to handle HTTPS directly.' However, it doesn't explicitly clarify the consequence if these files are not provided when direct HTTPS is intended (i.e., no reverse proxy is used, but WebhookURL is HTTPS). This ambiguity could lead to misconfiguration: the bot might attempt to start an insecure HTTP server on a public port, fail to start, or behave unexpectedly. A user might assume the bot will automatically handle TLS if WebhookURL is HTTPS, leading to runtime errors or an insecure setup.
💡 Suggestion
Clarify that if WebhookURL is set to an HTTPS URL and no reverse proxy is used, then TLSKeyFile and TLSCertFile are strictly mandatory for the bot to function correctly and securely. Explicitly state that the bot will fail to start or refuse to serve HTTPS if these files are missing in such a scenario. This prevents silent failures or insecure deployments.
--- a/README.md
+++ b/README.md
@@ -142,8 +142,8 @@
- Environment variable: `BOTE_LISTEN_ADDRESS`
- Defaults to `:8443` if `WebhookURL` is set and `ListenAddress` is not.
- **`TLSKeyFile`** (`string`, optional): Path to your TLS private key file. Required if you want the bot to handle HTTPS directly.
-- Environment variable: `BOTE_TLS_KEY_FILE`
+- Environment variable: `BOTE_TLS_KEY_FILE`\n+ - **Note**: If `WebhookURL` is HTTPS and you are not using a reverse proxy, this field is mandatory for the bot to serve HTTPS.
- **`TLSCertFile`** (`string`, optional): Path to your TLS public certificate file. Required if you want the bot to handle HTTPS directly.
-- Environment variable: `BOTE_TLS_CERT_FILE`
+- Environment variable: `BOTE_TLS_CERT_FILE`\n+ - **Note**: If `WebhookURL` is HTTPS and you are not using a reverse proxy, this field is mandatory for the bot to serve HTTPS.
**When to use TLS Key/Cert files:**
| Listen: opts.Config.ListenAddress, | ||
| Endpoint: &tele.WebhookEndpoint{PublicURL: opts.Config.WebhookURL}, | ||
| } | ||
| if opts.Config.TLSKeyFile != "" && opts.Config.TLSCertFile != "" { |
There was a problem hiding this comment.
🚨 Critical issue
Model confidence: very high (90-100%)
Issue priority: must be fixed immediately 🔴
Unhandled TLS Key/Certificate File Read Errors
The code attempts to configure TLS for the webhook using opts.Config.TLSKeyFile and opts.Config.TLSCertFile without prior validation of file existence or readability. If either file is missing, corrupted, or has incorrect permissions, the tele.Webhook might fail to initialize or start, leading to a runtime panic or a silent failure later when the bot attempts to start the webhook server. This can cause critical service downtime, is difficult to debug, and compromises the reliability of the bot in webhook mode.
💡 Suggestion
Before assigning the file paths to webhook.TLS and webhook.Endpoint.Cert, verify the existence and readability of both the key and certificate files. If an error occurs during this check, return it immediately to prevent the bot from starting in a misconfigured or unstable state. This shifts the error detection to an earlier, more manageable phase, ensuring robust startup.
import (
"fmt"
"os"
)
// Assuming this code is within a function that returns (*tele.Bot, error)
// or similar, allowing error propagation.
} else {
webhook := &tele.Webhook{
Listen: opts.Config.ListenAddress,
Endpoint: &tele.WebhookEndpoint{PublicURL: opts.Config.WebhookURL},
}
if opts.Config.TLSKeyFile != "" && opts.Config.TLSCertFile != "" {
// Validate TLS key and certificate files
if _, err := os.Stat(opts.Config.TLSKeyFile); os.IsNotExist(err) {
return nil, fmt.Errorf("TLS key file not found or inaccessible: %s", opts.Config.TLSKeyFile)
}
if _, err := os.Stat(opts.Config.TLSCertFile); os.IsNotExist(err) {
return nil, fmt.Errorf("TLS certificate file not found or inaccessible: %s", opts.Config.TLSCertFile)
}
webhook.TLS = &tele.WebhookTLS{
Key: opts.Config.TLSKeyFile,
Cert: opts.Config.TLSCertFile,
}
// Endpoint.Cert is used by Telegram to verify the certificate presented by the bot.
// It should be the public certificate file.
webhook.Endpoint.Cert = opts.Config.TLSCertFile
}
poller = webhook
}
| Token: token, | ||
| Poller: tele.NewMiddlewarePoller(poller, b.middleware), | ||
| Client: &http.Client{Timeout: 2 * opts.Config.LPTimeout}, | ||
| Client: &http.Client{Timeout: 2 * opts.Config.LPTimeout}, // Consider if this timeout is appropriate for webhooks |
There was a problem hiding this comment.
🚀 Performance improvement
Model confidence: high (70-90%)
Issue priority: should be fixed soon 🟡
Shared and Potentially Excessive HTTP Client Timeout
The http.Client timeout is uniformly set to 2 * opts.Config.LPTimeout. While this long duration is suitable for tele.LongPoller (which maintains long-lived connections for updates), it is also applied to the tele.Bot's internal HTTP client used for outgoing API requests (e.g., sending messages) when in webhook mode. An excessively long timeout for these outgoing calls can lead to resource exhaustion if many requests hang, or significantly delay error propagation for failed API calls, impacting the bot's responsiveness and overall reliability.
💡 Suggestion
Decouple the http.Client timeout from the LPTimeout. Introduce a separate, more appropriate configuration option (e.g., APIRequestTimeout) for outgoing API call timeouts, especially for webhook mode. A typical timeout for such requests would be in the range of 5-30 seconds, not several minutes as implied by a typical LPTimeout.
import (
"net/http"
"time"
)
// ... inside the bot initialization function
Client: &http.Client{
Timeout: func() time.Duration {
if opts.Config.WebhookURL == "" { // Long Polling mode
return 2 * opts.Config.LPTimeout
} else { // Webhook mode
// Use a dedicated API request timeout if available, otherwise a sensible default
if opts.Config.APIRequestTimeout > 0 { // Assuming APIRequestTimeout is a new config field
return opts.Config.APIRequestTimeout
}
return 15 * time.Second // Sensible default for outgoing API calls
}
}(),
},
| poller = opts.Poller | ||
| } | ||
| } else { | ||
| webhook := &tele.Webhook{ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model confidence: high (70-90%)
Issue priority: could be fixed later 🟢
Missing Validation for Webhook URL and Listen Address
The WebhookURL and ListenAddress parameters are used directly from opts.Config without any explicit validation. A malformed WebhookURL will prevent Telegram from successfully sending updates to the bot. An invalid ListenAddress (e.g., syntactically incorrect, or a port already in use) will cause the webhook server to fail to bind, leading to bot startup failure. While the underlying tele library or Go's net/http might return errors during Start(), validating these critical configuration parameters upfront provides clearer error messages and prevents runtime surprises, improving overall system reliability.
💡 Suggestion
Implement explicit validation for opts.Config.WebhookURL (e.g., parse it with net/url to ensure it's a valid URI) and opts.Config.ListenAddress (e.g., use net.SplitHostPort to check its format). This pre-validation ensures that the bot attempts to start with correct and robust configuration, making debugging easier and preventing unexpected failures.
import (
"fmt"
"net"
"net/url"
)
// Assuming this code is within a function that returns (*tele.Bot, error)
// or similar, allowing error propagation.
if opts.Config.WebhookURL == "" {
// ... long poller setup ...
} else {
// Validate WebhookURL format
if _, err := url.ParseRequestURI(opts.Config.WebhookURL); err != nil {
return nil, fmt.Errorf("invalid WebhookURL format: %w", err)
}
// Validate ListenAddress format (e.g., host:port)
if _, _, err := net.SplitHostPort(opts.Config.ListenAddress); err != nil {
return nil, fmt.Errorf("invalid ListenAddress format (expected host:port): %w", err)
}
webhook := &tele.Webhook{
Listen: opts.Config.ListenAddress,
Endpoint: &tele.WebhookEndpoint{PublicURL: opts.Config.WebhookURL},
}
// ... rest of webhook setup including TLS validation ...
poller = webhook
}
| func TestBotInitialization_PollerSelection(t *testing.T) { | ||
| // Test Case 1: Default (Long Poller) | ||
| t.Run("DefaultLongPoller", func(t *testing.T) { | ||
| boteBot, err := bote.New("test_token", bote.WithTestMode(NewTestPoller())) // Using TestMode to avoid actual API calls & provide a poller |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model confidence: very high (90-100%)
Issue priority: could be fixed later 🟢
Redundant Bot Initialization and Misleading Logic in Default Poller Test
The 'DefaultLongPoller' subtest initializes a boteBot on line 1148 using bote.New with bote.WithTestMode(NewTestPoller()). This explicitly provides a test poller, which contradicts the stated goal of testing the default poller behavior when no specific poller is provided. The actual verification of the default poller type is performed on a separate defaultBot created later (line 1164). The initial boteBot and its associated assertions (lines 1149-1152) are effectively dead code for the primary purpose of this subtest, leading to unnecessary object creation, confusing test intent, and reduced readability.
💡 Suggestion
Remove the initial, redundant boteBot initialization and its related assertions (lines 1148-1152). The test should directly proceed to create the defaultBot using bote.NewWithOptions with TestMode: true (without a specific poller option) to accurately verify the default poller type. This clarifies the test's purpose and removes unnecessary setup.
func TestBotInitialization_PollerSelection(t *testing.T) {
t.Run("DefaultLongPoller", func(t *testing.T) {
// For the purpose of this test, we'll create a bot *without* WithTestMode's poller to inspect the default.
opts := bote.Options{
Config: bote.Config{TestMode: true}, // Keep TestMode to prevent actual polling
}
defaultBot, err := bote.NewWithOptions("test_token", opts)
assert.NoError(t, err)
if assert.NotNil(t, defaultBot) {
actualTeleBot := defaultBot.Bot()
if assert.NotNil(t, actualTeleBot) && assert.NotNil(t, actualTeleBot.Poller) {
middlewarePoller, ok := actualTeleBot.Poller.(*tele.MiddlewarePoller)
if assert.True(t, ok, "Poller should be MiddlewarePoller") {
assert.IsType(t, &tele.LongPoller{}, middlewarePoller.Poller, "Underlying poller should be LongPoller")
}
}
}
})
// ... rest of the test cases ...
}
| if assert.NotNil(t, defaultBot) { | ||
| actualTeleBot := defaultBot.Bot() | ||
| if assert.NotNil(t, actualTeleBot) && assert.NotNil(t, actualTeleBot.Poller) { | ||
| middlewarePoller, ok := actualTeleBot.Poller.(*tele.MiddlewarePoller) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model confidence: high (70-90%)
Issue priority: could be fixed later 🟢
Repetitive Poller Extraction Logic in Tests
The logic for safely extracting the underlying poller from *tele.MiddlewarePoller (i.e., casting teleBot.Poller to *tele.MiddlewarePoller and then accessing middlewarePoller.Poller) is repeated across multiple subtests (lines 1169-1172, 1193-1196, 1225-1228). While necessary due to the telebot library's design, this repetition makes the tests slightly less readable, increases verbosity, and makes them more cumbersome to update if the wrapping mechanism or type assertions need to change in the future. It also highlights a tight coupling to the internal structure of telebot's poller types.
💡 Suggestion
Extract the common logic for safely retrieving the inner poller from *tele.MiddlewarePoller into a small, reusable helper function. This improves readability, reduces repetition, and centralizes the logic for handling the MiddlewarePoller wrapper. While it doesn't eliminate the underlying coupling to telebot's internal types, it significantly improves the maintainability and clarity of the test suite.
import (
"testing"
"github.com/stretchr/testify/assert"
tele "gopkg.in/telebot.v3"
"path/to/bote" // Adjust path as necessary
)
// getInnerPoller is a helper function to safely extract the underlying poller from MiddlewarePoller
// It asserts the types and returns nil if assertions fail, allowing the test to continue or fail gracefully.
func getInnerPoller(t *testing.T, teleBot *tele.Bot) tele.Poller {
if !assert.NotNil(t, teleBot, "teleBot should not be nil") || !assert.NotNil(t, teleBot.Poller, "teleBot.Poller should not be nil") {
return nil
}
middlewarePoller, ok := teleBot.Poller.(*tele.MiddlewarePoller)
if !assert.True(t, ok, "Poller should be MiddlewarePoller") {
return nil
}
return middlewarePoller.Poller
}
func TestBotInitialization_PollerSelection(t *testing.T) {
t.Run("DefaultLongPoller", func(t *testing.T) {
opts := bote.Options{
Config: bote.Config{TestMode: true},
}
defaultBot, err := bote.NewWithOptions("test_token", opts)
assert.NoError(t, err)
if assert.NotNil(t, defaultBot) {
actualTeleBot := defaultBot.Bot()
innerPoller := getInnerPoller(t, actualTeleBot)
if assert.NotNil(t, innerPoller) {
assert.IsType(t, &tele.LongPoller{}, innerPoller, "Underlying poller should be LongPoller")
}
}
})
t.Run("WebhookPoller_NoTLS", func(t *testing.T) {
opts := bote.Options{
Config: bote.Config{
WebhookURL: "https://test.com/webhook",
ListenAddress: "127.0.0.1:8080",
TestMode: true,
},
}
boteBot, err := bote.NewWithOptions("test_token", opts)
assert.NoError(t, err)
if assert.NotNil(t, boteBot) {
teleBot := boteBot.Bot()
innerPoller := getInnerPoller(t, teleBot)
if assert.NotNil(t, innerPoller) {
webhookPoller, ok := innerPoller.(*tele.Webhook)
if assert.True(t, ok, "Underlying poller should be Webhook") {
assert.Equal(t, "127.0.0.1:8080", webhookPoller.Listen)
if assert.NotNil(t, webhookPoller.Endpoint) {
assert.Equal(t, "https://test.com/webhook", webhookPoller.Endpoint.PublicURL)
assert.Empty(t, webhookPoller.Endpoint.Cert, "Endpoint.Cert should be empty for NoTLS case")
}
assert.Nil(t, webhookPoller.TLS, "TLS config should be nil for NoTLS case")
}
}
}
})
// ... other test cases can also use getInnerPoller ...
}
| // The proxy would handle HTTPS and forward plain HTTP to your bot's ListenAddress. | ||
|
|
||
| // Example 1: Webhook without direct TLS handling by the bot (e.g., behind a reverse proxy) | ||
| // WebhookURL: "https://your.domain.com/webhook-path", // Replace with your actual public URL |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model confidence: high (70-90%)
Issue priority: could be fixed later 🟢
Hardcoded Configuration in Webhook Examples
The added comments provide webhook configuration examples where WebhookURL, ListenAddress, TLSKeyFile, and TLSCertFile are hardcoded. While these lines are currently commented out, directly adopting this pattern for production deployments would lead to significant maintainability challenges, requiring code modifications for different environments (development, staging, production). More critically, hardcoding file paths for TLS keys and certificates introduces a security risk by embedding sensitive infrastructure details directly into the source code, making it less flexible and potentially exposing information if the source code is compromised. This violates the principle of separating configuration from code, which is a cornerstone of robust application design.
💡 Suggestion
Configuration parameters, especially those that are environment-specific or sensitive (like file paths to private keys), should always be externalized. The recommended approach is to use environment variables, a dedicated configuration file (e.g., config.json, config.yaml), or a secrets management system. This promotes the 12-factor app principles, enhances security by separating configuration from code, and improves deployment flexibility. The main function should retrieve these values from environment variables, allowing the application to be deployed without code changes. Additionally, for production-ready applications, panic calls for unrecoverable configuration errors should be replaced with log.Fatal or log.Fatalf to provide more context and ensure a graceful exit.
package main
import (
"log"
"os"
"os/signal"
"github.com/maxbolgarin/bote"
)
func main() {
token := os.Getenv("TELEGRAM_BOT_TOKEN")
if token == "" {
log.Fatal("TELEGRAM_BOT_TOKEN is not set") // Use log.Fatal for unrecoverable config errors
}
cfg := bote.Config{
DefaultLanguageCode: "ru",
NoPreview: true,
}
// Load webhook configuration from environment variables
webhookURL := os.Getenv("TELEGRAM_WEBHOOK_URL")
listenAddress := os.Getenv("TELEGRAM_LISTEN_ADDRESS")
tlsKeyFile := os.Getenv("TELEGRAM_TLS_KEY_FILE")
tlsCertFile := os.Getenv("TELEGRAM_TLS_CERT_FILE")
if webhookURL != "" && listenAddress != "" {
cfg.WebhookURL = webhookURL
cfg.ListenAddress = listenAddress
cfg.TLSKeyFile = tlsKeyFile
cfg.TLSCertFile = tlsCertFile
log.Println("Webhook configuration loaded from environment variables.")
// The bote library should automatically switch to webhook mode if these are set.
// If the bot's Start method behaves differently for webhooks vs. long polling,
// conditional logic for b.Start(nil, nil) vs b.Start(startHandler, nil) might be needed.
// For this example, assuming 'bote.New' and 'b.Start' handle the mode switch based on config.
} else {
log.Println("Webhook environment variables not fully set. Bot will likely use long polling.")
}
b, err := bote.New(token, bote.WithConfig(cfg))
if err != nil {
log.Fatalf("Failed to create bot: %v", err) // Use log.Fatalf for bot initialization errors
}
b.Start(startHandler, nil)
ch := make(chan os.Signal, 1)
signal.Notify(ch, os.Interrupt)
<-ch
b.Stop()
log.Println("Bot stopped.")
}
func startHandler(ctx bote.Context) error {
kb := bote.InlineBuilder(3, bote.OneBytePerRune,
ctx.Btn("1", oneNumberHandler),
ctx.Btn("2", twoNumbersHandler),
ctx.Btn("3", nil),
ctx.Btn("4", nil),
ctx.Btn("some text for a long inline button", nil),
ctx.Btn("use Bote to build bots", nil),
)
return ctx.SendMain(bote.NoChange, "Main message", kb)
}
func oneNumberHandler(ctx bote.Context) error {
return ctx.SendMain(bote.NoChange, "One number", nil)
}
func twoNumbersHandler(ctx bote.Context) error {
return ctx.SendMain(bote.NoChange, "Two numbers", nil)
}
| cfg.UserCacheCapacity = lang.Check(cfg.UserCacheCapacity, 10000) | ||
| cfg.UserCacheTTL = lang.Check(cfg.UserCacheTTL, 24*time.Hour) | ||
|
|
||
| if cfg.WebhookURL != "" { |
There was a problem hiding this comment.
🔒 Security improvement
Model confidence: very high (90-100%)
Issue priority: must be fixed immediately 🔴
Incomplete Webhook TLS Configuration Validation
The current validation logic for webhook configuration checks if TLSKeyFile and TLSCertFile are either both provided or both empty. However, Telegram webhooks strictly require HTTPS. If a WebhookURL is provided, it must be an HTTPS URL, and consequently, valid TLSKeyFile and TLSCertFile must be provided. The current check does not enforce this, potentially leading to a misconfigured bot that attempts to set up an HTTP webhook or fails to start the HTTPS listener, resulting in a non-functional webhook and service outage. This is a critical misconfiguration that will prevent the bot from receiving updates.
💡 Suggestion
Strengthen the webhook configuration validation. If WebhookURL is set, ensure it's an HTTPS URL and that both TLSKeyFile and TLSCertFile are non-empty. This ensures that the bot can correctly establish a secure webhook connection required by Telegram.
if cfg.WebhookURL != "" {
u, err := url.ParseRequestURI(cfg.WebhookURL)
if err != nil {
return errm.Wrap(err, "invalid webhook url")
}
if u.Scheme != "https" {
return errm.New("webhook url must use https scheme")
}
cfg.ListenAddress = lang.Check(cfg.ListenAddress, ":8443")
if cfg.TLSKeyFile == "" || cfg.TLSCertFile == "" {
return errm.New("tls key and cert files must be provided for webhook")
}
// Further check for file existence/readability could be added here if desired
}
| if _, err := url.ParseRequestURI(cfg.WebhookURL); err != nil { | ||
| return errm.Wrap(err, "invalid webhook url") | ||
| } | ||
| cfg.ListenAddress = lang.Check(cfg.ListenAddress, ":8443") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model confidence: high (70-90%)
Issue priority: should be fixed soon 🟡
Missing Validation for Webhook ListenAddress Format
The ListenAddress field, which specifies the address for the webhook listener, is defaulted if empty but not validated for its format. An improperly formatted address (e.g., 'invalid-address' instead of ':8443' or '127.0.0.1:8080') will lead to a runtime error when the underlying net.Listen call is made, causing the bot to fail during startup. This prevents the bot from becoming operational.
💡 Suggestion
Add validation for the ListenAddress format using net.SplitHostPort. This allows for early detection of configuration errors during the prepareAndValidate phase, preventing runtime failures and providing clearer error messages to the user.
if cfg.WebhookURL != "" {
// ... (previous webhook URL and TLS checks)
cfg.ListenAddress = lang.Check(cfg.ListenAddress, ":8443")
// Validate ListenAddress format
if _, _, err := net.SplitHostPort(cfg.ListenAddress); err != nil {
return errm.Wrap(err, "invalid listen address format")
}
// ... (TLS file existence/readability check - see next issue)
}
| return errm.Wrap(err, "invalid webhook url") | ||
| } | ||
| cfg.ListenAddress = lang.Check(cfg.ListenAddress, ":8443") | ||
| if (cfg.TLSKeyFile == "") != (cfg.TLSCertFile == "") { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model confidence: high (70-90%)
Issue priority: should be fixed soon 🟡
Missing Validation for TLS Key/Cert File Existence
While the code correctly checks if TLSKeyFile and TLSCertFile are both provided (or both empty), it does not verify if these paths actually point to existing and readable files on the filesystem. If the specified files do not exist or the application lacks permissions to read them, the bot will encounter a runtime error when attempting to load the TLS certificates, leading to startup failure and service disruption. This is a common operational oversight.
💡 Suggestion
After ensuring both TLS files are provided, add checks using os.Stat to confirm their existence and accessibility. This provides robust configuration validation and prevents runtime crashes due to missing or inaccessible files.
if cfg.WebhookURL != "" {
// ... (previous webhook URL and ListenAddress checks)
if cfg.TLSKeyFile == "" || cfg.TLSCertFile == "" {
return errm.New("tls key and cert files must be provided for webhook")
}
// Check if TLS files exist and are readable
if _, err := os.Stat(cfg.TLSKeyFile); os.IsNotExist(err) {
return errm.Wrapf(err, "tls key file not found: %s", cfg.TLSKeyFile)
} else if err != nil {
return errm.Wrapf(err, "cannot access tls key file: %s", cfg.TLSKeyFile)
}
if _, err := os.Stat(cfg.TLSCertFile); os.IsNotExist(err) {
return errm.Wrapf(err, "tls cert file not found: %s", cfg.TLSCertFile)
} else if err != nil {
return errm.Wrapf(err, "cannot access tls cert file: %s", cfg.TLSCertFile)
}
}1 participant
🤖 Review Summary
This update introduces comprehensive support for Telegram webhooks, enabling bots to receive updates via HTTP callbacks as an alternative to long polling. It includes new configuration options for webhook URL, listen address, and TLS settings, alongside robust validation and updated documentation.
⚡️ New features
bote.Config:WebhookURL,ListenAddress,TLSKeyFile, andTLSCertFile.WebhookURLin the configuration. WhenWebhookURLis provided, atele.Webhookpoller is initialized, with optional TLS configuration for direct HTTPS handling.🧪 Testing
ListenAddress,WebhookURL, and TLS certificates.📚 Documentation
README.mdhas been updated with a dedicated section detailing how to configure webhooks. It explains each new configuration option and provides guidance on when to use direct TLS handling versus deploying behind a reverse proxy.example/main.gofile includes commented-out code snippets demonstrating how to set up webhook configurations for both direct TLS and reverse proxy scenarios.🔄 Other changes
WebhookURLis provided, it is a valid URL.TLSKeyFileandTLSCertFilemust both be provided or both be empty when webhook is enabled, preventing misconfigurations.ListenAddressof:8443is now automatically applied ifWebhookURLis set butListenAddressis not explicitly defined.Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests