From 65c8e60a7a5854eaa39c35fbf35140e6a0cf8fdd Mon Sep 17 00:00:00 2001 From: imSp4rky Date: Sun, 1 Nov 2026 20:21:42 -0700 Subject: [PATCH] fix(api): check and cast download job service options like a session does A bad value now gets 400 INVALID_PARAMETERS at submit and the worker casts each option; a flat dl key such as quality or lang no longer fills a service option of the same name. --- docs/dev/rest-api/endpoints.md | 6 ++- .../unit/test_api_service_dl_overrides.py | 41 +++++++++++++++++- tests/remote/unit/test_download_manager.py | 34 +++++++++++++++ unshackle/core/api/download_manager.py | 6 ++- unshackle/core/api/handlers.py | 43 +++++++++++-------- unshackle/core/utils/service_params.py | 24 +++++++++++ 6 files changed, 130 insertions(+), 24 deletions(-) diff --git a/docs/dev/rest-api/endpoints.md b/docs/dev/rest-api/endpoints.md index d3e2ef43..4381318d 100644 --- a/docs/dev/rest-api/endpoints.md +++ b/docs/dev/rest-api/endpoints.md @@ -492,9 +492,11 @@ Make a download job. It requires `service` and `title_id`. Every other field is **Service options.** A service's own CLI options travel in the same body. Send them in a nested `service_params` object, or as flat top-level keys. When the body has `service_params`, the server -reads service options only from it, and every flat key is a `dl` value. An option you leave out +reads service options only from it, and every flat key is a `dl` value. Without it, a flat key that +is also a `dl` option, such as `quality` or `lang`, stays a `dl` value. An option you leave out takes the default declared on the service's `cli` command, so the job matches the equivalent `dl` -run. `profile` at the top level always means the credential +run. The server checks each service option against that option's type, and a value it refuses gets +`400 INVALID_PARAMETERS`. `profile` at the top level always means the credential profile, never a service's own `--profile` option. === "Request" diff --git a/tests/remote/unit/test_api_service_dl_overrides.py b/tests/remote/unit/test_api_service_dl_overrides.py index 8899eb3a..9cb9fb01 100644 --- a/tests/remote/unit/test_api_service_dl_overrides.py +++ b/tests/remote/unit/test_api_service_dl_overrides.py @@ -4,10 +4,12 @@ import asyncio from datetime import datetime from types import SimpleNamespace +import click import pytest from unshackle.core.api import download_manager as dm from unshackle.core.api import handlers +from unshackle.core.api.errors import APIError, APIErrorCode pytestmark = pytest.mark.unit @@ -106,8 +108,6 @@ def test_client_sent_keys_without_stamp_counts_all_keys(): def test_service_defaults_and_options_travel_nested(env, monkeypatch): - import click - @click.command() @click.option("--profile", type=int, default=1) @click.option("--device", default="chrome") @@ -119,3 +119,40 @@ def test_service_defaults_and_options_travel_nested(env, monkeypatch): assert params["profile"] == "work" assert params["service_params"] == {"profile": 2, "device": "chrome"} assert env(body={"device": "tv"})["service_params"] == {"profile": 1, "device": "tv"} + + +def use_cli(monkeypatch, *options): + """Make the loaded service declare ``options`` on its cli.""" + cli = click.Command("svc", params=list(options)) + monkeypatch.setattr(handlers.Services, "load", lambda service: SimpleNamespace(cli=cli)) + + +def test_a_service_option_its_type_refuses_is_a_400(env, monkeypatch): + use_cli(monkeypatch, click.Option(["--episodes"], type=click.IntRange(1, 3), default=1)) + with pytest.raises(APIError) as exc_info: + env(body={"service_params": {"episodes": 9}}) + assert exc_info.value.error_code == APIErrorCode.INVALID_PARAMETERS + assert "episodes" in exc_info.value.message + assert env(body={"service_params": {"episodes": "2"}})["service_params"]["episodes"] == "2" + + +@pytest.mark.parametrize( + ("name", "value"), + [("episodes", {"a": 1}), ("episodes", [1]), ("movie", 2), ("movie", [1]), ("device", {"a": 1}), ("device", [1])], +) +def test_a_json_shape_click_cannot_convert_is_a_400(env, monkeypatch, name, value): + use_cli( + monkeypatch, + click.Option(["--episodes"], type=int), + click.Option(["--movie"], is_flag=True), + click.Option(["--device"]), + ) + with pytest.raises(APIError) as exc_info: + env(body={"service_params": {name: value}}) + assert exc_info.value.error_code == APIErrorCode.INVALID_PARAMETERS + assert name in exc_info.value.message + + +def test_a_flat_dl_key_never_fills_a_service_option_of_the_same_name(env, monkeypatch): + use_cli(monkeypatch, click.Option(["--quality"], type=click.Choice(["SD", "HD"]), default="HD")) + assert env(body={"quality": [1080]})["service_params"] == {"quality": "HD"} diff --git a/tests/remote/unit/test_download_manager.py b/tests/remote/unit/test_download_manager.py index 943d5ebe..418880b8 100644 --- a/tests/remote/unit/test_download_manager.py +++ b/tests/remote/unit/test_download_manager.py @@ -268,6 +268,40 @@ def test_perform_download_passes_latest_episodes_to_dl_result(tmp_path: Path, mo assert exc.value.args[0]["latest_episodes"] == 3 +def test_perform_download_casts_service_options(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + import click + + import unshackle.commands.dl as dl_module + from unshackle.core.api import download_manager, handlers + from unshackle.core.services import Services + + class FakeDl: + cli = click.Command("dl") + + def __init__(self, ctx: click.Context, **kwargs: object) -> None: + pass + + class FakeService: + cli = click.Command( + "svc", params=[click.Option(["--episodes"], type=int), click.Option(["--tag"], multiple=True)] + ) + + def __init__(self, ctx: click.Context, episodes: object = None, tag: object = ()) -> None: + raise _CapturedCtx(episodes, tag) + + monkeypatch.setattr(dl_module, "dl", FakeDl) + monkeypatch.setattr(Services, "get_path", staticmethod(lambda s: tmp_path)) + monkeypatch.setattr(Services, "load", staticmethod(lambda s: FakeService)) + monkeypatch.setattr(handlers, "load_full_cdm", lambda *a: None) + + with pytest.raises(_CapturedCtx) as exc: + download_manager.perform_download( + "job-1", "EXAMPLE", "t1", {"service_params": {"episodes": "2", "tag": ["a", "b"]}} + ) + + assert exc.value.args == (2, ("a", "b")) + + def test_worker_relays_a_prompt_and_reads_the_answer(monkeypatch: pytest.MonkeyPatch) -> None: import json import os diff --git a/unshackle/core/api/download_manager.py b/unshackle/core/api/download_manager.py index b6f38edf..98ecef64 100644 --- a/unshackle/core/api/download_manager.py +++ b/unshackle/core/api/download_manager.py @@ -21,7 +21,7 @@ from unshackle.core.api.events import bus, publish_service_event from unshackle.core.api.sanitize import sanitize_log from unshackle.core.utils.click_types import AUDIO_CODEC_LIST, QUALITY_LIST, VIDEO_CODEC_LIST from unshackle.core.utils.redact import REDACTED, URL_USERINFO_RE, redact_all, redact_path, redact_text -from unshackle.core.utils.service_params import service_options +from unshackle.core.utils.service_params import cast_service_options, service_options if TYPE_CHECKING: from unshackle.core.service import Service @@ -579,7 +579,9 @@ def perform_download( if "title" in service_init_params: service_kwargs["title"] = title_id - service_kwargs.update(service_options(params, service_init_params.keys() - {"title"})) + cli_params = getattr(getattr(service_module, "cli", None), "params", None) or [] + options = service_options(params, service_init_params.keys() - {"title"}) + service_kwargs.update(cast_service_options(cli_params, service_ctx, options)) for param_name, param_info in service_init_params.items(): if param_name not in service_kwargs and param_name not in ["self", "ctx"]: diff --git a/unshackle/core/api/handlers.py b/unshackle/core/api/handlers.py index bd33b6a7..8acc48f5 100644 --- a/unshackle/core/api/handlers.py +++ b/unshackle/core/api/handlers.py @@ -56,7 +56,7 @@ from unshackle.core.utils.click_types import AUDIO_CODEC_LIST, QUALITY_LIST, SUB from unshackle.core.utils.collections import ci_get from unshackle.core.utils.hdr10plus import probed_tracks from unshackle.core.utils.redact import REDACTED, URL_USERINFO_RE, redact_all, redact_secrets, redact_text -from unshackle.core.utils.service_params import service_options +from unshackle.core.utils.service_params import FLAT_DL_KEYS, cast_service_options, service_options log = logging.getLogger("api") @@ -253,6 +253,17 @@ def build_parent_ctx( return parent +def cast_or_refuse(params: Iterable[click.Parameter], ctx: Any, options: Dict[str, Any]) -> Dict[str, Any]: + """``cast_service_options``, with a refused value as ``400 INVALID_PARAMETERS``.""" + try: + return cast_service_options(params, ctx, options) + except click.BadParameter as e: + name = e.param.name if e.param and e.param.name else "?" + raise APIError( + APIErrorCode.INVALID_PARAMETERS, f"Invalid service option '{sanitize_log(name)}': {sanitize_log(e.message)}" + ) from e + + def instantiate_service( parent_ctx: Any, service_module: Any, @@ -266,22 +277,13 @@ def instantiate_service( goes through its option's type here: JSON or a remote client's str-typed stub can send "1" for an int. """ cli_params = getattr(getattr(service_module, "cli", None), "params", []) or [] - params_by_name = {p.name: p for p in cli_params if getattr(p, "name", None) and p.name != "title"} - extras: Dict[str, Any] = {} - if data: - for k, v in service_options(data, params_by_name, transport_keys).items(): - try: - extras[k] = params_by_name[k].type_cast_value(parent_ctx, v) - except click.BadParameter as e: - raise APIError( - APIErrorCode.INVALID_PARAMETERS, f"Invalid service option '{sanitize_log(k)}': {e.message}" - ) from e - if data.get("service_params") is None: - for k in params_by_name.keys() & transport_keys & data.keys(): - if k not in ("service", "title_id", "proxy", "no_proxy") and data.get(k) is not None: - log.warning( - f"Ignoring flat '{sanitize_log(k)}' as a service option, send it under 'service_params'" - ) + names = {p.name for p in cli_params if getattr(p, "name", None) and p.name != "title"} + data = data or {} + extras = cast_or_refuse(cli_params, parent_ctx, service_options(data, names, transport_keys)) + if data and data.get("service_params") is None: + for k in names & transport_keys & data.keys(): + if k not in ("service", "title_id", "proxy", "no_proxy") and data.get(k) is not None: + log.warning(f"Ignoring flat '{sanitize_log(k)}' as a service option, send it under 'service_params'") return parent_ctx.invoke(service_module.cli, title=title, **extras) @@ -2491,7 +2493,12 @@ async def download_handler(data: Dict[str, Any], request: Optional[web.Request] for param in cli_params if param.name and param.default is not None and not isinstance(param.default, enum.Enum) } - service_params = {**defaults, **service_options(data, {param.name for param in cli_params})} + # A flat dl key (quality, lang) stays a dl value, as in instantiate_service. + sent = service_options( + data, {param.name for param in cli_params}, FLAT_DL_KEYS | DEFAULT_DOWNLOAD_PARAMS.keys() + ) + cast_or_refuse(cli_params, click.Context(click.Command(normalized_service)), sent) + service_params = {**defaults, **sent} filtered_params = {k: v for k, v in data.items() if k not in ["service", "title_id"]} # Overlay any dl-relevant keys from `serve:` config (e.g. downloads, workers) so the API diff --git a/unshackle/core/utils/service_params.py b/unshackle/core/utils/service_params.py index 7aed319e..7e598185 100644 --- a/unshackle/core/utils/service_params.py +++ b/unshackle/core/utils/service_params.py @@ -1,5 +1,7 @@ from typing import Any, Collection, Dict, Iterable +import click + FLAT_DL_KEYS = frozenset({"title", "service", "title_id", "profile", "proxy", "no_proxy"}) @@ -16,3 +18,25 @@ def service_options( return {k: v for k, v in nested.items() if k in names} excluded = set(flat_excluded) return {k: v for k, v in data.items() if k in names and k not in excluded} + + +def cast_service_options(params: Iterable[click.Parameter], ctx: Any, options: Dict[str, Any]) -> Dict[str, Any]: + """``options`` with each value cast by the click parameter of the same name; raises ``click.BadParameter``. + + A name that no parameter declares keeps its value. A JSON object, or an array for a single-value option, + is refused before the cast, since click would stringify it. + """ + by_name = {p.name: p for p in params if p.name} + cast: Dict[str, Any] = {} + for k, v in options.items(): + param = by_name.get(k) + if param is None: + cast[k] = v + continue + if isinstance(v, dict) or (isinstance(v, list) and not param.multiple and param.nargs == 1): + raise click.BadParameter("expected a single value, not a JSON object or array", ctx=ctx, param=param) + try: + cast[k] = param.type_cast_value(ctx, v) + except (TypeError, ValueError, AttributeError) as e: # convert() only promises BadParameter for strings + raise click.BadParameter(f"not a valid {param.type.name} value", ctx=ctx, param=param) from e + return cast