[IMP] sql_injection: Detect possible sql injections when using fstrings

[antonag32]

Supposed to fix #363. This is a work in progress as there are still some things to complete, like:

• [x] Detect use of fstrings inside execute calls
• [x] Do not flag fstrings which only use constants as possible sql injections
• [x] Write unit tests

And perhaps some other things which will come up during development.

[antonag32]

@moylop260

[antonag32]

@moylop260 The idea to allow fstrings that only use constants came from your commit 5d9878a35512 which allowed calls like:

self.env.cr.execute("SELECT * FROM %s" % 'table_constant')

And its equivalent would be

self.env.cr.execute(f'SELECT * FROM {"table_constant"}')

Which I don't know why anyone would use, so it is worth checking for?

[oca-clabot]

Hey @antonag32, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet. You can read and sign our full Contributor License Agreement here: https://odoo-community.org/page/cla Here is a list of the users:

• @antonag32 (login unknown in OCA database)

Appreciation of efforts, OCA CLAbot

[antonag32]

Implemented a recursive strategy to check for constants inside fstrings and mark them as safe if all formatted values are made up of constants only. Probably no one will use it in real life but its the technically correct thing to do.

Added a unit test to verify SQLI detection is working as corrected.

[antonag32]

As of today, 4 tests are failing on the master branch when I run the whole suit:

• test_20_expected_errors
• test_30_disabling_errors
• test_70_without_jslint_installed
• test_80_with_jslint_error

They still fail after my changes, but no other tests failed, so I assume my work didn't break previous work.

[moylop260]

I think you will need to bump the sql-injection in the EXPECTED_ERRORS dict:

• https://github.com/OCA/pylint-odoo/blob/80442ff58ff22810929f949fe496fce7223abf75/pylint_odoo/test/main.py#L61

[moylop260]

And fix the lints, please

$ flake8 --ignore=E722,F601,F841,W503,W504,W605 --max-line-length=88 --exclude=__init__.py .
./pylint_odoo/test_repo/broken_module/models/broken_model.py:501:68: E999 SyntaxError: invalid syntax
./pylint_odoo/test_repo/broken_module/models/broken_model.py:539:89: E501 line too long (105 > 88 characters)
./pylint_odoo/test/main.py:436:89: E501 line too long (107 > 88 characters)

https://app.travis-ci.com/github/OCA/pylint-odoo/jobs/565831678#L287-L290

[antonag32]

@moylop260. Done, I bumped the sql-injection in EXPECTED_ERRORS and fixed the linting warnings, although locally I was not able to get the E999 SyntaxError warning.

[antonag32]

@moylop260 Since fstrings are not supported until Python 3.6 I made a separate test and embedded data int it instead of modifying test repos. All fstring testing is now done inside a single test which will be skipped if the version is not at least 3.6.

[antonag32]

@moylop260 Apparently I nested existing code inside my checks, which meant certain injections were not being detected. It is fixed now. test_20_expected_errors still blows up but sql-injections are no longer one of the reasons.

[antonag32]

@moylop260 All requested changes were attended.