mirror of
https://github.com/magnus919/agent-skills.git
synced 2026-09-11 19:47:12 +03:00
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>
This commit is contained in:
Magnus Hedemark
co-authored by
factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
parent
a31381bd37
commit
2a9a81e29b
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user