Introduction

The --player argument value has always been used for building the entire player command line, consisting of the player path and optional player arguments. --player-args only was added in Livestreamer as an addition to --player in 1ff89b8 for being able to append more player arguments (half a year after the initial PlayerOutput implementation in 331350f).

With that addition, --player should've immediately been turned into a path-argument for only resolving the player executable path and nothing else, so no optional player arguments, but this was never done, because it's a breaking change which requires users to update their config files (especially on Windows).

I was just in the process of refactoring PlayerOutput (as a follow-up of the other recent CLI output code refactorings), which is the reason why I'm bringing this up now and why I'm proposing making this breaking change now.

The issue

The big issue here is how the resulting player "command-line" is built when executing the player process and also how it gets parsed when trying to detect the player executable for the --title CLI argument.

The current PlayerOutput implementation uses two different approaches for building the player command-line:

1. On POSIX systems, it's an argument vector (list of arguments) that gets parsed via shlex.split() and that then gets passed to subprocess.Popen() or subprocess.call().
2. On Windows, it's a simple string, because that's how Windows works (which is terrible). That string later on gets parsed by the win32 APIs and it therefore needs to get formatted correctly, either by the application logic or by the Python stdlib.

While the PlayerOutput's POSIX implementation is mostly fine, the Windows implementation is flawed.

Player detection

First, the issue with the player detection. Detecting the player is necessary for implementing the --title argument (automated player-specific title args).

The goal here is to get the player-path's basename (file name) from the --player argument value, so that it can be compared with a list of known player executable names.

On POSIX, we're simply parsing the argument value via shlex.split() which handles quoted player paths, and then we're using the first token, which is the player path. Done.

On Windows however, this is not possible, because even though shlex.split(posix=False) exists for supporting Windows-style file paths, it won't work in all cases because shlex does only support Unix-style shell syntax, and worst of all, quoted executable paths on Windows are optional (more about that later).

That means that a workaround is required for detecting the player, which is the following one which was added by the initial --title implementation:

>>> os.path.basename('C:\\Program Files\\VideoLAN\\VLC\\vlc.exe')
'vlc.exe'
>>> os.path.basename('"C:\\Program Files\\VideoLAN\\VLC\\vlc.exe"')
'vlc.exe"'
>>> os.path.basename('C:\\Program Files\\VideoLAN\\VLC\\vlc.exe --play-and-exit')
'vlc.exe --play-and-exit'
>>> os.path.basename('"C:\\Program Files\\VideoLAN\\VLC\\vlc.exe" --play-and-exit')
'vlc.exe" --play-and-exit'

Not a particularly nice solution, but fixing that wouldn't justify introducing a breaking change yet.

Player execution

Now to the problematic player execution part.

As explained, on POSIX systems, subprocess.Popen() and subprocess.call() receive a list of arguments. Those are coming from the tokenized --player and --player-args argument values, as well as the built player input value and optional player title, so there's not much interesting going on here.

But on Windows, the current implementation operates on strings, which was most likely done because of the parsing issues of the --player argument mentioned above. None of the user inputs are sanitized and only the generated player arguments get shell-escaped (input path and optional title) via subprocess.list2cmdline(). The rest is simple string concatentation with a " " separator.

subprocess.Popen() and subprocess.call() also support passing an argv on Windows, which is always the preferred way, so the resulting command-line string on Windows can be built internally by the stdlib. That however can't be done right now, because we can't reliably parse and tokenize the --player argument value on Windows, hence the string nonsense.

The big issue is about player paths with spaces and without quotation marks, as those are optional on Windows. When executing such a command, Windows will do the following: first, it tokenizes the command as usual, then it merges the tokens from left to right until a matching executable path gets found on the filesystem. For example, C:\Program Files\sub dir\program name gets resolved and executed in this order:

1. C:\Program.exe
2. C:\Program Files\sub.exe
3. C:\Program Files\sub dir\program.exe
4. C:\Program Files\sub dir\program name.exe

This essentially means that when the --player argument value doesn't include additional quotation marks, for any player in C:\Program Files\ (which is the case on 99% of all Windows systems), Windows will try to find matching rogue executables up the root path first. Streamlink doesn't do any path validation, because it can't parse the input reliably:

>>> shlex.split('"C:\\Program Files\\VideoLAN\\VLC\\vlc.exe"')
['C:\\Program Files\\VideoLAN\\VLC\\vlc.exe']
>>> shlex.split('"C:\\Program Files\\VideoLAN\\VLC\\vlc.exe" --play-and-exit')
['C:\\Program Files\\VideoLAN\\VLC\\vlc.exe', '--play-and-exit']
>>> shlex.split('C:\\Program Files\\VideoLAN\\VLC\\vlc.exe')
['C:Program', 'FilesVideoLANVLCvlc.exe']
>>> shlex.split('C:\\Program Files\\VideoLAN\\VLC\\vlc.exe --play-and-exit')
['C:Program', 'FilesVideoLANVLCvlc.exe', '--play-and-exit']

>>> shlex.split('"C:\\Program Files\\VideoLAN\\VLC\\vlc.exe"', posix=False)
['"C:\\Program Files\\VideoLAN\\VLC\\vlc.exe"']
>>> shlex.split('"C:\\Program Files\\VideoLAN\\VLC\\vlc.exe" --play-and-exit', posix=False)
['"C:\\Program Files\\VideoLAN\\VLC\\vlc.exe"', '--play-and-exit']
>>> shlex.split('C:\\Program Files\\VideoLAN\\VLC\\vlc.exe', posix=False)
['C:\\Program', 'Files\\VideoLAN\\VLC\\vlc.exe']
>>> shlex.split('C:\\Program Files\\VideoLAN\\VLC\\vlc.exe --play-and-exit', posix=False)
['C:\\Program', 'Files\\VideoLAN\\VLC\\vlc.exe', '--play-and-exit']

So player paths with spaces and without quotation marks pose a security issue on Windows, because no validation is being done. Parsing the user input is not reliable. And error messages when a path is invalid are also not great.

On Windows shells like Batch and PowerShell, player paths actually need to be defined as follows in order to be precise and safe:

streamlink -p "\`"C:\Program Files\...\`" optional player args" -a "more optional player args" ...

but the vast majority of users won't do that correctly.

The config file is a bit easier, because shell syntax doesn't need to be escaped:

player="C:\Program Files\..." optional player args
player-args=more optional player args

How to fix

In order to fix this mess, --player needs to be turned into a path-only argument. Then the argument value doesn't have to be parsed, the player path can be reliably validated, and an argv can be built on Windows by parsing the --player-args value instead (which works just fine via the shlex module) and combining that with the player path and generated arguments.

This would fix any security issues and would help cleaning up and simplifying the PlayerOutput implementation (in a follow-up code refactor).

Another benefit is removing any kind of confusion around Streamlink's config file, where unlike on command-line shells, argument values don't require quotation marks for arbitrary argument values with spaces. For the player argument however, it currently does, because it's interpreted as a command-line.
https://github.com/streamlink/windows-builds/blob/5.4.0-1/files/config#L13-L31

Follow-up plans

As mentioned, I was in the process of refactoring the player arguments logic in PlayerOutput. After this issue has been fixed and PlayerOutput been improved, more player-specific stuff could be added, like for example for IINA on macOS, which requires the --stdin argument instead of -, where currently the ugly {playerinput}-stdin workaround is needed to be able to use it.

Feedback

Opinions on introducing this significant breaking change of the --player argument?

This will affect almost every single Windows user who's set a player path via the config file, and it will affect everyone on any OS who's set player arguments via --player instead of --player-args. The docs haven't done a great job over the years to make people use --player-args over --player for any custom player argument.

Migration / Deprecation

I haven't made any plans yet on how to implement any automated migrations, if it's even possible. The Windows installer's NSIS code isn't particularly nice, and touching user configs isn't a great idea either.

Deprecations can't be added, unless we're implementing some kind of cheap detections for certain things like quoted player paths, but that doesn't catch every single case like unquoted player paths with spaces and player args on Windows.

What would change

The changes are pretty easy to explain though

• Before (POSIX shell)

  streamlink -p "\"PLAYERPATH\" PLAYERARGS1" -a "PLAYERARGS2" ...

  Before (Powershell)

  streamlink -p "\`"PLAYERPATH\`" PLAYERARGS1" -a "PLAYERARGS2" ...

  After

  -p "PLAYERPATH" -a "PLAYERARGS1 PLAYERARGS2"
  

• Before (config file):

  player="PLAYERPATH" PLAYERARGS1
  player-args=PLAYERARGS2
  

  After

  player=PLAYERPATH
  player-args=PLAYERARGS1 PLAYERARGS2
  

Current state of PlayerOutput

https://github.com/streamlink/streamlink/blob/78c319508c80a5c4cf72992e7b976d0ff741c81b/src/streamlink_cli/output/player.py