rcrowe / TwigBridge

Give the power of Twig to Laravel
MIT License
894 stars 168 forks source link

Sandboxed Properties #265

Closed bytestream closed 8 years ago

bytestream commented 8 years ago

Accessing properties on an object of type Illuminate\Database\Eloquent\Model bypasses sandbox checks. See: https://github.com/rcrowe/TwigBridge/blob/master/src/Twig/Template.php#L116

As far as I can tell we just need to add the below to that function (line 121):

if ($this->env->hasExtension('sandbox')) {
    $this->env->getExtension('sandbox')->checkPropertyAllowed($object, $item);
}
bytestream commented 8 years ago

If you agree I can submit a PR.

barryvdh commented 8 years ago

Okay

barryvdh commented 8 years ago

I'd rather strip that method override out completely, but not sure if that's possible.

bytestream commented 8 years ago

Sure. I'll do that if it's possible, otherwise I'll just submit the fix mentioned above.

bytestream commented 8 years ago

@barryvdh it seems like it might be possible to scrap the override. It falls to #L462 because Laravel models implement ArrayAccess:

public function offsetGet($offset)
{
    return $this->$offset;
}

That being said... #L462 doesn't support sandboxing array items. See @fabpot and PR#1863

Thoughts? I think given the Laravel docs (see below) explicitly say to access Model column values via property and not array we keep the override and add the sandbox code #L526.

foreach ($flights as $flight) {
    echo $flight->name;
}

Accessing Column Values

barryvdh commented 8 years ago

I think the override was done because even though ArrayAccess is used, this doesn't work for all cases (isset with null values, relations which are not yet loaded etc). If we do that, we need to make a proper test case.

bytestream commented 8 years ago

Sorry, test case for what? Ensuring the sandbox works for model properties?