Concatenating CSV filenames into a single file

Question

I've created a PHP script which take an array of filenames as a comma-separated parameter and concatenates them, serving them as one, single JavaScript file.

I've also built a caching mechanism into it - each time, it checks if any of the individual JS files have been updated and, if one or more of them has, it updates the cache and serves the new output. Otherwise, it simply loads the relevant cached file.

<?php
    $scripts_to_concatenate = $_GET['scripts'];
    $scripts_array = explode(',', $scripts_to_concatenate);
    $cached_script = '/public_html/assets/scripts/cache/' . str_replace('.js', '', str_replace(',', '_', $scripts_to_concatenate)) . '.js.cache';
    $cache_expired = false;
    if (file_exists($cached_script)) {
        foreach ($scripts_array as $script) {
                if (filemtime($script) > filemtime($cached_script)) {
                    $cache_expired = true;
                }
        }
    } else {
        $cache_expired = true;
    }
    if ($cache_expired) {
        foreach ($scripts_array as $script) {
            $script = '/public_html/assets/scripts/' . $script;
            if (file_exists($script)) {
                $output = $output . @file_get_contents($script);
            }
        }
        file_put_contents($cached_script, $output);
    } else {
        $output = @file_get_contents($cached_script);
    }
    echo $output;

Is anyone able to suggest any ways for me to improve this code, to make it more efficient or more fool-proof?

UPDATE: I've incorporated janos's recommendations into my code (original, unoptimised code above). Does anyone have any other suggestions?

Answer 1

As soon as you find an outdated file, you can break out from the first loop:

    foreach ($scripts_array as $script) {
            if (filemtime($script) > filemtime($cached_script)) {
                $cache_expired = true;
                break;
            }
    }

And instead of accumulating the script content in a variable, you can write to a file and echo directly:

if ($cache_expired) {
    unlink($cached_script);
    foreach ($scripts_array as $script) {
        $script = '/public_html/assets/scripts/' . $script;
        if (file_exists($script)) {
            $content = @file_get_contents($script);
            file_put_contents($cached_script, $content, FILE_APPEND);
            echo $content;
        }
    }
} else {
    echo @file_get_contents($cached_script);
}

Answer 2

There is a security vulnerability. An attacker could set scripts to

../../index.php

And the script will send the raw PHP to the browser. From there it would be easy to find your config file with passwords in.

You should check that the file name is alphanumeric and full stops only.

if (ctype_alnum(str_replace('.', '', $script)) {

    $script = '/public_html/assets/scripts/' . $script);

    if (file_exists($script)) {
        $output = $output . @file_get_contents($script);
    }

}

Although to be honest I don't think you should be doing this at all anyway. Shouldn't concatenating scripts be part of your build process?