diff --git a/lib/Models/Newsletter.php b/lib/Models/Newsletter.php index 42172cc78a..182941df6a 100644 --- a/lib/Models/Newsletter.php +++ b/lib/Models/Newsletter.php @@ -19,7 +19,6 @@ class Newsletter extends Model { const STATUS_SENT = 'sent'; // automatic newsletters status const STATUS_ACTIVE = 'active'; - const NEWSLETTER_HASH_LENGTH = 6; function __construct() { parent::__construct(); @@ -80,7 +79,7 @@ class Newsletter extends Model { $this->set('hash', ($this->hash) ? $this->hash - : self::generateHash() + : Security::generateHash() ); return parent::save(); } @@ -787,12 +786,4 @@ class Newsletter extends Model { return parent::where('hash', $hash) ->findOne(); } - - static function generateHash() { - return substr( - md5(AUTH_KEY . Security::generateRandomString(15)), - 0, - self::NEWSLETTER_HASH_LENGTH - ); - } } diff --git a/lib/Newsletter/Links/Links.php b/lib/Newsletter/Links/Links.php index 394a19eb97..0cd2e9bdca 100644 --- a/lib/Newsletter/Links/Links.php +++ b/lib/Newsletter/Links/Links.php @@ -12,7 +12,6 @@ use MailPoet\Util\Security; class Links { const DATA_TAG_CLICK = '[mailpoet_click_data]'; const DATA_TAG_OPEN = '[mailpoet_open_data]'; - const HASH_LENGTH = 5; const LINK_TYPE_SHORTCODE = 'shortcode'; const LINK_TYPE_LINK = 'link'; @@ -72,7 +71,7 @@ class Links { static function hash($extracted_links) { $processed_links = array(); foreach($extracted_links as $extracted_link) { - $hash = Security::generateRandomString(self::HASH_LENGTH); + $hash = Security::generateRandomString(); // Use URL as a key to map between extracted and processed links // regardless of their sequential position (useful for link skips etc.) $key = $extracted_link['link']; @@ -216,4 +215,4 @@ class Links { $transformed_data['preview'] = (!empty($data[4])) ? $data[4] : false; return $transformed_data; } -} +} \ No newline at end of file diff --git a/lib/Router/Endpoints/Track.php b/lib/Router/Endpoints/Track.php index acfb1194dd..d40f949589 100644 --- a/lib/Router/Endpoints/Track.php +++ b/lib/Router/Endpoints/Track.php @@ -49,7 +49,9 @@ class Track { Newsletter::findOne($data->queue->newsletter_id) : false; if(!empty($data->link_hash)) { - $data->link = NewsletterLink::getByHash($data->link_hash); + $data->link = NewsletterLink::where('queue_id', $data->queue_id) + ->where('hash', $data->link_hash) + ->findOne(); } return $this->_validateTrackData($data); } diff --git a/lib/Util/Security.php b/lib/Util/Security.php index f916df25b7..61063fa0a6 100644 --- a/lib/Util/Security.php +++ b/lib/Util/Security.php @@ -5,6 +5,8 @@ if(!defined('ABSPATH')) exit; require_once(ABSPATH . 'wp-includes/pluggable.php'); class Security { + const HASH_LENGTH = 6; + static function generateToken($action = 'mailpoet_token') { return wp_create_nonce($action); } @@ -17,4 +19,13 @@ class Security { min(max(5, (int)$length), 32) ); } + + static function generateHash($length = false) { + $length = ($length) ? $length : self::HASH_LENGTH; + return substr( + md5(AUTH_KEY . self::generateRandomString(15)), + 0, + $length + ); + } } \ No newline at end of file diff --git a/tests/unit/Models/NewsletterTest.php b/tests/unit/Models/NewsletterTest.php index ad12f723cb..df1d69ff49 100644 --- a/tests/unit/Models/NewsletterTest.php +++ b/tests/unit/Models/NewsletterTest.php @@ -10,6 +10,7 @@ use MailPoet\Models\NewsletterOption; use MailPoet\Models\StatisticsOpens; use MailPoet\Models\StatisticsClicks; use MailPoet\Models\StatisticsUnsubscribes; +use MailPoet\Util\Security; class NewsletterTest extends MailPoetTest { function _before() { @@ -358,21 +359,21 @@ class NewsletterTest extends MailPoetTest { function testItGeneratesHashOnNewsletterSave() { expect(strlen($this->newsletter->hash)) - ->equals(Newsletter::NEWSLETTER_HASH_LENGTH); + ->equals(Security::HASH_LENGTH); } function testItRegeneratesHashOnNewsletterDuplication() { $duplicate_newsletter = $this->newsletter->duplicate(); expect($duplicate_newsletter->hash)->notEquals($this->newsletter->hash); expect(strlen($duplicate_newsletter->hash)) - ->equals(Newsletter::NEWSLETTER_HASH_LENGTH); + ->equals(Security::HASH_LENGTH); } function testItRegeneratesHashOnNotificationHistoryCreation() { $notification_history = $this->newsletter->createNotificationHistory(); expect($notification_history->hash)->notEquals($this->newsletter->hash); expect(strlen($notification_history->hash)) - ->equals(Newsletter::NEWSLETTER_HASH_LENGTH); + ->equals(Security::HASH_LENGTH); } function testItGetsQueueFromNewsletter() { diff --git a/tests/unit/Util/SecurityTest.php b/tests/unit/Util/SecurityTest.php index 6d80f234c8..6f8b437abf 100644 --- a/tests/unit/Util/SecurityTest.php +++ b/tests/unit/Util/SecurityTest.php @@ -29,4 +29,15 @@ class SecurityTest extends MailPoetTest { expect(ctype_alnum($short_hash))->true(); expect(ctype_alnum($long_hash))->true(); } + + function testItGeneratesRandomHash() { + $hash_1 = Security::generateHash(); + $hash_2 = Security::generateHash(); + expect($hash_1)->notEquals($hash_2); + expect(strlen($hash_1))->equals(Security::HASH_LENGTH); + } + + function testItGeneratesRandomHashWithCustomLength() { + expect(strlen(Security::generateHash(10)))->equals(10); + } } \ No newline at end of file