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 inserted into Name fields #13559

Merged

Conversation

Projects
None yet
7 participants
@matks
Copy link
Contributor

commented Apr 25, 2019

Questions Answers
Branch? 1.6.1.x
Description? Strengthen the isName validation to avoir URLs to be inserted in it
Type? bug fix
Category? CO
BC breaks? yes, if someone used such a field to store URLs
Deprecations? no
Fixed ticket? Fixes #13524
How to test? Attempt, from front-office, to create a customer with an URL as first name or last name. Now it is not possible anymore.

This is a backport of #13549


This change is Reviewable

@eternoendless eternoendless added this to the 1.6.1.24 milestone Apr 25, 2019

@eternoendless eternoendless changed the title [Backport] Forbid URLs to be inserted into Name fields Forbid URLs to be inserted into Name fields Apr 25, 2019

@marionf marionf added QA ✔️ and removed waiting for QA labels Apr 25, 2019

@eternoendless

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Thank you @matks

@eternoendless eternoendless merged commit bad3f6e into PrestaShop:1.6.1.x Apr 25, 2019

1 of 2 checks passed

PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@okom3pom

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

With this PR my customer can't update account information

I have a lot of customers with dot in name or lastname.

I think is not the good way

@matks matks deleted the matks:forbid-urls-in-name-fields-161x branch Apr 26, 2019

@matks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

With this PR my customer can't update account information

I have a lot of customers with dot in name or lastname.

I think is not the good way

Hi, thank you for feedback,

Can you provide examples ? We have made sure that full stop abbreviations (example: "John D. Rockefeller") are valid.

@okom3pom

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

I tested the PR with a customer who have a lastname L.B

Return : Le Le champ Nom de famille est invalide.

@doekia

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

For information, I have deployed this solution on only a single shop rather than mine (using isCustomerName()).

It cause various order to crash.
isName is used on various module and collides. Here it is a module regarding relais-pickup that uses the brand name of the pickup point into the address. (socolissimo)

@doekia

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

I tested the PR with a client who has a lastname L.B

Return : Le Le champ Nom de famille est invalide.

Should be spelled L. B

@okom3pom

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

I have a lot of occurrence with X.X in the first and last names.

I have a shop with 300k customers it's crazy the number of times when the [dot] is used.

@eternoendless

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

So people abuse a flawed method and now fixing it is considered a regression 💩 ...

These are our options as I see them now:

  1. We keep our fix and let people deal with the fallout of having misused this method for the wrong purposes.
  2. We keep the previous behavior and let people block the spammers using a captcha.
  3. We keep the previous behavior, but add a new validation method specifically for this form. You still won't be able to name yourself "Some.thing", and this flaw may be exploited somewhere else in the future.
  4. We remove the name from the confirmation email. This may be counterproductive as the mail may be flagged as spam due to its genericness, and also spammers won't realize this change right away and continue abusing the form.

Wdyt @doekia @okom3pom ?

@matks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

For information, I have deployed this solution on only a single shop rather than mine (using isCustomerName()).

It cause various order to crash.
isName is used on various module and collides. Here it is a module regarding relais-pickup that uses the brand name of the pickup point into the address. (socolissimo)

Thanks for the constructive feedback 😉

@doekia

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

So people abuse a flawed method and now fixing it is considered a regression shit

Did you really said so? UWAGA! policja myśli and Code of Conduct are in ambush ;)
I will not overreact, no offense taken

However I understand most of your frustration, let me clarify this:

A/ Regarding @okom3pom question, it is rather simple. He needs to sanitize his database with some thing like: update ps_customer set firstname = replace(replace(firstname,'.','. '),' ',' '); similar request with lastname should be run too. (could be part of the install/update/sql scripts btw)

B/ The function Validate::isName(), lets call it an API, have been in use as-is over the last decade. Unchanged since at least version 1.0 of PrestaShop. The actual change indeed block misuse during registration, but at the cost of breaking compatibility with all third party that where using, legitimately or not this API. That is as sufficient reason IMHO to change not the function but the validation for customer first/lastname, hence creating a new function Validate::isCustomerName()

@okom3pom

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

A/ Regarding @okom3pom question, it is rather simple. He needs to sanitize his database with some thing like: update ps_customer set firstname = replace(replace(firstname,'.','. '),' ',' '); similar request with lastname should be run too. (could be part of the install/update/sql scripts btw)

Yes i think it's a good idea, actually my shop is fixed, my feedback is for other prestashop shops

@eternoendless

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Did you really said so?

Hahaha, I was just expressing how effed up this kind of situation is, where fixing bugs is next to impossible because the bug is considered a feature sometimes 🤯. In any case I'm glad you didn't take it personally, it wasn't directed to anyone in particular.

Looks like we don't have any other choice than

  1. We keep the previous behavior, but add a new validation method specifically for this form. You still won't be able to name yourself "Some.thing", and this flaw may be exploited somewhere else in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.