Repository navigation

Fix SSRF check bypass, rate-limit !lyrics, and clean up - #86

Merged

Isma-L154 merged 8 commits into

mainfrom

fix/85-review-cleanup

Oct 11, 2026

Merged

Fix SSRF check bypass, rate-limit !lyrics, and clean up#86
Isma-L154 merged 8 commits into
mainfrom
fix/85-review-cleanup

Isma-L154 commented Oct 11, 2026

Owner

Closes #85.

Context

A repository-wide review of code quality and security found two security gaps, two user-visible bugs and some cleanup. Each change is its own commit.

Changes

Security

Private-network check bypass (efb8bac). !play ran media.is_public_url only when the query started with a lowercase http, while _is_playlist_url lowercased the query first. !play HTTP://10.0.0.1/?list=x reached yt-dlp unchecked; under systemd IPAddressDeny still blocked it, outside systemd nothing did. media.is_url is now the single, case-insensitive definition used by the play check, the playlist detection, the yt-dlp target and the suggestions. It also stops a search such as httpster live from being refused as a link.
!lyrics rate limit (2efdb96). One lookup can make two LRCLIB and two Genius requests. It is now limited to 3 per 10 s per user, and the README's security section says so.

Bugs

!nowplaying credited whoever ran it instead of whoever queued the track (c9ce4af).
!queue 0, negative pages and pages past the end rendered wrong positions or an empty page; they now show the nearest real page (c9ce4af).

Cleanup

Drop the loop= parameter from media.search/search_many/get_playlist/related. Every caller passed the running loop, and the fallback used the deprecated get_event_loop() (944efe6).
Effects._switch_to takes an Effect (3ab94cd).
Unexpected command errors are logged at ERROR, like the player loop's (d37c53a).
Small readability fixes in the player, the Now Playing view and the voice-state listener (ebe75be).

How to test

Automated: ruff check ., mypy and pytest -ra pass locally (628 passed, 1 POSIX-only skip). New regression tests:

test_play_refuses_links_into_private_networks: uppercase-scheme and playlist-path cases.
test_play_does_not_mistake_a_search_for_a_link
test_url_detection_ignores_the_case_of_the_scheme
test_a_word_starting_with_http_is_a_search_not_a_link
test_lyrics_is_limited_per_user
test_nowplaying_credits_whoever_queued_the_track
test_queue_pages_out_of_range_show_the_nearest_real_one

Manual, in a test server:

!play HTTP://192.168.1.1/?list=x replies "I can only play links to public websites."
!play httpster live searches YouTube instead of being refused.
User A runs !play <song>, then user B runs !np. The embed credits A.
With fewer than 10 tracks queued, !queue 0 and !queue 5 both show page 1.
!lyrics bohemian rhapsody four times within 10 s: the fourth gets "Slow down".

Isma-L154 added 8 commits

October 10, 2026 21:37


          Detect links case-insensitively so !play cannot skip the private-netw…

efb8bac

…ork check

!play ran media.is_public_url only when the query started with a lowercase
"http", while _is_playlist_url lowercased first. HTTP://10.0.0.1/?list=x
therefore reached yt-dlp unchecked. media.is_url is now the one definition
every caller shares; it also stops a search like "httpster" from being
refused as a link.

Refs #85


          Rate-limit !lyrics per user

2efdb96

A single lookup can make two LRCLIB and two Genius requests, and nothing
limited how often one user could trigger it. Three per ten seconds, per user,
matches the shape of the !play limit.

Refs #85


          Credit the real requester in !nowplaying and clamp !queue pages

c9ce4af

!nowplaying passed ctx.author as the requester, so it credited whoever asked
rather than whoever queued the track. !queue 0, a negative page or one past
the end rendered wrong positions or an empty page; it now shows the nearest
real page.

Refs #85


          Drop the unused loop parameter from the media API

944efe6

Every caller passed bot.loop, which is the running loop, and the default fell
back to the deprecated get_event_loop(). _run now takes the running loop
itself, as services.lyrics_api already does.

Refs #85


          Pass an Effect to Effects._switch_to instead of its fields

3ab94cd

Each of the three callers unpacked filter, label and rate by hand; reset now
uses a named no-effect Effect, so every path reads the same.

Refs #85


          Log unexpected command errors at ERROR

d37c53a

What reaches the end of errors.handle is a bug or an outage, not a user
mistake. The player loop and the lyrics follower already log theirs at ERROR;
commands logged the same class of failure as a warning.

Refs #85


          Tidy small readability issues in the player, view and voice listener

ebe75be

- _prefetch_delay guarded a Track that is never None.
- set_volume tested vc.source before an isinstance that already rejects None.
- The alone-in-channel test was written out twice; it is now one helper.
- Fix a misaligned continuation line in NowPlayingControls.

Refs #85


          Document the !lyrics rate limit in the README

a0325d3

Refs #85

Isma-L154 merged commit 57ef730 into main

8 checks passed

Isma-L154 deleted the fix/85-review-cleanup branch

October 11, 2026 03:45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet