Skip to content

Commit feac780

Browse files
committed
Use resolve_dir for direct URL destination (follow-up to #19)
_run_direct_url_with_cli now calls resolve_dir on the destination, matching the queue path in engine/job_queue.py:_resolve_job_output_dir. This handles empty/relative/absolute destinations consistently and rejects paths that escape DOWNLOADS_DIR (e.g. "../etc") with a ValueError, instead of silently resolving outside the base. Adds two regression tests in tests/test_direct_url_contracts.py: - relative destination "Singles" lands under single_downloads_dir/Singles/ - escape destination "../../etc" raises ValueError and download_with_ytdlp is never invoked Addresses the review comment on #19.
1 parent a12c873 commit feac780

3 files changed

Lines changed: 89 additions & 8 deletions

File tree

.gitignore

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,5 @@
66
.pytest_cache/
77
.DS_Store
88
.vscode/
9-
_project_management/
9+
_project_management/
10+
.venv/

api/main.py

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3551,13 +3551,9 @@ def _run_direct_url_with_cli(
35513551
if not url or not isinstance(url, str):
35523552
raise ValueError("single_url is required")
35533553

3554-
# Resolve destination (default to configured DOWNLOADS_DIR).
3555-
# Relative paths are resolved against DOWNLOADS_DIR, matching the job queue path.
3556-
if destination:
3557-
raw = destination.strip()
3558-
dest_dir = raw if os.path.isabs(raw) else os.path.join(str(DOWNLOADS_DIR), raw)
3559-
else:
3560-
dest_dir = str(DOWNLOADS_DIR)
3554+
# Match the queue path: resolve_dir handles empty/relative/absolute and rejects
3555+
# destinations that escape the base via inputs like "../etc".
3556+
dest_dir = resolve_dir((destination or "").strip(), paths.single_downloads_dir)
35613557
ensure_dir(dest_dir)
35623558

35633559
# Direct URL runs are intentionally NOT persisted into the unified download_jobs queue.

tests/test_direct_url_contracts.py

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ def api_module(monkeypatch, tmp_path: Path):
3131
db_path=str(db_path),
3232
temp_downloads_dir=str(temp_dir),
3333
thumbs_dir=str(thumbs_dir),
34+
single_downloads_dir=str(tmp_path),
3435
)
3536
module.app.state.state = "idle"
3637
module.app.state.current_download_proc = None
@@ -145,6 +146,89 @@ def _fake_download_with_ytdlp(url, temp_dir, config_arg, **kwargs):
145146
assert files[0].endswith(".mkv")
146147

147148

149+
def test_server_direct_url_relative_destination_resolves_under_downloads_dir(
150+
api_module, monkeypatch, tmp_path: Path
151+
) -> None:
152+
module = api_module
153+
module.app.state.paths.single_downloads_dir = str(tmp_path / "downloads_root")
154+
Path(module.app.state.paths.single_downloads_dir).mkdir(parents=True, exist_ok=True)
155+
config = {
156+
"final_format": "mkv",
157+
"filename_template": "VID-%(title)s.%(ext)s",
158+
}
159+
160+
def _fake_download_with_ytdlp(url, temp_dir, config_arg, **kwargs):
161+
_ = config_arg, kwargs
162+
output = Path(temp_dir) / "payload.mkv"
163+
output.parent.mkdir(parents=True, exist_ok=True)
164+
output.write_bytes(b"video")
165+
return {
166+
"id": "abc123xyz99",
167+
"title": "Video Title",
168+
"uploader": "Channel",
169+
"webpage_url": url,
170+
}, str(output)
171+
172+
monkeypatch.setattr(module, "get_loaded_config", lambda: config)
173+
monkeypatch.setattr(module, "download_with_ytdlp", _fake_download_with_ytdlp)
174+
monkeypatch.setattr(module, "embed_metadata", lambda *args, **kwargs: None)
175+
monkeypatch.setattr(module, "_record_direct_url_history", lambda *args, **kwargs: None)
176+
monkeypatch.setattr(module, "ensure_mb_bound_music_track", lambda *args, **kwargs: None)
177+
178+
module._run_direct_url_with_cli(
179+
url="https://youtu.be/-LI8X-GhFA8",
180+
paths=module.app.state.paths,
181+
config=config,
182+
destination="Singles",
183+
final_format_override="mkv",
184+
media_type="video",
185+
media_intent="episode",
186+
music_mode=False,
187+
stop_event=threading.Event(),
188+
status=None,
189+
)
190+
191+
singles_dir = Path(module.app.state.paths.single_downloads_dir) / "Singles"
192+
files = [p.name for p in singles_dir.glob("*") if p.is_file()]
193+
assert len(files) == 1
194+
assert files[0].startswith("VID-Video Title.")
195+
assert files[0].endswith(".mkv")
196+
197+
198+
def test_server_direct_url_escape_destination_is_rejected(
199+
api_module, monkeypatch, tmp_path: Path
200+
) -> None:
201+
module = api_module
202+
module.app.state.paths.single_downloads_dir = str(tmp_path / "downloads_root")
203+
Path(module.app.state.paths.single_downloads_dir).mkdir(parents=True, exist_ok=True)
204+
config = {"final_format": "mkv"}
205+
206+
called = {"download": False}
207+
208+
def _fake_download_with_ytdlp(*args, **kwargs):
209+
called["download"] = True
210+
raise AssertionError("download_with_ytdlp should not be called for escape destinations")
211+
212+
monkeypatch.setattr(module, "get_loaded_config", lambda: config)
213+
monkeypatch.setattr(module, "download_with_ytdlp", _fake_download_with_ytdlp)
214+
215+
with pytest.raises(ValueError, match="within base directory"):
216+
module._run_direct_url_with_cli(
217+
url="https://youtu.be/-LI8X-GhFA8",
218+
paths=module.app.state.paths,
219+
config=config,
220+
destination="../../etc",
221+
final_format_override="mkv",
222+
media_type="video",
223+
media_intent="episode",
224+
music_mode=False,
225+
stop_event=threading.Event(),
226+
status=None,
227+
)
228+
229+
assert called["download"] is False
230+
231+
148232
def test_client_direct_url_music_mode_is_rejected(api_module) -> None:
149233
module = api_module
150234
config = {"music_final_format": "mp3"}

0 commit comments

Comments
 (0)