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

[REF] CONTRIBUTING.md: Add section for "don't bypass the ORM" #183

Merged
merged 1 commit into from
Mar 16, 2016
Merged

[REF] CONTRIBUTING.md: Add section for "don't bypass the ORM" #183

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,33 @@ Before continuing, please be sure to read the online documentation of pyscopg2 t
- [Advanced parameter types](http://initd.org/psycopg/docs/usage.html#adaptation-of-python-values-to-sql-types)


### Do not bypass the ORM

You should never use the database cursor directly when the ORM can do the same thing! By doing so you are bypassing all the ORM features, possibly the transactions, access rights and so on.

And chances are that you are also making the code harder to read and probably less secure (see also next guideline):

```python
# very very wrong
cr.execute('select id from auction_lots where auction_id in (' +
','.join(map(str, ids)) + ') and state=%s and obj_price>0',
('draft',))
Copy link
Member

Choose a reason for hiding this comment

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

Assuming ids to be a list of integers, how can injection be made here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you don't have a constraint validating that the value must be a list of integer we are susceptible to sql injection with concatenate.

The sql injection is explain better here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eLBati
FYI I have created follow script to understand the real issue of sql injection:
https://github.com/vauxoo-dev/gist-vauxoo/blob/master/sql_injection.py

Copy link
Member

Choose a reason for hiding this comment

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

@moylop260 thanks

auction_lots_ids = [x[0] for x in cr.fetchall()]

# no injection, but still wrong
cr.execute('select id from auction_lots where auction_id in %s '
'and state=%s and obj_price>0',
(tuple(ids), 'draft',))
auction_lots_ids = [x[0] for x in cr.fetchall()]

# better
auction_lots_ids = self.search(cr, uid, [
('auction_id', 'in', ids),
('state', '=', 'draft'),
('obj_price', '>', 0),
])
```

### Field
* `One2Many` and `Many2Many` fields should always have `_ids` as suffix
(example: sale_order_line_ids)
Expand Down Expand Up @@ -829,6 +856,7 @@ The differences include:
* Use underscore uppercase notation for global variables or constants
* [SQL](#sql)
* Add section for No SQL Injection
* Add section for don't bypass the ORM
* [Field](#field)
* A hint for function defaults
* Use default label string if is posible.
Expand Down