Palash Debnath
73ebd6518d
* fix(errors): four failures that reached users as raw OS text (#1262, #1256, #1251, #1252) #1262 — a voice profile named in any non-latin-1 script 500'd every download endpoint with "'latin-1' codec can't encode characters in position 22-25". `attachment; filename="` is exactly 22 characters, so those were the first four characters of the user's own name. The sanitisers in front of the header filtered with str.isalnum(), which is True for every alphabetic script — they stripped punctuation and passed exactly what breaks the header. Ten sites, one RFC 6266 builder, plus a guard so an eleventh can't be hand-written. #1256 — a synth died on FileNotFoundError: 'ffprobe' and was reported as "an error OmniVoice doesn't recognize", on a Mac where the app's own ffprobe was resolvable the whole time. Our call sites pass explicit paths; a dependency shelling out by bare name does not. The resolved directories are now published on PATH, and the failure is classified either way. #1251 — "The paging file is too small" reached the user as a bare 500. It was already counted as an OOM, but that remedy (close apps, lighter engine) is wrong on a 32 GB machine — the fix is a Windows setting, and the hint now says which. Matched on the code in both the Python and Rust spellings. #1252/#1253 — deleting a dub mid-import crashed it with `ingest: 'mgw39lx3'`: str(KeyError) is the repr of the key. The pipeline blind-subscripted a job that DELETE /dub/history/{id} had popped minutes earlier. It now stops quietly, and no exception whose str() is a bare value can present itself that way again. * fix(engines): Unload 400'd, a wrong language said nothing, DRM was retried by hand (#1247, #1257, #1254) #1247 — list_loaded() advertises in-process engines as `engine:<id>` with "unloadable": true, but unload() only ever handled tts/diarization/sidecars. The panel was rendering a button for ids the dispatcher rejected. The engines already implement unload(); only the routing was missing. The contract test written for it immediately found a second instance — `capture-asr`, listed the same way with no branch either — which is why it enumerates the listing rather than hard-coding ids. #1257 — MLXAudioBackend.supported_languages() returns ["multi"] on the stated assumption that "each engine silently ignores languages it doesn't know". It doesn't; the library raises. So the picker offers all 646 languages and the rejection arrived as a bare list of 23 codes, naming neither the engine nor the way out. Enumerating each model's real language set would be a brittle map that goes stale every engine update — name the engine and the fix instead. #1254 — reported as intermittent: the same URL failed as DRM-protected, then succeeded on retry. Real DRM doesn't lapse; the player client varies. That is the same shape as the 403 case which already escalates through _YT_PLAYER_CLIENTS, so DRM now routes into it. If every client still refuses, the failure is classified instead of arriving as a raw yt-dlp line. * fix(review): close the delete race, narrow the tool match, sanitize the fallback Greptile P1 + CodeRabbit Major — verified real, and mine: splitting merge from save left a window where a delete lands between them, so the pending save UPSERTs the row straight back and a dub the user deleted reappears. Now one atomic step under _dub_jobs_lock, with both delete endpoints purging rows and memory under that same lock. That also fixed DELETE /dub/history, which deleted every row but evicted nothing — an in-flight job survived 'clear history' outright and re-saved itself on completion. CodeRabbit Minor (#1256): the media-tool match accepted any message ending in 'ffmpeg'/'ffprobe', so a missing FILE at /tmp/ffmpeg got the 'repair your media engine' remedy. Now requires the name unquoted-and-unqualified. CodeRabbit Major (#1262): `fallback` reached the header verbatim whenever the real name folded away entirely, walking past every guard the name goes through. Folded like the name. CodeRabbit Major (#1256): the PATH log printed resolved directories, and a user-set FFMPEG_PATH sits under their home. Logs a count now. CodeRabbit Minor (#1262): the subtitle-route assertion also passed against the pre-fix header; it now asserts filename*= too. Skipped: 'Highlights bullets must end with (#N)'. CLAUDE.md scopes that to the ### subsections; none of the seven pre-existing highlights carry refs, and tests/test_changelog_style.py encodes the rule already. * fix(review): the remaining unlocked save paths, an over-broad signature, two weak tests Greptile P1 — the mid-pipeline put_job + save_job pairs were still unlocked, so a clear-history landing between them left a ghost row behind the purge. Both go through put_and_save_job now; only the final completion gate decides whether a withdrawn job's work is kept. CodeRabbit Major (#1257) — 'unsupported language' as a bare prefix also matches 'Unsupported language model configuration', handing a model/config failure engine-switch advice it has no use for. The loose wordings now require the rejected thing to be a code or to end there. CodeRabbit Major (#1257) — the OOM test asserted on SOURCE TEXT, which passes even if the call is unreachable or its result discarded; #1224 taught this same lesson on this codebase. Both it and the language rewrite now drive the real _run_backend_inference with a raising backend. CodeRabbit Minor (#1257) — 'or "engine" in message' always passed, since the production template contains the word. Asserts the resolved class name now. CodeRabbit Major (#1252) — the delete-race test deleted the job BEFORE the merge, which only re-tested the absent case and would pass with the two steps still split. It now interleaves a real second thread against a slow save. CodeRabbit Major (#1256) — a hardcoded /tmp literal trips Ruff S108; built from tmp_path instead. * fix(review): a withdrawal must survive the job's first write CodeRabbit Major — the concern is real, though its suggested fix (gate the checkpoint on the job already existing) would break creation: an ingest's FIRST persistence is what creates the entry, so that gate would never pass. The actual defect is that dict membership cannot express 'withdrawn'. An absent key means 'not written yet' for a new job and 'deleted' for an established one — two opposite instructions from one signal. So a clear-history arriving before the first checkpoint was silently undone by that checkpoint recreating the row, and the run then persisted its result into history the user had just cleared. Tombstone it explicitly: the ingest declares itself in flight, a purge marks any in-flight id withdrawn, and both write paths refuse a withdrawn id. Released in , so it's bounded by concurrent ingests and can't poison a later run that reuses the id. That also fixed clear-history properly: a job with no row yet appears in no id list, so only an in-flight sweep can catch it. CodeRabbit Minor — my race test waited on an event that could not be set while the save held the lock, so it burned its full 2s timeout every run and synchronised nothing. It now waits for the purge thread to REACH the purge. * test(dub): the race test was not testing the race Caught by verifying fail-before rather than trusting the test: splitting merge from save — the exact resurrection bug — passed all 22 tests. The assertions checked WHAT happened (the save ran, the row was deleted, the job left memory) but never WHEN. A save landing after the delete is indistinguishable from one landing before if you only assert that both occurred — and 'after' is precisely the resurrection. Now recorded and asserted as an order. With merge+save atomic the purge cannot start until the save finishes, so the sequence is always save-then-delete; split them and it fails with ['delete', 'save']. That is the second time this test needed rewriting: v1 deleted the job before the merge and only re-checked the absent case, v2 interleaved a real thread but asserted the wrong thing. Both looked like tests. Also documents why the DB write sits inside the lock (atomicity beats a rare 5 s sqlite busy-timeout stall) and that no locked region calls another, so the non-reentrant lock cannot deadlock — verified by walking every locked region. * fix(dub): gate the withdrawal at save_job, not at its callers Greptile P1 — and the same class I'd already fixed, unfixed elsewhere. The withdrawal check sat in the two ingest helpers, but eight direct save_job call sites across dub generate / translate / export / core bypass those entirely. Deleting a dub mid-RENDER therefore still resurrected it, which is at least as likely as deleting mid-import. Moved the gate into save_job itself: one choke point, every caller inherits it, and the ninth cannot forget. That needs a re-entrant lock, since the atomic helpers call save_job while already holding it — a plain Lock would deadlock the backend, so a test pins the lock type and another exercises the nested path. Verified fail-before: removing the gate fails the new test. * fix(dub): the withdrawal only covered ingests, so it covered almost nothing Caught by testing the reported scenario directly instead of trusting a green suite: CI passed, 26 tests passed, and a dub deleted during a RENDER was still resurrected. The tombstone was scoped to in-flight ingests. But a dub is imported once and rendered many times, so the realistic delete lands during a render — long after its ingest ended — and end_ingest was CLEARING the tombstone at exactly that point. The rare case was protected and the common one left open. Now scoped to deletions, not ingests. Kept in a bounded LRU rather than cleared on completion, because there is no moment at which a delete stops mattering: any operation still holding that job can persist it. Re-importing an id is the only thing that legitimately revives it. Verified fail-before: the previous scoping fails three of the new tests. * refactor: move the dub delete-resurrection fix to its own PR (#1270) The six fixes left here are independent error-message changes that needed no corrections. The dub concurrency change needed five rounds, each finding something real in work that was already reviewed, tested and CI-green — the last of them being that the fix did not fix the reported case at all. Riding a release on that record is a bad trade, so it ships separately as #1270. This branch keeps #1262, #1256, #1251, #1247, #1257 and #1254; the KeyError message half goes with the dub PR, since it is that issue's other half.
157 lines
6.3 KiB
Python
157 lines
6.3 KiB
Python
"""#1262: a non-Latin voice-profile name 500'd every download endpoint.
|
||
|
||
500 Internal Server Error: 'latin-1' codec can't encode characters in
|
||
position 22-25: ordinal not in range(256)
|
||
|
||
``attachment; filename="`` is exactly 22 characters long, so positions 22-25
|
||
were the first four characters of the reporter's own profile name. HTTP header
|
||
values are latin-1 by definition; every download endpoint interpolated the
|
||
filename straight into the header.
|
||
|
||
The sanitisers in front of those f-strings looked like they covered it — but
|
||
they all filtered with ``str.isalnum()``, which is ``True`` for *every*
|
||
alphabetic script. They stripped punctuation and let through exactly the
|
||
characters that break the header.
|
||
|
||
This was never one endpoint: the same ``isalnum()`` idiom was copy-pasted
|
||
across persona export, marketplace, stories, the OpenAI-compatible speech
|
||
route, and eight dub-export routes. All of them now go through one RFC 6266
|
||
builder, and a guard below keeps the next one from being written by hand.
|
||
"""
|
||
from __future__ import annotations
|
||
|
||
import pathlib
|
||
import re
|
||
|
||
import pytest
|
||
|
||
from core.http_headers import ascii_filename, content_disposition
|
||
|
||
REPO = pathlib.Path(__file__).resolve().parents[1]
|
||
|
||
|
||
# ── the header is always encodable ───────────────────────────────────────
|
||
|
||
|
||
@pytest.mark.parametrize(
|
||
"name",
|
||
[
|
||
"我的声音.ovsvoice", # Chinese — the reported shape
|
||
"私の声.ovsvoice", # Japanese
|
||
"내 목소리.ovsvoice", # Korean
|
||
"Моя речь.ovsvoice", # Cyrillic
|
||
"φωνή.ovsvoice", # Greek
|
||
"קול.ovsvoice", # Hebrew
|
||
"🎙️ voice.ovsvoice", # emoji
|
||
"Sébastien’s voix.ovsvoice", # accented Latin + smart quote
|
||
],
|
||
)
|
||
def test_the_header_survives_any_script(name):
|
||
header = content_disposition(name)
|
||
# The actual failure: Starlette encodes header values as latin-1.
|
||
header.encode("latin-1")
|
||
|
||
|
||
def test_the_exact_reported_failure():
|
||
"""A four-character CJK name — the one that produced 'position 22-25'."""
|
||
header = content_disposition("我的声音.ovsvoice")
|
||
header.encode("latin-1")
|
||
assert 'filename="' in header
|
||
assert "filename*=UTF-8''" in header
|
||
|
||
|
||
def test_the_real_name_is_preserved_for_modern_clients():
|
||
header = content_disposition("我的声音.ovsvoice")
|
||
# RFC 5987 percent-encoded UTF-8 — browsers prefer this over `filename=`.
|
||
assert "%E6%88%91%E7%9A%84%E5%A3%B0%E9%9F%B3" in header
|
||
|
||
|
||
def test_accented_latin_is_folded_not_deleted():
|
||
assert ascii_filename("Sébastien.ovsvoice") == "Sebastien.ovsvoice"
|
||
|
||
|
||
def test_an_entirely_non_ascii_name_still_yields_a_usable_filename():
|
||
"""Stripping CJK leaves only ".ovsvoice", which is not a filename."""
|
||
safe = ascii_filename("我的声音.ovsvoice")
|
||
assert safe.endswith(".ovsvoice")
|
||
stem = safe[: -len(".ovsvoice")]
|
||
assert stem and stem.strip("_ "), f"no usable stem in {safe!r}"
|
||
|
||
|
||
def test_ascii_names_are_left_alone():
|
||
assert ascii_filename("dubbed_output_en.mp4") == "dubbed_output_en.mp4"
|
||
header = content_disposition("dubbed_output_en.mp4")
|
||
assert 'filename="dubbed_output_en.mp4"' in header
|
||
|
||
|
||
@pytest.mark.parametrize("hostile", ['a"b.mp4', "a\\b.mp4", "a\r\nX-Evil: 1.mp4", "a/b/c.mp4"])
|
||
def test_quoting_and_header_injection_are_neutralised(hostile):
|
||
"""A dub filename comes from a video title, i.e. from the internet. A bare
|
||
quote would end the quoted-string; a CRLF would split the header."""
|
||
header = content_disposition(hostile)
|
||
header.encode("latin-1")
|
||
assert "\r" not in header and "\n" not in header
|
||
# Exactly the two parameters we intend, no smuggled third.
|
||
assert header.count("filename=") == 1
|
||
assert header.count("filename*=") == 1
|
||
|
||
|
||
def test_inline_disposition_is_supported():
|
||
"""The OpenAI-compatible speech route streams inline, not as a download."""
|
||
assert content_disposition("speech.mp3", disposition="inline").startswith("inline;")
|
||
|
||
|
||
# ── and no endpoint builds the header by hand again ──────────────────────
|
||
|
||
|
||
def test_no_router_interpolates_a_filename_into_the_header():
|
||
"""The recurrence guard. This bug shipped in ten places because the header
|
||
was written by f-string ten times; the eleventh must not compile."""
|
||
offenders = []
|
||
pattern = re.compile(r'"Content-Disposition"\s*:\s*f[\'"]')
|
||
for path in (REPO / "backend").rglob("*.py"):
|
||
if "test" in path.parts:
|
||
continue
|
||
for i, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1):
|
||
if pattern.search(line):
|
||
offenders.append(f"{path.relative_to(REPO)}:{i}")
|
||
assert offenders == [], (
|
||
"build the value with core.http_headers.content_disposition() — an "
|
||
"f-string here 500s on any non-latin-1 filename (#1262)"
|
||
)
|
||
|
||
|
||
def test_every_download_endpoint_actually_uses_the_builder():
|
||
"""Complements the guard above: proves the call sites were converted, not
|
||
merely reworded into something the regex misses."""
|
||
routers = REPO / "backend" / "api" / "routers"
|
||
users = {
|
||
path.name
|
||
for path in routers.rglob("*.py")
|
||
if "content_disposition(" in path.read_text(encoding="utf-8")
|
||
}
|
||
for expected in (
|
||
"personas.py",
|
||
"marketplace.py",
|
||
"stories.py",
|
||
"dub_export.py",
|
||
"openai_compat.py",
|
||
):
|
||
assert expected in users, f"{expected} still builds the header itself"
|
||
|
||
|
||
def test_a_hostile_custom_fallback_cannot_reach_the_header():
|
||
"""Review finding (#1262): `fallback` landed in `filename=` verbatim
|
||
whenever the real name folded away entirely, so a non-ASCII or CRLF
|
||
fallback walked past every guard the real name goes through."""
|
||
header = content_disposition("我的声音.ovsvoice", fallback='ev"il\r\nX-Evil: 1')
|
||
header.encode("latin-1")
|
||
assert "\r" not in header and "\n" not in header
|
||
assert header.count("filename=") == 1
|
||
assert header.count("filename*=") == 1
|
||
|
||
# A fallback that is ENTIRELY non-ASCII must still leave a usable name.
|
||
header = content_disposition("我的声音.ovsvoice", fallback="声音")
|
||
header.encode("latin-1")
|
||
assert 'filename=""' not in header
|