play plugin: $playlist marker for precise control where the playlist …

[cvx35isl]

…file is placed in the command

Description

see included doc; placing the playlist filename at the end of command just isn't working for all players

I have this in use with mpv

To Do

• Documentation. (done, let me know if more is needed)
• Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
• Tests. (let me know if needed)

[sampsyo]

Looks good overall! But it would be nice to use proper debug logging so the messages are suppressed by default. Also, could you please add a quick changelog entry?

[sampsyo]

Cool; thanks for moving this along! It looks like there are a couple of suggestions from the style checker tool.

Also, it occurs to me after thinking about the code for a moment that there could be problems with shell escaping. Do you think it would make sense to pass each of the open_args values through shlex.quote? That would prevent issues when an argument has a space in it, for example.

[cvx35isl]

Do you think it would make sense to pass each of the open_args values through shlex.quote?

hmm, from skipping over I see open_args should either contain one absolute path to the playlist file stored in tempfile.tempdir, usually /tmp/. Looks save enough tome.

With config.raw=yes many paths to music files will be passed. With that shell escaping may be be a problem if a "bad" path was put into config.directory or when config.paths.XXX parameters create a "bad" library structure - which I guess would be unusable for many more things than just the play plugin.

Then in play() (lines 51 and 52), the command_str is put through shlex.split() for passing to subprocess.call() - no quoting done at all. I humbly guess this leads to a real exec() of an array produced by shlex.split().

IF that is so, I see no security problem as these strings never get handed to a shell.

It can cause 'file not found' issues when playing ...

For that extra robustness I would change play() to run a combination of shlex.split() and shlex.quote() on command_str as well on whatever open_args has.

Is that what you are after?

... i guess this would create the same problems it the user handles quoting himself in the config. Would be a don't-ever-think-about-quoting config then.

[cvx35isl]

thinking some more I'll probably have to unquote again right before subprocess.call(), since if its not a shell it won't do that. It would try to execute and play files that have quoting characters in their names ... and fail.

So quoting may really only be able to improve the splitting in case config.play.raw=true is used.

(tell me if I get this totally wrong - I dont do much with python, really)

[sampsyo]

I think the main thing is that, when we eventually called shlex.split on the newly-interpolated command, we will want the filename to be preserved as a single shell token. It's true that open_args will typically not contain any weird characters because it's a generated /tmp/... name when raw=False, but I guess it seems like a good practice to make sure that no filename can ever trip up the command-line construction?

[cvx35isl]

hello, sorry for the long delay, i was otherwise busy

this branch / PR is now rebased upon the current master and should just merge

I did more digging in how and where to make use of shlex.quote() and don't see it. Cause the paths list, especially when raw=True is configured, is composed by calling Item.path which returns a 'byte' object. shlex.quote() does not accept byte input, so I would need to stringify first... but then I see util.interactive_open() is aware of this and somehow handles these bytes as Unicode or passes them on to whatever handles Unicode in the OS. I don't understand that part really, but all goes into os.execlp() where each path is handed over as one argN. No shell in sight.

If raw=False is configured the bytes get separated by newline and written straight into a .m3u file where its then up to the actual player program to properly read it. In most cases this won't be a shell either unless the user specifically does so.

To my understanding there is no chance of unintentionally running into a escaping problem.

[sampsyo]

Consider the case where the config is:

play:
    command: mpv $playlist

So the _command_str function returns the string "mpv $playlist". Eventually, we will call shlex.split on this string (after interpolating it) and then hand it off to os.execlp. So, if the playlist path is /foo/bar, then we will effectively call:

os.execlp(['mpv', 'mpv', '/foo/bar'])

However, if the playlist path is /foo/bar baz/qux, then we will construct the string "mpv /foo/bar baz/qux" and then split it, so we will effectively call:

os.execlp(['mpv', 'mpv', '/foo/bar', 'baz/qux'])

…and the command will see the wrong number of arguments. Does this make sense?

[cvx35isl]

yes, that could happen if a space or something happens to be in that tmp dir path -> I added a shlex.quote() where that path gets created, before it gets into command_str.

I don't see how to properly quote the beet play <query> --args ... list if given, since it would need shlex.split() first and then quoting these parts would pursue the problem ... that must be done right by the user.

regarding the failed tests: think that wasn't me, some package server was unreachable

[sampsyo]

Thanks! Since it seems like somewhere where things could easily go wrong, maybe it would be instructive to have a few tests to show that this is working correctly in any configuration setup we can think of?

[cvx35isl]

how to implement these, in general?

i think the main imponderables are play.command: and the library itself, esp. the paths of where it is on the system and how the user structured it using the paths: section.

to test play.command: a mock-player could be written that checks how it gets called and/or does wrong things when being called. Or do you think of calling a real music player(s) and verify their outputs / return codes?

As for the library: I dont know. Does some sort of mock-library exist in the tests already? Something with weird characters that are known to cause trouble?

[stale]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[cvx35isl]

I will bring it up to master over the next days.

Regarding the security: i still have no idea how to effectively improve it and dont see a problem either. It works flawless for me since published.