Repository navigation
Validate Telegram webhook and command config - #613
rissrice2105-agent wants to merge 1 commit into
Conversation
Greptile SummaryThis PR adds early-exit validation for the Telegram adapter:
Confidence Score: 4/5Safe to merge; the new validation paths are straightforward and well-tested, with no changes to the Telegram API call flow itself. The two observations are both non-blocking: commands going unvalidated during build defers errors to ship rather than preventing them, and the trailing-slash normalization only affects bare-domain webhook URLs which are uncommon in practice. Core ship logic, token handling, and command normalization are all correct. packages/targets/chat-telegram/src/index.ts — specifically the requireWebhookUrl return value and the absence of command validation in build. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([User calls build / ship]) --> B{build or ship?}
B -->|build| C[normalizeUsername]
C --> D[requireWebhookUrl validate HTTPS]
D -->|invalid| E1[throw: webhookUrl must be HTTPS]
D -->|valid| F[return artifact path]
B -->|ship| G[normalizeUsername]
G --> H[requireWebhookUrl validate + normalize]
H -->|invalid| E2[throw: webhookUrl must be HTTPS]
H -->|valid| I[commands.map normalizeCommand]
I -->|invalid command| E3[throw: command format error]
I -->|valid| J{dryRun?}
J -->|yes| K[return dry-run id]
J -->|no| L[lookup bot token from vault]
L -->|missing| E4[throw: token not in vault]
L -->|found| M[callTelegram setWebhook]
M --> N{commands?}
N -->|yes| O[callTelegram setMyCommands]
N -->|no| P[optional description calls]
O --> P
P --> T[return id + url]
Reviews (1): Last reviewed commit: "Validate Telegram webhook and command co..." | Re-trigger Greptile |
| async build(ctx, config) { | ||
| const username = normalizeUsername(config.botUsername); | ||
| requireWebhookUrl(config.webhookUrl); | ||
| ctx.log(`telegram prepare bot manifest for @${username}`); | ||
| return { artifact: join(ctx.outDir, `telegram-${safeFilename(username)}.json`) }; | ||
| }, |
There was a problem hiding this comment.
Command validation skipped in
build
requireWebhookUrl is called in build but config.commands is never validated here. A config with a valid webhookUrl but malformed commands (e.g. a space in the command name or a blank description) will pass build successfully and only fail when ship is called — even in dry-run mode. The failure surfaces later than users would expect, after the manifest artifact path has already been returned.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| function requireWebhookUrl(value: string): string { | ||
| let url: URL; | ||
| try { | ||
| url = new URL(value); | ||
| } catch { | ||
| throw new Error('chat-telegram webhookUrl must be a valid HTTPS URL'); | ||
| } | ||
| if (url.protocol !== 'https:' || !url.hostname) { | ||
| throw new Error('chat-telegram webhookUrl must be a valid HTTPS URL'); | ||
| } | ||
| return url.toString(); | ||
| } |
There was a problem hiding this comment.
url.toString() may silently add a trailing slash
new URL('https://example.com').toString() returns 'https://example.com/' — the WHATWG parser adds a trailing / to the path for bare-domain URLs. The normalized string is then sent verbatim to Telegram's setWebhook. If the webhook server or a monitoring tool compares the registered URL to the configured one, the silently added slash can cause mismatches. URLs with an explicit path (e.g. https://example.com/telegram) are unaffected, but it is worth preserving the original value (after HTTPS-protocol validation) rather than returning the serialized URL object.
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
7 similar comments
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
Fixes #612.
Changes:
Validation: