extract() and the path you already checked

The skeleton application's Responder renders a view. It resolves a name to a path, checks the file is there, puts the view data into scope, and includes it. Four steps, and they were in the wrong order.

$viewPath = str_replace('.', '/', $viewName);
$file = views_dir() . "{$viewPath}.php";

if (!file_exists($file)) {
    throw new \RuntimeException("View not found: {$viewName}");
}

ob_start();
extract($data);
include $file;

Read it again with extract() in mind.

extract() takes an array and creates a local variable for every key. ['title' => 'Home'] becomes $title. It is a convenient way to get view data into a template's scope, and it is why so many small frameworks reach for it.

It also creates $file if the array has a file key. After the existence check. Before the include.

Confirming it

I did not want to report this on reading alone, so I reproduced it against the real method:

normal:            LEGIT VIEW
with a 'file' key: ARBITRARY FILE INCLUDED

The check passed on the legitimate view. The include ran on something else entirely.

$viewName, $viewPath and $data itself are clobberable the same way — $file is just the one with teeth.

How bad is it, honestly

This is where I want to be precise rather than dramatic, because it would be easy to write "arbitrary file inclusion in a PHP framework" and collect the replies.

View data comes from a Domain, which is application code. It is not attacker-controlled by default. To turn this into a real vulnerability you need user input reaching a view-data key — a Domain that merges request data into what it returns, say, which is an entirely normal thing to write and exactly the kind of code someone would write without a second thought.

So: not a remote exploit sitting in the framework. A footgun that converts into one the moment an application does something reasonable. That is still worth fixing at the framework level, because the whole point of a skeleton is that people inherit its habits.

The fix

Two changes. The extraction moved inside a closure that holds nothing worth overwriting, and EXTR_SKIP tells extract() to leave existing variables alone:

private function renderInIsolation(string $__file, array $__data): string
{
    $render = static function () use ($__file, $__data): string {
        extract($__data, EXTR_SKIP);
        unset($__data);

        ob_start();
        include $__file;

        return (string) ob_get_clean();
    };

    return $render();
}

The underscore-prefixed names are not decoration. They are the two variables the closure needs to survive extraction, and prefixing them makes a collision essentially impossible on top of EXTR_SKIP already preventing it. The static closure means $this is not in scope either.

Same test after the change:

normal:            LEGIT VIEW
with a 'file' key: LEGIT VIEW

The shape of the bug

This is time-of-check to time-of-use, which usually gets discussed in the context of filesystem races — you stat() a file, an attacker swaps it, you open() the thing they swapped in. Same shape here, but the window is not a race and there is no attacker process. The value simply changed between the check and the use, because a function call in the middle was allowed to rewrite it.

The generalisable version: if you validate a variable and then call something that can write to your scope, you have not validated anything. extract(), variable variables, parse_str() with one argument, import_request_variables() in older code — they all have this property.

The cheapest defence is to stop treating a validated value as a variable at all. Pass it as an argument to something whose scope you control, and check it there.