From 2a9a81e29b50504e40666ea8dbbd174765e041d8 Mon Sep 17 00:00:00 2001 From: Magnus Hedemark Date: Sun, 30 Aug 2026 00:45:16 -0400 Subject: [PATCH] fix(jellyfin): harden dispatch against subcommand-named flag values main() sliced argv at the first occurrence of a known subcommand name, so a value-flag pair whose value names a subcommand (e.g. login's --server given `search`) made argparse dispatch the wrong subparser: `jellyfin --server search browse ...` errored inside the `search` sub-parser instead of running browse. Dispatch now splits such misplaced pairs out of the top-level argv (find_subcommand_token + split_misplaced_value_pairs) and re-attaches them to the command tail, where each handler's parse_known_args already tolerates unknown flags. A properly placed occurrence of the same flag later in the tail still wins. Any other pre-command token (unknown flags, stray positionals, `--`, dangling value flags) is untouched, so argparse errors stay byte-identical to the pre-hardening CLI. Also dedupe the twice-repeated "Bundled CLI --dry-run and exit-code contract" section in references/worked-recipes.md (user-testing round 1 finding); content merged into one section. Adds DispatchHardeningTests: the mis-slice scenario for every subcommand shape, clean-argv dispatch for all 11 subcommands, flag priority, argparse-owned error paths, and unit pinning of both helper return-value tables. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- jellyfin/references/worked-recipes.md | 22 ---- jellyfin/scripts/jellyfin | 81 ++++++++++++-- jellyfin/scripts/test_jellyfin_cli.py | 151 ++++++++++++++++++++++++++ 3 files changed, 224 insertions(+), 30 deletions(-) diff --git a/jellyfin/references/worked-recipes.md b/jellyfin/references/worked-recipes.md index 90bc52d..3629745 100644 --- a/jellyfin/references/worked-recipes.md +++ b/jellyfin/references/worked-recipes.md @@ -153,28 +153,6 @@ Exit codes: `0` on success (including dry-run previews), `1` on CLI errors (miss credentials, unreachable server, API 4xx/5xx, missing required `--user-id`), `2` on argparse misuse such as `--movies --episodes` together or a missing required flag. -## Bundled CLI `--dry-run` and exit-code contract - -The CLI's dry-run plans are pinned by its offline test suite (`scripts/test_jellyfin_cli.py`), -so jq keys match tested reality exactly. Every plan carries: - -```json -{ "dry_run": true, "path": "/Items/Latest", "params": { "userId": null, "limit": 10 } } -``` - -- `dry_run` (bool, always true), `path` (string) and `params` (object) appear on every - command plan; `login` instead emits `path: "/Users/AuthenticateByName"`, - `server`, `username`, `authorization_header`, and `pre_token_header: true` (its - `authorization_header` is the complete pre-token MediaBrowser header, no `Token=` - segment); `info` composes a `requests` array of `{path, params}` steps instead of a - single `path`/`params` pair. -- `params` mirrors the exact query the live call would send (`userId` is JSON `null` - when not supplied). - -Exit codes: `0` on success (including dry-run previews), `1` on CLI errors (missing -credentials, unreachable server, API 4xx/5xx, missing required `--user-id`), `2` on -argparse misuse such as `--movies --episodes` together or a missing required flag. - ## Cross-version-safe baseline (derive your own recipes from these rules) 1. Speak modern auth (`Authorization: MediaBrowser ...`) and put the token in exactly ONE diff --git a/jellyfin/scripts/jellyfin b/jellyfin/scripts/jellyfin index 909e611..d56d329 100755 --- a/jellyfin/scripts/jellyfin +++ b/jellyfin/scripts/jellyfin @@ -34,6 +34,16 @@ ENV_PASSWORD = os.getenv("JELLYFIN_PASSWORD", "") CLIENT_NAME = "jellyfin-cli" CLIENT_VERSION = "1.0.0" +# Option strings of the subcommand parsers that consume a value (store_true +# flags excluded). A flag whose VALUE names a subcommand (e.g. login's --server +# given `search`) must not be mistaken for the command itself; keep this in +# sync when adding subcommand flags. Flags listed here behave exactly as +# before when they appear after the subcommand. +VALUE_FLAGS = ( + "--server --username -u --password --query -q --type --limit --user-id " + "--series-id --season-id --id --library-id --device-id --start-index" +).split() + GLOBAL_FLAGS: Dict[str, Any] = {"json": False, "dry_run": False} @@ -94,6 +104,55 @@ def _preparse_global_flags(argv): return flags, filtered +def find_subcommand_token(argv, subcommands, value_flags): + """Locate the first subcommand token in argv that no flag consumes as a value. + + argv[0] is the program name. Returns (command_index, pair_start): the index + of the first subcommand token that is not a flag value, and the index where + the consumed flag/value pairs begin (None when no value names a subcommand). + A value-flag paired with a value that NAMES a subcommand (e.g. `--server + search`) must not hijack dispatch, so the pair is consumed and scanning + continues for the first unconsumed subcommand token. Any other token the + top-level parser would reject (unknown flags, stray positionals, `--`, a + dangling value-flag, or a value that is not a subcommand name) stops the + scan with command_index=None so argparse produces its usual error. + """ + i = 1 + pair_start = None + while i < len(argv): + token = argv[i] + if token in value_flags: + if i + 1 >= len(argv): + break # flag with no value: argparse reports the misuse + if pair_start is None and argv[i + 1] in subcommands: + pair_start = i # a value naming a subcommand must not dispatch + i += 2 # consume the flag and its value as a pair + continue + if token in subcommands: + return i, pair_start + break # unknown flag, stray positional, or `--`: argparse owns this error + return None, pair_start + + +def split_misplaced_value_pairs(argv, subcommands, value_flags): + """Split argv into (parse_argv, misplaced_pairs) around a hijacking flag value. + + A value-flag pair whose value NAMES a subcommand while sitting before the + real command (e.g. `--server search browse`) would make argparse dispatch + the wrong subparser. Such pairs are lifted out of parse_argv and returned + as misplaced_pairs so main() can re-attach them to the command tail, where + parse_known_args handles them as it handles any unknown flag today. Any + other pre-command token (unknown flags, stray positionals, `--`, a + dangling value-flag) is left in place so argparse keeps reporting it + exactly as before, and argv without the mis-slice shape comes back + unchanged with an empty misplaced_pairs list. + """ + command_index, pair_start = find_subcommand_token(argv, subcommands, value_flags) + if command_index is None or pair_start is None: + return argv, [] + return argv[:pair_start] + argv[command_index:], argv[pair_start:command_index] + + class JellyfinClient: """Jellyfin API client (10.8+ compatible, modern Authorization header).""" @@ -663,25 +722,31 @@ def main(): sub.add_parser("libraries", help="List libraries", description="List configured media libraries.", epilog="Example: jellyfin libraries") sub.add_parser("stats", help="Library statistics", description="Show media library item counts.", epilog="Example: jellyfin stats") - args = parser.parse_args(filtered_argv[1:]) - if not args.command: - parser.print_help() - sys.exit(1) - - client = JellyfinClient(dry_run=GLOBAL_FLAGS.get("dry_run", False)) - cmd_map = { "login": cmd_login, "info": cmd_info, "recent": cmd_recent, "search": cmd_search, "next-up": cmd_next_up, "item": cmd_item, "seasons": cmd_seasons, "episodes": cmd_episodes, "browse": cmd_browse, "libraries": cmd_libraries, "stats": cmd_stats, } + + parse_argv, misplaced = split_misplaced_value_pairs(filtered_argv, cmd_map, VALUE_FLAGS) + args = parser.parse_args(parse_argv[1:]) + if not args.command: + parser.print_help() + sys.exit(1) + + client = JellyfinClient(dry_run=GLOBAL_FLAGS.get("dry_run", False)) + handler = cmd_map.get(args.command) if not handler: parser.print_help() sys.exit(1) - remaining = filtered_argv[filtered_argv.index(args.command) + 1:] + # Misplaced flag/value pairs are re-attached ahead of the command tail so + # parse_known_args sees them where it tolerates unknown flags today; a + # properly placed occurrence of the same flag later in the tail still wins. + tail_start = parse_argv.index(args.command) + 1 + remaining = misplaced + parse_argv[tail_start:] handler(client, remaining) diff --git a/jellyfin/scripts/test_jellyfin_cli.py b/jellyfin/scripts/test_jellyfin_cli.py index c14ae18..32a2aac 100644 --- a/jellyfin/scripts/test_jellyfin_cli.py +++ b/jellyfin/scripts/test_jellyfin_cli.py @@ -612,5 +612,156 @@ class PipelineChainTests(unittest.TestCase): "DateCreated") +class DispatchHardeningTests(unittest.TestCase): + """main() must dispatch on the first UNCONSUMED subcommand token. + + A value-flag pair whose value NAMES a subcommand while sitting before the + real command (e.g. `--server search browse ...`) used to make argparse + dispatch the wrong subparser (the token was swallowed as the flag's value + and the real command shifted into the value slot). The hardened dispatch + lifts such pairs out of the top-level argv and re-attaches them to the + command tail, where parse_known_args already tolerates unknown flags. + Every other pre-command token still reaches argparse so its errors are + byte-identical to the pre-hardening CLI. + """ + + def run_cli(self, *args): + return subprocess.run([str(SCRIPT), *args], text=True, capture_output=True, + env=clean_env(), cwd=tempfile.gettempdir()) + + def test_flag_value_equal_to_subcommand_dispatches_browse_not_search(self): + # The exact mis-slice scenario: `--server search` must not dispatch + # the `search` sub-parser; the real command is `browse`. + result = self.run_cli("--server", "search", "browse", + "--library-id", "lib-1", "--dry-run", "--json") + self.assertEqual(result.returncode, 0, result.stderr) + payload = json.loads(result.stdout) + self.assertEqual(payload["path"], "/Items") + self.assertEqual(payload["params"]["parentId"], "lib-1") + + def test_value_hijack_variants_dispatch_the_real_command(self): + cases = ( + (("recent", "--user-id", "u1", "--limit", "3"), "/Items/Latest"), + (("search", "--query", "dune"), "/Search/Hints"), + (("next-up", "--user-id", "u1"), "/Shows/NextUp"), + (("item", "--id", "i1", "--user-id", "u1"), "/Items/i1"), + (("seasons", "--series-id", "s1", "--user-id", "u1"), "/Shows/s1/Seasons"), + (("episodes", "--series-id", "s1", "--user-id", "u1"), "/Shows/s1/Episodes"), + (("libraries",), "/Library/MediaFolders"), + (("stats",), "/Items/Counts"), + ) + for command, expected_path in cases: + with self.subTest(command=command): + result = self.run_cli("--server", "search", *command, + "--dry-run", "--json") + self.assertEqual(result.returncode, 0, result.stderr) + payload = json.loads(result.stdout) + self.assertEqual(payload["path"], expected_path) + + def test_hijack_variants_cover_flag_command_and_no_flag_value_commands(self): + # info emits a `requests` array instead of a path/params pair. + result = self.run_cli("--server", "search", "info", "--dry-run", "--json") + self.assertEqual(result.returncode, 0, result.stderr) + payload = json.loads(result.stdout) + self.assertEqual(payload["requests"][0]["path"], "/System/Info") + + # login keeps its plan shape; the misplaced pair rides along as the + # server value rather than being dropped. + result = self.run_cli("--server", "search", "login", "--username", "alice", + "--dry-run", "--json") + self.assertEqual(result.returncode, 0, result.stderr) + payload = json.loads(result.stdout) + self.assertEqual(payload["path"], "/Users/AuthenticateByName") + self.assertEqual(payload["server"], "search") + self.assertTrue(payload["pre_token_header"]) + + def test_all_subcommands_dispatch_from_clean_argv(self): + cases = ( + (("login", "--username", "alice"), "/Users/AuthenticateByName"), + (("info",), "/System/Info"), + (("recent", "--user-id", "u1"), "/Items/Latest"), + (("search", "--query", "dune"), "/Search/Hints"), + (("next-up", "--user-id", "u1"), "/Shows/NextUp"), + (("item", "--id", "i1", "--user-id", "u1"), "/Items/i1"), + (("seasons", "--series-id", "s1", "--user-id", "u1"), "/Shows/s1/Seasons"), + (("episodes", "--series-id", "s1", "--user-id", "u1"), "/Shows/s1/Episodes"), + (("browse", "--library-id", "lib-1"), "/Items"), + (("libraries",), "/Library/MediaFolders"), + (("stats",), "/Items/Counts"), + ) + for command, expected_path in cases: + with self.subTest(command=command[0]): + result = self.run_cli(*command, "--dry-run", "--json") + self.assertEqual(result.returncode, 0, result.stderr) + payload = json.loads(result.stdout) + if "requests" in payload: # info composes a request array + self.assertEqual(payload["requests"][0]["path"], expected_path) + else: + self.assertEqual(payload["path"], expected_path) + + def test_properly_placed_flag_value_still_wins_over_misplaced_pair(self): + result = self.run_cli("--server", "search", "login", + "--server", "http://real:8096", + "--username", "alice", "--dry-run", "--json") + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(json.loads(result.stdout)["server"], "http://real:8096") + + def test_pre_command_tokens_argparse_owns_are_unchanged(self): + # Unknown flags, stray positionals, a non-subcommand --server value, + # a dangling value flag, and `--` all keep their pre-hardening + # argparse errors (exit 2, no traceback, no tolerant dispatch). + cases = ( + ("--bogus", "info"), + ("junk", "browse", "--library-id", "lib-1"), + ("--server", "http://x:8096", "info"), + ("--server",), + ("--", "search", "--query", "dune"), + ) + for argv in cases: + with self.subTest(argv=argv): + result = self.run_cli(*argv, "--dry-run", "--json") + self.assertEqual(result.returncode, 2) + self.assertIn("error:", result.stderr) + self.assertNotIn("Traceback", result.stderr) + + def test_find_subcommand_token_returns_command_and_pair_indices(self): + cli = jellyfin_cli + subs = {"login", "info", "recent", "search", "next-up", "item", "seasons", + "episodes", "browse", "libraries", "stats"} + cases = ( + (["jf", "--server", "search", "browse", "--library-id", "L"], (3, 1)), + (["jf", "browse", "--library-id", "L"], (1, None)), + (["jf", "--server", "http://x", "info"], (3, None)), + (["jf", "login", "--server", "search"], (1, None)), + (["jf", "--bogus", "info"], (None, None)), + (["jf", "--server"], (None, None)), + (["jf", "--"], (None, None)), + ) + for argv, expected in cases: + with self.subTest(argv=argv): + self.assertEqual( + cli.find_subcommand_token(argv, subs, cli.VALUE_FLAGS), expected) + + def test_split_misplaced_value_pairs_lifts_only_hijacking_pair(self): + cli = jellyfin_cli + subs = {"login", "info", "recent", "search", "next-up", "item", "seasons", + "episodes", "browse", "libraries", "stats"} + parse_argv, misplaced = cli.split_misplaced_value_pairs( + ["jf", "--server", "search", "browse", "--library-id", "L"], + subs, cli.VALUE_FLAGS) + self.assertEqual(parse_argv, ["jf", "browse", "--library-id", "L"]) + self.assertEqual(misplaced, ["--server", "search"]) + + # A value that is not a subcommand name never lifts anything, and + # clean argv passes through untouched. + parse_argv, misplaced = cli.split_misplaced_value_pairs( + ["jf", "--server", "http://x", "info"], subs, cli.VALUE_FLAGS) + self.assertEqual((parse_argv, misplaced), (["jf", "--server", "http://x", "info"], [])) + parse_argv, misplaced = cli.split_misplaced_value_pairs( + ["jf", "login", "--server", "search", "--username", "a"], + subs, cli.VALUE_FLAGS) + self.assertEqual(misplaced, []) + + if __name__ == "__main__": unittest.main()