Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forbid URLS to be added in customer names #13599

Merged
merged 2 commits into from Apr 30, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -173,8 +173,8 @@ class CustomerCore extends ObjectModel
'primary' => 'id_customer',
'fields' => array(
'secure_key' => array('type' => self::TYPE_STRING, 'validate' => 'isMd5', 'copy_post' => false),
'lastname' => array('type' => self::TYPE_STRING, 'validate' => 'isName', 'required' => true, 'size' => 255),
'firstname' => array('type' => self::TYPE_STRING, 'validate' => 'isName', 'required' => true, 'size' => 255),
'lastname' => array('type' => self::TYPE_STRING, 'validate' => 'isCustomerName', 'required' => true, 'size' => 255),
'firstname' => array('type' => self::TYPE_STRING, 'validate' => 'isCustomerName', 'required' => true, 'size' => 255),
'email' => array('type' => self::TYPE_STRING, 'validate' => 'isEmail', 'required' => true, 'size' => 255),
'passwd' => array('type' => self::TYPE_STRING, 'validate' => 'isPasswd', 'required' => true, 'size' => 255),
'last_passwd_gen' => array('type' => self::TYPE_STRING, 'copy_post' => false),
@@ -156,6 +156,22 @@ public static function isImageSize($size)
return preg_match('/^[0-9]{1,4}$/', $size);
}

/**
* Check whether given customer name is valid
*
* @param string $name Name to validate
*
* @return int 1 if given input is a name, 0 else
*/
public static function isCustomerName($name)
{
$validityPattern = Tools::cleanNonUnicodeSupport(
'/^(?:[^0-9!<>,;?=+()\/\\@#"°*`{}_^$%:¤\[\]|\.。]|[\.。](?:\s|$))*$/u'

This comment has been minimized.

Copy link
@123monsite-regis

123monsite-regis Apr 30, 2019

Contributor

hi,
i take a look at our real customers databases, and it appears that some customers use the dot as separator like "jean.marc" or "J.R." ,
if there is no sql upgrade , we will have some customers blocked during some objects loads or saving.

regards

This comment has been minimized.

Copy link
@doekia

doekia Apr 30, 2019

Contributor

I did mentionned this case and the fact it should be inserted in an upgrade task on the other thread
#13559 (comment)
update ps_customer set firstname = replace(replace(firstname,'.','. '),' ',' ');
update ps_customer set lastname = replace(replace(lastname,'.','. '),' ',' ');

This comment has been minimized.

Copy link
@matks

matks Apr 30, 2019

Contributor

I dont think a database upgrade is the right thing to do. Imagine this: you update to the latest patch version of Prestashop that states "Protects you against spam" (so you expects this patch to change nothing in your regular shop behavior*) and you find out that this patch modified your database behind your back ?

What about the people that use the lastname as an identifier (because they have nothing better) for a synchronizing system between their shop and their ERP ?

Not to forget the cost of this query on very large databases, might trigger timeouts or deadlocks.

That's why the sql upgrade task does not look a suitable solution for me.

*Which should always be the case for patch versions, but sometimes it cant be avoided. But not here.

This comment has been minimized.

Copy link
@doekia

doekia Apr 30, 2019

Contributor

cost of the query? Just applied those 2 queries on 272455 customer in a breeze, less than 800ms

This comment has been minimized.

Copy link
@eternoendless

eternoendless Apr 30, 2019

Member

I agree with @matks in that it's risky to change existing data without warning, not to mention that the process could potentially be too long to perform during the autoupgrade.

I propose adding a release note explaining the potential problem and suggesting to run similar queries manually when upgrading -- and I insist on the "similar" because replace('John B. B.', '.', '. ') returns:

"John B.  B. "

... and regexp_replace is only available on MySql 8+

);

return preg_match($validityPattern, $name);
}

/**
* Check whether given name is valid
*
@@ -166,7 +182,7 @@ public static function isImageSize($size)
public static function isName($name)
{
$validityPattern = Tools::cleanNonUnicodeSupport(
'/^(?:[^0-9!<>,;?=+()\/\\@#"°*`{}_^$%:¤|\.。]|[\.。](?:\s|$))*$/u'
'/^[^0-9!<>,;?=+()@#"°{}_$%:¤|]*$/u'
);

return preg_match($validityPattern, $name);
@@ -49,23 +49,33 @@ public function __construct(TranslatorInterface $translator)
*/
public function translate($validator)
{
if ($validator === 'isName') {
if ($validator === 'isName' || $validator === 'isCustomerName') {
return $this->translator->trans(
'Invalid name', array(), 'Shop.Forms.Errors'
'Invalid name',
[],
'Shop.Forms.Errors'
);
} elseif ($validator === 'isBirthDate') {
}

if ($validator === 'isBirthDate') {
return $this->translator->trans(
'Format should be %s.', array(Tools::formatDateStr('31 May 1970')), 'Shop.Forms.Errors'
'Format should be %s.',
[Tools::formatDateStr('31 May 1970')],
'Shop.Forms.Errors'
);
} elseif ($validator === 'required') {
}

if ($validator === 'required') {
return $this->translator->trans(
'Required field', array(), 'Shop.Forms.Errors'
'Required field',
[],
'Shop.Forms.Errors'
);
}

return $this->translator->trans(
'Invalid format.',
array(),
[],
'Shop.Forms.Errors'
);
}
@@ -1,11 +1,11 @@
/* unicode_hack.js
* Copyright (C) 2010-2012 Marcelo Gibson de Castro Gonçalves. All rights reserved.
*
* Copying and distribution of this file, with or without modification,
* are permitted in any medium without royalty provided the copyright
* notice and this notice are preserved. This file is offered as-is,
* without any warranty.
*/
* Copyright (C) 2010-2012 Marcelo Gibson de Castro Gonçalves. All rights reserved.
*
* Copying and distribution of this file, with or without modification,
* are permitted in any medium without royalty provided the copyright
* notice and this notice are preserved. This file is offered as-is,
* without any warranty.
*/
var unicode_hack = (function() {
/* Regexps to match characters in the BMP according to their Unicode category.
Extracted from Unicode specification, version 5.0.0, source:
@@ -99,9 +99,15 @@ var unicode_hack = (function() {
* @license http://opensource.org/licenses/osl-3.0.php Open Software License (OSL 3.0)
* International Registered Trademark & Property of PrestaShop SA
*/
function validate_isCustomerName(s)
{
var reg = /^(?:[^0-9!<>,;?=+()\/\\@#"°*`\{\}_^$%:¤\[\]|\.]|[\.](?:\s|$))*$/;
return reg.test(s);
}

function validate_isName(s)
{
var reg = /^[^0-9!<>,;?=+()@#"°{}_$%:]+$/;
var reg = /^[^0-9!<>,;?=+()@#"°\{\}_$%:]+$/;

This comment has been minimized.

Copy link
@matks

matks Apr 30, 2019

Contributor

Nice fix ! 😉

return reg.test(s);
}

@@ -98,6 +98,14 @@ public function testIsName($expected, $input)
$this->assertSame($expected, Validate::isName($input));
}

/**
* @dataProvider isCustomerNameDataProvider
*/
public function testIsCustomerName($expected, $input)
{
$this->assertSame($expected, Validate::isCustomerName($input));
}

/**
* @dataProvider isFloatDataProvider
*/
@@ -157,7 +165,41 @@ public function isSha1DataProvider()
);
}


public function isNameDataProvider()
{
return array(
array(1, 'Mathieu'),
array(1, 'Dupont'),
array(1, 'Jaçinthé'),
array(1, 'Jaçinthø'),
array(1, 'John D.'),
array(1, 'John D.John'),
array(1, 'John D. John'),
array(1, 'John D. John D.'),
array(1, 'Mario Bros.'),
array(1, 'ââââ'),
array(0, 'https://www.website.com'),
array(1, 'www.website.com'),
array(1, 'www\.website\.com'),
array(1, 'www\\.website\\.com'),
array(1, 'www.website.com.'),
array(1, 'website。com'),
array(1, 'John D. www.some.site'),
array(1, 'www.website.com is cool'),
array(1, 'website。com。'),
array(1, 'website。com'),
array(0, 'website%2Ecom'),
array(1, 'website/./com'),
array(1, '.rn'),
array(1, 'websitecom/a'),
array(0, 'websitecom%20a'),
array(1, '`hello'),
array(1, 'hello[my friend]'),
);
}

public function isCustomerNameDataProvider()
{
return array(
array(1, 'Mathieu'),
@@ -175,6 +217,7 @@ public function isNameDataProvider()
array(0, 'www\\.website\\.com'),
array(0, 'www.website.com.'),
array(0, 'website。com'),
array(0, 'John D.John'),
array(0, 'John D. www.some.site'),
array(0, 'www.website.com is cool'),
array(0, 'website。com。'),
@@ -185,6 +228,7 @@ public function isNameDataProvider()
array(0, 'websitecom/a'),
array(0, 'websitecom%20a'),
array(0, '`hello'),
array(0, 'hello[my friend]'),
);
}

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.