Opened 5 years ago

Closed 5 years ago

#13632 closed (wontfix)

lack of builtin range checking of id fields

Reported by: anonymous Owned by: glassresistor
Component: Documentation Version: master
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The lack of builtin range checking for id fields is a vulnerability.

/service/docserver/papers/3/ --> produces a document
/service/docserver/papers/6578/ --> produces 404 page
/service/docserver/papers/9999999999999999999/ --> throws OverflowError

Traceback is at http://paste.pocoo.org/show/218865/

I think the last case should throw DoesNotExist instead of causing server error.
In the case at hand I used generic views and sqlite3 DB backend.
Of course, one can check this himself all over the places, however that would be against the DRY principle.
Not to mention, it would be complicated when using generic views.

Change History (4)

comment:1 Changed 5 years ago by gregmuellegger

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

The problem with this is that the ID passed into a generic view doesn't need to be an integer. It's also reasonable that a Model has a non-integer primary key. So the key from the URL must be passed to the queryset without any type checks - we don't know the type!

You can still avoid this issue without writing custom code that checks the value before passing it into the generic view: Write the url-regexs in such a way that they force a valid type. For example:

    url(r^/service/docserver/papers/(?P<id>\d{1,6})/

This will throw a 404 for all IDs bigger than 1 000 000.

So this is generally a "wont fix" from me.

comment:2 follow-up: Changed 5 years ago by yuval_a

I'm not sure this is a won't fix.

It doesn't seem reasonable to expect the user to enforce type overflow protection in her url conf.

Not to mention this is not documented...

comment:3 in reply to: ↑ 2 ; follow-up: Changed 5 years ago by glassresistor

  • Component changed from Database layer (models, ORM) to Documentation
  • Owner changed from nobody to glassresistor

Replying to yuval_a:

I'm not sure this is a won't fix.

It doesn't seem reasonable to expect the user to enforce type overflow protection in her url conf.

Not to mention this is not documented...

I agree I think in the generic view documentation and maybe afew other places need to have these checks in the example.

comment:4 in reply to: ↑ 3 Changed 5 years ago by glassresistor

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

Replying to glassresistor:

Replying to yuval_a:

I'm not sure this is a won't fix.

It doesn't seem reasonable to expect the user to enforce type overflow protection in her url conf.

Not to mention this is not documented...

I agree I think in the generic view documentation and maybe afew other places need to have these checks in the example.

Apparently this is not something people think it is not important to have described in a tutorial. "Too complex for new users"

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