From 3632568409b1f71d24f1a735aaca1de98b2ee321 Mon Sep 17 00:00:00 2001 From: KevinBKozan Date: Wed, 21 Feb 2018 14:01:01 -0600 Subject: [PATCH 1/3] MQE-802: Make ActionObject list of allowed characters comprehensive - changed actionObject to use two patterns for replacement: (word.word and word.word(anycharacters)). Fixes the blacklist problem. - added check for alphanumeric/underscore naming in various objects. - added sanitization of parameters, so that commas can now be used in 'stringliteral' --- .../Resources/CharacterReplacementTest.txt | 35 +++++++++++++++ .../TestModule/Test/BasicFunctionalTest.xml | 2 +- .../Test/CharacterReplacementTest.xml | 18 ++++++++ .../ReferenceReplacementGenerationTest.php | 9 ++++ .../Handlers/DataObjectHandler.php | 5 +++ .../Page/Handlers/PageObjectHandler.php | 5 +++ .../Page/Handlers/SectionObjectHandler.php | 44 ++++++++++++------- .../Test/Objects/ActionObject.php | 25 ++++++++--- .../Test/Util/ActionObjectExtractor.php | 7 ++- .../Test/Util/TestObjectExtractor.php | 19 +++++--- 10 files changed, 140 insertions(+), 29 deletions(-) create mode 100644 dev/tests/verification/Resources/CharacterReplacementTest.txt create mode 100644 dev/tests/verification/TestModule/Test/CharacterReplacementTest.xml diff --git a/dev/tests/verification/Resources/CharacterReplacementTest.txt b/dev/tests/verification/Resources/CharacterReplacementTest.txt new file mode 100644 index 000000000..db4d4f6d9 --- /dev/null +++ b/dev/tests/verification/Resources/CharacterReplacementTest.txt @@ -0,0 +1,35 @@ +click("#element .abcdefghijklmnopqrstuvwxyz1234567890"); + $I->click("#element .`~!@#$%^&*()-_=+{}[]|\;:\".,>click("#abcdefghijklmnopqrstuvwxyz1234567890 .abcdefghijklmnopqrstuvwxyz1234567890"); + $I->click("#`~!@#$%^&*()-_=+{}[]|\;:\".,> - + \ No newline at end of file diff --git a/dev/tests/verification/TestModule/Test/CharacterReplacementTest.xml b/dev/tests/verification/TestModule/Test/CharacterReplacementTest.xml new file mode 100644 index 000000000..b4f141238 --- /dev/null +++ b/dev/tests/verification/TestModule/Test/CharacterReplacementTest.xml @@ -0,0 +1,18 @@ + + + + + + + + + + + + diff --git a/dev/tests/verification/Tests/ReferenceReplacementGenerationTest.php b/dev/tests/verification/Tests/ReferenceReplacementGenerationTest.php index b6ccbdc8f..ef5c0bce6 100644 --- a/dev/tests/verification/Tests/ReferenceReplacementGenerationTest.php +++ b/dev/tests/verification/Tests/ReferenceReplacementGenerationTest.php @@ -69,4 +69,13 @@ public function testSectionReferenceReplacementCest() { $this->generateAndCompareTest(self::SECTION_REPLACEMENT_TEST); } + + /** + * Tests replacement of all characters into string literal references. + * Used to ensure users can input everything but single quotes into 'stringLiteral' in parameterized selectors + */ + public function testCharacterReplacementCest() + { + $this->generateAndCompareTest("CharacterReplacementTest"); + } } diff --git a/src/Magento/FunctionalTestingFramework/DataGenerator/Handlers/DataObjectHandler.php b/src/Magento/FunctionalTestingFramework/DataGenerator/Handlers/DataObjectHandler.php index 3bd3cffe3..f639280ec 100644 --- a/src/Magento/FunctionalTestingFramework/DataGenerator/Handlers/DataObjectHandler.php +++ b/src/Magento/FunctionalTestingFramework/DataGenerator/Handlers/DataObjectHandler.php @@ -29,6 +29,7 @@ class DataObjectHandler implements ObjectHandlerInterface const _ENTITY_KEY = 'entityKey'; const _SEPARATOR = '->'; const _REQUIRED_ENTITY = 'requiredEntity'; + const DATA_NAME_ERROR_MSG = "Entity names cannot contain non alphanumeric characters.\tData='%s'"; /** * The singleton instance of this class @@ -110,6 +111,10 @@ private function processParserOutput($parserOutput) $rawEntities = $parserOutput[self::_ENTITY]; foreach ($rawEntities as $name => $rawEntity) { + if (preg_match('/[^a-zA-Z0-9_]/', $name)) { + throw new XmlException(sprintf(self::DATA_NAME_ERROR_MSG, $name)); + } + $type = $rawEntity[self::_TYPE]; $data = []; $linkedEntities = []; diff --git a/src/Magento/FunctionalTestingFramework/Page/Handlers/PageObjectHandler.php b/src/Magento/FunctionalTestingFramework/Page/Handlers/PageObjectHandler.php index 7e0b4dd9b..49484b491 100644 --- a/src/Magento/FunctionalTestingFramework/Page/Handlers/PageObjectHandler.php +++ b/src/Magento/FunctionalTestingFramework/Page/Handlers/PageObjectHandler.php @@ -10,6 +10,7 @@ use Magento\FunctionalTestingFramework\ObjectManagerFactory; use Magento\FunctionalTestingFramework\Page\Objects\PageObject; use Magento\FunctionalTestingFramework\XmlParser\PageParser; +use Magento\FunctionalTestingFramework\Exceptions\XmlException; class PageObjectHandler implements ObjectHandlerInterface { @@ -19,6 +20,7 @@ class PageObjectHandler implements ObjectHandlerInterface const MODULE = 'module'; const PARAMETERIZED = 'parameterized'; const AREA = 'area'; + const NAME_BLACKLIST_ERROR_MSG = "Page names cannot contain non alphanumeric characters.\tPage='%s'"; /** * The singleton instance of this class @@ -49,6 +51,9 @@ private function __construct() } foreach ($parserOutput as $pageName => $pageData) { + if (preg_match('/[^a-zA-Z0-9_]/', $pageName)) { + throw new XmlException(sprintf(self::NAME_BLACKLIST_ERROR_MSG, $pageName)); + } $url = $pageData[self::URL]; $module = $pageData[self::MODULE]; $sectionNames = array_keys($pageData[self::SECTION]); diff --git a/src/Magento/FunctionalTestingFramework/Page/Handlers/SectionObjectHandler.php b/src/Magento/FunctionalTestingFramework/Page/Handlers/SectionObjectHandler.php index b07f9428f..a361579e8 100644 --- a/src/Magento/FunctionalTestingFramework/Page/Handlers/SectionObjectHandler.php +++ b/src/Magento/FunctionalTestingFramework/Page/Handlers/SectionObjectHandler.php @@ -11,6 +11,7 @@ use Magento\FunctionalTestingFramework\Page\Objects\ElementObject; use Magento\FunctionalTestingFramework\Page\Objects\SectionObject; use Magento\FunctionalTestingFramework\XmlParser\SectionParser; +use Magento\FunctionalTestingFramework\Exceptions\XmlException; class SectionObjectHandler implements ObjectHandlerInterface { @@ -21,6 +22,8 @@ class SectionObjectHandler implements ObjectHandlerInterface const LOCATOR_FUNCTION = 'locatorFunction'; const TIMEOUT = 'timeout'; const PARAMETERIZED = 'parameterized'; + const SECTION_NAME_ERROR_MSG = "Section names cannot contain non alphanumeric characters.\tSection='%s'"; + const ELEMENT_NAME_ERROR_MSG = "Element names cannot contain non alphanumeric characters.\tElement='%s'"; /** * Singleton instance of this class @@ -54,21 +57,32 @@ private function __construct() foreach ($parserOutput as $sectionName => $sectionData) { $elements = []; - foreach ($sectionData[SectionObjectHandler::ELEMENT] as $elementName => $elementData) { - $elementType = $elementData[SectionObjectHandler::TYPE]; - $elementSelector = $elementData[SectionObjectHandler::SELECTOR] ?? null; - $elementLocatorFunc = $elementData[SectionObjectHandler::LOCATOR_FUNCTION] ?? null; - $elementTimeout = $elementData[SectionObjectHandler::TIMEOUT] ?? null; - $elementParameterized = $elementData[SectionObjectHandler::PARAMETERIZED] ?? false; - - $elements[$elementName] = new ElementObject( - $elementName, - $elementType, - $elementSelector, - $elementLocatorFunc, - $elementTimeout, - $elementParameterized - ); + if (preg_match('/[^a-zA-Z0-9_]/', $sectionName)) { + throw new XmlException(sprintf(self::SECTION_NAME_ERROR_MSG, $sectionName)); + } + + try { + foreach ($sectionData[SectionObjectHandler::ELEMENT] as $elementName => $elementData) { + if (preg_match('/[^a-zA-Z0-9_]/', $elementName)) { + throw new XmlException(sprintf(self::ELEMENT_NAME_ERROR_MSG, $elementName, $sectionName)); + } + $elementType = $elementData[SectionObjectHandler::TYPE]; + $elementSelector = $elementData[SectionObjectHandler::SELECTOR] ?? null; + $elementLocatorFunc = $elementData[SectionObjectHandler::LOCATOR_FUNCTION] ?? null; + $elementTimeout = $elementData[SectionObjectHandler::TIMEOUT] ?? null; + $elementParameterized = $elementData[SectionObjectHandler::PARAMETERIZED] ?? false; + + $elements[$elementName] = new ElementObject( + $elementName, + $elementType, + $elementSelector, + $elementLocatorFunc, + $elementTimeout, + $elementParameterized + ); + } + } catch (XmlException $exception) { + throw new XmlException($exception->getMessage() . " in Section '{$sectionName}'"); } $this->sectionObjects[$sectionName] = new SectionObject($sectionName, $elements); diff --git a/src/Magento/FunctionalTestingFramework/Test/Objects/ActionObject.php b/src/Magento/FunctionalTestingFramework/Test/Objects/ActionObject.php index 88d45db5d..e87ffe034 100644 --- a/src/Magento/FunctionalTestingFramework/Test/Objects/ActionObject.php +++ b/src/Magento/FunctionalTestingFramework/Test/Objects/ActionObject.php @@ -32,8 +32,8 @@ class ActionObject const ACTION_ATTRIBUTE_URL = 'url'; const ACTION_ATTRIBUTE_SELECTOR = 'selector'; const ACTION_ATTRIBUTE_VARIABLE_REGEX_PARAMETER = '/\(.+\)/'; - const ACTION_ATTRIBUTE_VARIABLE_REGEX_PATTERN = '/{{[\w.\[\]()\',$ ]+}}/'; - const ACTION_ATTRIBUTE_VARIABLE_REGEX_PATTERN_WITH_PARAMS= '/{{[\w]+\.[\w]+\([\w ,.\'{$}]+\)}}/'; + const ACTION_ATTRIBUTE_VARIABLE_REGEX_PATTERN = '/{{[\w]+\.[\w]+}}/'; + const ACTION_ATTRIBUTE_VARIABLE_REGEX_PATTERN_WITH_PARAMS= '/{{[\w]+\.[\w]+\(.+\)}}/'; /** * The unique identifier for the action @@ -338,7 +338,7 @@ private function stripAndSplitReference($reference) } /** - * Returns an array containing all parameters found inside () block of test input. + * Returns an array containing all parameters found inside () block of test input. Expected delimiter is ', '. * Returns null if no parameters were found. * * @param string $reference @@ -346,10 +346,25 @@ private function stripAndSplitReference($reference) */ private function stripAndReturnParameters($reference) { + // 'string', or 'string,!@#$%^&*()_+' + $literalParametersRegex = "/'[^']+'/"; + preg_match(ActionObject::ACTION_ATTRIBUTE_VARIABLE_REGEX_PARAMETER, $reference, $matches); if (!empty($matches)) { - $strippedReference = str_replace(')', '', str_replace('(', '', $matches[0])); - return explode(',', $strippedReference); + $strippedReference = ltrim(rtrim($matches[0], ")"), "("); + + // Pull out all 'string' references, as they can contain 'string with , comma in it' + preg_match_all($literalParametersRegex, $strippedReference, $literalReferences); + $strippedReference = preg_replace($literalParametersRegex, '&&stringReference&&', $strippedReference); + + // Sanitize all 'string,data.field,$persisted.field$' => 'string, data.field, $persisted.field$' for return + $strippedReference = preg_replace('/,(?! )/', ', ', $strippedReference); + + foreach ($literalReferences[0] as $key => $value) { + $strippedReference = preg_replace('/&&stringReference&&/', $value, $strippedReference, 1); + } + + return explode(', ', $strippedReference); } return null; } diff --git a/src/Magento/FunctionalTestingFramework/Test/Util/ActionObjectExtractor.php b/src/Magento/FunctionalTestingFramework/Test/Util/ActionObjectExtractor.php index ae1f65280..4b3419660 100644 --- a/src/Magento/FunctionalTestingFramework/Test/Util/ActionObjectExtractor.php +++ b/src/Magento/FunctionalTestingFramework/Test/Util/ActionObjectExtractor.php @@ -21,7 +21,8 @@ class ActionObjectExtractor extends BaseObjectExtractor const ACTION_GROUP_REF = 'ref'; const ACTION_GROUP_ARGUMENTS = 'arguments'; const ACTION_GROUP_ARG_VALUE = 'value'; - const BEFORE_AFTER_ERROR_MSG = "Merge Error - Steps cannot have both before and after attributes.\tTestStep='%s'"; + const BEFORE_AFTER_ERROR_MSG = "Merge Error - Steps cannot have both before and after attributes.\tStepKey='%s'"; + const STEP_KEY_BLACKLIST_ERROR_MSG = "StepKeys cannot contain non alphanumeric characters.\tStepKey='%s'"; /** * ActionObjectExtractor constructor. @@ -46,6 +47,10 @@ public function extractActions($testActions) foreach ($testActions as $actionName => $actionData) { $stepKey = $actionData[self::TEST_STEP_MERGE_KEY]; + if (preg_match('/[^a-zA-Z0-9_]/', $stepKey)) { + throw new XmlException(sprintf(self::STEP_KEY_BLACKLIST_ERROR_MSG, $actionName)); + } + $actionAttributes = $this->stripDescriptorTags( $actionData, self::TEST_STEP_MERGE_KEY, diff --git a/src/Magento/FunctionalTestingFramework/Test/Util/TestObjectExtractor.php b/src/Magento/FunctionalTestingFramework/Test/Util/TestObjectExtractor.php index 0ec3fd0dc..d9044a0a1 100644 --- a/src/Magento/FunctionalTestingFramework/Test/Util/TestObjectExtractor.php +++ b/src/Magento/FunctionalTestingFramework/Test/Util/TestObjectExtractor.php @@ -6,6 +6,7 @@ namespace Magento\FunctionalTestingFramework\Test\Util; +use Magento\FunctionalTestingFramework\Exceptions\XmlException; use Magento\FunctionalTestingFramework\Test\Objects\TestObject; /** @@ -104,12 +105,16 @@ public function extractTestData($testData) } // TODO extract filename info and store - return new TestObject( - $testData[self::NAME], - $this->actionObjectExtractor->extractActions($testActions), - $testAnnotations, - $testHooks, - $filename - ); + try { + return new TestObject( + $testData[self::NAME], + $this->actionObjectExtractor->extractActions($testActions), + $testAnnotations, + $testHooks, + $filename + ); + } catch (XmlException $exception) { + throw new XmlException($exception->getMessage() . ' in Test ' . $filename); + } } } From 07fe3e90f19e0d1416510fbbc34a9bbb999675e9 Mon Sep 17 00:00:00 2001 From: KevinBKozan Date: Fri, 23 Feb 2018 11:55:55 -0600 Subject: [PATCH 2/3] MQE-802: Make ActionObject list of allowed characters comprehensive - Changed delimiter from ', ', to '::::' --- .../Resources/CharacterReplacementTest.txt | 4 +++- .../TestModule/Test/CharacterReplacementTest.xml | 9 +++++---- .../Test/Objects/ActionObject.php | 12 ++++++++---- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/dev/tests/verification/Resources/CharacterReplacementTest.txt b/dev/tests/verification/Resources/CharacterReplacementTest.txt index db4d4f6d9..13b66cb50 100644 --- a/dev/tests/verification/Resources/CharacterReplacementTest.txt +++ b/dev/tests/verification/Resources/CharacterReplacementTest.txt @@ -28,8 +28,10 @@ class CharacterReplacementTestCest public function CharacterReplacementTest(AcceptanceTester $I) { $I->click("#element .abcdefghijklmnopqrstuvwxyz1234567890"); - $I->click("#element .`~!@#$%^&*()-_=+{}[]|\;:\".,>click("#element .`~!@#$%^&*()-_=+{}[]|\;:\".,>click("#element .words, and, commas, and, spaces"); $I->click("#abcdefghijklmnopqrstuvwxyz1234567890 .abcdefghijklmnopqrstuvwxyz1234567890"); $I->click("#`~!@#$%^&*()-_=+{}[]|\;:\".,>click("#words, and, commas, and, spaces .words, and, commas, and, spaces"); } } diff --git a/dev/tests/verification/TestModule/Test/CharacterReplacementTest.xml b/dev/tests/verification/TestModule/Test/CharacterReplacementTest.xml index b4f141238..12450ed90 100644 --- a/dev/tests/verification/TestModule/Test/CharacterReplacementTest.xml +++ b/dev/tests/verification/TestModule/Test/CharacterReplacementTest.xml @@ -9,10 +9,11 @@ - - - - + + + + + diff --git a/src/Magento/FunctionalTestingFramework/Test/Objects/ActionObject.php b/src/Magento/FunctionalTestingFramework/Test/Objects/ActionObject.php index e87ffe034..d4312ae36 100644 --- a/src/Magento/FunctionalTestingFramework/Test/Objects/ActionObject.php +++ b/src/Magento/FunctionalTestingFramework/Test/Objects/ActionObject.php @@ -346,8 +346,9 @@ private function stripAndSplitReference($reference) */ private function stripAndReturnParameters($reference) { - // 'string', or 'string,!@#$%^&*()_+' + // 'string', or 'string,!@#$%^&*()_+, ' $literalParametersRegex = "/'[^']+'/"; + $postCleanupDelimiter = "::::"; preg_match(ActionObject::ACTION_ATTRIBUTE_VARIABLE_REGEX_PARAMETER, $reference, $matches); if (!empty($matches)) { @@ -357,14 +358,17 @@ private function stripAndReturnParameters($reference) preg_match_all($literalParametersRegex, $strippedReference, $literalReferences); $strippedReference = preg_replace($literalParametersRegex, '&&stringReference&&', $strippedReference); - // Sanitize all 'string,data.field,$persisted.field$' => 'string, data.field, $persisted.field$' for return - $strippedReference = preg_replace('/,(?! )/', ', ', $strippedReference); + // Sanitize all 'string, data.field,$persisted.field$' => 'string::::data.field::::$persisted.field$' for return + $strippedReference = preg_replace('/,/', ', ', $strippedReference); + $strippedReference = str_replace(',', $postCleanupDelimiter, $strippedReference); + $strippedReference = str_replace(' ', '', $strippedReference); + // Replace string references into string, needed to keep sequence foreach ($literalReferences[0] as $key => $value) { $strippedReference = preg_replace('/&&stringReference&&/', $value, $strippedReference, 1); } - return explode(', ', $strippedReference); + return explode($postCleanupDelimiter, $strippedReference); } return null; } From 23556f4901bb78f0aeb7087142c812ed86dbd1af Mon Sep 17 00:00:00 2001 From: KevinBKozan Date: Tue, 27 Feb 2018 08:43:28 -0600 Subject: [PATCH 3/3] MQE-802: Make ActionObject list of allowed characters comprehensive - CR Fix --- .../FunctionalTestingFramework/Test/Objects/ActionObject.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Magento/FunctionalTestingFramework/Test/Objects/ActionObject.php b/src/Magento/FunctionalTestingFramework/Test/Objects/ActionObject.php index d4312ae36..db7bb2f1b 100644 --- a/src/Magento/FunctionalTestingFramework/Test/Objects/ActionObject.php +++ b/src/Magento/FunctionalTestingFramework/Test/Objects/ActionObject.php @@ -358,7 +358,7 @@ private function stripAndReturnParameters($reference) preg_match_all($literalParametersRegex, $strippedReference, $literalReferences); $strippedReference = preg_replace($literalParametersRegex, '&&stringReference&&', $strippedReference); - // Sanitize all 'string, data.field,$persisted.field$' => 'string::::data.field::::$persisted.field$' for return + // Sanitize 'string, data.field,$persisted.field$' => 'string::::data.field::::$persisted.field$' $strippedReference = preg_replace('/,/', ', ', $strippedReference); $strippedReference = str_replace(',', $postCleanupDelimiter, $strippedReference); $strippedReference = str_replace(' ', '', $strippedReference);