From 98f95f72adefd3f5ca3d2c79f9e997d80a3d387c Mon Sep 17 00:00:00 2001 From: Vlad Date: Mon, 17 Oct 2016 20:22:25 -0400 Subject: [PATCH 1/2] - Adds validation for import data, including column names (fixes #633) - Prevents nonexistent custom fields from being associated with subscribers --- .../ImportExport/Import/Import.php | 29 +++++++++++++++++-- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/lib/Subscribers/ImportExport/Import/Import.php b/lib/Subscribers/ImportExport/Import/Import.php index 0dea607aae..459b1d4868 100644 --- a/lib/Subscribers/ImportExport/Import/Import.php +++ b/lib/Subscribers/ImportExport/Import/Import.php @@ -21,6 +21,7 @@ class Import { public $updated_at; public function __construct($data) { + $this->validateData($data); $this->subscribers_data = $this->transformSubscribersData( $data['subscribers'], $data['columns'] @@ -41,6 +42,23 @@ class Import { $this->updated_at = date('Y-m-d H:i:s', (int)$data['timestamp'] + 1); } + function validateData($data) { + $required_data_fields = array( + 'subscribers', + 'columns', + 'segments', + 'timestamp', + 'updateSubscribers' + ); + // 1. data should contain all required fields + // 2. column names should only contain alphanumeric & underscore characters + if(count(array_intersect_key(array_flip($required_data_fields), $data)) !== count($required_data_fields) || + preg_grep('/[^a-zA-Z0-9_]/', array_keys($data['columns'])) + ) { + throw new \Exception(__('Missing or invalid subscriber data.', 'mailpoet')); + } + } + function getSubscriberFieldsValidationRules($subscriber_fields) { $validation_rules = array(); foreach($subscriber_fields as $column => $field) { @@ -89,8 +107,8 @@ class Import { $this->synchronizeWPUsers($wp_users); } } - } catch(\PDOException $e) { - throw new \Exception($e->getMessage()); + } catch(\Exception $e) { + throw new \Exception(__('Unable to save imported subscribers.', 'mailpoet')); } $import_factory = new ImportExportFactory('import'); $segments = $import_factory->getSegments(); @@ -364,6 +382,11 @@ class Import { $subscribers_data, $subscriber_custom_fields ) { + // check if custom fields exist in the database + $subscriber_custom_fields = Helpers::flattenArray( + CustomField::whereIn('id', $subscriber_custom_fields)->select('id')->findArray() + ); + if(!$subscriber_custom_fields) return; $subscribers = array_map( function($column) use ($db_subscribers, $subscribers_data) { $count = range(0, count($subscribers_data[$column]) - 1); @@ -406,4 +429,4 @@ class Import { ); } } -} +} \ No newline at end of file From 1285252a8c60c9053d42a8005725cf9dcfba7345 Mon Sep 17 00:00:00 2001 From: Vlad Date: Mon, 17 Oct 2016 20:27:58 -0400 Subject: [PATCH 2/2] - Adds unit tests --- .../ImportExport/Import/ImportTest.php | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/unit/Subscribers/ImportExport/Import/ImportTest.php b/tests/unit/Subscribers/ImportExport/Import/ImportTest.php index fa948b3f06..adf783c92c 100644 --- a/tests/unit/Subscribers/ImportExport/Import/ImportTest.php +++ b/tests/unit/Subscribers/ImportExport/Import/ImportTest.php @@ -66,6 +66,31 @@ class ImportTest extends MailPoetTest { expect($this->import->updated_at)->notEmpty(); } + function testItChecksForRequiredDataFields() { + $data = $this->data; + // exception should be thrown when one or more fields do not exist + unset($data['timestamp']); + try { + $this->import->validateData($data); + self::fail('Missing or invalid data exception not thrown.'); + } catch(Exception $e) { + expect($e->getMessage())->equals('Missing or invalid subscriber data.'); + } + // exception should not be thrown when all fields exist + $this->import->validateData($this->data); + } + + function testItValidatesColumnNames() { + $data = $this->data; + $data['columns']['test) values ((ExtractValue(1,CONCAT(0x5c, (SELECT version())))))%23'] = true; + try { + $this->import->validateData($data); + self::fail('Missing or invalid data exception not thrown.'); + } catch(Exception $e) { + expect($e->getMessage())->equals('Missing or invalid subscriber data.'); + } + } + function testItCanTransformSubscribers() { $custom_field = $this->subscriber_custom_fields[0]; expect($this->import->subscribers_data['first_name'][0])