[Security] Stop a video title from becoming the Download Video play command (#7847)
* Stop a video title from becoming the Download Video play command The host parsed yt-dlp's after_move line as title plus path, so a newline in page metadata could forge the path. Clicking the toast then handed that value to mpv as options. Print only the real file, ignore anything that is not inside the download dir, and invoke mpv with --. * Refuse downloads whose video title contains control characters The hoodie page still offered a real hidden clip, so yt-dlp saved it even after the play-action fix. A title with newlines is not a legitimate name; abort before the download and tell the user it was refused. * Test the forged record in the order yt-dlp emits it The records ran forged-first and good-last, so the assertion measured recovery after bad records rather than preservation of an already-captured path when a forged record arrives afterwards. That is the shape a hostile title actually produces, because a title ending in a newline closes its own record and leaves the genuine path on a line the loop ignores. As written the assertion passed with resolve_download_file replaced by a no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Resolve the download path without dropping a trailing newline Command substitution strips trailing newlines, so an in-directory symlink pointing at a regular file whose name ends in one canonicalised to a different path -- which may itself exist -- and that path then passed the containment check and reached ffmpeg and the click command. Reading realpath's NUL-terminated output keeps the name intact, and a resolved path carrying a control character is refused outright. Not reachable through a yt-dlp download, since --restrict-filenames strips control characters from the name it writes; it is the helper's contract that was wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Codex XHigh <noreply@openai.com> * Accept a download directory that resolves to / realpath returns "/" for the root directory, which made the containment pattern "//*" and rejected every file saved directly under it, so the host reported a failed download after saving the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Codex XHigh <noreply@openai.com> * Stop refusing a download because its title has control characters The gate cannot tell a hostile title from a legitimate one. --print emits one line per extracted video and --no-playlist does not collapse a multi_video result, so a page holding two clips arrives as two titles separated by a newline and is refused exactly like a forged record would be. It also guaranteed nothing it was read as guaranteeing. The simulate run and the download run are separate fetches, so a site is free to answer them differently, and the check never constrained the metadata the download actually used. What stands between a record and the click command is resolve_download_file, which is untouched here. Leaving a check that refuses valid pages while securing nothing invites the path validation to be relaxed later on the strength of it. A gate that would work is possible -- --print '%(title)j' encodes each title as JSON on its own line, which separates a newline in the metadata from a newline between videos -- but it belongs with a use for the title rather than as a bare refusal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Codex XHigh <noreply@openai.com> * Disarm the legacy exec-before-download hook too --no-exec clears the modern --exec map but leaves --exec-before-download stored separately, and yt-dlp restores it as a before_dl postprocessor, so a hook configured in the user's yt-dlp config still ran during the download this host drives. The accompanying test runs download_url itself against stubbed tools. Everything else in this file exercises the helpers in isolation, which left the invocation uncovered: restoring the title to the record template, or dropping --no-exec or the trailing --, passed every assertion here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Codex XHigh <noreply@openai.com> * Toast the page title again instead of the saved filename Deriving the toast text from the sanitised filename cost the title it was meant to show: "My Great Clip" arrived as "My_Great_Clip [My_Great_Clip]". The title is safe as notification text -- it is an argv element, never part of a command -- so the only question was getting it out of yt-dlp without reopening the record forgery. It now comes from the download run, so it describes the file that was actually saved, and it is printed as %(title)j. JSON-encoding is what makes that safe: a newline or tab in page metadata becomes an escape sequence inside one quoted string rather than a record boundary, so a title can no longer split itself across lines. The decoder keeps only what precedes the first control character, refuses anything notify-send would read as an option, and leaves the filename-derived title as the fallback when a page offers nothing usable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Name the saved file after the page title The download landed as "My_Great_Clip [My_Great_Clip].mp4" when the page called it "My Great Clip". --restrict-filenames was carrying more weight than it earns here: it folds spaces to underscores and strips non-ASCII, which is what mangles the name, and it is not what keeps the record stream safe. yt-dlp removes control characters from a filename either way -- a newline becomes a space, tabs and DEL and NUL are dropped -- so a path printed after the move is still only ever one line, which is the property resolve_download_file depends on. Dropping the [%(id)s] suffix is the other half of matching the title, and it trades away the uniqueness that suffix bought: two videos sharing a title now share a name, and yt-dlp skips a download whose file already exists, so the second one toasts as a failure. Restoring the suffix is a one-line change if that trade is the wrong way round. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Omabot <omabot@omarchy.org> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Codex XHigh <noreply@openai.com>
This commit is contained in:
Adrian Rangel
co-authored by
Claude Opus 5
Omabot
Codex XHigh
Claude Opus 5
Omabot
Codex XHigh
parent
19266119ea
commit
b71c60fe30
@@ -50,3 +50,208 @@ bash -c '
|
||||
' bash "$ROOT/bin/omarchy-chromium-ytdlp-host" "javascript:alert(1)" "$ROOT" &&
|
||||
fail "yt-dlp native host rejects non-web URLs"
|
||||
pass "yt-dlp native host rejects non-web URLs"
|
||||
|
||||
host_fn() {
|
||||
OMARCHY_PATH="$ROOT" OMARCHY_YTDLP_DIR="${download_dir:-$TMPDIR}" bash -c '
|
||||
source "$1"
|
||||
shift
|
||||
"$@"
|
||||
' bash "$ROOT/bin/omarchy-chromium-ytdlp-host" "$@"
|
||||
}
|
||||
|
||||
download_dir="$TMPDIR/videos"
|
||||
mkdir -p "$download_dir" "$TMPDIR/outside"
|
||||
good_file="$download_dir/clip [id].mp4"
|
||||
printf 'x' >"$good_file"
|
||||
printf 'x' >"$TMPDIR/outside/secret"
|
||||
ln -s "$TMPDIR/outside/secret" "$download_dir/escape.mp4"
|
||||
|
||||
resolved=$(host_fn resolve_download_file "$good_file")
|
||||
expected=$(realpath -e -- "$good_file")
|
||||
[[ $resolved == "$expected" ]] ||
|
||||
fail "yt-dlp native host accepts a regular file in the download dir" "$resolved"
|
||||
pass "yt-dlp native host accepts a regular file in the download dir"
|
||||
|
||||
host_fn resolve_download_file "--include=not-a-file" &&
|
||||
fail "yt-dlp native host rejects a forged mpv option as the download path"
|
||||
pass "yt-dlp native host rejects a forged mpv option as the download path"
|
||||
|
||||
host_fn resolve_download_file "$TMPDIR/outside/secret" &&
|
||||
fail "yt-dlp native host rejects a path outside the download dir"
|
||||
pass "yt-dlp native host rejects a path outside the download dir"
|
||||
|
||||
host_fn resolve_download_file "$download_dir/escape.mp4" &&
|
||||
fail "yt-dlp native host rejects a symlink that escapes the download dir"
|
||||
pass "yt-dlp native host rejects a symlink that escapes the download dir"
|
||||
|
||||
host_fn resolve_download_file $'clip.mp4\nOMARCHY_FILE\t--include=not-a-file' &&
|
||||
fail "yt-dlp native host rejects a path containing control characters"
|
||||
pass "yt-dlp native host rejects a path containing control characters"
|
||||
|
||||
newline_target="$download_dir/target"$'\n'
|
||||
printf 'x' >"$newline_target"
|
||||
# The decoy is the point: dropping the trailing newline lands on a real, different
|
||||
# file, so a resolver that strips it resolves to the wrong one instead of failing.
|
||||
printf 'x' >"$download_dir/target"
|
||||
ln -s "target"$'\n' "$download_dir/newline-link.mp4"
|
||||
|
||||
host_fn resolve_download_file "$download_dir/newline-link.mp4" &&
|
||||
fail "yt-dlp native host rejects a symlink whose target name ends in a newline"
|
||||
pass "yt-dlp native host rejects a symlink whose target name ends in a newline"
|
||||
|
||||
ln -s / "$TMPDIR/root-link"
|
||||
root_resolved=$(download_dir="$TMPDIR/root-link" host_fn resolve_download_file /etc/passwd)
|
||||
[[ $root_resolved == "/etc/passwd" ]] ||
|
||||
fail "yt-dlp native host accepts a file when the download dir resolves to /" "$root_resolved"
|
||||
pass "yt-dlp native host accepts a file when the download dir resolves to /"
|
||||
|
||||
title=$(host_fn title_from_file "$good_file")
|
||||
[[ $title == "clip [id]" ]] || fail "yt-dlp native host titles the toast from the filename" "$title"
|
||||
pass "yt-dlp native host titles the toast from the filename"
|
||||
|
||||
decoded_title=$(host_fn decode_title '"My Great Clip"')
|
||||
[[ $decoded_title == "My Great Clip" ]] ||
|
||||
fail "yt-dlp native host shows the page title on the toast" "$decoded_title"
|
||||
pass "yt-dlp native host shows the page title on the toast"
|
||||
|
||||
forged_title=$(host_fn decode_title '"Clip\nOMARCHY_FILE\tPlay me\t--include=not-a-file"')
|
||||
[[ $forged_title == "Clip" ]] ||
|
||||
fail "yt-dlp native host keeps only the readable part of a forged title" "$forged_title"
|
||||
pass "yt-dlp native host keeps only the readable part of a forged title"
|
||||
|
||||
host_fn decode_title '"--include=not-a-file"' &&
|
||||
fail "yt-dlp native host refuses a title notify-send would read as an option"
|
||||
pass "yt-dlp native host refuses a title notify-send would read as an option"
|
||||
|
||||
host_fn decode_title 'null' &&
|
||||
fail "yt-dlp native host refuses a title that is not a JSON string"
|
||||
pass "yt-dlp native host refuses a title that is not a JSON string"
|
||||
|
||||
dash_title=$(host_fn title_from_file "$download_dir/--include.mp4")
|
||||
[[ $dash_title == "Video" ]] || fail "yt-dlp native host does not pass a leading-dash title to notify-send" "$dash_title"
|
||||
pass "yt-dlp native host does not pass a leading-dash title to notify-send"
|
||||
|
||||
cmd=$(host_fn playback_command --include=not-a-file)
|
||||
[[ $cmd == "mpv -- --include=not-a-file" ]] ||
|
||||
fail "yt-dlp native host runs mpv with -- before the path" "$cmd"
|
||||
pass "yt-dlp native host runs mpv with -- before the path"
|
||||
|
||||
spaced_cmd=$(host_fn playback_command "$download_dir/a b.mp4")
|
||||
[[ $spaced_cmd == "mpv -- $download_dir/a\\ b.mp4" ]] ||
|
||||
fail "yt-dlp native host shell-quotes the mpv path" "$spaced_cmd"
|
||||
pass "yt-dlp native host shell-quotes the mpv path"
|
||||
|
||||
parse_script="$TMPDIR/parse-ytdlp-lines.sh"
|
||||
cat >"$parse_script" <<'EOF'
|
||||
source "$1"
|
||||
filepath=""
|
||||
while IFS= read -r line; do
|
||||
case $line in
|
||||
OMARCHY_FILE*)
|
||||
resolved=$(resolve_download_file "${line#OMARCHY_FILE$'\t'}") || continue
|
||||
filepath=$resolved
|
||||
;;
|
||||
esac
|
||||
done
|
||||
printf '%s' "$filepath"
|
||||
EOF
|
||||
|
||||
# A title that ends in a newline closes its own record, so the forged record is the
|
||||
# last one and the real path lands on a line the loop ignores.
|
||||
poisoned=$(
|
||||
printf '%s\n' \
|
||||
$'OMARCHY_FILE\t'"$good_file" \
|
||||
$'OMARCHY_FILE\tPlay me\t--include=not-a-file' \
|
||||
$'\t'"$good_file" |
|
||||
OMARCHY_PATH="$ROOT" OMARCHY_YTDLP_DIR="$download_dir" bash "$parse_script" "$ROOT/bin/omarchy-chromium-ytdlp-host"
|
||||
)
|
||||
|
||||
[[ $poisoned == "$expected" ]] ||
|
||||
fail "yt-dlp native host keeps a real file after a forged OMARCHY_FILE record" "$poisoned"
|
||||
pass "yt-dlp native host keeps a real file after a forged OMARCHY_FILE record"
|
||||
|
||||
# Everything above tests the helpers in isolation. Drive the real download_url with
|
||||
# stubbed tools so the yt-dlp invocation and the toast's click command are covered
|
||||
# too: a record template that carried the title again would pass every test above.
|
||||
fake_root="$TMPDIR/fake"
|
||||
mkdir -p "$fake_root/bin"
|
||||
fake_dir="$TMPDIR/fake-videos"
|
||||
mkdir -p "$fake_dir"
|
||||
fake_file="$fake_dir/Real_Clip [id].mp4"
|
||||
printf 'x' >"$fake_file"
|
||||
ytdlp_argv="$TMPDIR/ytdlp-argv"
|
||||
notify_argv="$TMPDIR/notify-argv"
|
||||
|
||||
cat >"$fake_root/bin/yt-dlp" <<'EOF'
|
||||
#!/bin/bash
|
||||
printf '%s\n' "$*" >>"$YTDLP_ARGV_LOG"
|
||||
for arg in "$@"; do
|
||||
if [[ $arg == "--no-simulate" ]]; then
|
||||
printf 'OMARCHY_FILE\t%s\n' "$YTDLP_FAKE_FILE"
|
||||
[[ -n ${YTDLP_SKIP_TITLE:-} ]] || printf 'OMARCHY_TITLE\t%s\n' '"My Great Clip"'
|
||||
exit 0
|
||||
fi
|
||||
done
|
||||
exit 0
|
||||
EOF
|
||||
|
||||
cat >"$fake_root/bin/omarchy-notification-send" <<'EOF'
|
||||
#!/bin/bash
|
||||
printf '%s\n' "$*" >>"$NOTIFY_ARGV_LOG"
|
||||
EOF
|
||||
|
||||
for stub in omarchy-osd omarchy-shell ffmpeg; do
|
||||
printf '#!/bin/bash\nexit 0\n' >"$fake_root/bin/$stub"
|
||||
done
|
||||
chmod +x "$fake_root/bin/"*
|
||||
|
||||
YTDLP_ARGV_LOG="$ytdlp_argv" NOTIFY_ARGV_LOG="$notify_argv" YTDLP_FAKE_FILE="$fake_file" \
|
||||
OMARCHY_PATH="$fake_root" OMARCHY_YTDLP_DIR="$fake_dir" \
|
||||
bash -c '
|
||||
source "$1"
|
||||
download_url "$2"
|
||||
' bash "$ROOT/bin/omarchy-chromium-ytdlp-host" "https://example.test/watch" >/dev/null 2>&1
|
||||
|
||||
(($(grep -c -- '--no-exec-before-download' "$ytdlp_argv") == 2)) ||
|
||||
fail "yt-dlp native host disarms configured exec hooks on both yt-dlp runs" "$(cat "$ytdlp_argv")"
|
||||
pass "yt-dlp native host disarms configured exec hooks on both yt-dlp runs"
|
||||
|
||||
# The stub answers with a title record whatever it is asked for, so assert the request
|
||||
# as well as the reply: without this the template could be dropped and nothing notice.
|
||||
grep -qF -- $'OMARCHY_TITLE\t%(title)j' "$ytdlp_argv" ||
|
||||
fail "yt-dlp native host asks for the title JSON-encoded" "$(cat "$ytdlp_argv")"
|
||||
pass "yt-dlp native host asks for the title JSON-encoded"
|
||||
|
||||
grep -qF -- "-o %(title)s.%(ext)s" "$ytdlp_argv" ||
|
||||
fail "yt-dlp native host names the saved file after the page title" "$(cat "$ytdlp_argv")"
|
||||
pass "yt-dlp native host names the saved file after the page title"
|
||||
|
||||
grep -q -- '--restrict-filenames' "$ytdlp_argv" &&
|
||||
fail "yt-dlp native host keeps spaces in the saved filename" "$(cat "$ytdlp_argv")"
|
||||
pass "yt-dlp native host keeps spaces in the saved filename"
|
||||
|
||||
grep -qF -- $'OMARCHY_FILE\t%(title)s' "$ytdlp_argv" &&
|
||||
fail "yt-dlp native host never prints the title into the file record" "$(cat "$ytdlp_argv")"
|
||||
pass "yt-dlp native host never prints the title into the file record"
|
||||
|
||||
grep -q -- "--exec mpv -- " "$notify_argv" ||
|
||||
fail "yt-dlp native host builds the click command as mpv -- <path>" "$(cat "$notify_argv")"
|
||||
pass "yt-dlp native host builds the click command as mpv -- <path>"
|
||||
|
||||
grep -qF -- "Download complete My Great Clip" "$notify_argv" ||
|
||||
fail "yt-dlp native host toasts the page title, not the sanitised filename" "$(cat "$notify_argv")"
|
||||
pass "yt-dlp native host toasts the page title, not the sanitised filename"
|
||||
|
||||
# A page that gives no usable title falls back to the filename rather than an empty toast.
|
||||
: >"$notify_argv"
|
||||
: >"$ytdlp_argv"
|
||||
YTDLP_ARGV_LOG="$ytdlp_argv" NOTIFY_ARGV_LOG="$notify_argv" YTDLP_FAKE_FILE="$fake_file" \
|
||||
YTDLP_SKIP_TITLE=1 OMARCHY_PATH="$fake_root" OMARCHY_YTDLP_DIR="$fake_dir" \
|
||||
bash -c '
|
||||
source "$1"
|
||||
download_url "$2"
|
||||
' bash "$ROOT/bin/omarchy-chromium-ytdlp-host" "https://example.test/watch" >/dev/null 2>&1
|
||||
|
||||
grep -qF -- "Download complete Real_Clip [id]" "$notify_argv" ||
|
||||
fail "yt-dlp native host falls back to the filename when no title record arrives" "$(cat "$notify_argv")"
|
||||
pass "yt-dlp native host falls back to the filename when no title record arrives"
|
||||
|
||||
Reference in New Issue
Block a user