Suggestions for civicrm#30

fix double encoding

src/ClickTracker/BaseClickTracker.php

@@ -0,0 +1,94 @@
+<?php
+/*
+ +--------------------------------------------------------------------+
+ | CiviCRM version 5.10                                                |
+ +--------------------------------------------------------------------+
+ | Copyright CiviCRM LLC (c) 2004-2017                                |
+ +--------------------------------------------------------------------+
+ | This file is a part of CiviCRM.                                    |
+ |                                                                    |
+ | CiviCRM is free software; you can copy, modify, and distribute it  |
+ | under the terms of the GNU Affero General Public License           |
+ | Version 3, 19 November 2007 and the CiviCRM Licensing Exception.   |
+ |                                                                    |
+ | CiviCRM is distributed in the hope that it will be useful, but     |
+ | WITHOUT ANY WARRANTY; without even the implied warranty of         |
+ | MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.               |
+ | See the GNU Affero General Public License for more details.        |
+ |                                                                    |
+ | You should have received a copy of the GNU Affero General Public   |
+ | License and the CiviCRM Licensing Exception along                  |
+ | with this program; if not, contact CiviCRM LLC                     |
+ | at info[AT]civicrm[DOT]org. If you have questions about the        |
+ | GNU Affero General Public License or the licensing of CiviCRM,     |
+ | see the CiviCRM license FAQ at http://civicrm.org/licensing        |
+ +--------------------------------------------------------------------+
+ */
+namespace Civi\FlexMailer\ClickTracker;
+
+class BaseClickTracker {
+
+  public static $getTrackerURL = ['CRM_Mailing_BAO_TrackableURL', 'getTrackerURL'];
+
+  /**
+   * Create a trackable URL for a URL with tokens.
+   *
+   * @param string $url
+   * @param int $mailing_id
+   * @param int|string $queue_id
+   *
+   * @return string
+   */
+  public static function getTrackerURLForUrlWithTokens($url, $mailing_id, $queue_id) {
+
+    // Parse the URL.
+    // (not using parse_url because it's messy to reassemble)
+    if (!preg_match('/^([^?#]+)([?][^#]*)?(#.*)?$/', $url, $parsed)) {
+      // Failed to parse it, give up and don't track it.
+      return $url;
+    }
+
+    // If we have a token in the URL + path section, we can't tokenise.
+    if (strpos($parsed[1], '{') !== FALSE) {
+      return $url;
+    }
+
+    $trackable_url = $parsed[1];
+
+    // Proces the query parameters, if there are any.
+    $tokenised_params = [];
+    $static_params = [];
+    if (!empty($parsed[2])) {
+      $query_key_value_pairs = explode('&', substr($parsed[2], 1));
+
+      // Separate the tokenised from the static parts.
+      foreach ($query_key_value_pairs as $_) {
+        if (strpos($_, '{') === FALSE) {
+          $static_params[] = $_;
+        }
+        else {
+          $tokenised_params[] = $_;
+        }
+      }
+      // Add the static params to the trackable part.
+      if ($static_params) {
+        $trackable_url .= '?' . implode('&', $static_params);
+      }
+    }
+
+    // Get trackable URL.
+    $getTrackerURL = static::$getTrackerURL;
+    $data = $getTrackerURL($trackable_url, $mailing_id, $queue_id);
+
+    // Append the tokenised bits and the fragment.
+    if ($tokenised_params) {
+      // We know the URL will already have the '?'
+      $data .= '&' . implode('&', $tokenised_params);
+    }
+    if (!empty($parsed[3])) {
+      $data .= $parsed[3];
+    }
+    return $data;
+  }
+}
+

src/ClickTracker/HtmlClickTracker.php

@@ -26,16 +26,20 @@
  */
 namespace Civi\FlexMailer\ClickTracker;
 
-class HtmlClickTracker implements ClickTrackerInterface {
+class HtmlClickTracker extends BaseClickTracker implements ClickTrackerInterface {
 
   public function filterContent($msg, $mailing_id, $queue_id) {
+
+    $getTrackerURL = BaseClickTracker::$getTrackerURL;
+
     return self::replaceHrefUrls($msg,
-      function ($url) use ($mailing_id, $queue_id) {
+      function ($url) use ($mailing_id, $queue_id, $getTrackerURL) {
         if (strpos($url, '{') !== FALSE) {
-          return $url;
+          $data = BaseClickTracker::getTrackerURLForUrlWithTokens($url, $mailing_id, $queue_id);
+        }
+        else {
+          $data = $getTrackerURL($url, $mailing_id, $queue_id);
         }
-        $data = \CRM_Mailing_BAO_TrackableURL::getTrackerURL(
-          $url, $mailing_id, $queue_id);
         $data = htmlentities($data, ENT_NOQUOTES);
         return $data;
       }
@@ -54,7 +58,9 @@ function ($url) use ($mailing_id, $queue_id) {
   public static function replaceHrefUrls($html, $replace) {
     $useNoFollow = TRUE;
     $callback = function ($matches) use ($replace, $useNoFollow) {
-      $replacement = $replace($matches[2]);
+      // Since we're dealing with HTML let's strip out the entities in the URL
+      // so tht we can add them back in later.
+      $replacement = $replace(html_entity_decode($matches[2]));
 
       // See: https://github.com/civicrm/civicrm-core/pull/12561
       // If we track click-throughs on a link, then don't encourage search-engines to traverse them.
@@ -75,6 +81,7 @@ public static function replaceHrefUrls($html, $replace) {
       ';(\<[^>]*href *= *\')([^\'>]+)(\');', $callback, $tmp);
   }
 
+
   //  /**
   //   * Find URL expressions; replace them with tracked URLs.
   //   *

src/ClickTracker/TextClickTracker.php

@@ -29,13 +29,18 @@
 class TextClickTracker implements ClickTrackerInterface {
 
   public function filterContent($msg, $mailing_id, $queue_id) {
+
+    $getTrackerURL = BaseClickTracker::$getTrackerURL;
+
     return self::replaceTextUrls($msg,
-      function ($url) use ($mailing_id, $queue_id) {
+      function ($url) use ($mailing_id, $queue_id, $getTrackerURL) {
         if (strpos($url, '{') !== FALSE) {
-          return $url;
+          $data = HtmlClickTracker::getTrackerURLForUrlWithTokens($url, $mailing_id, $queue_id);
+        }
+        else {
+          $data = $getTrackerURL($url, $mailing_id, $queue_id);
         }
-        return \CRM_Mailing_BAO_TrackableURL::getTrackerURL($url, $mailing_id,
-          $queue_id);
+        return $data;
       }
     );
   }

tests/phpunit/Civi/FlexMailer/ClickTrackerTest.php

@@ -0,0 +1,129 @@
+<?php
+
+use CRM_Flexmailer_ExtensionUtil as E;
+use Civi\Test\HeadlessInterface;
+use Civi\Test\HookInterface;
+use Civi\Test\TransactionalInterface;
+
+use Civi\FlexMailer\ClickTracker\TextClickTracker;
+use Civi\FlexMailer\ClickTracker\HtmlClickTracker;
+use Civi\FlexMailer\ClickTracker\BaseClickTracker;
+
+/**
+ * Tests that URLs are converted to tracked ones if at all possible.
+ *
+ * @group headless
+ */
+class Civi_FlexMailer_ClickTrackerTest extends \PHPUnit_Framework_TestCase implements HeadlessInterface, HookInterface, TransactionalInterface {
+
+  protected $mailing_id;
+
+  public function setUpHeadless() {
+    // Civi\Test has many helpers, like install(), uninstall(), sql(), and sqlFile().
+    // See: https://docs.civicrm.org/dev/en/latest/testing/phpunit/#civitest
+    return \Civi\Test::headless()
+      ->installMe(__DIR__)
+      ->apply();
+  }
+
+  public function setUp() {
+    // Mock the getTrackerURL call; we don't need to test creating a row in a table.
+    BaseClickTracker::$getTrackerURL = function($a, $b, $c) { return 'http://example.com/extern?u=1&qid=1'; };
+
+    parent::setUp();
+  }
+
+  public function tearDown() {
+    // Reset the class.
+    BaseClickTracker::$getTrackerURL = ['CRM_Mailing_BAO_TrackableURL', 'getTrackerURL'];
+    parent::tearDown();
+  }
+
+  /**
+   * Example: Test that a link without any tokens works.
+   */
+  public function testLinkWithoutTokens() {
+    $filter = new TextClickTracker();
+    $msg = 'See this: https://example.com/foo/bar?a=b&c=d#frag';
+    $result = $filter->filterContent($msg, 1, 1);
+    $this->assertEquals('See this: http://example.com/extern?u=1&qid=1', $result);
+  }
+  /**
+   * Example: Test that a link with tokens in the query works.
+   */
+  public function testLinkWithTokensInQueryWithStaticParams() {
+    $filter = new TextClickTracker();
+    $msg = 'See this: https://example.com/foo/bar?a=b&cid={contact.id}';
+    $result = $filter->filterContent($msg, 1, 1);
+    $this->assertEquals('See this: http://example.com/extern?u=1&qid=1&cid={contact.id}', $result);
+  }
+  /**
+   * Example: Test that a link with tokens in the query works.
+   */
+  public function testLinkWithTokensInQueryWithMultipleStaticParams() {
+    $filter = new TextClickTracker();
+    $msg = 'See this: https://example.com/foo/bar?cs={contact.checksum}&a=b&cid={contact.id}';
+    $result = $filter->filterContent($msg, 1, 1);
+    $this->assertEquals('See this: http://example.com/extern?u=1&qid=1&cs={contact.checksum}&cid={contact.id}', $result);
+  }
+  /**
+   * Example: Test that a link with tokens in the query works.
+   */
+  public function testLinkWithTokensInQueryWithMultipleStaticParamsHtml() {
+    $filter = new HtmlClickTracker();
+    $msg = '<a href="https://example.com/foo/bar?cs={contact.checksum}&amp;a=b&amp;cid={contact.id}">See this</a>';
+    $result = $filter->filterContent($msg, 1, 1);
+    $this->assertEquals('<a href="http://example.com/extern?u=1&amp;qid=1&amp;cs={contact.checksum}&amp;cid={contact.id}" rel=\'nofollow\'>See this</a>', $result);
+  }
+  /**
+   * Example: Test that a link with tokens in the query works.
+   */
+  public function testLinkWithTokensInQueryWithoutStaticParams() {
+    $filter = new TextClickTracker();
+    $msg = 'See this: https://example.com/foo/bar?cid={contact.id}';
+    $result = $filter->filterContent($msg, 1, 1);
+    $this->assertEquals('See this: http://example.com/extern?u=1&qid=1&cid={contact.id}', $result);
+  }
+  /**
+   * Example: Test that a link with tokens in the fragment works.
+   *
+   * Seems browsers maintain the fragment when they receive a redirect, so a
+   * token here might still work.
+   */
+  public function testLinkWithTokensInFragment() {
+    $filter = new TextClickTracker();
+    $msg = 'See this: https://example.com/foo/bar?a=b#cid={contact.id}';
+    $result = $filter->filterContent($msg, 1, 1);
+    $this->assertEquals('See this: http://example.com/extern?u=1&qid=1#cid={contact.id}', $result);
+  }
+  /**
+   * Example: Test that a link with tokens in the fragment works.
+   *
+   * Seems browsers maintain the fragment when they receive a redirect, so a
+   * token here might still work.
+   */
+  public function testLinkWithTokensInQueryAndFragment() {
+    $filter = new TextClickTracker();
+    $msg = 'See this: https://example.com/foo/bar?a=b&cid={contact.id}#cid={contact.id}';
+    $result = $filter->filterContent($msg, 1, 1);
+    $this->assertEquals('See this: http://example.com/extern?u=1&qid=1&cid={contact.id}#cid={contact.id}', $result);
+  }
+  /**
+   * We can't handle tokens in the domain so it should not be tracked.
+   */
+  public function testLinkWithTokensInDomainFails() {
+    $filter = new TextClickTracker();
+    $msg = 'See this: https://{some.domain}.com/foo/bar';
+    $result = $filter->filterContent($msg, 1, 1);
+    $this->assertEquals('See this: https://{some.domain}.com/foo/bar', $result);
+  }
+  /**
+   * We can't handle tokens in the path so it should not be tracked.
+   */
+  public function testLinkWithTokensInPathFails() {
+    $filter = new TextClickTracker();
+    $msg = 'See this: https://example.com/{some.path}';
+    $result = $filter->filterContent($msg, 1, 1);
+    $this->assertEquals('See this: https://example.com/{some.path}', $result);
+  }
+}

tests/phpunit/bootstrap.php

@@ -1,21 +1,16 @@
 <?php
 
-require_once dirname(dirname(__DIR__)) . '/flexmailer.php';
-
 ini_set('memory_limit', '2G');
 ini_set('safe_mode', 0);
+eval(cv('php:boot --level=classloader', 'phpcode'));
 
-// We need to allow CiviUnitTestCase... but may break E2E support....
-
-// eval(cv('php:boot --level=classloader', 'phpcode'));
-define('CIVICRM_CONTAINER_CACHE', 'never');
-define('CIVICRM_TEST', 1);
-putenv('CIVICRM_UF=' . ($_ENV['CIVICRM_UF'] = 'UnitTests'));
-eval(cv('php:boot --level=settings', 'phpcode'));
-
-if (CIVICRM_UF === 'UnitTests') {
-  Civi\Test::headless()->apply();
-}
+// Allow autoloading of PHPUnit helper classes in this extension.
+$loader = new \Composer\Autoload\ClassLoader();
+$loader->add('CRM_', __DIR__);
+$loader->add('Civi\\', __DIR__);
+$loader->add('api_', __DIR__);
+$loader->add('api\\', __DIR__);
+$loader->register();
 
 /**
  * Call the "cv" command.
@@ -34,6 +29,11 @@ function cv($cmd, $decode = 'json') {
   $descriptorSpec = array(0 => array("pipe", "r"), 1 => array("pipe", "w"), 2 => STDERR);
   $oldOutput = getenv('CV_OUTPUT');
   putenv("CV_OUTPUT=json");
+
+  // Execute `cv` in the original folder. This is a work-around for
+  // phpunit/codeception, which seem to manipulate PWD.
+  $cmd = sprintf('cd %s; %s', escapeshellarg(getenv('PWD')), $cmd);
+
   $process = proc_open($cmd, $descriptorSpec, $pipes, __DIR__);
   putenv("CV_OUTPUT=$oldOutput");
   fclose($pipes[0]);