Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#15067 closed (fixed)

base36_to_int returns a long in certain situations

Reported by: Garthex Owned by: nobody
Component: Core (Other) Version: 1.2
Severity: Keywords: blocker
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

django.utils.http.base36_to_int uses the int() cast, which returns a long if it can't fit within an int. The input is truncated to 13 characters before calling int(), but if you supply 13 Z's you receive a long instead of an int.

>>> type(int('zzzzzzzzzzzzz', 36))
<type 'long'>

This causes an OverflowError with django.contrib.auth.views.password_reset_done if you supply uidb36='zzzzzzzzzzzzz' and token='123'

Note: testing of the password_reset_done was done on version 1.2.3 and testing of base36_to_int was done on 1.2.3 and the trunk.

Attachments (2)

15067.patch (517 bytes) - added by kfrazier 4 years ago.
15067_test.diff (835 bytes) - added by jboutros 4 years ago.
Test case for overflow condition. The supplied patch passes this test.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 4 years ago by Garthex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
$ python --version
Python 2.7.1

comment:2 Changed 4 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

I suspect the answer here will be to only allow 12 digits instead of 13. That will allow integers up to 4738381338321616895 instead of 9223372036854775807, but (ahem) that should be enough for anybody.

comment:3 Changed 4 years ago by ramiro

  • Keywords blocker added
  • milestone set to 1.3

Changed 4 years ago by kfrazier

comment:4 Changed 4 years ago by kfrazier

  • Has patch set
  • Needs tests set

Limiting it to 12 characters would ensure that the return value is an int, but for 32-bit systems, it is far too large. For 32-bit systems, it would need to be limited to 5 characters. We discussed changes to throw a ValueError from base36_to_int if the value is greater than sys.maxint, but that would potentially break a lot of user systems that may be happily using longs without issue. The safest approach seems to be to just catch the OverflowError in password_reset_confirm and set user to None if it occurs. Patch is attached.

Changed 4 years ago by jboutros

Test case for overflow condition. The supplied patch passes this test.

comment:5 Changed 4 years ago by jboutros

  • Needs tests unset

comment:6 Changed 4 years ago by russellm

Fair point about 32 bit systems; in which case, a check should be performed against sys.maxint. That enables us to make the contract for base36_to_int into "always returns an int", which makes a certain amount of sense anyway.

comment:7 Changed 4 years ago by russellm

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

(In [15288]) Fixed #15067 -- Modified the range checks on base36_to_int so you are guaranteed to always get an int, avoiding possible OverflowErrors. Thanks to Garthex for the report, jboutros for the patch, and kfrazier for the feedback.

comment:8 Changed 4 years ago by russellm

(In [15289]) [1.2.X] Fixed #15067 -- Modified the range checks on base36_to_int so you are guaranteed to always get an int, avoiding possible OverflowErrors. Thanks to Garthex for the report, jboutros for the patch, and kfrazier for the feedback.

Backport of r15288 from trunk.

comment:9 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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