Opened 7 years ago

Closed 7 years ago

#10160 closed (fixed)

F expression literals should use bind parameters

Reported by: ikelly Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


To prevent SQL injection attacks, Python values used within F() expressions should be processed as bind parameters rather than be inserted directly into the SQL.

Attachments (2)

10160.diff (1.3 KB) - added by ikelly 7 years ago.
10160-2.diff (1.4 KB) - added by ikelly 7 years ago.

Download all attachments as: .zip

Change History (6)

Changed 7 years ago by ikelly

comment:1 Changed 7 years ago by ikelly

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Changed 7 years ago by ikelly

comment:2 Changed 7 years ago by ikelly

The first patch doesn't quite work. It successfully passes non-string types as bind parameters, but string values were still just getting processed by quote_name, and string values are the most dangerous.

The new patch fixes this by removing the attempt to process the value with quote_name entirely. This introduces a minor backward incompatibility, since it's no longer possible to do things like .filter(amount__lt=F('max_amount')-'surplus') and have 'surplus' automatically be included as a field name. But this can still be accomplished by using F('surplus') instead, which is better anyway since it's explicit.

comment:3 Changed 7 years ago by Alex

Yeah that's defintely an improvement F('field') - 'string' shouldn't turn 'string' into a fieldname automatically.

comment:4 Changed 7 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

(In [9820]) Fixed #10160 -- Modified evaluation of F() expressions to protect against potential SQL injection attacks. Thanks to Ian Kelly for the suggestion and patch.

Note: See TracTickets for help on using tickets.
Back to Top