OCA / pylint-odoo

Odoo plugin for Pylint
http://www.pylint.org
143 stars 168 forks source link

New check: .format() over translated string _() #302

Closed yajo closed 3 years ago

yajo commented 3 years ago

Today I noticed in https://github.com/OCA/server-tools/pull/1941#discussion_r537426404 that our friend @legalsylvain didn't know about the implicit dangers of using format string syntax for translated content. Certainly it's not something very obvious, but it's quite dangerous, as it lets a translator to inject malicious translations that execute server-side code in the odoo server. Translated content must always use printf style formatting, which is dumber but secure.

Until today, I have never noticed any such usage in our code base, but I think a linter would be very helpful here.

pedrobaeza commented 3 years ago

Not only dangerous, but can't be possible to be used with translatable fields, as the symbol replacement is done on linking time, and thus, with that, you won't have any translation.

yajo commented 3 years ago

I think you're talking about f-strings, which is a different thing (that indeed could also be linted)

AFAIK yes you could have a translation with _("My name is {}").format("Jairo")

legalsylvain commented 3 years ago

@Yajo thanks a lot !

pedrobaeza commented 3 years ago

Yes, I'm taking about f-strings

moylop260 commented 3 years ago

@Yajo So, Should we do a dance to kukulcan to resurrect the following old proposal?

moylop260 commented 3 years ago

Could you check the following PR, please?

moylop260 commented 3 years ago

@Yajo Could you make a PR to change the following section:

In order to warn a security risk instead of a translation matter

Please

yajo commented 3 years ago

So, Should we do a dance to kukulcan to resurrect the following old proposal?

No, that one's fixed by python 3 which is the norm these days (finally). The problem we're talking here is just about translations.

yajo commented 3 years ago

Could you make a PR to change the following section:

* [Prefer % over .format(), prefer %(varname) instead of positional. This is better for translation and clarity.](https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#33idioms)

In order to warn a security risk instead of a translation matter

Done in https://github.com/OCA/odoo-community.org/pull/58

StefanRijnhart commented 3 years ago

I just found out that pyupgrade from the OCA's pre-commit replaces printf formatting by str.format() when it's safe to do so (https://github.com/asottile/pyupgrade#printf-style-string-formatting). That to me looks like a huge encouragement for contributors to use str.format() without giving consideration whether it's safe to do so. We may not be able to rely on pylint-odoo to catch all cases when it's not safe. Would it be a good idea to disable this feature from pyupgrade?

yajo commented 3 years ago

Would it be a good idea to disable this feature from pyupgrade?

Yes, maybe it would be less confusing.