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.

Sign up for GitHub

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

Jump to bottom

[ADD] sql-injection: Add new check to avoid sql injection #29

Merged
merged 2 commits into from
Jun 1, 2016
Merged

[ADD] sql-injection: Add new check to avoid sql injection #29

merged 2 commits into from
Jun 1, 2016

Conversation

moylop260
Copy link
Collaborator

@moylop260 moylop260 commented May 17, 2016 •
edited

Note: This is a progressive PR from #28 (Now is merged!)

Example of comment by @dufresnedavid
screen shot 2016-05-18 at 9 07 28 am

@moylop260 moylop260 self-assigned this May 17, 2016
@moylop260 moylop260 added this to the 1.3.0 milestone May 17, 2016
@moylop260 moylop260 force-pushed the master-oca-add-sql-injection-moy branch from abff63a to a619d68 Compare May 17, 2016 03:14
pedrobaeza
pedrobaeza reviewed May 17, 2016
View reviewed changes
pylint_odoo/test_repo/broken_module/models/broken_model.py
'WHERE parent_id IN %s'
% (tuple(ids),))

def sql_injection_method2(self, ids):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is the same as the method 1. Why putting it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right!
Thanks for feedback

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@moylop260 moylop260 force-pushed the master-oca-add-sql-injection-moy branch from a619d68 to a247588 Compare May 17, 2016 06:54
@moylop260 moylop260 changed the title [ADD] sql-injection: Verify guideline no-sql-injection [ADD] sql-injection: Add new check no-sql-injection May 17, 2016
@coveralls
Copy link

coveralls commented May 17, 2016 •
edited

Coverage Status

Coverage remained the same at 100.0% when pulling a247588 on vauxoo-dev:master-oca-add-sql-injection-moy into 6a8f696 on OCA:master.

@pedrobaeza
Copy link
Member

👍

2 similar comments
@luiseevaquer
Copy link

👍

@oscarolar
Copy link

👍

@moylop260 moylop260 mentioned this pull request May 18, 2016
Closed
@moylop260 moylop260 force-pushed the master-oca-add-sql-injection-moy branch from a247588 to 3bd7698 Compare May 18, 2016 13:32
ghost
ghost reviewed May 18, 2016
View reviewed changes
pylint_odoo/test_repo/broken_module/models/broken_model.py
def sql_injection_method5(self, ids):
var = 'SELECT name FROM account WHERE id IN %s'
values = ([1, 2, 3, ], )
self._cr.execute(var % values) # sql injection too
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moylop260 how does this work exactly. I guess these methods will be called somehow in unit tests ? For now, I don't get how you can be sure that sql_method will not be detected and sql_injection_method will be.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a coverage of 100% here

screen shot 2016-05-18 at 8 55 01 am

Maybe you could be interest in review the following material:

asciicast

asciicast

Copy link
Collaborator Author

@moylop260 moylop260 May 18, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could see the output of the tests here

pylint_odoo/test_repo/broken_module/models/broken_model.py:93: [E8103(sql-injection), TestModel.sql_injection_method] Use of "%" operator in execute database method. Better use parameters instead. - More info https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#no-sql-injection
pylint_odoo/test_repo/broken_module/models/broken_model.py:95: [E8103(sql-injection), TestModel.sql_injection_method] Use of "%" operator in execute database method. Better use parameters instead. - More info https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#no-sql-injection
pylint_odoo/test_repo/broken_module/models/broken_model.py:97: [E8103(sql-injection), TestModel.sql_injection_method] Use of "%" operator in execute database method. Better use parameters instead. - More info https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#no-sql-injection
pylint_odoo/test_repo/broken_module/models/broken_model.py:110: [E8103(sql-injection), TestModel.sql_injection_method5] Use of "%" operator in execute database method. Better use parameters instead. - More info https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#no-sql-injection

We have a expected number of errors by check here

    'sql-injection': 4,

I hope to be clear

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moylop260 ok, I get it, you must check the logs to verify that your test succeded. I'm ok with that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@ghost
Copy link

ghost commented May 18, 2016

👍

@coveralls
Copy link

coveralls commented May 18, 2016 •
edited

Coverage Status

Coverage remained the same at 100.0% when pulling 3bd7698 on vauxoo-dev:master-oca-add-sql-injection-moy into d91b094 on OCA:master.

@moylop260 moylop260 changed the title [ADD] sql-injection: Add new check no-sql-injection [ADD] sql-injection: Add new check to avoid sql injection May 18, 2016
@moylop260
Copy link
Collaborator Author

Could merge it?

@guewen
Copy link
Member

guewen commented May 18, 2016

What about:

cr.execute('SELECT id FROM %s' % table_name)

Which is valid as we are not injecting a value but relation name (injection using params would not work here).

@moylop260
Copy link
Collaborator Author

moylop260 commented May 18, 2016 •
edited

@guewen
Thanks for feeback

IMHO here we can use the same criteria like as invalid-commit comment

  • if we need a % instead of params, we should add an explicit comment explaining...
  • in this comment you can add # pylint: disable=sql-injection to bypass this check.

What do you think?

@moylop260 moylop260 force-pushed the master-oca-add-sql-injection-moy branch 2 times, most recently from 5b57677 to 19d60b0 Compare May 18, 2016 19:07
@moylop260
Copy link
Collaborator Author

FYI rebased!

@moylop260 moylop260 force-pushed the master-oca-add-sql-injection-moy branch from 19d60b0 to 044b883 Compare May 18, 2016 19:13
@coveralls
Copy link

coveralls commented May 18, 2016 •
edited

Coverage Status

Coverage remained the same at 100.0% when pulling 044b883 on vauxoo-dev:master-oca-add-sql-injection-moy into 0cbd023 on OCA:master.

@coveralls
Copy link

coveralls commented May 18, 2016 •
edited

Coverage Status

Coverage remained the same at 100.0% when pulling 4ae1125 on vauxoo-dev:master-oca-add-sql-injection-moy into 0cbd023 on OCA:master.

@moylop260
Copy link
Collaborator Author

@guewen
If you don't have a new comment... Could we merge it?

@moylop260
Copy link
Collaborator Author

@guewen
ping
Second call...

@guewen
Copy link
Member

guewen commented May 23, 2016

@moylop260 I was busy for a few days.

Creative people will use .format() instead of % or do the substitution before the call. That's what I'd do too to avoid to put the # pylint: disable=sql-injection comment.

@moylop260
Copy link
Collaborator Author

@guewen
Thanks for reply.

Yes, you are right!
Currently we don't have detected .format() and previous variable assign before the call...
You could add that way for now without disable comment.

@moylop260 moylop260 mentioned this pull request May 25, 2016
Merged
@moylop260
Copy link
Collaborator Author

@guewen
Could we merge it?

@moylop260 moylop260 mentioned this pull request May 25, 2016
Merged
@moylop260
Copy link
Collaborator Author

@pedrobaeza @dreispt
What about merge it?

@pedrobaeza pedrobaeza merged commit 32a39c1 into OCA:master Jun 1, 2016
@luisg123v luisg123v deleted the master-oca-add-sql-injection-moy branch August 6, 2020 02:30
@antonag32 antonag32 mentioned this pull request Oct 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@moylop260 moylop260

Labels
need review needs review
Projects
None yet
Milestone
1.3.x
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@moylop260 @coveralls @pedrobaeza @luiseevaquer @oscarolar @guewen @OCA-git-bot