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

[REF] Fix Security status check urls to work on WordPress #20111

Conversation

seamuslee001
Copy link
Contributor

Overview

This fixes a bug where by in the status check it tries to user a url for the temporaryUploads and customUploads directory checks that adds /files on the end of the image upload url.

Before

Wrong url constructed for the check on temporaryUploads and CustomUploads folders in WordPress
e.g. /wp-content/uploads/civicrm/persist/contribute/files

After

Correct url for temporary uploads etc
e.g. /wp-content/uploads/civicrm/custom

Technical Details

Because there is no specific setting for these urls it uses this fileMarker to split up the imageUploadURL into a base url that it then can replace with the target directory e.g. uploads/civicrm/custom here https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/Check/Component/Security.php#L382 because of fact it was using /files/ instead of /uploads/ it meant that the $heuristicURL here wasn't checking either the uploadDIr or customFileUploadDir correctly so was potentially giving off a false negative on WordPress (potentially) https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/Check/Component/Security.php#L119.

I am concerned that this may not work well for those that still have the file uploads for civicrm within the plugin directory but it might be ok

ping @kcristiano @haystack @totten

@civibot
Copy link

civibot bot commented Apr 21, 2021

(Standard links)

@civibot civibot bot added the master label Apr 21, 2021
@kcristiano
Copy link
Member

@seamuslee001 I've tested this on a site with the old plugins_dir/files/civicrm structure and sites with the curent setup.

I do not see any issues with an r-run once the patch is applied.

on any site I have with the legacy/old file structure I have defined

global $civicrm_paths;
$civicrm_paths['civicrm.files']['path'] = '/home/site/public_html/wp-content/plugins/files/civicrm/';
$civicrm_paths['civicrm.files']['url'] = 'https://site.example.org/wp-content/plugins/files/civicrm/';

global $civicrm_setting;
$civicrm_setting['Directory Preferences']['customTemplateDir'] = '/home/site/public_html/wp-content/plugins/files/civicrm/templates/';
$civicrm_setting['Directory Preferences']['customPHPPathDir'] = '/home/site/public_html/wp-content/plugins/files/civicrm/php/';
$civicrm_setting['Directory Preferences']['extensionsDir'] = '/home/site/public_html/wp-content/plugins/files/civicrm/extensions/';
$civicrm_setting['URL Preferences']['extensionsURL'] = 'https://site.example.org/wp-content/plugins/files/civicrm/extensions/';
$civicrm_setting['Directory Preferences']['imageUploadDir'] = '/home/site/public_html/wp-content/plugins/files/civicrm/persist/contribute/images/';
$civicrm_setting['URL Preferences']['imageUploadURL'] = 'https://site.example.org/wp-content/plugins/files/civicrm/persist/contribute/images/';
$civicrm_setting['URL Preferences']['userFrameworkResourceURL'] = 'https://site.example.org/wp-content/plugins/civicrm/civicrm/';

I've done this once we upgraded to 5.29 based on this article: https://docs.civicrm.org/sysadmin/en/latest/upgrade/version-specific/#civicrm-529

I think we should merge and then make note of the change so it can be tested. Maybe add to a dev-digest once 5.28 RC is cut?

@eileenmcnaughton
Copy link
Contributor

I'm merging per the above - but I think you mean 5.38 @kcristiano ?

I've noted this in the dev-digest but if you have some text that would be great

@eileenmcnaughton eileenmcnaughton merged commit 0e59da6 into civicrm:master Apr 27, 2021
@eileenmcnaughton eileenmcnaughton deleted the wordpress_security_status_check_fix branch April 27, 2021 01:58
@kcristiano
Copy link
Member

Thanks Eileen. I did mean 5.38, not looking for a backport of almost 1 year :)

@eileenmcnaughton
Copy link
Contributor

@seamuslee001 @kcristiano I have a note in the dev-digest to say 'something about' this PR - is there something we want to communicate here?

@kcristiano
Copy link
Member

@eileenmcnaughton I wanted to alert people that the check is now looking for uploads in WP as oppsed to the default files directly.

Thinking on this, Maybe we don't need to highlight. It was wrong before and this will only be 'broken' if the civicrm.settings.php is badly misconfigured.

I say let's not mention as it could lead to confusion. Release notes will be enough here.

@eileenmcnaughton
Copy link
Contributor

thanks @kcristiano

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants