Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`playlist index ?` returns any value sent to it, doesn't validate input #207

Open
decibyte opened this issue Jun 19, 2018 · 3 comments

Comments

@decibyte
Copy link

commented Jun 19, 2018

I just ran into a a case twice (although I don't know how to reliably reproduce), where issuing the CLI command <player id> playlist index ? return <player id> playlist index b1.

Now, the docs for this command say:

The value of the current song index may be obtained by passing in "?" as a parameter.

And:

Examples:

(...)

Request: "04:20:00:12:23:45 playlist index ?<LF>"
Response: "04:20:00:12:23:45 playlist index 5<LF>"

How am I supposed to interpret the value b1? Does the leading b indicate something that's not mentioned in the docs? I'm not even sure I was at the playlist position 1 at this point, so I'm reluctant to just strip away any leading bs.

There's an unlikely chance that this is an actual bug in the software, but my guess is that this is an undocumented feature and the bug only lies in the documentation.

@mherger

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2018

Can you reproduce this easily? Because I can't.

If you look at that call, then really this is a very basic handler:
https://github.com/Logitech/slimserver/blob/public/7.9/Slim/Control/Queries.pm#L2459
https://github.com/Logitech/slimserver/blob/public/7.9/Slim/Player/Source.pm#L229

There's no additional "feature" which would add anything. And I certainly can't reproduce this.

What are you running your LMS on? How did you send that query?

@decibyte

This comment has been minimized.

Copy link
Author

commented Jun 20, 2018

Can you reproduce this easily? Because I can't.

Yes, actually I can! A good night's sleep always helps :)

The bug is actually something completely different. If you want me to open another issue or change the content of this one, let me know. Here we go:

I was in fact messing around with some shell scripts I use for basic player control (play/pause/perv/next) that use the HTTP based status.html interface, but checking the results via my CLI based Cursebox utility, which is why I was led to believe this is a CLI issue.

Skipping tracks are done by calling status.html?player=<player id>&p0=playlist&p1=jump&p2=<position>, where <position> can be a number (e.g. 2 to skip to track number 3 on the current playlist) or -1 and +1 to go to previous and next track, respectively.

The plus sign (+) in URLs is often interpreted as a space ( ), so I wanted to urlencode this. And somehow I managed to fail in this, sending a b1, which are the last two characters of the urlencoded +1 (%2b1).

Now, I did some additional testing, and it seems that anything is accepted as a position value. But if it's not an integer, -1 or +1, it just skips to the first track on the playlist. But what's even worse: The provided value is then reported back by the CLI.

If I send a value of horse via the HTTP interface, this is the value I get back via the CLI.

It seems like some input validation would be useful. I haven't tested whether the same problem is there when changing the playlist position via the CLI, but I assume that it's easy for you to tell whether the checks can be implemented in a central place or also need to be done in the CLI.

:)

@mherger

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2018

You send a horse and you get a horse back - sounds like a fair deal to me.

But yes, I think this would deserve a more suitable issue title.

@mherger mherger changed the title CLI `playlist index ?` returns non-numeric value `playlist index ?` returns any value sent to it, doesn't validate input Jun 20, 2018
@mherger mherger self-assigned this Jun 20, 2018
@mherger mherger added the bug label Jun 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.