From f4eb0af10405730d0234895d9127d1dca7e45d98 Mon Sep 17 00:00:00 2001 From: shoobee <417196+shoobee@users.noreply.github.com> Date: Sun, 10 May 2026 20:48:01 +0000 Subject: [PATCH] fix(youtube): address Greptile review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three changes from automated review on PR #376: 1. Add `--` option terminator before host in _wrap_ytdlp_cmd (P1 security) Prevents SSH option injection if LAST30DAYS_YT_SSH_HOST were ever set to a value starting with `-` (e.g. `-oProxyCommand=...`). Low exploitability since the env var is user-controlled config — but the fix is a single arg and turns a self-harm footgun into no footgun. 2. Hoist `import shlex` to module-level (P2 style) Pure stdlib import, no reason for the deferred form. Cleaner. 3. Cache _ytdlp_ssh_host() result in fetch_transcript (P2 style) Was being called 2-3x per video; the function is cheap (env lookup + strip) so this is purely about readability. Adds test_wrap_cmd_uses_option_terminator covering the security fix explicitly with a `-oFoo=bar` host value. Updates index assertions in the two existing tests that check command shape (host is now at index 4, command string at 5, with `--` at 3). --- skills/last30days/scripts/lib/youtube_yt.py | 11 ++++--- tests/test_youtube_yt.py | 36 +++++++++++++++------ 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/skills/last30days/scripts/lib/youtube_yt.py b/skills/last30days/scripts/lib/youtube_yt.py index a34068c..d805124 100644 --- a/skills/last30days/scripts/lib/youtube_yt.py +++ b/skills/last30days/scripts/lib/youtube_yt.py @@ -10,6 +10,7 @@ import json import math import os import re +import shlex import shutil import sys import tempfile @@ -134,13 +135,14 @@ def _wrap_ytdlp_cmd(cmd: List[str]) -> List[str]: Args are shell-quoted to survive the remote shell. Uses BatchMode=yes so a misconfigured key fails fast instead of hanging on a password prompt. + The `--` option terminator prevents an SSH option-injection if + LAST30DAYS_YT_SSH_HOST were ever set to a value starting with `-`. """ host = _ytdlp_ssh_host() if not host: return cmd - import shlex remote_cmd = " ".join(shlex.quote(a) for a in cmd) - return ["ssh", "-o", "BatchMode=yes", host, remote_cmd] + return ["ssh", "-o", "BatchMode=yes", "--", host, remote_cmd] def _extract_core_subject(topic: str) -> str: @@ -520,14 +522,15 @@ def fetch_transcript(video_id: str, temp_dir: str) -> Optional[str]: # file on the remote host that we can't easily read back. Skip it and # use the HTTP transcript fallback (different YouTube endpoint, less # bot-walled, works fine from datacenter IPs). - use_ytdlp = is_ytdlp_installed() and not _ytdlp_ssh_host() + ssh_host = _ytdlp_ssh_host() + use_ytdlp = is_ytdlp_installed() and not ssh_host if use_ytdlp: raw_vtt = _fetch_transcript_ytdlp(video_id, temp_dir) if not raw_vtt: _log(f"yt-dlp transcript failed for {video_id}, trying direct HTTP fallback") raw_vtt = _fetch_transcript_direct(video_id) else: - if _ytdlp_ssh_host(): + if ssh_host: _log("SSH-routing active, using direct HTTP transcript fetch") else: _log("yt-dlp not installed, using direct HTTP transcript fetch") diff --git a/tests/test_youtube_yt.py b/tests/test_youtube_yt.py index 2071749..ed8d1d4 100644 --- a/tests/test_youtube_yt.py +++ b/tests/test_youtube_yt.py @@ -452,10 +452,13 @@ class TestYtdlpSSHRouting(unittest.TestCase): self.assertEqual(wrapped[0], "ssh") self.assertEqual(wrapped[1], "-o") self.assertEqual(wrapped[2], "BatchMode=yes") - self.assertEqual(wrapped[3], "macmini") + # `--` terminates SSH option parsing so a host starting with `-` + # (e.g. `-oProxyCommand=...`) cannot be reinterpreted as a flag. + self.assertEqual(wrapped[3], "--") + self.assertEqual(wrapped[4], "macmini") # Final arg is the shell-quoted command string - self.assertIn("yt-dlp", wrapped[4]) - self.assertIn("ytsearch5:test", wrapped[4]) + self.assertIn("yt-dlp", wrapped[5]) + self.assertIn("ytsearch5:test", wrapped[5]) def test_wrap_cmd_quotes_args_with_spaces(self): """Args containing spaces or special chars are shell-quoted.""" @@ -463,7 +466,21 @@ class TestYtdlpSSHRouting(unittest.TestCase): cmd = ["yt-dlp", "ytsearch5:hello world", "--dump-json"] wrapped = youtube_yt._wrap_ytdlp_cmd(cmd) # shlex.quote wraps the whole arg in single quotes when it contains spaces - self.assertIn("'ytsearch5:hello world'", wrapped[4]) + self.assertIn("'ytsearch5:hello world'", wrapped[5]) + + def test_wrap_cmd_uses_option_terminator(self): + """`--` is inserted before host to prevent SSH option injection. + + Without `--`, an env var like `LAST30DAYS_YT_SSH_HOST=-oProxyCommand=...` + would be parsed by ssh as an option flag. The terminator forces it + to be treated as a hostname (which will then fail clean if invalid). + """ + os.environ["LAST30DAYS_YT_SSH_HOST"] = "-oFoo=bar" + cmd = ["yt-dlp", "--version"] + wrapped = youtube_yt._wrap_ytdlp_cmd(cmd) + # Find the `--` terminator and verify the host comes immediately after + dash_idx = wrapped.index("--") + self.assertEqual(wrapped[dash_idx + 1], "-oFoo=bar") def test_is_ytdlp_installed_short_circuits_with_ssh(self): """is_ytdlp_installed returns True without local check when SSH routing is on.""" @@ -489,11 +506,12 @@ class TestYtdlpSSHRouting(unittest.TestCase): youtube_yt.search_youtube("test", "2026-02-01", "2026-03-01") cmd = run_mock.call_args.args[0] self.assertEqual(cmd[0], "ssh") - self.assertEqual(cmd[3], "macmini") - # The shell-quoted yt-dlp invocation lives at index 4 - self.assertIn("yt-dlp", cmd[4]) - self.assertIn("--ignore-config", cmd[4]) - self.assertIn("--no-cookies-from-browser", cmd[4]) + self.assertEqual(cmd[3], "--") + self.assertEqual(cmd[4], "macmini") + # The shell-quoted yt-dlp invocation lives at index 5 + self.assertIn("yt-dlp", cmd[5]) + self.assertIn("--ignore-config", cmd[5]) + self.assertIn("--no-cookies-from-browser", cmd[5]) if __name__ == "__main__":