Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closes #388 #398

Merged
merged 1 commit into from
Jun 22, 2020
Merged

Closes #388 #398

merged 1 commit into from
Jun 22, 2020

Conversation

bytestream
Copy link
Contributor

No description provided.

@bytestream
Copy link
Contributor Author

Also closes #378

@barryvdh
Copy link
Collaborator

wouldn't this be breaking, when passing normal objects?

@coveralls
Copy link

coveralls commented Jun 22, 2020

Coverage Status

Coverage increased (+0.8%) to 55.237% when pulling e247747 on bytestream:#388 into b50247f on rcrowe:master.

@bytestream
Copy link
Contributor Author

Should only be breaking if you're using a sandbox policy? But I would argue it's a bug fix and previous functionality was not intended.

@barryvdh
Copy link
Collaborator

But this prevents all array access fields from being accessed in a Sandbox? Or am I misunderstanding it? I understand we should prevent the app variable (eg check Container contract?), but why all ArrayAccess items?

@bytestream
Copy link
Contributor Author

The sandbox policy should prevent access to anything that's not explicitly listed in the policy. I don't see why objects that implement ArrayAccess should bypass the policy with the exception of Contract / Model instances.

Looking at twig, the only side effect of this change (for those who do not use a sandbox) is that the isset() check on ArrayAccess objects is removed:
https://github.com/twigphp/Twig/blob/3.x/src/Extension/CoreExtension.php#L1370

@barryvdh barryvdh merged commit 3b2068a into rcrowe:master Jun 22, 2020
@bytestream bytestream deleted the #388 branch June 25, 2020 11:17
@bytestream
Copy link
Contributor Author

Do you mind tagging? 🤞 and if you want any help with the repo, particularly closing a lot of old issues / PR then I'm happy to help :)

@barryvdh
Copy link
Collaborator

Tagged 0.12
Yeah we should probably get support for 3.x in order, including getting the tests fixed (so #395 and #394)

@barryvdh
Copy link
Collaborator

barryvdh commented Jul 3, 2020

This lead to problems with Symfony forms. Reverting this.

barryvdh added a commit that referenced this pull request Jul 3, 2020
This reverts commit 3b2068a.
@barryvdh barryvdh mentioned this pull request Jul 3, 2020
barryvdh added a commit that referenced this pull request Jul 3, 2020
@bytestream
Copy link
Contributor Author

Looking at twig, the only side effect of this change (for those who do not use a sandbox) is that the isset() check on ArrayAccess objects is removed:
https://github.com/twigphp/Twig/blob/3.x/src/Extension/CoreExtension.php#L1370

Twig's sandbox requires you to add the magic __isset() method to your classes in order to allow the sandbox' security policy to check if the method or property you are calling on your class is allowed.

I think it just needs to call isset(), but only if that method exists...

@bytestream
Copy link
Contributor Author

@barryvdh why do we use return $object->$item instead of return $object[$item]

I don't think there would be a problem if we add isset() and use $object[$item].

The problem seems to be we're checking for ArrayAccess (offsetGet, offsetExists) but as the code uses $object->$item it's actually using __get, __isset

I'll resubmit a PR...

@bytestream bytestream mentioned this pull request Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants