[RFC] Added `--apply-closure` option

[dantleech]

This option gives us a very quick and flexible solution for migrations:

./bin/phpcr phpcr:nodes:update --query="SELECT * FROM [nt:unstructured] WHERE ..." --apply-closure="function ($session, $node) { if (null !== $node->getProperty('url')) { $node->setProperty('linkType', 'externalUrl'); }"

What do people think?

[lsmith77]

I am not sure. seems like this opens up a lot of power to the CLI. usually anyone with CLI access can add code but still I don't feel comfortable with this.

[dbu]

What about making it optional? The command code would have to configure itself i guess, as commands are not Services...

----- Reply message ----- Von: "Lukas Kahwe Smith" notifications@github.com An: "phpcr/phpcr-utils" phpcr-utils@noreply.github.com Betreff: [phpcr-utils] [RFC] Added --apply-closure option (#71) Datum: So., Jun. 16, 2013 19:26 I am not sure. seems like this opens up a lot of power to the CLI. usually anyone with CLI access can add code but still I don't feel comfortable with this.

— Reply to this email directly or view it on GitHub.

[lsmith77]

if it needs to be enabled by code i am not sure i see the sense in it.

[dbu]

what i meant: we can have it as a config option and DI can put a parameter into the container, but the command will need to do this->getContainer->hasParameter('cmf_menu.command.update_closure') or something like that. from a user point of view there is no difference, i was just thinking about the implementation already.

[lsmith77]

not even sure that i like it as a config option. as soon as you allow injecting code via the CLI, there are almost no more limits to what can be done with a CLI command.

[dbu]

if we default the option to false, people can chose if they want this feature or not.

the alternative would be some sort of separate git repo for this command, then there is no way people could steal rights. makes it less useful however. the thing is that it will allow to do any kind of crazy migration things by copy-paste, without needing to deploy temporary migration code.

[pgodel]

Although this is very practical, I agree with @lsmith77 on the concerns about this. Just look at all the apps that offer some sort of "eval" and see the consequences.

[dantleech]

Yeah, maybe another repo or alternatively sandbox it by embedding a scripting language, don't know how feasable that would be.

Pablo Godel notifications@github.com a écrit :

Although this is very practical, I agree with @lsmith77 on the concerns about this. Just look at all the apps that offer some sort of "eval" and see the consequences.

---

Reply to this email directly or view it on GitHub: https://github.com/phpcr/phpcr-utils/pull/71#issuecomment-19529317

Envoyé de mon téléphone Android avec K-9 Mail. Excusez la brièveté.

[cryptocompress]

+1 ... i like this one

• eval is evil and should be "create_function"
• as of a hacker perspective the difference is execute- vs read- permission?

[dbu]

can we use the "migrator" feature for code-requiring migrations or is there still something missing? if needed, we could refactor this command in a way that its easily extendable and if somebody wants, he can then do that extra parameter to inject a closure. and if he wants it safe, he can extend the command with custom update commands that have the logic in their code and thus can be tested.

[dantleech]

Well, would using create_function solve our security problems?

Alternatively we could use the V8Js extension:

• http://php.net/manual/en/book.v8js.php

And pass in an array to the javascript function representing the node. Then remap the array back to the node in PHP. We could also sandbox the session object in a JS object.

We could also use this (closure) method as an additional way of filtering queries.

[lsmith77]

create_function would at least mitigate the scope a bit .. i am still not convinced though. i mean you can just as well write the code to the file system and remove it again as part of your deploy script.

[cryptocompress]

I cannot see any security problems in here.

1. If someone can execute a destructive operation like bin/phpcr phpcr:nodes:update there is always a way to read/kill whole accessible DB. e.g.: read config and do it manually
2. End-user can't access this and there is no need to protect devs/admins from their intension.
3. This command may not execute arbitrary code. So create_function is the right tool for the job. Injected code still as (un-)trustworthy as current user.
4. This command is only accessible with/through phpcr:nodes:update.
5. Internal access restrictions can be implemented (but may be useless as in 1.?).

This is a very powerful and flexible tool and migrations are only one use case.

[cryptocompress]

• err, what is the point in limiting this to phpcr:nodes:update only?
• and would it make sense if closure return a bool/node/null?

this is like awk for CMF :100:

[dantleech]

@lsmith77 what are your concerns with create_function ?

If create_function is not good enough then what do people think about writing the closures in Javascript as I mentioned earlier?

[dbu]

i would prefer not to bring in js, thats too complicated. if lukas really can't live with an apply-closure that we have a config option that the command reads from the container to know if its even enabled, then lets provide the command for that as a separate repo rather than doing js.

[dantleech]

But if its in another repo, the use cases for this Command is to help with migrations, so people will install this command. From that point onwards it will be there. The same for explictly enabling it - people will enable it and, well, its probably going to stay enabled.

But I guess at least having to manually enable it makes people more aware.

The only security problem I can think of is that some people like to expose their commands to the web (Sensio Desktop??)

What is a worst-case scenario?

On Wed, Jun 26, 2013 at 11:12:40PM -0700, David Buchmann wrote:

i would prefer not to bring in js, thats too complicated. if lukas really can't live with an apply-closure that we have a config option that the command reads from the container to know if its even enabled, then lets provide the command for that as a separate repo rather than doing js.

— Reply to this email directly or [1]view it on GitHub.

References

Visible links

1. https://github.com/phpcr/phpcr-utils/pull/71#issuecomment-20099152

[lsmith77]

create_function I guess is a compromise I can live with

[dantleech]

Rebased and updated. It now uses create_function which also makes the syntax better:

     php bin/phpcr phpcr:nodes:update \
         --query="SELECT * FROM [nt:unstructured] WHERE [phpcr:class]=\"Some\Class\Here\"" \
         --apply-closure="\$session->doSomething(); \$node->setProperty('foo', 'bar');"

Are people now happy to merge this?

[lsmith77]

+1

[cryptocompress]

:+1:

output should be in <comment>-tags (ob_*), if any?

[dbu]

thanks a lot dan. is there a good place to document this? but then again, commands are kind of self-documenting