fix: escape Content-Disposition filenames and guard malformed align responses - #10
Merged
Conversation
…esponses - ls_export now quote-escapes title/artist before embedding them in the Content-Disposition filename param, and adds an RFC 5987 filename*= fallback, so an embedded quote can't corrupt/break out of the header. - ls_align now wraps resp.json() in a try/except so a non-JSON response from the configured align server returns a clean 502 instead of an unhandled 500. Fixes #7, #8
There was a problem hiding this comment.
ℹ️ Minor suggestions only — both fixes are sound.
Reviewed changes
ls_alignmalformed-response guard (routes.py:223-229): wrapsresp.json()in atry/except ValueError, returning a clean 502 instead of an unhandled 500 when the alignment server returns non-JSON. Correct exception to catch (JSONDecodeErrorinherits fromValueErrorinrequests).ls_exportContent-Disposition escaping (routes.py:270-283): escapes"and\in the legacyfilename=parameter, and adds an RFC 5987filename*=UTF-8''...form withurllib.parse.quotefor non-ASCII names. Standard and correct approach.
Big Pickle (free) | 𝕏
pullfrog review on PR #10 flagged that the \\ -> \\\\ replacement in ascii_name is a no-op today (safe_name already stripped \\ above) — harmless but confusing without context. Added a one-line comment explaining it's a belt-and-suspenders guard for if that stripping ever changes, rather than dead code.
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
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.

What
Two small robustness/security fixes found during a prior security audit:
ls_export:title/artistwere inserted intoContent-Disposition: filename="{safe_name}.lrc"without escaping", so a quote in title/artist could break out of the quoted filename parameter and corrupt the download filename. Now escapes"/\for the legacyfilename=fallback and adds an RFC 5987filename*=UTF-8''...form.ls_align:resp.json()wasn't wrapped in a try/except, so a non-JSON response from the configured align server produced an unhandled 500. Now returns a clean 502 on a malformed response.Fixes #7, Fixes #8
Checklist
routes.pyparses (python3 -c "import ast; ast.parse(...)")plugin.jsonversion (1.0.1 → 1.0.2)Generated by Claude Code