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 #13549

Merged
merged 1 commit into from Apr 24, 2019

Conversation

Projects
None yet
9 participants
@matks
Copy link
Contributor

commented Apr 24, 2019

Questions Answers
Branch? 1.7.5.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 change is Reviewable

@matks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Important

In this PR, I considered that we can enforce this validation rule on all Name fields from the database. I dont see why a Customer first name could not be an URL but an Employee first name could be.

If too broad, we might restrain the scope of this PR to only customer name fields.

I tried to think of the different usecases for an URL, including the chinese dot punctuation (Chrome does consider it is a valid dot). If you can think of another, please feel free to suggest it.

The last thing I'm considering to handle as well is unicode non-breaking space characters 🤔 I'm wondering if they can be used to insert an URL by encapsulating the dot character.

@matks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Oh, seems like PrettyCI is not happy 😛

@jolelievre
Copy link
Contributor

left a comment

Unusual test ^^ but seems legit to me

@eternoendless eternoendless added this to the 1.7.5.2 milestone Apr 24, 2019

@matks matks force-pushed the matks:forbid-urls-in-name-fields branch from 8b61a9c to 7b64f25 Apr 24, 2019

@matks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

@eternoendless Requested change has been applied and amended into the commit

Do you agree with In this PR, I considered that we can enforce this validation rule on all Name fields from the database. I dont see why a Customer first name could not be an URL but an Employee first name could be. ?

This will impact the following ObjectModel fields:

  • StockMvtWSCore : employee_firstname and employee_lastname
  • SupplyOrderHistoryCore : employee_firstname and employee_lastname
  • SupplyOrderReceiptHistoryCore : employee_firstname and employee_lastname
  • StockMvtCore : employee_firstname and employee_lastname
  • Employee: firstname and lastname
  • Customer: firstname and lastname
  • AddressCore : firstname and lastname
  • PrestaShopLoggerCore: object_type
@eternoendless

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

This will impact the following ObjectModel fields:

  • [...]
  • PrestaShopLoggerCore: object_type

WTF

@eternoendless

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Thank you @matks

@eternoendless eternoendless merged commit 10880c7 into PrestaShop:1.7.5.x Apr 24, 2019

1 of 2 checks passed

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

@matks matks deleted the matks:forbid-urls-in-name-fields branch Apr 25, 2019

@doekia

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Sorry but this patch is dumb
try this in your firstname "/etc/passwd"

How can you not ask yourself what happend after a stripslashes?

@matks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

Sorry but this patch is dumb
try this in your firstname "/etc/passwd"

How can you not ask yourself what happend after a stripslashes?

Hi @doekia,

I dont understand the issue with stripslashes. This php function does remove the backslashes, not the regular slashes (see the function doc). As we are interested here in detecting URLs, we are checking for slashes.

In order to be sure there is no hidden behavior of prestashop (maybe overriding the stripslashes standard function by its own implementation ?) I checked your suggestion and I was successfully rejected when using "/etc/passwd" as a first name. See screenshot:

Capture d’écran 2019-04-25 à 13 28 07

If you have knowledge of a situation where stripslashes could make this check unusable, would you please share it, so that we can improve the PR and the project ?

However please remember that github promotes "a respectful and friendly atmosphere where people feel comfortable asking questions, participating in discussions, and making contributions" (see github Community guidelines). Using "dumb" word or "How can you not ask yourself" statement can be seen as offensive as they are belittleing. Please remember that we are all humans behind the desktop.

@doekia

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

My bad regarding / get tired.
But we don't want backslashes either - do we?
In which senses did I was disrespectful? Saying such is disrespectful.

@ttoine

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

You could have done it this way:
"I think that this patch is not complete enough
...
I don't understand why you are using striplashes, could you please explain?"

That would have been respectful.

Like on our forum, you really need to improve your communication: this is not about your expertise (we know that you know your topic), but just the way you write to others. Also, keep in mind that GitHub have their own moderation rules and they enforce them strictly. It would be a shame to be banned from GitHub because you are not friendly enough in your writings.

We are all here to make PrestaShop a good software, so please, let's try to collaborate with respect and politeness first.

@doekia

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

La politesse aurait probablement de lire le patch que j'ai fait il y a plus de 5 jours, qui a été déployé partout en attendant la communication officielle et de me demander pourquoi j'avais écris cela comme cela.
Vous avez également empêché les . ce qui rend impossible l'inscription de Geoges W. Bush Jr. pourquoi cela?
Quand le forme prime sur le fond, j'avoue commencer à me poser de sérieuses questions.
D'autant que là il est totalement impossible suite a des changements de filtres sur le forum de poster la moindre aide à la communauté. Tous les messages restent en rose hidden. A moins bien sûr que ce ne soit un traitement de faveur me concernant.
N'hésite pas à me le faire savoir. Je peux également quitter le forum

@eternoendless

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Vous avez également empêché les . ce qui rend impossible l'inscription de Geoges W. Bush Jr. pourquoi cela?

@doekia that's not true. Did you have a look at the unit test?
https://github.com/PrestaShop/PrestaShop/pull/13549/files#diff-49ceb946b4bb7238caec469fe9a5de09

@eternoendless

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Btw this is not the forum, please write in English

@matks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

But we don't want backslashes either - do we?

In our understanding, the spam attacks goal is to push people to click on spam or phishing links. That is why we eliminate . and / characters.

Indeed we could also consider that \ are irrelevant in a name field, however since this is unusable for spam attack authors (they cannot inject links using it), I prefer to let the \ available in case someone uses it. I was thinking about usecase where people would import large batch of customers into the database, and some names would have \ characters in it as an escape mechanism. Rejecting the \ character would make these customers invalid.

Vous avez également empêché les . ce qui rend impossible l'inscription de Geoges W. Bush Jr. pourquoi cela?

As said by @eternoendless, I added an extra validation step to allow names like "John D." and this is being checked by unit tests.

This code is a compromise between trying to find out as much as possible URLs and not restricting too much the field possible inputs in order to allow complex or exotic names to be used.

@doekia

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

\ is a valid path separator under windows, based on tool used can trigger some unexpected behavior

Your use case for \ addresses iditiotic import scenario. They should not exists. It is not the role of the core to fix all developer mistakes IMHO. The core does stripslashes during Tools::getValue() if someone does import data it should respect the conventions.

My bad regarding the . I did not spotted the dot/space.

Characters such as ^ * [ ] ` are still accepted, why such?

I too have think of use case BO related, hence I created a specific Validate::isCustomerName

@matks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

\ is a valid path separator under windows, based on tool used can trigger some unexpected behavior

Characters such as ^ * [ ] ` are still accepted, why such?

The goal of this PR was to mitigate the attack vector used by current spam attacks that, in our understanding, aims to push people to click on spam or phishing links. Characters such as \ ^ * [ ] cannot be used to build usable links as far as I know.

If you suggest that these characters might create other security issues if printed / displayed in another context (and not properly escaped). That might be true, but we must find a valid usecase (meaning: a proof of concept) where such an usage can harm the application. If you have knowledge of such a usecase, please reach out to security@prestashop.com in order to get it fixed without public disclosure, in order not to endanger current shops.

If you suggest that these characters are irrelevant for a field supposed to contain a first name or a last name, this is a relevant idea.
Would you please open an issue to suggest this idea, if this is what you mean ? This PR is now closed, it's better to talk about improvements in dedicated issues as everyone can see and participate. (edited)

@doekia

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Unfortunatly in nowadays with all the different tools that try to assist - the wrong way - end-user. Mail client applications, web browser it is merely impossible to know how this will be displayed in all situations.
There is no use case where a firstname or lastname use any of those characters - my approach is better safe that sorry.

@eternoendless

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

I agree in that stripslashes is not really useful in this scenario and that backslashes should not be accepted in any case.

Regarding why special characters are still allowed in names, that is a good question for which I don't have the answer, as that code dates from before the first git commit back in 2011... I don't know if they are allowed on purpose or just missing. I'm going to forbid some of them in another PR.

@eternoendless

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Here it is #13567

@indusone

This comment has been minimized.

Copy link

commented May 10, 2019

This fix does not work, my store continues to receive spam registrations non stop.

@Matt75

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

@indusone

This comment has been minimized.

Copy link

commented May 10, 2019

I read that and i updated prestashop to new version as told. I posted this comment after i did that. Unless i missed something here.

@matks

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

@indusone If there are other not-protected ways to register on your store, that might be how spammers continue to post. For example some modules create new registration forms that might not be protected. Also an override would prevent this from working as it overrides the default behavior of PrestaShop. Check your shop theme, modules and overrides.

@indusone

This comment has been minimized.

Copy link

commented May 14, 2019

Hi Matt
Thanks for reply. But i use a popular theme for presta and no user has reported any issues. Could you or someone please take some time out and look at my store?

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.