fix: prevframe round 2 — non-dict payload crash, generative-unit scoping bug
Second adversarial pass on the previous fix found two real regressions it introduced: a non-dict prev_scene_media (malformed client JSON) threw AttributeError inside the request's try block, failing the ENTIRE generate call instead of gracefully leaving the slot unconnected; and the extraction filename was scoped to target.scenes[0].id, which is the first scene of whatever generative unit _solo() expanded the request into — not the scene actually being generated — so two concurrent solo requests for different scenes in the same unit would clobber each other's extracted frame. Also swapped the hand-rolled tempdir-containment check for folder_paths.is_within_directory, which the codebase already has and resolves symlinks before comparing (the hand-rolled version did not). Added direct tests for _extract_prev_scene_frame itself (malformed input, traversal, missing ffmpeg, scene-scoped naming) — round 1's fix had none, only builder-level tests of its output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
cc7e06893b
commit
094694bcc1
@@ -424,7 +424,7 @@ def _extract_prev_scene_frame(prev_media: Optional[dict], scene_id: str) -> Opti
|
||||
import os
|
||||
import shutil
|
||||
import subprocess
|
||||
if not prev_media or not prev_media.get("filename"):
|
||||
if not isinstance(prev_media, dict) or not prev_media.get("filename"):
|
||||
return None
|
||||
if shutil.which("ffmpeg") is None:
|
||||
return None
|
||||
@@ -438,8 +438,8 @@ def _extract_prev_scene_frame(prev_media: Optional[dict], scene_id: str) -> Opti
|
||||
# is then re-checked against tempdir in case a crafted subfolder still escaped it.
|
||||
fn_in = os.path.basename(str(prev_media["filename"]))
|
||||
sub_in = str(prev_media.get("subfolder") or "").strip("/\\")
|
||||
src = os.path.normpath(os.path.join(tempdir, sub_in, fn_in))
|
||||
if os.path.commonpath([src, tempdir]) != tempdir or not os.path.isfile(src):
|
||||
src = os.path.join(tempdir, sub_in, fn_in)
|
||||
if not folder_paths.is_within_directory(tempdir, src) or not os.path.isfile(src):
|
||||
return None
|
||||
out_fn = f"funpack_prevframe_{scene_id}.png"
|
||||
out_path = os.path.join(tempdir, out_fn)
|
||||
@@ -1848,8 +1848,13 @@ if web is not None and PromptServer is not None:
|
||||
sampler_inputs, _settings = pipeline_caps.apply_simple_mode(sampler_inputs, _settings)
|
||||
studio_inputs["studio_settings"] = json.dumps(_settings)
|
||||
prev_scene_media = body.get("prev_scene_media")
|
||||
# `only_scene` (not target.scenes[0]) is the requesting scene: _solo() expands
|
||||
# `target` to every scene sharing a generative unit, so two concurrent solo
|
||||
# requests for different scenes in the same unit would otherwise scope their
|
||||
# extraction to the same first-scene id and clobber each other's frame.
|
||||
scene_scope = body.get("only_scene") or (target.scenes[0].id if target.scenes else "run")
|
||||
prev_scene_frame = (
|
||||
_extract_prev_scene_frame(prev_scene_media, target.scenes[0].id if target.scenes else "run")
|
||||
_extract_prev_scene_frame(prev_scene_media, scene_scope)
|
||||
if prev_scene_media else None
|
||||
)
|
||||
graph, report = builder.build(oi, models_cfg, {
|
||||
|
||||
@@ -14,6 +14,7 @@ from movie_editor.server import (
|
||||
_build_render_filter,
|
||||
_clip_bytes_for_media,
|
||||
_clip_needs_trim,
|
||||
_extract_prev_scene_frame,
|
||||
_has_graphics_export_content,
|
||||
_parse_has_scenes,
|
||||
_parse_prompt_variants,
|
||||
@@ -60,6 +61,66 @@ def test_list_temp_media_lists_media_newest_first(tmp_path, monkeypatch):
|
||||
clip = next(f for f in files if f["filename"] == "clip.webm")
|
||||
assert clip["subfolder"] == "previews"
|
||||
assert clip["kind"] == "video"
|
||||
# ── _extract_prev_scene_frame: prevframe extraction, never a media-bin asset ──────
|
||||
|
||||
|
||||
def _fake_folder_paths(tempdir, monkeypatch):
|
||||
fake = type("FP", (), {
|
||||
"get_temp_directory": staticmethod(lambda: str(tempdir)),
|
||||
"is_within_directory": staticmethod(
|
||||
lambda d, t: __import__("os").path.commonpath(
|
||||
[__import__("os").path.realpath(d), __import__("os").path.realpath(t)]
|
||||
) == __import__("os").path.realpath(d)),
|
||||
})
|
||||
monkeypatch.setitem(__import__("sys").modules, "folder_paths", fake)
|
||||
|
||||
|
||||
def test_prev_scene_frame_rejects_a_non_dict_payload(tmp_path, monkeypatch):
|
||||
"""A malformed prev_scene_media must degrade to 'nothing to extract', not blow up the
|
||||
whole generate request — it round-trips through client JSON and is never trusted."""
|
||||
_fake_folder_paths(tmp_path, monkeypatch)
|
||||
assert _extract_prev_scene_frame("not-a-dict", "scene1") is None
|
||||
assert _extract_prev_scene_frame(["also", "not"], "scene1") is None
|
||||
assert _extract_prev_scene_frame({}, "scene1") is None
|
||||
|
||||
|
||||
def test_prev_scene_frame_blocks_subfolder_traversal(tmp_path, monkeypatch):
|
||||
(tmp_path / "real.mp4").write_bytes(b"x")
|
||||
secret_dir = tmp_path.parent / "funpack_traversal_secret"
|
||||
secret_dir.mkdir(exist_ok=True)
|
||||
(secret_dir / "real.mp4").write_bytes(b"secret")
|
||||
_fake_folder_paths(tmp_path, monkeypatch)
|
||||
escaping = {"filename": "real.mp4", "subfolder": f"../{secret_dir.name}"}
|
||||
assert _extract_prev_scene_frame(escaping, "scene1") is None
|
||||
|
||||
|
||||
def test_prev_scene_frame_returns_none_without_ffmpeg(tmp_path, monkeypatch):
|
||||
(tmp_path / "clip.mp4").write_bytes(b"x")
|
||||
_fake_folder_paths(tmp_path, monkeypatch)
|
||||
monkeypatch.setattr(__import__("shutil"), "which", lambda name: None)
|
||||
assert _extract_prev_scene_frame({"filename": "clip.mp4"}, "scene1") is None
|
||||
|
||||
|
||||
def test_prev_scene_frame_scopes_output_name_to_the_requesting_scene(tmp_path, monkeypatch):
|
||||
"""Regression: the output filename must key off the scene actually asking (`only_scene`),
|
||||
not the first scene of whatever generative unit the server expanded it into — otherwise
|
||||
two different scenes in one unit clobber each other's extracted frame."""
|
||||
(tmp_path / "clip.mp4").write_bytes(b"x")
|
||||
_fake_folder_paths(tmp_path, monkeypatch)
|
||||
monkeypatch.setattr(__import__("shutil"), "which", lambda name: "/usr/bin/ffmpeg")
|
||||
|
||||
def fake_run(cmd, **kw):
|
||||
out_path = cmd[-1]
|
||||
with open(out_path, "wb") as f:
|
||||
f.write(b"png")
|
||||
return type("R", (), {"returncode": 0})()
|
||||
monkeypatch.setattr(__import__("subprocess"), "run", fake_run)
|
||||
|
||||
fn = _extract_prev_scene_frame({"filename": "clip.mp4"}, "scene_b")
|
||||
assert fn == "funpack_prevframe_scene_b.png"
|
||||
assert (tmp_path / fn).is_file()
|
||||
|
||||
|
||||
FRESH = "__funpack_fresh_prompt__"
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user