6
\$\begingroup\$

I wanted a readfile with 304 Not Modified and 206 Partial Content support, I think i got right, but probably spent way more effort on error-checking than worthwhile.. Also I didn't develop it by reading the specs, but instead studying how Nginx handles Range requests:

<?php
function httpreadfile(string $path): void
{
    $mtime = filemtime($path);
    $cacheTimeSeconds = 1 * 60 * 60 * 24;
    if ($mtime !== false) {
        $ifModifiedSince = $_SERVER['HTTP_IF_MODIFIED_SINCE'] ?? null;
        if ($ifModifiedSince !== null) {
            $ifModifiedSince = strtotime($ifModifiedSince);
            if ($ifModifiedSince === false) {
                http_response_code(400);
                echo "Invalid If-Modified-Since header";
                return;
            }
            if ($mtime <= $ifModifiedSince) {
                http_response_code(304);
                return;
            }
        }
        header("Last-Modified: " . gmdate('D, d M Y H:i:s', $mtime) . ' GMT');
    }
    header("Expires: " . gmdate('D, d M Y H:i:s', time() + $cacheTimeSeconds) . ' GMT');
    header("Cache-Control: max-age=" . $cacheTimeSeconds);
    header("Pragma: cache");
    $filesize = filesize($path);
    if ($filesize == false) {
        // unable to get filesize?? having a hard time imagining this actually happening
        header("Accept-Ranges: none");
    } else {
        // check range
        $range = $_SERVER['HTTP_RANGE'] ?? null;
        if ($range === null) {
            header("Accept-Ranges: bytes");
            header("Content-Length: " . $filesize);
        } else {
            if (substr_compare($range, "bytes=", 0, 6, false) !== 0) {
                http_response_code(416);
                echo "Invalid Range header: does not start with 'bytes='";
                return;
            }
            $range = substr($range, strlen("bytes="));
            $range = explode('-', $range);
            if (count($range) < 2) {
                http_response_code(416);
                echo "Invalid range: dash missing";
                return;
            }
            if (count($range) > 2) {
                http_response_code(416);
                echo "Invalid range: more than 1 dash";
                return;
            }
            $start = filter_var(trim($range[0]), FILTER_VALIDATE_INT);
            if ($start === false || $start < 0) {
                http_response_code(416);
                echo "Invalid range: start is not an integer >=0";
                return;
            }
            if ($start >= $filesize) {
                http_response_code(416);
                echo "Invalid range: start is >= filesize";
                return;
            }
            $end = $range[1];
            if ($end === "") {
                $end = $filesize - 1;
            } else {
                $end = filter_var(trim($range[1]), FILTER_VALIDATE_INT);
                if ($end === false) {
                    http_response_code(416);
                    echo "Invalid range: end is not an integer";
                    return;
                }
                if ($end >= $filesize) {
                    //echo "Invalid range: end is larger than filesize";
                    // this request is actually legal, i think. at least nginx accepts it:
                    $end = $filesize - 1;
                } elseif ($end < $start) {
                    http_response_code(416);
                    echo "Invalid range: end is smaller than start";
                    return;
                }
            }
            $offset = $start;
            $length = ($end - $start) + 1;
            header("Content-Length: {$length}");
            header("Content-Range: bytes {$start}-{$end}/{$filesize}");
            $fp = fopen($path, 'rb');
            if ($fp === false) {
                http_response_code(500);
                $errstr = "Failed to open file " . $path . " for reading: " . var_export(error_get_last(), true);
                echo $errstr;
                throw new \RuntimeException($errstr);
            }
            // using this php://output hack because fpassthru is unsuitable: https://github.com/php/php-src/issues/9673
            $output = fopen('php://output', 'wb');
            if ($output === false) {
                http_response_code(500);
                $errstr = "Failed to open php://output for writing: " . var_export(error_get_last(), true);
                echo $errstr;
                throw new \RuntimeException($errstr);
            }
            http_response_code(206);
            stream_copy_to_stream($fp, $output, $length, $offset);
            fclose($fp);
            fclose($output);
            return;
        }
    }
    readfile($path);
}
\$\endgroup\$
0

2 Answers 2

3
\$\begingroup\$

Looks like a great implementation, even though I am also not an avid specs reader. And yes, I think that error processing could be at least improved.

System error handling

First of all I would remove the checking for the system errors. That's, although widely adopted, but really a weird practice. For many reasons:

  • logic-wise, a failed fopen() already results in an error. Why checking anything manually if the error is already there?
  • security-wise, any outside entity has no business with the inner workings of our site. Paths, filenames, error messages - everything we must keep to ourselves.
  • DRY-wise: the handling code for the most part is the same. It would be a good idea to write it only once.
  • logic-wise again: a system error many occur everywhere. All right, we handled the fopen problem. What about, I don't know... the infamous "Headers already sent" error? Or any other error that may happen literally on the every line? It's just impossible to write an error handling code for every possible error on the every line. A typo, An undefined constant, a division by zero - whatever. Definitely, it should be done differently.

Instead, I would offer a generic solution. The reasoning and code examples below are based on two my articles,

First, with a simple wrapper, all remaining PHP errors get converted into Exceptions:

set_error_handler(function ($level, $message, $file = '', $line = 0)
{
    throw new ErrorException($message, 0, $level, $file, $line);
});

Here, we are hitting two birds with one stone: we don't have to throw exceptions manually anymore, they are thrown automatically; we don't have to handle every single error manually either and can write the handling code in a single place. That would be the uniform Exception handler:

function myExceptionHandler ($e)
{
    error_log($e);
    http_response_code(500);
    if (filter_var(ini_get('display_errors'),FILTER_VALIDATE_BOOLEAN)) {
        echo $e;
    } else {
        echo "<h1>500 Internal Server Error</h1>
              An internal server error has been occurred.<br>
              Please try again later.";
    }
    exit;
}

set_exception_handler('myExceptionHandler');

by adding these two code snippets to our bootstrap file, we will achieve the uniform error processing. In case fopen() will fail,

  • the programmer will be able see the problem during development right in the output for the easier debugging

  • on a live site, however, no sensitive detail will make it to the output, but will be logged instead

  • the client will get a generic 500 response saying that it's a server problem without any details, as it should

  • and, the most delighting part - now we can simply throw away all the manual error handling, so the code will become just

      ...
      $fp = fopen($path, 'rb');
      $output = fopen('php://output', 'wb');
      http_response_code(206); 
      ...
    

Not only it's just natural and much easier to read but it's more maintainable as well. Imagine your service will be asked to add some json response. With all the current manual error reporting you will have to laboriously add it in a thousand places. With a uniform error handler it will be just one place.

Uniform HTTP response helper.

That's a trifle improvement but still. Given that a custom HTTP response always takes the same three lines, it would be natural to write a simple helper function to be called in one

return http_resp(400, "Invalid If-Modified-Since header");

Imaginary errors

I agree that handling for imaginary errors, like filesize failed, should be avoided.

Checking the path

Beside removing useless checks, I think hat one should be actually added. I don't know how this function is called, and whether $path gets verified prior, but it could make sense to check whether $path exists and readable.

\$\endgroup\$
2
\$\begingroup\$

Delegation, etc.

    $mtime = filemtime($path);
    $cacheTimeSeconds = 1 * 60 * 60 * 24;
    if ($mtime !== false) {
        $ifModifiedSince = $_SERVER['HTTP_IF_MODIFIED_SINCE'] ?? null;
        if ($ifModifiedSince !== null) {
            $ifModifiedSince = strtotime($ifModifiedSince);
            if ($ifModifiedSince === false) {
                http_response_code(400);
                echo "Invalid If-Modified-Since header";
                return;
            }
            if ($mtime <= $ifModifiedSince) {
                http_response_code(304);
                return;
            }
        }
        header("Last-Modified: " . gmdate('D, d M Y H:i:s', $mtime) . ' GMT');
    }
    header("Expires: " . gmdate('D, d M Y H:i:s', time() + $cacheTimeSeconds) . ' GMT');
    header("Cache-Control: max-age=" . $cacheTimeSeconds);
    header("Pragma: cache");

There is no vertical whitespace in this code. It would be easier to read if you inserted blank lines based on some plan.

There is no need for $cacheTimeSeconds to be defined at the beginning of this block, as you don't use it until the end.

I find 1 * 60 * 60 * 24 hard to read. Is it 1 day times 60 seconds per minute times 60 minutes per hour times 24 hours per day? I guess that gets all the units, but wouldn't it be easier to read as 1 * 24 * 60 * 60 or 24 * 60 * 60? Going from largest to smallest (or smallest to largest) makes it easier to follow.

If the filemtime returns false, you might be better off throwing an exception. As is, you silently ignore it. I.e. your test for falseness might actually hide problems that should be logged.

Since you do no processing of null or missing HTTP_IF_MODIFIED_SINCE values, you could simplify the logic by doing an explicit isset check.

I find return to be an odd exit in the terminal cases. You return to transfer control back to the caller. What's a caller supposed to do? It's a bad request or an unmodified file. Report those back to the client and end processing. I would find exit to be more appropriate here. You've reached a terminal state and should terminate.

It's possible that you do additional handling in the caller. However, since you don't provide any context of how your function is called, we can't see that. And it might still be better to arrange things so that you don't have to return from this code.

If you were exiting from the terminal cases, you could put them into their own function. And you could reuse that function in other situations.

Rather than time() + $cacheTimeSeconds, you could do strtotime('+1 day'), which is much more readable.

Putting all this together, your code could have the following helper functions:

function validate_if_modified_since(int $mtime): void
{
    if (isset($_SERVER['HTTP_IF_MODIFIED_SINCE'])) {
        $ifModifiedSince = strtotime($_SERVER['HTTP_IF_MODIFIED_SINCE']);
        if ($ifModifiedSince === false) {
            http_response_code(400);
            echo 'Invalid If-Modified-Since header';
            exit;
        }

        if ($mtime <= $ifModifiedSince) {
            http_response_code(304);
            exit;
        }
    }
}

function output_last_modified_header_if_valid(string $path): void
{
    $mtime = filemtime($path);
    // perhaps this case should not be checked, but including to show how it could look
    if ($mtime === false) {
        return;
    }
    // note that without this, a false result would log an error in 
    // the validate_if_modified_since call (since false is not an int)

    validate_if_modified_since($mtime);

    header('Last-Modified: ' . gmdate('D, d M Y H:i:s', $mtime) . ' GMT');
}

function output_expires_headers(int $expires): void
{
    header('Expires: ' . gmdate('D, d M Y H:i:s', $expires) . ' GMT');
    header('Cache-Control: max-age=' . $expires - time());
    header('Pragma: cache');
}

And then the actual code is much simpler:

    output_last_modified_header_if_valid($path);
    output_expires_headers(strtotime('+1 day'));

Now your original function doesn't even have to know that filemtime exists. And we've specified the expiration period in a human readable form.

DRY

            if (substr_compare($range, "bytes=", 0, 6, false) !== 0) {
                http_response_code(416);
                echo "Invalid Range header: does not start with 'bytes='";
                return;
            }
            $range = substr($range, strlen("bytes="));

What's 6? Answer: strlen('bytes='). So why not say that?

function extract_range_in_bytes(string $range): string
{
    $prefix = 'bytes=';
    $prefix_length = strlen($prefix);
    if (substr($range, 0, $prefix_length) === $prefix) {
        return substr($range, $prefix_length);
    }

    http_response_code(416);
    echo "Invalid Range header: does not start with 'bytes='";
    exit;
}

Now we can just say

        if (isset($_SERVER['HTTP_RANGE'])) {
            $range = extract_range_in_bytes($_SERVER['HTTP_RANGE']);
        }

It might also make sense to push the range management into its own function. Then you could have code like

    output_last_modified_header_if_valid($path);
    output_expires_headers(strtotime('+1 day'));

    if (isset($_SERVER['HTTP_RANGE'])) {
        http_readfile_ranges($path, $_SERVER['HTTP_RANGE']);
    }

    // output range headers unless size can't be obtained or is zero
    // e.g. in case $path produces a stream rather than a file
    $filesize = filesize($path);
    if ($filesize) {
        header('Accept-Ranges: bytes');
        header("Content-Length: $filesize");
    }

    readfile($path);
    exit;

This assumes that http_readfile_ranges will either do nothing or exit.

I retained the filesize check for two cases. One, if the file is empty, there is no reason to talk about ranges. Two, if $path points to something where the filesize is unknown, e.g. some kind of streaming resource, then we might validly handle a file without a filesize. I.e. I'm not convinced that a missing filesize is an error condition. I did omit the 'Accept-Ranges: none', as that is the default.

Note that this is the entirety of the original function. We've delegated the bulk of the original to three other functions.

Doesn't handle all valid range specifications

You require exactly one range. However, a range specification can include multiple ranges. You don't handle that but could.

$ranges = explode(',', $_SERVER['HTTP_RANGE']);
if (count($ranges) > 1) {
    foreach ($ranges as $range) {

Now you could do your per-range processing with some additional handling for the borders. Or you could do some kind of more complicated validation first.

Normalize before processing

            $end = $range[1];
            if ($end === "") {
                $end = $filesize - 1;
            } else {
                $end = filter_var(trim($range[1]), FILTER_VALIDATE_INT);

It's better if you trim first.

            $end = trim($range[1]);
            if ($end === '') {
                $end = $filesize - 1;
            } else {
                $end = filter_var($end, FILTER_VALIDATE_INT);

Now you won't reject an end that is empty except for whitespace.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.