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

Set CMS Root in "civicrm_paths" more reliably #171

Merged
merged 1 commit into from
Nov 25, 2019

Conversation

christianwach
Copy link
Member

Overview

Builds on https://lab.civicrm.org/dev/core/issues/1412 to set $civicrm_paths['cms.root']['path'] more reliably during the following requests:

  • WP-CLI
  • "Heartbeat" AJAX
  • WordPress pseudo-cron

Before

CRM_Utils_System_WordPress::cmsRootPath() would be called without $civicrm_paths['cms.root']['path'] being set for the above requests, causing the unreliable directory-traversal code to be run.

After

CRM_Utils_System_WordPress::cmsRootPath() runs with $civicrm_paths['cms.root']['path'] being correctly set.

@kcristiano
Copy link
Member

kcristiano commented Nov 25, 2019

  • Standard installl with PR Applied.
    • pass on mailings, searches, contrib pages, wp-cli, and cv
  • WP in it's own directory
    • pass on mailings, searches, contrib pages, wp-cli, and cv
  • WP in it's own directory and wp-content moved
    • pass on mailings, searches, contrib pages, wp-cli, and cv

This looks good to merge.

@seamuslee001 any further thoughts?

@kcristiano
Copy link
Member

As far as testing:

  • "Heartbeat" AJAX
  • WordPress pseudo-cron

@christianwach Are there any specific tests that you can think of?

wp cron is running properly in my test scenarios, heartbeat is not tossing errors. Is there an example where CiviCRM uses admin-ajax? I don't see it, am I missing the obvious?

@christianwach
Copy link
Member Author

christianwach commented Nov 25, 2019

As far as testing:

  • "Heartbeat" AJAX
  • WordPress pseudo-cron

@christianwach Are there any specific tests that you can think of?

"Heartbeat" AJAX

I can't think of anything specific right now, but that's not to say that other plugins might not hook into Heartbeat operations - for example doing something when a draft post (or post revision) is saved. Given we have been thinking about CiviCRM Entities syncing with WordPress CPTs, this might prove an important fix in future.

WordPress pseudo-cron

Again, struggling to think of anything that's guaranteed to be affected. One of the options in CiviCRM WP Member Sync allows it to hook into WordPress pseudo-cron and trigger processes that rely on proper CiviCRM configuration. That it works as it stands would seem to be fortuitous rather than by design.

I think the test is simply that $civicrm_paths['cms.root']['path'] is correctly set when triggered via these routes. I have confirmed that it is. Throw some error logging in at the very top of CRM_Utils_System_WordPress::cmsRootPath() if you want to confirm this:

    $e = new Exception;
    $trace = $e->getTraceAsString();
    error_log( print_r( array(
      'method' => __METHOD__,
      'civicrm_paths' => $civicrm_paths,
      'backtrace' => $trace,
    ), true ) );

The backtrace will show you the origin of the request and as long as $civicrm_paths is populated, then all's well.

@kcristiano
Copy link
Member

Thanks @christianwach Based on the testing, I'll merge

@kcristiano kcristiano merged commit 16629b8 into civicrm:5.20 Nov 25, 2019
@kcristiano kcristiano mentioned this pull request Nov 25, 2019
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.

2 participants