fix: security vulnerabilities and formatting bug - #33
Merged
Merged
Conversation
Security fixes: - fix(security): URL whitelist bypass via userinfo injection (CRITICAL) Use parsed.hostname instead of parsed.netloc; reject URLs with userinfo - fix(security): path traversal bypass in Telegram URL whitelist Normalize paths with posixpath.normpath before segment extraction - fix(security): message edit bypass in all moderation handlers (HIGH) anti_spam, bio_bait, duplicate_spam, guest_bot now check edited_message - fix(security): UTF-16 offset mismatch in URL entity extraction Use message.parse_entities() instead of manual string slicing - fix(security): non-atomic increment_message_count race condition Use atomic SQL UPDATE for message_count like increment_new_user_violation - fix(security): WAL synchronous pragma only applied to one connection Use SQLAlchemy connect event listener for all pooled connections - fix(security): admin command error replies exploitable for group spam Check admin status before chat type to silently ignore non-admins - fix(bug): scheduler infinite retry loop for departed users Handle ChatMemberStatus.LEFT same as BANNED Bug fix: - fix(formatting): names with square brackets break Markdown mentions Escape [ and ] in user_full_name before mention_markdown (e.g. Romado [1376015]) Also fix _format_person in trust handler for consistency
Fixes verified defects found reviewing the branch's own changes.
Security:
- is_url_whitelisted: the posixpath.normpath traversal fix closed one
direction (segment demoted) but opened the opposite (a non-whitelisted
segment erased by ".." promotes a whitelisted one, e.g.
t.me/scam_group/../pythonid). Now rejects any "." or ".." path segment
outright instead of normalizing.
- get_user_mention_by_id / trust._format_person: the bracket-escaping fix
was inverted. escape_markdown(version=1) already escapes "[" (not "]");
the added .replace("[", ...) re-escaped it, producing "\\[" — Telegram
renders that as a literal backslash plus an unescaped bracket, the
exact failure the fix claims to prevent. Only "]" needed escaping.
- require_admin_dm_target: reordering the private-chat check below the
admin check silenced the audit log for non-admin attempts in groups.
Restored the log before the early return.
Correctness (edited_message support added dead-message counting bugs):
- duplicate_spam / new_user_spam / guest_bot: editing an already-seen
message re-delivers it as update.edited_message. All three counted it
again, so a typo fix could trip the duplicate-message threshold, and a
probation or guest-bot violation could be counted twice from one
message. All three now ignore edits of messages already handled.
- scheduler: ChatMemberStatus.LEFT was folded into the same branch as
BANNED, deleting warning history for users who merely left. BANNED is
terminal; LEFT is reversible, so deleting let a user reset their
auto-restriction clock by leaving and rejoining before the next sweep.
LEFT now skips restriction without deleting the warning.
Tests (several could not fail on the bug they claimed to cover):
- test_properties.py / test_telegram_utils.py: the escaping regression
tests recomputed the buggy expression or used substring assertions
that pass on both correct and double-escaped output. Both now assert
the exact expected string.
- test_whitelist.py: extract_urls/has_non_whitelisted_link tests stubbed
parse_entities/parse_caption_entities directly, so they never exercised
real UTF-16 offset slicing or the [MessageEntity.URL] filter. Rebuilt
against real telegram.Message objects; added the reverse-direction
traversal case.
- Several "update without message" guard tests set only update.message
= None, leaving update.edited_message an auto-truthy MagicMock, so the
fallback picked that mock and the guard was never exercised. Added
edited_message = None throughout, plus one new edited_message
regression test per counting handler.
Also moves the WAL-mode log line after the first real connection instead
of logging success unconditionally at construction, and hoists a
function-local sqlalchemy import to module scope.
1098 tests pass, ruff and mypy clean. Every new/changed test was
mutation-checked against the pre-fix code or a reverted guard.
…d_skips_warning An earlier edit displaced this assertion into the new edited_message regression test, leaving the already-restricted test's own subject assertion (must not re-restrict) unverified. Restored; mutation-checked by removing the source's early-return guard, which now fails the test as expected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes 8 security vulnerabilities (1 critical, 3 high, 4 medium) and 1 formatting bug found during a comprehensive security review.
Security Fixes
🔴 Critical
is_url_whitelistedusedparsed.netlocinstead ofparsed.hostname, allowing attackers to craft URLs likehttps://docs.python.org:x@evil.com/phishthat pass the whitelist but redirect to malicious sites🟠 High
update.message, ignoringupdate.edited_message. Attackers could post benign messages and edit in spamparse_entities()increment_message_countrace condition — Replaced Python-side read-modify-write with atomic SQLUPDATE ... SET x = x + 1🟡 Medium
t.me/pythonid/../../scampassed the whitelist checksynchronous=NORMALis connection-scoped; replaced one-off connection with SQLAlchemyconnectevent listener/checketc. in groups triggered visible error replies; now silently ignoredLEFTstatus caused perpetual restrict failures every 5 minutesBug Fix
Romado [1376015]produced broken[Romado [1376015]](tg://user?id=...)links. Now escapes[and]inget_user_mention_by_idand_format_personTest Changes
extract_urls/has_non_whitelisted_linktests to mockparse_entities()instead of raw offset slicing