wecodemore / wpstarter

Easily bootstrap whole site Composer packages for WordPress.
https://wecodemore.github.io/wpstarter/
MIT License
244 stars 34 forks source link

Removing `wp/wp-content` automatically #28

Closed dnaber-de closed 5 years ago

dnaber-de commented 8 years ago

What do you think about deleting the wp-content directory inside the WordPress package automatically on install/update? It ships all the default themes and they are not directly under update control. So they might have potential security issues and I'm not sure whether they get updated on minor core updates.

dnaber-de commented 8 years ago

Okay I see, this is by design:

/**
 * Register default themes inside WordPress package wp-content folder.
 */
WCM\WPStarter\Helpers::addHook('plugins_loaded', function () {
    WCM\WPStarter\Helpers::loadThemeFolder(true);
}, 0);

Can I safely remove this line from the generated wp-config.php or is there a configuration for that?

Edit: Okay, I really should RTFM before asking: https://wecodemore.github.io/wpstarter/docs/options.html Option register-theme-folder.

But the main question still exists: should wpstarter delete the wp/wp-content directory when register-theme-folder is set to false?

franz-josef-kaiser commented 8 years ago

But the main question still exists: should wpstarter delete the wp/wp-content directory when register-theme-folder is set to false?

I am not sure this would really reflect the user decision to not load it. But I agree that deleting it (with a separate flag to do so), might be a good enhancement.

gmazzap commented 8 years ago

I disagree.

Deleting default theme folder means that a website using WP Starter ends with a WSOD after installation, unless a custom theme is required and DEFAULT_THEME env var is set pointing that theme.

Scanning all requirements to find a WorPress theme is not that simple (doable, surely), and checking DEFAULT_THEME may be impossible, because .env file could not exist when WP Starter script first run.

So is a thing that for sure I don't want to unless explicitly required by user, e.g using a setting in wpstrater section of composer.json.

However, I don't see how having default themes in WordPress folder might be a security issue.

Themes code is not loaded when they are just present in theme folder, so even if malicious code is present in those folders it will not be loaded and may not hurt.

Even we assume that a malicious user found a way to put malicious code in those folders and to make it loaded in some way it would mean:

The only reason that default WordPress themes may be a real security concern, is that we are concerned that those folders aready contain malicious code when we fetch them via Composer.

That's could be possible, for sure, nothing online is 100% secure, but the fact themes got hacked can happen with the same possibility that other parts of core are hacked. If that concern is scary for us, we should not use WordPress at all.

And, again, if I would be a malicious cracker skilled enough to compromise WordPress repo, I would attack core folders, not theme folders with high chance to not be used by users (and easily swapped in any case).

In short, I don't see any real benefit on deleting default theme folders.

If someone really want do that, for any reason, to add

rm -rf /wp/wpcontent

to scripts section in composer.json, would require the same (or even less) effort than adding an ipotetical remove-theme-folders: true to wpstarter configuration.

This is why my idea is to close this issue with wontfix, but I'm happy to hear some good reasons to don't do that.

dnaber-de commented 8 years ago

However, I don't see how having default themes in WordPress folder might be a security issue. Remember when Twentyfifteen shipped with an example.html with an XSS vulnerability? https://wpvulndb.com/vulnerabilities/7965

However, the code inside wp/wp-content is not under composer control and therefore cannot be updated directly.

Deleting default theme folder means that a website using WP Starter ends with a WSOD after installation, unless a custom theme is required and DEFAULT_THEME env var is set pointing that theme.

Yes but isn't that already the situation when setting register-theme-folder to false? @franz-josef-kaiser Suggestion of an additional option would therefore lead to nothing unexpected or completely new behavior.

gmazzap commented 8 years ago

However, the code inside wp/wp-content is not under Composer control and therefore cannot be updated directly.

Via Composer you control the version of WordPress. Themes inside wp/wp-content are the ones shipped with WordPress and they are updated when WordPress is updated. So, actually, with Composer you control what's inside wp/wp-content by controlling wordPress version.

Yes but isn't that already the situation when setting register-theme-folder to false?

Yes, this is why I said that if we will ever choose to delete wp/wp-content we would only do that only with a specific requirement by the user, and never "by default". Assuming that the user knows what is doing (the same assumption we apply for register-theme-folder).

My point is that adding that option is will require:

  1. some work for us (the code to actually imeplement it, add documentation)
  2. some work for user (that have to add that option to composer.json)

Being that the real benefit of removing theme folder is minimal (if it exists), because all the reasons I tried to explain in previous comment, and being that an user can automate the themes folder deletion by adding a simple line to scripts, so with pretty much the same trivial effort needed to add an option (point 2. above) I don't see why we should put effort into this (point 1. above).

dnaber-de commented 8 years ago

Okay.

franz-josef-kaiser commented 8 years ago

Added docs tag now to have a reminder that we can add this to the docs: rm -rf /wp/wp-content

lkraav commented 5 years ago

Added docs tag now to have a reminder that we can add this to the docs: rm -rf /wp/wp-content

I, too, like this. Are the docs getting this info for v3?

EDIT itmw, I added to scripts item and it worked as expected.

gmazzap commented 5 years ago

Yes @lkraav different ways on how to deal with default content will be documented for v3.

gmazzap commented 5 years ago

I'm not gonna update v2 docs for this, I want to focus on v3.

v3 docs has detailed explainatation on different ways on how to deal with default content.

So I consider this issue closed. Thanks anyone involved.