Skip to content

Commit f03f1e1

Browse files
KevinBKozanKevinBKozan
authored
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'
1 parent 40465e8 commit f03f1e1

File tree

10 files changed

+147
-29
lines changed

10 files changed

+147
-29
lines changed

‎dev/tests/verification/Resources/CharacterReplacementTest.txt

+37
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<?php
2+
namespace Magento\AcceptanceTest\_generated\Backend;
3+
4+
use Magento\FunctionalTestingFramework\AcceptanceTester;
5+
use Magento\FunctionalTestingFramework\DataGenerator\Handlers\DataObjectHandler;
6+
use Magento\FunctionalTestingFramework\DataGenerator\Persist\DataPersistenceHandler;
7+
use Magento\FunctionalTestingFramework\DataGenerator\Objects\EntityDataObject;
8+
use \Codeception\Util\Locator;
9+
use Yandex\Allure\Adapter\Annotation\Features;
10+
use Yandex\Allure\Adapter\Annotation\Stories;
11+
use Yandex\Allure\Adapter\Annotation\Title;
12+
use Yandex\Allure\Adapter\Annotation\Description;
13+
use Yandex\Allure\Adapter\Annotation\Parameter;
14+
use Yandex\Allure\Adapter\Annotation\Severity;
15+
use Yandex\Allure\Adapter\Model\SeverityLevel;
16+
use Yandex\Allure\Adapter\Annotation\TestCaseId;
17+
18+
/**
19+
*/
20+
class CharacterReplacementTestCest
21+
{
22+
/**
23+
* @Parameter(name = "AcceptanceTester", value="$I")
24+
* @param AcceptanceTester $I
25+
* @return void
26+
* @throws \Exception
27+
*/
28+
public function CharacterReplacementTest(AcceptanceTester $I)
29+
{
30+
$I->click("#element .abcdefghijklmnopqrstuvwxyz1234567890");
31+
$I->click("#element .`~!@#$%^&*()-_=+{}[]|\;:\".,></?()3., ");
32+
$I->click("#element .words, and, commas, and, spaces");
33+
$I->click("#abcdefghijklmnopqrstuvwxyz1234567890 .abcdefghijklmnopqrstuvwxyz1234567890");
34+
$I->click("#`~!@#$%^&*()-_=+{}[]|\;:\".,></?() .`~!@#$%^&*()-_=+{}[]|\;:\".,></?()");
35+
$I->click("#words, and, commas, and, spaces .words, and, commas, and, spaces");
36+
}
37+
}

‎dev/tests/verification/TestModule/Test/BasicFunctionalTest.xml

+1-1
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,6 @@
108108
<waitForElementNotVisible selector=".functionalTestSelector" time="30" stepKey="waitForElementNotVisibleKey1" />
109109
<waitForElementVisible selector=".functionalTestSelector" time="30" stepKey="waitForElementVisibleKey1" />
110110
<waitForJS function="someJsFunction" time="30" stepKey="waitForJSKey1" />
111-
<waitForText selector=".functionalTestSelector" userInput="someInput" time="30" stepKey=""/>
111+
<waitForText selector=".functionalTestSelector" userInput="someInput" time="30" stepKey="waitForText1"/>
112112
</test>
113113
</tests>

‎dev/tests/verification/TestModule/Test/CharacterReplacementTest.xml

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<!--
3+
/**
4+
* Copyright © Magento, Inc. All rights reserved.
5+
* See COPYING.txt for license details.
6+
*/
7+
-->
8+
9+
<tests xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
10+
xsi:noNamespaceSchemaLocation="../../../../../src/Magento/FunctionalTestingFramework/Test/etc/testSchema.xsd">
11+
<test name="CharacterReplacementTest">
12+
<click stepKey="allChars1" selector="{{SampleSection.oneParamElement('abcdefghijklmnopqrstuvwxyz1234567890')}}"/>
13+
<click stepKey="allChars2" selector="{{SampleSection.oneParamElement('`~!@#$%^&amp;*()-_=+{}[]|\;:&quot;.,&gt;&lt;/?()$213., ')}}"/>
14+
<click stepKey="allChars3" selector="{{SampleSection.oneParamElement('words, and, commas, and, spaces')}}"/>
15+
<click stepKey="allChars4" selector="{{SampleSection.twoParamElement('abcdefghijklmnopqrstuvwxyz1234567890','abcdefghijklmnopqrstuvwxyz1234567890')}}"/>
16+
<click stepKey="allChars5" selector="{{SampleSection.twoParamElement('`~!@#$%^&amp;*()-_=+{}[]|\;:&quot;.,&gt;&lt;/?()','`~!@#$%^&amp;*()-_=+{}[]|\;:&quot;.,&gt;&lt;/?()')}}"/>
17+
<click stepKey="allChars6" selector="{{SampleSection.twoParamElement('words, and, commas, and, spaces', 'words, and, commas, and, spaces')}}"/>
18+
</test>
19+
</tests>

‎dev/tests/verification/Tests/ReferenceReplacementGenerationTest.php

+9
Original file line numberDiff line numberDiff line change
@@ -69,4 +69,13 @@ public function testSectionReferenceReplacementCest()
6969
{
7070
$this->generateAndCompareTest(self::SECTION_REPLACEMENT_TEST);
7171
}
72+
73+
/**
74+
* Tests replacement of all characters into string literal references.
75+
* Used to ensure users can input everything but single quotes into 'stringLiteral' in parameterized selectors
76+
*/
77+
public function testCharacterReplacementCest()
78+
{
79+
$this->generateAndCompareTest("CharacterReplacementTest");
80+
}
7281
}

‎src/Magento/FunctionalTestingFramework/DataGenerator/Handlers/DataObjectHandler.php

+5
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ class DataObjectHandler implements ObjectHandlerInterface
2929
const _ENTITY_KEY = 'entityKey';
3030
const _SEPARATOR = '->';
3131
const _REQUIRED_ENTITY = 'requiredEntity';
32+
const DATA_NAME_ERROR_MSG = "Entity names cannot contain non alphanumeric characters.\tData='%s'";
3233

3334
/**
3435
* The singleton instance of this class
@@ -110,6 +111,10 @@ private function processParserOutput($parserOutput)
110111
$rawEntities = $parserOutput[self::_ENTITY];
111112

112113
foreach ($rawEntities as $name => $rawEntity) {
114+
if (preg_match('/[^a-zA-Z0-9_]/', $name)) {
115+
throw new XmlException(sprintf(self::DATA_NAME_ERROR_MSG, $name));
116+
}
117+
113118
$type = $rawEntity[self::_TYPE];
114119
$data = [];
115120
$linkedEntities = [];

‎src/Magento/FunctionalTestingFramework/Page/Handlers/PageObjectHandler.php

+5
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use Magento\FunctionalTestingFramework\ObjectManagerFactory;
1111
use Magento\FunctionalTestingFramework\Page\Objects\PageObject;
1212
use Magento\FunctionalTestingFramework\XmlParser\PageParser;
13+
use Magento\FunctionalTestingFramework\Exceptions\XmlException;
1314

1415
class PageObjectHandler implements ObjectHandlerInterface
1516
{
@@ -19,6 +20,7 @@ class PageObjectHandler implements ObjectHandlerInterface
1920
const MODULE = 'module';
2021
const PARAMETERIZED = 'parameterized';
2122
const AREA = 'area';
23+
const NAME_BLACKLIST_ERROR_MSG = "Page names cannot contain non alphanumeric characters.\tPage='%s'";
2224

2325
/**
2426
* The singleton instance of this class
@@ -49,6 +51,9 @@ private function __construct()
4951
}
5052

5153
foreach ($parserOutput as $pageName => $pageData) {
54+
if (preg_match('/[^a-zA-Z0-9_]/', $pageName)) {
55+
throw new XmlException(sprintf(self::NAME_BLACKLIST_ERROR_MSG, $pageName));
56+
}
5257
$area = $pageData[self::AREA];
5358
$url = $pageData[self::URL];
5459

‎src/Magento/FunctionalTestingFramework/Page/Handlers/SectionObjectHandler.php

+29-15
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use Magento\FunctionalTestingFramework\Page\Objects\ElementObject;
1212
use Magento\FunctionalTestingFramework\Page\Objects\SectionObject;
1313
use Magento\FunctionalTestingFramework\XmlParser\SectionParser;
14+
use Magento\FunctionalTestingFramework\Exceptions\XmlException;
1415

1516
class SectionObjectHandler implements ObjectHandlerInterface
1617
{
@@ -21,6 +22,8 @@ class SectionObjectHandler implements ObjectHandlerInterface
2122
const LOCATOR_FUNCTION = 'locatorFunction';
2223
const TIMEOUT = 'timeout';
2324
const PARAMETERIZED = 'parameterized';
25+
const SECTION_NAME_ERROR_MSG = "Section names cannot contain non alphanumeric characters.\tSection='%s'";
26+
const ELEMENT_NAME_ERROR_MSG = "Element names cannot contain non alphanumeric characters.\tElement='%s'";
2427

2528
/**
2629
* Singleton instance of this class
@@ -54,21 +57,32 @@ private function __construct()
5457
foreach ($parserOutput as $sectionName => $sectionData) {
5558
$elements = [];
5659

57-
foreach ($sectionData[SectionObjectHandler::ELEMENT] as $elementName => $elementData) {
58-
$elementType = $elementData[SectionObjectHandler::TYPE];
59-
$elementSelector = $elementData[SectionObjectHandler::SELECTOR] ?? null;
60-
$elementLocatorFunc = $elementData[SectionObjectHandler::LOCATOR_FUNCTION] ?? null;
61-
$elementTimeout = $elementData[SectionObjectHandler::TIMEOUT] ?? null;
62-
$elementParameterized = $elementData[SectionObjectHandler::PARAMETERIZED] ?? false;
63-
64-
$elements[$elementName] = new ElementObject(
65-
$elementName,
66-
$elementType,
67-
$elementSelector,
68-
$elementLocatorFunc,
69-
$elementTimeout,
70-
$elementParameterized
71-
);
60+
if (preg_match('/[^a-zA-Z0-9_]/', $sectionName)) {
61+
throw new XmlException(sprintf(self::SECTION_NAME_ERROR_MSG, $sectionName));
62+
}
63+
64+
try {
65+
foreach ($sectionData[SectionObjectHandler::ELEMENT] as $elementName => $elementData) {
66+
if (preg_match('/[^a-zA-Z0-9_]/', $elementName)) {
67+
throw new XmlException(sprintf(self::ELEMENT_NAME_ERROR_MSG, $elementName, $sectionName));
68+
}
69+
$elementType = $elementData[SectionObjectHandler::TYPE];
70+
$elementSelector = $elementData[SectionObjectHandler::SELECTOR] ?? null;
71+
$elementLocatorFunc = $elementData[SectionObjectHandler::LOCATOR_FUNCTION] ?? null;
72+
$elementTimeout = $elementData[SectionObjectHandler::TIMEOUT] ?? null;
73+
$elementParameterized = $elementData[SectionObjectHandler::PARAMETERIZED] ?? false;
74+
75+
$elements[$elementName] = new ElementObject(
76+
$elementName,
77+
$elementType,
78+
$elementSelector,
79+
$elementLocatorFunc,
80+
$elementTimeout,
81+
$elementParameterized
82+
);
83+
}
84+
} catch (XmlException $exception) {
85+
throw new XmlException($exception->getMessage() . " in Section '{$sectionName}'");
7286
}
7387

7488
$this->sectionObjects[$sectionName] = new SectionObject($sectionName, $elements);

‎src/Magento/FunctionalTestingFramework/Test/Objects/ActionObject.php

+24-5
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,8 @@ class ActionObject
3232
const ACTION_ATTRIBUTE_URL = 'url';
3333
const ACTION_ATTRIBUTE_SELECTOR = 'selector';
3434
const ACTION_ATTRIBUTE_VARIABLE_REGEX_PARAMETER = '/\(.+\)/';
35-
const ACTION_ATTRIBUTE_VARIABLE_REGEX_PATTERN = '/{{[\w.\[\]()\',$ ]+}}/';
36-
const ACTION_ATTRIBUTE_VARIABLE_REGEX_PATTERN_WITH_PARAMS= '/{{[\w]+\.[\w]+\([\w ,.\'{$}]+\)}}/';
35+
const ACTION_ATTRIBUTE_VARIABLE_REGEX_PATTERN = '/{{[\w]+\.[\w]+}}/';
36+
const ACTION_ATTRIBUTE_VARIABLE_REGEX_PATTERN_WITH_PARAMS= '/{{[\w]+\.[\w]+\(.+\)}}/';
3737

3838
/**
3939
* The unique identifier for the action
@@ -343,18 +343,37 @@ private function stripAndSplitReference($reference)
343343
}
344344

345345
/**
346-
* Returns an array containing all parameters found inside () block of test input.
346+
* Returns an array containing all parameters found inside () block of test input. Expected delimiter is ', '.
347347
* Returns null if no parameters were found.
348348
*
349349
* @param string $reference
350350
* @return array|null
351351
*/
352352
private function stripAndReturnParameters($reference)
353353
{
354+
// 'string', or 'string,!@#$%^&*()_+, '
355+
$literalParametersRegex = "/'[^']+'/";
356+
$postCleanupDelimiter = "::::";
357+
354358
preg_match(ActionObject::ACTION_ATTRIBUTE_VARIABLE_REGEX_PARAMETER, $reference, $matches);
355359
if (!empty($matches)) {
356-
$strippedReference = str_replace(')', '', str_replace('(', '', $matches[0]));
357-
return explode(',', $strippedReference);
360+
$strippedReference = ltrim(rtrim($matches[0], ")"), "(");
361+
362+
// Pull out all 'string' references, as they can contain 'string with , comma in it'
363+
preg_match_all($literalParametersRegex, $strippedReference, $literalReferences);
364+
$strippedReference = preg_replace($literalParametersRegex, '&&stringReference&&', $strippedReference);
365+
366+
// Sanitize 'string, data.field,$persisted.field$' => 'string::::data.field::::$persisted.field$'
367+
$strippedReference = preg_replace('/,/', ', ', $strippedReference);
368+
$strippedReference = str_replace(',', $postCleanupDelimiter, $strippedReference);
369+
$strippedReference = str_replace(' ', '', $strippedReference);
370+
371+
// Replace string references into string, needed to keep sequence
372+
foreach ($literalReferences[0] as $key => $value) {
373+
$strippedReference = preg_replace('/&&stringReference&&/', $value, $strippedReference, 1);
374+
}
375+
376+
return explode($postCleanupDelimiter, $strippedReference);
358377
}
359378
return null;
360379
}

‎src/Magento/FunctionalTestingFramework/Test/Util/ActionObjectExtractor.php

+6-1
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@ class ActionObjectExtractor extends BaseObjectExtractor
2121
const ACTION_GROUP_REF = 'ref';
2222
const ACTION_GROUP_ARGUMENTS = 'arguments';
2323
const ACTION_GROUP_ARG_VALUE = 'value';
24-
const BEFORE_AFTER_ERROR_MSG = "Merge Error - Steps cannot have both before and after attributes.\tTestStep='%s'";
24+
const BEFORE_AFTER_ERROR_MSG = "Merge Error - Steps cannot have both before and after attributes.\tStepKey='%s'";
25+
const STEP_KEY_BLACKLIST_ERROR_MSG = "StepKeys cannot contain non alphanumeric characters.\tStepKey='%s'";
2526

2627
/**
2728
* ActionObjectExtractor constructor.
@@ -46,6 +47,10 @@ public function extractActions($testActions)
4647
foreach ($testActions as $actionName => $actionData) {
4748
$stepKey = $actionData[self::TEST_STEP_MERGE_KEY];
4849

50+
if (preg_match('/[^a-zA-Z0-9_]/', $stepKey)) {
51+
throw new XmlException(sprintf(self::STEP_KEY_BLACKLIST_ERROR_MSG, $actionName));
52+
}
53+
4954
$actionAttributes = $this->stripDescriptorTags(
5055
$actionData,
5156
self::TEST_STEP_MERGE_KEY,

‎src/Magento/FunctionalTestingFramework/Test/Util/TestObjectExtractor.php

+12-7
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
namespace Magento\FunctionalTestingFramework\Test\Util;
88

9+
use Magento\FunctionalTestingFramework\Exceptions\XmlException;
910
use Magento\FunctionalTestingFramework\Test\Objects\TestObject;
1011

1112
/**
@@ -104,12 +105,16 @@ public function extractTestData($testData)
104105
}
105106

106107
// TODO extract filename info and store
107-
return new TestObject(
108-
$testData[self::NAME],
109-
$this->actionObjectExtractor->extractActions($testActions),
110-
$testAnnotations,
111-
$testHooks,
112-
$filename
113-
);
108+
try {
109+
return new TestObject(
110+
$testData[self::NAME],
111+
$this->actionObjectExtractor->extractActions($testActions),
112+
$testAnnotations,
113+
$testHooks,
114+
$filename
115+
);
116+
} catch (XmlException $exception) {
117+
throw new XmlException($exception->getMessage() . ' in Test ' . $filename);
118+
}
114119
}
115120
}

0 commit comments

Comments
 (0)