Bug if the artist string contains "$"

[MightyCreak]

Hi!

I've got the album Ten$ion from Die Antwoord, it's in FLAC and it doesn't work although other FLAC songs do work.

It seems that the "$" is the problem (and it seems to be a potential security issue).

Here is the error.log part where I try to play one of these FLAC song:

2016-04-22 04:31:58 Error: [NotFoundException] The requested file was not found
Request URL: /songs/download/474
Stack Trace:
#0 /var/www/sonerezh/app/Controller/SongsController.php(521): CakeResponse->file('/var/www/sonerezh/...', Array)
#1 [internal function]: SongsController->download('474')
#2 /var/www/sonerezh/lib/Cake/Controller/Controller.php(491): ReflectionMethod->invokeArgs(Object(SongsController), Array)
#3 /var/www/sonerezh/lib/Cake/Routing/Dispatcher.php(193): Controller->invokeAction(Object(CakeRequest))
#4 /var/www/sonerezh/lib/Cake/Routing/Dispatcher.php(167): Dispatcher->_invoke(Object(SongsController), Object(CakeRequest))
#5 /var/www/sonerezh/app/webroot/index.php(108): Dispatcher->dispatch(Object(CakeRequest), Object(CakeResponse))
#6 {main}

As you can see it tries to get /var/www/sonerezh/...

[lGuillaume124]

As you can see we like Die Antwoord too, and it works for us:

What is the version of your OS / PHP / Sonerezh please?

[MightyCreak]

You have good tastes breh ;)

I also see them in the Artists and Album views, but it's the FLAC to OGG conversion that doesn't work (the PHP script doesn't seem to find the file from what I see in error.log).

[lGuillaume124]

Just fixed it. In fact, the $ion from "Ten$ion" was interpreted as a variable by the passthru() and avconv command, due to a misuse of single and double quotes.

If you cannot wait for the next release, you can do:

# From master
git pull
git cherry-pick 14b4884005a4ba7b28263d3441a2046021ac16cc

And do not worry about /var/www/sonerezh, in fact Sonerezh tries to convert the track to /var/www/sonerezh/app/tmp/<datetime>.[ogg|mp3]

[MightyCreak]

I cherry-picked the commit, but I still have the bug. I activated the debug log (as I could by commenting the if/else in app/Config/core.php) and here is the output:

2016-04-23 22:29:13 Error: [NotFoundException] The requested file /home/sonerezh/app//home/sonerezh/app/tmp/20160410215429.mp3 was not found or not readable
Request URL: /songs/download/486
Stack Trace:
#0 /home/sonerezh/app/Controller/SongsController.php(523): CakeResponse->file('/home/sonerezh/...', Array)
#1 [internal function]: SongsController->download('486')
#2 /home/sonerezh/lib/Cake/Controller/Controller.php(491): ReflectionMethod->invokeArgs(Object(SongsController), Array)
#3 /home/sonerezh/lib/Cake/Routing/Dispatcher.php(193): Controller->invokeAction(Object(CakeRequest))
#4 /home/sonerezh/lib/Cake/Routing/Dispatcher.php(167): Dispatcher->_invoke(Object(SongsController), Object(CakeRequest))
#5 /home/sonerezh/app/webroot/index.php(108): Dispatcher->dispatch(Object(CakeRequest), Object(CakeResponse))
#6 {main}

As you can see, it's trying to add the app path twice: /home/sonerezh/app//home/sonerezh/app/tmp/20160410215429.mp3

I don't understand why... I tried to get more output in the log, but I couldn't figure out how to add a line in it (I tried trigger_error but it didn't work).

My config is Nginx with a symbolic link from /var/www/sonerezh to /home/sonerezh (needed for disk space).

[lGuillaume124]

Indeed. I realize you have just discovered a huge security hole... I don't know why we did not properly escaped the passed arguments, but it's widely open to arbitrary code execution here :/

6436406 fixes this security issue and properly escapes special characters. It should work now, here is my test:

As you can see @ ' "` are escaped and avconv can do its job.

Do not forget to empty the path column for all the tracks with special chars:

UPDATE songs SET path = NULL WHERE album = 'Ten$ion';

Or you can use the "Clear cache" button from the settings page but it will also remove all the thumbnails...

Thanks a lot for your report!

[MightyCreak]

It works! ;)

I think that considering the security hole this commit fixes, it may be good to merge it in master for v1.1.2, don't you think?