Limpieza a fondo: DMs, SSRF en !play, errores de arranque de yt-dlp y tipado - #43
Merged
Merged
Conversation
- Refuse !play links whose host resolves to a private, loopback or link-local address. yt-dlp fetches from the host the bot runs on, which is now a home LAN, so any guild member could make it probe the router. - End yt-dlp's option parsing with -- before the stream target, so a target can never be read as a flag. - Give the CI job a read-only GITHUB_TOKEN. - Stop defaulting launch_ec2.sh to the owner's personal key pair name. - Document why PyNaCl stays at 1.5.0 despite pip-audit (discord.py caps it below 1.6; the advisory is in code voice never calls) and why curl_cffi is required although nothing imports it. - The README asked for the Server Members intent, which the bot never requests.
If spawning yt-dlp raised (out of processes under TasksMax, say), the exception escaped into the playback loop's catch-all, which destroyed the player: the bot left voice and nobody in the channel was told why. Opening the stream now lives in _open_stream, which announces the failure and returns None, and the loop moves on to the next track. The replay flag is cleared on that path too — left set after a failed effect respawn, it made _advance return a track that was gone, which read as an empty queue. tests/test_player_loop.py is the first test to run _player_loop end to end; both cases fail against the previous loop. Also drop discord.Forbidden from two except clauses that already catch its base class, HTTPException.
COMMAND_PREFIX is configurable, but the effects list, the effect help, the !lyrics hint, the Now Playing footer and the YouTube-blocked message all spelled commands with a literal '!'. On a server using another prefix they pointed at commands that do not exist there. Replies to a command use ctx.clean_prefix; embeds built without a context, and help text fixed at import time, use config.COMMAND_PREFIX. !move's help no longer repeats the signature the help already shows.
The project asks for type hints on every function; 85 had none, and the track dict that every layer passes around was described only in two docstrings. It is now services.media.Track, a TypedDict, and mypy runs clean with disallow_untyped_defs. Getting there surfaced two real problems: - Nothing limited commands to servers. !play or !queue sent in a DM died on ctx.guild / ctx.author.voice and answered with the generic 'something went wrong' plus a logged traceback. A global guild_only check now refuses DMs, and the error handler tells the user to use a server. Because the check guarantees a guild, utils.context.GuildContext tells the type checker so, without a None test in every command body. - !play resolves a link's host (the SSRF guard) between @user_in_voice and _ensure_voice, so the author can leave voice in between. That now gets the voice-channel message instead of an AttributeError. Mechanical parts: 'return await ctx.send(...)' became a send followed by 'return' so commands can be typed -> None, and one cast each where discord.py's stubs are wider than what this bot does (VoiceProtocol vs VoiceClient, IO[bytes] vs BufferedIOBase), with the reason beside it.
yt-dlp reports some durations as floats (SoundCloud: 187.43), and timedelta rendered those in Now Playing and the queue as 0:03:07.430000.
There was no linter and no type checker, so nothing held the type hints in place once written, and an undefined name in a rarely-run branch would only show up as a crash in production. ruff (correctness rules only: pyflakes, syntax errors, bugbear) and mypy (every function typed) now run before the tests. Both are pinned, so a new release cannot fail CI on unchanged code. The first ruff run already caught an unused import from the typing work. Also drops a trailing space and an unused variable in two tests.
The player-client chain was already shared between the metadata options and the streaming command because two copies drift apart. The format and the source address were still written out twice, with the same risk.
.env is anchored to the project root so the same file is found however the bot is started. A relative COOKIES_PATH was not: resolved against the working directory, cookies.txt was silently missed whenever the bot ran from anywhere else. Under systemd the two coincide, so the deployed bot behaves exactly as before.
It was GitHub's full Python template: Django, Flask, Scrapy, Celery, SageMath, Marimo and a dozen more, with the load-bearing .cache rule sitting under 'Unit test / coverage reports' where it read as test output. It is the runtime cache the systemd unit points XDG_CACHE_HOME and DENO_DIR at, and now says so. The set of ignored files in the checkout is unchanged.
setup.sh fetched Deno to the fixed path /tmp/deno.zip, unpacked it into /tmp and then moved /tmp/deno into /usr/local/bin with sudo. On a shared machine another user could place that file first. mktemp -d gives a directory only this user can write, and install sets the mode in one step.
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.
Closes #42.
Revisión completa del repo. Cada commit es un cambio independiente y se puede
revisar o revertir por separado. 506 tests (antes 475);
ruff check .ymypypasan limpios y ahora son pasos obligatorios del CI.Bugs
utils/context.guild_only)que los rechaza, y el manejador central responde "I only take commands in a
server". Como el check garantiza que hay guild,
GuildContextse lo dice altype checker sin añadir una comprobación de
Noneen cada comando._open_streamavisa del fallo y el loop pasa a lasiguiente canción en vez de destruir el reproductor. También limpia la marca
de replay: si el respawn por un cambio de efecto fallaba,
_advancedevolvíaun track que ya no existía y el bot se desconectaba como si la cola estuviera
vacía.
tests/test_player_loop.pyes el primer test que corre_player_loopde punta a punta; ambos casos fallan con el loop anterior.ctx.clean_prefix; los embeds sin contextousan
config.COMMAND_PREFIX.COOKIES_PATHrelativo se lee desde la raíz del proyecto. Bajo systemdya coincidían, así que el bot desplegado no cambia.
!play. Ahora hay unawaitde DNS entre@user_in_voicey_ensure_voice, y si el autor sale en ese momento recibe elaviso de canal de voz, no un
AttributeError.Seguridad
!playrechaza links cuyo host resuelve a una dirección privada,de loopback o link-local (incluye
169.254.169.254e IPv4 disfrazada de IPv6).No sigue redirecciones, porque de eso se encarga yt-dlp: reduce la exposición,
no la elimina.
--antes del destino en el comando de yt-dlp. Hoy no era explotable(
_search_targetanteponeytsearch1:), pero el argv de un subproceso nodebe depender de eso.
permissions: contents: read.mktemp -dprivado y se instala coninstall -m 755.launch_ec2.shexigeKEY_NAME; ya no usa un key pair personal por defecto.<1.6, y el aviso está enla validación de puntos ed25519 de libsodium, que la voz no usa. Queda
documentado en
requirements.txt.Tipado y herramientas
services.media.Track(TypedDict).mypy.ini(disallow_untyped_defs) yruff.toml(solo corrección: F, E9, B),ambos fijados en
requirements-dev.txt. El primer pase de ruff ya encontró unimport sin usar de este mismo trabajo.
Limpieza
source_addressde yt-dlp se definen una sola vez, como yapasaba con los player clients.
.gitignorepasa de la plantilla completa de GitHub (218 líneas) a las 41reglas que usa el proyecto. El conjunto de archivos ignorados es idéntico, y
la regla de
.cache, que es la caché de runtime de systemd y no de tests,ahora lo explica.
docs/design/.Cómo probar
En Discord, después de desplegar:
!play algopor DM → "I only take commands in a server".!play http://192.168.1.1/→ "I can only play links to public websites."!play sc: lofi→ la duración sale sin decimales.COMMAND_PREFIX=?en.env:?effectslista?bassy?reset.!play,!skip, un efecto a mitad de canción,!lyrics) sincambios de comportamiento.