From 5c9b70e587b61e984094731d01bda29b502e5f60 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Tue, 12 May 2020 20:39:34 -0700 Subject: [PATCH] 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. --- CRM/Mailing/Page/Url.php | 38 ++++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-) 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; + } + } -- 2.25.1