A PHP script used on my home server to download YouTube videos

Question

I have a PHP page I used to download YouTube videos, hosted on my Pi 4.

The site is only going to be used on my home network (so a dot local address), but it allows me to download videos on my phone or computer without downloading any executables or special apps.

The user can submit a request on an HTML page with four options:

Form ID      : Description to user
----------------------------------------
bestquality  : MP4 (video with sound)
worstquality : MP4 (video without sound)
bestaudio    : M4A
worstaudio   : WEBM

This is a majority of the code used to make this:

<?php

//the file extension
$formatExt = "";

switch($_GET['format']) {
    case 'bestvideo': case 'worstvideo': $formatExt = "MP4"; break;
    case 'bestaudio': $formatExt = "m4a"; break;
    case 'worstaudio': $formatExt = "webm";
        break;
    default:
        echo("Illegal video format '".$_GET['format']."'!");
        exit(1);
}

$url = $_GET['url'];
$format = $_GET['format'];
$ytcmd = 'youtube-dl';

$directoriesInUse = array_filter(glob("./workers/*"), 'is_dir');

$integerIDs = array(sizeof($directoriesInUse));


//if the working directory changes, make sure to change the 2 at the end of the
//expression in the for loop to match the amount of '/' characters.
for($i = 0; $i < sizeof($directoriesInUse); $i++)
    $integerIDs[$i] = (int)explode("/", $directoriesInUse[$i])[2];

//If these are not sorted, it will see a list of 1 thru 14 and pick 9 as the largest number (because that's how the directory would be listed)
sort($integerIDs);

$numberToUse = $integerIDs[sizeof($integerIDs) - 1] + 1;

//prevent exploiting and issuing server commands
$url = str_replace(" ", "", $url);
$url = str_replace("|", "", $url);

//url may contain & for a playlist, so it will be removed here to prevent other issues.
$url = substr($url, 0, strpos($url, "&"));

//Download the video, the "--format" option will not download audio for a video (worstvideo is video w/o audio)
if($format === "bestaudio" or $format === "worstaudio" or $format === "worstvideo")
        $response = exec('mkdir ./workers/'.$numberToUse.' && cd ./workers/'.$numberToUse.'/ && '.$ytcmd.' --no-playlist --format '.$format.' -o \'%(title)s.%(ext)s\' '.$url);
else
        $response = exec('mkdir ./workers/'.$numberToUse.' && cd ./workers/'.$numberToUse.'/ && '.$ytcmd.' --no-playlist -o \'%(title)s.%(ext)s\' '.$url);

//Get the first file in a directory, this SHOULD be the video that was downloaded.  It is not possible (AFAIK) to get the file name of a video from the youtube-dl binary
function magicWorkers($path) {
    return array_diff(scandir($path), array('.', '..'))[2];
}

//change file date so the crontab doesn't delete the file too early (youtube-dl may save the file with the date it was uploaded)
echo exec("touch ./workers/".$numberToUse."/".magicWorkers("./workers/".$numberToUse));

//sleep 2 seconds for good measure (in case things haven't updated for whatever reason, this has happened!)
sleep(2);

//This works, but I would like to find an alternative soon (prompt download vs direct)
header("Location: ./workers/".$numberToUse.'/'.magicWorkers("./workers/".$numberToUse));

exit(0);

The server is being hosted on an Apache2 web server in the /var/www/html/ directory, and Apache2 has rw- access in the /var/www/html/workers/ directory. It is running running PHP 7.2.24.

I have seen some security problems with this before, for instance, inputting example.com/video && cowsay MOO will execute the cowsay command, so I made a small patch by disabling the characters " " (space), "&", and "|".

My other concern is that I am linking the user directly to the file to download, whereas it would be better (and probably more secure) if they were prompted for the download.

One last thing: I have a crontab that runs every minute to delete files in the workers directory (older than two hours) to make sure that the space on the Pi doesn't fill up (directories do not get deleted).

Answer 1

Initial thoughts

I must admit I hadn't heard of the youtube-dl program but it seems quite nifty. Perhaps it was what many of the websites I have seen for downloading YT videos use behind the scenes.

I tried using the script locally and found that the URL must contain an ampersand (i.e. have 2+ query string variables), otherwise the logic to look for a playlist parameter and remove it would lead to an empty string passed to the youtube-dl program. But some URLs I see from Youtube only have zero or one parameters (e.g. https://www.youtube.com/watch?v=DuB6UjEsY_Y and the equivalent sharing link: https://youtu.be/DuB6UjEsY_Y).

Initially I was wondering why $numberToUse couldn't be set to count($directoriesInUse) + 1 but then I realized that some of those directories could be deleted and that might lead to issues. Then I considered that if you could parse out a video id (which seems to have remained the same since 2014) from the URL then that could be used instead of an integer for the sub-directory name.

Suggestions

Use a mapping instead of a switch

The switch could be simplified:

switch($_GET['format']) {
    case 'bestvideo': case 'worstvideo': $formatExt = "MP4"; break;
    case 'bestaudio': $formatExt = "m4a"; break;
    case 'worstaudio': $formatExt = "webm";
        break;
    default:
        echo("Illegal video format '".$_GET['format']."'!");
        exit(1);
}

A mapping could be defined of formats to extensions:

const FORMAT_MAPPING = [
    'bestvideo' => "MP4",
    'worstvideo' => "MP4",
    'bestaudio' => "m4a",
    'worstaudio' => "webm"
];

Then if the query string is missing that key or that key does not exist in the mapping, show the error and exit early.

if (!isset($_GET['format']) || !array_key_exists($_GET['format'], FORMAT_MAPPING)) {
    echo("Illegal video format '".$_GET['format']."'!");
    exit(1);
}

Otherwise just assign the value to $formatExt:

$formatExt = FORMAT_MAPPING[$_GET['format']];

Control structures not using statement groups

While it may seem unlikely that there would be a need for multiple lines following if, else or for statements, it is best to put the statements to execute in a group (i.e. with curly braces/brackets). Then when you determine that you need to add a line to one of those statement groups, it eliminates any chance of logic error.

if($format === "bestaudio" or $format === "worstaudio" or $format === "worstvideo") {
        $response = exec('mkdir ./workers/'.$numberToUse.' && cd ./workers/'.$numberToUse.'/ && '.$ytcmd.' --no-playlist --format '.$format.' -o \'%(title)s.%(ext)s\' '.$url);
}
else {
        $response = exec('mkdir ./workers/'.$numberToUse.' && cd ./workers/'.$numberToUse.'/ && '.$ytcmd.' --no-playlist -o \'%(title)s.%(ext)s\' '.$url);
}

or has lower precedence than || and others like assignment

Note the operator precedence list and how or is at the end of the list, while logical or i.e. ||, is above assignment and others:

There is a good illustration of the difference in the user contributed notes by fabmlk:

Watch out for the difference of priority between 'and vs &&' or '|| vs or':

<?php
$bool = true && false;
var_dump($bool); // false, that's expected

$bool = true and false;
var_dump($bool); // true, ouch!
?>

Because 'and/or' have lower priority than '=' but '||/&&' have higher.

So it is wise to get in the practice of using || unless you know you need or

use foreach instead of for

Instead of this for loop:

for($i = 0; $i < sizeof($directoriesInUse); $i++)
    $integerIDs[$i] = (int)explode("/", $directoriesInUse[$i])[2];

A foreach could be used

foreach ($directoriesInUse as $i => $directory) {
    $integerIDs[$i] = (int)explode("/", $directory)[2];
}

One could also use array_map(). Instead of storing an array of integers, you could just keep track of the maximum number seen - using max(). That would avoid the need to sort the array. This likely wouldn’t be a major optimization unless there were a large number of directories.

Logical flaw when no ampersand exists in URL

As was mentioned above, this line doesn't work well when there is no ampersand in $url

//url may contain & for a playlist, so it will be removed here to prevent other issues.
$url = substr($url, 0, strpos($url, "&"));

This is because strpos() "Returns FALSE if the needle was not found."¹. I would update the code to only perform that alteration if the string actually contains an ampersand.

¹_{http://php.net/strpos}

Answer 2

Security

Command injection

I made a small patch by disabling the characters " " (space), "&", and "|".

This can easily be bypassed. Example:

url=http://example.com/;id;?%26x=y;

Other relevant characters include - but are not limited to - $, ```, etc.

As mentioned in the comments, you should use escapeshellarg instead (input filtering is quite difficult).

XSS

This is vulnerable to XSS:

echo("Illegal video format '".$_GET['format']."'!");

An attacker can exploit this to run arbitrary JavaScript on the domain the script is hosted on. If there are other applications hosted there, this might have a significant impact. XSS also allows bypass of CSRF protection (see next point).

You should apply htmlentities to any user-supplied value you output.

CSRF / Missing authentication

As-is, anyone could get you to download a video by including an img tag to the PHP script in a website, an email, etc.

This also allows remote attackers who are not in your home network to execute arbitrary code on your computer via the command injection issue.

I would suggest to add some form of authentication here, and if it's only a hardcoded key you pass in the URL.