Closes #388 [2]

[bytestream]

Remake of https://github.com/rcrowe/TwigBridge/pull/398 which was reverted due to https://github.com/rcrowe/TwigBridge/pull/398#issuecomment-653434596

Before we were use checking for calls against Eloquent models. #388 is lack of sandbox checks on Container objects.

The problem with the old PR was that it was checking ArrayAccess but using $object->$item - see https://github.com/rcrowe/TwigBridge/pull/398#issuecomment-653512286

This PR resolves that issue by using $object[$item] and also adds the isset() check which happens in the upstream code - https://github.com/twigphp/Twig/blob/3.x/src/Extension/CoreExtension.php#L1370

[bytestream]

This should resolve your issue with the old PR @barryvdh ?

[coveralls]

Coverage increased (+0.8%) to 55.237% when pulling 1bf1d896313242e95636a5edff4573042403a83a on bytestream:#388-2 into c7e4acdfb3d55a055508b9d3cc8cf71c00329e90 on rcrowe:master.

[bytestream]

@barryvdh this version does the trick. You can compare https://github.com/rcrowe/TwigBridge/pull/401/files vs https://github.com/rcrowe/TwigBridge/pull/398/files

The key part is the isset check that I mentioned was probably missing from the old PR. That's what caused your issue.

[nVuln]

I think this shouldn't enable by default, this extension just implement sandbox checking for ArrayAccess (also I think you should rename this extension) If twig not support this feature, we shouldn't change it

For issue #388 we need remake app variable like this: https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/AppVariable.php

[barryvdh]

So in sandbox, just add that object instead?

[barryvdh]

Just to be clear, does this PR impact non-sandbox behavior?

[bytestream]

Can you justify why we shouldn't support it? Technically twig doesn't sorry Eloquent model checks, hence the override... it doesn't seem like a far stretch to extend this to all arrayaccess usage hidden how wide spread it is in Laravel

I would be ok for your to be being a feature flag in the config if that pleases people though...

[nVuln]

So in sandbox, just add that object instead?

we apply it global, not sandbox only replace app variable with custom object, not using laravel application object same as Symfony, app variable only wrap request, session, user (or any SAFE object, not ALL) @barryvdh what do you think ?

Can you justify why we shouldn't support it? Technically twig doesn't sorry Eloquent model checks, hence the override... it doesn't seem like a far stretch to extend this to all arrayaccess usage hidden how wide spread it is in Laravel

I would be ok for your to be being a feature flag in the config if that pleases people though...

in #362 he said call {{model.save}} will execute $model->save(), that is bug in laravel 5.2 only, they fixed it, so we don't care that problem right now ArrayAccess in laravel eloquent only access to attributes, getter and relationship (eloquent again), I don't see any risk with that my opinion, this package is bridge, not customize twig for laravel, if we make any breaking change, it should be optional

[bytestream]

Can you provide a link to the PR that fixes that in Eloquent?

If it is fixed then I don't mind the approach you're suggesting. It removes a lot of unnecessary complexity from this package. The only problem... are there people on v0.12 of this package that are using a Laravel version where it's not fixed upstream?

[bytestream]

It looks like current branch supports Laravel 5.5 or above. So is this fixed in Laravel 5.5 and above? Link to PR would be good.

[nVuln]

this commit: https://github.com/laravel/framework/commit/7ce97bec546d607ca207082722ceecabfbcc11a9#diff-d8e24a5815a26f7211e66db787be647f

[bytestream]

Cool ta. Looks like we can get rid of the node visitors and overriding twig_get_attribute functionality.

Do you want to have a go at what you're suggesting @ndhan93 ?

[nVuln]

Cool ta. Looks like we can get rid of the node visitors and overriding twig_get_attribute functionality.

Do you want to have a go at what you're suggesting @ndhan93 ?

your extension so cool, I think many people will need it you should rename your extension to make it sense (eg: ArrayAccessSandboxCheck), and comment it in config file I'll make PR to fix issue #388 right now

[bytestream]

@barryvdh would you mind tagging this?

[jshah4517]

@barryvdh would it be possible to have a new release containing this please?

[ovp87]

Any resolution to this? all solutions appear to have been reverted.