From: Tim Otten <totten@civicrm.org>
Date: Wed, 13 May 2020 03:39:34 +0000 (-0700)
Subject: civicrm/mailing/url - Do not pass CMS route as part of redirect
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=5c9b70e587b61e984094731d01bda29b502e5f60;p=civicrm-core.git

civicrm/mailing/url - Do not pass CMS route as part of redirect

Ex: In a request for `http://dmaster.bknix:8001/civicrm/mailing/url?u=3&qid=2`, the redirect URL
has the value `q=civicrm/mailing/url` incorrectly appended.
---

diff --git a/CRM/Mailing/Page/Url.php b/CRM/Mailing/Page/Url.php
index bd74484e02..40daca3de6 100644
--- a/CRM/Mailing/Page/Url.php
+++ b/CRM/Mailing/Page/Url.php
@@ -17,6 +17,10 @@
 
 /**
  * Redirects a user to the full url from a mailing url.
+ *
+ * General Usage: civicrm/mailing/url?qid={event_queue_id}&u={url_id}
+ *
+ * Additional arguments may be handled by extractPassthroughParameters().
  */
 class CRM_Mailing_Page_Url extends CRM_Core_Page {
 
@@ -29,12 +33,7 @@ class CRM_Mailing_Page_Url extends CRM_Core_Page {
     $queue_id = CRM_Utils_Request::retrieveValue('qid', 'Integer');
     $url_id = CRM_Utils_Request::retrieveValue('u', 'Integer', NULL, TRUE);
     $url = CRM_Mailing_Event_BAO_TrackableURLOpen::track($queue_id, $url_id);
-
-    // CRM-7103
-    // Looking for additional query variables and append them when redirecting.
-    $query_param = $_GET;
-    unset($query_param['qid'], $query_param['u']);
-    $query_string = http_build_query($query_param);
+    $query_string = $this->extractPassthroughParameters();
 
     if (strlen($query_string) > 0) {
       // Parse the url to preserve the fragment.
@@ -62,4 +61,31 @@ class CRM_Mailing_Page_Url extends CRM_Core_Page {
     CRM_Utils_System::redirect($url);
   }
 
+  /**
+   * Determine if this request has any valid pass-through parameters.
+   *
+   * Under CRM-7103 (v3.3), all unrecognized query-parameters (besides qid/u) are passed
+   * through as part of the redirect. This mechanism is relevant to certain
+   * customizations (eg using `hook_alterMailParams` to append extra URL args)
+   * but does not matter for normal URLs.
+   *
+   * The functionality seems vaguely problematic (IMHO) - especially now that
+   * 'extern/url.php' is moving into the CMS/Civi router ('civicrm/mailing/url').
+   * But it's the current protocol.
+   *
+   * A better design might be to support `hook_alterRedirect` in the CiviMail
+   * click-through tracking. Then you don't have to take any untrusted inputs
+   * and you can fix URL mistakes in realtime.
+   *
+   * @return string
+   * @link https://issues.civicrm.org/jira/browse/CRM-7103
+   */
+  protected function extractPassthroughParameters():string {
+    $query_param = $_GET;
+    unset($query_param['qid'], $query_param['u']);
+    unset($query_param[CRM_Core_Config::singleton()->userFrameworkURLVar]);
+    $query_string = http_build_query($query_param);
+    return $query_string;
+  }
+
 }