3.0 An error occurred while saving the plugin - Static plugin

pbowyer commented 6 years ago

Summary

When I save a static plugin, I get the error:

Step to reproduce

Create a new plugin
Name it anything
Check the "Is Static" box
Browse for a Static file
Click the 'Save' button

Observed behavior

The above error appears. If you refresh the page, the plugin has been created in the database with the correct static file path.

If you edit the plugin code and click 'Save' you get the same error, and the updated code isn't written to the file.

The MODX log contains lines like:

[2018-05-10 20:33:27] (ERROR @ /var/www/web/core/model/modx/modplugin.class.php : 58) An error occurred while creating the plugin.Array
[2018-05-10 20:38:40] (ERROR @ /var/www/web/core/model/modx/modplugin.class.php : 58) An error occurred while saving the plugin.Array

Expected behavior

No error. And changes to the plugin code should be written back to the file

Environment

MODX @ 2f762359b163674cd6e845a52d6de72111547cb3, Apache 2.4, PHP-FPM 7.2.3, Vagrant.

My static file has permissions

$ ls -lah core/components/
total 13K
drwxrwxrwx 1 root root 4.0K May 10 21:24 ./
drwxrwxrwx 1 root root 4.0K Apr  3 18:02 ../
drwxrwxrwx 1 root root 4.0K Apr  3 18:09 getresources/
-rwxrwxrwx 1 root root  184 May 10 21:24 staticplugin.php*

Any user within the Virtual Machine can write to the file.

pbowyer commented 6 years ago

The problem is with \modMediaSource::checkFileType() which isn't permitting a .php file to be saved.

bezumkin commented 6 years ago

Yes, previously static elements worked with their own logic for files - and you was able to create any file on disk. I think it was some kind of vulnerability.

Now it works with media sources and obey their rules. If you can't create php files in media manager - you can't create them with static elements too.

But, you don't need to store static files as php, it can be any allowed extension.

pbowyer commented 6 years ago

I understand why you've aligned static elements with the new media sources, and it works great for chunks.

Blocking the .php extension makes perfect sense for the asset manager, media library - everywhere else. It causes problems for snippet and plugin static elements for a couple of reasons:

Backwards compatibility for sites upgrading to MODX 3.0.
Editor recognition

But, you don't need to store static files as php, it can be any allowed extension.

It can, but I'd rather we didn't have to educate people how to set up their code editor of choice to treat a chosen file extension (.phpx for example) as PHP.

Can the set of allowed file extensions be changed per role? So Administrators could upload .php files if needed?

ghost commented 6 years ago

It's a major version number because there are breaking changes.
I think you can use group settings to override system settings, so you should be able to allow upload of .php files for the users. Not sure if a media source just for the static files can override systems settings to allow .php upload.

pbowyer commented 6 years ago

Re 1): Breaking changes are good if they've been discussed and agreed by the community, as a new major version is a great opportunity to clean out the old and set direction for the next few years.

However if breaking backwards compatibility comes as a side-effect of other changes, and slips into CORE because "it's a major version", I have a problem with that.

Not least because for the upgrade guide and/or upgrade scripts (we are going to have a better story than Evo=>Revo I trust) BC breaks will need to be documented/worked around.

Re 2): Thanks, I will experiment with that!

OptimusCrime commented 6 years ago

@pbowyer Changing the file ending to something else than .php is not smart. Because php files are interpreted by the web server, the content of the file will not be passed to the client. If you change the file ending to something else, e.g. phpx, then the raw file content is returned in clear text to the browser. If the file contains passwords, critical code etc, that can be pretty scary.

And I do not think you can compare this to evo -> revo. Revo was a brand new product, while revo 3.x is still the same product, just with breaking changes.

I personally think this enhancement if fine. We utilize and reuse the same logic (mediaSources) instead of repeating the same type of code all over the place. A major version is allowed to break stuff, and I think we can justify the breaking here. However, I think the error message should be better.

bezumkin commented 6 years ago

@OptimusCrime Make you core accessible from web is not smart either.

I think the error message should be better

Agree. But I see this on current version

OptimusCrime commented 6 years ago

That is true, but the static files does not have be placed in the core. They can be placed anywhere. So unless you configure your webserver not to return your custom file ending in clear text, that solution is very poor in my opinion.

pbowyer commented 6 years ago

@OptimusCrime

@pbowyer Changing the file ending to something else than .php is not smart.

That is not my idea, that is @bezumkin's fix for the problem. See https://github.com/modxcms/revolution/issues/13898#issuecomment-388293144. Did you mean to @ him?

I personally think this enhancement if fine. We utilize and reuse the same logic (mediaSources) instead of repeating the same type of code all over the place.

To confirm I understand you correctly: the enhancement you are OK with is that static sources cannot be PHP files with the .php extension, because they use the updated mediaSources? Or the enhancement you are OK with is the new mediaSources?

pbowyer commented 6 years ago

And I do not think you can compare this to evo -> revo. Revo was a brand new product, while revo 3.x is still the same product, just with breaking changes.

I disagree. Clients expected to use the latest MODX, and they will expect to use the latest MODX when 3.X is released. To them Revo wasn't something new, it was the next version of MODX Evolution.

I understand there will be breaking changes, but they need to be chosen, not side-effects; and documented in the upgrade notes. If this is the first breaking change in 3.X then documenting it will be easy. If others are already in the branch and haven't been identified, tracking them down is going to be fun during the testing period.

Jako commented 6 years ago

I really don't know how to change this in a good manner. It should be possible to add file types to a media source, so maybe the php type should be added to the filesystem media source or we have to create a static files media source that could handle every file type.

JoshuaLuckers commented 6 years ago

I use static files a lot for templates, chunks, snippets and plugins. HTML files for templates and chunks. PHP files for snippets and plugins.

Does this change mean I have to manually add the php extension to the list of allowed files for each media source?

muzzwood commented 6 years ago

So, just to clarify... Can anyone actually get static elements to load in MODX 3 Alpha? I'm attempting to load a .tpl file. I initially got the File extension .tpl is not permitted so I added it to the allowed file types system setting and now the error doesn't appear but all I'm getting is false in the main element textarea.

JoshuaLuckers commented 6 years ago

I really think this change should be reverted.

JoshuaLuckers commented 4 years ago

At the time of writing creating/updating static elements will show an error but: