Dev#6
Open
LearningBotsOfficial wants to merge 74 commits into
Open
Conversation
…, and replace render.py with a FastAPI web service for improved stability.
…delines and security warnings
…e/help plugin modules
…ution to support concurrent task management
…n Dockerfile, and update web service logging settings
…uts, fonts, and alive status in bot.py
…including DM shield, tagall, and others
…d core bot and utility modules
…utdown logic to core system
…d storage plugins
…d profile management plugins
…gin library, and database integration
…bot functionality
…g and greeting support
… Docker, Procfile, and heroku.yml
chore: migrate dependency management from requirements.txt to pyproject.toml and uv.lock
…implified uv workflow
…ering and user management
feat: update Documantation & Update dm shield logic
updated dm shield and protection logic
… warning limits and add logs edit/delete actions in Message Logger. Remove plugin load logs for cleaner output.
fix: migrate to pip from uv. & updated render deployment blueprint.
📝 WalkthroughWalkthroughThis PR refactors Ether Userbot from a simple local session-based userbot to a production-ready cloud-deployed application with containerization, session persistence via MongoDB, owner-restricted access control, comprehensive DM protection with spam detection, and an expanded plugin ecosystem supporting admin operations, logging, user profiling, and utility commands. ChangesProduction Infrastructure & Deployment
Core Lifecycle & Session Architecture
Bot UI, Authentication, and Owner Access Control
DM Protection, Spam Control & Welcome System
Plugin Ecosystem: Core Loader & Feature Plugins
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
plugins/shortcut.py (1)
158-171:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAppend normalized button rows, not Telethon row objects
Line 168 appends
rowinstead ofrow_buttons. That stores rawKeyboardButtonRowobjects and can break shortcut persistence/rendering paths expecting JSON-like button dicts.Proposed fix
if hasattr(msg.reply_markup, 'rows'): for row in msg.reply_markup.rows: row_buttons = [] for btn in row.buttons: if isinstance(btn, KeyboardButtonUrl): row_buttons.append({"text": btn.text, "url": btn.url, "type": "url"}) elif isinstance(btn, KeyboardButtonCallback): row_buttons.append({"text": btn.text, "data": btn.data.decode(), "type": "callback"}) if row_buttons: - button_rows.append(row) + button_rows.append(row_buttons)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/shortcut.py` around lines 158 - 171, The code collects normalized button dicts into row_buttons but mistakenly appends the Telethon row object `row` into `button_rows`; change the append to push the normalized list/dict (`row_buttons`) instead so `button_rows` contains JSON-serializable button dicts (keep the existing checks using `msg.reply_markup`, `row`, `btn`, `KeyboardButtonUrl`, `KeyboardButtonCallback` and the final `if button_rows:` assignment to `buttons`).core/bot.py (1)
998-1050:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBase
/removesuccess on DB deletion too.In the StringSession flow there often won't be a local
.sessionfile, so this path can successfully delete the persisted Mongo session and still reply with “No Session Found”. Track disk and DB removal separately and build the response from both outcomes.Suggested fix
- deleted = False + deleted_disk = False + deleted_db = False ... - deleted = True + deleted_disk = True ... - deleted = True + deleted_disk = True ... - await ether_db.db.sessions.delete_one({"name": Config.SESSION_NAME}) + result = await ether_db.db.sessions.delete_one({"name": Config.SESSION_NAME}) + deleted_db = result.deleted_count > 0 logger.info("Session: DELETED from database.") ... - if deleted: + if deleted_disk or deleted_db: await event.reply( "🗑️ <b>Session Removed</b>\n\n" - "Your session has been deleted from disk and database.\n" + "Your session has been removed.\n" "Use /login to create a new session." )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/bot.py` around lines 998 - 1050, The reply logic currently only considers the disk deletion flag (deleted) so when the session is stored in MongoDB (StringSession) the handler may delete the DB row but still reply "No Session Found"; add a separate boolean (e.g., db_deleted) and set it when the DB delete in the try block for ether_db.db.sessions.delete_one({"name": Config.SESSION_NAME}) succeeds (or when delete_one reports a deleted_count), then build the final event.reply based on both deleted and db_deleted (three states: both removed, only DB removed, neither removed) using the existing event.reply messages adjusted to reflect which storage was cleared; update references to session_file/session_journal, deleted, db_deleted, ether_db.db and Config.SESSION_NAME to find the correct spots.plugins/dm_shield.py (1)
164-175:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAllowed-user count is stale after the allowlist refactor.
Allow/disallow now writes to
DMService, but this screen still countssettings["allowed"], which is no longer maintained. The owner will see the wrong allowed-user total here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/dm_shield.py` around lines 164 - 175, The allowed-user count is read from the stale settings["allowed"] list; instead query the live allowlist maintained by DMService for the owner_id and use its length. After fetching settings = await shield.get(owner_id) keep using settings for flags, but replace len(allowed) with an awaited call to the DMService allowlist API (e.g., await DMService.get_allowed(owner_id) or DMService.allowed_users(owner_id)) and use the returned list's length; ensure you reference the DMService instance used elsewhere in the plugin rather than the old settings["allowed"] variable.
🟠 Major comments (26)
Dockerfile-1-30 (1)
1-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the container as a non-root user.
The image currently runs
main.pyas root, which increases blast radius if the app is compromised. Add a dedicated unprivileged user and switch to it beforeCMD.🔧 Suggested hardening patch
FROM python:3.11-slim @@ WORKDIR /app @@ COPY . . # Create necessary directories -RUN mkdir -p media sessions logs +RUN mkdir -p media sessions logs \ + && addgroup --system appgroup \ + && adduser --system --ingroup appgroup appuser \ + && chown -R appuser:appgroup /app + +USER appuser # Expose port EXPOSE 10000🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 1 - 30, The Dockerfile runs the app as root (CMD ["python", "main.py"]); create a dedicated unprivileged user and switch to it: add steps after creating directories (references: RUN mkdir -p media sessions logs and CMD) to create a user/group (e.g., appuser), chown the app directory and required subdirs (media, sessions, logs) to that user, and set USER appuser before the CMD so the container runs the process unprivileged.docker-compose.yml-18-18 (1)
18-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid bind-mounting the entire repository into the container.
On Line 18,
./:/app/sessions_dataexposes the full host project tree inside the container and broadens the blast radius for accidental writes/secret leakage. You already persist sessions via./sessions:/app/sessions, so this mount should be removed or narrowed to explicit session files only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose.yml` at line 18, Remove the unsafe bind-mount './:/app/sessions_data' from docker-compose.yml (the host root-to-container mount shown as "- ./:/app/sessions_data") and either delete that volume entry or replace it with a narrow, explicit mount such as the existing './sessions:/app/sessions' or a specific file glob (e.g., './sessions/*.session:/app/sessions') so only session data is exposed to the container, not the entire repository.requirements.txt-7-7 (1)
7-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin/upgrade transitive
h11to address GHSA-vqfr-h8mv-ghfj risk fromuvicorn[standard].
requirements.txt:uvicorn[standard]==0.47.0
- GHSA-vqfr-h8mv-ghfj affects
h11versions < 0.16.0; patched versions areh11>=0.16.0—soh11==0.9.0is vulnerable.- No explicit
h11version pin was found inrequirements*.txt/*lock*/pyproject.toml, so transitive resolution may still land on the vulnerable version.Add an explicit constraint (e.g.,
h11>=0.16.0) or update/regenerate your lock to ensure the resolved transitiveh11version is fixed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@requirements.txt` at line 7, The requirements pin for uvicorn[standard]==0.47.0 can pull a vulnerable h11; add an explicit constraint for h11 (e.g., add a line "h11>=0.16.0") to requirements.txt or regenerate/update your lockfile to ensure the transitive dependency resolves to h11>=0.16.0 (alternatively upgrade uvicorn to a version that already depends on h11>=0.16.0); make sure the added constraint references package name "h11" so dependency resolution uses the safe release.plugins/profile.py-50-60 (1)
50-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways clean up downloaded profile photo in a
finallyblock.If upload/edit fails after download, the temp file is left behind.
Proposed fix
- try: - photo = await reply.download_media() + photo = None + try: + photo = await reply.download_media() await ether(functions.photos.UploadProfilePhotoRequest( file=await ether.upload_file(photo) )) await event.edit("<blockquote><b>Profile Picture Updated Successfully!</b></blockquote>") - os.remove(photo) except Exception as e: logger.error(f"PFP update error: {e}") await event.edit(f"<blockquote>Error updating PFP: {str(e)}</blockquote>") + finally: + if photo and os.path.exists(photo): + os.remove(photo)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/profile.py` around lines 50 - 60, The temp photo file downloaded via reply.download_media() is not guaranteed to be removed on errors; ensure cleanup happens in a finally block: keep the download and upload logic (reply.download_media(), ether.upload_file(), functions.photos.UploadProfilePhotoRequest) inside try/except as now, but move the os.remove(photo) into a finally clause that checks photo exists before removing, and preserve the existing logger.error and event.edit calls in the except block so errors are reported while still deleting the temp file.plugins/profile.py-93-95 (1)
93-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle whitespace-only
.setnameinput before indexing.A command like
.setnamemakessplit()return an empty list, causingIndexErrorbefore yourtryblock.Proposed fix
- names = event.pattern_match.group(1).split(maxsplit=1) + raw_name = event.pattern_match.group(1).strip() + if not raw_name: + await event.edit("<blockquote>Please provide at least a first name.</blockquote>") + return + names = raw_name.split(maxsplit=1) first_name = names[0]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/profile.py` around lines 93 - 95, The code uses event.pattern_match.group(1).split(maxsplit=1) and immediately indexes names[0], which throws on whitespace-only `.setname ` input; before splitting, call .strip() on event.pattern_match.group(1) and if the stripped string is empty handle it (e.g., return an error/usage message or set first_name/last_name to empty) so you never index an empty list—update the logic around names/first_name/last_name to use the stripped value and guard when len(names) == 0.plugins/admin.py-124-138 (1)
124-138:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t suppress
.ungbanfailures; report them.Right now failures are silently ignored, so the completion message can be misleading during a security/admin operation.
Proposed fix
success_count = 0 + fail_count = 0 async for dialog in ether.iter_dialogs(): if not (dialog.is_group or dialog.is_channel): continue @@ - except Exception: - pass + except Exception as e: + fail_count += 1 + logger.warning(f"Ungban failed in dialog {dialog.id}: {e!s}") - await event.edit(f"<blockquote><b>Global Ban Lifted</b> from {success_count} groups.</blockquote>") + await event.edit( + "<blockquote>" + f"<b>Global Ban Lifted</b> from {success_count} groups.\n" + f"<b>Failed/No Rights:</b> {fail_count}" + "</blockquote>" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/admin.py` around lines 124 - 138, The try/except around the call to ether(functions.channels.EditBannedRequest(...)) currently swallows all exceptions; change it to catch Exception as e, record failures (e.g., increment a failure_count and append failed dialog identifiers or error messages to a list), log the exception (including dialog.id and user_id) and continue, then update the final event.edit message to report both success_count and failures (or list of failed groups and their errors). Keep the flood sleep and don't re-raise; reference EditBannedRequest, ChatBannedRights, success_count and event.edit when making the changes.plugins/tagall.py-63-65 (1)
63-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard DB-dependent code when
dbis unavailable.Line 63 initializes
tag_colonly whendbis notNone, but Lines 85/104+ use it unconditionally. If setup is called without DB, this handler will crash on first command.Suggested fix
def setup(ether, db, owner_id): @@ - # MongoDB collection for process tracking - if db is not None: - tag_col = db["tag_tasks"] + # MongoDB collection for process tracking + if db is None: + logger.warning("TagAll plugin disabled: database handle is missing") + return + tag_col = db["tag_tasks"]Also applies to: 85-87, 104-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/tagall.py` around lines 63 - 65, The handler assumes tag_col exists when db may be None; initialize tag_col to None in the setup scope when db is not provided and guard every usage (places referencing tag_col, e.g., in the setup function and the command handlers that read/write tag_col) with a check like "if tag_col is None" to return/skip or send a graceful error message instead of accessing it; update all references (the earlier initialization block that currently does "if db is not None: tag_col = db['tag_tasks']" and uses around the command handlers) so no unconditional accesses occur when db is unavailable.plugins/tagall.py-103-117 (1)
103-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake tagging run acquisition atomic to prevent parallel runs per chat.
plugins/tagall.pylines ~104-116 do a non-atomic check-then-set (find_oneforactivefollowed byupdate_one(..., {$set: {active: True}}, upsert=True)), so two near-simultaneous commands can both pass the check and start concurrently. Acquire the lock with a single atomic operation (e.g.,find_one_and_updatefiltered to “inactive/missing” so only one caller wins).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/tagall.py` around lines 103 - 117, Replace the non-atomic check-then-set (tag_col.find_one + tag_col.update_one) with a single atomic acquisition using tag_col.find_one_and_update: call find_one_and_update with a filter matching {"chat_id": chat_id, "$or":[{"active": False}, {"active": {"$exists": False}}]} (or equivalent) and an update of {"$set": {"active": True}}, using upsert=True so a missing doc is created; if the call returns None (no match) treat it as “already running” and send the Process Error via event.edit then return, otherwise proceed to delete the invoking event and send status_msg/ether.send_message as before. Ensure you reference tag_col, find_one_and_update, chat_id, active, event, and ether.send_message in the change.plugins/stickers.py-85-98 (1)
85-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways clean temporary files and avoid editing after command deletion.
In both handlers, temp files are only removed on the happy path, and
event.delete()happens before cleanup in the sametry. A later error can leave files behind and create noisy failure handling.💡 Suggested fix pattern
- try: - photo = await reply.download_media() + photo = None + try: + photo = await reply.download_media() await ether.send_file( event.chat_id, photo, force_document=False, reply_to=event.reply_to_msg_id ) await event.delete() - os.remove(photo) except Exception as e: logger.error(f"Sticker convert error: {e}") - await event.edit(f"<blockquote>Conversion failed: {str(e)}</blockquote>") + await event.edit(f"<blockquote>Conversion failed: {e!s}</blockquote>") + finally: + if isinstance(photo, str) and os.path.exists(photo): + os.remove(photo)Also applies to: 119-131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/stickers.py` around lines 85 - 98, Temporary files and message cleanup are only done on the success path and event.delete() is called before cleanup; change the handlers that call reply.download_media(), ether.send_file(...), event.delete(), event.edit(...) and os.remove(photo) to use a try/except/finally: download and send in try, on exception only edit the event if it still exists (avoid editing after deletion), and always remove the temp file in finally (check photo is set and file exists before os.remove). Apply the same pattern to the other handler mentioned (lines 119-131) so temp files are always cleaned up and you never call event.edit after event.delete().plugins/utils.py-78-85 (1)
78-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEncode the translation query parameters and add aiohttp timeout/status handling (plugins/utils.py lines 80-85)
- Line 80 interpolates raw
textinto the query string (q={text}), which can break requests whentextcontains&,?,#, etc.- Lines 82-85 have no explicit
aiohttptimeout and noresponse.raise_for_status()/ status guard before parsing JSON.💡 Suggested fix
- url = f"https://api.mymemory.translated.net/get?q={text}&langpair=auto|{lang}" - - async with aiohttp.ClientSession() as session: - async with session.get(url) as response: - data = await response.json() - translated_text = data['responseData']['translatedText'] + url = "https://api.mymemory.translated.net/get" + params = {"q": text, "langpair": f"auto|{lang}"} + timeout = aiohttp.ClientTimeout(total=15) + + async with aiohttp.ClientSession(timeout=timeout) as session: + async with session.get(url, params=params) as response: + response.raise_for_status() + data = await response.json() + translated_text = data["responseData"]["translatedText"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/utils.py` around lines 78 - 85, The code interpolates raw text into the query string and lacks request timeout and status checks; change the call to use aiohttp's params (e.g., params = {"q": text, "langpair": f"auto|{lang}"} and pass it to session.get) or build a yarl.URL with encoded query params instead of f-string interpolation, create an aiohttp.ClientTimeout (e.g., aiohttp.ClientTimeout(total=10)) and pass it to ClientSession or to session.get, and call response.raise_for_status() (or check response.status) before awaiting response.json() so you only parse valid responses; update the usage around aiohttp.ClientSession, session.get, response.raise_for_status, and response.json accordingly.plugins/stickers.py-87-92 (1)
87-92:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix
.stickerto actually upload a Telegram sticker + harden temp-file cleanup
- The
.stickerhandler just downloads the replied media and sends it back viasend_file(..., force_document=False)without converting to sticker format (e.g., sticker.webp) or using a sticker-specific upload, so it won’t produce a real Telegram sticker.- Temp-file cleanup looks success-path only (no
os.remove(...)on exceptions) and errors can run afterevent.delete(), risking leaks and additional failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/stickers.py` around lines 87 - 92, The .sticker handler currently re-sends the downloaded file via ether.send_file(photo, force_document=False) which doesn't create a Telegram sticker and also only removes the temp file on the success path; change the handler to convert the downloaded media (variable photo) to a proper WebP sticker file (e.g., using an image conversion path or ffmpeg/Pillow producing a .webp with appropriate dimensions/alpha), then upload that .webp as a sticker by calling ether.send_file with the sticker file and force_document=True (and any needed mimetype or filename to ensure Telegram treats it as a sticker), and wrap download/convert/send in try/finally so the temp file (photo and converted file) is always removed even on exceptions and ensure event.delete() is called only after cleanup or inside the same try/finally so failures don't leak temp files.main.py-31-58 (1)
31-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the single-instance lock atomic.
Separating “check” and “create” lets two concurrent startups both pass
check_instance()and both writeether.lock, which defeats the whole single-instance guarantee. Use an atomic create (O_CREAT|O_EXCL/flock) or a lockfile library so acquisition is one step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.py` around lines 31 - 58, The current two-step check_instance() + acquire_lock() is racy; make lock acquisition atomic by combining them into a single acquire_lock() that uses an atomic OS operation (e.g., os.open with flags O_CREAT|O_EXCL to create LOCK_FILE and write os.getpid(), or obtain an exclusive fcntl.flock on the file) and treat FileExistsError / lock contention as "already running" (read existing PID for messaging if desired); on failure log and exit as check_instance() currently does, and keep release_lock() to remove the file or unlock the descriptor.main.py-78-87 (1)
78-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFall back cleanly when the stored session is bad.
This bootstrap path runs before the retry loop, and
init_client(stored_session)is unguarded. If the DB contains a corrupted or stale session payload,run_userbot()dies immediately and the bot UI never gets a usable userbot client to recover with/login. Catch init failures here, clear the bad record, and continue with a blank session instead.Suggested fix
if db_connected: stored_session = await ether_db.get_session(Config.SESSION_NAME) if stored_session: logger.info("Session: FOUND in database — resuming stored session.") - client_wrapper.init_client(stored_session) + try: + client_wrapper.init_client(stored_session) + except Exception as e: + logger.warning(f"Session: INVALID STORED SESSION ({e}); falling back to blank login.") + if ether_db.sessions is not None: + await ether_db.sessions.delete_one({"name": Config.SESSION_NAME}) + client_wrapper.init_client() else: logger.info("Session: NOT found in database — starting blank session.") client_wrapper.init_client()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.py` around lines 78 - 87, Wrap the unguarded client_wrapper.init_client(stored_session) call in a try/except so a raised exception from a corrupted/stale stored_session does not crash boot; on exception log the failure via logger.error including the exception, delete the bad record using the DB API (e.g. call ether_db.delete_session(Config.SESSION_NAME) or the equivalent remove/clear method on ether_db), and then call client_wrapper.init_client() to continue with a blank session. Ensure the same guarded pattern applies when db_connected is False path if you later pass any stored data.storage/mongo.py-140-146 (1)
140-146:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEncrypt the Telegram/Telethon session string before persisting it to MongoDB.
storage/mongo.py’ssave_session()stores the raw authenticated session in plaintext (sessions.session), andget_session()loads it back for client bootstrap; there’s no encryption/key management around this value. Anyone with MongoDB read access (or backups) can restore the session and hijack the Telegram account. Encrypt with an app-managed key (preferably via KMS/secret manager) or move the session to a dedicated secret store.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@storage/mongo.py` around lines 140 - 146, The save_session function currently writes plaintext session strings to MongoDB; change save_session to encrypt the session value using an application-managed key (e.g., via your KMS/secret manager client) before calling db.sessions.update_one (refer to save_session and db.sessions.update_one in this diff), and update the corresponding get_session path to decrypt the stored value back into the original session string before returning/bootstrapping the client; ensure key retrieval is done securely (secret manager/KMS) and handle encryption/decryption errors and key rotation edge cases (fallback/fail-fast) when you implement the change.plugins/logger.py-109-116 (1)
109-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape logged message text before wrapping it in HTML.
old_text,new_text, and cached message bodies come from other users. A message containing<or&can break the log send or render misleading markup in the owner's log chat.Suggested fix
+import html ... log_msg = ( "<blockquote>" "<b>Message Edited</b>\n" f"<b>User:</b> <code>{event.sender_id}</code>\n\n" - f"<b>Before:</b>\n<code>{old_text}</code>\n\n" - f"<b>After:</b>\n<code>{new_text}</code>" + f"<b>Before:</b>\n<code>{html.escape(old_text)}</code>\n\n" + f"<b>After:</b>\n<code>{html.escape(new_text)}</code>" "</blockquote>" ) ... log_msg = ( "<blockquote>" "<b>Message Deleted</b>\n" f"<b>User:</b> <code>{cached['sender_id']}</code>\n\n" - f"<b>Original:</b>\n<code>{cached.get('text', '')}</code>" + f"<b>Original:</b>\n<code>{html.escape(cached.get('text', ''))}</code>" "</blockquote>" )Also applies to: 151-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/logger.py` around lines 109 - 116, The log assembly is inserting raw user-controlled strings (old_text, new_text and cached message bodies) into HTML, which can break rendering or introduce markup; update the code that builds log_msg (and the similar block around lines 151-156) to HTML-escape those values before interpolation (e.g., use html.escape or an equivalent sanitizer) so that old_text, new_text and any cached message body variables are escaped prior to being wrapped in the "<blockquote>" HTML; ensure you apply the same escaping wherever those cached message bodies are logged.plugins/logger.py-197-203 (1)
197-203:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis pattern only matches
.logonand.logoff.If the intended command is
.log on/.log off, the current regex never fires because it does not allow whitespace between.logand the mode.Suggested fix
- `@ether.on`(events.NewMessage(pattern=r"^\.log(on|off)$", outgoing=True)) + `@ether.on`(events.NewMessage(pattern=r"^\.log\s+(on|off)$", outgoing=True))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/logger.py` around lines 197 - 203, The message handler decorator's regex only matches ".logon" and ".logoff" so the toggle_logger handler never fires for ".log on" / ".log off"; update the events.NewMessage pattern on the `@ether.on`(...) decorator that wraps the toggle_logger function to accept whitespace between ".log" and the mode (e.g., use a pattern that allows one or more spaces before "(on|off)" or makes the space optional if you want to support both forms) so the regex matches ".log on" and ".log off" (reference: the toggle_logger function and its events.NewMessage pattern).plugins/logger.py-165-191 (1)
165-191:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor the
enabledflag before persisting private messages.
.log offcurrently disables notifications, butcache_messages()still stores every incoming private message for 48 hours. For a privacy-sensitive feature, that is a surprising mismatch between "off" and the actual behavior.Suggested fix
`@ether.on`(events.NewMessage(incoming=True)) async def cache_messages(event): if event.sender_id == owner_id: return + + cfg = await get_config() + if not cfg.get("enabled"): + return sender = await event.get_sender()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/logger.py` around lines 165 - 191, The cache_messages handler currently persists incoming private messages unconditionally; update cache_messages to honor the existing enabled flag (e.g., a module/global variable named enabled) by returning early when enabled is False before performing any DB insert; keep the existing early returns for owner_id, non-private chats and bots, but add a check like "if not enabled: return" (or equivalent) near the top of cache_messages so messages are not stored when logging is turned off.plugins/info.py-142-150 (1)
142-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape entity fields before inserting them into HTML.
title,name,bio, anddescriptionare user-controlled. If any of them contains<,>, or&, the formatted response can fail to send or render spoofed markup instead of the actual value.Suggested fix
+import html ... info_text = "<blockquote>" info_text += f"<b>{details.get('Type', 'Entity')} Info</b>\n\n" for k, v in details.items(): if k not in ["Type", "Bio", "Description"]: - info_text += f"<b>{k}:</b> <code>{v}</code>\n" + info_text += f"<b>{k}:</b> <code>{html.escape(str(v))}</code>\n" # Add multiline content at end desc_label = "Bio" if "Bio" in details else "Description" - info_text += f"\n<b>{desc_label}:</b>\n<i>{details.get(desc_label)}</i>" + info_text += f"\n<b>{desc_label}:</b>\n<i>{html.escape(str(details.get(desc_label, '')))}</i>" info_text += "</blockquote>"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/info.py` around lines 142 - 150, The HTML being built into the info_text string uses user-controlled values from the details dict (keys like "Type", "Bio", "Description", and other fields) without escaping, which can break rendering or allow spoofed markup; fix by escaping all inserted values before concatenation (e.g., use a HTML-escaping util such as html.escape on details.get(...) and on each v in the loop) so that values used in the loop and the final desc_label insertion are safely encoded; update the code paths that reference info_text, details.items(), details.get(desc_label) to pass values through the escape function before formatting.plugins/info.py-115-123 (1)
115-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
full.full_chatin the channel branch.
GetFullChannelRequestdoes not populatefull_user, so this path will raise for channels/megagroups and.infowill fall into the generic error handler instead of showing details.Suggested fix
elif isinstance(entity, types.Channel): full = await ether(functions.channels.GetFullChannelRequest(channel=entity)) + full_chat = full.full_chat details = { "Type": "Channel" if entity.broadcast else "Megagroup", "Title": entity.title, "ID": entity.id, "Username": f"@{entity.username}" if entity.username else "None", - "Members": full.full_user.participants_count or "N/A", + "Members": full_chat.participants_count or "N/A", "DC ID": entity.photo.dc_id if entity.photo else "N/A", - "Description": full.full_user.about or "No Description", + "Description": full_chat.about or "No Description", "Restricted": "Yes" if entity.restricted else "No" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/info.py` around lines 115 - 123, The details dict is accessing full.full_user after calling functions.channels.GetFullChannelRequest (stored in variable full), but GetFullChannelRequest populates full.full_chat for channels/megagroups; update the code that builds details (in plugins/info.py where full = await ether(functions.channels.GetFullChannelRequest(channel=entity))) to use full.full_chat (e.g., full.full_chat.participants_count and full.full_chat.about) instead of full.full_user when entity is a channel/megagroup so the channel branch does not raise.plugins/logger.py-120-129 (1)
120-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
sender_idandexpire_atwhen the edit-cache misses.
plugins/logger.py’sedit_snifferusesupdate_one(..., upsert=True)and, on a cache miss, only$setstextandedit_count. That inserts documents withoutsender_idandexpire_at.delete_snifferlater readscached['sender_id'](no fallback), which will raiseKeyError, and the TTL index onexpire_atwon’t expire these documents.Suggested fix
await msg_col.update_one( {"msg_id": event.id, "chat_id": event.chat_id}, { "$set": { "text": event.text, "edit_count": cached.get("edit_count", 0) + 1 if cached else 1 - } + }, + "$setOnInsert": { + "sender_id": event.sender_id, + "date": datetime.utcnow(), + "expire_at": datetime.utcnow() + timedelta(hours=48), + }, }, upsert=True )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/logger.py` around lines 120 - 129, The edit_sniffer upsert currently only $set"s text and edit_count, so on cache miss documents are created without sender_id and expire_at causing delete_sniffer to KeyError and TTL failure; update the upsert to preserve or populate those fields by using $setOnInsert for sender_id and expire_at (or read them from cached when present) and ensure edit_count increments correctly (use cached.get("edit_count", 0) + 1 when cached exists else 1). Target the msg_col.update_one call inside edit_sniffer and ensure delete_sniffer still reads cached['sender_id'] safely after this change.plugins/logger.py-39-53 (1)
39-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix Mongo config toggling to avoid updating immutable
_id
find_one()returns documents with_id, and the.loghandler passes that raw dict intoset_config(), which does{"$set": data}. Once the config doc exists, Mongo rejects updates that attempt to modify_id.Suggested fix
async def get_config(): cfg = await config_col.find_one({"owner_id": owner_id}) if not cfg: return { "enabled": True, "log_chat_id": None } - return cfg + return { + "enabled": cfg.get("enabled", True), + "log_chat_id": cfg.get("log_chat_id") + } async def set_config(data: dict): + data = { + "enabled": data.get("enabled", True), + "log_chat_id": data.get("log_chat_id") + } await config_col.update_one( {"owner_id": owner_id}, {"$set": data}, upsert=True )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/logger.py` around lines 39 - 53, The update fails because get_config returns a document including the immutable _id and set_config blindly does {"$set": data}; modify the code so set_config (or the path that forwards the dict) strips/out-excludes the _id before calling config_col.update_one, e.g., remove the "_id" key from the incoming data dict or use a projection in get_config to exclude _id, then call config_col.update_one({"owner_id": owner_id}, {"$set": cleaned_data}, upsert=True); reference functions/vars: get_config, set_config, config_col, owner_id.plugins/dm.py-1161-1177 (1)
1161-1177:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRoute spam blocks through the temporary-ban API.
This still calls
block_user(), which storesunblock_at=None.plugins/privacy.pyonly auto-unblocks rows whoseunblock_athas expired, so the new temporary-ban flow never activates from the spam path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/dm.py` around lines 1161 - 1177, The spam path currently calls dm_service.block_user(...) (and sends ether(BlockRequest(user_id))) which creates a permanent block (unblock_at=None); change it to call the temporary-ban API instead—use the service method that registers a timed ban (e.g., dm_service.temp_ban_user or equivalent) and set unblock_at to the configured temporary ban expiry (based on max_warns/spam policy), and update the outbound event to send the matching temporary-ban request object instead of BlockRequest if required so plugins/privacy.py will auto-unblock when the timer expires; keep the event.respond and sleep logic unchanged.plugins/dm.py-928-955 (1)
928-955:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis flags every follow-up DM as spam.
There is no timing/content check before
warnsis incremented, so even normal second/third messages walk toward a block. On top of that,warnsandmessage_countare derived from a staleusersnapshot and written back with$set, so two quick messages can overwrite each other. Uselast_message_time/last_message_textto decide whether this is actually spam, and increment counters atomically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/dm.py` around lines 928 - 955, The current handle_spam_detection increments warns blindly and writes back from a stale user snapshot; change it to first decide spam by comparing the incoming message timestamp (event.date / event.ts) and content to user.get("last_message_time") and user.get("last_message_text") (e.g., time delta and similarity/identical text) and only treat as spam when thresholds are exceeded, then perform an atomic update via dm_service.update_user using atomic operators (e.g., $inc for warns/message_count and $set for last_message_time/last_message_text) or a conditional findOneAndUpdate that matches the previous last_message_time to avoid lost updates from concurrent messages; update the logic in handle_spam_detection so warns/message_count are not derived from the stale user object but modified atomically in the DB and last_message_time/text are updated on every message (or only when not considered spam) as appropriate.services/dm_service.py-305-312 (1)
305-312:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't delete the entire owner config document here.
dm_configalso storesmax_warns, so.clearwelcomecurrently wipes the persisted warn-limit along with the welcome data. Only unset the welcome fields.Suggested fix
- await self.config.delete_one({ - "owner_id": owner_id - }) + await self.config.update_one( + { + "owner_id": owner_id + }, + { + "$unset": { + "text": "", + "image": "", + "buttons": "", + "media_type": "" + } + } + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/dm_service.py` around lines 305 - 312, The delete_welcome method currently removes the entire owner document (using config.delete_one) which also deletes unrelated fields like max_warns; change it to only unset the welcome-related fields by using config.update_one for the owner_id and an $unset payload for the welcome fields (e.g., keys such as welcome_channel_id, welcome_message, welcome_enabled or whatever welcome_* names are used in the document) so the rest of the owner's config (like max_warns) is preserved.services/dm_service.py-48-65 (1)
48-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake user creation atomic.
With the unique
user_idindex added in the Mongo layer, thisget_user()+insert_one()sequence can race on concurrent first-contact messages and raiseDuplicateKeyError. Use a single upsert with$setOnInsertinstead.Suggested fix
- existing = await self.get_user(user_id) - - if existing: - return - - from config.config import Config - - await self.users.insert_one({ - "user_id": user_id, - "allowed": False, - "warns": 0, - "max_warns": Config.DM_MAX_WARNS, - "blocked": False, - "message_count": 0, - "last_message_time": 0, - "last_message_text": "", - "unblock_at": None - }) + from config.config import Config + + await self.users.update_one( + { + "user_id": user_id + }, + { + "$setOnInsert": { + "user_id": user_id, + "allowed": False, + "warns": 0, + "max_warns": Config.DM_MAX_WARNS, + "blocked": False, + "message_count": 0, + "last_message_time": 0, + "last_message_text": "", + "unblock_at": None + } + }, + upsert=True + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/dm_service.py` around lines 48 - 65, Replace the separate get_user() check and users.insert_one() with a single atomic upsert: call users.update_one (or find_one_and_update) filtered by {"user_id": user_id} with upsert=True and an update that uses "$setOnInsert" to provide the initial document fields (allowed, warns, max_warns using Config.DM_MAX_WARNS, blocked, message_count, last_message_time, last_message_text, unblock_at). This avoids DuplicateKeyError on concurrent first-contact messages while keeping the same initial values; you can omit the prior get_user() call and use the upsert result to determine whether the document was newly inserted if needed.plugins/dm.py-989-999 (1)
989-999:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist the warn-limit change.
This only updates process memory. After a restart,
get_max_warns()falls back to the old stored value becauseset_global_max_warns()is never called.Suggested fix
Config.DM_MAX_WARNS = limit global max_warns max_warns = limit + + await dm_service.set_global_max_warns( + owner_id, + limit + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/dm.py` around lines 989 - 999, The change updates Config.DM_MAX_WARNS and the module-level global max_warns but never persists the new limit; call the persistence function set_global_max_warns(limit) after updating Config.DM_MAX_WARNS and max_warns so get_max_warns() returns the new value after restarts—modify the handler that currently sets Config.DM_MAX_WARNS and global max_warns (the code around Config.DM_MAX_WARNS, max_warns assignment and event.edit) to invoke set_global_max_warns(limit) and handle/report any error from that call.
🧹 Nitpick comments (5)
plugins/afk.py (3)
67-71: ⚡ Quick winExtract duplicated duration calculation logic.
The duration formatting logic (
uptime → h/m/s → formatted string) is duplicated three times (lines 67-71, 95-101, 123-127). Consider extracting it into a helper function to follow the DRY principle and improve maintainability.♻️ Proposed refactor
Add this helper function after the AFK_STATE definition:
def format_duration(start_time): """Format duration from start_time to now as 'Xh Ym Zs'.""" uptime = int(time.time() - start_time) h = uptime // 3600 m = (uptime % 3600) // 60 s = uptime % 60 return f"{h}h {m}m {s}s" if h > 0 else (f"{m}m {s}s" if m > 0 else f"{s}s")Then replace all three occurrences:
- uptime = int(time.time() - AFK_STATE["time"]) - h = uptime // 3600 - m = (uptime % 3600) // 60 - s = uptime % 60 - duration = f"{h}h {m}m {s}s" if h > 0 else (f"{m}m {s}s" if m > 0 else f"{s}s") + duration = format_duration(AFK_STATE["time"])Also applies to: 95-101, 123-127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/afk.py` around lines 67 - 71, The AWK duration-formatting logic is duplicated (used around the AFK_STATE uptime calculations); extract it into a single helper (e.g., format_duration(start_time)) that takes AFK_STATE["time"] (or any start_time), computes uptime -> h/m/s and returns the formatted string, then replace the three inline blocks (the uptime/h/m/s/duration assignments) with calls to format_duration(AFK_STATE["time"]). Ensure the helper name (format_duration) is defined near the AFK_STATE declaration and used in all places where the original duration formatting code appears.
95-101: 💤 Low valueUnused duration calculation.
The AFK duration is calculated but never used, logged, or displayed. If this computation was intended for logging or notifications, please complete the implementation; otherwise, consider removing it.
♻️ Proposed fix to remove unused code
AFK_STATE["is_afk"] = False - uptime = int(time.time() - AFK_STATE["time"]) - - # Convert seconds to readable format for the log - h = uptime // 3600 - m = (uptime % 3600) // 60 - s = uptime % 60 - duration = f"{h}h {m}m {s}s" if h > 0 else (f"{m}m {s}s" if m > 0 else f"{s}s") -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/afk.py` around lines 95 - 101, The computed AFK duration (uptime/duration) is never used; either remove the unused calculation or include it in the AFK-clear flow. Fix by reusing the computed duration when clearing AFK (read AFK_STATE["time"], compute uptime and duration as shown) and append it to the notify/log call that announces the user is back (or to the function that handles AFK removal), referencing AFK_STATE, uptime and duration; alternatively, delete the uptime/duration lines if no display/log is needed.
23-23: 💤 Low valueRemove unused import.
The
asynciomodule is imported but never used in this file.♻️ Proposed fix
from telethon import events -import asyncio import time🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/afk.py` at line 23, Remove the unused top-level import of the asyncio module in afk.py: delete the "import asyncio" line (the unused symbol is asyncio) so the file doesn't contain unused imports and re-run the linter/formatter to ensure imports are ordered correctly.plugins/tagall.py (1)
173-176: ⚡ Quick winAvoid bare
exceptwith silentpassduring cleanup.If status-message deletion fails, the exception is fully swallowed, which makes production debugging harder.
Suggested fix
finally: await tag_col.update_one({"chat_id": chat_id}, {"$set": {"active": False}}) try: await status_msg.delete() - except: - pass + except Exception as e: + logger.debug(f"Failed to delete status message: {e}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/tagall.py` around lines 173 - 176, The cleanup block currently swallows all exceptions with a bare except when calling status_msg.delete(); replace it with a typed exception handler (e.g., except Exception as e) and log the error instead of passing silently — for example, call logger.warning/exception or logging.getLogger(__name__).warning with a message like "failed to delete status_msg" and include the exception details; keep the delete in a try/except to avoid crashing but do not suppress the exception information for debugging.utils/task_helper.py (1)
35-44: ⚡ Quick winLog the task’s traceback, not the callback’s.
exc_info=Truehere captures the callback context, not the original task failure, so crashes inUserbotCore/BotUIlose their useful stack trace. Pass the task exception tuple explicitly instead.Suggested fix
try: # Check if task raised an exception exception = t.exception() if exception: task_name = name or t.get_name() - logger.error(f"Task '{task_name}' failed with error: {exception}", exc_info=True) + logger.error( + f"Task '{task_name}' failed", + exc_info=(type(exception), exception, exception.__traceback__), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/task_helper.py` around lines 35 - 44, The current task callback logs with exc_info=True which records the callback stack instead of the original task traceback; when a task (t) has an exception, capture its exception tuple and pass it to logger.error as exc_info=(type(exception), exception, exception.__traceback__) so the original traceback from the failed task (not the callback) is logged; update the block that checks exception = t.exception() (and uses task_name/name and logger.error) to pass the explicit exc_info tuple and keep the existing asyncio.CancelledError handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd0d1a03-2814-4e33-a3d2-0999a7deeb2f
📒 Files selected for processing (42)
.dockerignore.gitignore.python-version.slugignoreCONTRIBUTING.mdDockerfileProcfileREADME.mdapp.jsonauth/__init__.pyauth/login.pyauth/session.pyconfig/config.pycore/bot.pycore/loader.pycore/user_client.pydocker-compose.ymlheroku.ymlmain.pyplugins/admin.pyplugins/afk.pyplugins/alive.pyplugins/dm.pyplugins/dm_shield.pyplugins/fonts.pyplugins/help.pyplugins/info.pyplugins/logger.pyplugins/ping.pyplugins/privacy.pyplugins/profile.pyplugins/shortcut.pyplugins/stickers.pyplugins/tagall.pyplugins/utils.pyrender.pyrender.yamlrequirements.txtservices/dm_service.pystorage/mongo.pyutils/task_helper.pyweb_service.py
💤 Files with no reviewable changes (5)
- auth/session.py
- render.py
- auth/init.py
- auth/login.py
- core/loader.py
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
2 participants
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 by CodeRabbit
Release Notes
New Features
Deployment
Documentation